Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 5, 2013, 18:17 UTC
- Message-ID
- <7vsiyoj932.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1375712354-13171-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 9 quoted lines
> 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.
Show 18 quoted lines
> +{
> + 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.
Show 34 quoted lines
> + 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.