Re: [PATCH v2 4/5] t1300: add more tests for whitespace and inline comments

From: Dragan Simic <dsimic@manjaro.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
	git@vger.kernel.org, rsbecker@nexbridge.com, github@seichter.de
Subject: Re: [PATCH v2 4/5] t1300: add more tests for whitespace and inline comments
Date: Mon, 18 Mar 2024 09:37:52 +0100	[thread overview]
Message-ID: <62a4ec0dace46d7c56e75dac9c42c297@manjaro.org> (raw)
In-Reply-To: <xmqqttl4kvch.fsf@gitster.g>

Hello Junio,

On 2024-03-18 05:38, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>>> >> +       x_to_tab >.git/config <<-\EOF
>>> >> +       [section]
>>> >> +               Xsolid = rock
>>> >> +               Xsparse = big XX blue
>>> >> +               ...
>>> >> +       EOF
>>> >> +'
> 
> Just this part.
> 
>> My assumption, perhaps incorrectly, was that existing tests already
>> verified correct behavior of leading whitespace and that the tests
>> added by this patch were about internal whitespace. If that's not the
>> case (and perhaps I didn't fully digest the commit message) then my
>> question about the leading "X" is off the mark.
>> 
>> If these new tests are also checking leading whitespace behavior, then
>> to improve coverage, would it make sense to have the leading "X" on
>> some lines but not others?
> 
> If "<<-" (I have here-doc but please strip the leading tabs because
> I am aligning the here-doc with them) gets in the way for testing
> material with leading tabs, the way to write and preprocess such a
> here-doc is:
> 
> 	sed -e 's/^|//' -e 's/Q/   /g' >.git/config <<-\EOF
> 	|[section]
> 	|	solid = rock
> 	|	sparse = big QQ blue
> 	|	...
> 	EOF
> 
> It will make it clear where the left-edge of the "sheet of paper"
> is, removal of leading '|' does not get in the way of using '|' in
> the middle of the line if needed, and Q being the least used letter
> makes them stand out more in the middle of the line.  As it is
> obvious that what is before solid and sparse is a tab (otherwise you
> would not be using that '|' trick), you do not have to write Xsolid
> or Qsolid there and still the result is much easier to read.

This looks quite neat.  Furthermore, I think we should also consider
the already existing tests in the t1300 that contain such indentation.
As I already explained in my earlier response to Eric, [1] the choice
of including the indentation or not seems random to me, so we should
perhaps consider taking some broader approach.

How about this as a plan for moving forward:

1) Sprinkle a couple of tests onto the t1300, which try to be
    focused on the verification of the indentation-handling logic;
    maybe those additional tests could be even seen as redundant,
    but I think they can only help with the test coverage

2) Create a new helper function that uses the logic you described
    above, to make it simpler to include the indentation into configs

3) Finally, propagate the use of this new helper function into the
    new test and the already existing tests in the t1300 that already
    include the indentation

I'd be happy to implement all of the above-proposed steps in the next
couple of days.  Sure, it would be quite time-consuming, especially the
third proposed step, but it should be worth it in the long run.

[1] 
https://lore.kernel.org/git/c579edaac0d67a6ff46fe02072bddbb4@manjaro.org/