On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic <dsimic@xxxxxxxxxxx> wrote: > On 2024-03-17 05:21, Eric Sunshine wrote: > > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@xxxxxxxxxxx> > > wrote: > >> + 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. > > 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?
