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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 2, 2018, 01:29 UTC
Message-ID: <CAPig+cT8vKyhq6DvFMz-0CPRO-Y7R4EE_JhN6yuiSUNXW8-Yww@mail.gmail.com>
In-Reply-To: <e99947cf-93ba-9376-f059-7f6a369d3ad5@suse.com>

On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com> wrote:

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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