{"thread":{"id":"38092","subject":"http-protocol question","startedAt":"2014-12-02T02:17:43Z","lastAt":"2014-12-02T19:50:51Z","messageCount":14,"participants":["Bryan Turner","Jonathan Nieder","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"252820","messageId":"CAGyf7-HmvvHQZkyLgKAs2rrZTTLFkBa8s828hbS9LedLNb2fWA@mail.gmail.com","threadId":"38092","inReplyTo":null,"subject":"http-protocol question","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2014-12-02T02:17:43Z","receivedAt":"2014-12-02T02:17:43Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"In Documentation/technical/http-protocol.txt, there is the following statement:\n\n\"S: Parse the git-upload-pack request:\n\nVerify all objects in `want` are directly reachable from refs.\n\nThe server MAY walk backwards through history or through\nthe reflog to permit slightly stale requests.\"\n\nIs there actually logic somewhere in Git that does that \"MAY walk\nbackwards\" step? Or is that something another implementation of Git\nmay do but the standard Git does not?\n\nThanks,\nBryan Turner\n"},{"id":"252824","messageId":"20141202033416.GY6527@google.com","threadId":"38092","inReplyTo":"CAGyf7-HmvvHQZkyLgKAs2rrZTTLFkBa8s828hbS9LedLNb2fWA@mail.gmail.com","subject":"Re: http-protocol question","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-02T03:34:16Z","receivedAt":"2014-12-02T03:34:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Bryan,\n\nBryan Turner wrote:\n\n> Is there actually logic somewhere in Git that does that \"MAY walk\n> backwards\" step?\n\nYes. See upload-pack.c::check_non_tip and\nhttp://thread.gmane.org/gmane.comp.version-control.git/178814.\n\nHope that helps,\nJonathan\n"},{"id":"252825","messageId":"CAGyf7-GZbRBN4O_yNgrmJCya=6f8XQ4O8m7WBa1k8Ve196ajYQ@mail.gmail.com","threadId":"38092","inReplyTo":"20141202033416.GY6527@google.com","subject":"Re: http-protocol question","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2014-12-02T04:28:09Z","receivedAt":"2014-12-02T04:28:09Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Tue, Dec 2, 2014 at 2:34 PM, Jonathan Nieder wrote:\n> Hi Bryan,\n>\n> Bryan Turner wrote:\n>\n>> Is there actually logic somewhere in Git that does that \"MAY walk\n>> backwards\" step?\n>\n> Yes. See upload-pack.c::check_non_tip and\n> http://thread.gmane.org/gmane.comp.version-control.git/178814.\n\nJonathan,\n\nThanks for the reply! I realize now I didn't really cite the part of\nthat documentation that I'm most interested in, which is: \"through\nhistory _or through the reflog_\". It's the reflog bit I'm looking for.\nSorry for not being more clear. check_non_tip appears to look at\nancestry, walking back up (or down, depending on your view) the DAG to\nsee if the requested SHA-1 is reachable from a current ref, so it\nlooks like that's covering the \"through history\" part of that blurb.\n\nThe reason I ask is that there is a race condition that exists where\nthe ref advertisement lists refs/heads/foo at abc1234, and then foo is\ndeleted before the actual upload-pack request comes in. In that\nsituation, walking backwards through _history_ will only allow the\nupload-pack to complete unless the deleted ref was merged into another\nref prior to being deleted (if I understand the code correctly). It\nseems like looking at the reflogs, and seeing that refs/heads/foo did\nin fact exist and did in fact refer to abc1234, is the only approach\nthat could allow the upload-pack to complete. The documentation hints\nthat such a thing is possible, but I don't see any code in Git that\nseems to do that.\n\nDoes that make more sense? Does that functionality exist and I've just\noverlooked it?\n\nThanks again,\nBryan Turner\n\n>\n> Hope that helps,\n> Jonathan\n"},{"id":"252826","messageId":"CAGyf7-GpPiMCj8=ewjVGv5Hkip__ysR_gs20rG4dTvDbEdd2RQ@mail.gmail.com","threadId":"38092","inReplyTo":"CAGyf7-GZbRBN4O_yNgrmJCya=6f8XQ4O8m7WBa1k8Ve196ajYQ@mail.gmail.com","subject":"Re: http-protocol question","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2014-12-02T04:29:43Z","receivedAt":"2014-12-02T04:29:43Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Tue, Dec 2, 2014 at 3:28 PM, Bryan Turner wrote:\n> On Tue, Dec 2, 2014 at 2:34 PM, Jonathan Nieder wrote:\n>> Hi Bryan,\n>>\n>> Bryan Turner wrote:\n>>\n>>> Is there actually logic somewhere in Git that does that \"MAY walk\n>>> backwards\" step?\n>>\n>> Yes. See upload-pack.c::check_non_tip and\n>> http://thread.gmane.org/gmane.comp.version-control.git/178814.\n>\n> Jonathan,\n>\n> Thanks for the reply! I realize now I didn't really cite the part of\n> that documentation that I'm most interested in, which is: \"through\n> history _or through the reflog_\". It's the reflog bit I'm looking for.\n> Sorry for not being more clear. check_non_tip appears to look at\n> ancestry, walking back up (or down, depending on your view) the DAG to\n> see if the requested SHA-1 is reachable from a current ref, so it\n> looks like that's covering the \"through history\" part of that blurb.\n>\n> The reason I ask is that there is a race condition that exists where\n> the ref advertisement lists refs/heads/foo at abc1234, and then foo is\n> deleted before the actual upload-pack request comes in. In that\n> situation, walking backwards through _history_ will only allow the\n> upload-pack to complete unless the deleted ref was merged into another\n\ns/unless/if, sorry. \"In that situation, walking backwards through\nhistory will only allow the upload-pack to complete if the deleted ref\nwas merged into another ref.\"\n\n> ref prior to being deleted (if I understand the code correctly). It\n> seems like looking at the reflogs, and seeing that refs/heads/foo did\n> in fact exist and did in fact refer to abc1234, is the only approach\n> that could allow the upload-pack to complete. The documentation hints\n> that such a thing is possible, but I don't see any code in Git that\n> seems to do that.\n>\n> Does that make more sense? Does that functionality exist and I've just\n> overlooked it?\n>\n> Thanks again,\n> Bryan Turner\n>\n>>\n>> Hope that helps,\n>> Jonathan\n"},{"id":"252827","messageId":"20141202044522.GZ6527@google.com","threadId":"38092","inReplyTo":"CAGyf7-GZbRBN4O_yNgrmJCya=6f8XQ4O8m7WBa1k8Ve196ajYQ@mail.gmail.com","subject":"Re: http-protocol question","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-02T04:45:22Z","receivedAt":"2014-12-02T04:45:22Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBryan Turner wrote:\n\n> The reason I ask is that there is a race condition that exists where\n> the ref advertisement lists refs/heads/foo at abc1234, and then foo is\n> deleted before the actual upload-pack request comes in.\n\nCan you say more about the context? For example, was this actually\nhappening, or is this for the sake of understanding the protocol\nbetter?\n\nI ask because knowing the context might help us give more specific\nadvice.\n\nSometimes when people delete a branch they really want those objects\nto be inaccessible *right away*. So for such people, git's behavior\nof failing the request unless the objects are still accessible by\nsome other path is helpful.\n\nA stateful server could theoretically cache the list of objects they\nhave advertised for a short time, to avoid clients having to suffer\nwhen something becomes inaccessible during the window between the\nupload-pack advertisement and upload-pack request. Or a permissive\nserver can allow all wants except a specific blacklist (and some\npeople do that).\n\nJonathan\n"},{"id":"252828","messageId":"CAGyf7-Gx1VU-1OicCHG0sStUnNXy_0Y8VYUP+PZjpN6nz7dTrw@mail.gmail.com","threadId":"38092","inReplyTo":"20141202044522.GZ6527@google.com","subject":"Re: http-protocol question","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2014-12-02T05:04:11Z","receivedAt":"2014-12-02T05:04:11Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Tue, Dec 2, 2014 at 3:45 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Bryan Turner wrote:\n>\n>> The reason I ask is that there is a race condition that exists where\n>> the ref advertisement lists refs/heads/foo at abc1234, and then foo is\n>> deleted before the actual upload-pack request comes in.\n>\n> Can you say more about the context? For example, was this actually\n> happening, or is this for the sake of understanding the protocol\n> better?\n\nI ask because it's actually happening. Heavy CI load sometimes has\nbuilds fail because git clone dies with \"not our ref\". That's the\nspecific context I'm working to try and address right now. Some teams\nuse rebase-heavy workflows, which also evades the check_non_tip easing\nand fails with \"not our ref\", so I can't be 100% certain it's ref\ndeletion in specific causing it (but I guess which of those it is is\nprobably largely academic; as long as hosting spans multiple requests\nit seems like this type of race condition is unavoidable).\n\nI'm trying to decide if there is something I can enable or tune to\nprevent it, and the type of twilighting hinted at by the http-protocol\ndocumentation seemed like it could be just the thing.\n\n>\n> I ask because knowing the context might help us give more specific\n> advice.\n>\n> Sometimes when people delete a branch they really want those objects\n> to be inaccessible *right away*. So for such people, git's behavior\n> of failing the request unless the objects are still accessible by\n> some other path is helpful.\n>\n> A stateful server could theoretically cache the list of objects they\n> have advertised for a short time, to avoid clients having to suffer\n> when something becomes inaccessible during the window between the\n> upload-pack advertisement and upload-pack request. Or a permissive\n> server can allow all wants except a specific blacklist (and some\n> people do that).\n>\n> Jonathan\n"},{"id":"252830","messageId":"20141202051755.GA6527@google.com","threadId":"38092","inReplyTo":"CAGyf7-Gx1VU-1OicCHG0sStUnNXy_0Y8VYUP+PZjpN6nz7dTrw@mail.gmail.com","subject":"Re: http-protocol question","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-02T05:17:55Z","receivedAt":"2014-12-02T05:17:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Bryan Turner wrote:\n\n> I ask because it's actually happening. Heavy CI load sometimes has\n> builds fail because git clone dies with \"not our ref\". That's the\n> specific context I'm working to try and address right now.\n\nThanks --- that makes sense.\n\n> Some teams\n> use rebase-heavy workflows, which also evades the check_non_tip easing\n> and fails with \"not our ref\", so I can't be 100% certain it's ref\n> deletion in specific causing it (but I guess which of those it is is\n> probably largely academic; as long as hosting spans multiple requests\n> it seems like this type of race condition is unavoidable).\n\nYeah, everything mentioned before about ref deletion also applies to\nnon-fast-forward updates.\n\n> I'm trying to decide if there is something I can enable or tune to\n> prevent it, and the type of twilighting hinted at by the http-protocol\n> documentation seemed like it could be just the thing.\n\nIf you control the side that clones, then just cloning the single branch\nyou are building (with --single-branch and -b) can help.\n\nUsing a bidirectional protocol like git:// or ssh:// (where the ref\nadvertisement and check of wants happen in the same connection) would\navoid the problem we're talking about.\n\nOn the server side, I agree that either mining reflogs or storing\nadvertisements to disk would be a nice way to take care of this.\nNo one has implemented either of those, but it would be a nice setting\nto have. :)\n\nThanks,\nJonathan\n"},{"id":"252831","messageId":"CACsJy8BtjM5CySt-V-bC3V9K1L2VQWT6pp=4fz8y6W_q1E8n6w@mail.gmail.com","threadId":"38092","inReplyTo":"20141202051755.GA6527@google.com","subject":"Re: http-protocol question","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-02T05:30:44Z","receivedAt":"2014-12-02T05:30:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 2, 2014 at 12:17 PM, Jonathan Nieder wrote:\n> On the server side, I agree that either mining reflogs or storing\n> advertisements to disk would be a nice way to take care of this.\n> No one has implemented either of those, but it would be a nice setting\n> to have. :)\n\nand could be dangerous too. If I accidentally add a password file,\nthen delete it (way before a clone is performed), I don't want that\npart of reflog visible to the cloner. Problem is we don't know how far\nwe should look back in reflog unless we keep some state.\n-- \nDuy\n"},{"id":"252832","messageId":"20141202053331.GA31516@peff.net","threadId":"38092","inReplyTo":"CAGyf7-Gx1VU-1OicCHG0sStUnNXy_0Y8VYUP+PZjpN6nz7dTrw@mail.gmail.com","subject":"Re: http-protocol question","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-02T05:33:32Z","receivedAt":"2014-12-02T05:33:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 02, 2014 at 04:04:11PM +1100, Bryan Turner wrote:\n\n> > Can you say more about the context? For example, was this actually\n> > happening, or is this for the sake of understanding the protocol\n> > better?\n> \n> I ask because it's actually happening. Heavy CI load sometimes has\n> builds fail because git clone dies with \"not our ref\". That's the\n> specific context I'm working to try and address right now. Some teams\n> use rebase-heavy workflows, which also evades the check_non_tip easing\n> and fails with \"not our ref\", so I can't be 100% certain it's ref\n> deletion in specific causing it (but I guess which of those it is is\n> probably largely academic; as long as hosting spans multiple requests\n> it seems like this type of race condition is unavoidable).\n\nThere is a practical reason to care. Ref deletion will also delete the\nreflog, leaving no trace of the reachability. Whereas a non-fast-forward\npush could be resolved by looking in the reflog.\n\nOne problem with hunting for sha1s in the reflog is that upload-pack\ndoes not know which ref the client thinks they are requesting. So a\nsearch would involve looking in _every_ reflog, which could be\nexpensive. It might not be too painful if you do the search only after\nhitting a \"not our ref\" condition, which should in theory be rare. A\nmalicious client could convince you to grep your reflogs repeatedly, but\nthat is hardly the only way to convince upload-pack to chew CPU. Asking\nit to make a pack comes to mind. :)\n\n> I'm trying to decide if there is something I can enable or tune to\n> prevent it, and the type of twilighting hinted at by the http-protocol\n> documentation seemed like it could be just the thing.\n\nFor a public repository, it might make sense to provide a config option\nto loosen the is_our_ref check completely (i.e., to just has_sha1_file).\nBut such an option does not yet exist.\n\n-Peff\n"},{"id":"252833","messageId":"CACsJy8B+b=SyhB3UX648drR2zxrn_3dfGQ9C21sELfABJfa2cQ@mail.gmail.com","threadId":"38092","inReplyTo":"20141202051755.GA6527@google.com","subject":"Re: http-protocol question","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-02T05:37:48Z","receivedAt":"2014-12-02T05:37:48Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 2, 2014 at 12:17 PM, Jonathan Nieder wrote:\n>> I'm trying to decide if there is something I can enable or tune to\n>> prevent it, and the type of twilighting hinted at by the http-protocol\n>> documentation seemed like it could be just the thing.\n>\n> If you control the side that clones, then just cloning the single branch\n> you are building (with --single-branch and -b) can help.\n\nOr we could extend this a bit on the server side, if one ref fail, let\nthe clone continue with the remaining refs (and warn loudly to the\nuser).\n-- \nDuy\n"},{"id":"252835","messageId":"CAGyf7-FDRS_zGQSFKhV8UGfN5GjGcyo0yCz8bcjEALYF3oj=Bg@mail.gmail.com","threadId":"38092","inReplyTo":"20141202053331.GA31516@peff.net","subject":"Re: http-protocol question","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2014-12-02T05:47:50Z","receivedAt":"2014-12-02T05:47:50Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Tue, Dec 2, 2014 at 4:33 PM, Jeff King wrote:\n> On Tue, Dec 02, 2014 at 04:04:11PM +1100, Bryan Turner wrote:\n>\n>> > Can you say more about the context? For example, was this actually\n>> > happening, or is this for the sake of understanding the protocol\n>> > better?\n>>\n>> I ask because it's actually happening. Heavy CI load sometimes has\n>> builds fail because git clone dies with \"not our ref\". That's the\n>> specific context I'm working to try and address right now. Some teams\n>> use rebase-heavy workflows, which also evades the check_non_tip easing\n>> and fails with \"not our ref\", so I can't be 100% certain it's ref\n>> deletion in specific causing it (but I guess which of those it is is\n>> probably largely academic; as long as hosting spans multiple requests\n>> it seems like this type of race condition is unavoidable).\n>\n> There is a practical reason to care. Ref deletion will also delete the\n> reflog, leaving no trace of the reachability. Whereas a non-fast-forward\n> push could be resolved by looking in the reflog.\n\nA fair point. I had mistakenly thought that reflogs would survive the\nref's deletion and be \"pruned\" as part of garbage collection, but a\nquick test shows that, as I'm sure you already know, that's not true.\n\nThanks for correcting my mistake!\nBryan Turner\n"},{"id":"252836","messageId":"20141202055228.GA2708@peff.net","threadId":"38092","inReplyTo":"CAGyf7-FDRS_zGQSFKhV8UGfN5GjGcyo0yCz8bcjEALYF3oj=Bg@mail.gmail.com","subject":"Re: http-protocol question","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-02T05:52:29Z","receivedAt":"2014-12-02T05:52:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 02, 2014 at 04:47:50PM +1100, Bryan Turner wrote:\n\n> > There is a practical reason to care. Ref deletion will also delete the\n> > reflog, leaving no trace of the reachability. Whereas a non-fast-forward\n> > push could be resolved by looking in the reflog.\n> \n> A fair point. I had mistakenly thought that reflogs would survive the\n> ref's deletion and be \"pruned\" as part of garbage collection, but a\n> quick test shows that, as I'm sure you already know, that's not true.\n\nI wish it worked that way. Unfortunately there are complications with\nkeeping the old reflogs in place, because they sometimes cause conflicts\nwith new refs being created (e.g., a reflog in \".git/logs/refs/heads/foo\"\nwould prevent \".git/logs/refs/heads/foo/bar\" from being created). I had\nsome patches long ago to try to keep a \"reflog graveyard\" around, but\nthey were quite invasive, and there were some corner cases that caused\nweird errors.\n\nHandling this sort of D/F conflict more gracefully is one of the things\nI'd like to experiment with once we have pluggable ref backends (I think\nwe'll also disallow \"foo/bar\" if \"foo\" exists, but the storage could at\nleast keep the reflogs around).\n\n-Peff\n"},{"id":"252882","messageId":"xmqqzjb6kkt9.fsf@gitster.dls.corp.google.com","threadId":"38092","inReplyTo":"20141202053331.GA31516@peff.net","subject":"Re: http-protocol question","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-02T17:45:06Z","receivedAt":"2014-12-02T17:45:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For a public repository, it might make sense to provide a config option\n> to loosen the is_our_ref check completely (i.e., to just has_sha1_file).\n> But such an option does not yet exist.\n\nIn principle, yes, but that cannot be has_sha1_file(); it has to\nhave a fully connected healthy history behind it.\n"},{"id":"252885","messageId":"20141202195050.GA23461@peff.net","threadId":"38092","inReplyTo":"xmqqzjb6kkt9.fsf@gitster.dls.corp.google.com","subject":"Re: http-protocol question","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-02T19:50:51Z","receivedAt":"2014-12-02T19:50:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 02, 2014 at 09:45:06AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > For a public repository, it might make sense to provide a config option\n> > to loosen the is_our_ref check completely (i.e., to just has_sha1_file).\n> > But such an option does not yet exist.\n> \n> In principle, yes, but that cannot be has_sha1_file(); it has to\n> have a fully connected healthy history behind it.\n\nI thought about that, but I wonder if it matters whether we check up\nfront. We are not advertising it, but only trying our best to fulfill\nthe \"want\" from the other side. So either:\n\n 1. We check immediately whether we have the full history behind it,\n and reject the \"want\" otherwise.\n\n 2. We start pack-objects on it, and the first thing it will do is\n collect the full set of objects to send. If it doesn't have them,\n it will barf.\n\nProbably (1) would produce nicer error messages, but (2) is a lot more\nefficient.\n\nI dunno. I am not volunteering to implement, so I will leave thinking on\nit more to somebody who wants to do so. :)\n\n-Peff\n"}]}