{"thread":{"id":"26237","subject":"[BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","startedAt":"2011-01-08T00:19:02Z","lastAt":"2011-01-12T00:54:53Z","messageCount":7,"participants":["Kevin Ballard","Junio C Hamano","Martin von Zweigbergk"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159188","messageId":"CEF26B82-4281-4B8F-A994-DE32EFB92BA7@sb.org","threadId":"26237","inReplyTo":null,"subject":"[BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2011-01-08T00:19:02Z","receivedAt":"2011-01-08T00:19:02Z","isPatch":false,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"-----------------------------------------------------------------------------\nRunning the command `git rev-list --no-walk A B C` should be expected to emit\nthe commits in the same order as they were specified. This is especially\nimportant as the same machinery is used for `git cherry-pick`, and so saying\n`git cherry-pick A B C` can be expected to pick A before B, and B before C.\n\nThis does not happen.\n\nInstead, it appears to be sorting the given commits according to the commit\ntimestamp. To make matters worse, it's not a stable sort. If commits A and\nB have the same timestamp (for example, if they were rebased together), then\ngit cherry-pick tends to apply B before A.\n\nIs there any rationale for this behavior? Any place where it makes sense to\nreorder the commits in this fashion? As far as I'm concerned, typing\n`git cherry-pick A B C` should behave identically to typing\n\n git cherry-pick A\n git cherry-pick B\n git cherry-pick C\n\nregardless of the actual commit dates on A, B, and C.\n\n-Kevin Ballard\n"},{"id":"159193","messageId":"7v62u043ba.fsf@alter.siamese.dyndns.org","threadId":"26237","inReplyTo":"CEF26B82-4281-4B8F-A994-DE32EFB92BA7@sb.org","subject":"Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-08T01:00:41Z","receivedAt":"2011-01-08T01:00:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Ballard writes:\n\n> Is there any rationale for this behavior?\n\nNot a rationale, but an explanation is that most of the time we walk the\nhistory and sorting by date is the first thing that needs to be done, and\nthe --no-walk option was an afterthought that was tucked in.\n\nI suspect that a three-liner patch to revision.c:prepare_revision_walk()\nwould give you what you want. Instead of calling insert-by-date, you\nappend to the tail when revs->no_walk is given, or something.\n"},{"id":"159199","messageId":"BB84A2F6-E6B0-49E4-9DC7-6BA8860623E6@sb.org","threadId":"26237","inReplyTo":"7v62u043ba.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2011-01-08T03:12:20Z","receivedAt":"2011-01-08T03:12:20Z","isPatch":false,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Jan 7, 2011, at 5:00 PM, Junio C Hamano wrote:\n\n> Kevin Ballard writes:\n> \n>> Is there any rationale for this behavior?\n> \n> Not a rationale, but an explanation is that most of the time we walk the\n> history and sorting by date is the first thing that needs to be done, and\n> the --no-walk option was an afterthought that was tucked in.\n> \n> I suspect that a three-liner patch to revision.c:prepare_revision_walk()\n> would give you what you want. Instead of calling insert-by-date, you\n> append to the tail when revs->no_walk is given, or something.\n\nIt almost works, but not quite. My inclination is to say\n`git rev-list --no-walk A B C` should emit A B C in that order. Implemented\nthis way, `git rev-list --no-walk ^HEAD~3 HEAD` emits commits in the wrong\norder, and I can't figure out how to change that. If I implement\n`git rev-list --no-walk A B C` to emit C B A instead, then the test for\n`git cherry-pick --stdin` fails (t3508), and I don't know why.\n\n-Kevin Ballard\n"},{"id":"159200","messageId":"7vk4ig7y0t.fsf@alter.siamese.dyndns.org","threadId":"26237","inReplyTo":"BB84A2F6-E6B0-49E4-9DC7-6BA8860623E6@sb.org","subject":"Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-08T05:41:22Z","receivedAt":"2011-01-08T05:41:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Ballard writes:\n\n> It almost works, but not quite. My inclination is to say\n> `git rev-list --no-walk A B C` should emit A B C in that order. Implemented\n> this way, `git rev-list --no-walk ^HEAD~3 HEAD` emits commits in the wrong\n> order,\n\n\"git rev-list --no-walk ^HEAD~3 HEAD\"? Isn't it a nonsense? If it is \"no\nwalk\", then why do you even list a negative one?\n\n\nAs to cherry-pick, I wouldn't be surprised if it relies on the current\ninternal working of pushing commits in the date order to the queue\nregardless of how they were given from the command line.\n\nIndeed, it does exactly that, and then tries to compensate it---notice\nthat builtin/revert.c:prepare_revs() gives \"revs->reverse\" to it. That\nalso needs to be fixed.\n"},{"id":"159201","messageId":"E2E98544-70D0-4549-8395-DBE2397F0FCB@sb.org","threadId":"26237","inReplyTo":"7vk4ig7y0t.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2011-01-08T05:51:54Z","receivedAt":"2011-01-08T05:51:54Z","isPatch":false,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Jan 7, 2011, at 9:41 PM, Junio C Hamano wrote:\n\n> Kevin Ballard writes:\n> \n>> It almost works, but not quite. My inclination is to say\n>> `git rev-list --no-walk A B C` should emit A B C in that order. Implemented\n>> this way, `git rev-list --no-walk ^HEAD~3 HEAD` emits commits in the wrong\n>> order,\n> \n> \"git rev-list --no-walk ^HEAD~3 HEAD\"? Isn't it a nonsense? If it is \"no\n> walk\", then why do you even list a negative one?\n\nThat seemed odd to me too, but t3508 tests to make sure git cherry-pick accepts\nthat syntax. Specifically it tests `git cherry-pick ^first fourth`. It does\nmake a certain sense, though; it should be (and, I believe, is) equivalent to\nsaying `git rev-list --no-walk HEAD~3..HEAD`, though I don't know if it's\nhandled the same internally.\n\n> As to cherry-pick, I wouldn't be surprised if it relies on the current\n> internal working of pushing commits in the date order to the queue\n> regardless of how they were given from the command line.\n\nMy belief is that it doesn't. It sets the reverse flag, so it gets the oldest\ncommit first. I think it's always just been tested taking commits in the same\norder that they were committed, which seems fine except when two commits have\nthe same date. In that case, if A and B have the same date, then A B C will\nget transformed to B A C. It's possible that this one quirk can be fixed by\nchanging the date test in commit_list_insert_by_date to use <= instead of <,\nbut that still leaves the issue where `git cherry-pick A B C` will sort those\ncommits even if the user explicitly wanted to apply them in the given order.\n\nI suspect this just wasn't noticed before because cherry-pick didn't used to\naccept multiple commits, and after that support was added, nobody's tried to\ncherry-pick commits in a different order than they were committed.\n\n> Indeed, it does exactly that, and then tries to compensate it---notice\n> that builtin/revert.c:prepare_revs() gives \"revs->reverse\" to it. That\n> also needs to be fixed.\n\nI did see that, I just left it out of my explanation.\n\n-Kevin Ballard"},{"id":"159238","messageId":"7vaaja8sxd.fsf@alter.siamese.dyndns.org","threadId":"26237","inReplyTo":"7vk4ig7y0t.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-09T06:58:22Z","receivedAt":"2011-01-09T06:58:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"git rev-list --no-walk ^HEAD~3 HEAD\"? Isn't it a nonsense? If it is \"no\n> walk\", then why do you even list a negative one?\n\nThe above was my thinko.\n\nWhen you explicitly give range to no-walk, you override that no-walk with\n\"please walk\". This is primarily to help Linus who wanted to do \"git show\nHEAD~3..HEAD\"---see how his thinking changed over time by comparing\naa27e461 and f222abde.\n\nThe right fix then would be to first always add in the order things were\ngiven, and sort by date at the end after adding everything to queue and we\nstill have no_walk set, or something like that.\n"},{"id":"159390","messageId":"alpine.DEB.1.10.1101111949230.856@debian","threadId":"26237","inReplyTo":"7vaaja8sxd.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git rev-list --no-walk A B C sorts by commit date incorrectly","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-12T00:54:53Z","receivedAt":"2011-01-12T00:54:53Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Sat, 8 Jan 2011, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > \"git rev-list --no-walk ^HEAD~3 HEAD\"? Isn't it a nonsense? If it is \"no\n> > walk\", then why do you even list a negative one?\n> \n> The above was my thinko.\n> \n> When you explicitly give range to no-walk, you override that no-walk with\n> \"please walk\". This is primarily to help Linus who wanted to do \"git show\n> HEAD~3..HEAD\"---see how his thinking changed over time by comparing\n> aa27e461 and f222abde.\n\nJust a quick note: I didn't know that 'git show' was supposed to\nsupport that syntax, so I had try it out. When I ran 'git show\norigin/master..' on my branch, which was not rebase on top of\norigin/master, it seemed to print the history all the way back. It\nseems to stop only if all the positive refences contain the negative\nreference. Is this intended? Not that it matter much, since 'git log\n-p' seems to do the same thing but stops where I expect...\n"}]}