diff mbox series

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

Message ID pull.1817.v4.git.1729534340786.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit dab0b9e176d64f7687029b176d96eddfbf74db64
Headers show
Series [v4] notes: teach the -e option to edit messages in editor | expand

Commit Message

Samuel Abraham Oct. 21, 2024, 6:12 p.m. UTC 
From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>

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 <abrahamadekunle50@gmail.com>
---
    [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(-)


base-commit: 15030f9556f545b167b1879b877a5d780252dc16

Comments

Taylor Blau Oct. 21, 2024, 7:53 p.m. UTC | #1 
On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
>
> 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 <abrahamadekunle50@gmail.com>

Thanks, will queue.

Thanks,
Taylor
Samuel Abraham Oct. 21, 2024, 9:44 p.m. UTC | #2 
On Mon, 21 Oct 2024, 20:53 Taylor Blau, <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> > From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> >
> > 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 <abrahamadekunle50@gmail.com>
>
> 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
>
> Thanks,
> Taylor
Patrick Steinhardt Oct. 23, 2024, 6:14 a.m. UTC | #3 
On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 99137fb2357..813dfd8db97 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -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 &&
> +	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]
> +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
Samuel Abraham Oct. 23, 2024, 4:51 p.m. UTC | #4 
On Wed, Oct 23, 2024 at 7:14 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> > From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index 99137fb2357..813dfd8db97 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -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 &&
> > +     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.
>

Okay, I will keep that in mind. Thanks
> [snip]
> > +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

Thank you very much, I really appreciate.
Abraham Samuel.
diff mbox series

Patch

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index c9221a68cce..84022f99d76 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -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).
diff --git a/builtin/notes.c b/builtin/notes.c
index 8c26e455269..72c8a51cfac 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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,
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 99137fb2357..813dfd8db97 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -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 &&
+	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
+	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" &&
+	MSG="edited" git notes append -F note_file -e &&
+
+	# Verify notes from file has been edited in editor and appended
+	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 &&
+	echo "edited" >expect &&
+
+	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
+	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