| Message ID | 20240109060417.1144647-3-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 |
First about the commit subject: "t7501: Add tests for various index usages, -i and -o, of commit command." it should be shorter, shouldn't end with a "." and "Add" should be "add". On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote: > > This commit adds tests for -i and -o flags of the commit command. It > includes testing -i with and without staged changes, testing -o with and > without staged changes, and testing -i and -o together. A few suggestions to make things a bit more clear: - tell that -i is a synonymous for --include and -o for --only - tell if there are already tests for these options - tell why the tests you add are worth it if tests for an option already exist > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > > diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh > index 3d8500a52e..71dc52ce3a 100755 > --- a/t/t7501-commit-basic-functionality.sh > +++ b/t/t7501-commit-basic-functionality.sh > @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' ' > test_must_fail git commit -m initial > ' > > +test_expect_success 'commit with -i fails with untracked files' ' > + test_when_finished "rm -rf testdir" && > + git init testdir && > + echo content >testdir/file.txt && > + test_must_fail git -C testdir commit -i file.txt -m initial > +' Existing tests didn't need a repo, so I am not sure it's worth creating a testdir repo just for this test. Also I am not sure this is the best place in the test script to add -i and -o related tests. Here we are between a 'nothing to commit' test and a '--dry-run fails with nothing to commit' and then more 'nothing to commit' related tests. I think it would be better if all those 'nothing to commit' related tests could stay together. > +test_expect_success 'commit with -i' ' > + echo content >bar && > + git add bar && > + git commit -m "bar" && Why is the above needed for this test? > + echo content2 >bar && > + git commit -i bar -m "bar second" > +' > + > +test_expect_success 'commit with -i multiple files' ' > + test_when_finished "git reset --hard" && > + echo content >bar && > + echo content >baz && > + echo content >saz && > + git add bar baz saz && > + git commit -m "bar baz saz" && Not sure why the above is needed here too. > + echo content2 >bar && > + echo content2 >baz && > + echo content2 >saz && > + git commit -i bar saz -m "bar" && > + git diff --name-only >remaining && > + test_grep "baz" remaining > +' > + > +test_expect_success 'commit with -i includes staged changes' ' > + test_when_finished "git reset --hard" && > + echo content >bar && > + git add bar && > + git commit -m "bar" && > + > + echo content2 >bar && > + echo content2 >baz && > + git add baz && > + git commit -i bar -m "bar baz" && > + git diff --name-only >remaining && > + test_must_be_empty remaining && > + git diff --name-only --staged >remaining && > + test_must_be_empty remaining > +' > + > +test_expect_success 'commit with -o' ' > + echo content >bar && > + git add bar && > + git commit -m "bar" && > + echo content2 >bar && > + git commit -o bar -m "bar again" > +' > + > +test_expect_success 'commit with -o fails with untracked files' ' > + test_when_finished "rm -rf testdir" && > + git init testdir && > + echo content >testdir/bar && > + test_must_fail git -C testdir commit -o bar -m "bar" > +' > + > +test_expect_success 'commit with -o exludes staged changes' ' s/exludes/excludes/ > + test_when_finished "git reset --hard" && > + echo content >bar && > + echo content >baz && > + git add . && > + git commit -m "bar baz" && > + > + echo content2 >bar && > + echo content2 >baz && > + git add baz && > + git commit -o bar -m "bar" && > + git diff --name-only --staged >actual && > + test_grep "baz" actual > +' Thanks.
On Tue Jan 9, 2024 at 2:50 PM IST, Christian Couder wrote: > First about the commit subject: > > "t7501: Add tests for various index usages, -i and -o, of commit command." > > it should be shorter, shouldn't end with a "." and "Add" should be "add". Updated in v2. > On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar > <shyamthakkar001@gmail.com> wrote: > > > > This commit adds tests for -i and -o flags of the commit command. It > > includes testing -i with and without staged changes, testing -o with and > > without staged changes, and testing -i and -o together. > > A few suggestions to make things a bit more clear: > > - tell that -i is a synonymous for --include and -o for --only > - tell if there are already tests for these options > - tell why the tests you add are worth it if tests for an option already exist I have updated the commit messages in v2 to address these points. > > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > > --- > > t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > > > diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh > > index 3d8500a52e..71dc52ce3a 100755 > > --- a/t/t7501-commit-basic-functionality.sh > > +++ b/t/t7501-commit-basic-functionality.sh > > @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' ' > > test_must_fail git commit -m initial > > ' > > > > +test_expect_success 'commit with -i fails with untracked files' ' > > + test_when_finished "rm -rf testdir" && > > + git init testdir && > > + echo content >testdir/file.txt && > > + test_must_fail git -C testdir commit -i file.txt -m initial > > +' > > Existing tests didn't need a repo, so I am not sure it's worth > creating a testdir repo just for this test. Yes, I just wanted to make sure the files were not tracked. However, I have updated these instaces to use the existing repo and use unique non-generic names for generating untracked files. > Also I am not sure this is the best place in the test script to add -i > and -o related tests. Here we are between a 'nothing to commit' test > and a '--dry-run fails with nothing to commit' and then more 'nothing > to commit' related tests. I think it would be better if all those > 'nothing to commit' related tests could stay together. I have moved these tests above the "--amend" related tests, which do not break the flow of the tests. > > +test_expect_success 'commit with -i' ' > > + echo content >bar && > > + git add bar && > > + git commit -m "bar" && > > Why is the above needed for this test? This was to make sure that the file is tracked, however, I realised that committing is not necessary, so I have updated this test to use existing files in repo and to not generate a new one. > > > + echo content2 >bar && > > + git commit -i bar -m "bar second" > > +' > > + > > +test_expect_success 'commit with -i multiple files' ' > > + test_when_finished "git reset --hard" && > > + echo content >bar && > > + echo content >baz && > > + echo content >saz && > > + git add bar baz saz && > > + git commit -m "bar baz saz" && > > Not sure why the above is needed here too. Similar to above, I have updated this test to use existing files in repo and to not generate a new one. > > > + echo content2 >bar && > > + echo content2 >baz && > > + echo content2 >saz && > > + git commit -i bar saz -m "bar" && > > + git diff --name-only >remaining && > > + test_grep "baz" remaining > > +' > > + > > +test_expect_success 'commit with -i includes staged changes' ' > > + test_when_finished "git reset --hard" && > > + echo content >bar && > > + git add bar && > > + git commit -m "bar" && > > + > > + echo content2 >bar && > > + echo content2 >baz && > > + git add baz && > > + git commit -i bar -m "bar baz" && > > + git diff --name-only >remaining && > > + test_must_be_empty remaining && > > + git diff --name-only --staged >remaining && > > + test_must_be_empty remaining > > +' > > + > > +test_expect_success 'commit with -o' ' > > + echo content >bar && > > + git add bar && > > + git commit -m "bar" && > > + echo content2 >bar && > > + git commit -o bar -m "bar again" > > +' > > + > > +test_expect_success 'commit with -o fails with untracked files' ' > > + test_when_finished "rm -rf testdir" && > > + git init testdir && > > + echo content >testdir/bar && > > + test_must_fail git -C testdir commit -o bar -m "bar" > > +' > > + > > +test_expect_success 'commit with -o exludes staged changes' ' > > s/exludes/excludes/ Done. > > > + test_when_finished "git reset --hard" && > > + echo content >bar && > > + echo content >baz && > > + git add . && > > + git commit -m "bar baz" && > > + > > + echo content2 >bar && > > + echo content2 >baz && > > + git add baz && > > + git commit -o bar -m "bar" && > > + git diff --name-only --staged >actual && > > + test_grep "baz" actual > > +' > > Thanks. Thanks for the review!
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > Subject: Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command. Overly long subject that has an unusual capitalization after "t7501:" (see "git log --no-merges --format=%s -20 v2.43.0" for example and try to write something that blends better). > +test_expect_success 'commit with -i fails with untracked files' ' > + test_when_finished "rm -rf testdir" && > + git init testdir && > + echo content >testdir/file.txt && > + test_must_fail git -C testdir commit -i file.txt -m initial > +' In addition to "why a new repository???" comment raised already, I do not want to see the last command spelled like so. Always write dashed options (and their parameters) before non-option arguments, i.e. git commit -i -m initial file.txt git -C testdir commit -i -m initial file.txt test_must_fail git -C testdir commit -i -m initial file.txt The command line parser does rotate the unrecognized arguments to the end and keeps looking for recognisable option (possibly followed by its parameter), but that is purely to help lazy writers (i.e., interactive command users). When writers know "-i" does not take any parameter, it may be convenient if the writer who forgot to say "-m" can just append "-m initial" to what has already be written. When writing source (be it the production code or test), however, we write for readers. What you wrote at a first glance, especially given that "-i" (or "-o" for that matter) is a relatively less commonly used option, would confuse less experienced readers by making them wonder what "-i file.txt" means (e.g., "is that taking input from the contents of file.txt?").
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh index 3d8500a52e..71dc52ce3a 100755 --- a/t/t7501-commit-basic-functionality.sh +++ b/t/t7501-commit-basic-functionality.sh @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' ' test_must_fail git commit -m initial ' +test_expect_success 'commit with -i fails with untracked files' ' + test_when_finished "rm -rf testdir" && + git init testdir && + echo content >testdir/file.txt && + test_must_fail git -C testdir commit -i file.txt -m initial +' + +test_expect_success 'commit with -i' ' + echo content >bar && + git add bar && + git commit -m "bar" && + + echo content2 >bar && + git commit -i bar -m "bar second" +' + +test_expect_success 'commit with -i multiple files' ' + test_when_finished "git reset --hard" && + echo content >bar && + echo content >baz && + echo content >saz && + git add bar baz saz && + git commit -m "bar baz saz" && + + echo content2 >bar && + echo content2 >baz && + echo content2 >saz && + git commit -i bar saz -m "bar" && + git diff --name-only >remaining && + test_grep "baz" remaining +' + +test_expect_success 'commit with -i includes staged changes' ' + test_when_finished "git reset --hard" && + echo content >bar && + git add bar && + git commit -m "bar" && + + echo content2 >bar && + echo content2 >baz && + git add baz && + git commit -i bar -m "bar baz" && + git diff --name-only >remaining && + test_must_be_empty remaining && + git diff --name-only --staged >remaining && + test_must_be_empty remaining +' + +test_expect_success 'commit with -o' ' + echo content >bar && + git add bar && + git commit -m "bar" && + echo content2 >bar && + git commit -o bar -m "bar again" +' + +test_expect_success 'commit with -o fails with untracked files' ' + test_when_finished "rm -rf testdir" && + git init testdir && + echo content >testdir/bar && + test_must_fail git -C testdir commit -o bar -m "bar" +' + +test_expect_success 'commit with -o exludes staged changes' ' + test_when_finished "git reset --hard" && + echo content >bar && + echo content >baz && + git add . && + git commit -m "bar baz" && + + echo content2 >bar && + echo content2 >baz && + git add baz && + git commit -o bar -m "bar" && + git diff --name-only --staged >actual && + test_grep "baz" actual +' + +test_expect_success 'commit with both -i and -o fails' ' + test_when_finished "git reset --hard" && + echo content >bar && + echo content >baz && + git add . && + git commit -m "bar baz" && + + echo content2 >bar && + echo content2 >baz && + test_must_fail git commit -i baz -o bar -m "bar" +' + test_expect_success '--dry-run fails with nothing to commit' ' test_must_fail git commit -m initial --dry-run '
This commit adds tests for -i and -o flags of the commit command. It includes testing -i with and without staged changes, testing -o with and without staged changes, and testing -i and -o together. Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)