From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 18 Sep 2026 07:10:51 GMT
Subject: Re: [PATCH v3 1/3] Update t/t4001-diff-rename.sh to modern style.
Message-ID: <xmqqjyojowzo.fsf@gitster.g>
In-Reply-To: <20260917195436.1102981-2-markchucarroll@fastmail.com>

"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

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

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

> -test_expect_success 'setup for many rename source candidates' '
> +test_expect_success 'rename with many candidates' '
>  	git reset --hard &&
>  	for i in 0 1 2 3 4 5 6 7 8 9;
>  	do
> @@ -177,14 +166,14 @@ test_expect_success 'setup for many rename source candidates' '
>  		done
>  	done &&
>  	git add "path??" &&
> -	test_tick &&
> +	test_tick  &&

Good eyes ;-).

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