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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 25, 2024, 16:31 UTC
Message-ID: <xmqqle1pigbk.fsf@gitster.g>
In-Reply-To: <20240725144548.3434-2-ddiss@suse.de>

David Disseldorp <ddiss@suse.de> writes:

Show 11 quoted lines

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

Show 12 quoted lines

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