Skip to content

Commit 3029970

Browse files
jrngitster
authored andcommitted
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 <[email protected]> Helped-by: Jeff King <[email protected]> Signed-off-by: Jonathan Nieder <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent fec2ed2 commit 3029970

File tree

3 files changed

+32
-10
lines changed

3 files changed

+32
-10
lines changed

Documentation/config.txt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1648,7 +1648,8 @@ will be repacked. After this the number of packs should go below
16481648
gc.autoPackLimit and gc.bigPackThreshold should be respected again.
16491649

16501650
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
16521653
unless that file is more than 'gc.logExpiry' old. Default is
16531654
"1.day". See `gc.pruneExpire` for more ways to specify its
16541655
value.

builtin/gc.c

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -438,9 +438,15 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
438438
return NULL;
439439
}
440440

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)
442447
{
443448
struct strbuf sb = STRBUF_INIT;
449+
int ret = 0;
444450
ssize_t len;
445451
struct stat st;
446452
char *gc_log_path = git_pathdup("gc.log");
@@ -449,26 +455,35 @@ static void report_last_gc_error(void)
449455
if (errno == ENOENT)
450456
goto done;
451457

452-
die_errno(_("cannot stat '%s'"), gc_log_path);
458+
ret = error_errno(_("cannot stat '%s'"), gc_log_path);
459+
goto done;
453460
}
454461

455462
if (st.st_mtime < gc_log_expire_time)
456463
goto done;
457464

458465
len = strbuf_read_file(&sb, gc_log_path, 0);
459466
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. "
463475
"Please correct the root cause\n"
464476
"and remove %s.\n"
465477
"Automatic cleanup will not be performed "
466478
"until the file is removed.\n\n"
467479
"%s"),
468480
gc_log_path, sb.buf);
481+
ret = 1;
482+
}
469483
strbuf_release(&sb);
470484
done:
471485
free(gc_log_path);
486+
return ret;
472487
}
473488

474489
static void gc_before_repack(void)
@@ -561,7 +576,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
561576
fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
562577
}
563578
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;
565586

566587
if (lock_repo_for_gc(force, &pid))
567588
return 0;

t/t6500-gc.sh

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re
116116
test_config gc.autopacklimit 1 &&
117117
test_config gc.autodetach true &&
118118
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 &&
121121
test_config gc.logexpiry 5.days &&
122122
test-tool chmtime =-345600 .git/gc.log &&
123-
test_must_fail git gc --auto &&
123+
git gc --auto &&
124124
test_config gc.logexpiry 2.days &&
125125
run_and_wait_for_auto_gc &&
126126
ls .git/objects/pack/pack-*.pack >packs &&

0 commit comments

Comments
 (0)