[PATCH v2 0/1] notes: fix editor invocation regression

[PATCH v2 0/1] notes: fix editor invocation regression

[David Disseldorp]

The following patch attempts to restore previous editor invocation
behavior for empty notes, which was inadvertantly changed via 90bc19b3ae
(notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27).
A new regression test is included, which passes prior to 90bc19b3ae and
after this fix.

Please cc me in any replies as I'm not subscribed.

Cheers, David

--

The following changes since commit ad57f148c6b5f8735b62238dda8f571c582e0e54:

  Git 2.46-rc2 (2024-07-23 16:54:35 -0700)

are available in the Git repository at:

  https://github.com/ddiss/git.git notes_empty_editor_add_v2

for you to fetch changes up to 63019aa2c7bbee5989944a990ca2c0229cef8ae2:

  notes: do not trigger editor when adding an empty note (2024-07-29 13:22:26 +0200)

----------------------------------------------------------------
David Disseldorp (1):
      notes: do not trigger editor when adding an empty note

 builtin/notes.c  | 22 ++++++++++------------
 t/t3301-notes.sh |  5 +++++
 2 files changed, 15 insertions(+), 12 deletions(-)

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

[David Disseldorp]

With "git notes add -C $blob", the given blob contents are to be made
into a note without involving an editor.  But when "--allow-empty" is
given, the editor is invoked, which can cause problems for
non-interactive callers[1].

This behaviour started with 90bc19b3ae (notes.c: introduce
'--separator=<paragraph-break>' option, 2023-05-27), which changed
editor invocation logic to check for a zero length note_data buffer.

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"

[1] https://github.com/ddiss/icyci/issues/12

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 builtin/notes.c  | 22 ++++++++++------------
 t/t3301-notes.sh |  5 +++++
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index d9c356e354..4cc5bfedc3 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -114,7 +114,6 @@ struct note_msg {
 };
 
 struct note_data {
-	int given;
 	int use_editor;
 	int stripspace;
 	char *edit_path;
@@ -193,7 +192,7 @@ static void write_commented_object(int fd, const struct object_id *object)
 static void prepare_note_data(const struct object_id *object, struct note_data *d,
 		const struct object_id *old_note)
 {
-	if (d->use_editor || !d->given) {
+	if (d->use_editor || !d->msg_nr) {
 		int fd;
 		struct strbuf buf = STRBUF_INIT;
 
@@ -201,7 +200,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
 		d->edit_path = git_pathdup("NOTES_EDITMSG");
 		fd = xopen(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
 
-		if (d->given)
+		if (d->msg_nr)
 			write_or_die(fd, d->buf.buf, d->buf.len);
 		else if (old_note)
 			copy_obj_to_fd(fd, old_note);
@@ -515,7 +514,6 @@ static int add(int argc, const char **argv, const char *prefix)
 
 	if (d.msg_nr)
 		concat_messages(&d);
-	d.given = !!d.buf.len;
 
 	object_ref = argc > 1 ? argv[1] : "HEAD";
 
@@ -528,7 +526,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (note) {
 		if (!force) {
 			free_notes(t);
-			if (d.given) {
+			if (d.msg_nr) {
 				free_note_data(&d);
 				return error(_("Cannot add notes. "
 					"Found existing notes for object %s. "
@@ -690,14 +688,14 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		usage_with_options(usage, options);
 	}
 
-	if (d.msg_nr)
+	if (d.msg_nr) {
 		concat_messages(&d);
-	d.given = !!d.buf.len;
-
-	if (d.given && edit)
-		fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
-			"for the 'edit' subcommand.\n"
-			"Please use 'git notes add -f -m/-F/-c/-C' instead.\n"));
+		if (edit)
+			fprintf(stderr, _("The -m/-F/-c/-C options have been "
+				"deprecated for the 'edit' subcommand.\n"
+				"Please use 'git notes add -f -m/-F/-c/-C' "
+				"instead.\n"));
+	}
 
 	object_ref = 1 < argc ? argv[1] : "HEAD";
 
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
-- 
2.43.0

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

[Junio C Hamano]

David Disseldorp <ddiss@suse.de> writes:

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

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

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

[David Disseldorp]

On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote:
...
> 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.

This looks good and works for me - feel free to squash it in.

Thanks, David

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

[Junio C Hamano]

David Disseldorp <ddiss@suse.de> writes:

> On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote:
> ...
>> I am tempted to squash in
>> ...
>> 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.
>
> This looks good and works for me - feel free to squash it in.

Will do.  Thanks for a quick response.