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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-03-18 20:17, Eric Sunshine wrote:
> 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).

Just to reiterate, both x_to_tab() and the test naming have already
been addressed in the future v3 of this series.

>> 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.

Thanks, I totally agree.

>> > 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.

Agreed once again.  Let's wrap this up, and I'll come back with the
follow-up patches.