Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2024, 21:37 UTC
Message-ID: <xmqqed7bhobk.fsf@gitster.g>
In-Reply-To: <20240729151639.19192-2-ddiss@suse.de>

David Disseldorp <ddiss@suse.de> writes:

Show 7 quoted lines

> Restore the original behaviour of "git note" that takes the contents
> given via the "-m", "-C", "-F" options without invoking an editor, by
> checking for any prior parameter callbacks, indicated by a non-zero
> note_data.msg_nr.  Remove the now-unneeded note_data.given flag.
>
> Add a test for this regression by checking whether GIT_EDITOR is
> invoked alongside "git notes add -C $empty_blob --allow-empty"

Makes perfect sense.

A #leftoverbit that we may want to look into after this topic settles is to teach the "-e" option, similar to "git commit" and "git tag", so that messages seeded with -m/-F can still be edited. In a sense, supporting both "-c/-C" was unnecessary if these commands supported "-e" in the first place, and that mistake has been inherited from "git commit" and "git tag" into this command.

Show 14 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
> +'
> +
>  test_done

I am tempted to squash in

        diff --git i/t/t3301-notes.sh w/t/t3301-notes.sh
        index c0dbacc161..99137fb235 100755
        --- i/t/t3301-notes.sh
        +++ w/t/t3301-notes.sh
        @@ -1559,7 +1559,12 @@ test_expect_success 'empty notes are displayed by git log' '

         test_expect_success 'empty notes do not invoke the editor' '
                test_commit 18th &&
        -	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
        +	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty &&
        +	git notes remove HEAD &&
        +	GIT_EDITOR="false" git notes add -m "" --allow-empty &&
        +	git notes remove HEAD &&
        +	GIT_EDITOR="false" git notes add -F /dev/null --allow-empty &&
        +	git notes remove HEAD
         '

         test_done

to make sure that all three options mentioned in the proposed commit log message are tested. It is not too much more effort to do so.

Will queue.

Thanks.