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

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Jul 17, 2018 at 3:53 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
> Subject: gc: do not return error for prior errors in daemonized mode
>
> Some build machines started failing to fetch updated source using
> "repo sync", with error
>
>   error: The last gc run reported the following. Please correct the root cause
>   and remove /build/.repo/projects/tools/git.git/gc.log.
>   Automatic cleanup will not be performed until the file is removed.
>
>   warning: There are too many unreachable loose objects; run 'git prune' to remove them.
>
> The cause takes some time to describe.
>
> In v2.0.0-rc0~145^2 (gc: config option for running --auto in
> background, 2014-02-08), "git gc --auto" learned to run in the
> background instead of blocking the invoking command.  In this mode, it
> closed stderr to avoid interleaving output with any subsequent
> commands, causing warnings like the above to be swallowed; v2.6.3~24^2
> (gc: save log from daemonized gc --auto and print it next time,
> 2015-09-19) addressed this by storing any diagnostic output in
> .git/gc.log and allowing the next "git gc --auto" run to print it.
>
> To avoid wasteful repeated fruitless gcs, when gc.log is present, the
> subsequent "gc --auto" would die after print its contents.  Most git
> 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.
>
> 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
>
>  - 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.
>
>  - if there is any diagnostic output in gc.log, subsequent runs return
>    a nonzero exit status to indicate that gc was not triggered.
>
> 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).  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.

This background gc thing is pretty much designed for interactive use.
When it's scripted, you probably should avoid it. Then you can fork a
new process by yourself and have much better control if you still want
"background" gc. So an alternative here is to turn on or off
gc.autodetach based on interactiveness (i.e. having tty). We can add a
new (and default) value "auto" to gc.autodetach for this purpose.

In automated scripts, it will always run in non-damonized mode. You
will get non-zero exit code when real errors occur. You don't worry
about hanging processes. Rate limit is also thrown out in this mode if
I'm not mistaken, but it could be added back and more tailored for
server needs.

> 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 &&
> --
> 2.18.0.233.g985f88cf7e
>


-- 
Duy