| Message ID | pull.1817.v2.git.1729382580491.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] notes: teach the -e option to edit messages in editor | expand |
On Sun, Oct 20, 2024 at 12:03:00AM +0000, Samuel Adekunle Abraham via GitGitGadget wrote: > 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, ... > 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 > 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. > 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. > 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. > +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 > + 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. > + 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. > + 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
On Mon, Oct 21, 2024 at 12:58 PM Patrick Steinhardt <ps@pks.im> wrote: > > On Sun, Oct 20, 2024 at 12:03:00AM +0000, Samuel Adekunle Abraham via GitGitGadget wrote: > > 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, ... Thank you Patrick. Noted. > > > 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. > > > 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. > > > 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. > > > 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. > > > +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 > > > + 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. > > > + 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. > > > + 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. > Thanks for the review, I will effect the changes. > Other than that this patch looks good to me, thanks a lot! You're welcome, Patrick. > > Patrick
On Mon, Oct 21, 2024 at 8:00 AM Patrick Steinhardt <ps@pks.im> wrote: > On Sun, Oct 20, 2024 at 12:03:00AM +0000, Samuel Adekunle Abraham via GitGitGadget wrote: > > + 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 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 &&
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> @@ -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..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 +' + +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 && + 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 + 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 '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