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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2024, 17:38 UTC
Message-ID: <xmqq8r2re265.fsf@gitster.g>
In-Reply-To: <01a9baa2-b36b-4b4e-8e54-7645e35d1a47@iencinas.com>

Ignacio Encinas Rubio <ignacio@iencinas.com> writes:

Show 16 quoted lines

>>> +test_expect_success 'conditional include, hostname' '
>>> +	echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
>>> +	echo "[test]twelve=12" >.git/bar12 &&
>>> +	test_must_fail git config test.twelve &&
>> 
>> Emulating other tests in this file that uses here document may make
>> it a bit easier to read?  E.g.,
>> 
>> 	cat >>.gitconfig <<-EOF &&
>> 	[includeIf "hostname:$(hostname)a"]
>> 		path = bar12
>> 	EOF
>
> Thanks for pointing that out. I just read the last tests from that file
> where they used the echo "..." >> approach. Do you think it is
> worthwhile rewriting those tests to use the approach you suggested?

I had an impression that existing ones do not have ugliness of backslash-quoting and do not benefit from such a rewrite to use here-document as much as the one you added does.

If that is not the case, and existing ones would concretely improve the readability with such a rewrite to use here-document, surely. If we want to do that route, we should either do one of the two.

 - The patch [1/2] does such a "style update" only on existing tests
   to improve their readability, and then patch [2/2] then does your
   addition to the tests, together with the code change.

 - Or you do this patch alone, without touching existing tests, but
   with your tests added in an easier-to-read style.  And then after
   the dust settles, a separate "style udpate" patch clean the
   existing ones up.

> By the way, before contributing, I saw there is some work on moving to
> unit tests. I wasn't sure how to test this particular feature there, so
> I went with the "old" approach as it seemed more natural. Is this ok?

We are not really "moving to". We did not have a test framework for effective unit tests, so some tests that would have been easier to manage as unit tests were instead written as an end-to-end integration tests, which was what we had a framework for. These tests are moving to, but for a test like "the user uses the '[includeIf X:Y] path = P' construct---does the git command really shows the effect of include from P when condition X:Y holds?", the unit testing framework would not be a better fit than the end-to-end behaviour test, I would say.