Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

[Jonathan Nieder <jrnieder@gmail.com>]

(out of order for convenience)
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>> Suggested-by: Johannes Sixt <j6t@kdbg.org>
>
> Could you link to the corresponding thread with Johannes?

No, I prefer not to.  If I did a good job, the commit message would
explain enough already, and in exceptional cases, the interested
reader can look up the mailing list message the commit comes from and
walk upthread, no?

> Cute feature, although I don't ever recall needing it personally.  Why
> does this relatively esoteric "feature" belong along with the other
> "maintenance patches" in  jn/maint-sequencer-fixes?

Read ahead in the series, or read the cover letter. :)

>                              What I'm really interested in seeing is
> how you persist opts for "cherry-pick --continue" when a single-commit
> pick fails: in other words, how you manage to get " --continue of
> single-pick respects -x" to pass.

That's a good question.  I did the lazy thing and let the existing
"git cherry-pick" logic take care of it (it writes MERGE_MSG).

>> +               struct commit *cmit;
>> +               if (prepare_revision_walk(opts->revs))
>> +                       die(_("revision walk setup failed"));
>> +               cmit = get_revision(opts->revs);
>> +               if (!cmit || get_revision(opts->revs))
>> +                       die("BUG: expected exactly one commit from walk");
>> +               return single_pick(cmit, opts);
>> +       }
>
> I'd have expected you to reuse prepare_revs().

Why?  The purposes do not overlap much.

>> +       if (opts->revs->cmdline.nr == 1 &&
>> +           opts->revs->cmdline.rev->whence == REV_CMD_REV &&
>> +           opts->revs->no_walk &&
>> +           !opts->revs->cmdline.rev->flags) {
>
> Yuck, seriously.
> 1. I'd have expected you to check opts->revs->commits, not
> opts->revs->cmdline.nr.  Okay, you're using the cmdline because the
> revision walk hasn't happened yet.

It would have been easy to do a revision walk and count and I'm using
the cmdline instead deliberately --- the goal really is "anything more
complicated than a simple rev on the command line should trip the
multi-pick logic".

I admit though that I'm not too familiar with the new cmdline_info
API.  I'd welcome a simpler expression with the same effect.

Also, I probably should have included a test that does some

		git cherry-pick picked^..picked

thing and verifies that this is treated as a multi-pick.  And
documented this. :)

Thanks for pointing out the questionable bits.  I am tempted to reroll
to put this after patch 6/7, which would make it possible to use "git
reset --merge" in the commit message for a more natural explanation.

That would also provide an opportunity to reuse some text from [1],
which in hindsight seems to have explained some aspects of each patch
a little more clearly.

Thanks, and hoping that clarifies a little,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/185716/focus=186811