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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It is relatively common for users to maintain identical `~/.gitconfig`
files across all of their setups, using the `includeIf` construct
liberally to adjust the settings to the respective setup as needed.

In case of Operating System-specific adjustments, Git currently offers
no support to the users and they typically use a work-around like this:

	[includeIf "gitdir:/home/"]
		path = ~/.gitconfig-linux
	[includeIf "gitdir:/Users/"]
		path = ~/.gitconfig-mac
	[includeIf "gitdir:C:"]
		path = ~/.gitconfig-windows

However, this is fragile, as it would not even allow to discern between
Operating Systems that happen to host their home directories in
`/home/`, such as Linux and the BSDs.

Let's introduce a new condition: `os:<uname-s>` where `<uname-s>` is the
system name, i.e. the output of `uname -s`.

This addresses https://github.com/git-for-windows/git/issues/4125

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    config: introduce an Operating System-specific includeIf condition
    
    I was about to write up guidelines how to write this patch, but it
    turned out that it was much faster to write the patch instead.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1429%2Fdscho%2Finclude-if-os-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1429/dscho/include-if-os-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1429

 Documentation/config.txt |  5 +++++
 config.c                 | 11 +++++++++++
 t/t1309-early-config.sh  | 16 ++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e376d547ce0..2db73d8228e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibiliy with
 a naming scheme that supports more variable-based include conditions,
 but currently Git only supports the exact keyword described above.
 
+`os`::
+	The data that follows this keyword is taken as the name of an
+	Operating System; If it matches the output of `uname -s`, the
+	include condition is met.
+
 A few more notes on matching via `gitdir` and `gitdir/i`:
 
  * Symlinks in `$GIT_DIR` are not resolved before matching.
diff --git a/config.c b/config.c
index 9b0e9c93285..9ab311ae99b 100644
--- a/config.c
+++ b/config.c
@@ -394,6 +394,15 @@ static int include_by_remote_url(struct config_include_data *inc,
 					     inc->remote_urls);
 }
 
+static int include_by_os(const char *cond, size_t cond_len)
+{
+	struct utsname uname_info;
+
+	return !uname(&uname_info) &&
+		!strncasecmp(uname_info.sysname, cond, cond_len) &&
+		!uname_info.sysname[cond_len];
+}
+
 static int include_condition_is_true(struct config_include_data *inc,
 				     const char *cond, size_t cond_len)
 {
@@ -408,6 +417,8 @@ static int include_condition_is_true(struct config_include_data *inc,
 	else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,
 				   &cond_len))
 		return include_by_remote_url(inc, cond, cond_len);
+	else if (skip_prefix_mem(cond, cond_len, "os:", &cond, &cond_len))
+		return include_by_os(cond, cond_len);
 
 	/* unknown conditionals are always false */
 	return 0;
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 537435b90ae..7da67af06e7 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -100,4 +100,20 @@ test_expect_success 'onbranch config outside of git repo' '
 	nongit git help
 '
 
+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)"
+'
+
 test_done

base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is relatively common for users to maintain identical `~/.gitconfig`
> files across all of their setups, using the `includeIf` construct
> liberally to adjust the settings to the respective setup as needed.
>
> In case of Operating System-specific adjustments, Git currently offers
> no support to the users and they typically use a work-around like this:
>
> 	[includeIf "gitdir:/home/"]
> 		path = ~/.gitconfig-linux
> 	[includeIf "gitdir:/Users/"]
> 		path = ~/.gitconfig-mac
> 	[includeIf "gitdir:C:"]
> 		path = ~/.gitconfig-windows
>
> However, this is fragile, as it would not even allow to discern between
> Operating Systems that happen to host their home directories in
> `/home/`, such as Linux and the BSDs.

This looks like a really sensible thing to do, thanks.

> +`os`::
> +	The data that follows this keyword is taken as the name of an
> +	Operating System; If it matches the output of `uname -s`, the
> +	include condition is met.

Here yu say it "matches uname -s", but later in the test we can see
that's not the case. This is because compat/mingw.c is the source of
that "Windows" string, which we use for the uname() at the C level.

I don't think we've needed to document it before, but we should do so
here I'd think. Per
https://stackoverflow.com/questions/3466166/how-to-check-if-running-in-cygwin-mac-or-linux
users would follow these docs, try MinGw, then be confused, no?

> +static int include_by_os(const char *cond, size_t cond_len)
> +{
> +	struct utsname uname_info;
> +
> +	return !uname(&uname_info) &&
> +		!strncasecmp(uname_info.sysname, cond, cond_len) &&

Our config.mak.uname doesn't to case-insensitive uname matching, and
AFAIK these don't change between platforms versions. So why do we need
to support LINUX, LiNuX etc. in addition to the canonical Linux?

I'm not opposed to it if there's a good reason, but as we have "gitdir"
and "gitdir/i" shouldn't we make that "os/i" for consistency, if it's
needed?

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

Re above: If it is important to support LINUX etc. these tests should
check it, they pass if it's converted to strncmp().

> +	test 0 = "$(git  config x.y)" &&

Hides segfaults, use test_cmp or test_cmp_config?

> +	test_config "includeIf.os:$uname_s.path" xyz &&
> +	test z = "$(git config x.y)"

Ditto segfault-hiding.

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

[Phillip Wood]

Hi Dscho and Ævar

On 21/11/2022 13:51, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Nov 21 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> It is relatively common for users to maintain identical `~/.gitconfig`
>> files across all of their setups, using the `includeIf` construct
>> liberally to adjust the settings to the respective setup as needed.
>>
>> In case of Operating System-specific adjustments, Git currently offers
>> no support to the users and they typically use a work-around like this:
>>
>> 	[includeIf "gitdir:/home/"]
>> 		path = ~/.gitconfig-linux
>> 	[includeIf "gitdir:/Users/"]
>> 		path = ~/.gitconfig-mac
>> 	[includeIf "gitdir:C:"]
>> 		path = ~/.gitconfig-windows
>>
>> However, this is fragile, as it would not even allow to discern between
>> Operating Systems that happen to host their home directories in
>> `/home/`, such as Linux and the BSDs.
> 
> This looks like a really sensible thing to do, thanks.

Yes, it looks like a really useful enhancement

>> +`os`::
>> +	The data that follows this keyword is taken as the name of an
>> +	Operating System; If it matches the output of `uname -s`, the

Maybe add e.g. "Windows or Linux" after Operating System?

>> +static int include_by_os(const char *cond, size_t cond_len)
>> +{
>> +	struct utsname uname_info;
>> +
>> +	return !uname(&uname_info) &&
>> +		!strncasecmp(uname_info.sysname, cond, cond_len) &&
> 
> Our config.mak.uname doesn't to case-insensitive uname matching, and
> AFAIK these don't change between platforms versions. So why do we need
> to support LINUX, LiNuX etc. in addition to the canonical Linux?
> 
> I'm not opposed to it if there's a good reason, but as we have "gitdir"
> and "gitdir/i" shouldn't we make that "os/i" for consistency, if it's
> needed?

Why should we penalize users who write "linux" rather than "Linux"? 
Making the comparison case insensitive seems eminently sensible. File 
systems can be case sensitive so having the option to specify whether a 
match for "gitdir" is case sensitive or not makes sense there but not 
for the name of an operating system.

Best Wishes

Phillip

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

Hi Phillip & Ævar,

[Ævar, in your mail, which I suggest could have used some editing for
spelling and brevity, I found only the concern about `uname -s`
relevant, and since Phillip's suggestion is more actionable than the
somewhat diffuse excurse about something to do with something in a
StackOverflow thread, I will only reply here.]

On Mon, 21 Nov 2022, Phillip Wood wrote:

> On 21/11/2022 13:51, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Mon, Nov 21 2022, Johannes Schindelin via GitGitGadget wrote:
> >
> > [...]
> > > +`os`::
> > > +	The data that follows this keyword is taken as the name of an
> > > +	Operating System; If it matches the output of `uname -s`, the
>
> Maybe add e.g. "Windows or Linux" after Operating System?

Excellent idea. I'll also drop mentioning `uname -s` because it assumes more
about the users' setup than can be assumed.

I also replaced a space character in the added test case that was doubled
by mistake with a single one.

Ciao,
Dscho

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It is relatively common for users to maintain identical `~/.gitconfig`
files across all of their setups, using the `includeIf` construct
liberally to adjust the settings to the respective setup as needed.

In case of Operating System-specific adjustments, Git currently offers
no support to the users and they typically use a work-around like this:

	[includeIf "gitdir:/home/"]
		path = ~/.gitconfig-linux
	[includeIf "gitdir:/Users/"]
		path = ~/.gitconfig-mac
	[includeIf "gitdir:C:"]
		path = ~/.gitconfig-windows

However, this is fragile, as it would not even allow to discern between
Operating Systems that happen to host their home directories in
`/home/`, such as Linux and the BSDs.

Let's introduce a new condition: `os:<uname-s>` where `<uname-s>` is the
system name, i.e. the output of `uname -s`.

This addresses https://github.com/git-for-windows/git/issues/4125

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    config: introduce an Operating System-specific includeIf condition
    
    I was about to write up guidelines how to write this patch, but it
    turned out that it was much faster to write the patch instead.
    
    Changes since v1:
    
     * The documentation now avoids mentioning uname -s and clarifies what
       it means by offering examples.
     * Replaced a double space in the test case with a single one.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1429%2Fdscho%2Finclude-if-os-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1429/dscho/include-if-os-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1429

Range-diff vs v1:

 1:  a7eb4a9d438 ! 1:  45231533883 config: introduce an Operating System-specific `includeIf` condition
     @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards
       
      +`os`::
      +	The data that follows this keyword is taken as the name of an
     -+	Operating System; If it matches the output of `uname -s`, the
     -+	include condition is met.
     ++	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`:
       
     @@ t/t1309-early-config.sh: test_expect_success 'onbranch config outside of git rep
      +		uname_s="$(uname -s)"
      +	fi &&
      +	test_config "includeIf.os:not-$uname_s.path" xyz &&
     -+	test 0 = "$(git  config x.y)" &&
     ++	test 0 = "$(git config x.y)" &&
      +	test_config "includeIf.os:$uname_s.path" xyz &&
      +	test z = "$(git config x.y)"
      +'


 Documentation/config.txt |  5 +++++
 config.c                 | 11 +++++++++++
 t/t1309-early-config.sh  | 16 ++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e376d547ce0..b90bcd8ecfe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibiliy with
 a naming scheme that supports more variable-based include conditions,
 but currently Git only supports the exact keyword described above.
 
+`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`:
 
  * Symlinks in `$GIT_DIR` are not resolved before matching.
diff --git a/config.c b/config.c
index 9b0e9c93285..9ab311ae99b 100644
--- a/config.c
+++ b/config.c
@@ -394,6 +394,15 @@ static int include_by_remote_url(struct config_include_data *inc,
 					     inc->remote_urls);
 }
 
+static int include_by_os(const char *cond, size_t cond_len)
+{
+	struct utsname uname_info;
+
+	return !uname(&uname_info) &&
+		!strncasecmp(uname_info.sysname, cond, cond_len) &&
+		!uname_info.sysname[cond_len];
+}
+
 static int include_condition_is_true(struct config_include_data *inc,
 				     const char *cond, size_t cond_len)
 {
@@ -408,6 +417,8 @@ static int include_condition_is_true(struct config_include_data *inc,
 	else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,
 				   &cond_len))
 		return include_by_remote_url(inc, cond, cond_len);
+	else if (skip_prefix_mem(cond, cond_len, "os:", &cond, &cond_len))
+		return include_by_os(cond, cond_len);
 
 	/* unknown conditionals are always false */
 	return 0;
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 537435b90ae..b36afe1a528 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -100,4 +100,20 @@ test_expect_success 'onbranch config outside of git repo' '
 	nongit git help
 '
 
+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)"
+'
+
 test_done

base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5
-- 
gitgitgadget

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

[Jeff King]

On Mon, Nov 21, 2022 at 07:19:48PM +0000, Johannes Schindelin via GitGitGadget wrote:

> It is relatively common for users to maintain identical `~/.gitconfig`
> files across all of their setups, using the `includeIf` construct
> liberally to adjust the settings to the respective setup as needed.

This seems like a reasonable thing to have in general, but I wonder if
you have an example of how people use this. Mostly I am wondering:

  - is it sometimes a misuse, where users _think_ that the OS is
    correlated with some feature of Git.  And they would be better off
    with some flag like "does the current platform support fsmonitor".

  - for cases where it really  is "uname -s" the best differentiator? Or
    would they commonly want to lump FreeBSD and Linux into the same
    category, or to tell the difference between Debian versus Fedora?

-Peff

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is relatively common for users to maintain identical `~/.gitconfig`
> files across all of their setups, using the `includeIf` construct
> liberally to adjust the settings to the respective setup as needed.
>
> In case of Operating System-specific adjustments, Git currently offers
> no support to the users and they typically use a work-around like this:
>
> 	[includeIf "gitdir:/home/"]
> 		path = ~/.gitconfig-linux
> 	[includeIf "gitdir:/Users/"]
> 		path = ~/.gitconfig-mac
> 	[includeIf "gitdir:C:"]
> 		path = ~/.gitconfig-windows
>
> However, this is fragile, as it would not even allow to discern between
> Operating Systems that happen to host their home directories in
> `/home/`, such as Linux and the BSDs.
>
> Let's introduce a new condition: `os:<uname-s>` where `<uname-s>` is the
> system name, i.e. the output of `uname -s`.

As I am not confident enough that we made the best choice that would
last forever with the initial attempt when we picked "uname -s" as
the way to switch on "OS", I wouldn't call it "OS".  Perhaps

	[includeIf "uname-s:Linux"] path = ...

would be what I would pick.  Other than that, the feature makes
quite a lot of sense.

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

[Ævar Arnfjörð Bjarmason]

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

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

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

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

[Phillip Wood]

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

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.

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

[Philippe Blain]

Hi Dscho,

Le 2022-11-21 à 14:19, Johannes Schindelin via GitGitGadget a écrit :
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> It is relatively common for users to maintain identical `~/.gitconfig`
> files across all of their setups, using the `includeIf` construct
> liberally to adjust the settings to the respective setup as needed.
> 
> In case of Operating System-specific adjustments, 

Here, as well as in the commit title, the rest of the message, and the changes to 
the doc in the patch, I would downcase "operating system". 
It's a common word that I don't think should be capitalized (in contrast to
proper OS names like Windows and Linux).

Cheers,
Philippe.

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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

You keep saying that you consider the OS name is case insensitive,
but I doubt that is the case, not in the sense that MacOS and macOS
are two different operating systems, but in the sense that OS
publishers have a single preferred way to spell their ware (which is
shown in "uname -s" output), and we should respect that.

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

[Philip Oakley]

Hi Dscho,

On 21/11/2022 19:19, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is relatively common for users to maintain identical `~/.gitconfig`
> files across all of their setups, using the `includeIf` construct
> liberally to adjust the settings to the respective setup as needed.
>
> In case of Operating System-specific adjustments, Git currently offers
> no support to the users and they typically use a work-around like this:
>
> 	[includeIf "gitdir:/home/"]
> 		path = ~/.gitconfig-linux
> 	[includeIf "gitdir:/Users/"]
> 		path = ~/.gitconfig-mac
> 	[includeIf "gitdir:C:"]
> 		path = ~/.gitconfig-windows
>
> However, this is fragile, as it would not even allow to discern between
> Operating Systems that happen to host their home directories in
> `/home/`, such as Linux and the BSDs.
>
> Let's introduce a new condition: `os:<uname-s>` where `<uname-s>` is the
> system name, i.e. the output of `uname -s`.

This `uname -s` doesn't work well on Git-for-Windows (GfW) / Windows
because GfW has its own internal compatibility code to spoof the result.
Internally GfW sets it to "Windows" (see
https://github.com/git/git/blob/master/compat/mingw.c#L3084-L3095).

If I ask for `uname -s` on the GfW bash, for me it returns
`MINGW64_NT-10.0-19045`, both on the normal GfW install, and the GfW-SDK.

If I (as dumb user) try the CMD window prompt it's
    'uname' is not recognized as an internal or external command,
    operable program or batch file.

The Windows PowerShell also doesn't recognise the uname command.

My WSL reports `Linux`, though I haven't played with that for a while
(do all *nix variants report that?).

Thus we'll need some way of assisting the users in determining the
internal system name that their local Git will find. It maybe that the
man page just needs to be explicit about using "Windows" for the
Git-for-Windows implementation.

Or less helpful, maybe a `git-uname` command to disambiguate any other
systems with a compat::uname() variant.

Or just drop the mentions of "<uname-s>" in this commit message and
rename it 'sysname' to match the field of the struct utsname?

>
> This addresses https://github.com/git-for-windows/git/issues/4125
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     config: introduce an Operating System-specific includeIf condition
>     
>     I was about to write up guidelines how to write this patch, but it
>     turned out that it was much faster to write the patch instead.
>     
>     Changes since v1:
>     
>      * The documentation now avoids mentioning uname -s and clarifies what
>        it means by offering examples.
>      * Replaced a double space in the test case with a single one.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1429%2Fdscho%2Finclude-if-os-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1429/dscho/include-if-os-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1429
>
> Range-diff vs v1:
>
>  1:  a7eb4a9d438 ! 1:  45231533883 config: introduce an Operating System-specific `includeIf` condition
>      @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards
>        
>       +`os`::
>       +	The data that follows this keyword is taken as the name of an
>      -+	Operating System; If it matches the output of `uname -s`, the
>      -+	include condition is met.
>      ++	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`:
>        
>      @@ t/t1309-early-config.sh: test_expect_success 'onbranch config outside of git rep
>       +		uname_s="$(uname -s)"
>       +	fi &&
>       +	test_config "includeIf.os:not-$uname_s.path" xyz &&
>      -+	test 0 = "$(git  config x.y)" &&
>      ++	test 0 = "$(git config x.y)" &&
>       +	test_config "includeIf.os:$uname_s.path" xyz &&
>       +	test z = "$(git config x.y)"
>       +'
>
>
>  Documentation/config.txt |  5 +++++
>  config.c                 | 11 +++++++++++
>  t/t1309-early-config.sh  | 16 ++++++++++++++++
>  3 files changed, 32 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e376d547ce0..b90bcd8ecfe 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibiliy with
>  a naming scheme that supports more variable-based include conditions,
>  but currently Git only supports the exact keyword described above.
>  
> +`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

Is 'matches' the appropriate way to indicate that we compare just the
characters from the data string and ignore any trailing chars in the
uname.sysname field?

> , the include condition is met.
> +
>  A few more notes on matching via `gitdir` and `gitdir/i`:
>  
>   * Symlinks in `$GIT_DIR` are not resolved before matching.
> diff --git a/config.c b/config.c
> index 9b0e9c93285..9ab311ae99b 100644
> --- a/config.c
> +++ b/config.c
> @@ -394,6 +394,15 @@ static int include_by_remote_url(struct config_include_data *inc,
>  					     inc->remote_urls);
>  }
>  
> +static int include_by_os(const char *cond, size_t cond_len)
> +{
> +	struct utsname uname_info;
> +
> +	return !uname(&uname_info) &&
> +		!strncasecmp(uname_info.sysname, cond, cond_len) &&
> +		!uname_info.sysname[cond_len];
> +}
> +
>  static int include_condition_is_true(struct config_include_data *inc,
>  				     const char *cond, size_t cond_len)
>  {
> @@ -408,6 +417,8 @@ static int include_condition_is_true(struct config_include_data *inc,
>  	else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,
>  				   &cond_len))
>  		return include_by_remote_url(inc, cond, cond_len);
> +	else if (skip_prefix_mem(cond, cond_len, "os:", &cond, &cond_len))
> +		return include_by_os(cond, cond_len);

(as above) We compare only on the basis of the few/many characters in
the config file so, IIUC, we could use `Win`, or `Lin` as the os:
string. Should this be noted in the man text? I'm thinking of users who
may be confused by having to change Win10 to Windows, but are happy to
shorten to `Win`.
>  
>  	/* unknown conditionals are always false */
>  	return 0;
> diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
> index 537435b90ae..b36afe1a528 100755
> --- a/t/t1309-early-config.sh
> +++ b/t/t1309-early-config.sh
> @@ -100,4 +100,20 @@ test_expect_success 'onbranch config outside of git repo' '
>  	nongit git help
>  '
>  
> +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

This (correctly) copies/follows the compat code
(https://github.com/git/git/blob/master/compat/mingw.c#L3088), but isn't
what a GfW user sees if `uname-s` is run in their bash. Maybe change
uname_s to sysname, as noted above.
> +	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)"
> +'
> +
>  test_done
>
> base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5
--
Philip

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

[Đoàn Trần Công Danh]

On 2022-11-21 18:32:11-0500, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 21, 2022 at 07:19:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
> > It is relatively common for users to maintain identical `~/.gitconfig`
> > files across all of their setups, using the `includeIf` construct
> > liberally to adjust the settings to the respective setup as needed.
> 
> This seems like a reasonable thing to have in general, but I wonder if
> you have an example of how people use this. Mostly I am wondering:
> 
>   - is it sometimes a misuse, where users _think_ that the OS is
>     correlated with some feature of Git.  And they would be better off
>     with some flag like "does the current platform support fsmonitor".

A possible use-case is setting credential.helper based on OS, let's say
libsecret on Linux, and osxkeychain on macOS. Of course, users can
have their own helper on specific OS.

> 
>   - for cases where it really  is "uname -s" the best differentiator? Or
>     would they commonly want to lump FreeBSD and Linux into the same
>     category, or to tell the difference between Debian versus Fedora?
> 
> -Peff

-- 
Danh

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

[Phillip Wood]

Hi Junio

Welcome back, I hope you enjoyed your time away from the list.

On 23/11/2022 00:16, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> 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
> 
> You keep saying that you consider the OS name is case insensitive,
> but I doubt that is the case, not in the sense that MacOS and macOS
> are two different operating systems, but in the sense that OS
> publishers have a single preferred way to spell their ware (which is
> shown in "uname -s" output), and we should respect that.

I can see that we would want to respect the preferred spelling in our 
documentation but it seems a bit mean to penalize users who write

	[IncludeIf "os:windows"]

rather than

	[IncludeIf "os:Windows"]

Best Wishes

Phillip

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I can see that we would want to respect the preferred spelling in our
> documentation but it seems a bit mean to penalize users who write
>
> 	[IncludeIf "os:windows"]
>
> rather than
>
> 	[IncludeIf "os:Windows"]

I do not think "uname-s/i:windows" is out of question.  I am saying
that the default should be case sensitive for consistency with
others.

Also, as I said already, I do not think we should squat on a good
name "os" with this "uname -s" feature that may not be a good
differenciator for two OSes for some cases (e.g. telling Debian and
Fedora apart was a good example raised upthread already).

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

[Jeff King]

On Wed, Nov 23, 2022 at 06:54:45PM +0700, Đoàn Trần Công Danh wrote:

> > This seems like a reasonable thing to have in general, but I wonder if
> > you have an example of how people use this. Mostly I am wondering:
> > 
> >   - is it sometimes a misuse, where users _think_ that the OS is
> >     correlated with some feature of Git.  And they would be better off
> >     with some flag like "does the current platform support fsmonitor".
> 
> A possible use-case is setting credential.helper based on OS, let's say
> libsecret on Linux, and osxkeychain on macOS. Of course, users can
> have their own helper on specific OS.

Thanks, that's a very nice concrete example. I agree that's a pretty
reasonable use of this feature, given the lack of other selection
mechanisms. I do wonder if folks might run into annoyances when they
want to use libsecret on Linux _and_ FreeBSD or similar. But this
feature is definitely better than the status quo, and is not very much
code to implement or support.

-Peff

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

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

>> Let's introduce a new condition: `os:<uname-s>` where `<uname-s>` is the
>> system name, i.e. the output of `uname -s`.
>
> This `uname -s` doesn't work well on Git-for-Windows (GfW) / Windows
> because GfW has its own internal compatibility code to spoof the result.
> ...
> Or just drop the mentions of "<uname-s>" in this commit message and
> rename it 'sysname' to match the field of the struct utsname?

FWIW I do not mind "sysname".  It is much better to say

	[includeIf "sysname:Linux"] path = ...

than "os:Linux", as "sysname" informs us the granularity used to
identify the system better than "os".