Grit::GitRuby::Repository.rev_list can produce results in the wrong order (parent before child)

[pauldowman]

First of all, thanks! Grit is awesome!!

This is my first time playing with Grit or even poking around in git internals, so I apologize if I've gotten confused somewhere here, but it looks like the pure-ruby version of rev_list is assuming that chronological order is correct, but it may not be if two commits have the same timestamp.

With "git rev-list --date-order" (the default ordering anyway) "no parent comes before all of its children".

But GitRuby::Repository.rev_list simply orders by date which can result in a parent coming after it's child if they have the same timestamp.

At least that's my best guess as to what's happening. The original problem I was having was that repo.commits.first wasn't actually the most recent commit, and I think this is the cause.

I know this sounds like a far-out edge case, but I'm using Grit to commit programmatically and it happens in all of my test cases.

Here's an example output from the ruby version of rev_list:

commit f4c455ea040e7fc340b99a4b7dcb36b540531e5b
tree d6b0a59a8caceb37e71da988cc773b86bb30e1cf
author Foo Bar <foo@bar.com> 1287448438 -0700
committer Foo Bar <foo@bar.com> 1287448438 -0700

    Commit #2

commit ed9dc105ca608fc4fb05029920f78dc0851a014b
tree 0600b590f843fb6bcf3fc69541bf77c8b38db72b
parent f4c455ea040e7fc340b99a4b7dcb36b540531e5b
author Foo Bar <foo@bar.com> 1287448438 -0700
committer Foo Bar <foo@bar.com> 1287448438 -0700

    Commit #1

You can see that the second one is the parent of the first. "git rev-list --pretty=raw" outputs them in the opposite order.

If I comment out Grit::GitRuby.rev_list (and let method_missing take care of it), then I get the correct result.

If that all sounds right I'd be happy to try to fix it but I wanted to get a sanity check first.

Thanks.

[pauldowman]

Looks like GitHub-Flavoured Markdown was a bit too smart inside the code block there, those shouldn't be links of course...

[pauldowman]

Crap, I clicked "comment and close" by accident! Can you please re-open? Thanks.

[technoweenie]

Sounds reasonable to me. I feel like Grit::GitRuby should have some tests that compare the GitRuby implementation against the native git version too. If you could come up with a test case that passes with the native command, and fails with GitRuby, that'd be awesome.

[zmalltalker]

@technoweenie: I have created a repository with three commits; the second one was committed before the other ones (by setting the system clock). There's a clone at https://github.com/zmalltalker/strange_order/commits/master

I have a failing test that creates a Grit::Repo from this repo, collects the commits of it (repo.commits) and verifies that their SHAs are in the wrong order. This test is probably at a too high level, but I'm having a hard time figuring out how to target the tests in a better way, probably because I don't really get what Grit::GitRuby::Repository#rev_list actually does.

Would it help at all putting up that test case, or do you have any ideas on how to target the test better?

[philandstuff]

@technoweenie I have a similar repo which exhibits this issue at https://github.com/philandstuff/grit-rev-list-problem-example

The following session demonstrates the difference in order between cmd-line git and git-ruby:

philippotter ~/tmp/grit-rev-list-problem-example $ git rev-list --pretty=raw master
commit 26c184858e44e109cc525632ed82d840b751bde6
tree 8a27fe8715be0cf38d8301c2eedadc180a787b02
parent e569728aceb65171508b07867610b1c36ad0f268
author Philip Potter <philipgpotter@gmail.com> 1329836752 +0100
committer Philip Potter <philipgpotter@gmail.com> 1329836752 +0100

    updated file nodes/dir/file.yaml

commit e569728aceb65171508b07867610b1c36ad0f268
tree 17b6d8b73adee6ea2139008db2c3eab2552ee860
author Philip Potter <philipgpotter@gmail.com> 1329836752 +0100
committer Philip Potter <philipgpotter@gmail.com> 1329836752 +0100

    first commit

philippotter ~/tmp/grit-rev-list-problem-example $ irb
>> require 'grit'
=> true
>> repo = Grit::Repo.new('.')
=> #<Grit::Repo "/Users/philippotter/tmp/grit-rev-list-problem-example/.git">
>> puts repo.git.rev_list({:pretty => "raw"})
commit e569728aceb65171508b07867610b1c36ad0f268
tree 17b6d8b73adee6ea2139008db2c3eab2552ee860
author Philip Potter <philipgpotter@gmail.com> 1329836752 +0100
committer Philip Potter <philipgpotter@gmail.com> 1329836752 +0100

    first commit

commit 26c184858e44e109cc525632ed82d840b751bde6
tree 8a27fe8715be0cf38d8301c2eedadc180a787b02
parent e569728aceb65171508b07867610b1c36ad0f268
author Philip Potter <philipgpotter@gmail.com> 1329836752 +0100
committer Philip Potter <philipgpotter@gmail.com> 1329836752 +0100

    updated file nodes/dir/file.yaml

=> nil

[smarter]

Here's a simple fix for the case where the commit dates are equal(so like https://github.com/philandstuff/grit-rev-list-problem-example but not like https://github.com/zmalltalker/strange_order/commits/master ):

-        log = log.sort { |a, b| a[2] <=> b[2] }.reverse
+        log = log.sort_by.with_index { |a, i| [a[2], -i] }.reverse

This makes the sort stable (see http://bugs.ruby-lang.org/issues/1089) and is enough to fix the issue in this case. Since the commit dates of rebased commits are all equal, this is an important issue, see for example the wrong order at https://gitorious.org/~smarter/libav/smarter-libav/commits/hevc and the corresponding bug report at https://issues.gitorious.org/issues/94 .

[bkeepers]

Grit is no longer maintained. See #183 and check out libgit2/rugged.