Skip to contentSkip to conversation

notes: teach the -e option to edit messages in editor

PATCH Superseded Samuel Adekunle Abraham via GitGitGadget 7 participants 22 messages Oct 19, 2024 – Oct 23, 2024 Documentation

lore
Share & tools
lore

Samuel Adekunle Abraham via GitGitGadget

author
#0 lore raw

PATCH 1/1 notes: teach the -e option to edit messages in editor

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message), -C (copy a note from a blob object) or
-F (read the note from a file) options.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

---
[Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor

Notes can be added to a commit using the -m (message), -C (copy a note from a blob object) or -F (read the note from a file) options. When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after they have been provided through -[mF].

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1817

 builtin/notes.c  |  4 ++++
 t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)
builtin/notes.c +4 -0
index 8c26e455269..02cdfdf1c9d 100644
@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),

@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT_CALLBACK_F(0, "separator", &separator,
t/t3301-notes.sh +29 -0
index 99137fb2357..7f45a324faa 100755
@@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' '
 	git notes remove HEAD
 '
 
+test_expect_success '"git notes add" with -m/-F invokes the editor with -e' '
+	test_commit 19th &&
+	GIT_EDITOR="true" git notes add -m "note message" -e &&
+	git notes remove HEAD &&
+	echo "message from file" >file_1 &&
+	GIT_EDITOR="true" git notes add -F file_1 -e &&
+	git notes remove HEAD
+'
+
+test_expect_success 'git notes append with -m/-F invokes editor with -e' '
+	test_commit 20th &&
+	GIT_EDITOR="true" git notes add -m "initial note" -e &&
+	GIT_EDITOR="true" git notes append -m "appended note" -e &&
+	git notes remove HEAD &&
+	echo  "initial note" >note_a &&
+	echo "appended note" >note_b &&
+	GIT_EDITOR="true" git notes add -F note_a -e &&
+	GIT_EDITOR="true" git notes append -F note_b -e &&
+	git notes remove HEAD
+'
+
+test_expect_success 'append note with multiple combinations of -m, -F and -e, invokes editor' '
+	test_commit 21st &&
+	echo "foo-file-1" >note_1 &&
+	echo "foo-file-2" >note_2 &&
+	GIT_EDITOR="true" git notes append -F note_1 -m "message-1" -F note_2 -m "message-2" -e &&
+	git notes remove HEAD
+'
+
 test_done

base-commit: 15030f9556f545b167b1879b877a5d780252dc16

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 9 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message),

(quoted diff) +0 -2
-C (copy a note from a blob object) or
-F (read the note from a file) options.

When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Quoted 3 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted after the messages have been provided through -[mF].

I don't use the notes feature, but I definitely see how this is valuable there much as it is for git commit.

Quoted 28 lines
builtin/notes.c +4 -0
index 8c26e455269..02cdfdf1c9d 100644
@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),

@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT_CALLBACK_F(0, "separator", &separator,
t/t3301-notes.sh +0 -0
index 99137fb2357..7f45a324faa 100755
@@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' '
 	git notes remove HEAD

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1577

Quoted 10 lines
t/t3301-notes.sh +8 -0
 '
 
+test_expect_success '"git notes add" with -m/-F invokes the editor with -e' '
+	test_commit 19th &&
+	GIT_EDITOR="true" git notes add -m "note message" -e &&
+	git notes remove HEAD &&
+	echo "message from file" >file_1 &&
+	GIT_EDITOR="true" git notes add -F file_1 -e &&
+	git notes remove HEAD
+'

Maybe I don't understand what this is supposed to be testing (and if so, please correct me), but how are we verifying that the editor is being invoked? If we're invoking true, then that doesn't change the message in any way, so if we suddenly stopped invoking the editor, I don't think this would fail.

Maybe we could use something else as GIT_EDITOR instead. For example, if we did GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e (with an appropriate PERL prerequisite), then we could test that the message after the fact was "message from editor", which would help us verify that both the -F and -e options were being honoured. (Similar things can be said about the tests you added below this as well.)

I suggest Perl here because sed -i is nonstandard and not portable, but you could also set up a fake editor script as in t7004, which would avoid the need for the Perl dependency by using sed with a temporary file. That might be more palatable to the project at large, but I'd be fine with either approach.

Do you think there are any cases where testing the --no-edit functionality might be helpful? For example, is git notes edit ever useful to invoke with such an option, like one might do with git commit amend? (This isn't rhetorical, since the notes code is one of the areas of Git I'm least familiar with, so I ask both because I'm curious and if you think it's a useful thing to do, then tests might be a good idea.)

Signature · 2 lines
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
#2 lore raw

Hi brian and Samuel

Quoted 15 lines · brian m. carlson

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1577

Quoted 8 lines · Samuel Adekunle Abraham via GitGitGadget
t/t3301-notes.sh +8 -0
+test_expect_success '"git notes add" with -m/-F invokes the editor with -e' '
+	test_commit 19th &&
+	GIT_EDITOR="true" git notes add -m "note message" -e &&
+	git notes remove HEAD &&
+	echo "message from file" >file_1 &&
+	GIT_EDITOR="true" git notes add -F file_1 -e &&
+	git notes remove HEAD
+'

Maybe I don't understand what this is supposed to be testing (and if so, please correct me), but how are we verifying that the editor is being invoked? If we're invoking true, then that doesn't change the message in any way, so if we suddenly stopped invoking the editor, I don't think this would fail.

I also didn’t understand these tests.

There is this test in this file/test suite which tests the negative case:

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

And this works because the commands would fail if the editor was invoked:

error: there was a problem with the editor 'false'

But this doesn’t work for true.

Quoted 7 lines

Maybe we could use something else as GIT_EDITOR instead. For example, if we did GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e (with an appropriate PERL prerequisite), then we could test that the message after the fact was "message from editor", which would help us verify that both the -F and -e options were being honoured. (Similar things can be said about the tests you added below this as well.)

This file defines a fake_editor:[1]

    write_script fake_editor <<\EOF
    echo "$MSG" >"$1"
    echo "$MSG" >&2
    EOF
    GIT_EDITOR=./fake_editor
    export GIT_EDITOR

And it looks like this is how it is used:

    test_expect_success 'create notes' '
            MSG=b4 git notes add &&
            test_path_is_missing .git/NOTES_EDITMSG &&
            git ls-tree -r refs/notes/commits >actual &&
            test_line_count = 1 actual &&
            echo b4 >expect &&
            git notes show >actual &&
            test_cmp expect actual &&
            git show HEAD^ &&
            test_must_fail git notes show HEAD^
    '

So it seems that the new tests here should use the test_cmp expect actual style.

† 1: The different test files use both fake_editor, fake-editor,
and fakeeditor.

Quoted 6 lines

Do you think there are any cases where testing the --no-edit functionality might be helpful? For example, is git notes edit ever useful to invoke with such an option, like one might do with git commit amend? (This isn't rhetorical, since the notes code is one of the areas of Git I'm least familiar with, so I ask both because I'm curious and if you think it's a useful thing to do, then tests might be a good idea.)

Yes, that is useful (both as a use-case and as a regression test[2]). git-notes(1) is often used to programmatically add metadata:

git show todo:post-applypatch | grep -C5 refs/notes/amlog

(And this non-interactive example is not affected by this change since -e is required in order to invoke the editor)

† 2: I seem to recall a regression in how git-notes(1) chose to invoke
the editor or not

#3 lore raw
Quoted 25 lines · Kristoffer Haugsbakk

Hi brian and Samuel

Quoted 10 lines · brian m. carlson

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1577

Quoted 8 lines · Samuel Adekunle Abraham via GitGitGadget
t/t3301-notes.sh +8 -0
+test_expect_success '"git notes add" with -m/-F invokes the editor with -e' '
+    test_commit 19th &&
+    GIT_EDITOR="true" git notes add -m "note message" -e &&
+    git notes remove HEAD &&
+    echo "message from file" >file_1 &&
+    GIT_EDITOR="true" git notes add -F file_1 -e &&
+    git notes remove HEAD
+'
Quoted 5 lines

Maybe I don't understand what this is supposed to be testing (and if so, please correct me), but how are we verifying that the editor is being invoked? If we're invoking true, then that doesn't change the message in any way, so if we suddenly stopped invoking the editor, I don't think this would fail.

I also didn’t understand these tests.

There is this test in this file/test suite which tests the negative case:

Quoted 10 lines
    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 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
    '

Thank you Kristoffer,

Yes incorrectly used this as a reference and I have however look deeper into the implementation
of the write_scripts function and the fake_editor file for better understanding.

Quoted 37 lines

And this works because the commands would fail if the editor was invoked:

error: there was a problem with the editor 'false'

But this doesn’t work for true.

Quoted 7 lines

Maybe we could use something else as GIT_EDITOR instead. For example, if we did GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e (with an appropriate PERL prerequisite), then we could test that the message after the fact was "message from editor", which would help us verify that both the -F and -e options were being honoured. (Similar things can be said about the tests you added below this as well.)

This file defines a fake_editor:[1]

    write_script fake_editor <<\EOF
    echo "$MSG" >"$1"
    echo "$MSG" >&2
    EOF
    GIT_EDITOR=./fake_editor
    export GIT_EDITOR

And it looks like this is how it is used:

    test_expect_success 'create notes' '
            MSG=b4 git notes add &&
            test_path_is_missing .git/NOTES_EDITMSG &&
            git ls-tree -r refs/notes/commits >actual &&
            test_line_count = 1 actual &&
            echo b4 >expect &&
            git notes show >actual &&
            test_cmp expect actual &&
            git show HEAD^ &&
            test_must_fail git notes show HEAD^
    '
Quoted 2 lines

So it seems that the new tests here should use the test_cmp expect actual style.

Thank you very much for the guide.
I will correct them and send a modified patch.

Quoted 21 lines

† 1: The different test files use both fake_editor, fake-editor,
and fakeeditor.

Quoted 6 lines

Do you think there are any cases where testing the --no-edit functionality might be helpful? For example, is git notes edit ever useful to invoke with such an option, like one might do with git commit amend? (This isn't rhetorical, since the notes code is one of the areas of Git I'm least familiar with, so I ask both because I'm curious and if you think it's a useful thing to do, then tests might be a good idea.)

Yes, that is useful (both as a use-case and as a regression test[2]). git-notes(1) is often used to programmatically add metadata:

git show todo:post-applypatch | grep -C5 refs/notes/amlog

(And this non-interactive example is not affected by this change since -e is required in order to invoke the editor)

† 2: I seem to recall a regression in how git-notes(1) chose to invoke
the editor or not

#4 lore raw
In reply to Samuel Adekunle Abraham via GitGitGadget

Hi Samuel

Quoted 8 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message),

(quoted diff) +0 -2
-C (copy a note from a blob object) or
-F (read the note from a file) options.

When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Thanks for this work. I think part of the motivation here is to make git-notes(1) act more in line with the conventions from git-commit(1), which is always nice to see.

It’s also useful in its own right.

Quoted 3 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted after the messages have been provided through -[mF].

Here you explain how the end-user will benefit from this change. Nice.

It’s important to explain the background, what is being done, and why it is being done. And this commit message does all of that.

#5 lore raw
Quoted 19 lines · Kristoffer Haugsbakk

Hi Samuel

Quoted 8 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message),

(quoted diff) +0 -2
-C (copy a note from a blob object) or
-F (read the note from a file) options.

When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Thanks for this work. I think part of the motivation here is to make git-notes(1) act more in line with the conventions from git-commit(1), which is always nice to see.

It’s also useful in its own right.

Quoted 8 lines
Quoted 3 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted after the messages have been provided through -[mF].

Here you explain how the end-user will benefit from this change. Nice.

It’s important to explain the background, what is being done, and why it is being done. And this commit message does all of that.

Hello Kristoffer,
Thank you very much for your response.

#6 lore raw
In reply to Samuel Adekunle Abraham via GitGitGadget
Quoted 3 lines · Samuel Adekunle Abraham via GitGitGadget
 builtin/notes.c  |  4 ++++
 t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

The documentation should be updated:

Documentation/git-notes.txt

builtin/notes.c static int add(int argc, const char **argv, const · line 494

Quoted 11 lines
builtin/notes.c +2 -0
index 8c26e455269..02cdfdf1c9d 100644
@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const
 char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),

The add subcommand does what I expect it to after some testing.

Quoted 12 lines
builtin/notes.c +0 -0
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
builtin/notes.c +2 -0
 			parse_reuse_arg),

@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv,
 const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
Quoted 1 line

OPT_CALLBACK_F(0, "separator", &separator,

Likewise for the append subcommand.

Signature · 1 line
Kristoffer Haugsbakk
#7 lore raw
Quoted 40 lines · Kristoffer Haugsbakk
Quoted 3 lines · Samuel Adekunle Abraham via GitGitGadget
 builtin/notes.c  |  4 ++++
 t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

The documentation should be updated:

Documentation/git-notes.txt

builtin/notes.c static int add(int argc, const char **argv, const · line 494

Quoted 11 lines
builtin/notes.c +2 -0
index 8c26e455269..02cdfdf1c9d 100644
@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const
 char *prefix)
              OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
                      N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
                      parse_reedit_arg),
+             OPT_BOOL('e', "edit", &d.use_editor,
+                     N_("edit note message in editor")),

The add subcommand does what I expect it to after some testing.

Quoted 12 lines
builtin/notes.c +0 -0
              OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
                      N_("reuse specified note object"), PARSE_OPT_NONEG,

parse_reuse_arg),

builtin/notes.c +2 -0
@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv,
 const char *prefix)
              OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
                      N_("reuse specified note object"), PARSE_OPT_NONEG,
                      parse_reuse_arg),
+             OPT_BOOL('e', "edit", &d.use_editor,
+                     N_("edit note message in editor")),
              OPT_BOOL(0, "allow-empty", &allow_empty,
                      N_("allow storing empty note")),
Quoted 1 line

OPT_CALLBACK_F(0, "separator", &separator,

Likewise for the append subcommand.

Quoted 3 lines

--
Kristoffer Haugsbakk

Okay, I will do that. Thank you very much, Kristoffer.

author
#8 lore raw

PATCH v2 1/1 notes: teach the -e option to edit messages in editor

In reply to Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message), -C (copy a note from a blob object) or
-F (read the note from a file) options.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

---
[Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor

Notes can be added to a commit using the -m (message), -C (copy a note from a blob object) or -F (read the note from a file) options. When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after they have been provided through -[mF].

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1817

Range-diff vs v1:

1: 61a6d2dbcb9 ! 1: 55804cd1269 notes: teach the -e option to edit messages in editor

     @@ Commit message
      
          Signed-off-by: Abraham Samuel Adekunle <[email protected]>
      
     + ## Documentation/git-notes.txt ##
     +@@ Documentation/git-notes.txt: SYNOPSIS
     + --------
     + [verse]
     + 'git notes' [list [<object>]]
     +-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
     ++'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
     + 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
     +-'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
     ++'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
     + 'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
     + 'git notes' show [<object>]
     + 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
     +@@ Documentation/git-notes.txt: add::
     + 	the existing notes will be opened in the editor (like the `edit`
     + 	subcommand). If you specify multiple `-m` and `-F`, a blank
     + 	line will be inserted between the messages. Use the `--separator`
     +-	option to insert other delimiters.
     ++	option to insert other delimiters. You can use `-e` to edit and
     ++	fine-tune the message(s) supplied from `-m` and `-F` options
     ++	interactively (using an editor) before adding the note.
     + 
     + copy::
     + 	Copy the notes for the first object onto the second object (defaults to
     +@@ Documentation/git-notes.txt: append::
     + 	an existing note, a blank line is added before each new
     + 	message as an inter-paragraph separator.  The separator can
     + 	be customized with the `--separator` option.
     ++	Edit the notes to be appended given by `-m` and `-F` options with
     ++	`-e` interactively (using an editor) before appending the note.
     + 
     + edit::
     + 	Edit the notes for a given object (defaults to HEAD).
     +
       ## builtin/notes.c ##
     +@@
     + static const char *separator = "\n";
     + static const char * const git_notes_usage[] = {
     + 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
     +-	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
     ++	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
     + 	N_("git notes [--ref <notes-ref>] copy [-f] <from-object> <to-object>"),
     +-	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
     ++	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
     + 	N_("git notes [--ref <notes-ref>] edit [--allow-empty] [<object>]"),
     + 	N_("git notes [--ref <notes-ref>] show [<object>]"),
     + 	N_("git notes [--ref <notes-ref>] merge [-v | -q] [-s <strategy>] <notes-ref>"),
      @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
       		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
       			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
     @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '
       	git notes remove HEAD
       '
       
     -+test_expect_success '"git notes add" with -m/-F invokes the editor with -e' '
     ++test_expect_success 'git notes add with -m/-F invokes editor with -e' '
      +	test_commit 19th &&
     -+	GIT_EDITOR="true" git notes add -m "note message" -e &&
     ++	MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
     ++	echo "Edited notes message" >expect &&
     ++	git notes show >actual &&
     ++	test_cmp expect actual &&
      +	git notes remove HEAD &&
     -+	echo "message from file" >file_1 &&
     -+	GIT_EDITOR="true" git notes add -F file_1 -e &&
     -+	git notes remove HEAD
     ++
     ++	# Add a note using -F and edit it
     ++	echo "Note from file" >note_file &&
     ++	MSG="Edited note from file" git notes add -F note_file -e &&
     ++	echo "Edited note from file" >expect &&
     ++	git notes show >actual &&
     ++	test_cmp expect actual
      +'
      +
     -+test_expect_success 'git notes append with -m/-F invokes editor with -e' '
     ++test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
      +	test_commit 20th &&
     -+	GIT_EDITOR="true" git notes add -m "initial note" -e &&
     -+	GIT_EDITOR="true" git notes append -m "appended note" -e &&
     ++	git notes add -m "Initial note message" &&
     ++	MSG="Appended edited note message" git notes append -m "New appended note" -e &&
     ++
     ++	# Verify the note content was appended and edited
     ++	echo "Initial note message" >expect &&
     ++	echo "" >>expect &&
     ++	echo "Appended edited note message" >>expect &&
     ++	git notes show >actual &&
     ++	test_cmp expect actual &&
      +	git notes remove HEAD &&
     -+	echo  "initial note" >note_a &&
     -+	echo "appended note" >note_b &&
     -+	GIT_EDITOR="true" git notes add -F note_a -e &&
     -+	GIT_EDITOR="true" git notes append -F note_b -e &&
     -+	git notes remove HEAD
     ++
     ++	#Append a note using -F and edit it
     ++	echo "Note from file" >note_file &&
     ++	git notes add -m "Initial note message" &&
     ++	MSG="Appended edited note from file" git notes append -F note_file -e &&
     ++
     ++	# Verify notes from file has been edited in editor and appended
     ++	echo "Initial note message" >expect &&
     ++	echo "" >>expect &&
     ++	echo "Appended edited note from file" >>expect &&
     ++	git notes show >actual &&
     ++	test_cmp expect actual
      +'
      +
     -+test_expect_success 'append note with multiple combinations of -m, -F and -e, invokes editor' '
     ++test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '
      +	test_commit 21st &&
      +	echo "foo-file-1" >note_1 &&
      +	echo "foo-file-2" >note_2 &&
     -+	GIT_EDITOR="true" git notes append -F note_1 -m "message-1" -F note_2 -m "message-2" -e &&
     -+	git notes remove HEAD
     ++
     ++	MSG="Collapsed edited notes" git notes append -F note_1 -m "message-1" -F note_2 -e &&
     ++
     ++	# Verify that combined messages from file and -m have been edited
     ++
     ++	echo "Collapsed edited notes" >expect &&
     ++	git notes show >actual &&
     ++	test_cmp expect actual
      +'
      +
       test_done
 Documentation/git-notes.txt | 10 +++++--
 builtin/notes.c             |  8 ++++--
 t/t3301-notes.sh            | 56 +++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 5 deletions(-)
Documentation/git-notes.txt +7 -3
index c9221a68cce..d5505a426aa 100644
@@ -9,9 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
-'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
 'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>

@@ -67,7 +67,9 @@ add::
 	the existing notes will be opened in the editor (like the `edit`
 	subcommand). If you specify multiple `-m` and `-F`, a blank
 	line will be inserted between the messages. Use the `--separator`
-	option to insert other delimiters.
+	option to insert other delimiters. You can use `-e` to edit and
+	fine-tune the message(s) supplied from `-m` and `-F` options
+	interactively (using an editor) before adding the note.
 
 copy::
 	Copy the notes for the first object onto the second object (defaults to

@@ -93,6 +95,8 @@ append::
 	an existing note, a blank line is added before each new
 	message as an inter-paragraph separator.  The separator can
 	be customized with the `--separator` option.
+	Edit the notes to be appended given by `-m` and `-F` options with
+	`-e` interactively (using an editor) before appending the note.
 
 edit::
 	Edit the notes for a given object (defaults to HEAD).
builtin/notes.c +6 -2
index 8c26e455269..72c8a51cfac 100644
@@ -32,9 +32,9 @@
 static const char *separator = "\n";
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
-	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
 	N_("git notes [--ref <notes-ref>] copy [-f] <from-object> <to-object>"),
-	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
 	N_("git notes [--ref <notes-ref>] edit [--allow-empty] [<object>]"),
 	N_("git notes [--ref <notes-ref>] show [<object>]"),
 	N_("git notes [--ref <notes-ref>] merge [-v | -q] [-s <strategy>] <notes-ref>"),

@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),

@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT_CALLBACK_F(0, "separator", &separator,
t/t3301-notes.sh +56 -0

60 lines not on this page · raw message

base-commit: 15030f9556f545b167b1879b877a5d780252dc16

Signature · 1 line
gitgitgadget
#9 lore raw

Review

Quoted 5 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message),

(quoted diff) +0 -2
-C (copy a note from a blob object) or
-F (read the note from a file) options.

Nit: this would read a bit better if this was a bulleted list, I think. E.g.:

Notes can be added to a commit using:

- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.

When these options are used, ...

Quoted 6 lines

When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted

s/editted/edited

Quoted 6 lines
Documentation/git-notes.txt +0 -0
index c9221a68cce..d5505a426aa 100644
@@ -9,9 +9,9 @@ SYNOPSIS
 --------

Documentation/git-notes.txt SYNOPSIS · line 17

Quoted 10 lines
Documentation/git-notes.txt +2 -2
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
-'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
 'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>

Nit: I'd move the [-e] before [<object>] so that -F, -C and -m are all close together.

Quoted 14 lines
builtin/notes.c +2 -0
index 8c26e455269..72c8a51cfac 100644
@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),

builtin/notes.c static int append_edit(int argc, const char **argv, const char *prefix) · line 676

Quoted 8 lines
builtin/notes.c +2 -0
@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT_CALLBACK_F(0, "separator", &separator,

Nice.

Quoted 13 lines
t/t3301-notes.sh +5 -0
index 99137fb2357..ffa1d21671d 100755
@@ -1567,4 +1567,60 @@ test_expect_success 'empty notes do not invoke the editor' '
 	git notes remove HEAD
 '
 
+test_expect_success 'git notes add with -m/-F invokes editor with -e' '
+	test_commit 19th &&
+	MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
+	echo "Edited notes message" >expect &&
+	git notes show >actual &&

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1584

Quoted 10 lines
t/t3301-notes.sh +10 -0
+	test_cmp expect actual &&
+	git notes remove HEAD &&
+
+	# Add a note using -F and edit it
+	echo "Note from file" >note_file &&
+	MSG="Edited note from file" git notes add -F note_file -e &&
+	echo "Edited note from file" >expect &&
+	git notes show >actual &&
+	test_cmp expect actual
+'

I was surprised at first why the MSG ended up in the commit message. But the setup of t3301 writes a fake editor that listens to this environment variable, so this looks good to me.

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1594

Quoted 9 lines
t/t3301-notes.sh +9 -0
+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
+	test_commit 20th &&
+	git notes add -m "Initial note message" &&
+	MSG="Appended edited note message" git notes append -m "New appended note" -e &&
+
+	# Verify the note content was appended and edited
+	echo "Initial note message" >expect &&
+	echo "" >>expect &&
+	echo "Appended edited note message" >>expect &&

When you want to write multiple lines we typically use HERE docs. E.g.:

cat >expect <<-EOF &&
Initial note message

Appended edited note message
EOF

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1599

Quoted 5 lines
t/t3301-notes.sh +5 -0
+	git notes show >actual &&
+	test_cmp expect actual &&
+	git notes remove HEAD &&
+
+	#Append a note using -F and edit it

There should be a space after the "#" here.

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1607

Quoted 8 lines
t/t3301-notes.sh +8 -0
+	echo "Note from file" >note_file &&
+	git notes add -m "Initial note message" &&
+	MSG="Appended edited note from file" git notes append -F note_file -e &&
+
+	# Verify notes from file has been edited in editor and appended
+	echo "Initial note message" >expect &&
+	echo "" >>expect &&
+	echo "Appended edited note from file" >>expect &&

Same comment here.

Quoted 9 lines
t/t3301-notes.sh +9 -0
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '
+	test_commit 21st &&
+	echo "foo-file-1" >note_1 &&
+	echo "foo-file-2" >note_2 &&
+

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1626

Quoted 10 lines
t/t3301-notes.sh +9 -0
+	MSG="Collapsed edited notes" git notes append -F note_1 -m "message-1" -F note_2 -e &&
+
+	# Verify that combined messages from file and -m have been edited
+
+	echo "Collapsed edited notes" >expect &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
 test_done

It would be nice to have another test that uses EDITOR=false to demonstrate that we abort when the editor returns an error.

Other than that this patch looks good to me, thanks a lot!

Patrick

#10 lore raw
Quoted 11 lines · Patrick Steinhardt

Review

Quoted 5 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message),

(quoted diff) +0 -2
-C (copy a note from a blob object) or
-F (read the note from a file) options.

Nit: this would read a bit better if this was a bulleted list, I think. E.g.:

Quoted 7 lines

Notes can be added to a commit using:

- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.

When these options are used, ...

Thank you Patrick. Noted.

Quoted 9 lines
Quoted 7 lines

When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and editted

s/editted/edited

Okay.

Quoted 130 lines
Quoted 7 lines
Documentation/git-notes.txt +0 -0
index c9221a68cce..d5505a426aa 100644
@@ -9,9 +9,9 @@ SYNOPSIS
 --------

Documentation/git-notes.txt SYNOPSIS · line 17

Quoted 10 lines
Documentation/git-notes.txt +2 -2
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
-'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
 'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>

Nit: I'd move the [-e] before [<object>] so that -F, -C and -m are all close together.

Quoted 14 lines
builtin/notes.c +2 -0
index 8c26e455269..72c8a51cfac 100644
@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
              OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
                      N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
                      parse_reedit_arg),
+             OPT_BOOL('e', "edit", &d.use_editor,
+                     N_("edit note message in editor")),
              OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
                      N_("reuse specified note object"), PARSE_OPT_NONEG,
                      parse_reuse_arg),

builtin/notes.c static int append_edit(int argc, const char **argv, const char *prefix) · line 676

Quoted 8 lines
builtin/notes.c +2 -0
@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
              OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
                      N_("reuse specified note object"), PARSE_OPT_NONEG,
                      parse_reuse_arg),
+             OPT_BOOL('e', "edit", &d.use_editor,
+                     N_("edit note message in editor")),
              OPT_BOOL(0, "allow-empty", &allow_empty,
                      N_("allow storing empty note")),
              OPT_CALLBACK_F(0, "separator", &separator,

Nice.

Quoted 13 lines
t/t3301-notes.sh +5 -0
index 99137fb2357..ffa1d21671d 100755
@@ -1567,4 +1567,60 @@ test_expect_success 'empty notes do not invoke the editor' '
      git notes remove HEAD
 '
 
+test_expect_success 'git notes add with -m/-F invokes editor with -e' '
+     test_commit 19th &&
+     MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
+     echo "Edited notes message" >expect &&
+     git notes show >actual &&

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1584

Quoted 10 lines
t/t3301-notes.sh +10 -0
+     test_cmp expect actual &&
+     git notes remove HEAD &&
+
+     # Add a note using -F and edit it
+     echo "Note from file" >note_file &&
+     MSG="Edited note from file" git notes add -F note_file -e &&
+     echo "Edited note from file" >expect &&
+     git notes show >actual &&
+     test_cmp expect actual
+'

I was surprised at first why the MSG ended up in the commit message. But the setup of t3301 writes a fake editor that listens to this environment variable, so this looks good to me.

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1594

Quoted 9 lines
t/t3301-notes.sh +9 -0
+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
+     test_commit 20th &&
+     git notes add -m "Initial note message" &&
+     MSG="Appended edited note message" git notes append -m "New appended note" -e &&
+
+     # Verify the note content was appended and edited
+     echo "Initial note message" >expect &&
+     echo "" >>expect &&
+     echo "Appended edited note message" >>expect &&

When you want to write multiple lines we typically use HERE docs. E.g.:

cat >expect <<-EOF &&
Initial note message

Appended edited note message
EOF

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1599

Quoted 5 lines
t/t3301-notes.sh +5 -0
+     git notes show >actual &&
+     test_cmp expect actual &&
+     git notes remove HEAD &&
+
+     #Append a note using -F and edit it

There should be a space after the "#" here.

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1607

Quoted 8 lines
t/t3301-notes.sh +8 -0
+     echo "Note from file" >note_file &&
+     git notes add -m "Initial note message" &&
+     MSG="Appended edited note from file" git notes append -F note_file -e &&
+
+     # Verify notes from file has been edited in editor and appended
+     echo "Initial note message" >expect &&
+     echo "" >>expect &&
+     echo "Appended edited note from file" >>expect &&

Same comment here.

Quoted 18 lines
t/t3301-notes.sh +18 -0
+     git notes show >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '
+     test_commit 21st &&
+     echo "foo-file-1" >note_1 &&
+     echo "foo-file-2" >note_2 &&
+
+     MSG="Collapsed edited notes" git notes append -F note_1 -m "message-1" -F note_2 -e &&
+
+     # Verify that combined messages from file and -m have been edited
+
+     echo "Collapsed edited notes" >expect &&
+     git notes show >actual &&
+     test_cmp expect actual
+'
+
Quoted 5 lines

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1626

Quoted 1 line
t/t3301-notes.sh +0 -0
 test_done

It would be nice to have another test that uses EDITOR=false to demonstrate that we abort when the editor returns an error.

Thanks for the review, I will effect the changes.

Quoted 1 line

Other than that this patch looks good to me, thanks a lot!

You're welcome, Patrick.

Quoted 2 lines

Patrick

#11 lore raw
In reply to Patrick Steinhardt
Quoted 5 lines · Patrick Steinhardt

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1594

Quoted 3 lines · Samuel Adekunle Abraham via GitGitGadget
t/t3301-notes.sh +3 -0
+     echo "Initial note message" >expect &&
+     echo "" >>expect &&
+     echo "Appended edited note message" >>expect &&
Quoted 7 lines

When you want to write multiple lines we typically use HERE docs. E.g.:

cat >expect <<-EOF &&
Initial note message

Appended edited note message
EOF

Nit: Since there are no variable interpolations inside the heredoc body and we don't expect any, we'd normally indicate that by using \EOF rather than EOF:

    cat >expect <<-\EOF &&
author
#12 lore raw

PATCH v3 1/1 notes: teach the -e option to edit messages in editor

In reply to Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using:
- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

---
[Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor

Notes can be added to a commit using the -m (message), -C (copy a note from a blob object) or -F (read the note from a file) options. When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after they have been provided through -[mF].

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1817

Range-diff vs v2:

1: 55804cd1269 ! 1: 8f80c61ec0d notes: teach the -e option to edit messages in editor

     @@ Metadata
       ## Commit message ##
          notes: teach the -e option to edit messages in editor
      
     -    Notes can be added to a commit using the -m (message),
     -    -C (copy a note from a blob object) or
     -    -F (read the note from a file) options.
     +    Notes can be added to a commit using:
     +            - "-m" to provide a message on the command line.
     +            - -C to copy a note from a blob object.
     +            - -F to read the note from a file.
          When these options are used, Git does not open an editor,
          it simply takes the content provided via these options and
          attaches it to the commit as a note.
      
          Improve flexibility to fine-tune the note before finalizing it
     -    by allowing the messages to be prefilled in the editor and editted
     +    by allowing the messages to be prefilled in the editor and edited
          after the messages have been provided through -[mF].
      
          Signed-off-by: Abraham Samuel Adekunle <[email protected]>
     @@ Documentation/git-notes.txt: SYNOPSIS
       [verse]
       'git notes' [list [<object>]]
      -'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
     -+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
     ++'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [-e] [<object>]
       'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
      -'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
     -+'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] [-e]
     ++'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [-e] [<object>]
       'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
       'git notes' show [<object>]
       'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
     @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '
      +	MSG="Appended edited note message" git notes append -m "New appended note" -e &&
      +
      +	# Verify the note content was appended and edited
     -+	echo "Initial note message" >expect &&
     -+	echo "" >>expect &&
     -+	echo "Appended edited note message" >>expect &&
     ++
     ++	cat >expect <<-EOF &&
     ++		Initial note message
     ++
     ++		Appended edited note message
     ++	EOF
      +	git notes show >actual &&
      +	test_cmp expect actual &&
      +	git notes remove HEAD &&
      +
     -+	#Append a note using -F and edit it
     ++	# Append a note using -F and edit it
      +	echo "Note from file" >note_file &&
      +	git notes add -m "Initial note message" &&
      +	MSG="Appended edited note from file" git notes append -F note_file -e &&
      +
      +	# Verify notes from file has been edited in editor and appended
     -+	echo "Initial note message" >expect &&
     -+	echo "" >>expect &&
     -+	echo "Appended edited note from file" >>expect &&
     ++	cat >expect <<-EOF &&
     ++		Initial note message
     ++
     ++		Appended edited note from file
     ++	EOF
      +	git notes show >actual &&
      +	test_cmp expect actual
      +'
     @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '
      +	git notes show >actual &&
      +	test_cmp expect actual
      +'
     ++test_expect_success 'git notes append aborts when editor fails with -e' '
     ++	test_commit 22nd &&
     ++	echo "foo-file-1" >note_1 &&
     ++
     ++	# Try to append a note with -F and -e, but make the editor fail
     ++	test_env GIT_EDITOR="false" test_must_fail git notes append -F note_1 -e &&
     ++
     ++	# Verify that no note was added due to editor failure
     ++	test_must_fail git notes show
     ++'
      +
       test_done
 Documentation/git-notes.txt | 10 ++++--
 builtin/notes.c             |  8 +++--
 t/t3301-notes.sh            | 71 +++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 5 deletions(-)
Documentation/git-notes.txt +7 -3
index c9221a68cce..84022f99d76 100644
@@ -9,9 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [-e] [<object>]
 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
-'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [-e] [<object>]
 'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>

@@ -67,7 +67,9 @@ add::
 	the existing notes will be opened in the editor (like the `edit`
 	subcommand). If you specify multiple `-m` and `-F`, a blank
 	line will be inserted between the messages. Use the `--separator`
-	option to insert other delimiters.
+	option to insert other delimiters. You can use `-e` to edit and
+	fine-tune the message(s) supplied from `-m` and `-F` options
+	interactively (using an editor) before adding the note.
 
 copy::
 	Copy the notes for the first object onto the second object (defaults to

@@ -93,6 +95,8 @@ append::
 	an existing note, a blank line is added before each new
 	message as an inter-paragraph separator.  The separator can
 	be customized with the `--separator` option.
+	Edit the notes to be appended given by `-m` and `-F` options with
+	`-e` interactively (using an editor) before appending the note.
 
 edit::
 	Edit the notes for a given object (defaults to HEAD).
builtin/notes.c +6 -2
index 8c26e455269..72c8a51cfac 100644
@@ -32,9 +32,9 @@
 static const char *separator = "\n";
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
-	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
 	N_("git notes [--ref <notes-ref>] copy [-f] <from-object> <to-object>"),
-	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
 	N_("git notes [--ref <notes-ref>] edit [--allow-empty] [<object>]"),
 	N_("git notes [--ref <notes-ref>] show [<object>]"),
 	N_("git notes [--ref <notes-ref>] merge [-v | -q] [-s <strategy>] <notes-ref>"),

@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),

@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT_CALLBACK_F(0, "separator", &separator,
t/t3301-notes.sh +71 -0

75 lines not on this page · raw message

base-commit: 15030f9556f545b167b1879b877a5d780252dc16

Signature · 1 line
gitgitgadget
#13 lore raw
Quoted 10 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using:
- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Quoted 5 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

Nicely described, this commit message is looking good. Let's take a look at the patch below...

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1573

Quoted 12 lines
t/t3301-notes.sh +4 -0
index 99137fb2357..2827f592c66 100755
@@ -1567,4 +1567,75 @@ test_expect_success 'empty notes do not invoke the editor' '
 	git notes remove HEAD
 '
 
+test_expect_success 'git notes add with -m/-F invokes editor with -e' '
+	test_commit 19th &&
+	MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
+	echo "Edited notes message" >expect &&

Very nice use of the fake_editor script here.

It is a little cumbersome to repeat the same message in MSG= and when populating the 'expect' file. Perhaps instead this could be written as:

    echo "edited notes message" >expect &&
    MSG="$(cat expect)" git notes -add -m "initial" -e

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1581

Quoted 8 lines
t/t3301-notes.sh +8 -0
+	git notes show >actual &&
+	test_cmp expect actual &&
+	git notes remove HEAD &&
+
+	# Add a note using -F and edit it
+	echo "Note from file" >note_file &&
+	MSG="Edited note from file" git notes add -F note_file -e &&
+	echo "Edited note from file" >expect &&

Same "note" here. ;-).

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1589

Quoted 8 lines
t/t3301-notes.sh +8 -0
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
+	test_commit 20th &&
+	git notes add -m "Initial note message" &&
+	MSG="Appended edited note message" git notes append -m "New appended note" -e &&

It's fine to use shorter values for -m and $MSG here. I think "appended" and "edited" would be fine for each, respectively.

Besides applying those suggestions throughout the patch's new tests (including the ones that I didn't explicitly comment on here), I think that this should be looking good after another round. Thanks for working on it.

Thanks,
Taylor

#14 lore raw
Quoted 36 lines · Taylor Blau
Quoted 10 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using:
- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Quoted 5 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

Nicely described, this commit message is looking good. Let's take a look at the patch below...

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1573

Quoted 12 lines
t/t3301-notes.sh +4 -0
index 99137fb2357..2827f592c66 100755
@@ -1567,4 +1567,75 @@ test_expect_success 'empty notes do not invoke the editor' '
      git notes remove HEAD
 '
 
+test_expect_success 'git notes add with -m/-F invokes editor with -e' '
+     test_commit 19th &&
+     MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
+     echo "Edited notes message" >expect &&

Very nice use of the fake_editor script here.

Quoted 5 lines

It is a little cumbersome to repeat the same message in MSG= and when populating the 'expect' file. Perhaps instead this could be written as:

    echo "edited notes message" >expect &&
    MSG="$(cat expect)" git notes -add -m "initial" -e

Concise! :-). Thank you very much Taylor.

Quoted 12 lines

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1581

Quoted 9 lines
t/t3301-notes.sh +8 -0
+     git notes show >actual &&
+     test_cmp expect actual &&
+     git notes remove HEAD &&
+
+     # Add a note using -F and edit it
+     echo "Note from file" >note_file &&
+     MSG="Edited note from file" git notes add -F note_file -e &&
+     echo "Edited note from file" >expect &&

Same "note" here. ;-).

Quoted 12 lines

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1589

Quoted 8 lines
t/t3301-notes.sh +8 -0
+     git notes show >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
+     test_commit 20th &&
+     git notes add -m "Initial note message" &&
+     MSG="Appended edited note message" git notes append -m "New appended note" -e &&

It's fine to use shorter values for -m and $MSG here. I think "appended" and "edited" would be fine for each, respectively.

Okay. Noted

Quoted 4 lines

Besides applying those suggestions throughout the patch's new tests (including the ones that I didn't explicitly comment on here), I think that this should be looking good after another round. Thanks for working on it.

Thank you very much,
Abraham Samuel

Quoted 3 lines

Thanks,
Taylor

#15 lore raw
In reply to Taylor Blau
Quoted 11 lines · Taylor Blau

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1573

Quoted 2 lines · Samuel Adekunle Abraham via GitGitGadget
t/t3301-notes.sh +2 -0
+     MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
+     echo "Edited notes message" >expect &&

Very nice use of the fake_editor script here.

It is a little cumbersome to repeat the same message in MSG= and when populating the 'expect' file. Perhaps instead this could be written as:

    echo "edited notes message" >expect &&
    MSG="$(cat expect)" git notes -add -m "initial" -e

This suggested rewrite feels unusually roundabout and increases cognitive load for readers who now have to trace the message flow from script to file and back into script, and to consider how the loss of trailing newline caused by use of $(...) impacts the behavior. It also wastes a subprocess (which can be expensive on some platforms, such as Windows). If we're really concerned about this minor duplication of the message, we can instead do this:

    msg="edited notes message" &&
    echo "$msg" >expect &&
    MSG="$msg" git notes -add -m "initial" -e
#16 lore raw
Quoted 21 lines · Eric Sunshine
Quoted 11 lines · Taylor Blau

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1573

Quoted 2 lines · Samuel Adekunle Abraham via GitGitGadget
t/t3301-notes.sh +2 -0
+     MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
+     echo "Edited notes message" >expect &&

Very nice use of the fake_editor script here.

It is a little cumbersome to repeat the same message in MSG= and when populating the 'expect' file. Perhaps instead this could be written as:

    echo "edited notes message" >expect &&
    MSG="$(cat expect)" git notes -add -m "initial" -e

This suggested rewrite feels unusually roundabout and increases cognitive load for readers who now have to trace the message flow from script to file and back into script, and to consider how the loss of trailing newline caused by use of $(...) impacts the behavior. It also wastes a subprocess (which can be expensive on some platforms, such as Windows). If we're really concerned about this minor duplication of the message, we can instead do this:

Quoted 3 lines
    msg="edited notes message" &&
    echo "$msg" >expect &&
    MSG="$msg" git notes -add -m "initial" -e

I am not sure I agree that the suggested version is clearer. The way I read mine is that we are writing what we expect to see in "expect", and then setting up MSG to be the same thing.

I definitely do not feel strongly between the two and would rather avoid nitpicking something as trivial as this when compared to the rest of the patch, especially considering that I would be equally happy with your version.

I think the whole thing is a little bit of a moot point, though, because by far the thing that is least clear here is that setting MSG has the side-effect of modifying the behavior of the fake-editor that we set up earlier in the script. So I don't know that optimizing for clarity in how we setup and write to the "expect" file is the right thing to spend our time on.

Thanks,
Taylor

author
#17 lore raw

PATCH v4 1/1 notes: teach the -e option to edit messages in editor

In reply to Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using:
- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

---
[Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor

Notes can be added to a commit using the -m (message), -C (copy a note from a blob object) or -F (read the note from a file) options. When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after they have been provided through -[mF].

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1817

Range-diff vs v3:

1: 8f80c61ec0d ! 1: 4ecf79e0766 notes: teach the -e option to edit messages in editor

     @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '
       
      +test_expect_success 'git notes add with -m/-F invokes editor with -e' '
      +	test_commit 19th &&
     -+	MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
     -+	echo "Edited notes message" >expect &&
     ++	echo "edited" >expect &&
     ++	MSG="$(cat expect)" git notes add -m "initial" -e &&
      +	git notes show >actual &&
      +	test_cmp expect actual &&
      +	git notes remove HEAD &&
      +
      +	# Add a note using -F and edit it
     -+	echo "Note from file" >note_file &&
     -+	MSG="Edited note from file" git notes add -F note_file -e &&
     -+	echo "Edited note from file" >expect &&
     ++	echo "initial" >note_file &&
     ++	MSG="$(cat expect)" git notes add -F note_file -e &&
      +	git notes show >actual &&
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
      +	test_commit 20th &&
     -+	git notes add -m "Initial note message" &&
     -+	MSG="Appended edited note message" git notes append -m "New appended note" -e &&
     -+
     -+	# Verify the note content was appended and edited
     -+
      +	cat >expect <<-EOF &&
     -+		Initial note message
     ++		initial
      +
     -+		Appended edited note message
     ++		edited
      +	EOF
     ++	git notes add -m "initial" &&
     ++	MSG="edited" git notes append -m "appended" -e &&
     ++
     ++	# Verify the note content was appended and edited
      +	git notes show >actual &&
      +	test_cmp expect actual &&
      +	git notes remove HEAD &&
      +
      +	# Append a note using -F and edit it
     -+	echo "Note from file" >note_file &&
     -+	git notes add -m "Initial note message" &&
     -+	MSG="Appended edited note from file" git notes append -F note_file -e &&
     ++	echo "note from file" >note_file &&
     ++	git notes add -m "initial" &&
     ++	MSG="edited" git notes append -F note_file -e &&
      +
      +	# Verify notes from file has been edited in editor and appended
     -+	cat >expect <<-EOF &&
     -+		Initial note message
     -+
     -+		Appended edited note from file
     -+	EOF
      +	git notes show >actual &&
      +	test_cmp expect actual
      +'
     @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '
      +	test_commit 21st &&
      +	echo "foo-file-1" >note_1 &&
      +	echo "foo-file-2" >note_2 &&
     ++	echo "edited" >expect &&
      +
     -+	MSG="Collapsed edited notes" git notes append -F note_1 -m "message-1" -F note_2 -e &&
     ++	MSG=$(cat expect) git notes append -F note_1 -m "message-1" -F note_2 -e &&
      +
      +	# Verify that combined messages from file and -m have been edited
     -+
     -+	echo "Collapsed edited notes" >expect &&
      +	git notes show >actual &&
      +	test_cmp expect actual
      +'
 Documentation/git-notes.txt | 10 ++++--
 builtin/notes.c             |  8 +++--
 t/t3301-notes.sh            | 63 +++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 5 deletions(-)
Documentation/git-notes.txt +7 -3
index c9221a68cce..84022f99d76 100644
@@ -9,9 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [-e] [<object>]
 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
-'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' append [--allow-empty] [--[no-]separator | --separator=<paragraph-break>] [--[no-]stripspace] [-F <file> | -m <msg> | (-c | -C) <object>] [-e] [<object>]
 'git notes' edit [--allow-empty] [<object>] [--[no-]stripspace]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>

@@ -67,7 +67,9 @@ add::
 	the existing notes will be opened in the editor (like the `edit`
 	subcommand). If you specify multiple `-m` and `-F`, a blank
 	line will be inserted between the messages. Use the `--separator`
-	option to insert other delimiters.
+	option to insert other delimiters. You can use `-e` to edit and
+	fine-tune the message(s) supplied from `-m` and `-F` options
+	interactively (using an editor) before adding the note.
 
 copy::
 	Copy the notes for the first object onto the second object (defaults to

@@ -93,6 +95,8 @@ append::
 	an existing note, a blank line is added before each new
 	message as an inter-paragraph separator.  The separator can
 	be customized with the `--separator` option.
+	Edit the notes to be appended given by `-m` and `-F` options with
+	`-e` interactively (using an editor) before appending the note.
 
 edit::
 	Edit the notes for a given object (defaults to HEAD).
builtin/notes.c +6 -2
index 8c26e455269..72c8a51cfac 100644
@@ -32,9 +32,9 @@
 static const char *separator = "\n";
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
-	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
 	N_("git notes [--ref <notes-ref>] copy [-f] <from-object> <to-object>"),
-	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>] [-e]"),
 	N_("git notes [--ref <notes-ref>] edit [--allow-empty] [<object>]"),
 	N_("git notes [--ref <notes-ref>] show [<object>]"),
 	N_("git notes [--ref <notes-ref>] merge [-v | -q] [-s <strategy>] <notes-ref>"),

@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
 			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
 			parse_reedit_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),

@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
 			N_("reuse specified note object"), PARSE_OPT_NONEG,
 			parse_reuse_arg),
+		OPT_BOOL('e', "edit", &d.use_editor,
+			N_("edit note message in editor")),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT_CALLBACK_F(0, "separator", &separator,
t/t3301-notes.sh +63 -0

67 lines not on this page · raw message

base-commit: 15030f9556f545b167b1879b877a5d780252dc16

Signature · 1 line
gitgitgadget
#18 lore raw
Quoted 10 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using:
- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Quoted 5 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

Thanks, will queue.

Thanks,
Taylor

#19 lore raw
Quoted 12 lines · On Mon, 21 Oct 2024, 20:53 Taylor Blau, <[email protected]> wrote:
Quoted 10 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using:
- "-m" to provide a message on the command line.
- -C to copy a note from a blob object.
- -F to read the note from a file.
When these options are used, Git does not open an editor, it simply takes the content provided via these options and attaches it to the commit as a note.

Quoted 8 lines
Quoted 5 lines

Improve flexibility to fine-tune the note before finalizing it by allowing the messages to be prefilled in the editor and edited after the messages have been provided through -[mF].

Signed-off-by Abraham Samuel Adekunle <[email protected]>

Thanks, will queue.

Thank you, Taylor, my Outreachy mentors Patrick and Phillip, Junio, and all the maintainers for your time and patience in reviewing my patches. It has been a good learning period. Thanks once again. Abraham Samuel

Quoted 3 lines

Thanks,
Taylor

#20 lore raw
In reply to Samuel Adekunle Abraham via GitGitGadget
Quoted 21 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

t/t3301-notes.sh +12 -0
index 99137fb2357..813dfd8db97 100755
@@ -1567,4 +1567,67 @@ test_expect_success 'empty notes do not invoke the editor' '
 	git notes remove HEAD
 '
 
+test_expect_success 'git notes add with -m/-F invokes editor with -e' '
+	test_commit 19th &&
+	echo "edited" >expect &&
+	MSG="$(cat expect)" git notes add -m "initial" -e &&
+	git notes show >actual &&
+	test_cmp expect actual &&
+	git notes remove HEAD &&
+
+	# Add a note using -F and edit it
+	echo "initial" >note_file &&
+	MSG="$(cat expect)" git notes add -F note_file -e &&
+	git notes show >actual &&

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1591

Quoted 10 lines
t/t3301-notes.sh +10 -0
+	test_cmp expect actual
+'
+
+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
+	test_commit 20th &&
+	cat >expect <<-EOF &&
+		initial
+
+		edited
+	EOF

Nit: we typically align the contents of HERE docs with cat. I'm not a huge fan of it and had been struggling with it initially, as well, but coding style is subjective anyway and it's totally fine that one doesn't agree with everything.

In any case, I don't think this warrants a reroll of this patch, just to keep it in mind for future patches you may send.

[snip]

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1632

Quoted 11 lines
t/t3301-notes.sh +11 -0
+test_expect_success 'git notes append aborts when editor fails with -e' '
+	test_commit 22nd &&
+	echo "foo-file-1" >note_1 &&
+
+	# Try to append a note with -F and -e, but make the editor fail
+	test_env GIT_EDITOR="false" test_must_fail git notes append -F note_1 -e &&
+
+	# Verify that no note was added due to editor failure
+	test_must_fail git notes show
+'
+

Nice.

Thanks, this looks good to me overall.

Patrick

#21 lore raw
Quoted 39 lines · Patrick Steinhardt
Quoted 21 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

t/t3301-notes.sh +12 -0
index 99137fb2357..813dfd8db97 100755
@@ -1567,4 +1567,67 @@ test_expect_success 'empty notes do not invoke the editor' '
      git notes remove HEAD
 '
 
+test_expect_success 'git notes add with -m/-F invokes editor with -e' '
+     test_commit 19th &&
+     echo "edited" >expect &&
+     MSG="$(cat expect)" git notes add -m "initial" -e &&
+     git notes show >actual &&
+     test_cmp expect actual &&
+     git notes remove HEAD &&
+
+     # Add a note using -F and edit it
+     echo "initial" >note_file &&
+     MSG="$(cat expect)" git notes add -F note_file -e &&
+     git notes show >actual &&

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1591

Quoted 10 lines
t/t3301-notes.sh +10 -0
+     test_cmp expect actual
+'
+
+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
+     test_commit 20th &&
+     cat >expect <<-EOF &&
+             initial
+
+             edited
+     EOF

Nit: we typically align the contents of HERE docs with cat. I'm not a huge fan of it and had been struggling with it initially, as well, but coding style is subjective anyway and it's totally fine that one doesn't agree with everything.

Quoted 3 lines

In any case, I don't think this warrants a reroll of this patch, just to keep it in mind for future patches you may send.

Okay, I will keep that in mind. Thanks

Quoted 15 lines

[snip]

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1632

Quoted 11 lines
t/t3301-notes.sh +11 -0
+test_expect_success 'git notes append aborts when editor fails with -e' '
+     test_commit 22nd &&
+     echo "foo-file-1" >note_1 &&
+
+     # Try to append a note with -F and -e, but make the editor fail
+     test_env GIT_EDITOR="false" test_must_fail git notes append -F note_1 -e &&
+
+     # Verify that no note was added due to editor failure
+     test_must_fail git notes show
+'
+

Nice.

Quoted 3 lines

Thanks, this looks good to me overall.

Patrick

Thank you very much, I really appreciate.
Abraham Samuel.