Re: [PATCH v2] config: introduce an Operating System-specific `includeIf` condition

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 22, 2022, 14:01 UTC
Message-ID: <221122.864juraxl2.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1429.v2.git.1669058388327.gitgitgadget@gmail.com>

On Mon, Nov 21 2022, Johannes Schindelin via GitGitGadget wrote:

Show 8 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> +`os`::
> +	The data that follows this keyword is taken as the name of an
> +	Operating System, e.g. `Linux` or `Windows`; If it matches the
> +	current Operating System, the include condition is met.
> +
>  A few more notes on matching via `gitdir` and `gitdir/i`:

The reste of the "includeif" use glob matching and "/i" for icase. IOW
this is how this new feature would fit in:
	
	|--------+--------+----------+----------+------------------+----|
	|        | gitdir | gitdir/i | onbranch | hasconfig:remote | os |
	|--------+--------+----------+----------+------------------+----|
	| icase? | N      | Y        | N        | N                | Y  |
	| glob?  | Y      | Y        | Y        | Y                | N  |
	| path?  | Y      | Y        | Y        | Y                | N  |
	|--------+--------+----------+----------+------------------+----|

I think at least flipping that "glob" to "Y" so you could match e.g. "*BSD" would be useful, and easier to explain in context, rather than why the rest use wildmatch() and this doesn't.

For matching the uname the case doesn't really matter, but for consistency of the interface I think making it case-sensitive or adding an "os/i" would make sense. I.e. let's consistently use "/i" if & when something's case-insensitive.

Show 15 quoted lines

> +test_expect_success '[includeIf "os:..."]' '
> +	test_config x.y 0 &&
> +	echo "[x] y = z" >.git/xyz &&
> +
> +	if test_have_prereq MINGW
> +	then
> +		uname_s=Windows
> +	else
> +		uname_s="$(uname -s)"
> +	fi &&
> +	test_config "includeIf.os:not-$uname_s.path" xyz &&
> +	test 0 = "$(git config x.y)" &&
> +	test_config "includeIf.os:$uname_s.path" xyz &&
> +	test z = "$(git config x.y)"
> +'

As I pointed out in the v1, this still:

 * Hides segfaults in "git config", let's check the exit code.
 * Doesn't test the "icase" semantics you're introducing. Let's do that
   if it's intentional.