{"thread":{"id":"17400","subject":"Bad objects error since upgrading GitHub servers to 1.6.1","startedAt":"2009-01-27T23:04:34Z","lastAt":"2009-01-28T19:00:12Z","messageCount":43,"participants":["PJ Hyett","Johannes Schindelin","Shawn O. Pearce","Junio C Hamano","Linus Torvalds","Björn Steinbrink","Stephen Bannasch","Jeff King","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"102190","messageId":"bab6a2ab0901271504j73dce7afjf8436c3c7c83b770@mail.gmail.com","threadId":"17400","inReplyTo":null,"subject":"Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"PJ Hyett","fromEmail":"pjhyett@gmail.com","sentAt":"2009-01-27T23:04:34Z","receivedAt":"2009-01-27T23:04:34Z","isPatch":false,"sender":{"key":"pjhyett@gmail.com","avatar":"https://gravatar.com/avatar/14ddb30a3ad7207eaf7f99f07c6ba9f0d1d47d9d10bbbd13d06ef1b9b8cdb824?d=mp&s=160"},"body":"Hi folks,\n\nWe upgraded our servers to Git 1.6.1 yesterday and almost immediately\nstarting hearing reports of \"Fatal: Bad Object Error.\" I have\nexperienced this myself, so I'm 99% certain this isn't user error. I'm\nalso using 1.6.1 locally.\n\nI ran into this error after trying to push code to GitHub after a\nseries of simple commits, I was doing absolutely nothing out of the\nordinary.\n\nPlease see our support thread for more examples:\nhttp://support.github.com/discussions/feature-requests/157-fatal-bad-object-error-when-doing-simple-push\n\nAll of the error messages are the same. Can anyone please shed some\nlight on this, I don't see any other recourse but to downgrade Git\nuntil this is resolved.\n\nThanks,\nPJ\n"},{"id":"102194","messageId":"bab6a2ab0901271510y1e3e6912t82ff16e0f912d4b6@mail.gmail.com","threadId":"17400","inReplyTo":"bab6a2ab0901271504j73dce7afjf8436c3c7c83b770@mail.gmail.com","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"PJ Hyett","fromEmail":"pjhyett@gmail.com","sentAt":"2009-01-27T23:10:06Z","receivedAt":"2009-01-27T23:10:06Z","isPatch":false,"sender":{"key":"pjhyett@gmail.com","avatar":"https://gravatar.com/avatar/14ddb30a3ad7207eaf7f99f07c6ba9f0d1d47d9d10bbbd13d06ef1b9b8cdb824?d=mp&s=160"},"body":"To expand further, here's the output from the command line when this happened.\n\n~/Development/github(jetty)$ git push pjhyett jetty\nfatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820\nerror: pack-objects died with strange error\nerror: failed to push some refs to 'git@github.com:pjhyett/github.git'\n\n~/Development/github(jetty)$ git fsck --full\nwarning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains\nzero-padded file modes\nwarning in tree 56fe2a1a3da446606aadf8861feccd592b636a34: contains\nzero-padded file modes\nwarning in tree 99e2a89db2aa9846fc2491b3e4ccd8861e8d3283: contains\nzero-padded file modes\nwarning in tree a6e532d7451bc4aadab86ade84df69180fab4765: contains\nzero-padded file modes\ndangling blob 43611213c3eff91e5fe071cf2907f69a99b630b2\ndangling commit b28b3ecd85a04ecbd1dcb8aedc6886a465f6ab18\ndangling commit 13a70c8687527936d2c375f0f7aefe71142de3c7\ndangling commit 2aa94c1199cb332f58b70c6ce19d8de3c45c6f3c\ndangling blob 61b910e7a97600691fd279e4db3662e751fb5fb7\ndangling commit c4f19e16208d59666323ae0575435720be9b865d\ndangling commit 19245f5d77aa449eebb4a0521b5ff4f6ce1865ab\ndangling commit 122995fb7c9a7e459b0801e0647eb918bea878bf\ndangling commit 7d51e3926b8720d1c7cad19aeb35d6ab4af755fd\ndangling commit 1162dd21370439416967a34915832125e4975239\ndangling blob 8c630b66927f6022a72e457be308de5c9ad9f4e6\ndangling blob 827d4d8855fe6a3a7856ea35cd641192140f2dcd\ndangling commit c9824506855d6cad9b52df115aa267d70872c2cc\ndangling blob fb9bbfc3aa17c5d1ae4e15c862bd874e3476fcfc\ndangling commit 46a4b39245a58ad867010f272991d6233db6288b\ndangling commit d6bf5f30853fecea745559dc3a718113f3619634\ndangling blob d4d66fc4c3a2cbc94d8ed9cb30a6b56daa86e58f\ndangling commit b4f8d7766e8905e5ac6d6cfeeaf7370a716c24a2\n\nVery odd that the bad object didn't appear in the fsck output.\n\nI was able to fix the error by copying a non-corrupted version of the\nobject back into .git/objects and then running a git fetch.\n\n-PJ\n"},{"id":"102199","messageId":"alpine.DEB.1.00.0901280034310.3586@pacific.mpi-cbg.de","threadId":"17400","inReplyTo":"bab6a2ab0901271510y1e3e6912t82ff16e0f912d4b6@mail.gmail.com","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-27T23:37:44Z","receivedAt":"2009-01-27T23:37:44Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 27 Jan 2009, PJ Hyett wrote:\n\n> To expand further, here's the output from the command line when this happened.\n> \n> ~/Development/github(jetty)$ git push pjhyett jetty\n> fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820\n> error: pack-objects died with strange error\n> error: failed to push some refs to 'git@github.com:pjhyett/github.git'\n> \n> ~/Development/github(jetty)$ git fsck --full\n> warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains\n> zero-padded file modes\n> warning in tree 56fe2a1a3da446606aadf8861feccd592b636a34: contains\n> zero-padded file modes\n> warning in tree 99e2a89db2aa9846fc2491b3e4ccd8861e8d3283: contains\n> zero-padded file modes\n> warning in tree a6e532d7451bc4aadab86ade84df69180fab4765: contains\n> zero-padded file modes\n> dangling blob 43611213c3eff91e5fe071cf2907f69a99b630b2\n> dangling commit b28b3ecd85a04ecbd1dcb8aedc6886a465f6ab18\n> dangling commit 13a70c8687527936d2c375f0f7aefe71142de3c7\n> dangling commit 2aa94c1199cb332f58b70c6ce19d8de3c45c6f3c\n> dangling blob 61b910e7a97600691fd279e4db3662e751fb5fb7\n> dangling commit c4f19e16208d59666323ae0575435720be9b865d\n> dangling commit 19245f5d77aa449eebb4a0521b5ff4f6ce1865ab\n> dangling commit 122995fb7c9a7e459b0801e0647eb918bea878bf\n> dangling commit 7d51e3926b8720d1c7cad19aeb35d6ab4af755fd\n> dangling commit 1162dd21370439416967a34915832125e4975239\n> dangling blob 8c630b66927f6022a72e457be308de5c9ad9f4e6\n> dangling blob 827d4d8855fe6a3a7856ea35cd641192140f2dcd\n> dangling commit c9824506855d6cad9b52df115aa267d70872c2cc\n> dangling blob fb9bbfc3aa17c5d1ae4e15c862bd874e3476fcfc\n> dangling commit 46a4b39245a58ad867010f272991d6233db6288b\n> dangling commit d6bf5f30853fecea745559dc3a718113f3619634\n> dangling blob d4d66fc4c3a2cbc94d8ed9cb30a6b56daa86e58f\n> dangling commit b4f8d7766e8905e5ac6d6cfeeaf7370a716c24a2\n> \n> Very odd that the bad object didn't appear in the fsck output.\n> \n> I was able to fix the error by copying a non-corrupted version of the\n> object back into .git/objects and then running a git fetch.\n\nHmm. The only thing I could think of is that the pack-objects used by \nyour git-daemon is somehow not at the right version...\n\nDo you have copies of the \"corrupt\" objects?\n\nCiao,\nDscho\n"},{"id":"102200","messageId":"20090127233939.GD1321@spearce.org","threadId":"17400","inReplyTo":"alpine.DEB.1.00.0901280034310.3586@pacific.mpi-cbg.de","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-27T23:39:39Z","receivedAt":"2009-01-27T23:39:39Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> On Tue, 27 Jan 2009, PJ Hyett wrote:\n> \n> > To expand further, here's the output from the command line when this happened.\n> > \n> > ~/Development/github(jetty)$ git push pjhyett jetty\n> > fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820\n> > error: pack-objects died with strange error\n> > error: failed to push some refs to 'git@github.com:pjhyett/github.git'\n> \n> Hmm. The only thing I could think of is that the pack-objects used by \n> your git-daemon is somehow not at the right version...\n\nNo, that's pack-objects on the client.\n\nIts freaking weird. I don't know why a server side upgrade would\ncause this on the client side.\n\nFWIW, in 1.6.1 the only mention of those bad object messages\nis inside revision.c. I can't see why we'd get one of those\nby itself. I would have expected messages from deeper down\ntoo, like from sha1_file.c.\n \n-- \nShawn.\n"},{"id":"102201","messageId":"7v1vuo1f6d.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090127233939.GD1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-27T23:51:06Z","receivedAt":"2009-01-27T23:51:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Johannes Schindelin wrote:\n>> On Tue, 27 Jan 2009, PJ Hyett wrote:\n>> \n>> > To expand further, here's the output from the command line when this happened.\n>> > \n>> > ~/Development/github(jetty)$ git push pjhyett jetty\n>> > fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820\n>> > error: pack-objects died with strange error\n>> > error: failed to push some refs to 'git@github.com:pjhyett/github.git'\n>> \n>> Hmm. The only thing I could think of is that the pack-objects used by \n>> your git-daemon is somehow not at the right version...\n>\n> No, that's pack-objects on the client.\n>\n> Its freaking weird. I don't know why a server side upgrade would\n> cause this on the client side.\n>\n> FWIW, in 1.6.1 the only mention of those bad object messages\n> is inside revision.c. I can't see why we'd get one of those\n> by itself. I would have expected messages from deeper down\n> too, like from sha1_file.c.\n\nAs we do not know what version github used to run (or for that matter what\ncustom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6\nand did some comparison. The client side pack_object() learned to take\nalternates on the server side into account to avoid pushing objects that\nthe target repository has through its alternates, so it is not totally\nunexpected the client side changes its behaviour depending on what the\nserver does.\n"},{"id":"102204","messageId":"bab6a2ab0901271615h7eadf190n45229d2a83b6dc7f@mail.gmail.com","threadId":"17400","inReplyTo":"7v1vuo1f6d.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"PJ Hyett","fromEmail":"pjhyett@gmail.com","sentAt":"2009-01-28T00:15:56Z","receivedAt":"2009-01-28T00:15:56Z","isPatch":false,"sender":{"key":"pjhyett@gmail.com","avatar":"https://gravatar.com/avatar/14ddb30a3ad7207eaf7f99f07c6ba9f0d1d47d9d10bbbd13d06ef1b9b8cdb824?d=mp&s=160"},"body":"On Tue, Jan 27, 2009 at 3:51 PM, Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n>\n>> Johannes Schindelin wrote:\n>>> On Tue, 27 Jan 2009, PJ Hyett wrote:\n>>>\n>>> > To expand further, here's the output from the command line when this happened.\n>>> >\n>>> > ~/Development/github(jetty)$ git push pjhyett jetty\n>>> > fatal: bad object e13a86261c6e710af8fd4b5fb093b28b8583d820\n>>> > error: pack-objects died with strange error\n>>> > error: failed to push some refs to 'git@github.com:pjhyett/github.git'\n>>>\n>>> Hmm. The only thing I could think of is that the pack-objects used by\n>>> your git-daemon is somehow not at the right version...\n>>\n>> No, that's pack-objects on the client.\n>>\n>> Its freaking weird. I don't know why a server side upgrade would\n>> cause this on the client side.\n>>\n>> FWIW, in 1.6.1 the only mention of those bad object messages\n>> is inside revision.c. I can't see why we'd get one of those\n>> by itself. I would have expected messages from deeper down\n>> too, like from sha1_file.c.\n>\n> As we do not know what version github used to run (or for that matter what\n> custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6\n> and did some comparison. The client side pack_object() learned to take\n> alternates on the server side into account to avoid pushing objects that\n> the target repository has through its alternates, so it is not totally\n> unexpected the client side changes its behaviour depending on what the\n> server does.\n\nOur servers were upgraded from 1.5.5.1 if that helps.\n\n-PJ\n"},{"id":"102207","messageId":"bab6a2ab0901271634x7201130bx4a565bd8bea6967b@mail.gmail.com","threadId":"17400","inReplyTo":"7v1vuo1f6d.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"PJ Hyett","fromEmail":"pjhyett@gmail.com","sentAt":"2009-01-28T00:34:20Z","receivedAt":"2009-01-28T00:34:20Z","isPatch":false,"sender":{"key":"pjhyett@gmail.com","avatar":"https://gravatar.com/avatar/14ddb30a3ad7207eaf7f99f07c6ba9f0d1d47d9d10bbbd13d06ef1b9b8cdb824?d=mp&s=160"},"body":"> As we do not know what version github used to run (or for that matter what\n> custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6\n> and did some comparison. The client side pack_object() learned to take\n> alternates on the server side into account to avoid pushing objects that\n> the target repository has through its alternates, so it is not totally\n> unexpected the client side changes its behaviour depending on what the\n> server does.\n\nThe only custom code we've written was a patch to git-daemon to map\npjhyett/github.git to a sharded location (eg.\n/repositories/1/1e/df/a0/pjhyett/github.git) instead of the default.\n\nThe new alternates code in 1.6.1 sounds like that could be the issue.\n\n-PJ\n"},{"id":"102212","messageId":"alpine.LFD.2.00.0901271655090.3123@localhost.localdomain","threadId":"17400","inReplyTo":"bab6a2ab0901271510y1e3e6912t82ff16e0f912d4b6@mail.gmail.com","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-28T01:00:54Z","receivedAt":"2009-01-28T01:00:54Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 27 Jan 2009, PJ Hyett wrote:\n> \n> ~/Development/github(jetty)$ git fsck --full\n> warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains zero-padded file modes\n> ..\n\nOuch. This is unrelated to your issue, but I'm wondering what project \ncontains these invalid trees, and how they were created.\n\nZero-padded tree entries can cause \"object aliases\", ie two trees that \nhave logically the same contents end up with different data (due to \ndifferent amounts of padding) and thus different SHA1's. It shouldn't be \nserious per se, but it's somethign that really shouldn't happen.\n\nWhat project does it come from, and how did such a tree get generated?\n\n\t\t\tLinus\n"},{"id":"102213","messageId":"7vvds0z1c1.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"bab6a2ab0901271634x7201130bx4a565bd8bea6967b@mail.gmail.com","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T01:06:06Z","receivedAt":"2009-01-28T01:06:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"PJ Hyett writes:\n\n>> As we do not know what version github used to run (or for that matter what\n>> custom code it adds to 1.6.1), I guessed that the previous one was 1.6.0.6\n>> and did some comparison. The client side pack_object() learned to take\n>> alternates on the server side into account to avoid pushing objects that\n>> the target repository has through its alternates, so it is not totally\n>> unexpected the client side changes its behaviour depending on what the\n>> server does.\n>\n> The only custom code we've written was a patch to git-daemon to map\n> pjhyett/github.git to a sharded location (eg.\n> /repositories/1/1e/df/a0/pjhyett/github.git) instead of the default.\n>\n> The new alternates code in 1.6.1 sounds like that could be the issue.\n\nIt could be.\n\nWith the old server, when project A has a forked project A1, and A1\nborrows (via alternates) objects from A, pushing into A1 did not look at\nrefs in A's repository (this all happens on the server end).\n\nWith the new server, the server side also advertises the tips of A's\nbranches as commits that are fully connected, when the client side tries\nto push into A1. Older clients ignored this advertisement, so when they\npushed into A1, because their push did not depend on what's in repository\nA on the server end, did not get affected if repository A (not A1) is\ncorrupted. A new client talking to the server would be affected because\nit believes what the server says.\n\nOlder client ignores this advertisement, so if you are seeing trouble\nreports from people who use older clients, then you can dismiss this\nconjecture as unrelated. But if you see the issue only from people with\nnew clients, this could be just exposing a repository corruption of A (not\nA1) on the server end that people did not know about before.\n"},{"id":"102215","messageId":"20090128011551.GB7503@atjola.homenet","threadId":"17400","inReplyTo":"alpine.LFD.2.00.0901271655090.3123@localhost.localdomain","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2009-01-28T01:15:51Z","receivedAt":"2009-01-28T01:15:51Z","isPatch":false,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2009.01.27 17:00:54 -0800, Linus Torvalds wrote:\n> \n> \n> On Tue, 27 Jan 2009, PJ Hyett wrote:\n> > \n> > ~/Development/github(jetty)$ git fsck --full\n> > warning in tree 0d640d99b492b0c7db034e92d0460a7f84b22356: contains zero-padded file modes\n> > ..\n> \n> Ouch. This is unrelated to your issue, but I'm wondering what project \n> contains these invalid trees, and how they were created.\n> \n> Zero-padded tree entries can cause \"object aliases\", ie two trees that \n> have logically the same contents end up with different data (due to \n> different amounts of padding) and thus different SHA1's. It shouldn't be \n> serious per se, but it's somethign that really shouldn't happen.\n> \n> What project does it come from, and how did such a tree get generated?\n\nI guess that's still from their webinterface that allows to edit file\ndirectly, without having a clone ofthe repo. The initial(?) version used\nto create such broken objects. It also got the order of entries in a\ntree object wrong IIRC. Back then, Scott and myself tracked that down on\n#git, to their ruby(?) stuff that creates the objects. But maybe the\nbreakage is back?\n\nBjörn\n"},{"id":"102216","messageId":"7vk58gz04l.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"7vvds0z1c1.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T01:32:10Z","receivedAt":"2009-01-28T01:32:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> PJ Hyett writes:\n> ...\n>> The new alternates code in 1.6.1 sounds like that could be the issue.\n>\n> It could be.\n>\n> With the old server, when project A has a forked project A1, and A1\n> borrows (via alternates) objects from A, pushing into A1 did not look at\n> refs in A's repository (this all happens on the server end).\n>\n> With the new server, the server side also advertises the tips of A's\n> branches as commits that are fully connected, when the client side tries\n> to push into A1. Older clients ignored this advertisement, so when they\n> pushed into A1, because their push did not depend on what's in repository\n> A on the server end, did not get affected if repository A (not A1) is\n> corrupted. A new client talking to the server would be affected because\n> it believes what the server says.\n>\n> Older client ignores this advertisement, so if you are seeing trouble\n> reports from people who use older clients, then you can dismiss this\n> conjecture as unrelated. But if you see the issue only from people with\n> new clients, this could be just exposing a repository corruption of A (not\n> A1) on the server end that people did not know about before.\n\nThe extra \"we also have these\" advertisement happened as a result of this\ndiscussion:\n\n http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256\n\nI think I know what is going on.\n\nConsider this sequence of events.\n\n (0) Alice creates a project and pushes to public.\n\n alice$ cd $HOME/existing-tarball-extract\n alice$ git init\n alice$ git add .\n alice$ git push /pub/alice.git master\n \n\n (1) Bob forks it.\n\n bob$ git clone --bare --reference /pub/alice.git /pub/bob.git\n\n (2) Bob clones his.\n\n bob$ cd $HOME && git clone /pub/bob.git bob\n\n (3) Alice works more and pushes\n\n alice$ edit foo\n alice$ git add foo\n alice$ git commit -a -m 'more'\n alice$ git push /pub/alice.git master\n\n (4) Bob works more and tries to push to his.\n\n bob$ cd $HOME/bob\n bob$ edit bar\n bob$ git add bar\n bob$ git commit -a -m 'yet more'\n bob$ git push /pub/bob.git master\n\nNow, the new server advertises the objects reachable from alice's branch\ntips as usable cut-off points for pack-objects bob will run when sending.\n\nAnd new builtin-send-pack.c has new code that feeds \"extra\" refs as\n\n\t^SHA1\\n\n\nto the pack-objects process.\n\nThe latest commit Alice created and pushed into her repository is one such\ncommit.\n\nBut the problem is that Bob does *NOT* have it. His \"push\" will run pack\nobject telling it that objects reachable from Alice's top commit do not\nhave to be sent, which was the whole point of doing this new \"we also have\nthese\" advertisement, but instead of ignoring that unknown commit,\npack-objects would say \"Huh? I do not even know that commit\" and dies.\n\nThis can and should be solved by client updates, as 1.6.1 server can work\nwith older client just fine.\n"},{"id":"102218","messageId":"20090128013840.GA7224@atjola.homenet","threadId":"17400","inReplyTo":"7vk58gz04l.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] send-pack: Filter unknown commits from alternates of the remote","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2009-01-28T01:38:40Z","receivedAt":"2009-01-28T01:38:40Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"Since 40c155ff14c, receive-pack on the remote also sends refs from its\nalternates. Unfortunately, we don't filter commits that don't exist in the\nlocal repository from that list. This made us pass those unknown commits\nto pack-objects, causing it to fail with a \"bad object\" error.\n\nSigned-off-by: Björn Steinbrink \n---\n builtin-send-pack.c | 14 +++++++++-----\n 1 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex a9fdbf9..10d7016 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -52,11 +52,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t * parameters by writing to the pipe.\n \t */\n \tfor (i = 0; i < extra->nr; i++) {\n-\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n-\t\tbuf[0] = '^';\n-\t\tbuf[41] = '\\n';\n-\t\tif (!write_or_whine(po.in, buf, 42, \"send-pack: send refs\"))\n-\t\t\tbreak;\n+\t\tif (!is_null_sha1(&extra->array[i][0]) &&\n+\t\t has_sha1_file(&extra->array[i][0])) {\n+\t\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n+\t\t\tbuf[0] = '^';\n+\t\t\tbuf[41] = '\\n';\n+\t\t\tif (!write_or_whine(po.in, buf, 42,\n+\t\t\t\t\t\t\"send-pack: send refs\"))\n+\t\t\t\tbreak;\n+\t\t}\n \t}\n \n \twhile (refs) {\n-- \n1.6.1.284.g5dc13.dirty\n"},{"id":"102219","messageId":"7vfxj4yzjj.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"7vk58gz04l.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T01:44:48Z","receivedAt":"2009-01-28T01:44:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The extra \"we also have these\" advertisement happened as a result of this\n> discussion:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256\n>\n> I think I know what is going on.\n>\n> Consider this sequence of events.\n>\n> (0) Alice creates a project and pushes to public.\n>\n> alice$ cd $HOME/existing-tarball-extract\n> alice$ git init\n> alice$ git add .\n> alice$ git push /pub/alice.git master\n> \n>\n> (1) Bob forks it.\n>\n> bob$ git clone --bare --reference /pub/alice.git /pub/bob.git\n\nI need another /pub/alice.git here, I think, but I hope I got the point\nacross to people who are capable of helping us to resolve this issue.\n\n>\n> (2) Bob clones his.\n>\n> bob$ cd $HOME && git clone /pub/bob.git bob\n>\n> (3) Alice works more and pushes\n>\n> alice$ edit foo\n> alice$ git add foo\n> alice$ git commit -a -m 'more'\n> alice$ git push /pub/alice.git master\n>\n> (4) Bob works more and tries to push to his.\n>\n> bob$ cd $HOME/bob\n> bob$ edit bar\n> bob$ git add bar\n> bob$ git commit -a -m 'yet more'\n> bob$ git push /pub/bob.git master\n>\n> Now, the new server advertises the objects reachable from alice's branch\n> tips as usable cut-off points for pack-objects bob will run when sending.\n>\n> And new builtin-send-pack.c has new code that feeds \"extra\" refs as\n>\n> \t^SHA1\\n\n>\n> to the pack-objects process.\n>\n> The latest commit Alice created and pushed into her repository is one such\n> commit.\n>\n> But the problem is that Bob does *NOT* have it. His \"push\" will run pack\n> object telling it that objects reachable from Alice's top commit do not\n> have to be sent, which was the whole point of doing this new \"we also have\n> these\" advertisement, but instead of ignoring that unknown commit,\n> pack-objects would say \"Huh? I do not even know that commit\" and dies.\n>\n> This can and should be solved by client updates, as 1.6.1 server can work\n> with older client just fine.\n\nHere is a *wrong* fix that should work most of the time. It will\ncertainly appear to fix the issue in the above reproduction recipe.\nYou may want to ask your users to try this to see if it makes their\nsymptom disappear.\n\nWhen we receive \".have\" advertisement, this wrong fix checks if that\nobject is available locally, and it ignores it otherwise.\n\nThis won't be acceptable as the official fix. We should be doing the\nfull connectivity check; in other words, not just \"do we have it\", but \"do\nwe have it *and* is it reachable from any of our own refs\".\n\n connect.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git c/connect.c w/connect.c\nindex 2f23ab3..8026850 100644\n--- c/connect.c\n+++ w/connect.c\n@@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n \n static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)\n {\n+\tif (!has_sha1_file(sha1))\n+\t\treturn;\n+\n \tALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);\n \thashcpy(&(extra->array[extra->nr][0]), sha1);\n \textra->nr++;\n"},{"id":"102221","messageId":"7vbptsyzfe.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128013840.GA7224@atjola.homenet","subject":"Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T01:47:17Z","receivedAt":"2009-01-28T01:47:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Steinbrink writes:\n\n> Since 40c155ff14c, receive-pack on the remote also sends refs from its\n> alternates. Unfortunately, we don't filter commits that don't exist in the\n> local repository from that list. This made us pass those unknown commits\n> to pack-objects, causing it to fail with a \"bad object\" error.\n\nYeah, it is a step in the right direction, but is the *wrong* fix I\ndescribed in my previous message.\n\nOur mails crossed ;-)\n\nAnd I think we should have this in the place where we receive .have,\ni.e. inside add_extra_have() in connect.c\n"},{"id":"102223","messageId":"bab6a2ab0901271757i4602774ahef1d881b7ed58097@mail.gmail.com","threadId":"17400","inReplyTo":"7vfxj4yzjj.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"PJ Hyett","fromEmail":"pjhyett@gmail.com","sentAt":"2009-01-28T01:57:49Z","receivedAt":"2009-01-28T01:57:49Z","isPatch":false,"sender":{"key":"pjhyett@gmail.com","avatar":"https://gravatar.com/avatar/14ddb30a3ad7207eaf7f99f07c6ba9f0d1d47d9d10bbbd13d06ef1b9b8cdb824?d=mp&s=160"},"body":"On Tue, Jan 27, 2009 at 5:44 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> The extra \"we also have these\" advertisement happened as a result of this\n>> discussion:\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256\n>>\n>> I think I know what is going on.\n>>\n>> Consider this sequence of events.\n>>\n>> (0) Alice creates a project and pushes to public.\n>>\n>> alice$ cd $HOME/existing-tarball-extract\n>> alice$ git init\n>> alice$ git add .\n>> alice$ git push /pub/alice.git master\n>>\n>>\n>> (1) Bob forks it.\n>>\n>> bob$ git clone --bare --reference /pub/alice.git /pub/bob.git\n>\n> I need another /pub/alice.git here, I think, but I hope I got the point\n> across to people who are capable of helping us to resolve this issue.\n>\n>>\n>> (2) Bob clones his.\n>>\n>> bob$ cd $HOME && git clone /pub/bob.git bob\n>>\n>> (3) Alice works more and pushes\n>>\n>> alice$ edit foo\n>> alice$ git add foo\n>> alice$ git commit -a -m 'more'\n>> alice$ git push /pub/alice.git master\n>>\n>> (4) Bob works more and tries to push to his.\n>>\n>> bob$ cd $HOME/bob\n>> bob$ edit bar\n>> bob$ git add bar\n>> bob$ git commit -a -m 'yet more'\n>> bob$ git push /pub/bob.git master\n>>\n>> Now, the new server advertises the objects reachable from alice's branch\n>> tips as usable cut-off points for pack-objects bob will run when sending.\n>>\n>> And new builtin-send-pack.c has new code that feeds \"extra\" refs as\n>>\n>> ^SHA1\\n\n>>\n>> to the pack-objects process.\n>>\n>> The latest commit Alice created and pushed into her repository is one such\n>> commit.\n>>\n>> But the problem is that Bob does *NOT* have it. His \"push\" will run pack\n>> object telling it that objects reachable from Alice's top commit do not\n>> have to be sent, which was the whole point of doing this new \"we also have\n>> these\" advertisement, but instead of ignoring that unknown commit,\n>> pack-objects would say \"Huh? I do not even know that commit\" and dies.\n>>\n>> This can and should be solved by client updates, as 1.6.1 server can work\n>> with older client just fine.\n>\n> Here is a *wrong* fix that should work most of the time. It will\n> certainly appear to fix the issue in the above reproduction recipe.\n> You may want to ask your users to try this to see if it makes their\n> symptom disappear.\n>\n> When we receive \".have\" advertisement, this wrong fix checks if that\n> object is available locally, and it ignores it otherwise.\n>\n> This won't be acceptable as the official fix. We should be doing the\n> full connectivity check; in other words, not just \"do we have it\", but \"do\n> we have it *and* is it reachable from any of our own refs\".\n>\n> connect.c | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n>\n> diff --git c/connect.c w/connect.c\n> index 2f23ab3..8026850 100644\n> --- c/connect.c\n> +++ w/connect.c\n> @@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n>\n> static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)\n> {\n> + if (!has_sha1_file(sha1))\n> + return;\n> +\n> ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);\n> hashcpy(&(extra->array[extra->nr][0]), sha1);\n> extra->nr++;\n>\n\nThank you for your detailed response. To answer your previous\nquestion, all of the bug reports have been made by users running\n1.6.1.\n\nMy concern is that we obviously have no control over what version of\nGit our 50k+ users are running, and we will be perpetually stuck\nrunning 1.5 on the servers to account for this issue.\n\nIs there any possibility to have the server code in an upcoming\nrelease account for clients running 1.6.1?\n\n-PJ\n"},{"id":"102224","messageId":"20090128020220.GE1321@spearce.org","threadId":"17400","inReplyTo":"bab6a2ab0901271757i4602774ahef1d881b7ed58097@mail.gmail.com","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-28T02:02:20Z","receivedAt":"2009-01-28T02:02:20Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"PJ Hyett wrote:\n> \n> Is there any possibility to have the server code in an upcoming\n> release account for clients running 1.6.1?\n\nI can't think off-hand of a way for the server to know what version\nthe client is. There's nothing really different in the protocol\nbetween a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of\ninclude-tag) or later client.\n\nSo there's no easy way for the server to work around this possible\nglitch in the client.\n\n-- \nShawn.\n"},{"id":"102231","messageId":"7v3af4yvmu.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128020220.GE1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T03:09:13Z","receivedAt":"2009-01-28T03:09:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> PJ Hyett wrote:\n>> \n>> Is there any possibility to have the server code in an upcoming\n>> release account for clients running 1.6.1?\n>\n> I can't think off-hand of a way for the server to know what version\n> the client is. There's nothing really different in the protocol\n> between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of\n> include-tag) or later client.\n\nHmm, I am puzzled.\n\nI do not know how 41fa7d2 (Teach git-fetch to exploit server side\nautomatic tag following, 2008-03-03), which is about the conversation\nbetween fetch-pack and upload-pack, is relevant to the issue at hand,\nwhich is about the conversation between send-pack and receive-pack.\n\nIn send-pack receive-pack protocol, the server talks first before\nlistening to the client, and the .have data is in this first part of the\nconversation.\n\nBy the way, I think Documentation/technical/pack-protocol.txt needs to be\nupdated. send-pack receive-pack protocol uses C and S to mean receiver\nand sender respectively. We should at least s/C/R/ that part, and\npossibly add description about \".have\" thing.\n"},{"id":"102235","messageId":"20090128033020.GF1321@spearce.org","threadId":"17400","inReplyTo":"7v3af4yvmu.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-28T03:30:20Z","receivedAt":"2009-01-28T03:30:20Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > PJ Hyett wrote:\n> >> \n> >> Is there any possibility to have the server code in an upcoming\n> >> release account for clients running 1.6.1?\n> >\n> > I can't think off-hand of a way for the server to know what version\n> > the client is. There's nothing really different in the protocol\n> > between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of\n> > include-tag) or later client.\n> \n> Hmm, I am puzzled.\n> \n> I do not know how 41fa7d2 (Teach git-fetch to exploit server side\n> automatic tag following, 2008-03-03), which is about the conversation\n> between fetch-pack and upload-pack, is relevant to the issue at hand,\n> which is about the conversation between send-pack and receive-pack.\n\nOh, right, its not. I was pointing out that the last time the\nprotocol changed in a way the server can infer something about the\nclient, which IIRC was 41fa7d2, we still don't have a way to tell\nwhat the client is.\n \n> In send-pack receive-pack protocol, the server talks first before\n> listening to the client, and the .have data is in this first part of the\n> conversation.\n\nBut as you rightly point out, that's the real problem. Since the\nserver talks first, there's no way for the server to avoid giving\nout the newer \".have\" lines to a buggy client, as it knows nothing\nat all about the client. Not even its capabilities.\n\nPJ - the short story here is, to forever work around these buggy\n1.6.1 clients, you'd have to either run an old server forever,\nor forever run a patched server that disables the newer \".have\"\nextension in the advertised data written by git-upload-pack.\nThere just isn't a way to hide this from the client.\n\nReally though, I'd recommend getting your users to upgrade to a\nnon-buggy client. Pasky has the same problem on repo.or.cz; if\nhe doesn't have it already he will soon when he upgrades...\n\n-- \nShawn.\n"},{"id":"102236","messageId":"7vskn4xfyg.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128013840.GA7224@atjola.homenet","subject":"Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T03:33:11Z","receivedAt":"2009-01-28T03:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Steinbrink writes:\n\n> Since 40c155ff14c, receive-pack on the remote also sends refs from its\n> alternates. Unfortunately, we don't filter commits that don't exist in the\n> local repository from that list. This made us pass those unknown commits\n> to pack-objects, causing it to fail with a \"bad object\" error.\n>\n> Signed-off-by: Björn Steinbrink \n> ---\n> builtin-send-pack.c | 14 +++++++++-----\n> 1 files changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index a9fdbf9..10d7016 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -52,11 +52,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n> \t * parameters by writing to the pipe.\n> \t */\n> \tfor (i = 0; i < extra->nr; i++) {\n> -\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n> -\t\tbuf[0] = '^';\n> -\t\tbuf[41] = '\\n';\n> -\t\tif (!write_or_whine(po.in, buf, 42, \"send-pack: send refs\"))\n> -\t\t\tbreak;\n> +\t\tif (!is_null_sha1(&extra->array[i][0]) &&\n> +\t\t has_sha1_file(&extra->array[i][0])) {\n> +\t\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n> +\t\t\tbuf[0] = '^';\n> +\t\t\tbuf[41] = '\\n';\n> +\t\t\tif (!write_or_whine(po.in, buf, 42,\n> +\t\t\t\t\t\t\"send-pack: send refs\"))\n> +\t\t\t\tbreak;\n> +\t\t}\n> \t}\n> \n> \twhile (refs) {\n\nActually I changed my mind.\n\nWe have the exactly the same issue for the real refs the target repository\nhas, not just borrowed phantom refs, in the code from day one of git-push.\nIn other words, this issue predates the \".have\" extension, and your update\nis in line with how the codepath for the real refs does its thing. So\nyour fix is not worse than the existing code.\n\nIt can be argued that at least in the \"real ref\" case you are in control\nof both ends and if you have a disconnected chain in your local repository\nthat you do not have a ref for, you are screwing yourself, and it is your\nproblem. But when you forked your repository from somebody else on a\nhosting site like github, you do not have much control over the other end\n(because it is a closed site you cannot ssh in to diagnose what is really\ngoing on), and if you do not exactly know from whom your hosted repository\nis borrowing, it is more likely that you will get into a situation where\nyou may have objects near the tip without having the full chain after an\naborted transfer, and the insufficient check of doing only has_sha1_file()\nmay become a larger issue in such a settings.\n\nBut still, let's take the approach I labeled as *wrong* as an interim\nsolution for the immediate future.\n\nI'd prefer a small helper function to consolidate the duplicated code,\nlike the attached patch, though. How about doing it like this?\n\n builtin-send-pack.c | 46 ++++++++++++++++++++++++----------------------\n 1 files changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git c/builtin-send-pack.c w/builtin-send-pack.c\nindex a9fdbf9..2d24cf2 100644\n--- c/builtin-send-pack.c\n+++ w/builtin-send-pack.c\n@@ -15,6 +15,23 @@ static struct send_pack_args args = {\n \t/* .receivepack = */ \"git-receive-pack\",\n };\n \n+static int feed_object(const unsigned char *theirs, int fd, int negative)\n+{\n+\tchar buf[42];\n+\n+\tif (!has_sha1_file(theirs))\n+\t\treturn 1;\n+\t/*\n+\t * NEEDSWORK: we should not be satisfied by simply having\n+\t * theirs, but should be making sure it is reachable from\n+\t * some of our refs.\n+\t */\n+\tmemcpy(buf + negative, sha1_to_hex(theirs), 40);\n+\tif (negative)\n+\t\tbuf[0] = '^';\n+\tbuf[40 + negative] = '\\n';\n+\treturn write_or_whine(fd, buf, 41 + negative, \"send-pack: send refs\");\n+}\n /*\n * Make a pack stream and spit it out into file descriptor fd\n */\n@@ -35,7 +52,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t};\n \tstruct child_process po;\n \tint i;\n-\tchar buf[42];\n \n \tif (args.use_thin_pack)\n \t\targv[4] = \"--thin\";\n@@ -51,31 +67,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t * We feed the pack-objects we just spawned with revision\n \t * parameters by writing to the pipe.\n \t */\n-\tfor (i = 0; i < extra->nr; i++) {\n-\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n-\t\tbuf[0] = '^';\n-\t\tbuf[41] = '\\n';\n-\t\tif (!write_or_whine(po.in, buf, 42, \"send-pack: send refs\"))\n+\tfor (i = 0; i < extra->nr; i++)\n+\t\tif (!feed_object(extra->array[i], po.in, 1))\n \t\t\tbreak;\n-\t}\n \n \twhile (refs) {\n \t\tif (!is_null_sha1(refs->old_sha1) &&\n-\t\t has_sha1_file(refs->old_sha1)) {\n-\t\t\tmemcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);\n-\t\t\tbuf[0] = '^';\n-\t\t\tbuf[41] = '\\n';\n-\t\t\tif (!write_or_whine(po.in, buf, 42,\n-\t\t\t\t\t\t\"send-pack: send refs\"))\n-\t\t\t\tbreak;\n-\t\t}\n-\t\tif (!is_null_sha1(refs->new_sha1)) {\n-\t\t\tmemcpy(buf, sha1_to_hex(refs->new_sha1), 40);\n-\t\t\tbuf[40] = '\\n';\n-\t\t\tif (!write_or_whine(po.in, buf, 41,\n-\t\t\t\t\t\t\"send-pack: send refs\"))\n-\t\t\t\tbreak;\n-\t\t}\n+\t\t !feed_object(refs->old_sha1, po.in, 1))\n+\t\t\tbreak;\n+\t\tif (!is_null_sha1(refs->new_sha1) &&\n+\t\t !feed_object(refs->new_sha1, po.in, 0))\n+\t\t\tbreak;\n \t\trefs = refs->next;\n \t}\n \n.\n"},{"id":"102238","messageId":"p06240812c5a58676a1e2@[63.138.152.192]","threadId":"17400","inReplyTo":"20090128033020.GF1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Stephen Bannasch","fromEmail":"stephen.bannasch@deanbrook.org","sentAt":"2009-01-28T03:52:42Z","receivedAt":"2009-01-28T03:52:42Z","isPatch":false,"sender":{"key":"stephen.bannasch@deanbrook.org","avatar":null},"body":"At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:\n>PJ - the short story here is, to forever work around these buggy\n>1.6.1 clients, you'd have to either run an old server forever,\n>or forever run a patched server that disables the newer \".have\"\n>extension in the advertised data written by git-upload-pack.\n>There just isn't a way to hide this from the client.\n>\n>Really though, I'd recommend getting your users to upgrade to a\n>non-buggy client. Pasky has the same problem on repo.or.cz; if\n>he doesn't have it already he will soon when he upgrades...\n\nDo you know if this problem is fixed in tag v1.6.1.1?\n\n Tagger: Junio C Hamano \n Date: Sun Jan 25 12:41:48 2009 -0800\n commit 5c415311f743ccb11a50f350ff1c385778f049d6\n"},{"id":"102239","messageId":"20090128035703.GG1321@spearce.org","threadId":"17400","inReplyTo":"p06240812c5a58676a1e2@[63.138.152.192]","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-28T03:57:03Z","receivedAt":"2009-01-28T03:57:03Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Stephen Bannasch wrote:\n> At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:\n>> PJ - the short story here is, to forever work around these buggy\n>> 1.6.1 clients, you'd have to either run an old server forever,\n>> or forever run a patched server that disables the newer \".have\"\n>> extension in the advertised data written by git-upload-pack.\n>> There just isn't a way to hide this from the client.\n>>\n>> Really though, I'd recommend getting your users to upgrade to a\n>> non-buggy client. Pasky has the same problem on repo.or.cz; if\n>> he doesn't have it already he will soon when he upgrades...\n>\n> Do you know if this problem is fixed in tag v1.6.1.1?\n>\n> Tagger: Junio C Hamano \n> Date: Sun Jan 25 12:41:48 2009 -0800\n> commit 5c415311f743ccb11a50f350ff1c385778f049d6\n\nWithout even checking Git I can tell you it isn't fixed by 1.6.1.1.\n\nThe date on the tag is Jan 25th, 2 full days before PJ reported\nthe problem and a solution was proposed...\n\n-- \nShawn.\n"},{"id":"102240","messageId":"20090128035804.GC7503@atjola.homenet","threadId":"17400","inReplyTo":"7vskn4xfyg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2009-01-28T03:58:04Z","receivedAt":"2009-01-28T03:58:04Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2009.01.27 19:33:11 -0800, Junio C Hamano wrote:\n> It can be argued that at least in the \"real ref\" case you are in control\n> of both ends and if you have a disconnected chain in your local repository\n> that you do not have a ref for, you are screwing yourself, and it is your\n> problem. But when you forked your repository from somebody else on a\n> hosting site like github, you do not have much control over the other end\n> (because it is a closed site you cannot ssh in to diagnose what is really\n> going on), and if you do not exactly know from whom your hosted repository\n> is borrowing, it is more likely that you will get into a situation where\n> you may have objects near the tip without having the full chain after an\n> aborted transfer, and the insufficient check of doing only has_sha1_file()\n> may become a larger issue in such a settings.\n\nUhm, it might be obvious, but what exactly could go wrong? Do we need to\nfetch from multiple repos when alternates are involved? Or how would we\nend up with a broken chain? I mean, it starts to make some sense to me\nwhy we would need the connectivity check, but how do we end up with a\n\"partial\" fetch at all?\n\n> I'd prefer a small helper function to consolidate the duplicated code,\n> like the attached patch, though. How about doing it like this?\n\nYeah, that looks a lot nicer :-)\n\nBjörn\n"},{"id":"102242","messageId":"7vljswxe3d.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128035804.GC7503@atjola.homenet","subject":"Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T04:13:26Z","receivedAt":"2009-01-28T04:13:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Steinbrink writes:\n\n> Uhm, it might be obvious, but what exactly could go wrong?\n\nBetween the refs and your object store, there is a contract that\nguarantees that everything that is reachable from your refs are complete\nand you won't hit unreachable object while traversing the reachability\nchain from them. But your object store can contain other garbage\nobjects. The contract is one of the things \"git fsck\" checks.\n\nImagine you have fetched from somewhere with a commit walker (e.g. fetch\nover http), that started fetching from the tip commit and its associated\nobjects, and then got interrupted. Such a transfer will leave the objects\nin your local repository but it is safe because it won't update your refs.\n\n>> I'd prefer a small helper function to consolidate the duplicated code,\n>> like the attached patch, though. How about doing it like this?\n>\n> Yeah, that looks a lot nicer :-)\n\nBut it was broken. The initial check feed_object() does with\nhas_sha1_file() and NEEDSWORK comment needs to be inside\n\n\tif (negative) {\n\t\tif (!has_sha1_file(theirs))\n\t\t\treturn 1;\n\t\t/*\n\t\t * NEEDSWORK: we should not be satisfied by simply having\n\t\t * theirs, but should be making sure it is reachable from\n\t\t * some of our refs.\n\t\t */\n\t}\n\nto make sure we won't trigger the availability or connectivity check for\npositive refs.\n"},{"id":"102246","messageId":"7v63k0xd7z.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128035804.GC7503@atjola.homenet","subject":"Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T04:32:16Z","receivedAt":"2009-01-28T04:32:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Steinbrink writes:\n\n> ... Do we need to\n> fetch from multiple repos when alternates are involved?\n\nThis part is a slightly different issue than the rest of your message, so\nI'll answer separately.\n\nYes, in the example, if Bob fetched from Alice before he pushed, his push\nwill succeed with the 1.6.1 send-pack.\n\nBut that is a workaround, and it is not a fix.\n"},{"id":"102248","messageId":"7v1vuoxcxk.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128033020.GF1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T04:38:31Z","receivedAt":"2009-01-28T04:38:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Junio C Hamano wrote:\n>> \n>> Hmm, I am puzzled.\n>> \n>> I do not know how 41fa7d2 (Teach git-fetch to exploit server side\n>> automatic tag following, 2008-03-03), which is about the conversation\n>> between fetch-pack and upload-pack, is relevant to the issue at hand,\n>> which is about the conversation between send-pack and receive-pack.\n>\n> Oh, right, its not. I was pointing out that the last time the\n> protocol changed in a way the server can infer something about the\n> client, which IIRC was 41fa7d2, we still don't have a way to tell\n> what the client is.\n\nBut you are still talking as if there is one protocol you can call \"the\nprotocol\", but it is not. The send-pack receive-pack protocol is on topic\nin this thread; the quoted commit was about a separate and independent\nfetch-pack upload-pack protocol. It does not matter when that unrelated\nprotocol was enhanced.\n\n> PJ - the short story here is, to forever work around these buggy\n> 1.6.1 clients, you'd have to either run an old server forever,\n> or forever run a patched server that disables the newer \".have\"\n> extension in the advertised data written by git-upload-pack.\n> There just isn't a way to hide this from the client.\n>\n> Really though, I'd recommend getting your users to upgrade to a\n> non-buggy client. Pasky has the same problem on repo.or.cz; if\n> he doesn't have it already he will soon when he upgrades...\n\nYeah, I'll apply the attached patch to 'maint' and it will be in the next\n1.6.1.X maintenance release. I suspect that your 1.6.1 users are the ones\nwho like to be on the cutting edge, and it wouldn't be unreasonable to\nexpect that they will update soon (1.6.1 has been out only for one month).\n\n-- >8 --\nSubject: [PATCH] send-pack: do not send unknown object name from \".have\" to pack-objects\n\nv1.6.1 introduced \".have\" extension to the protocol to allow the receiving\nside to advertise objects that are reachable from refs in the repositories\nit borrows from. This was meant to be used by the sending side to avoid\nsending such objects; they are already available through the alternates\nmechanism.\n\nThe client side implementation in v1.6.1, which was introduced with\n40c155f (push: prepare sender to receive extended ref information from the\nreceiver, 2008-09-09) aka v1.6.1-rc1~203^2~1, were faulty in that it did\nnot consider the possiblity that the repository receiver borrows from\nmight have objects it does not know about.\n\nThis implements a tentative fix.\n\nSigned-off-by: Junio C Hamano \n---\n builtin-send-pack.c | 49 +++++++++++++++++++++++++++----------------------\n 1 files changed, 27 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex a9fdbf9..fae597b 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -15,6 +15,26 @@ static struct send_pack_args args = {\n \t/* .receivepack = */ \"git-receive-pack\",\n };\n \n+static int feed_object(const unsigned char *theirs, int fd, int negative)\n+{\n+\tchar buf[42];\n+\n+\tif (negative) {\n+\t\tif (!has_sha1_file(theirs))\n+\t\t\treturn 1;\n+\t\t/*\n+\t\t * NEEDSWORK: we should not be satisfied by simply having\n+\t\t * theirs, but should be making sure it is reachable from\n+\t\t * some of our refs.\n+\t\t */\n+\t}\n+\n+\tmemcpy(buf + negative, sha1_to_hex(theirs), 40);\n+\tif (negative)\n+\t\tbuf[0] = '^';\n+\tbuf[40 + negative] = '\\n';\n+\treturn write_or_whine(fd, buf, 41 + negative, \"send-pack: send refs\");\n+}\n /*\n * Make a pack stream and spit it out into file descriptor fd\n */\n@@ -35,7 +55,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t};\n \tstruct child_process po;\n \tint i;\n-\tchar buf[42];\n \n \tif (args.use_thin_pack)\n \t\targv[4] = \"--thin\";\n@@ -51,31 +70,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t * We feed the pack-objects we just spawned with revision\n \t * parameters by writing to the pipe.\n \t */\n-\tfor (i = 0; i < extra->nr; i++) {\n-\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n-\t\tbuf[0] = '^';\n-\t\tbuf[41] = '\\n';\n-\t\tif (!write_or_whine(po.in, buf, 42, \"send-pack: send refs\"))\n+\tfor (i = 0; i < extra->nr; i++)\n+\t\tif (!feed_object(extra->array[i], po.in, 1))\n \t\t\tbreak;\n-\t}\n \n \twhile (refs) {\n \t\tif (!is_null_sha1(refs->old_sha1) &&\n-\t\t has_sha1_file(refs->old_sha1)) {\n-\t\t\tmemcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);\n-\t\t\tbuf[0] = '^';\n-\t\t\tbuf[41] = '\\n';\n-\t\t\tif (!write_or_whine(po.in, buf, 42,\n-\t\t\t\t\t\t\"send-pack: send refs\"))\n-\t\t\t\tbreak;\n-\t\t}\n-\t\tif (!is_null_sha1(refs->new_sha1)) {\n-\t\t\tmemcpy(buf, sha1_to_hex(refs->new_sha1), 40);\n-\t\t\tbuf[40] = '\\n';\n-\t\t\tif (!write_or_whine(po.in, buf, 41,\n-\t\t\t\t\t\t\"send-pack: send refs\"))\n-\t\t\t\tbreak;\n-\t\t}\n+\t\t !feed_object(refs->old_sha1, po.in, 1))\n+\t\t\tbreak;\n+\t\tif (!is_null_sha1(refs->new_sha1) &&\n+\t\t !feed_object(refs->new_sha1, po.in, 0))\n+\t\t\tbreak;\n \t\trefs = refs->next;\n \t}\n \n-- \n1.6.1.1.273.g0e555\n"},{"id":"102249","messageId":"20090128044150.GI1321@spearce.org","threadId":"17400","inReplyTo":"7v1vuoxcxk.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-28T04:41:50Z","receivedAt":"2009-01-28T04:41:50Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> >\n> > Oh, right, its not. I was pointing out that the last time the\n> > protocol changed in a way the server can infer something about the\n> > client, which IIRC was 41fa7d2, we still don't have a way to tell\n> > what the client is.\n> \n> But you are still talking as if there is one protocol you can call \"the\n> protocol\", but it is not. The send-pack receive-pack protocol is on topic\n> in this thread; the quoted commit was about a separate and independent\n> fetch-pack upload-pack protocol. It does not matter when that unrelated\n> protocol was enhanced.\n\nBlargh. Of course you are right. Its been a long 2 months for me\nat work. I'm too #@*#@!@! tired to keep the basics straight anymore.\n\nI'm going to shut up now.\n \n-- \nShawn.\n"},{"id":"102254","messageId":"7vr62ovvbe.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"p06240812c5a58676a1e2@[63.138.152.192]","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T05:44:21Z","receivedAt":"2009-01-28T05:44:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Bannasch writes:\n\n> At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:\n>>PJ - the short story here is, to forever work around these buggy\n>>1.6.1 clients, you'd have to either run an old server forever,\n>>or forever run a patched server that disables the newer \".have\"\n>>extension in the advertised data written by git-upload-pack.\n>>There just isn't a way to hide this from the client.\n>>\n>>Really though, I'd recommend getting your users to upgrade to a\n>>non-buggy client. Pasky has the same problem on repo.or.cz; if\n>>he doesn't have it already he will soon when he upgrades...\n>\n> Do you know if this problem is fixed in tag v1.6.1.1?\n>\n> Tagger: Junio C Hamano \n> Date: Sun Jan 25 12:41:48 2009 -0800\n> commit 5c415311f743ccb11a50f350ff1c385778f049d6\n\nGive us a break. This was reported today and diagnosed a few hours ago.\n\nIn the meantime, here is a minimum patch that should help you to help us\nconvince the approach we decided to take would work fine for people.\n\n\n\n connect.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git c/connect.c w/connect.c\nindex 2f23ab3..8026850 100644\n--- c/connect.c\n+++ w/connect.c\n@@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n \n static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)\n {\n+\tif (!has_sha1_file(sha1))\n+\t\treturn;\n+\n \tALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);\n \thashcpy(&(extra->array[extra->nr][0]), sha1);\n \textra->nr++;\n"},{"id":"102259","messageId":"7vd4e7x5ov.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128044150.GI1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T07:14:56Z","receivedAt":"2009-01-28T07:14:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Junio C Hamano wrote:\n>> \"Shawn O. Pearce\" writes:\n>> >\n>> > Oh, right, its not. I was pointing out that the last time the\n>> > protocol changed in a way the server can infer something about the\n>> > client, which IIRC was 41fa7d2, we still don't have a way to tell\n>> > what the client is.\n>> \n>> But you are still talking as if there is one protocol you can call \"the\n>> protocol\", but it is not. The send-pack receive-pack protocol is on topic\n>> in this thread; the quoted commit was about a separate and independent\n>> fetch-pack upload-pack protocol. It does not matter when that unrelated\n>> protocol was enhanced.\n>\n> Blargh. Of course you are right. Its been a long 2 months for me\n> at work. I'm too #@*#@!@! tired to keep the basics straight anymore.\n>\n> I'm going to shut up now.\n\nPlease don't.\n\nI've been toying with an idea for an alternative solution, and need\nsomebody competent to bounce it around with.\n\npack-objects ends up doing eventually\n\n rev-list --objects $send1 $send2 $send3 ... --not $have1 $have2 ...\n\nwhich lists commits and associated objects reachable from $sendN,\nexcluding the ones that are reachable from $haveN.\n\nThe tentative solution Björn Steinbrink and I came up with excludes\nmissing commit from $haveN to avoid rev-list machinery to barf, but it\nviolates the ref-object contract as I explained to Björn in my other\nmessage.\n\n\tSide note. We often cite \"interrupted commit walkers\" as the\n\treason why has_sha1_file() is not a good enough check, but you can\n\tdiscard a deep commit chain by deleting a branch, and have gc\n\texpire older commit in the commit chain while retaining newer ones\n\tnear the tip of that branch. If (1) you earlier gave that branch\n\tto somebody else, (2) that somebody else has the tip of the branch\n\tyou discarded in his repository, and (3) the repository you are\n\tpushing into borrows from that somebody else's repository, then\n\tyou have the same situation that your has_sha1_file() succeeds but\n\tit will fail when you start digging deeper.\n\nChecking if each commit is reachable from any of the refs is quite\nexpensive, and it would especially be so if it is done once per \".have\"\nand real ref we receive from the other end.\n\nAn alternative is to realize that rev-list traversal already does\nsomething quite similar to what is needed to prove if these \".have\"s are\nreachable from refs when listing the reachable objects. This computation\nis what it needs to do anyway, so if we teach rev-list to ignore missing\nor broken chain while traversing negative refs, we do not have to incur\nany overhead over existing code.\n\nHere is my work in progress. It introduces \"ignore-missing-negative\"\noption to the revision traversal machinery, and squelches the places we\ncurrently complain loudly and die when we expect an object to be\navailable, when the color we are going to paint the object with is\nUNINTERESTING.\n\nI have a mild suspicion that it may even be the right thing to ignore them\nunconditionally, and it might even match the intention of Linus's original\ncode. That would make many hunks in this patch much simpler.\n\nThe evidences behind this suspicion are found in a handful of places in\nrevision.c. mark_blob_uninteresting() does not complain if the caller\nfails to find the blob. mark_tree_uninteresting() does not, either.\nmark_parents_uninteresting() does not, either, and it even has a comment\nthat strongly suggests the original intention was not to care about\nmissing UNINTERESTING objects.\n\n builtin-pack-objects.c | 1 +\n revision.c | 24 ++++++++++++++++++++----\n revision.h | 1 +\n 3 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git i/builtin-pack-objects.c w/builtin-pack-objects.c\nindex cedef52..c615a2f 100644\n--- i/builtin-pack-objects.c\n+++ w/builtin-pack-objects.c\n@@ -2026,6 +2026,7 @@ static void get_object_list(int ac, const char **av)\n \tint flags = 0;\n \n \tinit_revisions(&revs, NULL);\n+\trevs.ignore_missing_negative = 1;\n \tsave_commit_buffer = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \ndiff --git i/revision.c w/revision.c\nindex db60f06..314341b 100644\n--- i/revision.c\n+++ w/revision.c\n@@ -132,6 +132,8 @@ void mark_parents_uninteresting(struct commit *commit)\n \n static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)\n {\n+\tif (!obj)\n+\t\treturn;\n \tif (revs->no_walk && (obj->flags & UNINTERESTING))\n \t\tdie(\"object ranges do not make sense when not walking revisions\");\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT &&\n@@ -163,8 +165,11 @@ static struct object *get_reference(struct rev_info *revs, const char *name, con\n \tstruct object *object;\n \n \tobject = parse_object(sha1);\n-\tif (!object)\n+\tif (!object) {\n+\t\tif (revs->ignore_missing_negative && (flags & UNINTERESTING))\n+\t\t\treturn NULL;\n \t\tdie(\"bad object %s\", name);\n+\t}\n \tobject->flags |= flags;\n \treturn object;\n }\n@@ -183,8 +188,11 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (!tag->tagged)\n \t\t\tdie(\"bad tag\");\n \t\tobject = parse_object(tag->tagged->sha1);\n-\t\tif (!object)\n+\t\tif (!object) {\n+\t\t\tif (revs->ignore_missing_negative && (flags & UNINTERESTING))\n+\t\t\t\treturn NULL;\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n+\t\t}\n \t}\n \n \t/*\n@@ -193,8 +201,11 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t */\n \tif (object->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *)object;\n-\t\tif (parse_commit(commit) < 0)\n+\t\tif (parse_commit(commit) < 0) {\n+\t\t\tif (revs->ignore_missing_negative && (flags & UNINTERESTING))\n+\t\t\t\treturn NULL;\n \t\t\tdie(\"unable to parse commit %s\", name);\n+\t\t}\n \t\tif (flags & UNINTERESTING) {\n \t\t\tcommit->object.flags |= UNINTERESTING;\n \t\t\tmark_parents_uninteresting(commit);\n@@ -479,8 +490,11 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n \t\twhile (parent) {\n \t\t\tstruct commit *p = parent->item;\n \t\t\tparent = parent->next;\n-\t\t\tif (parse_commit(p) < 0)\n+\t\t\tif (parse_commit(p) < 0) {\n+\t\t\t\tif (revs->ignore_missing_negative)\n+\t\t\t\t\treturn 0;\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tp->object.flags |= UNINTERESTING;\n \t\t\tif (p->parents)\n \t\t\t\tmark_parents_uninteresting(p);\n@@ -1110,6 +1124,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->tree_objects = 1;\n \t\trevs->blob_objects = 1;\n \t\trevs->edge_hint = 1;\n+\t} else if (!strcmp(arg, \"--ignore-missing-negative\")) {\n+\t\trevs->ignore_missing_negative = 1;\n \t} else if (!strcmp(arg, \"--unpacked\")) {\n \t\trevs->unpacked = 1;\n \t\tfree(revs->ignore_packed);\ndiff --git i/revision.h w/revision.h\nindex 7cf8487..bb90399 100644\n--- i/revision.h\n+++ w/revision.h\n@@ -48,6 +48,7 @@ struct rev_info {\n \t\t\ttree_objects:1,\n \t\t\tblob_objects:1,\n \t\t\tedge_hint:1,\n+\t\t\tignore_missing_negative:1,\n \t\t\tlimited:1,\n \t\t\tunpacked:1, /* see also ignore_packed below */\n \t\t\tboundary:2,\n"},{"id":"102269","messageId":"7vvdrzvpwd.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"7vd4e7x5ov.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T07:41:22Z","receivedAt":"2009-01-28T07:41:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Here is my work in progress. It introduces \"ignore-missing-negative\"\n> option to the revision traversal machinery, and squelches the places we\n> currently complain loudly and die when we expect an object to be\n> available, when the color we are going to paint the object with is\n> UNINTERESTING.\n>\n> I have a mild suspicion that it may even be the right thing to ignore them\n> unconditionally, and it might even match the intention of Linus's original\n> code. That would make many hunks in this patch much simpler.\n>\n> The evidences behind this suspicion are found in a handful of places in\n> revision.c. mark_blob_uninteresting() does not complain if the caller\n> fails to find the blob. mark_tree_uninteresting() does not, either.\n> mark_parents_uninteresting() does not, either, and it even has a comment\n> that strongly suggests the original intention was not to care about\n> missing UNINTERESTING objects.\n\nHere is what I ended up with doing. It lost \"ignore-missing-negative\"\nso missing UNINTERESTING objects are non-error events more uniformly,\nbut on the other hand get_reference() which is about the command line\narguments always wants the named objects to exist, even if they are\nmarked as UNINTERESTING.\n\nI'll send [PATCH 1/2] which is an update to my previous fix as a follow-up\nto this message.\n\n-- >8 --\nSubject: [PATCH 2/2] revision traversal: allow UNINTERESTING objects to be missing\n\nMost of the existing codepaths were meant to treat missing uninteresting\nobjects to be a silently ignored non-error, but there were a few places\nin handle_commit() and add_parents_to_list(), which are two key functions\nin the revision traversal machinery, that cared:\n\n - When a tag refers to an object that we do not have, we barfed. We\n ignore such a tag if it is painted as UNINTERESTING with this change.\n\n - When digging deeper into the ancestry chain of a commit that is already\n painted as UNINTERESTING, in order to paint its parents UNINTERESTING,\n we barfed if parse_parent() for a parent commit object failed. We can\n ignore such a parent commit object.\n\nSigned-off-by: Junio C Hamano \n---\n revision.c | 7 +++++--\n t/t5519-push-alternates.sh | 37 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex db60f06..ea8ba0f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -183,8 +183,11 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (!tag->tagged)\n \t\t\tdie(\"bad tag\");\n \t\tobject = parse_object(tag->tagged->sha1);\n-\t\tif (!object)\n+\t\tif (!object) {\n+\t\t\tif (flags & UNINTERESTING)\n+\t\t\t\treturn NULL;\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n+\t\t}\n \t}\n \n \t/*\n@@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n \t\t\tstruct commit *p = parent->item;\n \t\t\tparent = parent->next;\n \t\t\tif (parse_commit(p) < 0)\n-\t\t\t\treturn -1;\n+\t\t\t\tcontinue;\n \t\t\tp->object.flags |= UNINTERESTING;\n \t\t\tif (p->parents)\n \t\t\t\tmark_parents_uninteresting(p);\ndiff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh\nindex 6dfc55a..96be523 100755\n--- a/t/t5519-push-alternates.sh\n+++ b/t/t5519-push-alternates.sh\n@@ -103,4 +103,41 @@ test_expect_success 'bob works and pushes' '\n \t)\n '\n \n+test_expect_success 'alice works and pushes yet again' '\n+\t(\n+\t\t# Alice does not care what Bob does. She does not\n+\t\t# even have to be aware of his existence. She just\n+\t\t# keeps working and pushing\n+\t\tcd alice-work &&\n+\t\techo more and more alice >file &&\n+\t\tgit commit -a -m sixth.1 &&\n+\t\techo more and more alice >>file &&\n+\t\tgit commit -a -m sixth.2 &&\n+\t\techo more and more alice >>file &&\n+\t\tgit commit -a -m sixth.3 &&\n+\t\tgit push ../alice-pub\n+\t)\n+'\n+\n+test_expect_success 'bob works and pushes again' '\n+\t(\n+\t\tcd alice-pub &&\n+\t\tgit cat-file commit master >../bob-work/commit\n+\t)\n+\t(\n+\t\t# This time Bob does not pull from Alice, and\n+\t\t# the master branch at her public repository points\n+\t\t# at a commit Bob does not fully know about, but\n+\t\t# he happens to have the commit object (but not the\n+\t\t# necessary tree) in his repository from Alice.\n+\t\t# This should not prevent the push by Bob from\n+\t\t# succeeding.\n+\t\tcd bob-work &&\n+\t\tgit hash-object -t commit -w commit &&\n+\t\techo even more bob >file &&\n+\t\tgit commit -a -m seventh &&\n+\t\tgit push ../bob-pub\n+\t)\n+'\n+\n test_done\n-- \n1.6.1.1.273.g0e555\n"},{"id":"102273","messageId":"7vocxrvpf1.fsf_-_@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"7vvdrzvpwd.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] send-pack: do not send unknown object name from \".have\" to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T07:51:46Z","receivedAt":"2009-01-28T07:51:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"v1.6.1 introduced \".have\" extension to the protocol to allow the receiving\nside to advertise objects that are reachable from refs in the repositories\nit borrows from. This was meant to be used by the sending side to avoid\nsending such objects; they are already available through the alternates\nmechanism.\n\nThe client side implementation in v1.6.1, which was introduced with\n40c155f (push: prepare sender to receive extended ref information from the\nreceiver, 2008-09-09) aka v1.6.1-rc1~203^2~1, were faulty in that it did\nnot consider the possiblity that the repository receiver borrows from\nmight have objects it does not know about.\n\nThis fixes it by refraining from passing missing commits to underlying\npack-objects. Revision machinery may need to be tightened further to\ntreat missing uninteresting objects as non-error events, but this is an\nobvious and safe fix for a maintenance release that is almost good enough.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This lost the \"NEEDSWORK\" comment, as it turns out that revision\n traversal machinery mostly ignores missing uninteresting objects as\n non-error events except for a few corner cases, which we can tighten\n separately.\n\n builtin-send-pack.c | 43 +++++++++---------\n t/t5519-push-alternates.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 127 insertions(+), 22 deletions(-)\n create mode 100755 t/t5519-push-alternates.sh\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex a9fdbf9..d65d019 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -15,6 +15,20 @@ static struct send_pack_args args = {\n \t/* .receivepack = */ \"git-receive-pack\",\n };\n \n+static int feed_object(const unsigned char *sha1, int fd, int negative)\n+{\n+\tchar buf[42];\n+\n+\tif (negative && !has_sha1_file(sha1))\n+\t\treturn 1;\n+\n+\tmemcpy(buf + negative, sha1_to_hex(sha1), 40);\n+\tif (negative)\n+\t\tbuf[0] = '^';\n+\tbuf[40 + negative] = '\\n';\n+\treturn write_or_whine(fd, buf, 41 + negative, \"send-pack: send refs\");\n+}\n+\n /*\n * Make a pack stream and spit it out into file descriptor fd\n */\n@@ -35,7 +49,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t};\n \tstruct child_process po;\n \tint i;\n-\tchar buf[42];\n \n \tif (args.use_thin_pack)\n \t\targv[4] = \"--thin\";\n@@ -51,31 +64,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t * We feed the pack-objects we just spawned with revision\n \t * parameters by writing to the pipe.\n \t */\n-\tfor (i = 0; i < extra->nr; i++) {\n-\t\tmemcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);\n-\t\tbuf[0] = '^';\n-\t\tbuf[41] = '\\n';\n-\t\tif (!write_or_whine(po.in, buf, 42, \"send-pack: send refs\"))\n+\tfor (i = 0; i < extra->nr; i++)\n+\t\tif (!feed_object(extra->array[i], po.in, 1))\n \t\t\tbreak;\n-\t}\n \n \twhile (refs) {\n \t\tif (!is_null_sha1(refs->old_sha1) &&\n-\t\t has_sha1_file(refs->old_sha1)) {\n-\t\t\tmemcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);\n-\t\t\tbuf[0] = '^';\n-\t\t\tbuf[41] = '\\n';\n-\t\t\tif (!write_or_whine(po.in, buf, 42,\n-\t\t\t\t\t\t\"send-pack: send refs\"))\n-\t\t\t\tbreak;\n-\t\t}\n-\t\tif (!is_null_sha1(refs->new_sha1)) {\n-\t\t\tmemcpy(buf, sha1_to_hex(refs->new_sha1), 40);\n-\t\t\tbuf[40] = '\\n';\n-\t\t\tif (!write_or_whine(po.in, buf, 41,\n-\t\t\t\t\t\t\"send-pack: send refs\"))\n-\t\t\t\tbreak;\n-\t\t}\n+\t\t !feed_object(refs->old_sha1, po.in, 1))\n+\t\t\tbreak;\n+\t\tif (!is_null_sha1(refs->new_sha1) &&\n+\t\t !feed_object(refs->new_sha1, po.in, 0))\n+\t\t\tbreak;\n \t\trefs = refs->next;\n \t}\n \ndiff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh\nnew file mode 100755\nindex 0000000..6dfc55a\n--- /dev/null\n+++ b/t/t5519-push-alternates.sh\n@@ -0,0 +1,106 @@\n+#!/bin/sh\n+\n+test_description='push to a repository that borrows from elsewhere'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tmkdir alice-pub &&\n+\t(\n+\t\tcd alice-pub &&\n+\t\tGIT_DIR=. git init\n+\t) &&\n+\tmkdir alice-work &&\n+\t(\n+\t\tcd alice-work &&\n+\t\tgit init &&\n+\t\t>file &&\n+\t\tgit add . &&\n+\t\tgit commit -m initial &&\n+\t\tgit push ../alice-pub master\n+\t) &&\n+\n+\t# Project Bob is a fork of project Alice\n+\tmkdir bob-pub &&\n+\t(\n+\t\tcd bob-pub &&\n+\t\tGIT_DIR=. git init &&\n+\t\tmkdir -p objects/info &&\n+\t\techo ../../alice-pub/objects >objects/info/alternates\n+\t) &&\n+\tgit clone alice-pub bob-work &&\n+\t(\n+\t\tcd bob-work &&\n+\t\tgit push ../bob-pub master\n+\t)\n+'\n+\n+test_expect_success 'alice works and pushes' '\n+\t(\n+\t\tcd alice-work &&\n+\t\techo more >file &&\n+\t\tgit commit -a -m second &&\n+\t\tgit push ../alice-pub\n+\t)\n+'\n+\n+test_expect_success 'bob fetches from alice, works and pushes' '\n+\t(\n+\t\t# Bob acquires what Alice did in his work tree first.\n+\t\t# Even though these objects are not directly in\n+\t\t# the public repository of Bob, this push does not\n+\t\t# need to send the commit Bob received from Alice\n+\t\t# to his public repository, as all the object Alice\n+\t\t# has at her public repository are available to it\n+\t\t# via its alternates.\n+\t\tcd bob-work &&\n+\t\tgit pull ../alice-pub master &&\n+\t\techo more bob >file &&\n+\t\tgit commit -a -m third &&\n+\t\tgit push ../bob-pub\n+\t) &&\n+\n+\t# Check that the second commit by Alice is not sent\n+\t# to ../bob-pub\n+\t(\n+\t\tcd bob-pub &&\n+\t\tsecond=$(git rev-parse HEAD^) &&\n+\t\trm -f objects/info/alternates &&\n+\t\ttest_must_fail git cat-file -t $second &&\n+\t\techo ../../alice-pub/objects >objects/info/alternates\n+\t)\n+'\n+\n+test_expect_success 'clean-up in case the previous failed' '\n+\t(\n+\t\tcd bob-pub &&\n+\t\techo ../../alice-pub/objects >objects/info/alternates\n+\t)\n+'\n+\n+test_expect_success 'alice works and pushes again' '\n+\t(\n+\t\t# Alice does not care what Bob does. She does not\n+\t\t# even have to be aware of his existence. She just\n+\t\t# keeps working and pushing\n+\t\tcd alice-work &&\n+\t\techo more alice >file &&\n+\t\tgit commit -a -m fourth &&\n+\t\tgit push ../alice-pub\n+\t)\n+'\n+\n+test_expect_success 'bob works and pushes' '\n+\t(\n+\t\t# This time Bob does not pull from Alice, and\n+\t\t# the master branch at her public repository points\n+\t\t# at a commit Bob does not know about. This should\n+\t\t# not prevent the push by Bob from succeeding.\n+\t\tcd bob-work &&\n+\t\techo yet more bob >file &&\n+\t\tgit commit -a -m fifth &&\n+\t\tgit push ../bob-pub\n+\t)\n+'\n+\n+test_done\n-- \n1.6.1.1.273.g0e555\n"},{"id":"102275","messageId":"20090128075515.GA1133@coredump.intra.peff.net","threadId":"17400","inReplyTo":"7vd4e7x5ov.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T07:55:15Z","receivedAt":"2009-01-28T07:55:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 27, 2009 at 11:14:56PM -0800, Junio C Hamano wrote:\n\n> I've been toying with an idea for an alternative solution, and need\n> somebody competent to bounce it around with.\n\nWell, unfortunately for you, you are stuck with me. ;P\n\n> Here is my work in progress. It introduces \"ignore-missing-negative\"\n> option to the revision traversal machinery, and squelches the places we\n> currently complain loudly and die when we expect an object to be\n> available, when the color we are going to paint the object with is\n> UNINTERESTING.\n> \n> I have a mild suspicion that it may even be the right thing to ignore them\n> unconditionally, and it might even match the intention of Linus's original\n> code. That would make many hunks in this patch much simpler.\n\nI'm not sure it is a good idea to do so unconditionally. In the case of\nnegatives for transferring files, a missed negative is simply a missed\nopportunity for optimizing the resulting pack.\n\nBut in other cases, it silently gives you the wrong answer. For\nexample, consider a history like:\n\n C--D\n /\n A--B\n \\\n E--F\n\nnow let's suppose I have everything except 'E'. If I ask for\n\n git rev-list F..D\n\nthen it will not realize that A and B are uninteresting, and I will get\nA-B-C-D. I think it is much better for git to complain loudly that it\ncould not compute the correct answer.\n\nAm I understanding the issue correctly?\n\n-Peff\n"},{"id":"102277","messageId":"7vfxj3vos2.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128075515.GA1133@coredump.intra.peff.net","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T08:05:33Z","receivedAt":"2009-01-28T08:05:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But in other cases, it silently gives you the wrong answer. For\n> example, consider a history like:\n>\n> C--D\n> /\n> A--B\n> \\\n> E--F\n>\n> now let's suppose I have everything except 'E'. If I ask for\n>\n> git rev-list F..D\n>\n> then it will not realize that A and B are uninteresting, and I will get\n> A-B-C-D. I think it is much better for git to complain loudly that it\n> could not compute the correct answer.\n\nFair enough. I think we can resurrect the conditional and the traversal\noption revs->ignore_missing_negative only for this hunk in my [2/2] patch\nto support that use case.\n\n@@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n \t\t\tstruct commit *p = parent->item;\n \t\t\tparent = parent->next;\n \t\t\tif (parse_commit(p) < 0)\n-\t\t\t\treturn -1;\n+\t\t\t\tcontinue;\n \t\t\tp->object.flags |= UNINTERESTING;\n \t\t\tif (p->parents)\n \t\t\t\tmark_parents_uninteresting(p);\n"},{"id":"102278","messageId":"20090128081745.GA2172@coredump.intra.peff.net","threadId":"17400","inReplyTo":"7vfxj3vos2.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T08:17:45Z","receivedAt":"2009-01-28T08:17:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 12:05:33AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But in other cases, it silently gives you the wrong answer. For\n> > example, consider a history like:\n> >\n> > C--D\n> > /\n> > A--B\n> > \\\n> > E--F\n> >\n> > now let's suppose I have everything except 'E'. If I ask for\n> >\n> > git rev-list F..D\n> >\n> > then it will not realize that A and B are uninteresting, and I will get\n> > A-B-C-D. I think it is much better for git to complain loudly that it\n> > could not compute the correct answer.\n> \n> Fair enough. I think we can resurrect the conditional and the traversal\n> option revs->ignore_missing_negative only for this hunk in my [2/2] patch\n> to support that use case.\n> [ hunk handling parent lookup]\n\nDon't the other changes have similar parallel use cases? [2/2] also deals\nwith tag lookup. Wouldn't you also expect, if you had a tag \"T\" pointing\nto \"E\" in the above scenario that \"git rev-list T..D\" would barf? I\nreally think you don't want to ignore missing negations _ever_ unless\nthe caller knows that such a miss is really only about optimization and\nnot correctness.\n\nSide note:\n\nAs you described, we expect to reach this situation from a partial\ntransfer. Which means that you don't actually have a _ref_ for \"T\" (or\n\"F\"). So it is unlikely to come up in normal use (you would have to\nmanually specify the sha1 of a broken portion of the graph).\n\nBut what is more important is that your repository _is_ corrupted, I\nthink we are losing an important method by which the user finds out. Git\nis usually very good at informing you of a problem in the repo early,\nand I think unconditionally ignoring missing objects would lose that.\n\n-Peff\n"},{"id":"102279","messageId":"7vbptrvo0m.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"7vfxj3vos2.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T08:22:01Z","receivedAt":"2009-01-28T08:22:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> But in other cases, it silently gives you the wrong answer. For\n>> example, consider a history like:\n>>\n>> C--D\n>> /\n>> A--B\n>> \\\n>> E--F\n>>\n>> now let's suppose I have everything except 'E'. If I ask for\n>>\n>> git rev-list F..D\n>>\n>> then it will not realize that A and B are uninteresting, and I will get\n>> A-B-C-D. I think it is much better for git to complain loudly that it\n>> could not compute the correct answer.\n>\n> Fair enough. I think we can resurrect the conditional and the traversal\n> option revs->ignore_missing_negative only for this hunk in my [2/2] patch\n> to support that use case.\n> ...\n\nNah, I take that back.\n\nEven the original code does not consider this case an error.\n\nIf you really want that, the revision machinery needs major surgery, as I\nalready noted that the design of mark_parents_uninteresting() wants to\ntreat a missing uninteresting commit as a non-error event.\n"},{"id":"102286","messageId":"20090128092425.GA2400@coredump.intra.peff.net","threadId":"17400","inReplyTo":"7vbptrvo0m.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T09:24:25Z","receivedAt":"2009-01-28T09:24:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 12:22:01AM -0800, Junio C Hamano wrote:\n\n> Nah, I take that back.\n> \n> Even the original code does not consider this case an error.\n> \n> If you really want that, the revision machinery needs major surgery, as I\n> already noted that the design of mark_parents_uninteresting() wants to\n> treat a missing uninteresting commit as a non-error event.\n\nHrm. Never mind my concern, then. I was worried that we were losing some\nexisting corruption checks, but it seems they are not there in the first\nplace.\n\n-Peff\n"},{"id":"102316","messageId":"alpine.LFD.2.00.0901280738430.3123@localhost.localdomain","threadId":"17400","inReplyTo":"7vvdrzvpwd.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-01-28T15:45:00Z","receivedAt":"2009-01-28T15:45:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 27 Jan 2009, Junio C Hamano wrote:\n> \n> - When digging deeper into the ancestry chain of a commit that is already\n> painted as UNINTERESTING, in order to paint its parents UNINTERESTING,\n> we barfed if parse_parent() for a parent commit object failed. We can\n> ignore such a parent commit object.\n\nWouldn't it be better to still mark it UNINTERESTING too?\n\n> @@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n> \t\t\tstruct commit *p = parent->item;\n> \t\t\tparent = parent->next;\n> \t\t\tif (parse_commit(p) < 0)\n> -\t\t\t\treturn -1;\n> +\t\t\t\tcontinue;\n> \t\t\tp->object.flags |= UNINTERESTING;\n> \t\t\tif (p->parents)\n> \t\t\t\tmark_parents_uninteresting(p);\n\nIOW, move that\n\n\tp->object.flags |= UNINTERESTING;\n\nto before parse_commit(). That's assuming 'parent' is never NULL, of \ncourse.\n\nSide note: parse_commit() is still going to print out the error message \nif the object is missing (\"Could not read %s\"). I guess that's fine, but \nif you really want to make this a \"not an error at all\" condition...\n\n\t\tLinus\n"},{"id":"102317","messageId":"20090128160900.GJ1321@spearce.org","threadId":"17400","inReplyTo":"7vd4e7x5ov.fsf@gitster.siamese.dyndns.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-28T16:09:00Z","receivedAt":"2009-01-28T16:09:00Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \n> I've been toying with an idea for an alternative solution, and need\n> somebody competent to bounce it around with.\n\nHeh, then we need to wait for Nico... :-)\n \n> pack-objects ends up doing eventually\n> \n> rev-list --objects $send1 $send2 $send3 ... --not $have1 $have2 ...\n> \n> which lists commits and associated objects reachable from $sendN,\n> excluding the ones that are reachable from $haveN.\n> \n> The tentative solution Björn Steinbrink and I came up with excludes\n> missing commit from $haveN to avoid rev-list machinery to barf, but it\n> violates the ref-object contract as I explained to Björn in my other\n> message.\n\nOh, OK, I now _finally_ understand what you were trying to say\nby the reachability thing. I kept scratching my head trying to\nunderstand you, and was going to say something stupid on list;\nbut waited because I just didn't get what the big deal was...\n\nIts the crash in rev-list that you were worried about.\n\n> Checking if each commit is reachable from any of the refs is quite\n> expensive, and it would especially be so if it is done once per \".have\"\n> and real ref we receive from the other end.\n\nYup.\n \n> An alternative is to realize that rev-list traversal already does\n> something quite similar to what is needed to prove if these \".have\"s are\n> reachable from refs when listing the reachable objects. This computation\n> is what it needs to do anyway, so if we teach rev-list to ignore missing\n> or broken chain while traversing negative refs, we do not have to incur\n> any overhead over existing code.\n\nEXACTLY.\n\nJGit does this.\n\nThe functional equivilant of rev-list in JGit will by default\nthrow an exception if any object is missing when we try to walk it.\nThat includes things we've painted UNINTERESTING, as it is a sure\nsign of repository corruption.\n\nHowever; our equivilant of pack-objects can toggle what you are\ncalling \"ignore-missing-negative\" when it starts enumeration.\nAny UNINTERESTING object which is missing or failed to parse is\nsimply tossed aside. Yes, the pack may be larger than necessary\nlike in Peff's example of:\n\n Q-R\n /\n D--E\n \\\n A-C\n\nIf the other side has C reachable, we are pushing R, and we have\nC but are missing A, we'll \"over push\" D-E, but its still a clean\nand valid push. Its no worse than we were before the \".have\" came\nabout, or if C hadn't been downloaded locally at all. (Of course\nyour tell-me-more extension would help fix this over-push, but lets\nnot get off topic.)\n\nIMHO, this corruption of A is harmless if C isn't reachable.\n\nIt isn't really local corruption unless C was reachable by a ref.\nBut we don't tend to see much corruption like that, and if it did\nexist, it would show up during *other* operations that access a\nlarger set of local refs, such as \"git gc\".\n\n> I have a mild suspicion that it may even be the right thing to ignore them\n> unconditionally, and it might even match the intention of Linus's original\n> code. That would make many hunks in this patch much simpler.\n\nI don't think its right to ignore broken UNINTERESTING chains all\nof the time. Today we would see fatal errors if I asked for\n\n git log R ^C\n\nand A was missing, but R and C are both local refs. I still want\nto see that fatal error. Its a local corruption that should be\nraised quickly to the user. In fact by A missing we'd compute the\nwrong result and produce D-E too, which is wrong.\n\nIMHO, the *only* time this missing uninteresting A is safe is\nduring send-pack, upload-pack, or bundle creation, where you are\nbringing the other side up to R by transferring any amount of data\nnecessary to reach that goal. Which is why JGit enables this.\n(Though at the API level we do let the caller flag if they want\nthe error to be fatal instead, but AFAIK nobody sets it for \"fatal\".)\n\nFWIW, Linus' most recent message on this thread about hoisting the\nUNINTERESTING test up sooner makes sense too.\n \n> The evidences behind this suspicion are found in a handful of places in\n> revision.c. mark_blob_uninteresting() does not complain if the caller\n> fails to find the blob. mark_tree_uninteresting() does not, either.\n> mark_parents_uninteresting() does not, either, and it even has a comment\n> that strongly suggests the original intention was not to care about\n> missing UNINTERESTING objects.\n\nThat feels wrong to me... given the \"git log R ^C\" example I give above.\n \n-- \nShawn.\n"},{"id":"102318","messageId":"20090128161652.GK1321@spearce.org","threadId":"17400","inReplyTo":"20090128081745.GA2172@coredump.intra.peff.net","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-01-28T16:16:53Z","receivedAt":"2009-01-28T16:16:53Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jeff King wrote:\n> On Wed, Jan 28, 2009 at 12:05:33AM -0800, Junio C Hamano wrote:\n> > Jeff King writes:\n> > >\n> > > C--D\n> > > /\n> > > A--B\n> > > \\\n> > > E--F\n> \n> Don't the other changes have similar parallel use cases? [2/2] also deals\n> with tag lookup. Wouldn't you also expect, if you had a tag \"T\" pointing\n> to \"E\" in the above scenario that \"git rev-list T..D\" would barf? I\n> really think you don't want to ignore missing negations _ever_ unless\n> the caller knows that such a miss is really only about optimization and\n> not correctness.\n\nExactly what I just said in my other message.\n \n> Side note:\n> \n> As you described, we expect to reach this situation from a partial\n> transfer. Which means that you don't actually have a _ref_ for \"T\" (or\n> \"F\"). So it is unlikely to come up in normal use (you would have to\n> manually specify the sha1 of a broken portion of the graph).\n\nTrue, but in the send-pack case we are discussing the remote side\nhas specified the SHA-1 of broken portions of the graph to us,\nand we've taken that into consideration. So we have to fix that\nassumption we've made.\n\n> But what is more important is that your repository _is_ corrupted,\n\nDepends. If the SHA-1 came from the remote side during send-pack,\nit doesn't matter that we have a broken chain along that path,\nit may have been a dumb transport fetch that was interrupted.\nOur local repository isn't corrupt, it just has some extra crap\nlaying around that hasn't gc'd yet.\n\nIf the SHA-1 came from the user, then it depends on the context\nof why the user is giving it to us. In pretty much every case,\nyes, its a corruption and we should be aborting. :-)\n\nActually, the only time where it *isn't* a corruption is when its\ninput to \"git bundle create A.bdl ... -not $SOMEBADID\" as that is\nthe exact same thing as coming from the other side via send-pack.\n\n> I\n> think we are losing an important method by which the user finds out. Git\n> is usually very good at informing you of a problem in the repo early,\n> and I think unconditionally ignoring missing objects would lose that.\n\nYup, I agree. But as you and Junio have already pointed out, C Git\ncan miss some types of corruption because the revision machinary has\nsome gaps. *sigh*\n\nI'd really like to see those gaps closed. But I don't have a good\nenough handle on the code structure of the C Git revision machinary\nto do that myself in a short period of time. I know JGit's well...\nbut that's only because I wrote it. ;-)\n\nIts now on my wish list of things I wish I had time for in C Git.\nBut perhaps someone who is more familiar with the revision machinary\nwill get to it first.\n\n-- \nShawn.\n"},{"id":"102320","messageId":"alpine.LFD.2.00.0901281128480.30940@xanadu.home","threadId":"17400","inReplyTo":"20090128160900.GJ1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-01-28T16:38:11Z","receivedAt":"2009-01-28T16:38:11Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 28 Jan 2009, Shawn O. Pearce wrote:\n\n> Junio C Hamano wrote:\n> > \n> > I've been toying with an idea for an alternative solution, and need\n> > somebody competent to bounce it around with.\n> \n> Heh, then we need to wait for Nico... :-)\n\nHmmm... ehhh... what have I done?\n\n/me tries to walk by innocently without being noticed ;-)\n\n\nNicolas\n"},{"id":"102329","messageId":"20090128181115.GF8863@coredump.intra.peff.net","threadId":"17400","inReplyTo":"20090128160900.GJ1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T18:11:15Z","receivedAt":"2009-01-28T18:11:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 08:09:00AM -0800, Shawn O. Pearce wrote:\n\n> I don't think its right to ignore broken UNINTERESTING chains all\n> of the time. Today we would see fatal errors if I asked for\n> \n> git log R ^C\n> \n> and A was missing, but R and C are both local refs. I still want\n> to see that fatal error. Its a local corruption that should be\n> raised quickly to the user. In fact by A missing we'd compute the\n> wrong result and produce D-E too, which is wrong.\n\nI think you wrote this before reading the other part of the thread where\nwe see that many of these checks are not in C git. But to be clear, even\nwithout Junio's patches the exact case I mentioned is not currently\nreported as an error (i.e., will produce incorrect results). I tested\nwith:\n\n-- >8 --\ncommit() {\n echo $1 >$1 && git add $1 && git commit -m $1 && git tag $1\n}\n\nmkdir repo && cd repo && git init\ncommit A\ncommit B\ncommit C\ncommit D\ngit checkout -b other B\ncommit E\ncommit F\n\nrm -f .git/objects/`git rev-parse E | sed 's,^..,&/,'`\ngit log F..D\n-- 8< --\n\nwhich shows A-B-C-D.\n\n-Peff\n"},{"id":"102331","messageId":"20090128181619.GG8863@coredump.intra.peff.net","threadId":"17400","inReplyTo":"20090128161652.GK1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T18:16:19Z","receivedAt":"2009-01-28T18:16:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 08:16:53AM -0800, Shawn O. Pearce wrote:\n\n> > But what is more important is that your repository _is_ corrupted,\n> \n> Depends. If the SHA-1 came from the remote side during send-pack,\n\nSorry, there is a critical typo in there. I meant to say \"_if_ your\nrepository is corrupted\" (and I have no idea how I ended up not only\nomitting a word, but emphasizing the wrong one, so I can only suspect\nthe typo was in my brain and not my fingers).\n\nSo basically I agree with everything you said.\n\n> Yup, I agree. But as you and Junio have already pointed out, C Git\n> can miss some types of corruption because the revision machinary has\n> some gaps. *sigh*\n> \n> I'd really like to see those gaps closed. But I don't have a good\n> enough handle on the code structure of the C Git revision machinary\n> to do that myself in a short period of time. I know JGit's well...\n> but that's only because I wrote it. ;-)\n\nI would like to see them closed, too. But keep in mind that this may\nactually be a harder form of corruption to achieve than something like\njust flipping some bits. Once the objects are in a packfile, you are\nunlikely to lose a single or a small number of objects. But like with\nany type of corruption, it is infrequent enough that it is hard to say\nwhich is more common or \"worse\".\n\n-Peff\n"},{"id":"102335","messageId":"7vmydbs2vo.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"20090128161652.GK1321@spearce.org","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T18:26:51Z","receivedAt":"2009-01-28T18:26:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Actually, the only time where it *isn't* a corruption is when its\n> input to \"git bundle create A.bdl ... -not $SOMEBADID\" as that is\n> the exact same thing as coming from the other side via send-pack.\n\nAnd notice that it is about a nagative ref.\n\nAnother case you may use an object ID that may or may not be good and it\nis not a corruption is when a Porcelain has an object ID obtained from\nsomewhere, and wants to know if it is safe to use the object. After\ndetermining that the object itself exists (e.g. via \"cat-file -t\"),\nyou run\n\n\trev-list --objects $THAT_UNKNOWN_ID --not --all\n\nto see if it is reachable from some of your own refs, or at least it is\nconnected to them without gaps. If it errors out while traversing, you\nknow it is bad; if it doesn't, you know you can merge one of the commits\nreachable from your refs with it and put the result in your ref without\nviolating the ref-objects contract.\n\nNotice that in this case, it is about a positive ref, and revision\nmachinery is set to notice the breakage.\n\nSo in that sense, the existing semantics is internally consistent. The\nrules (I am not making up a new rule here, but just spelling out) are:\n\n (1) You cannot just pick a random object that happens to exist in your\n repository, traverse to the objects it refers to and expect\n everything exists;\n\n (2) If an object is reachable from any of your refs, however, you can\n expect everything reachable from that object exists. Otherwise you\n have a corrupt repository [*1*]).\n\n (3) Your object store may have garbage objects that are not reachable\n from any of your refs and it is normal.\n\n (4) You can use random objects that may not be well connected as negative\n revs to limit the range of revs (and optionally objects reachable\n from them) listed by object traversal. If they are well connected,\n they will affect the outcome, but it is not an error if they are\n leftover cruft that is not connected to the positive ones you start\n your listing traversal at.\n\n\n[Footnote]\n\n*1* You don't have to bring up grafts and shallow. People who know about\nthem know they are ways to hide or deliberately introduce this type of\ncorruption while keeping the system (mostly) working.\n"},{"id":"102338","messageId":"7vy6wvqmrn.fsf@gitster.siamese.dyndns.org","threadId":"17400","inReplyTo":"alpine.LFD.2.00.0901280738430.3123@localhost.localdomain","subject":"Re: Bad objects error since upgrading GitHub servers to 1.6.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T19:00:12Z","receivedAt":"2009-01-28T19:00:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Tue, 27 Jan 2009, Junio C Hamano wrote:\n>> \n>> - When digging deeper into the ancestry chain of a commit that is already\n>> painted as UNINTERESTING, in order to paint its parents UNINTERESTING,\n>> we barfed if parse_parent() for a parent commit object failed. We can\n>> ignore such a parent commit object.\n>\n> Wouldn't it be better to still mark it UNINTERESTING too?\n>\n>> @@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,\n>> \t\t\tstruct commit *p = parent->item;\n>> \t\t\tparent = parent->next;\n>> \t\t\tif (parse_commit(p) < 0)\n>> -\t\t\t\treturn -1;\n>> +\t\t\t\tcontinue;\n>> \t\t\tp->object.flags |= UNINTERESTING;\n>> \t\t\tif (p->parents)\n>> \t\t\t\tmark_parents_uninteresting(p);\n>\n> IOW, move that\n>\n> \tp->object.flags |= UNINTERESTING;\n>\n> to before parse_commit(). That's assuming 'parent' is never NULL, of \n> course.\n\nOk, makes sense. Will do.\n"}]}