[PATCH] fetch: only run 'gc' once when fetching multiple remotes

[PATCH] fetch: only run 'gc' once when fetching multiple remotes

[Nguyễn Thái Ngọc Duy]

In multiple remotes mode, git-fetch is launched for n-1 remotes and the
last remote is handled by the current process. Each of these processes
will in turn run 'gc' at the end.

This is not really a problem because even if multiple 'gc --auto' is run
at the same time we still handle it correctly. It does show multiple
"auto packing in the background" messages though. And we may waste some
resources when gc actually runs because we still do some stuff before
checking the lock and moving it to background.

So let's try to avoid that. We should only need one 'gc' run after all
objects and references are added anyway. Add a new option --no-auto-gc
that will be used by those n-1 processes. 'gc --auto' will always run on
the main fetch process (*).

(*) even if we fetch remotes in parallel at some point in future, this
    should still be fine because we should "join" all those processes
    before this step.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt |  4 ++++
 builtin/fetch.c                 | 17 +++++++++++------
 t/t5514-fetch-multiple.sh       |  7 +++++--
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 91c47752ec..592f391298 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -88,6 +88,10 @@ ifndef::git-pull[]
 	Allow several <repository> and <group> arguments to be
 	specified. No <refspec>s may be specified.
 
+--[no-]auto-gc::
+	Run `git gc --auto` at the end to perform garbage collection
+	if needed. This is enabled by default.
+
 -p::
 --prune::
 	Before fetching, remove any remote-tracking references that no
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ba63d5ac6..6a3c507897 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -48,6 +48,7 @@ static int prune_tags = -1; /* unspecified */
 
 static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;
 static int progress = -1;
+static int enable_auto_gc = 1;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
 static enum transport_family family;
@@ -169,6 +170,8 @@ static struct option builtin_fetch_options[] = {
 	OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
 			N_("report that we have only objects reachable from this object")),
 	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
+	OPT_BOOL(0, "auto-gc", &enable_auto_gc,
+		 N_("run 'gc --auto' after fetching")),
 	OPT_END()
 };
 
@@ -1424,7 +1427,7 @@ static int fetch_multiple(struct string_list *list)
 			return errcode;
 	}
 
-	argv_array_pushl(&argv, "fetch", "--append", NULL);
+	argv_array_pushl(&argv, "fetch", "--append", "--no-auto-gc", NULL);
 	add_options_to_argv(&argv);
 
 	for (i = 0; i < list->nr; i++) {
@@ -1674,11 +1677,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 	close_all_packs(the_repository->objects);
 
-	argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
-	if (verbosity < 0)
-		argv_array_push(&argv_gc_auto, "--quiet");
-	run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
-	argv_array_clear(&argv_gc_auto);
+	if (enable_auto_gc) {
+		argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
+		if (verbosity < 0)
+			argv_array_push(&argv_gc_auto, "--quiet");
+		run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
+		argv_array_clear(&argv_gc_auto);
+	}
 
 	return result;
 }
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 0030c92e1a..5426d4b5ab 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -105,9 +105,12 @@ test_expect_success 'git fetch --multiple (two remotes)' '
 	 git remote rm origin &&
 	 git remote add one ../one &&
 	 git remote add two ../two &&
-	 git fetch --multiple one two &&
+	 GIT_TRACE=1 git fetch --multiple one two 2>trace &&
 	 git branch -r > output &&
-	 test_cmp ../expect output)
+	 test_cmp ../expect output &&
+	 grep "built-in: git gc" trace >gc &&
+	 test_line_count = 1 gc
+	)
 '
 
 test_expect_success 'git fetch --multiple (bad remote names)' '
-- 
2.22.0.rc0.322.g2b0371e29a

[RFC/PATCH] gc: run more pre-detach operations under lock

[Ævar Arnfjörð Bjarmason]

Continue the work started in c45af94dbc ("gc: run pre-detach
operations under lock", 2017-07-11).

Now we'll take the lock before we print out "Auto packing the
repository[...]", and we'll optimistically tolerate a locking failure
at that point instead of dying.

This (mostly) solves two issues:

 1) Fetching in parallel from multiple remotes (or other
    parallel/concurrent "gc --auto") would, when the need_to_gc()
    heuristic fired, emit a bunch of "Auto packing the repository in
    background" messages. See [1] for a description of a "pfetch"
    alias that can trigger such an issue. Now we'll (usually) only
    print one of these messages.

 2) Because we'd call hold_lock_file_for_update() with
    "LOCK_DIE_ON_ERROR" such concurrent "gc" would error out with
    "Another git process seems to be running[...]".

    Now we'll (usually) avoid that by saying that "gc --auto" locking
    isn't so important. We tolerate a lock failure on "gc --auto" with
    the assumption that a "gc --auto" runs frequently enough that any
    one such invocation can fail, whereas without "--auto" we'll error
    on failure to acquire the lock

Why "mostly" and "usually"? There's still unaddressed caveats with
these two, respectively:

 1) The gc.pid lock is only held while the "git gc" runs. Thus a
    "pfetch" operation as described in [1] might start N jobs, one of
    which runs "gc", finishes, and then "gc --auto" finds it needs to
    run again in the context of a single (from the user's perspective)
    git command.

    I don't think that's worth dealing with. The cases where we'll
    find we need to take action "gc --auto" right after it's finished
    are rare (see e.g. [2]).

 2) There's still the unresolved race condition noted in c45af94dbc
    where we "hand off" the lock to the child process we fork under
    gc.autoDetach=true. It means we might start another "gc --auto" if
    we're so unlucky as to make the check and get the lock in the time
    the "real" earlier "gc --auto" parent/child process takes to
    "unlock() && fork() && lock()".

    Fixing that is outside the scope of this change. It's fixable by
    refactoring the lockfile.c code and daemonize() so that we'd e.g.:

      1. parent: lock() in parent, write PID to gc.pid
      2. parent: fork() the child process
      3. child: spin for X amount of time waiting until gc.pid lists
         our PID, not our parent's
      4. parent: amend the gc.pid lock to
         s/PARENT_PID/CHILD_PID/ (in-place rename(gc.pid.new,
         gc.pid)), fsync(gc.pid) and exit()
      5. child: notices updated gc.pid, proceeds with its gc. Does
         delete_tempfile(gc.pid) before exit()
      6. parent: Once lock is handed over (child trusted to poll the
         lock) exit() without delete_tempfile(gc.pid)

    Using some cross-process lock handover like that we could
    guarantee that there's never a point at which an outside process
    could get the gc.pid lock, that the PID written in the file always
    referred to a live process, and with #3 reasonably
    guarantee (aside from pathological scheduling shenanigans) that we
    don't blindly hand the PID off to a process that's already
    finished with its gc.

Testing for this new behavior is hard.

1. https://public-inbox.org/git/20170712200054.mxcabiyttijpbkbb@sigill.intra.peff.net/ (should
   be https://public-inbox.org/git/87bmopzbqx.fsf@gmail.com/ but vger
   seems to not have gotten a copy)
2. https://public-inbox.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/

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

This patch is part of a WIP branch I have that's a bit of a mess. It's
more-gc-detach-under-lock on github.com/avar/git.git. It doesn't apply
on master because it relies on some previous test work, but for RFC
purposes I figured it was better to send it stand-alone.

But I think this sort of approach is better than Duy's proposed patch,
because...

On Wed, Jun 19 2019, Nguyễn Thái Ngọc Duy wrote:

> So let's try to avoid that. We should only need one 'gc' run after all
> objects and references are added anyway. Add a new option --no-auto-gc
> that will be used by those n-1 processes. 'gc --auto' will always run on
> the main fetch process (*).
>
> (*) even if we fetch remotes in parallel at some point in future, this
>     should still be fine because we should "join" all those processes
>     before this step.

This is what I'm trying to fix in my version of this patch. This is
only true for yours if you assume that the user is going to be
invoking "fetch" in a single terminal window, IOW that we have an
implicit global mutex of one top-level git command at a time.

Wheras mine fixes e.g. the same issue for:

    parallel 'git fetch {}' ::: $(git remote)

Ditto for you running a "git" command and your editor running a
"fetch" at the same time.

A similar "one terminal" assumption was made when changing the
auto-detach behavior in your 62aad1849f ("gc --auto: do not lock refs
in the background", 2014-05-25).

To be clear, I think that (and your patch here) would (mostly) be an
improvement. I just wonder if we can do better.

I got stuck with this WIP series of mine because while it's mostly
ready sans the 'Why "mostly" and "usually"' caveats mentioned in my
commit message, I wondered if we couldn't do much better by:

 a) Do some version of reverting your 62aad1849f, this would entirely
    get rid of this need for handing off a lock to a child (noted
    above), but as-is would have e.g. "commit" run into a *.lock.

    Is that mitigated by 4ff0f01cb7 ("refs: retry acquiring reference
    locks for 100ms", 2017-08-21)? Or could we have "gc" run in some
    "lock-less" mode where it does all the work of expiring a
    ref/reflog at a given OID, and if it changed just tries again
    without needing to *.lock it except for the brief period of
    changing the already prepared file?

 b) If we couldn't do "a" for whatever reason implement some "locking
    priority" where a not-gc command could create a file (which "gc"
    would poll) saying "I want it for real work, release it!", or
    alternatively "gc" would create "*.locked-by-gc" in addition to
    "*.lock" files, and if the "*.locked-by-gc" was seen when the
    100ms retry would kick in, we'd make that say 10 seconds instead
    of 100 ms.

 builtin/gc.c  | 19 ++++++++++++-------
 lockfile.c    |  2 ++
 lockfile.h    |  8 +++++++-
 t/t6500-gc.sh |  8 ++++----
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index d12316fa48..2cd5803e86 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -391,7 +391,7 @@ static int need_to_gc(void)
 }
 
 /* return NULL on success, else hostname running the gc */
-static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
+static const char *lock_repo_for_gc(int force, pid_t* ret_pid, int die_on_error)
 {
 	struct lock_file lock = LOCK_INIT;
 	char my_host[HOST_NAME_MAX + 1];
@@ -401,13 +401,17 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	FILE *fp;
 	int fd;
 	char *pidfile_path;
+	int flags = die_on_error ? LOCK_DIE_ON_ERROR : LOCK_QUIET_ON_ERROR;
 
 	if (xgethostname(my_host, sizeof(my_host)))
 		xsnprintf(my_host, sizeof(my_host), "unknown");
 
 	pidfile_path = git_pathdup("gc.pid");
-	fd = hold_lock_file_for_update(&lock, pidfile_path,
-				       LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&lock, pidfile_path, flags);
+	if (fd < 0) {
+		assert(!die_on_error); /* should die in unable_to_lock_die() */
+		return pidfile_path;
+	}
 	if (!force) {
 		static char locking_host[HOST_NAME_MAX + 1];
 		static char *scan_fmt;
@@ -533,7 +537,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int auto_gc = 0;
 	int quiet = -1;
 	int force = 0;
-	const char *name;
+	const char *name = NULL;
 	pid_t pid;
 	int daemonized = 0;
 	int keep_base_pack = -1;
@@ -599,6 +603,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		 */
 		if (!need_to_gc())
 			return 0;
+		if (lock_repo_for_gc(force, &pid, 0))
+			return 0;
 		if (!quiet) {
 			if (detach_auto)
 				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
@@ -616,8 +622,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 				/* Last gc --auto failed. Skip this one. */
 				return 0;
 
-			if (lock_repo_for_gc(force, &pid))
-				return 0;
 			gc_before_repack(); /* dies on failure */
 			git_test_sleep("GIT_TEST_GC_SLEEP_PRE_FORK");
 			delete_tempfile(&pidfile);
@@ -644,7 +648,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		string_list_clear(&keep_pack, 0);
 	}
 
-	name = lock_repo_for_gc(force, &pid);
+	if (!pidfile)
+		name = lock_repo_for_gc(force, &pid, 1);
 	if (name) {
 		if (auto_gc)
 			return 0; /* be quiet on --auto */
diff --git a/lockfile.c b/lockfile.c
index 8e8ab4f29f..2a86d3a656 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -173,6 +173,8 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
 				      int flags, long timeout_ms)
 {
 	int fd = lock_file_timeout(lk, path, flags, timeout_ms);
+	if (flags & LOCK_QUIET_ON_ERROR)
+		return fd;
 	if (fd < 0) {
 		if (flags & LOCK_DIE_ON_ERROR)
 			unable_to_lock_die(path, errno);
diff --git a/lockfile.h b/lockfile.h
index 9843053ce8..dc366d70cc 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -135,10 +135,16 @@ struct lock_file {
 
 /*
  * ... this flag can be passed instead to return -1 and give the usual
- * error message upon an error.
+ * error message upon an error, or ...
  */
 #define LOCK_REPORT_ON_ERROR 4
 
+/*
+ * ... just be quiet and let the caller handle it by checking the
+ * return return value.
+ */
+#define LOCK_QUIET_ON_ERROR 8
+
 /*
  * Usually symbolic links in the destination path are resolved. This
  * means that (1) the lockfile is created by adding ".lock" to the
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 363ca18980..4dea98f1c3 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -175,14 +175,14 @@ test_racy_gc_auto () {
 	"
 }
 
-test_racy_gc_auto failure false N/A N/A N/A
+test_racy_gc_auto success false N/A N/A N/A
 for fork_works in true false
 do
 	for sleep_before_fork in 0 1
 	do
 		for sleep_before_fork_no_lock in 0 1
 		do
-			test_racy_gc_auto failure true $sleep_before_fork $sleep_before_fork_no_lock $fork_works
+			test_racy_gc_auto success true $sleep_before_fork $sleep_before_fork_no_lock $fork_works
 		done
 	done
 done
@@ -204,8 +204,8 @@ test_racy_faked_gc_auto () {
 		test_line_count = 0 errors
 	"
 }
-test_racy_faked_gc_auto failure true
-test_racy_faked_gc_auto failure false
+test_racy_faked_gc_auto success true
+test_racy_faked_gc_auto success false
 
 run_and_wait_for_auto_gc () {
 	# We read stdout from gc for the side effect of waiting until the
-- 
2.22.0.rc1.257.g3120a18244

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Duy Nguyen]

On Wed, Jun 19, 2019 at 5:26 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> This patch is part of a WIP branch I have that's a bit of a mess. It's
> more-gc-detach-under-lock on github.com/avar/git.git. It doesn't apply
> on master because it relies on some previous test work, but for RFC
> purposes I figured it was better to send it stand-alone.
>
> But I think this sort of approach is better than Duy's proposed patch,
> because...
>
> On Wed, Jun 19 2019, Nguyễn Thái Ngọc Duy wrote:
>
> > So let's try to avoid that. We should only need one 'gc' run after all
> > objects and references are added anyway. Add a new option --no-auto-gc
> > that will be used by those n-1 processes. 'gc --auto' will always run on
> > the main fetch process (*).
> >
> > (*) even if we fetch remotes in parallel at some point in future, this
> >     should still be fine because we should "join" all those processes
> >     before this step.
>
> This is what I'm trying to fix in my version of this patch. This is
> only true for yours if you assume that the user is going to be
> invoking "fetch" in a single terminal window, IOW that we have an
> implicit global mutex of one top-level git command at a time.
>
> Wheras mine fixes e.g. the same issue for:
>
>     parallel 'git fetch {}' ::: $(git remote)
>
> Ditto for you running a "git" command and your editor running a
> "fetch" at the same time.

You could sort of avoid the problem here too with

parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
git gc --auto

It's definitely simpler, but of course we have to manually add
--no-auto-gc in everywhere we need, so not quite as elegant.

Actually you could already do that with 'git -c gc.auto=false fetch', I guess.
-- 
Duy

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 19 2019, Duy Nguyen wrote:

> On Wed, Jun 19, 2019 at 5:26 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> This patch is part of a WIP branch I have that's a bit of a mess. It's
>> more-gc-detach-under-lock on github.com/avar/git.git. It doesn't apply
>> on master because it relies on some previous test work, but for RFC
>> purposes I figured it was better to send it stand-alone.
>>
>> But I think this sort of approach is better than Duy's proposed patch,
>> because...
>>
>> On Wed, Jun 19 2019, Nguyễn Thái Ngọc Duy wrote:
>>
>> > So let's try to avoid that. We should only need one 'gc' run after all
>> > objects and references are added anyway. Add a new option --no-auto-gc
>> > that will be used by those n-1 processes. 'gc --auto' will always run on
>> > the main fetch process (*).
>> >
>> > (*) even if we fetch remotes in parallel at some point in future, this
>> >     should still be fine because we should "join" all those processes
>> >     before this step.
>>
>> This is what I'm trying to fix in my version of this patch. This is
>> only true for yours if you assume that the user is going to be
>> invoking "fetch" in a single terminal window, IOW that we have an
>> implicit global mutex of one top-level git command at a time.
>>
>> Wheras mine fixes e.g. the same issue for:
>>
>>     parallel 'git fetch {}' ::: $(git remote)
>>
>> Ditto for you running a "git" command and your editor running a
>> "fetch" at the same time.
>
> You could sort of avoid the problem here too with
>
> parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
> git gc --auto
>
> It's definitely simpler, but of course we have to manually add
> --no-auto-gc in everywhere we need, so not quite as elegant.
>
> Actually you could already do that with 'git -c gc.auto=false fetch', I guess.

The point of the 'parallel' example is to show disconnected git
commands, think trying to run 'git' in a terminal while your editor
asynchronously runs a polling 'fetch', or a server with multiple
concurrent clients running 'gc --auto'.

That's the question my RFC patch raises. As far as I can tell the
approach in your patch is only needed because our locking for gc is
buggy, rather than introduce the caveat that an fetch(N) operation won't
do "gc" until it's finished (we may have hundreds, thousands of remotes,
I use that for some more obscure use-cases) shouldn't we just fix the
locking?

Re: [PATCH] fetch: only run 'gc' once when fetching multiple remotes

[Jeff King]

On Wed, Jun 19, 2019 at 04:46:30PM +0700, Nguyễn Thái Ngọc Duy wrote:

> In multiple remotes mode, git-fetch is launched for n-1 remotes and the
> last remote is handled by the current process. Each of these processes
> will in turn run 'gc' at the end.
> 
> This is not really a problem because even if multiple 'gc --auto' is run
> at the same time we still handle it correctly. It does show multiple
> "auto packing in the background" messages though. And we may waste some
> resources when gc actually runs because we still do some stuff before
> checking the lock and moving it to background.
> 
> So let's try to avoid that. We should only need one 'gc' run after all
> objects and references are added anyway. Add a new option --no-auto-gc
> that will be used by those n-1 processes. 'gc --auto' will always run on
> the main fetch process (*).

Yeah, that makes sense.

I was surprised that we needed a new command-line option here, but I
guess the sub-fetch processes really have no idea that they're
subservient to a multi-remote fetch (they do get "--append", but of
course somebody could specify that independently).

Another option would be to just pass "-c gc.auto=0" to the child
processes to inhibit auto-gc. But maybe it makes sense to have a nicer
interface (after all, somebody else could be doing the same "let's do a
bunch of fetches in a row" without using the multi-fetch code).

Though there I kind of wonder if this would apply to other scripted
uses, too. E.g., if I'm doing a bunch of commits, I might want to
inhibit auto-gc and then run it myself at the end. Should we support
"GIT_AUTO_GC=0" in the environment (and a matching "git --no-auto-gc
..." option that could be used here)?

>  Documentation/fetch-options.txt |  4 ++++
>  builtin/fetch.c                 | 17 +++++++++++------
>  t/t5514-fetch-multiple.sh       |  7 +++++--
>  3 files changed, 20 insertions(+), 8 deletions(-)

My musings above aside, the patch looks correct to me.

-Peff

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Jeff King]

On Wed, Jun 19, 2019 at 07:51:00PM +0700, Duy Nguyen wrote:

> > Wheras mine fixes e.g. the same issue for:
> >
> >     parallel 'git fetch {}' ::: $(git remote)
> >
> > Ditto for you running a "git" command and your editor running a
> > "fetch" at the same time.
> 
> You could sort of avoid the problem here too with
> 
> parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
> git gc --auto
> 
> It's definitely simpler, but of course we have to manually add
> --no-auto-gc in everywhere we need, so not quite as elegant.

This has the added advantage that the gc is deterministically run only
once after all of the fetches. Whereas any locking scheme is going to
run it randomly for at least _one_ of the fetches, but there may be
other fetches afterwards.

In a sense it might not matter, because any fetches which complete after
the auto-gc finishes would either trigger a new auto-gc or not. And if
not, then one could argue that it wasn't necessary.

But as a general rule, the cost of gc scales with the repo size, not
with the number of unpacked objects. So it's more efficient to stick as
many updates as you can into a single gc; the cost is running a gc at
all, not the incremental cost of including the new fetches. Or put
another way, by leaving some fetches from this round of commands out of
the gc, we will require another expensive gc sooner.

> Actually you could already do that with 'git -c gc.auto=false fetch', I guess.

Yeah. I wrote my other response before reading this part of the thread,
but IMHO Ævar's example argues even more for "git --no-auto-gc".

-Peff

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Jeff King]

On Wed, Jun 19, 2019 at 08:01:55PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > You could sort of avoid the problem here too with
> >
> > parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
> > git gc --auto
> >
> > It's definitely simpler, but of course we have to manually add
> > --no-auto-gc in everywhere we need, so not quite as elegant.
> >
> > Actually you could already do that with 'git -c gc.auto=false fetch', I guess.
> 
> The point of the 'parallel' example is to show disconnected git
> commands, think trying to run 'git' in a terminal while your editor
> asynchronously runs a polling 'fetch', or a server with multiple
> concurrent clients running 'gc --auto'.
> 
> That's the question my RFC patch raises. As far as I can tell the
> approach in your patch is only needed because our locking for gc is
> buggy, rather than introduce the caveat that an fetch(N) operation won't
> do "gc" until it's finished (we may have hundreds, thousands of remotes,
> I use that for some more obscure use-cases) shouldn't we just fix the
> locking?

I think there may be room for both approaches. Yours fixes the repeated
message in the more general case, but Duy's suggestion is the most
efficient thing.

I agree that the "thousands of remotes" case means we might want to gc
in the interim. But we probably ought to do that deterministically
rather than hoping that the pattern of lock contention makes sense.

-Peff

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 19 2019, Jeff King wrote:

> On Wed, Jun 19, 2019 at 08:01:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > You could sort of avoid the problem here too with
>> >
>> > parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
>> > git gc --auto
>> >
>> > It's definitely simpler, but of course we have to manually add
>> > --no-auto-gc in everywhere we need, so not quite as elegant.
>> >
>> > Actually you could already do that with 'git -c gc.auto=false fetch', I guess.
>>
>> The point of the 'parallel' example is to show disconnected git
>> commands, think trying to run 'git' in a terminal while your editor
>> asynchronously runs a polling 'fetch', or a server with multiple
>> concurrent clients running 'gc --auto'.
>>
>> That's the question my RFC patch raises. As far as I can tell the
>> approach in your patch is only needed because our locking for gc is
>> buggy, rather than introduce the caveat that an fetch(N) operation won't
>> do "gc" until it's finished (we may have hundreds, thousands of remotes,
>> I use that for some more obscure use-cases) shouldn't we just fix the
>> locking?
>
> I think there may be room for both approaches. Yours fixes the repeated
> message in the more general case, but Duy's suggestion is the most
> efficient thing.
>
> I agree that the "thousands of remotes" case means we might want to gc
> in the interim. But we probably ought to do that deterministically
> rather than hoping that the pattern of lock contention makes sense.

We do it deterministically, when gc.auto thresholds et al are exceeded
we kick one off without waiting for other stuff, if we can get the lock.

I don't think this desire to just wait a bit until all the fetches are
complete makes sense as a special-case.

If, as you noted in <20190619190845.GD28145@sigill.intra.peff.net>, the
desire is to reduce GC CPU use then you're better off just tweaking the
limits upwards. Then you get that with everything, like when you run
"commit" in a for-loop, not just this one special case of "fetch".

We have existing potentially long-running operations like "fetch",
"rebase" and "git svn fetch" that run "gc --auto" for their incremental
steps, and that's a feature.

It keeps "gc --auto" dumb enough to avoid a pathological case where
we'll have a ballooning objects dir because we figure we can run
something "at the end", when "the end" could be hours away, and we're
adding a new pack or hundreds of loose objects every second.

So I don't think Duy's patch is a good way to go.

The rationale in its commit message for including it can be better
addressed by something like my WIP for just fixing the locking
mechanism, since it fixes the stated problems of multiple "auto packing
in the background" messages and the "we may waste some resources" (we
take the lock earlier before doing 'real' work) without introducing its
own pathological case of deferring "gc --auto" too much as we have
unchecked object growth.

I think that's really important. It's OK if "gc --auto" isn't optimal,
but we should really avoid such pathological cases.

It's also important that it's easy to explain, i.e. if this patch goes
in I think it should update the second paragraph of the git-gc.txt
docs. I.e. now it's not just:

    When common porcelain operations that create objects are run, they
    will check whether the repository has grown substantially since the
    last maintenance[...]

But also something like:

    Except in cases where we're running porcelain commads that
    themselves might re-run aggregates of themselves, in which case we
    defer "gc" until the end. This currently only applies to "fetch",
    but other commands such as "rebase" etc. might learn to do this in
    the future. Note that if the sub-commands are numerous enough this
    might itself become pathological as "gc --auto" is deferred too
    much, so use option/config XYZ to ...

Or whatever...

[PATCH 0/6] Change <non-empty?> GIT_TEST_* variables to <boolean>

[Ævar Arnfjörð Bjarmason]

This changes the remaining <non-empty?> special snowflake test modes
to <boolean> and gets rid of test_tristate() in favor of the now
standard "boolea" test.

I'm replying to my "gc: run more pre-detach operations under lock"
thread because one of the things my WIP patches to make gc locking
less sucky depends on is new GIT_TEST_GC_* test modes to test its
racyness, which in turn depends on these cleanups.

Ævar Arnfjörð Bjarmason (6):
  env--helper: new undocumented builtin wrapping git_env_*()
  t6040 test: stop using global "script" variable
  tests: make GIT_TEST_GETTEXT_POISON a boolean
  tests README: re-flow a previously changed paragraph
  tests: replace test_tristate with "git env--helper"
  tests: make GIT_TEST_FAIL_PREREQS a boolean

 .gitignore                |  1 +
 Makefile                  |  1 +
 builtin.h                 |  1 +
 builtin/env--helper.c     | 74 +++++++++++++++++++++++++++++++++++++++
 ci/lib.sh                 |  2 +-
 gettext.c                 |  6 ++--
 git-sh-i18n.sh            |  4 ++-
 git.c                     |  1 +
 po/README                 |  2 +-
 t/README                  | 12 +++----
 t/lib-git-daemon.sh       |  7 ++--
 t/lib-git-svn.sh          | 11 +++---
 t/lib-httpd.sh            | 15 ++++----
 t/t0000-basic.sh          | 10 +++---
 t/t0016-env-helper.sh     | 70 ++++++++++++++++++++++++++++++++++++
 t/t0205-gettext-poison.sh |  2 +-
 t/t5512-ls-remote.sh      |  3 +-
 t/t6040-tracking-info.sh  |  6 ++--
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib-functions.sh   | 58 +++++-------------------------
 t/test-lib.sh             | 29 ++++++++++++---
 22 files changed, 220 insertions(+), 99 deletions(-)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0016-env-helper.sh

-- 
2.22.0.rc1.257.g3120a18244

[PATCH 1/6] env--helper: new undocumented builtin wrapping git_env_*()

[Ævar Arnfjörð Bjarmason]

We have many GIT_TEST_* variables that accept a <boolean> because
they're implemented in C, and then some that take <non-empty?> because
they're implemented at least partially in shellscript.

Add a helper that wraps git_env_bool() and git_env_ulong() as the
first step in fixing this. This isn't being added as a test-tool mode
because some of these are used outside the test suite.

Part of what this tool does can be done via a trick with "git config"
added in 83d842dc8c ("tests: turn on network daemon tests by default",
2014-02-10) for test_tristate(), i.e.:

    git -c magic.variable="$1" config --bool magic.variable 2>/dev/null

But as subsequent changes will show being able to pass along the
default value makes all the difference, and we'll be able to replace
test_tristate() itself with that.

The --mode-bool option will be used by subsequent patches, but not
--mode-ulong. I figured it was easy enough to add it & test for it so
I left it in so we'd have wrappers for both git_env_*() functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 builtin.h             |  1 +
 builtin/env--helper.c | 74 +++++++++++++++++++++++++++++++++++++++++++
 git.c                 |  1 +
 t/t0016-env-helper.sh | 70 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 148 insertions(+)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0016-env-helper.sh

diff --git a/.gitignore b/.gitignore
index 4470d7cfc0..1f7a83fb3c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -58,6 +58,7 @@
 /git-difftool
 /git-difftool--helper
 /git-describe
+/git-env--helper
 /git-fast-export
 /git-fast-import
 /git-fetch
diff --git a/Makefile b/Makefile
index f58bf14c7b..f2cfc8d812 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
 BUILTIN_OBJS += builtin/difftool.o
+BUILTIN_OBJS += builtin/env--helper.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index ec7e0954c4..93bd49fe4f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -160,6 +160,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix);
 int cmd_diff(int argc, const char **argv, const char *prefix);
 int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 int cmd_difftool(int argc, const char **argv, const char *prefix);
+int cmd_env__helper(int argc, const char **argv, const char *prefix);
 int cmd_fast_export(int argc, const char **argv, const char *prefix);
 int cmd_fetch(int argc, const char **argv, const char *prefix);
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/env--helper.c b/builtin/env--helper.c
new file mode 100644
index 0000000000..2bb65ecf3f
--- /dev/null
+++ b/builtin/env--helper.c
@@ -0,0 +1,74 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+
+static char const * const env__helper_usage[] = {
+	N_("git env--helper [--mode-bool | --mode-ulong] --env-variable=<VAR> --env-default=<DEF> [<options>]"),
+	NULL
+};
+
+int cmd_env__helper(int argc, const char **argv, const char *prefix)
+{
+	enum {
+		ENV_HELPER_BOOL = 1,
+		ENV_HELPER_ULONG,
+	} cmdmode = 0;
+	int exit_code = 0;
+	int quiet = 0;
+	const char *env_variable = NULL;
+	const char *env_default = NULL;
+	int ret;
+	int ret_int, tmp_int;
+	unsigned long ret_ulong, tmp_ulong;
+	struct option opts[] = {
+		OPT_CMDMODE(0, "mode-bool", &cmdmode,
+			    N_("invoke git_env_bool(...)"), ENV_HELPER_BOOL),
+		OPT_CMDMODE(0, "mode-ulong", &cmdmode,
+			    N_("invoke git_env_ulong(...)"), ENV_HELPER_ULONG),
+		OPT_STRING(0, "variable", &env_variable, N_("name"),
+			   N_("which environment variable to ask git_env_*(...) about")),
+		OPT_STRING(0, "default", &env_default, N_("value"),
+			   N_("what default value does git_env_*(...) fall back on?")),
+		OPT_BOOL(0, "exit-code", &exit_code,
+			 N_("exit code determined by truth of the git_env_*() function")),
+		OPT_BOOL(0, "quiet", &quiet,
+			 N_("don't print the git_env_*() return value")),
+		OPT_END(),
+	};
+
+	if (parse_options(argc, argv, prefix, opts, env__helper_usage, 0))
+		usage_with_options(env__helper_usage, opts);
+	if (!env_variable || !env_default ||
+	    !*env_variable || !*env_default)
+		usage_with_options(env__helper_usage, opts);
+
+	switch (cmdmode) {
+	case ENV_HELPER_BOOL:
+		tmp_int = strtol(env_default, (char **)&env_default, 10);
+		if (*env_default) {
+			error(_("option `--default' expects a numerical value with `--mode-bool`"));
+			usage_with_options(env__helper_usage, opts);
+		}
+		ret_int = git_env_bool(env_variable, tmp_int);
+		if (!quiet)
+			printf("%d\n", ret_int);
+		ret = ret_int;
+		break;
+	case ENV_HELPER_ULONG:
+		tmp_ulong = strtoll(env_default, (char **)&env_default, 10);
+		if (*env_default) {
+			error(_("option `--default' expects a numerical value with `--mode-ulong`"));
+			usage_with_options(env__helper_usage, opts);
+		}
+		ret_ulong = git_env_ulong(env_variable, tmp_ulong);
+		if (!quiet)
+			printf("%lu\n", ret_ulong);
+		ret = ret_ulong;
+		break;
+	}
+
+	if (exit_code)
+		return !ret;
+
+	return 0;
+}
diff --git a/git.c b/git.c
index c2eec470c9..a43e1dd98e 100644
--- a/git.c
+++ b/git.c
@@ -500,6 +500,7 @@ static struct cmd_struct commands[] = {
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
+	{ "env--helper", cmd_env__helper },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
diff --git a/t/t0016-env-helper.sh b/t/t0016-env-helper.sh
new file mode 100755
index 0000000000..4dc4ab35e5
--- /dev/null
+++ b/t/t0016-env-helper.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='test env--helper'
+
+. ./test-lib.sh
+
+
+test_expect_success 'env--helper usage' '
+	test_must_fail git env--helper &&
+	test_must_fail git env--helper --mode-bool &&
+	test_must_fail git env--helper --mode-ulong &&
+	test_must_fail git env--helper --mode-bool --variable &&
+	test_must_fail git env--helper --mode-bool --variable --default &&
+	test_must_fail git env--helper --mode-bool --variable= --default=
+'
+
+test_expect_success 'env--helper bad default values' '
+	test_must_fail git env--helper --mode-bool --variable=MISSING --default=1xyz &&
+	test_must_fail git env--helper --mode-ulong --variable=MISSING --default=1xyz
+'
+
+test_expect_success 'env--helper --mode-bool' '
+	echo 1 >expected &&
+	git env--helper --mode-bool --variable=MISSING --default=1 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	echo 0 >expected &&
+	test_must_fail git env--helper --mode-bool --variable=MISSING --default=0 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	git env--helper --mode-bool --variable=MISSING --default=0 >actual &&
+	test_cmp expected actual &&
+
+	>expected &&
+	git env--helper --mode-bool --variable=MISSING --default=1 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	EXISTS=true git env--helper --mode-bool --variable=EXISTS --default=0 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	echo 1 >expected &&
+	EXISTS=true git env--helper --mode-bool --variable=EXISTS --default=0 --exit-code >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'env--helper --mode-ulong' '
+	echo 1234567890 >expected &&
+	git env--helper --mode-ulong --variable=MISSING --default=1234567890 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	echo 0 >expected &&
+	test_must_fail git env--helper --mode-ulong --variable=MISSING --default=0 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	git env--helper --mode-ulong --variable=MISSING --default=0 >actual &&
+	test_cmp expected actual &&
+
+	>expected &&
+	git env--helper --mode-ulong --variable=MISSING --default=1234567890 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	EXISTS=1234567890 git env--helper --mode-ulong --variable=EXISTS --default=0 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	echo 1234567890 >expected &&
+	EXISTS=1234567890 git env--helper --mode-ulong --variable=EXISTS --default=0 --exit-code >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
2.22.0.rc1.257.g3120a18244

[PATCH 2/6] t6040 test: stop using global "script" variable

[Ævar Arnfjörð Bjarmason]

Change test code added in c0234b2ef6 ("stat_tracking_info(): clear
object flags used during counting", 2008-07-03) to stop using the
"script" variable also used for lazy prerequisites in
test-lib-functions.sh.

Since this test uses test_i18ncmp and expects to use its own "script"
variable twice it implicitly depends on the C_LOCALE_OUTPUT
prerequisite not being a lazy prerequisite. A follow-up change will
make it a lazy prerequisite, so we must remove this landmine before
inadvertently stepping on it as we make that change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6040-tracking-info.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 716283b274..970b25a289 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -38,7 +38,7 @@ test_expect_success setup '
 	advance h
 '
 
-script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
+t6040_script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
 cat >expect <<\EOF
 b1 [ahead 1, behind 1] d
 b2 [ahead 1, behind 1] d
@@ -53,7 +53,7 @@ test_expect_success 'branch -v' '
 		cd test &&
 		git branch -v
 	) |
-	sed -n -e "$script" >actual &&
+	sed -n -e "$t6040_script" >actual &&
 	test_i18ncmp expect actual
 '
 
@@ -71,7 +71,7 @@ test_expect_success 'branch -vv' '
 		cd test &&
 		git branch -vv
 	) |
-	sed -n -e "$script" >actual &&
+	sed -n -e "$t6040_script" >actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.22.0.rc1.257.g3120a18244

[PATCH 3/6] tests: make GIT_TEST_GETTEXT_POISON a boolean

[Ævar Arnfjörð Bjarmason]

Change the GIT_TEST_GETTEXT_POISON variable from being "non-empty?" to
being a more standard boolean variable.

Since it needed to be checked in both C code and shellscript (via test
-n) it was one of the remaining shellscript-like variables. Now that
we have "git env--helper" we can change that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ci/lib.sh                 | 2 +-
 gettext.c                 | 6 ++----
 git-sh-i18n.sh            | 4 +++-
 po/README                 | 2 +-
 t/README                  | 4 ++--
 t/t0205-gettext-poison.sh | 2 +-
 t/t7201-co.sh             | 2 +-
 t/t9902-completion.sh     | 2 +-
 t/test-lib.sh             | 4 ++++
 9 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 288a5b3884..fd799ae663 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -184,7 +184,7 @@ osx-clang|osx-gcc)
 	export GIT_SKIP_TESTS="t9810 t9816"
 	;;
 GIT_TEST_GETTEXT_POISON)
-	export GIT_TEST_GETTEXT_POISON=YesPlease
+	export GIT_TEST_GETTEXT_POISON=true
 	;;
 esac
 
diff --git a/gettext.c b/gettext.c
index d4021d690c..5c71f4c8b9 100644
--- a/gettext.c
+++ b/gettext.c
@@ -50,10 +50,8 @@ const char *get_preferred_languages(void)
 int use_gettext_poison(void)
 {
 	static int poison_requested = -1;
-	if (poison_requested == -1) {
-		const char *v = getenv("GIT_TEST_GETTEXT_POISON");
-		poison_requested = v && strlen(v) ? 1 : 0;
-	}
+	if (poison_requested == -1)
+		poison_requested = git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
 	return poison_requested;
 }
 
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index e1d917fd27..de8ae67d7b 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,9 @@ export TEXTDOMAINDIR
 
 # First decide what scheme to use...
 GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_TEST_GETTEXT_POISON"
+if test -n "$GIT_TEST_GETTEXT_POISON" &&
+	    git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON \
+		--default=0 --exit-code --quiet
 then
 	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
 elif test -n "@@USE_GETTEXT_SCHEME@@"
diff --git a/po/README b/po/README
index aa704ffcb7..07595d369b 100644
--- a/po/README
+++ b/po/README
@@ -293,7 +293,7 @@ To smoke out issues like these, Git tested with a translation mode that
 emits gibberish on every call to gettext. To use it run the test suite
 with it, e.g.:
 
-    cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh
+    cd t && GIT_TEST_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh
 
 If tests break with it you should inspect them manually and see if
 what you're translating is sane, i.e. that you're not translating
diff --git a/t/README b/t/README
index 9747971d58..9a131f472e 100644
--- a/t/README
+++ b/t/README
@@ -343,8 +343,8 @@ whether this mode is active, and e.g. skip some tests that are hard to
 refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
-GIT_TEST_GETTEXT_POISON=<non-empty?> turns all strings marked for
-translation into gibberish if non-empty (think "test -n"). Used for
+GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
+translation into gibberish if true. Used for
 spotting those tests that need to be marked with a C_LOCALE_OUTPUT
 prerequisite when adding more strings for translation. See "Testing
 marked strings" in po/README for details.
diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index a06269f38a..1675d3e171 100755
--- a/t/t0205-gettext-poison.sh
+++ b/t/t0205-gettext-poison.sh
@@ -5,7 +5,7 @@
 
 test_description='Gettext Shell poison'
 
-GIT_TEST_GETTEXT_POISON=YesPlease
+GIT_TEST_GETTEXT_POISON=true
 export GIT_TEST_GETTEXT_POISON
 . ./lib-gettext.sh
 
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 5990299fc9..b696bae5f5 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -249,7 +249,7 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '
 test_expect_success 'checkout to detach HEAD' '
 	git config advice.detachedHead true &&
 	git checkout -f renamer && git clean -f &&
-	GIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&
+	GIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&
 	grep "HEAD is now at 7329388" messages &&
 	test_line_count -gt 1 messages &&
 	H=$(git rev-parse --verify HEAD) &&
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43cf313a1c..75512c3403 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1706,7 +1706,7 @@ test_expect_success 'sourcing the completion script clears cached commands' '
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
-	GIT_TEST_GETTEXT_POISON= &&
+	GIT_TEST_GETTEXT_POISON=false &&
 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
 	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4b346467df..bdd5017d24 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1443,6 +1443,10 @@ then
 	unset GIT_TEST_GETTEXT_POISON_ORIG
 fi
 
+test_lazy_prereq C_LOCALE_OUTPUT '
+	! git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet
+'
+
 # Can we rely on git's output in the C locale?
 if test -z "$GIT_TEST_GETTEXT_POISON"
 then
-- 
2.22.0.rc1.257.g3120a18244

[PATCH 4/6] tests README: re-flow a previously changed paragraph

[Ævar Arnfjörð Bjarmason]

A previous change to the "GIT_TEST_GETTEXT_POISON" variable left this
paragraph needing to be re-flowed. Let's do that in this separate
change to make it easy to see that there's no change here when viewed
with "--word-diff".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index 9a131f472e..072c9854d1 100644
--- a/t/README
+++ b/t/README
@@ -344,10 +344,10 @@ refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
 GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
-translation into gibberish if true. Used for
-spotting those tests that need to be marked with a C_LOCALE_OUTPUT
-prerequisite when adding more strings for translation. See "Testing
-marked strings" in po/README for details.
+translation into gibberish if true. Used for spotting those tests that
+need to be marked with a C_LOCALE_OUTPUT prerequisite when adding more
+strings for translation. See "Testing marked strings" in po/README for
+details.
 
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
-- 
2.22.0.rc1.257.g3120a18244

[PATCH 5/6] tests: replace test_tristate with "git env--helper"

[Ævar Arnfjörð Bjarmason]

The test_tristate helper introduced in 83d842dc8c ("tests: turn on
network daemon tests by default", 2014-02-10) can now be better
implemented with "git env--helper" to give the variables in question
the standard boolean behavior.

The reason for the "tristate" was to have all of false/true/auto,
where "auto" meant either "false" or "true" depending on what the
fallback was. With the --default option to "git env--helper" we can
simply have e.g. GIT_TEST_HTTPD where we know if it's true because the
user asked explicitly ("true"), or true implicitly ("auto").

This breaks backwards compatibility for explicitly setting "auto" for
these variables, but I don't think anyone cares. That was always
intended to be internal.

This means the test_normalize_bool() code in test-lib-functions.sh
goes away in addition to test_tristate(). We still need the
test_skip_or_die() helper, but now it takes the variable name instead
of the value, and uses "git env--bool" to distinguish a default "true"
from an explicit "true" (in those "explicit true" cases we want to
fail the test in question).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-git-daemon.sh     |  7 +++---
 t/lib-git-svn.sh        | 11 +++-----
 t/lib-httpd.sh          | 15 ++++++-----
 t/t5512-ls-remote.sh    |  3 +--
 t/test-lib-functions.sh | 56 ++++++-----------------------------------
 5 files changed, 22 insertions(+), 70 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 7b3407134e..770c5218ea 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -15,8 +15,7 @@
 #
 #	test_done
 
-test_tristate GIT_TEST_GIT_DAEMON
-if test "$GIT_TEST_GIT_DAEMON" = false
+if ! git env--helper --mode-bool --variable=GIT_TEST_GIT_DAEMON --default=1 --exit-code --quiet
 then
 	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
 	test_done
@@ -24,7 +23,7 @@ fi
 
 if test_have_prereq !PIPE
 then
-	test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
+	test_skip_or_die GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
 fi
 
 test_set_port LIB_GIT_DAEMON_PORT
@@ -73,7 +72,7 @@ start_git_daemon() {
 		kill "$GIT_DAEMON_PID"
 		wait "$GIT_DAEMON_PID"
 		unset GIT_DAEMON_PID
-		test_skip_or_die $GIT_TEST_GIT_DAEMON \
+		test_skip_or_die GIT_TEST_GIT_DAEMON \
 			"git daemon failed to start"
 	fi
 }
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index c1271d6863..853d33a57a 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -69,14 +69,12 @@ svn_cmd () {
 maybe_start_httpd () {
 	loc=${1-svn}
 
-	test_tristate GIT_SVN_TEST_HTTPD
-	case $GIT_SVN_TEST_HTTPD in
-	true)
+	if git env--helper --mode-bool --variable=GIT_TEST_HTTPD --default=0 --exit-code --quiet	
+	then
 		. "$TEST_DIRECTORY"/lib-httpd.sh
 		LIB_HTTPD_SVN="$loc"
 		start_httpd
-		;;
-	esac
+	fi
 }
 
 convert_to_rev_db () {
@@ -106,8 +104,7 @@ EOF
 }
 
 require_svnserve () {
-	test_tristate GIT_TEST_SVNSERVE
-	if ! test "$GIT_TEST_SVNSERVE" = true
+	if ! git env--helper --mode-bool --variable=GIT_TEST_SVNSERVE --default=0 --exit-code --quiet
 	then
 		skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to enable)'
 		test_done
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b3cc62bd36..eef3250552 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -41,15 +41,14 @@ then
 	test_done
 fi
 
-test_tristate GIT_TEST_HTTPD
-if test "$GIT_TEST_HTTPD" = false
+if ! git env--helper --mode-bool --variable=GIT_TEST_HTTPD --default=1 --exit-code --quiet
 then
 	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
 	test_done
 fi
 
 if ! test_have_prereq NOT_ROOT; then
-	test_skip_or_die $GIT_TEST_HTTPD \
+	test_skip_or_die GIT_TEST_HTTPD \
 		"Cannot run httpd tests as root"
 fi
 
@@ -95,7 +94,7 @@ GIT_TRACE=$GIT_TRACE; export GIT_TRACE
 
 if ! test -x "$LIB_HTTPD_PATH"
 then
-	test_skip_or_die $GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
+	test_skip_or_die GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
 fi
 
 HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \
@@ -107,19 +106,19 @@ then
 	then
 		if ! test $HTTPD_VERSION -ge 2
 		then
-			test_skip_or_die $GIT_TEST_HTTPD \
+			test_skip_or_die GIT_TEST_HTTPD \
 				"at least Apache version 2 is required"
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
-			test_skip_or_die $GIT_TEST_HTTPD \
+			test_skip_or_die GIT_TEST_HTTPD \
 				"Apache module directory not found"
 		fi
 
 		LIB_HTTPD_MODULE_PATH="$DEFAULT_HTTPD_MODULE_PATH"
 	fi
 else
-	test_skip_or_die $GIT_TEST_HTTPD \
+	test_skip_or_die GIT_TEST_HTTPD \
 		"Could not identify web server at '$LIB_HTTPD_PATH'"
 fi
 
@@ -184,7 +183,7 @@ start_httpd() {
 	if test $? -ne 0
 	then
 		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
-		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
+		test_skip_or_die GIT_TEST_HTTPD "web server setup failed"
 	fi
 }
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index e3c4a48c85..7161148280 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -267,8 +267,7 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 '
 
 test_lazy_prereq GIT_DAEMON '
-	test_tristate GIT_TEST_GIT_DAEMON &&
-	test "$GIT_TEST_GIT_DAEMON" != false
+	git env--helper --mode-bool --variable=GIT_TEST_GIT_DAEMON --default=1 --exit-code --quiet
 '
 
 # This test spawns a daemon, so run it only if the user would be OK with
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0367cec5fd..527508c350 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1035,62 +1035,20 @@ perl () {
 	command "$PERL_PATH" "$@" 2>&7
 } 7>&2 2>&4
 
-# Is the value one of the various ways to spell a boolean true/false?
-test_normalize_bool () {
-	git -c magic.variable="$1" config --bool magic.variable 2>/dev/null
-}
-
-# Given a variable $1, normalize the value of it to one of "true",
-# "false", or "auto" and store the result to it.
-#
-#     test_tristate GIT_TEST_HTTPD
-#
-# A variable set to an empty string is set to 'false'.
-# A variable set to 'false' or 'auto' keeps its value.
-# Anything else is set to 'true'.
-# An unset variable defaults to 'auto'.
-#
-# The last rule is to allow people to set the variable to an empty
-# string and export it to decline testing the particular feature
-# for versions both before and after this change.  We used to treat
-# both unset and empty variable as a signal for "do not test" and
-# took any non-empty string as "please test".
-
-test_tristate () {
-	if eval "test x\"\${$1+isset}\" = xisset"
-	then
-		# explicitly set
-		eval "
-			case \"\$$1\" in
-			'')	$1=false ;;
-			auto)	;;
-			*)	$1=\$(test_normalize_bool \$$1 || echo true) ;;
-			esac
-		"
-	else
-		eval "$1=auto"
-	fi
-}
-
 # Exit the test suite, either by skipping all remaining tests or by
-# exiting with an error. If "$1" is "auto", we then we assume we were
-# opportunistically trying to set up some tests and we skip. If it is
-# "true", then we report a failure.
+# exiting with an error. If our prerequisite variable $1 falls back
+# on a default assume we were opportunistically trying to set up some
+# tests and we skip. If it is explicitly "true", then we report a failure.
 #
 # The error/skip message should be given by $2.
 #
 test_skip_or_die () {
-	case "$1" in
-	auto)
+	if ! git env--helper --mode-bool --variable=$1 --default=0 --exit-code --quiet
+	then
 		skip_all=$2
 		test_done
-		;;
-	true)
-		error "$2"
-		;;
-	*)
-		error "BUG: test tristate is '$1' (real error: $2)"
-	esac
+	fi
+	error "$2"
 }
 
 # The following mingw_* functions obey POSIX shell syntax, but are actually
-- 
2.22.0.rc1.257.g3120a18244

[PATCH 6/6] tests: make GIT_TEST_FAIL_PREREQS a boolean

[Ævar Arnfjörð Bjarmason]

Change the GIT_TEST_FAIL_PREREQS variable from being "non-empty?" to
being a more standard boolean variable. I recently added the variable
in dfe1a17df9 ("tests: add a special setup where prerequisites fail",
2019-05-13), having to add another "non-empty?" special-case is what
prompted me to write the "git env--helper" utility being used here.

Converting this one is a bit tricky since we use it so early and
frequently in the guts of the test code itself, so let's set a
GIT_TEST_FAIL_PREREQS_INTERNAL which can be tested with the old "test
-n" for the purposes of the shell code, and change the user-exposed
and documented GIT_TEST_FAIL_PREREQS variable to a boolean.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README                |  2 +-
 t/t0000-basic.sh        | 10 +++++-----
 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 25 +++++++++++++++++++++----
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/t/README b/t/README
index 072c9854d1..60d5b77bcc 100644
--- a/t/README
+++ b/t/README
@@ -334,7 +334,7 @@ that cannot be easily covered by a few specific test cases. These
 could be enabled by running the test suite with correct GIT_TEST_
 environment set.
 
-GIT_TEST_FAIL_PREREQS<non-empty?> fails all prerequisites. This is
+GIT_TEST_FAIL_PREREQS=<boolean> fails all prerequisites. This is
 useful for discovering issues with the tests where say a later test
 implicitly depends on an optional earlier test.
 
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 31de7e90f3..e89438e619 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -726,7 +726,7 @@ donthaveit=yes
 test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be skipped' '
 	donthaveit=no
 '
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit != yesyes
 then
 	say "bug in test framework: prerequisite tags do not work reliably"
 	exit 1
@@ -747,7 +747,7 @@ donthaveiteither=yes
 test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '
 	donthaveiteither=no
 '
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit$donthaveiteither != yesyesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit$donthaveiteither != yesyesyes
 then
 	say "bug in test framework: multiple prerequisite tags do not work reliably"
 	exit 1
@@ -763,7 +763,7 @@ test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
 	donthavetrue=no
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a "$havetrue$donthavetrue" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a "$havetrue$donthavetrue" != yesyes
 then
 	say 'bug in test framework: lazy prerequisites do not work'
 	exit 1
@@ -779,7 +779,7 @@ test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
 	havefalse=no
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a "$nothavefalse$havefalse" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a "$nothavefalse$havefalse" != yesyes
 then
 	say 'bug in test framework: negative lazy prerequisites do not work'
 	exit 1
@@ -790,7 +790,7 @@ test_expect_success 'tests clean up after themselves' '
 	test_when_finished clean=yes
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $clean != yes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $clean != yes
 then
 	say "bug in test framework: basic cleanup command does not work reliably"
 	exit 1
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 527508c350..3fba71c358 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -309,7 +309,7 @@ test_unset_prereq () {
 }
 
 test_set_prereq () {
-	if test -n "$GIT_TEST_FAIL_PREREQS"
+	if test -n "$GIT_TEST_FAIL_PREREQS_INTERNAL"
 	then
 		case "$1" in
 		# The "!" case is handled below with
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd5017d24..552461435e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1389,6 +1389,27 @@ yes () {
 	done
 }
 
+# The GIT_TEST_FAIL_PREREQS code hooks into test_set_prereq(), and
+# thus needs to be set up really early, and set an internal variable
+# for convenience so the hot test_set_prereq() codepath doesn't need
+# to call "git env--helper". Only do that work if needed by seeing if
+# GIT_TEST_FAIL_PREREQS is set at all.
+GIT_TEST_FAIL_PREREQS_INTERNAL=
+if test -n "$GIT_TEST_FAIL_PREREQS"
+then
+	if git env--helper --mode-bool --variable=GIT_TEST_FAIL_PREREQS \
+		--default=0 --exit-code --quiet
+	then
+		GIT_TEST_FAIL_PREREQS_INTERNAL=true
+		test_set_prereq FAIL_PREREQS
+	fi
+else
+	test_lazy_prereq FAIL_PREREQS '
+		git env--helper --mode-bool --variable=GIT_TEST_FAIL_PREREQS \
+			--default=0 --exit-code --quiet
+	'
+fi
+
 # Fix some commands on Windows
 uname_s=$(uname -s)
 case $uname_s in
@@ -1611,7 +1632,3 @@ test_lazy_prereq SHA1 '
 test_lazy_prereq REBASE_P '
 	test -z "$GIT_TEST_SKIP_REBASE_P"
 '
-
-test_lazy_prereq FAIL_PREREQS '
-	test -n "$GIT_TEST_FAIL_PREREQS"
-'
-- 
2.22.0.rc1.257.g3120a18244

Re: [PATCH] fetch: only run 'gc' once when fetching multiple remotes

[Duy Nguyen]

On Thu, Jun 20, 2019 at 1:59 AM Jeff King <peff@peff.net> wrote:
> I was surprised that we needed a new command-line option here, but I
> guess the sub-fetch processes really have no idea that they're
> subservient to a multi-remote fetch (they do get "--append", but of
> course somebody could specify that independently).
>
> Another option would be to just pass "-c gc.auto=0" to the child
> processes to inhibit auto-gc. But maybe it makes sense to have a nicer
> interface (after all, somebody else could be doing the same "let's do a
> bunch of fetches in a row" without using the multi-fetch code).

Nah to me -c is much nicer (and flexible too). The only thing I'm not
sure about is whether a user could override it. If fetch.c adds -c
gc.auto=0 automatically, and the user wants auto gc back, will "git -c
gc.auto=non-zero fetch --multiple" still work?

I haven't checked git_config_push_parameter() carefully, but I have an
impression that the parameter order there is "wrong", at least in this
case.

> Though there I kind of wonder if this would apply to other scripted
> uses, too. E.g., if I'm doing a bunch of commits, I might want to
> inhibit auto-gc and then run it myself at the end. Should we support
> "GIT_AUTO_GC=0" in the environment (and a matching "git --no-auto-gc
> ..." option that could be used here)?

export GIT_CONFIG=gc.auto=0 ?
-- 
Duy

Re: [PATCH] fetch: only run 'gc' once when fetching multiple remotes

[Jeff King]

On Thu, Jun 20, 2019 at 05:11:03PM +0700, Duy Nguyen wrote:

> > Another option would be to just pass "-c gc.auto=0" to the child
> > processes to inhibit auto-gc. But maybe it makes sense to have a nicer
> > interface (after all, somebody else could be doing the same "let's do a
> > bunch of fetches in a row" without using the multi-fetch code).
> 
> Nah to me -c is much nicer (and flexible too). The only thing I'm not
> sure about is whether a user could override it. If fetch.c adds -c
> gc.auto=0 automatically, and the user wants auto gc back, will "git -c
> gc.auto=non-zero fetch --multiple" still work?
> 
> I haven't checked git_config_push_parameter() carefully, but I have an
> impression that the parameter order there is "wrong", at least in this
> case.

It depends what you're trying to get it to do.

I'd expect that command to turn on auto-gc for just the outer fetch
(i.e., overriding any on-disk disabling of auto-gc), but keep it off for
the child fetches. And I think that is how it would behave: the outer
fetch only sees the config you provided. The inner ones see
"gc.auto=non-zero gc.auto=0-zero" because git_config_push_parameter()
appends. And they accept the latter under last-one-wins rules.

If you expect to be able to re-enable auto-gc in the child fetches, then
I don't think there would be a way to do that.

> > Though there I kind of wonder if this would apply to other scripted
> > uses, too. E.g., if I'm doing a bunch of commits, I might want to
> > inhibit auto-gc and then run it myself at the end. Should we support
> > "GIT_AUTO_GC=0" in the environment (and a matching "git --no-auto-gc
> > ..." option that could be used here)?
> 
> export GIT_CONFIG=gc.auto=0 ?

Almost. The parser for GIT_CONFIG_PARAMETERS is very picky about seeing
single quotes around each item, which makes it a little unfriendly to
use manually. Plus there may be existing values if your script was
invoked as "git -c whatever my-script".

It's probably not that big a deal for a script to just use "git -c
gc.auto" to inhibit auto-gc for each command that needs it.

-Peff

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Duy Nguyen]

On Thu, Jun 20, 2019 at 5:49 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Jun 19 2019, Jeff King wrote:
>
> > On Wed, Jun 19, 2019 at 08:01:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >
> >> > You could sort of avoid the problem here too with
> >> >
> >> > parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
> >> > git gc --auto
> >> >
> >> > It's definitely simpler, but of course we have to manually add
> >> > --no-auto-gc in everywhere we need, so not quite as elegant.
> >> >
> >> > Actually you could already do that with 'git -c gc.auto=false fetch', I guess.
> >>
> >> The point of the 'parallel' example is to show disconnected git
> >> commands, think trying to run 'git' in a terminal while your editor
> >> asynchronously runs a polling 'fetch', or a server with multiple
> >> concurrent clients running 'gc --auto'.
> >>
> >> That's the question my RFC patch raises. As far as I can tell the
> >> approach in your patch is only needed because our locking for gc is
> >> buggy, rather than introduce the caveat that an fetch(N) operation won't
> >> do "gc" until it's finished (we may have hundreds, thousands of remotes,
> >> I use that for some more obscure use-cases) shouldn't we just fix the
> >> locking?
> >
> > I think there may be room for both approaches. Yours fixes the repeated
> > message in the more general case, but Duy's suggestion is the most
> > efficient thing.
> >
> > I agree that the "thousands of remotes" case means we might want to gc
> > in the interim. But we probably ought to do that deterministically
> > rather than hoping that the pattern of lock contention makes sense.
>
> We do it deterministically, when gc.auto thresholds et al are exceeded
> we kick one off without waiting for other stuff, if we can get the lock.
>
> I don't think this desire to just wait a bit until all the fetches are
> complete makes sense as a special-case.
>
> If, as you noted in <20190619190845.GD28145@sigill.intra.peff.net>, the
> desire is to reduce GC CPU use then you're better off just tweaking the
> limits upwards. Then you get that with everything, like when you run
> "commit" in a for-loop, not just this one special case of "fetch".
>
> We have existing potentially long-running operations like "fetch",
> "rebase" and "git svn fetch" that run "gc --auto" for their incremental
> steps, and that's a feature.

gc --auto is added at arbitrary points to help garbage collection. I
don't think it's ever intended to "do gc at this and that exact
moment", just "hey this command has taken a lot of time already (i.e.
no instant response needed) and it may have added a bit more garbage,
let's just check real quick".

> It keeps "gc --auto" dumb enough to avoid a pathological case where
> we'll have a ballooning objects dir because we figure we can run
> something "at the end", when "the end" could be hours away, and we're
> adding a new pack or hundreds of loose objects every second.

Are we optimizing for a rare (large scale) case? Such setup requires
tuning regardless to me.

> So I don't think Duy's patch is a good way to go.

This reminds me of being perfect is the enemy of the good. A normal
user has a couple remotes at most, finishing fast (enough) and in such
case it's a good idea to wait until everything is in before running
gc.

Of course making git-gc more robust wrt. parallel access is great, but
it's hard work. Dealing with locks is always tricky, especially when
new locks can come up any time.

Having said that, I don't mind if my patch gets dropped. It was just a
"hey that multiple gc output looks strange, hah the fix is quite
simple" moment for me.
-- 
Duy

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Jeff King]

On Thu, Jun 20, 2019 at 12:49:32AM +0200, Ævar Arnfjörð Bjarmason wrote:

> We do it deterministically, when gc.auto thresholds et al are exceeded
> we kick one off without waiting for other stuff, if we can get the lock.
> 
> I don't think this desire to just wait a bit until all the fetches are
> complete makes sense as a special-case.
> 
> If, as you noted in <20190619190845.GD28145@sigill.intra.peff.net>, the
> desire is to reduce GC CPU use then you're better off just tweaking the
> limits upwards. Then you get that with everything, like when you run
> "commit" in a for-loop, not just this one special case of "fetch".

If you tweak the limit upwards, then you're more likely to exist in the
non-gc'd state, where reads are penalized. The gc limit is a tradeoff
between paying the price for maintenance work versus paying the price
for having an unmaintained state. So the optimal time is generally right
after you've finished a big chunk of writing, but before you've started
doing a bunch of reading (for continuous operations that are reading and
writing, there's probably some periodic crossover point every N units of
operation).

That said, I doubt it matters more than a few percent either way (if
that). So I'm fine if we want to optimize for simplicity.

-Peff

Re: [PATCH 0/6] Change <non-empty?> GIT_TEST_* variables to <boolean>

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This changes the remaining <non-empty?> special snowflake test modes
> to <boolean> and gets rid of test_tristate() in favor of the now
> standard "boolea" test.
>
> I'm replying to my "gc: run more pre-detach operations under lock"
> thread because one of the things my WIP patches to make gc locking
> less sucky depends on is new GIT_TEST_GC_* test modes to test its
> racyness, which in turn depends on these cleanups.

That sounds like the "gc: run more ..." depends on these (iow, that
should be the reply to these, not the other way around)?

I am asking because I see obvious value in these "uniformly require
<boolean>" consistency change (which could be backward incompatible,
but as long as these are GIT_TEST_*, we do not mind too much forcing
developers to adjust), but not yet in the "gc: run more ..." one,
and do not want these to be taken hostage.

Thanks.

>
> Ævar Arnfjörð Bjarmason (6):
>   env--helper: new undocumented builtin wrapping git_env_*()
>   t6040 test: stop using global "script" variable
>   tests: make GIT_TEST_GETTEXT_POISON a boolean
>   tests README: re-flow a previously changed paragraph
>   tests: replace test_tristate with "git env--helper"
>   tests: make GIT_TEST_FAIL_PREREQS a boolean
>
>  .gitignore                |  1 +
>  Makefile                  |  1 +
>  builtin.h                 |  1 +
>  builtin/env--helper.c     | 74 +++++++++++++++++++++++++++++++++++++++
>  ci/lib.sh                 |  2 +-
>  gettext.c                 |  6 ++--
>  git-sh-i18n.sh            |  4 ++-
>  git.c                     |  1 +
>  po/README                 |  2 +-
>  t/README                  | 12 +++----
>  t/lib-git-daemon.sh       |  7 ++--
>  t/lib-git-svn.sh          | 11 +++---
>  t/lib-httpd.sh            | 15 ++++----
>  t/t0000-basic.sh          | 10 +++---
>  t/t0016-env-helper.sh     | 70 ++++++++++++++++++++++++++++++++++++
>  t/t0205-gettext-poison.sh |  2 +-
>  t/t5512-ls-remote.sh      |  3 +-
>  t/t6040-tracking-info.sh  |  6 ++--
>  t/t7201-co.sh             |  2 +-
>  t/t9902-completion.sh     |  2 +-
>  t/test-lib-functions.sh   | 58 +++++-------------------------
>  t/test-lib.sh             | 29 ++++++++++++---
>  22 files changed, 220 insertions(+), 99 deletions(-)
>  create mode 100644 builtin/env--helper.c
>  create mode 100755 t/t0016-env-helper.sh

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think there may be room for both approaches. Yours fixes the repeated
> message in the more general case, but Duy's suggestion is the most
> efficient thing.

Yeah, not just the most efficient, but it is a low-hanging-fruit
that is very obvious.

> I agree that the "thousands of remotes" case means we might want to gc
> in the interim. But we probably ought to do that deterministically
> rather than hoping that the pattern of lock contention makes sense.

In the process chain starting from the topmost "git fetch
<multiple>", that calls multiple "git fetch <one>" for individual
remotes, that does network transfer and calls "git index-pack" (or
"git unpack-objects"), the bottommost layer knows how much cruft one
step among "thousands of remotes" created for a later "gc", but
there is no mechanism for them to report the information back to
their callers.  Unlike "git svn" that periodically calls the auto gc
every N commits and can claim to be aware of its cruft creation rate
;-), the information available to the topmost "git fetch" is only
the number of underlying fetches, counting a no-op fetch and a fetch
that is close to a full clone equally, which is not a good way to
gauge the cruft creation rate X-<.

A useful deteministic triggering would be a good thing to aim in the
longer run, but would be somewhat involved to design and implement,
I am afraid.

Re: [PATCH 1/6] env--helper: new undocumented builtin wrapping git_env_*()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +	struct option opts[] = {
> +		OPT_CMDMODE(0, "mode-bool", &cmdmode,
> +			    N_("invoke git_env_bool(...)"), ENV_HELPER_BOOL),
> +		OPT_CMDMODE(0, "mode-ulong", &cmdmode,
> +			    N_("invoke git_env_ulong(...)"), ENV_HELPER_ULONG),

It may be a fairly useless nitpick, but is there a reason why we
should not just reuse "--type=<bool|int|...>" that is understood by
"git config"?  In the longer term, it might make sense to expose
this subcommand as "git env" that sits next to "git config", and at
that point we would regret if we miss the opportunity for obvious
parallel between the two.

> +		OPT_STRING(0, "variable", &env_variable, N_("name"),
> +			   N_("which environment variable to ask git_env_*(...) about")),
> +		OPT_STRING(0, "default", &env_default, N_("value"),
> +			   N_("what default value does git_env_*(...) fall back on?")),
> +		OPT_BOOL(0, "exit-code", &exit_code,
> +			 N_("exit code determined by truth of the git_env_*() function")),
> +		OPT_BOOL(0, "quiet", &quiet,
> +			 N_("don't print the git_env_*() return value")),
> +		OPT_END(),
> +	};
> +
> +	if (parse_options(argc, argv, prefix, opts, env__helper_usage, 0))
> +		usage_with_options(env__helper_usage, opts);
> +	if (!env_variable || !env_default ||
> +	    !*env_variable || !*env_default)
> +		usage_with_options(env__helper_usage, opts);

The default must be supplied?  That makes it smell like it should
not be a command line "option", as it is not optional at all.  Off
the top of my head without the benefit of insight you gained by
working on the remainder of the series (read: this is merely my knee
jerk reaction, and it is very probable that there is sound rationale
why your design is not like what I'll outline), I would imagine an
interface that look more like:

 $ git env --type=bool --default=yes GIT_TEST_FOO 

    Says "true"/"false" on the standard output and exits with
    success status if $GIT_TEST_FOO is set to the usual
    truth/falsehood values, and gives "true" on the exits with
    success status if $GIT_TEST_FOO is unset.

 $ git env --type=bool GIT_TEST_FOO

    The same as above when $GIT_TEST_FOO is set; exits with failure
    when GIT_TEST_FOO is not exported.

 $ git env --type=bool [--default=yes] --exit-code GIT_TEST_FOO

    Similar to the above two, but the standard output is silent, and
    true/false/failure are given via the exit status (perhaps 0, 1
    and 125 or something like that).

> +	switch (cmdmode) {
> +	case ENV_HELPER_BOOL:
> +		tmp_int = strtol(env_default, (char **)&env_default, 10);
> +		if (*env_default) {
> +			error(_("option `--default' expects a numerical value with `--mode-bool`"));

No kidding.  "--mode-bool" does not like "--default=false"?

Re: [PATCH 2/6] t6040 test: stop using global "script" variable

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change test code added in c0234b2ef6 ("stat_tracking_info(): clear
> object flags used during counting", 2008-07-03) to stop using the
> "script" variable also used for lazy prerequisites in
> test-lib-functions.sh.
>
> Since this test uses test_i18ncmp and expects to use its own "script"
> variable twice it implicitly depends on the C_LOCALE_OUTPUT
> prerequisite not being a lazy prerequisite. A follow-up change will
> make it a lazy prerequisite, so we must remove this landmine before
> inadvertently stepping on it as we make that change.

Well spotted (eh, I suspect that you didn't until you actually stomped
on the landmine and saw it trigger, and then pretending to have had
the perfect foresight in this series, which is perfectly fine ;-),
and nicely done.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t6040-tracking-info.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 716283b274..970b25a289 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -38,7 +38,7 @@ test_expect_success setup '
>  	advance h
>  '
>  
> -script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
> +t6040_script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
>  cat >expect <<\EOF
>  b1 [ahead 1, behind 1] d
>  b2 [ahead 1, behind 1] d
> @@ -53,7 +53,7 @@ test_expect_success 'branch -v' '
>  		cd test &&
>  		git branch -v
>  	) |
> -	sed -n -e "$script" >actual &&
> +	sed -n -e "$t6040_script" >actual &&
>  	test_i18ncmp expect actual
>  '
>  
> @@ -71,7 +71,7 @@ test_expect_success 'branch -vv' '
>  		cd test &&
>  		git branch -vv
>  	) |
> -	sed -n -e "$script" >actual &&
> +	sed -n -e "$t6040_script" >actual &&
>  	test_i18ncmp expect actual
>  '

Re: [PATCH 3/6] tests: make GIT_TEST_GETTEXT_POISON a boolean

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the GIT_TEST_GETTEXT_POISON variable from being "non-empty?" to
> being a more standard boolean variable.
>
> Since it needed to be checked in both C code and shellscript (via test
> -n) it was one of the remaining shellscript-like variables. Now that
> we have "git env--helper" we can change that.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

Hmph.  Even though I earlier said "we do not terribly mind breaking
developers and that is why we allow these patches", I have second
thoughts.  Turning "If it is empty, it is false" to "if you want to
say false, say it in one of those approved ways" is one thing.
Forcing SWITCH=YesPlease to be rewritten to SWITCH=yes is quite
different---we are breaking everybody who would have to read and
follow po/README and t/README.

We _might_ have to grandfarther YesPlease as a special value that is
understood by "git env" (but not "git config") to ease the
transition, as that token has been used as a sample true value in
many places.

But let's read on.  Assuming that breaking those who hardcoded
YesPlease in their scripts is OK, this patch looked sensible.

Thanks.

Re: [PATCH 0/6] Change <non-empty?> GIT_TEST_* variables to <boolean>

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This changes the remaining <non-empty?> special snowflake test modes
> to <boolean> and gets rid of test_tristate() in favor of the now
> standard "boolea" test.

Is that "boolean" test?

I had a lot of trouble with the external interface to "env--helper",
but I kind of liked the changes to the tests that makes the use of
these environment variables uniform and consistent.  One fewer
things to remember.

In addition to the other review on 1/6, t0016 is taken at the tip of
'pu'; we would want to renumber to avoid test-lint complaints.

Re: [PATCH 0/6] Change <non-empty?> GIT_TEST_* variables to <boolean>

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 20 2019, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This changes the remaining <non-empty?> special snowflake test modes
>> to <boolean> and gets rid of test_tristate() in favor of the now
>> standard "boolea" test.
>>
>> I'm replying to my "gc: run more pre-detach operations under lock"
>> thread because one of the things my WIP patches to make gc locking
>> less sucky depends on is new GIT_TEST_GC_* test modes to test its
>> racyness, which in turn depends on these cleanups.
>
> That sounds like the "gc: run more ..." depends on these (iow, that
> should be the reply to these, not the other way around)?

Yeah the tests in that otherwise unrelated series loosely depends on
this, so I figured I'd try to get this in first.

> I am asking because I see obvious value in these "uniformly require
> <boolean>" consistency change (which could be backward incompatible,
> but as long as these are GIT_TEST_*, we do not mind too much forcing
> developers to adjust), but not yet in the "gc: run more ..." one,
> and do not want these to be taken hostage.

Yeah these should be viewed independently. Perhaps I shouldn't have
filled that In-Reply-To...

>>
>> Ævar Arnfjörð Bjarmason (6):
>>   env--helper: new undocumented builtin wrapping git_env_*()
>>   t6040 test: stop using global "script" variable
>>   tests: make GIT_TEST_GETTEXT_POISON a boolean
>>   tests README: re-flow a previously changed paragraph
>>   tests: replace test_tristate with "git env--helper"
>>   tests: make GIT_TEST_FAIL_PREREQS a boolean
>>
>>  .gitignore                |  1 +
>>  Makefile                  |  1 +
>>  builtin.h                 |  1 +
>>  builtin/env--helper.c     | 74 +++++++++++++++++++++++++++++++++++++++
>>  ci/lib.sh                 |  2 +-
>>  gettext.c                 |  6 ++--
>>  git-sh-i18n.sh            |  4 ++-
>>  git.c                     |  1 +
>>  po/README                 |  2 +-
>>  t/README                  | 12 +++----
>>  t/lib-git-daemon.sh       |  7 ++--
>>  t/lib-git-svn.sh          | 11 +++---
>>  t/lib-httpd.sh            | 15 ++++----
>>  t/t0000-basic.sh          | 10 +++---
>>  t/t0016-env-helper.sh     | 70 ++++++++++++++++++++++++++++++++++++
>>  t/t0205-gettext-poison.sh |  2 +-
>>  t/t5512-ls-remote.sh      |  3 +-
>>  t/t6040-tracking-info.sh  |  6 ++--
>>  t/t7201-co.sh             |  2 +-
>>  t/t9902-completion.sh     |  2 +-
>>  t/test-lib-functions.sh   | 58 +++++-------------------------
>>  t/test-lib.sh             | 29 ++++++++++++---
>>  22 files changed, 220 insertions(+), 99 deletions(-)
>>  create mode 100644 builtin/env--helper.c
>>  create mode 100755 t/t0016-env-helper.sh

[PATCH v2 0/8] Change <non-empty?> GIT_TEST_* variables to <boolean>

[Ævar Arnfjörð Bjarmason]

This v2 fixes tricky bugs I noticed after sending v1 in calling
gettext so early in the setup, and the t0016 name clash with pu Junio
pointed out.

I didn't change the "env--helper" interface as suggested because I
already had this ready and figured I'd send a v2 for review of the
stuff I have now.

The interface suggested in
<xmqqa7ecnjot.fsf@gitster-ct.c.googlers.com> is indeed prettier. FWIW
I did things like --mode-bool instead of --type=bool because it's
easier to do just with the getopt framework, likewise --variable=X
instead of "X" being last on the argv since you can do it all with the
flag parsing doing the work for you, and since it's an internal-only
command I figured it didn't matter much.

Ævar Arnfjörð Bjarmason (8):
  config tests: simplify include cycle test
  env--helper: new undocumented builtin wrapping git_env_*()
  config.c: refactor die_bad_number() to not call gettext() early
  t6040 test: stop using global "script" variable
  tests: make GIT_TEST_GETTEXT_POISON a boolean
  tests README: re-flow a previously changed paragraph
  tests: replace test_tristate with "git env--helper"
  tests: make GIT_TEST_FAIL_PREREQS a boolean

 .gitignore                |  1 +
 Makefile                  |  1 +
 builtin.h                 |  1 +
 builtin/env--helper.c     | 74 +++++++++++++++++++++++++++++++++
 ci/lib.sh                 |  2 +-
 config.c                  | 28 +++++++++----
 gettext.c                 |  6 +--
 git-sh-i18n.sh            |  4 +-
 git.c                     |  1 +
 po/README                 |  2 +-
 t/README                  | 12 +++---
 t/lib-git-daemon.sh       |  7 ++--
 t/lib-git-svn.sh          | 11 ++---
 t/lib-httpd.sh            | 15 ++++---
 t/t0000-basic.sh          | 10 ++---
 t/t0017-env-helper.sh     | 86 +++++++++++++++++++++++++++++++++++++++
 t/t0205-gettext-poison.sh |  7 +++-
 t/t1305-config-include.sh | 21 ++++------
 t/t5512-ls-remote.sh      |  3 +-
 t/t6040-tracking-info.sh  |  6 +--
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib-functions.sh   | 58 ++++----------------------
 t/test-lib.sh             | 33 +++++++++++----
 24 files changed, 266 insertions(+), 127 deletions(-)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0017-env-helper.sh

Range-diff:
-:  ---------- > 1:  c3483c37a1 config tests: simplify include cycle test
1:  8da3cd4240 ! 2:  e689759f7c env--helper: new undocumented builtin wrapping git_env_*()
    @@ -154,10 +154,10 @@
      	{ "fetch", cmd_fetch, RUN_SETUP },
      	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
     
    - diff --git a/t/t0016-env-helper.sh b/t/t0016-env-helper.sh
    + diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
      new file mode 100755
      --- /dev/null
    - +++ b/t/t0016-env-helper.sh
    + +++ b/t/t0017-env-helper.sh
     @@
     +#!/bin/sh
     +
-:  ---------- > 3:  f759d5e91e config.c: refactor die_bad_number() to not call gettext() early
2:  8315c4ecdc = 4:  1ac798e8ce t6040 test: stop using global "script" variable
3:  f1ee208d70 ! 5:  d7d6e6c874 tests: make GIT_TEST_GETTEXT_POISON a boolean
    @@ -7,7 +7,30 @@
     
         Since it needed to be checked in both C code and shellscript (via test
         -n) it was one of the remaining shellscript-like variables. Now that
    -    we have "git env--helper" we can change that.
    +    we have "env--helper" we can change that.
    +
    +    There's a couple of tricky edge cases that arise because we're using
    +    git_env_bool() early, and the config-reading "env--helper".
    +
    +    If GIT_TEST_GETTEXT_POISON is set to an invalid value die_bad_number()
    +    will die, but to do so it would usually call gettext(). Let's detect
    +    the special case of GIT_TEST_GETTEXT_POISON and always emit that
    +    message in the C locale, lest we infinitely loop.
    +
    +    As seen in the updated tests in t0016-env-helper.sh there's also a
    +    caveat related to "env--helper" needing to read the config for trace2
    +    purposes.
    +
    +    Since the C_LOCALE_OUTPUT prerequisite is lazy and relies on
    +    "env--helper" we could get invalid results if we failed to read the
    +    config (e.g. because we'd loop on includes) when combined with
    +    e.g. "test_i18ngrep" wanting to check with "env--helper" if
    +    GIT_TEST_GETTEXT_POISON was true or not.
    +
    +    I'm crossing my fingers and hoping that a test similar to the one I
    +    removed in the earlier "config tests: simplify include cycle test"
    +    change in this series won't happen again, and testing for this
    +    explicitly in "env--helper"'s own tests.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -24,6 +47,26 @@
      esac
      
     
    + diff --git a/config.c b/config.c
    + --- a/config.c
    + +++ b/config.c
    +@@
    + 	if (!value)
    + 		value = "";
    + 
    ++	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
    ++		/*
    ++		 * We explicitly *don't* use _() here since it would
    ++		 * cause an infinite loop with _() needing to call
    ++		 * use_gettext_poison(). This is why marked up
    ++		 * translations with N_() above.
    ++		 */
    ++		die(bad_numeric, value, name, error_type);
    ++
    + 	if (!(cf && cf->name))
    + 		die(_(bad_numeric), value, name, _(error_type));
    + 
    +
      diff --git a/gettext.c b/gettext.c
      --- a/gettext.c
      +++ b/gettext.c
    @@ -84,6 +127,31 @@
      prerequisite when adding more strings for translation. See "Testing
      marked strings" in po/README for details.
     
    + diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
    + --- a/t/t0017-env-helper.sh
    + +++ b/t/t0017-env-helper.sh
    +@@
    + 	test_cmp expected actual
    + '
    + 
    ++test_expect_success 'env--helper reads config thanks to trace2' '
    ++	mkdir home &&
    ++	git config -f home/.gitconfig include.path cycle &&
    ++	git config -f home/cycle include.path .gitconfig &&
    ++
    ++	test_must_fail \
    ++		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
    ++		git config -l 2>err &&
    ++	grep "exceeded maximum include depth" err &&
    ++
    ++	test_must_fail \
    ++		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
    ++		git -C cycle env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet 2>err &&
    ++	grep "# GETTEXT POISON #" err
    ++'
    ++
    + test_done
    +
      diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
      --- a/t/t0205-gettext-poison.sh
      +++ b/t/t0205-gettext-poison.sh
    @@ -96,6 +164,29 @@
      export GIT_TEST_GETTEXT_POISON
      . ./lib-gettext.sh
      
    +@@
    +     test_cmp expect actual
    + '
    + 
    ++test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
    ++	test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
    ++	grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
    ++"
    ++
    + test_done
    +
    + diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
    + --- a/t/t1305-config-include.sh
    + +++ b/t/t1305-config-include.sh
    +@@
    + 	git -C cycle config include.path cycle &&
    + 	git config -f cycle/cycle include.path config &&
    + 	test_must_fail \
    +-		env GIT_TEST_GETTEXT_POISON= \
    ++		env GIT_TEST_GETTEXT_POISON=false \
    + 		git -C cycle config --get-all test.value 2>stderr &&
    + 	grep "exceeded maximum include depth" stderr
    + '
     
      diff --git a/t/t7201-co.sh b/t/t7201-co.sh
      --- a/t/t7201-co.sh
    @@ -130,10 +221,14 @@
      	unset GIT_TEST_GETTEXT_POISON_ORIG
      fi
      
    +-# Can we rely on git's output in the C locale?
    +-if test -z "$GIT_TEST_GETTEXT_POISON"
    +-then
    +-	test_set_prereq C_LOCALE_OUTPUT
    +-fi
     +test_lazy_prereq C_LOCALE_OUTPUT '
     +	! git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet
     +'
    -+
    - # Can we rely on git's output in the C locale?
    - if test -z "$GIT_TEST_GETTEXT_POISON"
    + 
    + if test -z "$GIT_TEST_CHECK_CACHE_TREE"
      then
4:  f1e28dff36 = 6:  954428b3cd tests README: re-flow a previously changed paragraph
5:  f046cf21fb = 7:  79b41cf01b tests: replace test_tristate with "git env--helper"
6:  e2c68e0239 = 8:  a9aa166b66 tests: make GIT_TEST_FAIL_PREREQS a boolean
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 1/8] config tests: simplify include cycle test

[Ævar Arnfjörð Bjarmason]

Simplify an overly verbose test added in 9b25a0b52e ("config: add
include directive", 2012-02-06). The "expect" file was never used, and
by using .gitconfig it's not as intuitive to reproduce this manually
with "-d" as some other tests, since HOME needs to be set in the
environment.

Also remove the use of test_i18ngrep added in a769bfc74f ("config.c:
mark more strings for translation", 2018-07-21) in favor of overriding
the GIT_TEST_GETTEXT_POISON value.

Using the i18n test wrappers hasn't been needed since my
6cdccfce1e ("i18n: make GETTEXT_POISON a runtime option", 2018-11-08).
As a follow-up change to the yet-to-be-added t0017-env-helper.sh will
show, doing it this way can hide a regression when combined with
trace2's early config reading. That early config reading was added in
bce9db6de9 ("trace2: use system/global config for default trace2
settings", 2019-04-15).

So let's remove the testing for that potential regression here, I'll
instead add it explicitly to t0017-env-helper.sh in a follow-up
change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1305-config-include.sh | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 579a86b7f8..6b388ba2d0 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -310,20 +310,13 @@ test_expect_success SYMLINKS 'conditional include, gitdir matching symlink, icas
 '
 
 test_expect_success 'include cycles are detected' '
-	cat >.gitconfig <<-\EOF &&
-	[test]value = gitconfig
-	[include]path = cycle
-	EOF
-	cat >cycle <<-\EOF &&
-	[test]value = cycle
-	[include]path = .gitconfig
-	EOF
-	cat >expect <<-\EOF &&
-	gitconfig
-	cycle
-	EOF
-	test_must_fail git config --get-all test.value 2>stderr &&
-	test_i18ngrep "exceeded maximum include depth" stderr
+	git init --bare cycle &&
+	git -C cycle config include.path cycle &&
+	git config -f cycle/cycle include.path config &&
+	test_must_fail \
+		env GIT_TEST_GETTEXT_POISON= \
+		git -C cycle config --get-all test.value 2>stderr &&
+	grep "exceeded maximum include depth" stderr
 '
 
 test_done
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Ævar Arnfjörð Bjarmason]

We have many GIT_TEST_* variables that accept a <boolean> because
they're implemented in C, and then some that take <non-empty?> because
they're implemented at least partially in shellscript.

Add a helper that wraps git_env_bool() and git_env_ulong() as the
first step in fixing this. This isn't being added as a test-tool mode
because some of these are used outside the test suite.

Part of what this tool does can be done via a trick with "git config"
added in 83d842dc8c ("tests: turn on network daemon tests by default",
2014-02-10) for test_tristate(), i.e.:

    git -c magic.variable="$1" config --bool magic.variable 2>/dev/null

But as subsequent changes will show being able to pass along the
default value makes all the difference, and we'll be able to replace
test_tristate() itself with that.

The --mode-bool option will be used by subsequent patches, but not
--mode-ulong. I figured it was easy enough to add it & test for it so
I left it in so we'd have wrappers for both git_env_*() functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 builtin.h             |  1 +
 builtin/env--helper.c | 74 +++++++++++++++++++++++++++++++++++++++++++
 git.c                 |  1 +
 t/t0017-env-helper.sh | 70 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 148 insertions(+)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0017-env-helper.sh

diff --git a/.gitignore b/.gitignore
index 4470d7cfc0..1f7a83fb3c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -58,6 +58,7 @@
 /git-difftool
 /git-difftool--helper
 /git-describe
+/git-env--helper
 /git-fast-export
 /git-fast-import
 /git-fetch
diff --git a/Makefile b/Makefile
index f58bf14c7b..f2cfc8d812 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
 BUILTIN_OBJS += builtin/difftool.o
+BUILTIN_OBJS += builtin/env--helper.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index ec7e0954c4..93bd49fe4f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -160,6 +160,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix);
 int cmd_diff(int argc, const char **argv, const char *prefix);
 int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 int cmd_difftool(int argc, const char **argv, const char *prefix);
+int cmd_env__helper(int argc, const char **argv, const char *prefix);
 int cmd_fast_export(int argc, const char **argv, const char *prefix);
 int cmd_fetch(int argc, const char **argv, const char *prefix);
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/env--helper.c b/builtin/env--helper.c
new file mode 100644
index 0000000000..2bb65ecf3f
--- /dev/null
+++ b/builtin/env--helper.c
@@ -0,0 +1,74 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+
+static char const * const env__helper_usage[] = {
+	N_("git env--helper [--mode-bool | --mode-ulong] --env-variable=<VAR> --env-default=<DEF> [<options>]"),
+	NULL
+};
+
+int cmd_env__helper(int argc, const char **argv, const char *prefix)
+{
+	enum {
+		ENV_HELPER_BOOL = 1,
+		ENV_HELPER_ULONG,
+	} cmdmode = 0;
+	int exit_code = 0;
+	int quiet = 0;
+	const char *env_variable = NULL;
+	const char *env_default = NULL;
+	int ret;
+	int ret_int, tmp_int;
+	unsigned long ret_ulong, tmp_ulong;
+	struct option opts[] = {
+		OPT_CMDMODE(0, "mode-bool", &cmdmode,
+			    N_("invoke git_env_bool(...)"), ENV_HELPER_BOOL),
+		OPT_CMDMODE(0, "mode-ulong", &cmdmode,
+			    N_("invoke git_env_ulong(...)"), ENV_HELPER_ULONG),
+		OPT_STRING(0, "variable", &env_variable, N_("name"),
+			   N_("which environment variable to ask git_env_*(...) about")),
+		OPT_STRING(0, "default", &env_default, N_("value"),
+			   N_("what default value does git_env_*(...) fall back on?")),
+		OPT_BOOL(0, "exit-code", &exit_code,
+			 N_("exit code determined by truth of the git_env_*() function")),
+		OPT_BOOL(0, "quiet", &quiet,
+			 N_("don't print the git_env_*() return value")),
+		OPT_END(),
+	};
+
+	if (parse_options(argc, argv, prefix, opts, env__helper_usage, 0))
+		usage_with_options(env__helper_usage, opts);
+	if (!env_variable || !env_default ||
+	    !*env_variable || !*env_default)
+		usage_with_options(env__helper_usage, opts);
+
+	switch (cmdmode) {
+	case ENV_HELPER_BOOL:
+		tmp_int = strtol(env_default, (char **)&env_default, 10);
+		if (*env_default) {
+			error(_("option `--default' expects a numerical value with `--mode-bool`"));
+			usage_with_options(env__helper_usage, opts);
+		}
+		ret_int = git_env_bool(env_variable, tmp_int);
+		if (!quiet)
+			printf("%d\n", ret_int);
+		ret = ret_int;
+		break;
+	case ENV_HELPER_ULONG:
+		tmp_ulong = strtoll(env_default, (char **)&env_default, 10);
+		if (*env_default) {
+			error(_("option `--default' expects a numerical value with `--mode-ulong`"));
+			usage_with_options(env__helper_usage, opts);
+		}
+		ret_ulong = git_env_ulong(env_variable, tmp_ulong);
+		if (!quiet)
+			printf("%lu\n", ret_ulong);
+		ret = ret_ulong;
+		break;
+	}
+
+	if (exit_code)
+		return !ret;
+
+	return 0;
+}
diff --git a/git.c b/git.c
index c2eec470c9..a43e1dd98e 100644
--- a/git.c
+++ b/git.c
@@ -500,6 +500,7 @@ static struct cmd_struct commands[] = {
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
+	{ "env--helper", cmd_env__helper },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
new file mode 100755
index 0000000000..4dc4ab35e5
--- /dev/null
+++ b/t/t0017-env-helper.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='test env--helper'
+
+. ./test-lib.sh
+
+
+test_expect_success 'env--helper usage' '
+	test_must_fail git env--helper &&
+	test_must_fail git env--helper --mode-bool &&
+	test_must_fail git env--helper --mode-ulong &&
+	test_must_fail git env--helper --mode-bool --variable &&
+	test_must_fail git env--helper --mode-bool --variable --default &&
+	test_must_fail git env--helper --mode-bool --variable= --default=
+'
+
+test_expect_success 'env--helper bad default values' '
+	test_must_fail git env--helper --mode-bool --variable=MISSING --default=1xyz &&
+	test_must_fail git env--helper --mode-ulong --variable=MISSING --default=1xyz
+'
+
+test_expect_success 'env--helper --mode-bool' '
+	echo 1 >expected &&
+	git env--helper --mode-bool --variable=MISSING --default=1 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	echo 0 >expected &&
+	test_must_fail git env--helper --mode-bool --variable=MISSING --default=0 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	git env--helper --mode-bool --variable=MISSING --default=0 >actual &&
+	test_cmp expected actual &&
+
+	>expected &&
+	git env--helper --mode-bool --variable=MISSING --default=1 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	EXISTS=true git env--helper --mode-bool --variable=EXISTS --default=0 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	echo 1 >expected &&
+	EXISTS=true git env--helper --mode-bool --variable=EXISTS --default=0 --exit-code >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'env--helper --mode-ulong' '
+	echo 1234567890 >expected &&
+	git env--helper --mode-ulong --variable=MISSING --default=1234567890 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	echo 0 >expected &&
+	test_must_fail git env--helper --mode-ulong --variable=MISSING --default=0 --exit-code >actual &&
+	test_cmp expected actual &&
+
+	git env--helper --mode-ulong --variable=MISSING --default=0 >actual &&
+	test_cmp expected actual &&
+
+	>expected &&
+	git env--helper --mode-ulong --variable=MISSING --default=1234567890 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	EXISTS=1234567890 git env--helper --mode-ulong --variable=EXISTS --default=0 --exit-code --quiet >actual &&
+	test_cmp expected actual &&
+
+	echo 1234567890 >expected &&
+	EXISTS=1234567890 git env--helper --mode-ulong --variable=EXISTS --default=0 --exit-code >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 3/8] config.c: refactor die_bad_number() to not call gettext() early

[Ævar Arnfjörð Bjarmason]

Prepare die_bad_number() for a change to specially handle
GIT_TEST_GETTEXT_POISON calling git_env_bool() by making
die_bad_number() not call gettext() early, which would in turn call
git_env_bool().

There's no meaningful change here yet, just a re-arrangement of the
current code to make that subsequent change easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/config.c b/config.c
index 296a6d9cc4..374cb33005 100644
--- a/config.c
+++ b/config.c
@@ -949,34 +949,35 @@ int git_parse_ssize_t(const char *value, ssize_t *ret)
 NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
-	const char * error_type = (errno == ERANGE)? _("out of range"):_("invalid unit");
+	const char *error_type = (errno == ERANGE) ?
+		N_("out of range") : N_("invalid unit");
+	const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s");
 
 	if (!value)
 		value = "";
 
 	if (!(cf && cf->name))
-		die(_("bad numeric config value '%s' for '%s': %s"),
-		    value, name, error_type);
+		die(_(bad_numeric), value, name, _(error_type));
 
 	switch (cf->origin_type) {
 	case CONFIG_ORIGIN_BLOB:
 		die(_("bad numeric config value '%s' for '%s' in blob %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	case CONFIG_ORIGIN_FILE:
 		die(_("bad numeric config value '%s' for '%s' in file %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	case CONFIG_ORIGIN_STDIN:
 		die(_("bad numeric config value '%s' for '%s' in standard input: %s"),
-		    value, name, error_type);
+		    value, name, _(error_type));
 	case CONFIG_ORIGIN_SUBMODULE_BLOB:
 		die(_("bad numeric config value '%s' for '%s' in submodule-blob %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	case CONFIG_ORIGIN_CMDLINE:
 		die(_("bad numeric config value '%s' for '%s' in command line %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	default:
 		die(_("bad numeric config value '%s' for '%s' in %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	}
 }
 
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 4/8] t6040 test: stop using global "script" variable

[Ævar Arnfjörð Bjarmason]

Change test code added in c0234b2ef6 ("stat_tracking_info(): clear
object flags used during counting", 2008-07-03) to stop using the
"script" variable also used for lazy prerequisites in
test-lib-functions.sh.

Since this test uses test_i18ncmp and expects to use its own "script"
variable twice it implicitly depends on the C_LOCALE_OUTPUT
prerequisite not being a lazy prerequisite. A follow-up change will
make it a lazy prerequisite, so we must remove this landmine before
inadvertently stepping on it as we make that change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6040-tracking-info.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 716283b274..970b25a289 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -38,7 +38,7 @@ test_expect_success setup '
 	advance h
 '
 
-script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
+t6040_script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
 cat >expect <<\EOF
 b1 [ahead 1, behind 1] d
 b2 [ahead 1, behind 1] d
@@ -53,7 +53,7 @@ test_expect_success 'branch -v' '
 		cd test &&
 		git branch -v
 	) |
-	sed -n -e "$script" >actual &&
+	sed -n -e "$t6040_script" >actual &&
 	test_i18ncmp expect actual
 '
 
@@ -71,7 +71,7 @@ test_expect_success 'branch -vv' '
 		cd test &&
 		git branch -vv
 	) |
-	sed -n -e "$script" >actual &&
+	sed -n -e "$t6040_script" >actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean

[Ævar Arnfjörð Bjarmason]

Change the GIT_TEST_GETTEXT_POISON variable from being "non-empty?" to
being a more standard boolean variable.

Since it needed to be checked in both C code and shellscript (via test
-n) it was one of the remaining shellscript-like variables. Now that
we have "env--helper" we can change that.

There's a couple of tricky edge cases that arise because we're using
git_env_bool() early, and the config-reading "env--helper".

If GIT_TEST_GETTEXT_POISON is set to an invalid value die_bad_number()
will die, but to do so it would usually call gettext(). Let's detect
the special case of GIT_TEST_GETTEXT_POISON and always emit that
message in the C locale, lest we infinitely loop.

As seen in the updated tests in t0016-env-helper.sh there's also a
caveat related to "env--helper" needing to read the config for trace2
purposes.

Since the C_LOCALE_OUTPUT prerequisite is lazy and relies on
"env--helper" we could get invalid results if we failed to read the
config (e.g. because we'd loop on includes) when combined with
e.g. "test_i18ngrep" wanting to check with "env--helper" if
GIT_TEST_GETTEXT_POISON was true or not.

I'm crossing my fingers and hoping that a test similar to the one I
removed in the earlier "config tests: simplify include cycle test"
change in this series won't happen again, and testing for this
explicitly in "env--helper"'s own tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ci/lib.sh                 |  2 +-
 config.c                  |  9 +++++++++
 gettext.c                 |  6 ++----
 git-sh-i18n.sh            |  4 +++-
 po/README                 |  2 +-
 t/README                  |  4 ++--
 t/t0017-env-helper.sh     | 16 ++++++++++++++++
 t/t0205-gettext-poison.sh |  7 ++++++-
 t/t1305-config-include.sh |  2 +-
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib.sh             |  8 +++-----
 12 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 288a5b3884..fd799ae663 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -184,7 +184,7 @@ osx-clang|osx-gcc)
 	export GIT_SKIP_TESTS="t9810 t9816"
 	;;
 GIT_TEST_GETTEXT_POISON)
-	export GIT_TEST_GETTEXT_POISON=YesPlease
+	export GIT_TEST_GETTEXT_POISON=true
 	;;
 esac
 
diff --git a/config.c b/config.c
index 374cb33005..b985d60fa4 100644
--- a/config.c
+++ b/config.c
@@ -956,6 +956,15 @@ static void die_bad_number(const char *name, const char *value)
 	if (!value)
 		value = "";
 
+	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+		/*
+		 * We explicitly *don't* use _() here since it would
+		 * cause an infinite loop with _() needing to call
+		 * use_gettext_poison(). This is why marked up
+		 * translations with N_() above.
+		 */
+		die(bad_numeric, value, name, error_type);
+
 	if (!(cf && cf->name))
 		die(_(bad_numeric), value, name, _(error_type));
 
diff --git a/gettext.c b/gettext.c
index d4021d690c..5c71f4c8b9 100644
--- a/gettext.c
+++ b/gettext.c
@@ -50,10 +50,8 @@ const char *get_preferred_languages(void)
 int use_gettext_poison(void)
 {
 	static int poison_requested = -1;
-	if (poison_requested == -1) {
-		const char *v = getenv("GIT_TEST_GETTEXT_POISON");
-		poison_requested = v && strlen(v) ? 1 : 0;
-	}
+	if (poison_requested == -1)
+		poison_requested = git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
 	return poison_requested;
 }
 
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index e1d917fd27..de8ae67d7b 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,9 @@ export TEXTDOMAINDIR
 
 # First decide what scheme to use...
 GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_TEST_GETTEXT_POISON"
+if test -n "$GIT_TEST_GETTEXT_POISON" &&
+	    git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON \
+		--default=0 --exit-code --quiet
 then
 	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
 elif test -n "@@USE_GETTEXT_SCHEME@@"
diff --git a/po/README b/po/README
index aa704ffcb7..07595d369b 100644
--- a/po/README
+++ b/po/README
@@ -293,7 +293,7 @@ To smoke out issues like these, Git tested with a translation mode that
 emits gibberish on every call to gettext. To use it run the test suite
 with it, e.g.:
 
-    cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh
+    cd t && GIT_TEST_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh
 
 If tests break with it you should inspect them manually and see if
 what you're translating is sane, i.e. that you're not translating
diff --git a/t/README b/t/README
index 9747971d58..9a131f472e 100644
--- a/t/README
+++ b/t/README
@@ -343,8 +343,8 @@ whether this mode is active, and e.g. skip some tests that are hard to
 refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
-GIT_TEST_GETTEXT_POISON=<non-empty?> turns all strings marked for
-translation into gibberish if non-empty (think "test -n"). Used for
+GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
+translation into gibberish if true. Used for
 spotting those tests that need to be marked with a C_LOCALE_OUTPUT
 prerequisite when adding more strings for translation. See "Testing
 marked strings" in po/README for details.
diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
index 4dc4ab35e5..f62814241d 100755
--- a/t/t0017-env-helper.sh
+++ b/t/t0017-env-helper.sh
@@ -67,4 +67,20 @@ test_expect_success 'env--helper --mode-ulong' '
 	test_cmp expected actual
 '
 
+test_expect_success 'env--helper reads config thanks to trace2' '
+	mkdir home &&
+	git config -f home/.gitconfig include.path cycle &&
+	git config -f home/cycle include.path .gitconfig &&
+
+	test_must_fail \
+		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
+		git config -l 2>err &&
+	grep "exceeded maximum include depth" err &&
+
+	test_must_fail \
+		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
+		git -C cycle env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet 2>err &&
+	grep "# GETTEXT POISON #" err
+'
+
 test_done
diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index a06269f38a..f9fa16ad83 100755
--- a/t/t0205-gettext-poison.sh
+++ b/t/t0205-gettext-poison.sh
@@ -5,7 +5,7 @@
 
 test_description='Gettext Shell poison'
 
-GIT_TEST_GETTEXT_POISON=YesPlease
+GIT_TEST_GETTEXT_POISON=true
 export GIT_TEST_GETTEXT_POISON
 . ./lib-gettext.sh
 
@@ -31,4 +31,9 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant
     test_cmp expect actual
 '
 
+test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
+	test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
+	grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
+"
+
 test_done
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 6b388ba2d0..de294c990e 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -314,7 +314,7 @@ test_expect_success 'include cycles are detected' '
 	git -C cycle config include.path cycle &&
 	git config -f cycle/cycle include.path config &&
 	test_must_fail \
-		env GIT_TEST_GETTEXT_POISON= \
+		env GIT_TEST_GETTEXT_POISON=false \
 		git -C cycle config --get-all test.value 2>stderr &&
 	grep "exceeded maximum include depth" stderr
 '
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 5990299fc9..b696bae5f5 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -249,7 +249,7 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '
 test_expect_success 'checkout to detach HEAD' '
 	git config advice.detachedHead true &&
 	git checkout -f renamer && git clean -f &&
-	GIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&
+	GIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&
 	grep "HEAD is now at 7329388" messages &&
 	test_line_count -gt 1 messages &&
 	H=$(git rev-parse --verify HEAD) &&
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43cf313a1c..75512c3403 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1706,7 +1706,7 @@ test_expect_success 'sourcing the completion script clears cached commands' '
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
-	GIT_TEST_GETTEXT_POISON= &&
+	GIT_TEST_GETTEXT_POISON=false &&
 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
 	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4b346467df..c45b0d2611 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1443,11 +1443,9 @@ then
 	unset GIT_TEST_GETTEXT_POISON_ORIG
 fi
 
-# Can we rely on git's output in the C locale?
-if test -z "$GIT_TEST_GETTEXT_POISON"
-then
-	test_set_prereq C_LOCALE_OUTPUT
-fi
+test_lazy_prereq C_LOCALE_OUTPUT '
+	! git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet
+'
 
 if test -z "$GIT_TEST_CHECK_CACHE_TREE"
 then
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 6/8] tests README: re-flow a previously changed paragraph

[Ævar Arnfjörð Bjarmason]

A previous change to the "GIT_TEST_GETTEXT_POISON" variable left this
paragraph needing to be re-flowed. Let's do that in this separate
change to make it easy to see that there's no change here when viewed
with "--word-diff".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index 9a131f472e..072c9854d1 100644
--- a/t/README
+++ b/t/README
@@ -344,10 +344,10 @@ refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
 GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
-translation into gibberish if true. Used for
-spotting those tests that need to be marked with a C_LOCALE_OUTPUT
-prerequisite when adding more strings for translation. See "Testing
-marked strings" in po/README for details.
+translation into gibberish if true. Used for spotting those tests that
+need to be marked with a C_LOCALE_OUTPUT prerequisite when adding more
+strings for translation. See "Testing marked strings" in po/README for
+details.
 
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 7/8] tests: replace test_tristate with "git env--helper"

[Ævar Arnfjörð Bjarmason]

The test_tristate helper introduced in 83d842dc8c ("tests: turn on
network daemon tests by default", 2014-02-10) can now be better
implemented with "git env--helper" to give the variables in question
the standard boolean behavior.

The reason for the "tristate" was to have all of false/true/auto,
where "auto" meant either "false" or "true" depending on what the
fallback was. With the --default option to "git env--helper" we can
simply have e.g. GIT_TEST_HTTPD where we know if it's true because the
user asked explicitly ("true"), or true implicitly ("auto").

This breaks backwards compatibility for explicitly setting "auto" for
these variables, but I don't think anyone cares. That was always
intended to be internal.

This means the test_normalize_bool() code in test-lib-functions.sh
goes away in addition to test_tristate(). We still need the
test_skip_or_die() helper, but now it takes the variable name instead
of the value, and uses "git env--bool" to distinguish a default "true"
from an explicit "true" (in those "explicit true" cases we want to
fail the test in question).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-git-daemon.sh     |  7 +++---
 t/lib-git-svn.sh        | 11 +++-----
 t/lib-httpd.sh          | 15 ++++++-----
 t/t5512-ls-remote.sh    |  3 +--
 t/test-lib-functions.sh | 56 ++++++-----------------------------------
 5 files changed, 22 insertions(+), 70 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 7b3407134e..770c5218ea 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -15,8 +15,7 @@
 #
 #	test_done
 
-test_tristate GIT_TEST_GIT_DAEMON
-if test "$GIT_TEST_GIT_DAEMON" = false
+if ! git env--helper --mode-bool --variable=GIT_TEST_GIT_DAEMON --default=1 --exit-code --quiet
 then
 	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
 	test_done
@@ -24,7 +23,7 @@ fi
 
 if test_have_prereq !PIPE
 then
-	test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
+	test_skip_or_die GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
 fi
 
 test_set_port LIB_GIT_DAEMON_PORT
@@ -73,7 +72,7 @@ start_git_daemon() {
 		kill "$GIT_DAEMON_PID"
 		wait "$GIT_DAEMON_PID"
 		unset GIT_DAEMON_PID
-		test_skip_or_die $GIT_TEST_GIT_DAEMON \
+		test_skip_or_die GIT_TEST_GIT_DAEMON \
 			"git daemon failed to start"
 	fi
 }
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index c1271d6863..853d33a57a 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -69,14 +69,12 @@ svn_cmd () {
 maybe_start_httpd () {
 	loc=${1-svn}
 
-	test_tristate GIT_SVN_TEST_HTTPD
-	case $GIT_SVN_TEST_HTTPD in
-	true)
+	if git env--helper --mode-bool --variable=GIT_TEST_HTTPD --default=0 --exit-code --quiet	
+	then
 		. "$TEST_DIRECTORY"/lib-httpd.sh
 		LIB_HTTPD_SVN="$loc"
 		start_httpd
-		;;
-	esac
+	fi
 }
 
 convert_to_rev_db () {
@@ -106,8 +104,7 @@ EOF
 }
 
 require_svnserve () {
-	test_tristate GIT_TEST_SVNSERVE
-	if ! test "$GIT_TEST_SVNSERVE" = true
+	if ! git env--helper --mode-bool --variable=GIT_TEST_SVNSERVE --default=0 --exit-code --quiet
 	then
 		skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to enable)'
 		test_done
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b3cc62bd36..eef3250552 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -41,15 +41,14 @@ then
 	test_done
 fi
 
-test_tristate GIT_TEST_HTTPD
-if test "$GIT_TEST_HTTPD" = false
+if ! git env--helper --mode-bool --variable=GIT_TEST_HTTPD --default=1 --exit-code --quiet
 then
 	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
 	test_done
 fi
 
 if ! test_have_prereq NOT_ROOT; then
-	test_skip_or_die $GIT_TEST_HTTPD \
+	test_skip_or_die GIT_TEST_HTTPD \
 		"Cannot run httpd tests as root"
 fi
 
@@ -95,7 +94,7 @@ GIT_TRACE=$GIT_TRACE; export GIT_TRACE
 
 if ! test -x "$LIB_HTTPD_PATH"
 then
-	test_skip_or_die $GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
+	test_skip_or_die GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
 fi
 
 HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \
@@ -107,19 +106,19 @@ then
 	then
 		if ! test $HTTPD_VERSION -ge 2
 		then
-			test_skip_or_die $GIT_TEST_HTTPD \
+			test_skip_or_die GIT_TEST_HTTPD \
 				"at least Apache version 2 is required"
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
-			test_skip_or_die $GIT_TEST_HTTPD \
+			test_skip_or_die GIT_TEST_HTTPD \
 				"Apache module directory not found"
 		fi
 
 		LIB_HTTPD_MODULE_PATH="$DEFAULT_HTTPD_MODULE_PATH"
 	fi
 else
-	test_skip_or_die $GIT_TEST_HTTPD \
+	test_skip_or_die GIT_TEST_HTTPD \
 		"Could not identify web server at '$LIB_HTTPD_PATH'"
 fi
 
@@ -184,7 +183,7 @@ start_httpd() {
 	if test $? -ne 0
 	then
 		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
-		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
+		test_skip_or_die GIT_TEST_HTTPD "web server setup failed"
 	fi
 }
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index e3c4a48c85..7161148280 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -267,8 +267,7 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 '
 
 test_lazy_prereq GIT_DAEMON '
-	test_tristate GIT_TEST_GIT_DAEMON &&
-	test "$GIT_TEST_GIT_DAEMON" != false
+	git env--helper --mode-bool --variable=GIT_TEST_GIT_DAEMON --default=1 --exit-code --quiet
 '
 
 # This test spawns a daemon, so run it only if the user would be OK with
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0367cec5fd..527508c350 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1035,62 +1035,20 @@ perl () {
 	command "$PERL_PATH" "$@" 2>&7
 } 7>&2 2>&4
 
-# Is the value one of the various ways to spell a boolean true/false?
-test_normalize_bool () {
-	git -c magic.variable="$1" config --bool magic.variable 2>/dev/null
-}
-
-# Given a variable $1, normalize the value of it to one of "true",
-# "false", or "auto" and store the result to it.
-#
-#     test_tristate GIT_TEST_HTTPD
-#
-# A variable set to an empty string is set to 'false'.
-# A variable set to 'false' or 'auto' keeps its value.
-# Anything else is set to 'true'.
-# An unset variable defaults to 'auto'.
-#
-# The last rule is to allow people to set the variable to an empty
-# string and export it to decline testing the particular feature
-# for versions both before and after this change.  We used to treat
-# both unset and empty variable as a signal for "do not test" and
-# took any non-empty string as "please test".
-
-test_tristate () {
-	if eval "test x\"\${$1+isset}\" = xisset"
-	then
-		# explicitly set
-		eval "
-			case \"\$$1\" in
-			'')	$1=false ;;
-			auto)	;;
-			*)	$1=\$(test_normalize_bool \$$1 || echo true) ;;
-			esac
-		"
-	else
-		eval "$1=auto"
-	fi
-}
-
 # Exit the test suite, either by skipping all remaining tests or by
-# exiting with an error. If "$1" is "auto", we then we assume we were
-# opportunistically trying to set up some tests and we skip. If it is
-# "true", then we report a failure.
+# exiting with an error. If our prerequisite variable $1 falls back
+# on a default assume we were opportunistically trying to set up some
+# tests and we skip. If it is explicitly "true", then we report a failure.
 #
 # The error/skip message should be given by $2.
 #
 test_skip_or_die () {
-	case "$1" in
-	auto)
+	if ! git env--helper --mode-bool --variable=$1 --default=0 --exit-code --quiet
+	then
 		skip_all=$2
 		test_done
-		;;
-	true)
-		error "$2"
-		;;
-	*)
-		error "BUG: test tristate is '$1' (real error: $2)"
-	esac
+	fi
+	error "$2"
 }
 
 # The following mingw_* functions obey POSIX shell syntax, but are actually
-- 
2.22.0.455.g172b71a6c5

[PATCH v2 8/8] tests: make GIT_TEST_FAIL_PREREQS a boolean

[Ævar Arnfjörð Bjarmason]

Change the GIT_TEST_FAIL_PREREQS variable from being "non-empty?" to
being a more standard boolean variable. I recently added the variable
in dfe1a17df9 ("tests: add a special setup where prerequisites fail",
2019-05-13), having to add another "non-empty?" special-case is what
prompted me to write the "git env--helper" utility being used here.

Converting this one is a bit tricky since we use it so early and
frequently in the guts of the test code itself, so let's set a
GIT_TEST_FAIL_PREREQS_INTERNAL which can be tested with the old "test
-n" for the purposes of the shell code, and change the user-exposed
and documented GIT_TEST_FAIL_PREREQS variable to a boolean.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README                |  2 +-
 t/t0000-basic.sh        | 10 +++++-----
 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 25 +++++++++++++++++++++----
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/t/README b/t/README
index 072c9854d1..60d5b77bcc 100644
--- a/t/README
+++ b/t/README
@@ -334,7 +334,7 @@ that cannot be easily covered by a few specific test cases. These
 could be enabled by running the test suite with correct GIT_TEST_
 environment set.
 
-GIT_TEST_FAIL_PREREQS<non-empty?> fails all prerequisites. This is
+GIT_TEST_FAIL_PREREQS=<boolean> fails all prerequisites. This is
 useful for discovering issues with the tests where say a later test
 implicitly depends on an optional earlier test.
 
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 31de7e90f3..e89438e619 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -726,7 +726,7 @@ donthaveit=yes
 test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be skipped' '
 	donthaveit=no
 '
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit != yesyes
 then
 	say "bug in test framework: prerequisite tags do not work reliably"
 	exit 1
@@ -747,7 +747,7 @@ donthaveiteither=yes
 test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '
 	donthaveiteither=no
 '
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit$donthaveiteither != yesyesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit$donthaveiteither != yesyesyes
 then
 	say "bug in test framework: multiple prerequisite tags do not work reliably"
 	exit 1
@@ -763,7 +763,7 @@ test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
 	donthavetrue=no
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a "$havetrue$donthavetrue" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a "$havetrue$donthavetrue" != yesyes
 then
 	say 'bug in test framework: lazy prerequisites do not work'
 	exit 1
@@ -779,7 +779,7 @@ test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
 	havefalse=no
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a "$nothavefalse$havefalse" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a "$nothavefalse$havefalse" != yesyes
 then
 	say 'bug in test framework: negative lazy prerequisites do not work'
 	exit 1
@@ -790,7 +790,7 @@ test_expect_success 'tests clean up after themselves' '
 	test_when_finished clean=yes
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $clean != yes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $clean != yes
 then
 	say "bug in test framework: basic cleanup command does not work reliably"
 	exit 1
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 527508c350..3fba71c358 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -309,7 +309,7 @@ test_unset_prereq () {
 }
 
 test_set_prereq () {
-	if test -n "$GIT_TEST_FAIL_PREREQS"
+	if test -n "$GIT_TEST_FAIL_PREREQS_INTERNAL"
 	then
 		case "$1" in
 		# The "!" case is handled below with
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c45b0d2611..238ef62401 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1389,6 +1389,27 @@ yes () {
 	done
 }
 
+# The GIT_TEST_FAIL_PREREQS code hooks into test_set_prereq(), and
+# thus needs to be set up really early, and set an internal variable
+# for convenience so the hot test_set_prereq() codepath doesn't need
+# to call "git env--helper". Only do that work if needed by seeing if
+# GIT_TEST_FAIL_PREREQS is set at all.
+GIT_TEST_FAIL_PREREQS_INTERNAL=
+if test -n "$GIT_TEST_FAIL_PREREQS"
+then
+	if git env--helper --mode-bool --variable=GIT_TEST_FAIL_PREREQS \
+		--default=0 --exit-code --quiet
+	then
+		GIT_TEST_FAIL_PREREQS_INTERNAL=true
+		test_set_prereq FAIL_PREREQS
+	fi
+else
+	test_lazy_prereq FAIL_PREREQS '
+		git env--helper --mode-bool --variable=GIT_TEST_FAIL_PREREQS \
+			--default=0 --exit-code --quiet
+	'
+fi
+
 # Fix some commands on Windows
 uname_s=$(uname -s)
 case $uname_s in
@@ -1605,7 +1626,3 @@ test_lazy_prereq SHA1 '
 test_lazy_prereq REBASE_P '
 	test -z "$GIT_TEST_SKIP_REBASE_P"
 '
-
-test_lazy_prereq FAIL_PREREQS '
-	test -n "$GIT_TEST_FAIL_PREREQS"
-'
-- 
2.22.0.455.g172b71a6c5

Re: [RFC/PATCH] gc: run more pre-detach operations under lock

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 20 2019, Duy Nguyen wrote:

> On Thu, Jun 20, 2019 at 5:49 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Wed, Jun 19 2019, Jeff King wrote:
>>
>> > On Wed, Jun 19, 2019 at 08:01:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> > You could sort of avoid the problem here too with
>> >> >
>> >> > parallel 'git fetch --no-auto-gc {}' ::: $(git remote)
>> >> > git gc --auto
>> >> >
>> >> > It's definitely simpler, but of course we have to manually add
>> >> > --no-auto-gc in everywhere we need, so not quite as elegant.
>> >> >
>> >> > Actually you could already do that with 'git -c gc.auto=false fetch', I guess.
>> >>
>> >> The point of the 'parallel' example is to show disconnected git
>> >> commands, think trying to run 'git' in a terminal while your editor
>> >> asynchronously runs a polling 'fetch', or a server with multiple
>> >> concurrent clients running 'gc --auto'.
>> >>
>> >> That's the question my RFC patch raises. As far as I can tell the
>> >> approach in your patch is only needed because our locking for gc is
>> >> buggy, rather than introduce the caveat that an fetch(N) operation won't
>> >> do "gc" until it's finished (we may have hundreds, thousands of remotes,
>> >> I use that for some more obscure use-cases) shouldn't we just fix the
>> >> locking?
>> >
>> > I think there may be room for both approaches. Yours fixes the repeated
>> > message in the more general case, but Duy's suggestion is the most
>> > efficient thing.
>> >
>> > I agree that the "thousands of remotes" case means we might want to gc
>> > in the interim. But we probably ought to do that deterministically
>> > rather than hoping that the pattern of lock contention makes sense.
>>
>> We do it deterministically, when gc.auto thresholds et al are exceeded
>> we kick one off without waiting for other stuff, if we can get the lock.
>>
>> I don't think this desire to just wait a bit until all the fetches are
>> complete makes sense as a special-case.
>>
>> If, as you noted in <20190619190845.GD28145@sigill.intra.peff.net>, the
>> desire is to reduce GC CPU use then you're better off just tweaking the
>> limits upwards. Then you get that with everything, like when you run
>> "commit" in a for-loop, not just this one special case of "fetch".
>>
>> We have existing potentially long-running operations like "fetch",
>> "rebase" and "git svn fetch" that run "gc --auto" for their incremental
>> steps, and that's a feature.
>
> gc --auto is added at arbitrary points to help garbage collection. I
> don't think it's ever intended to "do gc at this and that exact
> moment", just "hey this command has taken a lot of time already (i.e.
> no instant response needed) and it may have added a bit more garbage,
> let's just check real quick".

I don't mean we can't ever change the algorithm, but that we've
documented:

    When common porcelain operations that create objects are run, they
    will check whether the repository has grown substantially since the
    last maintenance[...]

The "fetch" command is a common porcelain operation, when it fetches
from N remotes it just runs an invocation of itself, so thus far it's
both worked & been intuitive that if we needed (potentially multiple)
gc's while doing that we'd just go ahead and run it then, even if
something concurrent was happening.

No that's not optimal in many cases, but at least doesn't create caveats
we don't have now where we have runaway object growth.

>> It keeps "gc --auto" dumb enough to avoid a pathological case where
>> we'll have a ballooning objects dir because we figure we can run
>> something "at the end", when "the end" could be hours away, and we're
>> adding a new pack or hundreds of loose objects every second.
>
> Are we optimizing for a rare (large scale) case? Such setup requires
> tuning regardless to me.

At least for me it doesn't require custom tuning before this patch of
yours.

I.e. now "gc --auto" is dumb enough that you can run it on everything
from stuff that just does "commit" from cron, user's laptops, massive
rebases that take forever, and e.g. "stats" like jobs where for
<reasons> I'll add thousands of repos and "fetch --all" them (so e.g. I
can run "log --author=<x> --all").

Yeah of course I'm an advanced user and I can just grumble and manually
invoke fetch, actually I'll probably submit a follow-up patch to add a
gc.* config to disable this thing.

But I think even if the use-case is rather obscure it's important that
if at all possible we keep "gc" elastic enough to work for pretty much
all combinations of object-adding porcelain commands, and I think in
this case we're better off doing things differently...

>> So I don't think Duy's patch is a good way to go.
>
> This reminds me of being perfect is the enemy of the good. A normal
> user has a couple remotes at most, finishing fast (enough) and in such
> case it's a good idea to wait until everything is in before running
> gc.

Even a user with two remotes will run into issues with your patch where
"gc" will print things twice, or outright error due to concurrent access
by another process, which as has been discussed here on-list is *very*
common e.g. with editor integration.

So the extent of my complains about this is:

 1) The edge case with runaway object growth (obscure)

 2) It doesn't really fix the bug except for the narrow case of users
    who invoke things one-terminal-at-a-time and don't e.g. have an
    editor with "git" integration & a terminal (not obscure).

Maybe I should have led with #2 :)

Anyway, I'm not *just* complaining. I have patches too, but so far I'm
up to 8 patches on what's probably just the first one-third of
it. *Sigh*.

But the 2/3 of that if you want to dig through my crappy WIP code on
GitHub is instrumenting the test suite to demonstrate that with an
approach like what your patch does we still get these GC race
conditions, because our locking still sucks, which brings me to...

> Of course making git-gc more robust wrt. parallel access is great, but
> it's hard work. Dealing with locks is always tricky, especially when
> new locks can come up any time.

I've poked at it a bit now, and it's really not hard, I think it's just
that nobody looked at it hard enough before.

The issue is that currently we do:

    1. parent: do_we_need_gc();
    2. parent: say_way_will_gc();
    3. parent: lock();
    4. parent: do_a_bit_of_work();
    5. parent: unlock();
    6. parent: fork();
    7. child: lock();
    8. child: do_a_lot_of_work();
    9. child: unlock();

My RFC patch currently changes that to:

    1. parent: do_we_need_gc();
    2. parent: lock_or_silently_exit();
    3. parent: say_way_will_gc();
    4. parent: do_a_bit_of_work();
    5. parent: unlock();
    6. parent: fork();
    7. child: lock();
    8. child: do_a_lot_of_work();
    9. child: unlock();

I.e. we won't duplicate the message, but *do* introduce the caveat that
it's in principle possible nobody gc's, but in practice when we fail to
get the lock in lock_or_silently_exit() it's because we lost the race to
a sister process that's going to do the actual GC, so all is well.

But we are left with the brief race when we fork. My RFC patch proposed
some elaborate PID hand-over dance to deal with this. But having looked
at it again I think we can easily get rid of that race too with just:

    1. parent: do_we_need_gc();
    2. parent: lock_or_silently_exit();
    3. parent: say_way_will_gc();
    4. parent: do_a_bit_of_work();
    5. parent: fork();
    6. parent: <writes child pid to gc.lock>
    7. parent: <exits without unlocking gc.lock>
    8. child: do_a_lot_of_work();
    9. child: unlock();

Which can fail in cases where the child segfaults, or manages to exit
earlier than the parent etc, hence my earlier proposed elaborate pid
hand-over dance.

But looking at it again we only usurp an existing gc.lock if the mtime
is >12hrs, so it's OK if we have very rare cases where the PID info got
corrupted, we can still back ourselves out of it, which is what I was
paranoid about.

Furthermore the gc.lock contains the <hostname><pid> of the working
process, but we can in a backwards-compatible way add new entries to
that file, i.e. list both the child & parent pid. Older clients will
just read whichever one comes first, but if we make new versions check
both we can be more paranoid going forward.

> Having said that, I don't mind if my patch gets dropped. It was just a
> "hey that multiple gc output looks strange, hah the fix is quite
> simple" moment for me.

Re: [PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +	switch (cmdmode) {
> +	case ENV_HELPER_BOOL:
> +		tmp_int = strtol(env_default, (char **)&env_default, 10);
> +		if (*env_default) {
> +			error(_("option `--default' expects a numerical value with `--mode-bool`"));
> +			usage_with_options(env__helper_usage, opts);
> +		}
> +		ret_int = git_env_bool(env_variable, tmp_int);
> +		if (!quiet)
> +			printf("%d\n", ret_int);
> +		ret = ret_int;
> +		break;
> +	case ENV_HELPER_ULONG:
> +		tmp_ulong = strtoll(env_default, (char **)&env_default, 10);
> +		if (*env_default) {
> +			error(_("option `--default' expects a numerical value with `--mode-ulong`"));
> +			usage_with_options(env__helper_usage, opts);
> +		}
> +		ret_ulong = git_env_ulong(env_variable, tmp_ulong);
> +		if (!quiet)
> +			printf("%lu\n", ret_ulong);
> +		ret = ret_ulong;
> +		break;

Perhaps have something like 'default: BUG("wrong cmdmode");'

> +	}
> +
> +	if (exit_code)
> +		return !ret;

as I am getting

    error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]

from here.

Giving an otherwise useless initial value to ret would be a
workaround.

Re: [PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...
> as I am getting
>
>     error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> from here.
>
> Giving an otherwise useless initial value to ret would be a
> workaround.

I've added this on top of the topic before merging to keep the
integration going at least for now.

commit 8f86948797a1152594a8dee50d0878604fec3e80
Author: Junio C Hamano <gitster@pobox.com>
Date:   Thu Jun 20 15:13:14 2019 -0700

    SQUASH??? avoid maybe-uninitialized

diff --git a/builtin/env--helper.c b/builtin/env--helper.c
index 2bb65ecf3f..29df0567fb 100644
--- a/builtin/env--helper.c
+++ b/builtin/env--helper.c
@@ -43,6 +43,9 @@ int cmd_env__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(env__helper_usage, opts);
 
 	switch (cmdmode) {
+	default:
+		BUG("wrong cmdmode");
+		break;
 	case ENV_HELPER_BOOL:
 		tmp_int = strtol(env_default, (char **)&env_default, 10);
 		if (*env_default) {

Re: [PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 21 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> ...
>> as I am getting
>>
>>     error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> from here.
>>
>> Giving an otherwise useless initial value to ret would be a
>> workaround.
>
> I've added this on top of the topic before merging to keep the
> integration going at least for now.
>
> commit 8f86948797a1152594a8dee50d0878604fec3e80
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Thu Jun 20 15:13:14 2019 -0700
>
>     SQUASH??? avoid maybe-uninitialized
>
> diff --git a/builtin/env--helper.c b/builtin/env--helper.c
> index 2bb65ecf3f..29df0567fb 100644
> --- a/builtin/env--helper.c
> +++ b/builtin/env--helper.c
> @@ -43,6 +43,9 @@ int cmd_env__helper(int argc, const char **argv, const char *prefix)
>  		usage_with_options(env__helper_usage, opts);
>
>  	switch (cmdmode) {
> +	default:
> +		BUG("wrong cmdmode");
> +		break;
>  	case ENV_HELPER_BOOL:
>  		tmp_int = strtol(env_default, (char **)&env_default, 10);
>  		if (*env_default) {

In this case the compiler is wrong, and gcc/clang in e.g. Debian
unstable doesn't warn about this since the analyzer sees that it's
impossible for "ret" to be uninitialized.

I can change it anyway, and if I rewrite the UI of this command it might
go away anyway.

Just thought I'd ask if appeasing older analyzers is what we want for
these sorts of optional warnings in general.

[PATCH v3 0/8] Change <non-empty?> GIT_TEST_* variables to <boolean>

[Ævar Arnfjörð Bjarmason]

Now with:

 * The --type=bool etc. Ui change to env--bool.

 * I considered supporting YesPlease for gettext poison, but didn't go
   for it. Details in updated commit message.

 * "default" "case" arm fork warning on some compilers.

Ævar Arnfjörð Bjarmason (8):
  config tests: simplify include cycle test
  env--helper: new undocumented builtin wrapping git_env_*()
  config.c: refactor die_bad_number() to not call gettext() early
  t6040 test: stop using global "script" variable
  tests: make GIT_TEST_GETTEXT_POISON a boolean
  tests README: re-flow a previously changed paragraph
  tests: replace test_tristate with "git env--helper"
  tests: make GIT_TEST_FAIL_PREREQS a boolean

 .gitignore                |  1 +
 Makefile                  |  1 +
 builtin.h                 |  1 +
 builtin/env--helper.c     | 95 +++++++++++++++++++++++++++++++++++++
 ci/lib.sh                 |  2 +-
 config.c                  | 28 +++++++----
 gettext.c                 |  6 +--
 git-sh-i18n.sh            |  4 +-
 git.c                     |  1 +
 po/README                 |  2 +-
 t/README                  | 12 ++---
 t/lib-git-daemon.sh       |  7 ++-
 t/lib-git-svn.sh          | 11 ++---
 t/lib-httpd.sh            | 15 +++---
 t/t0000-basic.sh          | 10 ++--
 t/t0017-env-helper.sh     | 99 +++++++++++++++++++++++++++++++++++++++
 t/t0205-gettext-poison.sh |  7 ++-
 t/t1305-config-include.sh | 21 +++------
 t/t5512-ls-remote.sh      |  3 +-
 t/t6040-tracking-info.sh  |  6 +--
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib-functions.sh   | 58 ++++-------------------
 t/test-lib.sh             | 31 ++++++++----
 24 files changed, 298 insertions(+), 127 deletions(-)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0017-env-helper.sh

Range-diff:
1:  c3483c37a1 = 1:  c3483c37a1 config tests: simplify include cycle test
2:  e689759f7c ! 2:  39cb96739a env--helper: new undocumented builtin wrapping git_env_*()
    @@ -20,9 +20,11 @@
         default value makes all the difference, and we'll be able to replace
         test_tristate() itself with that.
     
    -    The --mode-bool option will be used by subsequent patches, but not
    -    --mode-ulong. I figured it was easy enough to add it & test for it so
    -    I left it in so we'd have wrappers for both git_env_*() functions.
    +    The --type=bool option will be used by subsequent patches, but not
    +    --type=ulong. I figured it was easy enough to add it & test for it so
    +    I left it in so we'd have wrappers for both git_env_*() functions, and
    +    to have a template to make it obvious how we'd add --type=int etc. if
    +    it's needed in the future.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -72,74 +74,95 @@
     +#include "parse-options.h"
     +
     +static char const * const env__helper_usage[] = {
    -+	N_("git env--helper [--mode-bool | --mode-ulong] --env-variable=<VAR> --env-default=<DEF> [<options>]"),
    ++	N_("git env--helper --type=[bool|ulong] <options> <env-var>"),
     +	NULL
     +};
     +
    ++enum {
    ++	ENV_HELPER_TYPE_BOOL = 1,
    ++	ENV_HELPER_TYPE_ULONG
    ++} cmdmode = 0;
    ++
    ++static int option_parse_type(const struct option *opt, const char *arg,
    ++			     int unset)
    ++{
    ++	if (!strcmp(arg, "bool"))
    ++		cmdmode = ENV_HELPER_TYPE_BOOL;
    ++	else if (!strcmp(arg, "ulong"))
    ++		cmdmode = ENV_HELPER_TYPE_ULONG;
    ++	else
    ++		die(_("unrecognized --type argument, %s"), arg);
    ++
    ++	return 0;
    ++}
    ++
     +int cmd_env__helper(int argc, const char **argv, const char *prefix)
     +{
    -+	enum {
    -+		ENV_HELPER_BOOL = 1,
    -+		ENV_HELPER_ULONG,
    -+	} cmdmode = 0;
     +	int exit_code = 0;
    -+	int quiet = 0;
     +	const char *env_variable = NULL;
     +	const char *env_default = NULL;
     +	int ret;
    -+	int ret_int, tmp_int;
    -+	unsigned long ret_ulong, tmp_ulong;
    ++	int ret_int, default_int;
    ++	unsigned long ret_ulong, default_ulong;
     +	struct option opts[] = {
    -+		OPT_CMDMODE(0, "mode-bool", &cmdmode,
    -+			    N_("invoke git_env_bool(...)"), ENV_HELPER_BOOL),
    -+		OPT_CMDMODE(0, "mode-ulong", &cmdmode,
    -+			    N_("invoke git_env_ulong(...)"), ENV_HELPER_ULONG),
    -+		OPT_STRING(0, "variable", &env_variable, N_("name"),
    -+			   N_("which environment variable to ask git_env_*(...) about")),
    ++		OPT_CALLBACK_F(0, "type", &cmdmode, N_("type"),
    ++			       N_("value is given this type"), PARSE_OPT_NONEG,
    ++			       option_parse_type),
     +		OPT_STRING(0, "default", &env_default, N_("value"),
    -+			   N_("what default value does git_env_*(...) fall back on?")),
    ++			   N_("default for git_env_*(...) to fall back on")),
     +		OPT_BOOL(0, "exit-code", &exit_code,
    -+			 N_("exit code determined by truth of the git_env_*() function")),
    -+		OPT_BOOL(0, "quiet", &quiet,
    -+			 N_("don't print the git_env_*() return value")),
    ++			 N_("be quiet only use git_env_*() value as exit code")),
     +		OPT_END(),
     +	};
     +
    -+	if (parse_options(argc, argv, prefix, opts, env__helper_usage, 0))
    ++	argc = parse_options(argc, argv, prefix, opts, env__helper_usage,
    ++			     PARSE_OPT_KEEP_UNKNOWN);
    ++	if (env_default && !*env_default)
     +		usage_with_options(env__helper_usage, opts);
    -+	if (!env_variable || !env_default ||
    -+	    !*env_variable || !*env_default)
    ++	if (!cmdmode)
     +		usage_with_options(env__helper_usage, opts);
    ++	if (argc != 1)
    ++		usage_with_options(env__helper_usage, opts);
    ++	env_variable = argv[0];
     +
     +	switch (cmdmode) {
    -+	case ENV_HELPER_BOOL:
    -+		tmp_int = strtol(env_default, (char **)&env_default, 10);
    -+		if (*env_default) {
    -+			error(_("option `--default' expects a numerical value with `--mode-bool`"));
    -+			usage_with_options(env__helper_usage, opts);
    ++	case ENV_HELPER_TYPE_BOOL:
    ++		if (env_default) {
    ++			default_int = git_parse_maybe_bool(env_default);
    ++			if (default_int == -1) {
    ++				error(_("option `--default' expects a boolean value with `--type=bool`, not `%s`"),
    ++				      env_default);
    ++				usage_with_options(env__helper_usage, opts);
    ++			}
    ++		} else {
    ++			default_int = 0;
     +		}
    -+		ret_int = git_env_bool(env_variable, tmp_int);
    -+		if (!quiet)
    -+			printf("%d\n", ret_int);
    ++		ret_int = git_env_bool(env_variable, default_int);
    ++		if (!exit_code)
    ++			puts(ret_int ? "true" : "false");
     +		ret = ret_int;
     +		break;
    -+	case ENV_HELPER_ULONG:
    -+		tmp_ulong = strtoll(env_default, (char **)&env_default, 10);
    -+		if (*env_default) {
    -+			error(_("option `--default' expects a numerical value with `--mode-ulong`"));
    -+			usage_with_options(env__helper_usage, opts);
    ++	case ENV_HELPER_TYPE_ULONG:
    ++		if (env_default) {
    ++			if (!git_parse_ulong(env_default, &default_ulong)) {
    ++				error(_("option `--default' expects an unsigned long value with `--type=ulong`, not `%s`"),
    ++				      env_default);
    ++				usage_with_options(env__helper_usage, opts);
    ++			}
    ++		} else {
    ++			default_ulong = 0;
     +		}
    -+		ret_ulong = git_env_ulong(env_variable, tmp_ulong);
    -+		if (!quiet)
    ++		ret_ulong = git_env_ulong(env_variable, default_ulong);
    ++		if (!exit_code)
     +			printf("%lu\n", ret_ulong);
     +		ret = ret_ulong;
     +		break;
    ++	default:
    ++		BUG("unknown <type> value");
    ++		break;
     +	}
     +
    -+	if (exit_code)
    -+		return !ret;
    -+
    -+	return 0;
    ++	return !ret;
     +}
     
      diff --git a/git.c b/git.c
    @@ -168,64 +191,77 @@
     +
     +test_expect_success 'env--helper usage' '
     +	test_must_fail git env--helper &&
    -+	test_must_fail git env--helper --mode-bool &&
    -+	test_must_fail git env--helper --mode-ulong &&
    -+	test_must_fail git env--helper --mode-bool --variable &&
    -+	test_must_fail git env--helper --mode-bool --variable --default &&
    -+	test_must_fail git env--helper --mode-bool --variable= --default=
    ++	test_must_fail git env--helper --type=bool &&
    ++	test_must_fail git env--helper --type=ulong &&
    ++	test_must_fail git env--helper --type=bool &&
    ++	test_must_fail git env--helper --type=bool --default &&
    ++	test_must_fail git env--helper --type=bool --default= &&
    ++	test_must_fail git env--helper --defaultxyz
     +'
     +
     +test_expect_success 'env--helper bad default values' '
    -+	test_must_fail git env--helper --mode-bool --variable=MISSING --default=1xyz &&
    -+	test_must_fail git env--helper --mode-ulong --variable=MISSING --default=1xyz
    ++	test_must_fail git env--helper --type=bool --default=1xyz MISSING &&
    ++	test_must_fail git env--helper --type=ulong --default=1xyz MISSING
     +'
     +
    -+test_expect_success 'env--helper --mode-bool' '
    -+	echo 1 >expected &&
    -+	git env--helper --mode-bool --variable=MISSING --default=1 --exit-code >actual &&
    ++test_expect_success 'env--helper --type=bool' '
    ++	# Test various --default bool values
    ++	echo true >expected &&
    ++	git env--helper --type=bool --default=1 MISSING >actual &&
     +	test_cmp expected actual &&
    -+
    -+	echo 0 >expected &&
    -+	test_must_fail git env--helper --mode-bool --variable=MISSING --default=0 --exit-code >actual &&
    ++	git env--helper --type=bool --default=yes MISSING >actual &&
     +	test_cmp expected actual &&
    -+
    -+	git env--helper --mode-bool --variable=MISSING --default=0 >actual &&
    ++	git env--helper --type=bool --default=true MISSING >actual &&
     +	test_cmp expected actual &&
    -+
    -+	>expected &&
    -+	git env--helper --mode-bool --variable=MISSING --default=1 --exit-code --quiet >actual &&
    ++	echo false >expected &&
    ++	test_must_fail git env--helper --type=bool --default=0 MISSING >actual &&
     +	test_cmp expected actual &&
    -+
    -+	EXISTS=true git env--helper --mode-bool --variable=EXISTS --default=0 --exit-code --quiet >actual &&
    ++	test_must_fail git env--helper --type=bool --default=no MISSING >actual &&
    ++	test_cmp expected actual &&
    ++	test_must_fail git env--helper --type=bool --default=false MISSING >actual &&
     +	test_cmp expected actual &&
     +
    -+	echo 1 >expected &&
    -+	EXISTS=true git env--helper --mode-bool --variable=EXISTS --default=0 --exit-code >actual &&
    -+	test_cmp expected actual
    ++	# No output with --exit-code
    ++	git env--helper --type=bool --default=true --exit-code MISSING >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_must_be_empty actual.err &&
    ++	test_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_must_be_empty actual.err &&
    ++
    ++	# Existing variable
    ++	EXISTS=true git env--helper --type=bool --default=false --exit-code EXISTS >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_must_be_empty actual.err &&
    ++	test_must_fail \
    ++		env EXISTS=false \
    ++		git env--helper --type=bool --default=true --exit-code EXISTS >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_must_be_empty actual.err
     +'
     +
    -+test_expect_success 'env--helper --mode-ulong' '
    ++test_expect_success 'env--helper --type=ulong' '
     +	echo 1234567890 >expected &&
    -+	git env--helper --mode-ulong --variable=MISSING --default=1234567890 --exit-code >actual &&
    -+	test_cmp expected actual &&
    ++	git env--helper --type=ulong --default=1234567890 MISSING >actual.out 2>actual.err &&
    ++	test_cmp expected actual.out &&
    ++	test_must_be_empty actual.err &&
     +
     +	echo 0 >expected &&
    -+	test_must_fail git env--helper --mode-ulong --variable=MISSING --default=0 --exit-code >actual &&
    ++	test_must_fail git env--helper --type=ulong --default=0 MISSING >actual &&
     +	test_cmp expected actual &&
     +
    -+	git env--helper --mode-ulong --variable=MISSING --default=0 >actual &&
    -+	test_cmp expected actual &&
    ++	git env--helper --type=ulong --default=1234567890 --exit-code MISSING >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_must_be_empty actual.err &&
     +
    -+	>expected &&
    -+	git env--helper --mode-ulong --variable=MISSING --default=1234567890 --exit-code --quiet >actual &&
    -+	test_cmp expected actual &&
    -+
    -+	EXISTS=1234567890 git env--helper --mode-ulong --variable=EXISTS --default=0 --exit-code --quiet >actual &&
    -+	test_cmp expected actual &&
    ++	EXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS --exit-code >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_must_be_empty actual.err &&
     +
     +	echo 1234567890 >expected &&
    -+	EXISTS=1234567890 git env--helper --mode-ulong --variable=EXISTS --default=0 --exit-code >actual &&
    -+	test_cmp expected actual
    ++	EXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS >actual.out 2>actual.err &&
    ++	test_cmp expected actual.out &&
    ++	test_must_be_empty actual.err
     +'
     +
     +test_done
3:  f759d5e91e = 3:  8a88f263bf config.c: refactor die_bad_number() to not call gettext() early
4:  1ac798e8ce = 4:  6594a95d6e t6040 test: stop using global "script" variable
5:  d7d6e6c874 ! 5:  248c0f9d46 tests: make GIT_TEST_GETTEXT_POISON a boolean
    @@ -17,7 +17,7 @@
         the special case of GIT_TEST_GETTEXT_POISON and always emit that
         message in the C locale, lest we infinitely loop.
     
    -    As seen in the updated tests in t0016-env-helper.sh there's also a
    +    As seen in the updated tests in t0017-env-helper.sh there's also a
         caveat related to "env--helper" needing to read the config for trace2
         purposes.
     
    @@ -32,6 +32,21 @@
         change in this series won't happen again, and testing for this
         explicitly in "env--helper"'s own tests.
     
    +    This change breaks existing uses of
    +    e.g. GIT_TEST_GETTEXT_POISON=YesPlease, which we've documented in
    +    po/README and other places. As noted in [1] we might want to consider
    +    also accepting "YesPlease" in "env--helper" as a special-case.
    +
    +    But as the lack of uproar over 6cdccfce1e ("i18n: make GETTEXT_POISON
    +    a runtime option", 2018-11-08) demonstrates the audience for this
    +    option is a really narrow set of git developers, who shouldn't have
    +    much trouble modifying their test scripts, so I think it's better to
    +    deal with that minor headache now and make all the relevant GIT_TEST_*
    +    variables boolean in the same way than carry the "YesPlease"
    +    special-case forward.
    +
    +    1. https://public-inbox.org/git/xmqqtvckm3h8.fsf@gitster-ct.c.googlers.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      diff --git a/ci/lib.sh b/ci/lib.sh
    @@ -93,8 +108,8 @@
      GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
     -if test -n "$GIT_TEST_GETTEXT_POISON"
     +if test -n "$GIT_TEST_GETTEXT_POISON" &&
    -+	    git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON \
    -+		--default=0 --exit-code --quiet
    ++	    git env--helper --type=bool --default=0 --exit-code \
    ++		GIT_TEST_GETTEXT_POISON
      then
      	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
      elif test -n "@@USE_GETTEXT_SCHEME@@"
    @@ -131,7 +146,7 @@
      --- a/t/t0017-env-helper.sh
      +++ b/t/t0017-env-helper.sh
     @@
    - 	test_cmp expected actual
    + 	test_must_be_empty actual.err
      '
      
     +test_expect_success 'env--helper reads config thanks to trace2' '
    @@ -146,7 +161,7 @@
     +
     +	test_must_fail \
     +		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
    -+		git -C cycle env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet 2>err &&
    ++		git -C cycle env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON 2>err &&
     +	grep "# GETTEXT POISON #" err
     +'
     +
    @@ -227,7 +242,7 @@
     -	test_set_prereq C_LOCALE_OUTPUT
     -fi
     +test_lazy_prereq C_LOCALE_OUTPUT '
    -+	! git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet
    ++	! git env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON
     +'
      
      if test -z "$GIT_TEST_CHECK_CACHE_TREE"
6:  954428b3cd = 6:  c9d44aa306 tests README: re-flow a previously changed paragraph
7:  79b41cf01b ! 7:  b0bab6adc8 tests: replace test_tristate with "git env--helper"
    @@ -35,7 +35,7 @@
      
     -test_tristate GIT_TEST_GIT_DAEMON
     -if test "$GIT_TEST_GIT_DAEMON" = false
    -+if ! git env--helper --mode-bool --variable=GIT_TEST_GIT_DAEMON --default=1 --exit-code --quiet
    ++if ! git env--helper --type=bool --default=true --exit-code GIT_TEST_GIT_DAEMON
      then
      	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
      	test_done
    @@ -68,7 +68,7 @@
     -	test_tristate GIT_SVN_TEST_HTTPD
     -	case $GIT_SVN_TEST_HTTPD in
     -	true)
    -+	if git env--helper --mode-bool --variable=GIT_TEST_HTTPD --default=0 --exit-code --quiet	
    ++	if git env--helper --type=bool --default=false --exit-code GIT_TEST_HTTPD
     +	then
      		. "$TEST_DIRECTORY"/lib-httpd.sh
      		LIB_HTTPD_SVN="$loc"
    @@ -85,7 +85,7 @@
      require_svnserve () {
     -	test_tristate GIT_TEST_SVNSERVE
     -	if ! test "$GIT_TEST_SVNSERVE" = true
    -+	if ! git env--helper --mode-bool --variable=GIT_TEST_SVNSERVE --default=0 --exit-code --quiet
    ++	if ! git env--helper --type=bool --default=false --exit-code GIT_TEST_SVNSERVE
      	then
      		skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to enable)'
      		test_done
    @@ -99,7 +99,7 @@
      
     -test_tristate GIT_TEST_HTTPD
     -if test "$GIT_TEST_HTTPD" = false
    -+if ! git env--helper --mode-bool --variable=GIT_TEST_HTTPD --default=1 --exit-code --quiet
    ++if ! git env--helper --type=bool --default=true --exit-code GIT_TEST_HTTPD
      then
      	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
      	test_done
    @@ -162,7 +162,7 @@
      test_lazy_prereq GIT_DAEMON '
     -	test_tristate GIT_TEST_GIT_DAEMON &&
     -	test "$GIT_TEST_GIT_DAEMON" != false
    -+	git env--helper --mode-bool --variable=GIT_TEST_GIT_DAEMON --default=1 --exit-code --quiet
    ++	git env--helper --type=bool --default=true --exit-code GIT_TEST_GIT_DAEMON
      '
      
      # This test spawns a daemon, so run it only if the user would be OK with
8:  a9aa166b66 ! 8:  d1a32debba tests: make GIT_TEST_FAIL_PREREQS a boolean
    @@ -90,6 +90,15 @@
      	then
      		case "$1" in
      		# The "!" case is handled below with
    +@@
    + # The error/skip message should be given by $2.
    + #
    + test_skip_or_die () {
    +-	if ! git env--helper --mode-bool --variable=$1 --default=0 --exit-code --quiet
    ++	if ! git env--helper --type=bool --default=false --exit-code $1
    + 	then
    + 		skip_all=$2
    + 		test_done
     
      diff --git a/t/test-lib.sh b/t/test-lib.sh
      --- a/t/test-lib.sh
    @@ -106,16 +115,14 @@
     +GIT_TEST_FAIL_PREREQS_INTERNAL=
     +if test -n "$GIT_TEST_FAIL_PREREQS"
     +then
    -+	if git env--helper --mode-bool --variable=GIT_TEST_FAIL_PREREQS \
    -+		--default=0 --exit-code --quiet
    ++	if git env--helper --type=bool --default=0 --exit-code GIT_TEST_FAIL_PREREQS
     +	then
     +		GIT_TEST_FAIL_PREREQS_INTERNAL=true
     +		test_set_prereq FAIL_PREREQS
     +	fi
     +else
     +	test_lazy_prereq FAIL_PREREQS '
    -+		git env--helper --mode-bool --variable=GIT_TEST_FAIL_PREREQS \
    -+			--default=0 --exit-code --quiet
    ++		git env--helper --type=bool --default=0 --exit-code GIT_TEST_FAIL_PREREQS
     +	'
     +fi
     +
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 1/8] config tests: simplify include cycle test

[Ævar Arnfjörð Bjarmason]

Simplify an overly verbose test added in 9b25a0b52e ("config: add
include directive", 2012-02-06). The "expect" file was never used, and
by using .gitconfig it's not as intuitive to reproduce this manually
with "-d" as some other tests, since HOME needs to be set in the
environment.

Also remove the use of test_i18ngrep added in a769bfc74f ("config.c:
mark more strings for translation", 2018-07-21) in favor of overriding
the GIT_TEST_GETTEXT_POISON value.

Using the i18n test wrappers hasn't been needed since my
6cdccfce1e ("i18n: make GETTEXT_POISON a runtime option", 2018-11-08).
As a follow-up change to the yet-to-be-added t0017-env-helper.sh will
show, doing it this way can hide a regression when combined with
trace2's early config reading. That early config reading was added in
bce9db6de9 ("trace2: use system/global config for default trace2
settings", 2019-04-15).

So let's remove the testing for that potential regression here, I'll
instead add it explicitly to t0017-env-helper.sh in a follow-up
change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1305-config-include.sh | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 579a86b7f8..6b388ba2d0 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -310,20 +310,13 @@ test_expect_success SYMLINKS 'conditional include, gitdir matching symlink, icas
 '
 
 test_expect_success 'include cycles are detected' '
-	cat >.gitconfig <<-\EOF &&
-	[test]value = gitconfig
-	[include]path = cycle
-	EOF
-	cat >cycle <<-\EOF &&
-	[test]value = cycle
-	[include]path = .gitconfig
-	EOF
-	cat >expect <<-\EOF &&
-	gitconfig
-	cycle
-	EOF
-	test_must_fail git config --get-all test.value 2>stderr &&
-	test_i18ngrep "exceeded maximum include depth" stderr
+	git init --bare cycle &&
+	git -C cycle config include.path cycle &&
+	git config -f cycle/cycle include.path config &&
+	test_must_fail \
+		env GIT_TEST_GETTEXT_POISON= \
+		git -C cycle config --get-all test.value 2>stderr &&
+	grep "exceeded maximum include depth" stderr
 '
 
 test_done
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Ævar Arnfjörð Bjarmason]

We have many GIT_TEST_* variables that accept a <boolean> because
they're implemented in C, and then some that take <non-empty?> because
they're implemented at least partially in shellscript.

Add a helper that wraps git_env_bool() and git_env_ulong() as the
first step in fixing this. This isn't being added as a test-tool mode
because some of these are used outside the test suite.

Part of what this tool does can be done via a trick with "git config"
added in 83d842dc8c ("tests: turn on network daemon tests by default",
2014-02-10) for test_tristate(), i.e.:

    git -c magic.variable="$1" config --bool magic.variable 2>/dev/null

But as subsequent changes will show being able to pass along the
default value makes all the difference, and we'll be able to replace
test_tristate() itself with that.

The --type=bool option will be used by subsequent patches, but not
--type=ulong. I figured it was easy enough to add it & test for it so
I left it in so we'd have wrappers for both git_env_*() functions, and
to have a template to make it obvious how we'd add --type=int etc. if
it's needed in the future.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 builtin.h             |  1 +
 builtin/env--helper.c | 95 +++++++++++++++++++++++++++++++++++++++++++
 git.c                 |  1 +
 t/t0017-env-helper.sh | 83 +++++++++++++++++++++++++++++++++++++
 6 files changed, 182 insertions(+)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0017-env-helper.sh

diff --git a/.gitignore b/.gitignore
index 4470d7cfc0..1f7a83fb3c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -58,6 +58,7 @@
 /git-difftool
 /git-difftool--helper
 /git-describe
+/git-env--helper
 /git-fast-export
 /git-fast-import
 /git-fetch
diff --git a/Makefile b/Makefile
index f58bf14c7b..f2cfc8d812 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
 BUILTIN_OBJS += builtin/difftool.o
+BUILTIN_OBJS += builtin/env--helper.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index ec7e0954c4..93bd49fe4f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -160,6 +160,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix);
 int cmd_diff(int argc, const char **argv, const char *prefix);
 int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 int cmd_difftool(int argc, const char **argv, const char *prefix);
+int cmd_env__helper(int argc, const char **argv, const char *prefix);
 int cmd_fast_export(int argc, const char **argv, const char *prefix);
 int cmd_fetch(int argc, const char **argv, const char *prefix);
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/env--helper.c b/builtin/env--helper.c
new file mode 100644
index 0000000000..1083c0f707
--- /dev/null
+++ b/builtin/env--helper.c
@@ -0,0 +1,95 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+
+static char const * const env__helper_usage[] = {
+	N_("git env--helper --type=[bool|ulong] <options> <env-var>"),
+	NULL
+};
+
+enum {
+	ENV_HELPER_TYPE_BOOL = 1,
+	ENV_HELPER_TYPE_ULONG
+} cmdmode = 0;
+
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	if (!strcmp(arg, "bool"))
+		cmdmode = ENV_HELPER_TYPE_BOOL;
+	else if (!strcmp(arg, "ulong"))
+		cmdmode = ENV_HELPER_TYPE_ULONG;
+	else
+		die(_("unrecognized --type argument, %s"), arg);
+
+	return 0;
+}
+
+int cmd_env__helper(int argc, const char **argv, const char *prefix)
+{
+	int exit_code = 0;
+	const char *env_variable = NULL;
+	const char *env_default = NULL;
+	int ret;
+	int ret_int, default_int;
+	unsigned long ret_ulong, default_ulong;
+	struct option opts[] = {
+		OPT_CALLBACK_F(0, "type", &cmdmode, N_("type"),
+			       N_("value is given this type"), PARSE_OPT_NONEG,
+			       option_parse_type),
+		OPT_STRING(0, "default", &env_default, N_("value"),
+			   N_("default for git_env_*(...) to fall back on")),
+		OPT_BOOL(0, "exit-code", &exit_code,
+			 N_("be quiet only use git_env_*() value as exit code")),
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix, opts, env__helper_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (env_default && !*env_default)
+		usage_with_options(env__helper_usage, opts);
+	if (!cmdmode)
+		usage_with_options(env__helper_usage, opts);
+	if (argc != 1)
+		usage_with_options(env__helper_usage, opts);
+	env_variable = argv[0];
+
+	switch (cmdmode) {
+	case ENV_HELPER_TYPE_BOOL:
+		if (env_default) {
+			default_int = git_parse_maybe_bool(env_default);
+			if (default_int == -1) {
+				error(_("option `--default' expects a boolean value with `--type=bool`, not `%s`"),
+				      env_default);
+				usage_with_options(env__helper_usage, opts);
+			}
+		} else {
+			default_int = 0;
+		}
+		ret_int = git_env_bool(env_variable, default_int);
+		if (!exit_code)
+			puts(ret_int ? "true" : "false");
+		ret = ret_int;
+		break;
+	case ENV_HELPER_TYPE_ULONG:
+		if (env_default) {
+			if (!git_parse_ulong(env_default, &default_ulong)) {
+				error(_("option `--default' expects an unsigned long value with `--type=ulong`, not `%s`"),
+				      env_default);
+				usage_with_options(env__helper_usage, opts);
+			}
+		} else {
+			default_ulong = 0;
+		}
+		ret_ulong = git_env_ulong(env_variable, default_ulong);
+		if (!exit_code)
+			printf("%lu\n", ret_ulong);
+		ret = ret_ulong;
+		break;
+	default:
+		BUG("unknown <type> value");
+		break;
+	}
+
+	return !ret;
+}
diff --git a/git.c b/git.c
index c2eec470c9..a43e1dd98e 100644
--- a/git.c
+++ b/git.c
@@ -500,6 +500,7 @@ static struct cmd_struct commands[] = {
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
+	{ "env--helper", cmd_env__helper },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
new file mode 100755
index 0000000000..709bbbd275
--- /dev/null
+++ b/t/t0017-env-helper.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='test env--helper'
+
+. ./test-lib.sh
+
+
+test_expect_success 'env--helper usage' '
+	test_must_fail git env--helper &&
+	test_must_fail git env--helper --type=bool &&
+	test_must_fail git env--helper --type=ulong &&
+	test_must_fail git env--helper --type=bool &&
+	test_must_fail git env--helper --type=bool --default &&
+	test_must_fail git env--helper --type=bool --default= &&
+	test_must_fail git env--helper --defaultxyz
+'
+
+test_expect_success 'env--helper bad default values' '
+	test_must_fail git env--helper --type=bool --default=1xyz MISSING &&
+	test_must_fail git env--helper --type=ulong --default=1xyz MISSING
+'
+
+test_expect_success 'env--helper --type=bool' '
+	# Test various --default bool values
+	echo true >expected &&
+	git env--helper --type=bool --default=1 MISSING >actual &&
+	test_cmp expected actual &&
+	git env--helper --type=bool --default=yes MISSING >actual &&
+	test_cmp expected actual &&
+	git env--helper --type=bool --default=true MISSING >actual &&
+	test_cmp expected actual &&
+	echo false >expected &&
+	test_must_fail git env--helper --type=bool --default=0 MISSING >actual &&
+	test_cmp expected actual &&
+	test_must_fail git env--helper --type=bool --default=no MISSING >actual &&
+	test_cmp expected actual &&
+	test_must_fail git env--helper --type=bool --default=false MISSING >actual &&
+	test_cmp expected actual &&
+
+	# No output with --exit-code
+	git env--helper --type=bool --default=true --exit-code MISSING >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+	test_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+
+	# Existing variable
+	EXISTS=true git env--helper --type=bool --default=false --exit-code EXISTS >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+	test_must_fail \
+		env EXISTS=false \
+		git env--helper --type=bool --default=true --exit-code EXISTS >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err
+'
+
+test_expect_success 'env--helper --type=ulong' '
+	echo 1234567890 >expected &&
+	git env--helper --type=ulong --default=1234567890 MISSING >actual.out 2>actual.err &&
+	test_cmp expected actual.out &&
+	test_must_be_empty actual.err &&
+
+	echo 0 >expected &&
+	test_must_fail git env--helper --type=ulong --default=0 MISSING >actual &&
+	test_cmp expected actual &&
+
+	git env--helper --type=ulong --default=1234567890 --exit-code MISSING >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+
+	EXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS --exit-code >actual.out 2>actual.err &&
+	test_must_be_empty actual.out &&
+	test_must_be_empty actual.err &&
+
+	echo 1234567890 >expected &&
+	EXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS >actual.out 2>actual.err &&
+	test_cmp expected actual.out &&
+	test_must_be_empty actual.err
+'
+
+test_done
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 3/8] config.c: refactor die_bad_number() to not call gettext() early

[Ævar Arnfjörð Bjarmason]

Prepare die_bad_number() for a change to specially handle
GIT_TEST_GETTEXT_POISON calling git_env_bool() by making
die_bad_number() not call gettext() early, which would in turn call
git_env_bool().

There's no meaningful change here yet, just a re-arrangement of the
current code to make that subsequent change easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/config.c b/config.c
index 296a6d9cc4..374cb33005 100644
--- a/config.c
+++ b/config.c
@@ -949,34 +949,35 @@ int git_parse_ssize_t(const char *value, ssize_t *ret)
 NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
-	const char * error_type = (errno == ERANGE)? _("out of range"):_("invalid unit");
+	const char *error_type = (errno == ERANGE) ?
+		N_("out of range") : N_("invalid unit");
+	const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s");
 
 	if (!value)
 		value = "";
 
 	if (!(cf && cf->name))
-		die(_("bad numeric config value '%s' for '%s': %s"),
-		    value, name, error_type);
+		die(_(bad_numeric), value, name, _(error_type));
 
 	switch (cf->origin_type) {
 	case CONFIG_ORIGIN_BLOB:
 		die(_("bad numeric config value '%s' for '%s' in blob %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	case CONFIG_ORIGIN_FILE:
 		die(_("bad numeric config value '%s' for '%s' in file %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	case CONFIG_ORIGIN_STDIN:
 		die(_("bad numeric config value '%s' for '%s' in standard input: %s"),
-		    value, name, error_type);
+		    value, name, _(error_type));
 	case CONFIG_ORIGIN_SUBMODULE_BLOB:
 		die(_("bad numeric config value '%s' for '%s' in submodule-blob %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	case CONFIG_ORIGIN_CMDLINE:
 		die(_("bad numeric config value '%s' for '%s' in command line %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	default:
 		die(_("bad numeric config value '%s' for '%s' in %s: %s"),
-		    value, name, cf->name, error_type);
+		    value, name, cf->name, _(error_type));
 	}
 }
 
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 4/8] t6040 test: stop using global "script" variable

[Ævar Arnfjörð Bjarmason]

Change test code added in c0234b2ef6 ("stat_tracking_info(): clear
object flags used during counting", 2008-07-03) to stop using the
"script" variable also used for lazy prerequisites in
test-lib-functions.sh.

Since this test uses test_i18ncmp and expects to use its own "script"
variable twice it implicitly depends on the C_LOCALE_OUTPUT
prerequisite not being a lazy prerequisite. A follow-up change will
make it a lazy prerequisite, so we must remove this landmine before
inadvertently stepping on it as we make that change.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6040-tracking-info.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 716283b274..970b25a289 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -38,7 +38,7 @@ test_expect_success setup '
 	advance h
 '
 
-script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
+t6040_script='s/^..\(b.\) *[0-9a-f]* \(.*\)$/\1 \2/p'
 cat >expect <<\EOF
 b1 [ahead 1, behind 1] d
 b2 [ahead 1, behind 1] d
@@ -53,7 +53,7 @@ test_expect_success 'branch -v' '
 		cd test &&
 		git branch -v
 	) |
-	sed -n -e "$script" >actual &&
+	sed -n -e "$t6040_script" >actual &&
 	test_i18ncmp expect actual
 '
 
@@ -71,7 +71,7 @@ test_expect_success 'branch -vv' '
 		cd test &&
 		git branch -vv
 	) |
-	sed -n -e "$script" >actual &&
+	sed -n -e "$t6040_script" >actual &&
 	test_i18ncmp expect actual
 '
 
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean

[Ævar Arnfjörð Bjarmason]

Change the GIT_TEST_GETTEXT_POISON variable from being "non-empty?" to
being a more standard boolean variable.

Since it needed to be checked in both C code and shellscript (via test
-n) it was one of the remaining shellscript-like variables. Now that
we have "env--helper" we can change that.

There's a couple of tricky edge cases that arise because we're using
git_env_bool() early, and the config-reading "env--helper".

If GIT_TEST_GETTEXT_POISON is set to an invalid value die_bad_number()
will die, but to do so it would usually call gettext(). Let's detect
the special case of GIT_TEST_GETTEXT_POISON and always emit that
message in the C locale, lest we infinitely loop.

As seen in the updated tests in t0017-env-helper.sh there's also a
caveat related to "env--helper" needing to read the config for trace2
purposes.

Since the C_LOCALE_OUTPUT prerequisite is lazy and relies on
"env--helper" we could get invalid results if we failed to read the
config (e.g. because we'd loop on includes) when combined with
e.g. "test_i18ngrep" wanting to check with "env--helper" if
GIT_TEST_GETTEXT_POISON was true or not.

I'm crossing my fingers and hoping that a test similar to the one I
removed in the earlier "config tests: simplify include cycle test"
change in this series won't happen again, and testing for this
explicitly in "env--helper"'s own tests.

This change breaks existing uses of
e.g. GIT_TEST_GETTEXT_POISON=YesPlease, which we've documented in
po/README and other places. As noted in [1] we might want to consider
also accepting "YesPlease" in "env--helper" as a special-case.

But as the lack of uproar over 6cdccfce1e ("i18n: make GETTEXT_POISON
a runtime option", 2018-11-08) demonstrates the audience for this
option is a really narrow set of git developers, who shouldn't have
much trouble modifying their test scripts, so I think it's better to
deal with that minor headache now and make all the relevant GIT_TEST_*
variables boolean in the same way than carry the "YesPlease"
special-case forward.

1. https://public-inbox.org/git/xmqqtvckm3h8.fsf@gitster-ct.c.googlers.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ci/lib.sh                 |  2 +-
 config.c                  |  9 +++++++++
 gettext.c                 |  6 ++----
 git-sh-i18n.sh            |  4 +++-
 po/README                 |  2 +-
 t/README                  |  4 ++--
 t/t0017-env-helper.sh     | 16 ++++++++++++++++
 t/t0205-gettext-poison.sh |  7 ++++++-
 t/t1305-config-include.sh |  2 +-
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib.sh             |  8 +++-----
 12 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 288a5b3884..fd799ae663 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -184,7 +184,7 @@ osx-clang|osx-gcc)
 	export GIT_SKIP_TESTS="t9810 t9816"
 	;;
 GIT_TEST_GETTEXT_POISON)
-	export GIT_TEST_GETTEXT_POISON=YesPlease
+	export GIT_TEST_GETTEXT_POISON=true
 	;;
 esac
 
diff --git a/config.c b/config.c
index 374cb33005..b985d60fa4 100644
--- a/config.c
+++ b/config.c
@@ -956,6 +956,15 @@ static void die_bad_number(const char *name, const char *value)
 	if (!value)
 		value = "";
 
+	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+		/*
+		 * We explicitly *don't* use _() here since it would
+		 * cause an infinite loop with _() needing to call
+		 * use_gettext_poison(). This is why marked up
+		 * translations with N_() above.
+		 */
+		die(bad_numeric, value, name, error_type);
+
 	if (!(cf && cf->name))
 		die(_(bad_numeric), value, name, _(error_type));
 
diff --git a/gettext.c b/gettext.c
index d4021d690c..5c71f4c8b9 100644
--- a/gettext.c
+++ b/gettext.c
@@ -50,10 +50,8 @@ const char *get_preferred_languages(void)
 int use_gettext_poison(void)
 {
 	static int poison_requested = -1;
-	if (poison_requested == -1) {
-		const char *v = getenv("GIT_TEST_GETTEXT_POISON");
-		poison_requested = v && strlen(v) ? 1 : 0;
-	}
+	if (poison_requested == -1)
+		poison_requested = git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
 	return poison_requested;
 }
 
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index e1d917fd27..8eef60b43f 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,9 @@ export TEXTDOMAINDIR
 
 # First decide what scheme to use...
 GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_TEST_GETTEXT_POISON"
+if test -n "$GIT_TEST_GETTEXT_POISON" &&
+	    git env--helper --type=bool --default=0 --exit-code \
+		GIT_TEST_GETTEXT_POISON
 then
 	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
 elif test -n "@@USE_GETTEXT_SCHEME@@"
diff --git a/po/README b/po/README
index aa704ffcb7..07595d369b 100644
--- a/po/README
+++ b/po/README
@@ -293,7 +293,7 @@ To smoke out issues like these, Git tested with a translation mode that
 emits gibberish on every call to gettext. To use it run the test suite
 with it, e.g.:
 
-    cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh
+    cd t && GIT_TEST_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh
 
 If tests break with it you should inspect them manually and see if
 what you're translating is sane, i.e. that you're not translating
diff --git a/t/README b/t/README
index 9747971d58..9a131f472e 100644
--- a/t/README
+++ b/t/README
@@ -343,8 +343,8 @@ whether this mode is active, and e.g. skip some tests that are hard to
 refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
-GIT_TEST_GETTEXT_POISON=<non-empty?> turns all strings marked for
-translation into gibberish if non-empty (think "test -n"). Used for
+GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
+translation into gibberish if true. Used for
 spotting those tests that need to be marked with a C_LOCALE_OUTPUT
 prerequisite when adding more strings for translation. See "Testing
 marked strings" in po/README for details.
diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
index 709bbbd275..c1ecf6aeac 100755
--- a/t/t0017-env-helper.sh
+++ b/t/t0017-env-helper.sh
@@ -80,4 +80,20 @@ test_expect_success 'env--helper --type=ulong' '
 	test_must_be_empty actual.err
 '
 
+test_expect_success 'env--helper reads config thanks to trace2' '
+	mkdir home &&
+	git config -f home/.gitconfig include.path cycle &&
+	git config -f home/cycle include.path .gitconfig &&
+
+	test_must_fail \
+		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
+		git config -l 2>err &&
+	grep "exceeded maximum include depth" err &&
+
+	test_must_fail \
+		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
+		git -C cycle env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON 2>err &&
+	grep "# GETTEXT POISON #" err
+'
+
 test_done
diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index a06269f38a..f9fa16ad83 100755
--- a/t/t0205-gettext-poison.sh
+++ b/t/t0205-gettext-poison.sh
@@ -5,7 +5,7 @@
 
 test_description='Gettext Shell poison'
 
-GIT_TEST_GETTEXT_POISON=YesPlease
+GIT_TEST_GETTEXT_POISON=true
 export GIT_TEST_GETTEXT_POISON
 . ./lib-gettext.sh
 
@@ -31,4 +31,9 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant
     test_cmp expect actual
 '
 
+test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
+	test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
+	grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
+"
+
 test_done
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 6b388ba2d0..de294c990e 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -314,7 +314,7 @@ test_expect_success 'include cycles are detected' '
 	git -C cycle config include.path cycle &&
 	git config -f cycle/cycle include.path config &&
 	test_must_fail \
-		env GIT_TEST_GETTEXT_POISON= \
+		env GIT_TEST_GETTEXT_POISON=false \
 		git -C cycle config --get-all test.value 2>stderr &&
 	grep "exceeded maximum include depth" stderr
 '
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 5990299fc9..b696bae5f5 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -249,7 +249,7 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '
 test_expect_success 'checkout to detach HEAD' '
 	git config advice.detachedHead true &&
 	git checkout -f renamer && git clean -f &&
-	GIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&
+	GIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&
 	grep "HEAD is now at 7329388" messages &&
 	test_line_count -gt 1 messages &&
 	H=$(git rev-parse --verify HEAD) &&
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43cf313a1c..75512c3403 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1706,7 +1706,7 @@ test_expect_success 'sourcing the completion script clears cached commands' '
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
-	GIT_TEST_GETTEXT_POISON= &&
+	GIT_TEST_GETTEXT_POISON=false &&
 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
 	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4b346467df..ed5d69dfe5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1443,11 +1443,9 @@ then
 	unset GIT_TEST_GETTEXT_POISON_ORIG
 fi
 
-# Can we rely on git's output in the C locale?
-if test -z "$GIT_TEST_GETTEXT_POISON"
-then
-	test_set_prereq C_LOCALE_OUTPUT
-fi
+test_lazy_prereq C_LOCALE_OUTPUT '
+	! git env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON
+'
 
 if test -z "$GIT_TEST_CHECK_CACHE_TREE"
 then
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 6/8] tests README: re-flow a previously changed paragraph

[Ævar Arnfjörð Bjarmason]

A previous change to the "GIT_TEST_GETTEXT_POISON" variable left this
paragraph needing to be re-flowed. Let's do that in this separate
change to make it easy to see that there's no change here when viewed
with "--word-diff".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index 9a131f472e..072c9854d1 100644
--- a/t/README
+++ b/t/README
@@ -344,10 +344,10 @@ refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
 GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
-translation into gibberish if true. Used for
-spotting those tests that need to be marked with a C_LOCALE_OUTPUT
-prerequisite when adding more strings for translation. See "Testing
-marked strings" in po/README for details.
+translation into gibberish if true. Used for spotting those tests that
+need to be marked with a C_LOCALE_OUTPUT prerequisite when adding more
+strings for translation. See "Testing marked strings" in po/README for
+details.
 
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 7/8] tests: replace test_tristate with "git env--helper"

[Ævar Arnfjörð Bjarmason]

The test_tristate helper introduced in 83d842dc8c ("tests: turn on
network daemon tests by default", 2014-02-10) can now be better
implemented with "git env--helper" to give the variables in question
the standard boolean behavior.

The reason for the "tristate" was to have all of false/true/auto,
where "auto" meant either "false" or "true" depending on what the
fallback was. With the --default option to "git env--helper" we can
simply have e.g. GIT_TEST_HTTPD where we know if it's true because the
user asked explicitly ("true"), or true implicitly ("auto").

This breaks backwards compatibility for explicitly setting "auto" for
these variables, but I don't think anyone cares. That was always
intended to be internal.

This means the test_normalize_bool() code in test-lib-functions.sh
goes away in addition to test_tristate(). We still need the
test_skip_or_die() helper, but now it takes the variable name instead
of the value, and uses "git env--bool" to distinguish a default "true"
from an explicit "true" (in those "explicit true" cases we want to
fail the test in question).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-git-daemon.sh     |  7 +++---
 t/lib-git-svn.sh        | 11 +++-----
 t/lib-httpd.sh          | 15 ++++++-----
 t/t5512-ls-remote.sh    |  3 +--
 t/test-lib-functions.sh | 56 ++++++-----------------------------------
 5 files changed, 22 insertions(+), 70 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 7b3407134e..fb8f887080 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -15,8 +15,7 @@
 #
 #	test_done
 
-test_tristate GIT_TEST_GIT_DAEMON
-if test "$GIT_TEST_GIT_DAEMON" = false
+if ! git env--helper --type=bool --default=true --exit-code GIT_TEST_GIT_DAEMON
 then
 	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
 	test_done
@@ -24,7 +23,7 @@ fi
 
 if test_have_prereq !PIPE
 then
-	test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
+	test_skip_or_die GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
 fi
 
 test_set_port LIB_GIT_DAEMON_PORT
@@ -73,7 +72,7 @@ start_git_daemon() {
 		kill "$GIT_DAEMON_PID"
 		wait "$GIT_DAEMON_PID"
 		unset GIT_DAEMON_PID
-		test_skip_or_die $GIT_TEST_GIT_DAEMON \
+		test_skip_or_die GIT_TEST_GIT_DAEMON \
 			"git daemon failed to start"
 	fi
 }
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index c1271d6863..5d4ae629e1 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -69,14 +69,12 @@ svn_cmd () {
 maybe_start_httpd () {
 	loc=${1-svn}
 
-	test_tristate GIT_SVN_TEST_HTTPD
-	case $GIT_SVN_TEST_HTTPD in
-	true)
+	if git env--helper --type=bool --default=false --exit-code GIT_TEST_HTTPD
+	then
 		. "$TEST_DIRECTORY"/lib-httpd.sh
 		LIB_HTTPD_SVN="$loc"
 		start_httpd
-		;;
-	esac
+	fi
 }
 
 convert_to_rev_db () {
@@ -106,8 +104,7 @@ EOF
 }
 
 require_svnserve () {
-	test_tristate GIT_TEST_SVNSERVE
-	if ! test "$GIT_TEST_SVNSERVE" = true
+	if ! git env--helper --type=bool --default=false --exit-code GIT_TEST_SVNSERVE
 	then
 		skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to enable)'
 		test_done
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b3cc62bd36..0d985758c6 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -41,15 +41,14 @@ then
 	test_done
 fi
 
-test_tristate GIT_TEST_HTTPD
-if test "$GIT_TEST_HTTPD" = false
+if ! git env--helper --type=bool --default=true --exit-code GIT_TEST_HTTPD
 then
 	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
 	test_done
 fi
 
 if ! test_have_prereq NOT_ROOT; then
-	test_skip_or_die $GIT_TEST_HTTPD \
+	test_skip_or_die GIT_TEST_HTTPD \
 		"Cannot run httpd tests as root"
 fi
 
@@ -95,7 +94,7 @@ GIT_TRACE=$GIT_TRACE; export GIT_TRACE
 
 if ! test -x "$LIB_HTTPD_PATH"
 then
-	test_skip_or_die $GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
+	test_skip_or_die GIT_TEST_HTTPD "no web server found at '$LIB_HTTPD_PATH'"
 fi
 
 HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \
@@ -107,19 +106,19 @@ then
 	then
 		if ! test $HTTPD_VERSION -ge 2
 		then
-			test_skip_or_die $GIT_TEST_HTTPD \
+			test_skip_or_die GIT_TEST_HTTPD \
 				"at least Apache version 2 is required"
 		fi
 		if ! test -d "$DEFAULT_HTTPD_MODULE_PATH"
 		then
-			test_skip_or_die $GIT_TEST_HTTPD \
+			test_skip_or_die GIT_TEST_HTTPD \
 				"Apache module directory not found"
 		fi
 
 		LIB_HTTPD_MODULE_PATH="$DEFAULT_HTTPD_MODULE_PATH"
 	fi
 else
-	test_skip_or_die $GIT_TEST_HTTPD \
+	test_skip_or_die GIT_TEST_HTTPD \
 		"Could not identify web server at '$LIB_HTTPD_PATH'"
 fi
 
@@ -184,7 +183,7 @@ start_httpd() {
 	if test $? -ne 0
 	then
 		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
-		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
+		test_skip_or_die GIT_TEST_HTTPD "web server setup failed"
 	fi
 }
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index e3c4a48c85..43e1d8d4d2 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -267,8 +267,7 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 '
 
 test_lazy_prereq GIT_DAEMON '
-	test_tristate GIT_TEST_GIT_DAEMON &&
-	test "$GIT_TEST_GIT_DAEMON" != false
+	git env--helper --type=bool --default=true --exit-code GIT_TEST_GIT_DAEMON
 '
 
 # This test spawns a daemon, so run it only if the user would be OK with
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0367cec5fd..527508c350 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1035,62 +1035,20 @@ perl () {
 	command "$PERL_PATH" "$@" 2>&7
 } 7>&2 2>&4
 
-# Is the value one of the various ways to spell a boolean true/false?
-test_normalize_bool () {
-	git -c magic.variable="$1" config --bool magic.variable 2>/dev/null
-}
-
-# Given a variable $1, normalize the value of it to one of "true",
-# "false", or "auto" and store the result to it.
-#
-#     test_tristate GIT_TEST_HTTPD
-#
-# A variable set to an empty string is set to 'false'.
-# A variable set to 'false' or 'auto' keeps its value.
-# Anything else is set to 'true'.
-# An unset variable defaults to 'auto'.
-#
-# The last rule is to allow people to set the variable to an empty
-# string and export it to decline testing the particular feature
-# for versions both before and after this change.  We used to treat
-# both unset and empty variable as a signal for "do not test" and
-# took any non-empty string as "please test".
-
-test_tristate () {
-	if eval "test x\"\${$1+isset}\" = xisset"
-	then
-		# explicitly set
-		eval "
-			case \"\$$1\" in
-			'')	$1=false ;;
-			auto)	;;
-			*)	$1=\$(test_normalize_bool \$$1 || echo true) ;;
-			esac
-		"
-	else
-		eval "$1=auto"
-	fi
-}
-
 # Exit the test suite, either by skipping all remaining tests or by
-# exiting with an error. If "$1" is "auto", we then we assume we were
-# opportunistically trying to set up some tests and we skip. If it is
-# "true", then we report a failure.
+# exiting with an error. If our prerequisite variable $1 falls back
+# on a default assume we were opportunistically trying to set up some
+# tests and we skip. If it is explicitly "true", then we report a failure.
 #
 # The error/skip message should be given by $2.
 #
 test_skip_or_die () {
-	case "$1" in
-	auto)
+	if ! git env--helper --mode-bool --variable=$1 --default=0 --exit-code --quiet
+	then
 		skip_all=$2
 		test_done
-		;;
-	true)
-		error "$2"
-		;;
-	*)
-		error "BUG: test tristate is '$1' (real error: $2)"
-	esac
+	fi
+	error "$2"
 }
 
 # The following mingw_* functions obey POSIX shell syntax, but are actually
-- 
2.22.0.455.g172b71a6c5

[PATCH v3 8/8] tests: make GIT_TEST_FAIL_PREREQS a boolean

[Ævar Arnfjörð Bjarmason]

Change the GIT_TEST_FAIL_PREREQS variable from being "non-empty?" to
being a more standard boolean variable. I recently added the variable
in dfe1a17df9 ("tests: add a special setup where prerequisites fail",
2019-05-13), having to add another "non-empty?" special-case is what
prompted me to write the "git env--helper" utility being used here.

Converting this one is a bit tricky since we use it so early and
frequently in the guts of the test code itself, so let's set a
GIT_TEST_FAIL_PREREQS_INTERNAL which can be tested with the old "test
-n" for the purposes of the shell code, and change the user-exposed
and documented GIT_TEST_FAIL_PREREQS variable to a boolean.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README                |  2 +-
 t/t0000-basic.sh        | 10 +++++-----
 t/test-lib-functions.sh |  4 ++--
 t/test-lib.sh           | 23 +++++++++++++++++++----
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 072c9854d1..60d5b77bcc 100644
--- a/t/README
+++ b/t/README
@@ -334,7 +334,7 @@ that cannot be easily covered by a few specific test cases. These
 could be enabled by running the test suite with correct GIT_TEST_
 environment set.
 
-GIT_TEST_FAIL_PREREQS<non-empty?> fails all prerequisites. This is
+GIT_TEST_FAIL_PREREQS=<boolean> fails all prerequisites. This is
 useful for discovering issues with the tests where say a later test
 implicitly depends on an optional earlier test.
 
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 31de7e90f3..e89438e619 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -726,7 +726,7 @@ donthaveit=yes
 test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be skipped' '
 	donthaveit=no
 '
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit != yesyes
 then
 	say "bug in test framework: prerequisite tags do not work reliably"
 	exit 1
@@ -747,7 +747,7 @@ donthaveiteither=yes
 test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '
 	donthaveiteither=no
 '
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit$donthaveiteither != yesyesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit$donthaveiteither != yesyesyes
 then
 	say "bug in test framework: multiple prerequisite tags do not work reliably"
 	exit 1
@@ -763,7 +763,7 @@ test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
 	donthavetrue=no
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a "$havetrue$donthavetrue" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a "$havetrue$donthavetrue" != yesyes
 then
 	say 'bug in test framework: lazy prerequisites do not work'
 	exit 1
@@ -779,7 +779,7 @@ test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
 	havefalse=no
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a "$nothavefalse$havefalse" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a "$nothavefalse$havefalse" != yesyes
 then
 	say 'bug in test framework: negative lazy prerequisites do not work'
 	exit 1
@@ -790,7 +790,7 @@ test_expect_success 'tests clean up after themselves' '
 	test_when_finished clean=yes
 '
 
-if test -z "$GIT_TEST_FAIL_PREREQS" -a $clean != yes
+if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $clean != yes
 then
 	say "bug in test framework: basic cleanup command does not work reliably"
 	exit 1
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 527508c350..1cd0655f96 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -309,7 +309,7 @@ test_unset_prereq () {
 }
 
 test_set_prereq () {
-	if test -n "$GIT_TEST_FAIL_PREREQS"
+	if test -n "$GIT_TEST_FAIL_PREREQS_INTERNAL"
 	then
 		case "$1" in
 		# The "!" case is handled below with
@@ -1043,7 +1043,7 @@ perl () {
 # The error/skip message should be given by $2.
 #
 test_skip_or_die () {
-	if ! git env--helper --mode-bool --variable=$1 --default=0 --exit-code --quiet
+	if ! git env--helper --type=bool --default=false --exit-code $1
 	then
 		skip_all=$2
 		test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ed5d69dfe5..1af4e50653 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1389,6 +1389,25 @@ yes () {
 	done
 }
 
+# The GIT_TEST_FAIL_PREREQS code hooks into test_set_prereq(), and
+# thus needs to be set up really early, and set an internal variable
+# for convenience so the hot test_set_prereq() codepath doesn't need
+# to call "git env--helper". Only do that work if needed by seeing if
+# GIT_TEST_FAIL_PREREQS is set at all.
+GIT_TEST_FAIL_PREREQS_INTERNAL=
+if test -n "$GIT_TEST_FAIL_PREREQS"
+then
+	if git env--helper --type=bool --default=0 --exit-code GIT_TEST_FAIL_PREREQS
+	then
+		GIT_TEST_FAIL_PREREQS_INTERNAL=true
+		test_set_prereq FAIL_PREREQS
+	fi
+else
+	test_lazy_prereq FAIL_PREREQS '
+		git env--helper --type=bool --default=0 --exit-code GIT_TEST_FAIL_PREREQS
+	'
+fi
+
 # Fix some commands on Windows
 uname_s=$(uname -s)
 case $uname_s in
@@ -1605,7 +1624,3 @@ test_lazy_prereq SHA1 '
 test_lazy_prereq REBASE_P '
 	test -z "$GIT_TEST_SKIP_REBASE_P"
 '
-
-test_lazy_prereq FAIL_PREREQS '
-	test -n "$GIT_TEST_FAIL_PREREQS"
-'
-- 
2.22.0.455.g172b71a6c5

Re: [PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Just thought I'd ask if appeasing older analyzers is what we want for
> these sorts of optional warnings in general.

Yeah, examples of such "let's help users with common compilers so
that they can focus on real errors" are abound.

Thanks.

Re: [PATCH v3 2/8] env--helper: new undocumented builtin wrapping git_env_*()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The --type=bool option will be used by subsequent patches, but not
> --type=ulong. I figured it was easy enough to add it & test for it so
> I left it in so we'd have wrappers for both git_env_*() functions, and
> to have a template to make it obvious how we'd add --type=int etc. if
> it's needed in the future.

Yup, it is fine to start small/minimal.  In addition, obviously we
would eventually want "string" type (in fact, it would probably be
the reasonable default when --type=<type> option is missing, once
this becomes a real command and not a test helper).

> +static char const * const env__helper_usage[] = {
> +	N_("git env--helper --type=[bool|ulong] <options> <env-var>"),

This makes it look as if --type=<type> is not among options, which I
am not sure is the impression we would want to give, as I doubt we
would want to keep it mandatory in the long run, but I'd say it is
OK for two reasons (1) as we start minimal, it is OK to keep this
mandatory, and (2) this gives us a place to enumerate available
types.

The output from "git env--helper -h" may redundantly list the
"--type" option, though.

> +	argc = parse_options(argc, argv, prefix, opts, env__helper_usage,
> +			     PARSE_OPT_KEEP_UNKNOWN);

Why KEEP_UNKNOWN?

> +	if (env_default && !*env_default)
> +		usage_with_options(env__helper_usage, opts);
> +	if (!cmdmode)
> +		usage_with_options(env__helper_usage, opts);
> +	if (argc != 1)
> +		usage_with_options(env__helper_usage, opts);
> +	env_variable = argv[0];
> +
> +	switch (cmdmode) {
> +	case ENV_HELPER_TYPE_BOOL:
> +		if (env_default) {
> +			default_int = git_parse_maybe_bool(env_default);
> +			if (default_int == -1) {
> +				error(_("option `--default' expects a boolean value with `--type=bool`, not `%s`"),
> +				      env_default);
> +				usage_with_options(env__helper_usage, opts);
> +			}

OK, that more-or-less is equivalent to what git_config_bool_or_int()
does, except that I would say "if (default_int < 0)" to match the
original more closely if I were doing this.

> +		} else {
> +			default_int = 0;
> +		}
> +		ret_int = git_env_bool(env_variable, default_int);
> +		if (!exit_code)
> +			puts(ret_int ? "true" : "false");
> +		ret = ret_int;
> +		break;

I do not think we want to assign 0 to 'ret' if we are not doing
"exit_code".  "We successfully found that the variable's value is
false, said that to standard output, and signal success by exiting
with 0 status" is what we would want, no?

> +	case ENV_HELPER_TYPE_ULONG:
> +		if (env_default) {
> +			if (!git_parse_ulong(env_default, &default_ulong)) {
> +				error(_("option `--default' expects an unsigned long value with `--type=ulong`, not `%s`"),
> +				      env_default);
> +				usage_with_options(env__helper_usage, opts);
> +			}
> +		} else {
> +			default_ulong = 0;
> +		}
> +		ret_ulong = git_env_ulong(env_variable, default_ulong);
> +		if (!exit_code)
> +			printf("%lu\n", ret_ulong);
> +		ret = ret_ulong;

Likewise.

> +		break;
> +	default:
> +		BUG("unknown <type> value");
> +		break;
> +	}
> +
> +	return !ret;
> +}
> diff --git a/git.c b/git.c
> index c2eec470c9..a43e1dd98e 100644
> --- a/git.c
> +++ b/git.c
> @@ -500,6 +500,7 @@ static struct cmd_struct commands[] = {
>  	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
>  	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
>  	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
> +	{ "env--helper", cmd_env__helper },
>  	{ "fast-export", cmd_fast_export, RUN_SETUP },
>  	{ "fetch", cmd_fetch, RUN_SETUP },
>  	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
> diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
> new file mode 100755
> index 0000000000..709bbbd275
> --- /dev/null
> +++ b/t/t0017-env-helper.sh
> @@ -0,0 +1,83 @@
> +#!/bin/sh
> +
> +test_description='test env--helper'
> +
> +. ./test-lib.sh
> +
> +
> +test_expect_success 'env--helper usage' '
> +	test_must_fail git env--helper &&
> +	test_must_fail git env--helper --type=bool &&
> +	test_must_fail git env--helper --type=ulong &&
> +	test_must_fail git env--helper --type=bool &&
> +	test_must_fail git env--helper --type=bool --default &&
> +	test_must_fail git env--helper --type=bool --default= &&
> +	test_must_fail git env--helper --defaultxyz
> +'
> +
> +test_expect_success 'env--helper bad default values' '

I think

	sane_unset MISSING &&

is needed, as I do not think test-lib.sh filters that variable from
the tester's environment from being seen by us.

> +	test_must_fail git env--helper --type=bool --default=1xyz MISSING &&
> +	test_must_fail git env--helper --type=ulong --default=1xyz MISSING

Both I and you know that the current implementation of env--helper
happens to notice and bail for failing to parse --default without
even consulting the variable, but we do not have to rely on such an
implementation detail.

Speaking of that particular implementation detail, if the variable
is not missing, should we even parse and complain what is in --default?
I think the answer is "yes, for catching bugs in the script, that is
more useful behaviour".

If that is the design principle, we should also test cases where an
existing variable is given with a --default that is unparseable and
make sure that the command fails.

> +'
> +
> +test_expect_success 'env--helper --type=bool' '

Ditto for sane_unset MISSING upfront.

> +	# Test various --default bool values
> +	echo true >expected &&
> +	git env--helper --type=bool --default=1 MISSING >actual &&
> +	test_cmp expected actual &&
> +	git env--helper --type=bool --default=yes MISSING >actual &&
> +	test_cmp expected actual &&
> +	git env--helper --type=bool --default=true MISSING >actual &&
> +	test_cmp expected actual &&
> +	echo false >expected &&
> +	test_must_fail git env--helper --type=bool --default=0 MISSING >actual &&
> +	test_cmp expected actual &&

As I said already, it is a horrible calling convention to make the
mode that produces textual output to fail when the variable is set
to false.  The above two should read more like

    echo true >expect &&
    git env--helper --type=bool --default=true MISSING >actual &&
    test_cmp expect actual &&
    echo false >expect &&
    git env--helper --type=bool --default=0 MISSING >actual &&
    test_cmp expect actual &&

Re: [PATCH v3 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/config.c b/config.c
> index 374cb33005..b985d60fa4 100644
> --- a/config.c
> +++ b/config.c
> @@ -956,6 +956,15 @@ static void die_bad_number(const char *name, const char *value)
>  	if (!value)
>  		value = "";
>  
> +	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
> +		/*
> +		 * We explicitly *don't* use _() here since it would
> +		 * cause an infinite loop with _() needing to call
> +		 * use_gettext_poison(). This is why marked up
> +		 * translations with N_() above.
> +		 */
> +		die(bad_numeric, value, name, error_type);

;-)  Nicely explained.

> diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
> index e1d917fd27..8eef60b43f 100644
> --- a/git-sh-i18n.sh
> +++ b/git-sh-i18n.sh
> @@ -17,7 +17,9 @@ export TEXTDOMAINDIR
>  
>  # First decide what scheme to use...
>  GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
> -if test -n "$GIT_TEST_GETTEXT_POISON"
> +if test -n "$GIT_TEST_GETTEXT_POISON" &&
> +	    git env--helper --type=bool --default=0 --exit-code \
> +		GIT_TEST_GETTEXT_POISON

The helper is called only when GIT_TEST_GETTEXT_POISON has a
non-empty string as its value, so it's default is meaningless, no?

[PATCH 1/2] t/lib-git-svn.sh: check GIT_TEST_SVN_HTTPD when running SVN HTTP tests

[SZEDER Gábor]

Once upon a time the GIT_SVN_TEST_HTTPD environment variable needed to
be set to enable SVN HTTP tests [1].

Then 3b072c577b (tests: replace test_tristate with "git env--helper",
2019-06-21) came along, and attempted to turn GIT_SVN_TEST_HTTPD into
a bool, but while doing so it mistyped the variable name, and started
to check GIT_TEST_HTTPD instead.  Consequently, if someone explicitly
set GIT_TEST_HTTPD to true and has only the general 'git-svn'
dependencies installed but not the Subversion server modules for
Apache (libapache2-mod-svn), then a couple of 'git-svn' tests fail,
because they can't start httpd due to the missing module.

We could simply fix this by checking the GIT_SVN_TEST_HTTPD
variablewith 'git env--helper', but notice that the name of this
variable doesn't conform to our usual GIT_TEST_* convention.

So let's check the GIT_TEST_SVN_HTTPD instead.

[1] a8a5d25118 (git svn: migrate tests to use lib-httpd, 2016-07-23)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-svn.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 5d4ae629e1..bc0b9c71f8 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -69,7 +69,7 @@ svn_cmd () {
 maybe_start_httpd () {
 	loc=${1-svn}
 
-	if git env--helper --type=bool --default=false --exit-code GIT_TEST_HTTPD
+	if git env--helper --type=bool --default=false --exit-code GIT_TEST_SVN_HTTPD
 	then
 		. "$TEST_DIRECTORY"/lib-httpd.sh
 		LIB_HTTPD_SVN="$loc"
-- 
2.23.0.331.g4e51dcdf11

[PATCH 2/2] ci: restore running httpd tests

[SZEDER Gábor]

Once upon a time GIT_TEST_HTTPD was a tristate variable and we
exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
that we run the httpd tests in the Linux Clang and GCC build jobs, or
error out if they can't be run for any reason [1].

Then 3b072c577b (tests: replace test_tristate with "git env--helper",
2019-06-21) came along, turned GIT_TEST_HTTPD into a bool, but forgot
to update our CI scripts accordingly.  So, since GIT_TEST_HTTPD is set
explicitly, but its value is not one of the standardized true values,
our CI jobs have been simply skipping the httpd tests in the last
couple of weeks.

Set 'GIT_TEST_HTTPD=true' to restore running httpd tests in our CI
jobs.

[1] a1157b76eb (travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh',
    2017-12-12)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 44db2d5cbb..29dc740d40 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -160,7 +160,7 @@ linux-clang|linux-gcc)
 		export CC=gcc-8
 	fi
 
-	export GIT_TEST_HTTPD=YesPlease
+	export GIT_TEST_HTTPD=true
 
 	# The Linux build installs the defined dependency versions below.
 	# The OS X build installs much more recent versions, whichever
-- 
2.23.0.331.g4e51dcdf11

Re: [PATCH 2/2] ci: restore running httpd tests

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> that we run the httpd tests in the Linux Clang and GCC build jobs, or
> error out if they can't be run for any reason [1].

Wow.  I vaguely recall having mumbled about keeping YesPlease as an
extra synomym for 'yes' for safety against misconversions, as its
use in the build/test infrastructure is so ingrained, but at the end
went with the standard set of boolean without doing such extending,
with the hope that soon any misconversion would be found out.

This discovery of a misconversion took 3 months, which may or may
not match the definition of "soon" X-<.

The fix is obviously correct.  Thanks.

> Then 3b072c577b (tests: replace test_tristate with "git env--helper",
> 2019-06-21) came along, turned GIT_TEST_HTTPD into a bool, but forgot
> to update our CI scripts accordingly.  So, since GIT_TEST_HTTPD is set
> explicitly, but its value is not one of the standardized true values,
> our CI jobs have been simply skipping the httpd tests in the last
> couple of weeks.
>
> Set 'GIT_TEST_HTTPD=true' to restore running httpd tests in our CI
> jobs.
>
> [1] a1157b76eb (travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh',
>     2017-12-12)
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  ci/lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 44db2d5cbb..29dc740d40 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -160,7 +160,7 @@ linux-clang|linux-gcc)
>  		export CC=gcc-8
>  	fi
>  
> -	export GIT_TEST_HTTPD=YesPlease
> +	export GIT_TEST_HTTPD=true
>  
>  	# The Linux build installs the defined dependency versions below.
>  	# The OS X build installs much more recent versions, whichever

Re: [PATCH 2/2] ci: restore running httpd tests

[Jeff King]

On Fri, Sep 06, 2019 at 02:13:26PM +0200, SZEDER Gábor wrote:

> Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> that we run the httpd tests in the Linux Clang and GCC build jobs, or
> error out if they can't be run for any reason [1].

Yikes, good catch.

I wonder if it would be possible for the test suite to catch this. I
think env--helper would have written a message to stderr, but because we
use --exit-code, we can't tell the difference between that and "false".

I think we'd have go back to something more like:

  test_tristate () {
	bool=$(git env--helper --type=bool --default=true "$1") ||
		eval "error \"$1 is not a bool: \$$1\""
	test "$bool" = "true"
  }
  ...
  if test_tristate GIT_TEST_HTTPD
  then
	... use httpd ...
  fi

Not sure if it's worth it.

-Peff

Re: [PATCH 2/2] ci: restore running httpd tests

[SZEDER Gábor]

On Fri, Sep 06, 2019 at 03:13:01PM -0400, Jeff King wrote:
> On Fri, Sep 06, 2019 at 02:13:26PM +0200, SZEDER Gábor wrote:
> 
> > Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> > exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> > that we run the httpd tests in the Linux Clang and GCC build jobs, or
> > error out if they can't be run for any reason [1].
> 
> Yikes, good catch.
> 
> I wonder if it would be possible for the test suite to catch this. I
> think env--helper would have written a message to stderr, but because we
> use --exit-code, we can't tell the difference between that and "false".

No, '--exit-code' only suppresses the printing of 'true' and 'false',
but it doesn't have any effect on the command's exit code.  And if the
value of the environment variable is not a bool, then it does print an
error message to standard error, and, more importantly, exits with 128
(as opposed to 1 indicating false).  So we could tell the difference,
but as it happens the command is invoked as 'if ! git env-helper ...',
which then interprets that 128 the same as on ordinary false.

  $ VAR=false git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  1
  $ VAR=true git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  0
  $ VAR=YesPlease git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  fatal: bad numeric config value 'YesPlease' for 'VAR': invalid unit
  128


> I think we'd have go back to something more like:
> 
>   test_tristate () {

The env var is supposed to be a bool, so there is no third state
anymore.

> 	bool=$(git env--helper --type=bool --default=true "$1") ||

Some callsites use '--default=false', so we need a second parameter to
specify the default.

> 		eval "error \"$1 is not a bool: \$$1\""
> 	test "$bool" = "true"
>   }
>   ...
>   if test_tristate GIT_TEST_HTTPD
>   then
> 	... use httpd ...
>   fi
> 
> Not sure if it's worth it.

Well...  On one hand, there are no other similar issues in our CI
scripts (there is still a GIT_TEST_CLONE_2GB=YesPlease, but it's only
ever checked with 'test -z' in 't5608-clone-2gb.sh', so it works as
expected, even though it's inconsistent, and
'GIT_TEST_CLONE_2GB=NoThanks' would do the wrong thing).

OTOH, some unsuspecting devs might still have a
'GIT_TEST_foo=YesPlease' in their 'config.mak' from the old days...

Furthermore, as for the "good catch", I just got lucky, and not only
once but several times in a row: that Perforce filehost outage the
other day errored one of my builds, so I came up with a fix [1], for
once took the extra effort and checked it on Azure Pipelines, and
since that was my first ever build over there, out of curiosity I
scrolled through the whole build output, by chance those "skipped:
Network testing disabled (unset GIT_TEST_HTTPD to enable)" lines
caught my eye, shrugged and thought that Dscho should better fix this,
but then an hour later got suspicious, so looked up a recent Travis CI
build, and...  here we are.

I have to wonder how much longer it could have been remained
unnoticed, if the Perforce filehost hadn't failed or if I hadn't made
the gitgitgadget PR for my fix, or...

Anyway, this is just a long-winded way to say that I think we should
validate those bools properly and error loudly on an invalid value
even if it doesn't seem to worth it.


[1] https://public-inbox.org/git/20190906102711.6401-1-szeder.dev@gmail.com/T/

[PATCH 0/2] tests: catch non-bool GIT_TEST_* values

[SZEDER Gábor]

On Sat, Sep 07, 2019 at 12:16:37PM +0200, SZEDER Gábor wrote:
> On Fri, Sep 06, 2019 at 03:13:01PM -0400, Jeff King wrote:
> > I wonder if it would be possible for the test suite to catch this.

[i.e. to catch a non-boolean value of a supposedly boolean GIT_TEST_*
environment variable]

> > Not sure if it's worth it.

[...]

> Anyway, this is just a long-winded way to say that I think we should
> validate those bools properly and error loudly on an invalid value
> even if it doesn't seem to worth it.

I was playing with a new GIT_TEST boolean environment variable,
mistyped it's value, and then it took me surprisingly long to figure
out why the damn tests were not doing what I thought they should.

So now I'm fairly convinced that it's definitely worth doing.  And the
callsites look much nicer, too.

These patches should apply cleanly both on current 'master' or on
'sg/git-test-boolean' (i.e. the email thread I'm replying to).  We
haven't introduced any new 'git env--helper' invocations in the test
suite/framework since then.


SZEDER Gábor (2):
  tests: add 'test_bool_env' to catch non-bool GIT_TEST_* values
  t5608-clone-2gb.sh: turn GIT_TEST_CLONE_2GB into a bool

 ci/lib.sh               |  2 +-
 t/README                |  9 +++++++++
 t/lib-git-daemon.sh     |  2 +-
 t/lib-git-svn.sh        |  4 ++--
 t/lib-httpd.sh          |  2 +-
 t/t0000-basic.sh        | 34 ++++++++++++++++++++++++++++++++++
 t/t5512-ls-remote.sh    |  2 +-
 t/t5608-clone-2gb.sh    |  2 +-
 t/test-lib-functions.sh | 30 +++++++++++++++++++++++++++++-
 t/test-lib.sh           | 10 +++++-----
 10 files changed, 84 insertions(+), 13 deletions(-)

-- 
2.24.0.532.ge18579ded8

[PATCH 1/2] tests: add 'test_bool_env' to catch non-bool GIT_TEST_* values

[SZEDER Gábor]

Since 3b072c577b (tests: replace test_tristate with "git env--helper",
2019-06-21) we get the normalized bool values of various GIT_TEST_*
environment variables via 'git env--helper'.  Now, while the 'git
env--helper' command itself does catch invalid values in the
environment variable or in the given --default and exits with error
(exit code 128 or 129, respectively), it's invoked in conditions like
'if ! git env--helper ...', which means that all invalid bool values
are interpreted the same as the ordinary 'false' (exit code 1).  This
has led to inadvertently skipped httpd tests in our CI builds for a
couple of weeks, see 3960290675 (ci: restore running httpd tests,
2019-09-06).

Let's be more careful about what the test suite accepts as bool values
in GIT_TEST_* environment variables, and error out loud and clear on
invalid values instead of simply skipping tests.  Add the
'test_bool_env' helper function to encapsulate the invocation of 'git
env--helper' and the verification of its exit code, and replace all
invocations of that command in our test framework and test suite with
a call to this new helper (except in 't0017-env-helper.sh', of
course).

  $ GIT_TEST_GIT_DAEMON=YesPlease ./t5570-git-daemon.sh
  fatal: bad numeric config value 'YesPlease' for 'GIT_TEST_GIT_DAEMON': invalid unit
  error: test_bool_env requires bool values both for $GIT_TEST_GIT_DAEMON and for the default fallback

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/README                |  9 +++++++++
 t/lib-git-daemon.sh     |  2 +-
 t/lib-git-svn.sh        |  4 ++--
 t/lib-httpd.sh          |  2 +-
 t/t0000-basic.sh        | 34 ++++++++++++++++++++++++++++++++++
 t/t5512-ls-remote.sh    |  2 +-
 t/test-lib-functions.sh | 30 +++++++++++++++++++++++++++++-
 t/test-lib.sh           | 10 +++++-----
 8 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/t/README b/t/README
index 60d5b77bcc..94e09d025e 100644
--- a/t/README
+++ b/t/README
@@ -978,6 +978,15 @@ library for your script to use.
    output to the downstream---unlike the real version, it generates
    only up to 99 lines.
 
+ - test_bool_env <env-variable-name> <default-value>
+
+   Given the name of an environment variable with a bool value,
+   normalize its value to a 0 (true) or 1 (false or empty string)
+   return code.  Return with code corresponding to the given default
+   value if the variable is unset.
+   Abort the test script if either the value of the variable or the
+   default are not valid bool values.
+
 
 Prerequisites
 -------------
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index fb8f887080..e62569222b 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -15,7 +15,7 @@
 #
 #	test_done
 
-if ! git env--helper --type=bool --default=true --exit-code GIT_TEST_GIT_DAEMON
+if ! test_bool_env GIT_TEST_GIT_DAEMON true
 then
 	skip_all="git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)"
 	test_done
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index bc0b9c71f8..7d248e6588 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -69,7 +69,7 @@ svn_cmd () {
 maybe_start_httpd () {
 	loc=${1-svn}
 
-	if git env--helper --type=bool --default=false --exit-code GIT_TEST_SVN_HTTPD
+	if test_bool_env GIT_TEST_SVN_HTTPD false
 	then
 		. "$TEST_DIRECTORY"/lib-httpd.sh
 		LIB_HTTPD_SVN="$loc"
@@ -104,7 +104,7 @@ EOF
 }
 
 require_svnserve () {
-	if ! git env--helper --type=bool --default=false --exit-code GIT_TEST_SVNSERVE
+	if ! test_bool_env GIT_TEST_SVNSERVE false
 	then
 		skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to enable)'
 		test_done
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 0d985758c6..656997b4d6 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -41,7 +41,7 @@ then
 	test_done
 fi
 
-if ! git env--helper --type=bool --default=true --exit-code GIT_TEST_HTTPD
+if ! test_bool_env GIT_TEST_HTTPD true
 then
 	skip_all="Network testing disabled (unset GIT_TEST_HTTPD to enable)"
 	test_done
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 9ca0818cbe..a297170915 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -878,6 +878,40 @@ test_expect_success 'test_oid can look up data for SHA-256' '
 	test "$hexsz" -eq 64
 '
 
+test_expect_success 'test_bool_env' '
+	(
+		sane_unset envvar &&
+
+		test_bool_env envvar true &&
+		! test_bool_env envvar false &&
+
+		envvar= &&
+		export envvar &&
+		! test_bool_env envvar true &&
+		! test_bool_env envvar false &&
+
+		envvar=true &&
+		test_bool_env envvar true &&
+		test_bool_env envvar false &&
+
+		envvar=false &&
+		! test_bool_env envvar true &&
+		! test_bool_env envvar false &&
+
+		envvar=invalid &&
+		# When encountering an invalid bool value, test_bool_env
+		# prints its error message to the original stderr of the
+		# test script, hence the redirection of fd 7, and aborts
+		# with "exit 1", hence the subshell.
+		! ( test_bool_env envvar true ) 7>err &&
+		grep "error: test_bool_env requires bool values" err &&
+
+		envvar=true &&
+		! ( test_bool_env envvar invalid ) 7>err &&
+		grep "error: test_bool_env requires bool values" err
+	)
+'
+
 ################################################################
 # Basics of the basics
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 43e1d8d4d2..d7b9f9078f 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -267,7 +267,7 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 '
 
 test_lazy_prereq GIT_DAEMON '
-	git env--helper --type=bool --default=true --exit-code GIT_TEST_GIT_DAEMON
+	test_bool_env GIT_TEST_GIT_DAEMON true
 '
 
 # This test spawns a daemon, so run it only if the user would be OK with
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e0b3f28d3a..bf5080cb9b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1173,6 +1173,34 @@ perl () {
 	command "$PERL_PATH" "$@" 2>&7
 } 7>&2 2>&4
 
+# Given the name of an environment variable with a bool value, normalize
+# its value to a 0 (true) or 1 (false or empty string) return code.
+#
+#   test_bool_env GIT_TEST_HTTPD <default-value>
+#
+# Return with code corresponding to the given default value if the variable
+# is unset.
+# Abort the test script if either the value of the variable or the default
+# are not valid bool values.
+
+test_bool_env () {
+	if test $# != 2
+	then
+		BUG "test_bool_env requires two parameters (variable name and default value)"
+	fi
+
+	git env--helper --type=bool --default="$2" --exit-code "$1"
+	ret=$?
+	case $ret in
+	0|1)	# unset or valid bool value
+		;;
+	*)	# invalid bool value or something unexpected
+		error >&7 "test_bool_env requires bool values both for \$$1 and for the default fallback"
+		;;
+	esac
+	return $ret
+}
+
 # Exit the test suite, either by skipping all remaining tests or by
 # exiting with an error. If our prerequisite variable $1 falls back
 # on a default assume we were opportunistically trying to set up some
@@ -1181,7 +1209,7 @@ perl () {
 # The error/skip message should be given by $2.
 #
 test_skip_or_die () {
-	if ! git env--helper --type=bool --default=false --exit-code $1
+	if ! test_bool_env "$1" false
 	then
 		skip_all=$2
 		test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 30b07e310f..959568fa43 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1391,19 +1391,19 @@ yes () {
 # The GIT_TEST_FAIL_PREREQS code hooks into test_set_prereq(), and
 # thus needs to be set up really early, and set an internal variable
 # for convenience so the hot test_set_prereq() codepath doesn't need
-# to call "git env--helper". Only do that work if needed by seeing if
-# GIT_TEST_FAIL_PREREQS is set at all.
+# to call "git env--helper" (via test_bool_env). Only do that work
+# if needed by seeing if GIT_TEST_FAIL_PREREQS is set at all.
 GIT_TEST_FAIL_PREREQS_INTERNAL=
 if test -n "$GIT_TEST_FAIL_PREREQS"
 then
-	if git env--helper --type=bool --default=0 --exit-code GIT_TEST_FAIL_PREREQS
+	if test_bool_env GIT_TEST_FAIL_PREREQS false
 	then
 		GIT_TEST_FAIL_PREREQS_INTERNAL=true
 		test_set_prereq FAIL_PREREQS
 	fi
 else
 	test_lazy_prereq FAIL_PREREQS '
-		git env--helper --type=bool --default=0 --exit-code GIT_TEST_FAIL_PREREQS
+		test_bool_env GIT_TEST_FAIL_PREREQS false
 	'
 fi
 
@@ -1462,7 +1462,7 @@ then
 fi
 
 test_lazy_prereq C_LOCALE_OUTPUT '
-	! git env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON
+	! test_bool_env GIT_TEST_GETTEXT_POISON false
 '
 
 if test -z "$GIT_TEST_CHECK_CACHE_TREE"
-- 
2.24.0.532.ge18579ded8

[PATCH 2/2] t5608-clone-2gb.sh: turn GIT_TEST_CLONE_2GB into a bool

[SZEDER Gábor]

The GIT_TEST_CLONE_2GB environment variable is only ever checked with
'test -z' in 't5608-clone-2gb.sh', so any non-empty value is
interpreted as "yes, run these expensive tests", even
'GIT_TEST_CLONE_2GB=NoThanks'.

Similar GIT_TEST_* environment variables have already been turned into
bools in 3b072c577b (tests: replace test_tristate with "git
env--helper", 2019-06-21), so let's turn GIT_TEST_CLONE_2GB into a
bool as well, to follow suit.

Our CI builds set GIT_TEST_CLONE_2GB=YesPlease, so adjust them
accordingly, thus removing the last 'YesPlease' from our CI scripts.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib.sh            | 2 +-
 t/t5608-clone-2gb.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 29dc740d40..2540bb6110 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -151,7 +151,7 @@ fi
 
 export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
-export GIT_TEST_CLONE_2GB=YesPlease
+export GIT_TEST_CLONE_2GB=true
 
 case "$jobname" in
 linux-clang|linux-gcc)
diff --git a/t/t5608-clone-2gb.sh b/t/t5608-clone-2gb.sh
index 2c6bc07344..eee0842888 100755
--- a/t/t5608-clone-2gb.sh
+++ b/t/t5608-clone-2gb.sh
@@ -3,7 +3,7 @@
 test_description='Test cloning a repository larger than 2 gigabyte'
 . ./test-lib.sh
 
-if test -z "$GIT_TEST_CLONE_2GB"
+if ! test_bool_env GIT_TEST_CLONE_2GB false
 then
 	say 'Skipping expensive 2GB clone test; enable it with GIT_TEST_CLONE_2GB=t'
 else
-- 
2.24.0.532.ge18579ded8

Re: [PATCH 1/2] tests: add 'test_bool_env' to catch non-bool GIT_TEST_* values

[Jeff King]

On Fri, Nov 22, 2019 at 02:14:36PM +0100, SZEDER Gábor wrote:

> Let's be more careful about what the test suite accepts as bool values
> in GIT_TEST_* environment variables, and error out loud and clear on
> invalid values instead of simply skipping tests.  Add the
> 'test_bool_env' helper function to encapsulate the invocation of 'git
> env--helper' and the verification of its exit code, and replace all
> invocations of that command in our test framework and test suite with
> a call to this new helper (except in 't0017-env-helper.sh', of
> course).
> 
>   $ GIT_TEST_GIT_DAEMON=YesPlease ./t5570-git-daemon.sh
>   fatal: bad numeric config value 'YesPlease' for 'GIT_TEST_GIT_DAEMON': invalid unit
>   error: test_bool_env requires bool values both for $GIT_TEST_GIT_DAEMON and for the default fallback

This patch looks good to me. A few musings below, but I'm not sure if
they're worth acting on.

> +test_bool_env () {
> +	if test $# != 2
> +	then
> +		BUG "test_bool_env requires two parameters (variable name and default value)"
> +	fi
> +
> +	git env--helper --type=bool --default="$2" --exit-code "$1"
> +	ret=$?
> +	case $ret in
> +	0|1)	# unset or valid bool value
> +		;;
> +	*)	# invalid bool value or something unexpected
> +		error >&7 "test_bool_env requires bool values both for \$$1 and for the default fallback"
> +		;;
> +	esac
> +	return $ret
> +}

The magic of exit code "1" is undocumented, but we have to rely on it
here. I suggested earlier that we could do:

  if ! val=$(git env--helper --type=bool --default="$2" "$1")
    error ...
  fi
  test "$val" = "true"

but as you noted, we exit with code 1 for "false" even without
--exit-code. IMHO this is a mis-design in the interface of env--helper.

I think it would be an option to change it. It's an undocumented
double-dashed internal helper, so I don't think we need to worry about
breaking compatibility. There's only one other caller that you didn't
touch in this patch, and it uses --exit-code (more on that in a second).

> +test_expect_success 'test_bool_env' '

These tests make sense. In fact, they're much more interesting than the
ones in t0017, since these cover a superset of the code that's actually
used in practice. t0017 covers non-exit-code and --ulong invocations,
but nobody uses them!

I'm wondering if this whole env--helper thing is kind of
over-engineered. Should it actually be a test-tool helper instead of a
shipped builtin? The only call outside of the test suite is this one in
git-sh-i18n:

  # First decide what scheme to use...
  GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
  if test -n "$GIT_TEST_GETTEXT_POISON" &&
              git env--helper --type=bool --default=0 --exit-code \
                  GIT_TEST_GETTEXT_POISON
  then
          GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
  elif test -n "@@USE_GETTEXT_SCHEME@@"
  ...

which suffers from the same problem your patch is fixing. But since this
is again a test-suite thing, it seems like it would be simpler for the
test suite to just set GIT_INTERNAL_GETTEXT_SH_SCHEME=poison itself
(with a little rearranging here to let that override the "fallthrough"
case).

That would make the remaining --exit-code problem go away, remove some
test cruft from production code, and remove the last non-test-suite
caller of env--helper.

At that point we could make it a test-tool builtin. Or even implement it
purely in shell, saving some processes (that would require duplicating
the internal bool logic, but that's way shorter than the boilerplate
needed to expose it via env--helper).

I do think env--helper _could_ be useful for user scripts. But then I
think we'd need to document and rename it to make it clear that it's
part of Git's plumbing that you can depend on.

-Peff

Re: [PATCH 2/2] t5608-clone-2gb.sh: turn GIT_TEST_CLONE_2GB into a bool

[Jeff King]

On Fri, Nov 22, 2019 at 02:14:37PM +0100, SZEDER Gábor wrote:

> The GIT_TEST_CLONE_2GB environment variable is only ever checked with
> 'test -z' in 't5608-clone-2gb.sh', so any non-empty value is
> interpreted as "yes, run these expensive tests", even
> 'GIT_TEST_CLONE_2GB=NoThanks'.
> 
> Similar GIT_TEST_* environment variables have already been turned into
> bools in 3b072c577b (tests: replace test_tristate with "git
> env--helper", 2019-06-21), so let's turn GIT_TEST_CLONE_2GB into a
> bool as well, to follow suit.
> 
> Our CI builds set GIT_TEST_CLONE_2GB=YesPlease, so adjust them
> accordingly, thus removing the last 'YesPlease' from our CI scripts.

This might break the setup of some developer or tester who put a similar
value into their config.mak. But I think that's worth it to reduce
confusion in the long run, especially since they will get a very clear
error message due to your first patch.

-Peff