Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 14, 2015, 17:24 UTC
- Message-ID
- <xmqq8u88ykv4.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1442108202-25746-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 19 quoted lines
> While commit 9f673f9 (gc: config option for running --auto in > background - 2014-02-08) helps reduce some complaints about 'gc > --auto' hogging the terminal, it creates another set of problems. > > The latest in this set is, as the result of daemonizing, stderr is > closed and all warnings are lost. This warning at the end of cmd_gc() > is particularly important because it tells the user how to avoid "gc > --auto" running repeatedly. Because stderr is closed, the user does > not know, naturally they complain about 'gc --auto' wasting CPU. > > Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc --auto > will not run and gc.log printed out until the user removes gc.log. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > When gc.log exists, gc --auto now simply exits > > builtin/gc.c | 33 ++++++++++++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-)
Thanks, will queue.
This is sufficient for non-novice users, but I wonder if the error message is strong enough.
A knee-jerk reaction to "Last run reported an error so we won't run until the log is removed" may be "Then why don't you remove the log for me automatically?", which is a total brain-less raction [*1*], and "then I'll remove the log" is not all that better. The message we want to convey is "correct the root cause so that gc can run without having to give an error message."
I guess it all depends on what is in gc.log; after all, this new codepath is not in a good position to read the diagnosis left in there and offer good pieces of advice to resolve them.
[Footnote]
*1* ... which is what a knee-jerk reaction is by definition.
Show 67 quoted lines
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index bcc75d9..2c3aaeb 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -43,9 +43,20 @@ static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;
> static struct argv_array rerere = ARGV_ARRAY_INIT;
>
> static char *pidfile;
> +static struct strbuf log_filename = STRBUF_INIT;
> +static int daemonized;
>
> static void remove_pidfile(void)
> {
> + if (daemonized && log_filename.len) {
> + struct stat st;
> +
> + close(2);
> + if (stat(log_filename.buf, &st) ||
> + !st.st_size ||
> + rename(log_filename.buf, git_path("gc.log")))
> + unlink(log_filename.buf);
> + }
> if (pidfile)
> unlink(pidfile);
> }
> @@ -330,13 +341,21 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
> }
> if (detach_auto) {
> + struct strbuf sb = STRBUF_INIT;
> + if (strbuf_read_file(&sb, git_path("gc.log"), 0) > 0)
> + return error(_("last gc run reported:\n"
> + "%s\n"
> + "not running until %s is removed"),
> + sb.buf, git_path("gc.log"));
> + strbuf_release(&sb);
> +
> if (gc_before_repack())
> return -1;
> /*
> * failure to daemonize is ok, we'll continue
> * in foreground
> */
> - daemonize();
> + daemonized = !daemonize();
> }
> } else
> add_repack_all_option();
> @@ -349,6 +368,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> name, (uintmax_t)pid);
> }
>
> + if (daemonized) {
> + int fd;
> +
> + strbuf_addstr(&log_filename, git_path("gc.log_XXXXXX"));
> + fd = xmkstemp(log_filename.buf);
> + if (fd >= 0) {
> + dup2(fd, 2);
> + close(fd);
> + } else
> + strbuf_release(&log_filename);
> + }
> +
> if (gc_before_repack())
> return -1;