{"thread":{"id":"46099","subject":"[PATCH] perf: work around the tested repo having an index.lock","startedAt":"2017-06-02T10:33:47Z","lastAt":"2017-06-05T06:25:08Z","messageCount":16,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"321400","messageId":"20170602103330.25663-1-avarab@gmail.com","threadId":"46099","inReplyTo":null,"subject":"[PATCH] perf: work around the tested repo having an index.lock","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T10:33:30Z","receivedAt":"2017-06-02T10:33:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the tested repo has an index.lock file it should be removed. This\nfile may be present if e.g. git-status previously crashed in that\nrepo, and it will make a lot of git commands fail. Let's try harder\nand remove the lock.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/perf-lib.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b6fc880395..b50211b259 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -108,7 +108,14 @@ test_perf_create_repo_from () {\n \t\tcd \"$repo\" &&\n \t\t\"$MODERN_GIT\" init -q &&\n \t\ttest_perf_do_repo_symlink_config_ &&\n-\t\tmv .git/hooks .git/hooks-disabled 2>/dev/null\n+\t\tmv .git/hooks .git/hooks-disabled 2>/dev/null &&\n+\t\tif test -f .git/index.lock\n+\t\tthen\n+\t\t\t# We may be copying a repo that can't run \"git\n+\t\t\t# status\" due to a locked index. Since we have\n+\t\t\t# a copy it's fine to remove the lock.\n+\t\t\trm .git/index.lock\n+\t\tfi\n \t) || error \"failed to copy repository '$source' to '$repo'\"\n }\n \n-- \n2.13.0.506.g27d5fe0cd\n\n"},{"id":"321430","messageId":"20170602184506.x2inwswmcwafyvfy@sigill.intra.peff.net","threadId":"46099","inReplyTo":"20170602103330.25663-1-avarab@gmail.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-02T18:45:07Z","receivedAt":"2017-06-02T19:45:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 10:33:30AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> When the tested repo has an index.lock file it should be removed. This\n> file may be present if e.g. git-status previously crashed in that\n> repo, and it will make a lot of git commands fail. Let's try harder\n> and remove the lock.\n\nIf your git-status is crashing, you probably have bigger problems (and\nneed to clean up the original, too).\n\nBut I think a more compelling case is that there may be an ongoing\noperation in the original repo (e.g., say you are in the middle of\nwriting a commit message) when we do a blind copy of the filesystem\ncontents. You might racily pick up a lockfile.\n\nShould we find and delete all *.lock files in the copied directory? That\nwould get ref locks, etc. Half-formed object files are OK. Technically\nif you want to get an uncorrupted repository you'd also want to copy\nrefs before objects (in case somebody makes a new object and updates a\nref while you're copying).\n\nI don't know how careful it's worth being. I don't really _object_ to\nthis patch exactly, but it does seem like it's picking up one random\ncase (that presumably you hit) and ignoring all of the related cases.\n\n-Peff\n"},{"id":"321433","messageId":"CACBZZX61uqRsB8p+VdA=DZEt6Aa9bA+c4zTa5-ngmG8G4U3U4Q@mail.gmail.com","threadId":"46099","inReplyTo":"20170602184506.x2inwswmcwafyvfy@sigill.intra.peff.net","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T20:12:36Z","receivedAt":"2017-06-02T20:13:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jun 2, 2017 at 8:45 PM, Jeff King wrote:\n> On Fri, Jun 02, 2017 at 10:33:30AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> When the tested repo has an index.lock file it should be removed. This\n>> file may be present if e.g. git-status previously crashed in that\n>> repo, and it will make a lot of git commands fail. Let's try harder\n>> and remove the lock.\n>\n> If your git-status is crashing, you probably have bigger problems (and\n> need to clean up the original, too).\n>\n> But I think a more compelling case is that there may be an ongoing\n> operation in the original repo (e.g., say you are in the middle of\n> writing a commit message) when we do a blind copy of the filesystem\n> contents. You might racily pick up a lockfile.\n>\n> Should we find and delete all *.lock files in the copied directory? That\n> would get ref locks, etc. Half-formed object files are OK. Technically\n> if you want to get an uncorrupted repository you'd also want to copy\n> refs before objects (in case somebody makes a new object and updates a\n> ref while you're copying).\n>\n> I don't know how careful it's worth being. I don't really _object_ to\n> this patch exactly, but it does seem like it's picking up one random\n> case (that presumably you hit) and ignoring all of the related cases.\n\nIt's not a perfect solution, but it seemed not to cause any harm, and\nwould allow us to just do what you mean. I can't think of a case where\nwe'd care to preserve the index.lock in our perf copy, of course the\ntest may fail due to various other reasons, but at least it won't be\ndue to this reason.\n\nThis is just something I happened to run into locally because I had a\nstale index.lock, but FWIW at work I have a git updater running on\ntens of thousands of machines that has to handle various edge cases\n(e.g. the machine ran out of disk space in the middle of an update, or\nsomething was kill -9'd).\n\nThe leftover index.lock is quite common, the second most common \"index\nis hosed\" error is \"fatal: index file smaller than expected\", but\nsolving that is more invasive, you need to rm the index and reset\n--hard.\n"},{"id":"321462","messageId":"xmqqd1amx80f.fsf@gitster.mtv.corp.google.com","threadId":"46099","inReplyTo":"20170602184506.x2inwswmcwafyvfy@sigill.intra.peff.net","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-02T23:52:16Z","receivedAt":"2017-06-02T23:52:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I think a more compelling case is that there may be an ongoing\n> operation in the original repo (e.g., say you are in the middle of\n> writing a commit message) when we do a blind copy of the filesystem\n> contents. You might racily pick up a lockfile.\n>\n> Should we find and delete all *.lock files in the copied directory? That\n> would get ref locks, etc. Half-formed object files are OK. Technically\n> if you want to get an uncorrupted repository you'd also want to copy\n> refs before objects (in case somebody makes a new object and updates a\n> ref while you're copying).\n>\n> I don't know how careful it's worth being. I don't really _object_ to\n> this patch exactly, but it does seem like it's picking up one random\n> case (that presumably you hit) and ignoring all of the related cases.\n\nMy feeling exactly. Diagnosing and failing upfront saying \"well you\nmade a copy but it is not suitable for testing\" sounds more sensible\nat lesat to me.\n"},{"id":"321482","messageId":"CACBZZX52O9Pf=5Xtq1Lg1=ZU26tm7pupvubk1ZjNJZp7kR450g@mail.gmail.com","threadId":"46099","inReplyTo":"xmqqd1amx80f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-03T16:24:14Z","receivedAt":"2017-06-03T16:24:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Jun 3, 2017 at 1:52 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> But I think a more compelling case is that there may be an ongoing\n>> operation in the original repo (e.g., say you are in the middle of\n>> writing a commit message) when we do a blind copy of the filesystem\n>> contents. You might racily pick up a lockfile.\n>>\n>> Should we find and delete all *.lock files in the copied directory? That\n>> would get ref locks, etc. Half-formed object files are OK. Technically\n>> if you want to get an uncorrupted repository you'd also want to copy\n>> refs before objects (in case somebody makes a new object and updates a\n>> ref while you're copying).\n>>\n>> I don't know how careful it's worth being. I don't really _object_ to\n>> this patch exactly, but it does seem like it's picking up one random\n>> case (that presumably you hit) and ignoring all of the related cases.\n>\n> My feeling exactly. Diagnosing and failing upfront saying \"well you\n> made a copy but it is not suitable for testing\" sounds more sensible\n> at lesat to me.\n\nThis change makes the repo suitable for testing when it wasn't before.\n\nYes, there are cases where there are other issues than index.lock\npreventing testing the repo, but I don't see why there shouldn't be a\npartial solution that solves a very common case in lieu of a perfect\nsolution.\n"},{"id":"321491","messageId":"xmqqefv0wrj7.fsf@gitster.mtv.corp.google.com","threadId":"46099","inReplyTo":"CACBZZX52O9Pf=5Xtq1Lg1=ZU26tm7pupvubk1ZjNJZp7kR450g@mail.gmail.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-04T00:00:28Z","receivedAt":"2017-06-04T00:00:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> My feeling exactly. Diagnosing and failing upfront saying \"well you\n>> made a copy but it is not suitable for testing\" sounds more sensible\n>> at lesat to me.\n>\n> This change makes the repo suitable for testing when it wasn't before.\n\nPerhaps \"not suitable\" was a bit too vague.\n\nThe copy you made is not in a consistent state that is good for\ntesting. This change may declare that it is now in a consistent\nstate, but removal of a single *.lock file does not make it so. We\ndo not know what other transient inconsistency the resulting copy\nhas; it is inherent to git-unaware copy---that is why we discouraged\nand removed rsync transport after all.\n\n> Yes, there are cases where there are other issues than index.lock\n> preventing testing the repo, but I don't see why there shouldn't be a\n> partial solution that solves a very common case in lieu of a perfect\n> solution.\n\nAs long as the partial solution makes sure that the case it\naddressed was the only breakage, I'd be happy to see that it leaves\nother kinds of inconsistencies \"too rare to bother fixing\". I however\nfeel dirty if the punting is \"we won't even bother diagnosing and\nassume that *.lock is the only thing we care about\".\n\nPerhaps run \"fsck\" and \"status\" immediately after copying to make\nsure they succeed, or something like that?\n"},{"id":"321493","messageId":"xmqq60gcwp6x.fsf@gitster.mtv.corp.google.com","threadId":"46099","inReplyTo":"xmqqefv0wrj7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-04T00:51:02Z","receivedAt":"2017-06-04T00:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps run \"fsck\" and \"status\" immediately after copying to make\n> sure they succeed, or something like that?\n\nHmph, this is me half-being silly, as this \"copying an existing one\"\nis meant for testing Git with a large repository, and having to run\nfsck may add meaningful overhead in addition to the actual copying\nin the set-up procedure. I do not offhand think of a good solution.\n"},{"id":"321495","messageId":"xmqqwp8sv77h.fsf@gitster.mtv.corp.google.com","threadId":"46099","inReplyTo":"20170602184506.x2inwswmcwafyvfy@sigill.intra.peff.net","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-04T02:04:50Z","receivedAt":"2017-06-04T02:04:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I think a more compelling case is that there may be an ongoing\n> operation in the original repo (e.g., say you are in the middle of\n> writing a commit message) when we do a blind copy of the filesystem\n> contents. You might racily pick up a lockfile.\n>\n> Should we find and delete all *.lock files in the copied directory? That\n> would get ref locks, etc. Half-formed object files are OK. Technically\n> if you want to get an uncorrupted repository you'd also want to copy\n> refs before objects (in case somebody makes a new object and updates a\n> ref while you're copying).\n\nOr \"git branch -m A B\" is in progress.\n\nI think it all depends on what your \"threat\" model is ;-). Do we\nassume that many users are \"time-sharing\" a box and a repository?\nIf not, i.e. if you are the sole user of a box and a repository on\nit, such a concurrent access to make the result of git-unaware copy\nproblematic will not be in index.lock (after all you are now doing\nthe perf thing, not editing a commit log message in the repository\nused for testing Git), but will be in ref locks (somebody else\npushing into the repository you are *not* currently using from\nsideways).\n"},{"id":"321498","messageId":"CAP8UFD1u-C-T2X+H8WxxB7O+ajcGfygRBLN0t7+zJcOh_SrvFQ@mail.gmail.com","threadId":"46099","inReplyTo":"xmqqefv0wrj7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-06-04T07:37:29Z","receivedAt":"2017-06-04T07:37:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> My feeling exactly. Diagnosing and failing upfront saying \"well you\n>>> made a copy but it is not suitable for testing\" sounds more sensible\n>>> at lesat to me.\n>>\n>> This change makes the repo suitable for testing when it wasn't before.\n>\n> Perhaps \"not suitable\" was a bit too vague.\n>\n> The copy you made is not in a consistent state that is good for\n> testing. This change may declare that it is now in a consistent\n> state, but removal of a single *.lock file does not make it so. We\n> do not know what other transient inconsistency the resulting copy\n> has; it is inherent to git-unaware copy---that is why we discouraged\n> and removed rsync transport after all.\n\nIf we don't like git-unaware copies, maybe we should go back to the\nreasons why we are making one here.\nIn 342e9ef2d9 (Introduce a performance testing framework, 2012-02-17),\nThomas wrote:\n\n 3. Creating test repos from scratch in every test is extremely\n time-consuming, and shipping or downloading such large/weird repos\n is out of the question.\n\n We leave this decision to the user. Two different sizes of test\n repos can be configured, and the scripts just copy one or more of\n those (using hardlinks for the object store). By default it tries\n to use the build tree's git.git repository.\n\n This is fairly fast and versatile. Using a copy instead of a clone\n preserves many properties that the user may want to test for, such\n as lots of loose objects, unpacked refs, etc.\n\nIs a local clone really much slower these days? Wouldn't it is use\nhard links too?\nBy the way the many properties that are preserved might not be worth\npreserving as they could make results depend a lot on the current\nstate of the original repo.\n"},{"id":"321500","messageId":"CACBZZX4eCJJS6iaod+tWFF5M998Y5x88SdYR2ZekAJeeOiU8ug@mail.gmail.com","threadId":"46099","inReplyTo":"CAP8UFD1u-C-T2X+H8WxxB7O+ajcGfygRBLN0t7+zJcOh_SrvFQ@mail.gmail.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-04T07:55:15Z","receivedAt":"2017-06-04T07:55:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Jun 4, 2017 at 9:37 AM, Christian Couder\n wrote:\n> On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>>> My feeling exactly. Diagnosing and failing upfront saying \"well you\n>>>> made a copy but it is not suitable for testing\" sounds more sensible\n>>>> at lesat to me.\n>>>\n>>> This change makes the repo suitable for testing when it wasn't before.\n>>\n>> Perhaps \"not suitable\" was a bit too vague.\n>>\n>> The copy you made is not in a consistent state that is good for\n>> testing. This change may declare that it is now in a consistent\n>> state, but removal of a single *.lock file does not make it so. We\n>> do not know what other transient inconsistency the resulting copy\n>> has; it is inherent to git-unaware copy---that is why we discouraged\n>> and removed rsync transport after all.\n>\n> If we don't like git-unaware copies, maybe we should go back to the\n> reasons why we are making one here.\n> In 342e9ef2d9 (Introduce a performance testing framework, 2012-02-17),\n> Thomas wrote:\n>\n> 3. Creating test repos from scratch in every test is extremely\n> time-consuming, and shipping or downloading such large/weird repos\n> is out of the question.\n>\n> We leave this decision to the user. Two different sizes of test\n> repos can be configured, and the scripts just copy one or more of\n> those (using hardlinks for the object store). By default it tries\n> to use the build tree's git.git repository.\n>\n> This is fairly fast and versatile. Using a copy instead of a clone\n> preserves many properties that the user may want to test for, such\n> as lots of loose objects, unpacked refs, etc.\n>\n> Is a local clone really much slower these days? Wouldn't it is use\n> hard links too?\n> By the way the many properties that are preserved might not be worth\n> preserving as they could make results depend a lot on the current\n> state of the original repo.\n\nAFAICT from some quick testing it'll hardlink the objects/ dir, so\ne.g. you preserve the loose objects. Making the results depend on the\nstate of the original repo is a feature, but perhaps it should be opt\nin. It's very useful to be able to take a repo that's accrued e.g. a\nmonth's worth of refs & loose objects and test that v.s. a fresh\nclone.\n\nBut there are other things that ever a hardlink local clone doesn't\npreserve which might be worth preserving...\n"},{"id":"321502","messageId":"20170604082328.wdjb5yvx24w7biwk@sigill.intra.peff.net","threadId":"46099","inReplyTo":"CACBZZX4eCJJS6iaod+tWFF5M998Y5x88SdYR2ZekAJeeOiU8ug@mail.gmail.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-04T08:23:29Z","receivedAt":"2017-06-04T08:23:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 04, 2017 at 09:55:15AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Is a local clone really much slower these days? Wouldn't it is use\n> > hard links too?\n> > By the way the many properties that are preserved might not be worth\n> > preserving as they could make results depend a lot on the current\n> > state of the original repo.\n> \n> AFAICT from some quick testing it'll hardlink the objects/ dir, so\n> e.g. you preserve the loose objects. Making the results depend on the\n> state of the original repo is a feature, but perhaps it should be opt\n> in. It's very useful to be able to take a repo that's accrued e.g. a\n> month's worth of refs & loose objects and test that v.s. a fresh\n> clone.\n> \n> But there are other things that ever a hardlink local clone doesn't\n> preserve which might be worth preserving...\n\nYes. Reflogs are one example. They aren't copied at all as part of a\nclone, but they impact pruning and repacking.\n\n-Peff\n"},{"id":"321503","messageId":"20170604082919.y5g34udvyjfaqtve@sigill.intra.peff.net","threadId":"46099","inReplyTo":"xmqqefv0wrj7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-04T08:29:19Z","receivedAt":"2017-06-04T08:29:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 04, 2017 at 09:00:28AM +0900, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> >> My feeling exactly. Diagnosing and failing upfront saying \"well you\n> >> made a copy but it is not suitable for testing\" sounds more sensible\n> >> at lesat to me.\n> >\n> > This change makes the repo suitable for testing when it wasn't before.\n> \n> Perhaps \"not suitable\" was a bit too vague.\n> \n> The copy you made is not in a consistent state that is good for\n> testing. This change may declare that it is now in a consistent\n> state, but removal of a single *.lock file does not make it so. We\n> do not know what other transient inconsistency the resulting copy\n> has; it is inherent to git-unaware copy---that is why we discouraged\n> and removed rsync transport after all.\n\nRight. What I was getting at in my original message was that this is the\ntip of the iceberg if we are worried about inconsistent states. And the\nright solution is probably to say \"you are on your own for making sure\nthe repo you point to is in a sane state\". Because there are so many\ncases to catch, and they're so rare, it's not worth trying to catch them\nall.\n\nI don't really mind addressing this one case that much. I'm not sure\nthat we want to accrue a pile of band-aids here that causes a\nmaintenance burden and doesn't really solve the problem completely. One\nway to do that is to say no to the first band-aid. But we could also\napply it and see what happens. At the very worst it's a few extra lines\nof code, and we can start to get worried on the second or third\nband-aid.\n\n-Peff\n"},{"id":"321504","messageId":"20170604083743.kovvk5k43fk4kmur@sigill.intra.peff.net","threadId":"46099","inReplyTo":"xmqqwp8sv77h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-04T08:37:43Z","receivedAt":"2017-06-04T08:37:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 04, 2017 at 11:04:50AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But I think a more compelling case is that there may be an ongoing\n> > operation in the original repo (e.g., say you are in the middle of\n> > writing a commit message) when we do a blind copy of the filesystem\n> > contents. You might racily pick up a lockfile.\n> >\n> > Should we find and delete all *.lock files in the copied directory? That\n> > would get ref locks, etc. Half-formed object files are OK. Technically\n> > if you want to get an uncorrupted repository you'd also want to copy\n> > refs before objects (in case somebody makes a new object and updates a\n> > ref while you're copying).\n> \n> Or \"git branch -m A B\" is in progress.\n> \n> I think it all depends on what your \"threat\" model is ;-). Do we\n> assume that many users are \"time-sharing\" a box and a repository?\n> If not, i.e. if you are the sole user of a box and a repository on\n> it, such a concurrent access to make the result of git-unaware copy\n> problematic will not be in index.lock (after all you are now doing\n> the perf thing, not editing a commit log message in the repository\n> used for testing Git), but will be in ref locks (somebody else\n> pushing into the repository you are *not* currently using from\n> sideways).\n\nI was specifically thinking of the case where you run \"git commit -a\",\nit locks the index, and then while you are writing the commit message\nyou need to collect some more perf results. The default perf repo is the\ncurrent repository itself, so running any perf script will copy the\nindex.lock and probably cause the script to misbehave.\n\nThat doesn't seem like an implausible sequence of events (frankly, I'm\nsurprised I haven't hit it myself, as I often run perf scripts while\nwriting commit messages. I think I've been saved by generally using a\nseparate linux.git clone as my test repo).\n\nSo it may be reasonable to say that the index.lock is special. We hold\nit for a long time (compared to reflocks, which do a quick compare-and-swap).\nAnd then we can throw up our hands for any other races. People who run\n\"git prune\" on their test repository running the perf scripts get what\nthey deserve. :)\n\n-Peff\n"},{"id":"321518","messageId":"xmqqa85nur85.fsf@gitster.mtv.corp.google.com","threadId":"46099","inReplyTo":"CAP8UFD1u-C-T2X+H8WxxB7O+ajcGfygRBLN0t7+zJcOh_SrvFQ@mail.gmail.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-05T02:02:18Z","receivedAt":"2017-06-05T02:02:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>>> My feeling exactly. Diagnosing and failing upfront saying \"well you\n>>>> made a copy but it is not suitable for testing\" sounds more sensible\n>>>> at lesat to me.\n>>>\n>>> This change makes the repo suitable for testing when it wasn't before.\n>>\n>> Perhaps \"not suitable\" was a bit too vague.\n>>\n>> The copy you made is not in a consistent state that is good for\n>> testing. This change may declare that it is now in a consistent\n>> state, but removal of a single *.lock file does not make it so. We\n>> do not know what other transient inconsistency the resulting copy\n>> has; it is inherent to git-unaware copy---that is why we discouraged\n>> and removed rsync transport after all.\n>\n> If we don't like git-unaware copies, maybe we should go back to the\n> reasons why we are making one here.\n\nWe do need git-unaware bit-for-bit copy for testing, because you may\nwant to see the effect of unreachable objects, for example. \n\nIt's just that git-unaware copies, because it cannot be an atomic\nsnapshot, can introduce inconsistencies the original repository did\nnot have, rendering the result ineffective.\n"},{"id":"321519","messageId":"xmqq60gbur4i.fsf@gitster.mtv.corp.google.com","threadId":"46099","inReplyTo":"20170604082919.y5g34udvyjfaqtve@sigill.intra.peff.net","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-05T02:04:29Z","receivedAt":"2017-06-05T02:04:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't really mind addressing this one case that much. I'm not sure\n> that we want to accrue a pile of band-aids here that causes a\n> maintenance burden and doesn't really solve the problem completely. One\n> way to do that is to say no to the first band-aid. \n\nYup, that was where my objection came from.\n\n> But we could also\n> apply it and see what happens. At the very worst it's a few extra lines\n> of code, and we can start to get worried on the second or third\n> band-aid.\n\nThat's OK as well. But we should resolve now that we will rip all\nof them out once we start seeing the second or third band-aid. I\nreally do not want to see the \"accring a pile of band aids\" in our\nfuture.\n"},{"id":"321523","messageId":"CAP8UFD1LD+FVNvS_bR8KB7TWqmz01+gsyesubu3o0SWJvGAX9g@mail.gmail.com","threadId":"46099","inReplyTo":"xmqqa85nur85.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] perf: work around the tested repo having an index.lock","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-06-05T06:25:01Z","receivedAt":"2017-06-05T06:25:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 5, 2017 at 4:02 AM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano wrote:\n>>> Ævar Arnfjörð Bjarmason writes:\n>>>\n>>>>> My feeling exactly. Diagnosing and failing upfront saying \"well you\n>>>>> made a copy but it is not suitable for testing\" sounds more sensible\n>>>>> at lesat to me.\n>>>>\n>>>> This change makes the repo suitable for testing when it wasn't before.\n>>>\n>>> Perhaps \"not suitable\" was a bit too vague.\n>>>\n>>> The copy you made is not in a consistent state that is good for\n>>> testing. This change may declare that it is now in a consistent\n>>> state, but removal of a single *.lock file does not make it so. We\n>>> do not know what other transient inconsistency the resulting copy\n>>> has; it is inherent to git-unaware copy---that is why we discouraged\n>>> and removed rsync transport after all.\n>>\n>> If we don't like git-unaware copies, maybe we should go back to the\n>> reasons why we are making one here.\n>\n> We do need git-unaware bit-for-bit copy for testing, because you may\n> want to see the effect of unreachable objects, for example.\n\nI think there might be different kind of people interested in performance tests.\n\nUsers with existing repositories might want to see how the different\nGit versions perform on their real life repos.\nDevelopers might want to test Git on different repos with different\ncharacteristics.\n\nFor example some developers might want to test on repos with and\nwithout a lot of unreachable objects, to make sure that the latest\nchanges they made improve perf in both cases. While some users might\nonly be interested in testing on their actual repositories to see how\nthe latest Git versions improve things (or not) in practice.\n\nIn this example the needs of developers would perhaps be better suited\nif they could control the amount of unreachable objects in the tests,\nwhile the needs of the users would be better suited if the tests just\nused actual repos as is.\n\nSo I wonder what changes would be needed to the perf framework and the\nperf tests to accomodate both of these kinds of needs.\n\n\n\n> It's just that git-unaware copies, because it cannot be an atomic\n> snapshot, can introduce inconsistencies the original repository did\n> not have, rendering the result ineffective.\n"}]}