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, 20:50 UTC
Message-ID: <CAOeW2eGcVQ74WLOOHWvKao9WXfWnJpOhQwE8Jxip_E4SzkFjyA@mail.gmail.com>
In-Reply-To: <7vr4r857au.fsf@alter.siamese.dyndns.org>

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

Show 24 quoted lines

> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> 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;
>
> Sorry, but I do not understand why you frutz with "reverse" after
> prepare, and not before.
>
> I think you can just set no_walk and let setup_revisions() turn it
> off upon seeing a range (this happens in add_pending_object()).

Ah, of course. For some reason I thought that was called from prepare_revision_walk()

> After setup_revisions() returns, if no_walk is still set, you only
> got individual refs without ranges, so no reversing required.

Yes, it's in the other case (e.g. 'git cherry-pick A..C', when no_walk is not set), that we need to set reverse before walking.

> You also need to be careful about "revert" that shares the code;
> when reverting range A..C in your example, you want to undo C and
> then B, and you do not want to reverse them.

Yep. It looks like this, so should be safe. But thanks for the reminder.

  if (opts->action != REPLAY_REVERT)
        opts->revs->reverse ^= 1;