{"thread":{"id":"31295","subject":"[PATCH 0/2] further fixes of check_ref_format() users","startedAt":"2012-08-20T17:39:55Z","lastAt":"2012-08-22T16:56:08Z","messageCount":8,"participants":["Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"197425","messageId":"1345484397-10958-1-git-send-email-gitster@pobox.com","threadId":"31295","inReplyTo":null,"subject":"[PATCH 0/2] further fixes of check_ref_format() users","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T17:39:55Z","receivedAt":"2012-08-20T17:39:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There were a few overtight \"check_ref_format()\" that was exposed\nwith a natural \"mirror\" refspec \"refs/*:refs/*\".\n\nJunio C Hamano (2):\n  fetch/push: allow refs/*:refs/*\n  get_fetch_map(): tighten checks on dest refs\n\n builtin/fetch-pack.c   |  2 +-\n builtin/receive-pack.c |  2 +-\n connect.c              |  8 ++------\n remote.c               |  6 +++---\n t/t5516-fetch-push.sh  | 33 +++++++++++++++++++++++++++++++++\n 5 files changed, 40 insertions(+), 11 deletions(-)\n\n-- \n1.7.12.92.g7963261\n"},{"id":"197426","messageId":"1345484397-10958-2-git-send-email-gitster@pobox.com","threadId":"31295","inReplyTo":"1345484397-10958-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T17:39:56Z","receivedAt":"2012-08-20T17:39:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There are a handful of places where we call check_refname_format() on a\nsubstring after \"refs/\" of a refname we are going to use, and filter out a\nvalid match with \"refs/stash\" with such a pathspec.  Not sending a stash\nmay arguably be a feature (as stash is inherently a local workflow\nelement), but the code in the transport layer is oblivious to this\nfiltering performed by the lower layer of the code, and complains that the\nother side did not send all the objects that needs to complete refs/stash\nat the end, even though the code will not write refs/stash out anyway, and\nmaking the whole command fail.\n\nThis is an attempt to \"fix\" it by using check_refname_format() on the\nwhole \"refs/.....\" string and allowing refs/stash to be also copied.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch-pack.c   |  2 +-\n builtin/receive-pack.c |  2 +-\n connect.c              |  8 ++------\n remote.c               |  2 +-\n t/t5516-fetch-push.sh  | 33 +++++++++++++++++++++++++++++++++\n 5 files changed, 38 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 6207ecd..a3e3fa3 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n \tfor (ref = *refs; ref; ref = next) {\n \t\tnext = ref->next;\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n-\t\t    check_refname_format(ref->name + 5, 0))\n+\t\t    check_refname_format(ref->name, 0))\n \t\t\t; /* trash */\n \t\telse if (args.fetch_all &&\n \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 7ec68a1..1935b80 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -401,7 +401,7 @@ static const char *update(struct command *cmd)\n \tstruct ref_lock *lock;\n \n \t/* only refs/... are allowed */\n-\tif (prefixcmp(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n+\tif (prefixcmp(name, \"refs/\") || check_refname_format(name, 0)) {\n \t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n \t\treturn \"funny refname\";\n \t}\ndiff --git a/connect.c b/connect.c\nindex c8d0ea5..b30639c 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -17,20 +17,16 @@ static int check_ref(const char *name, int len, unsigned int flags)\n \tif (len < 5 || memcmp(name, \"refs/\", 5))\n \t\treturn 0;\n \n-\t/* Skip the \"refs/\" part */\n-\tname += 5;\n-\tlen -= 5;\n-\n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n \tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\n-\tif ((flags & REF_HEADS) && !memcmp(name, \"heads/\", 6))\n+\tif ((flags & REF_HEADS) && !memcmp(name, \"refs/heads/\", 11))\n \t\treturn 1;\n \n \t/* REF_TAGS means that we want tags */\n-\tif ((flags & REF_TAGS) && !memcmp(name, \"tags/\", 5))\n+\tif ((flags & REF_TAGS) && !memcmp(name, \"refs/tags/\", 10))\n \t\treturn 1;\n \n \t/* All type bits clear means that we are ok with anything */\ndiff --git a/remote.c b/remote.c\nindex e2ef991..eacd8ad 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1595,7 +1595,7 @@ static int one_local_ref(const char *refname, const unsigned char *sha1, int fla\n \tint len;\n \n \t/* we already know it starts with refs/ to get here */\n-\tif (check_refname_format(refname + 5, 0))\n+\tif (check_refname_format(refname, 0))\n \t\treturn 0;\n \n \tlen = strlen(refname) + 1;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b69cf57..3fdfaea 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -929,6 +929,39 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n \t)\n '\n \n+test_expect_success 'push all hierarchies with stash' '\n+\tmk_empty &&\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo >>path1 &&\n+\tgit stash save \"Tweak path1\" &&\n+\tgit push testrepo \"refs/*:refs/*\" &&\n+\t# without fix to connect.c::check_ref(), the second push\n+\t# would fail.\n+\tgit push testrepo refs/stash &&\n+\tgit ls-remote . >expect &&\n+\tgit ls-remote testrepo >actual &&\n+\ttest_cmp actual expect\n+'\n+\n+test_expect_success 'fetch all hierarchies with stash' '\n+\tmk_empty &&\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo >>path1 &&\n+\tgit stash save \"Tweak path1\" &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit checkout HEAD^0 &&\n+\t\tgit fetch .. \"+refs/*:refs/*\" &&\n+\t\tgit checkout master\n+\t) &&\n+\tgit ls-remote . >expect &&\n+\tgit ls-remote testrepo >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'push --porcelain' '\n \tmk_empty &&\n \techo >.git/foo  \"To testrepo\" &&\n-- \n1.7.12.92.g7963261\n"},{"id":"197427","messageId":"1345484397-10958-3-git-send-email-gitster@pobox.com","threadId":"31295","inReplyTo":"1345484397-10958-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] get_fetch_map(): tighten checks on dest refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T17:39:57Z","receivedAt":"2012-08-20T17:39:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code to check the refname we store the fetched result locally did not\nbother checking the first 5 bytes of it, presumably assuming that it\nalways begin with \"refs/\".  For a fetch refspec (or the result of applying\nwildcard on one), we always want the RHS to map to something inside\n\"refs/\" hierarchy, so let's spell that rule out in a more explicit way.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n remote.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex eacd8ad..0f2b1db 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1402,8 +1402,8 @@ int get_fetch_map(const struct ref *remote_refs,\n \n \tfor (rmp = &ref_map; *rmp; ) {\n \t\tif ((*rmp)->peer_ref) {\n-\t\t\tif (check_refname_format((*rmp)->peer_ref->name + 5,\n-\t\t\t\tREFNAME_ALLOW_ONELEVEL)) {\n+\t\t\tif (prefixcmp((*rmp)->peer_ref->name, \"refs/\") ||\n+\t\t\t    check_refname_format((*rmp)->peer_ref->name, 0)) {\n \t\t\t\tstruct ref *ignore = *rmp;\n \t\t\t\terror(\"* Ignoring funny ref '%s' locally\",\n \t\t\t\t      (*rmp)->peer_ref->name);\n-- \n1.7.12.92.g7963261\n"},{"id":"197509","messageId":"50332E2D.2070602@alum.mit.edu","threadId":"31295","inReplyTo":"1345484397-10958-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-21T06:43:57Z","receivedAt":"2012-08-21T06:43:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/20/2012 07:39 PM, Junio C Hamano wrote:\n> There are a handful of places where we call check_refname_format() on a\n> substring after \"refs/\" of a refname we are going to use, and filter out a\n> valid match with \"refs/stash\" with such a pathspec.  Not sending a stash\n> may arguably be a feature (as stash is inherently a local workflow\n> element), but the code in the transport layer is oblivious to this\n> filtering performed by the lower layer of the code, and complains that the\n> other side did not send all the objects that needs to complete refs/stash\n> at the end, even though the code will not write refs/stash out anyway, and\n> making the whole command fail.\n> \n> This is an attempt to \"fix\" it by using check_refname_format() on the\n> whole \"refs/.....\" string and allowing refs/stash to be also copied.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/fetch-pack.c   |  2 +-\n>  builtin/receive-pack.c |  2 +-\n>  connect.c              |  8 ++------\n>  remote.c               |  2 +-\n>  t/t5516-fetch-push.sh  | 33 +++++++++++++++++++++++++++++++++\n>  5 files changed, 38 insertions(+), 9 deletions(-)\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 6207ecd..a3e3fa3 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n>  \tfor (ref = *refs; ref; ref = next) {\n>  \t\tnext = ref->next;\n>  \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n> -\t\t    check_refname_format(ref->name + 5, 0))\n> +\t\t    check_refname_format(ref->name, 0))\n>  \t\t\t; /* trash */\n>  \t\telse if (args.fetch_all &&\n>  \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\n\nI understand that you didn't introduce this code, but it seems like a\nsuspicious combination of conditions:\n\n    if ((ref->name starts with \"refs/\")\n        and (ref->name has invalid format))\n\nIt would seem more plausible to want\n\n    if ((ref->name starts with \"refs/\")\n        and (ref->name has *valid* format))\n\nor its logical inverse\n\n    if ((ref->name does not start with \"refs/\")\n        or (ref->name has invalid format))\n\nIf you know the purpose of the code here, maybe you could give it a\nquick sanity check.  (I believe the test was originally introduced by\nDscho in 1baaae5e1f9ac09aafe11b5aa4daa69bae68173b.)\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 7ec68a1..1935b80 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -401,7 +401,7 @@ static const char *update(struct command *cmd)\n>  \tstruct ref_lock *lock;\n>  \n>  \t/* only refs/... are allowed */\n> -\tif (prefixcmp(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> +\tif (prefixcmp(name, \"refs/\") || check_refname_format(name, 0)) {\n>  \t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n>  \t\treturn \"funny refname\";\n>  \t}\n> diff --git a/connect.c b/connect.c\n> index c8d0ea5..b30639c 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -17,20 +17,16 @@ static int check_ref(const char *name, int len, unsigned int flags)\n>  \tif (len < 5 || memcmp(name, \"refs/\", 5))\n>  \t\treturn 0;\n>  \n> -\t/* Skip the \"refs/\" part */\n> -\tname += 5;\n> -\tlen -= 5;\n> -\n>  \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n>  \tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n>  \t\treturn 0;\n>  \n>  \t/* REF_HEADS means that we want regular branch heads */\n> -\tif ((flags & REF_HEADS) && !memcmp(name, \"heads/\", 6))\n> +\tif ((flags & REF_HEADS) && !memcmp(name, \"refs/heads/\", 11))\n>  \t\treturn 1;\n>  \n>  \t/* REF_TAGS means that we want tags */\n> -\tif ((flags & REF_TAGS) && !memcmp(name, \"tags/\", 5))\n> +\tif ((flags & REF_TAGS) && !memcmp(name, \"refs/tags/\", 10))\n>  \t\treturn 1;\n>  \n>  \t/* All type bits clear means that we are ok with anything */\n> diff --git a/remote.c b/remote.c\n> index e2ef991..eacd8ad 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1595,7 +1595,7 @@ static int one_local_ref(const char *refname, const unsigned char *sha1, int fla\n>  \tint len;\n>  \n>  \t/* we already know it starts with refs/ to get here */\n> -\tif (check_refname_format(refname + 5, 0))\n> +\tif (check_refname_format(refname, 0))\n>  \t\treturn 0;\n>  \n>  \tlen = strlen(refname) + 1;\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index b69cf57..3fdfaea 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -929,6 +929,39 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'push all hierarchies with stash' '\n> +\tmk_empty &&\n> +\tgit stash clear &&\n> +\tgit reset --hard &&\n> +\techo >>path1 &&\n> +\tgit stash save \"Tweak path1\" &&\n> +\tgit push testrepo \"refs/*:refs/*\" &&\n> +\t# without fix to connect.c::check_ref(), the second push\n> +\t# would fail.\n> +\tgit push testrepo refs/stash &&\n> +\tgit ls-remote . >expect &&\n> +\tgit ls-remote testrepo >actual &&\n> +\ttest_cmp actual expect\n> +'\n> +\n> +test_expect_success 'fetch all hierarchies with stash' '\n> +\tmk_empty &&\n> +\tgit stash clear &&\n> +\tgit reset --hard &&\n> +\techo >>path1 &&\n> +\tgit stash save \"Tweak path1\" &&\n> +\t(\n> +\t\tcd testrepo &&\n> +\t\tgit commit --allow-empty -m initial &&\n> +\t\tgit checkout HEAD^0 &&\n> +\t\tgit fetch .. \"+refs/*:refs/*\" &&\n> +\t\tgit checkout master\n> +\t) &&\n> +\tgit ls-remote . >expect &&\n> +\tgit ls-remote testrepo >actual &&\n> +\ttest_cmp actual expect\n> +'\n> +\n>  test_expect_success 'push --porcelain' '\n>  \tmk_empty &&\n>  \techo >.git/foo  \"To testrepo\" &&\n> \n\nOtherwise I see nothing wrong with your changes.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197539","messageId":"7vpq6kdu31.fsf@alter.siamese.dyndns.org","threadId":"31295","inReplyTo":"50332E2D.2070602@alum.mit.edu","subject":"Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T17:37:54Z","receivedAt":"2012-08-21T17:37:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n>> index 6207ecd..a3e3fa3 100644\n>> --- a/builtin/fetch-pack.c\n>> +++ b/builtin/fetch-pack.c\n>> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n>>  \tfor (ref = *refs; ref; ref = next) {\n>>  \t\tnext = ref->next;\n>>  \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n>> -\t\t    check_refname_format(ref->name + 5, 0))\n>> +\t\t    check_refname_format(ref->name, 0))\n>>  \t\t\t; /* trash */\n>>  \t\telse if (args.fetch_all &&\n>>  \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\n>\n> I understand that you didn't introduce this code, but it seems like a\n> suspicious combination of conditions:\n>\n>     if ((ref->name starts with \"refs/\")\n>         and (ref->name has invalid format))\n\nThis protects us from getting contaminated by bogus ref under refs/\nwhen running \"fetch refs/heads/*:refs/remotes/origin/*\" no?\n\nThe remote side can also throw phony \"I have this object, too, but\nnot at a particular ref---this entry is only to let you know I have\nit, so that we can negotiate minimal transfer better\" entries that\nare labelled with strings that do not begin with \"refs/\" and do not\npass check_refname_format() (and because they are not refs, they do\nnot have to pass the test) at us, and we do not want to filter them\nout in this function.  But we do not want anything that is malformed\nunder \"refs/\".\n"},{"id":"197573","messageId":"50348C97.4040606@alum.mit.edu","threadId":"31295","inReplyTo":"7vpq6kdu31.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-22T07:39:03Z","receivedAt":"2012-08-22T07:39:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/21/2012 07:37 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n>>> index 6207ecd..a3e3fa3 100644\n>>> --- a/builtin/fetch-pack.c\n>>> +++ b/builtin/fetch-pack.c\n>>> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n>>>  \tfor (ref = *refs; ref; ref = next) {\n>>>  \t\tnext = ref->next;\n>>>  \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n>>> -\t\t    check_refname_format(ref->name + 5, 0))\n>>> +\t\t    check_refname_format(ref->name, 0))\n>>>  \t\t\t; /* trash */\n>>>  \t\telse if (args.fetch_all &&\n>>>  \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\n>>\n>> I understand that you didn't introduce this code, but it seems like a\n>> suspicious combination of conditions:\n>>\n>>     if ((ref->name starts with \"refs/\")\n>>         and (ref->name has invalid format))\n> \n> This protects us from getting contaminated by bogus ref under refs/\n> when running \"fetch refs/heads/*:refs/remotes/origin/*\" no?\n> \n> The remote side can also throw phony \"I have this object, too, but\n> not at a particular ref---this entry is only to let you know I have\n> it, so that we can negotiate minimal transfer better\" entries that\n> are labelled with strings that do not begin with \"refs/\" and do not\n> pass check_refname_format() (and because they are not refs, they do\n> not have to pass the test) at us, and we do not want to filter them\n> out in this function.  But we do not want anything that is malformed\n> under \"refs/\".\n\nThanks for the explanation.  I'm trying to dig some more into this so\nthat I can add some documentation, because this area of the code is\nrather obscure.\n\nHere is the loop being discussed, in full (from builtin/fetch-pack.c,\nfilter_refs()):\n\n> \tfor (ref = *refs; ref; ref = next) {\n> \t\tnext = ref->next;\n> \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n> \t\t    check_refname_format(ref->name, 0))\n> \t\t\t; /* trash */\n> \t\telse if (args.fetch_all &&\n> \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\n> \t\t\t*newtail = ref;\n> \t\t\tref->next = NULL;\n> \t\t\tnewtail = &ref->next;\n> \t\t\tcontinue;\n> \t\t}\n> \t\telse {\n> \t\t\tint i;\n> \t\t\tfor (i = 0; i < nr_match; i++) {\n> \t\t\t\tif (!strcmp(ref->name, match[i])) {\n> \t\t\t\t\tmatch[i][0] = '\\0';\n> \t\t\t\t\treturn_refs[i] = ref;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t}\n> \t\t\tif (i < nr_match)\n> \t\t\t\tcontinue; /* we will link it later */\n> \t\t}\n> \t\tfree(ref);\n> \t}\n\nEmpirically (determined by instrumenting the code and running the git\ntest suite):\n\n* The first branch of the if statement is only executed for ref->name of\nthe form \"refs/tags/foo^{}\" for various \"foo\".\n\n* The second branch of the if is *never* executed.\n\n* The third branch is invoked for various reference names under \"refs/\"\n(including oddballs like \"refs/for/refs/heads/master\", \"refs/stash\",\n\"refs/replace/<SHA1>\"), and also for \"HEAD\".\n\nThis doesn't quite agree with your explanation, because the phony refs\n(at least in this dataset) *do* start with \"refs/\" and they *are* trashed.\n\nI'll continue to try to figure out this area.  I already found an\napparent memory leak...\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197577","messageId":"7v7gsrb1xs.fsf@alter.siamese.dyndns.org","threadId":"31295","inReplyTo":"50348C97.4040606@alum.mit.edu","subject":"Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T11:28:47Z","receivedAt":"2012-08-22T11:28:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n>> The remote side can also throw phony \"I have this object, too, but\n>> not at a particular ref---this entry is only to let you know I have\n>> it, so that we can negotiate minimal transfer better\" entries that\n>> are labelled with strings that do not begin with \"refs/\" and do not\n>> pass check_refname_format() (and because they are not refs, they do\n>> not have to pass the test) at us, and we do not want to filter them\n>> out in this function.  But we do not want anything that is malformed\n>> under \"refs/\".\n>\n> Thanks for the explanation.  I'm trying to dig some more into this so\n> that I can add some documentation, because this area of the code is\n> rather obscure.\n>\n> Here is the loop being discussed, in full (from builtin/fetch-pack.c,\n> filter_refs()):\n> ...\n> Empirically (determined by instrumenting the code and running the git\n> test suite):\n>\n> * The first branch of the if statement is only executed for ref->name of\n> the form \"refs/tags/foo^{}\" for various \"foo\".\n\nWe do not want \"fetch --mirror\" and \"refs/*:refs/*\" to add a tag\nwhose name is \"foo^{}\" to us.\n\n> * The second branch of the if is *never* executed.\n\nI am not familiar with (nor particularly interested in) the \"args.depth\"\ncode, so I have no comment on this part offhand.\n\n> * The third branch is invoked for various reference names under \"refs/\"\n> (including oddballs like \"refs/for/refs/heads/master\", \"refs/stash\",\n> \"refs/replace/<SHA1>\"), and also for \"HEAD\".\n>\n> This doesn't quite agree with your explanation, because the phony refs\n> (at least in this dataset) *do* start with \"refs/\" and they *are* trashed.\n\nTry fetching from a repository that has an alternate, and you would\nsee those \".have\" phoney refs.\n\nBut yes, they are trashed as well, as they are not likely to match,\nso you are right; the \".have\" entries are red-herring (they have\nalready been used before the caller calls this function for their\nsole purpose of marking the other side has).\n"},{"id":"197595","messageId":"7vlih6ams7.fsf@alter.siamese.dyndns.org","threadId":"31295","inReplyTo":"7v7gsrb1xs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T16:56:08Z","receivedAt":"2012-08-22T16:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> ...\n>> * The second branch of the if is *never* executed.\n> \t\telse if (args.fetch_all &&\n> \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\n> \t\t\t*newtail = ref;\n> \t\t\tref->next = NULL;\n> \t\t\tnewtail = &ref->next;\n> \t\t\tcontinue;\n> \t\t}\n>\n> I am not familiar with (nor particularly interested in) the \"args.depth\"\n> code, so I have no comment on this part offhand.\n\nThis is \"fetch --all\" case.  I think the original intent was to\nfetch all branches (hence exclusion of tags) so the prefixcmp() may\nnot be correct (!prefixcmp() with \"refs/heads\" would be more kosher)\nthese days.  I am not sure what !args.depth check wants to do here\n(that is, I can see it wants to defeat \"fetch --all\" and instead\nmake it go through the usual refspec matching codepath, but I do not\nknow why we thought that is a good thing to do here).\n"}]}