| Message ID | 20240109060417.1144647-4-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff. | expand |
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. > +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. > + 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 > + 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 Best Wishes Phillip
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!
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. > + 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.
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh index 71dc52ce3a..35c69c3ddd 100755 --- a/t/t7501-commit-basic-functionality.sh +++ b/t/t7501-commit-basic-functionality.sh @@ -464,6 +464,24 @@ test_expect_success 'amend commit to fix author' ' ' +test_expect_success 'amend commit to add signoff' ' + + test_when_finished "rm -rf testdir" && + git init testdir && + 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 && + ( + 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 +' + test_expect_success 'amend commit to fix date' ' test_tick &&
This commit adds test for amending the latest commit to add Signed-off-by trailer at the end of commit message. Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- t/t7501-commit-basic-functionality.sh | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)