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

[Junio C Hamano <gitster@pobox.com>]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> 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.