diff mbox series

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

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

Commit Message

Samuel Abraham Oct. 21, 2024, 2:38 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-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 <abrahamadekunle50@gmail.com>
     @@ 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(-)


base-commit: 15030f9556f545b167b1879b877a5d780252dc16

Comments

Taylor Blau Oct. 21, 2024, 4:52 p.m. UTC | #1 
On Mon, Oct 21, 2024 at 02:38:15PM +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>

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

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 99137fb2357..2827f592c66 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -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

> +	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. ;-).

> +	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
Samuel Abraham Oct. 21, 2024, 5:12 p.m. UTC | #2 
On Mon, Oct 21, 2024 at 5:53 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Oct 21, 2024 at 02:38:15PM +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>
>
> Nicely described, this commit message is looking good. Let's take a look
> at the patch below...
>
> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index 99137fb2357..2827f592c66 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -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

Concise! :-). Thank you very much Taylor.
>
> > +     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. ;-).
>
> > +     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
> 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
>
> Thanks,
> Taylor
Eric Sunshine Oct. 21, 2024, 6:28 p.m. UTC | #3 
On Mon, Oct 21, 2024 at 12:53 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> > +     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
Taylor Blau Oct. 21, 2024, 7:52 p.m. UTC | #4 
On Mon, Oct 21, 2024 at 02:28:05PM -0400, Eric Sunshine wrote:
> On Mon, Oct 21, 2024 at 12:53 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> > > +     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

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
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..2827f592c66 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -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 &&
+	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 &&
+	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
+
+		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
+	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
+	cat >expect <<-EOF &&
+		Initial note message
+
+		Appended edited note from file
+	EOF
+	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
+'
+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