{"thread":{"id":"41551","subject":"[PATCH] fetch-pack: fix unadvertised requests validation","startedAt":"2016-02-27T12:43:54Z","lastAt":"2016-03-05T19:35:42Z","messageCount":28,"participants":["Gabriel Souza Franco","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279639","messageId":"1456577034-6494-1-git-send-email-gabrielfrancosouza@gmail.com","threadId":"41551","inReplyTo":null,"subject":"[PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-02-27T12:43:54Z","receivedAt":"2016-02-27T12:43:54Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"Check was introduced in b791642 (filter_ref: avoid overwriting\nref->old_sha1 with garbage, 2015-03-19), but was always false because\nref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.\n\nSigned-off-by: Gabriel Souza Franco \n---\n fetch-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 01e34b6..83b937b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\tif (ref->matched)\n \t\t\t\tcontinue;\n \t\t\tif (get_sha1_hex(ref->name, sha1) ||\n-\t\t\t ref->name[40] != '\\0' ||\n-\t\t\t hashcmp(sha1, ref->old_oid.hash))\n+\t\t\t ref->name[40] != '\\0')\n \t\t\t\tcontinue;\n \n \t\t\tref->matched = 1;\n+\t\t\thashcpy(ref->old_oid.hash, sha1);\n \t\t\t*newtail = copy_ref(ref);\n \t\t\tnewtail = &(*newtail)->next;\n \t\t}\n-- \n2.7.1\n"},{"id":"279657","messageId":"xmqqd1riggd7.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"1456577034-6494-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-27T18:25:40Z","receivedAt":"2016-02-27T18:25:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gabriel Souza Franco writes:\n\n> Check was introduced in b791642 (filter_ref: avoid overwriting\n> ref->old_sha1 with garbage, 2015-03-19), but was always false because\n> ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.\n>\n> Signed-off-by: Gabriel Souza Franco \n> ---\n\nPeff, that commit points me at your direction. And I can see the\noriginal patch avoids overwriting old_sha1 by saving the result from\nget_sha1_hex() in a temporary, it is true that old_sha1 is not\nupdated from the temporary.\n\nThe original code before b791642 wanted to say \"if ref->name is not\n40-hex, continue, and otherwise, do the ref->matched thing\" and an\nimplementation of b791642 that is more faithful to the original\nwould indeed have been the result of applying this patch from\nGabriel, but I am scratching my head why we have hashcmp() there.\n\nWas it to avoid adding the same thing twice to the resulting list,\nor something?\n\n\n> fetch-pack.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 01e34b6..83b937b 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\t\tif (ref->matched)\n> \t\t\t\tcontinue;\n> \t\t\tif (get_sha1_hex(ref->name, sha1) ||\n> -\t\t\t ref->name[40] != '\\0' ||\n> -\t\t\t hashcmp(sha1, ref->old_oid.hash))\n> +\t\t\t ref->name[40] != '\\0')\n> \t\t\t\tcontinue;\n> \n> \t\t\tref->matched = 1;\n> +\t\t\thashcpy(ref->old_oid.hash, sha1);\n> \t\t\t*newtail = copy_ref(ref);\n> \t\t\tnewtail = &(*newtail)->next;\n> \t\t}\n"},{"id":"279661","messageId":"xmqq60xagg1w.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"xmqqd1riggd7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-27T18:32:27Z","receivedAt":"2016-02-27T18:32:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Gabriel Souza Franco writes:\n>\n>> Check was introduced in b791642 (filter_ref: avoid overwriting\n>> ref->old_sha1 with garbage, 2015-03-19), but was always false because\n>> ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.\n>>\n>> Signed-off-by: Gabriel Souza Franco \n>> ---\n>\n> Peff, that commit points me at your direction. And I can see the\n> original patch avoids overwriting old_sha1 by saving the result from\n> get_sha1_hex() in a temporary, it is true that old_sha1 is not\n> updated from the temporary.\n>\n> The original code before b791642 wanted to say \"if ref->name is not\n> 40-hex, continue, and otherwise, do the ref->matched thing\" and an\n> implementation of b791642 that is more faithful to the original\n> would indeed have been the result of applying this patch from\n> Gabriel, but I am scratching my head why we have hashcmp() there.\n>\n> Was it to avoid adding the same thing twice to the resulting list,\n> or something?\n\nNah, I think you just misspelt hashcpy() as hashcmp(). The original\nwanted to get the binary representation of the hex in old_sha1 when\nit continued, and you wanted to delay the touching of old_sha1 until\nwe make sure that the input is valid 40-hex, so something like this\nis what Gabriel wants to do (which I agree with), isn't it?\n\n fetch-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 058c258..bb5237f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -551,7 +551,7 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\t\tcontinue;\n \t\t\tif (get_sha1_hex(ref->name, sha1) ||\n \t\t\t ref->name[40] != '\\0' ||\n-\t\t\t hashcmp(sha1, ref->old_sha1))\n+\t\t\t hashcpy(ref->old_sha1, sha1))\n \t\t\t\tcontinue;\n \n \t\t\tref->matched = 1;\n"},{"id":"279662","messageId":"xmqq1t7ygfsa.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"xmqq60xagg1w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-27T18:38:13Z","receivedAt":"2016-02-27T18:38:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nah, I think you just misspelt hashcpy() as hashcmp(). The original\n> wanted to get the binary representation of the hex in old_sha1 when\n> it continued, and you wanted to delay the touching of old_sha1 until\n> we make sure that the input is valid 40-hex.\n\nThat much was correct, but...\n\n> , so something like this\n> is what Gabriel wants to do (which I agree with), isn't it?\n\n... definitely not that X-<. The \"do this or fail || do that or\nfail || ...\" chain in the if condition part confused me.\n\nI think Gabriel's patch is exactly what we want.\n\nSorry for starting a review before caffeine ;-)\n"},{"id":"279665","messageId":"20160227190712.GC12822@sigill.intra.peff.net","threadId":"41551","inReplyTo":"xmqqd1riggd7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T19:07:12Z","receivedAt":"2016-02-27T19:07:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 10:25:40AM -0800, Junio C Hamano wrote:\n\n> Gabriel Souza Franco writes:\n> \n> > Check was introduced in b791642 (filter_ref: avoid overwriting\n> > ref->old_sha1 with garbage, 2015-03-19), but was always false because\n> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.\n> >\n> > Signed-off-by: Gabriel Souza Franco \n> > ---\n> \n> Peff, that commit points me at your direction. And I can see the\n> original patch avoids overwriting old_sha1 by saving the result from\n> get_sha1_hex() in a temporary, it is true that old_sha1 is not\n> updated from the temporary.\n> \n> The original code before b791642 wanted to say \"if ref->name is not\n> 40-hex, continue, and otherwise, do the ref->matched thing\" and an\n> implementation of b791642 that is more faithful to the original\n> would indeed have been the result of applying this patch from\n> Gabriel, but I am scratching my head why we have hashcmp() there.\n> \n> Was it to avoid adding the same thing twice to the resulting list,\n> or something?\n\nIt is a sanity check. The code is looking in our list of things to fetch\nfor items that are pure objects, not refs (we already matched the refs\nby name, but obviously would not have matched any pure-sha1 requests to\nrefnames). So the conditional really is:\n\n if (!is_a_pure_sha1(ref))\n\tcontinue;\n\nWe can implement that as:\n\n if (get_sha1_hex(ref->name, sha1) || ref->name[40] != '\\0')\n\nbut as noted in the commit message for b791642:\n\n We could just check that we have exactly 40 characters of\n sha1. But let's be even more careful and make sure that we\n have a 40-char hex refname that matches what is in old_sha1.\n This is perhaps overly defensive, but spells out our\n assumptions clearly.\n\nE.g., if you did this:\n\n git fetch-pack --stdin $remote <<\\EOF\n 1234567890123456789012345678901234567890 abcdef1234abcdef1234abcdef1234abcdef1234\n EOF\n\nyou'd have a \"struct ref\" with a 40-hex sha1, but which does _not_ want\nthe object of the same name. This is not a pure-object request, and we\nshould only request 1234... if the ref abcd... is present on the remote.\n\nI doubt it would ever come up in real life; refs tend to start with\n\"refs/\", and I suspect short of manual prodding as above, you could not\nget anything without \"refs/\" to this point of the code.\n\nSo the patch:\n\n> > diff --git a/fetch-pack.c b/fetch-pack.c\n> > index 01e34b6..83b937b 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,\n> > \t\t\tif (ref->matched)\n> > \t\t\t\tcontinue;\n> > \t\t\tif (get_sha1_hex(ref->name, sha1) ||\n> > -\t\t\t ref->name[40] != '\\0' ||\n> > -\t\t\t hashcmp(sha1, ref->old_oid.hash))\n> > +\t\t\t ref->name[40] != '\\0')\n> > \t\t\t\tcontinue;\n> > \n> > \t\t\tref->matched = 1;\n> > +\t\t\thashcpy(ref->old_oid.hash, sha1);\n> > \t\t\t*newtail = copy_ref(ref);\n> > \t\t\tnewtail = &(*newtail)->next;\n> > \t\t}\n\nis a wrong direction, I think. It removes the extra safety check that\nskips the ref above. But worse, in the example above, it overwrites the\nreal object \"1234...\" with the name of the ref \"abcd...\" in the sha1\nfield. We'll ask for an object that may not even exist.\n\nThe commit message for Gabriel's patch says:\n\n> > Check was introduced in b791642 (filter_ref: avoid overwriting\n> > ref->old_sha1 with garbage, 2015-03-19), but was always false because\n> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.\n\nbut I don't think ref->old_oid.hash _is_ empty. At least, that was not\nthe conclusion from our discussion in:\n\n http://thread.gmane.org/gmane.comp.version-control.git/265480\n\nWe expect whoever creates the \"sought\" list to fill in the name and sha1\nas appropriate. If that is not happening in some code path, then yeah,\nfilter_refs() will not work as intended. But I think the solution there\nwould be to fix the caller to set up the \"struct ref\" more completely.\n\nGabriel, did this come from a bug you noticed in practice, or was it\njust an intended cleanup?\n\n-Peff\n"},{"id":"279667","messageId":"20160227191943.GD12822@sigill.intra.peff.net","threadId":"41551","inReplyTo":"20160227190712.GC12822@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T19:19:44Z","receivedAt":"2016-02-27T19:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 02:07:12PM -0500, Jeff King wrote:\n\n> We expect whoever creates the \"sought\" list to fill in the name and sha1\n> as appropriate. If that is not happening in some code path, then yeah,\n> filter_refs() will not work as intended. But I think the solution there\n> would be to fix the caller to set up the \"struct ref\" more completely.\n> \n> Gabriel, did this come from a bug you noticed in practice, or was it\n> just an intended cleanup?\n\nI double-checked that the code for git-fetch does so. It's in\nget_fetch_map()\n\n if (refspec->exact_sha1) {\n\t ref_map = alloc_ref(name);\n\t get_oid_hex(name, &ref_map->old_oid);\n } else ...\n\nSo we should always have old_oid filled in already, and there is no need\nto do so in filter_refs() (and in fact it is wrong, for the degenerate\nexample I gave earlier).\n\n-Peff\n"},{"id":"279671","messageId":"CABaesJ+yNJ6_z=sFc+bDEPqDDsw9fkB5bYgxJaAkAMVqHNWwrQ@mail.gmail.com","threadId":"41551","inReplyTo":"20160227191943.GD12822@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-02-27T20:28:55Z","receivedAt":"2016-02-27T20:28:55Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"On Sat, Feb 27, 2016 at 4:19 PM, Jeff King wrote:\n> On Sat, Feb 27, 2016 at 02:07:12PM -0500, Jeff King wrote:\n>\n>> We expect whoever creates the \"sought\" list to fill in the name and sha1\n>> as appropriate. If that is not happening in some code path, then yeah,\n>> filter_refs() will not work as intended. But I think the solution there\n>> would be to fix the caller to set up the \"struct ref\" more completely.\n>>\n>> Gabriel, did this come from a bug you noticed in practice, or was it\n>> just an intended cleanup?\n\nI was experimenting with uploadpack.hiderefs and uploadpack.allowTipSHA1InWant\nand couldn't get\n\n git fetch-pack $remote \n\nto work, and I traced the failure until that check. Reading more, I now see\nthat currently it requires\n\n git fetch-pack $remote \" \"\n\nto do what I want.\n\n>\n> I double-checked that the code for git-fetch does so. It's in\n> get_fetch_map()\n>\n> if (refspec->exact_sha1) {\n> ref_map = alloc_ref(name);\n> get_oid_hex(name, &ref_map->old_oid);\n> } else ...\n>\n> So we should always have old_oid filled in already, and there is no need\n> to do so in filter_refs() (and in fact it is wrong, for the degenerate\n> example I gave earlier).\n\ngit fetch-pack doesn't use these code paths. I'll send a new patch\nshortly to allow\nbare sha1's in fetch-pack.\n\n>\n> -Peff\n"},{"id":"279672","messageId":"1456605174-28360-1-git-send-email-gabrielfrancosouza@gmail.com","threadId":"41551","inReplyTo":"CABaesJ+yNJ6_z=sFc+bDEPqDDsw9fkB5bYgxJaAkAMVqHNWwrQ@mail.gmail.com","subject":"[PATCH] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-02-27T20:32:54Z","receivedAt":"2016-02-27T20:32:54Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n2013-12-05) added support for specifying a SHA-1 as well as a ref name.\nAdd support for specifying just a SHA-1 and set the ref name to the same\nvalue in this case.\n\nSigned-off-by: Gabriel Souza Franco \n---\n builtin/fetch-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 79a611f..d7e439a 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -16,10 +16,10 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,\n \tstruct ref *ref;\n \tstruct object_id oid;\n \n-\tif (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n-\t\tname += GIT_SHA1_HEXSZ + 1;\n-\telse\n+\tif (get_oid_hex(name, &oid))\n \t\toidclr(&oid);\n+\telse if (name[GIT_SHA1_HEXSZ] == ' ')\n+\t\tname += GIT_SHA1_HEXSZ + 1;\n \n \tref = alloc_ref(name);\n \toidcpy(&ref->old_oid, &oid);\n-- \n2.7.1\n"},{"id":"279673","messageId":"20160227220816.GA17475@sigill.intra.peff.net","threadId":"41551","inReplyTo":"CABaesJ+yNJ6_z=sFc+bDEPqDDsw9fkB5bYgxJaAkAMVqHNWwrQ@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T22:08:16Z","receivedAt":"2016-02-27T22:08:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 05:28:55PM -0300, Gabriel Souza Franco wrote:\n\n> On Sat, Feb 27, 2016 at 4:19 PM, Jeff King wrote:\n> > On Sat, Feb 27, 2016 at 02:07:12PM -0500, Jeff King wrote:\n> >\n> >> We expect whoever creates the \"sought\" list to fill in the name and sha1\n> >> as appropriate. If that is not happening in some code path, then yeah,\n> >> filter_refs() will not work as intended. But I think the solution there\n> >> would be to fix the caller to set up the \"struct ref\" more completely.\n> >>\n> >> Gabriel, did this come from a bug you noticed in practice, or was it\n> >> just an intended cleanup?\n> \n> I was experimenting with uploadpack.hiderefs and uploadpack.allowTipSHA1InWant\n> and couldn't get\n> \n> git fetch-pack $remote \n> \n> to work, and I traced the failure until that check. Reading more, I now see\n> that currently it requires\n> \n> git fetch-pack $remote \" \"\n> \n> to do what I want.\n\nAh, that makes sense. I do think the \" \" syntax is a bit\nweird, and I think mostly because the pure-object fetch came much later\nin git's life; at this point hardly anybody uses a manual fetch-pack.\n\nIt would probably make sense to \"\" to set up the ref correctly.\n\n> > I double-checked that the code for git-fetch does so. It's in\n> > get_fetch_map()\n> [...]\n> \n> git fetch-pack doesn't use these code paths. I'll send a new patch\n> shortly to allow\n> bare sha1's in fetch-pack.\n\nRight. Sounds like a good plan.\n\n-Peff\n"},{"id":"279674","messageId":"20160227221248.GB17475@sigill.intra.peff.net","threadId":"41551","inReplyTo":"1456605174-28360-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T22:12:48Z","receivedAt":"2016-02-27T22:12:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 05:32:54PM -0300, Gabriel Souza Franco wrote:\n\n> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n> Add support for specifying just a SHA-1 and set the ref name to the same\n> value in this case.\n> \n> Signed-off-by: Gabriel Souza Franco \n> ---\n> builtin/fetch-pack.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 79a611f..d7e439a 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -16,10 +16,10 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,\n> \tstruct ref *ref;\n> \tstruct object_id oid;\n> \n> -\tif (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n> -\t\tname += GIT_SHA1_HEXSZ + 1;\n> -\telse\n> +\tif (get_oid_hex(name, &oid))\n> \t\toidclr(&oid);\n> +\telse if (name[GIT_SHA1_HEXSZ] == ' ')\n> +\t\tname += GIT_SHA1_HEXSZ + 1;\n\nThis makes sense to me. I wonder if we should be more particular about\nthe pure-sha1 case consuming the whole string, though. E.g., if we get:\n\n 1234567890123456789012345678901234567890-bananas\n\nthat should probably not have sha1 1234...\n\nI don't think it should ever really happen in practice, but it might be\nworth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither\nspace nor '\\0'.\n\n-Peff\n"},{"id":"279675","messageId":"CABaesJJCfd6FzgEVCWMJH8-gKiv3pT8BGq4f-qFgprVde+ViQQ@mail.gmail.com","threadId":"41551","inReplyTo":"20160227221248.GB17475@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-02-27T22:23:22Z","receivedAt":"2016-02-27T22:23:22Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"On Sat, Feb 27, 2016 at 7:12 PM, Jeff King wrote:\n> On Sat, Feb 27, 2016 at 05:32:54PM -0300, Gabriel Souza Franco wrote:\n>\n>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n>> Add support for specifying just a SHA-1 and set the ref name to the same\n>> value in this case.\n>>\n>> Signed-off-by: Gabriel Souza Franco \n>> ---\n>> builtin/fetch-pack.c | 6 +++---\n>> 1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n>> index 79a611f..d7e439a 100644\n>> --- a/builtin/fetch-pack.c\n>> +++ b/builtin/fetch-pack.c\n>> @@ -16,10 +16,10 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,\n>> struct ref *ref;\n>> struct object_id oid;\n>>\n>> - if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n>> - name += GIT_SHA1_HEXSZ + 1;\n>> - else\n>> + if (get_oid_hex(name, &oid))\n>> oidclr(&oid);\n>> + else if (name[GIT_SHA1_HEXSZ] == ' ')\n>> + name += GIT_SHA1_HEXSZ + 1;\n>\n> This makes sense to me. I wonder if we should be more particular about\n> the pure-sha1 case consuming the whole string, though. E.g., if we get:\n>\n> 1234567890123456789012345678901234567890-bananas\n>\n> that should probably not have sha1 1234...\n>\n> I don't think it should ever really happen in practice, but it might be\n> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither\n> space nor '\\0'.\n\nRight. What kind of complaining? Is doing oidclr(&oid) and letting it\nfail elsewhere enough?\nAlso, it already fails precisely because of that check I wanted to\nremove earlier.\n\n>\n> -Peff\n"},{"id":"279726","messageId":"xmqqr3fwejg6.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"20160227190712.GC12822@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix unadvertised requests validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-28T19:14:17Z","receivedAt":"2016-02-28T19:14:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So the patch:\n>\n>> > diff --git a/fetch-pack.c b/fetch-pack.c\n>>...\n> is a wrong direction, I think. It removes the extra safety check that\n> skips the ref above. But worse, in the example above, it overwrites the\n> real object \"1234...\" with the name of the ref \"abcd...\" in the sha1\n> field. We'll ask for an object that may not even exist.\n>\n> The commit message for Gabriel's patch says:\n>\n>> > Check was introduced in b791642 (filter_ref: avoid overwriting\n>> > ref->old_sha1 with garbage, 2015-03-19), but was always false because\n>> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.\n>\n> but I don't think ref->old_oid.hash _is_ empty. At least, that was not\n> the conclusion from our discussion in:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/265480\n>\n> We expect whoever creates the \"sought\" list to fill in the name and sha1\n> as appropriate. If that is not happening in some code path, then yeah,\n> filter_refs() will not work as intended. But I think the solution there\n> would be to fix the caller to set up the \"struct ref\" more completely.\n\nAh, I forgot that thread completely.\n"},{"id":"279727","messageId":"xmqqh9gseiqk.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"CABaesJJCfd6FzgEVCWMJH8-gKiv3pT8BGq4f-qFgprVde+ViQQ@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: fix object_id of exact sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-28T19:29:39Z","receivedAt":"2016-02-28T19:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gabriel Souza Franco writes:\n\n>>> struct object_id oid;\n>>>\n>>> - if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n>>> - name += GIT_SHA1_HEXSZ + 1;\n>>> - else\n>>> + if (get_oid_hex(name, &oid))\n>>> oidclr(&oid);\n>>> + else if (name[GIT_SHA1_HEXSZ] == ' ')\n>>> + name += GIT_SHA1_HEXSZ + 1;\n>>\n>> This makes sense to me. I wonder if we should be more particular about\n>> the pure-sha1 case consuming the whole string, though. E.g., if we get:\n>>\n>> 1234567890123456789012345678901234567890-bananas\n>>\n>> that should probably not have sha1 1234...\n>>\n>> I don't think it should ever really happen in practice, but it might be\n>> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither\n>> space nor '\\0'.\n>\n> Right. What kind of complaining? Is doing oidclr(&oid) and letting it\n> fail elsewhere enough?\n\nI think that is the most sensible. After all, the first get-oid-hex\nexpects to find a valid 40-hex object name at the beginning of line,\nand oidclr() is the way for it to signal a broken input, which is\nhow the callers of this codepath expects errors to be caught.\n\nThanks.\n"},{"id":"279733","messageId":"1456698144-11519-1-git-send-email-gabrielfrancosouza@gmail.com","threadId":"41551","inReplyTo":"xmqqh9gseiqk.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-02-28T22:22:24Z","receivedAt":"2016-02-28T22:22:24Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n2013-12-05) added support for specifying a SHA-1 as well as a ref name.\nAdd support for specifying just a SHA-1 and set the ref name to the same\nvalue in this case.\n\nSigned-off-by: Gabriel Souza Franco \n---\n\nNot the cleanest conditional I've ever written, but it should handle\nall cases correctly.\n\n builtin/fetch-pack.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 79a611f..763f510 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -16,10 +16,12 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,\n \tstruct ref *ref;\n \tstruct object_id oid;\n \n-\tif (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n-\t\tname += GIT_SHA1_HEXSZ + 1;\n-\telse\n+\tif (get_oid_hex(name, &oid) ||\n+\t\t\t(name[GIT_SHA1_HEXSZ] != ' ' &&\n+\t\t\t name[GIT_SHA1_HEXSZ] != '\\0'))\n \t\toidclr(&oid);\n+\telse if (name[GIT_SHA1_HEXSZ] == ' ')\n+\t\tname += GIT_SHA1_HEXSZ + 1;\n \n \tref = alloc_ref(name);\n \toidcpy(&ref->old_oid, &oid);\n-- \n2.7.2\n"},{"id":"279744","messageId":"alpine.DEB.2.20.1602290912260.3152@virtualbox","threadId":"41551","inReplyTo":"1456698144-11519-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH v2] fetch-pack: fix object_id of exact sha1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-29T08:30:53Z","receivedAt":"2016-02-29T08:30:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gabriel,\n\nOn Sun, 28 Feb 2016, Gabriel Souza Franco wrote:\n\n> Not the cleanest conditional I've ever written, but it should handle\n> all cases correctly.\n\nIt could be much worse:\n\n> +\tif (get_oid_hex(name, &oid) ||\n> +\t\t\t(name[GIT_SHA1_HEXSZ] != ' ' &&\n> +\t\t\t name[GIT_SHA1_HEXSZ] != '\\0'))\n\nI know developers who would write this as\n\n\tif (get_oid_hex(name, &oid) || (name[GIT_SHA1_HEXSZ] & ' '))\n\nand not even begin to realize that this is a problem.\n\nSo I'd say your conditional is good.\n\nHaving said that, this *might* be a good opportunity to imitate the\nskip_prefix() function. If there are enough similar code constructs, we\ncould simplify all of them by introducing the function\n\n\tskip_oid_hex(const char *str, struct object_id *oid, const char **out)\n\nthat returns 1 if and only if an oid was parsed, and stores the pointer\nafter the oid in \"out\" (skipping an additional space if there is one)?\n\nCiao,\nDscho\n"},{"id":"279756","messageId":"20160229095041.GA2950@sigill.intra.peff.net","threadId":"41551","inReplyTo":"xmqqh9gseiqk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T09:50:41Z","receivedAt":"2016-02-29T09:50:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 28, 2016 at 11:29:39AM -0800, Junio C Hamano wrote:\n\n> Gabriel Souza Franco writes:\n> \n> >>> struct object_id oid;\n> >>>\n> >>> - if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n> >>> - name += GIT_SHA1_HEXSZ + 1;\n> >>> - else\n> >>> + if (get_oid_hex(name, &oid))\n> >>> oidclr(&oid);\n> >>> + else if (name[GIT_SHA1_HEXSZ] == ' ')\n> >>> + name += GIT_SHA1_HEXSZ + 1;\n> >>\n> >> This makes sense to me. I wonder if we should be more particular about\n> >> the pure-sha1 case consuming the whole string, though. E.g., if we get:\n> >>\n> >> 1234567890123456789012345678901234567890-bananas\n> >>\n> >> that should probably not have sha1 1234...\n> >>\n> >> I don't think it should ever really happen in practice, but it might be\n> >> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither\n> >> space nor '\\0'.\n> >\n> > Right. What kind of complaining? Is doing oidclr(&oid) and letting it\n> > fail elsewhere enough?\n> \n> I think that is the most sensible. After all, the first get-oid-hex\n> expects to find a valid 40-hex object name at the beginning of line,\n> and oidclr() is the way for it to signal a broken input, which is\n> how the callers of this codepath expects errors to be caught.\n\nActually, I think we _don't_ want to signal an error here, but checking\nfor '\\0' is still the right thing to do.\n\nOnce upon a time, fetch-pack took just the names of refs, like:\n\n git fetch-pack $remote refs/heads/foo\n\nand the same format was used for --stdin. Then in 58f2ed0 (remote-curl:\npass ref SHA-1 to fetch-pack as well, 2013-12-05), it learned to take\n\"$sha1 $ref\". But if we didn't see a sha1, then we continued to treat it\nas a refname.\n\nThis patch adds a new format, just \"$sha1\". So if get_oid_hex() succeeds\n_and_ we see '\\0', we know we have that case. But if we don't see '\\0',\nthen we should assume it's a refname (e.g., \"1234abcd...-bananas\").\n\nI think in practice it shouldn't matter much, as callers should be\nfeeding fully qualified refs (and we document this). However, we still\nwant to distinguish so that we give the correct error (\"no such remote\nref 1234abcd...-bananas\", not \"whoops, the other side doesn't have\n1234abcd\").\n\n-Peff\n"},{"id":"279758","messageId":"20160229100030.GB2950@sigill.intra.peff.net","threadId":"41551","inReplyTo":"1456698144-11519-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH v2] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T10:00:30Z","receivedAt":"2016-02-29T10:00:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:\n\n> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n> Add support for specifying just a SHA-1 and set the ref name to the same\n> value in this case.\n> \n> Signed-off-by: Gabriel Souza Franco \n> ---\n> \n> Not the cleanest conditional I've ever written, but it should handle\n> all cases correctly.\n\nI think it does. But I wonder if it wouldn't be more readable to cover\nthe three formats independently, like:\n\n if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == ' ') {\n\t/* , find refname */\n\tname += GIT_SHA1_HEXSZ + 1;\n } else if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == '\\0') {\n\t/* , leave sha1 as name */\n } else {\n\t/* , clear any cruft from get_oid_hex */\n\toidclr(&ref->old_oid);\n }\n\nAnd as a bonus you get rid of the separate \"oid\". That does call into\nget_oid_hex twice, but I doubt the performance impact is measurable.\n\nWe could also do:\n\n if (!get_oid_hex(name, &ref->old_oid)) {\n\tif (name[GIT_SHA1_HEXSZ] == ' ') {\n\t\t/* , find refname */\n\t\tname += GIT_SHA1_HEXSZ + 1;\n\t} else if (name[GIT_SHA1_HEXSZ] == '\\0') {\n\t\t/* , leave sha1 as name */\n\t} else {\n\t\t/* , clear cruft from oid */\n\t\toidclr(&ref->old_oid);\n\t}\n } else {\n\t/* , clear cruft from get_oid_hex */\n\toidclr(&ref->old_oid);\n }\n\nif you want to minimize the calls at the expense of having to repeat the\noidclr().\n\n-Peff\n"},{"id":"279934","messageId":"CABaesJK+zuuYAJ6YaEugLMFywMqE8V0W1=_4mJPGDAnfT9yXJg@mail.gmail.com","threadId":"41551","inReplyTo":"20160229100030.GB2950@sigill.intra.peff.net","subject":"Re: [PATCH v2] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-03-01T02:08:07Z","receivedAt":"2016-03-01T02:08:07Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"On Mon, Feb 29, 2016 at 5:30 AM, Johannes Schindelin\n wrote:\n> Having said that, this *might* be a good opportunity to imitate the\n> skip_prefix() function. If there are enough similar code constructs, we\n> could simplify all of them by introducing the function\n>\n> skip_oid_hex(const char *str, struct object_id *oid, const char **out)\n>\n> that returns 1 if and only if an oid was parsed, and stores the pointer\n> after the oid in \"out\" (skipping an additional space if there is one)?\n\nI don't think there's any other place that accepts all of \"\",\n\" \" and \"\"\nbased on a quick grep for get_oid_hex.\n\nOn Mon, Feb 29, 2016 at 7:00 AM, Jeff King wrote:\n> On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:\n>\n>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n>> Add support for specifying just a SHA-1 and set the ref name to the same\n>> value in this case.\n>>\n>> Signed-off-by: Gabriel Souza Franco \n>> ---\n>>\n>> Not the cleanest conditional I've ever written, but it should handle\n>> all cases correctly.\n>\n> I think it does. But I wonder if it wouldn't be more readable to cover\n> the three formats independently, like:\n>\n> if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == ' ') {\n> /* , find refname */\n> name += GIT_SHA1_HEXSZ + 1;\n> } else if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == '\\0') {\n> /* , leave sha1 as name */\n> } else {\n> /* , clear any cruft from get_oid_hex */\n> oidclr(&ref->old_oid);\n> }\n>\n> And as a bonus you get rid of the separate \"oid\". That does call into\n> get_oid_hex twice, but I doubt the performance impact is measurable.\n>\n> We could also do:\n>\n> if (!get_oid_hex(name, &ref->old_oid)) {\n> if (name[GIT_SHA1_HEXSZ] == ' ') {\n> /* , find refname */\n> name += GIT_SHA1_HEXSZ + 1;\n> } else if (name[GIT_SHA1_HEXSZ] == '\\0') {\n> /* , leave sha1 as name */\n> } else {\n> /* , clear cruft from oid */\n> oidclr(&ref->old_oid);\n> }\n> } else {\n> /* , clear cruft from get_oid_hex */\n> oidclr(&ref->old_oid);\n> }\n>\n> if you want to minimize the calls at the expense of having to repeat the\n> oidclr().\n\nI think I like this version more, and is close to what I had initially\nbefore I tried to be clever about it.\nBesides, this isn't a performance critical function, so it shouldn't\nmatter much.\nWill send a new (and hopefully final) version shortly.\n\n>\n> -Peff\n"},{"id":"279935","messageId":"1456798376-29904-1-git-send-email-gabrielfrancosouza@gmail.com","threadId":"41551","inReplyTo":"CABaesJK+zuuYAJ6YaEugLMFywMqE8V0W1=_4mJPGDAnfT9yXJg@mail.gmail.com","subject":"[PATCH v3] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-03-01T02:12:56Z","receivedAt":"2016-03-01T02:12:56Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n2013-12-05) added support for specifying a SHA-1 as well as a ref name.\nAdd support for specifying just a SHA-1 and set the ref name to the same\nvalue in this case.\n\nSigned-off-by: Gabriel Souza Franco \n---\n\nI did keep the oid variable because ref is uninitialized at that point,\nand this means having to copy either name or old_oid afterwards anyway.\n\n builtin/fetch-pack.c | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 79a611f..50c9901 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -16,10 +16,20 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,\n \tstruct ref *ref;\n \tstruct object_id oid;\n \n-\tif (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n-\t\tname += GIT_SHA1_HEXSZ + 1;\n-\telse\n+\tif (!get_oid_hex(name, &oid)) {\n+\t\tif (name[GIT_SHA1_HEXSZ] == ' ') {\n+\t\t\t/* , find refname */\n+\t\t\tname += GIT_SHA1_HEXSZ + 1;\n+\t\t} else if (name[GIT_SHA1_HEXSZ] == '\\0') {\n+\t\t\t/* , leave sha1 as name */\n+\t\t} else {\n+\t\t\t/* , clear cruft from oid */\n+\t\t\toidclr(&oid);\n+\t\t}\n+\t} else {\n+\t\t/* , clear cruft from get_oid_hex */\n \t\toidclr(&oid);\n+\t}\n \n \tref = alloc_ref(name);\n \toidcpy(&ref->old_oid, &oid);\n-- \n2.7.2\n"},{"id":"279936","messageId":"20160301044033.GA19272@sigill.intra.peff.net","threadId":"41551","inReplyTo":"CABaesJK+zuuYAJ6YaEugLMFywMqE8V0W1=_4mJPGDAnfT9yXJg@mail.gmail.com","subject":"Re: [PATCH v2] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-01T04:40:33Z","receivedAt":"2016-03-01T04:40:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 29, 2016 at 11:08:07PM -0300, Gabriel Souza Franco wrote:\n\n> On Mon, Feb 29, 2016 at 5:30 AM, Johannes Schindelin\n> wrote:\n> > Having said that, this *might* be a good opportunity to imitate the\n> > skip_prefix() function. If there are enough similar code constructs, we\n> > could simplify all of them by introducing the function\n> >\n> > skip_oid_hex(const char *str, struct object_id *oid, const char **out)\n> >\n> > that returns 1 if and only if an oid was parsed, and stores the pointer\n> > after the oid in \"out\" (skipping an additional space if there is one)?\n> \n> I don't think there's any other place that accepts all of \"\",\n> \" \" and \"\"\n> based on a quick grep for get_oid_hex.\n\nYes, but there are places where we get_oid_hex and then skip past that,\nwhich could use the skip_oid_hex function, like:\n\ndiff --git a/connect.c b/connect.c\nindex 0478631..ba22ee6 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -149,10 +149,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||\n-\t\t\tbuffer[GIT_SHA1_HEXSZ] != ' ')\n+\t\tif (!skip_oid_hex(buffer, &old_oid, &name) ||\n+\t\t !skip_prefix(name, \" \", &name))\n \t\t\tdie(\"protocol error: expected sha/ref, got '%s'\", buffer);\n-\t\tname = buffer + GIT_SHA1_HEXSZ + 1;\n \n \t\tname_len = strlen(name);\n \t\tif (len != name_len + GIT_SHA1_HEXSZ + 1) {\n\n_But_, if you look at the context just below, we make another implicit\nassumption about GIT_SHA1_HEXSZ. So it's really not buying us that much\n(unless we switch around the whole function to keep reading to the final\npointer, and compare \"end - start\" to the original \"len\" here).\n\nSo I'm not sure it's worth the trouble. I am really happy with the\nskip_prefix() function for parsing like this, but I think it's just not\nnearly as big a deal with oid-parsing, because we already have a nice\nconstant of GIT_SHA1_HEXSZ to match it (whereas skipping \"foo\" requires\nus writing the magical \"3\" somewhere).\n\nAnyway. Whether we want to pursue that or not, I don't think it needs to\nbe part of your series. Let's focus on the original goal.\n\n-Peff\n"},{"id":"279937","messageId":"20160301045453.GB19272@sigill.intra.peff.net","threadId":"41551","inReplyTo":"1456798376-29904-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH v3] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-01T04:54:53Z","receivedAt":"2016-03-01T04:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 29, 2016 at 11:12:56PM -0300, Gabriel Souza Franco wrote:\n\n> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n> Add support for specifying just a SHA-1 and set the ref name to the same\n> value in this case.\n> \n> Signed-off-by: Gabriel Souza Franco \n> ---\n> \n> I did keep the oid variable because ref is uninitialized at that point,\n> and this means having to copy either name or old_oid afterwards anyway.\n\nOh, right. That's why we had the variable in the first place (even in\nthe original, we could otherwise have gone without the extra variable).\n\n> builtin/fetch-pack.c | 16 +++++++++++++---\n> 1 file changed, 13 insertions(+), 3 deletions(-)\n\nThe code looks good to me. Do we need documentation or test updates?\n\nHere's a test that can be squashed in. For documentation, it looks like\nwe don't cover the \" \" form at all. That's maybe OK, as it's\nmostly for internal use by remote-http (though fetch-pack _is_ plumbing,\nso perhaps some other remote-* could make use of it). But perhaps we\nshould document that \"\" should work.\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex e5f83bf..9b9bec4 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -531,6 +531,20 @@ test_expect_success 'shallow fetch with tags does not break the repository' '\n \t\tgit fsck\n \t)\n '\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1' '\n+\tgit init hidden &&\n+\t(\n+\t\tcd hidden &&\n+\t\ttest_commit 1 &&\n+\t\ttest_commit 2 &&\n+\t\tgit update-ref refs/hidden/one HEAD^ &&\n+\t\tgit config transfer.hiderefs refs/hidden &&\n+\t\tgit config uploadpack.allowtipsha1inwant true\n+\t) &&\n+\tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n"},{"id":"280162","messageId":"CABaesJ+5B9WEqJQsK5s+WFczWMfvBgTx6UmVwjAL-WAf55zJUw@mail.gmail.com","threadId":"41551","inReplyTo":"20160301045453.GB19272@sigill.intra.peff.net","subject":"Re: [PATCH v3] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-03-03T23:35:54Z","receivedAt":"2016-03-03T23:35:54Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"On Tue, Mar 1, 2016 at 1:54 AM, Jeff King wrote:\n> On Mon, Feb 29, 2016 at 11:12:56PM -0300, Gabriel Souza Franco wrote:\n>\n>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n>> Add support for specifying just a SHA-1 and set the ref name to the same\n>> value in this case.\n>>\n>> Signed-off-by: Gabriel Souza Franco \n>> ---\n>>\n>> I did keep the oid variable because ref is uninitialized at that point,\n>> and this means having to copy either name or old_oid afterwards anyway.\n>\n> Oh, right. That's why we had the variable in the first place (even in\n> the original, we could otherwise have gone without the extra variable).\n>\n>> builtin/fetch-pack.c | 16 +++++++++++++---\n>> 1 file changed, 13 insertions(+), 3 deletions(-)\n>\n> The code looks good to me. Do we need documentation or test updates?\n>\n> Here's a test that can be squashed in. For documentation, it looks like\n> we don't cover the \" \" form at all. That's maybe OK, as it's\n> mostly for internal use by remote-http (though fetch-pack _is_ plumbing,\n> so perhaps some other remote-* could make use of it). But perhaps we\n> should document that \"\" should work.\n\nThanks for providing a test, I hadn't looked up those yet. For\ndocumentation, should\nit be on the same patch or a new one? Also, I'm not exactly sure how\nto word that ...\ncan also contain a hash instead of a ref.\n\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index e5f83bf..9b9bec4 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -531,6 +531,20 @@ test_expect_success 'shallow fetch with tags does not break the repository' '\n> git fsck\n> )\n> '\n> +\n> +test_expect_success 'fetch-pack can fetch a raw sha1' '\n> + git init hidden &&\n> + (\n> + cd hidden &&\n> + test_commit 1 &&\n> + test_commit 2 &&\n> + git update-ref refs/hidden/one HEAD^ &&\n> + git config transfer.hiderefs refs/hidden &&\n> + git config uploadpack.allowtipsha1inwant true\n> + ) &&\n> + git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n> +'\n> +\n> check_prot_path () {\n> cat >expected <<-EOF &&\n> Diag: url=$1\n"},{"id":"280163","messageId":"20160304005000.GA1074@sigill.intra.peff.net","threadId":"41551","inReplyTo":"CABaesJ+5B9WEqJQsK5s+WFczWMfvBgTx6UmVwjAL-WAf55zJUw@mail.gmail.com","subject":"Re: [PATCH v3] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-04T00:50:00Z","receivedAt":"2016-03-04T00:50:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 08:35:54PM -0300, Gabriel Souza Franco wrote:\n\n> > The code looks good to me. Do we need documentation or test updates?\n> >\n> > Here's a test that can be squashed in. For documentation, it looks like\n> > we don't cover the \" \" form at all. That's maybe OK, as it's\n> > mostly for internal use by remote-http (though fetch-pack _is_ plumbing,\n> > so perhaps some other remote-* could make use of it). But perhaps we\n> > should document that \"\" should work.\n> \n> Thanks for providing a test, I hadn't looked up those yet. For\n> documentation, should\n> it be on the same patch or a new one? Also, I'm not exactly sure how\n> to word that ...\n> can also contain a hash instead of a ref.\n\nI think it make sense as part of the same patch. I guess you could still\ncall the argument \"\" even though it takes more now, and just\nexplain the new feature in the appropriate section. I can't think of a\nbetter word to use (somehow \"\" feels too broad, and the primary\nuse would still be a list of refs).\n\n-Peff\n"},{"id":"280224","messageId":"1457140298-2400-1-git-send-email-gabrielfrancosouza@gmail.com","threadId":"41551","inReplyTo":"20160304005000.GA1074@sigill.intra.peff.net","subject":"[PATCH v4] fetch-pack: fix object_id of exact sha1","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-03-05T01:11:38Z","receivedAt":"2016-03-05T01:11:38Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n2013-12-05) added support for specifying a SHA-1 as well as a ref name.\nAdd support for specifying just a SHA-1 and set the ref name to the same\nvalue in this case.\n\nSigned-off-by: Gabriel Souza Franco \n---\n Documentation/git-fetch-pack.txt | 4 ++++\n builtin/fetch-pack.c | 16 +++++++++++++---\n t/t5500-fetch-pack.sh | 14 ++++++++++++++\n 3 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex 8680f45..239623c 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -104,6 +104,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tThe remote heads to update from. This is relative to\n \t$GIT_DIR (e.g. \"HEAD\", \"refs/heads/master\"). When\n \tunspecified, update from all heads the remote side has.\n++\n+If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or\n+`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex\n+sha1s present on the remote.\n \n SEE ALSO\n --------\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 79a611f..50c9901 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -16,10 +16,20 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,\n \tstruct ref *ref;\n \tstruct object_id oid;\n \n-\tif (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')\n-\t\tname += GIT_SHA1_HEXSZ + 1;\n-\telse\n+\tif (!get_oid_hex(name, &oid)) {\n+\t\tif (name[GIT_SHA1_HEXSZ] == ' ') {\n+\t\t\t/* , find refname */\n+\t\t\tname += GIT_SHA1_HEXSZ + 1;\n+\t\t} else if (name[GIT_SHA1_HEXSZ] == '\\0') {\n+\t\t\t/* , leave sha1 as name */\n+\t\t} else {\n+\t\t\t/* , clear cruft from oid */\n+\t\t\toidclr(&oid);\n+\t\t}\n+\t} else {\n+\t\t/* , clear cruft from get_oid_hex */\n \t\toidclr(&oid);\n+\t}\n \n \tref = alloc_ref(name);\n \toidcpy(&ref->old_oid, &oid);\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex e5f83bf..9b9bec4 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -531,6 +531,20 @@ test_expect_success 'shallow fetch with tags does not break the repository' '\n \t\tgit fsck\n \t)\n '\n+\n+test_expect_success 'fetch-pack can fetch a raw sha1' '\n+\tgit init hidden &&\n+\t(\n+\t\tcd hidden &&\n+\t\ttest_commit 1 &&\n+\t\ttest_commit 2 &&\n+\t\tgit update-ref refs/hidden/one HEAD^ &&\n+\t\tgit config transfer.hiderefs refs/hidden &&\n+\t\tgit config uploadpack.allowtipsha1inwant true\n+\t) &&\n+\tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\n-- \n2.7.2\n"},{"id":"280249","messageId":"20160305165946.GA23114@sigill.intra.peff.net","threadId":"41551","inReplyTo":"1457140298-2400-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH v4] fetch-pack: fix object_id of exact sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-05T16:59:47Z","receivedAt":"2016-03-05T16:59:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 04, 2016 at 10:11:38PM -0300, Gabriel Souza Franco wrote:\n\n> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n> Add support for specifying just a SHA-1 and set the ref name to the same\n> value in this case.\n> \n> Signed-off-by: Gabriel Souza Franco \n> ---\n> Documentation/git-fetch-pack.txt | 4 ++++\n> builtin/fetch-pack.c | 16 +++++++++++++---\n> t/t5500-fetch-pack.sh | 14 ++++++++++++++\n> 3 files changed, 31 insertions(+), 3 deletions(-)\n\nThanks, this version looks good to me.\n\n-Peff\n"},{"id":"280255","messageId":"xmqqd1r8vjqi.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"20160305165946.GA23114@sigill.intra.peff.net","subject":"Re: [PATCH v4] fetch-pack: fix object_id of exact sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-05T18:54:13Z","receivedAt":"2016-03-05T18:54:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 04, 2016 at 10:11:38PM -0300, Gabriel Souza Franco wrote:\n>\n>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,\n>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.\n>> Add support for specifying just a SHA-1 and set the ref name to the same\n>> value in this case.\n>> \n>> Signed-off-by: Gabriel Souza Franco \n>> ---\n>> Documentation/git-fetch-pack.txt | 4 ++++\n>> builtin/fetch-pack.c | 16 +++++++++++++---\n>> t/t5500-fetch-pack.sh | 14 ++++++++++++++\n>> 3 files changed, 31 insertions(+), 3 deletions(-)\n>\n> Thanks, this version looks good to me.\n\nIt does to me too, but unfortunately the previous one is already in\n'next'. Something like this as an incremental update would suffice.\n\n-- >8 --\nFrom: Gabriel Souza Franco \nDate: Fri, 4 Mar 2016 22:11:38 -0300\nSubject: fetch-pack: update the documentation for \"...\" arguments\n\nWhen we started allowing an exact object name to be fetched from the\ncommand line, we forgot to update the documentation. \n\nSigned-off-by: Gabriel Souza Franco \n--\n Documentation/git-fetch-pack.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex 8680f45..239623c 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -104,6 +104,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tThe remote heads to update from. This is relative to\n \t$GIT_DIR (e.g. \"HEAD\", \"refs/heads/master\"). When\n \tunspecified, update from all heads the remote side has.\n++\n+If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or\n+`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex\n+sha1s present on the remote.\n \n SEE ALSO\n --------\n"},{"id":"280258","messageId":"1457206444-26134-1-git-send-email-gabrielfrancosouza@gmail.com","threadId":"41551","inReplyTo":"xmqqd1r8vjqi.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] fetch-pack: update the documentation for \"...\" arguments","fromName":"Gabriel Souza Franco","fromEmail":"gabrielfrancosouza@gmail.com","sentAt":"2016-03-05T19:34:04Z","receivedAt":"2016-03-05T19:34:04Z","isPatch":true,"sender":{"key":"gabrielfrancosouza@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9276001?v=4"},"body":"When we started allowing an exact object name to be fetched from the\ncommand line, we forgot to update the documentation.\n\nSigned-off-by: Gabriel Souza Franco \n---\n Documentation/git-fetch-pack.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex 8680f45..239623c 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -104,6 +104,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tThe remote heads to update from. This is relative to\n \t$GIT_DIR (e.g. \"HEAD\", \"refs/heads/master\"). When\n \tunspecified, update from all heads the remote side has.\n++\n+If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or\n+`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex\n+sha1s present on the remote.\n \n SEE ALSO\n --------\n-- \n2.7.2\n"},{"id":"280259","messageId":"xmqqvb50u38x.fsf@gitster.mtv.corp.google.com","threadId":"41551","inReplyTo":"1457206444-26134-1-git-send-email-gabrielfrancosouza@gmail.com","subject":"Re: [PATCH] fetch-pack: update the documentation for \"...\" arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-05T19:35:42Z","receivedAt":"2016-03-05T19:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gabriel Souza Franco writes:\n\n> When we started allowing an exact object name to be fetched from the\n> command line, we forgot to update the documentation.\n>\n> Signed-off-by: Gabriel Souza Franco \n> ---\n\nThanks for resending ;-)\n"}]}