{"thread":{"id":"49917","subject":"How de-duplicate similar repositories with alternates","startedAt":"2018-11-29T14:59:32Z","lastAt":"2019-04-09T14:14:52Z","messageCount":29,"participants":["Ævar Arnfjörð Bjarmason","Stefan Beller","Duy Nguyen","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"364329","messageId":"87zhtsx73l.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":null,"subject":"How de-duplicate similar repositories with alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-29T14:59:26Z","receivedAt":"2018-11-29T14:59:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A co-worker asked me today how space could be saved when you have\nmultiple checkouts of the same repository (at different revs) on the\nsame machine. I said since these won't block-level de-duplicate well[1]\none way to do this is with alternates.\n\nHowever, once you have an existing clone I didn't know how to get the\ngains without a full re-clone, but I hadn't looked deeply into it. As it\nturns out I'm wrong about that, which I found when writing the following\ntest-case which shows that it works:\n\n    (\n        cd /tmp &&\n        rm -rf /tmp/git-{master,pu,pu-alt}.git &&\n\n        # Normal clones\n        git clone --bare --no-tags --single-branch --branch master https://github.com/git/git.git /tmp/git-master.git &&\n        git clone --bare --no-tags --single-branch --branch pu https://github.com/git/git.git /tmp/git-pu.git &&\n\n        # An 'alternate' clone using 'master' objects from another repo\n        git --bare init /tmp/git-pu-alt.git &&\n        for git in git-pu.git git-pu-alt.git\n        do\n            echo /tmp/git-master.git/objects >/tmp/$git/objects/info/alternates\n        done &&\n        git -C git-pu-alt.git fetch --no-tags https://github.com/git/git.git pu:pu\n\n        # Respective sizes, 'alternate' clone much smaller\n        du -shc /tmp/git-*.git &&\n\n        # GC them all. Compacts the git-pu.git to git-pu-alt.git's size\n        for repo in git-*.git\n        do\n            git -C $repo gc\n        done &&\n        du -shc /tmp/git-*.git\n\n        # Add another big history (GFW) to git-{pu,master}.git (in that order!)\n        for repo in $(ls -d /tmp/git-*.git | sort -r)\n        do\n            git -C $repo fetch --no-tags https://github.com/git-for-windows/git master:master-gfw\n        done &&\n        du -shc /tmp/git-*.git &&\n\n        # Another GC. The objects now in git-master.git will be de-duped by all\n        for repo in git-*.git\n        do\n            git -C $repo gc\n        done &&\n        du -shc /tmp/git-*.git\n    )\n\nThis shows a scenario where we clone git.git at \"master\" and \"pu\" in\ndifferent places. After clone the relevant sizes are:\n\n    108M    /tmp/git-master.git\n    3.2M    /tmp/git-pu-alt.git\n    109M    /tmp/git-pu.git\n    219M    total\n\nI.e. git-pu-alt.git is much smaller since it points via alternates to\ngit-master.git, and the history of \"pu\" shares most of the objects with\n\"master\". But then how do you get those gains for git-pu.git? Turns out\nyou just \"git gc\"\n\n    111M    /tmp/git-master.git\n    2.1M    /tmp/git-pu-alt.git\n    2.1M    /tmp/git-pu.git\n    115M    total\n\nThis is the thing I was wrong about, in retrospect probably because I'd\nbeen putting PATH_TO_REPO in objects/info/alternates, but we actually\nneed PATH_TO_REPO/objects, and \"git gc\" won't warn about this (or \"git\nfsck\"). Probably a good idea to patch that at some point, i.e. whine\nabout paths in alternates that don't have objects, or at the very least\nthose that don't exist. #leftoverbits\n\nThen when we fetch git-for-windows:master to all the repos they all grow\nby the amount git-for-windows has diverged:\n\n    144M    /tmp/git-master.git\n    36M     /tmp/git-pu-alt.git\n    36M     /tmp/git-pu.git\n    214M    total\n\nNote that the \"sort -r\" is critical here. If we fetched git-master.git\nfirst (at this point the alternate for git-pu*.git) we wouldn't get the\nduplication in the first place, but instead:\n\n    144M    /tmp/git-master.git\n    2.1M    /tmp/git-pu-alt.git\n    2.1M    /tmp/git-pu.git\n    148M    total\n\nThis shows the importance of keeping such an 'alternate' repo\nup-to-date, i.e. we don't get the duplication in the first place, but\nregardless (this from a run with sort -r) a \"git gc\" will coalesce them:\n\n    131M    /tmp/git-master.git\n    2.1M    /tmp/git-pu-alt.git\n    2.2M    /tmp/git-pu.git\n    135M    total\n\nIf you find this interesting make sure to read my\nhttps://public-inbox.org/git/87k1s3bomt.fsf@evledraar.gmail.com/ and\nhttps://public-inbox.org/git/87in7nbi5b.fsf@evledraar.gmail.com/ for the\ncaveats, i.e. if this is something intended for users then no ref in the\nalternate can ever be rewound, that'll potentially result in repository\ncorruption.\n\n1. https://public-inbox.org/git/87bmhiykvw.fsf@evledraar.gmail.com/\n"},{"id":"364342","messageId":"87sgzjyif2.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"87zhtsx73l.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-29T16:09:37Z","receivedAt":"2018-11-29T16:09:43Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 29 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> A co-worker asked me today how space could be saved when you have\n> multiple checkouts of the same repository (at different revs) on the\n> same machine. I said since these won't block-level de-duplicate well[1]\n> one way to do this is with alternates.\n>\n> However, once you have an existing clone I didn't know how to get the\n> gains without a full re-clone, but I hadn't looked deeply into it. As it\n> turns out I'm wrong about that, which I found when writing the following\n> test-case which shows that it works:\n>\n>     (\n>         cd /tmp &&\n>         rm -rf /tmp/git-{master,pu,pu-alt}.git &&\n>\n>         # Normal clones\n>         git clone --bare --no-tags --single-branch --branch master https://github.com/git/git.git /tmp/git-master.git &&\n>         git clone --bare --no-tags --single-branch --branch pu https://github.com/git/git.git /tmp/git-pu.git &&\n>\n>         # An 'alternate' clone using 'master' objects from another repo\n>         git --bare init /tmp/git-pu-alt.git &&\n>         for git in git-pu.git git-pu-alt.git\n>         do\n>             echo /tmp/git-master.git/objects >/tmp/$git/objects/info/alternates\n>         done &&\n>         git -C git-pu-alt.git fetch --no-tags https://github.com/git/git.git pu:pu\n>\n>         # Respective sizes, 'alternate' clone much smaller\n>         du -shc /tmp/git-*.git &&\n>\n>         # GC them all. Compacts the git-pu.git to git-pu-alt.git's size\n>         for repo in git-*.git\n>         do\n>             git -C $repo gc\n>         done &&\n>         du -shc /tmp/git-*.git\n>\n>         # Add another big history (GFW) to git-{pu,master}.git (in that order!)\n>         for repo in $(ls -d /tmp/git-*.git | sort -r)\n>         do\n>             git -C $repo fetch --no-tags https://github.com/git-for-windows/git master:master-gfw\n>         done &&\n>         du -shc /tmp/git-*.git &&\n>\n>         # Another GC. The objects now in git-master.git will be de-duped by all\n>         for repo in git-*.git\n>         do\n>             git -C $repo gc\n>         done &&\n>         du -shc /tmp/git-*.git\n>     )\n>\n> This shows a scenario where we clone git.git at \"master\" and \"pu\" in\n> different places. After clone the relevant sizes are:\n>\n>     108M    /tmp/git-master.git\n>     3.2M    /tmp/git-pu-alt.git\n>     109M    /tmp/git-pu.git\n>     219M    total\n>\n> I.e. git-pu-alt.git is much smaller since it points via alternates to\n> git-master.git, and the history of \"pu\" shares most of the objects with\n> \"master\". But then how do you get those gains for git-pu.git? Turns out\n> you just \"git gc\"\n>\n>     111M    /tmp/git-master.git\n>     2.1M    /tmp/git-pu-alt.git\n>     2.1M    /tmp/git-pu.git\n>     115M    total\n>\n> This is the thing I was wrong about, in retrospect probably because I'd\n> been putting PATH_TO_REPO in objects/info/alternates, but we actually\n> need PATH_TO_REPO/objects, and \"git gc\" won't warn about this (or \"git\n> fsck\"). Probably a good idea to patch that at some point, i.e. whine\n> about paths in alternates that don't have objects, or at the very least\n> those that don't exist. #leftoverbits\n\nActually looking at this again the thing that may have stumped me last\ntime is that this has a bad interaction with gc.bigPackThreshold. If you\nhave an alternate that would otherwise house most of your objects *and*\nyou have a pack that's larger than the gc.bigPackThreshold your mostly\nredundant pack won't be removed.\n\nThat's understandable in terms of implementation, but unfortunate. It\nwould be nice if we learned some way to detect this, i.e. \"I have this\n10GB pack, but with this alternate I can extract this 100MB out of it\nand throw it away\". Now we just keep the 10GB pack even if it's mostly\nredundant to what's in the alternate.\n\n> Then when we fetch git-for-windows:master to all the repos they all grow\n> by the amount git-for-windows has diverged:\n>\n>     144M    /tmp/git-master.git\n>     36M     /tmp/git-pu-alt.git\n>     36M     /tmp/git-pu.git\n>     214M    total\n>\n> Note that the \"sort -r\" is critical here. If we fetched git-master.git\n> first (at this point the alternate for git-pu*.git) we wouldn't get the\n> duplication in the first place, but instead:\n>\n>     144M    /tmp/git-master.git\n>     2.1M    /tmp/git-pu-alt.git\n>     2.1M    /tmp/git-pu.git\n>     148M    total\n>\n> This shows the importance of keeping such an 'alternate' repo\n> up-to-date, i.e. we don't get the duplication in the first place, but\n> regardless (this from a run with sort -r) a \"git gc\" will coalesce them:\n>\n>     131M    /tmp/git-master.git\n>     2.1M    /tmp/git-pu-alt.git\n>     2.2M    /tmp/git-pu.git\n>     135M    total\n>\n> If you find this interesting make sure to read my\n> https://public-inbox.org/git/87k1s3bomt.fsf@evledraar.gmail.com/ and\n> https://public-inbox.org/git/87in7nbi5b.fsf@evledraar.gmail.com/ for the\n> caveats, i.e. if this is something intended for users then no ref in the\n> alternate can ever be rewound, that'll potentially result in repository\n> corruption.\n>\n> 1. https://public-inbox.org/git/87bmhiykvw.fsf@evledraar.gmail.com/\n"},{"id":"364348","messageId":"CAGZ79ka1sjU+rHctRP4SVMP0GQsK2iZghkU46=f96ugqvX5Neg@mail.gmail.com","threadId":"49917","inReplyTo":"87zhtsx73l.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-29T18:55:49Z","receivedAt":"2018-11-29T18:56:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 29, 2018 at 7:00 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> A co-worker asked me today how space could be saved when you have\n> multiple checkouts of the same repository (at different revs) on the\n> same machine. I said since these won't block-level de-duplicate well[1]\n> one way to do this is with alternates.\n\nAnother way is to use git-worktree, which would solve the gc issues\nmentioned below?\n\nI view alternates as a historic artefact as the deduping\nof objects client side can be done using worktrees, and on the\nserverside - I think - most of the git hosters use namespaces\nand put a fork network into the same repository and use pack islands.\n\nCan you elaborate on why worktrees would not solve the problem?\n(I initially was hesitant to use them as I liked going into .git and tempering\nwith files such as the config directly. But now I cannot `cd .git` any more;\nit turns out the advantages outweigh this corner case that I was attached to)\n"},{"id":"364353","messageId":"87r2f3y7a9.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"CAGZ79ka1sjU+rHctRP4SVMP0GQsK2iZghkU46=f96ugqvX5Neg@mail.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-29T20:10:06Z","receivedAt":"2018-11-29T20:10:15Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 29 2018, Stefan Beller wrote:\n\n> On Thu, Nov 29, 2018 at 7:00 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> A co-worker asked me today how space could be saved when you have\n>> multiple checkouts of the same repository (at different revs) on the\n>> same machine. I said since these won't block-level de-duplicate well[1]\n>> one way to do this is with alternates.\n>\n> Another way is to use git-worktree, which would solve the gc issues\n> mentioned below?\n>\n> I view alternates as a historic artefact as the deduping\n> of objects client side can be done using worktrees, and on the\n> serverside - I think - most of the git hosters use namespaces\n> and put a fork network into the same repository and use pack islands.\n>\n> Can you elaborate on why worktrees would not solve the problem?\n> (I initially was hesitant to use them as I liked going into .git and tempering\n> with files such as the config directly. But now I cannot `cd .git` any more;\n> it turns out the advantages outweigh this corner case that I was attached to)\n\nThis was discussed recently on-list & I chimed in with details about\nthat here:\nhttps://public-inbox.org/git/87po1waqyc.fsf@evledraar.gmail.com/ &\nhttps://public-inbox.org/git/87muwzc2kv.fsf@evledraar.gmail.com/\n\nIn particular the \"multiple devs\" use-case described in the latter\nE-Mail is what I have in mind. Worktrees are inherently unsuitable for\nthat.\n"},{"id":"364354","messageId":"CACsJy8A8XyzpMFDTFBaB58-AhDz0d6haacNMNuNEUNjFghYgsA@mail.gmail.com","threadId":"49917","inReplyTo":"87r2f3y7a9.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-29T20:43:15Z","receivedAt":"2018-11-29T20:43:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 29, 2018 at 9:15 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Thu, Nov 29 2018, Stefan Beller wrote:\n>\n> > On Thu, Nov 29, 2018 at 7:00 AM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >>\n> >> A co-worker asked me today how space could be saved when you have\n> >> multiple checkouts of the same repository (at different revs) on the\n> >> same machine. I said since these won't block-level de-duplicate well[1]\n> >> one way to do this is with alternates.\n> >\n> > Another way is to use git-worktree, which would solve the gc issues\n> > mentioned below?\n> >\n> > I view alternates as a historic artefact as the deduping\n> > of objects client side can be done using worktrees, and on the\n> > serverside - I think - most of the git hosters use namespaces\n> > and put a fork network into the same repository and use pack islands.\n> >\n> > Can you elaborate on why worktrees would not solve the problem?\n> > (I initially was hesitant to use them as I liked going into .git and tempering\n> > with files such as the config directly. But now I cannot `cd .git` any more;\n> > it turns out the advantages outweigh this corner case that I was attached to)\n>\n> This was discussed recently on-list & I chimed in with details about\n> that here:\n> https://public-inbox.org/git/87po1waqyc.fsf@evledraar.gmail.com/ &\n> https://public-inbox.org/git/87muwzc2kv.fsf@evledraar.gmail.com/\n>\n> In particular the \"multiple devs\" use-case described in the latter\n> E-Mail is what I have in mind. Worktrees are inherently unsuitable for\n> that.\n\nYeah, the separate ref namespace is something I would like to have on\nthe client side too. I did consider implementing it a couple times but\nit's really no small task. Naively, I could achieve that pretty quick\nif all refs are loose, but performance would tank once the number of\nrefs goes over say a hundred.\n-- \nDuy\n"},{"id":"364539","messageId":"20181204065930.GA11010@sigill.intra.peff.net","threadId":"49917","inReplyTo":"87zhtsx73l.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-04T06:59:31Z","receivedAt":"2018-12-04T06:59:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 29, 2018 at 03:59:26PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> This is the thing I was wrong about, in retrospect probably because I'd\n> been putting PATH_TO_REPO in objects/info/alternates, but we actually\n> need PATH_TO_REPO/objects, and \"git gc\" won't warn about this (or \"git\n> fsck\"). Probably a good idea to patch that at some point, i.e. whine\n> about paths in alternates that don't have objects, or at the very least\n> those that don't exist. #leftoverbits\n\nWe do complain about missing directories; see alt_odb_usable().\nPointing to a real directory that doesn't happen to contain any objects\nis harder. If there are no loose objects, there might not be any hashed\nobject directories. For a \"real\" object database, there should always be\na \"pack/\" directory. But technically the object storage directory does\nnot even need to have that; it can just be a directory full of loose\nobjects that happens not to have any at this moment.\n\nThat said, I suspect if we issued a warning for \"woah, it looks like\nthis doesn't have any objects in it, nor does it even have a pack\ndirectory\" that nobody would complain.\n\n-Peff\n"},{"id":"364540","messageId":"20181204070602.GB11010@sigill.intra.peff.net","threadId":"49917","inReplyTo":"CAGZ79ka1sjU+rHctRP4SVMP0GQsK2iZghkU46=f96ugqvX5Neg@mail.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-04T07:06:02Z","receivedAt":"2018-12-04T07:06:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 29, 2018 at 10:55:49AM -0800, Stefan Beller wrote:\n\n> On Thu, Nov 29, 2018 at 7:00 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n> >\n> > A co-worker asked me today how space could be saved when you have\n> > multiple checkouts of the same repository (at different revs) on the\n> > same machine. I said since these won't block-level de-duplicate well[1]\n> > one way to do this is with alternates.\n> \n> Another way is to use git-worktree, which would solve the gc issues\n> mentioned below?\n> \n> I view alternates as a historic artefact as the deduping\n> of objects client side can be done using worktrees, and on the\n> serverside - I think - most of the git hosters use namespaces\n> and put a fork network into the same repository and use pack islands.\n\nNope, we definitely use alternates. The ref namespace support in Git is\nnot nearly complete enough to run a modern hosting site; it only kicks\nin for upload-pack and receive-pack. Other commands (e.g., rev-list to\ntraverse for a history-view page) have no support at all. So we share\nobject storage, but not ref storage.\n\nIn theory the caller could namespace requests (e.g., the user asks for\n\"foo\", the web site feeds \"refs/forks/$id/refs/heads/foo\" to git). But\nany bugs are a lot more likely to lead to security problems (oops, you\naccidentally wrote into somebody else's fork!). And ref storage has\ntraditionally been a sore point for scaling, so giving each fork its own\nrepo and refs helps break that up.\n\nBy contrast, object storage is pretty easy to share. It scales\nreasonably well, and the security model is much simpler due to the\nimmutable nature of object names.\n\n-Peff\n"},{"id":"364544","messageId":"87tvjtvah0.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"20181204065930.GA11010@sigill.intra.peff.net","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-04T10:43:07Z","receivedAt":"2018-12-04T10:43:13Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 04 2018, Jeff King wrote:\n\n> On Thu, Nov 29, 2018 at 03:59:26PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This is the thing I was wrong about, in retrospect probably because I'd\n>> been putting PATH_TO_REPO in objects/info/alternates, but we actually\n>> need PATH_TO_REPO/objects, and \"git gc\" won't warn about this (or \"git\n>> fsck\"). Probably a good idea to patch that at some point, i.e. whine\n>> about paths in alternates that don't have objects, or at the very least\n>> those that don't exist. #leftoverbits\n>\n> We do complain about missing directories; see alt_odb_usable().\n> Pointing to a real directory that doesn't happen to contain any objects\n> is harder. If there are no loose objects, there might not be any hashed\n> object directories. For a \"real\" object database, there should always be\n> a \"pack/\" directory. But technically the object storage directory does\n> not even need to have that; it can just be a directory full of loose\n> objects that happens not to have any at this moment.\n>\n> That said, I suspect if we issued a warning for \"woah, it looks like\n> this doesn't have any objects in it, nor does it even have a pack\n> directory\" that nobody would complain.\n\nYeah, although see my <87sgzjyif2.fsf@evledraar.gmail.com>, I also ran\ninto a different issue.\n\nI think a warning (or even error) like this would be more useful:\n\n    test ! -d $objdir && error... # current behavior\n    test -d $objdir/objects && error \"Did you mean $objdir/objects, silly?\" # new error\n\nI.e. I suspect I'm not the only one who's not read the documentation\ncarefully enough and thought it was a path to the root of the repo and\nwondered why it silently didn't work.\n"},{"id":"364549","messageId":"5f2f5c9f-c27b-2577-ca4d-375c92ab9186@gmail.com","threadId":"49917","inReplyTo":"20181204070602.GB11010@sigill.intra.peff.net","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-12-04T12:07:38Z","receivedAt":"2018-12-04T12:07:44Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/4/2018 2:06 AM, Jeff King wrote:\n> On Thu, Nov 29, 2018 at 10:55:49AM -0800, Stefan Beller wrote:\n>\n>> I view alternates as a historic artefact as the deduping\n>> of objects client side can be done using worktrees, and on the\n>> serverside - I think - most of the git hosters use namespaces\n>> and put a fork network into the same repository and use pack islands.\n> By contrast, object storage is pretty easy to share. It scales\n> reasonably well, and the security model is much simpler due to the\n> immutable nature of object names.\n\nAnd for the client side, we use alternates as an important way to scale \nVFS for Git to multiple enlistments on the same machine. VFS for Git \nmanages a \"shared object cache\" (the alternate) that is updated in the \nbackground (including multi-pack-index and commit-graph).\n\nUsing worktrees for the same effect would add complications to the user \ninteractions, not only when creating an enlistment but the fact that two \nenlistments cannot check out the same ref will confuse users.\n\nThanks,\n-Stolee\n"},{"id":"364550","messageId":"20181204132716.19208-2-avarab@gmail.com","threadId":"49917","inReplyTo":"87tvjtvah0.fsf@evledraar.gmail.com","subject":"[PATCH 1/3] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-04T13:27:14Z","receivedAt":"2018-12-04T13:27:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a test for the error() case in alt_odb_usable() where an alternate\ndirectory doesn't exist. This behavior has been the same since\n26125f6b9b (\"detect broken alternates.\", 2006-02-22), but if that\nerror() was turned into die() the entire test suite would still pass.\n\nPerhaps we should die() in that case, but let's start by adding a test\nhere to assert the long-standing existing behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5613-info-alternate.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 895f46bb91..d2964c57b7 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -136,4 +136,11 @@ test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n \ttest_cmp expect actual.alternates\n '\n \n+test_expect_success 'print \"error\" on non-existing alternate' '\n+\tgit init --bare I &&\n+\techo DOES_NOT_EXIST >I/objects/info/alternates &&\n+\tgit -C I fsck 2>stderr &&\n+\ttest_i18ngrep \"does not exist; check\" stderr\n+'\n+\n test_done\n-- \n2.20.0.rc2.403.gdbc3b29805\n\n"},{"id":"364551","messageId":"20181204132716.19208-1-avarab@gmail.com","threadId":"49917","inReplyTo":"87tvjtvah0.fsf@evledraar.gmail.com","subject":"[PATCH 0/3] sha1-file: warn if alternate is a git repo (not object dir)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-04T13:27:13Z","receivedAt":"2018-12-04T13:27:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This adds a warning for the issue discussed upthread. As noted in\nthese patches we've been emitting an \"error\" while not impacting the\nexit code, should we die() instead? Maybe, but until there's consensus\non that let's change this to warning() while we're at it.\n\nÆvar Arnfjörð Bjarmason (3):\n  sha1-file: test the error behavior of alt_odb_usable()\n  sha1-file: emit error if an alternate looks like a repository\n  sha1-file: change alternate \"error:\" message to \"warning:\"\n\n sha1-file.c               | 16 ++++++++++++----\n t/t5613-info-alternate.sh | 21 +++++++++++++++++++++\n 2 files changed, 33 insertions(+), 4 deletions(-)\n\n-- \n2.20.0.rc2.403.gdbc3b29805\n\n"},{"id":"364552","messageId":"20181204132716.19208-3-avarab@gmail.com","threadId":"49917","inReplyTo":"87tvjtvah0.fsf@evledraar.gmail.com","subject":"[PATCH 2/3] sha1-file: emit error if an alternate looks like a repository","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-04T13:27:15Z","receivedAt":"2018-12-04T13:27:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 26125f6b9b (\"detect broken alternates.\", 2006-02-22) we've\nemitted an error if the alternates directory doesn't exist, but not\nfor the common misstep of adding a path to another git repository as\nan alternate, as opposed to its \"objects\" directory.\n\nLet's check for this, i.e. whether X/objects or X/.git/objects exists\nif the user supplies X and print an error (which as a commit leading\nup to this one shows doesn't change the exit code, just \"warns\").\n\nThis check is intentionally not implemented by e.g. requiring that any\nof X/?? exists or X/info or X/pack exists. It's a legitimate use-case\nto point to an existing alternate that hasn't been populated yet, but\npointing to one where an \"X/objects\" or \"X/.git/objects\" directory\nexists is definitely a mistake we should warn the user about.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n sha1-file.c               | 10 +++++++++-\n t/t5613-info-alternate.sh | 14 ++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 5bd11c85bc..f142f81658 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -376,12 +376,20 @@ static int alt_odb_usable(struct raw_object_store *o,\n {\n \tstruct alternate_object_database *alt;\n \n-\t/* Detect cases where alternate disappeared */\n \tif (!is_directory(path->buf)) {\n+\t\t/* Detect cases where alternate disappeared */\n \t\terror(_(\"object directory %s does not exist; \"\n \t\t\t\"check .git/objects/info/alternates\"),\n \t\t      path->buf);\n \t\treturn 0;\n+\t} else if (is_directory(mkpath(\"%s/objects\", path->buf)) ||\n+\t\t   is_directory(mkpath(\"%s/.git/objects\", path->buf))) {\n+\t\t/* Detect cases where alternate is a git repository */\n+\t\terror(_(\"object directory %s looks like a git repository; \"\n+\t\t\t\"alternates must point to the 'objects' directory. \"\n+\t\t\t\"check .git/objects/info/alternates\"),\n+\t\t      path->buf);\n+\t\treturn 0;\n \t}\n \n \t/*\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex d2964c57b7..b959e21421 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -143,4 +143,18 @@ test_expect_success 'print \"error\" on non-existing alternate' '\n \ttest_i18ngrep \"does not exist; check\" stderr\n '\n \n+test_expect_success 'print \"error\" on alternate that looks like a git repository' '\n+\tgit init --bare J &&\n+\tgit init --bare K &&\n+\n+\t# H is bare, G is not\n+\techo ../../H >J/objects/info/alternates &&\n+\techo ../../G >K/objects/info/alternates &&\n+\n+\tgit -C J fsck 2>stderr &&\n+\ttest_i18ngrep \"looks like a git repository; alternates must\" stderr &&\n+\tgit -C K fsck 2>stderr &&\n+\ttest_i18ngrep \"looks like a git repository; alternates must\" stderr\n+'\n+\n test_done\n-- \n2.20.0.rc2.403.gdbc3b29805\n\n"},{"id":"364553","messageId":"20181204132716.19208-4-avarab@gmail.com","threadId":"49917","inReplyTo":"87tvjtvah0.fsf@evledraar.gmail.com","subject":"[PATCH 3/3] sha1-file: change alternate \"error:\" message to \"warning:\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-04T13:27:16Z","receivedAt":"2018-12-04T13:27:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"error\" message emitted by alt_odb_usable() to be a\n\"warning\" instead. As noted in commits leading up to this one this has\nnever impacted the exit code ever since the check was initially added\nin 26125f6b9b (\"detect broken alternates.\", 2006-02-22).\n\nIt's confusing to emit an \"error\" when e.g. \"git fsck\" will exit with\n0, so let's emit a \"warning:\" instead.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n sha1-file.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex f142f81658..4b9b63bdcb 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -378,17 +378,17 @@ static int alt_odb_usable(struct raw_object_store *o,\n \n \tif (!is_directory(path->buf)) {\n \t\t/* Detect cases where alternate disappeared */\n-\t\terror(_(\"object directory %s does not exist; \"\n-\t\t\t\"check .git/objects/info/alternates\"),\n-\t\t      path->buf);\n+\t\twarning(_(\"object directory %s does not exist; \"\n+\t\t\t  \"check .git/objects/info/alternates\"),\n+\t\t\tpath->buf);\n \t\treturn 0;\n \t} else if (is_directory(mkpath(\"%s/objects\", path->buf)) ||\n \t\t   is_directory(mkpath(\"%s/.git/objects\", path->buf))) {\n \t\t/* Detect cases where alternate is a git repository */\n-\t\terror(_(\"object directory %s looks like a git repository; \"\n-\t\t\t\"alternates must point to the 'objects' directory. \"\n-\t\t\t\"check .git/objects/info/alternates\"),\n-\t\t      path->buf);\n+\t\twarning(_(\"object directory %s looks like a git repository; \"\n+\t\t\t  \"alternates must point to the 'objects' directory. \"\n+\t\t\t  \"check .git/objects/info/alternates\"),\n+\t\t\tpath->buf);\n \t\treturn 0;\n \t}\n \n-- \n2.20.0.rc2.403.gdbc3b29805\n\n"},{"id":"364554","messageId":"87sgzdv2gz.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"87zhtsx73l.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-04T13:35:56Z","receivedAt":"2018-12-04T13:36:03Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 29 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> A co-worker asked me today how space could be saved when you have\n> multiple checkouts of the same repository (at different revs) on the\n> same machine. I said since these won't block-level de-duplicate well[1]\n> one way to do this is with alternates.\n>\n> However, once you have an existing clone I didn't know how to get the\n> gains without a full re-clone, but I hadn't looked deeply into it. As it\n> turns out I'm wrong about that, which I found when writing the following\n> test-case which shows that it works:\n>\n>     (\n>         cd /tmp &&\n>         rm -rf /tmp/git-{master,pu,pu-alt}.git &&\n>\n>         # Normal clones\n>         git clone --bare --no-tags --single-branch --branch master https://github.com/git/git.git /tmp/git-master.git &&\n>         git clone --bare --no-tags --single-branch --branch pu https://github.com/git/git.git /tmp/git-pu.git &&\n>\n>         # An 'alternate' clone using 'master' objects from another repo\n>         git --bare init /tmp/git-pu-alt.git &&\n>         for git in git-pu.git git-pu-alt.git\n>         do\n>             echo /tmp/git-master.git/objects >/tmp/$git/objects/info/alternates\n>         done &&\n>         git -C git-pu-alt.git fetch --no-tags https://github.com/git/git.git pu:pu\n>\n>         # Respective sizes, 'alternate' clone much smaller\n>         du -shc /tmp/git-*.git &&\n>\n>         # GC them all. Compacts the git-pu.git to git-pu-alt.git's size\n>         for repo in git-*.git\n>         do\n>             git -C $repo gc\n>         done &&\n>         du -shc /tmp/git-*.git\n>\n>         # Add another big history (GFW) to git-{pu,master}.git (in that order!)\n>         for repo in $(ls -d /tmp/git-*.git | sort -r)\n>         do\n>             git -C $repo fetch --no-tags https://github.com/git-for-windows/git master:master-gfw\n>         done &&\n>         du -shc /tmp/git-*.git &&\n>\n>         # Another GC. The objects now in git-master.git will be de-duped by all\n>         for repo in git-*.git\n>         do\n>             git -C $repo gc\n>         done &&\n>         du -shc /tmp/git-*.git\n>     )\n>\n> This shows a scenario where we clone git.git at \"master\" and \"pu\" in\n> different places. After clone the relevant sizes are:\n>\n>     108M    /tmp/git-master.git\n>     3.2M    /tmp/git-pu-alt.git\n>     109M    /tmp/git-pu.git\n>     219M    total\n>\n> I.e. git-pu-alt.git is much smaller since it points via alternates to\n> git-master.git, and the history of \"pu\" shares most of the objects with\n> \"master\". But then how do you get those gains for git-pu.git? Turns out\n> you just \"git gc\"\n>\n>     111M    /tmp/git-master.git\n>     2.1M    /tmp/git-pu-alt.git\n>     2.1M    /tmp/git-pu.git\n>     115M    total\n>\n> This is the thing I was wrong about, in retrospect probably because I'd\n> been putting PATH_TO_REPO in objects/info/alternates, but we actually\n> need PATH_TO_REPO/objects, and \"git gc\" won't warn about this (or \"git\n> fsck\"). Probably a good idea to patch that at some point, i.e. whine\n> about paths in alternates that don't have objects, or at the very least\n> those that don't exist. #leftoverbits\n>\n> Then when we fetch git-for-windows:master to all the repos they all grow\n> by the amount git-for-windows has diverged:\n>\n>     144M    /tmp/git-master.git\n>     36M     /tmp/git-pu-alt.git\n>     36M     /tmp/git-pu.git\n>     214M    total\n>\n> Note that the \"sort -r\" is critical here. If we fetched git-master.git\n> first (at this point the alternate for git-pu*.git) we wouldn't get the\n> duplication in the first place, but instead:\n>\n>     144M    /tmp/git-master.git\n>     2.1M    /tmp/git-pu-alt.git\n>     2.1M    /tmp/git-pu.git\n>     148M    total\n>\n> This shows the importance of keeping such an 'alternate' repo\n> up-to-date, i.e. we don't get the duplication in the first place, but\n> regardless (this from a run with sort -r) a \"git gc\" will coalesce them:\n>\n>     131M    /tmp/git-master.git\n>     2.1M    /tmp/git-pu-alt.git\n>     2.2M    /tmp/git-pu.git\n>     135M    total\n>\n> If you find this interesting make sure to read my\n> https://public-inbox.org/git/87k1s3bomt.fsf@evledraar.gmail.com/ and\n> https://public-inbox.org/git/87in7nbi5b.fsf@evledraar.gmail.com/ for the\n> caveats, i.e. if this is something intended for users then no ref in the\n> alternate can ever be rewound, that'll potentially result in repository\n> corruption.\n>\n> 1. https://public-inbox.org/git/87bmhiykvw.fsf@evledraar.gmail.com/\n\nMaybe this is useful to someone. Here's a cronjob I wrote since I wrote\nthis thread that runs in daily cron on some of our systems.\n\nIt expects repositories in /var/lib/git_tree-for-alternates like\n/var/lib/git_tree-for-alternates/git/git.git to exist, then scours /home\nand /etc/puppet/environments (which we had a lot of) for \"config\" files\nwith the string in git/git (this saves us some work) and then tries to\nfind a git repository relative to that \"config\" file with \"rev-parse\n--absolute-git-dir\".\n\nIf there is one, we check if the repository has a SHA-1 that the history\nof our /var/lib/git_tree-for-alternates/git/git.git started with (if >1\nwe pick the oldest), if so this is a repository that can benefit from\nusing /var/lib/git_tree-for-alternates/git/git.git/objects as an\nalternate, and we add the appropriate alternate info, unset\ngc.bigPackThreshold so GC will actually do its work, and run \"git gc\"\nsudo'd as the the user who owns the thing.\n\nOne one server the .git directories in /home went from ~2TB to ~100GB\nusing this script. On another from ~250G to ~5G. The leftover space\nspent is the commit-grah (not de-duped like objects are), and whatever\naccumulated divergence (topic branches mainly) exist in those repos\ndifferent than what the alternate store has in the HEAD branch.\n\n#!/bin/bash\n\nset -euo pipefail\n\nALTERNATES_STORE=/var/lib/git_tree-for-alternates\n\nif ! test -d $ALTERNATES_STORE\nthen\n    echo 'We have no alternates repositories here to point to!' >&2\n    exit 0\nfi\n\n\nfind_owning_user() {\n    path=$1\n    case $path in\n        /home/*|/etc/puppet/environments/*)\n            who=$(echo $path | perl -pe 's[^\n                (?:\n                    /home\n                    |\n                    /etc/puppet/environments\n                )\n                /\n                ([^/]+)\n                /\n                .*\n            ][$1]gx')\n            if getent passwd $who >/dev/null\n            then\n                echo $who\n            else\n                echo \"Know how to get user from path '$path', but '$who' is not a valid user!\" >&2\n            fi\n            ;;\n        *)\n            echo \"Don't know how to get user from path '$path' yet!\" >&2\n            ;;\n    esac\n}\n\nfind $ALTERNATES_STORE -type d -name '*.git' -printf \"%P\\n\" |\nwhile read alternate\ndo\n    alternate_no_git=$(echo $alternate | sed 's/\\.git//')\n    ALTERNATES_STORE_OBJECTS=$ALTERNATES_STORE/$alternate/objects\n\n    # If these repositories we're finding don't share a root commit\n    # with the repo we have this is not going to work and we have the\n    # wrong match. Note that we can have more than one root commit\n    # and try to find the oldest one. Pretty sure bet that that's\n    # the \"real\" root.\n    root_commit=$(git -C $ALTERNATES_STORE/$alternate log --max-parents=0 --date-order --reverse --pretty=format:%H | head -n 1)\n    echo \"> Finding repositories on the system that share the $root_commit commit with $alternate\" >&2\n\n    find \\\n        /home \\\n        $(if test -d /etc/puppet/environments; then echo /etc/puppet/environments; fi) \\\n        -type f -name 'config' -exec grep -Hl $alternate_no_git {} \\; 2>/dev/null |\n    while read config\n    do\n        dirname=$(dirname $config)\n        echo \">> Checking if $dirname is in a $alternate git repository...\" >&2\n        if git_dir=$(git -C $dirname rev-parse --absolute-git-dir) &&\n                git -C $git_dir cat-file -e $root_commit\n        then\n            echo \">>> ...Yes it was, at $git_dir\" >&2\n            echo \">>>> Is it already migrated?...\" >&2\n            if test -e $git_dir/objects/info/alternates &&\n                    grep -x -F -q $ALTERNATES_STORE_OBJECTS $git_dir/objects/info/alternates\n            then\n                echo \">>>> ...yes, nothing to do here\" >&2\n                continue\n            else\n                echo \">>>> ...no, doing migration\" >&2\n\n                who=$(find_owning_user $git_dir)\n                if test -z \"$who\"\n                then\n                    echo \">>>>> unable to find who owns $git_dir\" >&2\n                    continue\n                else\n                    echo \">>>>> found that $who owns $git_dir\" >&2\n                fi\n\n                if test \"$DRY_RUN\" = \"1\"\n                then\n                    echo \">>>>>> Would have ran commands migrating $git_dir\"\n                else\n                    if ! sudo -u $who stat $git_dir >/dev/null 2>&1\n                    then\n                        echo \">>>>>> The '$who' user can't access his own '$git_dir'. Could be e.g. ex-employee. Using 'root'\"\n                        who=root\n                    fi\n\n                    echo \">>>>>> Migrating $git_dir is now $(sudo -u $who du -sh $git_dir | cut -f1)\"\n                    sudo -u $who git -C $git_dir config gc.bigPackThreshold 0\n                    echo $ALTERNATES_STORE_OBJECTS | sudo tee -a $git_dir/objects/info/alternates >/dev/null\n                    sudo -u $who git -C $git_dir gc\n                    echo \">>>>>> Migrated $git_dir is now $(sudo -u $who du -sh $git_dir | cut -f1)\"\n                fi\n            fi\n        else\n            echo \">>> No it isn't. Skipping it\" >&2\n            continue\n        fi\n    done\ndone\n"},{"id":"364556","messageId":"c8a2b2c7-e8f1-3da2-cb39-5f7bd22d1818@gmail.com","threadId":"49917","inReplyTo":"87sgzdv2gz.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-12-04T14:17:20Z","receivedAt":"2018-12-04T14:17:28Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/4/2018 8:35 AM, Ævar Arnfjörð Bjarmason wrote:\n>   The leftover space\n> spent is the commit-grah (not de-duped like objects are), and...\n\nThe commit-graph could be shared, as the commits in each enlistment can \nbe parsed from local with GENERATION_NUMBER_INFINITY, giving us similar \nspeedups.\n\nThe issue is: who updates the file? As the commit-graph gets behind, \nperformance will degrade. But it seems like you'd need similar \nmaintenance on the alternate object store, anyway.\n\nThanks,\n-Stolee\n"},{"id":"364593","messageId":"xmqq4lbsejld.fsf@gitster-ct.c.googlers.com","threadId":"49917","inReplyTo":"87tvjtvah0.fsf@evledraar.gmail.com","subject":"Re: How de-duplicate similar repositories with alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T03:30:22Z","receivedAt":"2018-12-05T03:30:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I think a warning (or even error) like this would be more useful:\n>\n>     test ! -d $objdir && error... # current behavior\n>     test -d $objdir/objects && error \"Did you mean $objdir/objects, silly?\" # new error\n\nIf it is an error common enough, perhaps we could even DWIM it, I\nguess, that is...\n\n\tif test ! -d $objdir\n\tthen\n\t\terror\n\telif test -d $objdir/objects/pack\n\tthen\n\t\tpossibly warn\n\t\tobjdir=$objdir/objects\n\tfi\n\n> I.e. I suspect I'm not the only one who's not read the documentation\n> carefully enough and thought it was a path to the root of the repo and\n> wondered why it silently didn't work.\n"},{"id":"364594","messageId":"xmqqzhtkd4sz.fsf@gitster-ct.c.googlers.com","threadId":"49917","inReplyTo":"20181204132716.19208-3-avarab@gmail.com","subject":"Re: [PATCH 2/3] sha1-file: emit error if an alternate looks like a repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T03:35:08Z","receivedAt":"2018-12-05T03:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Since 26125f6b9b (\"detect broken alternates.\", 2006-02-22) we've\n> emitted an error if the alternates directory doesn't exist, but not\n> for the common misstep of adding a path to another git repository as\n> an alternate, as opposed to its \"objects\" directory.\n>\n> Let's check for this, i.e. whether X/objects or X/.git/objects exists\n> if the user supplies X and print an error (which as a commit leading\n> up to this one shows doesn't change the exit code, just \"warns\").\n\nI agree that \"Let's check for this\" is a good idea, but do not\nnecessarily agree with \"i.e.\".  Don't we have a helper that takes\nthe path to an existing directory and answers \"Yup, it does look\nlike a Git repository\"?  Using that is a lot more in line with what\nyou claimed to do in the title for this patch.\n\nI haven't read 3/3 yet, but as I said, I suspect it is reasonable to\nDWIM and use the object store associated with the directory we found\nto be a repository.\n\n"},{"id":"364595","messageId":"xmqqva48d4o9.fsf@gitster-ct.c.googlers.com","threadId":"49917","inReplyTo":"20181204132716.19208-4-avarab@gmail.com","subject":"Re: [PATCH 3/3] sha1-file: change alternate \"error:\" message to \"warning:\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T03:37:58Z","receivedAt":"2018-12-05T03:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change the \"error\" message emitted by alt_odb_usable() to be a\n> \"warning\" instead. As noted in commits leading up to this one this has\n> never impacted the exit code ever since the check was initially added\n> in 26125f6b9b (\"detect broken alternates.\", 2006-02-22).\n>\n> It's confusing to emit an \"error\" when e.g. \"git fsck\" will exit with\n> 0, so let's emit a \"warning:\" instead.\n\nOK, that sounds sensible.  An alternative that may be more sensible\nis to actually diagnose this as an error, but the purpose of this\nmessage is to help users diagnose a possible misconfiguration and\nkeeping the repository \"working\" with the remaining set of object\nstores by leaving it as a mere warning, like this patch does, is\nprobably a better approach.\n\n"},{"id":"364609","messageId":"20181205055415.GA12980@sigill.intra.peff.net","threadId":"49917","inReplyTo":"xmqqva48d4o9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] sha1-file: change alternate \"error:\" message to \"warning:\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-05T05:54:16Z","receivedAt":"2018-12-05T05:54:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 05, 2018 at 12:37:58PM +0900, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> \n> > Change the \"error\" message emitted by alt_odb_usable() to be a\n> > \"warning\" instead. As noted in commits leading up to this one this has\n> > never impacted the exit code ever since the check was initially added\n> > in 26125f6b9b (\"detect broken alternates.\", 2006-02-22).\n> >\n> > It's confusing to emit an \"error\" when e.g. \"git fsck\" will exit with\n> > 0, so let's emit a \"warning:\" instead.\n> \n> OK, that sounds sensible.  An alternative that may be more sensible\n> is to actually diagnose this as an error, but the purpose of this\n> message is to help users diagnose a possible misconfiguration and\n> keeping the repository \"working\" with the remaining set of object\n> stores by leaving it as a mere warning, like this patch does, is\n> probably a better approach.\n\nYeah, I think it's better to keep it as a warning. It's actually\nreasonably likely to be benign (e.g., you did a \"git repack -ad && rm\n/path/to/alternate\" to remove the dependency, but forgot to clean up the\nalternates). And when it _is_ a problem, the object-reading code paths\nwill definitely let you know.\n\n-Peff\n"},{"id":"364612","messageId":"20181205061035.GB12980@sigill.intra.peff.net","threadId":"49917","inReplyTo":"xmqqzhtkd4sz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] sha1-file: emit error if an alternate looks like a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-05T06:10:35Z","receivedAt":"2018-12-05T06:10:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 05, 2018 at 12:35:08PM +0900, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> \n> > Since 26125f6b9b (\"detect broken alternates.\", 2006-02-22) we've\n> > emitted an error if the alternates directory doesn't exist, but not\n> > for the common misstep of adding a path to another git repository as\n> > an alternate, as opposed to its \"objects\" directory.\n> >\n> > Let's check for this, i.e. whether X/objects or X/.git/objects exists\n> > if the user supplies X and print an error (which as a commit leading\n> > up to this one shows doesn't change the exit code, just \"warns\").\n> \n> I agree that \"Let's check for this\" is a good idea, but do not\n> necessarily agree with \"i.e.\".  Don't we have a helper that takes\n> the path to an existing directory and answers \"Yup, it does look\n> like a Git repository\"?  Using that is a lot more in line with what\n> you claimed to do in the title for this patch.\n\nHmm. Yeah, one case this does not handle is when \".git\" is a git-file\npointing elsewhere, which should trigger the condition, too.\n\nI think we can afford to be a bit loose with this check if it's just\ngenerating a warning for a case that would otherwise not work (and the\nworst is that we might fail to correctly diagnose a broken setup). But\nthat I think points to another issue: this kicks in even if the path is\notherwise usable.\n\nSo if had, say, a git repository whose worktree was full of objects and\npackfiles, it currently works for me to point to that as an alternate.\nBut after this patch, we'd complain \"wait, this looks like a git repo!\".\n\nSo I'd much rather see the logic check first for something usable, and\nonly when we fail to find it, start doing a loose diagnosis. Something\nlike:\n\n  if !is_directory($path)\n\tcomplain that it does not exist, as now\n  else if !is_directory($path/pack)\n\t/*\n\t * it doesn't look like an object directory; technically it\n\t * _could_ just have loose objects, and maybe we ought to check\n\t * for directories matching [0-9a-f]{2}, though it seems\n\t * somewhat unlikely these days.\n\t */\n\tif is_directory($path/objects) || exists($path/.git)\n\t\tcomplain that it looks like a git dir\n\telse\n\t\tcomplain that it doesn't look like an object dir\n  fi\n\nHmm. I managed to write a gross mix of C and shell for my pseudocode,\nbut hopefully you can read it. ;)\n\n> I haven't read 3/3 yet, but as I said, I suspect it is reasonable to\n> DWIM and use the object store associated with the directory we found\n> to be a repository.\n\nYeah, I'm tempted by that, too, though I worry about corner cases. How\nmuch effort should we put into discovery? I guess the rules from\nenter_repo() would make sense, though the logic in that function would\nneed some refactoring to be reused elsewhere.\n\n-Peff\n"},{"id":"372644","messageId":"20190328200456.29240-1-avarab@gmail.com","threadId":"49917","inReplyTo":"20181204132716.19208-2-avarab@gmail.com","subject":"[PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T20:04:56Z","receivedAt":"2019-03-28T20:05:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a test for the error() case in alt_odb_usable() where an alternate\ndirectory doesn't exist. This behavior has been the same since\n26125f6b9b (\"detect broken alternates.\", 2006-02-22), but if that\nerror() was turned into die() the entire test suite would still pass.\n\nPerhaps we should die() in that case, but let's start by adding a test\nhere to assert the long-standing existing behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nUnchanged from a 3-part series I submitted in December:\nhttps://public-inbox.org/git/20181204132716.19208-1-avarab@gmail.com/\n\nPart of trying to re-submit the uncontentious parts of whatever I've\nsubmitted in the past that I have stalled for whatever reason...\n\n t/t5613-info-alternate.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 895f46bb91..d2964c57b7 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -136,4 +136,11 @@ test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n \ttest_cmp expect actual.alternates\n '\n \n+test_expect_success 'print \"error\" on non-existing alternate' '\n+\tgit init --bare I &&\n+\techo DOES_NOT_EXIST >I/objects/info/alternates &&\n+\tgit -C I fsck 2>stderr &&\n+\ttest_i18ngrep \"does not exist; check\" stderr\n+'\n+\n test_done\n-- \n2.21.0.392.gf8f6787159e\n\n"},{"id":"372700","messageId":"20190329134603.GB21802@sigill.intra.peff.net","threadId":"49917","inReplyTo":"20190328200456.29240-1-avarab@gmail.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-29T13:46:04Z","receivedAt":"2019-03-29T13:46:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2019 at 09:04:56PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Add a test for the error() case in alt_odb_usable() where an alternate\n> directory doesn't exist. This behavior has been the same since\n> 26125f6b9b (\"detect broken alternates.\", 2006-02-22), but if that\n> error() was turned into die() the entire test suite would still pass.\n>\n> Perhaps we should die() in that case, but let's start by adding a test\n> here to assert the long-standing existing behavior.\n\nI think if anything we might go the other direction, and downgrade the\nerror() to a warning() or even omit it entirely. It's not an error to\nhave a missing or transient alternate. Unless of course it has objects\nyou need, but then those generate their own errors.\n\nI actually think in an ideal world we wouldn't say anything at all about\nalternates which aren't present, don't appear to contain objects, etc,\non their own. And then when we hit an error because an object is\nmissing, only _then_ diagnose and say \"hey, you have this alternate but\nit doesn't have anything in it. Maybe that's an error?\". Doing that\ndiagnosis in the error path helps in two ways:\n\n  - we don't have to worry about it being slow\n\n  - we can be a bit more loose about things that _might_ be an issue.\n    E.g., it's not an error to point to an alternate directory that has\n    no files in it. It might be a misconfiguration, or it might just not\n    have any objects right now. It's hard to justify complaining about\n    it in _every_ git command that loads alternates. But after hitting a\n    fatal error due to a missing object, it seems like a convenient\n    thing to mention to the user.\n\nI suspect that implementing it that way might be a pain, though. Even if\nwe had a convenient diagnose_missing_object() one-liner, there are\nprobably dozens of separate places it would need to be called from.\n\n> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n> index 895f46bb91..d2964c57b7 100755\n> --- a/t/t5613-info-alternate.sh\n> +++ b/t/t5613-info-alternate.sh\n> @@ -136,4 +136,11 @@ test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n>  \ttest_cmp expect actual.alternates\n>  '\n>  \n> +test_expect_success 'print \"error\" on non-existing alternate' '\n> +\tgit init --bare I &&\n> +\techo DOES_NOT_EXIST >I/objects/info/alternates &&\n> +\tgit -C I fsck 2>stderr &&\n> +\ttest_i18ngrep \"does not exist; check\" stderr\n> +'\n\nAll that said, I don't really have an objection against this patch,\nsince it's just testing the current behavior. Anybody who wants to\nchange it would find it pretty easy to tweak this test, too.\n\n-Peff\n"},{"id":"372702","messageId":"87pnq9aipl.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"20190329134603.GB21802@sigill.intra.peff.net","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-29T13:55:18Z","receivedAt":"2019-03-29T13:55:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 29 2019, Jeff King wrote:\n\n> On Thu, Mar 28, 2019 at 09:04:56PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Add a test for the error() case in alt_odb_usable() where an alternate\n>> directory doesn't exist. This behavior has been the same since\n>> 26125f6b9b (\"detect broken alternates.\", 2006-02-22), but if that\n>> error() was turned into die() the entire test suite would still pass.\n>>\n>> Perhaps we should die() in that case, but let's start by adding a test\n>> here to assert the long-standing existing behavior.\n>\n> I think if anything we might go the other direction, and downgrade the\n> error() to a warning() or even omit it entirely. It's not an error to\n> have a missing or transient alternate. Unless of course it has objects\n> you need, but then those generate their own errors.\n\nYeah that sounds fine. FWIW it's just an \"error\" in the sense of being\nprinted out by error(), but we proceed, so it's really a warning,\nsort-of.\n\n> I actually think in an ideal world we wouldn't say anything at all about\n> alternates which aren't present, don't appear to contain objects, etc,\n> on their own. And then when we hit an error because an object is\n> missing, only _then_ diagnose and say \"hey, you have this alternate but\n> it doesn't have anything in it. Maybe that's an error?\". Doing that\n> diagnosis in the error path helps in two ways:\n>\n>   - we don't have to worry about it being slow\n>\n>   - we can be a bit more loose about things that _might_ be an issue.\n>     E.g., it's not an error to point to an alternate directory that has\n>     no files in it. It might be a misconfiguration, or it might just not\n>     have any objects right now. It's hard to justify complaining about\n>     it in _every_ git command that loads alternates. But after hitting a\n>     fatal error due to a missing object, it seems like a convenient\n>     thing to mention to the user.\n>\n> I suspect that implementing it that way might be a pain, though. Even if\n> we had a convenient diagnose_missing_object() one-liner, there are\n> probably dozens of separate places it would need to be called from.\n>\n>> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n>> index 895f46bb91..d2964c57b7 100755\n>> --- a/t/t5613-info-alternate.sh\n>> +++ b/t/t5613-info-alternate.sh\n>> @@ -136,4 +136,11 @@ test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n>>  \ttest_cmp expect actual.alternates\n>>  '\n>>\n>> +test_expect_success 'print \"error\" on non-existing alternate' '\n>> +\tgit init --bare I &&\n>> +\techo DOES_NOT_EXIST >I/objects/info/alternates &&\n>> +\tgit -C I fsck 2>stderr &&\n>> +\ttest_i18ngrep \"does not exist; check\" stderr\n>> +'\n>\n> All that said, I don't really have an objection against this patch,\n> since it's just testing the current behavior. Anybody who wants to\n> change it would find it pretty easy to tweak this test, too.\n\nYup. Just wanted to get the patch to test what we do *currently* out,\nmight loop back to finishing up the rest of this.\n"},{"id":"373428","messageId":"874l78a3rz.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"87pnq9aipl.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-08T15:57:20Z","receivedAt":"2019-04-08T15:57:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 29 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Mar 29 2019, Jeff King wrote:\n>\n>> On Thu, Mar 28, 2019 at 09:04:56PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> Add a test for the error() case in alt_odb_usable() where an alternate\n>>> directory doesn't exist. This behavior has been the same since\n>>> 26125f6b9b (\"detect broken alternates.\", 2006-02-22), but if that\n>>> error() was turned into die() the entire test suite would still pass.\n>>>\n>>> Perhaps we should die() in that case, but let's start by adding a test\n>>> here to assert the long-standing existing behavior.\n>>\n>> I think if anything we might go the other direction, and downgrade the\n>> error() to a warning() or even omit it entirely. It's not an error to\n>> have a missing or transient alternate. Unless of course it has objects\n>> you need, but then those generate their own errors.\n>\n> Yeah that sounds fine. FWIW it's just an \"error\" in the sense of being\n> printed out by error(), but we proceed, so it's really a warning,\n> sort-of.\n>\n>> I actually think in an ideal world we wouldn't say anything at all about\n>> alternates which aren't present, don't appear to contain objects, etc,\n>> on their own. And then when we hit an error because an object is\n>> missing, only _then_ diagnose and say \"hey, you have this alternate but\n>> it doesn't have anything in it. Maybe that's an error?\". Doing that\n>> diagnosis in the error path helps in two ways:\n>>\n>>   - we don't have to worry about it being slow\n>>\n>>   - we can be a bit more loose about things that _might_ be an issue.\n>>     E.g., it's not an error to point to an alternate directory that has\n>>     no files in it. It might be a misconfiguration, or it might just not\n>>     have any objects right now. It's hard to justify complaining about\n>>     it in _every_ git command that loads alternates. But after hitting a\n>>     fatal error due to a missing object, it seems like a convenient\n>>     thing to mention to the user.\n>>\n>> I suspect that implementing it that way might be a pain, though. Even if\n>> we had a convenient diagnose_missing_object() one-liner, there are\n>> probably dozens of separate places it would need to be called from.\n>>\n>>> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\n>>> index 895f46bb91..d2964c57b7 100755\n>>> --- a/t/t5613-info-alternate.sh\n>>> +++ b/t/t5613-info-alternate.sh\n>>> @@ -136,4 +136,11 @@ test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '\n>>>  \ttest_cmp expect actual.alternates\n>>>  '\n>>>\n>>> +test_expect_success 'print \"error\" on non-existing alternate' '\n>>> +\tgit init --bare I &&\n>>> +\techo DOES_NOT_EXIST >I/objects/info/alternates &&\n>>> +\tgit -C I fsck 2>stderr &&\n>>> +\ttest_i18ngrep \"does not exist; check\" stderr\n>>> +'\n>>\n>> All that said, I don't really have an objection against this patch,\n>> since it's just testing the current behavior. Anybody who wants to\n>> change it would find it pretty easy to tweak this test, too.\n>\n> Yup. Just wanted to get the patch to test what we do *currently* out,\n> might loop back to finishing up the rest of this.\n\nJunio: *ping* about picking up this trivial test coverage improvement\n(missed in the latest What's Cooking).\n"},{"id":"373468","messageId":"xmqqimvn7fnt.fsf@gitster-ct.c.googlers.com","threadId":"49917","inReplyTo":"874l78a3rz.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T08:21:10Z","receivedAt":"2019-04-09T08:21:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> Yup. Just wanted to get the patch to test what we do *currently* out,\n>> might loop back to finishing up the rest of this.\n>\n> Junio: *ping* about picking up this trivial test coverage improvement\n\nI was waiting for you to make up your mind about your earlier \"might\nloop back to finishing up\".  For a topic like this, for which the\nball has been in your court for a long time, just resending would be\neasier to handle than pinging, as I do not remember much about the\noriginal attempt anyway ;-)\n\n\n\n"},{"id":"373469","messageId":"xmqqef6b7fa4.fsf@gitster-ct.c.googlers.com","threadId":"49917","inReplyTo":"874l78a3rz.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T08:29:23Z","receivedAt":"2019-04-09T08:29:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>>> +test_expect_success 'print \"error\" on non-existing alternate' '\n>>>> +\tgit init --bare I &&\n>>>> +\techo DOES_NOT_EXIST >I/objects/info/alternates &&\n>>>> +\tgit -C I fsck 2>stderr &&\n>>>> +\ttest_i18ngrep \"does not exist; check\" stderr\n>>>> +'\n>>>\n>>> All that said, I don't really have an objection against this patch,\n>>> since it's just testing the current behavior. Anybody who wants to\n>>> change it would find it pretty easy to tweak this test, too.\n>>\n>> Yup. Just wanted to get the patch to test what we do *currently* out,\n>> might loop back to finishing up the rest of this.\n>\n> Junio: *ping* about picking up this trivial test coverage improvement\n> (missed in the latest What's Cooking).\n\nI went back to the original thread to see what the patch was about.\n\nI actually am fairly negative on casting the current behaviour in\nstone for this one.  Isn't the current status something like: we\nnotice that we say 'error', we suspect that we would want to demote\nit to 'warning' or even silence, but this is not too pressing and it\nis OK as long as we do not make things worse by turning it into a\nmore severe error.\n\nI do not mind if the last \"look in stderr\" step did not exist,\nthough.  It is good to make sure that fsck does not die when it\nsees a non-existing directory listed in the alternates file.\n"},{"id":"373470","messageId":"8736mra7nq.fsf@evledraar.gmail.com","threadId":"49917","inReplyTo":"xmqqimvn7fnt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-09T08:45:45Z","receivedAt":"2019-04-09T08:45:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 09 2019, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> Yup. Just wanted to get the patch to test what we do *currently* out,\n>>> might loop back to finishing up the rest of this.\n>>\n>> Junio: *ping* about picking up this trivial test coverage improvement\n>\n> I was waiting for you to make up your mind about your earlier \"might\n> loop back to finishing up\".  For a topic like this, for which the\n> ball has been in your court for a long time, just resending would be\n> easier to handle than pinging, as I do not remember much about the\n> original attempt anyway ;-)\n\nI don't know when I'll get to finishing the rest of it.\n\nI'll leave it up to you if you want to queue just the test patch or drop\nit. I figured I'd re-send just that since I figured just fixing the\nblindspot of the current behavior would be a good thing on its own, not\nas an endorsement of the current behavior, just a \"this is the current\nknown behavior\" regression test.\n"},{"id":"373472","messageId":"xmqq5zrn7bua.fsf@gitster-ct.c.googlers.com","threadId":"49917","inReplyTo":"8736mra7nq.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T09:43:41Z","receivedAt":"2019-04-09T09:43:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I'll leave it up to you if you want to queue just the test patch or drop\n\nAs I said in a separate message, I think it is good to make sure\nthat fsck does not crash.  I do not think it is good to grep in its\noutput.\n\n> it. I figured I'd re-send just that since I figured just fixing the\n> blindspot of the current behavior would be a good thing on its own, not\n> as an endorsement of the current behavior, just a \"this is the current\n> known behavior\" regression test.\n\nIf the behaviour is undesirable one, we could document the current\n\"breakage\" with \"test_expect_failure\", whether we plan to fix it\nimmediately.  It is OK if readers cannot tell between a bug that is\nexpected to stay forever with us, or a bug that somebody is actively\nworking on.  \n\nBut unfortunately, there is no separate \"test_merely_documenting\",\nthat is different from \"test_expect_success\", so even if we claim\n\"this is not an endorsement, but is merely documenting the current\nbehaviour\" when we add such a test, there is no way for future\nreaders to tell between the two, short of going back to \"git blame\"\nand seeing the log message.\n\nFor that reason, I do not think it is a good practice to document\nthe \"current behaviour that happens to be\" the same way as \"the\nbehaviour we desire\" in test_expect_success.\n"},{"id":"373489","messageId":"20190409141445.GA7371@sigill.intra.peff.net","threadId":"49917","inReplyTo":"xmqq5zrn7bua.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1-file: test the error behavior of alt_odb_usable()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-09T14:14:45Z","receivedAt":"2019-04-09T14:14:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 09, 2019 at 06:43:41PM +0900, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n> > I'll leave it up to you if you want to queue just the test patch or drop\n> \n> As I said in a separate message, I think it is good to make sure\n> that fsck does not crash.  I do not think it is good to grep in its\n> output.\n\nFWIW, that approach (and your rationale below) makes a lot of sense to\nme for this case.\n\n-Peff\n"}]}