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

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

 



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

> Kevin Ballard <kevin@xxxxxx> 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--
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]