Add some t5501-fetch tests from the official suite

[sirnewton01]

This adds the first three sanity test cases of t5501-fetch from the official test suite.

Note that there is a special patch against the official git-upload-pack to prevent it from invoking dgit for the pack-objects, since the subcommand is not yet ready to support what is required from upload-pack.

Also, there is a cache that is disabled in this PR. When the clone finishes and it attempts to do a hard reset of the repo, the commit ID cannot be converted into a commit object. Disabling the cache in this case appears to decrease the instances of the problem. I think I'm going to need another set of eyes on this area to figure out why there's an apparent race condition.

The run-tests script has been changed so that it no longer invokes make. The reason for this is that this form is much easier to modify to add "-d" and "-v" beside the necessary suites to get more details when tests are failing in travis. Also, you can add DGIT_TRACE in there to get the inner logs of dgit when tests fail.

[sirnewton01]

After dumping some traces during the travis CI builds. There is an intermittent case where the reset after a clone fails because it can't find the commit to reset to in any of the two pack files that are present. There is no difference between the size of those pack files between the success or fail case. The hashes are all different each time though making it harder to determine what's wrong. No sign of corruption. The pack files are successfully parsed but simply do not have the commit in them.

The next question is where the commit ID comes from for the reset.

[driusan]

The commit should come from HEAD, shouldn't it?

Do you have the pack files that are failing? Can you run index-pack on it and compare the pack index generated against the official git client?

[sirnewton01]

Do you have the pack files that are failing? Can you run index-pack on it and compare the pack index generated against the official git client?

I think that the pack files come directly git-upload-pack, no?

[driusan]

git-upload-pack sends the .pack file, but doesn't send the .idx file that accompanies it.. it expects that to be generated by the client who received the pack (via git index-pack). Getting commits looks up the commit in the .idx file, so if it was incorrectly generated, or a delta chain was incorrectly resolved, it would have the wrong commit id recorded.

[sirnewton01]

As I add more tracing this is getting harder to reproduce in the travis build farm. There's seems to be some kind of race happening here, I'm just not quite sure where.

[driusan]

Is it always in the fetch test that it fails? Do you know what command it's failing on?

[sirnewton01]

Is it always in the fetch test that it fails? Do you know what command it's failing on?

It's failing in the reset at the end of the clone command. In one out of every eight travis CI builds or so it will finish fetching, updating the local branches and then fail on the reset because it can't find the commit object to reset to. Very mysterious.

I have recently determined that it does successfully get the commit ID to reset to from the HEAD and in this case remote master branch ref. Next step is to check why the index or pack file is wrong. The pack file seems to be non-empty at 224 bytes. The index is 1156 bytes. Weird that it is larger than the pack file. Maybe the pack file gets truncated somehow.

[driusan]

Is it only in Travis? Is there any way to reproduce locally and get the pack file and index file with the -d argument? (Then maybe compare to a successful run..)

It's pretty normal for the index file to be bigger than the pack for a small packfile, I think.

[driusan]

Also, you can try and log the SHA1s in the pack while it's being indexed if it's only on Travis.. (I still strongly suspect it's a problem with the indexing calculating the wrong sha1, especially since that's done in goroutines so it's the most likely place to get a race condition)

[sirnewton01]

Is it only in Travis? Is there any way to reproduce locally and get the pack file and index file with the -d argument? (Then maybe compare to a successful run..)

It happens on my laptop too, but only every once in a while. Travis seems to be reproducing it more often due to the many parallel runs on different Go versions.

Ok, I'll add logging for the indexing of the pack file and see how it goes. Alternatively, I could put some temporary code in that crawls the pack file if there's an index miss.

[driusan]

If you can reproduce it and put the corrupted and non-corrupted pack/index file somewhere accessible I can try and look into with you.

[sirnewton01]

@driusan here are the pack files and index files for point of comparison. Check the notes.txt for the commit ID that was supposed to be there in the good and bad cases.

packfile-race.zip

[sirnewton01]

The official git index-pack is producing identical indices that dgit does. It must be the pack file doesn't contain the correct objects in the failure case.

[sirnewton01]

This is very strange. The fetch is asking for the commit ID from the git-upload-pack and the remote sends a pack file that doesn't contain the commit in it??

[driusan]

The pack has the commit in the bad packfile, somehow we're just not finding it:

$ cp pack-7470daf9dfa23e039872e5f33a44fa0d25357637.pack-bad pack-7470daf9dfa23e039872e5f33a44fa0d25357637.pack
$ cp pack-7470daf9dfa23e039872e5f33a44fa0d25357637.idx-bad pack-7470daf9dfa23e039872e5f33a44fa0d25357637.idx
$ git verify-pack -v pack-7470daf9dfa23e039872e5f33a44fa0d25357637.pack
0145770bb55e903c69d81f65089030a918af235f commit 172 131 12
22feea448b023a2d864ef94b013735af34d238ba tree   32 43 143
4b48deed3a433909bfd6b6ab3d4b91348b6af464 blob   9 18 186
non delta: 3 objects
pack-7470daf9dfa23e039872e5f33a44fa0d25357637.pack: ok

(the notes say 0145770bb55e903c69d81f65089030a918af235f is the commit you're trying to find..)

Maybe it has something to do with the 0 prefix tripping up some part of the code?

[driusan]

This https://github.com/driusan/dgit/blob/master/git/indexpack.go#L442 needs to be >= .. I can send a PR when I get home from work if you haven't fixed it by then.

[sirnewton01]

@driusan I'm confident now that this is ready to be merged.