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

From: Dragan Simic <dsimic@manjaro.org>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, 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 22:57:00 +0100	[thread overview]
Message-ID: <f37d753485094a3ba66fde5e85d0e2dc@manjaro.org> (raw)
In-Reply-To: <CAPig+cQgejS5dFpOYkMT49kwn5zvn5ihvyFqViFwHhEmtr9mqw@mail.gmail.com>

On 2024-03-18 20:21, Eric Sunshine wrote:
> On Mon, Mar 18, 2024 at 4:37 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-03-18 05:38, Junio C Hamano wrote:
>> >       sed -e 's/^|//' -e 's/Q/   /g' >.git/config <<-\EOF
>> >       |[section]
>> >       |       solid = rock
>> >       |       sparse = big QQ blue
>> >       |       ...
>> >       EOF
>> >
>> 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.
> 
> As noted in my other response, while such cleanups may be nice in the
> long-run, the bug-fix patch series under discussion should not be held
> hostage by an ever-increasing set of potential cleanups. Let's focus
> on landing the current series; these tangential cleanups can be done
> in a separate series.

Totally agreed, let's keep this plan for the follow-up patches.