Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[David Disseldorp <ddiss@suse.de>]

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.