From: Dragan Simic Date: Mon, 18 Mar 2024 21:57:00 GMT Subject: Re: [PATCH v2 4/5] t1300: add more tests for whitespace and inline comments Message-ID: In-Reply-To: On 2024-03-18 20:21, Eric Sunshine wrote: > On Mon, Mar 18, 2024 at 4:37 AM Dragan Simic > 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.