* Re: [PATCH v2] gc: ignore old gc.log files
2017-02-09 19:17 [PATCH v2] gc: ignore old gc.log files David Turner
@ 2017-02-10 9:16 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2017-02-10 9:16 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, pclouds
David Turner <dturner@twosigma.com> writes:
> The intent of automatic gc is to have a git repository be relatively
> low-maintenance from a server-operator perspective.
This is diametrically opposite from how I recall the auto-gc came
about in Sep 2007. The primary purpose was to help desktop clients
that never runs repack.
By pointing this out, I do not mean that we shouldn't make auto-gc
work well in the server settings. I however do not want our log
messages to distort history in order to justify a change that is
worth making, and I do not think this change needs to do that.
For example, a paragraph like this:
It would be really nice if the auto gc mechanism can be used to
help server operators, even though the original purpose it was
introduced was primarily to help desktop clients that never
repacks.
followed by a description of what makes it not exactly helpful for
server operators in the current behaviour (iow, "what is it that you
are fixing?"), would be a useful justification that is faithful to
the history. Of course, ", even though..." part is irrelevant
and/or unnecessary in that description of the motivation, and if you
omit it, I wouldn't call that is distorting the history.
> Of course, large
> operators like GitHub will need a more complicated management strategy,
> but for ordinary usage, git should just work.
True. "should just work" may want to be replaced by what exactly
are the things it currently does that you view as its problems.
Once you say that, "git learns to do x and y" in the next paragraph,
i.e. the description of the solution to the problem, starts making
sense.
>
> In this commit, git learns to ignore gc.log files which are older than
> (by default) one day old. It also learns about a config, gc.logExpiry
> to manage this. There is also some cleanup: a successful manual gc,
> or a warning-free auto gc with an old log file, will remove any old
> gc.log files.
>
> So git should never get itself into a state where it refuses to do any
> maintenance, just because at some point some piece of the maintenance
> didn't make progress. That might still happen (e.g. because the repo
> is corrupt), but at the very least it won't be because Git is too dumb
> to try again.
IOW, what you wrote in this last paragraph can come earlier to
explain what you perceive as problems the current behaviour has.
> Signed-off-by: David Turner <dturner@twosigma.com>
> Helped-by: Jeff King <peff@peff.net>
> ---
A v2 patch is unfriendly to reviewers unless it is sent with summary
of what got changed since v1, taking input from the discussion on
the previous round, and here before the diffstat is a good place to
do so.
> +gc.logExpiry::
> + If the file gc.log exists, then `git gc --auto` won't run
> + unless that file is more than 'gc.logExpiry' old. Default is
> + "1.day". See `gc.pruneExpire` for more possible values.
> +
Micronit. Perhaps you meant by "more possible values" "more ways to
specify its values", IOW, you didn't mean to say "instead of 1.day,
you can say 2.days".
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 331f21926..46edcff30 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -33,6 +33,7 @@ static int aggressive_window = 250;
> static int gc_auto_threshold = 6700;
> static int gc_auto_pack_limit = 50;
> static int detach_auto = 1;
> +static unsigned long gc_log_expire_time;
> static const char *prune_expire = "2.weeks.ago";
> static const char *prune_worktrees_expire = "3.months.ago";
>
> @@ -76,10 +77,12 @@ static void git_config_date_string(const char *key, const char **output)
> static void process_log_file(void)
> {
> struct stat st;
> - if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
> + if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size) {
> commit_lock_file(&log_lock);
> - else
> + } else {
> + unlink(git_path("gc.log"));
> rollback_lock_file(&log_lock);
After we grab a lock by creating gc.log.lock, if we fail to fstat(2),
we remove gc.log? That does not sound quite right, as the failure
to fstat(2) sounds like a log-worthy event. Removing the log after
noticing that we didn't write anything (i.e. st.st_size being 0) is
quite sensible, though.
> @@ -111,6 +114,11 @@ static void gc_config(void)
> git_config_get_int("gc.auto", &gc_auto_threshold);
> git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> git_config_get_bool("gc.autodetach", &detach_auto);
> +
> + if (!git_config_get_value("gc.logexpiry", &value)) {
> + parse_expiry_date(value, &gc_log_expire_time);
> + }
Drop {}?
> @@ -290,19 +298,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> static int report_last_gc_error(void)
> {
> struct strbuf sb = STRBUF_INIT;
> - int ret;
> + int ret = 0;
> + struct stat st;
> + char *gc_log_path = git_pathdup("gc.log");
>
> - ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
> + if (stat(gc_log_path, &st)) {
> + if (errno == ENOENT)
> + goto done;
> +
> + ret = error(_("Can't read %s"), gc_log_path);
You probably want to use error_errno() instead here. This is not
"can't read"; your stat() noticed there is something wrong and you
gave up before you even attempted to read.
> + goto done;
> + }
> +
> + if (st.st_mtime < gc_log_expire_time)
> + goto done;
OK.
> + ret = strbuf_read_file(&sb, gc_log_path, 0);
> if (ret > 0)
> - return error(_("The last gc run reported the following. "
> + ret = error(_("The last gc run reported the following. "
> "Please correct the root cause\n"
> "and remove %s.\n"
> "Automatic cleanup will not be performed "
> "until the file is removed.\n\n"
> "%s"),
> - git_path("gc.log"), sb.buf);
> + gc_log_path, sb.buf);
> strbuf_release(&sb);
> - return 0;
> +done:
> + free(gc_log_path);
> + return ret;
> }
OK.
> @@ -349,6 +372,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> argv_array_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
> argv_array_pushl(&rerere, "rerere", "gc", NULL);
>
> + /* default expiry time, overwritten in gc_config */
> + parse_expiry_date("1.day", &gc_log_expire_time);
Alternatively, we can mimick the way in which prune_expire and
prune_worktrees_expire are set up (i.e. they are kept as strings,
configuration overwrites the string), and then turn the final string
into value after gc_config() returns. I think what you wrote here
may be simpler. Nice.
> gc_config();
>
> if (pack_refs < 0)
> @@ -448,5 +473,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> warning(_("There are too many unreachable loose objects; "
> "run 'git prune' to remove them."));
>
> + if (!daemonized)
> + unlink(git_path("gc.log"));
> +
OK. We want to remove "gc.log" after running a successful
foreground gc and this does exactly that.
^ permalink raw reply [flat|nested] 2+ messages in thread