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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2013, 18:12 UTC
Message-ID: <7vk3jw9hlm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1375959938-6395-1-git-send-email-pclouds@gmail.com>

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

Show 27 quoted lines

> 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.