From: Mark C. Chu-Carroll <markchucarroll@fastmail.com>
Date: Fri, 18 Sep 2026 14:54:15 GMT
Subject: Re: [PATCH v3 1/3] Update t/t4001-diff-rename.sh to modern style.
Message-ID: <DLIJ195ABVQL.3OD6QX7D3VNBE@fastmail.com>
In-Reply-To: <xmqqjyojowzo.fsf@gitster.g>

On Fri Sep 18, 2026 at 3:10 AM EDT, Junio C Hamano wrote:
> "Mark C. Chu-Carroll" <markchucarroll@fastmail.com> writes:
>
>> Subject: Re: [PATCH v3 1/3] Update t/t4001-diff-rename.sh to modern style.
>
> Documentation/SubmittingPatches::[[summary-section]]
>
> Perhaps
>
>     Subject: [PATCH v3 1/3] t4001: modernize

Good suggestion, thanks.

>
>> Old tests were written in a different style than modern
>> ones; for better readability and test error messages,
>> update t4001 to the modern style.
>
> That is a laudable goal, but enumerate the rules you followed to
> achieve that goal.  For example, a few of them may have been
>
>  * Run everything inside test_expect_success block.
>
>  * Write the title on the same line as test_expect_success and end
>    that line with a singul quote that opens the body of the test; a
>    line with a sole single quote then closes the body.
>
>
>> Signed-off-by: Mark C. Chu-Carroll <markchucarroll@fastmail.com>
>> ---
>
>> -test_expect_success 'setup' '
>> +initial_setup () {
>
> Why a shell function that is called only once by a test titled "Diff
> shows path0 ..." below?  It is customary to perform the setup in the
> very first test_expect_success whose title is 'setup'.  I doubt it
> would contribute to better readability to change that.

I went back and forth on that in terms of readability. I'll switch
it back.

>
>> -test_expect_success 'test diff.renames=true' '
>> +test_expect_success 'With diff.renames=true, patch includes rename of path0 to path1' '
>> -test_expect_success 'test diff.renames=false' '
>> +test_expect_success 'With diff.renames=false, patch does not include rename' '
>> ...
>>  
>> -test_expect_success 'favour same basenames even with minor differences' '
>> +test_expect_success 'rename logic favours same basenames even with minor differences' '
>
> Here you are failing to follow your own renaming rules to start test
> titles in uppercase letters.  I personally wouldn't have chosen to
> upcase many of them; rather, the majority of tests in this file seem
> to be named in lowercase so I would have instead made sure I'd start
> them in lowercase as well.  In any case, please be consistent.

Good eye - I didn't even see that.

>> -test_expect_success 'rename pretty print with common prefix' '
>> +test_expect_success 'rename pretty prints paths with common prefix' '
>
> Meh.
>
> Neither way, it is grammatical at all 'rename' may not be plural, so
> the verb 'print' may want 's' for third-person-singular, but it does
> not make sense to use 'rename' as the subject for "pretty-print"
> anyway.  It is not that 'rename' is the one that prints something.
>

I'll try to come up with something better.

Thanks for the review! I'll send an update later today.

         -Mark


-- 
Mark Craig Chu-Carroll (@MarkChuCarroll at gitlab)
*** Software Tools/Math Geek - Software Engineer at Gitlab
*** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com
*** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com