[RFC PATCH 0/1] Add hostname condition to includeIf

[RFC PATCH 0/1] Add hostname condition to includeIf

[Ignacio Encinas]

Hello,

First of all hello everyone, and thanks for developing git :)

I recently came across [1], which proposes further extending 
includeIf by supporting "hostname" as a condition.

I thought it would be a good feature to have in git so I gave 
it a try. Let me know what you think.

If you like the idea, I would be happy to add similar conditions
like "username".

[1] https://github.com/gitgitgadget/git/issues/1665

Ignacio Encinas (1):
  config: learn the "hostname:" includeIf condition

 Documentation/config.txt  |  9 +++++++++
 config.c                  | 16 ++++++++++++++++
 t/t1305-config-include.sh | 22 ++++++++++++++++++++++
 3 files changed, 47 insertions(+)


base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
-- 
2.44.0

[RFC PATCH 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas]

Currently, customizing the configuration depending on the machine running
git has to be done manually.

Add support for a new includeIf keyword "hostname:" to conditionally
include configuration files depending on the hostname.

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
 Documentation/config.txt  |  9 +++++++++
 config.c                  | 16 ++++++++++++++++
 t/t1305-config-include.sh | 22 ++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e3a74dd1c1..9a22fd2609 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
 a naming scheme that supports more variable-based include conditions,
 but currently Git only supports the exact keyword described above.
 
+`hostname`::
+	The data that follows the keyword `hostname:` is taken to be a
+	pattern with standard globbing wildcards. If the current
+	hostname matches the pattern, 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.
@@ -261,6 +266,10 @@ Example
 	path = foo.inc
 [remote "origin"]
 	url = https://example.com/git
+
+; include only if the hostname of the machine matches some-hostname
+[includeIf "hostname:some-hostname"]
+	path = foo.inc
 ----
 
 Values
diff --git a/config.c b/config.c
index 3cfeb3d8bd..e0611fc342 100644
--- a/config.c
+++ b/config.c
@@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)
 	return ret;
 }
 
+static int include_by_hostname(const char *cond, size_t cond_len)
+{
+	int ret;
+	char my_host[HOST_NAME_MAX + 1];
+	struct strbuf pattern = STRBUF_INIT;
+	if (xgethostname(my_host, sizeof(my_host)))
+		return 0;
+
+	strbuf_add(&pattern, cond, cond_len);
+	ret = !wildmatch(pattern.buf, my_host, 0);
+	strbuf_release(&pattern);
+	return ret;
+}
+
 static int add_remote_url(const char *var, const char *value,
 			  const struct config_context *ctx UNUSED, void *data)
 {
@@ -406,6 +420,8 @@ static int include_condition_is_true(const struct key_value_info *kvi,
 	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, "hostname:", &cond, &cond_len))
+		return include_by_hostname(cond, cond_len);
 
 	/* unknown conditionals are always false */
 	return 0;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 5cde79ef8c..ee78d9cade 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '
 	grep "exceeded maximum include depth" stderr
 '
 
+test_expect_success 'conditional include, hostname' '
+	echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
+	echo "[test]twelve=12" >.git/bar12 &&
+	test_must_fail git config test.twelve &&
+
+	echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
+	echo 12 >expect &&
+	git config test.twelve >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'conditional include, hostname, wildcard' '
+	echo "[includeIf \"hostname:$(hostname)a*\"]path=bar13" >>.git/config &&
+	echo "[test]thirteen=13" >.git/bar13 &&
+	test_must_fail git config test.thirteen &&
+
+	echo "[includeIf \"hostname:$(hostname)*\"]path=bar13" >>.git/config &&
+	echo 13 >expect &&
+	git config test.thirteen >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0

Re: [RFC PATCH 1/1] config: learn the "hostname:" includeIf condition

[Junio C Hamano]

Ignacio Encinas <ignacio@iencinas.com> writes:

> Currently, customizing the configuration depending on the machine running
> git has to be done manually.
>
> Add support for a new includeIf keyword "hostname:" to conditionally
> include configuration files depending on the hostname.
>
> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
> ---
>  Documentation/config.txt  |  9 +++++++++
>  config.c                  | 16 ++++++++++++++++
>  t/t1305-config-include.sh | 22 ++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e3a74dd1c1..9a22fd2609 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>  a naming scheme that supports more variable-based include conditions,
>  but currently Git only supports the exact keyword described above.

> +`hostname`::
> +	The data that follows the keyword `hostname:` is taken to be a
> +	pattern with standard globbing wildcards. If the current
> +	hostname matches the pattern, the include condition is met.
> +

OK.  This seems to copy its phrasing from the existing text for
"gitdir" and "onbranch", which greatly helps the description for
these features consistent.

> diff --git a/config.c b/config.c
> index 3cfeb3d8bd..e0611fc342 100644
> --- a/config.c
> +++ b/config.c
> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)
>  	return ret;
>  }
>  
> +static int include_by_hostname(const char *cond, size_t cond_len)
> +{
> +	int ret;
> +	char my_host[HOST_NAME_MAX + 1];
> +	struct strbuf pattern = STRBUF_INIT;
> +	if (xgethostname(my_host, sizeof(my_host)))
> +		return 0;
> +
> +	strbuf_add(&pattern, cond, cond_len);
> +	ret = !wildmatch(pattern.buf, my_host, 0);
> +	strbuf_release(&pattern);
> +	return ret;
> +}

Have a blank line between the end of the decl block (our codebase
frowns upon decl-after-statement) and the first statement,
i.e. before "if (xgethostname...".

Otherwise this looks reasonable to me.

> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index 5cde79ef8c..ee78d9cade 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '
>  	grep "exceeded maximum include depth" stderr
>  '
>  
> +test_expect_success 'conditional include, hostname' '
> +	echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
> +	echo "[test]twelve=12" >.git/bar12 &&
> +	test_must_fail git config test.twelve &&

Emulating other tests in this file that uses here document may make
it a bit easier to read?  E.g.,

	cat >>.gitconfig <<-EOF &&
	[includeIf "hostname:$(hostname)a"]
		path = bar12
	EOF

> +	echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&

Ditto for the remainder of the patch.

Thanks.

Re: [RFC PATCH 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 7/3/24 22:40, Junio C Hamano wrote:
> Ignacio Encinas <ignacio@iencinas.com> writes:
> 
>> Currently, customizing the configuration depending on the machine running
>> git has to be done manually.
>>
>> Add support for a new includeIf keyword "hostname:" to conditionally
>> include configuration files depending on the hostname.
>>
>> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
>> ---
>>  Documentation/config.txt  |  9 +++++++++
>>  config.c                  | 16 ++++++++++++++++
>>  t/t1305-config-include.sh | 22 ++++++++++++++++++++++
>>  3 files changed, 47 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e3a74dd1c1..9a22fd2609 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>>  a naming scheme that supports more variable-based include conditions,
>>  but currently Git only supports the exact keyword described above.
> 
>> +`hostname`::
>> +	The data that follows the keyword `hostname:` is taken to be a
>> +	pattern with standard globbing wildcards. If the current
>> +	hostname matches the pattern, the include condition is met.
>> +
> 
> OK.  This seems to copy its phrasing from the existing text for
> "gitdir" and "onbranch", which greatly helps the description for
> these features consistent.
> 
>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..e0611fc342 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)
>>  	return ret;
>>  }
>>  
>> +static int include_by_hostname(const char *cond, size_t cond_len)
>> +{
>> +	int ret;
>> +	char my_host[HOST_NAME_MAX + 1];
>> +	struct strbuf pattern = STRBUF_INIT;
>> +	if (xgethostname(my_host, sizeof(my_host)))
>> +		return 0;
>> +
>> +	strbuf_add(&pattern, cond, cond_len);
>> +	ret = !wildmatch(pattern.buf, my_host, 0);
>> +	strbuf_release(&pattern);
>> +	return ret;
>> +}
> 
> Have a blank line between the end of the decl block (our codebase
> frowns upon decl-after-statement) and the first statement,
> i.e. before "if (xgethostname...".
> 
> Otherwise this looks reasonable to me.

Got it. 
 
>> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
>> index 5cde79ef8c..ee78d9cade 100755
>> --- a/t/t1305-config-include.sh
>> +++ b/t/t1305-config-include.sh
>> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '
>>  	grep "exceeded maximum include depth" stderr
>>  '
>>  
>> +test_expect_success 'conditional include, hostname' '
>> +	echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
>> +	echo "[test]twelve=12" >.git/bar12 &&
>> +	test_must_fail git config test.twelve &&
> 
> Emulating other tests in this file that uses here document may make
> it a bit easier to read?  E.g.,
> 
> 	cat >>.gitconfig <<-EOF &&
> 	[includeIf "hostname:$(hostname)a"]
> 		path = bar12
> 	EOF

Thanks for pointing that out. I just read the last tests from that file
where they used the echo "..." >> approach. Do you think it is
worthwhile rewriting those tests to use the approach you suggested?

By the way, before contributing, I saw there is some work on moving to
unit tests. I wasn't sure how to test this particular feature there, so
I went with the "old" approach as it seemed more natural. Is this ok?

>> +	echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
> 
> Ditto for the remainder of the patch.
> 
> Thanks.

Thank you for the review.

Re: [RFC PATCH 1/1] config: learn the "hostname:" includeIf condition

[Junio C Hamano]

Ignacio Encinas Rubio <ignacio@iencinas.com> writes:

>>> +test_expect_success 'conditional include, hostname' '
>>> +	echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
>>> +	echo "[test]twelve=12" >.git/bar12 &&
>>> +	test_must_fail git config test.twelve &&
>> 
>> Emulating other tests in this file that uses here document may make
>> it a bit easier to read?  E.g.,
>> 
>> 	cat >>.gitconfig <<-EOF &&
>> 	[includeIf "hostname:$(hostname)a"]
>> 		path = bar12
>> 	EOF
>
> Thanks for pointing that out. I just read the last tests from that file
> where they used the echo "..." >> approach. Do you think it is
> worthwhile rewriting those tests to use the approach you suggested?

I had an impression that existing ones do not have ugliness of
backslash-quoting and do not benefit from such a rewrite to use
here-document as much as the one you added does.  

If that is not the case, and existing ones would concretely improve
the readability with such a rewrite to use here-document, surely.
If we want to do that route, we should either do one of the two.

 - The patch [1/2] does such a "style update" only on existing tests
   to improve their readability, and then patch [2/2] then does your
   addition to the tests, together with the code change.

 - Or you do this patch alone, without touching existing tests, but
   with your tests added in an easier-to-read style.  And then after
   the dust settles, a separate "style udpate" patch clean the
   existing ones up.

> By the way, before contributing, I saw there is some work on moving to
> unit tests. I wasn't sure how to test this particular feature there, so
> I went with the "old" approach as it seemed more natural. Is this ok?

We are not really "moving to".  We did not have a test framework for
effective unit tests, so some tests that would have been easier to
manage as unit tests were instead written as an end-to-end
integration tests, which was what we had a framework for.  These
tests are moving to, but for a test like "the user uses the
'[includeIf X:Y] path = P' construct---does the git command really
shows the effect of include from P when condition X:Y holds?", the
unit testing framework would not be a better fit than the end-to-end
behaviour test, I would say.

[PATCH v2 0/1] Add hostname condition to includeIf

[Ignacio Encinas]

Extend includeIf to take hostname into account. Motivating request can
be found here [1].

[1] https://github.com/gitgitgadget/git/issues/1665

Changes since v1:
* Add blank line between declarations and code in `include_by_branch`.
* Rewrite "echo"s used in tests to make them more readable. 

  config: learn the "hostname:" includeIf condition

 Documentation/config.txt  |  9 +++++++++
 config.c                  | 17 ++++++++++++++++
 t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)

Range-diff against v1:
1:  10a9bca68753 ! 1:  cf175154109e config: learn the "hostname:" includeIf condition
    @@ config.c: static int include_by_branch(const char *cond, size_t cond_len)
     +	int ret;
     +	char my_host[HOST_NAME_MAX + 1];
     +	struct strbuf pattern = STRBUF_INIT;
    ++
     +	if (xgethostname(my_host, sizeof(my_host)))
     +		return 0;
     +
    @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '
      '
      
     +test_expect_success 'conditional include, hostname' '
    -+	echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
    -+	echo "[test]twelve=12" >.git/bar12 &&
    ++	cat >>.git/config <<-EOF &&
    ++	[includeIf "hostname:$(hostname)a"]
    ++		path = bar12
    ++	EOF
    ++	cat >>.git/bar12 <<-EOF &&
    ++	[test]
    ++		twelve=12
    ++	EOF
    ++
     +	test_must_fail git config test.twelve &&
     +
    -+	echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
    ++	cat >>.git/config <<-EOF &&
    ++	[includeIf "hostname:$(hostname)"]
    ++		path = bar12
    ++	EOF
     +	echo 12 >expect &&
     +	git config test.twelve >actual &&
     +	test_cmp expect actual
     +'
     +
     +test_expect_success 'conditional include, hostname, wildcard' '
    -+	echo "[includeIf \"hostname:$(hostname)a*\"]path=bar13" >>.git/config &&
    -+	echo "[test]thirteen=13" >.git/bar13 &&
    ++	cat >>.git/config <<-EOF &&
    ++	[includeIf "hostname:$(hostname)a*"]
    ++		path = bar13
    ++	EOF
    ++	cat >>.git/bar13 <<-EOF &&
    ++	[test]
    ++		thirteen = 13
    ++	EOF
    ++
     +	test_must_fail git config test.thirteen &&
     +
    -+	echo "[includeIf \"hostname:$(hostname)*\"]path=bar13" >>.git/config &&
    ++	cat >>.git/config <<-EOF &&
    ++	[includeIf "hostname:$(hostname)*"]
    ++		path = bar13
    ++	EOF
     +	echo 13 >expect &&
     +	git config test.thirteen >actual &&
     +	test_cmp expect actual

base-commit: e09f1254c54329773904fe25d7c545a1fb4fa920
-- 
2.44.0

[PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas]

Currently, customizing the configuration depending on the machine running
git has to be done manually.

Add support for a new includeIf keyword "hostname:" to conditionally
include configuration files depending on the hostname.

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
 Documentation/config.txt  |  9 +++++++++
 config.c                  | 17 ++++++++++++++++
 t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e3a74dd1c19d..9a22fd260935 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
 a naming scheme that supports more variable-based include conditions,
 but currently Git only supports the exact keyword described above.
 
+`hostname`::
+	The data that follows the keyword `hostname:` is taken to be a
+	pattern with standard globbing wildcards. If the current
+	hostname matches the pattern, 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.
@@ -261,6 +266,10 @@ Example
 	path = foo.inc
 [remote "origin"]
 	url = https://example.com/git
+
+; include only if the hostname of the machine matches some-hostname
+[includeIf "hostname:some-hostname"]
+	path = foo.inc
 ----
 
 Values
diff --git a/config.c b/config.c
index 3cfeb3d8bd99..50b3f6d24c50 100644
--- a/config.c
+++ b/config.c
@@ -317,6 +317,21 @@ static int include_by_branch(const char *cond, size_t cond_len)
 	return ret;
 }
 
+static int include_by_hostname(const char *cond, size_t cond_len)
+{
+	int ret;
+	char my_host[HOST_NAME_MAX + 1];
+	struct strbuf pattern = STRBUF_INIT;
+
+	if (xgethostname(my_host, sizeof(my_host)))
+		return 0;
+
+	strbuf_add(&pattern, cond, cond_len);
+	ret = !wildmatch(pattern.buf, my_host, 0);
+	strbuf_release(&pattern);
+	return ret;
+}
+
 static int add_remote_url(const char *var, const char *value,
 			  const struct config_context *ctx UNUSED, void *data)
 {
@@ -406,6 +421,8 @@ static int include_condition_is_true(const struct key_value_info *kvi,
 	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, "hostname:", &cond, &cond_len))
+		return include_by_hostname(cond, cond_len);
 
 	/* unknown conditionals are always false */
 	return 0;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 5cde79ef8c4f..e0a1d51d50ad 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -357,4 +357,46 @@ test_expect_success 'include cycles are detected' '
 	grep "exceeded maximum include depth" stderr
 '
 
+test_expect_success 'conditional include, hostname' '
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(hostname)a"]
+		path = bar12
+	EOF
+	cat >>.git/bar12 <<-EOF &&
+	[test]
+		twelve=12
+	EOF
+
+	test_must_fail git config test.twelve &&
+
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(hostname)"]
+		path = bar12
+	EOF
+	echo 12 >expect &&
+	git config test.twelve >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'conditional include, hostname, wildcard' '
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(hostname)a*"]
+		path = bar13
+	EOF
+	cat >>.git/bar13 <<-EOF &&
+	[test]
+		thirteen = 13
+	EOF
+
+	test_must_fail git config test.thirteen &&
+
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(hostname)*"]
+		path = bar13
+	EOF
+	echo 13 >expect &&
+	git config test.thirteen >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Junio C Hamano]

Ignacio Encinas <ignacio@iencinas.com> writes:

> +test_expect_success 'conditional include, hostname' '
> +	cat >>.git/config <<-EOF &&
> +	[includeIf "hostname:$(hostname)a"]

This unconditionally runs the $(hostname) command assuming it exists
everywhere, but

    $ git grep '$(hostname' t/
    t/t6500-gc.sh:	hostname=$(hostname || echo unknown) &&

tells us that we should be prepared to meet a platform where such a
command does not exist.

I have a feeling that it is better done with a test prerequisite
than hardcoded "unknown", as xgethostname() at C level may come up
with some random string but it is not guaranteed to be "unknown".

Perhaps have one instance of this before these added tests

	test_lazy_prereq WORKING_HOSTNAME '
		hostname >/dev/null 2>&1
	'

and then start them with

	test_expect_success WORKING_HOSTNAME 'hostname: includeIf' '
		...
	'

or something?  Others may think of a better way to make sure this
test does not cause false failures on platforms only because they
lack working hostname(1) but have a working gethostname(2) and their
xgethostname() may be working fine.

Thanks.

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 10/3/24 17:59, Junio C Hamano wrote:
> Ignacio Encinas <ignacio@iencinas.com> writes:
> 
>> +test_expect_success 'conditional include, hostname' '
>> +	cat >>.git/config <<-EOF &&
>> +	[includeIf "hostname:$(hostname)a"]
> 
> This unconditionally runs the $(hostname) command assuming it exists
> everywhere, but
> 
>     $ git grep '$(hostname' t/
>     t/t6500-gc.sh:	hostname=$(hostname || echo unknown) &&
> 
> tells us that we should be prepared to meet a platform where such a
> command does not exist.

Oops, it didn't even cross my mind. Thanks for the catch!

> I have a feeling that it is better done with a test prerequisite
> than hardcoded "unknown", as xgethostname() at C level may come up
> with some random string but it is not guaranteed to be "unknown".

I agree. Not being able to query the current hostname defeats the
purpose of the tests.

> Perhaps have one instance of this before these added tests
> 
> 	test_lazy_prereq WORKING_HOSTNAME '
> 		hostname >/dev/null 2>&1
> 	'
> 
> and then start them with
> 
> 	test_expect_success WORKING_HOSTNAME 'hostname: includeIf' '
> 		...
> 	'
 
Thanks for providing an example. 

> or something?  Others may think of a better way to make sure this
> test does not cause false failures on platforms only because they
> lack working hostname(1) but have a working gethostname(2) and their
> xgethostname() may be working fine.

I can't think of any room for improvement other than integrating
hostname (or a custom hostname) into git and using it in the tests, but
I doubt it is worth it.

> Thanks.

Thank you. I will wait a couple of days to post the v3 to see if anyone 
has a suggestion.

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Junio C Hamano]

Ignacio Encinas Rubio <ignacio@iencinas.com> writes:

> I can't think of any room for improvement other than integrating
> hostname (or a custom hostname) into git and using it in the tests, but
> I doubt it is worth it.

Ah, that is a thought.  We have t/helper that builds "test-tool"
just for that, and exposing the output of xhostname() does sounds
like a reasonable way to go.  It would roughly involve

 * Add t/helper/test-xhostname.c that defines cmd__xhostname() and
   writes the result of calling xhostname() to its standard output.

 * Plumb it through by adding it to a few places:

   - t/helper/test-tool.h wants the extern definition.
   - t/helper/test-tool.c wants it in its cmds[] array.
   - Makefile wants to list it in TEST_BUILTIN_OBJS

 * Then use "test-tool xhostname" in your tests, instead of
   "hostname".

You can run

    $ git grep chmtime ':!t/*.sh"

to find places that needed to be touched when a similar internal
tool "chmtime" was added.

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 11/3/24 18:39, Junio C Hamano wrote:
> Ignacio Encinas Rubio <ignacio@iencinas.com> writes:
> 
>> I can't think of any room for improvement other than integrating
>> hostname (or a custom hostname) into git and using it in the tests, but
>> I doubt it is worth it.
> 
> Ah, that is a thought.  We have t/helper that builds "test-tool"
> just for that, and exposing the output of xhostname() does sounds
> like a reasonable way to go.  It would roughly involve

Great! I hadn't noticed "test-tool". Just to double-check, what name do
we want to use for this? xhostname, hostname, xgethostname, gethostname?

If I didn't miss something, the only place the test use hostname is in 

    $ git grep '$(hostname' t/
    t/t6500-gc.sh:	hostname=$(hostname || echo unknown) &&

as you previously pointed out. So my plan is:

1. Extend test-tool, migrate t6500-gc.sh to test-tool xhostname(*)
2. Update my v2 to use "test-tool xhostname(*)"

(*) or however we want to name it

>  * Add t/helper/test-xhostname.c that defines cmd__xhostname() and
>    writes the result of calling xhostname() to its standard output.
> 
>  * Plumb it through by adding it to a few places:
> 
>    - t/helper/test-tool.h wants the extern definition.
>    - t/helper/test-tool.c wants it in its cmds[] array.
>    - Makefile wants to list it in TEST_BUILTIN_OBJS
> 
>  * Then use "test-tool xhostname" in your tests, instead of
>    "hostname".
> 
> You can run
> 
>     $ git grep chmtime ':!t/*.sh"
> 
> to find places that needed to be touched when a similar internal
> tool "chmtime" was added.

Thank you very much for the pointers, they were very helpful!

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Jeff King]

On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e3a74dd1c19d..9a22fd260935 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>  a naming scheme that supports more variable-based include conditions,
>  but currently Git only supports the exact keyword described above.
>  
> +`hostname`::
> +	The data that follows the keyword `hostname:` is taken to be a
> +	pattern with standard globbing wildcards. If the current
> +	hostname matches the pattern, the include condition is met.

Do we need to define "hostname" in more detail here? Specifically, I'm
wondering whether the result will be a FQDN or not (i.e., the output of
"hostname" vs "hostname -f"). Looking at the code I think it will just
be the short name returned. That's probably OK, but it may be worth
documenting.

-Peff

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 16/3/24 7:57, Jeff King wrote:
> On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
> 
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e3a74dd1c19d..9a22fd260935 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>>  a naming scheme that supports more variable-based include conditions,
>>  but currently Git only supports the exact keyword described above.
>>  
>> +`hostname`::
>> +	The data that follows the keyword `hostname:` is taken to be a
>> +	pattern with standard globbing wildcards. If the current
>> +	hostname matches the pattern, the include condition is met.
> 
> Do we need to define "hostname" in more detail here? Specifically, I'm
> wondering whether the result will be a FQDN or not (i.e., the output of
> "hostname" vs "hostname -f"). Looking at the code I think it will just
> be the short name returned. That's probably OK, but it may be worth
> documenting.

Thanks for pointing it out. I agree that it should be further clarified. 

Indeed, I was referring to the short name reported by gethostname(2), 
which should agree with "hostname". What do you think about

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9a22fd260935..268a9fab7be0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -189,7 +189,8 @@ but currently Git only supports the exact keyword described above.
 `hostname`::
        The data that follows the keyword `hostname:` is taken to be a
        pattern with standard globbing wildcards. If the current
-       hostname matches the pattern, the include condition is met.
+       hostname (output of gethostname(2)) matches the
+       pattern, the include condition is met.

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Taylor Blau]

On Sat, Mar 16, 2024 at 12:19:44PM +0100, Ignacio Encinas Rubio wrote:
>
>
> On 16/3/24 7:57, Jeff King wrote:
> > On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
> >
> >> diff --git a/Documentation/config.txt b/Documentation/config.txt
> >> index e3a74dd1c19d..9a22fd260935 100644
> >> --- a/Documentation/config.txt
> >> +++ b/Documentation/config.txt
> >> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
> >>  a naming scheme that supports more variable-based include conditions,
> >>  but currently Git only supports the exact keyword described above.
> >>
> >> +`hostname`::
> >> +	The data that follows the keyword `hostname:` is taken to be a
> >> +	pattern with standard globbing wildcards. If the current
> >> +	hostname matches the pattern, the include condition is met.
> >
> > Do we need to define "hostname" in more detail here? Specifically, I'm
> > wondering whether the result will be a FQDN or not (i.e., the output of
> > "hostname" vs "hostname -f"). Looking at the code I think it will just
> > be the short name returned. That's probably OK, but it may be worth
> > documenting.
>
> Thanks for pointing it out. I agree that it should be further clarified.
>
> Indeed, I was referring to the short name reported by gethostname(2),
> which should agree with "hostname". What do you think about
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9a22fd260935..268a9fab7be0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -189,7 +189,8 @@ but currently Git only supports the exact keyword described above.
>  `hostname`::
>         The data that follows the keyword `hostname:` is taken to be a
>         pattern with standard globbing wildcards. If the current
> -       hostname matches the pattern, the include condition is met.
> +       hostname (output of gethostname(2)) matches the

Hmm. gethostname(2)'s manual page isn't overly specific on the details
here, either.

I admittedly don't love the idea of documenting this implementation
detail (that is, that we are calling gethostname() and using its output
to compare against). I think it's fine to say instead, "the short
hostname", and leave it at that.

Alternatively, you could say "If the machine's short hostname (as
opposed to a fully-qualified hostname, as returned by `hostname -f`)
matches the pattern [...]".

I think I have a vague preference towards the latter.

Thanks,
Taylor

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Taylor Blau]

Hi Ignacio,

On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
> ---
>  Documentation/config.txt  |  9 +++++++++
>  config.c                  | 17 ++++++++++++++++
>  t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+)

I took a look at this patch and felt that it was all very sensible. I
left one comment in reply to the sub-thread with you and Peff with some
minor suggestions on the documentation.

Otherwise, the code changes here all look reasonable to me.

Thanks,
Taylor

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 16/3/24 17:00, Taylor Blau wrote:
> On Sat, Mar 16, 2024 at 12:19:44PM +0100, Ignacio Encinas Rubio wrote:
>>
>>
>> On 16/3/24 7:57, Jeff King wrote:
>>> On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
>>>
>>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>>> index e3a74dd1c19d..9a22fd260935 100644
>>>> --- a/Documentation/config.txt
>>>> +++ b/Documentation/config.txt
>>>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>>>>  a naming scheme that supports more variable-based include conditions,
>>>>  but currently Git only supports the exact keyword described above.
>>>>
>>>> +`hostname`::
>>>> +	The data that follows the keyword `hostname:` is taken to be a
>>>> +	pattern with standard globbing wildcards. If the current
>>>> +	hostname matches the pattern, the include condition is met.
>>>
>>> Do we need to define "hostname" in more detail here? Specifically, I'm
>>> wondering whether the result will be a FQDN or not (i.e., the output of
>>> "hostname" vs "hostname -f"). Looking at the code I think it will just
>>> be the short name returned. That's probably OK, but it may be worth
>>> documenting.
>>
>> Thanks for pointing it out. I agree that it should be further clarified.
>>
>> Indeed, I was referring to the short name reported by gethostname(2),
>> which should agree with "hostname". What do you think about
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9a22fd260935..268a9fab7be0 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -189,7 +189,8 @@ but currently Git only supports the exact keyword described above.
>>  `hostname`::
>>         The data that follows the keyword `hostname:` is taken to be a
>>         pattern with standard globbing wildcards. If the current
>> -       hostname matches the pattern, the include condition is met.
>> +       hostname (output of gethostname(2)) matches the
> 
> Hmm. gethostname(2)'s manual page isn't overly specific on the details
> here, either.
> 
> I admittedly don't love the idea of documenting this implementation
> detail (that is, that we are calling gethostname() and using its output
> to compare against). I think it's fine to say instead, "the short
> hostname", and leave it at that.

I agree it isn't too descriptive, but the reason I chose to do it was
because this doesn't seem thoroughly documented anywhere:

hostname(1):

  hostname will print the name of the system as returned by the gethostname(2) function.

       -s, --short
              Display the short host name. This is the host name cut at the first dot.

I have only superficial knowledge about the terminology, but from what I
have read, it seems like we're actually reading the "nodename" (see
uname(2)), which shouldn't but can contain dots ".", which "hostname -s"
will trim, but "hostname" won't.

After seeing all this and the huge potential for confusing everybody, I
chose the easy way out.

I'm ok with saying "short hostname" but I'm not terribly happy with it
as it won't match "hostname -s" if "nodename" has dots (it will always
match "hostname" from what I have seen in the hostname source code from
the debian package which I assume everyone uses).

Do you think this is worth worrying about? Or people with "nodename"s
making "hostname" and "hostname --short" disagree should know that by
short hostname we mean "hostname" and not "hostname --short".

I might be missing something, but I somehow find all of this pretty
confusing.

> Alternatively, you could say "If the machine's short hostname (as
> opposed to a fully-qualified hostname, as returned by `hostname -f`)
> matches the pattern [...]".
> 
> I think I have a vague preference towards the latter.
> Thanks,
> Taylor

Thank you for the review!

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 16/3/24 17:01, Taylor Blau wrote:
> Hi Ignacio,
> 
> On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
>> ---
>>  Documentation/config.txt  |  9 +++++++++
>>  config.c                  | 17 ++++++++++++++++
>>  t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 68 insertions(+)
> 
> I took a look at this patch and felt that it was all very sensible. I
> left one comment in reply to the sub-thread with you and Peff with some
> minor suggestions on the documentation.
>
> Otherwise, the code changes here all look reasonable to me.

Thanks for the review!
 
> Thanks,
> Taylor

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Do we need to define "hostname" in more detail here? Specifically, I'm
> wondering whether the result will be a FQDN or not (i.e., the output of
> "hostname" vs "hostname -f"). Looking at the code I think it will just
> be the short name returned. That's probably OK, but it may be worth
> documenting.

That was my first reaction but there are places where "hostname"
already gives a name that is not "short" at all, without being
invoked with "-f".

For example, the (virtual) workstation I am typing this message on
sits in a $WORK datacenter, where "hostname" gives the same string
as "hostname -f", which looks like "git.c.xxxxxx.tld" ("git" is the
only part I picked myself for it, "c" is shared by those employee
workstations hosted at datacenters, "xxxxxx.tld" is redacted to
conceal the real domain name to protect the culprits ;-).

I think the most honest answer we can give in the documentation is
that we use what gethostname() [*] gives.


[References]

* https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html

RE: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[rsbecker]

On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>
>> Do we need to define "hostname" in more detail here? Specifically, I'm
>> wondering whether the result will be a FQDN or not (i.e., the output
>> of "hostname" vs "hostname -f"). Looking at the code I think it will
>> just be the short name returned. That's probably OK, but it may be
>> worth documenting.
>
>That was my first reaction but there are places where "hostname"
>already gives a name that is not "short" at all, without being invoked with
"-f".
>
>For example, the (virtual) workstation I am typing this message on sits in
a $WORK datacenter, where "hostname" gives the same
>string as "hostname -f", which looks like "git.c.xxxxxx.tld" ("git" is the
only part I picked myself for it, "c" is shared by those employee
>workstations hosted at datacenters, "xxxxxx.tld" is redacted to conceal the
real domain name to protect the culprits ;-).
>
>I think the most honest answer we can give in the documentation is that we
use what gethostname() [*] gives.

I think this is probably a good idea and but value should not be cached. My
dev box has a multi-home, multi-cpu IP stack. It makes things really weird
sometimes. For example, hostname replies with:

ztc0.xxxxxxxx.local

and includes the current default IP stack, which is known to DNS, while
uname -n, which I prefer to use when deciding what system I am on during
tests, reports:

xxxxxxxx

I am not sure how meaningful hostname is; however, "hostname -f" is not
portable. However, includeif depending on whatever gethostname() returns is
reasonable, in my opinion, also. I think the series should include a $(uname
-n) option in some form for completeness.

>
>
>[References]
>
>*
https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html

--Randall

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 16/3/24 18:41, rsbecker@nexbridge.com wrote:
> On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> Do we need to define "hostname" in more detail here? Specifically, I'm
>>> wondering whether the result will be a FQDN or not (i.e., the output
>>> of "hostname" vs "hostname -f"). Looking at the code I think it will
>>> just be the short name returned. That's probably OK, but it may be
>>> worth documenting.
>>
>> That was my first reaction but there are places where "hostname"
>> already gives a name that is not "short" at all, without being invoked with
> "-f".
>>
>> For example, the (virtual) workstation I am typing this message on sits in
> a $WORK datacenter, where "hostname" gives the same
>> string as "hostname -f", which looks like "git.c.xxxxxx.tld" ("git" is the
> only part I picked myself for it, "c" is shared by those employee
>> workstations hosted at datacenters, "xxxxxx.tld" is redacted to conceal the
> real domain name to protect the culprits ;-).
>>
>> I think the most honest answer we can give in the documentation is that we
> use what gethostname() [*] gives.
> 
> I think this is probably a good idea and but value should not be cached. My
> dev box has a multi-home, multi-cpu IP stack. It makes things really weird
> sometimes. For example, hostname replies with:
> 
> ztc0.xxxxxxxx.local
> 
> and includes the current default IP stack, which is known to DNS, while
> uname -n, which I prefer to use when deciding what system I am on during
> tests, reports:
> 
> xxxxxxxx
> 
> I am not sure how meaningful hostname is; however, "hostname -f" is not
> portable. However, includeif depending on whatever gethostname() returns is
> reasonable, in my opinion, also. I think the series should include a $(uname
> -n) option in some form for completeness.

Correct me if I'm wrong, but gethostname() seems to be equivalent to
$(uname -n)

[1] https://git.musl-libc.org/cgit/musl/tree/src/unistd/gethostname.c 
[2] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/gethostname.c;h=3c50706b5823368a0b3e876491e554461a4d515e;hb=HEAD

>>
>>
>> [References]
>>
>> *
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html
> 
> --Randall
> 

RE: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[rsbecker]

On Saturday, March 16, 2024 2:06 PM, Ignacio Encinas Rubio wrote:
>On 16/3/24 18:41, rsbecker@nexbridge.com wrote:
>> On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> Do we need to define "hostname" in more detail here? Specifically,
>>>> I'm wondering whether the result will be a FQDN or not (i.e., the
>>>> output of "hostname" vs "hostname -f"). Looking at the code I think
>>>> it will just be the short name returned. That's probably OK, but it
>>>> may be worth documenting.
>>>
>>> That was my first reaction but there are places where "hostname"
>>> already gives a name that is not "short" at all, without being
>>> invoked with
>> "-f".
>>>
>>> For example, the (virtual) workstation I am typing this message on
>>> sits in
>> a $WORK datacenter, where "hostname" gives the same
>>> string as "hostname -f", which looks like "git.c.xxxxxx.tld" ("git"
>>> is the
>> only part I picked myself for it, "c" is shared by those employee
>>> workstations hosted at datacenters, "xxxxxx.tld" is redacted to
>>> conceal the
>> real domain name to protect the culprits ;-).
>>>
>>> I think the most honest answer we can give in the documentation is
>>> that we
>> use what gethostname() [*] gives.
>>
>> I think this is probably a good idea and but value should not be
>> cached. My dev box has a multi-home, multi-cpu IP stack. It makes
>> things really weird sometimes. For example, hostname replies with:
>>
>> ztc0.xxxxxxxx.local
>>
>> and includes the current default IP stack, which is known to DNS,
>> while uname -n, which I prefer to use when deciding what system I am
>> on during tests, reports:
>>
>> xxxxxxxx
>>
>> I am not sure how meaningful hostname is; however, "hostname -f" is
>> not portable. However, includeif depending on whatever gethostname()
>> returns is reasonable, in my opinion, also. I think the series should
>> include a $(uname
>> -n) option in some form for completeness.
>
>Correct me if I'm wrong, but gethostname() seems to be equivalent to $(uname -n)

Glibc definitely uses uname, according to its man page, but that is the exception, not the rule. Evidence from my experimentation on various platforms says that the two values may sometimes be the same but the host configuration may be different, particularly if two stacks are on the same machine with different IP addresses. uname does not go to DNS. gethostname() generally (Windows, S/390, NonStop, Linux where glibc is not used), uses DNS as its first attempt to resolve the name.

Re: [PATCH v2 1/1] config: learn the "hostname:" includeIf condition

[Jeff King]

On Sat, Mar 16, 2024 at 10:02:31AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Do we need to define "hostname" in more detail here? Specifically, I'm
> > wondering whether the result will be a FQDN or not (i.e., the output of
> > "hostname" vs "hostname -f"). Looking at the code I think it will just
> > be the short name returned. That's probably OK, but it may be worth
> > documenting.
> 
> That was my first reaction but there are places where "hostname"
> already gives a name that is not "short" at all, without being
> invoked with "-f".

Thanks, that was the vague buzzing in the back of my head that led to
my first comment. It has been a while since I've dealt with this, but I
think in some circles it is a holy war akin to tabs vs spaces. A quick
search shows I am not alone:

  https://serverfault.com/questions/331936/setting-the-hostname-fqdn-or-short-name

So I think we probably need to say something like:

  Depending on how your system is configured, the hostname used for
  matching may be short (e.g., "myhost") or a fully qualified domain
  name ("myhost.example.com").

> I think the most honest answer we can give in the documentation is
> that we use what gethostname() [*] gives.

That is honest, but I wonder if it is very useful to most users, as they
cannot easily see what it returns. It's tempting to give an extra note
like this tacked on to what I said above:

  You can run the hostname(1) tool to see which hostname your system
  uses.

But I'm not sure that it is available everywhere (especially Windows).
I guess we could provide "git config --show-hostname-for-includes" or
something, but that feels like overkill.

Maybe just the "Depending..." note is enough, and people who are
interested in hostname conditionals hopefully know enough to dig further
on their system. What I think we want to avoid is saying nothing, and
then somebody tries "foo.example.com", finds that it doesn't work, and
gets confused with no hints about why.

I guess yet another alternative is to try to qualify the name ourselves
using getaddrinfo(), either unconditionally or if the hostname doesn't
contain a ".". That may involve a DNS lookup, though (if your hostname
isn't in /etc/hosts).

-Peff

[PATCH v3 0/2] Add hostname condition to includeIf

[Ignacio Encinas]

Extend includeIf to take hostname into account. Motivating request can
be found here [1].

[1] https://github.com/gitgitgadget/git/issues/1665

A lot of feedback was given to the v2 of this patch.

It was pointed out that it wasn't particularly obvious what it was meant by 

  "If the current hostname matches the pattern, the include condition is met."

which is definitely true. Despite this, to my knowledge, there isn't a
way to precisely define what we mean by "hostname" other than saying 
that we mean whatever is returned by gethostname(2). 

In my opinion, terms like "short hostname" can be confusing in some 
cases, and I'm not even sure we can rely on $hostname to agree with 
gethostname(2) in every platform.

I still think the documentation isn't great, but I don't see a way to
improve it further.

Thanks everyone for the feedback!

Changes since v2:
* Expose the result of xgethostname through test-tool
* Rewrite test to rely on test-tool xgethostname rather than using
  $hostname
* Clarify documentation, specifying that by "hostname" we mean output of
  gethostname(2)

Changes since v1:
* Add blank line between declarations and code in `include_by_branch`.
* Rewrite "echo"s used in tests to make them more readable. 

Ignacio Encinas (2):
  t: add a test helper for getting hostname
  config: learn the "hostname:" includeIf condition

 Documentation/config.txt     | 10 +++++++++
 Makefile                     |  1 +
 config.c                     | 17 +++++++++++++++
 t/helper/test-tool.c         |  1 +
 t/helper/test-tool.h         |  1 +
 t/helper/test-xgethostname.c | 12 +++++++++++
 t/t1305-config-include.sh    | 42 ++++++++++++++++++++++++++++++++++++
 t/t6500-gc.sh                |  3 +--
 8 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-xgethostname.c

Range-diff against v2:
-:  ------------ > 1:  ee1f9b1da037 t: add a test helper for getting hostname
1:  cf175154109e ! 2:  dec622c38916 config: learn the "hostname:" includeIf condition
    @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards
     +`hostname`::
     +	The data that follows the keyword `hostname:` is taken to be a
     +	pattern with standard globbing wildcards. If the current
    -+	hostname matches the pattern, the include condition is met.
    ++	hostname (output of gethostname(2)) matches the
    ++	pattern, the include condition is met.
     +
      A few more notes on matching via `gitdir` and `gitdir/i`:
      
    @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '
      
     +test_expect_success 'conditional include, hostname' '
     +	cat >>.git/config <<-EOF &&
    -+	[includeIf "hostname:$(hostname)a"]
    ++	[includeIf "hostname:$(test-tool xgethostname)a"]
     +		path = bar12
     +	EOF
     +	cat >>.git/bar12 <<-EOF &&
    @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '
     +	test_must_fail git config test.twelve &&
     +
     +	cat >>.git/config <<-EOF &&
    -+	[includeIf "hostname:$(hostname)"]
    ++	[includeIf "hostname:$(test-tool xgethostname)"]
     +		path = bar12
     +	EOF
     +	echo 12 >expect &&
    @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '
     +
     +test_expect_success 'conditional include, hostname, wildcard' '
     +	cat >>.git/config <<-EOF &&
    -+	[includeIf "hostname:$(hostname)a*"]
    ++	[includeIf "hostname:$(test-tool xgethostname)a*"]
     +		path = bar13
     +	EOF
     +	cat >>.git/bar13 <<-EOF &&
    @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '
     +	test_must_fail git config test.thirteen &&
     +
     +	cat >>.git/config <<-EOF &&
    -+	[includeIf "hostname:$(hostname)*"]
    ++	[includeIf "hostname:$(test-tool xgethostname)*"]
     +		path = bar13
     +	EOF
     +	echo 13 >expect &&

base-commit: e09f1254c54329773904fe25d7c545a1fb4fa920
-- 
2.44.0

[PATCH v3 1/2] t: add a test helper for getting hostname

[Ignacio Encinas]

Avoid relying on whether the system running the test has "hostname"
installed or not, and expose the output of xgethostname through
test-tool.

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
 Makefile                     |  1 +
 t/helper/test-tool.c         |  1 +
 t/helper/test-tool.h         |  1 +
 t/helper/test-xgethostname.c | 12 ++++++++++++
 t/t6500-gc.sh                |  3 +--
 5 files changed, 16 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-xgethostname.c

diff --git a/Makefile b/Makefile
index 4e255c81f223..561d7a1fa268 100644
--- a/Makefile
+++ b/Makefile
@@ -863,6 +863,7 @@ TEST_BUILTINS_OBJS += test-userdiff.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
 TEST_BUILTINS_OBJS += test-windows-named-pipe.o
 TEST_BUILTINS_OBJS += test-write-cache.o
+TEST_BUILTINS_OBJS += test-xgethostname.o
 TEST_BUILTINS_OBJS += test-xml-encode.o
 
 # Do not add more tests here unless they have extra dependencies. Add
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 482a1e58a4b6..9318900a2981 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -86,6 +86,7 @@ static struct test_cmd cmds[] = {
 	{ "truncate", cmd__truncate },
 	{ "userdiff", cmd__userdiff },
 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
+	{ "xgethostname", cmd__xgethostname },
 	{ "xml-encode", cmd__xml_encode },
 	{ "wildmatch", cmd__wildmatch },
 #ifdef GIT_WINDOWS_NATIVE
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b1be7cfcf593..075d34bd3c0a 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -79,6 +79,7 @@ int cmd__trace2(int argc, const char **argv);
 int cmd__truncate(int argc, const char **argv);
 int cmd__userdiff(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);
+int cmd__xgethostname(int argc, const char **argv);
 int cmd__xml_encode(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
 #ifdef GIT_WINDOWS_NATIVE
diff --git a/t/helper/test-xgethostname.c b/t/helper/test-xgethostname.c
new file mode 100644
index 000000000000..285746aef54a
--- /dev/null
+++ b/t/helper/test-xgethostname.c
@@ -0,0 +1,12 @@
+#include "test-tool.h"
+
+int cmd__xgethostname(int argc, const char **argv)
+{
+	char hostname[HOST_NAME_MAX + 1];
+
+	if (xgethostname(hostname, sizeof(hostname)))
+		die("unable to get the host name");
+
+	puts(hostname);
+	return 0;
+}
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 18fe1c25e6a0..613c766e2bb4 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '
 
 	# now fake a concurrent gc that holds the lock; we can use our
 	# shell pid so that it looks valid.
-	hostname=$(hostname || echo unknown) &&
 	shell_pid=$$ &&
 	if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
 	then
@@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '
 		# the Windows PID in this case.
 		shell_pid=$(cat /proc/$shell_pid/winpid)
 	fi &&
-	printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid &&
+	printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid &&
 
 	# our gc should exit zero without doing anything
 	run_and_wait_for_auto_gc &&
-- 
2.44.0

[PATCH v3 2/2] config: learn the "hostname:" includeIf condition

[Ignacio Encinas]

Currently, customizing the configuration depending on the machine running
git has to be done manually.

Add support for a new includeIf keyword "hostname:" to conditionally
include configuration files depending on the hostname.

Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
---
 Documentation/config.txt  | 10 ++++++++++
 config.c                  | 17 ++++++++++++++++
 t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e3a74dd1c19d..268a9fab7be0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -186,6 +186,12 @@ As for the naming of this keyword, it is for forwards compatibility with
 a naming scheme that supports more variable-based include conditions,
 but currently Git only supports the exact keyword described above.
 
+`hostname`::
+	The data that follows the keyword `hostname:` is taken to be a
+	pattern with standard globbing wildcards. If the current
+	hostname (output of gethostname(2)) matches the
+	pattern, 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.
@@ -261,6 +267,10 @@ Example
 	path = foo.inc
 [remote "origin"]
 	url = https://example.com/git
+
+; include only if the hostname of the machine matches some-hostname
+[includeIf "hostname:some-hostname"]
+	path = foo.inc
 ----
 
 Values
diff --git a/config.c b/config.c
index 3cfeb3d8bd99..50b3f6d24c50 100644
--- a/config.c
+++ b/config.c
@@ -317,6 +317,21 @@ static int include_by_branch(const char *cond, size_t cond_len)
 	return ret;
 }
 
+static int include_by_hostname(const char *cond, size_t cond_len)
+{
+	int ret;
+	char my_host[HOST_NAME_MAX + 1];
+	struct strbuf pattern = STRBUF_INIT;
+
+	if (xgethostname(my_host, sizeof(my_host)))
+		return 0;
+
+	strbuf_add(&pattern, cond, cond_len);
+	ret = !wildmatch(pattern.buf, my_host, 0);
+	strbuf_release(&pattern);
+	return ret;
+}
+
 static int add_remote_url(const char *var, const char *value,
 			  const struct config_context *ctx UNUSED, void *data)
 {
@@ -406,6 +421,8 @@ static int include_condition_is_true(const struct key_value_info *kvi,
 	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, "hostname:", &cond, &cond_len))
+		return include_by_hostname(cond, cond_len);
 
 	/* unknown conditionals are always false */
 	return 0;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 5cde79ef8c4f..ef9272fd8e53 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -357,4 +357,46 @@ test_expect_success 'include cycles are detected' '
 	grep "exceeded maximum include depth" stderr
 '
 
+test_expect_success 'conditional include, hostname' '
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(test-tool xgethostname)a"]
+		path = bar12
+	EOF
+	cat >>.git/bar12 <<-EOF &&
+	[test]
+		twelve=12
+	EOF
+
+	test_must_fail git config test.twelve &&
+
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(test-tool xgethostname)"]
+		path = bar12
+	EOF
+	echo 12 >expect &&
+	git config test.twelve >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'conditional include, hostname, wildcard' '
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(test-tool xgethostname)a*"]
+		path = bar13
+	EOF
+	cat >>.git/bar13 <<-EOF &&
+	[test]
+		thirteen = 13
+	EOF
+
+	test_must_fail git config test.thirteen &&
+
+	cat >>.git/config <<-EOF &&
+	[includeIf "hostname:$(test-tool xgethostname)*"]
+		path = bar13
+	EOF
+	echo 13 >expect &&
+	git config test.thirteen >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Junio C Hamano]

Ignacio Encinas <ignacio@iencinas.com> writes:

> diff --git a/Makefile b/Makefile
> index 4e255c81f223..561d7a1fa268 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -863,6 +863,7 @@ TEST_BUILTINS_OBJS += test-userdiff.o
>  TEST_BUILTINS_OBJS += test-wildmatch.o
>  TEST_BUILTINS_OBJS += test-windows-named-pipe.o
>  TEST_BUILTINS_OBJS += test-write-cache.o
> +TEST_BUILTINS_OBJS += test-xgethostname.o
>  TEST_BUILTINS_OBJS += test-xml-encode.o
>  
>  # Do not add more tests here unless they have extra dependencies. Add
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 482a1e58a4b6..9318900a2981 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -86,6 +86,7 @@ static struct test_cmd cmds[] = {
>  	{ "truncate", cmd__truncate },
>  	{ "userdiff", cmd__userdiff },
>  	{ "urlmatch-normalization", cmd__urlmatch_normalization },
> +	{ "xgethostname", cmd__xgethostname },
>  	{ "xml-encode", cmd__xml_encode },
>  	{ "wildmatch", cmd__wildmatch },
>  #ifdef GIT_WINDOWS_NATIVE
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index b1be7cfcf593..075d34bd3c0a 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -79,6 +79,7 @@ int cmd__trace2(int argc, const char **argv);
>  int cmd__truncate(int argc, const char **argv);
>  int cmd__userdiff(int argc, const char **argv);
>  int cmd__urlmatch_normalization(int argc, const char **argv);
> +int cmd__xgethostname(int argc, const char **argv);
>  int cmd__xml_encode(int argc, const char **argv);
>  int cmd__wildmatch(int argc, const char **argv);
>  #ifdef GIT_WINDOWS_NATIVE
> diff --git a/t/helper/test-xgethostname.c b/t/helper/test-xgethostname.c
> new file mode 100644
> index 000000000000..285746aef54a
> --- /dev/null
> +++ b/t/helper/test-xgethostname.c
> @@ -0,0 +1,12 @@
> +#include "test-tool.h"
> +
> +int cmd__xgethostname(int argc, const char **argv)
> +{
> +	char hostname[HOST_NAME_MAX + 1];
> +
> +	if (xgethostname(hostname, sizeof(hostname)))
> +		die("unable to get the host name");
> +
> +	puts(hostname);
> +	return 0;
> +}


OK.  Given that xgethostname() is meant as a safe (and improved)
replacement for the underlying gethostname(), I would have used
gethostname() as the name for the above, but that alone is not big
enough reason for another update.

> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index 18fe1c25e6a0..613c766e2bb4 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '
>  
>  	# now fake a concurrent gc that holds the lock; we can use our
>  	# shell pid so that it looks valid.
> -	hostname=$(hostname || echo unknown) &&
>  	shell_pid=$$ &&
>  	if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
>  	then
> @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '
>  		# the Windows PID in this case.
>  		shell_pid=$(cat /proc/$shell_pid/winpid)
>  	fi &&
> -	printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid &&
> +	printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid &&

We should replace the "hostname || echo unknown" in the original,
instead of doing this change, as it loses the exit status from the
"test-tool xgethostname" command.

Thanks.

>  	# our gc should exit zero without doing anything
>  	run_and_wait_for_auto_gc &&

Re: [PATCH v3 2/2] config: learn the "hostname:" includeIf condition

[Junio C Hamano]

Ignacio Encinas <ignacio@iencinas.com> writes:

> Currently, customizing the configuration depending on the machine running
> git has to be done manually.

Drop "currently" (cf. https://lore.kernel.org/git/xmqqle6xbep5.fsf@gitster.g/)

It does not actually have to be done manually.  I and many others
have ~/src/home/dot/ directory where ~/src/home/dot/Makefile uses
information available in the environment (like output from the
`hostname` command), produces the .gitconfig file out of a template,
and the build procedure can even install with "make install" the
resulting file to "~/.gitconfig".  Together with other configuration
files that are kept track of in the ~/src/home/ repository, it is
managed wihtout much manual labor.

Another reason why "[includeif hostname:<name>]" may be useful is
when the same home directory is shared across multiple machines.  
As ~/.gitconfig is shared, if you need to have different settings
depending on the host, you would need to have something that a
single file ~/.gitconfig is read in different ways on these hosts.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e3a74dd1c19d..268a9fab7be0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -186,6 +186,12 @@ As for the naming of this keyword, it is for forwards compatibility with
>  a naming scheme that supports more variable-based include conditions,
>  but currently Git only supports the exact keyword described above.
>  
> +`hostname`::
> +	The data that follows the keyword `hostname:` is taken to be a
> +	pattern with standard globbing wildcards. If the current
> +	hostname (output of gethostname(2)) matches the
> +	pattern, the include condition is met.
> +

I do not think of a better way to phrase to explain what `hostname`
means in this context than the above, either.  This should be good
enough, hopefully ;-).

The entry above this one is really an oddball (it only depends on
what other repositories the current repository interacts with, and
does not care about host, directory, or anything of the sort); we
may want to move it either before the `onbranch` entry.

> diff --git a/config.c b/config.c
> index 3cfeb3d8bd99..50b3f6d24c50 100644
> --- a/config.c
> +++ b/config.c
> @@ -317,6 +317,21 @@ static int include_by_branch(const char *cond, size_t cond_len)
>  	return ret;
>  }
>  
> +static int include_by_hostname(const char *cond, size_t cond_len)
> +{
> +	int ret;
> +	char my_host[HOST_NAME_MAX + 1];
> +	struct strbuf pattern = STRBUF_INIT;
> +
> +	if (xgethostname(my_host, sizeof(my_host)))
> +		return 0;
> +
> +	strbuf_add(&pattern, cond, cond_len);
> +	ret = !wildmatch(pattern.buf, my_host, 0);
> +	strbuf_release(&pattern);
> +	return ret;
> +}

OK.  Just as other conditions, it is a bit annoying that we need to
make a copy of cond string only to NUL terminate it, because
wildmatch() does not take a counted string as its input, but the
above code looks good.

> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index 5cde79ef8c4f..ef9272fd8e53 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -357,4 +357,46 @@ test_expect_success 'include cycles are detected' '
>  	grep "exceeded maximum include depth" stderr
>  '
>  
> +test_expect_success 'conditional include, hostname' '
> +	cat >>.git/config <<-EOF &&
> +	[includeIf "hostname:$(test-tool xgethostname)a"]
> +		path = bar12
> +	EOF

Exactly the same comment about lost exit status from test-tool
applies here, too.

> +	cat >>.git/bar12 <<-EOF &&
> +	[test]
> +		twelve=12
> +	EOF
> +
> +	test_must_fail git config test.twelve &&
> +
> +	cat >>.git/config <<-EOF &&
> +	[includeIf "hostname:$(test-tool xgethostname)"]
> +		path = bar12
> +	EOF
> +	echo 12 >expect &&
> +	git config test.twelve >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'conditional include, hostname, wildcard' '
> +	cat >>.git/config <<-EOF &&
> +	[includeIf "hostname:$(test-tool xgethostname)a*"]

Hmph, a* is not even "one-or-more a" but "a followed by anything",
so this will not match, OK.

> +		path = bar13
> +	EOF
> +	cat >>.git/bar13 <<-EOF &&
> +	[test]
> +		thirteen = 13
> +	EOF
> +
> +	test_must_fail git config test.thirteen &&
> +
> +	cat >>.git/config <<-EOF &&
> +	[includeIf "hostname:$(test-tool xgethostname)*"]

And this is "exactly what gethostname gives, followed by anything
(including nothing)", so gethostname output should match.  OK.

> +		path = bar13
> +	EOF
> +	echo 13 >expect &&
> +	git config test.thirteen >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Eric Sunshine]

On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas <ignacio@iencinas.com> wrote:
> It was pointed out that it wasn't particularly obvious what it was meant by
>
>   "If the current hostname matches the pattern, the include condition is met."
>
> which is definitely true. Despite this, to my knowledge, there isn't a
> way to precisely define what we mean by "hostname" other than saying
> that we mean whatever is returned by gethostname(2).
>
> I still think the documentation isn't great, but I don't see a way to
> improve it further.

Peff provided the answer when he suggested[1] implementing `git config
--show-hostname-for-includes`.

[1]: https://lore.kernel.org/git/20240318081722.GA602575@coredump.intra.peff.net/

> 1:  cf175154109e ! 2:  dec622c38916 config: learn the "hostname:" includeIf condition
>     @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards
>      +`hostname`::
>      +  The data that follows the keyword `hostname:` is taken to be a
>      +  pattern with standard globbing wildcards. If the current
>     -+  hostname matches the pattern, the include condition is met.
>     ++  hostname (output of gethostname(2)) matches the
>     ++  pattern, the include condition is met.

This is still unnecessarily user-hostile, especially to users who are
not programmers, but also to programmers who don't want to waste time
writing a little test program to determine what gethostname(2) returns
on each platform they use. That's not a great situation.

Peff felt that adding `git config --show-hostname-for-includes` was
probably overkill, but I'd argue that it is necessary to enable users
to deterministically figure out the value to use in their
configuration rather than having to grope around in the dark via
guesswork and trial-and-error to figure out exactly what works.

And the option name doesn't necessarily have to be so verbose; a
shorter name, such as `git config --show-hostname` may be good enough.
Implementing this option would also obviate the need to implement
`test-tool xgethostname` (though, I agree with Junio that `test-tool
gethostname` would have been a better, less implementation-revealing
name).

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Jeff King]

On Tue, Mar 19, 2024 at 07:37:21PM +0100, Ignacio Encinas wrote:

> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index 18fe1c25e6a0..613c766e2bb4 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '
>  
>  	# now fake a concurrent gc that holds the lock; we can use our
>  	# shell pid so that it looks valid.
> -	hostname=$(hostname || echo unknown) &&
>  	shell_pid=$$ &&
>  	if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
>  	then
> @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '
>  		# the Windows PID in this case.
>  		shell_pid=$(cat /proc/$shell_pid/winpid)
>  	fi &&
> -	printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid &&
> +	printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid &&

Hmm. Before, we were compensating for a failure to run "hostname" by
putting in the string "unknown". But I wonder if there were platforms
where gethostname() simply fails (e.g., because the hostname is too
long). In that case your test-tool returns the empty string, but git-gc
internally will use the string "unknown".

I think it may be OK, though. The failing exit code from test-tool will
be ignored, and we'll end up with a file containing "123 " or similar.
Normally we'd use kill() to check that the pid is valid, but with a
mis-matched hostname we'll just assume the process is still around, and
the outcome is the same.

-Peff

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Jeff King]

On Tue, Mar 19, 2024 at 01:31:06PM -0700, Junio C Hamano wrote:

> > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> > index 18fe1c25e6a0..613c766e2bb4 100755
> > --- a/t/t6500-gc.sh
> > +++ b/t/t6500-gc.sh
> > @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '
> >  
> >  	# now fake a concurrent gc that holds the lock; we can use our
> >  	# shell pid so that it looks valid.
> > -	hostname=$(hostname || echo unknown) &&
> >  	shell_pid=$$ &&
> >  	if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
> >  	then
> > @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '
> >  		# the Windows PID in this case.
> >  		shell_pid=$(cat /proc/$shell_pid/winpid)
> >  	fi &&
> > -	printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid &&
> > +	printf "%d %s" "$shell_pid" "$(test-tool xgethostname)" >.git/gc.pid &&
> 
> We should replace the "hostname || echo unknown" in the original,
> instead of doing this change, as it loses the exit status from the
> "test-tool xgethostname" command.

I think you need to lose the exit status. Or alternatively do:

  hostname=$(test-tool xgethostname || echo unknown)

See my other reply.

-Peff

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think you need to lose the exit status. Or alternatively do:
>
>   hostname=$(test-tool xgethostname || echo unknown)
>
> See my other reply.

As "test-tool xgethostname" runs exactly the same codepath as
"includeIf hostname:blah" feature, I would actually prefer for a
failing "test-tool gethostname" to _break_ this test so that people
can take notice.

Re: [PATCH v3 2/2] config: learn the "hostname:" includeIf condition

[Jeff King]

On Tue, Mar 19, 2024 at 07:37:22PM +0100, Ignacio Encinas wrote:

> +`hostname`::
> +	The data that follows the keyword `hostname:` is taken to be a
> +	pattern with standard globbing wildcards. If the current
> +	hostname (output of gethostname(2)) matches the
> +	pattern, the include condition is met.

I was going to comment further here, but I see Eric already replied with
everything I was going to say. ;)

One small comment on the patch...

> +static int include_by_hostname(const char *cond, size_t cond_len)
> +{
> +	int ret;
> +	char my_host[HOST_NAME_MAX + 1];
> +	struct strbuf pattern = STRBUF_INIT;
> +
> +	if (xgethostname(my_host, sizeof(my_host)))
> +		return 0;
> +
> +	strbuf_add(&pattern, cond, cond_len);
> +	ret = !wildmatch(pattern.buf, my_host, 0);
> +	strbuf_release(&pattern);
> +	return ret;
> +}

This is absolutely a nit, but I think using xmemdupz() like:

  char *pattern;
  ...

  pattern = xmemdupz(cond, cond_len);
  ...
  free(pattern);

expresses the intent more directly (it's also a little more efficient,
but that's probably not measurable).

-Peff

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Jeff King]

On Tue, Mar 19, 2024 at 02:00:34PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think you need to lose the exit status. Or alternatively do:
> >
> >   hostname=$(test-tool xgethostname || echo unknown)
> >
> > See my other reply.
> 
> As "test-tool xgethostname" runs exactly the same codepath as
> "includeIf hostname:blah" feature, I would actually prefer for a
> failing "test-tool gethostname" to _break_ this test so that people
> can take notice.

But we are not testing "includeIf" in this patch; we are testing git-gc,
which falls back to the string "unknown". The includeIf tests in patch 2
will naturally fail if either xgethostname() fails (because then we will
refuse to do any host matching) or if it works but somehow test-tool is
broken (because then it won't match what the config code sees
internally).

-Peff

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Peff felt that adding `git config --show-hostname-for-includes` was
> probably overkill, but I'd argue that it is necessary to enable users
> to deterministically figure out the value to use in their
> configuration rather than having to grope around in the dark via
> guesswork and trial-and-error to figure out exactly what works.
>
> And the option name doesn't necessarily have to be so verbose; a
> shorter name, such as `git config --show-hostname` may be good enough.
> Implementing this option would also obviate the need to implement
> `test-tool xgethostname` (though, I agree with Junio that `test-tool
> gethostname` would have been a better, less implementation-revealing
> name).

Yeah, I like that show-hostname thing (which I do not know if "config"
is a good home for, though).

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Eric Sunshine]

On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Peff felt that adding `git config --show-hostname-for-includes` was
> > probably overkill, but I'd argue that it is necessary to enable users
> > to deterministically figure out the value to use in their
> > configuration rather than having to grope around in the dark via
> > guesswork and trial-and-error to figure out exactly what works.
> >
> > And the option name doesn't necessarily have to be so verbose; a
> > shorter name, such as `git config --show-hostname` may be good enough.
> > Implementing this option would also obviate the need to implement
> > `test-tool xgethostname` (though, I agree with Junio that `test-tool
> > gethostname` would have been a better, less implementation-revealing
> > name).
>
> Yeah, I like that show-hostname thing (which I do not know if "config"
> is a good home for, though).

The other possibility which came to mind was adding a GIT_HOSTNAME
variable to the output of `git var -l`.

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Ignacio Encinas Rubio]

On 19/3/24 21:55, Eric Sunshine wrote:
> On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas <ignacio@iencinas.com> wrote:
>> It was pointed out that it wasn't particularly obvious what it was meant by
>>
>>   "If the current hostname matches the pattern, the include condition is met."
>>
>> which is definitely true. Despite this, to my knowledge, there isn't a
>> way to precisely define what we mean by "hostname" other than saying
>> that we mean whatever is returned by gethostname(2).
>>
>> I still think the documentation isn't great, but I don't see a way to
>> improve it further.
> 
> Peff provided the answer when he suggested[1] implementing `git config
> --show-hostname-for-includes`.
> 
> [1]: https://lore.kernel.org/git/20240318081722.GA602575@coredump.intra.peff.net/
 
Sorry if it sounded like I disregarded the opinion. I did see it and
liked the idea, but I guessed something like that would face a lot of
resistance. My bad.

>> 1:  cf175154109e ! 2:  dec622c38916 config: learn the "hostname:" includeIf condition
>>     @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards
>>      +`hostname`::
>>      +  The data that follows the keyword `hostname:` is taken to be a
>>      +  pattern with standard globbing wildcards. If the current
>>     -+  hostname matches the pattern, the include condition is met.
>>     ++  hostname (output of gethostname(2)) matches the
>>     ++  pattern, the include condition is met.
> 
> This is still unnecessarily user-hostile, especially to users who are
> not programmers, but also to programmers who don't want to waste time
> writing a little test program to determine what gethostname(2) returns
> on each platform they use. That's not a great situation.
> 
> Peff felt that adding `git config --show-hostname-for-includes` was
> probably overkill, but I'd argue that it is necessary to enable users
> to deterministically figure out the value to use in their
> configuration rather than having to grope around in the dark via
> guesswork and trial-and-error to figure out exactly what works.
> 
> And the option name doesn't necessarily have to be so verbose; a
> shorter name, such as `git config --show-hostname` may be good enough.
> Implementing this option would also obviate the need to implement
> `test-tool xgethostname` (though, I agree with Junio that `test-tool
> gethostname` would have been a better, less implementation-revealing
> name).

Lets find this a good "home" then [1]. Thanks!

[1] https://lore.kernel.org/git/CAPig+cTFRAmzBGiJv2F-k1XWvGSbT8UeAG57T+XpB-1w66HRkQ@mail.gmail.com/

Re: [PATCH v3 2/2] config: learn the "hostname:" includeIf condition

[Ignacio Encinas Rubio]

On 19/3/24 22:04, Jeff King wrote:
> On Tue, Mar 19, 2024 at 07:37:22PM +0100, Ignacio Encinas wrote:
> 
>> +`hostname`::
>> +	The data that follows the keyword `hostname:` is taken to be a
>> +	pattern with standard globbing wildcards. If the current
>> +	hostname (output of gethostname(2)) matches the
>> +	pattern, the include condition is met.
> 
> I was going to comment further here, but I see Eric already replied with
> everything I was going to say. ;)

Please see my reply there. Thanks for the suggestion and sorry again if 
I sounded rude!

> One small comment on the patch...
> 
>> +static int include_by_hostname(const char *cond, size_t cond_len)
>> +{
>> +	int ret;
>> +	char my_host[HOST_NAME_MAX + 1];
>> +	struct strbuf pattern = STRBUF_INIT;
>> +
>> +	if (xgethostname(my_host, sizeof(my_host)))
>> +		return 0;
>> +
>> +	strbuf_add(&pattern, cond, cond_len);
>> +	ret = !wildmatch(pattern.buf, my_host, 0);
>> +	strbuf_release(&pattern);
>> +	return ret;
>> +}
> 
> This is absolutely a nit, but I think using xmemdupz() like:
> 
>   char *pattern;
>   ...
> 
>   pattern = xmemdupz(cond, cond_len);
>   ...
>   free(pattern);
> 
> expresses the intent more directly (it's also a little more efficient,
> but that's probably not measurable).

Noted, thanks!
 
> -Peff

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Dirk Gouders]

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Peff felt that adding `git config --show-hostname-for-includes` was
>> probably overkill, but I'd argue that it is necessary to enable users
>> to deterministically figure out the value to use in their
>> configuration rather than having to grope around in the dark via
>> guesswork and trial-and-error to figure out exactly what works.
>>
>> And the option name doesn't necessarily have to be so verbose; a
>> shorter name, such as `git config --show-hostname` may be good enough.
>> Implementing this option would also obviate the need to implement
>> `test-tool xgethostname` (though, I agree with Junio that `test-tool
>> gethostname` would have been a better, less implementation-revealing
>> name).
>
> Yeah, I like that show-hostname thing (which I do not know if "config"
> is a good home for, though).

A thought when I was reading this: wouldn't it be enough to document
that `uname -n` can be used to get the hostname that should be used?

As far as I know this should be POSIX-compliant and uses gethostname(2).

Dirk

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But we are not testing "includeIf" in this patch; we are testing git-gc,
> which falls back to the string "unknown".

Ah, I wasn't aware of such a hardcoded default.  Then replacing the
existing "hostname" with "test-tool xgethostname" and doing nothing
else is of course the right thing to do here.

Thanks.

RE: [PATCH v3 0/2] Add hostname condition to includeIf

[rsbecker]

On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> Peff felt that adding `git config --show-hostname-for-includes` was
>>> probably overkill, but I'd argue that it is necessary to enable users
>>> to deterministically figure out the value to use in their
>>> configuration rather than having to grope around in the dark via
>>> guesswork and trial-and-error to figure out exactly what works.
>>>
>>> And the option name doesn't necessarily have to be so verbose; a
>>> shorter name, such as `git config --show-hostname` may be good enough.
>>> Implementing this option would also obviate the need to implement
>>> `test-tool xgethostname` (though, I agree with Junio that `test-tool
>>> gethostname` would have been a better, less implementation-revealing
>>> name).
>>
>> Yeah, I like that show-hostname thing (which I do not know if "config"
>> is a good home for, though).
>
>A thought when I was reading this: wouldn't it be enough to document that
`uname -n` can be used to get the hostname that should
>be used?
>
>As far as I know this should be POSIX-compliant and uses gethostname(2).

As previously pointed out, uname -n and gethostname(2) are not equivalent.
uname -n does not (depending on implementation) go to DNS while
gethostname(2) goes to DNS first (although apparently glibc may not). This
is particularly important in a multi-home situation where more than one IP
adapter has a different IP address on the same host, and where DNS does not
consider the different addresses to be equivalent (which otherwise could
cause problems for reverse lookups).
--Randall

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Dirk Gouders]

<rsbecker@nexbridge.com> writes:

> On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:
>>Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>>> Peff felt that adding `git config --show-hostname-for-includes` was
>>>> probably overkill, but I'd argue that it is necessary to enable users
>>>> to deterministically figure out the value to use in their
>>>> configuration rather than having to grope around in the dark via
>>>> guesswork and trial-and-error to figure out exactly what works.
>>>>
>>>> And the option name doesn't necessarily have to be so verbose; a
>>>> shorter name, such as `git config --show-hostname` may be good enough.
>>>> Implementing this option would also obviate the need to implement
>>>> `test-tool xgethostname` (though, I agree with Junio that `test-tool
>>>> gethostname` would have been a better, less implementation-revealing
>>>> name).
>>>
>>> Yeah, I like that show-hostname thing (which I do not know if "config"
>>> is a good home for, though).
>>
>>A thought when I was reading this: wouldn't it be enough to document that
> `uname -n` can be used to get the hostname that should
>>be used?
>>
>>As far as I know this should be POSIX-compliant and uses gethostname(2).
>
> As previously pointed out, uname -n and gethostname(2) are not equivalent.
> uname -n does not (depending on implementation) go to DNS while
> gethostname(2) goes to DNS first (although apparently glibc may not). This
> is particularly important in a multi-home situation where more than one IP
> adapter has a different IP address on the same host, and where DNS does not
> consider the different addresses to be equivalent (which otherwise could
> cause problems for reverse lookups).

Thanks for the explanation, I did not notice this has already been
discussed.

Interestingly, I strace(1)'ed uname -n here on Linux and noticed it uses
uname(2) (what else?) and not gethostname(2), so it seems I was
completely wrong.

Sorry for disturbing the discussion.

Dirk

RE: [PATCH v3 0/2] Add hostname condition to includeIf

[rsbecker]

On Tuesday, March 19, 2024 6:27 PM, Dirk Gouders wrote:
><rsbecker@nexbridge.com> writes:
>
>> On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:
>>>Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>>
>>>>> Peff felt that adding `git config --show-hostname-for-includes` was
>>>>> probably overkill, but I'd argue that it is necessary to enable
>>>>> users to deterministically figure out the value to use in their
>>>>> configuration rather than having to grope around in the dark via
>>>>> guesswork and trial-and-error to figure out exactly what works.
>>>>>
>>>>> And the option name doesn't necessarily have to be so verbose; a
>>>>> shorter name, such as `git config --show-hostname` may be good enough.
>>>>> Implementing this option would also obviate the need to implement
>>>>> `test-tool xgethostname` (though, I agree with Junio that
>>>>> `test-tool gethostname` would have been a better, less
>>>>> implementation-revealing name).
>>>>
>>>> Yeah, I like that show-hostname thing (which I do not know if "config"
>>>> is a good home for, though).
>>>
>>>A thought when I was reading this: wouldn't it be enough to document
>>>that
>> `uname -n` can be used to get the hostname that should
>>>be used?
>>>
>>>As far as I know this should be POSIX-compliant and uses gethostname(2).
>>
>> As previously pointed out, uname -n and gethostname(2) are not
equivalent.
>> uname -n does not (depending on implementation) go to DNS while
>> gethostname(2) goes to DNS first (although apparently glibc may not).
>> This is particularly important in a multi-home situation where more
>> than one IP adapter has a different IP address on the same host, and
>> where DNS does not consider the different addresses to be equivalent
>> (which otherwise could cause problems for reverse lookups).
>
>Thanks for the explanation, I did not notice this has already been
discussed.
>
>Interestingly, I strace(1)'ed uname -n here on Linux and noticed it uses
>uname(2) (what else?) and not gethostname(2), so it seems I was completely
>wrong.
>
>Sorry for disturbing the discussion.

No worries. I only know this point because I was rather deeply in a related
code base back in 1994. I did not know that glibc varied from an old UNIX (I
think that's where the code was from) code base prior to this thread.
Learning is good and never a problem.

Regards,
Randall

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> No worries. I only know this point because I was rather deeply in a related
> code base back in 1994. I did not know that glibc varied from an old UNIX (I
> think that's where the code was from) code base prior to this thread.
> Learning is good and never a problem.

It is not surprising that you were doing gethostname in 1994, but it
is very surprising that you still remember such details ;-)

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Jeff King]

On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote:

> On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > Peff felt that adding `git config --show-hostname-for-includes` was
> > > probably overkill, but I'd argue that it is necessary to enable users
> > > to deterministically figure out the value to use in their
> > > configuration rather than having to grope around in the dark via
> > > guesswork and trial-and-error to figure out exactly what works.
> > >
> > > And the option name doesn't necessarily have to be so verbose; a
> > > shorter name, such as `git config --show-hostname` may be good enough.
> > > Implementing this option would also obviate the need to implement
> > > `test-tool xgethostname` (though, I agree with Junio that `test-tool
> > > gethostname` would have been a better, less implementation-revealing
> > > name).
> >
> > Yeah, I like that show-hostname thing (which I do not know if "config"
> > is a good home for, though).
> 
> The other possibility which came to mind was adding a GIT_HOSTNAME
> variable to the output of `git var -l`.

That strikes me as a more appropriate spot than an option to git-config.
Even if config is the only thing _now_ which cares about the hostname,
it may be something that other parts of the system care about in the
future.

Some care may need to be taken for error handling, though. For "git var
GIT_HOSTNAME" it is OK to exit non-zero, but "git var -l" should not
bail on a system where gethostname() doesn't work (it is still not clear
to me if that is a real case to worry about or not).

-Peff

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Eric Sunshine]

On Tue, Mar 19, 2024 at 8:19 PM Jeff King <peff@peff.net> wrote:
> On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote:
> > The other possibility which came to mind was adding a GIT_HOSTNAME
> > variable to the output of `git var -l`.
>
> That strikes me as a more appropriate spot than an option to git-config.
> Even if config is the only thing _now_ which cares about the hostname,
> it may be something that other parts of the system care about in the
> future.

Also, taking into consideration Patrick's proposed revamp[1] of
git-config to give it a subcommand API, then git-config becomes an
even less welcome place for a standalone --show-hostname option which,
by itself, doesn't really fit into the subcommand paradigm, and it
probably doesn't make sense to add a new subcommand ("info" or
whatever) just for that.

[1]: https://lore.kernel.org/git/cover.1710198711.git.ps@pks.im/T/#u

> Some care may need to be taken for error handling, though. For "git var
> GIT_HOSTNAME" it is OK to exit non-zero, but "git var -l" should not
> bail on a system where gethostname() doesn't work (it is still not clear
> to me if that is a real case to worry about or not).

Ports to oddball platforms should probably be providing a
gethostname() in "compat/" anyhow, just as is done for Windows in
"compat/mingw.c".

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Eric Sunshine]

On Tue, Mar 19, 2024 at 10:49 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Mar 19, 2024 at 8:19 PM Jeff King <peff@peff.net> wrote:
> > Some care may need to be taken for error handling, though. For "git var
> > GIT_HOSTNAME" it is OK to exit non-zero, but "git var -l" should not
> > bail on a system where gethostname() doesn't work (it is still not clear
> > to me if that is a real case to worry about or not).
>
> Ports to oddball platforms should probably be providing a
> gethostname() in "compat/" anyhow, just as is done for Windows in
> "compat/mingw.c".

Ignore my mumbo jumbo response. You are, of course, correct that the
implementation of `git var -l` needs to be done with care so that it
doesn't bail if gethostname() fails; that's true regardless of whether
or not a platform-specific "compat/" implementation is part of the
mix.

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Chris Torek]

The suggestion for having `git var` list GIT_HOSTNAME gave me
an idea: perhaps instead of, or in addition to, a `hostname`
condition in the `includeif` code, we could:

 * have an `includeif:env:...` condition that tests an env
   variable against a pattern; and/or
 * use $GIT_HOSTNAME as the variable.

We'd then set `GIT_HOSTNAME` to the gethostname() result *unless*
it's already set.

This gives users much more flexibility, because:

 * they can use the hostname and/or arbitrary-env-var condition;
 * they can then *set* GIT_HOSTNAME to the short or full
   hostname at their discretion if the default is not suitable
   for some reason; and of course
 * they can, as noted, use `git var` to find the default setting.

Chris

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Eric Sunshine]

On Wed, Mar 20, 2024 at 10:35 AM Chris Torek <chris.torek@gmail.com> wrote:
> The suggestion for having `git var` list GIT_HOSTNAME gave me
> an idea: perhaps instead of, or in addition to, a `hostname`
> condition in the `includeif` code, we could:
>
>  * have an `includeif:env:...` condition that tests an env
>    variable against a pattern; and/or
>  * use $GIT_HOSTNAME as the variable.
>
> We'd then set `GIT_HOSTNAME` to the gethostname() result *unless*
> it's already set.
>
> This gives users much more flexibility, because:
>
>  * they can use the hostname and/or arbitrary-env-var condition;
>  * they can then *set* GIT_HOSTNAME to the short or full
>    hostname at their discretion if the default is not suitable
>    for some reason; and of course
>  * they can, as noted, use `git var` to find the default setting.

This certainly is a much more generic approach, and simplifies the
implementation considerably since it obviates the need for
GIT_HOSTNAME (or --show-hostname) since the choice of variable name
and value is fully under the user's control.

I have some vague feeling that this idea of using an environment
variable as a condition may have been discussed before and possibly
rejected due to potential security concerns, but I don't use
`includeif` myself and haven't really followed past discussions, so I
could be wrong about that. Peff would probably have better
recollection.

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Junio C Hamano]

Chris Torek <chris.torek@gmail.com> writes:

> The suggestion for having `git var` list GIT_HOSTNAME gave me
> an idea: perhaps instead of, or in addition to, a `hostname`
> condition in the `includeif` code, we could:
>
>  * have an `includeif:env:...` condition that tests an env
>    variable against a pattern; and/or
>  * use $GIT_HOSTNAME as the variable.

Nice.

> We'd then set `GIT_HOSTNAME` to the gethostname() result *unless*
> it's already set.
>
> This gives users much more flexibility, because:
>
>  * they can use the hostname and/or arbitrary-env-var condition;
>  * they can then *set* GIT_HOSTNAME to the short or full
>    hostname at their discretion if the default is not suitable
>    for some reason; and of course
>  * they can, as noted, use `git var` to find the default setting.
>
> Chris

Re: [PATCH v3 0/2] Add hostname condition to includeIf

[Jeff King]

On Wed, Mar 20, 2024 at 12:37:22PM -0400, Eric Sunshine wrote:

> I have some vague feeling that this idea of using an environment
> variable as a condition may have been discussed before and possibly
> rejected due to potential security concerns, but I don't use
> `includeif` myself and haven't really followed past discussions, so I
> could be wrong about that. Peff would probably have better
> recollection.

I can't think of any security concerns; if you can control the
environment you can already set GIT_CONFIG_PARAMETERS to do whatever you
like.

In fact, I think I've suggested includeIf.env before. ;)

Ævar even wrote a patch, but I think we got bogged down in issues of
syntax:

  https://lore.kernel.org/git/patch-1.1-1fe6f60d2bf-20210924T005553Z-avarab@gmail.com/

-Peff

Re: [PATCH v3 1/2] t: add a test helper for getting hostname

[Chris Torek]

On Tue, Mar 19, 2024 at 2:44 PM Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
> > But we are not testing "includeIf" in this patch; we are testing git-gc,
> > which falls back to the string "unknown".
>
> Ah, I wasn't aware of such a hardcoded default.  Then replacing the
> existing "hostname" with "test-tool xgethostname" and doing nothing
> else is of course the right thing to do here.

Note that if we choose to use $GIT_HOSTNAME (auto-set-if-unset),
we can change the test to simply force $GIT_HOSTNAME to something
known to the test script.

(That seems orthogonal to this change, but I thought I would mention it.)

Chris