{"thread":{"id":"61072","subject":"[RFC PATCH 0/1] Add hostname condition to includeIf","startedAt":"2024-03-07T20:50:31Z","lastAt":"2024-03-21T00:11:39Z","messageCount":51,"participants":["Ignacio Encinas","Junio C Hamano","Ignacio Encinas Rubio","Jeff King","Taylor Blau","rsbecker@nexbridge.com","Eric Sunshine","Dirk Gouders","Chris Torek"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"490217","messageId":"20240307205006.467443-1-ignacio@iencinas.com","threadId":"61072","inReplyTo":null,"subject":"[RFC PATCH 0/1] Add hostname condition to includeIf","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-07T20:50:05Z","receivedAt":"2024-03-07T20:50:31Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Hello,\n\nFirst of all hello everyone, and thanks for developing git :)\n\nI recently came across [1], which proposes further extending \nincludeIf by supporting \"hostname\" as a condition.\n\nI thought it would be a good feature to have in git so I gave \nit a try. Let me know what you think.\n\nIf you like the idea, I would be happy to add similar conditions\nlike \"username\".\n\n[1] https://github.com/gitgitgadget/git/issues/1665\n\nIgnacio Encinas (1):\n config: learn the \"hostname:\" includeIf condition\n\n Documentation/config.txt | 9 +++++++++\n config.c | 16 ++++++++++++++++\n t/t1305-config-include.sh | 22 ++++++++++++++++++++++\n 3 files changed, 47 insertions(+)\n\n\nbase-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\n-- \n2.44.0\n\n"},{"id":"490218","messageId":"20240307205006.467443-2-ignacio@iencinas.com","threadId":"61072","inReplyTo":"20240307205006.467443-1-ignacio@iencinas.com","subject":"[RFC PATCH 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-07T20:50:06Z","receivedAt":"2024-03-07T20:50:35Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Currently, customizing the configuration depending on the machine running\ngit has to be done manually.\n\nAdd support for a new includeIf keyword \"hostname:\" to conditionally\ninclude configuration files depending on the hostname.\n\nSigned-off-by: Ignacio Encinas \n---\n Documentation/config.txt | 9 +++++++++\n config.c | 16 ++++++++++++++++\n t/t1305-config-include.sh | 22 ++++++++++++++++++++++\n 3 files changed, 47 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e3a74dd1c1..9a22fd2609 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n a naming scheme that supports more variable-based include conditions,\n but currently Git only supports the exact keyword described above.\n \n+`hostname`::\n+\tThe data that follows the keyword `hostname:` is taken to be a\n+\tpattern with standard globbing wildcards. If the current\n+\thostname matches the pattern, the include condition is met.\n+\n A few more notes on matching via `gitdir` and `gitdir/i`:\n \n * Symlinks in `$GIT_DIR` are not resolved before matching.\n@@ -261,6 +266,10 @@ Example\n \tpath = foo.inc\n [remote \"origin\"]\n \turl = https://example.com/git\n+\n+; include only if the hostname of the machine matches some-hostname\n+[includeIf \"hostname:some-hostname\"]\n+\tpath = foo.inc\n ----\n \n Values\ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd..e0611fc342 100644\n--- a/config.c\n+++ b/config.c\n@@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \treturn ret;\n }\n \n+static int include_by_hostname(const char *cond, size_t cond_len)\n+{\n+\tint ret;\n+\tchar my_host[HOST_NAME_MAX + 1];\n+\tstruct strbuf pattern = STRBUF_INIT;\n+\tif (xgethostname(my_host, sizeof(my_host)))\n+\t\treturn 0;\n+\n+\tstrbuf_add(&pattern, cond, cond_len);\n+\tret = !wildmatch(pattern.buf, my_host, 0);\n+\tstrbuf_release(&pattern);\n+\treturn ret;\n+}\n+\n static int add_remote_url(const char *var, const char *value,\n \t\t\t const struct config_context *ctx UNUSED, void *data)\n {\n@@ -406,6 +420,8 @@ static int include_condition_is_true(const struct key_value_info *kvi,\n \telse if (skip_prefix_mem(cond, cond_len, \"hasconfig:remote.*.url:\", &cond,\n \t\t\t\t &cond_len))\n \t\treturn include_by_remote_url(inc, cond, cond_len);\n+\telse if (skip_prefix_mem(cond, cond_len, \"hostname:\", &cond, &cond_len))\n+\t\treturn include_by_hostname(cond, cond_len);\n \n \t/* unknown conditionals are always false */\n \treturn 0;\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 5cde79ef8c..ee78d9cade 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '\n \tgrep \"exceeded maximum include depth\" stderr\n '\n \n+test_expect_success 'conditional include, hostname' '\n+\techo \"[includeIf \\\"hostname:$(hostname)a\\\"]path=bar12\" >>.git/config &&\n+\techo \"[test]twelve=12\" >.git/bar12 &&\n+\ttest_must_fail git config test.twelve &&\n+\n+\techo \"[includeIf \\\"hostname:$(hostname)\\\"]path=bar12\" >>.git/config &&\n+\techo 12 >expect &&\n+\tgit config test.twelve >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'conditional include, hostname, wildcard' '\n+\techo \"[includeIf \\\"hostname:$(hostname)a*\\\"]path=bar13\" >>.git/config &&\n+\techo \"[test]thirteen=13\" >.git/bar13 &&\n+\ttest_must_fail git config test.thirteen &&\n+\n+\techo \"[includeIf \\\"hostname:$(hostname)*\\\"]path=bar13\" >>.git/config &&\n+\techo 13 >expect &&\n+\tgit config test.thirteen >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"490227","messageId":"xmqqil1xk9fv.fsf@gitster.g","threadId":"61072","inReplyTo":"20240307205006.467443-2-ignacio@iencinas.com","subject":"Re: [RFC PATCH 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T21:40:04Z","receivedAt":"2024-03-07T21:40:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Encinas writes:\n\n> Currently, customizing the configuration depending on the machine running\n> git has to be done manually.\n>\n> Add support for a new includeIf keyword \"hostname:\" to conditionally\n> include configuration files depending on the hostname.\n>\n> Signed-off-by: Ignacio Encinas \n> ---\n> Documentation/config.txt | 9 +++++++++\n> config.c | 16 ++++++++++++++++\n> t/t1305-config-include.sh | 22 ++++++++++++++++++++++\n> 3 files changed, 47 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index e3a74dd1c1..9a22fd2609 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n> a naming scheme that supports more variable-based include conditions,\n> but currently Git only supports the exact keyword described above.\n\n> +`hostname`::\n> +\tThe data that follows the keyword `hostname:` is taken to be a\n> +\tpattern with standard globbing wildcards. If the current\n> +\thostname matches the pattern, the include condition is met.\n> +\n\nOK. This seems to copy its phrasing from the existing text for\n\"gitdir\" and \"onbranch\", which greatly helps the description for\nthese features consistent.\n\n> diff --git a/config.c b/config.c\n> index 3cfeb3d8bd..e0611fc342 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)\n> \treturn ret;\n> }\n> \n> +static int include_by_hostname(const char *cond, size_t cond_len)\n> +{\n> +\tint ret;\n> +\tchar my_host[HOST_NAME_MAX + 1];\n> +\tstruct strbuf pattern = STRBUF_INIT;\n> +\tif (xgethostname(my_host, sizeof(my_host)))\n> +\t\treturn 0;\n> +\n> +\tstrbuf_add(&pattern, cond, cond_len);\n> +\tret = !wildmatch(pattern.buf, my_host, 0);\n> +\tstrbuf_release(&pattern);\n> +\treturn ret;\n> +}\n\nHave a blank line between the end of the decl block (our codebase\nfrowns upon decl-after-statement) and the first statement,\ni.e. before \"if (xgethostname...\".\n\nOtherwise this looks reasonable to me.\n\n> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n> index 5cde79ef8c..ee78d9cade 100755\n> --- a/t/t1305-config-include.sh\n> +++ b/t/t1305-config-include.sh\n> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '\n> \tgrep \"exceeded maximum include depth\" stderr\n> '\n> \n> +test_expect_success 'conditional include, hostname' '\n> +\techo \"[includeIf \\\"hostname:$(hostname)a\\\"]path=bar12\" >>.git/config &&\n> +\techo \"[test]twelve=12\" >.git/bar12 &&\n> +\ttest_must_fail git config test.twelve &&\n\nEmulating other tests in this file that uses here document may make\nit a bit easier to read? E.g.,\n\n\tcat >>.gitconfig <<-EOF &&\n\t[includeIf \"hostname:$(hostname)a\"]\n\t\tpath = bar12\n\tEOF\n\n> +\techo \"[includeIf \\\"hostname:$(hostname)\\\"]path=bar12\" >>.git/config &&\n\nDitto for the remainder of the patch.\n\nThanks.\n\n"},{"id":"490290","messageId":"01a9baa2-b36b-4b4e-8e54-7645e35d1a47@iencinas.com","threadId":"61072","inReplyTo":"xmqqil1xk9fv.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-09T10:47:55Z","receivedAt":"2024-03-09T10:47:59Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 7/3/24 22:40, Junio C Hamano wrote:\n> Ignacio Encinas writes:\n> \n>> Currently, customizing the configuration depending on the machine running\n>> git has to be done manually.\n>>\n>> Add support for a new includeIf keyword \"hostname:\" to conditionally\n>> include configuration files depending on the hostname.\n>>\n>> Signed-off-by: Ignacio Encinas \n>> ---\n>> Documentation/config.txt | 9 +++++++++\n>> config.c | 16 ++++++++++++++++\n>> t/t1305-config-include.sh | 22 ++++++++++++++++++++++\n>> 3 files changed, 47 insertions(+)\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index e3a74dd1c1..9a22fd2609 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n>> a naming scheme that supports more variable-based include conditions,\n>> but currently Git only supports the exact keyword described above.\n> \n>> +`hostname`::\n>> +\tThe data that follows the keyword `hostname:` is taken to be a\n>> +\tpattern with standard globbing wildcards. If the current\n>> +\thostname matches the pattern, the include condition is met.\n>> +\n> \n> OK. This seems to copy its phrasing from the existing text for\n> \"gitdir\" and \"onbranch\", which greatly helps the description for\n> these features consistent.\n> \n>> diff --git a/config.c b/config.c\n>> index 3cfeb3d8bd..e0611fc342 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)\n>> \treturn ret;\n>> }\n>> \n>> +static int include_by_hostname(const char *cond, size_t cond_len)\n>> +{\n>> +\tint ret;\n>> +\tchar my_host[HOST_NAME_MAX + 1];\n>> +\tstruct strbuf pattern = STRBUF_INIT;\n>> +\tif (xgethostname(my_host, sizeof(my_host)))\n>> +\t\treturn 0;\n>> +\n>> +\tstrbuf_add(&pattern, cond, cond_len);\n>> +\tret = !wildmatch(pattern.buf, my_host, 0);\n>> +\tstrbuf_release(&pattern);\n>> +\treturn ret;\n>> +}\n> \n> Have a blank line between the end of the decl block (our codebase\n> frowns upon decl-after-statement) and the first statement,\n> i.e. before \"if (xgethostname...\".\n> \n> Otherwise this looks reasonable to me.\n\nGot it. \n \n>> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n>> index 5cde79ef8c..ee78d9cade 100755\n>> --- a/t/t1305-config-include.sh\n>> +++ b/t/t1305-config-include.sh\n>> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '\n>> \tgrep \"exceeded maximum include depth\" stderr\n>> '\n>> \n>> +test_expect_success 'conditional include, hostname' '\n>> +\techo \"[includeIf \\\"hostname:$(hostname)a\\\"]path=bar12\" >>.git/config &&\n>> +\techo \"[test]twelve=12\" >.git/bar12 &&\n>> +\ttest_must_fail git config test.twelve &&\n> \n> Emulating other tests in this file that uses here document may make\n> it a bit easier to read? E.g.,\n> \n> \tcat >>.gitconfig <<-EOF &&\n> \t[includeIf \"hostname:$(hostname)a\"]\n> \t\tpath = bar12\n> \tEOF\n\nThanks for pointing that out. I just read the last tests from that file\nwhere they used the echo \"...\" >> approach. Do you think it is\nworthwhile rewriting those tests to use the approach you suggested?\n\nBy the way, before contributing, I saw there is some work on moving to\nunit tests. I wasn't sure how to test this particular feature there, so\nI went with the \"old\" approach as it seemed more natural. Is this ok?\n\n>> +\techo \"[includeIf \\\"hostname:$(hostname)\\\"]path=bar12\" >>.git/config &&\n> \n> Ditto for the remainder of the patch.\n> \n> Thanks.\n\nThank you for the review.\n"},{"id":"490298","messageId":"xmqq8r2re265.fsf@gitster.g","threadId":"61072","inReplyTo":"01a9baa2-b36b-4b4e-8e54-7645e35d1a47@iencinas.com","subject":"Re: [RFC PATCH 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T17:38:10Z","receivedAt":"2024-03-09T17:38:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Encinas Rubio writes:\n\n>>> +test_expect_success 'conditional include, hostname' '\n>>> +\techo \"[includeIf \\\"hostname:$(hostname)a\\\"]path=bar12\" >>.git/config &&\n>>> +\techo \"[test]twelve=12\" >.git/bar12 &&\n>>> +\ttest_must_fail git config test.twelve &&\n>> \n>> Emulating other tests in this file that uses here document may make\n>> it a bit easier to read? E.g.,\n>> \n>> \tcat >>.gitconfig <<-EOF &&\n>> \t[includeIf \"hostname:$(hostname)a\"]\n>> \t\tpath = bar12\n>> \tEOF\n>\n> Thanks for pointing that out. I just read the last tests from that file\n> where they used the echo \"...\" >> approach. Do you think it is\n> worthwhile rewriting those tests to use the approach you suggested?\n\nI had an impression that existing ones do not have ugliness of\nbackslash-quoting and do not benefit from such a rewrite to use\nhere-document as much as the one you added does. \n\nIf that is not the case, and existing ones would concretely improve\nthe readability with such a rewrite to use here-document, surely.\nIf we want to do that route, we should either do one of the two.\n\n - The patch [1/2] does such a \"style update\" only on existing tests\n to improve their readability, and then patch [2/2] then does your\n addition to the tests, together with the code change.\n\n - Or you do this patch alone, without touching existing tests, but\n with your tests added in an easier-to-read style. And then after\n the dust settles, a separate \"style udpate\" patch clean the\n existing ones up.\n\n> By the way, before contributing, I saw there is some work on moving to\n> unit tests. I wasn't sure how to test this particular feature there, so\n> I went with the \"old\" approach as it seemed more natural. Is this ok?\n\nWe are not really \"moving to\". We did not have a test framework for\neffective unit tests, so some tests that would have been easier to\nmanage as unit tests were instead written as an end-to-end\nintegration tests, which was what we had a framework for. These\ntests are moving to, but for a test like \"the user uses the\n'[includeIf X:Y] path = P' construct---does the git command really\nshows the effect of include from P when condition X:Y holds?\", the\nunit testing framework would not be a better fit than the end-to-end\nbehaviour test, I would say.\n\n\n"},{"id":"490304","messageId":"20240309181828.45496-1-ignacio@iencinas.com","threadId":"61072","inReplyTo":"20240307205006.467443-1-ignacio@iencinas.com","subject":"[PATCH v2 0/1] Add hostname condition to includeIf","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-09T18:18:27Z","receivedAt":"2024-03-09T18:20:48Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Extend includeIf to take hostname into account. Motivating request can\nbe found here [1].\n\n[1] https://github.com/gitgitgadget/git/issues/1665\n\nChanges since v1:\n* Add blank line between declarations and code in `include_by_branch`.\n* Rewrite \"echo\"s used in tests to make them more readable. \n\n config: learn the \"hostname:\" includeIf condition\n\n Documentation/config.txt | 9 +++++++++\n config.c | 17 ++++++++++++++++\n t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 68 insertions(+)\n\nRange-diff against v1:\n1: 10a9bca68753 ! 1: cf175154109e config: learn the \"hostname:\" includeIf condition\n @@ config.c: static int include_by_branch(const char *cond, size_t cond_len)\n +\tint ret;\n +\tchar my_host[HOST_NAME_MAX + 1];\n +\tstruct strbuf pattern = STRBUF_INIT;\n ++\n +\tif (xgethostname(my_host, sizeof(my_host)))\n +\t\treturn 0;\n +\n @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '\n '\n \n +test_expect_success 'conditional include, hostname' '\n -+\techo \"[includeIf \\\"hostname:$(hostname)a\\\"]path=bar12\" >>.git/config &&\n -+\techo \"[test]twelve=12\" >.git/bar12 &&\n ++\tcat >>.git/config <<-EOF &&\n ++\t[includeIf \"hostname:$(hostname)a\"]\n ++\t\tpath = bar12\n ++\tEOF\n ++\tcat >>.git/bar12 <<-EOF &&\n ++\t[test]\n ++\t\ttwelve=12\n ++\tEOF\n ++\n +\ttest_must_fail git config test.twelve &&\n +\n -+\techo \"[includeIf \\\"hostname:$(hostname)\\\"]path=bar12\" >>.git/config &&\n ++\tcat >>.git/config <<-EOF &&\n ++\t[includeIf \"hostname:$(hostname)\"]\n ++\t\tpath = bar12\n ++\tEOF\n +\techo 12 >expect &&\n +\tgit config test.twelve >actual &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'conditional include, hostname, wildcard' '\n -+\techo \"[includeIf \\\"hostname:$(hostname)a*\\\"]path=bar13\" >>.git/config &&\n -+\techo \"[test]thirteen=13\" >.git/bar13 &&\n ++\tcat >>.git/config <<-EOF &&\n ++\t[includeIf \"hostname:$(hostname)a*\"]\n ++\t\tpath = bar13\n ++\tEOF\n ++\tcat >>.git/bar13 <<-EOF &&\n ++\t[test]\n ++\t\tthirteen = 13\n ++\tEOF\n ++\n +\ttest_must_fail git config test.thirteen &&\n +\n -+\techo \"[includeIf \\\"hostname:$(hostname)*\\\"]path=bar13\" >>.git/config &&\n ++\tcat >>.git/config <<-EOF &&\n ++\t[includeIf \"hostname:$(hostname)*\"]\n ++\t\tpath = bar13\n ++\tEOF\n +\techo 13 >expect &&\n +\tgit config test.thirteen >actual &&\n +\ttest_cmp expect actual\n\nbase-commit: e09f1254c54329773904fe25d7c545a1fb4fa920\n-- \n2.44.0\n\n"},{"id":"490305","messageId":"20240309181828.45496-2-ignacio@iencinas.com","threadId":"61072","inReplyTo":"20240309181828.45496-1-ignacio@iencinas.com","subject":"[PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-09T18:18:28Z","receivedAt":"2024-03-09T18:20:51Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Currently, customizing the configuration depending on the machine running\ngit has to be done manually.\n\nAdd support for a new includeIf keyword \"hostname:\" to conditionally\ninclude configuration files depending on the hostname.\n\nSigned-off-by: Ignacio Encinas \n---\n Documentation/config.txt | 9 +++++++++\n config.c | 17 ++++++++++++++++\n t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 68 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e3a74dd1c19d..9a22fd260935 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n a naming scheme that supports more variable-based include conditions,\n but currently Git only supports the exact keyword described above.\n \n+`hostname`::\n+\tThe data that follows the keyword `hostname:` is taken to be a\n+\tpattern with standard globbing wildcards. If the current\n+\thostname matches the pattern, the include condition is met.\n+\n A few more notes on matching via `gitdir` and `gitdir/i`:\n \n * Symlinks in `$GIT_DIR` are not resolved before matching.\n@@ -261,6 +266,10 @@ Example\n \tpath = foo.inc\n [remote \"origin\"]\n \turl = https://example.com/git\n+\n+; include only if the hostname of the machine matches some-hostname\n+[includeIf \"hostname:some-hostname\"]\n+\tpath = foo.inc\n ----\n \n Values\ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd99..50b3f6d24c50 100644\n--- a/config.c\n+++ b/config.c\n@@ -317,6 +317,21 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \treturn ret;\n }\n \n+static int include_by_hostname(const char *cond, size_t cond_len)\n+{\n+\tint ret;\n+\tchar my_host[HOST_NAME_MAX + 1];\n+\tstruct strbuf pattern = STRBUF_INIT;\n+\n+\tif (xgethostname(my_host, sizeof(my_host)))\n+\t\treturn 0;\n+\n+\tstrbuf_add(&pattern, cond, cond_len);\n+\tret = !wildmatch(pattern.buf, my_host, 0);\n+\tstrbuf_release(&pattern);\n+\treturn ret;\n+}\n+\n static int add_remote_url(const char *var, const char *value,\n \t\t\t const struct config_context *ctx UNUSED, void *data)\n {\n@@ -406,6 +421,8 @@ static int include_condition_is_true(const struct key_value_info *kvi,\n \telse if (skip_prefix_mem(cond, cond_len, \"hasconfig:remote.*.url:\", &cond,\n \t\t\t\t &cond_len))\n \t\treturn include_by_remote_url(inc, cond, cond_len);\n+\telse if (skip_prefix_mem(cond, cond_len, \"hostname:\", &cond, &cond_len))\n+\t\treturn include_by_hostname(cond, cond_len);\n \n \t/* unknown conditionals are always false */\n \treturn 0;\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 5cde79ef8c4f..e0a1d51d50ad 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -357,4 +357,46 @@ test_expect_success 'include cycles are detected' '\n \tgrep \"exceeded maximum include depth\" stderr\n '\n \n+test_expect_success 'conditional include, hostname' '\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(hostname)a\"]\n+\t\tpath = bar12\n+\tEOF\n+\tcat >>.git/bar12 <<-EOF &&\n+\t[test]\n+\t\ttwelve=12\n+\tEOF\n+\n+\ttest_must_fail git config test.twelve &&\n+\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(hostname)\"]\n+\t\tpath = bar12\n+\tEOF\n+\techo 12 >expect &&\n+\tgit config test.twelve >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'conditional include, hostname, wildcard' '\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(hostname)a*\"]\n+\t\tpath = bar13\n+\tEOF\n+\tcat >>.git/bar13 <<-EOF &&\n+\t[test]\n+\t\tthirteen = 13\n+\tEOF\n+\n+\ttest_must_fail git config test.thirteen &&\n+\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(hostname)*\"]\n+\t\tpath = bar13\n+\tEOF\n+\techo 13 >expect &&\n+\tgit config test.thirteen >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"490328","messageId":"xmqqy1aqvx9n.fsf@gitster.g","threadId":"61072","inReplyTo":"20240309181828.45496-2-ignacio@iencinas.com","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-10T16:59:00Z","receivedAt":"2024-03-10T16:59:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Encinas writes:\n\n> +test_expect_success 'conditional include, hostname' '\n> +\tcat >>.git/config <<-EOF &&\n> +\t[includeIf \"hostname:$(hostname)a\"]\n\nThis unconditionally runs the $(hostname) command assuming it exists\neverywhere, but\n\n $ git grep '$(hostname' t/\n t/t6500-gc.sh:\thostname=$(hostname || echo unknown) &&\n\ntells us that we should be prepared to meet a platform where such a\ncommand does not exist.\n\nI have a feeling that it is better done with a test prerequisite\nthan hardcoded \"unknown\", as xgethostname() at C level may come up\nwith some random string but it is not guaranteed to be \"unknown\".\n\nPerhaps have one instance of this before these added tests\n\n\ttest_lazy_prereq WORKING_HOSTNAME '\n\t\thostname >/dev/null 2>&1\n\t'\n\nand then start them with\n\n\ttest_expect_success WORKING_HOSTNAME 'hostname: includeIf' '\n\t\t...\n\t'\n\nor something? Others may think of a better way to make sure this\ntest does not cause false failures on platforms only because they\nlack working hostname(1) but have a working gethostname(2) and their\nxgethostname() may be working fine.\n\nThanks.\n\n\n"},{"id":"490329","messageId":"9fb02065-12d2-4c92-b1a5-74c06125c692@iencinas.com","threadId":"61072","inReplyTo":"xmqqy1aqvx9n.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-10T18:46:14Z","receivedAt":"2024-03-10T18:46:19Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 10/3/24 17:59, Junio C Hamano wrote:\n> Ignacio Encinas writes:\n> \n>> +test_expect_success 'conditional include, hostname' '\n>> +\tcat >>.git/config <<-EOF &&\n>> +\t[includeIf \"hostname:$(hostname)a\"]\n> \n> This unconditionally runs the $(hostname) command assuming it exists\n> everywhere, but\n> \n> $ git grep '$(hostname' t/\n> t/t6500-gc.sh:\thostname=$(hostname || echo unknown) &&\n> \n> tells us that we should be prepared to meet a platform where such a\n> command does not exist.\n\nOops, it didn't even cross my mind. Thanks for the catch!\n\n> I have a feeling that it is better done with a test prerequisite\n> than hardcoded \"unknown\", as xgethostname() at C level may come up\n> with some random string but it is not guaranteed to be \"unknown\".\n\nI agree. Not being able to query the current hostname defeats the\npurpose of the tests.\n\n> Perhaps have one instance of this before these added tests\n> \n> \ttest_lazy_prereq WORKING_HOSTNAME '\n> \t\thostname >/dev/null 2>&1\n> \t'\n> \n> and then start them with\n> \n> \ttest_expect_success WORKING_HOSTNAME 'hostname: includeIf' '\n> \t\t...\n> \t'\n \nThanks for providing an example. \n\n> or something? Others may think of a better way to make sure this\n> test does not cause false failures on platforms only because they\n> lack working hostname(1) but have a working gethostname(2) and their\n> xgethostname() may be working fine.\n\nI can't think of any room for improvement other than integrating\nhostname (or a custom hostname) into git and using it in the tests, but\nI doubt it is worth it.\n\n> Thanks.\n\nThank you. I will wait a couple of days to post the v3 to see if anyone \nhas a suggestion.\n"},{"id":"490372","messageId":"xmqqil1ssm5y.fsf@gitster.g","threadId":"61072","inReplyTo":"9fb02065-12d2-4c92-b1a5-74c06125c692@iencinas.com","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T17:39:21Z","receivedAt":"2024-03-11T17:39:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Encinas Rubio writes:\n\n> I can't think of any room for improvement other than integrating\n> hostname (or a custom hostname) into git and using it in the tests, but\n> I doubt it is worth it.\n\nAh, that is a thought. We have t/helper that builds \"test-tool\"\njust for that, and exposing the output of xhostname() does sounds\nlike a reasonable way to go. It would roughly involve\n\n * Add t/helper/test-xhostname.c that defines cmd__xhostname() and\n writes the result of calling xhostname() to its standard output.\n\n * Plumb it through by adding it to a few places:\n\n - t/helper/test-tool.h wants the extern definition.\n - t/helper/test-tool.c wants it in its cmds[] array.\n - Makefile wants to list it in TEST_BUILTIN_OBJS\n\n * Then use \"test-tool xhostname\" in your tests, instead of\n \"hostname\".\n\nYou can run\n\n $ git grep chmtime ':!t/*.sh\"\n\nto find places that needed to be touched when a similar internal\ntool \"chmtime\" was added.\n\n"},{"id":"490585","messageId":"fda3e8f4-fd9e-4a43-a307-c6607d982436@iencinas.com","threadId":"61072","inReplyTo":"xmqqil1ssm5y.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-13T21:53:50Z","receivedAt":"2024-03-13T21:54:04Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 11/3/24 18:39, Junio C Hamano wrote:\n> Ignacio Encinas Rubio writes:\n> \n>> I can't think of any room for improvement other than integrating\n>> hostname (or a custom hostname) into git and using it in the tests, but\n>> I doubt it is worth it.\n> \n> Ah, that is a thought. We have t/helper that builds \"test-tool\"\n> just for that, and exposing the output of xhostname() does sounds\n> like a reasonable way to go. It would roughly involve\n\nGreat! I hadn't noticed \"test-tool\". Just to double-check, what name do\nwe want to use for this? xhostname, hostname, xgethostname, gethostname?\n\nIf I didn't miss something, the only place the test use hostname is in \n\n $ git grep '$(hostname' t/\n t/t6500-gc.sh:\thostname=$(hostname || echo unknown) &&\n\nas you previously pointed out. So my plan is:\n\n1. Extend test-tool, migrate t6500-gc.sh to test-tool xhostname(*)\n2. Update my v2 to use \"test-tool xhostname(*)\"\n\n(*) or however we want to name it\n\n> * Add t/helper/test-xhostname.c that defines cmd__xhostname() and\n> writes the result of calling xhostname() to its standard output.\n> \n> * Plumb it through by adding it to a few places:\n> \n> - t/helper/test-tool.h wants the extern definition.\n> - t/helper/test-tool.c wants it in its cmds[] array.\n> - Makefile wants to list it in TEST_BUILTIN_OBJS\n> \n> * Then use \"test-tool xhostname\" in your tests, instead of\n> \"hostname\".\n> \n> You can run\n> \n> $ git grep chmtime ':!t/*.sh\"\n> \n> to find places that needed to be touched when a similar internal\n> tool \"chmtime\" was added.\n\nThank you very much for the pointers, they were very helpful!\n"},{"id":"490774","messageId":"20240316065737.GA544929@coredump.intra.peff.net","threadId":"61072","inReplyTo":"20240309181828.45496-2-ignacio@iencinas.com","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-16T06:57:37Z","receivedAt":"2024-03-16T06:57:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index e3a74dd1c19d..9a22fd260935 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n> a naming scheme that supports more variable-based include conditions,\n> but currently Git only supports the exact keyword described above.\n> \n> +`hostname`::\n> +\tThe data that follows the keyword `hostname:` is taken to be a\n> +\tpattern with standard globbing wildcards. If the current\n> +\thostname matches the pattern, the include condition is met.\n\nDo we need to define \"hostname\" in more detail here? Specifically, I'm\nwondering whether the result will be a FQDN or not (i.e., the output of\n\"hostname\" vs \"hostname -f\"). Looking at the code I think it will just\nbe the short name returned. That's probably OK, but it may be worth\ndocumenting.\n\n-Peff\n"},{"id":"490777","messageId":"857e512c-d6d8-4bad-9eb5-5ca233a1c884@iencinas.com","threadId":"61072","inReplyTo":"20240316065737.GA544929@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-16T11:19:44Z","receivedAt":"2024-03-16T11:19:49Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 16/3/24 7:57, Jeff King wrote:\n> On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:\n> \n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index e3a74dd1c19d..9a22fd260935 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n>> a naming scheme that supports more variable-based include conditions,\n>> but currently Git only supports the exact keyword described above.\n>> \n>> +`hostname`::\n>> +\tThe data that follows the keyword `hostname:` is taken to be a\n>> +\tpattern with standard globbing wildcards. If the current\n>> +\thostname matches the pattern, the include condition is met.\n> \n> Do we need to define \"hostname\" in more detail here? Specifically, I'm\n> wondering whether the result will be a FQDN or not (i.e., the output of\n> \"hostname\" vs \"hostname -f\"). Looking at the code I think it will just\n> be the short name returned. That's probably OK, but it may be worth\n> documenting.\n\nThanks for pointing it out. I agree that it should be further clarified. \n\nIndeed, I was referring to the short name reported by gethostname(2), \nwhich should agree with \"hostname\". What do you think about\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9a22fd260935..268a9fab7be0 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -189,7 +189,8 @@ but currently Git only supports the exact keyword described above.\n `hostname`::\n The data that follows the keyword `hostname:` is taken to be a\n pattern with standard globbing wildcards. If the current\n- hostname matches the pattern, the include condition is met.\n+ hostname (output of gethostname(2)) matches the\n+ pattern, the include condition is met.\n"},{"id":"490783","messageId":"ZfXCJAsACzKLndNm@nand.local","threadId":"61072","inReplyTo":"857e512c-d6d8-4bad-9eb5-5ca233a1c884@iencinas.com","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-03-16T16:00:36Z","receivedAt":"2024-03-16T16:00:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Mar 16, 2024 at 12:19:44PM +0100, Ignacio Encinas Rubio wrote:\n>\n>\n> On 16/3/24 7:57, Jeff King wrote:\n> > On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:\n> >\n> >> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> >> index e3a74dd1c19d..9a22fd260935 100644\n> >> --- a/Documentation/config.txt\n> >> +++ b/Documentation/config.txt\n> >> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n> >> a naming scheme that supports more variable-based include conditions,\n> >> but currently Git only supports the exact keyword described above.\n> >>\n> >> +`hostname`::\n> >> +\tThe data that follows the keyword `hostname:` is taken to be a\n> >> +\tpattern with standard globbing wildcards. If the current\n> >> +\thostname matches the pattern, the include condition is met.\n> >\n> > Do we need to define \"hostname\" in more detail here? Specifically, I'm\n> > wondering whether the result will be a FQDN or not (i.e., the output of\n> > \"hostname\" vs \"hostname -f\"). Looking at the code I think it will just\n> > be the short name returned. That's probably OK, but it may be worth\n> > documenting.\n>\n> Thanks for pointing it out. I agree that it should be further clarified.\n>\n> Indeed, I was referring to the short name reported by gethostname(2),\n> which should agree with \"hostname\". What do you think about\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 9a22fd260935..268a9fab7be0 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -189,7 +189,8 @@ but currently Git only supports the exact keyword described above.\n> `hostname`::\n> The data that follows the keyword `hostname:` is taken to be a\n> pattern with standard globbing wildcards. If the current\n> - hostname matches the pattern, the include condition is met.\n> + hostname (output of gethostname(2)) matches the\n\nHmm. gethostname(2)'s manual page isn't overly specific on the details\nhere, either.\n\nI admittedly don't love the idea of documenting this implementation\ndetail (that is, that we are calling gethostname() and using its output\nto compare against). I think it's fine to say instead, \"the short\nhostname\", and leave it at that.\n\nAlternatively, you could say \"If the machine's short hostname (as\nopposed to a fully-qualified hostname, as returned by `hostname -f`)\nmatches the pattern [...]\".\n\nI think I have a vague preference towards the latter.\n\nThanks,\nTaylor\n"},{"id":"490784","messageId":"ZfXCXAdlYdS/p8BC@nand.local","threadId":"61072","inReplyTo":"20240309181828.45496-2-ignacio@iencinas.com","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-03-16T16:01:32Z","receivedAt":"2024-03-16T16:01:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Ignacio,\n\nOn Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:\n> ---\n> Documentation/config.txt | 9 +++++++++\n> config.c | 17 ++++++++++++++++\n> t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 68 insertions(+)\n\nI took a look at this patch and felt that it was all very sensible. I\nleft one comment in reply to the sub-thread with you and Peff with some\nminor suggestions on the documentation.\n\nOtherwise, the code changes here all look reasonable to me.\n\nThanks,\nTaylor\n"},{"id":"490785","messageId":"f20a342c-d6e2-47da-95cf-b61d9dd282c8@iencinas.com","threadId":"61072","inReplyTo":"ZfXCJAsACzKLndNm@nand.local","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-16T16:46:09Z","receivedAt":"2024-03-16T16:46:16Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 16/3/24 17:00, Taylor Blau wrote:\n> On Sat, Mar 16, 2024 at 12:19:44PM +0100, Ignacio Encinas Rubio wrote:\n>>\n>>\n>> On 16/3/24 7:57, Jeff King wrote:\n>>> On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:\n>>>\n>>>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>>>> index e3a74dd1c19d..9a22fd260935 100644\n>>>> --- a/Documentation/config.txt\n>>>> +++ b/Documentation/config.txt\n>>>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with\n>>>> a naming scheme that supports more variable-based include conditions,\n>>>> but currently Git only supports the exact keyword described above.\n>>>>\n>>>> +`hostname`::\n>>>> +\tThe data that follows the keyword `hostname:` is taken to be a\n>>>> +\tpattern with standard globbing wildcards. If the current\n>>>> +\thostname matches the pattern, the include condition is met.\n>>>\n>>> Do we need to define \"hostname\" in more detail here? Specifically, I'm\n>>> wondering whether the result will be a FQDN or not (i.e., the output of\n>>> \"hostname\" vs \"hostname -f\"). Looking at the code I think it will just\n>>> be the short name returned. That's probably OK, but it may be worth\n>>> documenting.\n>>\n>> Thanks for pointing it out. I agree that it should be further clarified.\n>>\n>> Indeed, I was referring to the short name reported by gethostname(2),\n>> which should agree with \"hostname\". What do you think about\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 9a22fd260935..268a9fab7be0 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -189,7 +189,8 @@ but currently Git only supports the exact keyword described above.\n>> `hostname`::\n>> The data that follows the keyword `hostname:` is taken to be a\n>> pattern with standard globbing wildcards. If the current\n>> - hostname matches the pattern, the include condition is met.\n>> + hostname (output of gethostname(2)) matches the\n> \n> Hmm. gethostname(2)'s manual page isn't overly specific on the details\n> here, either.\n> \n> I admittedly don't love the idea of documenting this implementation\n> detail (that is, that we are calling gethostname() and using its output\n> to compare against). I think it's fine to say instead, \"the short\n> hostname\", and leave it at that.\n\nI agree it isn't too descriptive, but the reason I chose to do it was\nbecause this doesn't seem thoroughly documented anywhere:\n\nhostname(1):\n\n hostname will print the name of the system as returned by the gethostname(2) function.\n\n -s, --short\n Display the short host name. This is the host name cut at the first dot.\n\nI have only superficial knowledge about the terminology, but from what I\nhave read, it seems like we're actually reading the \"nodename\" (see\nuname(2)), which shouldn't but can contain dots \".\", which \"hostname -s\"\nwill trim, but \"hostname\" won't.\n\nAfter seeing all this and the huge potential for confusing everybody, I\nchose the easy way out.\n\nI'm ok with saying \"short hostname\" but I'm not terribly happy with it\nas it won't match \"hostname -s\" if \"nodename\" has dots (it will always\nmatch \"hostname\" from what I have seen in the hostname source code from\nthe debian package which I assume everyone uses).\n\nDo you think this is worth worrying about? Or people with \"nodename\"s\nmaking \"hostname\" and \"hostname --short\" disagree should know that by\nshort hostname we mean \"hostname\" and not \"hostname --short\".\n\nI might be missing something, but I somehow find all of this pretty\nconfusing.\n\n> Alternatively, you could say \"If the machine's short hostname (as\n> opposed to a fully-qualified hostname, as returned by `hostname -f`)\n> matches the pattern [...]\".\n> \n> I think I have a vague preference towards the latter.\n> Thanks,\n> Taylor\n\nThank you for the review!\n"},{"id":"490786","messageId":"4ffc4ed2-0a18-452d-baca-6d72cc0ab2ca@iencinas.com","threadId":"61072","inReplyTo":"ZfXCXAdlYdS/p8BC@nand.local","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-16T16:50:50Z","receivedAt":"2024-03-16T16:50:53Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 16/3/24 17:01, Taylor Blau wrote:\n> Hi Ignacio,\n> \n> On Sat, Mar 09, 2024 at 07:18:28PM +0100, Ignacio Encinas wrote:\n>> ---\n>> Documentation/config.txt | 9 +++++++++\n>> config.c | 17 ++++++++++++++++\n>> t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++\n>> 3 files changed, 68 insertions(+)\n> \n> I took a look at this patch and felt that it was all very sensible. I\n> left one comment in reply to the sub-thread with you and Peff with some\n> minor suggestions on the documentation.\n>\n> Otherwise, the code changes here all look reasonable to me.\n\nThanks for the review!\n \n> Thanks,\n> Taylor\n"},{"id":"490787","messageId":"xmqqo7bem7o8.fsf@gitster.g","threadId":"61072","inReplyTo":"20240316065737.GA544929@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-16T17:02:31Z","receivedAt":"2024-03-16T17:02:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Do we need to define \"hostname\" in more detail here? Specifically, I'm\n> wondering whether the result will be a FQDN or not (i.e., the output of\n> \"hostname\" vs \"hostname -f\"). Looking at the code I think it will just\n> be the short name returned. That's probably OK, but it may be worth\n> documenting.\n\nThat was my first reaction but there are places where \"hostname\"\nalready gives a name that is not \"short\" at all, without being\ninvoked with \"-f\".\n\nFor example, the (virtual) workstation I am typing this message on\nsits in a $WORK datacenter, where \"hostname\" gives the same string\nas \"hostname -f\", which looks like \"git.c.xxxxxx.tld\" (\"git\" is the\nonly part I picked myself for it, \"c\" is shared by those employee\nworkstations hosted at datacenters, \"xxxxxx.tld\" is redacted to\nconceal the real domain name to protect the culprits ;-).\n\nI think the most honest answer we can give in the documentation is\nthat we use what gethostname() [*] gives.\n\n\n[References]\n\n* https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html\n"},{"id":"490789","messageId":"006c01da77c9$298b2e50$7ca18af0$@nexbridge.com","threadId":"61072","inReplyTo":"xmqqo7bem7o8.fsf@gitster.g","subject":"RE: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-16T17:41:17Z","receivedAt":"2024-03-16T17:41:55Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:\n>Jeff King writes:\n>\n>> Do we need to define \"hostname\" in more detail here? Specifically, I'm\n>> wondering whether the result will be a FQDN or not (i.e., the output\n>> of \"hostname\" vs \"hostname -f\"). Looking at the code I think it will\n>> just be the short name returned. That's probably OK, but it may be\n>> worth documenting.\n>\n>That was my first reaction but there are places where \"hostname\"\n>already gives a name that is not \"short\" at all, without being invoked with\n\"-f\".\n>\n>For example, the (virtual) workstation I am typing this message on sits in\na $WORK datacenter, where \"hostname\" gives the same\n>string as \"hostname -f\", which looks like \"git.c.xxxxxx.tld\" (\"git\" is the\nonly part I picked myself for it, \"c\" is shared by those employee\n>workstations hosted at datacenters, \"xxxxxx.tld\" is redacted to conceal the\nreal domain name to protect the culprits ;-).\n>\n>I think the most honest answer we can give in the documentation is that we\nuse what gethostname() [*] gives.\n\nI think this is probably a good idea and but value should not be cached. My\ndev box has a multi-home, multi-cpu IP stack. It makes things really weird\nsometimes. For example, hostname replies with:\n\nztc0.xxxxxxxx.local\n\nand includes the current default IP stack, which is known to DNS, while\nuname -n, which I prefer to use when deciding what system I am on during\ntests, reports:\n\nxxxxxxxx\n\nI am not sure how meaningful hostname is; however, \"hostname -f\" is not\nportable. However, includeif depending on whatever gethostname() returns is\nreasonable, in my opinion, also. I think the series should include a $(uname\n-n) option in some form for completeness.\n\n>\n>\n>[References]\n>\n>*\nhttps://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html\n\n--Randall\n\n"},{"id":"490790","messageId":"ea07fbc2-8937-486b-87c2-e2abe9856d26@iencinas.com","threadId":"61072","inReplyTo":"006c01da77c9$298b2e50$7ca18af0$@nexbridge.com","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-16T18:05:38Z","receivedAt":"2024-03-16T18:05:43Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 16/3/24 18:41, rsbecker@nexbridge.com wrote:\n> On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> Do we need to define \"hostname\" in more detail here? Specifically, I'm\n>>> wondering whether the result will be a FQDN or not (i.e., the output\n>>> of \"hostname\" vs \"hostname -f\"). Looking at the code I think it will\n>>> just be the short name returned. That's probably OK, but it may be\n>>> worth documenting.\n>>\n>> That was my first reaction but there are places where \"hostname\"\n>> already gives a name that is not \"short\" at all, without being invoked with\n> \"-f\".\n>>\n>> For example, the (virtual) workstation I am typing this message on sits in\n> a $WORK datacenter, where \"hostname\" gives the same\n>> string as \"hostname -f\", which looks like \"git.c.xxxxxx.tld\" (\"git\" is the\n> only part I picked myself for it, \"c\" is shared by those employee\n>> workstations hosted at datacenters, \"xxxxxx.tld\" is redacted to conceal the\n> real domain name to protect the culprits ;-).\n>>\n>> I think the most honest answer we can give in the documentation is that we\n> use what gethostname() [*] gives.\n> \n> I think this is probably a good idea and but value should not be cached. My\n> dev box has a multi-home, multi-cpu IP stack. It makes things really weird\n> sometimes. For example, hostname replies with:\n> \n> ztc0.xxxxxxxx.local\n> \n> and includes the current default IP stack, which is known to DNS, while\n> uname -n, which I prefer to use when deciding what system I am on during\n> tests, reports:\n> \n> xxxxxxxx\n> \n> I am not sure how meaningful hostname is; however, \"hostname -f\" is not\n> portable. However, includeif depending on whatever gethostname() returns is\n> reasonable, in my opinion, also. I think the series should include a $(uname\n> -n) option in some form for completeness.\n\nCorrect me if I'm wrong, but gethostname() seems to be equivalent to\n$(uname -n)\n\n[1] https://git.musl-libc.org/cgit/musl/tree/src/unistd/gethostname.c \n[2] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/gethostname.c;h=3c50706b5823368a0b3e876491e554461a4d515e;hb=HEAD\n\n>>\n>>\n>> [References]\n>>\n>> *\n> https://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html\n> \n> --Randall\n> \n"},{"id":"490793","messageId":"006d01da77d2$ae4361e0$0aca25a0$@nexbridge.com","threadId":"61072","inReplyTo":"ea07fbc2-8937-486b-87c2-e2abe9856d26@iencinas.com","subject":"RE: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-16T18:49:25Z","receivedAt":"2024-03-16T18:49:43Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Saturday, March 16, 2024 2:06 PM, Ignacio Encinas Rubio wrote:\n>On 16/3/24 18:41, rsbecker@nexbridge.com wrote:\n>> On Saturday, March 16, 2024 1:03 PM, Junio C Hamano wrote:\n>>> Jeff King writes:\n>>>\n>>>> Do we need to define \"hostname\" in more detail here? Specifically,\n>>>> I'm wondering whether the result will be a FQDN or not (i.e., the\n>>>> output of \"hostname\" vs \"hostname -f\"). Looking at the code I think\n>>>> it will just be the short name returned. That's probably OK, but it\n>>>> may be worth documenting.\n>>>\n>>> That was my first reaction but there are places where \"hostname\"\n>>> already gives a name that is not \"short\" at all, without being\n>>> invoked with\n>> \"-f\".\n>>>\n>>> For example, the (virtual) workstation I am typing this message on\n>>> sits in\n>> a $WORK datacenter, where \"hostname\" gives the same\n>>> string as \"hostname -f\", which looks like \"git.c.xxxxxx.tld\" (\"git\"\n>>> is the\n>> only part I picked myself for it, \"c\" is shared by those employee\n>>> workstations hosted at datacenters, \"xxxxxx.tld\" is redacted to\n>>> conceal the\n>> real domain name to protect the culprits ;-).\n>>>\n>>> I think the most honest answer we can give in the documentation is\n>>> that we\n>> use what gethostname() [*] gives.\n>>\n>> I think this is probably a good idea and but value should not be\n>> cached. My dev box has a multi-home, multi-cpu IP stack. It makes\n>> things really weird sometimes. For example, hostname replies with:\n>>\n>> ztc0.xxxxxxxx.local\n>>\n>> and includes the current default IP stack, which is known to DNS,\n>> while uname -n, which I prefer to use when deciding what system I am\n>> on during tests, reports:\n>>\n>> xxxxxxxx\n>>\n>> I am not sure how meaningful hostname is; however, \"hostname -f\" is\n>> not portable. However, includeif depending on whatever gethostname()\n>> returns is reasonable, in my opinion, also. I think the series should\n>> include a $(uname\n>> -n) option in some form for completeness.\n>\n>Correct me if I'm wrong, but gethostname() seems to be equivalent to $(uname -n)\n\nGlibc 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.\n\n"},{"id":"490837","messageId":"20240318081722.GA602575@coredump.intra.peff.net","threadId":"61072","inReplyTo":"xmqqo7bem7o8.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] config: learn the \"hostname:\" includeIf condition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-18T08:17:22Z","receivedAt":"2024-03-18T08:17:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 16, 2024 at 10:02:31AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Do we need to define \"hostname\" in more detail here? Specifically, I'm\n> > wondering whether the result will be a FQDN or not (i.e., the output of\n> > \"hostname\" vs \"hostname -f\"). Looking at the code I think it will just\n> > be the short name returned. That's probably OK, but it may be worth\n> > documenting.\n> \n> That was my first reaction but there are places where \"hostname\"\n> already gives a name that is not \"short\" at all, without being\n> invoked with \"-f\".\n\nThanks, that was the vague buzzing in the back of my head that led to\nmy first comment. It has been a while since I've dealt with this, but I\nthink in some circles it is a holy war akin to tabs vs spaces. A quick\nsearch shows I am not alone:\n\n https://serverfault.com/questions/331936/setting-the-hostname-fqdn-or-short-name\n\nSo I think we probably need to say something like:\n\n Depending on how your system is configured, the hostname used for\n matching may be short (e.g., \"myhost\") or a fully qualified domain\n name (\"myhost.example.com\").\n\n> I think the most honest answer we can give in the documentation is\n> that we use what gethostname() [*] gives.\n\nThat is honest, but I wonder if it is very useful to most users, as they\ncannot easily see what it returns. It's tempting to give an extra note\nlike this tacked on to what I said above:\n\n You can run the hostname(1) tool to see which hostname your system\n uses.\n\nBut I'm not sure that it is available everywhere (especially Windows).\nI guess we could provide \"git config --show-hostname-for-includes\" or\nsomething, but that feels like overkill.\n\nMaybe just the \"Depending...\" note is enough, and people who are\ninterested in hostname conditionals hopefully know enough to dig further\non their system. What I think we want to avoid is saying nothing, and\nthen somebody tries \"foo.example.com\", finds that it doesn't work, and\ngets confused with no hints about why.\n\nI guess yet another alternative is to try to qualify the name ourselves\nusing getaddrinfo(), either unconditionally or if the hostname doesn't\ncontain a \".\". That may involve a DNS lookup, though (if your hostname\nisn't in /etc/hosts).\n\n-Peff\n"},{"id":"490945","messageId":"20240319183722.211300-1-ignacio@iencinas.com","threadId":"61072","inReplyTo":"20240309181828.45496-1-ignacio@iencinas.com","subject":"[PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-19T18:37:20Z","receivedAt":"2024-03-19T18:38:09Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Extend includeIf to take hostname into account. Motivating request can\nbe found here [1].\n\n[1] https://github.com/gitgitgadget/git/issues/1665\n\nA lot of feedback was given to the v2 of this patch.\n\nIt was pointed out that it wasn't particularly obvious what it was meant by \n\n \"If the current hostname matches the pattern, the include condition is met.\"\n\nwhich is definitely true. Despite this, to my knowledge, there isn't a\nway to precisely define what we mean by \"hostname\" other than saying \nthat we mean whatever is returned by gethostname(2). \n\nIn my opinion, terms like \"short hostname\" can be confusing in some \ncases, and I'm not even sure we can rely on $hostname to agree with \ngethostname(2) in every platform.\n\nI still think the documentation isn't great, but I don't see a way to\nimprove it further.\n\nThanks everyone for the feedback!\n\nChanges since v2:\n* Expose the result of xgethostname through test-tool\n* Rewrite test to rely on test-tool xgethostname rather than using\n $hostname\n* Clarify documentation, specifying that by \"hostname\" we mean output of\n gethostname(2)\n\nChanges since v1:\n* Add blank line between declarations and code in `include_by_branch`.\n* Rewrite \"echo\"s used in tests to make them more readable. \n\nIgnacio Encinas (2):\n t: add a test helper for getting hostname\n config: learn the \"hostname:\" includeIf condition\n\n Documentation/config.txt | 10 +++++++++\n Makefile | 1 +\n config.c | 17 +++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/helper/test-xgethostname.c | 12 +++++++++++\n t/t1305-config-include.sh | 42 ++++++++++++++++++++++++++++++++++++\n t/t6500-gc.sh | 3 +--\n 8 files changed, 85 insertions(+), 2 deletions(-)\n create mode 100644 t/helper/test-xgethostname.c\n\nRange-diff against v2:\n-: ------------ > 1: ee1f9b1da037 t: add a test helper for getting hostname\n1: cf175154109e ! 2: dec622c38916 config: learn the \"hostname:\" includeIf condition\n @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards\n +`hostname`::\n +\tThe data that follows the keyword `hostname:` is taken to be a\n +\tpattern with standard globbing wildcards. If the current\n -+\thostname matches the pattern, the include condition is met.\n ++\thostname (output of gethostname(2)) matches the\n ++\tpattern, the include condition is met.\n +\n A few more notes on matching via `gitdir` and `gitdir/i`:\n \n @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '\n \n +test_expect_success 'conditional include, hostname' '\n +\tcat >>.git/config <<-EOF &&\n -+\t[includeIf \"hostname:$(hostname)a\"]\n ++\t[includeIf \"hostname:$(test-tool xgethostname)a\"]\n +\t\tpath = bar12\n +\tEOF\n +\tcat >>.git/bar12 <<-EOF &&\n @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '\n +\ttest_must_fail git config test.twelve &&\n +\n +\tcat >>.git/config <<-EOF &&\n -+\t[includeIf \"hostname:$(hostname)\"]\n ++\t[includeIf \"hostname:$(test-tool xgethostname)\"]\n +\t\tpath = bar12\n +\tEOF\n +\techo 12 >expect &&\n @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '\n +\n +test_expect_success 'conditional include, hostname, wildcard' '\n +\tcat >>.git/config <<-EOF &&\n -+\t[includeIf \"hostname:$(hostname)a*\"]\n ++\t[includeIf \"hostname:$(test-tool xgethostname)a*\"]\n +\t\tpath = bar13\n +\tEOF\n +\tcat >>.git/bar13 <<-EOF &&\n @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '\n +\ttest_must_fail git config test.thirteen &&\n +\n +\tcat >>.git/config <<-EOF &&\n -+\t[includeIf \"hostname:$(hostname)*\"]\n ++\t[includeIf \"hostname:$(test-tool xgethostname)*\"]\n +\t\tpath = bar13\n +\tEOF\n +\techo 13 >expect &&\n\nbase-commit: e09f1254c54329773904fe25d7c545a1fb4fa920\n-- \n2.44.0\n\n"},{"id":"490947","messageId":"20240319183722.211300-2-ignacio@iencinas.com","threadId":"61072","inReplyTo":"20240319183722.211300-1-ignacio@iencinas.com","subject":"[PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-19T18:37:21Z","receivedAt":"2024-03-19T18:38:13Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Avoid relying on whether the system running the test has \"hostname\"\ninstalled or not, and expose the output of xgethostname through\ntest-tool.\n\nSigned-off-by: Ignacio Encinas \n---\n Makefile | 1 +\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/helper/test-xgethostname.c | 12 ++++++++++++\n t/t6500-gc.sh | 3 +--\n 5 files changed, 16 insertions(+), 2 deletions(-)\n create mode 100644 t/helper/test-xgethostname.c\n\ndiff --git a/Makefile b/Makefile\nindex 4e255c81f223..561d7a1fa268 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -863,6 +863,7 @@ TEST_BUILTINS_OBJS += test-userdiff.o\n TEST_BUILTINS_OBJS += test-wildmatch.o\n TEST_BUILTINS_OBJS += test-windows-named-pipe.o\n TEST_BUILTINS_OBJS += test-write-cache.o\n+TEST_BUILTINS_OBJS += test-xgethostname.o\n TEST_BUILTINS_OBJS += test-xml-encode.o\n \n # Do not add more tests here unless they have extra dependencies. Add\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 482a1e58a4b6..9318900a2981 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -86,6 +86,7 @@ static struct test_cmd cmds[] = {\n \t{ \"truncate\", cmd__truncate },\n \t{ \"userdiff\", cmd__userdiff },\n \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n+\t{ \"xgethostname\", cmd__xgethostname },\n \t{ \"xml-encode\", cmd__xml_encode },\n \t{ \"wildmatch\", cmd__wildmatch },\n #ifdef GIT_WINDOWS_NATIVE\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex b1be7cfcf593..075d34bd3c0a 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -79,6 +79,7 @@ int cmd__trace2(int argc, const char **argv);\n int cmd__truncate(int argc, const char **argv);\n int cmd__userdiff(int argc, const char **argv);\n int cmd__urlmatch_normalization(int argc, const char **argv);\n+int cmd__xgethostname(int argc, const char **argv);\n int cmd__xml_encode(int argc, const char **argv);\n int cmd__wildmatch(int argc, const char **argv);\n #ifdef GIT_WINDOWS_NATIVE\ndiff --git a/t/helper/test-xgethostname.c b/t/helper/test-xgethostname.c\nnew file mode 100644\nindex 000000000000..285746aef54a\n--- /dev/null\n+++ b/t/helper/test-xgethostname.c\n@@ -0,0 +1,12 @@\n+#include \"test-tool.h\"\n+\n+int cmd__xgethostname(int argc, const char **argv)\n+{\n+\tchar hostname[HOST_NAME_MAX + 1];\n+\n+\tif (xgethostname(hostname, sizeof(hostname)))\n+\t\tdie(\"unable to get the host name\");\n+\n+\tputs(hostname);\n+\treturn 0;\n+}\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 18fe1c25e6a0..613c766e2bb4 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '\n \n \t# now fake a concurrent gc that holds the lock; we can use our\n \t# shell pid so that it looks valid.\n-\thostname=$(hostname || echo unknown) &&\n \tshell_pid=$$ &&\n \tif test_have_prereq MINGW && test -f /proc/$shell_pid/winpid\n \tthen\n@@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '\n \t\t# the Windows PID in this case.\n \t\tshell_pid=$(cat /proc/$shell_pid/winpid)\n \tfi &&\n-\tprintf \"%d %s\" \"$shell_pid\" \"$hostname\" >.git/gc.pid &&\n+\tprintf \"%d %s\" \"$shell_pid\" \"$(test-tool xgethostname)\" >.git/gc.pid &&\n \n \t# our gc should exit zero without doing anything\n \trun_and_wait_for_auto_gc &&\n-- \n2.44.0\n\n"},{"id":"490946","messageId":"20240319183722.211300-3-ignacio@iencinas.com","threadId":"61072","inReplyTo":"20240319183722.211300-1-ignacio@iencinas.com","subject":"[PATCH v3 2/2] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-19T18:37:22Z","receivedAt":"2024-03-19T18:38:14Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"Currently, customizing the configuration depending on the machine running\ngit has to be done manually.\n\nAdd support for a new includeIf keyword \"hostname:\" to conditionally\ninclude configuration files depending on the hostname.\n\nSigned-off-by: Ignacio Encinas \n---\n Documentation/config.txt | 10 ++++++++++\n config.c | 17 ++++++++++++++++\n t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 69 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e3a74dd1c19d..268a9fab7be0 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -186,6 +186,12 @@ As for the naming of this keyword, it is for forwards compatibility with\n a naming scheme that supports more variable-based include conditions,\n but currently Git only supports the exact keyword described above.\n \n+`hostname`::\n+\tThe data that follows the keyword `hostname:` is taken to be a\n+\tpattern with standard globbing wildcards. If the current\n+\thostname (output of gethostname(2)) matches the\n+\tpattern, the include condition is met.\n+\n A few more notes on matching via `gitdir` and `gitdir/i`:\n \n * Symlinks in `$GIT_DIR` are not resolved before matching.\n@@ -261,6 +267,10 @@ Example\n \tpath = foo.inc\n [remote \"origin\"]\n \turl = https://example.com/git\n+\n+; include only if the hostname of the machine matches some-hostname\n+[includeIf \"hostname:some-hostname\"]\n+\tpath = foo.inc\n ----\n \n Values\ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd99..50b3f6d24c50 100644\n--- a/config.c\n+++ b/config.c\n@@ -317,6 +317,21 @@ static int include_by_branch(const char *cond, size_t cond_len)\n \treturn ret;\n }\n \n+static int include_by_hostname(const char *cond, size_t cond_len)\n+{\n+\tint ret;\n+\tchar my_host[HOST_NAME_MAX + 1];\n+\tstruct strbuf pattern = STRBUF_INIT;\n+\n+\tif (xgethostname(my_host, sizeof(my_host)))\n+\t\treturn 0;\n+\n+\tstrbuf_add(&pattern, cond, cond_len);\n+\tret = !wildmatch(pattern.buf, my_host, 0);\n+\tstrbuf_release(&pattern);\n+\treturn ret;\n+}\n+\n static int add_remote_url(const char *var, const char *value,\n \t\t\t const struct config_context *ctx UNUSED, void *data)\n {\n@@ -406,6 +421,8 @@ static int include_condition_is_true(const struct key_value_info *kvi,\n \telse if (skip_prefix_mem(cond, cond_len, \"hasconfig:remote.*.url:\", &cond,\n \t\t\t\t &cond_len))\n \t\treturn include_by_remote_url(inc, cond, cond_len);\n+\telse if (skip_prefix_mem(cond, cond_len, \"hostname:\", &cond, &cond_len))\n+\t\treturn include_by_hostname(cond, cond_len);\n \n \t/* unknown conditionals are always false */\n \treturn 0;\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex 5cde79ef8c4f..ef9272fd8e53 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -357,4 +357,46 @@ test_expect_success 'include cycles are detected' '\n \tgrep \"exceeded maximum include depth\" stderr\n '\n \n+test_expect_success 'conditional include, hostname' '\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(test-tool xgethostname)a\"]\n+\t\tpath = bar12\n+\tEOF\n+\tcat >>.git/bar12 <<-EOF &&\n+\t[test]\n+\t\ttwelve=12\n+\tEOF\n+\n+\ttest_must_fail git config test.twelve &&\n+\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(test-tool xgethostname)\"]\n+\t\tpath = bar12\n+\tEOF\n+\techo 12 >expect &&\n+\tgit config test.twelve >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'conditional include, hostname, wildcard' '\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(test-tool xgethostname)a*\"]\n+\t\tpath = bar13\n+\tEOF\n+\tcat >>.git/bar13 <<-EOF &&\n+\t[test]\n+\t\tthirteen = 13\n+\tEOF\n+\n+\ttest_must_fail git config test.thirteen &&\n+\n+\tcat >>.git/config <<-EOF &&\n+\t[includeIf \"hostname:$(test-tool xgethostname)*\"]\n+\t\tpath = bar13\n+\tEOF\n+\techo 13 >expect &&\n+\tgit config test.thirteen >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"490953","messageId":"xmqq8r2eneut.fsf@gitster.g","threadId":"61072","inReplyTo":"20240319183722.211300-2-ignacio@iencinas.com","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T20:31:06Z","receivedAt":"2024-03-19T20:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Encinas writes:\n\n> diff --git a/Makefile b/Makefile\n> index 4e255c81f223..561d7a1fa268 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -863,6 +863,7 @@ TEST_BUILTINS_OBJS += test-userdiff.o\n> TEST_BUILTINS_OBJS += test-wildmatch.o\n> TEST_BUILTINS_OBJS += test-windows-named-pipe.o\n> TEST_BUILTINS_OBJS += test-write-cache.o\n> +TEST_BUILTINS_OBJS += test-xgethostname.o\n> TEST_BUILTINS_OBJS += test-xml-encode.o\n> \n> # Do not add more tests here unless they have extra dependencies. Add\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 482a1e58a4b6..9318900a2981 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -86,6 +86,7 @@ static struct test_cmd cmds[] = {\n> \t{ \"truncate\", cmd__truncate },\n> \t{ \"userdiff\", cmd__userdiff },\n> \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n> +\t{ \"xgethostname\", cmd__xgethostname },\n> \t{ \"xml-encode\", cmd__xml_encode },\n> \t{ \"wildmatch\", cmd__wildmatch },\n> #ifdef GIT_WINDOWS_NATIVE\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index b1be7cfcf593..075d34bd3c0a 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -79,6 +79,7 @@ int cmd__trace2(int argc, const char **argv);\n> int cmd__truncate(int argc, const char **argv);\n> int cmd__userdiff(int argc, const char **argv);\n> int cmd__urlmatch_normalization(int argc, const char **argv);\n> +int cmd__xgethostname(int argc, const char **argv);\n> int cmd__xml_encode(int argc, const char **argv);\n> int cmd__wildmatch(int argc, const char **argv);\n> #ifdef GIT_WINDOWS_NATIVE\n> diff --git a/t/helper/test-xgethostname.c b/t/helper/test-xgethostname.c\n> new file mode 100644\n> index 000000000000..285746aef54a\n> --- /dev/null\n> +++ b/t/helper/test-xgethostname.c\n> @@ -0,0 +1,12 @@\n> +#include \"test-tool.h\"\n> +\n> +int cmd__xgethostname(int argc, const char **argv)\n> +{\n> +\tchar hostname[HOST_NAME_MAX + 1];\n> +\n> +\tif (xgethostname(hostname, sizeof(hostname)))\n> +\t\tdie(\"unable to get the host name\");\n> +\n> +\tputs(hostname);\n> +\treturn 0;\n> +}\n\n\nOK. Given that xgethostname() is meant as a safe (and improved)\nreplacement for the underlying gethostname(), I would have used\ngethostname() as the name for the above, but that alone is not big\nenough reason for another update.\n\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 18fe1c25e6a0..613c766e2bb4 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '\n> \n> \t# now fake a concurrent gc that holds the lock; we can use our\n> \t# shell pid so that it looks valid.\n> -\thostname=$(hostname || echo unknown) &&\n> \tshell_pid=$$ &&\n> \tif test_have_prereq MINGW && test -f /proc/$shell_pid/winpid\n> \tthen\n> @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '\n> \t\t# the Windows PID in this case.\n> \t\tshell_pid=$(cat /proc/$shell_pid/winpid)\n> \tfi &&\n> -\tprintf \"%d %s\" \"$shell_pid\" \"$hostname\" >.git/gc.pid &&\n> +\tprintf \"%d %s\" \"$shell_pid\" \"$(test-tool xgethostname)\" >.git/gc.pid &&\n\nWe should replace the \"hostname || echo unknown\" in the original,\ninstead of doing this change, as it loses the exit status from the\n\"test-tool xgethostname\" command.\n\nThanks.\n\n> \t# our gc should exit zero without doing anything\n> \trun_and_wait_for_auto_gc &&\n"},{"id":"490956","messageId":"xmqqsf0mlz8d.fsf@gitster.g","threadId":"61072","inReplyTo":"20240319183722.211300-3-ignacio@iencinas.com","subject":"Re: [PATCH v3 2/2] config: learn the \"hostname:\" includeIf condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T20:53:54Z","receivedAt":"2024-03-19T20:53:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Encinas writes:\n\n> Currently, customizing the configuration depending on the machine running\n> git has to be done manually.\n\nDrop \"currently\" (cf. https://lore.kernel.org/git/xmqqle6xbep5.fsf@gitster.g/)\n\nIt does not actually have to be done manually. I and many others\nhave ~/src/home/dot/ directory where ~/src/home/dot/Makefile uses\ninformation available in the environment (like output from the\n`hostname` command), produces the .gitconfig file out of a template,\nand the build procedure can even install with \"make install\" the\nresulting file to \"~/.gitconfig\". Together with other configuration\nfiles that are kept track of in the ~/src/home/ repository, it is\nmanaged wihtout much manual labor.\n\nAnother reason why \"[includeif hostname:]\" may be useful is\nwhen the same home directory is shared across multiple machines. \nAs ~/.gitconfig is shared, if you need to have different settings\ndepending on the host, you would need to have something that a\nsingle file ~/.gitconfig is read in different ways on these hosts.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index e3a74dd1c19d..268a9fab7be0 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -186,6 +186,12 @@ As for the naming of this keyword, it is for forwards compatibility with\n> a naming scheme that supports more variable-based include conditions,\n> but currently Git only supports the exact keyword described above.\n> \n> +`hostname`::\n> +\tThe data that follows the keyword `hostname:` is taken to be a\n> +\tpattern with standard globbing wildcards. If the current\n> +\thostname (output of gethostname(2)) matches the\n> +\tpattern, the include condition is met.\n> +\n\nI do not think of a better way to phrase to explain what `hostname`\nmeans in this context than the above, either. This should be good\nenough, hopefully ;-).\n\nThe entry above this one is really an oddball (it only depends on\nwhat other repositories the current repository interacts with, and\ndoes not care about host, directory, or anything of the sort); we\nmay want to move it either before the `onbranch` entry.\n\n> diff --git a/config.c b/config.c\n> index 3cfeb3d8bd99..50b3f6d24c50 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -317,6 +317,21 @@ static int include_by_branch(const char *cond, size_t cond_len)\n> \treturn ret;\n> }\n> \n> +static int include_by_hostname(const char *cond, size_t cond_len)\n> +{\n> +\tint ret;\n> +\tchar my_host[HOST_NAME_MAX + 1];\n> +\tstruct strbuf pattern = STRBUF_INIT;\n> +\n> +\tif (xgethostname(my_host, sizeof(my_host)))\n> +\t\treturn 0;\n> +\n> +\tstrbuf_add(&pattern, cond, cond_len);\n> +\tret = !wildmatch(pattern.buf, my_host, 0);\n> +\tstrbuf_release(&pattern);\n> +\treturn ret;\n> +}\n\nOK. Just as other conditions, it is a bit annoying that we need to\nmake a copy of cond string only to NUL terminate it, because\nwildmatch() does not take a counted string as its input, but the\nabove code looks good.\n\n> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n> index 5cde79ef8c4f..ef9272fd8e53 100755\n> --- a/t/t1305-config-include.sh\n> +++ b/t/t1305-config-include.sh\n> @@ -357,4 +357,46 @@ test_expect_success 'include cycles are detected' '\n> \tgrep \"exceeded maximum include depth\" stderr\n> '\n> \n> +test_expect_success 'conditional include, hostname' '\n> +\tcat >>.git/config <<-EOF &&\n> +\t[includeIf \"hostname:$(test-tool xgethostname)a\"]\n> +\t\tpath = bar12\n> +\tEOF\n\nExactly the same comment about lost exit status from test-tool\napplies here, too.\n\n> +\tcat >>.git/bar12 <<-EOF &&\n> +\t[test]\n> +\t\ttwelve=12\n> +\tEOF\n> +\n> +\ttest_must_fail git config test.twelve &&\n> +\n> +\tcat >>.git/config <<-EOF &&\n> +\t[includeIf \"hostname:$(test-tool xgethostname)\"]\n> +\t\tpath = bar12\n> +\tEOF\n> +\techo 12 >expect &&\n> +\tgit config test.twelve >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'conditional include, hostname, wildcard' '\n> +\tcat >>.git/config <<-EOF &&\n> +\t[includeIf \"hostname:$(test-tool xgethostname)a*\"]\n\nHmph, a* is not even \"one-or-more a\" but \"a followed by anything\",\nso this will not match, OK.\n\n> +\t\tpath = bar13\n> +\tEOF\n> +\tcat >>.git/bar13 <<-EOF &&\n> +\t[test]\n> +\t\tthirteen = 13\n> +\tEOF\n> +\n> +\ttest_must_fail git config test.thirteen &&\n> +\n> +\tcat >>.git/config <<-EOF &&\n> +\t[includeIf \"hostname:$(test-tool xgethostname)*\"]\n\nAnd this is \"exactly what gethostname gives, followed by anything\n(including nothing)\", so gethostname output should match. OK.\n\n> +\t\tpath = bar13\n> +\tEOF\n> +\techo 13 >expect &&\n> +\tgit config test.thirteen >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"490957","messageId":"CAPig+cT4fpX7Kczu0+H5TZnmpVqqq0h8nBafj4UqDs7Xv2Nf4A@mail.gmail.com","threadId":"61072","inReplyTo":"20240319183722.211300-1-ignacio@iencinas.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-19T20:55:12Z","receivedAt":"2024-03-19T20:55:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas wrote:\n> It was pointed out that it wasn't particularly obvious what it was meant by\n>\n> \"If the current hostname matches the pattern, the include condition is met.\"\n>\n> which is definitely true. Despite this, to my knowledge, there isn't a\n> way to precisely define what we mean by \"hostname\" other than saying\n> that we mean whatever is returned by gethostname(2).\n>\n> I still think the documentation isn't great, but I don't see a way to\n> improve it further.\n\nPeff provided the answer when he suggested[1] implementing `git config\n--show-hostname-for-includes`.\n\n[1]: https://lore.kernel.org/git/20240318081722.GA602575@coredump.intra.peff.net/\n\n> 1: cf175154109e ! 2: dec622c38916 config: learn the \"hostname:\" includeIf condition\n> @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards\n> +`hostname`::\n> + The data that follows the keyword `hostname:` is taken to be a\n> + pattern with standard globbing wildcards. If the current\n> -+ hostname matches the pattern, the include condition is met.\n> ++ hostname (output of gethostname(2)) matches the\n> ++ pattern, the include condition is met.\n\nThis is still unnecessarily user-hostile, especially to users who are\nnot programmers, but also to programmers who don't want to waste time\nwriting a little test program to determine what gethostname(2) returns\non each platform they use. That's not a great situation.\n\nPeff felt that adding `git config --show-hostname-for-includes` was\nprobably overkill, but I'd argue that it is necessary to enable users\nto deterministically figure out the value to use in their\nconfiguration rather than having to grope around in the dark via\nguesswork and trial-and-error to figure out exactly what works.\n\nAnd the option name doesn't necessarily have to be so verbose; a\nshorter name, such as `git config --show-hostname` may be good enough.\nImplementing this option would also obviate the need to implement\n`test-tool xgethostname` (though, I agree with Junio that `test-tool\ngethostname` would have been a better, less implementation-revealing\nname).\n"},{"id":"490958","messageId":"20240319205630.GA1159535@coredump.intra.peff.net","threadId":"61072","inReplyTo":"20240319183722.211300-2-ignacio@iencinas.com","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-19T20:56:30Z","receivedAt":"2024-03-19T20:56:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 07:37:21PM +0100, Ignacio Encinas wrote:\n\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 18fe1c25e6a0..613c766e2bb4 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '\n> \n> \t# now fake a concurrent gc that holds the lock; we can use our\n> \t# shell pid so that it looks valid.\n> -\thostname=$(hostname || echo unknown) &&\n> \tshell_pid=$$ &&\n> \tif test_have_prereq MINGW && test -f /proc/$shell_pid/winpid\n> \tthen\n> @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '\n> \t\t# the Windows PID in this case.\n> \t\tshell_pid=$(cat /proc/$shell_pid/winpid)\n> \tfi &&\n> -\tprintf \"%d %s\" \"$shell_pid\" \"$hostname\" >.git/gc.pid &&\n> +\tprintf \"%d %s\" \"$shell_pid\" \"$(test-tool xgethostname)\" >.git/gc.pid &&\n\nHmm. Before, we were compensating for a failure to run \"hostname\" by\nputting in the string \"unknown\". But I wonder if there were platforms\nwhere gethostname() simply fails (e.g., because the hostname is too\nlong). In that case your test-tool returns the empty string, but git-gc\ninternally will use the string \"unknown\".\n\nI think it may be OK, though. The failing exit code from test-tool will\nbe ignored, and we'll end up with a file containing \"123 \" or similar.\nNormally we'd use kill() to check that the pid is valid, but with a\nmis-matched hostname we'll just assume the process is still around, and\nthe outcome is the same.\n\n-Peff\n"},{"id":"490959","messageId":"20240319205753.GB1159535@coredump.intra.peff.net","threadId":"61072","inReplyTo":"xmqq8r2eneut.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-19T20:57:53Z","receivedAt":"2024-03-19T20:57:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 01:31:06PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> > index 18fe1c25e6a0..613c766e2bb4 100755\n> > --- a/t/t6500-gc.sh\n> > +++ b/t/t6500-gc.sh\n> > @@ -395,7 +395,6 @@ test_expect_success 'background auto gc respects lock for all operations' '\n> > \n> > \t# now fake a concurrent gc that holds the lock; we can use our\n> > \t# shell pid so that it looks valid.\n> > -\thostname=$(hostname || echo unknown) &&\n> > \tshell_pid=$$ &&\n> > \tif test_have_prereq MINGW && test -f /proc/$shell_pid/winpid\n> > \tthen\n> > @@ -404,7 +403,7 @@ test_expect_success 'background auto gc respects lock for all operations' '\n> > \t\t# the Windows PID in this case.\n> > \t\tshell_pid=$(cat /proc/$shell_pid/winpid)\n> > \tfi &&\n> > -\tprintf \"%d %s\" \"$shell_pid\" \"$hostname\" >.git/gc.pid &&\n> > +\tprintf \"%d %s\" \"$shell_pid\" \"$(test-tool xgethostname)\" >.git/gc.pid &&\n> \n> We should replace the \"hostname || echo unknown\" in the original,\n> instead of doing this change, as it loses the exit status from the\n> \"test-tool xgethostname\" command.\n\nI think you need to lose the exit status. Or alternatively do:\n\n hostname=$(test-tool xgethostname || echo unknown)\n\nSee my other reply.\n\n-Peff\n"},{"id":"490960","messageId":"xmqqo7balyx9.fsf@gitster.g","threadId":"61072","inReplyTo":"20240319205753.GB1159535@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T21:00:34Z","receivedAt":"2024-03-19T21:00:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think you need to lose the exit status. Or alternatively do:\n>\n> hostname=$(test-tool xgethostname || echo unknown)\n>\n> See my other reply.\n\nAs \"test-tool xgethostname\" runs exactly the same codepath as\n\"includeIf hostname:blah\" feature, I would actually prefer for a\nfailing \"test-tool gethostname\" to _break_ this test so that people\ncan take notice.\n\n\n"},{"id":"490962","messageId":"20240319210428.GC1159535@coredump.intra.peff.net","threadId":"61072","inReplyTo":"20240319183722.211300-3-ignacio@iencinas.com","subject":"Re: [PATCH v3 2/2] config: learn the \"hostname:\" includeIf condition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-19T21:04:28Z","receivedAt":"2024-03-19T21:04:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 07:37:22PM +0100, Ignacio Encinas wrote:\n\n> +`hostname`::\n> +\tThe data that follows the keyword `hostname:` is taken to be a\n> +\tpattern with standard globbing wildcards. If the current\n> +\thostname (output of gethostname(2)) matches the\n> +\tpattern, the include condition is met.\n\nI was going to comment further here, but I see Eric already replied with\neverything I was going to say. ;)\n\nOne small comment on the patch...\n\n> +static int include_by_hostname(const char *cond, size_t cond_len)\n> +{\n> +\tint ret;\n> +\tchar my_host[HOST_NAME_MAX + 1];\n> +\tstruct strbuf pattern = STRBUF_INIT;\n> +\n> +\tif (xgethostname(my_host, sizeof(my_host)))\n> +\t\treturn 0;\n> +\n> +\tstrbuf_add(&pattern, cond, cond_len);\n> +\tret = !wildmatch(pattern.buf, my_host, 0);\n> +\tstrbuf_release(&pattern);\n> +\treturn ret;\n> +}\n\nThis is absolutely a nit, but I think using xmemdupz() like:\n\n char *pattern;\n ...\n\n pattern = xmemdupz(cond, cond_len);\n ...\n free(pattern);\n\nexpresses the intent more directly (it's also a little more efficient,\nbut that's probably not measurable).\n\n-Peff\n"},{"id":"490963","messageId":"20240319211112.GD1159535@coredump.intra.peff.net","threadId":"61072","inReplyTo":"xmqqo7balyx9.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-19T21:11:12Z","receivedAt":"2024-03-19T21:11:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 02:00:34PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think you need to lose the exit status. Or alternatively do:\n> >\n> > hostname=$(test-tool xgethostname || echo unknown)\n> >\n> > See my other reply.\n> \n> As \"test-tool xgethostname\" runs exactly the same codepath as\n> \"includeIf hostname:blah\" feature, I would actually prefer for a\n> failing \"test-tool gethostname\" to _break_ this test so that people\n> can take notice.\n\nBut we are not testing \"includeIf\" in this patch; we are testing git-gc,\nwhich falls back to the string \"unknown\". The includeIf tests in patch 2\nwill naturally fail if either xgethostname() fails (because then we will\nrefuse to do any host matching) or if it works but somehow test-tool is\nbroken (because then it won't match what the config code sees\ninternally).\n\n-Peff\n"},{"id":"490964","messageId":"xmqqa5mulycz.fsf@gitster.g","threadId":"61072","inReplyTo":"CAPig+cT4fpX7Kczu0+H5TZnmpVqqq0h8nBafj4UqDs7Xv2Nf4A@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T21:12:44Z","receivedAt":"2024-03-19T21:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Peff felt that adding `git config --show-hostname-for-includes` was\n> probably overkill, but I'd argue that it is necessary to enable users\n> to deterministically figure out the value to use in their\n> configuration rather than having to grope around in the dark via\n> guesswork and trial-and-error to figure out exactly what works.\n>\n> And the option name doesn't necessarily have to be so verbose; a\n> shorter name, such as `git config --show-hostname` may be good enough.\n> Implementing this option would also obviate the need to implement\n> `test-tool xgethostname` (though, I agree with Junio that `test-tool\n> gethostname` would have been a better, less implementation-revealing\n> name).\n\nYeah, I like that show-hostname thing (which I do not know if \"config\"\nis a good home for, though).\n"},{"id":"490965","messageId":"CAPig+cTFRAmzBGiJv2F-k1XWvGSbT8UeAG57T+XpB-1w66HRkQ@mail.gmail.com","threadId":"61072","inReplyTo":"xmqqa5mulycz.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-19T21:13:47Z","receivedAt":"2024-03-19T21:13:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > Peff felt that adding `git config --show-hostname-for-includes` was\n> > probably overkill, but I'd argue that it is necessary to enable users\n> > to deterministically figure out the value to use in their\n> > configuration rather than having to grope around in the dark via\n> > guesswork and trial-and-error to figure out exactly what works.\n> >\n> > And the option name doesn't necessarily have to be so verbose; a\n> > shorter name, such as `git config --show-hostname` may be good enough.\n> > Implementing this option would also obviate the need to implement\n> > `test-tool xgethostname` (though, I agree with Junio that `test-tool\n> > gethostname` would have been a better, less implementation-revealing\n> > name).\n>\n> Yeah, I like that show-hostname thing (which I do not know if \"config\"\n> is a good home for, though).\n\nThe other possibility which came to mind was adding a GIT_HOSTNAME\nvariable to the output of `git var -l`.\n"},{"id":"490966","messageId":"29901b21-83bd-4746-b08a-c428eed27604@iencinas.com","threadId":"61072","inReplyTo":"CAPig+cT4fpX7Kczu0+H5TZnmpVqqq0h8nBafj4UqDs7Xv2Nf4A@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-19T21:22:54Z","receivedAt":"2024-03-19T21:23:00Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 19/3/24 21:55, Eric Sunshine wrote:\n> On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas wrote:\n>> It was pointed out that it wasn't particularly obvious what it was meant by\n>>\n>> \"If the current hostname matches the pattern, the include condition is met.\"\n>>\n>> which is definitely true. Despite this, to my knowledge, there isn't a\n>> way to precisely define what we mean by \"hostname\" other than saying\n>> that we mean whatever is returned by gethostname(2).\n>>\n>> I still think the documentation isn't great, but I don't see a way to\n>> improve it further.\n> \n> Peff provided the answer when he suggested[1] implementing `git config\n> --show-hostname-for-includes`.\n> \n> [1]: https://lore.kernel.org/git/20240318081722.GA602575@coredump.intra.peff.net/\n \nSorry if it sounded like I disregarded the opinion. I did see it and\nliked the idea, but I guessed something like that would face a lot of\nresistance. My bad.\n\n>> 1: cf175154109e ! 2: dec622c38916 config: learn the \"hostname:\" includeIf condition\n>> @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards\n>> +`hostname`::\n>> + The data that follows the keyword `hostname:` is taken to be a\n>> + pattern with standard globbing wildcards. If the current\n>> -+ hostname matches the pattern, the include condition is met.\n>> ++ hostname (output of gethostname(2)) matches the\n>> ++ pattern, the include condition is met.\n> \n> This is still unnecessarily user-hostile, especially to users who are\n> not programmers, but also to programmers who don't want to waste time\n> writing a little test program to determine what gethostname(2) returns\n> on each platform they use. That's not a great situation.\n> \n> Peff felt that adding `git config --show-hostname-for-includes` was\n> probably overkill, but I'd argue that it is necessary to enable users\n> to deterministically figure out the value to use in their\n> configuration rather than having to grope around in the dark via\n> guesswork and trial-and-error to figure out exactly what works.\n> \n> And the option name doesn't necessarily have to be so verbose; a\n> shorter name, such as `git config --show-hostname` may be good enough.\n> Implementing this option would also obviate the need to implement\n> `test-tool xgethostname` (though, I agree with Junio that `test-tool\n> gethostname` would have been a better, less implementation-revealing\n> name).\n\nLets find this a good \"home\" then [1]. Thanks!\n\n[1] https://lore.kernel.org/git/CAPig+cTFRAmzBGiJv2F-k1XWvGSbT8UeAG57T+XpB-1w66HRkQ@mail.gmail.com/\n"},{"id":"490968","messageId":"cf3f27f9-c3c5-465e-bd49-767a1acabf0d@iencinas.com","threadId":"61072","inReplyTo":"20240319210428.GC1159535@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] config: learn the \"hostname:\" includeIf condition","fromName":"Ignacio Encinas Rubio","fromEmail":"ignacio@iencinas.com","sentAt":"2024-03-19T21:32:56Z","receivedAt":"2024-03-19T21:33:01Z","isPatch":true,"sender":{"key":"ignacio@iencinas.com","avatar":null},"body":"\n\nOn 19/3/24 22:04, Jeff King wrote:\n> On Tue, Mar 19, 2024 at 07:37:22PM +0100, Ignacio Encinas wrote:\n> \n>> +`hostname`::\n>> +\tThe data that follows the keyword `hostname:` is taken to be a\n>> +\tpattern with standard globbing wildcards. If the current\n>> +\thostname (output of gethostname(2)) matches the\n>> +\tpattern, the include condition is met.\n> \n> I was going to comment further here, but I see Eric already replied with\n> everything I was going to say. ;)\n\nPlease see my reply there. Thanks for the suggestion and sorry again if \nI sounded rude!\n\n> One small comment on the patch...\n> \n>> +static int include_by_hostname(const char *cond, size_t cond_len)\n>> +{\n>> +\tint ret;\n>> +\tchar my_host[HOST_NAME_MAX + 1];\n>> +\tstruct strbuf pattern = STRBUF_INIT;\n>> +\n>> +\tif (xgethostname(my_host, sizeof(my_host)))\n>> +\t\treturn 0;\n>> +\n>> +\tstrbuf_add(&pattern, cond, cond_len);\n>> +\tret = !wildmatch(pattern.buf, my_host, 0);\n>> +\tstrbuf_release(&pattern);\n>> +\treturn ret;\n>> +}\n> \n> This is absolutely a nit, but I think using xmemdupz() like:\n> \n> char *pattern;\n> ...\n> \n> pattern = xmemdupz(cond, cond_len);\n> ...\n> free(pattern);\n> \n> expresses the intent more directly (it's also a little more efficient,\n> but that's probably not measurable).\n\nNoted, thanks!\n \n> -Peff\n"},{"id":"490969","messageId":"ghv85hj446.fsf@gouders.net","threadId":"61072","inReplyTo":"xmqqa5mulycz.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Dirk Gouders","fromEmail":"dirk@gouders.net","sentAt":"2024-03-19T21:36:41Z","receivedAt":"2024-03-19T21:37:10Z","isPatch":true,"sender":{"key":"dirk@gouders.net","avatar":"https://avatars.githubusercontent.com/u/81326422?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> Peff felt that adding `git config --show-hostname-for-includes` was\n>> probably overkill, but I'd argue that it is necessary to enable users\n>> to deterministically figure out the value to use in their\n>> configuration rather than having to grope around in the dark via\n>> guesswork and trial-and-error to figure out exactly what works.\n>>\n>> And the option name doesn't necessarily have to be so verbose; a\n>> shorter name, such as `git config --show-hostname` may be good enough.\n>> Implementing this option would also obviate the need to implement\n>> `test-tool xgethostname` (though, I agree with Junio that `test-tool\n>> gethostname` would have been a better, less implementation-revealing\n>> name).\n>\n> Yeah, I like that show-hostname thing (which I do not know if \"config\"\n> is a good home for, though).\n\nA thought when I was reading this: wouldn't it be enough to document\nthat `uname -n` can be used to get the hostname that should be used?\n\nAs far as I know this should be POSIX-compliant and uses gethostname(2).\n\nDirk\n"},{"id":"490971","messageId":"xmqq5xxhnbgg.fsf@gitster.g","threadId":"61072","inReplyTo":"20240319211112.GD1159535@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T21:44:31Z","receivedAt":"2024-03-19T21:44:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But we are not testing \"includeIf\" in this patch; we are testing git-gc,\n> which falls back to the string \"unknown\".\n\nAh, I wasn't aware of such a hardcoded default. Then replacing the\nexisting \"hostname\" with \"test-tool xgethostname\" and doing nothing\nelse is of course the right thing to do here.\n\nThanks.\n"},{"id":"490972","messageId":"02d601da7a49$3bbf1230$b33d3690$@nexbridge.com","threadId":"61072","inReplyTo":"ghv85hj446.fsf@gouders.net","subject":"RE: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-19T22:03:05Z","receivedAt":"2024-03-19T22:03:52Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:\n>Junio C Hamano writes:\n>\n>> Eric Sunshine writes:\n>>\n>>> Peff felt that adding `git config --show-hostname-for-includes` was\n>>> probably overkill, but I'd argue that it is necessary to enable users\n>>> to deterministically figure out the value to use in their\n>>> configuration rather than having to grope around in the dark via\n>>> guesswork and trial-and-error to figure out exactly what works.\n>>>\n>>> And the option name doesn't necessarily have to be so verbose; a\n>>> shorter name, such as `git config --show-hostname` may be good enough.\n>>> Implementing this option would also obviate the need to implement\n>>> `test-tool xgethostname` (though, I agree with Junio that `test-tool\n>>> gethostname` would have been a better, less implementation-revealing\n>>> name).\n>>\n>> Yeah, I like that show-hostname thing (which I do not know if \"config\"\n>> is a good home for, though).\n>\n>A thought when I was reading this: wouldn't it be enough to document that\n`uname -n` can be used to get the hostname that should\n>be used?\n>\n>As far as I know this should be POSIX-compliant and uses gethostname(2).\n\nAs previously pointed out, uname -n and gethostname(2) are not equivalent.\nuname -n does not (depending on implementation) go to DNS while\ngethostname(2) goes to DNS first (although apparently glibc may not). This\nis particularly important in a multi-home situation where more than one IP\nadapter has a different IP address on the same host, and where DNS does not\nconsider the different addresses to be equivalent (which otherwise could\ncause problems for reverse lookups).\n--Randall\n\n"},{"id":"490973","messageId":"ghr0g5j1sc.fsf@gouders.net","threadId":"61072","inReplyTo":"02d601da7a49$3bbf1230$b33d3690$@nexbridge.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Dirk Gouders","fromEmail":"dirk@gouders.net","sentAt":"2024-03-19T22:26:59Z","receivedAt":"2024-03-19T22:27:23Z","isPatch":true,"sender":{"key":"dirk@gouders.net","avatar":"https://avatars.githubusercontent.com/u/81326422?v=4"},"body":" writes:\n\n> On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:\n>>Junio C Hamano writes:\n>>\n>>> Eric Sunshine writes:\n>>>\n>>>> Peff felt that adding `git config --show-hostname-for-includes` was\n>>>> probably overkill, but I'd argue that it is necessary to enable users\n>>>> to deterministically figure out the value to use in their\n>>>> configuration rather than having to grope around in the dark via\n>>>> guesswork and trial-and-error to figure out exactly what works.\n>>>>\n>>>> And the option name doesn't necessarily have to be so verbose; a\n>>>> shorter name, such as `git config --show-hostname` may be good enough.\n>>>> Implementing this option would also obviate the need to implement\n>>>> `test-tool xgethostname` (though, I agree with Junio that `test-tool\n>>>> gethostname` would have been a better, less implementation-revealing\n>>>> name).\n>>>\n>>> Yeah, I like that show-hostname thing (which I do not know if \"config\"\n>>> is a good home for, though).\n>>\n>>A thought when I was reading this: wouldn't it be enough to document that\n> `uname -n` can be used to get the hostname that should\n>>be used?\n>>\n>>As far as I know this should be POSIX-compliant and uses gethostname(2).\n>\n> As previously pointed out, uname -n and gethostname(2) are not equivalent.\n> uname -n does not (depending on implementation) go to DNS while\n> gethostname(2) goes to DNS first (although apparently glibc may not). This\n> is particularly important in a multi-home situation where more than one IP\n> adapter has a different IP address on the same host, and where DNS does not\n> consider the different addresses to be equivalent (which otherwise could\n> cause problems for reverse lookups).\n\nThanks for the explanation, I did not notice this has already been\ndiscussed.\n\nInterestingly, I strace(1)'ed uname -n here on Linux and noticed it uses\nuname(2) (what else?) and not gethostname(2), so it seems I was\ncompletely wrong.\n\nSorry for disturbing the discussion.\n\nDirk\n"},{"id":"490974","messageId":"02d701da7a4d$3e60e960$bb22bc20$@nexbridge.com","threadId":"61072","inReplyTo":"ghr0g5j1sc.fsf@gouders.net","subject":"RE: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-19T22:31:47Z","receivedAt":"2024-03-19T22:32:18Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, March 19, 2024 6:27 PM, Dirk Gouders wrote:\n> writes:\n>\n>> On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote:\n>>>Junio C Hamano writes:\n>>>\n>>>> Eric Sunshine writes:\n>>>>\n>>>>> Peff felt that adding `git config --show-hostname-for-includes` was\n>>>>> probably overkill, but I'd argue that it is necessary to enable\n>>>>> users to deterministically figure out the value to use in their\n>>>>> configuration rather than having to grope around in the dark via\n>>>>> guesswork and trial-and-error to figure out exactly what works.\n>>>>>\n>>>>> And the option name doesn't necessarily have to be so verbose; a\n>>>>> shorter name, such as `git config --show-hostname` may be good enough.\n>>>>> Implementing this option would also obviate the need to implement\n>>>>> `test-tool xgethostname` (though, I agree with Junio that\n>>>>> `test-tool gethostname` would have been a better, less\n>>>>> implementation-revealing name).\n>>>>\n>>>> Yeah, I like that show-hostname thing (which I do not know if \"config\"\n>>>> is a good home for, though).\n>>>\n>>>A thought when I was reading this: wouldn't it be enough to document\n>>>that\n>> `uname -n` can be used to get the hostname that should\n>>>be used?\n>>>\n>>>As far as I know this should be POSIX-compliant and uses gethostname(2).\n>>\n>> As previously pointed out, uname -n and gethostname(2) are not\nequivalent.\n>> uname -n does not (depending on implementation) go to DNS while\n>> gethostname(2) goes to DNS first (although apparently glibc may not).\n>> This is particularly important in a multi-home situation where more\n>> than one IP adapter has a different IP address on the same host, and\n>> where DNS does not consider the different addresses to be equivalent\n>> (which otherwise could cause problems for reverse lookups).\n>\n>Thanks for the explanation, I did not notice this has already been\ndiscussed.\n>\n>Interestingly, I strace(1)'ed uname -n here on Linux and noticed it uses\n>uname(2) (what else?) and not gethostname(2), so it seems I was completely\n>wrong.\n>\n>Sorry for disturbing the discussion.\n\nNo worries. I only know this point because I was rather deeply in a related\ncode base back in 1994. I did not know that glibc varied from an old UNIX (I\nthink that's where the code was from) code base prior to this thread.\nLearning is good and never a problem.\n\nRegards,\nRandall\n\n"},{"id":"490978","messageId":"xmqqbk79ltep.fsf@gitster.g","threadId":"61072","inReplyTo":"02d701da7a4d$3e60e960$bb22bc20$@nexbridge.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T22:59:42Z","receivedAt":"2024-03-19T22:59:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> No worries. I only know this point because I was rather deeply in a related\n> code base back in 1994. I did not know that glibc varied from an old UNIX (I\n> think that's where the code was from) code base prior to this thread.\n> Learning is good and never a problem.\n\nIt is not surprising that you were doing gethostname in 1994, but it\nis very surprising that you still remember such details ;-)\n\n"},{"id":"490979","messageId":"20240320001934.GA903718@coredump.intra.peff.net","threadId":"61072","inReplyTo":"CAPig+cTFRAmzBGiJv2F-k1XWvGSbT8UeAG57T+XpB-1w66HRkQ@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:19:34Z","receivedAt":"2024-03-20T00:19:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote:\n\n> On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano wrote:\n> > Eric Sunshine writes:\n> > > Peff felt that adding `git config --show-hostname-for-includes` was\n> > > probably overkill, but I'd argue that it is necessary to enable users\n> > > to deterministically figure out the value to use in their\n> > > configuration rather than having to grope around in the dark via\n> > > guesswork and trial-and-error to figure out exactly what works.\n> > >\n> > > And the option name doesn't necessarily have to be so verbose; a\n> > > shorter name, such as `git config --show-hostname` may be good enough.\n> > > Implementing this option would also obviate the need to implement\n> > > `test-tool xgethostname` (though, I agree with Junio that `test-tool\n> > > gethostname` would have been a better, less implementation-revealing\n> > > name).\n> >\n> > Yeah, I like that show-hostname thing (which I do not know if \"config\"\n> > is a good home for, though).\n> \n> The other possibility which came to mind was adding a GIT_HOSTNAME\n> variable to the output of `git var -l`.\n\nThat strikes me as a more appropriate spot than an option to git-config.\nEven if config is the only thing _now_ which cares about the hostname,\nit may be something that other parts of the system care about in the\nfuture.\n\nSome care may need to be taken for error handling, though. For \"git var\nGIT_HOSTNAME\" it is OK to exit non-zero, but \"git var -l\" should not\nbail on a system where gethostname() doesn't work (it is still not clear\nto me if that is a real case to worry about or not).\n\n-Peff\n"},{"id":"490988","messageId":"CAPig+cT9QxRZhZyZV=Txt1VfqzDZX=gDbXvFg1nO=rjeMFaBeQ@mail.gmail.com","threadId":"61072","inReplyTo":"20240320001934.GA903718@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-20T02:49:30Z","receivedAt":"2024-03-20T02:49:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 19, 2024 at 8:19 PM Jeff King wrote:\n> On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote:\n> > The other possibility which came to mind was adding a GIT_HOSTNAME\n> > variable to the output of `git var -l`.\n>\n> That strikes me as a more appropriate spot than an option to git-config.\n> Even if config is the only thing _now_ which cares about the hostname,\n> it may be something that other parts of the system care about in the\n> future.\n\nAlso, taking into consideration Patrick's proposed revamp[1] of\ngit-config to give it a subcommand API, then git-config becomes an\neven less welcome place for a standalone --show-hostname option which,\nby itself, doesn't really fit into the subcommand paradigm, and it\nprobably doesn't make sense to add a new subcommand (\"info\" or\nwhatever) just for that.\n\n[1]: https://lore.kernel.org/git/cover.1710198711.git.ps@pks.im/T/#u\n\n> Some care may need to be taken for error handling, though. For \"git var\n> GIT_HOSTNAME\" it is OK to exit non-zero, but \"git var -l\" should not\n> bail on a system where gethostname() doesn't work (it is still not clear\n> to me if that is a real case to worry about or not).\n\nPorts to oddball platforms should probably be providing a\ngethostname() in \"compat/\" anyhow, just as is done for Windows in\n\"compat/mingw.c\".\n"},{"id":"490989","messageId":"CAPig+cS_hCOnAuwRd_dY5h37-wgHCk2tjS7asm2Dm_p=s41X3Q@mail.gmail.com","threadId":"61072","inReplyTo":"CAPig+cT9QxRZhZyZV=Txt1VfqzDZX=gDbXvFg1nO=rjeMFaBeQ@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-20T03:07:31Z","receivedAt":"2024-03-20T03:07:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 19, 2024 at 10:49 PM Eric Sunshine wrote:\n> On Tue, Mar 19, 2024 at 8:19 PM Jeff King wrote:\n> > Some care may need to be taken for error handling, though. For \"git var\n> > GIT_HOSTNAME\" it is OK to exit non-zero, but \"git var -l\" should not\n> > bail on a system where gethostname() doesn't work (it is still not clear\n> > to me if that is a real case to worry about or not).\n>\n> Ports to oddball platforms should probably be providing a\n> gethostname() in \"compat/\" anyhow, just as is done for Windows in\n> \"compat/mingw.c\".\n\nIgnore my mumbo jumbo response. You are, of course, correct that the\nimplementation of `git var -l` needs to be done with care so that it\ndoesn't bail if gethostname() fails; that's true regardless of whether\nor not a platform-specific \"compat/\" implementation is part of the\nmix.\n"},{"id":"491013","messageId":"CAPx1GvcVZgsU+WHecLd8PWo_MN+wJj6wQ-G5nxYvWnOUOhfQng@mail.gmail.com","threadId":"61072","inReplyTo":"CAPig+cS_hCOnAuwRd_dY5h37-wgHCk2tjS7asm2Dm_p=s41X3Q@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-03-20T14:34:48Z","receivedAt":"2024-03-20T14:35:01Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"The suggestion for having `git var` list GIT_HOSTNAME gave me\nan idea: perhaps instead of, or in addition to, a `hostname`\ncondition in the `includeif` code, we could:\n\n * have an `includeif:env:...` condition that tests an env\n variable against a pattern; and/or\n * use $GIT_HOSTNAME as the variable.\n\nWe'd then set `GIT_HOSTNAME` to the gethostname() result *unless*\nit's already set.\n\nThis gives users much more flexibility, because:\n\n * they can use the hostname and/or arbitrary-env-var condition;\n * they can then *set* GIT_HOSTNAME to the short or full\n hostname at their discretion if the default is not suitable\n for some reason; and of course\n * they can, as noted, use `git var` to find the default setting.\n\nChris\n"},{"id":"491022","messageId":"CAPig+cQOAfRNrzyr8ABMxkNzbazD95AN4u=T=xgTUnMF0zHZpQ@mail.gmail.com","threadId":"61072","inReplyTo":"CAPx1GvcVZgsU+WHecLd8PWo_MN+wJj6wQ-G5nxYvWnOUOhfQng@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-20T16:37:22Z","receivedAt":"2024-03-20T16:37:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 20, 2024 at 10:35 AM Chris Torek wrote:\n> The suggestion for having `git var` list GIT_HOSTNAME gave me\n> an idea: perhaps instead of, or in addition to, a `hostname`\n> condition in the `includeif` code, we could:\n>\n> * have an `includeif:env:...` condition that tests an env\n> variable against a pattern; and/or\n> * use $GIT_HOSTNAME as the variable.\n>\n> We'd then set `GIT_HOSTNAME` to the gethostname() result *unless*\n> it's already set.\n>\n> This gives users much more flexibility, because:\n>\n> * they can use the hostname and/or arbitrary-env-var condition;\n> * they can then *set* GIT_HOSTNAME to the short or full\n> hostname at their discretion if the default is not suitable\n> for some reason; and of course\n> * they can, as noted, use `git var` to find the default setting.\n\nThis certainly is a much more generic approach, and simplifies the\nimplementation considerably since it obviates the need for\nGIT_HOSTNAME (or --show-hostname) since the choice of variable name\nand value is fully under the user's control.\n\nI have some vague feeling that this idea of using an environment\nvariable as a condition may have been discussed before and possibly\nrejected due to potential security concerns, but I don't use\n`includeif` myself and haven't really followed past discussions, so I\ncould be wrong about that. Peff would probably have better\nrecollection.\n"},{"id":"491023","messageId":"xmqqfrwkhmqf.fsf@gitster.g","threadId":"61072","inReplyTo":"CAPx1GvcVZgsU+WHecLd8PWo_MN+wJj6wQ-G5nxYvWnOUOhfQng@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T16:49:44Z","receivedAt":"2024-03-20T16:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Torek writes:\n\n> The suggestion for having `git var` list GIT_HOSTNAME gave me\n> an idea: perhaps instead of, or in addition to, a `hostname`\n> condition in the `includeif` code, we could:\n>\n> * have an `includeif:env:...` condition that tests an env\n> variable against a pattern; and/or\n> * use $GIT_HOSTNAME as the variable.\n\nNice.\n\n> We'd then set `GIT_HOSTNAME` to the gethostname() result *unless*\n> it's already set.\n>\n> This gives users much more flexibility, because:\n>\n> * they can use the hostname and/or arbitrary-env-var condition;\n> * they can then *set* GIT_HOSTNAME to the short or full\n> hostname at their discretion if the default is not suitable\n> for some reason; and of course\n> * they can, as noted, use `git var` to find the default setting.\n>\n> Chris\n"},{"id":"491036","messageId":"20240320205141.GB2966606@coredump.intra.peff.net","threadId":"61072","inReplyTo":"CAPig+cQOAfRNrzyr8ABMxkNzbazD95AN4u=T=xgTUnMF0zHZpQ@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Add hostname condition to includeIf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T20:51:41Z","receivedAt":"2024-03-20T20:51:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 20, 2024 at 12:37:22PM -0400, Eric Sunshine wrote:\n\n> I have some vague feeling that this idea of using an environment\n> variable as a condition may have been discussed before and possibly\n> rejected due to potential security concerns, but I don't use\n> `includeif` myself and haven't really followed past discussions, so I\n> could be wrong about that. Peff would probably have better\n> recollection.\n\nI can't think of any security concerns; if you can control the\nenvironment you can already set GIT_CONFIG_PARAMETERS to do whatever you\nlike.\n\nIn fact, I think I've suggested includeIf.env before. ;)\n\nÆvar even wrote a patch, but I think we got bogged down in issues of\nsyntax:\n\n https://lore.kernel.org/git/patch-1.1-1fe6f60d2bf-20210924T005553Z-avarab@gmail.com/\n\n-Peff\n"},{"id":"491084","messageId":"CAPx1GvfZJpM6T_sbvFvNhg787o20QxHqVAhTXO0a+UEX7Gu9=Q@mail.gmail.com","threadId":"61072","inReplyTo":"xmqq5xxhnbgg.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] t: add a test helper for getting hostname","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-03-21T00:11:26Z","receivedAt":"2024-03-21T00:11:39Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Tue, Mar 19, 2024 at 2:44 PM Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > But we are not testing \"includeIf\" in this patch; we are testing git-gc,\n> > which falls back to the string \"unknown\".\n>\n> Ah, I wasn't aware of such a hardcoded default. Then replacing the\n> existing \"hostname\" with \"test-tool xgethostname\" and doing nothing\n> else is of course the right thing to do here.\n\nNote that if we choose to use $GIT_HOSTNAME (auto-set-if-unset),\nwe can change the test to simply force $GIT_HOSTNAME to something\nknown to the test script.\n\n(That seems orthogonal to this change, but I thought I would mention it.)\n\nChris\n"}]}