From: Junio C Hamano Date: Wed, 15 Aug 2012 17:16:09 GMT Subject: Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order Message-ID: <7vk3x06ppi.fsf@alter.siamese.dyndns.org> In-Reply-To: Martin von Zweigbergk writes: > 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.