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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22/11/2022 14:01, Ævar Arnfjörð Bjarmason wrote:

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

From: Johannes Schindelin <johannes.schindelin@xxxxxx>

+`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.

Globbing could be useful for the BSDs.

One other thing I thought of is will users know "Darwin" means MacOS?

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.

All the other items listed in your table such as branch names are case sensitive. The os name is not so it is of no benefit at all to the user to match it case sensitively. Let's consistently test case sensitive keys cases sensitively and case insensitive keys case insensitively.

Best Wishes

Phillip

+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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux