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

From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
Date: Aug 15, 2012, 18:22 UTC
Message-ID: <CAOeW2eFK+cKt9Tnh5oe74dU+f8rOOTaWk3KvE2rtUpgcOeDD7g@mail.gmail.com>
In-Reply-To: <7vk3x06ppi.fsf@alter.siamese.dyndns.org>

On Wed, Aug 15, 2012 at 10:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> 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.

Exactly.

Show 6 quoted lines

> 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.

Yes, I noticed that's why "show" has to do it that way.

Show 6 quoted lines

> 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.

Makes sense, I'll try to implement it that way. I was afraid that we would need to call prepare_revision_walk() once first and then if we afterwards find out that we should not walk, we would need to call it again without the reverse option. But after looking at how rev_info.reverse is used, it seem like it's only used in get_revision(), so we can leave it either on or off during the prepare_revision_walk() and the and set appropriately before calling get_revision(), like so:

  init_revisions(&revs);
  revs.no_walk = REVISION_WALK_NO_WALK_UNSORTED;
  setup_revisions(...);
  prepare_revision_walk(&revs);
  revs.reverse = !revs.no_walk;
  // iterate over revisions