Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> 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@xxxxxxxxx>
> ---
> 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.
>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html