Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
Junio C Hamano wrote: >>> 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. >> >> The whole point of this exercise is to _not_ lock up the repo during >> gc,... > > I do not think it is a misnomer to call the entity that locks other > instances of gc's "a lock on the repository for gc". Nothing in > Duy's code suggests any other commands paying attention to this > mechanism and stalling, and I think my comments were clear enough > that I was not suggesting such a change. > > So I am not sure what you are complaining. Not complaining; I wrote it down because of your "lock_repo_for_gc" suggestion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
Ramkumar Ramachandra writes: > Junio C Hamano wrote: >> [...] > > The other comments mostly make sense. > >> 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. > > The whole point of this exercise is to _not_ lock up the repo during > gc,... I do not think it is a misnomer to call the entity that locks other instances of gc's "a lock on the repository for gc". Nothing in Duy's code suggests any other commands paying attention to this mechanism and stalling, and I think my comments were clear enough that I was not suggesting such a change. So I am not sure what you are complaining. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
Junio C Hamano wrote: > [...] The other comments mostly make sense. > 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. The whole point of this exercise is to _not_ lock up the repo during gc, so I can do minimal commit/ worktree/ ref update operations when it's running. I can't expect the reflog to work, so complex history-rewriting operations should be avoided; that's about it, I think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] gc: reject if another gc is running, unless --force is given
Nguyễn Thái Ngọc Duy 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
