On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <abrahamadekunle50@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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
