{"thread":{"id":"33274","subject":"propagating repo corruption across clone","startedAt":"2013-03-24T18:31:33Z","lastAt":"2013-03-31T07:57:12Z","messageCount":60,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Ilari Liusvaara","Jeff Mitchell","Duy Nguyen","Junio C Hamano","Jonathan Nieder","Eric Sunshine","Philip Oakley","Rich Fromm","Sitaram Chamarty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"212101","messageId":"20130324183133.GA11200@sigill.intra.peff.net","threadId":"33274","inReplyTo":null,"subject":"propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-24T18:31:33Z","receivedAt":"2013-03-24T18:31:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I saw this post-mortem on recent disk corruption seen on git.kde.org:\n\n http://jefferai.org/2013/03/24/too-perfect-a-mirror/\n\nThe interesting bit to me is that object corruption propagated across a\nclone (and oddly, that --mirror made complaints about corruption go\naway). I did a little testing and found some curious results (this ended\nup long; skip to the bottom for my conclusions).\n\nHere's a fairly straight-forward corruption recipe:\n\n-- >8 --\nobj_to_file() {\n echo \".git/objects/$(echo $1 | sed 's,..,&/,')\"\n}\n\n# corrupt a single byte inside the object\ncorrupt_object() {\n fn=$(obj_to_file \"$1\") &&\n chmod +w \"$fn\" &&\n printf '\\0' | dd of=\"$fn\" bs=1 conv=notrunc seek=10\n}\n\ngit init repo &&\ncd repo &&\necho content >file &&\ngit add file &&\ngit commit -m one &&\ncorrupt_object $(git rev-parse HEAD:file)\n-- 8< --\n\nreport git clone . fast-local\nreport git clone --no-local . no-local\nreport git -c transfer.unpackLimit=1 clone --no-local . index-pack\nreport git -c fetch.fsckObjects=1 clone --no-local . fsck\n\nand here is how clone reacts in a few situations:\n\n $ git clone --bare . local-bare && echo WORKED\n Cloning into bare repository 'local-bare'...\n done.\n WORKED\n\nWe don't notice the problem during the transport phase, which is to be\nexpected; we're using the fast \"just hardlink it\" code path. So that's\nOK.\n\n $ git clone . local-tree && echo WORKED\n Cloning into 'local-tree'...\n done.\n error: inflate: data stream error (invalid distance too far back)\n error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header\n WORKED\n\nWe _do_ see a problem during the checkout phase, but we don't propagate\na checkout failure to the exit code from clone. That is bad in general,\nand should probably be fixed. Though it would never find corruption of\nolder objects in the history, anyway, so checkout should not be relied\non for robustness.\n\n $ git clone --no-local . non-local && echo WORKED\n Cloning into 'non-local'...\n remote: Counting objects: 3, done.\n remote: error: inflate: data stream error (invalid distance too far back)\n remote: error: unable to unpack d95f3ad14dee633a758d2e331151e950dd13e4ed header\n remote: error: inflate: data stream error (invalid distance too far back)\n remote: fatal: loose object d95f3ad14dee633a758d2e331151e950dd13e4ed (stored in ./objects/d9/5f3ad14dee633a758d2e331151e950dd13e4ed) is corrupt\n error: git upload-pack: git-pack-objects died with error.\n fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n remote: aborting due to possible repository corruption on the remote side.\n fatal: early EOF\n fatal: index-pack failed\n\nHere we detect the error. It's noticed by pack-objects on the remote\nside as it tries to put the bogus object into a pack. But what if we\nalready have a pack that's been corrupted, and pack-objects is just\npushing out entries without doing any recompression?\n\nLet's change our corrupt_object to:\n\n corrupt_object() {\n git repack -ad &&\n pack=`echo .git/objects/pack/*.pack` &&\n chmod +w \"$pack\" &&\n printf '\\0' | dd of=\"$pack\" bs=1 conv=notrunc seek=175\n }\n\nand try again:\n\n $ git clone --no-local . non-local && echo WORKED\n Cloning into 'non-local'...\n remote: Counting objects: 3, done.\n remote: Total 3 (delta 0), reused 3 (delta 0)\n error: inflate: data stream error (invalid distance too far back)\n fatal: pack has bad object at offset 169: inflate returned -3\n fatal: index-pack failed\n\nGreat, we still notice the problem in unpack-objects on the receiving\nend. But what if there's a more subtle corruption, where filesystem\ncorruption points the directory entry for one object at the inode of\nanother. Like:\n\n corrupt_object() {\n corrupt=$(echo corrupted | git hash-object -w --stdin) &&\n mv -f $(obj_to_file $corrupt) $(obj_to_file $1)\n }\n\nThis is going to be more subtle, because the object in the packfile is\nself-consistent but the object graph as a whole is broken.\n\n $ git clone --no-local . non-local && echo WORKED\n Cloning into 'non-local'...\n remote: Counting objects: 3, done.\n remote: Total 3 (delta 0), reused 0 (delta 0)\n Receiving objects: 100% (3/3), done.\n error: unable to find d95f3ad14dee633a758d2e331151e950dd13e4ed\n WORKED\n\nLike the --local cases earlier, we notice the missing object during the\ncheckout phase, but do not correctly propagate the error.\n\nWe do not notice the sha1 mis-match on the sending side (which we could,\nif we checked the sha1 as we were sending). We do not notice the broken\nobject graph during the receive process either. I would have expected\ncheck_everything_connected to handle this, but we don't actually call it\nduring clone! If you do this:\n\n $ git init non-local && cd non-local && git fetch ..\n remote: Counting objects: 3, done.\n remote: Total 3 (delta 0), reused 3 (delta 0)\n Unpacking objects: 100% (3/3), done.\n fatal: missing blob object 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n error: .. did not send all necessary objects\n\nwe do notice.\n\nAnd one final check:\n\n $ git -c transfer.fsckobjects=1 clone --no-local . fsck\n Cloning into 'fsck'...\n remote: Counting objects: 3, done.\n remote: Total 3 (delta 0), reused 3 (delta 0)\n Receiving objects: 100% (3/3), done.\n error: unable to find d95f3ad14dee633a758d2e331151e950dd13e4ed\n fatal: object of unexpected type\n fatal: index-pack failed\n\nFscking the incoming objects does work, but of course it comes at a cost\nin the normal case (for linux-2.6, I measured an increase in CPU time\nwith \"index-pack --strict\" from ~2.5 minutes to ~4 minutes). And I think\nit is probably overkill for finding corruption; index-pack already\nrecognizes bit corruption inside an object, and\ncheck_everything_connected can detect object graph problems much more\ncheaply.\n\nOne thing I didn't check is bit corruption inside a packed object that\nstill correctly zlib inflates. check_everything_connected will end up\nreading all of the commits and trees (to walk them), but not the blobs.\nAnd I don't think that we explicitly re-sha1 every incoming object (only\nif we detect a possible collision). So it may be that\ntransfer.fsckObjects would save us there (it also introduces new\nproblems if there are ignorable warnings in the objects you receive,\nlike zero-padded trees).\n\nSo I think at the very least we should:\n\n 1. Make sure clone propagates errors from checkout to the final exit\n code.\n\n 2. Teach clone to run check_everything_connected.\n\nI don't have details on the KDE corruption, or why it wasn't detected\n(if it was one of the cases I mentioned above, or a more subtle issue).\n\n-Peff\n"},{"id":"212102","messageId":"CACBZZX6czzJRF9TEsc8c+=LND6SxaVvrZdbcZ+TfUZTWQOpW0Q@mail.gmail.com","threadId":"33274","inReplyTo":"20130324183133.GA11200@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2013-03-24T19:01:33Z","receivedAt":"2013-03-24T19:01:33Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Mar 24, 2013 at 7:31 PM, Jeff King wrote:\n>\n> I don't have details on the KDE corruption, or why it wasn't detected\n> (if it was one of the cases I mentioned above, or a more subtle issue).\n\nOne thing worth mentioning is this part of the article:\n\n\"Originally, mirrored clones were in fact not used, but non-mirrored\nclones on the anongits come with their own set of issues, and are more\nprone to getting stopped up by legitimate, authenticated force pushes,\nref deletions, and so on – and if we set the refspec such that those\nare allowed through silently, we don’t gain much. \"\n\nSo the only reason they were even using --mirror was because they were\nrunning into those problems with fetching.\n\nSo aside from the problems with --mirror I think we should have\nsomething that updates your local refs to be exactly like they are on\nthe other end, i.e. deletes some, non-fast-forwards others etc.\n(obviously behind several --force options and so on). But such an\noption *wouldn't* accept corrupted objects.\n\nThat would give KDE and other parties a safe way to do exact repo\nmirroring like this, wouldn't protect them from someone maliciously\ndeleting all the refs in all the repos, but would prevent FS\ncorruption from propagating.\n"},{"id":"212104","messageId":"20130324191614.GA15275@LK-Perkele-VII","threadId":"33274","inReplyTo":"20130324183133.GA11200@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2013-03-24T19:16:14Z","receivedAt":"2013-03-24T19:16:14Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Sun, Mar 24, 2013 at 02:31:33PM -0400, Jeff King wrote:\n> \n> Fscking the incoming objects does work, but of course it comes at a cost\n> in the normal case (for linux-2.6, I measured an increase in CPU time\n> with \"index-pack --strict\" from ~2.5 minutes to ~4 minutes). And I think\n> it is probably overkill for finding corruption; index-pack already\n> recognizes bit corruption inside an object, and\n> check_everything_connected can detect object graph problems much more\n> cheaply.\n\nAFAIK, standard checks index-pack has to do + checking that the object\ngraph has no broken links (and every ref points to something valid) will\ncatch everything except:\n\n- SHA-1 collisions between corrupt objects and clean ones.\n- Corrupted refs (that still point to something valid).\n- \"Born-corrupted\" objects.\n\n> One thing I didn't check is bit corruption inside a packed object that\n> still correctly zlib inflates. check_everything_connected will end up\n> reading all of the commits and trees (to walk them), but not the blobs.\n\nChecking that everything is connected will (modulo SHA-1 collisions) save\nyou there, at least if packv3 is used as transport stream.\n\n-Ilari\n"},{"id":"212103","messageId":"20130324192350.GA20688@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CACBZZX6czzJRF9TEsc8c+=LND6SxaVvrZdbcZ+TfUZTWQOpW0Q@mail.gmail.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-24T19:23:50Z","receivedAt":"2013-03-24T19:23:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 24, 2013 at 08:01:33PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sun, Mar 24, 2013 at 7:31 PM, Jeff King wrote:\n> >\n> > I don't have details on the KDE corruption, or why it wasn't detected\n> > (if it was one of the cases I mentioned above, or a more subtle issue).\n> \n> One thing worth mentioning is this part of the article:\n> \n> \"Originally, mirrored clones were in fact not used, but non-mirrored\n> clones on the anongits come with their own set of issues, and are more\n> prone to getting stopped up by legitimate, authenticated force pushes,\n> ref deletions, and so on – and if we set the refspec such that those\n> are allowed through silently, we don’t gain much. \"\n> \n> So the only reason they were even using --mirror was because they were\n> running into those problems with fetching.\n\nI think the --mirror thing is a red herring. It should not be changing\nthe transport used, and that is the part of git that is expected to\ncatch such corruption.\n\nBut I haven't seen exactly what the corruption is, nor exactly what\ncommands they used to clone. I've invited the blog author to give more\ndetails in this thread.\n\n> So aside from the problems with --mirror I think we should have\n> something that updates your local refs to be exactly like they are on\n> the other end, i.e. deletes some, non-fast-forwards others etc.\n> (obviously behind several --force options and so on). But such an\n> option *wouldn't* accept corrupted objects.\n\nThat _should_ be how \"git fetch --prune +refs/*:refs/*\" behaves (and\nthat refspec is set up when you use \"--mirror\"; we should probably have\nit turn on --prune, too, but I do not think you can do so via a config\noption currently).\n\n-Peff\n"},{"id":"212165","messageId":"CAOx6V3YtM-e8-S41v1KnC+uSymYwZw8QBwiCJRYw0MYJXRjj-w@mail.gmail.com","threadId":"33274","inReplyTo":"20130324192350.GA20688@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Jeff Mitchell","fromEmail":"jeffrey.mitchell@gmail.com","sentAt":"2013-03-25T13:43:23Z","receivedAt":"2013-03-25T13:43:23Z","isPatch":false,"sender":{"key":"jeffrey.mitchell@gmail.com","avatar":null},"body":"On Sun, Mar 24, 2013 at 3:23 PM, Jeff King wrote:\n> On Sun, Mar 24, 2013 at 08:01:33PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Sun, Mar 24, 2013 at 7:31 PM, Jeff King wrote:\n>> >\n>> > I don't have details on the KDE corruption, or why it wasn't detected\n>> > (if it was one of the cases I mentioned above, or a more subtle issue).\n>>\n>> One thing worth mentioning is this part of the article:\n>>\n>> \"Originally, mirrored clones were in fact not used, but non-mirrored\n>> clones on the anongits come with their own set of issues, and are more\n>> prone to getting stopped up by legitimate, authenticated force pushes,\n>> ref deletions, and so on – and if we set the refspec such that those\n>> are allowed through silently, we don’t gain much. \"\n>>\n>> So the only reason they were even using --mirror was because they were\n>> running into those problems with fetching.\n\nWith a normal fetch. We actually *wanted* things like force updates\nand ref deletions to propagate, because we have not just Gitolite's\nchecks but our own checks on the servers, and wanted that to be\nconsidered the authenticated source. Besides just daily use and\npreventing cruft, we wanted to ensure that such actions propagated so\nthat if a branch was removed because it contained personal\ninformation, accidental commits, or a security issue (for instance)\nthat the branch was removed on the anongits too, within a timely\nfashion.\n\n> I think the --mirror thing is a red herring. It should not be changing\n> the transport used, and that is the part of git that is expected to\n> catch such corruption.\n>\n> But I haven't seen exactly what the corruption is, nor exactly what\n> commands they used to clone. I've invited the blog author to give more\n> details in this thread.\n\nThe syncing was performed via a clone with git clone --mirror (and a\ngit:// URL) and updates with git remote update.\n\nSo I should mention that my experiments after the fact were using\nlocal paths, but with --no-hardlinks. If you're saying that the\ntransport is where corruption is supposed to be caught, then it's\npossible that we shouldn't see corruption propagate on an initial\nmirror clone across git://, and that something else was responsible\nfor the trouble we saw with the repositories that got cloned\nafter-the-fact. But then I'd argue that this is non-obvious. In\nparticular, when using --no-hardlinks, I wouldn't expect that behavior\nto be different with a straight path and with file://.\n\nSomething else: apparently one of my statements prompted joeyh to\nthink about potential issues with backing up live git repos\n(http://joeyh.name/blog/entry/difficulties_in_backing_up_live_git_repositories/).\nLooking at that post made me realize that, when we were doing our\ninitial thinking about the system three years ago, we made an\nassumption that, in fact, taking a .tar.gz of a repo as it's in the\nprocess of being written to or garbage collected or repacked could be\nproblematic. This isn't a totally baseless assumption, as I once had a\ngit repository that I was in the process of updating when I had a\nsudden power outage that suffered corruption. (It could totally have\nbeen the filesystem, of course, although it was a journaled file\nsystem.)\n\nSo, we decided to use Git's built-in capabilities of consistency\nchecking to our advantage (with, as it turns out, a flaw in our\nimplementation). But the question remains: are we wrong about thinking\nthat rsyncing or tar.gz live repositories in the middle of being\npushed to/gc'd/repacked could result in a bogus backup?\n\nThanks,\nJeff\n"},{"id":"212167","messageId":"20130325145644.GA16576@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CAOx6V3YtM-e8-S41v1KnC+uSymYwZw8QBwiCJRYw0MYJXRjj-w@mail.gmail.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T14:56:44Z","receivedAt":"2013-03-25T14:56:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 09:43:23AM -0400, Jeff Mitchell wrote:\n\n> > But I haven't seen exactly what the corruption is, nor exactly what\n> > commands they used to clone. I've invited the blog author to give more\n> > details in this thread.\n> \n> The syncing was performed via a clone with git clone --mirror (and a\n> git:// URL) and updates with git remote update.\n\nOK. That should be resilient to corruption, then[1].\n\n> So I should mention that my experiments after the fact were using\n> local paths, but with --no-hardlinks.\n\nYeah, we will do a direct copy in that case, and there is nothing to\nprevent corruption propagating.\n\n> If you're saying that the transport is where corruption is supposed to\n> be caught, then it's possible that we shouldn't see corruption\n> propagate on an initial mirror clone across git://, and that something\n> else was responsible for the trouble we saw with the repositories that\n> got cloned after-the-fact.\n\nRight. Either it was something else, or there is a bug in git's\nprotections (but I haven't been able to reproduce anything likely).\n\n> But then I'd argue that this is non-obvious. In particular, when using\n> --no-hardlinks, I wouldn't expect that behavior to be different with a\n> straight path and with file://.\n\nThere are basically three levels of transport that can be used on a\nlocal machine:\n\n 1. Hard-linking (very fast, no redundancy).\n\n 2. Byte-for-byte copy (medium speed, makes a separate copy of the\n data, but does not check the integrity of the original).\n\n 3. Regular git transport, creating a pack (slowest, but should include\n redundancy checks).\n\nUsing --no-hardlinks turns off (1), but leaves (2) as an option. I\nthink the documentation in \"git clone\" could use some improvement in\nthat area.\n\n> Something else: apparently one of my statements prompted joeyh to\n> think about potential issues with backing up live git repos\n> (http://joeyh.name/blog/entry/difficulties_in_backing_up_live_git_repositories/).\n> Looking at that post made me realize that, when we were doing our\n> initial thinking about the system three years ago, we made an\n> assumption that, in fact, taking a .tar.gz of a repo as it's in the\n> process of being written to or garbage collected or repacked could be\n> problematic. This isn't a totally baseless assumption, as I once had a\n> git repository that I was in the process of updating when I had a\n> sudden power outage that suffered corruption. (It could totally have\n> been the filesystem, of course, although it was a journaled file\n> system.)\n\nYes, if you take a snapshot of a repository with rsync or tar, it may be\nin an inconsistent state. Using the git protocol should always be\nrobust, but if you want to do it with other tools, you need to follow a\nparticular order:\n\n 1. copy the refs (refs/ and packed-refs) first\n\n 2. copy everything else (including object/)\n\nThat covers the case where somebody is pushing an update simultaneously\n(you _may_ get extra objects in step 2 that they have not yet\nreferenced, but you will never end up with a case where you are\nreferencing objects that you did not yet transfer).\n\nIf it's possible that the repository might be repacked during your\ntransfer, I think the issue a bit trickier, as there's a moment where\nthe new packfile is renamed into place, and then the old ones are\ndeleted. Depending on the timing and how your readdir() implementation\nbehaves with respect to new and deleted entries, it might be possible to\nmiss both the new one appearing and the old ones disappearing. This is\nquite a tight race to catch, I suspect, but if you were to rsync\nobjects/pack twice in a row, that would be sufficient.\n\nFor pruning, I think you could run into the opposite situation: you grab\nthe refs, somebody updates them with a history rewind (or branch\ndeletion), then somebody prunes and objects go away. Again, the timing\non this race is quite tight and it's unlikely in practice. I'm not sure\nof a simple way to eliminate it completely.\n\nYet another option is to simply rsync the whole thing and then \"git\nfsck\" the result. If it's not 100% good, just re-run the rsync. This is\nsimple and should be robust, but is more CPU intensive (you'll end up\nre-checking all of the data on each update).\n\n> So, we decided to use Git's built-in capabilities of consistency\n> checking to our advantage (with, as it turns out, a flaw in our\n> implementation). But the question remains: are we wrong about thinking\n> that rsyncing or tar.gz live repositories in the middle of being\n> pushed to/gc'd/repacked could result in a bogus backup?\n\nNo, I think you are right. If you do the refs-then-objects ordering,\nthat saves you from most of it, but I do think there are still some\nraces that exist during repacking or pruning.\n\n-Peff\n\n[1] I mentioned that clone-over-git:// is resilient to corruption. I\n think that is true for bit corruption, but my tests did show that we\n are not as careful about checking graph connectivity during clone as\n we are with fetch. The circumstances in which that would matter are\n quite unlikely, though.\n"},{"id":"212168","messageId":"CACsJy8A0eOWEJ2aqPSLof_CodJM6BadFxQHy5Vb0kAwwTSTS3w@mail.gmail.com","threadId":"33274","inReplyTo":"20130325145644.GA16576@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-25T15:31:04Z","receivedAt":"2013-03-25T15:31:04Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 25, 2013 at 9:56 PM, Jeff King wrote:\n> There are basically three levels of transport that can be used on a\n> local machine:\n>\n> 1. Hard-linking (very fast, no redundancy).\n>\n> 2. Byte-for-byte copy (medium speed, makes a separate copy of the\n> data, but does not check the integrity of the original).\n>\n> 3. Regular git transport, creating a pack (slowest, but should include\n> redundancy checks).\n>\n> Using --no-hardlinks turns off (1), but leaves (2) as an option. I\n> think the documentation in \"git clone\" could use some improvement in\n> that area.\n\nNot only git-clone. How git-fetch and git-push verify the new pack\nshould also be documented. I don't think many people outside the\ncontributor circle know what is done (and maybe how) when data is\nreceived from outside.\n-- \nDuy\n"},{"id":"212172","messageId":"20130325155600.GA18216@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CACsJy8A0eOWEJ2aqPSLof_CodJM6BadFxQHy5Vb0kAwwTSTS3w@mail.gmail.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T15:56:00Z","receivedAt":"2013-03-25T15:56:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 10:31:04PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Mon, Mar 25, 2013 at 9:56 PM, Jeff King wrote:\n> > There are basically three levels of transport that can be used on a\n> > local machine:\n> >\n> > 1. Hard-linking (very fast, no redundancy).\n> >\n> > 2. Byte-for-byte copy (medium speed, makes a separate copy of the\n> > data, but does not check the integrity of the original).\n> >\n> > 3. Regular git transport, creating a pack (slowest, but should include\n> > redundancy checks).\n> >\n> > Using --no-hardlinks turns off (1), but leaves (2) as an option. I\n> > think the documentation in \"git clone\" could use some improvement in\n> > that area.\n> \n> Not only git-clone. How git-fetch and git-push verify the new pack\n> should also be documented. I don't think many people outside the\n> contributor circle know what is done (and maybe how) when data is\n> received from outside.\n\nI think it's less of a documentation issue there, though, because they\n_only_ do (3). There is no option to do anything else, so there is\nnothing to warn the user about in terms of tradeoffs.\n\nI agree that in general git's handling of corruption could be documented\nsomewhere, but I'm not sure where.\n\n-Peff\n"},{"id":"212178","messageId":"CAOx6V3a6vGJvJ4HEmAXdTRKKCzRJS23OYd_em1b3aQLzPNEtQA@mail.gmail.com","threadId":"33274","inReplyTo":"20130325155600.GA18216@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Jeff Mitchell","fromEmail":"jeffrey.mitchell@gmail.com","sentAt":"2013-03-25T16:32:50Z","receivedAt":"2013-03-25T16:32:50Z","isPatch":false,"sender":{"key":"jeffrey.mitchell@gmail.com","avatar":null},"body":"On Mon, Mar 25, 2013 at 11:56 AM, Jeff King wrote:\n> On Mon, Mar 25, 2013 at 10:31:04PM +0700, Nguyen Thai Ngoc Duy wrote:\n>\n>> On Mon, Mar 25, 2013 at 9:56 PM, Jeff King wrote:\n>> > There are basically three levels of transport that can be used on a\n>> > local machine:\n>> >\n>> > 1. Hard-linking (very fast, no redundancy).\n>> >\n>> > 2. Byte-for-byte copy (medium speed, makes a separate copy of the\n>> > data, but does not check the integrity of the original).\n>> >\n>> > 3. Regular git transport, creating a pack (slowest, but should include\n>> > redundancy checks).\n>> >\n>> > Using --no-hardlinks turns off (1), but leaves (2) as an option. I\n>> > think the documentation in \"git clone\" could use some improvement in\n>> > that area.\n>>\n>> Not only git-clone. How git-fetch and git-push verify the new pack\n>> should also be documented. I don't think many people outside the\n>> contributor circle know what is done (and maybe how) when data is\n>> received from outside.\n>\n> I think it's less of a documentation issue there, though, because they\n> _only_ do (3). There is no option to do anything else, so there is\n> nothing to warn the user about in terms of tradeoffs.\n>\n> I agree that in general git's handling of corruption could be documented\n> somewhere, but I'm not sure where.\n\nHi there,\n\nFirst of all, thanks for the analysis, it's much appreciated. It's\ngood to know that we weren't totally off-base in thinking that a naive\ncopy may be out of sync, as small as the chance are (certainly we\nwouldn't have known the right ordering).\n\nI think what was conflating the issue in my testing is that with\n--mirror it implies --bare, so there would be checking of the objects\nwhen the working tree was being created, hence --mirror won't show the\nerror a normal clone will -- it's not a transport question, it's just\na matter of the normal clone doing more and so having more data run\nthrough checks.\n\nHowever, there are still problems. For blob corruptions, even in this\n--no-hardlinks, non --mirror case where an error was found, the exit\ncode from the clone was 0. I can see this tripping up all sorts of\nautomated scripts or repository GUIs that ignore the output and only\ncheck the error code, which is not an unreasonable thing to do.\n\nFor commit corruptions, the --no-hardlinks, non --mirror case refused\nto create the new repository and exited with an error code of 128. The\n--no-hardlinks, --mirror case spewed errors to the console, yet\n*still* created the new clone *and* returned an error code of zero.\n\nIt seems that when there is an \"error\" as opposed to a \"fatal\" it\ndoesn't affect the status code on a clone; I'd argue that it ought to.\nIf Git knows that the source repository has problems, it ought to be\nreflected in the status code so that scripts performing clones have a\nnormal way to detect this and alert a user/sysadmin/whoever. Even if a\nparticular cloning method doesn't perform all sanity checks, if it\nfinds something in the sanity checks it *does* perform, this should be\ntrumpeted, loudly, regardless of transport mechanism and regardless of\nwhether a user is watching the process or a script is.\n\nThanks,\nJeff\n"},{"id":"212205","messageId":"7vboa7xn7s.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"20130324183133.GA11200@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T20:01:59Z","receivedAt":"2013-03-25T20:01:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We _do_ see a problem during the checkout phase, but we don't propagate\n> a checkout failure to the exit code from clone. That is bad in general,\n> and should probably be fixed. Though it would never find corruption of\n> older objects in the history, anyway, so checkout should not be relied\n> on for robustness.\n\nIt is obvious that we should exit with non-zero status when we see a\nfailure from the checkout, but do we want to nuke the resulting\nrepository as in the case of normal transport failure? A checkout\nfailure might be due to being under quota for object store but\nrunning out of quota upon populating the working tree, in which case\nwe probably do not want to.\n\n> We do not notice the sha1 mis-match on the sending side (which we could,\n> if we checked the sha1 as we were sending). We do not notice the broken\n> object graph during the receive process either. I would have expected\n> check_everything_connected to handle this, but we don't actually call it\n> during clone! If you do this:\n>\n> $ git init non-local && cd non-local && git fetch ..\n> remote: Counting objects: 3, done.\n> remote: Total 3 (delta 0), reused 3 (delta 0)\n> Unpacking objects: 100% (3/3), done.\n> fatal: missing blob object 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n> error: .. did not send all necessary objects\n>\n> we do notice.\n\nYes, it is OK to add connectedness check to \"git clone\".\n\n> And one final check:\n>\n> $ git -c transfer.fsckobjects=1 clone --no-local . fsck\n> Cloning into 'fsck'...\n> remote: Counting objects: 3, done.\n> remote: Total 3 (delta 0), reused 3 (delta 0)\n> Receiving objects: 100% (3/3), done.\n> error: unable to find d95f3ad14dee633a758d2e331151e950dd13e4ed\n> fatal: object of unexpected type\n> fatal: index-pack failed\n>\n> Fscking the incoming objects does work, but of course it comes at a cost\n> in the normal case (for linux-2.6, I measured an increase in CPU time\n> with \"index-pack --strict\" from ~2.5 minutes to ~4 minutes). And I think\n> it is probably overkill for finding corruption; index-pack already\n> recognizes bit corruption inside an object, and\n> check_everything_connected can detect object graph problems much more\n> cheaply.\n\n> One thing I didn't check is bit corruption inside a packed object that\n> still correctly zlib inflates. check_everything_connected will end up\n> reading all of the commits and trees (to walk them), but not the blobs.\n\nCorrect.\n\n> So I think at the very least we should:\n>\n> 1. Make sure clone propagates errors from checkout to the final exit\n> code.\n>\n> 2. Teach clone to run check_everything_connected.\n\nI agree with both but with a slight reservation on the former one\n(see above).\n\nThanks.\n"},{"id":"212206","messageId":"20130325200525.GA3902@sigill.intra.peff.net","threadId":"33274","inReplyTo":"7vboa7xn7s.fsf@alter.siamese.dyndns.org","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:05:25Z","receivedAt":"2013-03-25T20:05:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 01:01:59PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We _do_ see a problem during the checkout phase, but we don't propagate\n> > a checkout failure to the exit code from clone. That is bad in general,\n> > and should probably be fixed. Though it would never find corruption of\n> > older objects in the history, anyway, so checkout should not be relied\n> > on for robustness.\n> \n> It is obvious that we should exit with non-zero status when we see a\n> failure from the checkout, but do we want to nuke the resulting\n> repository as in the case of normal transport failure? A checkout\n> failure might be due to being under quota for object store but\n> running out of quota upon populating the working tree, in which case\n> we probably do not want to.\n\nI'm just running through my final tests on a large-ish patch series\nwhich deals with this (among other issues). I had the same thought,\nthough we do already die on a variety of checkout errors. I left it as a\ndie() for now, but I think we should potentially address it with a\nfurther patch.\n\n> > $ git init non-local && cd non-local && git fetch ..\n> > remote: Counting objects: 3, done.\n> > remote: Total 3 (delta 0), reused 3 (delta 0)\n> > Unpacking objects: 100% (3/3), done.\n> > fatal: missing blob object 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n> > error: .. did not send all necessary objects\n> >\n> > we do notice.\n> \n> Yes, it is OK to add connectedness check to \"git clone\".\n\nThat's in my series, too. Unfortunately, in the local clone case, it\nslows down the clone considerably (since we otherwise would not have to\ntraverse the objects at all).\n\n-Peff\n"},{"id":"212208","messageId":"20130325200752.GB3902@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CAOx6V3a6vGJvJ4HEmAXdTRKKCzRJS23OYd_em1b3aQLzPNEtQA@mail.gmail.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:07:52Z","receivedAt":"2013-03-25T20:07:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote:\n\n> I think what was conflating the issue in my testing is that with\n> --mirror it implies --bare, so there would be checking of the objects\n> when the working tree was being created, hence --mirror won't show the\n> error a normal clone will -- it's not a transport question, it's just\n> a matter of the normal clone doing more and so having more data run\n> through checks.\n\nExactly.\n\n> However, there are still problems. For blob corruptions, even in this\n> --no-hardlinks, non --mirror case where an error was found, the exit\n> code from the clone was 0. I can see this tripping up all sorts of\n> automated scripts or repository GUIs that ignore the output and only\n> check the error code, which is not an unreasonable thing to do.\n\nYes, this is a bug. I'll post a series in a minute which fixes it.\n\n> For commit corruptions, the --no-hardlinks, non --mirror case refused\n> to create the new repository and exited with an error code of 128. The\n> --no-hardlinks, --mirror case spewed errors to the console, yet\n> *still* created the new clone *and* returned an error code of zero.\n\nI wasn't able to reproduce this; can you post a succint test case?\n\n> It seems that when there is an \"error\" as opposed to a \"fatal\" it\n> doesn't affect the status code on a clone; I'd argue that it ought to.\n\nAgreed completely. The current behavior is buggy.\n\n-Peff\n"},{"id":"212210","messageId":"20130325201427.GA15798@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130324183133.GA11200@sigill.intra.peff.net","subject":"[PATCH 0/9] corrupt object potpourri","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:14:27Z","receivedAt":"2013-03-25T20:14:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I started these patches with the intent of improving clone's behavior\non corrupt objects, but my testing uncovered some other nastiness,\nincluding two infinite loops in the streaming code!. Yikes.\n\nI think 1-7 are good. We might want to tweak the die() behavior of patch\n8, but I think it should come on top. Patch 9 has some pretty ugly\nperformance implications.\n\nAt the end of the series, all of the introduced tests pass except for\none, which is that \"git clone\" may silently write out a bogus working\ntree entry. I haven't tracked that one down yet.\n\n [1/9]: stream_blob_to_fd: detect errors reading from stream\n [2/9]: check_sha1_signature: check return value from read_istream\n [3/9]: read_istream_filtered: propagate read error from upstream\n [4/9]: avoid infinite loop in read_istream_loose\n [5/9]: add test for streaming corrupt blobs\n [6/9]: streaming_write_entry: propagate streaming errors\n [7/9]: add tests for cloning corrupted repositories\n [8/9]: clone: die on errors from unpack_trees\n [9/9]: clone: run check_everything_connected\n\n-Peff\n"},{"id":"212212","messageId":"20130325201650.GA16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 1/9] stream_blob_to_fd: detect errors reading from stream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:16:50Z","receivedAt":"2013-03-25T20:16:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call read_istream, but never check its return value for\nerrors. This can lead to us looping infinitely, as we just\nkeep trying to write \"-1\" bytes (and we do not notice the\nerror, as we simply check that write_in_full reports the\nsame number of bytes we fed it, which of course is also -1).\n\nSigned-off-by: Jeff King \n---\nNo test yet, as my method for triggering this causes _another_ infinite\nloop. So the test comes after the fixes, to avoid infinite loops when\nbisecting the history later. :)\n\n streaming.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/streaming.c b/streaming.c\nindex 4d978e5..f4126a7 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -514,6 +514,8 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n \t\tssize_t wrote, holeto;\n \t\tssize_t readlen = read_istream(st, buf, sizeof(buf));\n \n+\t\tif (readlen < 0)\n+\t\t\tgoto close_and_exit;\n \t\tif (!readlen)\n \t\t\tbreak;\n \t\tif (can_seek && sizeof(buf) == readlen) {\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212213","messageId":"20130325201717.GB16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 2/9] check_sha1_signature: check return value from read_istream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:17:17Z","receivedAt":"2013-03-25T20:17:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible for read_istream to return an error, in which\ncase we just end up in an infinite loop (aside from EOF, we\ndo not even look at the result, but just feed it straight\ninto our running hash).\n\nSigned-off-by: Jeff King \n---\nI didn't actually trigger this code path in any of my tests, but I\naudited all of the callers of read_istream after the last patch, and\nnoticed this one (the rest looked fine to me).\n\n sha1_file.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 16967d3..0b99f33 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1266,6 +1266,10 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \t\tchar buf[1024 * 16];\n \t\tssize_t readlen = read_istream(st, buf, sizeof(buf));\n \n+\t\tif (readlen < 0) {\n+\t\t\tclose_istream(st);\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (!readlen)\n \t\t\tbreak;\n \t\tgit_SHA1_Update(&c, buf, readlen);\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212214","messageId":"20130325201816.GC16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 3/9] read_istream_filtered: propagate read error from upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:18:16Z","receivedAt":"2013-03-25T20:18:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The filter istream pulls data from an \"upstream\" stream,\nrunning it through a filter function. However, we did not\nproperly notice when the upstream filter yielded an error,\nand just returned what we had read. Instead, we should\npropagate the error.\n\nSigned-off-by: Jeff King \n---\nI don't know if we should be preserving fs->i_end from getting a\nnegative value. I would think the internal state of the istream after an\nerror is undefined.\n\n streaming.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex f4126a7..f4ab12b 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -237,7 +237,7 @@ static read_method_decl(filtered)\n \t\tif (!fs->input_finished) {\n \t\t\tfs->i_end = read_istream(fs->upstream, fs->ibuf, FILTER_BUFFER);\n \t\t\tif (fs->i_end < 0)\n-\t\t\t\tbreak;\n+\t\t\t\treturn -1;\n \t\t\tif (fs->i_end)\n \t\t\t\tcontinue;\n \t\t}\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212215","messageId":"20130325202114.GD16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 4/9] avoid infinite loop in read_istream_loose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:21:14Z","receivedAt":"2013-03-25T20:21:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The read_istream_loose function loops on inflating a chunk of data\nfrom an mmap'd loose object. We end the loop when we run out\nof space in our output buffer, or if we see a zlib error.\n\nWe need to treat Z_BUF_ERROR specially, though, as it is not\nfatal; it is just zlib's way of telling us that we need to\neither feed it more input or give it more output space. It\nis perfectly normal for us to hit this when we are at the\nend of our buffer.\n\nHowever, we may also get Z_BUF_ERROR because we have run out\nof input. In a well-formed object, this should not happen,\nbecause we have fed the whole mmap'd contents to zlib. But\nif the object is truncated or corrupt, we will loop forever,\nnever giving zlib any more data, but continuing to ask it to\ninflate.\n\nWe can fix this by considering it an error when zlib returns\nZ_BUF_ERROR but we still have output space left (which means\nit must want more input, which we know is a truncation\nerror). It would not be sufficient to just check whether\nzlib had consumed all the input at the start of the loop, as\nit might still want to generate output from what is in its\ninternal state.\n\nSigned-off-by: Jeff King \n---\nThe read_istream_pack_non_delta function does not suffer from the same\nissue, because it continually feeds more data via use_pack(). Although\nit may run into problems if it reads to the very end of a pack. I also\ndidn't audit the other zlib code paths for similar problems; we may want\nto do that.\n\n streaming.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex f4ab12b..cabcd9d 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -309,7 +309,7 @@ static read_method_decl(loose)\n \t\t\tst->z_state = z_done;\n \t\t\tbreak;\n \t\t}\n-\t\tif (status != Z_OK && status != Z_BUF_ERROR) {\n+\t\tif (status != Z_OK && (status != Z_BUF_ERROR || total_read < sz)) {\n \t\t\tgit_inflate_end(&st->z);\n \t\t\tst->z_state = z_error;\n \t\t\treturn -1;\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212216","messageId":"20130325202134.GE16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 5/9] add test for streaming corrupt blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:21:34Z","receivedAt":"2013-03-25T20:21:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not have many tests for handling corrupt objects. This\nnew test at least checks that we detect a byte error in a\ncorrupt blob object while streaming it out with cat-file.\n\nSigned-off-by: Jeff King \n---\n t/t1060-object-corruption.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n create mode 100755 t/t1060-object-corruption.sh\n\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nnew file mode 100755\nindex 0000000..d36994a\n--- /dev/null\n+++ b/t/t1060-object-corruption.sh\n@@ -0,0 +1,34 @@\n+#!/bin/sh\n+\n+test_description='see how we handle various forms of corruption'\n+. ./test-lib.sh\n+\n+# convert \"1234abcd\" to \".git/objects/12/34abcd\"\n+obj_to_file() {\n+\techo \"$(git rev-parse --git-dir)/objects/$(git rev-parse \"$1\" | sed 's,..,&/,')\"\n+}\n+\n+# Convert byte at offset \"$2\" of object \"$1\" into '\\0'\n+corrupt_byte() {\n+\tobj_file=$(obj_to_file \"$1\") &&\n+\tchmod +w \"$obj_file\" &&\n+\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n+}\n+\n+test_expect_success 'setup corrupt repo' '\n+\tgit init bit-error &&\n+\t(\n+\t\tcd bit-error &&\n+\t\ttest_commit content &&\n+\t\tcorrupt_byte HEAD:content.t 10\n+\t)\n+'\n+\n+test_expect_success 'streaming a corrupt blob fails' '\n+\t(\n+\t\tcd bit-error &&\n+\t\ttest_must_fail git cat-file blob HEAD:content.t\n+\t)\n+'\n+\n+test_done\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212217","messageId":"20130325202216.GF16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 6/9] streaming_write_entry: propagate streaming errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:22:17Z","receivedAt":"2013-03-25T20:22:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are streaming an index blob to disk, we store the\nerror from stream_blob_to_fd in the \"result\" variable, and\nthen immediately overwrite that with the return value of\n\"close\". That means we catch errors on close (e.g., problems\ncommitting the file to disk), but miss anything which\nhappened before then.\n\nSigned-off-by: Jeff King \n---\n entry.c | 6 ++++--\n t/t1060-object-corruption.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 17a6bcc..002b2f2 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -126,8 +126,10 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n \tfd = open_output_fd(path, ce, to_tempfile);\n \tif (0 <= fd) {\n \t\tresult = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n-\t\t*fstat_done = fstat_output(fd, state, statbuf);\n-\t\tresult = close(fd);\n+\t\tif (!result) {\n+\t\t\t*fstat_done = fstat_output(fd, state, statbuf);\n+\t\t\tresult = close(fd);\n+\t\t}\n \t}\n \tif (result && 0 <= fd)\n \t\tunlink(path);\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex d36994a..0792132 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -24,6 +24,15 @@ test_expect_success 'setup corrupt repo' '\n \t)\n '\n \n+test_expect_success 'setup repo with missing object' '\n+\tgit init missing &&\n+\t(\n+\t\tcd missing &&\n+\t\ttest_commit content &&\n+\t\trm -f \"$(obj_to_file HEAD:content.t)\"\n+\t)\n+'\n+\n test_expect_success 'streaming a corrupt blob fails' '\n \t(\n \t\tcd bit-error &&\n@@ -31,4 +40,20 @@ test_expect_success 'streaming a corrupt blob fails' '\n \t)\n '\n \n+test_expect_success 'read-tree -u detects bit-errors in blobs' '\n+\t(\n+\t\tcd bit-error &&\n+\t\trm content.t &&\n+\t\ttest_must_fail git read-tree --reset -u FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success 'read-tree -u detects missing objects' '\n+\t(\n+\t\tcd missing &&\n+\t\trm content.t &&\n+\t\ttest_must_fail git read-tree --reset -u FETCH_HEAD\n+\t)\n+'\n+\n test_done\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212218","messageId":"20130325202229.GG16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 7/9] add tests for cloning corrupted repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:22:29Z","receivedAt":"2013-03-25T20:22:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We try not to let corruption pass unnoticed over fetches and\nclones. For the most part, this works, but there are some\nbroken corner cases, including:\n\n 1. We do not detect missing objects over git-aware\n transports. This is a little hard to test, because the\n sending side will actually complain about the missing\n object.\n\n To fool it, we corrupt a repository such that we have a\n \"misnamed\" object: it claims to be sha1 X, but is\n really Y. This lets the sender blindly transmit it, but\n it is the receiver's responsibility to verify that what\n it got is sane (and it does not).\n\n 2. We do not detect missing or misnamed blobs during the\n checkout phase of clone.\n\nSigned-off-by: Jeff King \n---\n t/t1060-object-corruption.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 0792132..eb285c0 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -33,6 +33,19 @@ test_expect_success 'setup repo with missing object' '\n \t)\n '\n \n+test_expect_success 'setup repo with misnamed object' '\n+\tgit init misnamed &&\n+\t(\n+\t\tcd misnamed &&\n+\t\ttest_commit content &&\n+\t\tgood=$(obj_to_file HEAD:content.t) &&\n+\t\tblob=$(echo corrupt | git hash-object -w --stdin) &&\n+\t\tbad=$(obj_to_file $blob) &&\n+\t\trm -f \"$good\" &&\n+\t\tmv \"$bad\" \"$good\"\n+\t)\n+'\n+\n test_expect_success 'streaming a corrupt blob fails' '\n \t(\n \t\tcd bit-error &&\n@@ -56,4 +69,32 @@ test_expect_success 'read-tree -u detects missing objects' '\n \t)\n '\n \n+# We use --bare to make sure that the transport detects it, not the checkout\n+# phase.\n+test_expect_success 'clone --no-local --bare detects corruption' '\n+\ttest_must_fail git clone --no-local --bare bit-error corrupt-transport\n+'\n+\n+test_expect_success 'clone --no-local --bare detects missing object' '\n+\ttest_must_fail git clone --no-local --bare missing missing-transport\n+'\n+\n+test_expect_failure 'clone --no-local --bare detects misnamed object' '\n+\ttest_must_fail git clone --no-local --bare misnamed misnamed-transport\n+'\n+\n+# We do not expect --local to detect corruption at the transport layer,\n+# so we are really checking the checkout() code path.\n+test_expect_success 'clone --local detects corruption' '\n+\ttest_must_fail git clone --local bit-error corrupt-checkout\n+'\n+\n+test_expect_failure 'clone --local detects missing objects' '\n+\ttest_must_fail git clone --local missing missing-checkout\n+'\n+\n+test_expect_failure 'clone --local detects misnamed objects' '\n+\ttest_must_fail git clone --local misnamed misnamed-checkout\n+'\n+\n test_done\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212219","messageId":"20130325202359.GH16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 8/9] clone: die on errors from unpack_trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:23:59Z","receivedAt":"2013-03-25T20:23:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When clone is populating the working tree, it ignores the\nreturn status from unpack_trees; this means we may report a\nsuccessful clone, even when the checkout fails.\n\nWhen checkout fails, we may want to leave the $GIT_DIR in\nplace, as it might be possible to recover the data through\nfurther use of \"git checkout\" (e.g., if the checkout failed\ndue to a transient error, disk full, etc). However, we\nalready die on a number of other checkout-related errors, so\nthis patch follows that pattern.\n\nIn addition to marking a now-passing test, we need to adjust\nt5710, which blindly assumed it could make bogus clones of\nvery deep alternates hierarchies. By using \"--bare\", we can\navoid it actually touching any objects.\n\nSigned-off-by: Jeff King \n---\nI think the \"leave the data behind\" fix may be to just set \"junk_pid =\n0\" a little sooner in cmd_clone (i.e., before checkout()). Then we\nwould still die, but at least leave the fetched objects intact.\n\n builtin/clone.c | 3 ++-\n t/t1060-object-corruption.sh | 2 +-\n t/t5710-info-alternate.sh | 2 +-\n 3 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex e0aaf13..7d48ef3 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -579,7 +579,8 @@ static int checkout(void)\n \ttree = parse_tree_indirect(sha1);\n \tparse_tree(tree);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\tunpack_trees(1, &t, &opts);\n+\tif (unpack_trees(1, &t, &opts) < 0)\n+\t\tdie(_(\"unable to checkout working tree\"));\n \n \tif (write_cache(fd, active_cache, active_nr) ||\n \t commit_locked_index(lock_file))\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex eb285c0..05ba4e7 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -89,7 +89,7 @@ test_expect_success 'clone --local detects corruption' '\n \ttest_must_fail git clone --local bit-error corrupt-checkout\n '\n \n-test_expect_failure 'clone --local detects missing objects' '\n+test_expect_success 'clone --local detects missing objects' '\n \ttest_must_fail git clone --local missing missing-checkout\n '\n \ndiff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh\nindex aa04529..5a6e49d 100755\n--- a/t/t5710-info-alternate.sh\n+++ b/t/t5710-info-alternate.sh\n@@ -58,7 +58,7 @@ git clone -l -s F G &&\n git clone -l -s D E &&\n git clone -l -s E F &&\n git clone -l -s F G &&\n-git clone -l -s G H'\n+git clone --bare -l -s G H'\n \n test_expect_success 'invalidity of deepest repository' \\\n 'cd H && {\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212220","messageId":"20130325202627.GI16019@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325201427.GA15798@sigill.intra.peff.net","subject":"[PATCH 9/9] clone: run check_everything_connected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T20:26:27Z","receivedAt":"2013-03-25T20:26:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fetch from a remote, we do a revision walk to make\nsure that what we received is connected to our existing\nhistory. We do not do the same check for clone, which should\nbe able to check that we received an intact history graph.\n\nThe upside of this patch is that it will make clone more\nresilient against propagating repository corruption. The\ndownside is that we will now traverse \"rev-list --objects\n--all\" down to the roots, which may take some time (it is\nespecially noticeable for a \"--local --bare\" clone).\n\nNote that we need to adjust t5710, which tries to make such\na bogus clone. Rather than checking after the fact that our\nclone is bogus, we can simplify it to just make sure \"git\nclone\" reports failure.\n\nSigned-off-by: Jeff King \n---\nThe slowdown is really quite terrible if you try \"git clone --bare\nlinux-2.6.git\". Even with this, the local-clone case already misses blob\ncorruption. So it probably makes sense to restrict it to just the\nnon-local clone case, which already has to do more work.\n\nEven still, it adds a non-trivial amount of work (linux-2.6 takes\nsomething like a minute to check). I don't like the idea of declaring\n\"git clone\" non-safe unless you turn on transfer.fsckObjects, though. It\nshould have the same safety as \"git fetch\".\n\n builtin/clone.c | 26 ++++++++++++++++++++++++++\n t/t1060-object-corruption.sh | 2 +-\n t/t5710-info-alternate.sh | 8 +-------\n 3 files changed, 28 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 7d48ef3..eceaa74 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -23,6 +23,7 @@\n #include \"branch.h\"\n #include \"remote.h\"\n #include \"run-command.h\"\n+#include \"connected.h\"\n \n /*\n * Overall FIXMEs:\n@@ -485,12 +486,37 @@ static void update_remote_refs(const struct ref *refs,\n \t}\n }\n \n+static int iterate_ref_map(void *cb_data, unsigned char sha1[20])\n+{\n+\tstruct ref **rm = cb_data;\n+\tstruct ref *ref = *rm;\n+\n+\t/*\n+\t * Skip anything missing a peer_ref, which we are not\n+\t * actually going to write a ref for.\n+\t */\n+\twhile (ref && !ref->peer_ref)\n+\t\tref = ref->next;\n+\t/* Returning -1 notes \"end of list\" to the caller. */\n+\tif (!ref)\n+\t\treturn -1;\n+\n+\thashcpy(sha1, ref->old_sha1);\n+\t*rm = ref->next;\n+\treturn 0;\n+}\n+\n static void update_remote_refs(const struct ref *refs,\n \t\t\t const struct ref *mapped_refs,\n \t\t\t const struct ref *remote_head_points_at,\n \t\t\t const char *branch_top,\n \t\t\t const char *msg)\n {\n+\tconst struct ref *rm = mapped_refs;\n+\n+\tif (check_everything_connected(iterate_ref_map, 0, &rm))\n+\t\tdie(_(\"remote did not send all necessary objects\"));\n+\n \tif (refs) {\n \t\twrite_remote_refs(mapped_refs);\n \t\tif (option_single_branch)\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 05ba4e7..fd314ef 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -79,7 +79,7 @@ test_expect_success 'clone --no-local --bare detects missing object' '\n \ttest_must_fail git clone --no-local --bare missing missing-transport\n '\n \n-test_expect_failure 'clone --no-local --bare detects misnamed object' '\n+test_expect_success 'clone --no-local --bare detects misnamed object' '\n \ttest_must_fail git clone --no-local --bare misnamed misnamed-transport\n '\n \ndiff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh\nindex 5a6e49d..8956c21 100755\n--- a/t/t5710-info-alternate.sh\n+++ b/t/t5710-info-alternate.sh\n@@ -58,13 +58,7 @@ git clone -l -s F G &&\n git clone -l -s D E &&\n git clone -l -s E F &&\n git clone -l -s F G &&\n-git clone --bare -l -s G H'\n-\n-test_expect_success 'invalidity of deepest repository' \\\n-'cd H && {\n-\ttest_valid_repo\n-\ttest $? -ne 0\n-}'\n+test_must_fail git clone --bare -l -s G H'\n \n cd \"$base_dir\"\n \n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212224","messageId":"20130325211038.GD1414@google.com","threadId":"33274","inReplyTo":"20130325202134.GE16019@sigill.intra.peff.net","subject":"Re: [PATCH 5/9] add test for streaming corrupt blobs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-25T21:10:38Z","receivedAt":"2013-03-25T21:10:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We do not have many tests for handling corrupt objects. This\n> new test at least checks that we detect a byte error in a\n> corrupt blob object while streaming it out with cat-file.\n\nThanks.\n\n[...]\n> +# convert \"1234abcd\" to \".git/objects/12/34abcd\"\n> +obj_to_file() {\n> +\techo \"$(git rev-parse --git-dir)/objects/$(git rev-parse \"$1\" | sed 's,..,&/,')\"\n> +}\n\nMaybe this would be clearer in multiple lines?\n\n\tcommit=$(git rev-parse --verify \"$1\") &&\n\tgit_dir=$(git rev-parse --git-dir) &&\n\ttail=${commit#??} &&\n\techo \"$git_dir/objects/${commit%$tail}/$tail\"\n\n> +\n> +# Convert byte at offset \"$2\" of object \"$1\" into '\\0'\n> +corrupt_byte() {\n> +\tobj_file=$(obj_to_file \"$1\") &&\n> +\tchmod +w \"$obj_file\" &&\n> +\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n\nSome other tests such as t4205 also rely on \"printf\" being\nbinary-safe. Phew.\n\n> +}\n> +\n> +test_expect_success 'setup corrupt repo' '\n> +\tgit init bit-error &&\n> +\t(\n> +\t\tcd bit-error &&\n> +\t\ttest_commit content &&\n> +\t\tcorrupt_byte HEAD:content.t 10\n> +\t)\n> +'\n> +\n> +test_expect_success 'streaming a corrupt blob fails' '\n\n\"fails gracefully\", maybe, to be more precise.\n\nWith or without the two changes suggested above,\nReviewed-by: Jonathan Nieder \n"},{"id":"212225","messageId":"20130325212605.GA19303@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325211038.GD1414@google.com","subject":"Re: [PATCH 5/9] add test for streaming corrupt blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T21:26:05Z","receivedAt":"2013-03-25T21:26:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 02:10:38PM -0700, Jonathan Nieder wrote:\n\n> > +# convert \"1234abcd\" to \".git/objects/12/34abcd\"\n> > +obj_to_file() {\n> > +\techo \"$(git rev-parse --git-dir)/objects/$(git rev-parse \"$1\" | sed 's,..,&/,')\"\n> > +}\n> \n> Maybe this would be clearer in multiple lines?\n> \n> \tcommit=$(git rev-parse --verify \"$1\") &&\n> \tgit_dir=$(git rev-parse --git-dir) &&\n> \ttail=${commit#??} &&\n> \techo \"$git_dir/objects/${commit%$tail}/$tail\"\n\nYeah, it started as:\n\n echo \"$1\" | sed 's,..,&/,'\n\nand kind of got out of hand as it grew features. I'd be fine with your\nversion (though $commit is not right, as it is any object, and in fact\nthe test uses blobs).\n\n> > +\n> > +# Convert byte at offset \"$2\" of object \"$1\" into '\\0'\n> > +corrupt_byte() {\n> > +\tobj_file=$(obj_to_file \"$1\") &&\n> > +\tchmod +w \"$obj_file\" &&\n> > +\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n> \n> Some other tests such as t4205 also rely on \"printf\" being\n> binary-safe. Phew.\n\nYeah, I think it should be fine, though the choice of character does not\nactually matter, as long as it is different from what is currently at\nthat position (for the sake of simplicity, I just determined\nexperimentally that the given object is corrupted with the offset and\ncharacter I chose).\n\n-Peff\n"},{"id":"212227","messageId":"CAPig+cRjK6mrRm+K4Qzf2CsjT3SYGotZ2PrVLniYzdBRC1Mv2A@mail.gmail.com","threadId":"33274","inReplyTo":"20130325202216.GF16019@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] streaming_write_entry: propagate streaming errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-25T21:35:51Z","receivedAt":"2013-03-25T21:35:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 25, 2013 at 4:22 PM, Jeff King wrote:\n> diff --git a/entry.c b/entry.c\n> index 17a6bcc..002b2f2 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -126,8 +126,10 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n> fd = open_output_fd(path, ce, to_tempfile);\n> if (0 <= fd) {\n> result = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n> - *fstat_done = fstat_output(fd, state, statbuf);\n> - result = close(fd);\n> + if (!result) {\n> + *fstat_done = fstat_output(fd, state, statbuf);\n> + result = close(fd);\n> + }\n\nIs this intentionally leaking the opened 'fd' when stream_blob_to_fd()\nreturns an error?\n\n> }\n> if (result && 0 <= fd)\n> unlink(path);\n\nWon't the unlink() now fail on Windows since 'fd' is still open?\n\n-- ES\n"},{"id":"212228","messageId":"20130325213737.GC19303@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CAPig+cRjK6mrRm+K4Qzf2CsjT3SYGotZ2PrVLniYzdBRC1Mv2A@mail.gmail.com","subject":"Re: [PATCH 6/9] streaming_write_entry: propagate streaming errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T21:37:37Z","receivedAt":"2013-03-25T21:37:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 05:35:51PM -0400, Eric Sunshine wrote:\n\n> On Mon, Mar 25, 2013 at 4:22 PM, Jeff King wrote:\n> > diff --git a/entry.c b/entry.c\n> > index 17a6bcc..002b2f2 100644\n> > --- a/entry.c\n> > +++ b/entry.c\n> > @@ -126,8 +126,10 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n> > fd = open_output_fd(path, ce, to_tempfile);\n> > if (0 <= fd) {\n> > result = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n> > - *fstat_done = fstat_output(fd, state, statbuf);\n> > - result = close(fd);\n> > + if (!result) {\n> > + *fstat_done = fstat_output(fd, state, statbuf);\n> > + result = close(fd);\n> > + }\n> \n> Is this intentionally leaking the opened 'fd' when stream_blob_to_fd()\n> returns an error?\n\nGood catch. I was so focused on making sure we still called unlink that\nI forgot about the cleanup side-effect of close.\n\nI'll re-roll it.\n\n-Peff\n"},{"id":"212230","messageId":"20130325213934.GE1414@google.com","threadId":"33274","inReplyTo":"20130325202216.GF16019@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] streaming_write_entry: propagate streaming errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-25T21:39:34Z","receivedAt":"2013-03-25T21:39:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> When we are streaming an index blob to disk, we store the\n> error from stream_blob_to_fd in the \"result\" variable, and\n> then immediately overwrite that with the return value of\n> \"close\".\n\nGood catch.\n\n[...]\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -126,8 +126,10 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n> \tfd = open_output_fd(path, ce, to_tempfile);\n> \tif (0 <= fd) {\n> \t\tresult = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n> -\t\t*fstat_done = fstat_output(fd, state, statbuf);\n> -\t\tresult = close(fd);\n> +\t\tif (!result) {\n> +\t\t\t*fstat_done = fstat_output(fd, state, statbuf);\n> +\t\t\tresult = close(fd);\n> +\t\t}\n\nShould this do something like\n\n\n\t{\n\t\tint fd, result = 0;\n\n\t\tfd = open_output_fd(path, ce, to_tempfile);\n\t\tif (fd < 0)\n\t\t\treturn -1;\n\n\t\tresult = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n\t\tif (result)\n\t\t\tgoto close_fd;\n\n\t\t*fstat_done = fstat_output(fd, state, statbuf);\n\tclose_fd:\n\t\tresult |= close(fd);\n\tunlink_path:\n\t\tif (result)\n\t\t\tunlink(path);\n\t\treturn result;\n\t}\n\nto avoid leaking the file descriptor?\n\n> @@ -31,4 +40,20 @@ test_expect_success 'streaming a corrupt blob fails' '\n> \t)\n> '\n> \n> +test_expect_success 'read-tree -u detects bit-errors in blobs' '\n> +\t(\n> +\t\tcd bit-error &&\n> +\t\trm content.t &&\n> +\t\ttest_must_fail git read-tree --reset -u FETCH_HEAD\n> +\t)\n\nMakes sense. Might make sense to use \"rm -f\" instead of \"rm\" to avoid\nfailures if content.t is removed already.\n\n> +'\n> +\n> +test_expect_success 'read-tree -u detects missing objects' '\n> +\t(\n> +\t\tcd missing &&\n> +\t\trm content.t &&\n\nEspecially here.\n\nThanks,\nJonathan\n"},{"id":"212234","messageId":"20130325214936.GA22419@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325213934.GE1414@google.com","subject":"[PATCH v2 6/9] streaming_write_entry: propagate streaming errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T21:49:36Z","receivedAt":"2013-03-25T21:49:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 02:39:34PM -0700, Jonathan Nieder wrote:\n\n> > --- a/entry.c\n> > +++ b/entry.c\n> > @@ -126,8 +126,10 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n> > \tfd = open_output_fd(path, ce, to_tempfile);\n> > \tif (0 <= fd) {\n> > \t\tresult = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n> > -\t\t*fstat_done = fstat_output(fd, state, statbuf);\n> > -\t\tresult = close(fd);\n> > +\t\tif (!result) {\n> > +\t\t\t*fstat_done = fstat_output(fd, state, statbuf);\n> > +\t\t\tresult = close(fd);\n> > +\t\t}\n> \n> Should this do something like\n> [...]\n> to avoid leaking the file descriptor?\n\nYes, Eric Sunshine noticed this, too. Re-rolled patch is below, which I\nthink is even a little cleaner.\n\n> > +test_expect_success 'read-tree -u detects bit-errors in blobs' '\n> > +\t(\n> > +\t\tcd bit-error &&\n> > +\t\trm content.t &&\n> > +\t\ttest_must_fail git read-tree --reset -u FETCH_HEAD\n> > +\t)\n> \n> Makes sense. Might make sense to use \"rm -f\" instead of \"rm\" to avoid\n> failures if content.t is removed already.\n\nYeah, good point. My original test looked like:\n\n git init bit-error &&\n git fetch .. &&\n corrupt ...\n test_must_fail ...\n\nbut I ended up refactoring it to re-use the corrupted directories, and\nadded the \"rm\" after the fact. The use of FETCH_HEAD is also bogus\n(read-tree is failing, but because we are giving it a bogus ref, not\nbecause of the corruption, so we are not actually testing anything\nanymore, even though it still passes).\n\nBoth fixed in my re-roll.\n\n-- >8 --\nSubject: [PATCH] streaming_write_entry: propagate streaming errors\n\nWhen we are streaming an index blob to disk, we store the\nerror from stream_blob_to_fd in the \"result\" variable, and\nthen immediately overwrite that with the return value of\n\"close\". That means we catch errors on close (e.g., problems\ncommitting the file to disk), but miss anything which\nhappened before then.\n\nWe can fix this by using bitwise-OR to accumulate errors in\nour result variable.\n\nWhile we're here, we can also simplify the error handling\nwith an early return, which makes it easier to see under\nwhich circumstances we need to clean up.\n\nSigned-off-by: Jeff King \n---\n entry.c | 16 +++++++++-------\n t/t1060-object-corruption.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 7 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 17a6bcc..a20bcbc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -120,16 +120,18 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,\n \t\t\t\t const struct checkout *state, int to_tempfile,\n \t\t\t\t int *fstat_done, struct stat *statbuf)\n {\n-\tint result = -1;\n+\tint result = 0;\n \tint fd;\n \n \tfd = open_output_fd(path, ce, to_tempfile);\n-\tif (0 <= fd) {\n-\t\tresult = stream_blob_to_fd(fd, ce->sha1, filter, 1);\n-\t\t*fstat_done = fstat_output(fd, state, statbuf);\n-\t\tresult = close(fd);\n-\t}\n-\tif (result && 0 <= fd)\n+\tif (fd < 0)\n+\t\treturn -1;\n+\n+\tresult |= stream_blob_to_fd(fd, ce->sha1, filter, 1);\n+\t*fstat_done = fstat_output(fd, state, statbuf);\n+\tresult |= close(fd);\n+\n+\tif (result)\n \t\tunlink(path);\n \treturn result;\n }\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex d36994a..2945395 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -24,6 +24,15 @@ test_expect_success 'setup corrupt repo' '\n \t)\n '\n \n+test_expect_success 'setup repo with missing object' '\n+\tgit init missing &&\n+\t(\n+\t\tcd missing &&\n+\t\ttest_commit content &&\n+\t\trm -f \"$(obj_to_file HEAD:content.t)\"\n+\t)\n+'\n+\n test_expect_success 'streaming a corrupt blob fails' '\n \t(\n \t\tcd bit-error &&\n@@ -31,4 +40,20 @@ test_expect_success 'streaming a corrupt blob fails' '\n \t)\n '\n \n+test_expect_success 'read-tree -u detects bit-errors in blobs' '\n+\t(\n+\t\tcd bit-error &&\n+\t\trm -f content.t &&\n+\t\ttest_must_fail git read-tree --reset -u HEAD\n+\t)\n+'\n+\n+test_expect_success 'read-tree -u detects missing objects' '\n+\t(\n+\t\tcd missing &&\n+\t\trm -f content.t &&\n+\t\ttest_must_fail git read-tree --reset -u HEAD\n+\t)\n+'\n+\n test_done\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212251","messageId":"20130325232947.GJ1414@google.com","threadId":"33274","inReplyTo":"20130325214936.GA22419@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/9] streaming_write_entry: propagate streaming errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-25T23:29:47Z","receivedAt":"2013-03-25T23:29:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Both fixed in my re-roll.\n\nThanks! This and the rest of the patches up to and including patch 8\nlook good to me.\n\nI haven't decided what to think about patch 9 yet, but I suspect it\nwould be good, too. In the long term I suspect \"git clone\n--worktree-only \" (or some other standard interface for\ngit-new-workdir functionality) is a better way to provide a convenient\nlightweight same-machine clone anyway.\n\nJonathan\n"},{"id":"212258","messageId":"CACsJy8CbBeuHmkEJs4FqGJs_kqEcjKi7RJkp9eNorxJAqgiCrg@mail.gmail.com","threadId":"33274","inReplyTo":"20130325202627.GI16019@sigill.intra.peff.net","subject":"Re: [PATCH 9/9] clone: run check_everything_connected","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-26T00:53:42Z","receivedAt":"2013-03-26T00:53:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 26, 2013 at 3:26 AM, Jeff King wrote:\n> static void update_remote_refs(const struct ref *refs,\n> const struct ref *mapped_refs,\n> const struct ref *remote_head_points_at,\n> const char *branch_top,\n> const char *msg)\n> {\n> + const struct ref *rm = mapped_refs;\n> +\n> + if (check_everything_connected(iterate_ref_map, 0, &rm))\n> + die(_(\"remote did not send all necessary objects\"));\n> +\n> if (refs) {\n> write_remote_refs(mapped_refs);\n> if (option_single_branch)\n\nMaybe move this after checkout, so that I can switch terminal and\nstart working while it's verifying? And maybe an option not to\ncheck_everything_connected, instead print a big fat warning telling\nthe user to fsck later?\n-- \nDuy\n"},{"id":"212259","messageId":"CACsJy8CZvDfCPPbsRkKJJXiQNpuQOXD-Hm-A7ePbu9tFaG_v5A@mail.gmail.com","threadId":"33274","inReplyTo":"20130325155600.GA18216@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-26T01:06:33Z","receivedAt":"2013-03-26T01:06:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 25, 2013 at 10:56 PM, Jeff King wrote:\n> On Mon, Mar 25, 2013 at 10:31:04PM +0700, Nguyen Thai Ngoc Duy wrote:\n>\n>> On Mon, Mar 25, 2013 at 9:56 PM, Jeff King wrote:\n>> > There are basically three levels of transport that can be used on a\n>> > local machine:\n>> >\n>> > 1. Hard-linking (very fast, no redundancy).\n>> >\n>> > 2. Byte-for-byte copy (medium speed, makes a separate copy of the\n>> > data, but does not check the integrity of the original).\n>> >\n>> > 3. Regular git transport, creating a pack (slowest, but should include\n>> > redundancy checks).\n>> >\n>> > Using --no-hardlinks turns off (1), but leaves (2) as an option. I\n>> > think the documentation in \"git clone\" could use some improvement in\n>> > that area.\n>>\n>> Not only git-clone. How git-fetch and git-push verify the new pack\n>> should also be documented. I don't think many people outside the\n>> contributor circle know what is done (and maybe how) when data is\n>> received from outside.\n>\n> I think it's less of a documentation issue there, though, because they\n> _only_ do (3). There is no option to do anything else, so there is\n> nothing to warn the user about in terms of tradeoffs.\n>\n> I agree that in general git's handling of corruption could be documented\n> somewhere, but I'm not sure where.\n\nI think either a section in git-fsck.txt or git.txt. Probably the\nformer as people who read it are probably more concerned about\ncorruption.\n-- \nDuy\n"},{"id":"212286","messageId":"CAOx6V3ZWB1ZpmXcaBeSaPOvHqmAMF3U1rTXuwinFGmEZQwFGYQ@mail.gmail.com","threadId":"33274","inReplyTo":"20130325200752.GB3902@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Jeff Mitchell","fromEmail":"jeffrey.mitchell@gmail.com","sentAt":"2013-03-26T13:43:01Z","receivedAt":"2013-03-26T13:43:01Z","isPatch":false,"sender":{"key":"jeffrey.mitchell@gmail.com","avatar":null},"body":"On Mon, Mar 25, 2013 at 4:07 PM, Jeff King wrote:\n> On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote:\n>> For commit corruptions, the --no-hardlinks, non --mirror case refused\n>> to create the new repository and exited with an error code of 128. The\n>> --no-hardlinks, --mirror case spewed errors to the console, yet\n>> *still* created the new clone *and* returned an error code of zero.\n>\n> I wasn't able to reproduce this; can you post a succint test case?\n\nThis actually seems hard to reproduce. I found this during testing\nwith an existing repository on-disk, but when I tried creating a new\nrepository with some commit objects, and modifying one of the commit\nobjects the same way I modified the an object in the previous\nrepository, I was unable to reproduce it.\n\nI do have the original repository though, so I'll tar.gz it up so that\nyou can have exactly the same content as I do. It's about 40MB and you\ncan grab it here:\nhttps://www.dropbox.com/s/e8dhedmpd1a1axs/tomahawk-corrupt.tar.gz (MD5\nsum: cde8a43233db5d649932407891f8366b).\n\nOnce you extract that, you should be able to run a clone using paths\n(not file://) with --no-hardlinks --mirror and replicate the behavior\nI saw. FYI, I'm on Git 1.8.2.\n\nThanks,\nJeff\n"},{"id":"212296","messageId":"20130326165553.GA7282@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CAOx6V3ZWB1ZpmXcaBeSaPOvHqmAMF3U1rTXuwinFGmEZQwFGYQ@mail.gmail.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T16:55:53Z","receivedAt":"2013-03-26T16:55:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 09:43:01AM -0400, Jeff Mitchell wrote:\n\n> On Mon, Mar 25, 2013 at 4:07 PM, Jeff King wrote:\n> > On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote:\n> >> For commit corruptions, the --no-hardlinks, non --mirror case refused\n> >> to create the new repository and exited with an error code of 128. The\n> >> --no-hardlinks, --mirror case spewed errors to the console, yet\n> >> *still* created the new clone *and* returned an error code of zero.\n> >\n> > I wasn't able to reproduce this; can you post a succint test case?\n>\n> [...link to tar.gz...]\n> Once you extract that, you should be able to run a clone using paths\n> (not file://) with --no-hardlinks --mirror and replicate the behavior\n> I saw. FYI, I'm on Git 1.8.2.\n\nThanks for providing an example.\n\nThe difference is the same \"--mirror implies --bare\" issue; the non-bare\ncase dies during the checkout (even before my patches, as the corruption\nis not in a blob, but rather in the HEAD commit object itself). You can\nreplace --mirror with --bare and see the same behavior.\n\nThe troubling part is that we see errors in the bare case, but do not\ndie. Those errors all come from upload-pack, the \"sending\" side of a\nclone/fetch. Even though we do not transfer the objects via the git\nprotocol, we still invoke upload-pack to get the ref list (and then copy\nthe objects themselves out-of-band).\n\nWhat happens is that upload-pack sees the errors while trying to see if\nthe object is a tag that can be peeled (the server advertises both tags\nand the objects they point to). It does not distinguish between \"errors\ndid not let me peel this object\" and \"this object is not a tag, and\ntherefore there is nothing to peel\".\n\nWe could change that, but I'm not sure whether it is a good idea. I\nthink the intent is that upload-pack's ref advertisement would remain\nresilient to corruption in the repository (e.g., even if that commit is\ncorrupt, you can still fetch the rest of the data). We should not worry\nabout advertising broken objects, because we will encounter the same\nerror when we actually do try to send the objects. Dying at the\nadvertisement phase would be premature, since we do not yet know what\nthe client will request.\n\nThe problem, of course, is that the --local optimization _skips_ the\npart where we actually ask upload-pack for data, and instead blindly\ncopies it. So this is the same issue as usual, which is that the local\ntransport is not thorough enough to catch corruption. It seems like a\nfailing in this case, because upload-pack does notice the problem, but\nthat is only luck; if the corruption were in a non-tip object, it would\nnot notice it at all. So trying to die on errors in the ref\nadvertisement would just be a band-aid. Fundamentally the problem is\nthat the --local transport is not safe from propagating corruption, and\nshould not be used if that's a requirement.\n\n-Peff\n"},{"id":"212337","messageId":"7v38vhua14.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"20130325201650.GA16019@sigill.intra.peff.net","subject":"Re: [PATCH 1/9] stream_blob_to_fd: detect errors reading from stream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T21:27:19Z","receivedAt":"2013-03-26T21:27:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We call read_istream, but never check its return value for\n> errors. This can lead to us looping infinitely, as we just\n> keep trying to write \"-1\" bytes (and we do not notice the\n> error, as we simply check that write_in_full reports the\n> same number of bytes we fed it, which of course is also -1).\n\nLooks sane. Thanks.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> No test yet, as my method for triggering this causes _another_ infinite\n> loop. So the test comes after the fixes, to avoid infinite loops when\n> bisecting the history later. :)\n>\n> streaming.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/streaming.c b/streaming.c\n> index 4d978e5..f4126a7 100644\n> --- a/streaming.c\n> +++ b/streaming.c\n> @@ -514,6 +514,8 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n> \t\tssize_t wrote, holeto;\n> \t\tssize_t readlen = read_istream(st, buf, sizeof(buf));\n> \n> +\t\tif (readlen < 0)\n> +\t\t\tgoto close_and_exit;\n> \t\tif (!readlen)\n> \t\t\tbreak;\n> \t\tif (can_seek && sizeof(buf) == readlen) {\n"},{"id":"212341","messageId":"7vy5d9suye.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"20130325214936.GA22419@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/9] streaming_write_entry: propagate streaming errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T21:38:17Z","receivedAt":"2013-03-26T21:38:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: [PATCH] streaming_write_entry: propagate streaming errors\n>\n> When we are streaming an index blob to disk, we store the\n> error from stream_blob_to_fd in the \"result\" variable, and\n> then immediately overwrite that with the return value of\n> \"close\". That means we catch errors on close (e.g., problems\n> committing the file to disk), but miss anything which\n> happened before then.\n>\n> We can fix this by using bitwise-OR to accumulate errors in\n> our result variable.\n>\n> While we're here, we can also simplify the error handling\n> with an early return, which makes it easier to see under\n> which circumstances we need to clean up.\n>\n> Signed-off-by: Jeff King \n\nVery sensible. Thanks.\n"},{"id":"212342","messageId":"7vtxnxsuty.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"20130325202359.GH16019@sigill.intra.peff.net","subject":"Re: [PATCH 8/9] clone: die on errors from unpack_trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T21:40:57Z","receivedAt":"2013-03-26T21:40:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When clone is populating the working tree, it ignores the\n> return status from unpack_trees; this means we may report a\n> successful clone, even when the checkout fails.\n>\n> When checkout fails, we may want to leave the $GIT_DIR in\n> place, as it might be possible to recover the data through\n> further use of \"git checkout\" (e.g., if the checkout failed\n> due to a transient error, disk full, etc). However, we\n> already die on a number of other checkout-related errors, so\n> this patch follows that pattern.\n>\n> In addition to marking a now-passing test, we need to adjust\n> t5710, which blindly assumed it could make bogus clones of\n> very deep alternates hierarchies. By using \"--bare\", we can\n> avoid it actually touching any objects.\n>\n> Signed-off-by: Jeff King \n> ---\n\nThanks.\n\n> I think the \"leave the data behind\" fix may be to just set \"junk_pid =\n> 0\" a little sooner in cmd_clone (i.e., before checkout()). Then we\n> would still die, but at least leave the fetched objects intact.\n\nYeah, perhaps, but I agree that is a much lower priority change.\n"},{"id":"212344","messageId":"7vppylsuei.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"20130325202627.GI16019@sigill.intra.peff.net","subject":"Re: [PATCH 9/9] clone: run check_everything_connected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-26T21:50:13Z","receivedAt":"2013-03-26T21:50:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The slowdown is really quite terrible if you try \"git clone --bare\n> linux-2.6.git\". Even with this, the local-clone case already misses blob\n> corruption. So it probably makes sense to restrict it to just the\n> non-local clone case, which already has to do more work.\n\nProbably. We may want to enable fsck even for local clones in the\nlonger term and also have this check. Those who know their filesystem\nis trustworthy can do the filesystem-level copy with \"cp -R\" themselves\nafter all.\n\n> Even still, it adds a non-trivial amount of work (linux-2.6 takes\n> something like a minute to check). I don't like the idea of declaring\n> \"git clone\" non-safe unless you turn on transfer.fsckObjects, though. It\n> should have the same safety as \"git fetch\".\n\nTrue.\n"},{"id":"212345","messageId":"102DBFFC4475445D9180A9C7D2A9D97C@PhilipOakley","threadId":"33274","inReplyTo":"20130326165553.GA7282@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2013-03-26T21:50:13Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \nSent: Tuesday, March 26, 2013 4:55 PM\n> On Tue, Mar 26, 2013 at 09:43:01AM -0400, Jeff Mitchell wrote:\n>\n>> On Mon, Mar 25, 2013 at 4:07 PM, Jeff King wrote:\n>> > On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote:\n>> >> For commit corruptions, the --no-hardlinks, non --mirror case\n>> >> refused\n>> >> to create the new repository and exited with an error code of 128.\n>> >> The\n>> >> --no-hardlinks, --mirror case spewed errors to the console, yet\n>> >> *still* created the new clone *and* returned an error code of\n>> >> zero.\n>> >\n>> > I wasn't able to reproduce this; can you post a succint test case?\n>>\n>> [...link to tar.gz...]\n>> Once you extract that, you should be able to run a clone using paths\n>> (not file://) with --no-hardlinks --mirror and replicate the behavior\n>> I saw. FYI, I'm on Git 1.8.2.\n>\n> Thanks for providing an example.\n>\n> The difference is the same \"--mirror implies --bare\" issue; the\n> non-bare\n> case dies during the checkout (even before my patches, as the\n> corruption\n> is not in a blob, but rather in the HEAD commit object itself). You\n> can\n> replace --mirror with --bare and see the same behavior.\n>\n> The troubling part is that we see errors in the bare case, but do not\n> die. Those errors all come from upload-pack, the \"sending\" side of a\n> clone/fetch. Even though we do not transfer the objects via the git\n> protocol, we still invoke upload-pack to get the ref list (and then\n> copy\n> the objects themselves out-of-band).\n>\n> What happens is that upload-pack sees the errors while trying to see\n> if\n> the object is a tag that can be peeled (the server advertises both\n> tags\n> and the objects they point to). It does not distinguish between\n> \"errors\n> did not let me peel this object\" and \"this object is not a tag, and\n> therefore there is nothing to peel\".\n>\n> We could change that, but I'm not sure whether it is a good idea. I\n> think the intent is that upload-pack's ref advertisement would remain\n> resilient to corruption in the repository (e.g., even if that commit\n> is\n> corrupt, you can still fetch the rest of the data). We should not\n> worry\n> about advertising broken objects, because we will encounter the same\n> error when we actually do try to send the objects. Dying at the\n> advertisement phase would be premature, since we do not yet know what\n> the client will request.\n>\n> The problem, of course, is that the --local optimization _skips_ the\n> part where we actually ask upload-pack for data, and instead blindly\n> copies it. So this is the same issue as usual, which is that the local\n> transport is not thorough enough to catch corruption. It seems like a\n> failing in this case, because upload-pack does notice the problem, but\n> that is only luck; if the corruption were in a non-tip object, it\n> would\n> not notice it at all. So trying to die on errors in the ref\n> advertisement would just be a band-aid. Fundamentally the problem is\n> that the --local transport is not safe from propagating corruption,\n> and\n> should not be used if that's a requirement.\n>\n> -Peff\n> --\n\nWhich way does `git bundle file.bundl --all` perform after the changes\nfor both the 'transport' checking and being reliable during updates.\n\nIs it an option for creating an archivable file that can be used for a\nlater `clone`?\n\nI wasn't sure if the bundle capability had been considered.\n\nPhilip\n"},{"id":"212346","messageId":"20130326220302.GA8880@sigill.intra.peff.net","threadId":"33274","inReplyTo":"102DBFFC4475445D9180A9C7D2A9D97C@PhilipOakley","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T22:03:02Z","receivedAt":"2013-03-26T22:03:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 09:59:42PM -0000, Philip Oakley wrote:\n\n> Which way does `git bundle file.bundl --all` perform after the changes\n> for both the 'transport' checking and being reliable during updates.\n\nBundles are treated at a fairly low level the same as a remote who\nprovides us a particular set of refs and a packfile. So we should get\nthe same protections via index-pack, and still run\ncheck_everything_connected on it, just as we would with a fetch over the\ngit protocol.\n\nI didn't test it, though.\n\n-Peff\n"},{"id":"212350","messageId":"20130326222209.GA16457@sigill.intra.peff.net","threadId":"33274","inReplyTo":"7vtxnxsuty.fsf@alter.siamese.dyndns.org","subject":"[PATCH 10/9] clone: leave repo in place after checkout errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T22:22:09Z","receivedAt":"2013-03-26T22:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 02:40:57PM -0700, Junio C Hamano wrote:\n\n> > I think the \"leave the data behind\" fix may be to just set \"junk_pid =\n> > 0\" a little sooner in cmd_clone (i.e., before checkout()). Then we\n> > would still die, but at least leave the fetched objects intact.\n> \n> Yeah, perhaps, but I agree that is a much lower priority change.\n\nAs it turns out, the checkout() error path sometimes _already_ leaves\nthe repository intact, but it's due to a bug. And it ends up deleting\nsomething random instead. :)\n\nI agree it's not a high priority, but I think it makes sense while we're\nin the area. And while it's very unlikely that the deletion would be\ndisastrous (see below), it makes me nervous. Patch is below.\n\n-- >8 --\nSubject: [PATCH] clone: leave repo in place after checkout errors\n\nIf we manage to clone a remote repository but run into an\nerror in the checkout, it is probably sane to leave the repo\ndirectory in place. That lets the user examine the situation\nwithout spending time to re-clone from the remote (which may\nbe a lengthy process).\n\nRather than try to convert each die() from the checkout code\npath into an error(), we simply set a flag that tells the\n\"remove_junk\" atexit function to print a helpful message and\nleave the repo in place.\n\nNote that the test added in this patch actually passes\nwithout the code change. The reason is that the cleanup code\nis buggy; we chdir into the working tree for the checkout,\nbut still may use relative paths to remove the directories\n(which means if you cloned into \"foo\", we would accidentally\nremove \"foo\" from the working tree!). There's no point in\nfixing it now, since this patch means we will never try to\nremove anything after the chdir, anyway.\n\nSigned-off-by: Jeff King \n---\nI think the accidental deletion could also escape the repository if you\ndid something like:\n\n git clone $remote ../../foo\n\nwhich would delete ../../foo/../../foo, or ../../../foo, which is not\nrelated to what you just cloned. But I didn't test, and we don't have to\ncare anymore after this patch.\n\n builtin/clone.c | 33 ++++++++++++++++++++++++++++++++-\n t/t1060-object-corruption.sh | 4 ++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex eceaa74..e145dfc 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -377,10 +377,40 @@ static void remove_junk(void)\n static const char *junk_work_tree;\n static const char *junk_git_dir;\n static pid_t junk_pid;\n+enum {\n+\tJUNK_LEAVE_NONE,\n+\tJUNK_LEAVE_REPO,\n+\tJUNK_LEAVE_ALL\n+} junk_mode = JUNK_LEAVE_NONE;\n+\n+static const char junk_leave_repo_msg[] =\n+N_(\"The remote repository was cloned successfully, but there was\\n\"\n+ \"an error checking out the HEAD branch. The repository has been left in\\n\"\n+ \"place but the working tree may be in an inconsistent state. You can\\n\"\n+ \"can inspect the contents with:\\n\"\n+ \"\\n\"\n+ \" git status\\n\"\n+ \"\\n\"\n+ \"and retry the checkout with\\n\"\n+ \"\\n\"\n+ \" git checkout -f HEAD\\n\"\n+ \"\\n\");\n \n static void remove_junk(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\n+\tswitch (junk_mode) {\n+\tcase JUNK_LEAVE_REPO:\n+\t\twarning(\"%s\", _(junk_leave_repo_msg));\n+\t\t/* fall-through */\n+\tcase JUNK_LEAVE_ALL:\n+\t\treturn;\n+\tdefault:\n+\t\t/* proceed to removal */\n+\t\tbreak;\n+\t}\n+\n \tif (getpid() != junk_pid)\n \t\treturn;\n \tif (junk_git_dir) {\n@@ -925,12 +955,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \ttransport_unlock_pack(transport);\n \ttransport_disconnect(transport);\n \n+\tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout();\n \n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n \tstrbuf_release(&value);\n-\tjunk_pid = 0;\n+\tjunk_mode = JUNK_LEAVE_ALL;\n \treturn err;\n }\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex a405b70..a84deb1 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -89,6 +89,10 @@ test_expect_success 'clone --local detects corruption' '\n \ttest_must_fail git clone --local bit-error corrupt-checkout\n '\n \n+test_expect_success 'error detected during checkout leaves repo intact' '\n+\ttest_path_is_dir corrupt-checkout/.git\n+'\n+\n test_expect_success 'clone --local detects missing objects' '\n \ttest_must_fail git clone --local missing missing-checkout\n '\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212351","messageId":"20130326222431.GB16457@sigill.intra.peff.net","threadId":"33274","inReplyTo":"CACsJy8CbBeuHmkEJs4FqGJs_kqEcjKi7RJkp9eNorxJAqgiCrg@mail.gmail.com","subject":"Re: [PATCH 9/9] clone: run check_everything_connected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T22:24:31Z","receivedAt":"2013-03-26T22:24:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 07:53:42AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Tue, Mar 26, 2013 at 3:26 AM, Jeff King wrote:\n> > static void update_remote_refs(const struct ref *refs,\n> > const struct ref *mapped_refs,\n> > const struct ref *remote_head_points_at,\n> > const char *branch_top,\n> > const char *msg)\n> > {\n> > + const struct ref *rm = mapped_refs;\n> > +\n> > + if (check_everything_connected(iterate_ref_map, 0, &rm))\n> > + die(_(\"remote did not send all necessary objects\"));\n> > +\n> > if (refs) {\n> > write_remote_refs(mapped_refs);\n> > if (option_single_branch)\n> \n> Maybe move this after checkout, so that I can switch terminal and\n> start working while it's verifying? And maybe an option not to\n> check_everything_connected, instead print a big fat warning telling\n> the user to fsck later?\n\nI tried to follow the fetch process of not installing the refs until we\nhad verified that the objects were reasonable. It probably doesn't\nmatter that much for clone, since you would not have simultaneous users\nexpecting the repository to be in a reasonable state until after clone\ncompletes, though.\n\nWe also would have to tweak check_everything_connected, which does\nsomething like \"--not --all\" to avoid rechecking objects we already\nhave. But that is not too hard to do.\n\n-Peff\n"},{"id":"212352","messageId":"20130326223259.GA28148@google.com","threadId":"33274","inReplyTo":"20130326222209.GA16457@sigill.intra.peff.net","subject":"Re: [PATCH 10/9] clone: leave repo in place after checkout errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-26T22:32:59Z","receivedAt":"2013-03-26T22:32:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -377,10 +377,40 @@ static void remove_junk(void)\n> static const char *junk_work_tree;\n> static const char *junk_git_dir;\n> static pid_t junk_pid;\n> +enum {\n> +\tJUNK_LEAVE_NONE,\n> +\tJUNK_LEAVE_REPO,\n> +\tJUNK_LEAVE_ALL\n> +} junk_mode = JUNK_LEAVE_NONE;\n\nNeat.\n\n> +\n> +static const char junk_leave_repo_msg[] =\n> +N_(\"The remote repository was cloned successfully, but there was\\n\"\n> + \"an error checking out the HEAD branch. The repository has been left in\\n\"\n> + \"place but the working tree may be in an inconsistent state. You can\\n\"\n> + \"can inspect the contents with:\\n\"\n> + \"\\n\"\n> + \" git status\\n\"\n> + \"\\n\"\n> + \"and retry the checkout with\\n\"\n> + \"\\n\"\n> + \" git checkout -f HEAD\\n\"\n> + \"\\n\");\n\nCan this be made more precise? I don't know what it means for the\nworking tree to be in an inconsistent state: do you mean that some files\nmight be partially checked out or not have been checked out at all yet?\n\n\terror: Clone succeeded, but checkout failed.\n\thint: You can inspect what was checked out with \"git status\".\n\thint: To retry the checkout, run \"git checkout -f HEAD\".\n\nAside from that, this looks very nice.\n\nThanks,\nJonathan\n"},{"id":"212355","messageId":"1364340037755-7580771.post@n2.nabble.com","threadId":"33274","inReplyTo":"20130326165553.GA7282@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Rich Fromm","fromEmail":"richard_fromm@yahoo.com","sentAt":"2013-03-26T23:20:37Z","receivedAt":"2013-03-26T23:20:37Z","isPatch":false,"sender":{"key":"richard_fromm@yahoo.com","avatar":null},"body":"Jeff King wrote\n> Fundamentally the problem is\n> that the --local transport is not safe from propagating corruption, and\n> should not be used if that's a requirement.\n\nI've read Jeff Mitchell's blog post, his update, relevant parts of the\ngit-clone(1) man page, and a decent chunk of this thread, and I'm still not\nclear on one thing. Is the danger of `git clone --mirror` propagating\ncorruption only true when using the --local option ?\n\nSpecifically, in my case, I'm using `git clone --mirror`, but I'm *not*\nusing --local, nor am I using --no-hardlinks. The host executing the clone\ncommand is different than the the host on which the remote repository lives,\nand I am using ssh as a transport protocol. If there is corruption, can I\nor can I not expect the clone operation to fail and return a non-zero exit\nvalue? If I can not expect this, is the workaround to run `git fsck` on the\nresulting clone?\n\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/propagating-repo-corruption-across-clone-tp7580504p7580771.html\nSent from the git mailing list archive at Nabble.com.\n"},{"id":"212356","messageId":"20130327010348.GA18405@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130326223259.GA28148@google.com","subject":"Re: [PATCH 10/9] clone: leave repo in place after checkout errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-27T01:03:48Z","receivedAt":"2013-03-27T01:03:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 03:32:59PM -0700, Jonathan Nieder wrote:\n\n> > +static const char junk_leave_repo_msg[] =\n> > +N_(\"The remote repository was cloned successfully, but there was\\n\"\n> > + \"an error checking out the HEAD branch. The repository has been left in\\n\"\n> > + \"place but the working tree may be in an inconsistent state. You can\\n\"\n> > + \"can inspect the contents with:\\n\"\n> > + \"\\n\"\n> > + \" git status\\n\"\n> > + \"\\n\"\n> > + \"and retry the checkout with\\n\"\n> > + \"\\n\"\n> > + \" git checkout -f HEAD\\n\"\n> > + \"\\n\");\n> \n> Can this be made more precise? I don't know what it means for the\n> working tree to be in an inconsistent state: do you mean that some files\n> might be partially checked out or not have been checked out at all yet?\n\nIt means that we died during the checkout procedure, and we don't have\nany idea what was left. Maybe something, maybe nothing. Maybe an index,\nmaybe not.\n\n> \terror: Clone succeeded, but checkout failed.\n> \thint: You can inspect what was checked out with \"git status\".\n> \thint: To retry the checkout, run \"git checkout -f HEAD\".\n\nThat is certainly more succint, if not more precise. I'd be fine with\nit.\n\n-Peff\n"},{"id":"212359","messageId":"20130327012515.GC28148@google.com","threadId":"33274","inReplyTo":"1364340037755-7580771.post@n2.nabble.com","subject":"Re: propagating repo corruption across clone","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-27T01:25:15Z","receivedAt":"2013-03-27T01:25:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRich Fromm wrote:\n\n> The host executing the clone\n> command is different than the the host on which the remote repository lives,\n> and I am using ssh as a transport protocol. If there is corruption, can I\n> or can I not expect the clone operation to fail and return a non-zero exit\n> value? If I can not expect this, is the workaround to run `git fsck` on the\n> resulting clone?\n\nIs the \"[transfer] fsckObjects\" configuration on the host executing the\nclone set to true?\n"},{"id":"212363","messageId":"7vr4j1qzao.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"1364340037755-7580771.post@n2.nabble.com","subject":"Re: propagating repo corruption across clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T03:47:27Z","receivedAt":"2013-03-27T03:47:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rich Fromm writes:\n\n> Jeff King wrote\n>> Fundamentally the problem is\n>> that the --local transport is not safe from propagating corruption, and\n>> should not be used if that's a requirement.\n>\n> I've read Jeff Mitchell's blog post, his update, relevant parts of the\n> git-clone(1) man page, and a decent chunk of this thread, and I'm still not\n> clear on one thing. Is the danger of `git clone --mirror` propagating\n> corruption only true when using the --local option ?\n\nIf you use --local, that is equivalent to \"cp -R\". Your corruption\nin the source will faithfully be byte-for-byte copied to the\ndestination. If you do not (and in your case you have two different\nmachines), unless you are using the long deprecated rsync transport\n(which again is the same as \"cp -R\"), transport layer will notice\nobject corruption. See Jeff's analysis earlier in the thread.\n\nIf you are lucky (or unlucky, depending on how you look at it), the\ncorruption you have in your object store may affect objects that are\nneeded to check out the version at the tip of the history, and \"git\ncheckout\" that happens as the last step of cloning may loudly\ncomplain, but that just means you can immediately notice the\nbreakage in that case. You may be unlucky and the corruption may\nnot affect objects that are needed to check out the tip. The initial\ncheckout will succeed as if nothing is wrong, but the corruption in\nyour object store is still there nevertheless. \"git log -p --all\"\nor \"git fsck\" will certainly be unhappy.\n\nThe difference between --mirror and no --mirror is a red herring.\nYou may want to ask Jeff Mitchell to remove the mention of it; it\nonly adds to the confusion without helping users. If you made\nbyte-for-byte copy of corrupt repository, it wouldn't make any\ndifference if the first \"checkout\" notices it.\n\nTo be paranoid, you may want to set transfer.fsckObjects to true,\nperhaps in your ~/.gitconfig.\n"},{"id":"212368","messageId":"CAMK1S_jZcoA9sy+ixXmy8uj2E9E4Q6W2pLQVFStZMgH9eRoo6g@mail.gmail.com","threadId":"33274","inReplyTo":"7vr4j1qzao.fsf@alter.siamese.dyndns.org","subject":"Re: propagating repo corruption across clone","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2013-03-27T06:19:29Z","receivedAt":"2013-03-27T06:19:29Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Wed, Mar 27, 2013 at 9:17 AM, Junio C Hamano wrote:\n\n> To be paranoid, you may want to set transfer.fsckObjects to true,\n> perhaps in your ~/.gitconfig.\n\ndo we have any numbers on the overhead of this?\n\nEven a \"guesstimate\" will do...\n"},{"id":"212380","messageId":"7v1ub0rijl.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"CAMK1S_jZcoA9sy+ixXmy8uj2E9E4Q6W2pLQVFStZMgH9eRoo6g@mail.gmail.com","subject":"Re: propagating repo corruption across clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T15:03:58Z","receivedAt":"2013-03-27T15:03:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sitaram Chamarty writes:\n\n> On Wed, Mar 27, 2013 at 9:17 AM, Junio C Hamano wrote:\n>\n>> To be paranoid, you may want to set transfer.fsckObjects to true,\n>> perhaps in your ~/.gitconfig.\n>\n> do we have any numbers on the overhead of this?\n>\n> Even a \"guesstimate\" will do...\n\nOn a reasonably slow machine:\n\n$ cd /project/git/git.git && git repack -a -d\n$ ls -hl .git/objects/pack/*.pack\n-r--r--r-- 1 junio src 44M Mar 26 13:18 .git/objects/pack/pack-c40635e5ee2b7094eb0e2c416e921a2b129bd8d2.pack\n\n$ cd .. && git --bare init junk && cd junk\n$ time git index-pack --strict --stdin <../git.git/.git/objects/pack/*.pack\nreal 0m13.873s\nuser 0m21.345s\nsys 0m2.248s\n\nThat's about 3.2 Mbps?\n\nCompare that with the speed your other side feeds you (or your line\nspeed could be the limiting factor) and decide how much you value\nyour data.\n"},{"id":"212386","messageId":"CAMK1S_jH-OJnH=XeCnEKvY6TkjZHZg_DLJ3KaVHNi9k0WA1REA@mail.gmail.com","threadId":"33274","inReplyTo":"7v1ub0rijl.fsf@alter.siamese.dyndns.org","subject":"Re: propagating repo corruption across clone","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2013-03-27T15:47:38Z","receivedAt":"2013-03-27T15:47:38Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Wed, Mar 27, 2013 at 8:33 PM, Junio C Hamano wrote:\n> Sitaram Chamarty writes:\n>\n>> On Wed, Mar 27, 2013 at 9:17 AM, Junio C Hamano wrote:\n>>\n>>> To be paranoid, you may want to set transfer.fsckObjects to true,\n>>> perhaps in your ~/.gitconfig.\n>>\n>> do we have any numbers on the overhead of this?\n>>\n>> Even a \"guesstimate\" will do...\n>\n> On a reasonably slow machine:\n>\n> $ cd /project/git/git.git && git repack -a -d\n> $ ls -hl .git/objects/pack/*.pack\n> -r--r--r-- 1 junio src 44M Mar 26 13:18 .git/objects/pack/pack-c40635e5ee2b7094eb0e2c416e921a2b129bd8d2.pack\n>\n> $ cd .. && git --bare init junk && cd junk\n> $ time git index-pack --strict --stdin <../git.git/.git/objects/pack/*.pack\n> real 0m13.873s\n> user 0m21.345s\n> sys 0m2.248s\n>\n> That's about 3.2 Mbps?\n>\n> Compare that with the speed your other side feeds you (or your line\n> speed could be the limiting factor) and decide how much you value\n> your data.\n\nThanks. I was also interested in overhead on the server just as a %-age.\n\nI have no idea why but when I did some tests a long time ago I got\nupwards of 40% or so, but now when I try these tests for git.git\n\n cd \n git init --bare\n # git config transfer.fsckobjects true\n git fetch file:///full/path/to/git.git refs/*:refs/*\n\nthen, the difference in elapsed time 18s -> 22s, so about 22%, and CPU\ntime is 31 -> 37, so about 20%. I didn't measure disk access\nincreases, but I guess 20% is not too bad.\n\nIs it likely to be linear in the size of the repo, by and large?\n"},{"id":"212423","messageId":"1364408595621-7580839.post@n2.nabble.com","threadId":"33274","inReplyTo":"20130327012515.GC28148@google.com","subject":"Re: propagating repo corruption across clone","fromName":"Rich Fromm","fromEmail":"richard_fromm@yahoo.com","sentAt":"2013-03-27T18:23:15Z","receivedAt":"2013-03-27T18:23:15Z","isPatch":false,"sender":{"key":"richard_fromm@yahoo.com","avatar":null},"body":"Jonathan Nieder-2 wrote\n> Is the \"[transfer] fsckObjects\" configuration on the host executing the\n> clone set to true?\n\nI hadn't been setting it at all, and according to git-config(1) it defaults\nto false, so the answer is no. It looks like setting it might be a good\nidea.\n\nBut I'm still somewhat confused about what is and is not checked under what\nconditions. Consider the three statements:\n\n# 1\ngit clone --mirror myuser@myhost:my_repo\n\n# 2\ngit clone --mirror --config transfer.fsckObjects=true myuser@myhost:my_repo\n\n# 3\ngit clone --mirror myuser@myhost:my_repo && cd my_repo.git && git-fsck\n\nAre 2 and 3 equivalent? Or is there an increasing level of checking that\noccurs from 1 to 2, and from 2 to 3? My guess is the latter, but perhaps\nthis could be clearer in the man pages.\n\ngit-config(1) says that transfer.fsckObjects essentially (if fetch... and\nreceive... are not explicitly set) \"git-fetch-pack will check all fetched\nobjects\" and \"git-receive-pack will check all received objects.\" While\ngit-fsck(1) says \"git-fsck tests SHA1 and general object sanity, and it does\nfull tracking of the resulting reachability and everything else.\" The\nlatter sounds like a stronger statement to me. But if that's true, perhaps\nshould the relevant section(s) of git-config(1) explicitly note that this is\nnot equivalent to a full git-fsck ?\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/propagating-repo-corruption-across-clone-tp7580504p7580839.html\nSent from the git mailing list archive at Nabble.com.\n"},{"id":"212428","messageId":"1364410309241-7580845.post@n2.nabble.com","threadId":"33274","inReplyTo":"7vr4j1qzao.fsf@alter.siamese.dyndns.org","subject":"Re: propagating repo corruption across clone","fromName":"Rich Fromm","fromEmail":"richard_fromm@yahoo.com","sentAt":"2013-03-27T18:51:49Z","receivedAt":"2013-03-27T18:51:49Z","isPatch":false,"sender":{"key":"richard_fromm@yahoo.com","avatar":null},"body":"Junio C Hamano wrote\n> If you use --local, that is equivalent to \"cp -R\". Your corruption\n> in the source will faithfully be byte-for-byte copied to the\n> destination. If you do not\n> ...\n> transport layer will notice\n> object corruption.\n> ...\n> The difference between --mirror and no --mirror is a red herring.\n> You may want to ask Jeff Mitchell to remove the mention of it; it\n> only adds to the confusion without helping users.\n\nJust to clarify, I don't know Jeff Mitchell personally, and I'm not\naffiliated with the KDE project. I happened to have recently implemented a\nbackup strategy for a different codebase, that relies on `git clone\n--mirror` to take the actual snapshots of the live repos, and I read about\nJeff's experiences, and that's why I started following this discussion. \nApologies if my questions are considered slightly off topic -- I'm not\npositive if this is supposed to be a list for developers, and not users.\n\nNevertheless, I will try to contact Jeff and point him at this. My initial\nreading of his blog posts definitely gave me the impression that this was a\n--mirror vs. not issue, but it really sounds like his main problem was using\n--local.\n\nHowever, I think there may be room for some additional clarity in the docs. \nThe --local option in git-config(1) says \"When the repository to clone from\nis on a local machine, this flag bypasses the normal \"git aware\" transport\nmechanism\". But there's no mention of the consequences of this transport\nbypass. There's also no mention of this in the \"GIT URLS\" section that\ndiscusses transport protocols, and I also don't see anything noting it in\neither of these sections of the git book:\n\nhttp://git-scm.com/book/en/Git-on-the-Server-The-Protocols\nhttp://git-scm.com/book/en/Git-Internals-Transfer-Protocols\n\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/propagating-repo-corruption-across-clone-tp7580504p7580845.html\nSent from the git mailing list archive at Nabble.com.\n"},{"id":"212433","messageId":"7v7gksmza5.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"1364410309241-7580845.post@n2.nabble.com","subject":"Re: propagating repo corruption across clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T19:13:38Z","receivedAt":"2013-03-27T19:13:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rich Fromm writes:\n\n> Apologies if my questions are considered slightly off topic -- I'm not\n> positive if this is supposed to be a list for developers, and not users.\n\nThe list is both for users and developers.\n\n> However, I think there may be room for some additional clarity in the docs. \n> The --local option in git-config(1) says \"When the repository to clone from\n> is on a local machine, this flag bypasses the normal \"git aware\" transport\n> mechanism\". But there's no mention of the consequences of this transport\n> bypass.\n\nYeah, I would not mind a patch to update the documentation for\n\"clone --local\" and rsync transport to say something about\nbyte-for-byte copying of broken repository.\n\nThanks.\n"},{"id":"212439","messageId":"20130327194938.GB26380@sigill.intra.peff.net","threadId":"33274","inReplyTo":"1364408595621-7580839.post@n2.nabble.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-27T19:49:38Z","receivedAt":"2013-03-27T19:49:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 27, 2013 at 11:23:15AM -0700, Rich Fromm wrote:\n\n> But I'm still somewhat confused about what is and is not checked under what\n> conditions. Consider the three statements:\n> \n> # 1\n> git clone --mirror myuser@myhost:my_repo\n> \n> # 2\n> git clone --mirror --config transfer.fsckObjects=true myuser@myhost:my_repo\n> \n> # 3\n> git clone --mirror myuser@myhost:my_repo && cd my_repo.git && git-fsck\n> \n> Are 2 and 3 equivalent? Or is there an increasing level of checking that\n> occurs from 1 to 2, and from 2 to 3? My guess is the latter, but perhaps\n> this could be clearer in the man pages.\n\n2 and 3 are not exactly equivalent, in that they are implemented\nslightly differently, but I do not know offhand of any case that would\npass 2 but not 3. We do check reachability with transfer.fsckObjects.\n\n-Peff\n"},{"id":"212447","messageId":"20130327200406.GA5124@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130327194938.GB26380@sigill.intra.peff.net","subject":"Re: propagating repo corruption across clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-27T20:04:06Z","receivedAt":"2013-03-27T20:04:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 27, 2013 at 03:49:38PM -0400, Jeff King wrote:\n\n> On Wed, Mar 27, 2013 at 11:23:15AM -0700, Rich Fromm wrote:\n> \n> > But I'm still somewhat confused about what is and is not checked under what\n> > conditions. Consider the three statements:\n> > \n> > # 1\n> > git clone --mirror myuser@myhost:my_repo\n> > \n> > # 2\n> > git clone --mirror --config transfer.fsckObjects=true myuser@myhost:my_repo\n> > \n> > # 3\n> > git clone --mirror myuser@myhost:my_repo && cd my_repo.git && git-fsck\n> > \n> > Are 2 and 3 equivalent? Or is there an increasing level of checking that\n> > occurs from 1 to 2, and from 2 to 3? My guess is the latter, but perhaps\n> > this could be clearer in the man pages.\n> \n> 2 and 3 are not exactly equivalent, in that they are implemented\n> slightly differently, but I do not know offhand of any case that would\n> pass 2 but not 3. We do check reachability with transfer.fsckObjects.\n\nOh, and in the case of #1, I think we would already find corruption, in\nthat index-pack will expand and check the sha1 of each object we\nreceive. The transfer.fsckObjects check adds some semantic checks as\nwell (e.g., making sure author identities are well-formed).\n\nClone will not currently detect missing objects and reachability\nwithout transfer.fsckObjects set, but that is IMHO a bug; fetch will\nnotice it, and clone should behave the same way.\n\n-Peff\n"},{"id":"212450","messageId":"20130327202705.GA5145@sigill.intra.peff.net","threadId":"33274","inReplyTo":"20130325202134.GE16019@sigill.intra.peff.net","subject":"Re: [PATCH 5/9] add test for streaming corrupt blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-27T20:27:05Z","receivedAt":"2013-03-27T20:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 04:21:34PM -0400, Jeff King wrote:\n\n> +# Convert byte at offset \"$2\" of object \"$1\" into '\\0'\n> +corrupt_byte() {\n> +\tobj_file=$(obj_to_file \"$1\") &&\n> +\tchmod +w \"$obj_file\" &&\n> +\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n> +}\n\nHmm, this last line should probably be:\n\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex a84deb1..3f87051 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -12,7 +12,7 @@ corrupt_byte() {\n corrupt_byte() {\n \tobj_file=$(obj_to_file \"$1\") &&\n \tchmod +w \"$obj_file\" &&\n-\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n+\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\" conv=notrunc\n }\n \n test_expect_success 'setup corrupt repo' '\n\nThe intent was to change a single byte, not truncate the file (though on\nthe plus side, that truncation is what found the other bugs).\n\n-Peff\n"},{"id":"212452","messageId":"7vhajwlgy0.fsf@alter.siamese.dyndns.org","threadId":"33274","inReplyTo":"20130327202705.GA5145@sigill.intra.peff.net","subject":"Re: [PATCH 5/9] add test for streaming corrupt blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T20:35:03Z","receivedAt":"2013-03-27T20:35:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Mar 25, 2013 at 04:21:34PM -0400, Jeff King wrote:\n>\n>> +# Convert byte at offset \"$2\" of object \"$1\" into '\\0'\n>> +corrupt_byte() {\n>> +\tobj_file=$(obj_to_file \"$1\") &&\n>> +\tchmod +w \"$obj_file\" &&\n>> +\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n>> +}\n>\n> Hmm, this last line should probably be:\n>\n> diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\n> index a84deb1..3f87051 100755\n> --- a/t/t1060-object-corruption.sh\n> +++ b/t/t1060-object-corruption.sh\n> @@ -12,7 +12,7 @@ corrupt_byte() {\n> corrupt_byte() {\n> \tobj_file=$(obj_to_file \"$1\") &&\n> \tchmod +w \"$obj_file\" &&\n> -\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\"\n> +\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\" conv=notrunc\n> }\n> \n> test_expect_success 'setup corrupt repo' '\n>\n> The intent was to change a single byte, not truncate the file (though on\n> the plus side, that truncation is what found the other bugs).\n\n;-). Thanks, I missed that.\n"},{"id":"212486","messageId":"CACsJy8BMfYnFv=PL8x5JOMkjYc39h630oNEdukkjmBKBTNCibg@mail.gmail.com","threadId":"33274","inReplyTo":"20130325202627.GI16019@sigill.intra.peff.net","subject":"Re: [PATCH 9/9] clone: run check_everything_connected","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-28T00:40:51Z","receivedAt":"2013-03-28T00:40:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 26, 2013 at 3:26 AM, Jeff King wrote:\n> The slowdown is really quite terrible if you try \"git clone --bare\n> linux-2.6.git\". Even with this, the local-clone case already misses blob\n> corruption. So it probably makes sense to restrict it to just the\n> non-local clone case, which already has to do more work.\n>\n> Even still, it adds a non-trivial amount of work (linux-2.6 takes\n> something like a minute to check). I don't like the idea of declaring\n> \"git clone\" non-safe unless you turn on transfer.fsckObjects, though. It\n> should have the same safety as \"git fetch\".\n\nMaybe we could do it in index-pack to save some (wall) time. I haven't\ntried but I think it might work. The problem is to make sure the pack\ncontains objects for all sha1 references in the pack. By that\ndescription, we don't need to do standard DAG traversal. We could\nextract sha-1 references in index-pack as we uncompress objects and\nput all \"want\" sha-1 in a hash table. At the end of index-pack, we\ncheck if any sha-1 in the hash table still points to non-existing\nobject.\n\nThis way, at least we don't need to uncompress all objects again in\nrev-list. We could parse+hash in both phases in index-pack. The first\nphase (parse_pack_objects) is usually I/O bound, we could hide some\ncost there. The second phase is multithreaded, all the better.\n-- \nDuy\n"},{"id":"212515","messageId":"CAOx6V3YdKUm4gr+_6UwOKfdn1EdND6B06Ne2wdbC=202aCfifA@mail.gmail.com","threadId":"33274","inReplyTo":"7vr4j1qzao.fsf@alter.siamese.dyndns.org","subject":"Re: propagating repo corruption across clone","fromName":"Jeff Mitchell","fromEmail":"jeffrey.mitchell@gmail.com","sentAt":"2013-03-28T13:48:34Z","receivedAt":"2013-03-28T13:48:34Z","isPatch":false,"sender":{"key":"jeffrey.mitchell@gmail.com","avatar":null},"body":"On Tue, Mar 26, 2013 at 11:47 PM, Junio C Hamano wrote:\n> The difference between --mirror and no --mirror is a red herring.\n> You may want to ask Jeff Mitchell to remove the mention of it; it\n> only adds to the confusion without helping users. If you made\n> byte-for-byte copy of corrupt repository, it wouldn't make any\n> difference if the first \"checkout\" notices it.\n\nHi,\n\nSeveral days ago I had actually already updated the post to indicate\nthat my testing methodology was incorrect as a result of mixing up\n--no-hardlinks and --no-local, and pointed to this thread.\n\nI will say that we did see corrupted repos on the downstream mirrors.\nI don't have an explanation for it, but have not been able to\nreproduce it either. My only potential guess (untested) is that\nperhaps when the corruption was detected the clone aborted but left\nthe objects already transferred locally. Again, untested -- I mention\nit only because it's my only potential explanation :-)\n\n> To be paranoid, you may want to set transfer.fsckObjects to true,\n> perhaps in your ~/.gitconfig.\n\nInteresting; I'd known about receive.fsckObjects but not\ntransfer/fetch. Thanks for the pointer.\n"},{"id":"212517","messageId":"CAOx6V3Yx5CYGQXMY_LUy0mxpROZveYjitdAqY_hC4dRcMU9sXw@mail.gmail.com","threadId":"33274","inReplyTo":"1364410309241-7580845.post@n2.nabble.com","subject":"Re: propagating repo corruption across clone","fromName":"Jeff Mitchell","fromEmail":"jeffrey.mitchell@gmail.com","sentAt":"2013-03-28T13:52:47Z","receivedAt":"2013-03-28T13:52:47Z","isPatch":false,"sender":{"key":"jeffrey.mitchell@gmail.com","avatar":null},"body":"On Wed, Mar 27, 2013 at 2:51 PM, Rich Fromm wrote:\n> Nevertheless, I will try to contact Jeff and point him at this. My initial\n> reading of his blog posts definitely gave me the impression that this was a\n> --mirror vs. not issue, but it really sounds like his main problem was using\n> --local.\n\nActually, I wasn't using --local, I just wasn't using --no-local, as I\nmixed up that and --no-hardlinks (which lies somewhere between, but is\nstill not the same as using file://).\n\nIt's entirely possible that you read the posts before I updated them.\n\nAlso, keep in mind, if you're evaluating what you're doing, that the\nsystem we have was not set up to be a backup system. It was set up to\nbe a mirror system. With proper sanity checking, it would have acted\nin a decent capacity as a backup system, but that wasn't why it was\nset up, nor how it was set up.\n"},{"id":"212715","messageId":"20130331075712.GA13136@lanh","threadId":"33274","inReplyTo":"CACsJy8BMfYnFv=PL8x5JOMkjYc39h630oNEdukkjmBKBTNCibg@mail.gmail.com","subject":"Re: [PATCH 9/9] clone: run check_everything_connected","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-31T07:57:12Z","receivedAt":"2013-03-31T07:57:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 28, 2013 at 07:40:51AM +0700, Duy Nguyen wrote:\n> Maybe we could do it in index-pack to save some (wall) time. I haven't\n> tried but I think it might work. The problem is to make sure the pack\n> contains objects for all sha1 references in the pack. By that\n> description, we don't need to do standard DAG traversal. We could\n> extract sha-1 references in index-pack as we uncompress objects and\n> put all \"want\" sha-1 in a hash table. At the end of index-pack, we\n> check if any sha-1 in the hash table still points to non-existing\n> object.\n> \n> This way, at least we don't need to uncompress all objects again in\n> rev-list. We could parse+hash in both phases in index-pack. The first\n> phase (parse_pack_objects) is usually I/O bound, we could hide some\n> cost there. The second phase is multithreaded, all the better.\n\nIt looks like what I describe above is exactly what index-pack\n--strict does. Except that it holds the lock longer and has more\nabstraction layers to slow things down. On linux-2.6 with 3 threads:\n\n$ rev-list --all --objects --quiet (aka check_everything_connected)\n34.26user 0.22system 0:34.56elapsed 99%CPU (0avgtext+0avgdata 2550528maxresident)k\n0inputs+0outputs (0major+208569minor)pagefaults 0swaps\n\n$ index-pack --stdin\n214.57user 8.38system 1:31.82elapsed 242%CPU (0avgtext+0avgdata 1357328maxresident)k\n8inputs+1421016outputs (0major+1222537minor)pagefaults 0swaps\n\n$ index-pack --stdin --strict\n297.36user 13.77system 2:11.82elapsed 236%CPU (0avgtext+0avgdata 1875040maxresident)k\n0inputs+1421016outputs (0major+1308718minor)pagefaults 0swaps\n\n$ index-pack --stdin --connectivity\n231.09user 7.42system 1:37.39elapsed 244%CPU (0avgtext+0avgdata 2080816maxresident)k\n0inputs+1421016outputs (0major+540069minor)pagefaults 0swaps\n\nThe last one does not hold locks by duplicating object hash table per\nthread. As you can see the consumed memory is much higher than --stdin.\nIn return it only adds up 1/3 of rev-list time.\n\nMaybe you should check which one is cheaper for clone case,\ncheck_everything_connected() or index-pack --strict.\n--\nDuy\n"}]}