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

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

On Tue Jan 9, 2024 at 4:14 PM IST, Phillip Wood wrote:
> Hi Ghanshyam
>
> On 09/01/2024 06:04, Ghanshyam Thakkar wrote:
> > This commit adds test for amending the latest commit to add
> > Signed-off-by trailer at the end of commit message.
>
> If we're not already testing this then it seems like a useful addition, 
> thanks for working on it. It would also be helpful to check that "git 
> commit --amend --signoff" does not add a Signed-off-by: trailer if it 
> already exists.

I hadn't thought of that. I have hastily sent the v2 without seeing this
comment. I will add this test in v3.
>
> > +test_expect_success 'amend commit to add signoff' '
> > +
> > +	test_when_finished "rm -rf testdir" &&
> > +	git init testdir &&
>
> As Christian said about the other patch in this series I don't think we 
> need a new repository here. In our test files we use the same repository 
> for the whole file unless there is a compelling reason not to.

Updated from v2 onwards.
>
> > +	echo content >testdir/file &&
> > +	git -C testdir add file &&
> > +	git -C testdir commit -m "file" &&
>
> I think these three lines can be replaced by
>
> 	test_commit --no-tag file file content

Thank you for the suggestion. I have updated the test to use test_commit.
>
> > +	git -C testdir commit --amend --signoff &&
>
> > +	git -C testdir log -1 --pretty=format:%B >actual &&
> > +	(
> > +		echo file &&
> > +		echo &&
> > +		git -C testdir var GIT_COMMITTER_IDENT >ident &&
> > +		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" ident
> > +	) >expected &&
> > +	test_cmp expected actual
>
> This section of the test can be improved by using test_commit_message
>
> 	test_commit_message HEAD <<-EOF
> 	file
> 	
> 	Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>
> 	EOF
I have updated the test to use above approach.

Thank you for the review!