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

From: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
Date: Feb 4, 2018, 15:57 UTC
Message-ID: <670db556-59f4-8236-be2f-e0c901e8df68@suse.com>
In-Reply-To: <CAPig+cQF2HzmtVdHqtQcOf0B-yA8Kpj-CZbPmdQotGwtdYpxvw@mail.gmail.com>

Le 02/02/2018 à 20:16, Eric Sunshine a écrit :

Show 14 quoted lines

> On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemartin@suse.com> wrote:
>> What message do you suggest ?  As I said in a previous mail, a
>> simple "Editor failure, cancelling {commit, tag}" should be enough
>> as launch_editor already outputs error messages describing what
>> issue the editor had.
>>
>> I don't think suggesting moving to --no-edit || -m || -F is that
>> helpful.  It's basically saying your setup is broken, but you can
>> workaround by setting those options (and not saying that you're
>> going to have some more issues later one).
> If it's the case the launch_editor() indeed outputs an appropriate
> error message, then the existing error message from tag.c is already
> appropriate when --edit is not specified.

I don't fully agree with the current message. The right thing to do is to fix the editor, not to hide the issue. A better message would be "Editor failed. Fix it, or supply the message using either..." At least we suggest the right way to do it first.

Show 15 quoted lines

>  It's only the --edit case
> that the tag.c's additional message is somewhat weird. And, in fact,
> suppressing tag.c's message might be the correct thing to do in the
> --edit case:
>
>     static void create_tag(...) {
>         ...
> if (launch_editor(...)) {
>    if (!opt->use_editor)
>        fprintf(stderr, _("... use either -m or -F ..."));
>             exit(1);
> }
>
> I don't feel strongly about it either way and am fine with just
> punting on the issue until someone actually complains about it.

The test should be opt->message_given && opt->use_editor. If just --edit is provided but no -m/-F, --edit does not have any effect and it should be the same error message as when no option is given.

Nicolas