Re: [PATCH v2] notes: teach the -e option to edit messages in editor

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 21, 2024, 11:58 UTC
Message-ID: <ZxZByGdre-lup2sO@pks.im>
In-Reply-To: <pull.1817.v2.git.1729382580491.gitgitgadget@gmail.com>

On Sun, Oct 20, 2024 at 12:03:00AM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:

Show 5 quoted lines

> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> 
> 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.

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, ...

Show 6 quoted 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

Show 16 quoted lines

> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index c9221a68cce..d5505a426aa 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>] [<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.

Show 22 quoted lines

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 8c26e455269..72c8a51cfac 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -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,

Nice.

Show 23 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

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

Show 19 quoted lines

> +	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_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