{"thread":{"id":"47783","subject":"Fetch-hooks","startedAt":"2018-02-07T22:03:28Z","lastAt":"2018-02-20T21:19:56Z","messageCount":38,"participants":["Leo Gaspard","Ævar Arnfjörð Bjarmason","Joey Hess","Jeff King","Junio C Hamano","Brandon Williams","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"338671","messageId":"5898be69-4211-d441-494d-93477179cf0e@gaspard.io","threadId":"47783","inReplyTo":null,"subject":"Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-07T21:56:41Z","receivedAt":"2018-02-07T22:03:28Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"Hello,\n\ntl;dr: Is there currently a way to have fetch hooks, and if not do you\nthink it could be a nice feature?\n\nI was in the process of implementing hooks for git that ensure the\nrepository is always cleanly signed by someone allowed to by the\nrepository itself. I think I've completed the signature-checking part\n[1] and the push hook [2] (even though it isn't really configurable at\nthe moment).\n\nHowever, I was starting to think about handling the fetch step, and\ncouldn't find any fetch hook. Is there one?\n\nIf not, would you think it is would be a good idea to add one, that\nwould eg. be passed the commit-before, commit-after and could block the\nchanging of the reference if it failed?\n\nThe only other solution I could think of is using a separate script for\nfetching, but that would be fragile, as the user could always not think\nabout it well and run a git fetch, breaking the objective that after the\nfirst clone all commits were correctly signature-checked.\n\nThanks for reading me!\nLeo\n\nPS1: I am not subscribed to the ML.\n\nPS2: I've tried asking freenode#git, without success so far.\n\n\n[1]\nhttps://github.com/Ekleog/signed-git/blob/master/git-hooks/check-range-signed.sh\n\n[2] https://github.com/Ekleog/signed-git/blob/master/git-hooks/pre-push\n"},{"id":"338676","messageId":"87inb8mn0w.fsf@evledraar.gmail.com","threadId":"47783","inReplyTo":"5898be69-4211-d441-494d-93477179cf0e@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-07T22:51:27Z","receivedAt":"2018-02-07T22:51:35Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 07 2018, Leo Gaspard jotted:\n\n> Hello,\n>\n> tl;dr: Is there currently a way to have fetch hooks, and if not do you\n> think it could be a nice feature?\n>\n> I was in the process of implementing hooks for git that ensure the\n> repository is always cleanly signed by someone allowed to by the\n> repository itself. I think I've completed the signature-checking part\n> [1] and the push hook [2] (even though it isn't really configurable at\n> the moment).\n>\n> However, I was starting to think about handling the fetch step, and\n> couldn't find any fetch hook. Is there one?\n>\n> If not, would you think it is would be a good idea to add one, that\n> would eg. be passed the commit-before, commit-after and could block the\n> changing of the reference if it failed?\n>\n> The only other solution I could think of is using a separate script for\n> fetching, but that would be fragile, as the user could always not think\n> about it well and run a git fetch, breaking the objective that after the\n> first clone all commits were correctly signature-checked.\n>\n> Thanks for reading me!\n> Leo\n>\n> PS1: I am not subscribed to the ML.\n>\n> PS2: I've tried asking freenode#git, without success so far.\n>\n>\n> [1]\n> https://github.com/Ekleog/signed-git/blob/master/git-hooks/check-range-signed.sh\n>\n> [2] https://github.com/Ekleog/signed-git/blob/master/git-hooks/pre-push\n\nThere is no fetch hook, however you may find that the\npost-{checkout,merge} hooks are suitable for what you want to do.\n\nSetting those to some custom comand is a common pattern for\ne.g. compiling some assets on \"git pull\", so you could similarly check\nthe commits from HEAD, of course those are post-* hooks, so they won't\nstop the checkout.\n"},{"id":"338683","messageId":"c8d1eb4d-c3d2-5834-a46b-931e825315aa@gaspard.io","threadId":"47783","inReplyTo":"87inb8mn0w.fsf@evledraar.gmail.com","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-08T00:06:26Z","receivedAt":"2018-02-08T00:06:32Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/07/2018 11:51 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Feb 07 2018, Leo Gaspard jotted:\n> \n>> Hello,\n>>\n>> tl;dr: Is there currently a way to have fetch hooks, and if not do you\n>> think it could be a nice feature?\n>>\n>> I was in the process of implementing hooks for git that ensure the\n>> repository is always cleanly signed by someone allowed to by the\n>> repository itself. I think I've completed the signature-checking part\n>> [1] and the push hook [2] (even though it isn't really configurable at\n>> the moment).\n>>\n>> However, I was starting to think about handling the fetch step, and\n>> couldn't find any fetch hook. Is there one?\n>>\n>> If not, would you think it is would be a good idea to add one, that\n>> would eg. be passed the commit-before, commit-after and could block the\n>> changing of the reference if it failed?\n>>\n>> The only other solution I could think of is using a separate script for\n>> fetching, but that would be fragile, as the user could always not think\n>> about it well and run a git fetch, breaking the objective that after the\n>> first clone all commits were correctly signature-checked.\n>>\n>> Thanks for reading me!\n>> Leo\n>>\n>> PS1: I am not subscribed to the ML.\n>>\n>> PS2: I've tried asking freenode#git, without success so far.\n>>\n>>\n>> [1]\n>> https://github.com/Ekleog/signed-git/blob/master/git-hooks/check-range-signed.sh\n>>\n>> [2] https://github.com/Ekleog/signed-git/blob/master/git-hooks/pre-push\n> \n> There is no fetch hook, however you may find that the\n> post-{checkout,merge} hooks are suitable for what you want to do.\n> \n> Setting those to some custom comand is a common pattern for\n> e.g. compiling some assets on \"git pull\", so you could similarly check\n> the commits from HEAD, of course those are post-* hooks, so they won't\n> stop the checkout.\n\nHmm, I don't think these would fit the bill. For post-merge, simply\nbecause I spend my life rebasing stuff around, and very rarely merge.\nFor post-checkout, it could work, but then I'd need to keep track\nmanually of up to where the commits have been checked and to search the\ngit graph for the latest checked ancestor (as otherwise checking-out\nanother branch then checking-out the first branch again would likely\ntrigger a failure, due to the keyring being dynamic), so it would likely\nbe a dealbreaker, due to the hook becoming too complex to be trusted.\n\n(Just in case you wonder, by “the keyring being dynamic” I mean the PGP\nkeys allowed to sign commits are stored directly inside the git repository)\n\nThat said, I just came upon [1] (esp. the description [2] and the patch\n[3]), and wondered: it looks like the patch was abandoned midway in\nfavor of a hook refactoring. Would you happen to know whether the hook\nrefactoring eventually took place, and/or whether this patch was\nresubmitted later, and/or whether it would still be possible to merge\nthis now? (not having any experience with git's internals yet, I don't\nreally know whether these are stupid questions or not)\n\nThanks!\nLeo\n\nPS: Cc'ing Joey, as you most likely know best what eventually happened,\nif you can remember it?\n\n\n[1] https://marc.info/?t=132477041500001&r=1&w=2\n\n[2] https://marc.info/?l=git&m=132483581218382&w=2\n\n[3] https://marc.info/?l=git&m=132486687023893&w=2\n"},{"id":"338705","messageId":"20180208153040.GA5180@kitenet.net","threadId":"47783","inReplyTo":"c8d1eb4d-c3d2-5834-a46b-931e825315aa@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2018-02-08T15:30:40Z","receivedAt":"2018-02-08T15:38:08Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Leo Gaspard wrote:\n> That said, I just came upon [1] (esp. the description [2] and the patch\n> [3]), and wondered: it looks like the patch was abandoned midway in\n> favor of a hook refactoring. Would you happen to know whether the hook\n> refactoring eventually took place, and/or whether this patch was\n> resubmitted later, and/or whether it would still be possible to merge\n> this now? (not having any experience with git's internals yet, I don't\n> really know whether these are stupid questions or not)\n> \n> PS: Cc'ing Joey, as you most likely know best what eventually happened,\n> if you can remember it?\n\nI don't remember it well, but reviewing the thread, I think it foundered\non this comment by Junio:\n\n> That use case sounds like that \"git fetch\" is called as a first class UI,\n> which is covered by \"git myfetch\" (you can call it \"git annex fetch\")\n> wrapper approach, the canonical example of a hook that we explicitly do\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n> not want to add.\n ^^^^^^^^^^^^^^^\n\nWhile I still think a fetch hook would be a good idea for reasons of\ncomposability, I then just went off and implemented such a wrapper for\nmy own particular use case, and the wrapper program then grew to cover\nuse cases that a hook would not have been able to cover, so ...\n\n-- \nsee shy jo\n"},{"id":"338740","messageId":"871af155-a159-2a29-2e48-74e7a98b60d4@gaspard.io","threadId":"47783","inReplyTo":"20180208153040.GA5180@kitenet.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-08T17:02:42Z","receivedAt":"2018-02-08T17:02:50Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/08/2018 04:30 PM, Joey Hess wrote:\n> Leo Gaspard wrote:\n>> That said, I just came upon [1] (esp. the description [2] and the patch\n>> [3]), and wondered: it looks like the patch was abandoned midway in\n>> favor of a hook refactoring. Would you happen to know whether the hook\n>> refactoring eventually took place, and/or whether this patch was\n>> resubmitted later, and/or whether it would still be possible to merge\n>> this now? (not having any experience with git's internals yet, I don't\n>> really know whether these are stupid questions or not)\n>>\n>> PS: Cc'ing Joey, as you most likely know best what eventually happened,\n>> if you can remember it?\n> \n> I don't remember it well, but reviewing the thread, I think it foundered\n> on this comment by Junio:\n> \n>> That use case sounds like that \"git fetch\" is called as a first class UI,\n>> which is covered by \"git myfetch\" (you can call it \"git annex fetch\")\n>> wrapper approach, the canonical example of a hook that we explicitly do\n> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n>> not want to add.\n> ^^^^^^^^^^^^^^^\n> \n> While I still think a fetch hook would be a good idea for reasons of\n> composability, I then just went off and implemented such a wrapper for\n> my own particular use case, and the wrapper program then grew to cover\n> use cases that a hook would not have been able to cover, so ...\n\nHmm, OK, so I guess I'll try to update the patch when I get some time to\ndelve into git's internals, as my use case (forbidding some fetches)\ncouldn't afaik be covered by a wrapper hook.\n\nThanks for the feedback!\nLeo\n"},{"id":"338775","messageId":"87bmgzmbsk.fsf@evledraar.gmail.com","threadId":"47783","inReplyTo":"871af155-a159-2a29-2e48-74e7a98b60d4@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-08T21:06:19Z","receivedAt":"2018-02-08T21:06:28Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 08 2018, Leo Gaspard jotted:\n\n> On 02/08/2018 04:30 PM, Joey Hess wrote:\n>> Leo Gaspard wrote:\n>>> That said, I just came upon [1] (esp. the description [2] and the patch\n>>> [3]), and wondered: it looks like the patch was abandoned midway in\n>>> favor of a hook refactoring. Would you happen to know whether the hook\n>>> refactoring eventually took place, and/or whether this patch was\n>>> resubmitted later, and/or whether it would still be possible to merge\n>>> this now? (not having any experience with git's internals yet, I don't\n>>> really know whether these are stupid questions or not)\n>>>\n>>> PS: Cc'ing Joey, as you most likely know best what eventually happened,\n>>> if you can remember it?\n>>\n>> I don't remember it well, but reviewing the thread, I think it foundered\n>> on this comment by Junio:\n>>\n>>> That use case sounds like that \"git fetch\" is called as a first class UI,\n>>> which is covered by \"git myfetch\" (you can call it \"git annex fetch\")\n>>> wrapper approach, the canonical example of a hook that we explicitly do\n>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n>>> not want to add.\n>> ^^^^^^^^^^^^^^^\n>>\n>> While I still think a fetch hook would be a good idea for reasons of\n>> composability, I then just went off and implemented such a wrapper for\n>> my own particular use case, and the wrapper program then grew to cover\n>> use cases that a hook would not have been able to cover, so ...\n>\n> Hmm, OK, so I guess I'll try to update the patch when I get some time to\n> delve into git's internals, as my use case (forbidding some fetches)\n> couldn't afaik be covered by a wrapper hook.\n\nPer my reading of\nhttps://public-inbox.org/git/20111224234212.GA21533@gnu.kitenet.net/\nwhat Joey implemented is not what you described in your initial mail.\n\nHis is a *post*-fetch hook, we've already done the fetch and are just\ntelling you as a courtesy what refs changed. You could also implement\nthis as some cronjob that polls git for-each-ref but it's easier as a\nhook, fine.\n\nWhat you're describing is something like a pre-fetch hook analogous to\nthe pre-receive hooks, where you're fed refs updated on the remote on\nstdin, and can say you don't want some of those to be updated.\n\nThis may just be a lack of imagination on my part, but I don't see how\nthat's sensible at all.\n\nThe refs we fetch are our *copy* of the remote refs, why would you not\nwant to track the upstream remote. You're going to refuse some branches\nand what? Be further behind until some point in the future where the tip\nis GPG-signed and you accept it, at which poich you'll need to do more\nwork than if you were up-to-date with the almost-GPG-signed version?\n\nI think you're confusing two things here. One is the reasonable concern\nof wanting to not have your local copy of remote refs have undesirable\ncontent, but a pre-fetch hook is not the way to accomplish that.\n\nThe other is e.g. to ensure that you never locally check out some \"bad\"\nref, we don't have hook support for that, but could add it,\ne.g. git-checkout and git reset --hard could be taught about some\npre-checkout hook.\n\nYou could also have some intermediate step between these two, where\ne.g. your refspec for \"origin\" is\n\"+refs/heads/*:refs/remotes/origin-untrusted/*\" instead of the default\n\"+refs/heads/*:refs/remotes/origin/*\", you fetch all refs to that\nlocation, then you move them (with some alias/hook) to\n\"refs/remotes/origin/*\" once they're seen to be \"OK\".\n"},{"id":"338787","messageId":"fa470be4-75fb-76ed-ed93-5c10fcfb8842@gaspard.io","threadId":"47783","inReplyTo":"87bmgzmbsk.fsf@evledraar.gmail.com","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-08T22:18:54Z","receivedAt":"2018-02-08T22:19:02Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/08/2018 10:06 PM, Ævar Arnfjörð Bjarmason wrote:>> Hmm, OK, so I\nguess I'll try to update the patch when I get some time to\n>> delve into git's internals, as my use case (forbidding some fetches)\n>> couldn't afaik be covered by a wrapper hook.\n> \n> Per my reading of\n> https://public-inbox.org/git/20111224234212.GA21533@gnu.kitenet.net/\n> what Joey implemented is not what you described in your initial mail.\n> \n> His is a *post*-fetch hook, we've already done the fetch and are just\n> telling you as a courtesy what refs changed. You could also implement\n> this as some cronjob that polls git for-each-ref but it's easier as a\n> hook, fine.\n\nI was thinking along the lines of\n https://marc.info/?l=git&m=132486687023893&w=2\nwith high-level description at\n https://marc.info/?l=git&m=132480559712592&w=2\n\nWith the high-level description given here, I'm pretty sure I can hack a\nhook together to make things work as I want them to.\n\n> What you're describing is something like a pre-fetch hook analogous to\n> the pre-receive hooks, where you're fed refs updated on the remote on\n> stdin, and can say you don't want some of those to be updated.\n> \n> This may just be a lack of imagination on my part, but I don't see how\n> that's sensible at all.\n> \n> The refs we fetch are our *copy* of the remote refs, why would you not\n> want to track the upstream remote. You're going to refuse some branches\n> and what? Be further behind until some point in the future where the tip\n> is GPG-signed and you accept it, at which poich you'll need to do more\n> work than if you were up-to-date with the almost-GPG-signed version?\n\nThat's about it. I want all fetching to be blocked in case of the tip\nnot being signed. As there is a pre-push hook ensuring committers don't\nforget to sign before pushing, the only case the tip could not be signed\nis in case of an attack, which means it's better to just force-push\nmaster because any git repo that fetched it is doomed anyway. Definitely\nwould not want to allow an untrusted revision get into anything that\ncould even remotely be taken as “endorsed” by the user.\n\n(BTW, in order to avoid the case of someone forgetting to sign the\ncommit and not having installed the pre-push hook, there can be holes in\nthe commit-signing chain, the drawback being that the committer pushing\na signed commit takes responsibility for all unsigned commits directly\npreceding his -- allowing them to recover in case of a mistaken push)\n\n> I think you're confusing two things here. One is the reasonable concern\n> of wanting to not have your local copy of remote refs have undesirable\n> content, but a pre-fetch hook is not the way to accomplish that.\n\nWell, a pre-fetch hook is a possible way of accomplishing that, and I\ndon't know of any better one?\n\n> The other is e.g. to ensure that you never locally check out some \"bad\"\n> ref, we don't have hook support for that, but could add it,\n> e.g. git-checkout and git reset --hard could be taught about some\n> pre-checkout hook.\n\nIssue is, once we have to fix checkout and reset, all other commands\nthat potentially touch the worktree also have to be fixed (eg. I don't\nknow whether worktree add triggers pre-checkout?)\n\nAlso, this requires the hook to store a database of all the paths that\nhave been checked, because there is no logic in how one may choose to\ncheckout the repo. While having a tweak-fetch hook would make the\nimplementation straightforward, because at the time of invoking the hook\nthe “refname at remote” commit is already trusted, and the “object name”\nis the commit whose validity we want to check, so we just have to check\nthe path between those two. (I don't know if you checked my current\nscripts, but basically as the set of allowed PGP keys can change at any\ncommit, it's only possible to check a commit path, not a single commit\nout-of-nowhere)\n\nThe only issue that could arise with a tweak-fetch hook is in case of a\nforce-fetch (and even maybe it's not even an actual issue, I haven't\ngiven it real thought yet), but this can reasonably be banned, as once a\ncommit is signed it enters the “real” master branch, that should never\nbe moved backward, as it can't be the sign of an attack.\n\n> You could also have some intermediate step between these two, where\n> e.g. your refspec for \"origin\" is\n> \"+refs/heads/*:refs/remotes/origin-untrusted/*\" instead of the default\n> \"+refs/heads/*:refs/remotes/origin/*\", you fetch all refs to that\n> location, then you move them (with some alias/hook) to\n> \"refs/remotes/origin/*\" once they're seen to be \"OK\".\n\nThat is indeed another possibility, but then the idea is to make things\nas transparent as possible for the end-user, not to completely change\ntheir git workflow. As such, considering only signed commits to be part\nof the upstream seems to make sense to me?\n\nCheers,\nLeo\n"},{"id":"338897","messageId":"3a5a2827-0f69-3a11-2664-51a60eefebf1@gaspard.io","threadId":"47783","inReplyTo":"871af155-a159-2a29-2e48-74e7a98b60d4@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-09T19:12:12Z","receivedAt":"2018-02-09T19:12:43Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/08/2018 06:02 PM, Leo Gaspard wrote:\n> On 02/08/2018 04:30 PM, Joey Hess wrote:\n>> [...]\n> \n> Hmm, OK, so I guess I'll try to update the patch when I get some time to\n> delve into git's internals, as my use case (forbidding some fetches)\n> couldn't afaik be covered by a wrapper hook.\n\nJoey,\n\nI just wanted to check, you did not put the Signed-off-by line in\npatches in https://marc.info/?l=git&m=132491485901482&w=2\n\nCould you confirm that the patches you sent are “covered under an\nappropriate open source license and I have the right under that license\nto submit that work with modifications, whether created in whole or in\npart by me, under the same open source license (unless I am permitted to\nsubmit under a different license), as indicated in the file”, so that I\ncould send the patch I wrote based on yours with a Signed-off-by line of\nmy own without breaking the DCO?\n\nThanks!\nLeo\n"},{"id":"338909","messageId":"20180209202044.GA6783@kitenet.net","threadId":"47783","inReplyTo":"3a5a2827-0f69-3a11-2664-51a60eefebf1@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2018-02-09T20:20:44Z","receivedAt":"2018-02-09T20:20:58Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Leo Gaspard wrote:\n> I just wanted to check, you did not put the Signed-off-by line in\n> patches in https://marc.info/?l=git&m=132491485901482&w=2\n> \n> Could you confirm that the patches you sent are “covered under an\n> appropriate open source license and I have the right under that license\n> to submit that work with modifications, whether created in whole or in\n> part by me, under the same open source license (unless I am permitted to\n> submit under a different license), as indicated in the file”, so that I\n> could send the patch I wrote based on yours with a Signed-off-by line of\n> my own without breaking the DCO?\n\nYes; my patches are under the same GPL-2 as the rest of git.\n\n-- \nsee shy jo\n"},{"id":"338938","messageId":"30753d19-d77d-1a1a-ba42-afcd6fbb4223@gaspard.io","threadId":"47783","inReplyTo":"20180209202044.GA6783@kitenet.net","subject":"[PATCH 0/2] fetch: add tweak-fetch hook","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-09T21:28:06Z","receivedAt":"2018-02-09T21:28:16Z","isPatch":true,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/09/2018 09:20 PM, Joey Hess wrote:> Yes; my patches are under the\nsame GPL-2 as the rest of git.\n\nThanks! So here comes my patch series, heavily based on yours.\n\nThere are some things to bear in mind while reviewing it:\n\n * This is my first real attempt at contributing to git, which means I\ncould be very very far off-track\n\n * Most of it is based on trying to make the 6-year-old patch series\nwork and pass all the tests, so if a new feature has been added since\nthen I likely didn't notice it or don't know how to handle it correctly\n\nThere are still three TODO's in the code:\n\n * In the documentation, one stating that I don't really get what this\n“ignore” parameter exactly does, and whether it should be handled\nspecially (a prime example of a new feature I'm not really sure how to\nhandle, somewhere in the code it's written all the “ignore” references\nare at the end of the list, but I'm already not self-confident enough\nabout the difference between “merge” and “not-for-merge” to even\nconsider making a good choice about how to handle “ignore”)\n\n * In `builtins/fetch.c`, function `do_fetch`, there is a conflict of\ninterest between placing the `prune` before the `fetch` (as done by\ncommit 10a6cc889 (\"fetch --prune: Run prune before fetching\",\n2014-01-02)), and placing the `fetch` before the `prune` (which would\nallow hooks that rename the local-ref to not be prune'd and then\nre-fetched when doing a `git fetch --prune` -- without that a hook that\nwould want to both read the old commit information and rename the\nlocal-ref would not be able to). Or maybe this question means actually\nthere should be a third solution? but I don't really know what. Maybe\nalso hooking into the prune operation?\n\n * In `templates/hooks--tweak-fetch.sample`, the “check this actually\nworks” todo, as I'd rather first check this patch series is not too far\noff-topic before doing non-essential work -- anyway another version of\nthe patch series will be required for the other two TODO's, so I can fix\nit at this point.\n\nThat being said, what do you think about these patches?\n\nThanks for your time!\nLeo Gaspard\n"},{"id":"338940","messageId":"20180209214458.16135-1-leo@gaspard.io","threadId":"47783","inReplyTo":"30753d19-d77d-1a1a-ba42-afcd6fbb4223@gaspard.io","subject":"[PATCH 1/2] fetch: preparations for tweak-fetch hook","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-09T21:44:57Z","receivedAt":"2018-02-09T21:45:05Z","isPatch":true,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"From: Léo Gaspard \n\nNo behavior changes yet, only some groundwork for the next change.\n\nThe refs_result structure combines a status code with a ref map, which\ncan be NULL even on success. This will be needed when there's a\ntweak-fetch hook, because it can filter out all refs, while still\nsucceeding.\n\nfetch_refs returns a refs_result, so that it can modify the ref_map.\n\nBased-on-patch-by: Joey Hess \nSigned-off-by: Leo Gaspard \n---\n builtin/fetch.c | 68 +++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 44 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7bbcd26fa..76dc05f61 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -34,6 +34,11 @@ enum {\n \tTAGS_SET = 2\n };\n \n+struct refs_result {\n+\tstruct ref *new_refs;\n+\tint status;\n+};\n+\n static int fetch_prune_config = -1; /* unspecified */\n static int prune = -1; /* unspecified */\n #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */\n@@ -57,6 +62,18 @@ static int shown_url = 0;\n static int refmap_alloc, refmap_nr;\n static const char **refmap_array;\n \n+static int add_existing(const char *refname, const struct object_id *oid,\n+\t\t\tint flag, void *cbdata)\n+{\n+\tstruct string_list *list = (struct string_list *)cbdata;\n+\tstruct string_list_item *item = string_list_insert(list, refname);\n+\tstruct object_id *old_oid = xmalloc(sizeof(*old_oid));\n+\n+\toidcpy(old_oid, oid);\n+\titem->util = old_oid;\n+\treturn 0;\n+}\n+\n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n \tif (!strcmp(k, \"fetch.prune\")) {\n@@ -217,18 +234,6 @@ static void add_merge_config(struct ref **head,\n \t}\n }\n \n-static int add_existing(const char *refname, const struct object_id *oid,\n-\t\t\tint flag, void *cbdata)\n-{\n-\tstruct string_list *list = (struct string_list *)cbdata;\n-\tstruct string_list_item *item = string_list_insert(list, refname);\n-\tstruct object_id *old_oid = xmalloc(sizeof(*old_oid));\n-\n-\toidcpy(old_oid, oid);\n-\titem->util = old_oid;\n-\treturn 0;\n-}\n-\n static int will_fetch(struct ref **head, const unsigned char *sha1)\n {\n \tstruct ref *rm = *head;\n@@ -920,15 +925,20 @@ static int quickfetch(struct ref *ref_map)\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n-static int fetch_refs(struct transport *transport, struct ref *ref_map)\n+static struct refs_result fetch_refs(struct transport *transport,\n+\t\tstruct ref *ref_map)\n {\n-\tint ret = quickfetch(ref_map);\n-\tif (ret)\n-\t\tret = transport_fetch_refs(transport, ref_map);\n-\tif (!ret)\n-\t\tret |= store_updated_refs(transport->url,\n+\tstruct refs_result ret;\n+\tret.status = quickfetch(ref_map);\n+\tif (ret.status) {\n+\t\tret.status = transport_fetch_refs(transport, ref_map);\n+\t}\n+\tif (!ret.status) {\n+\t\tret.new_refs = ref_map;\n+\t\tret.status |= store_updated_refs(transport->url,\n \t\t\t\ttransport->remote->name,\n-\t\t\t\tref_map);\n+\t\t\t\tret.new_refs);\n+\t}\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\n@@ -1048,9 +1058,11 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \treturn transport;\n }\n \n-static void backfill_tags(struct transport *transport, struct ref *ref_map)\n+static struct refs_result backfill_tags(struct transport *transport,\n+\t\tstruct ref *ref_map)\n {\n \tint cannot_reuse;\n+\tstruct refs_result res;\n \n \t/*\n \t * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it\n@@ -1069,12 +1081,14 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tfetch_refs(transport, ref_map);\n+\tres = fetch_refs(transport, ref_map);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n \t\tgsecondary = NULL;\n \t}\n+\n+\treturn res;\n }\n \n static int do_fetch(struct transport *transport,\n@@ -1083,6 +1097,7 @@ static int do_fetch(struct transport *transport,\n \tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n \tstruct ref *ref_map;\n \tstruct ref *rm;\n+\tstruct refs_result res;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \n@@ -1135,7 +1150,10 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t transport->url);\n \t\t}\n \t}\n-\tif (fetch_refs(transport, ref_map)) {\n+\n+\tres = fetch_refs(transport, ref_map);\n+\tref_map = res.new_refs;\n+\tif (res.status) {\n \t\tfree_refs(ref_map);\n \t\tretcode = 1;\n \t\tgoto cleanup;\n@@ -1148,8 +1166,10 @@ static int do_fetch(struct transport *transport,\n \t\tstruct ref **tail = &ref_map;\n \t\tref_map = NULL;\n \t\tfind_non_local_tags(transport, &ref_map, &tail);\n-\t\tif (ref_map)\n-\t\t\tbackfill_tags(transport, ref_map);\n+\t\tif (ref_map) {\n+\t\t\tres = backfill_tags(transport, ref_map);\n+\t\t\tref_map = res.new_refs;\n+\t\t}\n \t\tfree_refs(ref_map);\n \t}\n \n-- \n2.16.1\n\n"},{"id":"338941","messageId":"20180209214458.16135-2-leo@gaspard.io","threadId":"47783","inReplyTo":"20180209214458.16135-1-leo@gaspard.io","subject":"[PATCH 2/2] fetch: add tweak-fetch hook","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-09T21:44:58Z","receivedAt":"2018-02-09T21:45:19Z","isPatch":true,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"From: Léo Gaspard \n\nThe tweak-fetch hook is fed lines on stdin for all refs that were\nfetched, and outputs on stdout possibly modified lines. Its output is\nthen parsed and used when `git fetch` updates the remote tracking refs,\nrecords the entries in FETCH_HEAD, and produces its report.\n\nThe modifications here are heavily based on prior work by Joey Hess.\n\nBased-on-patch-by: Joey Hess \nSigned-off-by: Leo Gaspard \n---\n Documentation/githooks.txt | 37 +++++++\n builtin/fetch.c | 210 +++++++++++++++++++++++++++++++++++-\n t/t5574-fetch-tweak-fetch-hook.sh | 90 ++++++++++++++++\n templates/hooks--tweak-fetch.sample | 24 +++++\n 4 files changed, 359 insertions(+), 2 deletions(-)\n create mode 100755 t/t5574-fetch-tweak-fetch-hook.sh\n create mode 100755 templates/hooks--tweak-fetch.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex f877f7b7c..1b4a18bf0 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -177,6 +177,43 @@ This hook can be used to perform repository validity checks, auto-display\n differences from the previous HEAD if different, or set working dir metadata\n properties.\n \n+tweak-fetch\n+~~~~~~~~~~~\n+\n+This hook is invoked by 'git fetch' (commonly called by 'git pull'), after refs\n+have been fetched from the remote repository. It is not executed, if nothing was\n+fetched.\n+\n+The output of the hook is used to update the remote-tracking branches, and\n+`.git/FETCH_HEAD`, in preparation for a later merge operation done by 'git\n+merge'.\n+\n+It takes no arguments, but is fed a line of the following format on its standard\n+input for each ref that was fetched.\n+\n+ SP not-for-merge|merge|ignore SP SP LF\n+\n+Where the \"not-for-merge\" flag indicates the ref is not to be merged into the\n+current branch, and the \"merge\" flag indicates that 'git merge' should later\n+merge it.\n+\n+The `` is the remote's name for the ref that was fetched, and\n+`` is a name of a remote-tracking branch, like\n+\"refs/remotes/origin/master\". `` can be undefined if the fetched\n+ref is not being stored in a local refname. In this case, it will be set to `@`,\n+an invalide refspec, so that scripts can be written more easily.\n+\n+TODO: Add documentation for the “ignore” parameter. Unfortunately, I'm not\n+really sure I get what this does or what invariants it is supposed to maintain\n+(eg. all “ignore” updates at the end of the refs list?), so this may also\n+require code changes.\n+\n+The hook must consume all of its standard input, and output back lines of the\n+same format. It can modify its input as desired, including adding or removing\n+lines, updating the sha1 (i.e. re-point the remote-tracking branch), changing\n+the merge flag, and changing the `` (i.e. use different\n+remote-tracking branch).\n+\n post-merge\n ~~~~~~~~~~\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 76dc05f61..1bb394530 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -28,6 +28,8 @@ static const char * const builtin_fetch_usage[] = {\n \tNULL\n };\n \n+static const char tweak_fetch_hook[] = \"tweak-fetch\";\n+\n enum {\n \tTAGS_UNSET = 0,\n \tTAGS_DEFAULT = 1,\n@@ -181,6 +183,206 @@ static struct option builtin_fetch_options[] = {\n \tOPT_END()\n };\n \n+static int feed_tweak_fetch_hook(int in, int out, void *data)\n+{\n+\tstruct ref *ref;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *kw, *peer_ref;\n+\tchar oid_buf[GIT_SHA1_HEXSZ + 1];\n+\tint ret;\n+\n+\tfor (ref = data; ref; ref = ref->next) {\n+\t\tif (ref->fetch_head_status == FETCH_HEAD_MERGE)\n+\t\t\tkw = \"merge\";\n+\t\telse if (ref->fetch_head_status == FETCH_HEAD_IGNORE)\n+\t\t\tkw = \"ignore\";\n+\t\telse\n+\t\t\tkw = \"not-for-merge\";\n+\t\tif (!ref->name)\n+\t\t\tdie(\"trying to fetch an inexistant ref\");\n+\t\tif (ref->peer_ref && ref->peer_ref->name)\n+\t\t\tpeer_ref = ref->peer_ref->name;\n+\t\telse\n+\t\t\tpeer_ref = \"@\";\n+\t\tstrbuf_addf(&buf, \"%s %s %s %s\\n\",\n+\t\t\t\toid_to_hex_r(oid_buf, &ref->old_oid), kw,\n+\t\t\t\tref->name, peer_ref);\n+\t}\n+\n+\tret = write_in_full(out, buf.buf, buf.len) != buf.len;\n+\tif (ret)\n+\t\twarning(\"%s hook failed to consume all its input\",\n+\t\t\t\ttweak_fetch_hook);\n+\tclose(out);\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static struct ref *parse_tweak_fetch_hook_line(char *l,\n+\t\tstruct string_list *existing_refs)\n+{\n+\tstruct ref *ref = NULL, *peer_ref = NULL;\n+\tstruct string_list_item *peer_item = NULL;\n+\tchar *words[4];\n+\tint i, word = 0;\n+\tchar *problem;\n+\n+\tfor (i = 0; l[i]; i++) {\n+\t\tif (isspace(l[i])) {\n+\t\t\tl[i] = '\\0';\n+\t\t\twords[word] = l;\n+\t\t\tl += i + 1;\n+\t\t\ti = 0;\n+\t\t\tword++;\n+\t\t\tif (word > 3) {\n+\t\t\t\tproblem = \"too many words\";\n+\t\t\t\tgoto unparsable;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tif (word < 3) {\n+\t\tproblem = \"not enough words\";\n+\t\tgoto unparsable;\n+\t}\n+\n+\tref = alloc_ref(words[2]);\n+\tpeer_ref = ref->peer_ref = alloc_ref(l);\n+\tref->peer_ref->force = 1;\n+\n+\tif (get_oid_hex(words[0], &ref->old_oid)) {\n+\t\tproblem=\"bad oid\";\n+\t\tgoto unparsable;\n+\t}\n+\n+\tif (!strcmp(words[1], \"merge\")) {\n+\t\tref->fetch_head_status = FETCH_HEAD_MERGE;\n+\t} else if (!strcmp(words[1], \"ignore\")) {\n+\t\tref->fetch_head_status = FETCH_HEAD_IGNORE;\n+\t} else if (!strcmp(words[1], \"not-for-merge\")) {\n+\t\tref->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t} else {\n+\t\tproblem = \"bad merge flag\";\n+\t\tgoto unparsable;\n+\t}\n+\n+\tpeer_item = string_list_lookup(existing_refs, peer_ref->name);\n+\tif (peer_item)\n+\t\thashcpy(peer_ref->old_oid.hash, peer_item->util);\n+\n+\treturn ref;\n+\n+unparsable:\n+\twarning(\"%s hook output a wrongly formed line: %s\",\n+\t\t\ttweak_fetch_hook, problem);\n+\tfree(ref);\n+\tfree(peer_ref);\n+\treturn NULL;\n+}\n+\n+static struct refs_result read_tweak_fetch_hook(int in)\n+{\n+\tstruct refs_result res;\n+\tFILE *f;\n+\tstruct strbuf buf;\n+\tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n+\tstruct ref *ref, *prevref = NULL;\n+\n+\tres.status = 0;\n+\tres.new_refs = NULL;\n+\n+\tf = fdopen(in, \"r\");\n+\tif (f == NULL) {\n+\t\tres.status = 1;\n+\t\treturn res;\n+\t}\n+\n+\tstrbuf_init(&buf, 128);\n+\tfor_each_ref(add_existing, &existing_refs);\n+\n+\twhile (strbuf_getline(&buf, f) != EOF) {\n+\t\tchar *l = strbuf_detach(&buf, NULL);\n+\t\tref = parse_tweak_fetch_hook_line(l, &existing_refs);\n+\t\tif (!ref) {\n+\t\t\tres.status = 1;\n+\t\t} else {\n+\t\t\tif (prevref) {\n+\t\t\t\tprevref->next = ref;\n+\t\t\t\tprevref = ref;\n+\t\t\t} else {\n+\t\t\t\tres.new_refs = prevref = ref;\n+\t\t\t}\n+\t\t}\n+\t\tfree(l);\n+\t}\n+\n+\tstring_list_clear(&existing_refs, 0);\n+\tstrbuf_release(&buf);\n+\tfclose(f);\n+\treturn res;\n+}\n+\n+/*\n+ * The hook is fed lines of the form:\n+ * SP SP SP LF\n+ * And should output rewritten lines of the same form.\n+ */\n+static struct ref *run_tweak_fetch_hook(struct ref *fetched_refs)\n+{\n+\tstruct child_process hook;\n+\tconst char *argv[2];\n+\tstruct async async;\n+\tstruct refs_result res;\n+\n+\tif (!fetched_refs)\n+\t\treturn fetched_refs;\n+\n+\targv[0] = find_hook(tweak_fetch_hook);\n+\tif (access(argv[0], X_OK) < 0)\n+\t\treturn fetched_refs;\n+\targv[1] = NULL;\n+\n+\tmemset(&hook, 0, sizeof(hook));\n+\thook.argv = argv;\n+\thook.in = -1;\n+\thook.out = -1;\n+\tif (start_command(&hook))\n+\t\treturn fetched_refs;\n+\n+\t/*\n+\t * Use an async writer to feed the hook process.\n+\t * This allows the hook to read and write a line at\n+\t * a time without blocking.\n+\t */\n+\tmemset(&async, 0, sizeof(async));\n+\tasync.proc = feed_tweak_fetch_hook;\n+\tasync.data = fetched_refs;\n+\tasync.out = hook.in;\n+\tif (start_async(&async)) {\n+\t\tclose(hook.in);\n+\t\tclose(hook.out);\n+\t\tfinish_command(&hook);\n+\t\treturn fetched_refs;\n+\t}\n+\n+\tres = read_tweak_fetch_hook(hook.out);\n+\tres.status |= finish_async(&async);\n+\tres.status |= finish_command(&hook);\n+\n+\tif (res.status) {\n+\t\twarning(\"%s hook failed, ignoring its output\", tweak_fetch_hook);\n+\t\tfree(res.new_refs);\n+\t\treturn fetched_refs;\n+\t} else {\n+\t\t/*\n+\t\t * The new_refs are returned, to be used in place of\n+\t\t * fetched_refs, so it is not needed anymore and can\n+\t\t * be freed here.\n+\t\t */\n+\t\tfree_refs(fetched_refs);\n+\t\treturn res.new_refs;\n+\t}\n+}\n+\n static void unlock_pack(void)\n {\n \tif (gtransport)\n@@ -934,7 +1136,7 @@ static struct refs_result fetch_refs(struct transport *transport,\n \t\tret.status = transport_fetch_refs(transport, ref_map);\n \t}\n \tif (!ret.status) {\n-\t\tret.new_refs = ref_map;\n+\t\tret.new_refs = run_tweak_fetch_hook(ref_map);\n \t\tret.status |= store_updated_refs(transport->url,\n \t\t\t\ttransport->remote->name,\n \t\t\t\tret.new_refs);\n@@ -1150,7 +1352,11 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t transport->url);\n \t\t}\n \t}\n-\n+\t// TODO(?): Were this placed above the `if (prune)`, it would avoid the\n+\t// unfortunate fact that `git fetch --prune` first drops the ref then\n+\t// re-adds it (in cases where the tweak-fetch hook renames it). There is\n+\t// likely a better solution than this one that would break Commit\n+\t// 10a6cc889 (\"fetch --prune: Run prune before fetching\", 2014-01-02)\n \tres = fetch_refs(transport, ref_map);\n \tref_map = res.new_refs;\n \tif (res.status) {\ndiff --git a/t/t5574-fetch-tweak-fetch-hook.sh b/t/t5574-fetch-tweak-fetch-hook.sh\nnew file mode 100755\nindex 000000000..17cf52684\n--- /dev/null\n+++ b/t/t5574-fetch-tweak-fetch-hook.sh\n@@ -0,0 +1,90 @@\n+#!/bin/sh\n+\n+test_description='testing tweak-fetch-hook'\n+. ./test-lib.sh\n+\n+HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n+HOOK=\"$HOOKDIR/tweak-fetch\"\n+mkdir -p \"$HOOKDIR\"\n+\n+# Setup\n+test_expect_success 'setup' '\n+\tgit init parent-repo &&\n+\tgit remote add parent parent-repo &&\n+\t(cd parent-repo && test_commit commit-100) &&\n+\tgit fetch parent &&\n+\tgit tag | grep -E \"^commit-100$\"\n+'\n+\n+# No-effect hook\n+write_script \"$HOOK\" < /dev/null 2>&1; then\n+ allowed_commit=\"$commit\"\n+ else\n+ echo \"Commit '$commit' is not signed! Refusing to fetch past it\" >&2\n+ break\n+ fi\n+ done\n+ echo \"$allowed_commit $merge $remote_ref $local_ref\"\n+done\n+# TODO: actually verify this hook works\n-- \n2.16.1\n\n"},{"id":"338943","messageId":"87po5dbz1a.fsf@evledraar.gmail.com","threadId":"47783","inReplyTo":"fa470be4-75fb-76ed-ed93-5c10fcfb8842@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-09T22:04:17Z","receivedAt":"2018-02-09T22:04:30Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 08 2018, Leo Gaspard jotted:\n\n> On 02/08/2018 10:06 PM, Ævar Arnfjörð Bjarmason wrote:>> Hmm, OK, so I\n> guess I'll try to update the patch when I get some time to\n>>> delve into git's internals, as my use case (forbidding some fetches)\n>>> couldn't afaik be covered by a wrapper hook.\n>>\n>> Per my reading of\n>> https://public-inbox.org/git/20111224234212.GA21533@gnu.kitenet.net/\n>> what Joey implemented is not what you described in your initial mail.\n>>\n>> His is a *post*-fetch hook, we've already done the fetch and are just\n>> telling you as a courtesy what refs changed. You could also implement\n>> this as some cronjob that polls git for-each-ref but it's easier as a\n>> hook, fine.\n>\n> I was thinking along the lines of\n> https://marc.info/?l=git&m=132486687023893&w=2\n> with high-level description at\n> https://marc.info/?l=git&m=132480559712592&w=2\n>\n> With the high-level description given here, I'm pretty sure I can hack a\n> hook together to make things work as I want them to.\n>\n>> What you're describing is something like a pre-fetch hook analogous to\n>> the pre-receive hooks, where you're fed refs updated on the remote on\n>> stdin, and can say you don't want some of those to be updated.\n>>\n>> This may just be a lack of imagination on my part, but I don't see how\n>> that's sensible at all.\n>>\n>> The refs we fetch are our *copy* of the remote refs, why would you not\n>> want to track the upstream remote. You're going to refuse some branches\n>> and what? Be further behind until some point in the future where the tip\n>> is GPG-signed and you accept it, at which poich you'll need to do more\n>> work than if you were up-to-date with the almost-GPG-signed version?\n>\n> That's about it. I want all fetching to be blocked in case of the tip\n> not being signed. As there is a pre-push hook ensuring committers don't\n> forget to sign before pushing, the only case the tip could not be signed\n> is in case of an attack, which means it's better to just force-push\n> master because any git repo that fetched it is doomed anyway. Definitely\n> would not want to allow an untrusted revision get into anything that\n> could even remotely be taken as “endorsed” by the user.\n>\n> (BTW, in order to avoid the case of someone forgetting to sign the\n> commit and not having installed the pre-push hook, there can be holes in\n> the commit-signing chain, the drawback being that the committer pushing\n> a signed commit takes responsibility for all unsigned commits directly\n> preceding his -- allowing them to recover in case of a mistaken push)\n>\n>> I think you're confusing two things here. One is the reasonable concern\n>> of wanting to not have your local copy of remote refs have undesirable\n>> content, but a pre-fetch hook is not the way to accomplish that.\n>\n> Well, a pre-fetch hook is a possible way of accomplishing that, and I\n> don't know of any better one?\n>\n>> The other is e.g. to ensure that you never locally check out some \"bad\"\n>> ref, we don't have hook support for that, but could add it,\n>> e.g. git-checkout and git reset --hard could be taught about some\n>> pre-checkout hook.\n>\n> Issue is, once we have to fix checkout and reset, all other commands\n> that potentially touch the worktree also have to be fixed (eg. I don't\n> know whether worktree add triggers pre-checkout?)\n>\n> Also, this requires the hook to store a database of all the paths that\n> have been checked, because there is no logic in how one may choose to\n> checkout the repo. While having a tweak-fetch hook would make the\n> implementation straightforward, because at the time of invoking the hook\n> the “refname at remote” commit is already trusted, and the “object name”\n> is the commit whose validity we want to check, so we just have to check\n> the path between those two. (I don't know if you checked my current\n> scripts, but basically as the set of allowed PGP keys can change at any\n> commit, it's only possible to check a commit path, not a single commit\n> out-of-nowhere)\n\nRight, it's certainly the case that refusing the refs is a more\neffective gatekeeper, we're not going to have hooks for all the\nlow-level stuff that might inspect sha1s or check them out.\n\nMy assumption was that hooking into just a couple of things would be\ngood enough, but yes, there's other trade-offs.\n\n> The only issue that could arise with a tweak-fetch hook is in case of a\n> force-fetch (and even maybe it's not even an actual issue, I haven't\n> given it real thought yet), but this can reasonably be banned, as once a\n> commit is signed it enters the “real” master branch, that should never\n> be moved backward, as it can't be the sign of an attack.\n>\n>> You could also have some intermediate step between these two, where\n>> e.g. your refspec for \"origin\" is\n>> \"+refs/heads/*:refs/remotes/origin-untrusted/*\" instead of the default\n>> \"+refs/heads/*:refs/remotes/origin/*\", you fetch all refs to that\n>> location, then you move them (with some alias/hook) to\n>> \"refs/remotes/origin/*\" once they're seen to be \"OK\".\n>\n> That is indeed another possibility, but then the idea is to make things\n> as transparent as possible for the end-user, not to completely change\n> their git workflow. As such, considering only signed commits to be part\n> of the upstream seems to make sense to me?\n\nI mean this would be something that would be part of a post-fetch hook,\nso it would be as transparent as what you're doing to the user, with the\ndifference that it doesn't need to involve changes to what you slurp\ndown from the server.\n\nI.e. we'd just fetch into refs/remotes/origin-untrusted/, then we run\nyour post-fetch hook and you go over the new refs, and copy what you\nlike (usually everything) to refs/remotes/origin/*.\n\nYou get the same thing, the logic just becomes inspecting something\nlocally and copying it over.\n\nThere are other caveats, notably that the local ref store now needs to\nstore 2x the amount of branches, unless we're smarter about it and only\nstore stuff in refs/remotes/origin-untrusted/ that's different from\nrefs/remotes/origin/, as a sort of ref staging area.\n\nAnyway, I don't have strong opinions on this, just thought it was worth\ndiscussing the whys.\n\nOne thing that's not discussed yet, and I know just enough about for it\nto tingle my spidey sense, but not enough to say for sure (CC'd Jeff &\nBrandon who know more) is that this feature once shipped might cause\nhigher load on git hosting providers.\n\nThis is because people will inevitably use it in popular projects for\nsome custom filtering, and because you're continually re-fetching and\ninspecting stuff what used to be a really cheap no-op \"pull\" most of the\ntime is a more expensive negotiation every time before the client\nrejects the refs again, and worse for hosting providers because you have\nbespoke ref fetching strategies you have less odds of being able to\ncache both the negotiation and the pack you serve.\n\nI.e. you want this for some security feature where 99.99% of the time\nyou accept all refs, but most people will probably use this to implement\ndynamic Turing-complete refspecs.\n\nMaybe that's worrying about nothing, but worth thinking about.\n"},{"id":"338947","messageId":"25bd770c-6a48-5b5d-04cc-6d02784ea3e7@gaspard.io","threadId":"47783","inReplyTo":"87po5dbz1a.fsf@evledraar.gmail.com","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-09T22:24:44Z","receivedAt":"2018-02-09T22:24:58Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/09/2018 11:04 PM, Ævar Arnfjörð Bjarmason wrote:>>> You could also\nhave some intermediate step between these two, where\n>>> e.g. your refspec for \"origin\" is\n>>> \"+refs/heads/*:refs/remotes/origin-untrusted/*\" instead of the default\n>>> \"+refs/heads/*:refs/remotes/origin/*\", you fetch all refs to that\n>>> location, then you move them (with some alias/hook) to\n>>> \"refs/remotes/origin/*\" once they're seen to be \"OK\".\n>>\n>> That is indeed another possibility, but then the idea is to make things\n>> as transparent as possible for the end-user, not to completely change\n>> their git workflow. As such, considering only signed commits to be part\n>> of the upstream seems to make sense to me?\n> \n> I mean this would be something that would be part of a post-fetch hook,\n> so it would be as transparent as what you're doing to the user, with the\n> difference that it doesn't need to involve changes to what you slurp\n> down from the server.\n> \n> I.e. we'd just fetch into refs/remotes/origin-untrusted/, then we run\n> your post-fetch hook and you go over the new refs, and copy what you\n> like (usually everything) to refs/remotes/origin/*.\n\nHmm... but that would then require a post-fetch hook, wouldn't it? And\nabout a post-fetch hook, if I understood correctly, Junio in [1] had a\nquite nice argument against it:\n\n Although I do not deeply care between such a \"trigger to only\n notify, no touching\" hook and a full-blown \"allow hook writers to\n easily lie about what happened in the fetch\" hook, I was hoping that\n we would get this right and useful if we were spending our brain\n bandwidth on it. I am not very fond of an easier \"trigger to only\n notify\" hook because people are bound to misuse the interface and\n try updating the refs anyway, making it easy to introduce\n inconsistencies between refs and FETCH_HEAD that will confuse the\n later \"merge\" step.\n\nOtherwise, if it doesn't require a post-fetch hook, then it would\nrequire the end-user to first fetch, then run the\n`copy-trusted-refs-over` script, which would add stuff to the user's\nworkflow.\n\nDid I miss another possibility?\n\n> [...]\n> \n> One thing that's not discussed yet, and I know just enough about for it\n> to tingle my spidey sense, but not enough to say for sure (CC'd Jeff &\n> Brandon who know more) is that this feature once shipped might cause\n> higher load on git hosting providers.\n> \n> This is because people will inevitably use it in popular projects for\n> some custom filtering, and because you're continually re-fetching and\n> inspecting stuff what used to be a really cheap no-op \"pull\" most of the\n> time is a more expensive negotiation every time before the client\n> rejects the refs again, and worse for hosting providers because you have\n> bespoke ref fetching strategies you have less odds of being able to\n> cache both the negotiation and the pack you serve.\n> \n> I.e. you want this for some security feature where 99.99% of the time\n> you accept all refs, but most people will probably use this to implement\n> dynamic Turing-complete refspecs.\n> \n> Maybe that's worrying about nothing, but worth thinking about.\n\nWell... First, I must say I didn't really understand your last paragraph\nabout Turing-complete refspecs.\n\nBut my understanding of how the fetch-hook patchset I sent this evening\nworks is that it first receives all the objects from the hosting\nprovider, then locally moves the refs, but never actually discards the\ndownloaded objects (well, until a `git gc` I guess).\n\nSo I don't think the network traffic with the provider would be any\ndifferent wrt. what it is now, even if a tweak-fetch hook rejects some\ncommits? Then again I don't know git's internals enough to be have even\na bit of certainty about what I'm saying right now, so...\n\n\n[1] https://marc.info/?l=git&m=132480559712592&w=2\n"},{"id":"338948","messageId":"20180209223011.GA24578@sigill.intra.peff.net","threadId":"47783","inReplyTo":"87po5dbz1a.fsf@evledraar.gmail.com","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-09T22:30:11Z","receivedAt":"2018-02-09T22:30:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 09, 2018 at 11:04:17PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> One thing that's not discussed yet, and I know just enough about for it\n> to tingle my spidey sense, but not enough to say for sure (CC'd Jeff &\n> Brandon who know more) is that this feature once shipped might cause\n> higher load on git hosting providers.\n> \n> This is because people will inevitably use it in popular projects for\n> some custom filtering, and because you're continually re-fetching and\n> inspecting stuff what used to be a really cheap no-op \"pull\" most of the\n> time is a more expensive negotiation every time before the client\n> rejects the refs again, and worse for hosting providers because you have\n> bespoke ref fetching strategies you have less odds of being able to\n> cache both the negotiation and the pack you serve.\n\nMost of the discussion so far seems to be about \"accept this ref or\ndon't accept this ref\", which seems OK. But if you are going to do\ncustom tweaking like rewriting objects, or making it common to refuse\nsome refs, then I think things get pretty inefficient for _everybody_.\n\nThe negotiation for future fetches uses the existing refs as the\nstarting point. And if we don't know that we have the objects because\nthere are no refs pointing at them, they're going to get transferred\nagain. That's extra load no the server, and extra time for the user\nwaiting on the network.\n\nI tend to agree with the direction of thinking you outlined: you're\ngenerally better off completing the fetch to a local namespace that\ntracks the other side completely, and then manipulating the local refs\nas you see fit (e.g., fetching into refs/quarantine, and then migrating\n\"good\" refs over to refs/remotes/origin).\n\n-Peff\n"},{"id":"338949","messageId":"xmqqo9kxvll5.fsf@gitster-ct.c.googlers.com","threadId":"47783","inReplyTo":"20180209214458.16135-1-leo@gaspard.io","subject":"Re: [PATCH 1/2] fetch: preparations for tweak-fetch hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-09T22:34:30Z","receivedAt":"2018-02-09T22:34:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Leo Gaspard writes:\n\n> From: Léo Gaspard \n>\n> No behavior changes yet, only some groundwork for the next change.\n>\n> The refs_result structure combines a status code with a ref map, which\n> can be NULL even on success. \n\nSorry, but I have absolutely no idea what this sentence wants to do\nby being here in the log message. \"even on success\" makes it sound\nas if it is normal to be NULL when status code != success, but it is\nnot even clear why it is the case, and \"can be NULL\" implies that it\nis not always NULL, but it does not make it clear when it is NULL\nand when it is not NULL when code == success. If you find the need\nto explain what each field of a new struct you are introducing to\nthe codebase means (and it probably is a good idea for this one),\nperhaps the right place to do so is in in-code comment next to the\nstruct definition?\n\nIt seems that you also moved add_existing() in the file, for no\napparent reason.\n\n> This will be needed when there's a\n> tweak-fetch hook, because it can filter out all refs, while still\n> succeeding.\n>\n> fetch_refs returns a refs_result, so that it can modify the ref_map.\n\nThe description keeps saying \"ref map\", but it is unclear what it\nis, why it is a good thing to allow the caller \"modify\" it, and what\nkind of modification the caller wants to make for what reason.\n\nAlso, in C, we tend to avoid passing structure by value. It seems\nthat the patch makes a callchain involving backfill_tags() return\nthis struct by value, which goes against the convention. I suspect\nthat it should be trivial to have the caller supply an on-stack\ninstance and pass the pointer to it down the callchain, though.\n\n> Based-on-patch-by: Joey Hess \n> Signed-off-by: Leo Gaspard \n> ---\n> builtin/fetch.c | 68 +++++++++++++++++++++++++++++++++++++--------------------\n> 1 file changed, 44 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 7bbcd26fa..76dc05f61 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -34,6 +34,11 @@ enum {\n> \tTAGS_SET = 2\n> };\n> \n> +struct refs_result {\n> +\tstruct ref *new_refs;\n> +\tint status;\n> +};\n> +\n> static int fetch_prune_config = -1; /* unspecified */\n> static int prune = -1; /* unspecified */\n> #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */\n> @@ -57,6 +62,18 @@ static int shown_url = 0;\n> static int refmap_alloc, refmap_nr;\n> static const char **refmap_array;\n> \n> +static int add_existing(const char *refname, const struct object_id *oid,\n> +\t\t\tint flag, void *cbdata)\n> +{\n> +\tstruct string_list *list = (struct string_list *)cbdata;\n> +\tstruct string_list_item *item = string_list_insert(list, refname);\n> +\tstruct object_id *old_oid = xmalloc(sizeof(*old_oid));\n> +\n> +\toidcpy(old_oid, oid);\n> +\titem->util = old_oid;\n> +\treturn 0;\n> +}\n> +\n> static int git_fetch_config(const char *k, const char *v, void *cb)\n> {\n> \tif (!strcmp(k, \"fetch.prune\")) {\n> @@ -217,18 +234,6 @@ static void add_merge_config(struct ref **head,\n> \t}\n> }\n> \n> -static int add_existing(const char *refname, const struct object_id *oid,\n> -\t\t\tint flag, void *cbdata)\n> -{\n> -\tstruct string_list *list = (struct string_list *)cbdata;\n> -\tstruct string_list_item *item = string_list_insert(list, refname);\n> -\tstruct object_id *old_oid = xmalloc(sizeof(*old_oid));\n> -\n> -\toidcpy(old_oid, oid);\n> -\titem->util = old_oid;\n> -\treturn 0;\n> -}\n> -\n> static int will_fetch(struct ref **head, const unsigned char *sha1)\n> {\n> \tstruct ref *rm = *head;\n> @@ -920,15 +925,20 @@ static int quickfetch(struct ref *ref_map)\n> \treturn check_connected(iterate_ref_map, &rm, &opt);\n> }\n> \n> -static int fetch_refs(struct transport *transport, struct ref *ref_map)\n> +static struct refs_result fetch_refs(struct transport *transport,\n> +\t\tstruct ref *ref_map)\n> {\n> -\tint ret = quickfetch(ref_map);\n> -\tif (ret)\n> -\t\tret = transport_fetch_refs(transport, ref_map);\n> -\tif (!ret)\n> -\t\tret |= store_updated_refs(transport->url,\n> +\tstruct refs_result ret;\n> +\tret.status = quickfetch(ref_map);\n> +\tif (ret.status) {\n> +\t\tret.status = transport_fetch_refs(transport, ref_map);\n> +\t}\n> +\tif (!ret.status) {\n> +\t\tret.new_refs = ref_map;\n> +\t\tret.status |= store_updated_refs(transport->url,\n> \t\t\t\ttransport->remote->name,\n> -\t\t\t\tref_map);\n> +\t\t\t\tret.new_refs);\n> +\t}\n> \ttransport_unlock_pack(transport);\n> \treturn ret;\n> }\n> @@ -1048,9 +1058,11 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n> \treturn transport;\n> }\n> \n> -static void backfill_tags(struct transport *transport, struct ref *ref_map)\n> +static struct refs_result backfill_tags(struct transport *transport,\n> +\t\tstruct ref *ref_map)\n> {\n> \tint cannot_reuse;\n> +\tstruct refs_result res;\n> \n> \t/*\n> \t * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it\n> @@ -1069,12 +1081,14 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n> \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n> \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n> \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n> -\tfetch_refs(transport, ref_map);\n> +\tres = fetch_refs(transport, ref_map);\n> \n> \tif (gsecondary) {\n> \t\ttransport_disconnect(gsecondary);\n> \t\tgsecondary = NULL;\n> \t}\n> +\n> +\treturn res;\n> }\n> \n> static int do_fetch(struct transport *transport,\n> @@ -1083,6 +1097,7 @@ static int do_fetch(struct transport *transport,\n> \tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n> \tstruct ref *ref_map;\n> \tstruct ref *rm;\n> +\tstruct refs_result res;\n> \tint autotags = (transport->remote->fetch_tags == 1);\n> \tint retcode = 0;\n> \n> @@ -1135,7 +1150,10 @@ static int do_fetch(struct transport *transport,\n> \t\t\t\t transport->url);\n> \t\t}\n> \t}\n> -\tif (fetch_refs(transport, ref_map)) {\n> +\n> +\tres = fetch_refs(transport, ref_map);\n> +\tref_map = res.new_refs;\n> +\tif (res.status) {\n> \t\tfree_refs(ref_map);\n> \t\tretcode = 1;\n> \t\tgoto cleanup;\n> @@ -1148,8 +1166,10 @@ static int do_fetch(struct transport *transport,\n> \t\tstruct ref **tail = &ref_map;\n> \t\tref_map = NULL;\n> \t\tfind_non_local_tags(transport, &ref_map, &tail);\n> -\t\tif (ref_map)\n> -\t\t\tbackfill_tags(transport, ref_map);\n> +\t\tif (ref_map) {\n> +\t\t\tres = backfill_tags(transport, ref_map);\n> +\t\t\tref_map = res.new_refs;\n> +\t\t}\n> \t\tfree_refs(ref_map);\n> \t}\n"},{"id":"338950","messageId":"xmqqk1vlvlan.fsf@gitster-ct.c.googlers.com","threadId":"47783","inReplyTo":"20180209214458.16135-2-leo@gaspard.io","subject":"Re: [PATCH 2/2] fetch: add tweak-fetch hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-09T22:40:48Z","receivedAt":"2018-02-09T22:40:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Leo Gaspard writes:\n\n> +tweak-fetch\n> +~~~~~~~~~~~\n> +\n> +This hook is invoked by 'git fetch' (commonly called by 'git pull'), after refs\n> +have been fetched from the remote repository. It is not executed, if nothing was\n> +fetched.\n\nNeed to tighten explanation of \"nothing was fetched\". If the only\nchange I made to my repository is that I created a new branch that\npoints at an existing object since last time you fetched, you would\nobtain no new object when you fetch from me. Would that count as\n\"nothing was fetched\"? Or would it be still fetching something\n(i.e. your remote-tracking hierarchy will record the fact that I now\nhave this new branch)?\n\n> + SP not-for-merge|merge|ignore SP SP LF\n> + ...\n> +The `` is the remote's name for the ref that was fetched, and\n> +`` is a name of a remote-tracking branch, like\n> +\"refs/remotes/origin/master\". `` can be undefined if the fetched\n> +ref is not being stored in a local refname. In this case, it will be set to `@`,\n\nDon't use \"@\"; leave it empty instead.\n\n> +TODO: Add documentation for the “ignore” parameter. Unfortunately, I'm not\n> +really sure I get what this does or what invariants it is supposed to maintain\n> +(eg. all “ignore” updates at the end of the refs list?), so this may also\n> +require code changes.\n\nIf you are not using the feature, wouldn't it make more sense not to\nadd it in the first place?\n\n"},{"id":"338952","messageId":"xmqqd11dvl2a.fsf@gitster-ct.c.googlers.com","threadId":"47783","inReplyTo":"20180209223011.GA24578@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-09T22:45:49Z","receivedAt":"2018-02-09T22:45:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The negotiation for future fetches uses the existing refs as the\n> starting point. And if we don't know that we have the objects because\n> there are no refs pointing at them, they're going to get transferred\n> again. That's extra load no the server, and extra time for the user\n> waiting on the network.\n>\n> I tend to agree with the direction of thinking you outlined: you're\n> generally better off completing the fetch to a local namespace that\n> tracks the other side completely, and then manipulating the local refs\n> as you see fit (e.g., fetching into refs/quarantine, and then migrating\n> \"good\" refs over to refs/remotes/origin).\n\nThanks for a dose of sanity ;-)\n"},{"id":"338954","messageId":"87lgg1bwmi.fsf@evledraar.gmail.com","threadId":"47783","inReplyTo":"25bd770c-6a48-5b5d-04cc-6d02784ea3e7@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-09T22:56:21Z","receivedAt":"2018-02-09T22:56:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 09 2018, Leo Gaspard jotted:\n\n> On 02/09/2018 11:04 PM, Ævar Arnfjörð Bjarmason wrote:>>> You could also\n> have some intermediate step between these two, where\n>>>> e.g. your refspec for \"origin\" is\n>>>> \"+refs/heads/*:refs/remotes/origin-untrusted/*\" instead of the default\n>>>> \"+refs/heads/*:refs/remotes/origin/*\", you fetch all refs to that\n>>>> location, then you move them (with some alias/hook) to\n>>>> \"refs/remotes/origin/*\" once they're seen to be \"OK\".\n>>>\n>>> That is indeed another possibility, but then the idea is to make things\n>>> as transparent as possible for the end-user, not to completely change\n>>> their git workflow. As such, considering only signed commits to be part\n>>> of the upstream seems to make sense to me?\n>>\n>> I mean this would be something that would be part of a post-fetch hook,\n>> so it would be as transparent as what you're doing to the user, with the\n>> difference that it doesn't need to involve changes to what you slurp\n>> down from the server.\n>>\n>> I.e. we'd just fetch into refs/remotes/origin-untrusted/, then we run\n>> your post-fetch hook and you go over the new refs, and copy what you\n>> like (usually everything) to refs/remotes/origin/*.\n>\n> Hmm... but that would then require a post-fetch hook, wouldn't it? And\n> about a post-fetch hook, if I understood correctly, Junio in [1] had a\n> quite nice argument against it:\n>\n> Although I do not deeply care between such a \"trigger to only\n> notify, no touching\" hook and a full-blown \"allow hook writers to\n> easily lie about what happened in the fetch\" hook, I was hoping that\n> we would get this right and useful if we were spending our brain\n> bandwidth on it. I am not very fond of an easier \"trigger to only\n> notify\" hook because people are bound to misuse the interface and\n> try updating the refs anyway, making it easy to introduce\n> inconsistencies between refs and FETCH_HEAD that will confuse the\n> later \"merge\" step.\n>\n> Otherwise, if it doesn't require a post-fetch hook, then it would\n> require the end-user to first fetch, then run the\n> `copy-trusted-refs-over` script, which would add stuff to the user's\n> workflow.\n>\n> Did I miss another possibility?\n\nYes, the assumption is that there would be a post-fetch hook of some\nsort to ferry the refs over from the quarantine.\n\nMy reading of that thread is not that Junio's outright against such a\nfacility (and also it was 2011 and the discussion can be re-visited),\nbut rather that there's specific concerns that need to be kept in mind\nso reliable things involving the ref store don't become brittle as a\nresult of unstable user hooks, or us offering an interface that's easily\nmisused.\n\n>> [...]\n>>\n>> One thing that's not discussed yet, and I know just enough about for it\n>> to tingle my spidey sense, but not enough to say for sure (CC'd Jeff &\n>> Brandon who know more) is that this feature once shipped might cause\n>> higher load on git hosting providers.\n>>\n>> This is because people will inevitably use it in popular projects for\n>> some custom filtering, and because you're continually re-fetching and\n>> inspecting stuff what used to be a really cheap no-op \"pull\" most of the\n>> time is a more expensive negotiation every time before the client\n>> rejects the refs again, and worse for hosting providers because you have\n>> bespoke ref fetching strategies you have less odds of being able to\n>> cache both the negotiation and the pack you serve.\n>>\n>> I.e. you want this for some security feature where 99.99% of the time\n>> you accept all refs, but most people will probably use this to implement\n>> dynamic Turing-complete refspecs.\n>>\n>> Maybe that's worrying about nothing, but worth thinking about.\n>\n> Well... First, I must say I didn't really understand your last paragraph\n> about Turing-complete refspecs.\n>\n> But my understanding of how the fetch-hook patchset I sent this evening\n> works is that it first receives all the objects from the hosting\n> provider, then locally moves the refs, but never actually discards the\n> downloaded objects (well, until a `git gc` I guess).\n>\n> So I don't think the network traffic with the provider would be any\n> different wrt. what it is now, even if a tweak-fetch hook rejects some\n> commits? Then again I don't know git's internals enough to be have even\n> a bit of certainty about what I'm saying right now, so...\n>\n>\n> [1] https://marc.info/?l=git&m=132480559712592&w=2\n\nAs Jeff notes in the side-thread in\n<20180209223011.GA24578@sigill.intra.peff.net> when you do a \"fetch\" the\nprotocol is not negotiating on the basis of what loose unreferenced\n(because you threw away the ref!) objects you have already, but what\nthe ref commonality is between you and the server (and this is just on\na best-effort basis).\n\nSo for example, let's say you only accept the \"master\" branch and have a\nhook that's refusing updates to the \"dev\" branch, fetch is going to be\nre-negotiating fetching the difference between the two over the wire\nevery time, only to have the hook say \"no thanks\" and throw away the\nresult.\n\nI.e. as an extreme example, if the dev branch is adding a 1GB file\ndivergent from master, you are going to be re-downloading that 1GB every\ntime you fetch, even though you have that blob locally, the remote can't\nknow that, and because you have no ref pointing to it you can't mention\nit during the negotiation.\n\nWhich is way less efficient than the case where your refspec only\nfetches \"master\", since then we won't try at all, or the case where you\nfetch both master & dev, but into some quarantine area and are just\ndeciding to update a local ref or not.\n"},{"id":"338962","messageId":"87e7c3b8-3b3c-1cb0-9b11-e4bf3044e539@gaspard.io","threadId":"47783","inReplyTo":"20180209223011.GA24578@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-09T23:49:31Z","receivedAt":"2018-02-09T23:49:53Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/09/2018 11:30 PM, Jeff King wrote:\n> On Fri, Feb 09, 2018 at 11:04:17PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> One thing that's not discussed yet, and I know just enough about for it\n>> to tingle my spidey sense, but not enough to say for sure (CC'd Jeff &\n>> Brandon who know more) is that this feature once shipped might cause\n>> higher load on git hosting providers.\n>>\n>> This is because people will inevitably use it in popular projects for\n>> some custom filtering, and because you're continually re-fetching and\n>> inspecting stuff what used to be a really cheap no-op \"pull\" most of the\n>> time is a more expensive negotiation every time before the client\n>> rejects the refs again, and worse for hosting providers because you have\n>> bespoke ref fetching strategies you have less odds of being able to\n>> cache both the negotiation and the pack you serve.\n> \n> Most of the discussion so far seems to be about \"accept this ref or\n> don't accept this ref\", which seems OK. But if you are going to do\n> custom tweaking like rewriting objects, or making it common to refuse\n> some refs, then I think things get pretty inefficient for _everybody_.\n> \n> The negotiation for future fetches uses the existing refs as the\n> starting point. And if we don't know that we have the objects because\n> there are no refs pointing at them, they're going to get transferred\n> again. That's extra load no the server, and extra time for the user\n> waiting on the network.\n\nOh. I thought the protocol git used was something like\nclient: I want to fetch refs A and B\nserver: so you'll need objects 12345678 and 90ABCDEF, A and B both point\nto 12345678\nclient: please give me object 12345678\nserver: here it is\n[...]\n\nI was clearly wrong, thanks! (and thanks Ævar for your explanation in\nthe side-thread, too!)\n\n> I tend to agree with the direction of thinking you outlined: you're\n> generally better off completing the fetch to a local namespace that\n> tracks the other side completely, and then manipulating the local refs\n> as you see fit (e.g., fetching into refs/quarantine, and then migrating\n> \"good\" refs over to refs/remotes/origin).\n\nHmm... so do I understand it correctly when I say the process you're\nthinking about works like this?\n * User installs hook for my-remote by running [something]\n * User runs git fetch\n * git fetch fetches remote refs/heads/* to local refs/quarantine/* (so\nI guess [something] changes the remote.my-remote.fetch refmap)\n * When this is done `git fetch` runs a notification-only post-fetch\nhook (that would need to be added)\n * The post-fetch hook then performs whatever it wants and updates the\nreferences in refs/remotes/my-remote/*\n\nSo the changes that are required are:\n * Adding a notification-only post-fetch hook\n * For handling tags, there is a need to have a refmap for tags. Maybe\nadding a remote.my-remote.fetchTags refmap, that would be used when\nrunning with --tags, and having it default to “refs/tags/*:refs/tags/*”\nto keep the current behavior by default?\n\nThe only remaining issue I can think of is: How do we avoid the issue of\nthe\ntrigger-only-hook-inciting-bad-behavior-by-hook-authors-who-really-want-modification\nraised in the side-thread that Junio wrote in [1]? Maybe just writing in\nthe documentation that the hook should use a quarantine-like approach if\nit wants modification would be enough to not have hook authors try to\nmodify the ref in the post-fetch hook?\n\nThanks for all your thoughts, and hope we're getting somewhere!\nLeo\n\n\nPS: I'll read over the reviews once I'm all clear as to what exactly is\nwanted for this patch, as most likely they'll just be dumped, given the\ncurrent state of affairs.\n\n[1] https://marc.info/?l=git&m=132480559712592&w=2\n"},{"id":"338963","messageId":"20180210001317.GA26856@sigill.intra.peff.net","threadId":"47783","inReplyTo":"87e7c3b8-3b3c-1cb0-9b11-e4bf3044e539@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-10T00:13:17Z","receivedAt":"2018-02-10T00:13:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 10, 2018 at 12:49:31AM +0100, Leo Gaspard wrote:\n\n> > I tend to agree with the direction of thinking you outlined: you're\n> > generally better off completing the fetch to a local namespace that\n> > tracks the other side completely, and then manipulating the local refs\n> > as you see fit (e.g., fetching into refs/quarantine, and then migrating\n> > \"good\" refs over to refs/remotes/origin).\n> \n> Hmm... so do I understand it correctly when I say the process you're\n> thinking about works like this?\n> * User installs hook for my-remote by running [something]\n> * User runs git fetch\n> * git fetch fetches remote refs/heads/* to local refs/quarantine/* (so\n> I guess [something] changes the remote.my-remote.fetch refmap)\n> * When this is done `git fetch` runs a notification-only post-fetch\n> hook (that would need to be added)\n> * The post-fetch hook then performs whatever it wants and updates the\n> references in refs/remotes/my-remote/*\n\nYeah, that was roughly my thinking.\n\n> So the changes that are required are:\n> * Adding a notification-only post-fetch hook\n> * For handling tags, there is a need to have a refmap for tags. Maybe\n> adding a remote.my-remote.fetchTags refmap, that would be used when\n> running with --tags, and having it default to “refs/tags/*:refs/tags/*”\n> to keep the current behavior by default?\n\nYeah, tag-following may be a little tricky, because it usually wants to\nwrite to refs/tags/. One workaround would be to have your config look\nlike this:\n\n [remote \"origin\"]\n fetch = +refs/heads/*:refs/quarantine/origin/heads/*\n fetch = +refs/tags/*:refs/quarantine/origin/tags/*\n tagOpt = --no-tags\n\nThat's not exactly the same thing, because it would fetch all tags, not\njust those that point to the history on the branches. But in most\nrepositories and workflows the distinction doesn't matter.\n\n(By the way, the I specifically chose the name \"refs/quarantine\" instead\nof anything in \"refs/remotes\" because we'd want to make sure that the\n\"git checkout\" DWIM behavior cannot accidentally pull from quarantine).\n\n> The only remaining issue I can think of is: How do we avoid the issue\n> of the\n> trigger-only-hook-inciting-bad-behavior-by-hook-authors-who-really-want-modification\n> raised in the side-thread that Junio wrote in [1]? Maybe just writing\n> in the documentation that the hook should use a quarantine-like\n> approach if it wants modification would be enough to not have hook\n> authors try to modify the ref in the post-fetch hook?\n\nI don't have a silver bullet there. Documenting the \"right\" way at least\nseems like a good first step.\n\n-Peff\n"},{"id":"338964","messageId":"3de8dec0-12c9-56e2-5902-97755f78ab50@gaspard.io","threadId":"47783","inReplyTo":"20180210001317.GA26856@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-10T00:37:20Z","receivedAt":"2018-02-10T00:37:26Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/10/2018 01:13 AM, Jeff King wrote:\n> On Sat, Feb 10, 2018 at 12:49:31AM +0100, Leo Gaspard wrote:\n>> So the changes that are required are:\n>> * Adding a notification-only post-fetch hook\n>> * For handling tags, there is a need to have a refmap for tags. Maybe\n>> adding a remote.my-remote.fetchTags refmap, that would be used when\n>> running with --tags, and having it default to “refs/tags/*:refs/tags/*”\n>> to keep the current behavior by default?\n> \n> Yeah, tag-following may be a little tricky, because it usually wants to\n> write to refs/tags/. One workaround would be to have your config look\n> like this:\n> \n> [remote \"origin\"]\n> fetch = +refs/heads/*:refs/quarantine/origin/heads/*\n> fetch = +refs/tags/*:refs/quarantine/origin/tags/*\n> tagOpt = --no-tags\n> \n> That's not exactly the same thing, because it would fetch all tags, not\n> just those that point to the history on the branches. But in most\n> repositories and workflows the distinction doesn't matter.\n\nHmm... apart from the implementation complexity (of which I have no\nidea), is there an argument against the idea of adding a\nremote..fetchTagsTo refmap similar to remote..fetch but used\nevery time a tag is fetched? (well, maybe not exactly similar to\nremote..fetch because we know the source is going to be\nrefs/tags/*; so just having the part of .fetch past the ':' would be\nmore like what's needed I guess)\n\nThe issue with your solution is that if the user runs 'git fetch\n--tags', he will get the (potentially compromised) tags directly in his\nrefs/tags/.\n\n> (By the way, the I specifically chose the name \"refs/quarantine\" instead\n> of anything in \"refs/remotes\" because we'd want to make sure that the\n> \"git checkout\" DWIM behavior cannot accidentally pull from quarantine).\n\n(Indeed, I understood after reading it, and would likely not have\nthought of it otherwise, thanks!)\n\n>> The only remaining issue I can think of is: How do we avoid the issue\n>> of the\n>> trigger-only-hook-inciting-bad-behavior-by-hook-authors-who-really-want-modification\n>> raised in the side-thread that Junio wrote in [1]? Maybe just writing\n>> in the documentation that the hook should use a quarantine-like\n>> approach if it wants modification would be enough to not have hook\n>> authors try to modify the ref in the post-fetch hook?\n> \n> I don't have a silver bullet there. Documenting the \"right\" way at least\n> seems like a good first step.\n\nSo long as it's not a merge-blocker it's good with me! (but then I'm\nlikely not the one who's going to be pointed at when things go wrong in\na hook, so I'm clearly biased on this matter)\n"},{"id":"338967","messageId":"xmqqvaf5tzw6.fsf@gitster-ct.c.googlers.com","threadId":"47783","inReplyTo":"3de8dec0-12c9-56e2-5902-97755f78ab50@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-10T01:08:25Z","receivedAt":"2018-02-10T01:08:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Leo Gaspard writes:\n\n> On 02/10/2018 01:13 AM, Jeff King wrote:\n>> On Sat, Feb 10, 2018 at 12:49:31AM +0100, Leo Gaspard wrote:\n>>> So the changes that are required are:\n>>> * Adding a notification-only post-fetch hook\n\nMaybe I missed a very early part of the discussion, but why does\nthis even need a hook? There are some numbers [*1*] of classes of\nvalid reasons we may want to have hooks triggered by operations, but\n\"always do something locally after doing something else locally,\nregardless of the outcome of that something else\" feels like the\nmost typical anti-pattern that we do not want a hook for. If you\nare doing \"git fetch\" (or \"git pull\"), you already know you are\ndoing that and you donot need a notification. You just create a\nworkflow specific script that calls fetch or pull, followed by\nwhatever you want to do and use that, instead of doing \"git pull\",\nand that is not any extra work than writing a hook and installing\nit.\n\nUnlike something like post-receive, which happens on the remote side\nwhere you may not even have an interactive access to, in response to\nthe operation you locally do (i.e. \"git push\"), fetching and then\ndoing something else in a repository you fetch into has no reason to\nbe done as a hook.\n\n[Footnote]\n\n*1* I think the number was 5 when I last counted/enumerated, but\n don't quote me on that ;-)\n"},{"id":"338969","messageId":"9ab95c53-61e5-06a3-535e-a8916b3e5ec1@gaspard.io","threadId":"47783","inReplyTo":"xmqqvaf5tzw6.fsf@gitster-ct.c.googlers.com","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-10T01:33:19Z","receivedAt":"2018-02-10T01:33:26Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/10/2018 02:08 AM, Junio C Hamano wrote:\n> Leo Gaspard writes:\n> \n>> On 02/10/2018 01:13 AM, Jeff King wrote:\n>>> On Sat, Feb 10, 2018 at 12:49:31AM +0100, Leo Gaspard wrote:\n>>>> So the changes that are required are:\n>>>> * Adding a notification-only post-fetch hook\n> \n> Maybe I missed a very early part of the discussion, but why does\n> this even need a hook? There are some numbers [*1*] of classes of\n> valid reasons we may want to have hooks triggered by operations, but\n> \"always do something locally after doing something else locally,\n> regardless of the outcome of that something else\" feels like the\n> most typical anti-pattern that we do not want a hook for. If you\n> are doing \"git fetch\" (or \"git pull\"), you already know you are\n> doing that and you donot need a notification. You just create a\n> workflow specific script that calls fetch or pull, followed by\n> whatever you want to do and use that, instead of doing \"git pull\",\n> and that is not any extra work than writing a hook and installing\n> it.\n> \n> Unlike something like post-receive, which happens on the remote side\n> where you may not even have an interactive access to, in response to\n> the operation you locally do (i.e. \"git push\"), fetching and then\n> doing something else in a repository you fetch into has no reason to\n> be done as a hook.\n\nI guess the very early part of the discussion you're speaking of is what\nI was assuming after reading\n https://marc.info/?l=git&m=132478296309094&w=2\n\nThen, it's always possible to just write workflow-specific scripts that\nmanually run git fetch then copy the refs (resp. run git fetch then copy\nthe refs then run git merge) to get git myfetch (resp. git mypull) [1].\n\nBut then, the question is, why is there a pre-push hook? it's already\npossible to have a custom script that first runs the hook then runs git\npush, for most if not all of the use cases of the pre-push hook. Yet the\npossibility to not change the end-user's workflow is what makes pre-push\n(or pre-commit, with which the similarity is perhaps even more obvious)\nso useful.\n\nSo the reason for a post-fetch in my opinion is the same as for a\npre-push / pre-commit: not changing the user's workflow, while providing\nadditional features.\n\n\n[1]\n\nWhich makes me notice that actually the post-fetch hook technique we\nwere discussing with Peff suffers the same not-updating-FETCH_HEAD issue\nthat was discussed in this early thread: a `git pull` would try to merge\nfrom refs/quarantine, I guess. So things are a bit harder than we thought.\n\nI guess the tweak-fetch hook could be left “as-is”, but with git\nautomatically doing the “quarantine-ing” thing transparently so that the\nreferences that the end-user (or the hook for that matter) see are the\n“curated” ones? Then it's too late for me to think efficiently right\nnow, so that idea may be stupid or over-complex.\n"},{"id":"338985","messageId":"20180210122131.GB21843@sigill.intra.peff.net","threadId":"47783","inReplyTo":"3de8dec0-12c9-56e2-5902-97755f78ab50@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-10T12:21:31Z","receivedAt":"2018-02-10T12:21:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 10, 2018 at 01:37:20AM +0100, Leo Gaspard wrote:\n\n> > Yeah, tag-following may be a little tricky, because it usually wants to\n> > write to refs/tags/. One workaround would be to have your config look\n> > like this:\n> > \n> > [remote \"origin\"]\n> > fetch = +refs/heads/*:refs/quarantine/origin/heads/*\n> > fetch = +refs/tags/*:refs/quarantine/origin/tags/*\n> > tagOpt = --no-tags\n> > \n> > That's not exactly the same thing, because it would fetch all tags, not\n> > just those that point to the history on the branches. But in most\n> > repositories and workflows the distinction doesn't matter.\n> \n> Hmm... apart from the implementation complexity (of which I have no\n> idea), is there an argument against the idea of adding a\n> remote..fetchTagsTo refmap similar to remote..fetch but used\n> every time a tag is fetched? (well, maybe not exactly similar to\n> remote..fetch because we know the source is going to be\n> refs/tags/*; so just having the part of .fetch past the ':' would be\n> more like what's needed I guess)\n\nI don't think it would be too hard to implement, and is at least\nlogically consistent with the way we handle tags.\n\nIf we were designing from scratch, I do think this would all be helped\nimmensely by having more separation of refs fetched from remotes. There\nwas a proposal in the v1.8 era to fetch everything for a remote,\nincluding tags, into \"refs/remotes/origin/heads/\",\n\"refs/remotes/origin/tags/\", etc. And then we'd teach the lookup side to\nlook for tags in each of the remote-tag namespaces.\n\nI still think that would be a good direction to go, but it would be a\nbig project (which is why the original stalled).\n\n> The issue with your solution is that if the user runs 'git fetch\n> --tags', he will get the (potentially compromised) tags directly in his\n> refs/tags/.\n\nTrue.\n\n-Peff\n"},{"id":"338990","messageId":"92130c51-9e5d-39e9-3129-046295ecb353@gaspard.io","threadId":"47783","inReplyTo":"9ab95c53-61e5-06a3-535e-a8916b3e5ec1@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-10T18:03:35Z","receivedAt":"2018-02-10T18:03:41Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/10/2018 02:33 AM, Leo Gaspard wrote:> I guess the very early part\nof the discussion you're speaking of is what\n> I was assuming after reading\n> https://marc.info/?l=git&m=132478296309094&w=2\n> \n> [...]\n> \n> So the reason for a post-fetch in my opinion is the same as for a\n> pre-push / pre-commit: not changing the user's workflow, while providing\n> additional features.\nWell, re-reading my email, it looks aggressive to me now... sorry about\nthat!\n\nWhat I meant was basically, that in my mind pre-push or pre-commit are\nalso local-only things, and they are really useful in that they allow to\nblock the push or the commit if some conditions are not met (eg. block\ncommit with trailing whitespace, or block push of unsigned commits).\n\nIn pretty much the same way, what I'm really looking for is a way to\n“block the fetch” (from a user-visible standpoint) -- like pre-push but\nin the opposite direction. I hope such a goal is not an anti-pattern for\nhook design?\n\nIn order to do this, I first tried updating this tweak-fetch hook patch\nfrom late 2011, and then learned that it would cause too high a load on\nservers. Then, while brainstorming another solution, this idea of a\nnotification-only post-fetch hook arose. But, as I noticed while writing\nmy previous answer, this suffers the same\nthe-hook-writer-must-correctly-update-FETCH_HEAD-concurrently-with-remote-branch\nissue that you pointed out just after the “*HOWEVER*” in [1]. So that\nsolution is likely a bad solution too. I guess we'll continue the search\nfor a good solution in the side-thread with Peff, hoping to figure one out.\n\nThat being said about what I meant in my last email, obviously you're\nthe one who has the say on what goes in or not! And if it's an\nanti-feature I'd much rather know it now than after spending a few\nnights coding :)\n\nSo, what do you think about this use case I'm thinking of? (ie.\n“blocking the fetch” like pre-push “blocks the push” and pre-commit\n“blocks the commit”?)\n\n\n[1] https://marc.info/?l=git&m=132478296309094&w=2\n"},{"id":"338991","messageId":"5abf8565-1aa1-c101-83a7-90781682bc7a@gaspard.io","threadId":"47783","inReplyTo":"20180210122131.GB21843@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-10T18:36:47Z","receivedAt":"2018-02-10T18:36:53Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/10/2018 01:21 PM, Jeff King wrote:\n> On Sat, Feb 10, 2018 at 01:37:20AM +0100, Leo Gaspard wrote:\n> \n>>> Yeah, tag-following may be a little tricky, because it usually wants to\n>>> write to refs/tags/. One workaround would be to have your config look\n>>> like this:\n>>>\n>>> [remote \"origin\"]\n>>> fetch = +refs/heads/*:refs/quarantine/origin/heads/*\n>>> fetch = +refs/tags/*:refs/quarantine/origin/tags/*\n>>> tagOpt = --no-tags\n>>>\n>>> That's not exactly the same thing, because it would fetch all tags, not\n>>> just those that point to the history on the branches. But in most\n>>> repositories and workflows the distinction doesn't matter.\n>>\n>> Hmm... apart from the implementation complexity (of which I have no\n>> idea), is there an argument against the idea of adding a\n>> remote..fetchTagsTo refmap similar to remote..fetch but used\n>> every time a tag is fetched? (well, maybe not exactly similar to\n>> remote..fetch because we know the source is going to be\n>> refs/tags/*; so just having the part of .fetch past the ':' would be\n>> more like what's needed I guess)\n> \n> I don't think it would be too hard to implement, and is at least\n> logically consistent with the way we handle tags.\n> \n> If we were designing from scratch, I do think this would all be helped\n> immensely by having more separation of refs fetched from remotes. There\n> was a proposal in the v1.8 era to fetch everything for a remote,\n> including tags, into \"refs/remotes/origin/heads/\",\n> \"refs/remotes/origin/tags/\", etc. And then we'd teach the lookup side to\n> look for tags in each of the remote-tag namespaces.\n> \n> I still think that would be a good direction to go, but it would be a\n> big project (which is why the original stalled).\n\nHmm... would this also drown the remote..fetch map? Also, I think\nthis behavior could be emulated with fetch and fetchTagsTo, and it would\nlook like:\n[remote \"my-remote\"]\n fetch = +refs/heads/*:refs/remotes/my-remote/heads/*\n fetchTagsTo = refs/remotes/my-remote/tags/*\nThe remaining issue being to teach the lookup side to look for tags in\nall the remote-tag namespaces (and the fact it's a breaking change).\n\nThat said, actually I just noticed an issue in the “add a\nremote..fetch option to fetch to refs/quarantine then have the\npost-fetch hook do the work”: it means if I run `git pull`, then:\n 1. The remote references will be pulled to refs/quarantine/...\n 2. The post-fetch hook will copy the accepted ones to refs/remotes/...\n 3. The `git merge FETCH_HEAD` called by pull will merge FETCH_HEAD into\nlocal branches... and so merge from refs/quarantine.\n\nA solution would be to also update FETCH_HEAD in the post-fetch hook,\nbut then we're back to the issue raised by Junio after the “*HOWEVER*”\nof [1]: the hook writer has to maintain consistency between the “copied”\nreferences and FETCH_HEAD.\n\nSo, when thinking about it, I'm back to thinking the proper hook\ninterface should be the one of the tweak-fetch hook, but its\nimplementation should make it not go crazy on remote servers. And so\nthat the implementation should do all this refs/quarantine wizardry\ninside git itself.\n\nSo basically what I'm getting at at the moment is that git fetch should:\n 1. fetch all the references to refs/real-remotes//{insert here a\ncopy of the refs/ tree of }\n 2. figure out what the “expected” names for these references will by,\nby looking at remote..fetch (and at whether --tags was passed)\n 3. for each “expected” name,\n 1. if a tweak-fetch hook is present, call it with the\nrefs/real-remotes/... refname and the “expected end-name” from\nremote..fetch\n 2. based on the hook's result, potentially to move the “expected\nend-name” to another commit than the one referenced by refs/real-remotes/...\n 3. move the “expected” name to the commit referenced in\nrefs/real-remotes\n\nWhich would make the tweak-fetch hook interface simpler (though more\nrestrictive, but I don't have any real use case for the other change\npossibilities) than it is now:\n 1. feed the hook with lines of\n“refs/real-remotes/my-remote/heads/my-branch\nrefs/remotes/my-remote/my-branch” (assuming remote.my-remote.fetch is\n“normal”) or “refs/real-remotes/my-remote/tags/my-tag refs/tags/my-tag”\n(if my-tag is being fetched from my-remote)\n 2. read lines of “ refs/remotes/my-remote/my-branch”, that\nwill re-point my-branch to instead of\nrefs/real-remotes/my-remote/heads/my-branch. In order to avoid any\nissue, the hook is not allowed to pass as second output a reference that\nwas not passed as second input.\n\nThis way, the behavior of the tweak-fetch hook is reasonably preserved\n(at least for my use case), and there is no additional load on the\nservers thanks to the up-to-date references being stored in\nrefs/real-remotes//\n\nDoes this reasoning make any sense?\n\n\n[1] https://marc.info/?l=git&m=132478296309094&w=2\n"},{"id":"339094","messageId":"20180212192327.GA209601@google.com","threadId":"47783","inReplyTo":"5abf8565-1aa1-c101-83a7-90781682bc7a@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-02-12T19:23:27Z","receivedAt":"2018-02-12T19:23:36Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/10, Leo Gaspard wrote:\n> On 02/10/2018 01:21 PM, Jeff King wrote:\n> > On Sat, Feb 10, 2018 at 01:37:20AM +0100, Leo Gaspard wrote:\n> > \n> >>> Yeah, tag-following may be a little tricky, because it usually wants to\n> >>> write to refs/tags/. One workaround would be to have your config look\n> >>> like this:\n> >>>\n> >>> [remote \"origin\"]\n> >>> fetch = +refs/heads/*:refs/quarantine/origin/heads/*\n> >>> fetch = +refs/tags/*:refs/quarantine/origin/tags/*\n> >>> tagOpt = --no-tags\n> >>>\n> >>> That's not exactly the same thing, because it would fetch all tags, not\n> >>> just those that point to the history on the branches. But in most\n> >>> repositories and workflows the distinction doesn't matter.\n> >>\n> >> Hmm... apart from the implementation complexity (of which I have no\n> >> idea), is there an argument against the idea of adding a\n> >> remote..fetchTagsTo refmap similar to remote..fetch but used\n> >> every time a tag is fetched? (well, maybe not exactly similar to\n> >> remote..fetch because we know the source is going to be\n> >> refs/tags/*; so just having the part of .fetch past the ':' would be\n> >> more like what's needed I guess)\n> > \n> > I don't think it would be too hard to implement, and is at least\n> > logically consistent with the way we handle tags.\n> > \n> > If we were designing from scratch, I do think this would all be helped\n> > immensely by having more separation of refs fetched from remotes. There\n> > was a proposal in the v1.8 era to fetch everything for a remote,\n> > including tags, into \"refs/remotes/origin/heads/\",\n> > \"refs/remotes/origin/tags/\", etc. And then we'd teach the lookup side to\n> > look for tags in each of the remote-tag namespaces.\n> > \n> > I still think that would be a good direction to go, but it would be a\n> > big project (which is why the original stalled).\n> \n> Hmm... would this also drown the remote..fetch map? Also, I think\n> this behavior could be emulated with fetch and fetchTagsTo, and it would\n> look like:\n> [remote \"my-remote\"]\n> fetch = +refs/heads/*:refs/remotes/my-remote/heads/*\n> fetchTagsTo = refs/remotes/my-remote/tags/*\n> The remaining issue being to teach the lookup side to look for tags in\n> all the remote-tag namespaces (and the fact it's a breaking change).\n> \n> That said, actually I just noticed an issue in the “add a\n> remote..fetch option to fetch to refs/quarantine then have the\n> post-fetch hook do the work”: it means if I run `git pull`, then:\n> 1. The remote references will be pulled to refs/quarantine/...\n> 2. The post-fetch hook will copy the accepted ones to refs/remotes/...\n> 3. The `git merge FETCH_HEAD` called by pull will merge FETCH_HEAD into\n> local branches... and so merge from refs/quarantine.\n> \n> A solution would be to also update FETCH_HEAD in the post-fetch hook,\n> but then we're back to the issue raised by Junio after the “*HOWEVER*”\n> of [1]: the hook writer has to maintain consistency between the “copied”\n> references and FETCH_HEAD.\n> \n> So, when thinking about it, I'm back to thinking the proper hook\n> interface should be the one of the tweak-fetch hook, but its\n> implementation should make it not go crazy on remote servers. And so\n> that the implementation should do all this refs/quarantine wizardry\n> inside git itself.\n> \n> So basically what I'm getting at at the moment is that git fetch should:\n> 1. fetch all the references to refs/real-remotes//{insert here a\n> copy of the refs/ tree of }\n> 2. figure out what the “expected” names for these references will by,\n> by looking at remote..fetch (and at whether --tags was passed)\n> 3. for each “expected” name,\n> 1. if a tweak-fetch hook is present, call it with the\n> refs/real-remotes/... refname and the “expected end-name” from\n> remote..fetch\n> 2. based on the hook's result, potentially to move the “expected\n> end-name” to another commit than the one referenced by refs/real-remotes/...\n> 3. move the “expected” name to the commit referenced in\n> refs/real-remotes\n> \n> Which would make the tweak-fetch hook interface simpler (though more\n> restrictive, but I don't have any real use case for the other change\n> possibilities) than it is now:\n> 1. feed the hook with lines of\n> “refs/real-remotes/my-remote/heads/my-branch\n> refs/remotes/my-remote/my-branch” (assuming remote.my-remote.fetch is\n> “normal”) or “refs/real-remotes/my-remote/tags/my-tag refs/tags/my-tag”\n> (if my-tag is being fetched from my-remote)\n> 2. read lines of “ refs/remotes/my-remote/my-branch”, that\n> will re-point my-branch to instead of\n> refs/real-remotes/my-remote/heads/my-branch. In order to avoid any\n> issue, the hook is not allowed to pass as second output a reference that\n> was not passed as second input.\n> \n> This way, the behavior of the tweak-fetch hook is reasonably preserved\n> (at least for my use case), and there is no additional load on the\n> servers thanks to the up-to-date references being stored in\n> refs/real-remotes//\n> \n> Does this reasoning make any sense?\n> \n> \n> [1] https://marc.info/?l=git&m=132478296309094&w=2\n\n\nMaybe this isn't helpful but you may be able to implement this by using\na remote-helper. The helper could perform any sort of caching it needed\nto prevent re-downloading large amounts of data that is potentially\nthrown away, while only sending through the relevant commits which\nsatisfy some criteria (signed, etc.).\n\n-- \nBrandon Williams\n"},{"id":"339200","messageId":"461e515c-350a-50f2-bc13-6a976df489fd@gaspard.io","threadId":"47783","inReplyTo":"20180212192327.GA209601@google.com","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-13T15:44:30Z","receivedAt":"2018-02-13T15:44:41Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/12/2018 08:23 PM, Brandon Williams wrote:> Maybe this isn't\nhelpful but you may be able to implement this by using\n> a remote-helper. The helper could perform any sort of caching it needed\n> to prevent re-downloading large amounts of data that is potentially\n> thrown away, while only sending through the relevant commits which\n> satisfy some criteria (signed, etc.).\n\nThis looks like a possibility, thanks! I'll likely try to implement it\nthis way if there can be no consensus on the features wanted for a\nfetch-hook (as the current state of discussion looks like to me).\n"},{"id":"339252","messageId":"20180214013520.GA25188@sigill.intra.peff.net","threadId":"47783","inReplyTo":"5abf8565-1aa1-c101-83a7-90781682bc7a@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T01:35:20Z","receivedAt":"2018-02-14T01:35:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 10, 2018 at 07:36:47PM +0100, Leo Gaspard wrote:\n\n> Hmm... would this also drown the remote..fetch map? Also, I think\n> this behavior could be emulated with fetch and fetchTagsTo, and it would\n> look like:\n> [remote \"my-remote\"]\n> fetch = +refs/heads/*:refs/remotes/my-remote/heads/*\n> fetchTagsTo = refs/remotes/my-remote/tags/*\n> The remaining issue being to teach the lookup side to look for tags in\n> all the remote-tag namespaces (and the fact it's a breaking change).\n\nRight, I think fetching into the right spots is the easy part. Designing\nthe new lookup rules is the tricky part.\n\nIf you're really interested in the gory details, here's a very old\ndiscussion on it:\n\n https://public-inbox.org/git/AANLkTi=yFwOAQMHhvLsB1_xmYOE9HHP2YB4H4TQzwwc8@mail.gmail.com/\n\nI think there may have been some more concrete proposals after that, but\nthat's what I was able to dig up quickly.\n\n> That said, actually I just noticed an issue in the “add a\n> remote..fetch option to fetch to refs/quarantine then have the\n> post-fetch hook do the work”: it means if I run `git pull`, then:\n> 1. The remote references will be pulled to refs/quarantine/...\n> 2. The post-fetch hook will copy the accepted ones to refs/remotes/...\n> 3. The `git merge FETCH_HEAD` called by pull will merge FETCH_HEAD into\n> local branches... and so merge from refs/quarantine.\n\nGood point. You can't munge FETCH_HEAD by playing with refspecs.\n\nI am starting to come around to the idea that \"pre-fetch\" might be the\nbest way to do what you want. Not to rewrite refs, but perhaps to simply\nreject them. In the same way that we allow pre-receive to reject pushed\nrefs (both are, after all, the final check on admitting new history into\nthe repository, just in opposite directions).\n\n> So, when thinking about it, I'm back to thinking the proper hook\n> interface should be the one of the tweak-fetch hook, but its\n> implementation should make it not go crazy on remote servers. And so\n> that the implementation should do all this refs/quarantine wizardry\n> inside git itself.\n\nSo does anybody actually want to be able to adjust the refs as they pass\nthrough? It really sounds like you just want to be able to reject or not\nreject the fetch. And that rejecting would be the uncommon case, so it's\nOK to just abort the whole operation and expect the user to figure it\nout.\n\n-Peff\n"},{"id":"339254","messageId":"20180214013848.GB25188@sigill.intra.peff.net","threadId":"47783","inReplyTo":"20180212192327.GA209601@google.com","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T01:38:48Z","receivedAt":"2018-02-14T01:38:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 11:23:27AM -0800, Brandon Williams wrote:\n\n> Maybe this isn't helpful but you may be able to implement this by using\n> a remote-helper. The helper could perform any sort of caching it needed\n> to prevent re-downloading large amounts of data that is potentially\n> thrown away, while only sending through the relevant commits which\n> satisfy some criteria (signed, etc.).\n\nInteresting idea, though it would still be calling git-fetch under the\nhood, right? So I guess we're just reimplementing this \"quarantine\"\nconcept, but in theory we'd have the flexibility to store that\nquarantine in another one-off sub-repository (instead of a special ref\nnamespace, though I suppose you could use the special ref namespace,\ntoo).\n\nI suspect it could work, but there would probably be a lot of rough\nedges.\n\n-Peff\n"},{"id":"339255","messageId":"CA+P7+xr5FfP=mnkhgC1AjxLQn_=X3m5o4caGmqjePqqj9FEG6w@mail.gmail.com","threadId":"47783","inReplyTo":"20180210122131.GB21843@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-02-14T01:46:13Z","receivedAt":"2018-02-14T01:46:39Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Feb 10, 2018 at 4:21 AM, Jeff King wrote:\n> On Sat, Feb 10, 2018 at 01:37:20AM +0100, Leo Gaspard wrote:\n>\n>> > Yeah, tag-following may be a little tricky, because it usually wants to\n>> > write to refs/tags/. One workaround would be to have your config look\n>> > like this:\n>> >\n>> > [remote \"origin\"]\n>> > fetch = +refs/heads/*:refs/quarantine/origin/heads/*\n>> > fetch = +refs/tags/*:refs/quarantine/origin/tags/*\n>> > tagOpt = --no-tags\n>> >\n>> > That's not exactly the same thing, because it would fetch all tags, not\n>> > just those that point to the history on the branches. But in most\n>> > repositories and workflows the distinction doesn't matter.\n>>\n>> Hmm... apart from the implementation complexity (of which I have no\n>> idea), is there an argument against the idea of adding a\n>> remote..fetchTagsTo refmap similar to remote..fetch but used\n>> every time a tag is fetched? (well, maybe not exactly similar to\n>> remote..fetch because we know the source is going to be\n>> refs/tags/*; so just having the part of .fetch past the ':' would be\n>> more like what's needed I guess)\n>\n> I don't think it would be too hard to implement, and is at least\n> logically consistent with the way we handle tags.\n>\n> If we were designing from scratch, I do think this would all be helped\n> immensely by having more separation of refs fetched from remotes. There\n> was a proposal in the v1.8 era to fetch everything for a remote,\n> including tags, into \"refs/remotes/origin/heads/\",\n> \"refs/remotes/origin/tags/\", etc. And then we'd teach the lookup side to\n> look for tags in each of the remote-tag namespaces.\n>\n> I still think that would be a good direction to go, but it would be a\n> big project (which is why the original stalled).\n>\n\nI want to go this direction, but the difficulty has always been\nlimited time to actually work on such a large project.\n\nThanks,\nJake\n\n>> The issue with your solution is that if the user runs 'git fetch\n>> --tags', he will get the (potentially compromised) tags directly in his\n>> refs/tags/.\n>\n> True.\n>\n> -Peff\n"},{"id":"339257","messageId":"96dd7fb3-849b-8de6-7c3a-cd6bde9da432@gaspard.io","threadId":"47783","inReplyTo":"20180214013520.GA25188@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-14T02:02:00Z","receivedAt":"2018-02-14T02:02:07Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/14/2018 02:35 AM, Jeff King wrote:\n> On Sat, Feb 10, 2018 at 07:36:47PM +0100, Leo Gaspard wrote:\n>> [...]\n> I think there may have been some more concrete proposals after that, but\n> that's what I was able to dig up quickly.\n\nThanks! Though it looks way above my knowledge of git internals as well\nas time available, it looks like a great project, and I hope it someday\nsucceeds!\n\n>> That said, actually I just noticed an issue in the “add a\n>> remote..fetch option to fetch to refs/quarantine then have the\n>> post-fetch hook do the work”: it means if I run `git pull`, then:\n>> 1. The remote references will be pulled to refs/quarantine/...\n>> 2. The post-fetch hook will copy the accepted ones to refs/remotes/...\n>> 3. The `git merge FETCH_HEAD` called by pull will merge FETCH_HEAD into\n>> local branches... and so merge from refs/quarantine.\n> \n> Good point. You can't munge FETCH_HEAD by playing with refspecs.\n> \n> I am starting to come around to the idea that \"pre-fetch\" might be the\n> best way to do what you want. Not to rewrite refs, but perhaps to simply\n> reject them. In the same way that we allow pre-receive to reject pushed\n> refs (both are, after all, the final check on admitting new history into\n> the repository, just in opposite directions).\n> \n>> So, when thinking about it, I'm back to thinking the proper hook\n>> interface should be the one of the tweak-fetch hook, but its\n>> implementation should make it not go crazy on remote servers. And so\n>> that the implementation should do all this refs/quarantine wizardry\n>> inside git itself.\n> \n> So does anybody actually want to be able to adjust the refs as they pass\n> through? It really sounds like you just want to be able to reject or not\n> reject the fetch. And that rejecting would be the uncommon case, so it's\n> OK to just abort the whole operation and expect the user to figure it\n> out.\n\nThis completely fits my use case (modulo the fact that it's more similar\nto the `update` hook than to `pre-receive` I think, as verifying the\nsignature requires the objects to already have been downloaded), indeed,\nthough I'm not sure it would have fit Joey's (based on my understanding,\nadding a merge was what was originally asked for).\n\nActually, I'm wondering if the existing semantics of `update` could not\nbe reused for the `pre-fetch`. Would it make sense to just call `update`\nduring a fetch in the same way as during a receive-pack? That said it\nlikely makes this a breaking change, though it's maybe unlikely that a\nrepository is used both for fetching and for receive-pack'ing, it could\nhappen.\n\nSo the proposal as I understand it would currently be adding a\n`fetch-update` hook that does exactly the same thing as `update` but for\n`fetch`. This solves my use case in a nice way, though it likely doesn't\nsolve Joey's original one (which has been taken care of by a wrapper\nsince then).\n\nWhat do you all think about it?\n"},{"id":"339640","messageId":"20180219212347.GA9748@sigill.intra.peff.net","threadId":"47783","inReplyTo":"96dd7fb3-849b-8de6-7c3a-cd6bde9da432@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-19T21:23:47Z","receivedAt":"2018-02-19T21:23:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 14, 2018 at 03:02:00AM +0100, Leo Gaspard wrote:\n\n> > So does anybody actually want to be able to adjust the refs as they pass\n> > through? It really sounds like you just want to be able to reject or not\n> > reject the fetch. And that rejecting would be the uncommon case, so it's\n> > OK to just abort the whole operation and expect the user to figure it\n> > out.\n> \n> This completely fits my use case (modulo the fact that it's more similar\n> to the `update` hook than to `pre-receive` I think, as verifying the\n> signature requires the objects to already have been downloaded), indeed,\n> though I'm not sure it would have fit Joey's (based on my understanding,\n> adding a merge was what was originally asked for).\n> \n> Actually, I'm wondering if the existing semantics of `update` could not\n> be reused for the `pre-fetch`. Would it make sense to just call `update`\n> during a fetch in the same way as during a receive-pack? That said it\n> likely makes this a breaking change, though it's maybe unlikely that a\n> repository is used both for fetching and for receive-pack'ing, it could\n> happen.\n> \n> So the proposal as I understand it would currently be adding a\n> `fetch-update` hook that does exactly the same thing as `update` but for\n> `fetch`. This solves my use case in a nice way, though it likely doesn't\n> solve Joey's original one (which has been taken care of by a wrapper\n> since then).\n> \n> What do you all think about it?\n\nI think it should be a separate hook; having an existing hook trigger in\nnew places is likely to cause confusion and regressions.\n\nIf you do go this route, please model it after \"pre-receive\" rather than\n\"update\". We had \"update\" originally but found it was too limiting for\nhooks to see only one ref at a time. So we introduced pre-receive. The\n\"update\" hook remains for historical reasons, but I don't think we'd\nwant to reproduce the mistake. :)\n\n-Peff\n"},{"id":"339648","messageId":"cdef8e46-92ee-2062-385a-999b5b49ae9a@gaspard.io","threadId":"47783","inReplyTo":"20180219212347.GA9748@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-19T22:50:37Z","receivedAt":"2018-02-19T22:51:02Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/19/2018 10:23 PM, Jeff King wrote:\n> [...]\n> If you do go this route, please model it after \"pre-receive\" rather than\n> \"update\". We had \"update\" originally but found it was too limiting for\n> hooks to see only one ref at a time. So we introduced pre-receive. The\n> \"update\" hook remains for historical reasons, but I don't think we'd\n> want to reproduce the mistake. :)\n\nHmm, what bothered me with “pre-receive” was that it was an\nall-or-nothing decision, without the ability to allow some references\nthrough and not others.\n\nIs there a way for “pre-receive” to individually filter hooks? I was\nunder the impression that the only way to do that was to use the\n“update” hook, which was the reason I wanted to model it after “update”\nrather than “pre-receive” (my use case being a check independent for\neach pushed ref)\n"},{"id":"339691","messageId":"CA+P7+xrb0PCsJtQbrT+E9hNhZ8yZ5qeR7vVGVYFtqEOeoK77kw@mail.gmail.com","threadId":"47783","inReplyTo":"cdef8e46-92ee-2062-385a-999b5b49ae9a@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-02-20T06:10:08Z","receivedAt":"2018-02-20T06:10:34Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 19, 2018 at 2:50 PM, Leo Gaspard wrote:\n> On 02/19/2018 10:23 PM, Jeff King wrote:\n>> [...]\n>> If you do go this route, please model it after \"pre-receive\" rather than\n>> \"update\". We had \"update\" originally but found it was too limiting for\n>> hooks to see only one ref at a time. So we introduced pre-receive. The\n>> \"update\" hook remains for historical reasons, but I don't think we'd\n>> want to reproduce the mistake. :)\n>\n> Hmm, what bothered me with “pre-receive” was that it was an\n> all-or-nothing decision, without the ability to allow some references\n> through and not others.\n>\n> Is there a way for “pre-receive” to individually filter hooks? I was\n> under the impression that the only way to do that was to use the\n> “update” hook, which was the reason I wanted to model it after “update”\n> rather than “pre-receive” (my use case being a check independent for\n> each pushed ref)\n\nAt least in the \"push\" case I think there is value in saying \"if one\nref fails, please fail the entire push, always\". This makes sense,\nbecause if a user happens to violate some rule in one thing, they very\nlikely may not wish any of the push to succeed, and it also creates\nproblems with whether a push is atomic or not.\n\nFor fetch, I could see either method being useful.\n\nThanks,\nJake\n"},{"id":"339692","messageId":"20180220074201.GA2154@sigill.intra.peff.net","threadId":"47783","inReplyTo":"cdef8e46-92ee-2062-385a-999b5b49ae9a@gaspard.io","subject":"Re: Fetch-hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-20T07:42:01Z","receivedAt":"2018-02-20T07:42:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 19, 2018 at 11:50:37PM +0100, Leo Gaspard wrote:\n\n> On 02/19/2018 10:23 PM, Jeff King wrote:\n> > [...]\n> > If you do go this route, please model it after \"pre-receive\" rather than\n> > \"update\". We had \"update\" originally but found it was too limiting for\n> > hooks to see only one ref at a time. So we introduced pre-receive. The\n> > \"update\" hook remains for historical reasons, but I don't think we'd\n> > want to reproduce the mistake. :)\n> \n> Hmm, what bothered me with “pre-receive” was that it was an\n> all-or-nothing decision, without the ability to allow some references\n> through and not others.\n> \n> Is there a way for “pre-receive” to individually filter hooks? I was\n> under the impression that the only way to do that was to use the\n> “update” hook, which was the reason I wanted to model it after “update”\n> rather than “pre-receive” (my use case being a check independent for\n> each pushed ref)\n\nNo, pre-receive is always atomic. However, that may actually be what you\nwant, if the point is to keep untrusted data out of the repository. By\nrejecting the whole thing, we could in theory keep the objects from\nentering the repository at all. This is how the push side works via the\n\"quarantine\" system (which is explained in git-receive-pack(1)).\nFetching doesn't currently quarantine objects, but it probably wouldn't\nbe very hard to make it do so.\n\n-Peff\n"},{"id":"339746","messageId":"51505f97-9d56-a938-7255-ed11fa0de814@gaspard.io","threadId":"47783","inReplyTo":"20180220074201.GA2154@sigill.intra.peff.net","subject":"Re: Fetch-hooks","fromName":"Leo Gaspard","fromEmail":"leo@gaspard.io","sentAt":"2018-02-20T21:19:49Z","receivedAt":"2018-02-20T21:19:56Z","isPatch":false,"sender":{"key":"leo@gaspard.io","avatar":null},"body":"On 02/20/2018 08:42 AM, Jeff King wrote:>> [...]\n>>\n>> Is there a way for “pre-receive” to individually filter [refs]? I was\n>> under the impression that the only way to do that was to use the\n>> “update” hook, which was the reason I wanted to model it after “update”\n>> rather than “pre-receive” (my use case being a check independent for\n>> each pushed ref)\n> \n> No, pre-receive is always atomic. However, that may actually be what you\n> want, if the point is to keep untrusted data out of the repository. By\n> rejecting the whole thing, we could in theory keep the objects from\n> entering the repository at all. This is how the push side works via the\n> \"quarantine\" system (which is explained in git-receive-pack(1)).\n> Fetching doesn't currently quarantine objects, but it probably wouldn't\n> be very hard to make it do so.\n\nOh, I didn't think about quarantining behavior, indeed!\n\nSo I guess, following your answer as well as Jake's, I'll try to\nimplement a pre-receive-like hook, and will come back to this list when\nI'll have a tentative implementation. Thanks for the advice! :)\n"}]}