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

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

[Stephen Morton]

When I cherry-pick n commits and one of the first (n-1), fails, what I
should do is resolve the conflict, 'git add' it, and then run 'git
cherry-pick --continue'. However git advises me to

error: could not apply d0fb756... Commit message for test commit
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

I suspect this is not just a little bit deceptive but actually a bit
destructive, as doing a 'git commit' removes some of the sequencing
information. If I do a 'git commit', while there is .git/sequencer
information and 'git cherry-pick --continue' will work, there is no
indication in 'git status' that a cherry-pick is in progress.

It would be extremely easy for somebody to follow cherry-pick's hints
by running 'git commit' and think that they were done, not realizing
that there were m more commits remaining to be cherry-picked.

Would it be possible to expand the hint message to tell users to run
'git cherry-pick --continue'

This patch is *not* meant as a serious patch for submission --I'm sure
it's all wrong-- it's just a proof of concept and showing some
goodwill on my part that I'm trying to help and I'm willing to put in
some work if I can be pointed more in the right direction.

Regards,
Stephen




diff --git a/sequencer.c b/sequencer.c
index cdfac82..b8fa534 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -171,14 +171,21 @@ static void print_advice(int show_hint, struct
replay_opts *opts)

        if (show_hint) {
                if (opts->no_commit)
                        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'"));
+               else if  (! file_exists(git_path_seq_dir())) {
+                       advise(_("SCM: 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'"));
+        }
+        else
+        {
+            advise(_("SCM: after resolving the conflicts, mark the
corrected paths\n"
+                     "with 'git add <paths>' or 'git rm <paths>'\n"
+                     "and continue the %s with 'git %s --continue'"),
action_name(opts), action_name(opts));
+        }
        }
 }

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

[Stefan Beller]

> Would it be possible to expand the hint message to tell users to run
> 'git cherry-pick --continue'

Instead of expanding I'd go for replacing?

I'd say the user is tempted for 2 choices,
a) aborting (for various reasons)
b) fix and continue.

So we'd want to point out the way for those two ways and not
give an additional arbitrary way that is not quite (b) but goes that
direction?

I realize this is what the patch does, but I wanted to point out the difference
on expanding vs replacing. I think a 4 line hint is "enough", so I'd object
adding more lines, but changing existing lines is great!

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

[Jeff King]

On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote:

> > Would it be possible to expand the hint message to tell users to run
> > 'git cherry-pick --continue'
> 
> Instead of expanding I'd go for replacing?
> 
> I'd say the user is tempted for 2 choices,
> a) aborting (for various reasons)
> b) fix and continue.

Yeah, I'd agree with this.

I think that advice comes from a time when you could only cherry-pick a
single commit. These days you can do several in a single run, and that's
why "git cherry-pick --continue" was invented.

So I think we would need to make sure that the "cherry-pick --continue"
advice applies in both cases (and that we do not need to give different
advice depending on whether we are in a single or multiple cherry-pick).

I did some basic tests and it _seems_ to work to use --continue in
either case. Probably due to 093a309 (revert: allow cherry-pick
--continue to commit before resuming, 2011-12-10), but I didn't dig.

-Peff

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

[Stephen Morton]

On Tue, Jul 26, 2016 at 4:30 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote:
>
>> > Would it be possible to expand the hint message to tell users to run
>> > 'git cherry-pick --continue'
>>
>> Instead of expanding I'd go for replacing?
>>
>> I'd say the user is tempted for 2 choices,
>> a) aborting (for various reasons)
>> b) fix and continue.
>
> Yeah, I'd agree with this.
>
> I think that advice comes from a time when you could only cherry-pick a
> single commit. These days you can do several in a single run, and that's
> why "git cherry-pick --continue" was invented.
>
> So I think we would need to make sure that the "cherry-pick --continue"
> advice applies in both cases (and that we do not need to give different
> advice depending on whether we are in a single or multiple cherry-pick).
>
> I did some basic tests and it _seems_ to work to use --continue in
> either case. Probably due to 093a309 (revert: allow cherry-pick
> --continue to commit before resuming, 2011-12-10), but I didn't dig.
>
> -Peff

The 'git status' text for a rebase/am/cherry-pick is

    fix conflicts and then run "git <op> --continue"
    use "git <op> --skip" to skip this patch"
    use "git <op> --abort" to cancel the <op> operation

(The --cancel text varies a bit actually, but that's the gist of it.)

The rebase/cherry-pick conflict case should really indicate how to
mark the conflict as resolved as that's the specific situation the
user is in. I don't know if there are guidelines to hint line length,
or how many actions should be on one line but if the above text was
changed to have this as the "fix" text, possibly over two lines, I
think that would do it.

    fix conflicts with 'git add <paths>' or 'git rm <paths>'" and then
run "git <op> --continue"

Stephen

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

[Stephen Morton]

Here is my patch then. (Personally, I would add some capitalization
and punctuation, but I didn't see much of that in the existing code.)
I'm not a regular pull-requester, do I do that, or can somebody else
handle that for me?


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));
        }
 }


Stephen



On Tue, Jul 26, 2016 at 4:44 PM, Stephen Morton
<stephen.c.morton@gmail.com> wrote:
> On Tue, Jul 26, 2016 at 4:30 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote:
>>
>>> > Would it be possible to expand the hint message to tell users to run
>>> > 'git cherry-pick --continue'
>>>
>>> Instead of expanding I'd go for replacing?
>>>
>>> I'd say the user is tempted for 2 choices,
>>> a) aborting (for various reasons)
>>> b) fix and continue.
>>
>> Yeah, I'd agree with this.
>>
>> I think that advice comes from a time when you could only cherry-pick a
>> single commit. These days you can do several in a single run, and that's
>> why "git cherry-pick --continue" was invented.
>>
>> So I think we would need to make sure that the "cherry-pick --continue"
>> advice applies in both cases (and that we do not need to give different
>> advice depending on whether we are in a single or multiple cherry-pick).
>>
>> I did some basic tests and it _seems_ to work to use --continue in
>> either case. Probably due to 093a309 (revert: allow cherry-pick
>> --continue to commit before resuming, 2011-12-10), but I didn't dig.
>>
>> -Peff
>
> The 'git status' text for a rebase/am/cherry-pick is
>
>     fix conflicts and then run "git <op> --continue"
>     use "git <op> --skip" to skip this patch"
>     use "git <op> --abort" to cancel the <op> operation
>
> (The --cancel text varies a bit actually, but that's the gist of it.)
>
> The rebase/cherry-pick conflict case should really indicate how to
> mark the conflict as resolved as that's the specific situation the
> user is in. I don't know if there are guidelines to hint line length,
> or how many actions should be on one line but if the above text was
> changed to have this as the "fix" text, possibly over two lines, I
> think that would do it.
>
>     fix conflicts with 'git add <paths>' or 'git rm <paths>'" and then
> run "git <op> --continue"
>
> Stephen

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

[Johannes Schindelin]

Hi Stephen,

On Wed, 27 Jul 2016, Stephen Morton wrote:

> Here is my patch then. (Personally, I would add some capitalization and
> punctuation, but I didn't see much of that in the existing code.) I'm
> not a regular pull-requester, do I do that, or can somebody else handle
> that for me?

The process of the patch submission is described in
Documentation/SubmittingPatches (yes, it is a bit involved, and it is
slightly easier when you use http://submitgit.herokuapp.com/, but please
note that this process has served us well over one decade).

Please also note that top-posting is highly discouraged on this list:

A: Because it messes up the order in which people normally read text.
>Q: Why is top-posting such a bad thing?
>>A: Top-posting.
>>>Q: What is the most annoying thing in e-mail?

Now to your patch:

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

Ciao,
Johannes

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

[Stephen Morton]

On Wed, Jul 27, 2016 at 11:03 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stephen,
>
> On Wed, 27 Jul 2016, Stephen Morton wrote:
>
>> Here is my patch then. (Personally, I would add some capitalization and
>> punctuation, but I didn't see much of that in the existing code.) I'm
>> not a regular pull-requester, do I do that, or can somebody else handle
>> that for me?
>
> The process of the patch submission is described in
> Documentation/SubmittingPatches (yes, it is a bit involved, and it is
> slightly easier when you use http://submitgit.herokuapp.com/, but please
> note that this process has served us well over one decade).
>
> Please also note that top-posting is highly discouraged on this list:
>
> A: Because it messes up the order in which people normally read text.
>>Q: Why is top-posting such a bad thing?
>>>A: Top-posting.
>>>>Q: What is the most annoying thing in e-mail?
>
> Now to your patch:
>
>> 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?
>
> Ciao,
> Johannes

Ok, knowing that I'm not on the last element of the sequencer is
beyond my git code knowledge. I see that in do_pick_commit() , we do
not have a copy of the todo_list. I would assume that would be
necessary, but I'm not certain. I could
file_exists(git_path_seq_dir()). This works to determine if one or
many commits are being cherry-picked / reverted, although it will
return true even on the last of n cherry-picks. I think that is still
reasonable.

I was trying to just take the same text as 'git status' already
displays. It could indeed be made more concise.

Happy to use the submission process, I just didn't know it. Thanks for
letting me know.

(Yup, sorry about the top-posting. I just wan't careful.)

Stephen

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

[Johannes Schindelin]

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

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

[Stephen Morton]

On Mon, Aug 1, 2016 at 5:12 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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

(Finally getting back to this.)
Something like this, then Johannes?
(I intentionally print the '--continue' hint even in the case where
it's last of n commits that fails.)

~/ws/extern/git (maint *%>) > git diff
diff --git a/sequencer.c b/sequencer.c
index 617a3df..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,14 +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
-                        if  (! file_exists(git_path_seq_dir()))
-                                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'"));
-                        else
+                        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'"));
        }
 }

@@ -445,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;
@@ -600,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;
        }
@@ -964,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;

        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
        if (opts->allow_ff)
@@ -973,7 +974,7 @@ static int pick_commits(struct commit_list
*todo_list, struct replay_opts *opts)

        for (cur = todo_list; cur; cur = cur->next) {
                save_todo(cur, opts);
-               res = do_pick_commit(cur->item, opts);
+               res = do_pick_commit(cur->item, opts, multiple_commits);
                if (res)
                        return res;
        }
@@ -1021,7 +1022,7 @@ static int sequencer_continue(struct replay_opts *opts)
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-       return do_pick_commit(cmit, opts);
+       return do_pick_commit(cmit, opts, 0);
 }

 int sequencer_pick_revisions(struct replay_opts *opts)
~/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;

        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
        if (opts->allow_ff)
@@ -968,7 +974,7 @@ static int pick_commits(struct commit_list
*todo_list, struct replay_opts *opts)

        for (cur = todo_list; cur; cur = cur->next) {
                save_todo(cur, opts);
-               res = do_pick_commit(cur->item, opts);
+               res = do_pick_commit(cur->item, opts, multiple_commits);
                if (res)
                        return res;
        }
@@ -1016,7 +1022,7 @@ static int sequencer_continue(struct replay_opts *opts)
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-       return do_pick_commit(cmit, opts);
+       return do_pick_commit(cmit, opts, 0);
 }

 int sequencer_pick_revisions(struct replay_opts *opts)


------

Stephen

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

[Stephen Morton]

On Mon, Aug 1, 2016 at 5:12 AM, Johannes Schindelin 
<Johannes.Schindelin@gmx.de> wrote:
> 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

Formatting on previous email was terrible, plus the diff wasn't 
performed against origin. Re-sending.

(Finally getting back to this.)

Something like the diff below, then 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;

         setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
         if (opts->allow_ff)
@@ -968,7 +974,7 @@ static int pick_commits(struct commit_list 
*todo_list, struct replay_opts *opts)

         for (cur = todo_list; cur; cur = cur->next) {
                 save_todo(cur, opts);
-               res = do_pick_commit(cur->item, opts);
+               res = do_pick_commit(cur->item, opts, multiple_commits);
                 if (res)
                         return res;
         }
@@ -1016,7 +1022,7 @@ static int sequencer_continue(struct replay_opts 
*opts)
  static int single_pick(struct commit *cmit, struct replay_opts *opts)
  {
         setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-       return do_pick_commit(cmit, opts);
+       return do_pick_commit(cmit, opts, 0);
  }

  int sequencer_pick_revisions(struct replay_opts *opts)



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

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

[Christian Couder]

Hi Stephen,

On Wed, Aug 10, 2016 at 9:21 PM, Stephen Morton
<stephen.morton@nokia.com> wrote:
>
> Formatting on previous email was terrible, plus the diff wasn't performed
> against origin. Re-sending.

Thanks for working on this...

> (Finally getting back to this.)
>
> Something like the diff below, then Johannes?

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

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

Here multiple_commits is not the last argument...

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

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

>  {
>         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;

Why not "last_commit" instead of "multiple_commits"?

Thanks,
Christian.

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

[Remi Galan Alfonso]

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?

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.

Thanks,
Rémi

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

[Johannes Schindelin]

Hi,

On Tue, 16 Aug 2016, Remi Galan Alfonso wrote:

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

I concur with this, and also with Christian's advice to append the
parameter consistently as last one, and also with renaming it to
"last_commit".

Ciao,
Johannes

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

[Stephen Morton]

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

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

[Remi Galan Alfonso]

Stephen Morton <stephen.morton@nokia.com> writes:
> [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?

Either way (3 or 4 lines) I find it strange to have both advices
start in the same way except that one is split and not the other.

I cannot tell if it's a big deal or not to exceed 80 characters
but FWIW most of my stuff (terminal and emacs) is 80 columns
long, and I haven't known the "xterm days".

Thanks,
Rémi

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

[Junio C Hamano]

Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
writes:

> Either way (3 or 4 lines) I find it strange to have both advices
> start in the same way except that one is split and not the other.

True.

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

[Johannes Schindelin]

Hi Stephen,

On Wed, 17 Aug 2016, Stephen Morton wrote:

> > multiple_commits = (todo_list->next) != NULL;
> > Why not "last_commit" instead of "multiple_commits"?
> 
> Because it *isn't*.

Personally, I do prefer to have the simpler instruction if the commit
happens to be the last one cherry-picked.

I do not follow your argument that it makes sense to decide based on the
total number of commits that were to be cherry-picked whether to use the
more cumbersome or the easier command.

Instead, I think the user should be given the simplest advice that gets
them out of the fix most quickly. And if `git commit --amend` is good
enough, we should not advise to use `git cherry-pick --continue`, and that
is that.

Ciao,
Johannes