Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2024, 17:45 UTC
Message-ID: <xmqqedeqienh.fsf@gitster.g>
In-Reply-To: <20240109060417.1144647-4-shyamthakkar001@gmail.com>

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Subject: Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.

The title is with unusual capitalization and final full-stop (again, check "git log --no-merges --format=%s -20 v2.43.0" and try to blend in).

> This commit adds test for amending the latest commit to add
> Signed-off-by trailer at the end of commit message.

"This commit adds ..." -> "Add ..."

Also what the patch does can be read from the patch text below, but it cannot be read _why_ the patch author thought it was a good idea to make such a change. The proposed commit log message is a place to describe the reason behind the patch. Why do we want a new test? Why do we want that new test in this particular file? etc.

> +test_expect_success 'amend commit to add signoff' '
> +
> +	test_when_finished "rm -rf testdir" &&
> +	git init testdir &&

The same "why a new repository for just this test???" applies here.

Show 5 quoted lines

> +	echo content >testdir/file &&
> +	git -C testdir add file &&
> +	git -C testdir commit -m "file" &&
> +	git -C testdir commit --amend --signoff &&
> +	git -C testdir log -1 --pretty=format:%B >actual &&

If you are doing many things in a separate directory, the usual pattern is

	# create a directory DIR (usuall "mkdir", not "git init")
	mkdir DIR &&
	(
		cd DIR &&
		git do this &&
		git do that &&
		inspect the result of this >actual &&
		prepare the expected outcome >expect &&
		test_cmp expect actual
	) &&

Thanks.