Re: [PATCH v3] 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..1f33908 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -167,11 +167,66 @@ static int need_to_gc(void)
>  	return 1;
>  }
>  
> +static int gc_running(int force)

Sounds like a bool asking "Is a GC running?  Yes, or no?".  Since
there is no room for "force" to enter in order to answer that
question, I have to guess that this function is somewhat misnamed.

> +{
> +	static struct lock_file lock;
> +	struct utsname utsname;
> +	struct stat st;
> +	uintmax_t pid;
> +	FILE *fp;
> +	int fd, should_exit;
> +
> +	if (uname(&utsname))
> +		strcpy(utsname.nodename, "unknown");
> +
> +	fd = hold_lock_file_for_update(&lock,
> +			git_path("gc-%s.pid", utsname.nodename), 0);
> +	if (!force) {
> +		if (fd < 0)
> +			return 1;
> +
> +		fp = fopen(git_path("gc-%s.pid", utsname.nodename), "r");

I would have imagined that you would use a lockfile gc.pid and write
nodename and pid to it (and if nodename matches, you know pid may
have a chance to actually match another instance of "gc", while
there will not way it matches if nodename is different, and do
something intelligent about it).  By letting GC that is running on
another node to be completely unnoticed, this change is closing the
door to "do something intelligent about it", like giving it the same
12 hour limit.

> +		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, &pid) == 1 &&
> +			!kill(pid, 0);
> +		if (fp != NULL)
> +			fclose(fp);
> +		if (should_exit) {
> +			if (fd >= 0)
> +				rollback_lock_file(&lock);
> +			return 1;
> +		}
> +	}
> +
> +	if (fd >= 0) {
> +		struct strbuf sb = STRBUF_INIT;
> +		strbuf_addf(&sb, "%"PRIuMAX"\n", (uintmax_t) getpid());
> +		write_in_full(fd, sb.buf, sb.len);
> +		strbuf_release(&sb);
> +		commit_lock_file(&lock);
> +	}
> +
> +	return 0;
> +}

After reading what the whole function does, I think the purpose of
this function is to take gc-lock (with optionally force).  Perhaps a
name along the lines of "lock_gc", "gc_lock", "lock_repo_for_gc",
would be more appropriate.