{"thread":{"id":"62348","subject":"[PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","startedAt":"2024-10-16T21:06:38Z","lastAt":"2024-10-21T19:48:55Z","messageCount":14,"participants":["Piotr Szlazak via GitGitGadget","Taylor Blau","Jeff King","Piotr Szlazak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505310","messageId":"pull.1814.git.git.1729112794671.gitgitgadget@gmail.com","threadId":"62348","inReplyTo":null,"subject":"[PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Piotr Szlazak via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-16T21:06:34Z","receivedAt":"2024-10-16T21:06:38Z","isPatch":true,"sender":{"key":"piotr.szlazak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11884243?v=4"},"body":"From: Piotr Szlazak \n\nALLOW_ANY_SHA1 implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1.\nYet ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 flags can be enabled\nindependently.\nIf uploadpack.allowAnySHA1InWant is not enabled in config file,\nother flags should not be disabled together with ALLOW_ANY_SHA1.\nThey should be kept enabled if they were separately enabled in\nconfig file with they respective options.\n\nSigned-off-by: Piotr Szlazak \n---\n upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v1\nPull-Request: https://github.com/git/git/pull/1814\n\n upload-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6d6e0f9f980..cf99b228719 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -53,6 +53,7 @@ enum allow_uor {\n \t/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */\n \tALLOW_REACHABLE_SHA1 = 0x02,\n \t/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */\n+\t/* As this flag implies other two flags, be careful when it must be disabled. */\n \tALLOW_ANY_SHA1 = 0x07\n };\n \n@@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,\n \t\tif (git_config_bool(var, value))\n \t\t\tdata->allow_uor |= ALLOW_ANY_SHA1;\n \t\telse\n-\t\t\tdata->allow_uor &= ~ALLOW_ANY_SHA1;\n+\t\t\tdata->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));\n \t} else if (!strcmp(\"uploadpack.keepalive\", var)) {\n \t\tdata->keepalive = git_config_int(var, value, ctx->kvi);\n \t\tif (!data->keepalive)\n\nbase-commit: ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f\n-- \ngitgitgadget\n"},{"id":"505314","messageId":"ZxAttC1dQUllR76m@nand.local","threadId":"62348","inReplyTo":"pull.1814.git.git.1729112794671.gitgitgadget@gmail.com","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-16T21:18:44Z","receivedAt":"2024-10-16T21:18:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 09:06:34PM +0000, Piotr Szlazak via GitGitGadget wrote:\n> From: Piotr Szlazak \n>\n> ALLOW_ANY_SHA1 implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1.\n> Yet ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 flags can be enabled\n> independently.\n> If uploadpack.allowAnySHA1InWant is not enabled in config file,\n> other flags should not be disabled together with ALLOW_ANY_SHA1.\n> They should be kept enabled if they were separately enabled in\n> config file with they respective options.\n>\n> Signed-off-by: Piotr Szlazak \n> ---\n> upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v1\n> Pull-Request: https://github.com/git/git/pull/1814\n>\n> upload-pack.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 6d6e0f9f980..cf99b228719 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -53,6 +53,7 @@ enum allow_uor {\n> \t/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */\n> \tALLOW_REACHABLE_SHA1 = 0x02,\n> \t/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */\n> +\t/* As this flag implies other two flags, be careful when it must be disabled. */\n> \tALLOW_ANY_SHA1 = 0x07\n> };\n>\n> @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,\n> \t\tif (git_config_bool(var, value))\n> \t\t\tdata->allow_uor |= ALLOW_ANY_SHA1;\n> \t\telse\n> -\t\t\tdata->allow_uor &= ~ALLOW_ANY_SHA1;\n> +\t\t\tdata->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));\n\nSubtracting the result of a bitwise-OR feels a little odd to me.\n\nSince ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and\n0x2, respectively, I think the end result is as you described it, but\nthe route to get there feels a little odd to me.\n\nI think it would probably make more sense to write this as:\n\n data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n\nStepping back a moment, I suppose this is handling the case where a user\nwrites:\n\n [uploadpack]\n allowTipSHA1InWant = true\n allowReachableSHA1InWant = true\n allowAnySHA1InWant = false\n\nand is surprised when the final \"uploadPack.allowAnySHA1InWant\" unsets\nthe previous two options.\n\nI'm not sure that the current behavior is actually wrong. The final line\nin the example above seems to indicate \"do not allow any SHA-1 in the\n'wants'\", which would indeed imply that the other two options should be\nset to false as well.\n\nAnd that is what the current code is doing, which I think is correct.\n\nI do see that our upload-pack section of \"git-config(1)\" is lacking in\nthis area, though, as it does not indicate that\nuploadPack.allowAnySHA1InWant implies the other two options. It may be\nworth saying something like:\n\n NOTE: this option implies both uploadPack.allowTipSHA1InWant and\n uploadPack.allowReachableSHA1InWant. Setting this option to \"false\"\n will do the same for the implied ones.\n\nThanks,\nTaylor\n"},{"id":"505322","messageId":"20241017023735.GB1858436@coredump.intra.peff.net","threadId":"62348","inReplyTo":"ZxAttC1dQUllR76m@nand.local","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-17T02:37:35Z","receivedAt":"2024-10-17T02:37:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 16, 2024 at 05:18:44PM -0400, Taylor Blau wrote:\n\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 6d6e0f9f980..cf99b228719 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -53,6 +53,7 @@ enum allow_uor {\n> > \t/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */\n> > \tALLOW_REACHABLE_SHA1 = 0x02,\n> > \t/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */\n> > +\t/* As this flag implies other two flags, be careful when it must be disabled. */\n> > \tALLOW_ANY_SHA1 = 0x07\n> > };\n> >\n> > @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value,\n> > \t\tif (git_config_bool(var, value))\n> > \t\t\tdata->allow_uor |= ALLOW_ANY_SHA1;\n> > \t\telse\n> > -\t\t\tdata->allow_uor &= ~ALLOW_ANY_SHA1;\n> > +\t\t\tdata->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1));\n> \n> Subtracting the result of a bitwise-OR feels a little odd to me.\n> \n> Since ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and\n> 0x2, respectively, I think the end result is as you described it, but\n> the route to get there feels a little odd to me.\n> \n> I think it would probably make more sense to write this as:\n> \n> data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n\nI think we have to treat them as a complete unit, as we don't know which\nbits were set by independent config lines and which were OR-ed in by\nALLOW_ANY.\n\nSo this case:\n\n> Stepping back a moment, I suppose this is handling the case where a user\n> writes:\n> \n> [uploadpack]\n> allowTipSHA1InWant = true\n> allowReachableSHA1InWant = true\n> allowAnySHA1InWant = false\n> \n> and is surprised when the final \"uploadPack.allowAnySHA1InWant\" unsets\n> the previous two options.\n\nis the one that Piotr is thinking about. But what about:\n\n [uploadpack]\n allowAnySHA1InWant = true\n allowAnySHA1InWant = false\n\nRight now that pair is a noop, which is what I'd expect. But after the\nproposed patch, it quietly enables ALLOW_TIP_SHA1 and\nALLOW_REACHABLE_SHA1.\n\nSo I think the code has to stay the same, but we perhaps should document\nthat \"allow any\" has the user-visible side effect of enabling/disabling\nthe other two.\n\n-Peff\n"},{"id":"505373","messageId":"ZxEr4+sb4DfmtrKv@nand.local","threadId":"62348","inReplyTo":"20241017023735.GB1858436@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-17T15:23:15Z","receivedAt":"2024-10-17T15:23:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:\n> > I think it would probably make more sense to write this as:\n> >\n> > data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n>\n> I think we have to treat them as a complete unit, as we don't know which\n> bits were set by independent config lines and which were OR-ed in by\n> ALLOW_ANY.\n>\n> So this case:\n>\n> > Stepping back a moment, I suppose this is handling the case where a user\n> > writes:\n> >\n> > [uploadpack]\n> > allowTipSHA1InWant = true\n> > allowReachableSHA1InWant = true\n> > allowAnySHA1InWant = false\n> >\n> > and is surprised when the final \"uploadPack.allowAnySHA1InWant\" unsets\n> > the previous two options.\n\nYeah, I think that you and I are in agreement here.\n\n> is the one that Piotr is thinking about. But what about:\n>\n> [uploadpack]\n> allowAnySHA1InWant = true\n> allowAnySHA1InWant = false\n>\n> Right now that pair is a noop, which is what I'd expect. But after the\n> proposed patch, it quietly enables ALLOW_TIP_SHA1 and\n> ALLOW_REACHABLE_SHA1.\n\nThat's an even clearer example of a new gotcha that would occur with\nthis proposed patch, IMHO. I don't think in general that successive\n\n $ git config core.foo true\n $ git config core.foo false\n\nshould have any user-visible effect, as the latter should nullify the\nformer.\n\n> So I think the code has to stay the same, but we perhaps should document\n> that \"allow any\" has the user-visible side effect of enabling/disabling\n> the other two.\n\nThat would be a useful direction, I think. Double checking\ngit-config(1), there is in deed no mention of allowAnySHA1InWant\nimplying the other two options, which seems like a gap that would be\ngood to address.\n\nPiotr: what do you think?\n\nThanks,\nTaylor\n"},{"id":"505376","messageId":"a328b6d5-e604-4517-b041-59db19e1f66c@gmail.com","threadId":"62348","inReplyTo":"ZxEr4+sb4DfmtrKv@nand.local","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Piotr Szlazak","fromEmail":"piotr.szlazak@gmail.com","sentAt":"2024-10-17T15:59:46Z","receivedAt":"2024-10-17T15:59:49Z","isPatch":true,"sender":{"key":"piotr.szlazak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11884243?v=4"},"body":"On 17.10.2024 17:23, Taylor Blau wrote:\n> On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:\n>>> I think it would probably make more sense to write this as:\n>>>\n>>> data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n\nMuch better! :-)\n\n>> I think we have to treat them as a complete unit, as we don't know which\n>> bits were set by independent config lines and which were OR-ed in by\n>> ALLOW_ANY.\n>>\n>> So this case:\n>>\n>>> Stepping back a moment, I suppose this is handling the case where a user\n>>> writes:\n>>>\n>>> [uploadpack]\n>>> allowTipSHA1InWant = true\n>>> allowReachableSHA1InWant = true\n>>> allowAnySHA1InWant = false\n>>>\n>>> and is surprised when the final \"uploadPack.allowAnySHA1InWant\" unsets\n>>> the previous two options.\n> Yeah, I think that you and I are in agreement here.\n>\n>> is the one that Piotr is thinking about. But what about:\n>>\n>> [uploadpack]\n>> allowAnySHA1InWant = true\n>> allowAnySHA1InWant = false\n>>\n>> Right now that pair is a noop, which is what I'd expect. But after the\n>> proposed patch, it quietly enables ALLOW_TIP_SHA1 and\n>> ALLOW_REACHABLE_SHA1.\n\nRather not as config file is parsed only once:\n\nhttps://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368\n\n>> So I think the code has to stay the same, but we perhaps should document\n>> that \"allow any\" has the user-visible side effect of enabling/disabling\n>> the other two.\n> That would be a useful direction, I think. Double checking\n> git-config(1), there is in deed no mention of allowAnySHA1InWant\n> implying the other two options, which seems like a gap that would be\n> good to address.\n>\n> Piotr: what do you think?\n\nI agree. I completely missed to test it like that (which works OK):\n\n[uploadpack]\n         allowTipSHA1InWant = true\n         allowReachableSHA1InWant = true\n\nEOF\n\nI was always testing with allowAnySHAInWant either set to 'true' or \n'false'. But always in place.\n\nAnd having it set to 'false' was disabling my previously set other \nallowXyzInWant options. Which was a surprise, as I was \nconsidering allowAnySHAInWant as a final level of openness for \nclient-side requests[1]. In contradiction to what Taylor expressed here:\n\n> I'm not sure that the current behavior is actually wrong. The final line \nin the example above seems to indicate \"do not allow any SHA-1 in the \n'wants'\", which would indeed imply that the other two options should be \nset to false as well.\n\nSo as suggested I will prepare a patch for documentation, so it will be \nalso clear for others. Should it be done using same thread or new one \nshould be created?\n\n[1] For example client can request not reachable objects, trees, blobs.\n\nRegards,\n\nPiotr\n\n"},{"id":"505384","messageId":"ZxFblRri8Xo/lGsK@nand.local","threadId":"62348","inReplyTo":"a328b6d5-e604-4517-b041-59db19e1f66c@gmail.com","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-17T18:46:45Z","receivedAt":"2024-10-17T18:46:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 17, 2024 at 05:59:46PM +0200, Piotr Szlazak wrote:\n> On 17.10.2024 17:23, Taylor Blau wrote:\n> > On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote:\n> > > > I think it would probably make more sense to write this as:\n> > > >\n> > > > data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n>\n> Much better! :-)\n>\n> > > I think we have to treat them as a complete unit, as we don't know which\n> > > bits were set by independent config lines and which were OR-ed in by\n> > > ALLOW_ANY.\n> > >\n> > > So this case:\n> > >\n> > > > Stepping back a moment, I suppose this is handling the case where a user\n> > > > writes:\n> > > >\n> > > > [uploadpack]\n> > > > allowTipSHA1InWant = true\n> > > > allowReachableSHA1InWant = true\n> > > > allowAnySHA1InWant = false\n> > > >\n> > > > and is surprised when the final \"uploadPack.allowAnySHA1InWant\" unsets\n> > > > the previous two options.\n> > Yeah, I think that you and I are in agreement here.\n> >\n> > > is the one that Piotr is thinking about. But what about:\n> > >\n> > > [uploadpack]\n> > > allowAnySHA1InWant = true\n> > > allowAnySHA1InWant = false\n> > >\n> > > Right now that pair is a noop, which is what I'd expect. But after the\n> > > proposed patch, it quietly enables ALLOW_TIP_SHA1 and\n> > > ALLOW_REACHABLE_SHA1.\n>\n> Rather not as config file is parsed only once:\n>\n> https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368\n\nI am not sure I follow... upload_pack_config() is invoked on every\nconfiguration line that we see. So the first time when we read\n\"allowAnySHA1InWant = true\", we take the first path through that\nfunction you linked. The second time, we take the else branch, and\ncorrespondingly unset the bits from ALLOW_ANY_SHA1.\n\nSo today that is doing the right thing (it will end with the same bits\nset that we started with). But under the proposed patch that behavior\nwould change, and the lower two bits would still be set.\n\n> So as suggested I will prepare a patch for documentation, so it will be also\n> clear for others. Should it be done using same thread or new one should be\n> created?\n\nEither is fine.\n\nThanks,\nTaylor\n"},{"id":"505403","messageId":"20241018043306.GB2408674@coredump.intra.peff.net","threadId":"62348","inReplyTo":"ZxFblRri8Xo/lGsK@nand.local","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-18T04:33:06Z","receivedAt":"2024-10-18T04:33:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote:\n\n> > Rather not as config file is parsed only once:\n> >\n> > https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368\n> \n> I am not sure I follow... upload_pack_config() is invoked on every\n> configuration line that we see. So the first time when we read\n> \"allowAnySHA1InWant = true\", we take the first path through that\n> function you linked. The second time, we take the else branch, and\n> correspondingly unset the bits from ALLOW_ANY_SHA1.\n> \n> So today that is doing the right thing (it will end with the same bits\n> set that we started with). But under the proposed patch that behavior\n> would change, and the lower two bits would still be set.\n\nReading Piotr's response I wondered if upload-pack might be using the\nconfigset API instead of a config callback. But no, it does look like a\nconfig callback.\n\nBut it does hint at a possible path if we wanted to process these\nindependently: we could read each of the config options separately,\nresolving them in the usual last-one-wins way, and then turning the\nresult into a bitfield. Something like this, outside of the callback\n(totally untested):\n\n int v;\n unsigned bits = 0;\n\n if (!git_config_get_bool(\"uploadpack.allowtipsha1inwant\", &v) && v)\n\tbits |= ALLOW_TIP_SHA1;\n if (!git_config_get_bool(\"uploadpack.allowreachablesha1inwant\", &v) && v)\n\tbits |= ALLOW_REACHABLE_SHA1;\n if (!git_config_get_bool(\"uploadpack.allowanysha1inwant\", &v) && v)\n\tbits |= ALLOW_ANY_SHA1;\n\nThat makes the config flags independent. But the features themselves are\nnot really independent. If you do:\n\n [uploadpack]\n allowAnySHA1InWant = true\n allowTipSHA1InWant = false\n\nit is still going to allow the \"tip\" flag, because it's a subset of the\n\"any\" flag. And you can't escape that.\n\nSo I still think we're better off to just document (and of course in the\nlong run all of these become less and less interesting as they are\nv0-only options).\n\n-Peff\n"},{"id":"505477","messageId":"ZxLXGG8+VUWydsle@nand.local","threadId":"62348","inReplyTo":"20241018043306.GB2408674@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-18T21:46:00Z","receivedAt":"2024-10-18T21:46:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 18, 2024 at 12:33:06AM -0400, Jeff King wrote:\n> On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote:\n>\n> > > Rather not as config file is parsed only once:\n> > >\n> > > https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368\n> >\n> > I am not sure I follow... upload_pack_config() is invoked on every\n> > configuration line that we see. So the first time when we read\n> > \"allowAnySHA1InWant = true\", we take the first path through that\n> > function you linked. The second time, we take the else branch, and\n> > correspondingly unset the bits from ALLOW_ANY_SHA1.\n> >\n> > So today that is doing the right thing (it will end with the same bits\n> > set that we started with). But under the proposed patch that behavior\n> > would change, and the lower two bits would still be set.\n>\n> Reading Piotr's response I wondered if upload-pack might be using the\n> configset API instead of a config callback. But no, it does look like a\n> config callback.\n\nYeah... I was pretty sure that was the case thinking back to 6dd3456a8c\n(upload-pack.c: allow banning certain object filter(s), 2020-08-03),\nwhich somehow was already more than four years ago :-<.\n\n> But it does hint at a possible path if we wanted to process these\n> independently: we could read each of the config options separately,\n> resolving them in the usual last-one-wins way, and then turning the\n> result into a bitfield. Something like this, outside of the callback\n> (totally untested):\n>\n> int v;\n> unsigned bits = 0;\n>\n> if (!git_config_get_bool(\"uploadpack.allowtipsha1inwant\", &v) && v)\n> \tbits |= ALLOW_TIP_SHA1;\n> if (!git_config_get_bool(\"uploadpack.allowreachablesha1inwant\", &v) && v)\n> \tbits |= ALLOW_REACHABLE_SHA1;\n> if (!git_config_get_bool(\"uploadpack.allowanysha1inwant\", &v) && v)\n> \tbits |= ALLOW_ANY_SHA1;\n>\n> That makes the config flags independent. But the features themselves are\n> not really independent. If you do:\n>\n> [uploadpack]\n> allowAnySHA1InWant = true\n> allowTipSHA1InWant = false\n>\n> it is still going to allow the \"tip\" flag, because it's a subset of the\n> \"any\" flag. And you can't escape that.\n\nYup.\n\n> So I still think we're better off to just document (and of course in the\n> long run all of these become less and less interesting as they are\n> v0-only options).\n\nI agree completely. I think the consensus here seems to be that, if\nanything, we should update the documentation and move on :-).\n\nThanks,\nTaylor\n"},{"id":"505501","messageId":"pull.1814.v2.git.git.1729355997353.gitgitgadget@gmail.com","threadId":"62348","inReplyTo":"pull.1814.git.git.1729112794671.gitgitgadget@gmail.com","subject":"[PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options","fromName":"Piotr Szlazak via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-19T16:39:57Z","receivedAt":"2024-10-19T16:40:01Z","isPatch":true,"sender":{"key":"piotr.szlazak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11884243?v=4"},"body":"From: Piotr Szlazak \n\nDocument how setting of `uploadpack.allowAnySHA1InWant`\ninfluences other `uploadpack` options - `allowTipSHA1InWant`\nand `allowReachableSHA1InWant`.\n\nSigned-off-by: Piotr Szlazak \n---\n doc: document how uploadpack.allowAnySHA1InWant impact other allow\n options\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v2\nPull-Request: https://github.com/git/git/pull/1814\n\nRange-diff vs v1:\n\n 1: 8a2673bdf31 < -: ----------- upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled\n -: ----------- > 1: 2a9fa4dabba doc: document how uploadpack.allowAnySHA1InWant impact other allow options\n\n\n Documentation/config/uploadpack.txt | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex 16264d82a72..0e1dda944a5 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -25,7 +25,11 @@ uploadpack.allowReachableSHA1InWant::\n uploadpack.allowAnySHA1InWant::\n \tAllow `upload-pack` to accept a fetch request that asks for any\n \tobject at all.\n-\tDefaults to `false`.\n+\tIt implies `uploadpack.allowTipSHA1InWant` and\n+\t`uploadpack.allowReachableSHA1InWant`. If set to `true` it will\n+\tenable both of them, it set to `false` it will disable both of\n+\tthem.\n+\tBy default not set.\n \n uploadpack.keepAlive::\n \tWhen `upload-pack` has started `pack-objects`, there may be a\n\nbase-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0\n-- \ngitgitgadget\n"},{"id":"505502","messageId":"72184384-07ff-4f74-91d7-280ca78407b5@gmail.com","threadId":"62348","inReplyTo":"pull.1814.v2.git.git.1729355997353.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options","fromName":"Piotr Szlazak","fromEmail":"piotr.szlazak@gmail.com","sentAt":"2024-10-19T16:46:59Z","receivedAt":"2024-10-19T16:47:04Z","isPatch":true,"sender":{"key":"piotr.szlazak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11884243?v=4"},"body":"\nOn 19.10.2024 18:39, Piotr Szlazak via GitGitGadget wrote:\n> From: Piotr Szlazak \n>\n> Document how setting of `uploadpack.allowAnySHA1InWant`\n> influences other `uploadpack` options - `allowTipSHA1InWant`\n> and `allowReachableSHA1InWant`.\n>\n> Signed-off-by: Piotr Szlazak \n> ---\n> doc: document how uploadpack.allowAnySHA1InWant impact other allow\n> options\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v2\n> Pull-Request: https://github.com/git/git/pull/1814\n>\n> Range-diff vs v1:\n>\n> 1: 8a2673bdf31 < -: ----------- upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled\n> -: ----------- > 1: 2a9fa4dabba doc: document how uploadpack.allowAnySHA1InWant impact other allow options\n>\n>\n> Documentation/config/uploadpack.txt | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\n> index 16264d82a72..0e1dda944a5 100644\n> --- a/Documentation/config/uploadpack.txt\n> +++ b/Documentation/config/uploadpack.txt\n> @@ -25,7 +25,11 @@ uploadpack.allowReachableSHA1InWant::\n> uploadpack.allowAnySHA1InWant::\n> \tAllow `upload-pack` to accept a fetch request that asks for any\n> \tobject at all.\n> -\tDefaults to `false`.\n> +\tIt implies `uploadpack.allowTipSHA1InWant` and\n> +\t`uploadpack.allowReachableSHA1InWant`. If set to `true` it will\n> +\tenable both of them, it set to `false` it will disable both of\n> +\tthem.\n> +\tBy default not set.\n> \n> uploadpack.keepAlive::\n> \tWhen `upload-pack` has started `pack-objects`, there may be a\n>\n> base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0\n\nPATCH v2 which updates documentation.\n\nI wrote 'By default not set', as definitely 'Defaults to `false`' in not \ntrue.\n\nOnly when `uploadpack.allowAnySHA1InWant` set to `true` or `false` it \nwill affect reported capabilities.\n\nRegards,\n\nPiotr\n\n"},{"id":"505584","messageId":"fd5fab45-1363-41c8-915a-5578a35d219d@gmail.com","threadId":"62348","inReplyTo":"72184384-07ff-4f74-91d7-280ca78407b5@gmail.com","subject":"Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options","fromName":"Piotr Szlazak","fromEmail":"piotr.szlazak@gmail.com","sentAt":"2024-10-21T05:55:06Z","receivedAt":"2024-10-21T05:55:13Z","isPatch":true,"sender":{"key":"piotr.szlazak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11884243?v=4"},"body":"On 19.10.2024 18:46, Piotr Szlazak wrote:\n>\n> On 19.10.2024 18:39, Piotr Szlazak via GitGitGadget wrote:\n>> From: Piotr Szlazak \n>>\n>> Document how setting of `uploadpack.allowAnySHA1InWant`\n>> influences other `uploadpack` options - `allowTipSHA1InWant`\n>> and `allowReachableSHA1InWant`.\n>>\n>> Signed-off-by: Piotr Szlazak \n>> ---\n>>      doc: document how uploadpack.allowAnySHA1InWant impact other allow\n>>      options\n>>\n>> Published-As: \n>> https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v2\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git \n>> pr-git-1814/pszlazak/upload-pack-allow-flags-v2\n>> Pull-Request: https://github.com/git/git/pull/1814\n>>\n>> Range-diff vs v1:\n>>\n>>   1:  8a2673bdf31 < -:  ----------- upload-pack: fix how \n>> ALLOW_ANY_SHA1 flag is disabled\n>>   -:  ----------- > 1:  2a9fa4dabba doc: document how \n>> uploadpack.allowAnySHA1InWant impact other allow options\n>>\n>>\n>>   Documentation/config/uploadpack.txt | 6 +++++-\n>>   1 file changed, 5 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/config/uploadpack.txt \n>> b/Documentation/config/uploadpack.txt\n>> index 16264d82a72..0e1dda944a5 100644\n>> --- a/Documentation/config/uploadpack.txt\n>> +++ b/Documentation/config/uploadpack.txt\n>> @@ -25,7 +25,11 @@ uploadpack.allowReachableSHA1InWant::\n>>   uploadpack.allowAnySHA1InWant::\n>>       Allow `upload-pack` to accept a fetch request that asks for any\n>>       object at all.\n>> -    Defaults to `false`.\n>> +    It implies `uploadpack.allowTipSHA1InWant` and\n>> +    `uploadpack.allowReachableSHA1InWant`. If set to `true` it will\n>> +    enable both of them, it set to `false` it will disable both of\n>> +    them.\n>> +    By default not set.\n>>     uploadpack.keepAlive::\n>>       When `upload-pack` has started `pack-objects`, there may be a\n>>\n>> base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0\n>\n> PATCH v2 which updates documentation.\n>\n> I wrote 'By default not set', as definitely 'Defaults to `false`' in \n> not true.\n>\n> Only when `uploadpack.allowAnySHA1InWant` set to `true` or `false` it \n> will affect reported capabilities.\n>\n> Regards,\n>\n> Piotr\n\nOn the second look code changes will be needed, as at the moment final \nresult will differ between:\n[uploadpack]\n         allowTipSHA1InWant = true\n         allowReachableSHA1InWant = true\n         allowAnySHAInWant = false\n\nand:\n\n[uploadpack]\n         allowAnySHAInWant = false\n         allowTipSHA1InWant = true\n         allowReachableSHA1InWant = true\n\nRegards,\nPiotr\n\n"},{"id":"505724","messageId":"20241021190309.GC1219228@coredump.intra.peff.net","threadId":"62348","inReplyTo":"fd5fab45-1363-41c8-915a-5578a35d219d@gmail.com","subject":"Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-21T19:03:09Z","receivedAt":"2024-10-21T19:03:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 21, 2024 at 07:55:06AM +0200, Piotr Szlazak wrote:\n\n> On the second look code changes will be needed, as at the moment final\n> result will differ between:\n> [uploadpack]\n>         allowTipSHA1InWant = true\n>         allowReachableSHA1InWant = true\n>         allowAnySHAInWant = false\n> \n> and:\n> \n> [uploadpack]\n>         allowAnySHAInWant = false\n>         allowTipSHA1InWant = true\n>         allowReachableSHA1InWant = true\n\nI'd expect them to differ. The config is read in order with a \"last one\nwins\" rule. The thing that the documentation should be making clear is\nthat the three overlap in what they are affecting, and so \"last one\" is\nnot just a single key, but these three inter-related keys.\n\n-Peff\n"},{"id":"505733","messageId":"Zxav3sIyupyjTuCi@nand.local","threadId":"62348","inReplyTo":"20241021190309.GC1219228@coredump.intra.peff.net","subject":"Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T19:47:42Z","receivedAt":"2024-10-21T19:47:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 03:03:09PM -0400, Jeff King wrote:\n> On Mon, Oct 21, 2024 at 07:55:06AM +0200, Piotr Szlazak wrote:\n>\n> > On the second look code changes will be needed, as at the moment final\n> > result will differ between:\n> > [uploadpack]\n> >         allowTipSHA1InWant = true\n> >         allowReachableSHA1InWant = true\n> >         allowAnySHAInWant = false\n> >\n> > and:\n> >\n> > [uploadpack]\n> >         allowAnySHAInWant = false\n> >         allowTipSHA1InWant = true\n> >         allowReachableSHA1InWant = true\n>\n> I'd expect them to differ. The config is read in order with a \"last one\n> wins\" rule. The thing that the documentation should be making clear is\n> that the three overlap in what they are affecting, and so \"last one\" is\n> not just a single key, but these three inter-related keys.\n\nAgreed. I think the \"last one wins\" policy is well understood throughout\nGit's rules for how configuration layers are parsed, and not worth\nrepeating purely for the purposes of this specific piece of the\ndocumentation.\n\nThanks,\nTaylor\n"},{"id":"505734","messageId":"ZxawJIYrmoGnhXgj@nand.local","threadId":"62348","inReplyTo":"pull.1814.v2.git.git.1729355997353.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] doc: document how uploadpack.allowAnySHA1InWant impact other allow options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T19:48:52Z","receivedAt":"2024-10-21T19:48:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 19, 2024 at 04:39:57PM +0000, Piotr Szlazak via GitGitGadget wrote:\n> From: Piotr Szlazak \n>\n> Document how setting of `uploadpack.allowAnySHA1InWant`\n> influences other `uploadpack` options - `allowTipSHA1InWant`\n> and `allowReachableSHA1InWant`.\n>\n> Signed-off-by: Piotr Szlazak \n> ---\n> doc: document how uploadpack.allowAnySHA1InWant impact other allow\n> options\n\nThanks, this version looks quite good to me.\n\nThanks,\nTaylor\n"}]}