From: Martin von Zweigbergk Date: Wed, 15 Aug 2012 20:50:01 GMT Subject: Re: [PATCH 2/4] revisions passed to cherry-pick should be in "default" order Message-ID: In-Reply-To: <7vr4r857au.fsf@alter.siamese.dyndns.org> On Wed, Aug 15, 2012 at 11:39 AM, Junio C Hamano wrote: > Martin von Zweigbergk 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;