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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Dragan Simic <dsimic@manjaro.org>
Cc: git@vger.kernel.org, gitster@pobox.com, 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 15:17:58 -0400	[thread overview]
Message-ID: <CAPig+cSJdBm+sRcXSpdZYUqSqktN3ytcjD3kmhu6WfTRuqkPrg@mail.gmail.com> (raw)
In-Reply-To: <c579edaac0d67a6ff46fe02072bddbb4@manjaro.org>

On Mon, Mar 18, 2024 at 4:17 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-03-18 03:48, Eric Sunshine wrote:
> > 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.
>
> I see.  How about including a small comment in the t1300 that would
> explain the additional indentation?

I'm just one reviewer. Unless others chime in with similar
observations or questions regarding the patch, I don't think such a
comment is necessary. Aside from the other more significant points
(such as not introducing x_to_tab(), using "setup" in the function
title, etc.), this is extremely minor, and what you have here is "good
enough" (though you may want to take Junio's suggestion of using a
leading "|" to protect indentation).

> As a note, there are already more tests in the t1300 that contain such
> indentation, so maybe we shoulddo something with those existing tests
> as well;  the above-proposed comment, which would be placed at the very
> beginning of t1300, may provide a satisfactory explanation for all the
> tests in t1300 that contain such additional indentation.
>
> Another option would be to either add the indentation to all relevant
> tests in the t1300, or to remove the indentation from all tests in the
> t1300 that already contain it.  I'd be happy to implement and submit
> patches that do that, after we choose the direction we want to follow.

It would be better to keep this series focused on its primary goal of
fixing a bug rather than being held hostage to an ever increasing set
of potential cleanups. Such cleanups can be done as separate patch
series either atop this series or alongside it. Let's land this series
first, and then, if you wish, tackle those other less significant
issues.

> > 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?
>
> Good point, despite that not being the main purpose of the added tests.
> I'll see to add a couple of tests that check the handling of
> indentation,
> possibly at some places in the t1300 that fit the best;  improving the
> tests coverage can only help in the long run.

As above, such additional tests probably aren't mandatory for this
bug-fix series. As a reviewer, I'd like to see fewer and fewer changes
between each version of a patch series; the series should converge so
that it can land rather than diverge from iteration to iteration. Such
additional leading-whitespace tests may be perfectly appropriate for a
follow-up series.