Re: [PATCH v3] notes: teach the -e option to edit messages in editor
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 21, 2024, 16:52 UTC
- Message-ID
- <ZxaG67wuvjOXc5kr@nand.local>
- In-Reply-To
- <pull.1817.v3.git.1729521495497.gitgitgadget@gmail.com>
On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
Show 15 quoted lines
> 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...
Show 12 quoted lines
> 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" -eShow 8 quoted lines
> + 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. ;-).
Show 8 quoted lines
> + 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