| Message ID | 20240725144548.3434-2-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] t3301-notes: check editor isn't invoked for empty notes add | expand |
Hi On Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote: > 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option") > changed note_data.given logic such that it's no longer set if a zero > length file or blob object is provided. This project uses the `git show -s --pretty=reference` format: 90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27) > t/t3301-notes.sh | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > index 536bd11ff4..c0dbacc161 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by > git log' ' > test_cmp expect actual > ' > > +test_expect_success 'empty notes do not invoke the editor' ' > + test_commit 18th && > + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty > +' > + > test_done > -- > 2.43.0 This test fails, obviously. Maybe you can reorder the patches so that both two patches pass the test suite? Introducing a regression test in one patch and then fixing the bug (and making the test pass) in the next patch is a style that some prefer. But I have received feedback before that it’s best to avoid that.
On Thu, 25 Jul 2024 17:52:25 +0200, Kristoffer Haugsbakk wrote: > Hi > > On Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote: > > 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option") > > changed note_data.given logic such that it's no longer set if a zero > > length file or blob object is provided. > > This project uses the `git show -s --pretty=reference` format: > > 90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>' > option, 2023-05-27) Okay, will fix in a subsequent version. > > t/t3301-notes.sh | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > > index 536bd11ff4..c0dbacc161 100755 > > --- a/t/t3301-notes.sh > > +++ b/t/t3301-notes.sh > > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by > > git log' ' > > test_cmp expect actual > > ' > > > > +test_expect_success 'empty notes do not invoke the editor' ' > > + test_commit 18th && > > + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty > > +' > > + > > test_done > > -- > > 2.43.0 > > This test fails, obviously. Maybe you can reorder the patches so that > both two patches pass the test suite? > > Introducing a regression test in one patch and then fixing the bug (and > making the test pass) in the next patch is a style that some prefer. But > I have received feedback before that it’s best to avoid that. Makes sense, thanks for the feedback.
David Disseldorp <ddiss@suse.de> writes: > 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option") > changed note_data.given logic such that it's no longer set if a zero > length file or blob object is provided. > > Add a test for this regression by checking whether GIT_EDITOR is > invoked. > > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > t/t3301-notes.sh | 5 +++++ > 1 file changed, 5 insertions(+) Having this test separate from 2/2 breaks bisectability. For a small test like this, it is often a lot more preferrable to squash it into the patch that updates the behaviour. It is easy to apply the whole thing, and when the reviewer/tester is curious, it is easy to tentatively "revert" only the behaviour changes out of the working tree to see how the original code behaved against the test to verify the alleged breakages were indeed there in the original. > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > index 536bd11ff4..c0dbacc161 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' ' > test_cmp expect actual > ' > > +test_expect_success 'empty notes do not invoke the editor' ' > + test_commit 18th && > + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty > +' Clever. By setting the editor to something that always fails, we will notice if the command invoked it, when we do not expect the editor to be used. Not questioning the usefulness of this fix, and not suggesting to remove the "--allow-empty" support out of the "git notes" command, but out of curiosity, what are these empty notes used for? Thanks.
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > Introducing a regression test in one patch and then fixing the bug (and > making the test pass) in the next patch is a style that some prefer. But > I have received feedback before that it’s best to avoid that. Especially for something very small like this topic, yes.
On Thu, 25 Jul 2024 09:31:27 -0700, Junio C Hamano wrote: > David Disseldorp <ddiss@suse.de> writes: > > > 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option") > > changed note_data.given logic such that it's no longer set if a zero > > length file or blob object is provided. > > > > Add a test for this regression by checking whether GIT_EDITOR is > > invoked. > > > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > --- > > t/t3301-notes.sh | 5 +++++ > > 1 file changed, 5 insertions(+) > > Having this test separate from 2/2 breaks bisectability. For a > small test like this, it is often a lot more preferrable to squash > it into the patch that updates the behaviour. It is easy to apply > the whole thing, and when the reviewer/tester is curious, it is easy > to tentatively "revert" only the behaviour changes out of the > working tree to see how the original code behaved against the test > to verify the alleged breakages were indeed there in the original. Understood. In that case I'll squash both patches together in the next version. > > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > > index 536bd11ff4..c0dbacc161 100755 > > --- a/t/t3301-notes.sh > > +++ b/t/t3301-notes.sh > > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' ' > > test_cmp expect actual > > ' > > > > +test_expect_success 'empty notes do not invoke the editor' ' > > + test_commit 18th && > > + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty > > +' > > Clever. By setting the editor to something that always fails, we > will notice if the command invoked it, when we do not expect the > editor to be used. > > Not questioning the usefulness of this fix, and not suggesting to > remove the "--allow-empty" support out of the "git notes" command, > but out of curiosity, what are these empty notes used for? icyci (https://github.com/ddiss/icyci) attaches test-suite stderr and stdout notes following test completion. There's no real value in attaching empty e.g. stderr notes, but tests can also provide their own arbitrary notes files and may wish to use (empty) note existence as a flag in results reporting.
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 536bd11ff4..c0dbacc161 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' ' test_cmp expect actual ' +test_expect_success 'empty notes do not invoke the editor' ' + test_commit 18th && + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty +' + test_done
90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option") changed note_data.given logic such that it's no longer set if a zero length file or blob object is provided. Add a test for this regression by checking whether GIT_EDITOR is invoked. Signed-off-by: David Disseldorp <ddiss@suse.de> --- t/t3301-notes.sh | 5 +++++ 1 file changed, 5 insertions(+)