Re: [BUG] multi-commit cherry-pick messes up the order of commits

From: Jeff King <peff@peff.net>
Date: Jan 12, 2012, 19:47 UTC
Message-ID: <20120112194710.GA28148@sigill.intra.peff.net>
In-Reply-To: <CALkWK0kk0mVNaetr=triuVYva7inyx2aZvam81qTVA9=Q=UzGw@mail.gmail.com>

On Fri, Jan 13, 2012 at 12:55:58AM +0530, Ramkumar Ramachandra wrote:

Show 7 quoted lines

> Junio C Hamano wrote:
> > You should be able to look at revs->cmdline and tell if you need to let
> > cherry-pick walk (i.e. "cherry-pick master..next"), or if the user wants
> > individual commits (i.e. "cherry-pick A B C").
> >
> > And you do prepare_revision_walk() only when you need to walk; otherwise
> > you use the contents of revs->pending in order.

I am tempted to suggest that cherry-pick should not feed its arguments to the revision machinery in the first place, but instead accept a set of arguments, each argument of which is either a single commit (interpreted by get_sha1) or a range specifier (which can be fed to setup-revisions). And then get a linearized set of commits for _each_ argument independently and concatenate them (possibly eliminating duplicates). That would make all of these work as most people would expect:

  git cherry-pick A B C
  git cherry-pick A..B
  git cherry-pick A..B B..C

but would be a regression for:

  git cherry-pick B ^A

versus the current code. I suspect that the latter form is not all that commonly used, though, and certainly I would accept it as a casualty of making the "A B C" form work. My only hesitation is that it is in fact a regression.

Show 8 quoted lines

> Okay, just to make sure I understand this correctly: if more than one
> argument is literally specified, I should not set up the revision
> walker and pick the commits listed in revs->pending, correct?  Then,
> when I encounter the following command,
> 
>   $ git cherry-pick maint ^master
> 
> I should just pick two commits: maint, and ^master.

But ^master is not a commit, it is a negation. So it is nonsensical if the arguments are considered independent of each other (you _could_ use a heuristic to guess that they are not independent, but I'd rather not go there). So you'd probably end up just rejecting the arguments.

-Peff