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

5 messages from 2024-07-29 to 2024-07-29. Participants: David Disseldorp, Junio C Hamano.
Thread: https://gitlist.dev/t/61859

## David Disseldorp, 2024-07-29 15:13

Subject: [PATCH v2 0/1] notes: fix editor invocation regression
Message-ID: <20240729151639.19192-1-ddiss@suse.de>
URL: https://gitlist.dev/e/20240729151639.19192-1-ddiss%40suse.de

```
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(-)

```

## David Disseldorp, 2024-07-29 15:14

Subject: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note
Message-ID: <20240729151639.19192-2-ddiss@suse.de>
URL: https://gitlist.dev/e/20240729151639.19192-2-ddiss%40suse.de
In-Reply-To: <20240729151639.19192-1-ddiss@suse.de>

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


```

## Junio C Hamano, 2024-07-29 21:37

Subject: Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note
Message-ID: <xmqqed7bhobk.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqed7bhobk.fsf%40gitster.g
In-Reply-To: <20240729151639.19192-2-ddiss@suse.de>

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


```

## David Disseldorp, 2024-07-29 21:53

Subject: Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note
Message-ID: <20240729235333.513eb1d9.ddiss@suse.de>
URL: https://gitlist.dev/e/20240729235333.513eb1d9.ddiss%40suse.de
In-Reply-To: <xmqqed7bhobk.fsf@gitster.g>

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

```

## Junio C Hamano, 2024-07-29 22:50

Subject: Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note
Message-ID: <xmqqa5hzhkye.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqa5hzhkye.fsf%40gitster.g
In-Reply-To: <20240729235333.513eb1d9.ddiss@suse.de>

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

```