git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode

From
Jeff King <peff@peff.net>
Date
Jul 17, 2018, 20:13 UTC
Message-ID
<20180717201348.GD26218@sigill.intra.peff.net>
In-Reply-To
<20180717065740.GD177907@aiede.svl.corp.google.com>
On Mon, Jul 16, 2018 at 11:57:40PM -0700, Jonathan Nieder wrote:
Show 21 quoted lines
> 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.
I think this is a good change.

In theory it might be useful to propagate the original exit code (_not_ "did we see a warning or an error", but the true original exit code. But as you note, it's not deterministic anyway (we'd miss that exit code on the first run, or even any simultaneous runs that exit early due to lock contention). So it's clear that callers can't really do anything robust based on the exit code of a daemonized "gc --auto".

I still think that "repo" should probably stop respecting the exit code. But that's no excuse for Git not to have a sensible exit code in the first place.

The patch itself looks good overall. A few comments (none of which I think even requires a fix):

Show 14 quoted lines
> 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.

Yeah, this is definitely worth documenting. I was surprised there's no discussion of daemonization at all in git-gc(1). I don't think adding it is a blocker for this series, though.

Show 10 quoted lines
>  static void gc_before_repack(void)
> @@ -561,7 +576,13 @@ 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 */
> +			int ret = report_last_gc_error();
> +			if (ret < 0)
> +				/* an I/O error occured, already reported */
> +				exit(128);

We have a few exit(128)'s sprinkled throughout the code-base, and I always wonder if they will one day go stale if we change the code that die() uses. But it probably doesn't matter, and anyway I don't think there is a better way to do this currently.

I would have written "return 128" since the other case arm also returns, but I really cannot think of a reason to prefer one over the other.

Show 16 quoted lines
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index c474a94a9f..a222efdbe1 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 "^warning:" err &&
>  	test_config gc.logexpiry 5.days &&
>  	test-tool chmtime =-345600 .git/gc.log &&
> -	test_must_fail git gc --auto &&
> +	git gc --auto &&

Nice. At first I thought this was changing an existing test to cover the new case (which I usually frown on), but it is just that your patch is intentionally changing the case covered here. So this is the right thing to do.

-Peff
Previous: Jonathan NiederNext: Junio C Hamano
Message 52 of 57 in “gc: do not warn about too many loose objects”
  1. gc: do not warn about too many loose objectsJonathan Tan, Jul 16, 2018
  2. Jeff KingJul 16, 2018
  3. Jonathan NiederJul 16, 2018
  4. Jeff KingJul 16, 2018
  5. Jonathan NiederJul 16, 2018
  6. Jeff KingJul 16, 2018
  7. Jonathan NiederJul 16, 2018
  8. Jeff KingJul 16, 2018
  9. Jonathan NiederJul 16, 2018
  10. Jeff KingJul 16, 2018
  11. Jonathan NiederJul 16, 2018
  12. Jeff KingJul 16, 2018
  13. Jonathan NiederJul 16, 2018
  14. Jeff KingJul 16, 2018
  15. Jonathan NiederJul 16, 2018
  16. Jeff KingJul 16, 2018
  17. Jonathan NiederJul 17, 2018
  18. Ævar Arnfjörð BjarmasonJul 17, 2018
  19. Jonathan NiederJul 17, 2018
  20. Ævar Arnfjörð BjarmasonJul 17, 2018
  21. Jeff KingJul 17, 2018
  22. Ævar Arnfjörð BjarmasonJul 18, 2018
  23. Jeff KingJul 18, 2018
  24. Duy NguyenJul 17, 2018
  25. Junio C HamanoJul 17, 2018
  26. Elijah NewrenJul 16, 2018
  27. Jonathan NiederJul 16, 2018
  28. Elijah NewrenJul 16, 2018
  29. Jeff KingJul 16, 2018
  30. Jonathan NiederJul 16, 2018
  31. Jeff KingJul 16, 2018
  32. Jeff KingJul 16, 2018
  33. Elijah NewrenJul 16, 2018
  34. Jeff KingJul 16, 2018
  35. Elijah NewrenJul 16, 2018
  36. Jeff KingJul 16, 2018
  37. Elijah NewrenJul 16, 2018
  38. Jeff KingJul 16, 2018
  39. Elijah NewrenJul 16, 2018
  40. Jonathan NiederJul 16, 2018
  41. 0/3 gc --auto: do not return error for prior errors in daemonized modeJonathan Nieder, Jul 17, 2018
  42. 1/3 gc: improve handling of errors reading gc.logJonathan Nieder, Jul 17, 2018
  43. Junio C HamanoJul 17, 2018
  44. Jeff KingJul 17, 2018
  45. 2/3 gc: exit with status 128 on failureJonathan Nieder, Jul 17, 2018
  46. Junio C HamanoJul 17, 2018
  47. Jeff KingJul 17, 2018
  48. Jeff KingSep 17, 2018
  49. Jonathan NiederSep 17, 2018
  50. Jeff KingSep 18, 2018
  51. 3/3 gc: do not return error for prior errors in daemonized modeJonathan Nieder, Jul 17, 2018
  52. Jeff KingJul 17, 2018
  53. Junio C HamanoJul 18, 2018
  54. Jeff KingJul 18, 2018
  55. Junio C HamanoJul 18, 2018
  56. Jeff KingJul 18, 2018
  57. Junio C HamanoJul 18, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.