Re: [PATCHv2] tag: add --edit option

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
<nmoreychaisemartin@suse.com> wrote:
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
>
> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com>
> ---
> Changes since v1:
> - Fix usage string
> - Use write_script to generate editor
> - Rename editor to fakeeditor to match the other tests in the testsuite

Thanks for explaining what changed since the previous attempt. It is
also helpful for reviewers if you include a reference to the previous
iteration, like this:
https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7f7c@suse.com/T/#u

Cc:'ing reviewers of previous iterations is also good etiquette when
submitting a new version.

> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails

Typically, it's easier on Junio, from a patch management standpoint,
if you submit all these related patches as a single series.
Alternately, if you do want to submit those changes separately, before
the current patch lands in "master", be sure to mention atop which
patch (this one) the additional patch(es) should live. Thanks.

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without annotation lines.
> +-e::
> +--edit::
> +       The message taken from file with `-F` and command line with
> +       `-m` are usually used as the tag message unmodified.
> +       This option lets you further edit the message taken from these sources.

You probably ought to add this new option to the command synopsis. In
the git-commit man page, the synopsis mentions only '-e' (not --edit),
so perhaps this man page could mirror that one. (Sorry for not
noticing this earlier.)

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -452,6 +452,21 @@ test_expect_success \
> +get_tag_header annotated-tag-edit $commit commit $time >expect
> +echo "An edited message" >>expect

Modern practice is to perform these "expect" setup actions (and all
other actions) within tests themselves rather than outside of tests.
However, consistency also has value, and since this test script is
filled with this sort of stylized "expect" setup already, this may be
fine, and probably not worth a re-roll. (A "modernization" patch by
someone can come later if warranted.)

> +test_expect_success 'set up editor' '
> +       write_script fakeeditor <<-\EOF
> +       sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> +       mv "$1-" "$1"
> +       EOF
> +'