gc: do not return error for prior errors in daemonized mode

Some build machines started consistently 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 that 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 printing 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. 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jonathan Nieder committed Jul 16, 2018 at 23:57 UTC 3029970275b473dbf62149887a19a6b4879528d7
3 files changed +32 -10
Documentation/config.txt
+2 -1
@@ -1648,7 +1648,8 @@ will be repacked. After this the number of packs should go below
1648 gc.autoPackLimit and gc.bigPackThreshold should be respected again.
1649
1650 gc.logExpiry::
1651 - If the file gc.log exists, then `git gc --auto` won't run
1651 + If the file gc.log exists, then `git gc --auto` will print
1652 + its content and exit with status zero instead of running
1653 unless that file is more than 'gc.logExpiry' old. Default is
1654 "1.day". See `gc.pruneExpire` for more ways to specify its
1655 value.
builtin/gc.c
+27 -6
@@ -438,9 +438,15 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
438 return NULL;
439 }
440
441 -static void report_last_gc_error(void)
441 +/*
442 + * Returns 0 if there was no previous error and gc can proceed, 1 if
443 + * gc should not proceed due to an error in the last run. Prints a
444 + * message and returns -1 if an error occured while reading gc.log
445 + */
446 +static int report_last_gc_error(void)
447 {
448 struct strbuf sb = STRBUF_INIT;
449 + int ret = 0;
450 ssize_t len;
451 struct stat st;
452 char *gc_log_path = git_pathdup("gc.log");
@@ -449,7 +455,8 @@ static void report_last_gc_error(void)
455 if (errno == ENOENT)
456 goto done;
457
452 - die_errno(_("cannot stat '%s'"), gc_log_path);
458 + ret = error_errno(_("cannot stat '%s'"), gc_log_path);
459 + goto done;
460 }
461
462 if (st.st_mtime < gc_log_expire_time)
@@ -457,18 +464,26 @@ static void report_last_gc_error(void)
464
465 len = strbuf_read_file(&sb, gc_log_path, 0);
466 if (len < 0)
460 - die_errno(_("cannot read '%s'"), gc_log_path);
461 - else if (len > 0)
462 - die(_("The last gc run reported the following. "
467 + ret = error_errno(_("cannot read '%s'"), gc_log_path);
468 + else if (len > 0) {
469 + /*
470 + * A previous gc failed. Report the error, and don't
471 + * bother with an automatic gc run since it is likely
472 + * to fail in the same way.
473 + */
474 + warning(_("The last gc run reported the following. "
475 "Please correct the root cause\n"
476 "and remove %s.\n"
477 "Automatic cleanup will not be performed "
478 "until the file is removed.\n\n"
479 "%s"),
480 gc_log_path, sb.buf);
481 + ret = 1;
482 + }
483 strbuf_release(&sb);
484 done:
485 free(gc_log_path);
486 + return ret;
487 }
488
489 static void gc_before_repack(void)
@@ -561,7 +576,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
576 fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
577 }
578 if (detach_auto) {
564 - report_last_gc_error(); /* dies on error */
579 + int ret = report_last_gc_error();
580 + if (ret < 0)
581 + /* an I/O error occured, already reported */
582 + exit(128);
583 + if (ret == 1)
584 + /* Last gc --auto failed. Skip this one. */
585 + return 0;
586
587 if (lock_repo_for_gc(force, &pid))
588 return 0;
t/t6500-gc.sh
+3 -3
@@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re
116 test_config gc.autopacklimit 1 &&
117 test_config gc.autodetach true &&
118 echo fleem >.git/gc.log &&
119 - test_must_fail git gc --auto 2>err &&
120 - test_i18ngrep "^fatal:" err &&
119 + git gc --auto 2>err &&
120 + test_i18ngrep "^warning:" err &&
121 test_config gc.logexpiry 5.days &&
122 test-tool chmtime =-345600 .git/gc.log &&
123 - test_must_fail git gc --auto &&
123 + git gc --auto &&
124 test_config gc.logexpiry 2.days &&
125 run_and_wait_for_auto_gc &&
126 ls .git/objects/pack/pack-*.pack >packs &&