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

From: Ignacio Encinas Rubio <ignacio@iencinas.com>
Date: Mar 10, 2024, 18:46 UTC
Message-ID: <9fb02065-12d2-4c92-b1a5-74c06125c692@iencinas.com>
In-Reply-To: <xmqqy1aqvx9n.fsf@gitster.g>

On 10/3/24 17:59, Junio C Hamano wrote:

Show 14 quoted lines

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

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

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

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

Show 11 quoted lines

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

 
Thanks for providing an example.

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

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

> Thanks.

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