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