{"thread":{"id":"45912","subject":"[PATCH] fetch-pack: always allow fetching of literal SHA1s","startedAt":"2017-05-09T18:20:56Z","lastAt":"2017-05-15T22:11:01Z","messageCount":47,"participants":["Jonathan Tan","Jeff King","Shawn Pearce","Mike Hommey","Jonathan Nieder","Ævar Arnfjörð Bjarmason","Martin Fick","Sebastian Schuberth","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"319209","messageId":"20170509182042.28389-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":null,"subject":"[PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-09T18:20:42Z","receivedAt":"2017-05-09T18:20:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 30 ++++++++++++++++--------------\n t/t5500-fetch-pack.sh | 6 ++++++\n 2 files changed, 22 insertions(+), 14 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..180405dff 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -592,6 +592,14 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static int is_literal_sha1(const struct ref *ref)\n+{\n+\tstruct object_id oid;\n+\treturn !get_oid_hex(ref->name, &oid) &&\n+\t !ref->name[40] &&\n+\t !oidcmp(&oid, &ref->old_oid);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n@@ -601,7 +609,6 @@ static void filter_refs(struct fetch_pack_args *args,\n \tstruct ref *ref, *next;\n \tint i;\n \n-\ti = 0;\n \tfor (ref = *refs; ref; ref = next) {\n \t\tint keep = 0;\n \t\tnext = ref->next;\n@@ -610,15 +617,14 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t check_refname_format(ref->name, 0))\n \t\t\t; /* trash */\n \t\telse {\n-\t\t\twhile (i < nr_sought) {\n-\t\t\t\tint cmp = strcmp(ref->name, sought[i]->name);\n-\t\t\t\tif (cmp < 0)\n-\t\t\t\t\tbreak; /* definitely do not have it */\n-\t\t\t\telse if (cmp == 0) {\n-\t\t\t\t\tkeep = 1; /* definitely have it */\n-\t\t\t\t\tsought[i]->match_status = REF_MATCHED;\n+\t\t\tfor (i = 0; i < nr_sought; i++) {\n+\t\t\t\tstruct ref *s = sought[i];\n+\t\t\t\tif (!strcmp(ref->name, s->name) ||\n+\t\t\t\t (is_literal_sha1(s) &&\n+\t\t\t\t !oidcmp(&ref->old_oid, &s->old_oid))) {\n+\t\t\t\t\tkeep = 1;\n+\t\t\t\t\ts->match_status = REF_MATCHED;\n \t\t\t\t}\n-\t\t\t\ti++;\n \t\t\t}\n \t\t}\n \n@@ -637,14 +643,10 @@ static void filter_refs(struct fetch_pack_args *args,\n \n \t/* Append unmatched requests to the list */\n \tfor (i = 0; i < nr_sought; i++) {\n-\t\tunsigned char sha1[20];\n-\n \t\tref = sought[i];\n \t\tif (ref->match_status != REF_NOT_MATCHED)\n \t\t\tcontinue;\n-\t\tif (get_sha1_hex(ref->name, sha1) ||\n-\t\t ref->name[40] != '\\0' ||\n-\t\t hashcmp(sha1, ref->old_oid.hash))\n+\t\tif (!is_literal_sha1(ref))\n \t\t\tcontinue;\n \n \t\tif ((allow_unadvertised_object_request &\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..d87983bc2 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\tgit init server &&\n+\ttest_commit -C server 4 &&\n+\tgit fetch-pack server $(git -C server rev-parse refs/heads/master)\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319231","messageId":"20170509221629.3z35qcz36oiix3kh@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T22:16:30Z","receivedAt":"2017-05-09T22:16:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote:\n\n> fetch-pack, when fetching a literal SHA-1 from a server that is not\n> configured with uploadpack.allowtipsha1inwant (or similar), always\n> returns an error message of the form \"Server does not allow request for\n> unadvertised object %s\". However, it is sometimes the case that such\n> object is advertised.\n> \n> Teach fetch-pack to also check the SHA-1s of the refs in the received\n> ref advertisement if a literal SHA-1 was given by the user.\n\nHmm. That makes sense generally, as the request should succeed. But it\nseems like we're creating a client that will sometimes succeed and\nsometimes fail, and the reasoning will be somewhat opaque to the user.\nI have a feeling I'm missing some context on when you'd expect this to\nkick in.\n\n> +static int is_literal_sha1(const struct ref *ref)\n> +{\n> +\tstruct object_id oid;\n> +\treturn !get_oid_hex(ref->name, &oid) &&\n> +\t !ref->name[40] &&\n> +\t !oidcmp(&oid, &ref->old_oid);\n> +}\n\nI think the preferred method these days is to avoid the bare \"40\":\n\n struct object_oid oid;\n const char *end;\n return !parse_oid_hex(ref->name, &oid, &end) &&\n !*end &&\n\t !oidcmp(&oid, &ref->old_oid);\n\nI was confused at first why we need this oidcmp() and the one below. But\nthis one is checking \"does the name parse to itself\", and the other is\nchecking \"does this parse to our sought ref?\". So both checks are\nneeded.\n\n> +\t\t\tfor (i = 0; i < nr_sought; i++) {\n> +\t\t\t\tstruct ref *s = sought[i];\n> +\t\t\t\tif (!strcmp(ref->name, s->name) ||\n> +\t\t\t\t (is_literal_sha1(s) &&\n> +\t\t\t\t !oidcmp(&ref->old_oid, &s->old_oid))) {\n> +\t\t\t\t\tkeep = 1;\n> +\t\t\t\t\ts->match_status = REF_MATCHED;\n> \t\t\t\t}\n> -\t\t\t\ti++;\n> \t\t\t}\n\nThis will reparse ref->name as an oid via is_literal_sha1() for each\npass through the loop. Should it be hoisted out? Maybe that is just\npremature optimization, though.\n\nOther than those minor nits, the code itself looks fine to me.\n\n-Peff\n"},{"id":"319246","messageId":"CAJo=hJvAg2WqpiuykpbHcB5vgQiHJ74CZ8Y4qudkYqZrmd30zg@mail.gmail.com","threadId":"45912","inReplyTo":"20170509221629.3z35qcz36oiix3kh@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2017-05-10T04:22:11Z","receivedAt":"2017-05-10T04:22:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, May 9, 2017 at 3:16 PM, Jeff King wrote:\n> On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote:\n>\n>> fetch-pack, when fetching a literal SHA-1 from a server that is not\n>> configured with uploadpack.allowtipsha1inwant (or similar), always\n>> returns an error message of the form \"Server does not allow request for\n>> unadvertised object %s\". However, it is sometimes the case that such\n>> object is advertised.\n>>\n>> Teach fetch-pack to also check the SHA-1s of the refs in the received\n>> ref advertisement if a literal SHA-1 was given by the user.\n>\n> Hmm. That makes sense generally, as the request should succeed. But it\n> seems like we're creating a client that will sometimes succeed and\n> sometimes fail, and the reasoning will be somewhat opaque to the user.\n> I have a feeling I'm missing some context on when you'd expect this to\n> kick in.\n\nSpecifically, someone I know was looking at building an application\nthat is passed only a SHA-1 for the tip of a ref on a popular hosting\nsite[1]. They wanted to run `git fetch URL SHA1`, but this failed\nbecause the site doesn't have upload.allowtipsha1inwant enabled.\nHowever the SHA1 was clearly in the output of git ls-remote.\n\nSo a workaround is to do this in shell, which is just weird:\n\n r=$(git ls-remote $url | grep ^$sha1);\n if [ -n \"$r\" ]; then\n exec git fetch $url $r:refs/tmp/foo\n else\n echo >&2 \"cannot find $sha1\"\n exit 1\n fi\n\nFor various reasons they expected this to work, because it works\nagainst other sites that do have upload.allowtipsha1inwant enabled.\nAnd I personally just expected it to work because the fetch client\naccepts SHA-1s, and the wire protocol uses \"want SHA1\" not \"want ref\",\nand the SHA-1 passed on the command line was currently in the\nadvertisement when the connection opened, so its certainly something\nthe server is currently willing to serve.\n\nThe application in question is using a SHA-1 rather than a ref name,\nbecause thats what it was given by something else. And the other thing\nbasically wants this to fail-fast if it can't get that exact SHA-1. So\nto pass a ref name to git fetch the supplier would have to actually\npass a tuple of ref+sha1, and then the fetcher has to check that the\nref it just obtained provides the sha1 it expected.\n\nSo this all turned into a bug report by me to Jonathan Tan, because\ngit fetch violated my assumption of what it would do if I handed it a\nSHA-1 and the SHA-1 was currently the tip of a remote ref.\n\n\n[1] github.com\n"},{"id":"319247","messageId":"20170510043343.mgb7heqzu2etcgvf@sigill.intra.peff.net","threadId":"45912","inReplyTo":"CAJo=hJvAg2WqpiuykpbHcB5vgQiHJ74CZ8Y4qudkYqZrmd30zg@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-10T04:33:44Z","receivedAt":"2017-05-10T04:33:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:\n\n> > Hmm. That makes sense generally, as the request should succeed. But it\n> > seems like we're creating a client that will sometimes succeed and\n> > sometimes fail, and the reasoning will be somewhat opaque to the user.\n> > I have a feeling I'm missing some context on when you'd expect this to\n> > kick in.\n> \n> Specifically, someone I know was looking at building an application\n> that is passed only a SHA-1 for the tip of a ref on a popular hosting\n> site[1]. They wanted to run `git fetch URL SHA1`, but this failed\n> because the site doesn't have upload.allowtipsha1inwant enabled.\n> However the SHA1 was clearly in the output of git ls-remote.\n\nOK. So this is basically a case where we expect that the user knows what\nthey're doing.\n\n> For various reasons they expected this to work, because it works\n> against other sites that do have upload.allowtipsha1inwant enabled.\n> And I personally just expected it to work because the fetch client\n> accepts SHA-1s, and the wire protocol uses \"want SHA1\" not \"want ref\",\n> and the SHA-1 passed on the command line was currently in the\n> advertisement when the connection opened, so its certainly something\n> the server is currently willing to serve.\n\nRight, makes sense. I wondered if GitHub should be turning on\nallowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\nsome internal refs[1], and they're things that users wouldn't want to\nfetch. The problem for your case really is just on the client side, and\nthis patch fixes it.\n\nSome of this context might be useful in the commit message.\n\n-Peff\n\n[1] The reachability checks from upload-pack don't actually do much on\n GitHub, because you can generally access the objects via the API or\n the web site anyway. So I'm not really opposed to turning on\n allowTipSHA1InWant if it would be useful for users, but after\n Jonathan's patch I don't see how it would be.\n"},{"id":"319250","messageId":"CAJo=hJuaPhxQwD+DdMcP_21j61KR_GY=P+tHrYScgzZFrtemEA@mail.gmail.com","threadId":"45912","inReplyTo":"20170510043343.mgb7heqzu2etcgvf@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2017-05-10T04:57:20Z","receivedAt":"2017-05-10T04:57:46Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, May 9, 2017 at 9:33 PM, Jeff King wrote:\n> On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:\n>\n>> > Hmm. That makes sense generally, as the request should succeed. But it\n>> > seems like we're creating a client that will sometimes succeed and\n>> > sometimes fail, and the reasoning will be somewhat opaque to the user.\n>> > I have a feeling I'm missing some context on when you'd expect this to\n>> > kick in.\n>>\n>> Specifically, someone I know was looking at building an application\n>> that is passed only a SHA-1 for the tip of a ref on a popular hosting\n>> site[1]. They wanted to run `git fetch URL SHA1`, but this failed\n>> because the site doesn't have upload.allowtipsha1inwant enabled.\n>> However the SHA1 was clearly in the output of git ls-remote.\n>\n> OK. So this is basically a case where we expect that the user knows what\n> they're doing.\n>\n>> For various reasons they expected this to work, because it works\n>> against other sites that do have upload.allowtipsha1inwant enabled.\n>> And I personally just expected it to work because the fetch client\n>> accepts SHA-1s, and the wire protocol uses \"want SHA1\" not \"want ref\",\n>> and the SHA-1 passed on the command line was currently in the\n>> advertisement when the connection opened, so its certainly something\n>> the server is currently willing to serve.\n>\n> Right, makes sense. I wondered if GitHub should be turning on\n> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\n> some internal refs[1], and they're things that users wouldn't want to\n> fetch. The problem for your case really is just on the client side, and\n> this patch fixes it.\n>\n> Some of this context might be useful in the commit message.\n>\n> -Peff\n>\n> [1] The reachability checks from upload-pack don't actually do much on\n> GitHub, because you can generally access the objects via the API or\n> the web site anyway. So I'm not really opposed to turning on\n> allowTipSHA1InWant if it would be useful for users, but after\n> Jonathan's patch I don't see how it would be.\n\nRight, I agree. That is why we proposed this patch to the client,\nrather than a support request with GitHub to change server\nconfiguration. :)\n"},{"id":"319254","messageId":"20170510044626.g4dwcujfr7vhv55d@glandium.org","threadId":"45912","inReplyTo":"20170510043343.mgb7heqzu2etcgvf@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2017-05-10T04:46:26Z","receivedAt":"2017-05-10T05:26:40Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, May 10, 2017 at 12:33:44AM -0400, Jeff King wrote:\n> On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:\n> \n> > > Hmm. That makes sense generally, as the request should succeed. But it\n> > > seems like we're creating a client that will sometimes succeed and\n> > > sometimes fail, and the reasoning will be somewhat opaque to the user.\n> > > I have a feeling I'm missing some context on when you'd expect this to\n> > > kick in.\n> > \n> > Specifically, someone I know was looking at building an application\n> > that is passed only a SHA-1 for the tip of a ref on a popular hosting\n> > site[1]. They wanted to run `git fetch URL SHA1`, but this failed\n> > because the site doesn't have upload.allowtipsha1inwant enabled.\n> > However the SHA1 was clearly in the output of git ls-remote.\n> \n> OK. So this is basically a case where we expect that the user knows what\n> they're doing.\n> \n> > For various reasons they expected this to work, because it works\n> > against other sites that do have upload.allowtipsha1inwant enabled.\n> > And I personally just expected it to work because the fetch client\n> > accepts SHA-1s, and the wire protocol uses \"want SHA1\" not \"want ref\",\n> > and the SHA-1 passed on the command line was currently in the\n> > advertisement when the connection opened, so its certainly something\n> > the server is currently willing to serve.\n> \n> Right, makes sense. I wondered if GitHub should be turning on\n> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\n> some internal refs[1], and they're things that users wouldn't want to\n> fetch. The problem for your case really is just on the client side, and\n> this patch fixes it.\n\nMore broadly, I think it is desirable that any commit that can be\nreached from public refs can be fetched by an explicit sha1 without\nallowTipSHA1InWant.\n\nMike\n"},{"id":"319278","messageId":"20170510164432.5447-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v2] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-10T16:44:32Z","receivedAt":"2017-05-10T16:44:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nSigned-off-by: Jonathan Tan \n---\n\nThanks, peff, for your suggestions. Changes from v1:\n - updated commit message to explain a bit of the context\n - use parse_oid_hex instead of get_oid_hex to avoid the constant 40\n\npeff also suggested a possible optimization in computing is_literal_sha1\noutside the loop. After some thought, I don't think that it takes that\nmuch time in the general case where the ref name starts with \"refs/\",\nbecause parse_oid_hex will see the \"r\" (which is not in \"[0-9a-f]\") and\nimmediately return. So I left it as-is.\n\n fetch-pack.c | 31 +++++++++++++++++--------------\n t/t5500-fetch-pack.sh | 6 ++++++\n 2 files changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..d4b57b9ba 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -592,6 +592,15 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static int is_literal_sha1(const struct ref *ref)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\treturn !parse_oid_hex(ref->name, &oid, &end) &&\n+\t !*end &&\n+\t !oidcmp(&oid, &ref->old_oid);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n@@ -601,7 +610,6 @@ static void filter_refs(struct fetch_pack_args *args,\n \tstruct ref *ref, *next;\n \tint i;\n \n-\ti = 0;\n \tfor (ref = *refs; ref; ref = next) {\n \t\tint keep = 0;\n \t\tnext = ref->next;\n@@ -610,15 +618,14 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t check_refname_format(ref->name, 0))\n \t\t\t; /* trash */\n \t\telse {\n-\t\t\twhile (i < nr_sought) {\n-\t\t\t\tint cmp = strcmp(ref->name, sought[i]->name);\n-\t\t\t\tif (cmp < 0)\n-\t\t\t\t\tbreak; /* definitely do not have it */\n-\t\t\t\telse if (cmp == 0) {\n-\t\t\t\t\tkeep = 1; /* definitely have it */\n-\t\t\t\t\tsought[i]->match_status = REF_MATCHED;\n+\t\t\tfor (i = 0; i < nr_sought; i++) {\n+\t\t\t\tstruct ref *s = sought[i];\n+\t\t\t\tif (!strcmp(ref->name, s->name) ||\n+\t\t\t\t (is_literal_sha1(s) &&\n+\t\t\t\t !oidcmp(&ref->old_oid, &s->old_oid))) {\n+\t\t\t\t\tkeep = 1;\n+\t\t\t\t\ts->match_status = REF_MATCHED;\n \t\t\t\t}\n-\t\t\t\ti++;\n \t\t\t}\n \t\t}\n \n@@ -637,14 +644,10 @@ static void filter_refs(struct fetch_pack_args *args,\n \n \t/* Append unmatched requests to the list */\n \tfor (i = 0; i < nr_sought; i++) {\n-\t\tunsigned char sha1[20];\n-\n \t\tref = sought[i];\n \t\tif (ref->match_status != REF_NOT_MATCHED)\n \t\t\tcontinue;\n-\t\tif (get_sha1_hex(ref->name, sha1) ||\n-\t\t ref->name[40] != '\\0' ||\n-\t\t hashcmp(sha1, ref->old_oid.hash))\n+\t\tif (!is_literal_sha1(ref))\n \t\t\tcontinue;\n \n \t\tif ((allow_unadvertised_object_request &\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..d87983bc2 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\tgit init server &&\n+\ttest_commit -C server 4 &&\n+\tgit fetch-pack server $(git -C server rev-parse refs/heads/master)\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319280","messageId":"20170510170044.GX28740@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170510043343.mgb7heqzu2etcgvf@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-10T17:00:44Z","receivedAt":"2017-05-10T17:00:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> Right, makes sense. I wondered if GitHub should be turning on\n> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\n> some internal refs[1], and they're things that users wouldn't want to\n> fetch. The problem for your case really is just on the client side, and\n> this patch fixes it.\n[...]\n> [1] The reachability checks from upload-pack don't actually do much on\n> GitHub, because you can generally access the objects via the API or\n> the web site anyway. So I'm not really opposed to turning on\n> allowTipSHA1InWant if it would be useful for users, but after\n> Jonathan's patch I don't see how it would be.\n\nGiven that, what would make me really happy is if github enables\nuploadpack.allowAnySHA1InWant. That would be useful for me, at least.\n\nThanks,\nJonathan\n"},{"id":"319283","messageId":"CACBZZX4AnmP2N_-WLtqQncUDCZDeWS3LFomZrWLYP6iW-hL5KA@mail.gmail.com","threadId":"45912","inReplyTo":"20170510044626.g4dwcujfr7vhv55d@glandium.org","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-10T17:50:01Z","receivedAt":"2017-05-10T17:50:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 10, 2017 at 6:46 AM, Mike Hommey wrote:\n> On Wed, May 10, 2017 at 12:33:44AM -0400, Jeff King wrote:\n>> On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:\n>>\n>> > > Hmm. That makes sense generally, as the request should succeed. But it\n>> > > seems like we're creating a client that will sometimes succeed and\n>> > > sometimes fail, and the reasoning will be somewhat opaque to the user.\n>> > > I have a feeling I'm missing some context on when you'd expect this to\n>> > > kick in.\n>> >\n>> > Specifically, someone I know was looking at building an application\n>> > that is passed only a SHA-1 for the tip of a ref on a popular hosting\n>> > site[1]. They wanted to run `git fetch URL SHA1`, but this failed\n>> > because the site doesn't have upload.allowtipsha1inwant enabled.\n>> > However the SHA1 was clearly in the output of git ls-remote.\n>>\n>> OK. So this is basically a case where we expect that the user knows what\n>> they're doing.\n>>\n>> > For various reasons they expected this to work, because it works\n>> > against other sites that do have upload.allowtipsha1inwant enabled.\n>> > And I personally just expected it to work because the fetch client\n>> > accepts SHA-1s, and the wire protocol uses \"want SHA1\" not \"want ref\",\n>> > and the SHA-1 passed on the command line was currently in the\n>> > advertisement when the connection opened, so its certainly something\n>> > the server is currently willing to serve.\n>>\n>> Right, makes sense. I wondered if GitHub should be turning on\n>> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\n>> some internal refs[1], and they're things that users wouldn't want to\n>> fetch. The problem for your case really is just on the client side, and\n>> this patch fixes it.\n>\n> More broadly, I think it is desirable that any commit that can be\n> reached from public refs can be fetched by an explicit sha1 without\n> allowTipSHA1InWant.\n\nJust a side question, what are the people who use this feature using\nit for? The only thing I can think of myself is some out of band ref\nadvertisement because you've got squillions of refs as a hack around\ngit's limitations in that area.\n\nAre there other use-cases for this? All the commits[1] that touched\nthis feature just explain what, not why.\n\n1. git log --reverse -p -i -Gallowtipsha1inwant\n"},{"id":"319284","messageId":"20170510180130.GY28740@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170510164432.5447-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-10T18:01:30Z","receivedAt":"2017-05-10T18:01:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> fetch-pack, when fetching a literal SHA-1 from a server that is not\n> configured with uploadpack.allowtipsha1inwant (or similar), always\n> returns an error message of the form \"Server does not allow request for\n> unadvertised object %s\". However, it is sometimes the case that such\n> object is advertised.\n\nThanks for fixing it.\n\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -592,6 +592,15 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n> \t}\n> }\n> \n> +static int is_literal_sha1(const struct ref *ref)\n> +{\n> +\tstruct object_id oid;\n> +\tconst char *end;\n> +\treturn !parse_oid_hex(ref->name, &oid, &end) &&\n> +\t !*end &&\n> +\t !oidcmp(&oid, &ref->old_oid);\n\nWhen would a ref have its name be a oid and its value be a different\noid?\n\nAh, this check comes from\n\n\tcommit b7916422c741b50925d454819b1977449fccc111\n\tAuthor: Jeff King \n\tDate: Thu Mar 19 16:34:51 2015 -0400\n\n\t filter_ref: avoid overwriting ref->old_sha1 with garbage\n\nwhich explains that the answer is \"never\" (leaving me even more\nconfused). Anyway, that's orthogonal to this patch.\n\n> +}\n> +\n> static void filter_refs(struct fetch_pack_args *args,\n> \t\t\tstruct ref **refs,\n> \t\t\tstruct ref **sought, int nr_sought)\n> @@ -601,7 +610,6 @@ static void filter_refs(struct fetch_pack_args *args,\n> \tstruct ref *ref, *next;\n> \tint i;\n> \n> -\ti = 0;\n> \tfor (ref = *refs; ref; ref = next) {\n> \t\tint keep = 0;\n> \t\tnext = ref->next;\n> @@ -610,15 +618,14 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\t check_refname_format(ref->name, 0))\n> \t\t\t; /* trash */\n> \t\telse {\n> -\t\t\twhile (i < nr_sought) {\n> -\t\t\t\tint cmp = strcmp(ref->name, sought[i]->name);\n> -\t\t\t\tif (cmp < 0)\n> -\t\t\t\t\tbreak; /* definitely do not have it */\n> -\t\t\t\telse if (cmp == 0) {\n> -\t\t\t\t\tkeep = 1; /* definitely have it */\n> -\t\t\t\t\tsought[i]->match_status = REF_MATCHED;\n> +\t\t\tfor (i = 0; i < nr_sought; i++) {\n\nThis resets i each time this code path is encountered. Doesn't that\nmake this more expensive e.g. in the case where there are no sha1s\namong the refs to be searched for?\n\nShould this be conditional on whether there are any sha1-shaped refs in\n'sought' to avoid that performance regression?\n\nIt's tempting to split 'sought' into two lists --- sha1-shaped refs\nthat have to be processed in full every time and the others that can\nbe processed in a single pass because they are in sorted order.\n\n[...]\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n> \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n> '\n> \n> +test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n> +\tgit init server &&\n> +\ttest_commit -C server 4 &&\n> +\tgit fetch-pack server $(git -C server rev-parse refs/heads/master)\n> +'\n\nIs there test for the error case, too? (I.e. the server does not advertise\nfetch-by-sha1 and the sha1 is not among the advertised refs)\n\nThanks,\nJonathan\n"},{"id":"319285","messageId":"20170510182049.GZ28740@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"CACBZZX4AnmP2N_-WLtqQncUDCZDeWS3LFomZrWLYP6iW-hL5KA@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-10T18:20:49Z","receivedAt":"2017-05-10T18:21:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n\n> Just a side question, what are the people who use this feature using\n> it for? The only thing I can think of myself is some out of band ref\n> advertisement because you've got squillions of refs as a hack around\n> git's limitations in that area.\n\nThat's one use case.\n\nAnother is when you really care about the exact sha1 (for example\nbecause you are an automated build system and this is the specific\nsha1 you have already decided you want to build).\n\n> Are there other use-cases for this? All the commits[1] that touched\n> this feature just explain what, not why.\n\nSimilar to the build system case I described above is when a human has\na sha1 (from a mailing list, or source browser, or whatever) and wants\nto fetch just that revision, with --depth=1. You could use \"git\narchive --remote\", but (1) github doesn't support that and (2) that\ndoesn't give you all the usual git-ish goodness.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"319286","messageId":"2839966.sklLQJ8GHA@mfick1-lnx","threadId":"45912","inReplyTo":"20170510182049.GZ28740@aiede.svl.corp.google.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Martin Fick","fromEmail":"mfick@codeaurora.org","sentAt":"2017-05-10T18:48:37Z","receivedAt":"2017-05-10T18:48:45Z","isPatch":true,"sender":{"key":"mfick@codeaurora.org","avatar":null},"body":"On Wednesday, May 10, 2017 11:20:49 AM Jonathan Nieder \nwrote:\n> Hi,\n> \n> Ævar Arnfjörð Bjarmason wrote:\n> > Just a side question, what are the people who use this\n> > feature using it for? The only thing I can think of\n> > myself is some out of band ref advertisement because\n> > you've got squillions of refs as a hack around git's\n> > limitations in that area.\n> \n> That's one use case.\n> \n> Another is when you really care about the exact sha1 (for\n> example because you are an automated build system and\n> this is the specific sha1 you have already decided you\n> want to build).\n> > Are there other use-cases for this? All the commits[1]\n> > that touched this feature just explain what, not why.\n> \n> Similar to the build system case I described above is when\n> a human has a sha1 (from a mailing list, or source\n> browser, or whatever) and wants to fetch just that\n> revision, with --depth=1. You could use \"git archive\n> --remote\", but (1) github doesn't support that and (2)\n> that doesn't give you all the usual git-ish goodness.\n\n\nPerhaps another use case is submodules and repo(android \ntool) subprojects since they can be \"pinned\" to sha1s,\n\n-Martin\n-- \nThe Qualcomm Innovation Center, Inc. is a member of Code \nAurora Forum, hosted by The Linux Foundation\n\n"},{"id":"319287","messageId":"20170510185412.GB28740@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"2839966.sklLQJ8GHA@mfick1-lnx","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-10T18:54:12Z","receivedAt":"2017-05-10T18:54:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Wed, May 10, 2017 at 12:48:37PM -0600, Martin Fick wrote:\n>> Ævar Arnfjörð Bjarmason wrote:\n\n>>> Just a side question, what are the people who use this\n>>> feature using it for? The only thing I can think of\n>>> myself is some out of band ref advertisement because\n>>> you've got squillions of refs as a hack around git's\n>>> limitations in that area.\n[...]\n> Perhaps another use case is submodules and repo(android\n> tool) subprojects since they can be \"pinned\" to sha1s,\n\nYes, thanks for mentioning that. We've talked a little about making\n'git fetch --recurse-submodules' use this feature, either always, or\nif after the usual fetch the desired commit is not available in the\nsubmodule.\n\nJonathan\n"},{"id":"319288","messageId":"86ede049-ac9e-0886-acde-42e53ef30243@gmail.com","threadId":"45912","inReplyTo":"20170510170044.GX28740@aiede.svl.corp.google.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-05-10T18:55:19Z","receivedAt":"2017-05-10T18:55:46Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2017-05-10 19:00, Jonathan Nieder wrote:\n\n>> Right, makes sense. I wondered if GitHub should be turning on\n>> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\n>> some internal refs[1], and they're things that users wouldn't want to\n>> fetch. The problem for your case really is just on the client side, and\n>> this patch fixes it.\n> [...]\n>> [1] The reachability checks from upload-pack don't actually do much on\n>> GitHub, because you can generally access the objects via the API or\n>> the web site anyway. So I'm not really opposed to turning on\n>> allowTipSHA1InWant if it would be useful for users, but after\n>> Jonathan's patch I don't see how it would be.\n> \n> Given that, what would make me really happy is if github enables\n> uploadpack.allowAnySHA1InWant. That would be useful for me, at least.\n\nMe too, BTW. If you're only interested in building / analyzing a \nspecific SHA1 it's really a waste of network resources to fetch all of \nthe history.\n\n-- \nSebastian Schuberth\n\n"},{"id":"319298","messageId":"20170510221157.8971-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-10T22:11:57Z","receivedAt":"2017-05-10T22:12:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nSigned-off-by: Jonathan Tan \n---\n\nReviewers, note that the patch has been substantially rewritten.\n\nAfter looking at ways to solve jrnieder's performance concerns, if we're\ngoing to need to manage one more item of state within the function, I\nmight as well use my earlier idea of storing unmatched refs in its own\nlist instead of immediately freeing them. This version of the patch\nshould have much better performance characteristics.\n\nI have also added the test requested by jrnieder (to show that\nfetch-pack cannot fetch a SHA-1 if it is not advertised as a ref).\n\n fetch-pack.c | 29 ++++++++++++++++++++++++++++-\n t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..5cace7458 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -598,6 +598,7 @@ static void filter_refs(struct fetch_pack_args *args,\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n \tint i;\n \n@@ -631,13 +632,15 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n \t/* Append unmatched requests to the list */\n \tfor (i = 0; i < nr_sought; i++) {\n \t\tunsigned char sha1[20];\n+\t\tint can_append = 0;\n \n \t\tref = sought[i];\n \t\tif (ref->match_status != REF_NOT_MATCHED)\n@@ -649,6 +652,25 @@ static void filter_refs(struct fetch_pack_args *args,\n \n \t\tif ((allow_unadvertised_object_request &\n \t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\t\tcan_append = 1;\n+\t\t} else {\n+\t\t\tstruct ref *u;\n+\t\t\t/* Check all refs, including those already matched */\n+\t\t\tfor (u = unmatched; u; u = u->next) {\n+\t\t\t\tif (!oidcmp(&ref->old_oid, &u->old_oid)) {\n+\t\t\t\t\tcan_append = 1;\n+\t\t\t\t\tgoto can_append;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tfor (u = newlist; u; u = u->next) {\n+\t\t\t\tif (!oidcmp(&ref->old_oid, &u->old_oid)) {\n+\t\t\t\t\tcan_append = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+can_append:\n+\t\tif (can_append) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -657,6 +679,11 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t}\n \t}\n \t*refs = newlist;\n+\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n }\n \n static void mark_alternate_complete(struct object *obj)\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..80a1a3239 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master)\n+'\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_commit -C server 2 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+\trm -rf server &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 5 &&\n+\tgit -C server tag -d 5 &&\n+\ttest_commit -C server 6 &&\n+\n+\tgit init client &&\n+\ttest_must_fail git -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319302","messageId":"20170510232231.GC28740@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170510221157.8971-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-10T23:22:31Z","receivedAt":"2017-05-10T23:22:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> fetch-pack, when fetching a literal SHA-1 from a server that is not\n> configured with uploadpack.allowtipsha1inwant (or similar), always\n> returns an error message of the form \"Server does not allow request for\n> unadvertised object %s\". However, it is sometimes the case that such\n> object is advertised. This situation would occur, for example, if a user\n> or a script was provided a SHA-1 instead of a branch or tag name for\n> fetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\n> that SHA-1.\n\nYay, thanks again.\n\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -598,6 +598,7 @@ static void filter_refs(struct fetch_pack_args *args,\n> {\n> \tstruct ref *newlist = NULL;\n> \tstruct ref **newtail = &newlist;\n> +\tstruct ref *unmatched = NULL;\n> \tstruct ref *ref, *next;\n> \tint i;\n> \n> @@ -631,13 +632,15 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\t\tref->next = NULL;\n> \t\t\tnewtail = &ref->next;\n> \t\t} else {\n> -\t\t\tfree(ref);\n> +\t\t\tref->next = unmatched;\n> +\t\t\tunmatched = ref;\n\nInteresting. Makes sense.\n\n[...]\n> \t/* Append unmatched requests to the list */\n> \tfor (i = 0; i < nr_sought; i++) {\n> \t\tunsigned char sha1[20];\n> +\t\tint can_append = 0;\n\nCan this be simplified by factoring out a function? I.e. something\nlike\n\n\tif ((... ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)\n\t || find_oid_among_refs(unmatched, oid)\n\t || find_oid_among_refs(newlist, oid)) {\n\t\tref->match_status = REF_MATCHED;\n\t\t...\n\t} else {\n\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n\t}\n\n[...]\n> @@ -657,6 +679,11 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\t}\n> \t}\n> \t*refs = newlist;\n> +\n> +\tfor (ref = unmatched; ref; ref = next) {\n> +\t\tnext = ref->next;\n> +\t\tfree(ref);\n> +\t}\n> }\n\noptional nit: keeping the \"*refs =\" line as the last line of the\nfunction would make it easier to see the contract at a glance. (That\nsaid, a doc comment at the top would make it even clearer.)\n\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n\nYay, thanks much for these.\n\n[...]\n> +test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n\nHa, you read my mind. :)\n\nExcept for the search-ref-list-for-oid function needing to be factored out,\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"319379","messageId":"20170511094635.ljpwg3bxkqj2wmgc@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170510221157.8971-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-11T09:46:36Z","receivedAt":"2017-05-11T09:46:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 10, 2017 at 03:11:57PM -0700, Jonathan Tan wrote:\n\n> After looking at ways to solve jrnieder's performance concerns, if we're\n> going to need to manage one more item of state within the function, I\n> might as well use my earlier idea of storing unmatched refs in its own\n> list instead of immediately freeing them. This version of the patch\n> should have much better performance characteristics.\n\nHrm. So the problem in your original was that the loop became quadratic\nin the number of refs when fetching all of them (because the original\nrelies on the sorting to essentially do a list-merge). Are there any\nquadratic bits left?\n\n> @@ -649,6 +652,25 @@ static void filter_refs(struct fetch_pack_args *args,\n> \n> \t\tif ((allow_unadvertised_object_request &\n> \t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n> +\t\t\tcan_append = 1;\n> +\t\t} else {\n> +\t\t\tstruct ref *u;\n> +\t\t\t/* Check all refs, including those already matched */\n> +\t\t\tfor (u = unmatched; u; u = u->next) {\n> +\t\t\t\tif (!oidcmp(&ref->old_oid, &u->old_oid)) {\n> +\t\t\t\t\tcan_append = 1;\n> +\t\t\t\t\tgoto can_append;\n> +\t\t\t\t}\n> +\t\t\t}\n\nThis is inside the nr_sought loop. So if I were to do:\n\n git fetch origin $(git ls-remote origin | awk '{print $1}')\n\nwe're back to being quadratic. I realize that's probably a silly thing\nto do, but in the general case, you're O(m*n), where \"n\" is number of\nunmatched remote refs and \"m\" is the number of SHA-1s you're looking\nfor.\n\nDoing better would require either sorting both lists, or storing the\noids in something that has better than linear-time lookup. Perhaps a\nsha1_array or an oidset? We don't actually need to know anything about\nthe unmatched refs after the first loop. We just need the list of oids\nthat let us do can_append.\n\nAIUI, you could also avoid creating the unmatched list entirely when the\nserver advertises tip/reachable sha1s. That's a small optimization, but\nI think it may actually make the logic clearer.\n\n-Peff\n"},{"id":"319383","messageId":"20170511095925.grmyagv4hesxqprj@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170510170044.GX28740@aiede.svl.corp.google.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-11T09:59:25Z","receivedAt":"2017-05-11T09:59:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 10, 2017 at 10:00:44AM -0700, Jonathan Nieder wrote:\n\n> > Right, makes sense. I wondered if GitHub should be turning on\n> > allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide\n> > some internal refs[1], and they're things that users wouldn't want to\n> > fetch. The problem for your case really is just on the client side, and\n> > this patch fixes it.\n> [...]\n> > [1] The reachability checks from upload-pack don't actually do much on\n> > GitHub, because you can generally access the objects via the API or\n> > the web site anyway. So I'm not really opposed to turning on\n> > allowTipSHA1InWant if it would be useful for users, but after\n> > Jonathan's patch I don't see how it would be.\n> \n> Given that, what would make me really happy is if github enables\n> uploadpack.allowAnySHA1InWant. That would be useful for me, at least.\n\nOne of my hesitations is that we've actually considered moving in the\nopposite direction. The object storage for all of the repositories in a\nnetwork is shared, so I can fork git.git, push up malicious crap, and\nthen point people to:\n\n https://github.com/git/git/commit/$sha1\n\nand it resolves. Obviously there's a social-engineering component to any\nsuch attack, but it's not great. And even without security in mind, it's\npotentially confusing. So we've looked at enforcing reachability from\nthe refs of git/git for a case like that. There's some collateral\ndamage, though (e.g., people might actually want to look at unreferenced\nobjects after a force-push). And there are complications around things\nlike refs/pull (which could still come from another fork, but which you\nmight reasonably want to reference as part of a PR in the context of\ngit/git).\n\nTurning on allowAnySHA1InWant brings that confusion to \"git fetch\", too.\nTo some degree it's already there for refs/pull, but with the current\nclient you at least know that you're fetching PR refs (and they're not\neven fetched by default). Whereas after Jonathan Tan's patch, you can\nsocial-engineer somebody into:\n\n git fetch https://github.com/git/git $sha1\n\nif you open a PR that points to some malicious $sha1. I don't think\nthat's a reason not to take his patch, though.\n\nArguably refs/pull/ is an abomination that mixes up ownership and should\nbe destroyed. There really isn't a great alternative, though, short of\nrepresenting it as a completely separate repository (which would mean\nanybody fetching those refs would have to make a separate fetch\nrequest).\n\nBut even leaving all the refs/pull stuff aside, allowAnySHA1InWant does\nseem to increase that confusion, and I don't see a way around it short\nof never sharing objects between repositories at all. So I think at most\nwe'd do allowReachableSHA1InWant.\n\n-Peff\n"},{"id":"319384","messageId":"20170511100553.ptyazchujgdfxotl@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170510221157.8971-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-11T10:05:54Z","receivedAt":"2017-05-11T10:06:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 10, 2017 at 03:11:57PM -0700, Jonathan Tan wrote:\n\n> fetch-pack, when fetching a literal SHA-1 from a server that is not\n> configured with uploadpack.allowtipsha1inwant (or similar), always\n> returns an error message of the form \"Server does not allow request for\n> unadvertised object %s\". However, it is sometimes the case that such\n> object is advertised. This situation would occur, for example, if a user\n> or a script was provided a SHA-1 instead of a branch or tag name for\n> fetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\n> that SHA-1.\n> \n> Teach fetch-pack to also check the SHA-1s of the refs in the received\n> ref advertisement if a literal SHA-1 was given by the user.\n\nStepping back a bit, what does this mean for a world where we implement\nprotocol extensions to let the client specify a set of refspecs to limit\nthe advertisement?\n\nIf we give the server our usual set of fetch refspecs, then we might\nfail to fulfill a request that would have been advertised outside of\nthat set. And the behavior is confusing and non-transparent to the user.\nI don't think that really makes sense, though; the advertisement we ask\nfor from the server should include only the bits we're interested in for\n_this_ fetch.\n\nIf we tell the server \"we are interested in abcd1234\", then it's not\ngoing to find any matching ref by name, obviously. So should servers\nthen treat 40-hex names in the incoming refspecs as a request to show\nany names which have a matching sha1? That works against any server-side\noptimizations to avoid looking at the complete set of refs, but it would\nonly have to kick in when the user actually specified a single SHA-1\n(and even then only when allowAnySHA1 isn't on). So that's probably\nworkable.\n\nNone of this is your problem now either way; the advertisement-limiting\nextension is still vaporware, albeit one we've discussed a lot. I just\nwanted to make sure we weren't painting ourselves into any corners. And\nI think this case could probably be handled.\n\n-Peff\n"},{"id":"319413","messageId":"20170511170050.GA83655@google.com","threadId":"45912","inReplyTo":"20170511100553.ptyazchujgdfxotl@sigill.intra.peff.net","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-11T17:00:50Z","receivedAt":"2017-05-11T17:01:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/11, Jeff King wrote:\n> On Wed, May 10, 2017 at 03:11:57PM -0700, Jonathan Tan wrote:\n> \n> > fetch-pack, when fetching a literal SHA-1 from a server that is not\n> > configured with uploadpack.allowtipsha1inwant (or similar), always\n> > returns an error message of the form \"Server does not allow request for\n> > unadvertised object %s\". However, it is sometimes the case that such\n> > object is advertised. This situation would occur, for example, if a user\n> > or a script was provided a SHA-1 instead of a branch or tag name for\n> > fetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\n> > that SHA-1.\n> > \n> > Teach fetch-pack to also check the SHA-1s of the refs in the received\n> > ref advertisement if a literal SHA-1 was given by the user.\n> \n> Stepping back a bit, what does this mean for a world where we implement\n> protocol extensions to let the client specify a set of refspecs to limit\n> the advertisement?\n> \n> If we give the server our usual set of fetch refspecs, then we might\n> fail to fulfill a request that would have been advertised outside of\n> that set. And the behavior is confusing and non-transparent to the user.\n> I don't think that really makes sense, though; the advertisement we ask\n> for from the server should include only the bits we're interested in for\n> _this_ fetch.\n> \n> If we tell the server \"we are interested in abcd1234\", then it's not\n> going to find any matching ref by name, obviously. So should servers\n> then treat 40-hex names in the incoming refspecs as a request to show\n> any names which have a matching sha1? That works against any server-side\n> optimizations to avoid looking at the complete set of refs, but it would\n> only have to kick in when the user actually specified a single SHA-1\n> (and even then only when allowAnySHA1 isn't on). So that's probably\n> workable.\n> \n> None of this is your problem now either way; the advertisement-limiting\n> extension is still vaporware, albeit one we've discussed a lot. I just\n> wanted to make sure we weren't painting ourselves into any corners. And\n> I think this case could probably be handled.\n\nI can't remember, is there any reason why it is still vaporware? As in\nwhat is holding us back from doing the advertisement-limiting (or doing\naway with the initial ref advertisement)?\n\n-- \nBrandon Williams\n"},{"id":"319429","messageId":"695d833e-9ef1-4d74-265e-f5e3af8a54cd@google.com","threadId":"45912","inReplyTo":"20170511094635.ljpwg3bxkqj2wmgc@sigill.intra.peff.net","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-11T17:51:37Z","receivedAt":"2017-05-11T17:51:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for your suggestions. I'll hold off on sending out a new patch \n(following Jonathan Nieder's suggestions [1]) until we decide if further \noptimizations (for example, as suggested by Peff) need to be done.\n\n[1] <20170510232231.GC28740@aiede.svl.corp.google.com>\n\nOn 05/11/2017 02:46 AM, Jeff King wrote:\n> On Wed, May 10, 2017 at 03:11:57PM -0700, Jonathan Tan wrote:\n>\n>> After looking at ways to solve jrnieder's performance concerns, if we're\n>> going to need to manage one more item of state within the function, I\n>> might as well use my earlier idea of storing unmatched refs in its own\n>> list instead of immediately freeing them. This version of the patch\n>> should have much better performance characteristics.\n>\n> Hrm. So the problem in your original was that the loop became quadratic\n> in the number of refs when fetching all of them (because the original\n> relies on the sorting to essentially do a list-merge). Are there any\n> quadratic bits left?\n>\n>> @@ -649,6 +652,25 @@ static void filter_refs(struct fetch_pack_args *args,\n>>\n>> \t\tif ((allow_unadvertised_object_request &\n>> \t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n>> +\t\t\tcan_append = 1;\n>> +\t\t} else {\n>> +\t\t\tstruct ref *u;\n>> +\t\t\t/* Check all refs, including those already matched */\n>> +\t\t\tfor (u = unmatched; u; u = u->next) {\n>> +\t\t\t\tif (!oidcmp(&ref->old_oid, &u->old_oid)) {\n>> +\t\t\t\t\tcan_append = 1;\n>> +\t\t\t\t\tgoto can_append;\n>> +\t\t\t\t}\n>> +\t\t\t}\n>\n> This is inside the nr_sought loop. So if I were to do:\n>\n> git fetch origin $(git ls-remote origin | awk '{print $1}')\n>\n> we're back to being quadratic. I realize that's probably a silly thing\n> to do, but in the general case, you're O(m*n), where \"n\" is number of\n> unmatched remote refs and \"m\" is the number of SHA-1s you're looking\n> for.\n\nThe original patch was quadratic regardless of whether we're fetching \nnames or SHA-1s, which can be said to be bad since it is a regression in \nan existing and common use case (and I agree). This one is O(m*n), as \nyou said - the \"quadratic-ness\" only kicks in if you fetch SHA-1s, which \nbefore this patch is impossible.\n\n> Doing better would require either sorting both lists, or storing the\n> oids in something that has better than linear-time lookup. Perhaps a\n> sha1_array or an oidset? We don't actually need to know anything about\n> the unmatched refs after the first loop. We just need the list of oids\n> that let us do can_append.\n\nHaving a sha1_array or oidset would require allocation (O(n log n) time, \nI think, in addition to O(n) space) and this cost would be incurred \nregardless of how many SHA-1s were actually fetched (if m is an order of \nmagnitude less than log n, for example, having a sha1_array might be \nactually worse). Also, presumably we don't want to incur this cost if we \nare fetching zero SHA-1s, so we would need to do some sort of pre-check. \nI'm inclined to leave it the way it is and consider this only if the use \ncase of fetching many SHA1-s comes up.\n\n> AIUI, you could also avoid creating the unmatched list entirely when the\n> server advertises tip/reachable sha1s. That's a small optimization, but\n> I think it may actually make the logic clearer.\n\nIf you mean adding an \"if\" block at the point where we add the unmatched \nref to the unmatched list (that either adds it to the list or \nimmediately frees it), I think it makes the logic slightly more \ncomplicated. Or maybe you had something else in mind?\n"},{"id":"319439","messageId":"20170511190354.GA12516@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170511095925.grmyagv4hesxqprj@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-11T19:03:54Z","receivedAt":"2017-05-11T19:04:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, May 10, 2017 at 10:00:44AM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> [1] The reachability checks from upload-pack don't actually do much on\n>>> GitHub, because you can generally access the objects via the API or\n>>> the web site anyway.\n[...]\n>> Given that, what would make me really happy is if github enables\n>> uploadpack.allowAnySHA1InWant. That would be useful for me, at least.\n>\n> One of my hesitations is that we've actually considered moving in the\n> opposite direction. The object storage for all of the repositories in a\n> network is shared, so I can fork git.git, push up malicious crap, and\n> then point people to:\n>\n> https://github.com/git/git/commit/$sha1\n>\n> and it resolves. Obviously there's a social-engineering component to any\n> such attack, but it's not great. And even without security in mind, it's\n> potentially confusing.\n[...]\n> But even leaving all the refs/pull stuff aside, allowAnySHA1InWant does\n> seem to increase that confusion, and I don't see a way around it short\n> of never sharing objects between repositories at all. So I think at most\n> we'd do allowReachableSHA1InWant.\n\nI had guessed you didn't want to do allowReachableSHA1InWant for\nperformance reasons. (I haven't checked to what extent we are already\ntaking advantage of bitmaps to avoid a slow reachability check.) If I\nwas wrong and allowReachableSHA1InWant is on the table then it is of\ncourse even better. :)\n\nThanks,\nJonathan\n"},{"id":"319466","messageId":"20170511205214.ofwkqogksohs6s4b@sigill.intra.peff.net","threadId":"45912","inReplyTo":"695d833e-9ef1-4d74-265e-f5e3af8a54cd@google.com","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-11T20:52:14Z","receivedAt":"2017-05-11T20:52:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 10:51:37AM -0700, Jonathan Tan wrote:\n\n> > This is inside the nr_sought loop. So if I were to do:\n> > \n> > git fetch origin $(git ls-remote origin | awk '{print $1}')\n> > \n> > we're back to being quadratic. I realize that's probably a silly thing\n> > to do, but in the general case, you're O(m*n), where \"n\" is number of\n> > unmatched remote refs and \"m\" is the number of SHA-1s you're looking\n> > for.\n> \n> The original patch was quadratic regardless of whether we're fetching names\n> or SHA-1s, which can be said to be bad since it is a regression in an\n> existing and common use case (and I agree). This one is O(m*n), as you said\n> - the \"quadratic-ness\" only kicks in if you fetch SHA-1s, which before this\n> patch is impossible.\n\nYeah, no question this is an improvement over the original. I think this\nstill \"regresses\" a certain request performance-wise, but it's a request\nthat could never have succeeded in the original. Mostly I'd just rather\nnot leave a hidden quadratic loop that will blow up in somebody's face\na year or two down the road.\n\n> Having a sha1_array or oidset would require allocation (O(n log n) time, I\n> think, in addition to O(n) space) and this cost would be incurred regardless\n> of how many SHA-1s were actually fetched (if m is an order of magnitude less\n> than log n, for example, having a sha1_array might be actually worse). Also,\n> presumably we don't want to incur this cost if we are fetching zero SHA-1s,\n> so we would need to do some sort of pre-check. I'm inclined to leave it the\n> way it is and consider this only if the use case of fetching many SHA1-s\n> comes up.\n\nAn oidset should be O(n) in both time and space. Which we're already\nspending in the earlier loop (and in general, when we allocate O(n) ref\nstructs in the first place). I don't think we need to care too much\nabout micro-optimizing bumps to the constant-factor here; we just need\nto make sure we don't end up in an algorithmic explosion.\n\nAnd if we initialize the oidset lazily, then we incur that cost only\nwhen we would be doing the linear walk in your patch anyway. See the\npatch below.\n\n> > AIUI, you could also avoid creating the unmatched list entirely when the\n> > server advertises tip/reachable sha1s. That's a small optimization, but\n> > I think it may actually make the logic clearer.\n> \n> If you mean adding an \"if\" block at the point where we add the unmatched ref\n> to the unmatched list (that either adds it to the list or immediately frees\n> it), I think it makes the logic slightly more complicated. Or maybe you had\n> something else in mind?\n\nI was just thinking that it does not leave a case where we create a\ndata structure (the unmatched list) even though we know right then that\nit will not ever be used. So it's an extra line of logic there, but the\noverall function may be clearer. I dunno, it's probably not a big deal\neither way.\n\n-Peff\n\n---\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 5cace7458..a660331e6 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,6 +593,12 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static void add_refs_to_oidset(struct oidset *oids, const struct ref *refs)\n+{\n+\tfor (; refs; refs = refs->next)\n+\t\toidset_insert(oids, &refs->old_oid);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n@@ -600,6 +607,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \tstruct ref **newtail = &newlist;\n \tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n+\tint tip_oids_initialized = 0;\n \tint i;\n \n \ti = 0;\n@@ -654,22 +663,15 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n \t\t\tcan_append = 1;\n \t\t} else {\n-\t\t\tstruct ref *u;\n-\t\t\t/* Check all refs, including those already matched */\n-\t\t\tfor (u = unmatched; u; u = u->next) {\n-\t\t\t\tif (!oidcmp(&ref->old_oid, &u->old_oid)) {\n-\t\t\t\t\tcan_append = 1;\n-\t\t\t\t\tgoto can_append;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tfor (u = newlist; u; u = u->next) {\n-\t\t\t\tif (!oidcmp(&ref->old_oid, &u->old_oid)) {\n-\t\t\t\t\tcan_append = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n+\t\t\tif (!tip_oids_initialized) {\n+\t\t\t\t/* Check all refs, including those already matched */\n+\t\t\t\tadd_refs_to_oidset(&tip_oids, unmatched);\n+\t\t\t\tadd_refs_to_oidset(&tip_oids, newlist);\n+\t\t\t\ttip_oids_initialized = 1;\n \t\t\t}\n+\t\t\tcan_append = oidset_contains(&tip_oids, &ref->old_oid);\n \t\t}\n-can_append:\n+\n \t\tif (can_append) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n@@ -680,6 +682,7 @@ static void filter_refs(struct fetch_pack_args *args,\n \t}\n \t*refs = newlist;\n \n+\toidset_clear(&tip_oids);\n \tfor (ref = unmatched; ref; ref = next) {\n \t\tnext = ref->next;\n \t\tfree(ref);\n"},{"id":"319467","messageId":"20170511210418.crsv2lolwoofdv3i@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170511190354.GA12516@aiede.svl.corp.google.com","subject":"Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-11T21:04:18Z","receivedAt":"2017-05-11T21:04:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 12:03:54PM -0700, Jonathan Nieder wrote:\n\n> > But even leaving all the refs/pull stuff aside, allowAnySHA1InWant does\n> > seem to increase that confusion, and I don't see a way around it short\n> > of never sharing objects between repositories at all. So I think at most\n> > we'd do allowReachableSHA1InWant.\n> \n> I had guessed you didn't want to do allowReachableSHA1InWant for\n> performance reasons. (I haven't checked to what extent we are already\n> taking advantage of bitmaps to avoid a slow reachability check.) If I\n> was wrong and allowReachableSHA1InWant is on the table then it is of\n> course even better. :)\n\nPerformance is definitely a consideration for resolving sha1's via the\nwebsite, because those can be any object, and you have to look in all\nthe trees. Traversing all the commits in linux.git is ~5s of CPU. Doing\nthe same for the whole object graph is more like ~45s.\n\nOur experiments did involve bitmaps, but there are some corner cases\nwhere we might not have good bitmap coverage of certain refs (I think\nthis is something that could be improved; the commit selection in the\ncurrent bitmap writer is fairly naive).\n\nBut I think upload-pack's reachability check only handles commits\nanyway. Adding 5s of CPU to a request isn't great, but it's pretty easy\nto convince Git to use 5s of CPU already. And this would only kick in\nwhen the client asked for a non-tip ref anyway, and in general I'd\nexpect requested sha1's to be near the ref tips.\n\nI'll open a discussion internally about enabling it and measuring the\nperformance.\n\n-Peff\n"},{"id":"319468","messageId":"20170511211403.5252-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v4] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-11T21:14:03Z","receivedAt":"2017-05-11T21:14:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n\nThanks, peff. I've incorporated your suggestions - I don't feel very\nstrongly about this, but I guess it's worthwhile to avoid the quadratic\nbehavior if we can.\n\nAlso incorporated Jonathan Nieder's suggestion about the placement of\nthe last line. The relevant function is also factored out (following\npeff's suggestion).\n---\n fetch-pack.c | 36 +++++++++++++++++++++++++++++++++++-\n t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..1adb1a6c2 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,13 +593,22 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static void add_refs_to_oidset(struct oidset *oids, const struct ref *refs)\n+{\n+\tfor (; refs; refs = refs->next)\n+\t\toidset_insert(oids, &refs->old_oid);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n+\tint tip_oids_initialized = 0;\n \tint i;\n \n \ti = 0;\n@@ -631,13 +641,15 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n \t/* Append unmatched requests to the list */\n \tfor (i = 0; i < nr_sought; i++) {\n \t\tunsigned char sha1[20];\n+\t\tint can_append = 0;\n \n \t\tref = sought[i];\n \t\tif (ref->match_status != REF_NOT_MATCHED)\n@@ -649,6 +661,21 @@ static void filter_refs(struct fetch_pack_args *args,\n \n \t\tif ((allow_unadvertised_object_request &\n \t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\t\tcan_append = 1;\n+\t\t} else {\n+\t\t\tif (!tip_oids_initialized) {\n+\t\t\t\t/*\n+\t\t\t\t * Check all refs, including those already\n+\t\t\t\t * matched\n+\t\t\t\t */\n+\t\t\t\tadd_refs_to_oidset(&tip_oids, unmatched);\n+\t\t\t\tadd_refs_to_oidset(&tip_oids, newlist);\n+\t\t\t\ttip_oids_initialized = 1;\n+\t\t\t}\n+\t\t\tcan_append = oidset_contains(&tip_oids, &ref->old_oid);\n+\t\t}\n+\n+\t\tif (can_append) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -656,6 +683,13 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n \t\t}\n \t}\n+\n+\toidset_clear(&tip_oids);\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n+\n \t*refs = newlist;\n }\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..80a1a3239 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master)\n+'\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_commit -C server 2 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+\trm -rf server &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 5 &&\n+\tgit -C server tag -d 5 &&\n+\ttest_commit -C server 6 &&\n+\n+\tgit init client &&\n+\ttest_must_fail git -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319472","messageId":"20170511213517.GA21723@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170511211403.5252-1-jonathantanmy@google.com","subject":"Re: [PATCH v4] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-11T21:35:17Z","receivedAt":"2017-05-11T21:35:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> Thanks, peff. I've incorporated your suggestions - I don't feel very\n> strongly about this, but I guess it's worthwhile to avoid the quadratic\n> behavior if we can.\n>\n> Also incorporated Jonathan Nieder's suggestion about the placement of\n> the last line. The relevant function is also factored out (following\n> peff's suggestion).\n\nThanks. The structure still seems more complicated than it needs to\nbe. More details below.\n\n[...]\n> +++ b/fetch-pack.c\n[...]\n> @@ -592,13 +593,22 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n> \t}\n> }\n> \n> +static void add_refs_to_oidset(struct oidset *oids, const struct ref *refs)\n> +{\n> +\tfor (; refs; refs = refs->next)\n> +\t\toidset_insert(oids, &refs->old_oid);\n\nMakes sense.\n\n[...]\n> \t/* Append unmatched requests to the list */\n> \tfor (i = 0; i < nr_sought; i++) {\n> \t\tunsigned char sha1[20];\n> +\t\tint can_append = 0;\n> \n> \t\tref = sought[i];\n> \t\tif (ref->match_status != REF_NOT_MATCHED)\n> @@ -649,6 +661,21 @@ static void filter_refs(struct fetch_pack_args *args,\n> \n> \t\tif ((allow_unadvertised_object_request &\n> \t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n> +\t\t\tcan_append = 1;\n> +\t\t} else {\n> +\t\t\tif (!tip_oids_initialized) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Check all refs, including those already\n> +\t\t\t\t * matched\n> +\t\t\t\t */\n> +\t\t\t\tadd_refs_to_oidset(&tip_oids, unmatched);\n> +\t\t\t\tadd_refs_to_oidset(&tip_oids, newlist);\n> +\t\t\t\ttip_oids_initialized = 1;\n> +\t\t\t}\n> +\t\t\tcan_append = oidset_contains(&tip_oids, &ref->old_oid);\n> +\t\t}\n> +\n> +\t\tif (can_append) {\n\nThis structure could be simplified by putting the lazy-initializing\ntip_oids lookup in a separate function. For example:\n\n\tint tip_oids_contain(struct oidset *tip_oids,\n\t\tstruct ref *unmatched, struct ref *newlist,\n\t\tconst struct oid *id)\n\t{\n\t\tif (oidset_is_empty(tip_oids)) {\n\t\t\tadd_refs_to_oidset(tip_oids, unmatched);\n\t\t\tadd_refs_to_oidset(tip_oids, newlist);\n\t\t}\n\t\treturn oidset_contains(tip_oids, id);\n\t}\n\nThat way, the caller could be kept simple (eliminating can_append\nand the repeated if).\n\nThanks,\nJonathan\n"},{"id":"319475","messageId":"20170511215948.yhxirikfqujzzgqk@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170511213517.GA21723@aiede.svl.corp.google.com","subject":"Re: [PATCH v4] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-11T21:59:49Z","receivedAt":"2017-05-11T21:59:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 02:35:17PM -0700, Jonathan Nieder wrote:\n\n> This structure could be simplified by putting the lazy-initializing\n> tip_oids lookup in a separate function. For example:\n> \n> \tint tip_oids_contain(struct oidset *tip_oids,\n> \t\tstruct ref *unmatched, struct ref *newlist,\n> \t\tconst struct oid *id)\n> \t{\n> \t\tif (oidset_is_empty(tip_oids)) {\n> \t\t\tadd_refs_to_oidset(tip_oids, unmatched);\n> \t\t\tadd_refs_to_oidset(tip_oids, newlist);\n> \t\t}\n> \t\treturn oidset_contains(tip_oids, id);\n> \t}\n\nYeah, I started to write it that way, but in the empty-ref case it will\ntry to add the empty refs over and over. I guess that's not that big a\ndeal, though, as we know the noop add is O(1) then. :)\n\n> That way, the caller could be kept simple (eliminating can_append\n> and the repeated if).\n\nYes, shoving it all into the sub-function is a big win for readability.\n\n-Peff\n"},{"id":"319484","messageId":"20170511223054.25239-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-11T22:30:54Z","receivedAt":"2017-05-11T22:31:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n\nChange from v4: incorporated Jonathan Nieder's suggestion about using\nanother function. There is no oidset_is_empty, so I checked for a\npresence of a member variable instead.\n---\n fetch-pack.c | 34 ++++++++++++++++++++++++++++++++--\n t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..9dd430a65 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,13 +593,32 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n+{\n+\tfor (; refs; refs = refs->next)\n+\t\toidset_insert(oids, &refs->old_oid);\n+}\n+\n+static int tip_oids_contain(struct oidset *tip_oids,\n+\t\t\t struct ref *unmatched, struct ref *newlist,\n+\t\t\t const struct object_id *id)\n+{\n+\tif (!tip_oids->map.cmpfn) {\n+\t\tadd_refs_to_oidset(tip_oids, unmatched);\n+\t\tadd_refs_to_oidset(tip_oids, newlist);\n+\t}\n+\treturn oidset_contains(tip_oids, id);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n \tint i;\n \n \ti = 0;\n@@ -631,7 +651,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n@@ -648,7 +669,9 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tcontinue;\n \n \t\tif ((allow_unadvertised_object_request &\n-\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||\n+\t\t tip_oids_contain(&tip_oids, unmatched, newlist,\n+\t\t\t\t &ref->old_oid)) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -656,6 +679,13 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n \t\t}\n \t}\n+\n+\toidset_clear(&tip_oids);\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n+\n \t*refs = newlist;\n }\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..80a1a3239 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master)\n+'\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_commit -C server 2 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+\trm -rf server &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 5 &&\n+\tgit -C server tag -d 5 &&\n+\ttest_commit -C server 6 &&\n+\n+\tgit init client &&\n+\ttest_must_fail git -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319485","messageId":"20170511224639.GC21723@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170511223054.25239-1-jonathantanmy@google.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-11T22:46:39Z","receivedAt":"2017-05-11T22:46:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -15,6 +15,7 @@\n> #include \"version.h\"\n> #include \"prio-queue.h\"\n> #include \"sha1-array.h\"\n> +#include \"oidset.h\"\n> \n> static int transfer_unpack_limit = -1;\n> static int fetch_unpack_limit = -1;\n> @@ -592,13 +593,32 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n> \t}\n> }\n> \n> +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n> +{\n> +\tfor (; refs; refs = refs->next)\n> +\t\toidset_insert(oids, &refs->old_oid);\n> +}\n> +\n> +static int tip_oids_contain(struct oidset *tip_oids,\n> +\t\t\t struct ref *unmatched, struct ref *newlist,\n> +\t\t\t const struct object_id *id)\n> +{\n> +\tif (!tip_oids->map.cmpfn) {\n\nThis feels like a layering violation. Could it be e.g. a static inline\nfunction oidset_is_initialized in oidset.h?\n\n> +\t\tadd_refs_to_oidset(tip_oids, unmatched);\n> +\t\tadd_refs_to_oidset(tip_oids, newlist);\n> +\t}\n> +\treturn oidset_contains(tip_oids, id);\n> +}\n\nThe rest looks good.\n\nWith or without that change,\nReviewed-by: Jonathan Nieder \n\nThanks for your patient work.\n\ndiff --git i/fetch-pack.c w/fetch-pack.c\nindex 9dd430a65a..0394580434 100644\n--- i/fetch-pack.c\n+++ w/fetch-pack.c\n@@ -603,7 +603,7 @@ static int tip_oids_contain(struct oidset *tip_oids,\n \t\t\t struct ref *unmatched, struct ref *newlist,\n \t\t\t const struct object_id *id)\n {\n-\tif (!tip_oids->map.cmpfn) {\n+\tif (!oidset_initialized(tip_oids)) {\n \t\tadd_refs_to_oidset(tip_oids, unmatched);\n \t\tadd_refs_to_oidset(tip_oids, newlist);\n \t}\ndiff --git i/oidset.c w/oidset.c\nindex ac169f05d3..f2a6753b8a 100644\n--- i/oidset.c\n+++ w/oidset.c\n@@ -18,7 +18,7 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid)\n {\n \tstruct hashmap_entry key;\n \n-\tif (!set->map.cmpfn)\n+\tif (!oidset_initialized(set))\n \t\treturn 0;\n \n \thashmap_entry_init(&key, sha1hash(oid->hash));\n@@ -29,7 +29,7 @@ int oidset_insert(struct oidset *set, const struct object_id *oid)\n {\n \tstruct oidset_entry *entry;\n \n-\tif (!set->map.cmpfn)\n+\tif (!oidset_initialized(set))\n \t\thashmap_init(&set->map, oidset_hashcmp, 0);\n \n \tif (oidset_contains(set, oid))\ndiff --git i/oidset.h w/oidset.h\nindex b7eaab5b88..2e7d889770 100644\n--- i/oidset.h\n+++ w/oidset.h\n@@ -22,6 +22,16 @@ struct oidset {\n \n #define OIDSET_INIT { { NULL } }\n \n+/**\n+ * Returns true iff \"set\" has been initialized (for example by inserting\n+ * an entry). An oidset is considered uninitialized if it hasn't had any\n+ * oids inserted since it was last cleared.\n+ */\n+static inline int oidset_initialized(const struct oidset *set)\n+{\n+\treturn set->map.cmpfn ? 1 : 0;\n+}\n+\n /**\n * Returns true iff `set` contains `oid`.\n */\n"},{"id":"319509","messageId":"CAGf8dgLcCeMYGPF1PSPy5M5zLvj2hb_EfpDBPbcNe+96c9YpRA@mail.gmail.com","threadId":"45912","inReplyTo":"20170512081417.w537fmd4o5rl4kja@sigill.intra.peff.net","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-12T18:00:36Z","receivedAt":"2017-05-12T18:44:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Fri, May 12, 2017 at 1:14 AM, Jeff King wrote:\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index afb8b0502..e167213c0 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -15,6 +15,7 @@\n> #include \"version.h\"\n> #include \"prio-queue.h\"\n> #include \"sha1-array.h\"\n> +#include \"oidset.h\"\n>\n> static int transfer_unpack_limit = -1;\n> static int fetch_unpack_limit = -1;\n> @@ -592,13 +593,27 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n> }\n> }\n>\n> +static int is_literal_sha1(const struct ref *ref)\n> +{\n> + struct object_id oid;\n> + const char *end;\n> + return !parse_oid_hex(ref->name, &oid, &end) &&\n> + !*end &&\n> + !oidcmp(&oid, &ref->old_oid);\n> +}\n> +\n> static void filter_refs(struct fetch_pack_args *args,\n> struct ref **refs,\n> struct ref **sought, int nr_sought)\n> {\n> struct ref *newlist = NULL;\n> struct ref **newtail = &newlist;\n> + struct ref *unmatched = NULL;\n> struct ref *ref, *next;\n> + struct oidset tip_oids = OIDSET_INIT;\n> + int send_raw_oids = (allow_unadvertised_object_request &\n> + (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n> + int seeking_raw_oid = 0;\n> int i;\n>\n> i = 0;\n> @@ -617,7 +632,8 @@ static void filter_refs(struct fetch_pack_args *args,\n> else if (cmp == 0) {\n> keep = 1; /* definitely have it */\n> sought[i]->match_status = REF_MATCHED;\n> - }\n> + } else if (is_literal_sha1(sought[i]))\n> + seeking_raw_oid = 1;\n\nAs far as I can tell, this seems to coincidentally work because SHA-1s\nas strings compare less than any ref name (HEAD or refs/...) - if this\nweren't the case, the \"break\" statement above might cause this line to\nnever be executed. I'm not sure if we want to rely on that.\n\n> i++;\n> }\n> }\n> @@ -631,24 +647,27 @@ static void filter_refs(struct fetch_pack_args *args,\n> ref->next = NULL;\n> newtail = &ref->next;\n> } else {\n> - free(ref);\n> + ref->next = unmatched;\n> + unmatched = ref;\n> }\n> }\n>\n> + if (seeking_raw_oid && !send_raw_oids) {\n> + for (ref = newlist; ref; ref = ref->next)\n> + oidset_insert(&tip_oids, &ref->old_oid);\n> + for (ref = unmatched; ref; ref = ref->next)\n> + oidset_insert(&tip_oids, &ref->old_oid);\n> + }\n> +\n> /* Append unmatched requests to the list */\n> for (i = 0; i < nr_sought; i++) {\n> - unsigned char sha1[20];\n> -\n> ref = sought[i];\n> if (ref->match_status != REF_NOT_MATCHED)\n> continue;\n> - if (get_sha1_hex(ref->name, sha1) ||\n> - ref->name[40] != '\\0' ||\n> - hashcmp(sha1, ref->old_oid.hash))\n> + if (!is_literal_sha1(ref))\n> continue;\n>\n> - if ((allow_unadvertised_object_request &\n> - (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n> + if (send_raw_oids || oidset_contains(&tip_oids, &ref->old_oid)) {\n> ref->match_status = REF_MATCHED;\n> *newtail = copy_ref(ref);\n> newtail = &(*newtail)->next;\n> @@ -656,6 +675,13 @@ static void filter_refs(struct fetch_pack_args *args,\n> ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n> }\n> }\n> +\n> + oidset_clear(&tip_oids);\n> + for (ref = unmatched; ref; ref = next) {\n> + next = ref->next;\n> + free(ref);\n> + }\n> +\n> *refs = newlist;\n> }\n>\n"},{"id":"319521","messageId":"20170512075931.umunxd72nj53snds@sigill.intra.peff.net","threadId":"45912","inReplyTo":"xmqq37ca7gw0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T07:59:31Z","receivedAt":"2017-05-12T18:45:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2017 at 03:01:35PM +0900, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> >> +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n> >> +{\n> >> +\tfor (; refs; refs = refs->next)\n> >> +\t\toidset_insert(oids, &refs->old_oid);\n> >> +}\n> >> +\n> >> +static int tip_oids_contain(struct oidset *tip_oids,\n> >> +\t\t\t struct ref *unmatched, struct ref *newlist,\n> >> +\t\t\t const struct object_id *id)\n> >> +{\n> >> +\tif (!tip_oids->map.cmpfn) {\n> >\n> > This feels like a layering violation. Could it be e.g. a static inline\n> > function oidset_is_initialized in oidset.h?\n> \n> Surely it does.\n> \n> Also, tip_oids_contain() uses unmatched and newlist only on the\n> first call, but the internal API this patch establishes gives an\n> illusion (confusion) that updating unmatched and newlist while\n> making repeated to calls to this function may affect the outcome of\n> tip_oids_contain() function. In fact, doesn't the loop that calls\n> this function extend \"newlist\" by extending the list at its tail?\n\nIt does, but only with elements whose oids were already in the set. So I\ndon't think it's wrong, but I agree the interface makes it a bit muddy.\n\nThe whole thing is much easier to follow if you just create the oidset\nbefore chopping it up into two lists. But that loses the \"lazy\"\nproperty, and you pay for the oidset even if there are no raw sha1s\nrequested. But it is nice and short. See the patch below (as a\nreplacement for what we've been discussing, not on top).\n\nIt's hard to resolve that because the loop that chops up the lists is\nalso the loop that is figuring out if there are leftover raw-sha1\nnames. But you could probably structure it like:\n\n 1. Loop and see if we have unmatched names that look like sha1s.\n\n 2. Set up the oidset from the two chopped-up lists if there are\n sha1 candidates (and if we aren't going to just send them\n regardless due to server options).\n\n 3. Loop over the unmatched candidates and check them against the\n oidset.\n\nThat avoids the lazy-init, so pulling (2) out into a function results in\na better interface.\n\n---\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..4f3e8fa6b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -599,6 +600,9 @@ static void filter_refs(struct fetch_pack_args *args,\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n+\tint send_raw_oids = (allow_unadvertised_object_request &\n+\t\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n \tint i;\n \n \ti = 0;\n@@ -626,6 +630,9 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n \t\t\tkeep = 1;\n \n+\t\tif (!send_raw_oids)\n+\t\t\toidset_insert(&tip_oids, &ref->old_oid);\n+\n \t\tif (keep) {\n \t\t\t*newtail = ref;\n \t\t\tref->next = NULL;\n@@ -647,8 +654,7 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t hashcmp(sha1, ref->old_oid.hash))\n \t\t\tcontinue;\n \n-\t\tif ((allow_unadvertised_object_request &\n-\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\tif (send_raw_oids || oidset_contains(&tip_oids, &ref->old_oid)) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -657,6 +663,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t}\n \t}\n \t*refs = newlist;\n+\n+\toidset_clear(&tip_oids);\n }\n \n static void mark_alternate_complete(struct object *obj)\n"},{"id":"319523","messageId":"20170512025912.smxc47pz64jzeclw@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170511224639.GC21723@aiede.svl.corp.google.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T02:59:12Z","receivedAt":"2017-05-12T18:45:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 03:46:39PM -0700, Jonathan Nieder wrote:\n\n> > +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n> > +{\n> > +\tfor (; refs; refs = refs->next)\n> > +\t\toidset_insert(oids, &refs->old_oid);\n> > +}\n> > +\n> > +static int tip_oids_contain(struct oidset *tip_oids,\n> > +\t\t\t struct ref *unmatched, struct ref *newlist,\n> > +\t\t\t const struct object_id *id)\n> > +{\n> > +\tif (!tip_oids->map.cmpfn) {\n> \n> This feels like a layering violation. Could it be e.g. a static inline\n> function oidset_is_initialized in oidset.h?\n\nIt definitely is a layering violation, though we normally are pretty\n\"open\" with letting callers peek in at our data structures.\n\nI'd be fine with it as-is or with the helper function you suggested.\nBut...\n\n> +/**\n> + * Returns true iff \"set\" has been initialized (for example by inserting\n> + * an entry). An oidset is considered uninitialized if it hasn't had any\n> + * oids inserted since it was last cleared.\n> + */\n> +static inline int oidset_initialized(const struct oidset *set)\n> +{\n> +\treturn set->map.cmpfn ? 1 : 0;\n> +}\n\nNow we're committing our own layering violation. I was surprised to find\nthat hashmap_free() clears \"cmpfn\", and I'm not sure if we would want to\ncount on that (not that it probably matters all that much, but it is\nrequired for the description you gave to be accurate).\n\nThe hashmap documentation suggests using \"tablesize\" for checking\nwhether something is initialized. Maybe we ought to use that.\n\n-Peff\n"},{"id":"319524","messageId":"xmqq37ca7gw0.fsf@gitster.mtv.corp.google.com","threadId":"45912","inReplyTo":"20170511224639.GC21723@aiede.svl.corp.google.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-12T06:01:35Z","receivedAt":"2017-05-12T18:45:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n>> +{\n>> +\tfor (; refs; refs = refs->next)\n>> +\t\toidset_insert(oids, &refs->old_oid);\n>> +}\n>> +\n>> +static int tip_oids_contain(struct oidset *tip_oids,\n>> +\t\t\t struct ref *unmatched, struct ref *newlist,\n>> +\t\t\t const struct object_id *id)\n>> +{\n>> +\tif (!tip_oids->map.cmpfn) {\n>\n> This feels like a layering violation. Could it be e.g. a static inline\n> function oidset_is_initialized in oidset.h?\n\nSurely it does.\n\nAlso, tip_oids_contain() uses unmatched and newlist only on the\nfirst call, but the internal API this patch establishes gives an\nillusion (confusion) that updating unmatched and newlist while\nmaking repeated to calls to this function may affect the outcome of\ntip_oids_contain() function. In fact, doesn't the loop that calls\nthis function extend \"newlist\" by extending the list at its tail?\n\n"},{"id":"319530","messageId":"CAGf8dgJxz+jA=pA5TRR6vmK=zP=CUenpsBDy2VEtjYB5CO9yuQ@mail.gmail.com","threadId":"45912","inReplyTo":"20170512075931.umunxd72nj53snds@sigill.intra.peff.net","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-12T18:09:42Z","receivedAt":"2017-05-12T18:45:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Fri, May 12, 2017 at 12:59 AM, Jeff King wrote:\n> On Fri, May 12, 2017 at 03:01:35PM +0900, Junio C Hamano wrote:\n>> Also, tip_oids_contain() uses unmatched and newlist only on the\n>> first call, but the internal API this patch establishes gives an\n>> illusion (confusion) that updating unmatched and newlist while\n>> making repeated to calls to this function may affect the outcome of\n>> tip_oids_contain() function. In fact, doesn't the loop that calls\n>> this function extend \"newlist\" by extending the list at its tail?\n>\n> It does, but only with elements whose oids were already in the set. So I\n> don't think it's wrong, but I agree the interface makes it a bit muddy.\n\nTo make the interface less muddy, would you agree with this (untested):\n\n@@ -648,7 +669,9 @@ static void filter_refs(struct fetch_pack_args *args,\n continue;\n\n if ((allow_unadvertised_object_request &\n- (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||\n+ (check_tip_oids_initialized(&tip_oids, unmatched, newlist) &&\noidset_contains(&tip_oids,\n+ &ref->old_oid))) {\n ref->match_status = REF_MATCHED;\n *newtail = copy_ref(ref);\n newtail = &(*newtail)->next;\n\n(making the function-to-abstract be merely an initialization one,\ninstead of one that does 2 things). That decreases the scope of the\nfunction that Jonathan Nieder and Peff wanted, but it might be a\nwarranted reduction in scope.\n"},{"id":"319536","messageId":"20170512081417.w537fmd4o5rl4kja@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170512075931.umunxd72nj53snds@sigill.intra.peff.net","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T08:14:17Z","receivedAt":"2017-05-12T18:45:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2017 at 03:59:31AM -0400, Jeff King wrote:\n\n> It's hard to resolve that because the loop that chops up the lists is\n> also the loop that is figuring out if there are leftover raw-sha1\n> names. But you could probably structure it like:\n> \n> 1. Loop and see if we have unmatched names that look like sha1s.\n> \n> 2. Set up the oidset from the two chopped-up lists if there are\n> sha1 candidates (and if we aren't going to just send them\n> regardless due to server options).\n> \n> 3. Loop over the unmatched candidates and check them against the\n> oidset.\n> \n> That avoids the lazy-init, so pulling (2) out into a function results in\n> a better interface.\n\nHere that is for reference. It's a bit more complex than the other\nsolutions, requiring the unmatched list, the is_literal_sha1() helper,\nand the oidset. But it avoids any extra allocation when it isn't\nnecessary, and drops the laziness.\n\nI mostly did this to give the discussion something concrete to look at.\nI'm actually OK with any of the options so far, as long as it does not\nhave any quadratic behavior. :)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..e167213c0 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,13 +593,27 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static int is_literal_sha1(const struct ref *ref)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\treturn !parse_oid_hex(ref->name, &oid, &end) &&\n+\t !*end &&\n+\t !oidcmp(&oid, &ref->old_oid);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n+\tint send_raw_oids = (allow_unadvertised_object_request &\n+\t\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));\n+\tint seeking_raw_oid = 0;\n \tint i;\n \n \ti = 0;\n@@ -617,7 +632,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\t\telse if (cmp == 0) {\n \t\t\t\t\tkeep = 1; /* definitely have it */\n \t\t\t\t\tsought[i]->match_status = REF_MATCHED;\n-\t\t\t\t}\n+\t\t\t\t} else if (is_literal_sha1(sought[i]))\n+\t\t\t\t\tseeking_raw_oid = 1;\n \t\t\t\ti++;\n \t\t\t}\n \t\t}\n@@ -631,24 +647,27 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n+\tif (seeking_raw_oid && !send_raw_oids) {\n+\t\tfor (ref = newlist; ref; ref = ref->next)\n+\t\t\toidset_insert(&tip_oids, &ref->old_oid);\n+\t\tfor (ref = unmatched; ref; ref = ref->next)\n+\t\t\toidset_insert(&tip_oids, &ref->old_oid);\n+\t}\n+\n \t/* Append unmatched requests to the list */\n \tfor (i = 0; i < nr_sought; i++) {\n-\t\tunsigned char sha1[20];\n-\n \t\tref = sought[i];\n \t\tif (ref->match_status != REF_NOT_MATCHED)\n \t\t\tcontinue;\n-\t\tif (get_sha1_hex(ref->name, sha1) ||\n-\t\t ref->name[40] != '\\0' ||\n-\t\t hashcmp(sha1, ref->old_oid.hash))\n+\t\tif (!is_literal_sha1(ref))\n \t\t\tcontinue;\n \n-\t\tif ((allow_unadvertised_object_request &\n-\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\tif (send_raw_oids || oidset_contains(&tip_oids, &ref->old_oid)) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -656,6 +675,13 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n \t\t}\n \t}\n+\n+\toidset_clear(&tip_oids);\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n+\n \t*refs = newlist;\n }\n \n"},{"id":"319540","messageId":"20170512030608.s4jw2g4ijivp3f52@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170511223054.25239-1-jonathantanmy@google.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-12T03:06:08Z","receivedAt":"2017-05-12T18:45:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 03:30:54PM -0700, Jonathan Tan wrote:\n\n> fetch-pack, when fetching a literal SHA-1 from a server that is not\n> configured with uploadpack.allowtipsha1inwant (or similar), always\n> returns an error message of the form \"Server does not allow request for\n> unadvertised object %s\". However, it is sometimes the case that such\n> object is advertised. This situation would occur, for example, if a user\n> or a script was provided a SHA-1 instead of a branch or tag name for\n> fetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\n> that SHA-1.\n> \n> Teach fetch-pack to also check the SHA-1s of the refs in the received\n> ref advertisement if a literal SHA-1 was given by the user.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Jonathan Tan \n> ---\n\nThis looks good to me. There's one minor nit that I don't think I saw\nmentioned, and I don't think needs to hold up the patch. But I wanted to\nmention it just in case I'm wrong that it doesn't matter.\n\n> static void filter_refs(struct fetch_pack_args *args,\n> \t\t\tstruct ref **refs,\n> \t\t\tstruct ref **sought, int nr_sought)\n> {\n> \tstruct ref *newlist = NULL;\n> \tstruct ref **newtail = &newlist;\n> +\tstruct ref *unmatched = NULL;\n> \tstruct ref *ref, *next;\n> +\tstruct oidset tip_oids = OIDSET_INIT;\n> \tint i;\n> \n> \ti = 0;\n> @@ -631,7 +651,8 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\t\tref->next = NULL;\n> \t\t\tnewtail = &ref->next;\n> \t\t} else {\n> -\t\t\tfree(ref);\n> +\t\t\tref->next = unmatched;\n> +\t\t\tunmatched = ref;\n> \t\t}\n\nThe incoming \"refs\" list is sorted, and we rely on that sorting to do\nthe linear walk. Likewise, we append to newlist via newtail, so it\nremains sorted (and I suspect the consumers of the list rely on that).\nBut your new \"unmatched\" list is done by prepending, so it's in reverse\norder.\n\nI don't think that matters for our purposes here, and the list doesn't\nescape our function. So there's no bug, but I just wonder if it might\nend up biting somebody in the future. I'm OK with leaving it, though.\n\n-Peff\n"},{"id":"319560","messageId":"20170512190617.GC27400@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"CAGf8dgJxz+jA=pA5TRR6vmK=zP=CUenpsBDy2VEtjYB5CO9yuQ@mail.gmail.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-12T19:06:17Z","receivedAt":"2017-05-12T19:06:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> To make the interface less muddy, would you agree with this (untested):\n>\n> @@ -648,7 +669,9 @@ static void filter_refs(struct fetch_pack_args *args,\n> continue;\n> \n> if ((allow_unadvertised_object_request &\n> - (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n> + (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||\n> + (check_tip_oids_initialized(&tip_oids, unmatched, newlist) &&\n> oidset_contains(&tip_oids,\n> + &ref->old_oid))) {\n> ref->match_status = REF_MATCHED;\n> *newtail = copy_ref(ref);\n> newtail = &(*newtail)->next;\n>\n> (making the function-to-abstract be merely an initialization one,\n> instead of one that does 2 things). That decreases the scope of the\n> function that Jonathan Nieder and Peff wanted, but it might be a\n> warranted reduction in scope.\n\nYeah, that sounds nicer than anything I suggested.\n\nnit that check_ would be clearer as ensure_\n\nThanks,\nJonathan\n"},{"id":"319563","messageId":"20170512204550.10373-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-12T20:45:50Z","receivedAt":"2017-05-12T20:46:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n\nChange from v5: used \"ensure_tip_oids_initialized\" function instead.\nThis removes some of the muddiness (e.g. with newlist being modified\nafter the function).\n\nI used an \"int\" return type for ensure_tip_oids_initialized so that I\ncould chain with \"&&\", but \"void\" and chaining with \",\" is also possible\n(but I don't see much use of the comma operator in Git code).\n---\n fetch-pack.c | 35 +++++++++++++++++++++++++++++++++--\n t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..f05f48d7b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,13 +593,32 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n+{\n+\tfor (; refs; refs = refs->next)\n+\t\toidset_insert(oids, &refs->old_oid);\n+}\n+\n+static int ensure_tip_oids_initialized(struct oidset *tip_oids,\n+\t\t\t\t struct ref *unmatched,\n+\t\t\t\t struct ref *newlist)\n+{\n+\tif (!tip_oids->map.tablesize) {\n+\t\tadd_refs_to_oidset(tip_oids, unmatched);\n+\t\tadd_refs_to_oidset(tip_oids, newlist);\n+\t}\n+\treturn 1;\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n \tint i;\n \n \ti = 0;\n@@ -631,7 +651,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n@@ -648,7 +669,10 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tcontinue;\n \n \t\tif ((allow_unadvertised_object_request &\n-\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||\n+\t\t (ensure_tip_oids_initialized(&tip_oids, unmatched,\n+\t\t\t\t\t\t newlist) &&\n+\t\t oidset_contains(&tip_oids, &ref->old_oid))) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -656,6 +680,13 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n \t\t}\n \t}\n+\n+\toidset_clear(&tip_oids);\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n+\n \t*refs = newlist;\n }\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..80a1a3239 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master)\n+'\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_commit -C server 2 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+\trm -rf server &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 5 &&\n+\tgit -C server tag -d 5 &&\n+\ttest_commit -C server 6 &&\n+\n+\tgit init client &&\n+\ttest_must_fail git -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319564","messageId":"20170512204648.10611-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v6] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-12T20:46:48Z","receivedAt":"2017-05-12T20:46:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n\nChange from v5: used \"ensure_tip_oids_initialized\" function instead.\nThis removes some of the muddiness (e.g. with newlist being modified\nafter the function).\n\nI used an \"int\" return type for ensure_tip_oids_initialized so that I\ncould chain with \"&&\", but \"void\" and chaining with \",\" is also possible\n(but I don't see much use of the comma operator in Git code).\n---\n fetch-pack.c | 35 +++++++++++++++++++++++++++++++++--\n t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..f05f48d7b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,13 +593,32 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n+{\n+\tfor (; refs; refs = refs->next)\n+\t\toidset_insert(oids, &refs->old_oid);\n+}\n+\n+static int ensure_tip_oids_initialized(struct oidset *tip_oids,\n+\t\t\t\t struct ref *unmatched,\n+\t\t\t\t struct ref *newlist)\n+{\n+\tif (!tip_oids->map.tablesize) {\n+\t\tadd_refs_to_oidset(tip_oids, unmatched);\n+\t\tadd_refs_to_oidset(tip_oids, newlist);\n+\t}\n+\treturn 1;\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n \tint i;\n \n \ti = 0;\n@@ -631,7 +651,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n@@ -648,7 +669,10 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tcontinue;\n \n \t\tif ((allow_unadvertised_object_request &\n-\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||\n+\t\t (ensure_tip_oids_initialized(&tip_oids, unmatched,\n+\t\t\t\t\t\t newlist) &&\n+\t\t oidset_contains(&tip_oids, &ref->old_oid))) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -656,6 +680,13 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n \t\t}\n \t}\n+\n+\toidset_clear(&tip_oids);\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n+\n \t*refs = newlist;\n }\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..80a1a3239 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master)\n+'\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_commit -C server 2 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+\trm -rf server &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 5 &&\n+\tgit -C server tag -d 5 &&\n+\ttest_commit -C server 6 &&\n+\n+\tgit init client &&\n+\ttest_must_fail git -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319575","messageId":"20170512222839.GD27400@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170512204648.10611-1-jonathantanmy@google.com","subject":"Re: [PATCH v6] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-12T22:28:39Z","receivedAt":"2017-05-12T22:28:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Helped-by: Jeff King \n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 35 +++++++++++++++++++++++++++++++++--\n> t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 68 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks for your patience.\n"},{"id":"319600","messageId":"20170513083012.lcs47tz3xb7avl45@sigill.intra.peff.net","threadId":"45912","inReplyTo":"CAGf8dgLcCeMYGPF1PSPy5M5zLvj2hb_EfpDBPbcNe+96c9YpRA@mail.gmail.com","subject":"Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-13T08:30:12Z","receivedAt":"2017-05-13T08:30:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2017 at 11:00:36AM -0700, Jonathan Tan wrote:\n\n> > @@ -617,7 +632,8 @@ static void filter_refs(struct fetch_pack_args *args,\n> > else if (cmp == 0) {\n> > keep = 1; /* definitely have it */\n> > sought[i]->match_status = REF_MATCHED;\n> > - }\n> > + } else if (is_literal_sha1(sought[i]))\n> > + seeking_raw_oid = 1;\n> \n> As far as I can tell, this seems to coincidentally work because SHA-1s\n> as strings compare less than any ref name (HEAD or refs/...) - if this\n> weren't the case, the \"break\" statement above might cause this line to\n> never be executed. I'm not sure if we want to rely on that.\n\nRight, I was thinking we'd hit all of the sought refs in this loop, but\nwe don't necessarily (if we run out of remote refs to compare to).\nFinishing it off with:\n\n /* scan the remainder of the requests */\n for (; i < nr_sought; i++)\n\tif (is_literal_sha1(...))\n\nthough it would probably be less confusing to just do a separate loop\nafter the original is done.\n\nAnyway, it looks like your v6 just keeps the lazy approach, so this is\nall moot.\n\n-Peff\n"},{"id":"319601","messageId":"20170513083615.dyfnmug35ghhy7ba@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170512204648.10611-1-jonathantanmy@google.com","subject":"Re: [PATCH v6] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-13T08:36:15Z","receivedAt":"2017-05-13T08:36:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2017 at 01:46:48PM -0700, Jonathan Tan wrote:\n\n> Change from v5: used \"ensure_tip_oids_initialized\" function instead.\n> This removes some of the muddiness (e.g. with newlist being modified\n> after the function).\n\nI don't think it really improves the muddiness. You are still calling\nthe ensure function each time through the loop with a potentially\nmodified list, but it doesn't actually look at the list after the first\ntime. So the muddy part is still there.\n\nI think rather than changing the code, you'd do better with a comment\nlike:\n\n /*\n * Note that this only looks at the ref lists the first time\n * it's called. This works out in filter_refs() because even\n * though it may add to \"newlist\" between calls, the additions\n * will always be for oids that are already in the set.\n */\n\nAt which point the original all-in-one function is probably fine (as it\navoids the \"return 1 just to join the &&-chain\" bit).\n\n-Peff\n"},{"id":"319608","messageId":"20170513092952.53h2l4nb727y5f7g@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170511170050.GA83655@google.com","subject":"Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-13T09:29:52Z","receivedAt":"2017-05-13T09:30:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2017 at 10:00:50AM -0700, Brandon Williams wrote:\n\n> > None of this is your problem now either way; the advertisement-limiting\n> > extension is still vaporware, albeit one we've discussed a lot. I just\n> > wanted to make sure we weren't painting ourselves into any corners. And\n> > I think this case could probably be handled.\n> \n> I can't remember, is there any reason why it is still vaporware? As in\n> what is holding us back from doing the advertisement-limiting (or doing\n> away with the initial ref advertisement)?\n\nThe tricky part is handling the compatibility issues. The client has no\nway to speak first in the current protocol, so we have to break protocol\nto tell the server our refspecs before the advertisement.\n\nI actually put together patches last fall for this, got derailed by a\nsmall snag while polishing them, and just haven't picked them up again.\nIf you're interested, they're at the jk/early-caps-wip branch of\nhttps://github.com/peff/git.\n\nI got as far as breaking it all up into patches, but the commit messages\nstill need to be written. Some of them are pretty obvious to me still\nafter 6 months, but a few are a bit inscrutable.\n\n-Peff\n"},{"id":"319735","messageId":"xmqqwp9iykp9.fsf@gitster.mtv.corp.google.com","threadId":"45912","inReplyTo":"20170513083615.dyfnmug35ghhy7ba@sigill.intra.peff.net","subject":"Re: [PATCH v6] fetch-pack: always allow fetching of literal SHA1s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T01:26:10Z","receivedAt":"2017-05-15T01:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 12, 2017 at 01:46:48PM -0700, Jonathan Tan wrote:\n>\n>> Change from v5: used \"ensure_tip_oids_initialized\" function instead.\n>> This removes some of the muddiness (e.g. with newlist being modified\n>> after the function).\n>\n> I don't think it really improves the muddiness. You are still calling\n> the ensure function each time through the loop with a potentially\n> modified list, but it doesn't actually look at the list after the first\n> time. So the muddy part is still there.\n>\n> I think rather than changing the code, you'd do better with a comment\n> like:\n>\n> /*\n> * Note that this only looks at the ref lists the first time\n> * it's called. This works out in filter_refs() because even\n> * though it may add to \"newlist\" between calls, the additions\n> * will always be for oids that are already in the set.\n> */\n>\n> At which point the original all-in-one function is probably fine (as it\n> avoids the \"return 1 just to join the &&-chain\" bit).\n\nYes. I agree that the in-code comment is the best approach to the\n\"muddy combination of 'grabbing it for the first time' and 'the\nthing that is grabbed mutates in the loop'\" confusion.\n\n"},{"id":"319802","messageId":"20170515173220.13103-1-jonathantanmy@google.com","threadId":"45912","inReplyTo":"20170509182042.28389-1-jonathantanmy@google.com","subject":"[PATCH v7] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-05-15T17:32:20Z","receivedAt":"2017-05-15T17:32:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-pack, when fetching a literal SHA-1 from a server that is not\nconfigured with uploadpack.allowtipsha1inwant (or similar), always\nreturns an error message of the form \"Server does not allow request for\nunadvertised object %s\". However, it is sometimes the case that such\nobject is advertised. This situation would occur, for example, if a user\nor a script was provided a SHA-1 instead of a branch or tag name for\nfetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\nthat SHA-1.\n\nTeach fetch-pack to also check the SHA-1s of the refs in the received\nref advertisement if a literal SHA-1 was given by the user.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n\nOK, one combined function (for lazy initialization and checking\ncontainment in the oidset) with comment it is.\n\nChange from v6: changed back to \"tip_oids_contain\", and included Peff's\ncomment.\n---\n fetch-pack.c | 40 ++++++++++++++++++++++++++++++++++++++--\n t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex afb8b0502..703e7ec78 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"oidset.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -592,13 +593,38 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,\n \t}\n }\n \n+static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)\n+{\n+\tfor (; refs; refs = refs->next)\n+\t\toidset_insert(oids, &refs->old_oid);\n+}\n+\n+static int tip_oids_contain(struct oidset *tip_oids,\n+\t\t\t struct ref *unmatched, struct ref *newlist,\n+\t\t\t const struct object_id *id)\n+{\n+\t/*\n+\t * Note that this only looks at the ref lists the first time it's\n+\t * called. This works out in filter_refs() because even though it may\n+\t * add to \"newlist\" between calls, the additions will always be for\n+\t * oids that are already in the set.\n+\t */\n+\tif (!tip_oids->map.tablesize) {\n+\t\tadd_refs_to_oidset(tip_oids, unmatched);\n+\t\tadd_refs_to_oidset(tip_oids, newlist);\n+\t}\n+\treturn oidset_contains(tip_oids, id);\n+}\n+\n static void filter_refs(struct fetch_pack_args *args,\n \t\t\tstruct ref **refs,\n \t\t\tstruct ref **sought, int nr_sought)\n {\n \tstruct ref *newlist = NULL;\n \tstruct ref **newtail = &newlist;\n+\tstruct ref *unmatched = NULL;\n \tstruct ref *ref, *next;\n+\tstruct oidset tip_oids = OIDSET_INIT;\n \tint i;\n \n \ti = 0;\n@@ -631,7 +657,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n-\t\t\tfree(ref);\n+\t\t\tref->next = unmatched;\n+\t\t\tunmatched = ref;\n \t\t}\n \t}\n \n@@ -648,7 +675,9 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tcontinue;\n \n \t\tif ((allow_unadvertised_object_request &\n-\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {\n+\t\t (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||\n+\t\t tip_oids_contain(&tip_oids, unmatched, newlist,\n+\t\t\t\t &ref->old_oid)) {\n \t\t\tref->match_status = REF_MATCHED;\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n@@ -656,6 +685,13 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tref->match_status = REF_UNADVERTISED_NOT_ALLOWED;\n \t\t}\n \t}\n+\n+\toidset_clear(&tip_oids);\n+\tfor (ref = unmatched; ref; ref = next) {\n+\t\tnext = ref->next;\n+\t\tfree(ref);\n+\t}\n+\n \t*refs = newlist;\n }\n \ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b5865b385..80a1a3239 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master)\n+'\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\ttest_commit -C server 2 &&\n+\n+\tgit init client &&\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+\trm -rf server &&\n+\n+\tgit init server &&\n+\ttest_commit -C server 5 &&\n+\tgit -C server tag -d 5 &&\n+\ttest_commit -C server 6 &&\n+\n+\tgit init client &&\n+\ttest_must_fail git -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319806","messageId":"20170515174638.GJ27400@aiede.svl.corp.google.com","threadId":"45912","inReplyTo":"20170515173220.13103-1-jonathantanmy@google.com","subject":"Re: [PATCH v7] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-15T17:46:38Z","receivedAt":"2017-05-15T17:46:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> fetch-pack, when fetching a literal SHA-1 from a server that is not\n> configured with uploadpack.allowtipsha1inwant (or similar), always\n> returns an error message of the form \"Server does not allow request for\n> unadvertised object %s\". However, it is sometimes the case that such\n> object is advertised. This situation would occur, for example, if a user\n> or a script was provided a SHA-1 instead of a branch or tag name for\n> fetching, and wanted to invoke \"git fetch\" or \"git fetch-pack\" using\n> that SHA-1.\n>\n> Teach fetch-pack to also check the SHA-1s of the refs in the received\n> ref advertisement if a literal SHA-1 was given by the user.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 40 ++++++++++++++++++++++++++++++++++++++--\n> t/t5500-fetch-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 73 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nI think we've done enough golf on this one.\n\nThanks for your patient work.\nJonathan\n"},{"id":"319841","messageId":"20170515221054.j3pmk6fnhnsa5oa6@sigill.intra.peff.net","threadId":"45912","inReplyTo":"20170515173220.13103-1-jonathantanmy@google.com","subject":"Re: [PATCH v7] fetch-pack: always allow fetching of literal SHA1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T22:10:54Z","receivedAt":"2017-05-15T22:11:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 10:32:20AM -0700, Jonathan Tan wrote:\n\n> OK, one combined function (for lazy initialization and checking\n> containment in the oidset) with comment it is.\n> \n> Change from v6: changed back to \"tip_oids_contain\", and included Peff's\n> comment.\n\nThanks, looks good to me.\n\n-Peff\n"}]}