threads / patch / 61859

v2notes: fix editor invocation regression

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

## tl;dr

5 messages between Jul 29, 2024 and Jul 29, 2024. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

David Disseldorp· Jul 29, 2024, 15:13 UTC · lore

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· Jul 29, 2024, 15:14 UTC · re: David Disseldorp · lore

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

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(-)
Show changes to 2 files +15 −12

builtin/notes.c, t/t3301-notes.sh

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· Jul 29, 2024, 21:37 UTC · re: David Disseldorp · lore

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

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.
David Disseldorp· Jul 29, 2024, 21:53 UTC · re: Junio C Hamano · lore

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

On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote: ...

Show 23 quoted lines
> 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· Jul 29, 2024, 22:50 UTC · re: David Disseldorp · lore

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

David Disseldorp <ddiss@suse.de> writes:
Show 8 quoted lines
> 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.

← back to recent threads