Re: [PATCH v2] gc: save log from daemonized gc --auto and print it next time

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.
>
> Besides reverting 9f673f9 and looking at the problem from another
> angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc
> --auto' will print the saved warnings, delete gc.log and exit.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  The test is dropped. It does not feel good enough.. The rest of
>  changes is minor

I still wonder what this buys us if multiple auto-gc's are triggered,
because the user is running a long svn import or something similar.

I cannot tell what problem this is trying to solve.

 (1) If the error output from the previous "gc --auto" indicates a
     grave error condition that further re-running of "gc --auto" is
     pointless, why is it a good idea to blindly remove the log and
     "let the next one run as usual"?

 (2) If the problem it is trying to solve is that "gc --auto"
     sometimes gives a good suggestion but that is lost when
     daemonized, why not address that problem in a more direct way,
     e.g. introduce a new option "gc --probe-auto" that only checks
     and reports if "gc --auto" would spend actual cycles to do its
     work, and then run the daemonized version of "gc --auto" when
     necessary?  Either "gc --probe-auto" itself or the caller of
     "gc --probe-auto" can give the "you should do this to avoid
     repeated auto-gc" when this happens.

     Also the same issue I have with (1) above applies; I do not see
     much linkage with solving this issue with halving the frequency
     of running "gc --auto" which is what this patch does.

In other words, I would understand what the change is trying to
achieve if it were to always stop, instruct the user to take
corrective action, and never remove the .log file itself (naturally,
the corrective action would include "remove the .log when you are
done").  I do not necessarily agree that it would be a good change
for the overall system, but at least such a change is internally
consistent between its perception of a problem and its approach to
solve it.

I would also understand, and I suspect I would prefer it if I see
the result, if the change were to allow "gc --auto" to report
severity of its findings (i.e. "nothing wrong in your repository but
you should do X to avoid triggering me too often" vs "no point
running me again and again"), do something similar to what this
patch does and after showing the message and (1) stop without
removing the log but give instruction when the situation is grave,
or (2) show the message, remove the .log, and continue to go ahead
with "gc --auto" when the situation is *not* grave.

But the approach taken by this patch looks very confused about its
own purpose to me and I cannot quite tell what it is trying to
solve.

>   diff --git a/builtin/gc.c b/builtin/gc.c
>   index 07769a9..3886937 100644
>   --- a/builtin/gc.c
>   +++ b/builtin/gc.c
>   @@ -32,8 +32,6 @@ static int aggressive_window = 250;
>    static int gc_auto_threshold = 6700;
>    static int gc_auto_pack_limit = 50;
>    static int detach_auto = 1;
>   -static struct strbuf log_filename = STRBUF_INIT;
>   -static int daemonized;
>    static const char *prune_expire = "2.weeks.ago";
>    
>    static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;
>   @@ -43,6 +41,8 @@ static struct argv_array prune = 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)
>    {
>   @@ -338,7 +338,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>    			struct strbuf sb = STRBUF_INIT;
>    
>    			if (strbuf_read_file(&sb, git_path("gc.log"), 0) > 0) {
>   -				warning(_("Last gc run reported the following, gc skipped"));
>   +				warning(_("skipping gc; last gc reported:"));
>    				fputs(sb.buf, stderr);
>    				strbuf_release(&sb);
>    				/* let the next gc --auto run as usual */
>   @@ -352,8 +352,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>    			 * failure to daemonize is ok, we'll continue
>    			 * in foreground
>    			 */
>   -			if (!daemonize())
>   -				daemonized = 1;
>   +			daemonized = !daemonize();
>    		}
>    	} else
>    		add_repack_all_option();
>
>  builtin/gc.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 5c634af..3886937 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -41,9 +41,20 @@ static struct argv_array prune = 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);
>  }
> @@ -324,13 +335,24 @@ 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) {
> +				warning(_("skipping gc; last gc reported:"));
> +				fputs(sb.buf, stderr);
> +				strbuf_release(&sb);
> +				/* let the next gc --auto run as usual */
> +				unlink(git_path("gc.log"));
> +				return 0;
> +			}
> +
>  			if (gc_before_repack())
>  				return -1;
>  			/*
>  			 * failure to daemonize is ok, we'll continue
>  			 * in foreground
>  			 */
> -			daemonize();
> +			daemonized = !daemonize();
>  		}
>  	} else
>  		add_repack_all_option();
> @@ -343,6 +365,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;