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

> 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?