Re: [PATCH v3 1/3] Update t/t4001-diff-rename.sh to modern style.

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2026, 07:10 UTC
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.

Show 8 quoted lines

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

Show 11 quoted lines

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