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

[Junio C Hamano <gitster@pobox.com>]

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.