{"thread":{"id":"39637","subject":"Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks","startedAt":"2015-06-15T19:49:36Z","lastAt":"2015-06-16T14:31:23Z","messageCount":5,"participants":["Brian Vandenberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"263895","messageId":"CAEJ-0i-BB=5W-Ah3nkeLFCe5-4XW4CvYaDgdqv1mMp4UGF81qw@mail.gmail.com","threadId":"39637","inReplyTo":null,"subject":"Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks","fromName":"Brian Vandenberg","fromEmail":"phantall@gmail.com","sentAt":"2015-06-15T19:49:36Z","receivedAt":"2015-06-15T19:49:36Z","isPatch":true,"sender":{"key":"phantall@gmail.com","avatar":null},"body":"From: Junio C Hamano \nDate: 2006-10-08 15:00:32\nPetr Baudis writes:\n\n>> Someone raised a concern that the update and post-update hooks are not\n>> invoked at fetch time in the similar way as they are invoked at push\n>> time, and the idea sort of makes sense. But this patch goes further - it\n>> makes Git invoke those hooks each time a ref is updated in a repository\n>> using the git-update-ref command, which I believe makes a lot of sense as\n>> well - the behaviour is consistent with the current pushing behaviour\n>> and you suddenly finally get a hook where you can properly notify even\n>> about fast-forwards etc.\n\n>In principle I do not have problem with this approach per-se,\n>but I wonder if we were to do this we might want to make\n>receive-pack.c::update() and cmd_update_ref() call the same\n>underlying function, and make that underlying function implement\n>this \"ask the hook if updating is ok\" dance. It might even make\n>sense to have update-ref honor deny_non_fast_forwards for that\n>matter (I am mildly doubtful of this last point, though).\n\nWas this rejected?\n\nI'm tweaking the configuration on our master repo; among other things:\nI've enabled reflog and disabled altering history or deleting branches\nthrough receive-pack, instead requiring that someone make history\nalterations directly (using update-ref).\n\nupdate-ref can optionally accept a comment explaining why the history\nneeded to be altered, but we would prefer to make it mandatory. Hooks\nseem like a natural place to do that, but after running update-ref\nunder strace/truss & looking through builtin/update-ref.c, it looks as\nthough it doesn't make use of hooks.\n\n-Brian\n"},{"id":"263899","messageId":"xmqqpp4w7mco.fsf@gitster.dls.corp.google.com","threadId":"39637","inReplyTo":"CAEJ-0i-BB=5W-Ah3nkeLFCe5-4XW4CvYaDgdqv1mMp4UGF81qw@mail.gmail.com","subject":"Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T20:22:47Z","receivedAt":"2015-06-15T20:22:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Vandenberg writes:\n\n> From: Junio C Hamano \n> Date: 2006-10-08 15:00:32\n> Petr Baudis writes:\n>\n>>> Someone raised a concern that the update and post-update hooks are not\n>>> invoked at fetch time in the similar way as they are invoked at push\n>>> time, and the idea sort of makes sense. But this patch goes further - it\n>>> makes Git invoke those hooks each time a ref is updated in a repository\n>>> using the git-update-ref command, which I believe makes a lot of sense as\n>>> well - the behaviour is consistent with the current pushing behaviour\n>>> and you suddenly finally get a hook where you can properly notify even\n>>> about fast-forwards etc.\n>\n>>In principle I do not have problem with this approach per-se,\n>>but I wonder if we were to do this we might want to make\n>>receive-pack.c::update() and cmd_update_ref() call the same\n>>underlying function, and make that underlying function implement\n>>this \"ask the hook if updating is ok\" dance. It might even make\n>>sense to have update-ref honor deny_non_fast_forwards for that\n>>matter (I am mildly doubtful of this last point, though).\n\nI am ultra-doubtful at this point ;-)\n\nFor one thing, update-ref is not a tool that is exclusive to\nreceiving object transfer aka receive-pack, so it makes no sense for\nit to pay attention to post-update.\n\nAlso it is a low-level plumbing; the policy issues should come at a\nlevel higher than that. I.e. Porcelain scripts implemented using\nthem as building blocks should be the ones that you do your policy,\ne.g.\n\n\tif whatever logic you want to use in your policy says OK\n then\n\t\tgit update-ref ...args...\n\telse\n \techo >&2 \"My policy does not like your arguments\"\n exit 1\n\tfi\n"},{"id":"263904","messageId":"CAEJ-0i8mfabWoLivX_ZJHRJBYFftoENWmHw5__CGphK54oOpug@mail.gmail.com","threadId":"39637","inReplyTo":"xmqqpp4w7mco.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks","fromName":"Brian Vandenberg","fromEmail":"phantall@gmail.com","sentAt":"2015-06-15T20:58:35Z","receivedAt":"2015-06-15T20:58:35Z","isPatch":true,"sender":{"key":"phantall@gmail.com","avatar":null},"body":"On Mon, Jun 15, 2015 at 2:22 PM, Junio C Hamano wrote:\n> I am ultra-doubtful at this point ;-)\n>\n> For one thing, update-ref is not a tool that is exclusive to\n> receiving object transfer aka receive-pack, so it makes no sense for\n> it to pay attention to post-update.\n\nI was a little dubious about that as well, though I like the idea of\nupdate-ref having a hook.\n\n> Also it is a low-level plumbing; the policy issues should come at a\n> level higher than that. I.e. Porcelain scripts implemented using\n> them as building blocks should be the ones that you do your policy,\n> e.g.\n>\n> if whatever logic you want to use in your policy says OK\n> then\n> git update-ref ...args...\n> else\n> echo >&2 \"My policy does not like your arguments\"\n> exit 1\n> fi\n>\n\nIn principle I agree, though telling people \"you should use xyz\nscript\" is one thing; in practice they're more likely to google \"git\nalter history\" and eventually find an explanation for using\nupdate-ref, completely bypassing any policy/protocol we put in place\nfor altering history on the master repo.\n\nWould you be less doubtful about adding a lower-level hook for\nupdate-ref? Or in lieu of that, a config option that can affect the\nbehavior of its \"-m\" and \"-d\" options?\n\n-Brian\n"},{"id":"263911","messageId":"xmqqd20w7ii4.fsf@gitster.dls.corp.google.com","threadId":"39637","inReplyTo":"CAEJ-0i8mfabWoLivX_ZJHRJBYFftoENWmHw5__CGphK54oOpug@mail.gmail.com","subject":"Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T21:45:55Z","receivedAt":"2015-06-15T21:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Vandenberg writes:\n\n> Would you be less doubtful about adding a lower-level hook for\n> update-ref? Or in lieu of that, a config option that can affect the\n> behavior of its \"-m\" and \"-d\" options?\n\nNot really.\n\nThose who want to bypass your policy can use \"vi .git/packed-refs\"\nand/or \"rm .git/refs/heads/foo\"; you would not propose to patch \"rm\"\nand \"vi\" to pay attention to git configuration, because they are not\nabout \"git\" at all, and there are cases where these bare-metal level\nediting of repositories is necessary. You would instead tell them\n\"don't do that\".\n\nWhen I say \"update-ref is a low-level plumbing command that should\nnot enforce policy\", I am suggesting you to treat the command just\nlike you would treat \"rm\" and \"vi\".\n"},{"id":"263942","messageId":"CAEJ-0i-QptjYLtr0CnYOgt4X+2R5tQGZqRhicFdxAPBxNA8J7Q@mail.gmail.com","threadId":"39637","inReplyTo":"xmqqd20w7ii4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks","fromName":"Brian Vandenberg","fromEmail":"phantall@gmail.com","sentAt":"2015-06-16T14:31:23Z","receivedAt":"2015-06-16T14:31:23Z","isPatch":true,"sender":{"key":"phantall@gmail.com","avatar":null},"body":"On Mon, Jun 15, 2015 at 3:45 PM, Junio C Hamano wrote:\n> Those who want to bypass your policy can use \"vi .git/packed-refs\"\n> and/or \"rm .git/refs/heads/foo\"; you would not propose to patch \"rm\"\n> and \"vi\" to pay attention to git configuration, because they are not\n> about \"git\" at all, and there are cases where these bare-metal level\n> editing of repositories is necessary. You would instead tell them\n> \"don't do that\".\n>\n> When I say \"update-ref is a low-level plumbing command that should\n> not enforce policy\", I am suggesting you to treat the command just\n> like you would treat \"rm\" and \"vi\".\n\nI suppose I see your point, though the line they've crossed at that\npoint is from benign to malicious.\n\nWhen you assume they've crossed that line, hooks and configuration\nparameters are mostly useless in other git commands as well. My\nassumption here is they want to do something productive and aren't\nintentionally seeking to subvert policies, but rather aren't aware of\nthe policies and/or correct procedure.\n\nThanks for your responses, I appreciate your time.\n\n-Brian\n"}]}