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

On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic [off-list ref] wrote:

On 2024-03-17 05:21, Eric Sunshine wrote:

quoted

On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic [off-list ref]
wrote:

quoted

+       x_to_tab >.git/config <<-\EOF
+       [section]
+               Xsolid = rock
+               Xsparse = big XX blue
+               ...
+       EOF
+'

The <<- operator strips all leading TAB characters, so the extra
indentation you've placed inside the "[section]" section is stripped
off. Thus, what you have above is the same as:

    x_to_tab >.git/config <<-\EOF
    [section]
    Xsolid = rock
    ...
    EOF

Yes, I was already aware that such indentation ends up wasted, but
having
it makes the test a bit more readable.  At least in my opinion, but if
you
find it better not to have such whitespace, for the sake of consistency,
I'll happily remove this indentation in the v3.

Readability wasn't my reason for bringing this up. As a reviewer,
every time a question pops into my mind as I'm reading the code, that
indicates that something about the code is unclear or that the commit
message doesn't properly explain why it was done in this way. People
coming across this code in the future may have the same questions but
they won't have the benefit of being able to easily ask you why it was
done this way.

So, the ideal patch is one in which the reviewer reads the code and
simply nods in understanding without having to question any of the
author's choices. And the ideal test is one in which does the bare
minimum to verify that the condition being checked is in fact correct;
there should be no extraneous code or behavior which could mislead the
reader into wondering why it was done that way.

In this particular case, it wasn't clear whether you, as author,
realized that all leading TABs are stripped from the heredoc body, and
whether or not that mattered to you and whether or not you expected it
to be significant to the test's behavior.

quoted

On a related note, it's not clear why you use 'X' to insert a TAB at
the beginning of each line. As I understand it, the configuration file
reader does not require such indentation, thus doing so is wasted.
Moreover, it confuses readers of this code (and reviewers) into
thinking that something unusual is going on, and leads to questions
such as this one: Why do you use 'X' to insert a TAB at the beginning
of the line?

Well, resorting to always not having such instances of 'X' to provide
leading indentation in test configuration files may actually make some
bugs go undetected in some tests.  To me, having leading indentation is
to be expected in the real configuration files in the field, thus
providing
the same indentation in a test configuration feels natural to me,
despite
admittedly making the test configuration a bit less readable.

Of course, consistency is good, but variety is also good when it comes
to automated tests.  I'm not very familiar with the tests in git, so
please let me know if consistency outweights variety in this case, and
I'll happily remove the leading "X" indentations in the v3.

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?