| Message ID | 20240112180109.59350-2-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t7501: add tests for --include, --only, --signoff | expand |
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' ' > test_must_fail git commit -m initial --long > ' > > +test_expect_success 'fail to commit untracked file' ' > + echo content >baz && > + test_must_fail git commit -m "baz" baz > +' > + > +test_expect_success '--only also fail to commit untracked file' ' > + test_must_fail git commit --only -m "baz" baz > +' > + > +test_expect_success '--include also fail to commit untracked file' ' > + test_must_fail git commit --include -m "baz" baz > +' As the latter two depends on the first one's side effect of leaving an untracked 'baz' file in the working tree, I do not know if it is sensible to split these into three tests. An obvious alternative is to have a single test test_expect_success 'pathspec that do not match any tracked path' ' echo content >baz && test_must_fail git commit -m baz baz && test_must_fail git commit -o -m baz baz && test_must_fail git commit -i -m baz baz ' By the way, I do not think presence of 'baz' in the working tree matters in the failures from these tests all that much, as the reason they fail is because the pathspec does not match any tracked file, whose contents in the index to be updated before made into a commit. > @@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' ' > git commit -m next -a --long > ' > > +test_expect_success 'only commit given path (also excluding additional staged changes)' ' > + echo content >file && > + echo content >baz && > + git add baz && > + git commit -m "file" file && > + > + git diff --name-only >actual && > + test_must_be_empty actual && > + > + git diff --name-only --staged >actual && > + test_cmp - actual <<-EOF && > + baz > + EOF > + > + git diff --name-only HEAD^ HEAD >actual && > + test_cmp - actual <<-EOF > + file > + EOF > +' OK. The change to baz is already in the index, but "commit file" will skip over it and record only the changes to file in the commit. Very much makes sense. > +test_expect_success 'same as above with -o/--only' ' > + echo change >file && > + echo change >baz && > + git add baz && > + git commit --only -m "file" file && > + > + git diff --name-only >actual && > + test_must_be_empty actual && > + > + git diff --name-only --staged >actual && > + test_cmp - actual <<-EOF && > + baz > + EOF > + > + git diff --name-only HEAD^ HEAD >actual && > + test_cmp - actual <<-EOF > + file > + EOF > +' Likewise. An obvious thing to notice is that this cannot use the same "contents" text as before, even though it claims to be "same as above". If the final contents of "file" and "baz" does not matter, but it matters more that these files have been changed, it often is a good idea to append to the file. That way, you can ensure that you will be making them different, no matter what the initial condition was, i.e., for opt in "" "-o" "--only" do test_expect_success 'skip over already added change' ' echo more >>file && echo more >>baz && git add baz && git commit $opt -m "file" file && ... ensure that changes to file are committed ... and changes to baz is only in the index ' done let's you test all three combinations. Thanks.
On Sat Jan 13, 2024 at 4:40 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > > > @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' ' > > test_must_fail git commit -m initial --long > > ' > > > > +test_expect_success 'fail to commit untracked file' ' > > + echo content >baz && > > + test_must_fail git commit -m "baz" baz > > +' > > + > > +test_expect_success '--only also fail to commit untracked file' ' > > + test_must_fail git commit --only -m "baz" baz > > +' > > + > > +test_expect_success '--include also fail to commit untracked file' ' > > + test_must_fail git commit --include -m "baz" baz > > +' > > As the latter two depends on the first one's side effect of leaving > an untracked 'baz' file in the working tree, I do not know if it is > sensible to split these into three tests. An obvious alternative is > to have a single test > > test_expect_success 'pathspec that do not match any tracked path' ' > echo content >baz && > test_must_fail git commit -m baz baz && > test_must_fail git commit -o -m baz baz && > test_must_fail git commit -i -m baz baz > ' > > By the way, I do not think presence of 'baz' in the working tree > matters in the failures from these tests all that much, as the > reason they fail is because the pathspec does not match any tracked > file, whose contents in the index to be updated before made into a > commit. Yes, that is true. However, as per your prior email in which you stated about --include: "Now which behaviour between "error out because there is no path in the index that matches pathspec 'baz'" and "add baz to the index and commit that addition, together with what is already in the index" we would want to take is probably open for discussion. Such a discussion may decide that the current behaviour is fine. Until then..." If in such a discussion it is decided that -i should add to index and commit, then by not having 'baz' in the working tree, the -i test would still error out regardless of whether its behaviour is to [add to the index and commit] or [error out]. Therefore, by having 'baz' we can detect the change between [-i adds to index and commits] or [errors out]. > Likewise. An obvious thing to notice is that this cannot use the > same "contents" text as before, even though it claims to be "same as > above". If the final contents of "file" and "baz" does not matter, > but it matters more that these files have been changed, it often is > a good idea to append to the file. That way, you can ensure that > you will be making them different, no matter what the initial > condition was, i.e., > > for opt in "" "-o" "--only" > do > test_expect_success 'skip over already added change' ' > echo more >>file && > echo more >>baz && > git add baz && > git commit $opt -m "file" file && > > ... ensure that changes to file are committed > ... and changes to baz is only in the index > ' > done > > let's you test all three combinations. Yeah, that is a more effective approach. I will change it and reroll quickly. > > Thanks. Thank you for all the help!
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > +test_expect_success '-i/--include includes staged changes' ' > + echo newcontent >file && > + echo newcontent >baz && > + git add file && > + git commit --include -m "file baz" baz && I may have said this already, but the command invocation that does not result in an error smells like a bug, and I doubt that we want to etch the current behaviour into stone, which may make it harder to fix [*]. Another related behaviour that I suspect is a bug is that if you did git add -u baz instead of this "git commit -i baz", I think the command will silently succeed without doing anything. They may be the same bug, because "git commit -i <pathspec>" is an equivalent to "git add -u <pathspec>" followed by "git commit" after all. Both should say "there is no such path to update that matches the pathspec <baz>" and error out, I suspect. Thanks. [Footnote] * A reasonable way out to unblock this particular patch may be to clarify that this test is only documenting the current behaviour without necessarily endorsing it. Perhaps echo more >>file && echo more >>baz && git add file && # Note: "git commit -i baz" is like "git add -u baz" # followed by "git commit" but because baz is untracked, # only "file" is committed. # This test only documents this current behaviour, which we # may want to fix, and when it happens, this needs to be # adjusted to the new behaviour. git commit -i -m "file and baz" baz && or something, probably.
On Sat Jan 13, 2024 at 6:46 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > > > +test_expect_success '-i/--include includes staged changes' ' > > + echo newcontent >file && > > + echo newcontent >baz && > > + git add file && > > + git commit --include -m "file baz" baz && > > I may have said this already, but the command invocation that does > not result in an error smells like a bug, and I doubt that we want > to etch the current behaviour into stone, which may make it harder > to fix [*]. Yeah, that is why baz is added in the index in the test before the one you quoted. And as I understand it, when everything is in the index, it works as intended. Therefore to not get in the way of amending this behaviour, no untracked files are being (attempted to be) committed in these tests (except 'fail to commit untracked files' test, but that is not what you quoted above). > Another related behaviour that I suspect is a bug is that if you did > > git add -u baz > > instead of this "git commit -i baz", I think the command will > silently succeed without doing anything. They may be the same bug, > because "git commit -i <pathspec>" is an equivalent to "git add -u > <pathspec>" followed by "git commit" after all. Both should say > "there is no such path to update that matches the pathspec <baz>" > and error out, I suspect. Yeah, just checked, that also succeeds silently. > Thanks. > > [Footnote] > > * A reasonable way out to unblock this particular patch may be to > clarify that this test is only documenting the current behaviour > without necessarily endorsing it. Perhaps > > echo more >>file && > echo more >>baz && > git add file && > > # Note: "git commit -i baz" is like "git add -u baz" > # followed by "git commit" but because baz is untracked, > # only "file" is committed. > # This test only documents this current behaviour, which we > # may want to fix, and when it happens, this needs to be > # adjusted to the new behaviour. > git commit -i -m "file and baz" baz && > > or something, probably. as stated above, baz is tracked from the previous test. In any case, I will add a note just to document that untracked files also do not give any error (when there are staged changes) but are not committed. Thanks.
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh index 3d8500a52e..e4633b4af5 100755 --- a/t/t7501-commit-basic-functionality.sh +++ b/t/t7501-commit-basic-functionality.sh @@ -3,7 +3,7 @@ # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com> # -# FIXME: Test the various index usages, -i and -o, test reflog, +# FIXME: Test the various index usages, test reflog, # signoff test_description='git commit' @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' ' test_must_fail git commit -m initial --long ' +test_expect_success 'fail to commit untracked file' ' + echo content >baz && + test_must_fail git commit -m "baz" baz +' + +test_expect_success '--only also fail to commit untracked file' ' + test_must_fail git commit --only -m "baz" baz +' + +test_expect_success '--include also fail to commit untracked file' ' + test_must_fail git commit --include -m "baz" baz +' + test_expect_success 'setup: non-initial commit' ' echo bongo bongo bongo >file && git commit -m next -a @@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' ' git commit -m next -a --long ' +test_expect_success 'only commit given path (also excluding additional staged changes)' ' + echo content >file && + echo content >baz && + git add baz && + git commit -m "file" file && + + git diff --name-only >actual && + test_must_be_empty actual && + + git diff --name-only --staged >actual && + test_cmp - actual <<-EOF && + baz + EOF + + git diff --name-only HEAD^ HEAD >actual && + test_cmp - actual <<-EOF + file + EOF +' + +test_expect_success 'same as above with -o/--only' ' + echo change >file && + echo change >baz && + git add baz && + git commit --only -m "file" file && + + git diff --name-only >actual && + test_must_be_empty actual && + + git diff --name-only --staged >actual && + test_cmp - actual <<-EOF && + baz + EOF + + git diff --name-only HEAD^ HEAD >actual && + test_cmp - actual <<-EOF + file + EOF +' + +test_expect_success '-i/--include includes staged changes' ' + echo newcontent >file && + echo newcontent >baz && + git add file && + git commit --include -m "file baz" baz && + + git diff --name-only HEAD >remaining && + test_must_be_empty remaining && + + git diff --name-only HEAD^ HEAD >changes && + test_cmp - changes <<-EOF + baz + file + EOF +' + +test_expect_success '--include and --only do not mix' ' + test_when_finished "git reset --hard" && + echo new >file && + echo new >baz && + test_must_fail git commit --include --only -m "file baz" file baz 2>actual && + test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual +' + test_expect_success 'commit message from non-existing file' ' echo more bongo: bongo bongo bongo bongo >file && test_must_fail git commit -F gah -a
Add tests for --only (-o) and --include (-i). This include testing with or without staged changes for both -i and -o. Also to test for committing untracked files with -i, -o and without -i/-o. Some tests already exist in t7501 for testing --only, however, it is tested in combination with --amend and --allow-empty and on to-be-born branch. The addition of these tests check, when the pathspec is provided, that only the files matching the pathspec get committed. This behavior is same when we provide --only (as --only is the default mode of operation when pathspec is provided.) As for --include, there is no prior test for checking if --include also commits staged changes. Therefore, these tests belong in t7501 with other similar existing tests, as described in the case of --only. And also add test for checking incompatibilty when using -o and -i together. Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- t/t7501-commit-basic-functionality.sh | 79 ++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-)