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

(out of order for convenience)
Ramkumar Ramachandra wrote:

Jonathan Nieder wrote:

quoted

Suggested-by: Johannes Sixt <redacted>

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

quoted

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

quoted

+       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