Martin von Zweigbergk <martin.von.zweigbergk@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html