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, 06:05 UTC
Message-ID: <CAOeW2eH--Y_gq4jBBhd5EQRw+uuaNWrMT-Sua7CeJO-N9KHCLg@mail.gmail.com>
In-Reply-To: <7vehnacxkf.fsf@alter.siamese.dyndns.org>

On Mon, Aug 13, 2012 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> To connect to the other mail I sent on this thread (in parallel with
>> yours), do you think "git cherrry-pick HEAD HEAD~1" should apply the
>> commits in the same order as "git cherry-pick HEAD~2..HEAD" (which
>> would give the same result if passed to 'rev-list --no-walk' for a
>> linear history) or in the order specified on the command line?
>
> Definitely the latter; I do not think of any semi-reasonable excuse
> to do otherwise.

Indeed. My patches tried to fix the wrong problem.

Sorry I'm slow, but I think I'm finally starting to understand what you've been saying all along about the bug being in sequencer. I'll try to recapitulate a bit for my own and maybe others' understanding. For simplicity, let's assume a linear history with unique timestamps, but not necessarily increasing with each commit.

Currently:

 1) 'git cherry-pick A..C' picks the commits order in
  reverse "default" order

 2) 'git cherry-pick B C' picks the commits in chronological
  order

 3) 'git rev-list --reverse A..C | git cherry-pick --stdin'
  behaves just like 'git cherry-pick B C' and therefore picks
  the commits in chronological order

In cases 2) and 3), even though cherry-pick tells the revision walker not to walk, it still sorts the commits in reverse chronological order. But cherry-pick also tells the revision walker explicitly to reverse the list, so in the end, the order is chronological.

In case 2), however, the first ordering make no difference in this "limited" case (IIUC). So the "default" ordering (which would be C, then B in this case, regardless of timestamps), gets reversed and B gets applied first, followed by C.

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.

When I noticed that the order of inputs to cases 2) and 3) above was ignored, and thinking that 'git rev-list A..C | git cherry-pick --stdin' should mimic 'git cherry-pick A..C', I incorrectly thought that the error was the use of --reverse to 'git rev-list' as well as the sorting done in the no-walk case. I think completely ignored case 2) at this point.

I now think I understand that the sorting done in the no-walk case is indeed incorrect, but that the --reverse passed to rev-list is correct. Instead, the final reversal, which is currently unconditional, should not be done in the no-walk case.

IIUC, this could be implemented by making cherry-pick iterate over rev_info.pending.objects just like 'git show' does when not walking.

Junio, I think it makes sense to just drop this whole series for now. I'll probably include patch 1/4 in my stalled rebase-range series instead. If I understood you correctly, you didn't have any objections to that patch.