On 7/3/24 22:40, Junio C Hamano wrote:
> Ignacio Encinas <ignacio@xxxxxxxxxxxx> writes:
>
>> Currently, customizing the configuration depending on the machine running
>> git has to be done manually.
>>
>> Add support for a new includeIf keyword "hostname:" to conditionally
>> include configuration files depending on the hostname.
>>
>> Signed-off-by: Ignacio Encinas <ignacio@xxxxxxxxxxxx>
>> ---
>> Documentation/config.txt | 9 +++++++++
>> config.c | 16 ++++++++++++++++
>> t/t1305-config-include.sh | 22 ++++++++++++++++++++++
>> 3 files changed, 47 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e3a74dd1c1..9a22fd2609 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>> a naming scheme that supports more variable-based include conditions,
>> but currently Git only supports the exact keyword described above.
>
>> +`hostname`::
>> + The data that follows the keyword `hostname:` is taken to be a
>> + pattern with standard globbing wildcards. If the current
>> + hostname matches the pattern, the include condition is met.
>> +
>
> OK. This seems to copy its phrasing from the existing text for
> "gitdir" and "onbranch", which greatly helps the description for
> these features consistent.
>
>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..e0611fc342 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)
>> return ret;
>> }
>>
>> +static int include_by_hostname(const char *cond, size_t cond_len)
>> +{
>> + int ret;
>> + char my_host[HOST_NAME_MAX + 1];
>> + struct strbuf pattern = STRBUF_INIT;
>> + if (xgethostname(my_host, sizeof(my_host)))
>> + return 0;
>> +
>> + strbuf_add(&pattern, cond, cond_len);
>> + ret = !wildmatch(pattern.buf, my_host, 0);
>> + strbuf_release(&pattern);
>> + return ret;
>> +}
>
> Have a blank line between the end of the decl block (our codebase
> frowns upon decl-after-statement) and the first statement,
> i.e. before "if (xgethostname...".
>
> Otherwise this looks reasonable to me.
Got it.
>> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
>> index 5cde79ef8c..ee78d9cade 100755
>> --- a/t/t1305-config-include.sh
>> +++ b/t/t1305-config-include.sh
>> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '
>> grep "exceeded maximum include depth" stderr
>> '
>>
>> +test_expect_success 'conditional include, hostname' '
>> + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
>> + echo "[test]twelve=12" >.git/bar12 &&
>> + test_must_fail git config test.twelve &&
>
> Emulating other tests in this file that uses here document may make
> it a bit easier to read? E.g.,
>
> cat >>.gitconfig <<-EOF &&
> [includeIf "hostname:$(hostname)a"]
> path = bar12
> EOF
Thanks for pointing that out. I just read the last tests from that file
where they used the echo "..." >> approach. Do you think it is
worthwhile rewriting those tests to use the approach you suggested?
By the way, before contributing, I saw there is some work on moving to
unit tests. I wasn't sure how to test this particular feature there, so
I went with the "old" approach as it seemed more natural. Is this ok?
>> + echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
>
> Ditto for the remainder of the patch.
>
> Thanks.
Thank you for the review.
