Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly

From: Kevin Ballard <kevin@sb.org>
Date: Jan 8, 2011, 05:51 UTC
Message-ID: <E2E98544-70D0-4549-8395-DBE2397F0FCB@sb.org>
In-Reply-To: <7vk4ig7y0t.fsf@alter.siamese.dyndns.org>

On Jan 7, 2011, at 9:41 PM, Junio C Hamano wrote:

Show 9 quoted lines

> Kevin Ballard <kevin@sb.org> writes:
> 
>> It almost works, but not quite. My inclination is to say
>> `git rev-list --no-walk A B C` should emit A B C in that order. Implemented
>> this way, `git rev-list --no-walk ^HEAD~3 HEAD` emits commits in the wrong
>> order,
> 
> "git rev-list --no-walk ^HEAD~3 HEAD"?  Isn't it a nonsense?  If it is "no
> walk", then why do you even list a negative one?

That seemed odd to me too, but t3508 tests to make sure git cherry-pick accepts that syntax. Specifically it tests `git cherry-pick ^first fourth`. It does make a certain sense, though; it should be (and, I believe, is) equivalent to saying `git rev-list --no-walk HEAD~3..HEAD`, though I don't know if it's handled the same internally.

> As to cherry-pick, I wouldn't be surprised if it relies on the current
> internal working of pushing commits in the date order to the queue
> regardless of how they were given from the command line.

My belief is that it doesn't. It sets the reverse flag, so it gets the oldest commit first. I think it's always just been tested taking commits in the same order that they were committed, which seems fine except when two commits have the same date. In that case, if A and B have the same date, then A B C will get transformed to B A C. It's possible that this one quirk can be fixed by changing the date test in commit_list_insert_by_date to use <= instead of <, but that still leaves the issue where `git cherry-pick A B C` will sort those commits even if the user explicitly wanted to apply them in the given order.

I suspect this just wasn't noticed before because cherry-pick didn't used to accept multiple commits, and after that support was added, nobody's tried to cherry-pick commits in a different order than they were committed.

> Indeed, it does exactly that, and then tries to compensate it---notice
> that builtin/revert.c:prepare_revs() gives "revs->reverse" to it.  That
> also needs to be fixed.

I did see that, I just left it out of my explanation.

-Kevin Ballard