Re: [PATCH v4] gc: reject if another gc is running, unless --force is given

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 6be6c8d..99682f0 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -167,11 +167,69 @@ static int need_to_gc(void)
> + ...
> +	fd = hold_lock_file_for_update(&lock, git_path("gc.pid"),
> +				       LOCK_DIE_ON_ERROR);
> +	if (!force) {
> +		fp = fopen(git_path("gc.pid"), "r");
> +		memset(locking_host, 0, sizeof(locking_host));
> +		should_exit =
> +			fp != NULL &&
> +			!fstat(fileno(fp), &st) &&
> +			/*
> +			 * 12 hour limit is very generous as gc should
> +			 * never take that long. On the other hand we
> +			 * don't really need a strict limit here,
> +			 * running gc --auto one day late is not a big
> +			 * problem. --force can be used in manual gc
> +			 * after the user verifies that no gc is
> +			 * running.
> +			 */
> +			time(NULL) - st.st_mtime <= 12 * 3600 &&
> +			fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 &&
> +			!strcmp(locking_host, my_host) &&
> +			!kill(pid, 0);

If there is a lockfile we can read, and if we can positively
determine that the process named in the lockfile is still running,
then we definitely do not want to do another "gc".  That part is
good.

If the lock is very stale, 12-hour test will kick in, and we do not
read who locked it, nor check if the locker is still alive.  By
doing so, we avoid misidentifying a new process that is unrelated to
the locker who died and left the lockfile behind, which is a good
thing.  The logic to ignore such lockfile as "unknown" applies
equally to a remote locker on a lockfile that is old.  So the logic
for an old lockfile is good, too.

When we see a recent lockfile created by a "gc" running elsewhere,
we do not set "should_exit".  Is that a good thing?  I am wondering
if the last two lines should be:

-	!strcmp(locking_host, my_host) &&
-	!kill(pid, 0);
+	(strcmp(locking_host, my_host) || !kill(pid, 0));

instead.

Thanks, looking good.