Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits

From: Stephen Morton <stephen.morton@nokia.com>
Date: Aug 17, 2016, 13:42 UTC
Message-ID: <f58933df-352a-9d2b-a35a-9c48cb2d958e@nokia.com>
In-Reply-To: <CAP8UFD04Z7JpoAA1kXkYFk5LD-GngbUDkbnpCEc3DNDXUgetEA@mail.gmail.com>

Responding to a few comments...

On 2016-08-14 7:44 AM, Christian Couder wrote:

> multiple_commits)
> ... but here multiple_commits is the last argument.
> It would be better if it was more consistent.

(Johannes made the same comment.) Yes. Will do.

>
> multiple_commits = (todo_list->next) != NULL;
> Why not "last_commit" instead of "multiple_commits"?
>

Because it *isn't*. You can see that in pick_commits(), I set multiple_commits outside of the `for todo_list` loop. It is not re-evaluated at every iteration of the loop. As per my comment when emailing the patch "I intentionally print the '--continue' hint even in the case where it's last of n commits that fails. " I think it makes much more sense that "this is the message you always get when cherry-picking multiple commits as opposed to "this is the message you sometimes get, except when it's the last one". (Yes, the careful observer will realize that if when cherry-picking multiple commits, there are conflicts in the second-last and last then the --continue from the second-last will result in multiple_commits being set to 0. I can live with that.)

On 2016-08-16 4:44 AM, Remi Galan Alfonso wrote:

Show 32 quoted lines

> Hi Stephen,
>
> Stephen Morton <stephen.morton@nokia.com> writes:
>> +                        if  (multiple_commits)
>> +                               advise(_("after resolving the conflicts,
>> mark the corrected paths with 'git add <paths>' or 'git rm <paths>'\n"
>> +                                        "then continue with 'git %s
>> --continue'\n"
>> +                                        "or cancel with 'git %s
>> --abort'" ), action_name(opts), action_name(opts));
>> +                        else
>> +                                advise(_("after resolving the
>> conflicts, mark the corrected paths\n"
>> +                                        "with 'git add <paths>' or 'git
>> rm <paths>'\n"
>> +                                        "and commit the result with
>> 'git commit'"));
> In both cases (multiple_commits or not), the beginning of the advise
> is nearly the same, with only a '\n' in the middle being the
> difference:
>
> multiple_commits:
>   "after resolving the conflicts, mark the corrected paths with 'git
>   add <paths>' or 'git rm <paths>'\n"
>
> !multiple_commits:
>   "after resolving the conflicts, mark the corrected paths\n with 'git
>   add <paths>' or 'git rm <paths>'\n"
>                                                    ~~~~~~~^
>
> In 'multiple_commits' case the advise is more than 80 characters long,
> did you forget the '\n' in that case?

A previous comment had indicated that having 4 lines was too many. And I tend to agree. So I tried to squash it into 3. Back in xterm days, 80 characters was sacrosanct, but is it really a big deal to exceed it now?

On 2016-08-14 7:44 AM, Christian Couder wrote:

> ...but please try to send a real patch.
>
> There is https://github.com/git/git/blob/master/Documentation/SubmittingPatches
> and also SubmitGit that can help you do that.

Agreed. I just want to send a patch that stands a reasonable chance of getting accepted.

Stephen

-- 
Stephen Morton, 7750 SR Product Group, SW Development Tools/DevOps
w: +1-613-784-6026 (int: 2-825-6026) m: +1-613-302-2589 | EST Time Zone