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

> Hi Stephen,
>
> On Wed, 27 Jul  2016, Stephen Morton wrote:
>
>> On Wed, Jul  27, 2016 at 11:03 AM, Johannes Schindelin
>>  <Johannes.Schindelin@gmx.de> wrote:
>> >
>> > On Wed,  27 Jul 2016, Stephen Morton wrote:
>> >
>> >>  diff --git a/sequencer.c b/sequencer.c
>> >>  index cdfac82..ce06876 100644
>> >> ---  a/sequencer.c
>> >> +++  b/sequencer.c
>> >> @@  -176,7 +176,8 @@ static void print_advice(int show_hint, struct
>> >>  replay_opts *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'"));
>> >> +                              "then continue the %s with 'git %s
>> >>  --continue'\n"
>> >> +                              "or cancel the %s operation with 'git
>> >> %s  --abort'" ),  action_name(opts), action_name(opts),
>> >>  action_name(opts), action_name(opts));
>> >
>> > That is  an awful lot of repetition right there, with an added
>> >  inconsistency that the action is referred to by its name alone in the
>> >  "--continue" case, but with "operation" added in the "--abort" case.
>> >
>> > And  additionally, in the most common case (one commit to cherry-pick), the
>> > advice  now suggests a more complicated operation than necessary: a simply
>> > `git  commit` would be enough, then.
>> >
>> > Can't  we have a test whether this is the last of the commits to be
>> >  cherry-picked, and if so, have the simpler advice again?
>>
>> Ok, knowing  that I'm not on the last element of the sequencer is
>> beyond my  git code knowledge.
>
> Oh, my mistake:  I meant to say that this information could be easily
> provided by  `pick_commits()` if it passed it to `print_advice()` via
>  `do_pick_commit()`.
>
> Ciao,
> Johannes

> (I intentionally print the '--continue' hint even in the case whereit's last
> of n commits that fails.)
>
>
> Stephen
>
>
> ~/ws/extern/git (maint *%>) > git diff @{u}
> diff --git a/sequencer.c b/sequencer.c
> index c6362d6..e0071aa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -154,7 +154,7 @@ static void free_message(struct commit *commit, struct
> commit_message *msg)
>         unuse_commit_buffer(commit, msg->message);
>  }
>
> -static void print_advice(int show_hint, struct replay_opts *opts)
> +static void print_advice(int show_hint, int multiple_commits, struct
> replay_opts *opts)

>  {
>         char *msg = getenv("GIT_CHERRY_PICK_HELP");
>
> @@ -174,9 +174,14 @@ static void print_advice(int show_hint, struct
> replay_opts *opts)
>                         advise(_("after resolving the conflicts, mark the
> corrected paths\n"
>                                  "with 'git add <paths>' or 'git rm
> <paths>'"));
>                 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'"));
> +                        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'"));
>         }
>  }
>
> @@ -440,7 +445,7 @@ static int allow_empty(struct replay_opts *opts, struct
> commit *commit)
>                 return 1;
>  }
>
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +static int do_pick_commit(struct commit *commit, struct replay_opts *opts,
> int multiple_commits)

>  {
>         unsigned char head[20];
>         struct commit *base, *next, *parent;
> @@ -595,7 +600,7 @@ static int do_pick_commit(struct commit *commit, struct
> replay_opts *opts)
>                       : _("could not apply %s... %s"),
>                       find_unique_abbrev(commit->object.oid.hash,
> DEFAULT_ABBREV),
>                       msg.subject);
> -               print_advice(res == 1, opts);
> +               print_advice(res == 1, multiple_commits, opts);
>                 rerere(opts->allow_rerere_auto);
>                 goto leave;
>         }
> @@ -959,6 +964,7 @@ static int pick_commits(struct commit_list *todo_list,
> struct replay_opts *opts)
>  {
>         struct commit_list *cur;
>         int res;
> +    int multiple_commits = (todo_list->next) != NULL;

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

> [snip]
> On 2016-08-16 4:44 AM, Remi Galan Alfonso wrote:
>> 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?

> +                        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'"));

> 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?
> 
> If you end up using the same beginning of advice, maybe it's possible
> to give it before the 'if(multiple_commits)' and avoid duplication of
> the lines.