Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2012, 17:16 UTC
Message-ID: <7vk3x06ppi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAOeW2eH--Y_gq4jBBhd5EQRw+uuaNWrMT-Sua7CeJO-N9KHCLg@mail.gmail.com>

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

Show 5 quoted lines

> So all of the above case give the right result in the end as long
> as the timestamps are chronological, and case 1) gives the right
> result regardless. The other two cases only works in most cases
> because the unexpcted sorting when no-walk is in effect
> counteracts the final reversal.

In short, if you have three commits in a row, A--B--C, with timestamps that are not skewed, and want to replay changes of B and then C in that order, all three you listed ends up doing the right thing. But if you want to apply the change C and then B:

    - "git cherry-pick A..C" is obviously not a way to do so, so we
      won't discuss it further.

    - "git cherry-pick C B" is the most natural way the user would
      want to express this request, but because of the sorting
      (i.e. commit_list_sort_by_date() in prepare_revision_walk(),
      combined with ->reverse in sequencer.c::prepare_revs()), it
      applies B and then C.  That is the real bug.

      Feeding the revs to "git cherry-pick --stdin" in the order the
      user wishes them to be applied has the same issue.

> IIUC, this could be implemented by making cherry-pick iterate
> over rev_info.pending.objects just like 'git show' does when not
> walking.

Yes, that was exactly why I said sequencer.c::prepare_revs() is wrong to call prepare_revision_walk() unconditionally, even when there is no revision walking involved.

I actually think your approach to place the "do not sort when we are not walking" logic in prepare_revision_walk() makes more sense. "show" has to look at pending.objects[] because it needs to show objects other than commits (e.g. "git show :foo"), so there won't be any change in its implementation with your change. It will have to look at pending.objects[] itself.

But "cherry-pick" and sequencer-derived commands only deal with commits. It would be far less error prone to let them call get_revision() repeatedly like all other revision enumerating commands do, than to have them go over the pending.objects[] list, dereferencing tags and using only commits. The resulting callers would be more readable, too, I would think.