| Message ID | b8a418bc63ab0a4add25724a11eb5f992e3d4472.1631067429.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A pair of git am --abort issues | expand |
On 08/09/21 09.17, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh > index 9d8d3c72e7e..501a7a9d211 100755 > --- a/t/t4151-am-abort.sh > +++ b/t/t4151-am-abort.sh > @@ -23,7 +23,11 @@ test_expect_success setup ' > test_tick && > git commit -a -m $i || return 1 > done && > + git branch changes && > git format-patch --no-numbered initial && > + git checkout -b conflicting initial && > + echo different >>file-1 && > + git commit -a -m different && > git checkout -b side initial && > echo local change >file-2-expect > ' > @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' ' > git diff-files --exit-code --quiet > ' > > +test_expect_failure 'git am --abort return failed exit status when it fails' ' > + test_when_finished "rm -rf file-2/ && git reset --hard" && > + git checkout changes && > + git format-patch -1 --stdout conflicting >changes.mbox && > + test_must_fail git am --3way changes.mbox && > + > + git rm file-2 && > + mkdir file-2 && > + echo precious >file-2/somefile && > + test_must_fail git am --abort && > + test_path_is_dir file-2/ > +' > + > +test_expect_failure 'git am --abort returns us to a clean state' ' > + git checkout changes && > + git format-patch -1 --stdout conflicting >changes.mbox && > + test_must_fail git am --3way changes.mbox && > + > + # Make a change related to the rest of the am work > + echo related change >>file-2 && > + > + # Abort, and expect the related change to go away too > + git am --abort && > + git status --porcelain -uno >actual && > + test_must_be_empty actual > +' > + > test_done > I expect BUGS section in git-am(1) to be added to describe known bugs you demonstrated above, judging from the patch subject.
On Tue, Sep 7, 2021 at 10:13 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > On 08/09/21 09.17, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@gmail.com> > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh > > index 9d8d3c72e7e..501a7a9d211 100755 > > --- a/t/t4151-am-abort.sh > > +++ b/t/t4151-am-abort.sh > > @@ -23,7 +23,11 @@ test_expect_success setup ' > > test_tick && > > git commit -a -m $i || return 1 > > done && > > + git branch changes && > > git format-patch --no-numbered initial && > > + git checkout -b conflicting initial && > > + echo different >>file-1 && > > + git commit -a -m different && > > git checkout -b side initial && > > echo local change >file-2-expect > > ' > > @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' ' > > git diff-files --exit-code --quiet > > ' > > > > +test_expect_failure 'git am --abort return failed exit status when it fails' ' > > + test_when_finished "rm -rf file-2/ && git reset --hard" && > > + git checkout changes && > > + git format-patch -1 --stdout conflicting >changes.mbox && > > + test_must_fail git am --3way changes.mbox && > > + > > + git rm file-2 && > > + mkdir file-2 && > > + echo precious >file-2/somefile && > > + test_must_fail git am --abort && > > + test_path_is_dir file-2/ > > +' > > + > > +test_expect_failure 'git am --abort returns us to a clean state' ' > > + git checkout changes && > > + git format-patch -1 --stdout conflicting >changes.mbox && > > + test_must_fail git am --3way changes.mbox && > > + > > + # Make a change related to the rest of the am work > > + echo related change >>file-2 && > > + > > + # Abort, and expect the related change to go away too > > + git am --abort && > > + git status --porcelain -uno >actual && > > + test_must_be_empty actual > > +' > > + > > test_done > > > > I expect BUGS section in git-am(1) to be added to describe known bugs > you demonstrated above, judging from the patch subject. There's no point documenting the first one since it'll be fixed by the next patch. As for the second, as I noted in my cover letter, I'm not quite sure that it really is a bug. If it isn't, the second testcase should be dropped. However, if the second testcase represents an actual bug rather than me just misjudging the intent, then your suggestion certainly makes sense.
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > +test_expect_failure 'git am --abort returns us to a clean state' ' > + git checkout changes && > + git format-patch -1 --stdout conflicting >changes.mbox && > + test_must_fail git am --3way changes.mbox && > + > + # Make a change related to the rest of the am work > + echo related change >>file-2 && > + > + # Abort, and expect the related change to go away too > + git am --abort && > + git status --porcelain -uno >actual && > + test_must_be_empty actual This test makes me worried. It is perfectly normal for "am" to be asked to work in a dirty working tree as long as the index is clean and the working tree files that are involved in the patch are unmodified. Even though you may want "am --abort" to restore the paths that the operation touched to their original state, I am not sure if that is always possible, given that there may have been dirty working tree files to begin with. And the above test would succeed if "git am --abort" internally called "git reset --hard", which definitely is not what we want to see. We want the local changes in dirty working tree files that weren't involved in the patch application to stay, even after running "am --abort".
On Wed, Sep 8, 2021 at 12:02 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > +test_expect_failure 'git am --abort returns us to a clean state' ' > > + git checkout changes && > > + git format-patch -1 --stdout conflicting >changes.mbox && > > + test_must_fail git am --3way changes.mbox && > > + > > + # Make a change related to the rest of the am work > > + echo related change >>file-2 && > > + > > + # Abort, and expect the related change to go away too > > + git am --abort && > > + git status --porcelain -uno >actual && > > + test_must_be_empty actual > > This test makes me worried. It is perfectly normal for "am" to be > asked to work in a dirty working tree as long as the index is clean > and the working tree files that are involved in the patch are > unmodified. Ah, I think I am just too used to rebase where it refuses to start if the working tree isn't clean, assumed the same with am (which I don't use that much), and then projected from there. I'll drop the second test; thanks for the explanation.
Elijah Newren <newren@gmail.com> writes: > On Wed, Sep 8, 2021 at 12:02 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >> > +test_expect_failure 'git am --abort returns us to a clean state' ' >> > + git checkout changes && >> > + git format-patch -1 --stdout conflicting >changes.mbox && >> > + test_must_fail git am --3way changes.mbox && >> > + >> > + # Make a change related to the rest of the am work >> > + echo related change >>file-2 && >> > + >> > + # Abort, and expect the related change to go away too >> > + git am --abort && >> > + git status --porcelain -uno >actual && >> > + test_must_be_empty actual >> >> This test makes me worried. It is perfectly normal for "am" to be >> asked to work in a dirty working tree as long as the index is clean >> and the working tree files that are involved in the patch are >> unmodified. > > Ah, I think I am just too used to rebase where it refuses to start if > the working tree isn't clean, assumed the same with am (which I don't > use that much), and then projected from there. > > I'll drop the second test; thanks for the explanation. Actually, if you test that unrelated dirty files are kept, then the test is a welcome addition. "returns us to a 'clean' state" needs a bit different title, though. Thanks.
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh index 9d8d3c72e7e..501a7a9d211 100755 --- a/t/t4151-am-abort.sh +++ b/t/t4151-am-abort.sh @@ -23,7 +23,11 @@ test_expect_success setup ' test_tick && git commit -a -m $i || return 1 done && + git branch changes && git format-patch --no-numbered initial && + git checkout -b conflicting initial && + echo different >>file-1 && + git commit -a -m different && git checkout -b side initial && echo local change >file-2-expect ' @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' ' git diff-files --exit-code --quiet ' +test_expect_failure 'git am --abort return failed exit status when it fails' ' + test_when_finished "rm -rf file-2/ && git reset --hard" && + git checkout changes && + git format-patch -1 --stdout conflicting >changes.mbox && + test_must_fail git am --3way changes.mbox && + + git rm file-2 && + mkdir file-2 && + echo precious >file-2/somefile && + test_must_fail git am --abort && + test_path_is_dir file-2/ +' + +test_expect_failure 'git am --abort returns us to a clean state' ' + git checkout changes && + git format-patch -1 --stdout conflicting >changes.mbox && + test_must_fail git am --3way changes.mbox && + + # Make a change related to the rest of the am work + echo related change >>file-2 && + + # Abort, and expect the related change to go away too + git am --abort && + git status --porcelain -uno >actual && + test_must_be_empty actual +' + test_done