diff mbox series

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

Message ID 20240729151639.19192-2-ddiss@suse.de (mailing list archive)
State New, archived
Headers show
Series [v2,1/1] notes: do not trigger editor when adding an empty note | expand

Commit Message

David Disseldorp July 29, 2024, 3:14 p.m. UTC
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(-)

Comments

Junio C Hamano July 29, 2024, 9:37 p.m. UTC | #1
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 July 29, 2024, 9:53 p.m. UTC | #2
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 July 29, 2024, 10:50 p.m. UTC | #3
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.
diff mbox series

Patch

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