threads / rfc / 61072

RFC patchAdd hostname condition to includeIf

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

## tl;dr

51 messages between Mar 7, 2024 and Mar 21, 2024. Diffs are folded; open one to read it.

replies: 50people: 8as markdown or json

Ignacio Encinas· Mar 7, 2024, 20:50 UTC · lore
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
Ignacio Encinas· Mar 7, 2024, 20:50 UTC · re: Ignacio Encinas · lore

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

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(+)
Show changes to 3 files +47 −0

Documentation/config.txt, config.c, t/t1305-config-include.sh

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
Junio C Hamano· Mar 7, 2024, 21:40 UTC · re: Ignacio Encinas · lore

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

Ignacio Encinas <ignacio@iencinas.com> writes:
Show 20 quoted lines
> 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.
Show 5 quoted lines
> +`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.

Show 21 quoted lines
> 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.
Show 12 quoted lines
> 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.
Ignacio Encinas Rubio· Mar 9, 2024, 10:47 UTC · re: Junio C Hamano · lore

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

On 7/3/24 22:40, Junio C Hamano wrote:
Show 60 quoted lines
> 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. 
 
Show 20 quoted lines
>> 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?

Show 5 quoted lines
>> +	echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
> 
> Ditto for the remainder of the patch.
> 
> Thanks.
Thank you for the review.
Junio C Hamano· Mar 9, 2024, 17:38 UTC · re: Ignacio Encinas Rubio · lore

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

Ignacio Encinas Rubio <ignacio@iencinas.com> writes:
Show 16 quoted lines
>>> +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.

Ignacio Encinas· Mar 9, 2024, 18:18 UTC · re: Ignacio Encinas · lore

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

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
Ignacio Encinas· Mar 9, 2024, 18:18 UTC · re: Ignacio Encinas · lore

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

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(+)
Show changes to 3 files +68 −0

Documentation/config.txt, config.c, t/t1305-config-include.sh

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
Junio C Hamano· Mar 10, 2024, 16:59 UTC · re: Ignacio Encinas · lore

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

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.
Ignacio Encinas Rubio· Mar 10, 2024, 18:46 UTC · re: Junio C Hamano · lore

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

On 10/3/24 17:59, Junio C Hamano wrote:
Show 14 quoted lines
> 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.

Show 11 quoted lines
> 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.

Junio C Hamano· Mar 11, 2024, 17:39 UTC · re: Ignacio Encinas Rubio · lore

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

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.

Ignacio Encinas Rubio· Mar 13, 2024, 21:53 UTC · re: Junio C Hamano · lore

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

On 11/3/24 18:39, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Show 18 quoted lines
>  * 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!
Jeff King· Mar 16, 2024, 06:57 UTC · re: Ignacio Encinas · lore

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

On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
Show 12 quoted lines
> 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
Ignacio Encinas Rubio· Mar 16, 2024, 11:19 UTC · re: Jeff King · lore

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

On 16/3/24 7:57, Jeff King wrote:
Show 20 quoted lines
> 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

Show changes to Documentation/config.txt +2 −1
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.
Taylor Blau· Mar 16, 2024, 16:00 UTC · re: Ignacio Encinas Rubio · lore

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

On Sat, Mar 16, 2024 at 12:19:44PM +0100, Ignacio Encinas Rubio wrote:
Show 39 quoted lines
>
>
> 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

Ignacio Encinas Rubio· Mar 16, 2024, 16:46 UTC · re: Taylor Blau · lore

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

On 16/3/24 17:00, Taylor Blau wrote:
Show 48 quoted lines
> 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.

Show 7 quoted lines
> 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!
Junio C Hamano· Mar 16, 2024, 17:02 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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
rsbecker@nexbridge.com· Mar 16, 2024, 17:41 UTC · re: Junio C Hamano · lore

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

On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:
Show 10 quoted lines
>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.

Show 5 quoted lines
>
>
>[References]
>
>*
https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html
--Randall
Ignacio Encinas Rubio· Mar 16, 2024, 18:05 UTC · re: rsbecker@nexbridge.com · lore

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

On 16/3/24 18:41, rsbecker@nexbridge.com wrote:
Show 39 quoted lines
> 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

Show 9 quoted lines
>>
>>
>> [References]
>>
>> *
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html
> 
> --Randall
> 
rsbecker@nexbridge.com· Mar 16, 2024, 18:49 UTC · re: Ignacio Encinas Rubio · lore

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

On Saturday, March 16, 2024 2:06 PM, Ignacio Encinas Rubio wrote:
Show 48 quoted lines
>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.
Jeff King· Mar 18, 2024, 08:17 UTC · re: Junio C Hamano · lore

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

On Sat, Mar 16, 2024 at 10:02:31AM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 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
Taylor Blau· Mar 16, 2024, 16:01 UTC · re: Ignacio Encinas · lore

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

Hi Ignacio,
On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:
Show 5 quoted lines
> ---
>  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

Ignacio Encinas Rubio· Mar 16, 2024, 16:50 UTC · re: Taylor Blau · lore

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

On 16/3/24 17:01, Taylor Blau wrote:
Show 14 quoted lines
> 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
Ignacio Encinas· Mar 19, 2024, 18:37 UTC · re: Ignacio Encinas · lore

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

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
Ignacio Encinas· Mar 19, 2024, 18:37 UTC · re: Ignacio Encinas · lore

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

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
Show changes to 5 files +16 −2

Makefile, t/helper/test-tool.c, t/helper/test-tool.h, t/helper/test-xgethostname.c, t/t6500-gc.sh

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
Junio C Hamano· Mar 19, 2024, 20:31 UTC · re: Ignacio Encinas · lore

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

Ignacio Encinas <ignacio@iencinas.com> writes:
Show 54 quoted lines
> 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.

Show 18 quoted lines
> 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 &&
Jeff King· Mar 19, 2024, 20:57 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 19, 2024 at 01:31:06PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> > 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
Junio C Hamano· Mar 19, 2024, 21:00 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.

Jeff King· Mar 19, 2024, 21:11 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 19, 2024 at 02:00:34PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Mar 19, 2024, 21:44 UTC · re: Jeff King · lore

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

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.
Chris Torek· Mar 21, 2024, 00:11 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 19, 2024 at 2:44 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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
Jeff King· Mar 19, 2024, 20:56 UTC · re: Ignacio Encinas · lore

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

On Tue, Mar 19, 2024 at 07:37:21PM +0100, Ignacio Encinas wrote:
Show 18 quoted lines
> 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
Ignacio Encinas· Mar 19, 2024, 18:37 UTC · re: Ignacio Encinas · lore

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

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(+)
Show changes to 3 files +69 −0

Documentation/config.txt, config.c, t/t1305-config-include.sh

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
Junio C Hamano· Mar 19, 2024, 20:53 UTC · re: Ignacio Encinas · lore

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

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.

Show 14 quoted lines
> 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.

Show 22 quoted lines
> 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.

Show 13 quoted lines
> 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.

Show 19 quoted lines
> +	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.

Show 11 quoted lines
> +		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.

Show 8 quoted lines
> +		path = bar13
> +	EOF
> +	echo 13 >expect &&
> +	git config test.thirteen >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done
Jeff King· Mar 19, 2024, 21:04 UTC · re: Ignacio Encinas · lore

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

On Tue, Mar 19, 2024 at 07:37:22PM +0100, Ignacio Encinas wrote:
Show 5 quoted lines
> +`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...
Show 14 quoted lines
> +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
Ignacio Encinas Rubio· Mar 19, 2024, 21:32 UTC · re: Jeff King · lore

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

On 19/3/24 22:04, Jeff King wrote:
Show 10 quoted lines
> 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!

Show 28 quoted lines
> 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
Eric Sunshine· Mar 19, 2024, 20:55 UTC · re: Ignacio Encinas · lore

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

On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas <ignacio@iencinas.com> wrote:
Show 10 quoted lines
> 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/
Show 8 quoted lines
> 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).

Junio C Hamano· Mar 19, 2024, 21:12 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
> 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).

Eric Sunshine· Mar 19, 2024, 21:13 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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`.

Jeff King· Mar 20, 2024, 00:19 UTC · re: Eric Sunshine · lore

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

On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote:
Show 20 quoted lines
> 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
Eric Sunshine· Mar 20, 2024, 02:49 UTC · re: Jeff King · lore

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

On Tue, Mar 19, 2024 at 8:19 PM Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> 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".

Eric Sunshine· Mar 20, 2024, 03:07 UTC · re: Eric Sunshine · lore

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

On Tue, Mar 19, 2024 at 10:49 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 9 quoted lines
> 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.

Chris Torek· Mar 20, 2024, 14:34 UTC · re: Eric Sunshine · lore

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

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
Eric Sunshine· Mar 20, 2024, 16:37 UTC · re: Chris Torek · lore

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

On Wed, Mar 20, 2024 at 10:35 AM Chris Torek <chris.torek@gmail.com> wrote:
Show 18 quoted lines
> 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.

Jeff King· Mar 20, 2024, 20:51 UTC · re: Eric Sunshine · lore

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

On Wed, Mar 20, 2024 at 12:37:22PM -0400, Eric Sunshine wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Mar 20, 2024, 16:49 UTC · re: Chris Torek · lore

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

Chris Torek <chris.torek@gmail.com> writes:
Show 7 quoted lines
> 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.
Show 12 quoted lines
> 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
Dirk Gouders· Mar 19, 2024, 21:36 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> 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
rsbecker@nexbridge.com· Mar 19, 2024, 22:03 UTC · re: Dirk Gouders · lore

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

On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:
Show 21 quoted lines
>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

Dirk Gouders· Mar 19, 2024, 22:26 UTC · re: rsbecker@nexbridge.com · lore

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

<rsbecker@nexbridge.com> writes:
Show 34 quoted lines
> 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
rsbecker@nexbridge.com· Mar 19, 2024, 22:31 UTC · re: Dirk Gouders · lore

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

On Tuesday, March 19, 2024 6:27 PM, Dirk Gouders wrote:
Show 31 quoted lines
><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.
Show 8 quoted lines
>> 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.
Show 6 quoted lines
>
>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

Junio C Hamano· Mar 19, 2024, 22:59 UTC · re: rsbecker@nexbridge.com · lore

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

<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 ;-)

Ignacio Encinas Rubio· Mar 19, 2024, 21:22 UTC · re: Eric Sunshine · lore

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

On 19/3/24 21:55, Eric Sunshine wrote:
Show 16 quoted lines
> 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.
Show 26 quoted lines
>> 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/

← back to recent threads