Re: [PATCH] gc: do not warn about too many loose objects

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

Jonathan Nieder <jrnieder@gmail.com> writes:

> That doesn't really solve the problem:
>
>  1. "gc --auto" would produce noise *every time it runs* until gc.log
>     is removed, for example via expiry
>
>  2. "gc --auto" would not do any garbage collection until gc.log is
>     removed, for example by expiry
>
>  3. "gc --auto" would still not ratelimit itself, for example when
>     there are a large number of loose unreachable objects that is not
>     large enough to hit the loose object threshold.
>
> but maybe it's better than the present state.
>
> To solve (1) and (2), we could introduce a gc.warnings file that
> behaves like gc.log except that it only gets printed once and then
> self-destructs, allowing gc --auto to proceed.  

That makes it not rate-limit, no?

> To solve (3), we could
> introduce a gc.lastrun file that is touched whenever "gc --auto" runs
> successfully and make "gc --auto" a no-op for a while after each run.

Does absolute time-interval make sense here?  Some repositories may
be busily churning new objects and for them 5-minute interval may be
too infrequent, while other repositories may be relatively slow and
once a day may be sufficient for them.  I wonder if we can somehow
auto-tune this.

> To avoid wasteful repeated fruitless gcs, when gc.log is present, the
> subsequent "gc --auto" would die after print its contents.  Most git

s/print/&ing/

> commands, such as "git fetch", ignore the exit status from "git gc
> --auto" so all is well at this point: the user gets to see the error
> message, and the fetch succeeds, without a wasteful additional attempt
> at an automatic gc.

The above, by saying "Most git commands", leaves readers want to
know "then what are minority git commands that do not correctly do
so?"  If you are not going to answer that question, it probably is
better not to even say "Most".

> External tools like repo[1], though, do care about the exit status
> from "git gc --auto".  In non-daemonized mode, the exit status is
> straightforward: if there is an error, it is nonzero, but after a
> warning like the above, the status is zero.  The daemonized mode, as a
> side effect of the other properties provided, offers a very strange
> exit code convention:
>
>  - if no housekeeping was required, the exit status is 0

OK.

>  - the first real run, after forking into the background, returns exit
>    status 0 unconditionally.  The parent process has no way to know
>    whether gc will succeed.

Understandable; allowing the parent to exit before we know the
outcome of the gc is the whole point of backgrounding.

>  - if there is any diagnostic output in gc.log, subsequent runs return
>    a nonzero exit status to indicate that gc was not triggered.

This is unfortunate.

> There's nothing for the calling program to act on on the basis of that
> error.  Use status 0 consistently instead, to indicate that we decided
> not to run a gc (just like if no housekeeping was required).  

s/.$/ in the last case./  And I fully agree with the reasoning.

> This
> way, repo and similar tools can get the benefit of the same behavior
> as tools like "git fetch" that ignore the exit status from gc --auto.
>
> Once the period of time described by gc.pruneExpire elapses, the
> unreachable loose objects will be removed by "git gc --auto"
> automatically.
>
> [1] https://gerrit-review.googlesource.com/c/git-repo/+/10598/
>
> Reported-by: Andrii Dehtiarov <adehtiarov@google.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  Documentation/config.txt |  3 ++-
>  builtin/gc.c             | 16 +++++++++++-----
>  t/t6500-gc.sh            |  6 +++---
>  3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1cc18a828c..5eaa4aaa7d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below
>  gc.autoPackLimit and gc.bigPackThreshold should be respected again.
>  
>  gc.logExpiry::
> -	If the file gc.log exists, then `git gc --auto` won't run
> +	If the file gc.log exists, then `git gc --auto` will print
> +	its content and exit with status zero instead of running
>  	unless that file is more than 'gc.logExpiry' old.  Default is
>  	"1.day".  See `gc.pruneExpire` for more ways to specify its
>  	value.
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 2bebc52bda..484ab21b8c 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -438,7 +438,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>  	return NULL;
>  }
>  
> -static void report_last_gc_error(void)
> +static int report_last_gc_error(void)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	ssize_t ret;
> @@ -449,7 +449,7 @@ static void report_last_gc_error(void)
>  		if (errno == ENOENT)
>  			goto done;
>  
> -		die_errno(_("cannot stat '%s'"), gc_log_path);
> +		return error_errno(_("cannot stat '%s'"), gc_log_path);
>  	}
>  
>  	if (st.st_mtime < gc_log_expire_time)
> @@ -457,9 +457,9 @@ static void report_last_gc_error(void)
>  
>  	ret = strbuf_read_file(&sb, gc_log_path, 0);
>  	if (ret < 0)
> -		die_errno(_("cannot read '%s'"), gc_log_path);
> +		return error_errno(_("cannot read '%s'"), gc_log_path);
>  	if (ret > 0)
> -		die(_("The last gc run reported the following. "
> +		return error(_("The last gc run reported the following. "
>  			       "Please correct the root cause\n"
>  			       "and remove %s.\n"
>  			       "Automatic cleanup will not be performed "
> @@ -469,6 +469,7 @@ static void report_last_gc_error(void)
>  	strbuf_release(&sb);
>  done:
>  	free(gc_log_path);
> +	return 0;
>  }
>  
>  static void gc_before_repack(void)
> @@ -561,7 +562,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
>  		}
>  		if (detach_auto) {
> -			report_last_gc_error(); /* dies on error */
> +			if (report_last_gc_error())
> +				/*
> +				 * A previous gc failed. We've reported the
> +				 * error, so there's nothing left to do.
> +				 */
> +				return 0;
>  
>  			if (lock_repo_for_gc(force, &pid))
>  				return 0;
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index c474a94a9f..3e62df616c 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re
>  	test_config gc.autopacklimit 1 &&
>  	test_config gc.autodetach true &&
>  	echo fleem >.git/gc.log &&
> -	test_must_fail git gc --auto 2>err &&
> -	test_i18ngrep "^fatal:" err &&
> +	git gc --auto 2>err &&
> +	test_i18ngrep "^error:" err &&
>  	test_config gc.logexpiry 5.days &&
>  	test-tool chmtime =-345600 .git/gc.log &&
> -	test_must_fail git gc --auto &&
> +	git gc --auto &&
>  	test_config gc.logexpiry 2.days &&
>  	run_and_wait_for_auto_gc &&
>  	ls .git/objects/pack/pack-*.pack >packs &&