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

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