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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 13, 2012 at 2:05 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]