Hello Junio,
On 2024-03-18 05:38, Junio C Hamano wrote:
Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx/
