[v2,07/11] maintenance: take a lock on the objects directory

Commit Message

Jean-Noël Avila via GitGitGadget Aug. 18, 2020, 2:23 p.m. UTC

From: Derrick Stolee <[e-mail]>

Performing maintenance on a Git repository involves writing data to the
.git directory, which is not safe to do with multiple writers attempting
the same operation. Ensure that only one 'git maintenance' process is
running at a time by holding a file-based lock. Simply the presence of
the .git/maintenance.lock file will prevent future maintenance. This
lock is never committed, since it does not represent meaningful data.
Instead, it is only a placeholder.

If the lock file already exists, then fail silently. This will become
very important later when we implement the 'fetch' task, as this is our
stop-gap from creating a recursive process loop between 'git fetch' and
'git maintenance run'.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/gc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Jonathan Tan Aug. 19, 2020, 12:04 a.m. UTC | #1

> If the lock file already exists, then fail silently. This will become

Maybe "skip all maintenance steps silently"?

> +	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
> +		/*
> +		 * Another maintenance command is running.
> +		 *
> +		 * If --auto was provided, then it is likely due to a
> +		 * recursive process stack. Do not report an error in
> +		 * that case.
> +		 */
> +		if (!opts->auto_flag && !opts->quiet)
> +			error(_("lock file '%s' exists, skipping maintenance"),
> +			      lock_path);
> +		free(lock_path);
> +		return 0;
> +	}

As it is, this doesn't seem very silent. :-) If we do want a message to
be printed maybe make it warning instead of error.

Other than that, the idea of having a lock file and the implementation
in this patch look good to me.

Derrick Stolee Aug. 19, 2020, 3:10 p.m. UTC | #2

On 8/18/2020 8:04 PM, Jonathan Tan wrote:
>> If the lock file already exists, then fail silently. This will become
> 
> Maybe "skip all maintenance steps silently"?
> 
>> +	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
>> +		/*
>> +		 * Another maintenance command is running.
>> +		 *
>> +		 * If --auto was provided, then it is likely due to a
>> +		 * recursive process stack. Do not report an error in
>> +		 * that case.
>> +		 */
>> +		if (!opts->auto_flag && !opts->quiet)
>> +			error(_("lock file '%s' exists, skipping maintenance"),
>> +			      lock_path);
>> +		free(lock_path);
>> +		return 0;
>> +	}
> 
> As it is, this doesn't seem very silent. :-) If we do want a message to
> be printed maybe make it warning instead of error.

Sorry, it is silent when "--auto" is specified. The commit message needs
to reflect this properly. This could easily be downgraded to a warning.

> Other than that, the idea of having a lock file and the implementation
> in this patch look good to me.

Thanks,
-Stolee

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index 0231d2f8c1..a47bf64623 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -830,6 +830,25 @@  static int maintenance_run(struct maintenance_opts *opts)
 {
 	int i, found_selected = 0;
 	int result = 0;
+	struct lock_file lk;
+	struct repository *r = the_repository;
+	char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);
+
+	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
+		/*
+		 * Another maintenance command is running.
+		 *
+		 * If --auto was provided, then it is likely due to a
+		 * recursive process stack. Do not report an error in
+		 * that case.
+		 */
+		if (!opts->auto_flag && !opts->quiet)
+			error(_("lock file '%s' exists, skipping maintenance"),
+			      lock_path);
+		free(lock_path);
+		return 0;
+	}
+	free(lock_path);
 
 	for (i = 0; !found_selected && i < TASK__COUNT; i++)
 		found_selected = tasks[i].selected;
@@ -850,6 +869,7 @@  static int maintenance_run(struct maintenance_opts *opts)
 		}
 	}
 
+	rollback_lock_file(&lk);
 	return result;
 }