[PATCH] gc: introduce an --auto-exit-code option for undoing 3029970275

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

Add an --auto-exit-code variable and a corresponding 'gc.autoExitCode'
configuration option to optionally bring back the 'git gc --auto' exit
code behavior as it existed between 2.6.3..2.19.0 (inclusive).

This was changed in 3029970275 ("gc: do not return error for prior
errors in daemonized mode", 2018-07-16). The motivation for that patch
was to appease 3rd party tools whose treatment of the 'git gc --auto'
exit code is different from that of git core where it has always been
ignored.

That means that out of the three modes gc --auto will operate in:

 1. gc --auto has nothing to do
 2. gc --auto has something to do, will fork and try to do it
 3. gc --auto has something to do, but notices that gc has been failing
    before when forked and can't do anything now.

We started exiting with zero in the case of #3, instead of
non-zero (see [1] for more details). As noted by the docs being added
here the #3 case is relatively rare, so I think it's fine to change
this as the default with the assumption that the use-case for tools
like the "repo" tool noted in commit 3029970275 above are more common
than not.

But it left us without any option to have "git gc --auto" tell us
about this failure except by starting to either parse its output, or
for the caller to start breaking the encapsulation and starting to
check for .git/gc.log themselves.

Let's instead provide an option to exit with non-zero when we have
errors to tell the user about, and provide a configuration option so
that it can be dropped in-place in anticipation of upgrading to Git
version 2.20 without having to make using --auto-exit-code conditional
on the git version in use.

1. https://public-inbox.org/git/878t69dgvx.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

> On Wed, Oct 10, 2018 at 09:51:52AM -0700, Jonathan Nieder wrote:
>
>> Ævar Arnfjörð Bjarmason wrote:
>> 
>> > I'm just saying it's hard in this case to remove one piece without the
>> > whole Jenga tower collapsing, and it's probably a good idea in some of
>> > these cases to pester the user about what he wants, but probably not via
>> > gc --auto emitting the same warning every time, e.g. in one of these
>> > threads I suggested maybe "git status" should emit this.
>> 
>> I have to say, I don't have a lot of sympathy for this.
>> 
>> I've been running with the patches I sent before for a while now, and
>> the behavior that they create is great.  I think we can make further
>> refinements on top.  To put it another way, I haven't actually
>> experienced any bad knock-on effects, and I think other feature
>> requests can be addressed separately.
>
> I think there may be some miscommunication here. The Jenga tower above
> is referring (I think) to Jonathan Tan's original patch to drop the
> warning entirely, which does have some unwanted side effects.
>
> Your patches are much less controversial, I think, and are in next and
> marked as "will merge to master" in the last "what's cooking".

[Junio: This goes on top of gitster/jn/gc-auto]

I thought the jn/gc-auto topic was still in "pu", my fault for not
paying attention. It seems the general consensus is against my notion
of what should be the default (which is fine), but since (as noted in
the patch) it seems yucky to start breaking the encapsulation of
gc.log, especially as it's looking more and more likely that it'll be
an implementation detail we might drop, here's a patch on top of
jn/gc-auto to optionally restore the old behavior.

 Documentation/config.txt | 28 ++++++++++++++++++++++++++++
 Documentation/git-gc.txt |  7 +++++++
 builtin/gc.c             |  7 ++++++-
 t/t6500-gc.sh            |  2 ++
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5b72684999..e37a463bf8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1635,6 +1635,34 @@ gc.autoDetach::
 	Make `git gc --auto` return immediately and run in background
 	if the system supports it. Default is true.
 
+gc.autoExitCode::
+	Make `git gc --auto` return non-zero if it would have
+	demonized itself (see `gc.autoDetach`) due to a needed gc, but
+	a 'gc.log' is found within the `gc.logExpiry` with an error
+	from a previous run.
++
+When 'git gc' is run with the default of `gc.autoDetach=true` a
+failure might have been noted in the 'gc.log' from a previously
+detached `--auto` run. If the failure is within the time configured in
+`gc.logExpiry` the `--auto` run will abort early and report the error
+in the 'gc.log'.
++
+From version 2.6.3 to version 2.19 of Git encountering this error
+would cause 'git gc' to exit with non-zero, but this was deemed to be
+a hassle for third-party tools to handle since it rarely happens, and
+they usually don't assume that 'git gc --auto' can fail. Therefore
+since version 2.20 of Git 'git gc --auto' will always exit with zero
+if it would have demonized itself, even when encountering such an
+error.
++
+Supplying this option will make 'git gc' exit with non-zero in that
+case, which allows for detecting cases where a repository is in a
+state where git 'gc --auto' is refusing to demonize due to previously
+encountered errors.
++
+This option can also be turned on as a one-off with the
+`--auto-exit-code` option, see linkgit:git-gc[1].
+
 gc.bigPackThreshold::
 	If non-zero, all packs larger than this limit are kept when
 	`git gc` is run. This is very similar to `--keep-base-pack`
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 24b2dd44fe..adf53fc959 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -71,6 +71,13 @@ If houskeeping is required due to many loose objects or packs, all
 other housekeeping tasks (e.g. rerere, working trees, reflog...) will
 be performed as well.
 
+--auto-exit-code::
+	Make `git gc --auto` return non-zero if it would have
+	demonized itself (see `gc.autoDetach`) due to a needed gc, but
+	a 'gc.log' is found within the time period of `gc.logExpiry`
+	with an error from a previous run. See linkgit:git-config[1]
+	for more details about this option which can also be
+	configured with the `gc.autoExitCode` boolean variable.
 
 --prune=<date>::
 	Prune loose objects older than date (default is 2 weeks ago,
diff --git a/builtin/gc.c b/builtin/gc.c
index ce8a663a01..69c0dedd8c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -41,6 +41,7 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static int detach_auto = 1;
+static int gc_auto_exit_code = 0;
 static timestamp_t gc_log_expire_time;
 static const char *gc_log_expire = "1.day.ago";
 static const char *prune_expire = "2.weeks.ago";
@@ -130,6 +131,7 @@ static void gc_config(void)
 	git_config_get_int("gc.auto", &gc_auto_threshold);
 	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
 	git_config_get_bool("gc.autodetach", &detach_auto);
+	git_config_get_bool("gc.autoexitcode", &gc_auto_exit_code);
 	git_config_get_expiry("gc.pruneexpire", &prune_expire);
 	git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);
 	git_config_get_expiry("gc.logexpiry", &gc_log_expire);
@@ -518,6 +520,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
 		OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"),
 			   PARSE_OPT_NOCOMPLETE),
+		OPT_BOOL_F(0, "auto-exit-code", &gc_auto_exit_code,
+			   N_("exit with non-zero if an error is encountered before gc is daemonized"),
+			   PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL_F(0, "force", &force,
 			   N_("force running gc even if there may be another gc running"),
 			   PARSE_OPT_NOCOMPLETE),
@@ -582,7 +587,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 				exit(128);
 			if (ret == 1)
 				/* Last gc --auto failed. Skip this one. */
-				return 0;
+				return gc_auto_exit_code;
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index a222efdbe1..25bce70316 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -121,6 +121,8 @@ test_expect_success 'background auto gc does not run if gc.log is present and re
 	test_config gc.logexpiry 5.days &&
 	test-tool chmtime =-345600 .git/gc.log &&
 	git gc --auto &&
+	test_must_fail git gc --auto --auto-exit-code &&
+	test_must_fail git -c gc.autoExitCode=true gc --auto &&
 	test_config gc.logexpiry 2.days &&
 	run_and_wait_for_auto_gc &&
 	ls .git/objects/pack/pack-*.pack >packs &&
-- 
2.19.1.390.gf3a00b506f