{"thread":{"id":"30385","subject":"[BUG] \"fetch $there +refs/*:refs/*\" fails if there is a stash","startedAt":"2012-05-01T22:19:07Z","lastAt":"2012-05-15T15:19:01Z","messageCount":10,"participants":["Junio C Hamano","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"190500","messageId":"7vsjfj7des.fsf@alter.siamese.dyndns.org","threadId":"30385","inReplyTo":null,"subject":"[BUG] \"fetch $there +refs/*:refs/*\" fails if there is a stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-01T22:19:07Z","receivedAt":"2012-05-01T22:19:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This ought to work:\n\n    $ git checkout HEAD^0 ;# make sure we are on detached HEAD\n    $ git fetch $somewhere +refs/*:refs/*\n\nif you want to pull down all the branches from somewhere, potentially\nnuking the refs we currently have.\n\nHowever, if $somewhere has a stash, i.e. refs/stash, even though our\n\"fetch\" sees it in the \"ls-remote $somewhere\" output and tries to make\nsure that the object comes over the wire, we never show \"want\" line for\nrefs/stash, because we silently drop it with check_ref_format().\n\nI have run out concentration before digging this through, but the attached\nsingle liner patch fixes it.  The thing is, this function is given a list\nof refs and drops refs/stash (which is not what I want in the context of\nthe above +refs/*:refs/*), and modifies the caller's list of refs, and\nthen the caller (i.e. do_fetch_pack() that called everything_local()) then\nuses updated list (i.e. without refs/stash) to run find_common() and\nget_pack(), but the layer higher above (namely, do_fetch() that calls\nfetch_refs() that in turn calls store_updated_refs(), all in\nbuiltin/fetch.c) is _not_ aware of this filtering and expects that the\ncode at this layer *must* have asked for and obtained all the objects\nreachable from what was listed in ls-remote output, leading to an\ninconsistent state.\n\n[Michael CC'ed as he seems to be dead set tightening check_ref_format()]\n\nThe specific failure of \"refs/stash\" is fixed with this patch, but I think\nthis call to check_ref_format() in the filter_refs() should be removed.\nThe function is trying to see what we _asked_ against what the remote side\nsaid they have, and if we tried to recover objects from a broken remote by\ndoing:\n\n\tgit fetch $somewhere refs/heads/a..b:refs/heads/a_dot_dot_b\n\nand the remote side advertised that it has such a ref (note that a..b is\nan illegal path component), we should be able to do so without a misguided\ncall to check_refname_format() getting in the way.\n\nIt is the same story if the name advertised by a broken remote were\n\"refs/head/../heads/master\".  As long as the RHS of the refspec\n(i.e. refs/heads/a_dot_dot_b) is kosher, we must allow such a request to\nsucceed, so that people can interoperate in less than perfect world.\n\n builtin/fetch-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 7124c4b..34cd8a5 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -541,7 +541,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"},{"id":"190531","messageId":"4FA0FB4B.8080503@alum.mit.edu","threadId":"30385","inReplyTo":"7vsjfj7des.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] \"fetch $there +refs/*:refs/*\" fails if there is a stash","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-02T09:15:55Z","receivedAt":"2012-05-02T09:15:55Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/02/2012 12:19 AM, Junio C Hamano wrote:\n> This ought to work:\n>\n>      $ git checkout HEAD^0 ;# make sure we are on detached HEAD\n>      $ git fetch $somewhere +refs/*:refs/*\n>\n> if you want to pull down all the branches from somewhere, potentially\n> nuking the refs we currently have.\n>\n> However, if $somewhere has a stash, i.e. refs/stash, even though our\n> \"fetch\" sees it in the \"ls-remote $somewhere\" output and tries to make\n> sure that the object comes over the wire, we never show \"want\" line for\n> refs/stash, because we silently drop it with check_ref_format().\n\nAs you obviously know (but for the information of other readers), the \nreason for the failure is that \"stash\" (not \"refs/stash\") is passed to \ncheck_ref_format().  \"stash\" is not a valid refname because it has only \na single level (i.e., does not contain a '/').  check_ref_format() would \naccept the refname if it were passed the REFNAME_ALLOW_ONELEVEL option, \nbut passing it the full refname (as your patch does) is better.\n\n> I have run out concentration before digging this through, but the attached\n> single liner patch fixes it.  The thing is, this function is given a list\n> of refs and drops refs/stash (which is not what I want in the context of\n> the above +refs/*:refs/*), and modifies the caller's list of refs, and\n> then the caller (i.e. do_fetch_pack() that called everything_local()) then\n> uses updated list (i.e. without refs/stash) to run find_common() and\n> get_pack(), but the layer higher above (namely, do_fetch() that calls\n> fetch_refs() that in turn calls store_updated_refs(), all in\n> builtin/fetch.c) is _not_ aware of this filtering and expects that the\n> code at this layer *must* have asked for and obtained all the objects\n> reachable from what was listed in ls-remote output, leading to an\n> inconsistent state.\n>\n> [Michael CC'ed as he seems to be dead set tightening check_ref_format()]\n>\n> The specific failure of \"refs/stash\" is fixed with this patch, but I think\n> this call to check_ref_format() in the filter_refs() should be removed.\n> The function is trying to see what we _asked_ against what the remote side\n> said they have, and if we tried to recover objects from a broken remote by\n> doing:\n>\n> \tgit fetch $somewhere refs/heads/a..b:refs/heads/a_dot_dot_b\n >\n> and the remote side advertised that it has such a ref (note that a..b is\n> an illegal path component), we should be able to do so without a misguided\n> call to check_refname_format() getting in the way.\n\nI agree; if the remote reference name never gets used as a local \nreference name, there is no reason to call check_ref_format() on it at \nall.  The local reference name that is derived from the remote reference \nname (even if it is derived via an identity operation) *should* be \nchecked using check_ref_format(); presumably that is already the case? \nOptimally the local refnames should be checked *before* the transfer to \navoid wasting the user's time.\n\n> It is the same story if the name advertised by a broken remote were\n> \"refs/head/../heads/master\".  As long as the RHS of the refspec\n> (i.e. refs/heads/a_dot_dot_b) is kosher, we must allow such a request to\n> succeed, so that people can interoperate in less than perfect world.\n\nA slightly more awkward question is if the broken remote advertises many \nreferences including \"refs/head/../heads/master\" and we fetch refspec \n\"+refs/*:refs/*\".  In this case it is pretty clear that we should fetch \nall of the valid references; otherwise, working around the problem would \nbe quite awkward.  But what to do about \"refs/head/../heads/master\"?  I \nthink we should emit a warning naming the reference that will not be \nfetched \"because it is formatted incorrectly\", and not include it in the \n\"want\" lines.  If the user really wants the reference, he can fetch it \nto another name using an explicit \"from:to\" refspec.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"190573","messageId":"7vy5paim5r.fsf@alter.siamese.dyndns.org","threadId":"30385","inReplyTo":"4FA0FB4B.8080503@alum.mit.edu","subject":"Re: [BUG] \"fetch $there +refs/*:refs/*\" fails if there is a stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-02T16:26:56Z","receivedAt":"2012-05-02T16:26:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 05/02/2012 12:19 AM, Junio C Hamano wrote:\n>\n>> The specific failure of \"refs/stash\" is fixed with this patch, but I think\n>> this call to check_ref_format() in the filter_refs() should be removed.\n>> The function is trying to see what we _asked_ against what the remote side\n>> said they have, and if we tried to recover objects from a broken remote by\n>> doing:\n>>\n>> \tgit fetch $somewhere refs/heads/a..b:refs/heads/a_dot_dot_b\n>>\n>> and the remote side advertised that it has such a ref (note that a..b is\n>> an illegal path component), we should be able to do so without a misguided\n>> call to check_refname_format() getting in the way.\n>\n> I agree; if the remote reference name never gets used as a local\n> reference name, there is no reason to call check_ref_format() on it at\n> all.  The local reference name that is derived from the remote\n> reference name (even if it is derived via an identity operation)\n> *should* be checked using check_ref_format(); presumably that is\n> already the case?\n\nWe should make sure the ref the refspec mapping told us to update is a\nvalid ref locally (didn't I say that), but I do not know offhand if we do\nso using check_ref_format() and/or if the check is not overly loose.\n\nHonestly, I didn't expect _you_ to be _asking_ that question; as you are\nthe person who has been advocating tightening of the function, you would\nknow the callers of the function better than anybody, no?\n\n>> It is the same story if the name advertised by a broken remote were\n>> \"refs/head/../heads/master\".  As long as the RHS of the refspec\n>> (i.e. refs/heads/a_dot_dot_b) is kosher, we must allow such a request to\n>> succeed, so that people can interoperate in less than perfect world.\n>\n> A slightly more awkward question is if the broken remote advertises\n> many references including \"refs/head/../heads/master\" and we fetch\n> refspec \"+refs/*:refs/*\".  In this case it is pretty clear that we\n> should fetch all of the valid references; otherwise, working around\n> the problem would be quite awkward.\n\nIf we ignore what will be stored in FETCH_HEAD, we could either (1) fetch\nhistories leading to all the valid commits, but not store incorrectly\nformatted refs, or (2) fetch histories leading to commits that will be\nstored in only the valid _refs_.\n\nBut because we cannot ignore FETCH_HEAD, we have to do (1).  That is,\nhistory leading to any ref the remote advertises that matches the LHS of\nthe refspec has to be fetched, and listed in the resulting FETCH_HEAD.\nSome of them may map to invalid refs on the local side, and we do not\nstore it inside our refs/, so that they do not pollute our local ref\nnamespace.\n\n> But what to do about\n> \"refs/head/../heads/master\"?  I think we should emit a warning naming\n> the reference that will not be fetched \"because it is formatted\n> incorrectly\", and not include it in the \"want\" lines.  \n\nWe have to ask for it, so we include it \"want\".  Remember, $A without\ncolon is a short-hand for $A: (fetch but not store locally), and as long\nas $A (the LHS of the refspec) matches the refs advertised by the remote\nside, we fetch it and list it in FETCH_HEAD.\n\nAs the user is _not_ asking to store it locally in our refs/ namespace, we\nwon't store it.  But if the request were $A:$A for any invalid refname $A,\nthe story is the same.  We fetch it because LHS of refspec matches what\nthey advertise, and we list it in FETCH_HEAD.  We do _not_ store it in our\nlocal refs/ space, because RHS of refspec is invalid.\n"},{"id":"190806","messageId":"7vwr4r8tpm.fsf@alter.siamese.dyndns.org","threadId":"30385","inReplyTo":"7vsjfj7des.fsf@alter.siamese.dyndns.org","subject":"[PATCH] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-04T22:30:45Z","receivedAt":"2012-05-04T22:30:45Z","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\n * With this patch:\n\n    $ git checkout HEAD^0 ;# make sure we are on detached HEAD\n    $ git fetch $somewhere +refs/*:refs/*\n\n   and\n\n    victim$ git config receive.denyCurrentBranch warn\n    master$ git push victim +refs/*:refs/*\n\n   should work.\n\n builtin/fetch-pack.c   |  2 +-\n builtin/receive-pack.c |  2 +-\n remote.c               |  2 +-\n t/t5516-fetch-push.sh  | 30 ++++++++++++++++++++++++++++++\n 4 files changed, 33 insertions(+), 3 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/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..1a45b19 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -929,6 +929,36 @@ 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+\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.10.1.500.g37b1e9a\n"},{"id":"190807","messageId":"7vsjff8ti1.fsf_-_@alter.siamese.dyndns.org","threadId":"30385","inReplyTo":"7vwr4r8tpm.fsf@alter.siamese.dyndns.org","subject":"[PATCH] get_fetch_map(): tighten checks on dest refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-04T22:35:18Z","receivedAt":"2012-05-04T22:35:18Z","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.10.1.500.g37b1e9a\n"},{"id":"190826","messageId":"4FA4C2CC.7080205@alum.mit.edu","threadId":"30385","inReplyTo":"7vwr4r8tpm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fetch/push: allow refs/*:refs/*","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-05T06:03:56Z","receivedAt":"2012-05-05T06:03:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/05/2012 12:30 AM, 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>\n>   * With this patch:\n>\n>      $ git checkout HEAD^0 ;# make sure we are on detached HEAD\n>      $ git fetch $somewhere +refs/*:refs/*\n>\n>     and\n>\n>      victim$ git config receive.denyCurrentBranch warn\n>      master$ git push victim +refs/*:refs/*\n>\n>     should work.\n>\n>   builtin/fetch-pack.c   |  2 +-\n>   builtin/receive-pack.c |  2 +-\n>   remote.c               |  2 +-\n>   t/t5516-fetch-push.sh  | 30 ++++++++++++++++++++++++++++++\n>   4 files changed, 33 insertions(+), 3 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\nThe patch looks fine to me.\n\nThis combination \"!memcmp(ref->name, \"refs/\", 5) && \ncheck_refname_format(ref->name, 0)\" is the reason that I suggested \nadding a REFNAME_FULL option [1], in which case it could be written \n\"check_refname_format(ref->name, REFNAME_FULL)\".  However, now I think \nthat the options should be constructed a little differently:\n\nflags==0: Require refname to start with \"refs/\"\n\nflags==REFNAME_ALLOW_SPECIAL: Also accept single-level ALL_CAPS refnames.\n\nflags==REFNAME_ALLOW_PARTIAL: Don't check the namespace or require '/'). \n  This could be used for checking partial names like \"master\" as \nshorthand for \"refs/master\".\n\nDoes this sound reasonable to you?\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/196632\n\n>   \t\t\t; /* trash */\n>   \t\telse if (args.fetch_all&&\n>   \t\t\t(!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\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/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..1a45b19 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -929,6 +929,36 @@ 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> +\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\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"190859","messageId":"4FA56FFC.9080201@alum.mit.edu","threadId":"30385","inReplyTo":"4FA4C2CC.7080205@alum.mit.edu","subject":"Re: [PATCH] fetch/push: allow refs/*:refs/*","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-05T18:22:52Z","receivedAt":"2012-05-05T18:22:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/05/2012 08:03 AM, Michael Haggerty wrote:\n> On 05/05/2012 12:30 AM, 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\n>> 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\n>> 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,\n>> 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>>\n>> * With this patch:\n>>\n>> $ git checkout HEAD^0 ;# make sure we are on detached HEAD\n>> $ git fetch $somewhere +refs/*:refs/*\n>>\n>> and\n>>\n>> victim$ git config receive.denyCurrentBranch warn\n>> master$ git push victim +refs/*:refs/*\n>>\n>> should work.\n>>\n>> builtin/fetch-pack.c | 2 +-\n>> builtin/receive-pack.c | 2 +-\n>> remote.c | 2 +-\n>> t/t5516-fetch-push.sh | 30 ++++++++++++++++++++++++++++++\n>> 4 files changed, 33 insertions(+), 3 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\n>> nr_match, char **match)\n>> for (ref = *refs; ref; ref = next) {\n>> next = ref->next;\n>> if (!memcmp(ref->name, \"refs/\", 5)&&\n>> - check_refname_format(ref->name + 5, 0))\n>> + check_refname_format(ref->name, 0))\n>\n> The patch looks fine to me.\n>\n> This combination \"!memcmp(ref->name, \"refs/\", 5) &&\n> check_refname_format(ref->name, 0)\" is the reason that I suggested\n> adding a REFNAME_FULL option [1], in which case it could be written\n> \"check_refname_format(ref->name, REFNAME_FULL)\". However, now I think\n> that the options should be constructed a little differently:\n>\n> flags==0: Require refname to start with \"refs/\"\n>\n> flags==REFNAME_ALLOW_SPECIAL: Also accept single-level ALL_CAPS refnames.\n>\n> flags==REFNAME_ALLOW_PARTIAL: Don't check the namespace or require '/').\n> This could be used for checking partial names like \"master\" as shorthand\n> for \"refs/master\".\n>\n> Does this sound reasonable to you?\n\nOops; I just realized that this particular example doesn't check \"starts \nwith \"refs/\" and valid refname\" as expected.  In fact it checks \"starts \nwith \"refs/\" and INVALID refname\".  The proposed REFNAME_FULL option \nwould of course be equivalent to the former.  (Why the latter is needed \nhere is something that needs a larger investment of time to figure out.)\n\nThe other examples that you patched do the expected combination of checks.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"191008","messageId":"7vobq06jt7.fsf@alter.siamese.dyndns.org","threadId":"30385","inReplyTo":"4FA4C2CC.7080205@alum.mit.edu","subject":"Re: [PATCH] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-07T16:24:20Z","receivedAt":"2012-05-07T16:24:20Z","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> This combination \"!memcmp(ref->name, \"refs/\", 5) &&\n> check_refname_format(ref->name, 0)\" is the reason that I suggested\n> adding a REFNAME_FULL option [1], in which case it could be written\n> \"check_refname_format(ref->name, REFNAME_FULL)\".\n\nI personally think that check-refname-format should lose its second\nparameter and always check for a concrete full refs, so that we can\ntighten the current allow-onelevel case a bit further (i.e. \".git/HEAD\" is\nOK at the root level, but \".git/refs/heads/HEAD\" is asking for trouble,\nand \".git/config\" is downright confusing. The function does not get a good\nenough clue by only ONELEVEL).  That would mean REFNAME_FULL will not be\nnecessary---it should be the only mode of operation, and the callers need\nto be adjusted accordingly.\n\nWe should also introduce another function check-refspec-half-format to be\nused for the checks for left and right halves of refspec (\"refs/heads/*\"\nis OK but \"refs/heads*\" is not, perhaps).\n\nA single function trying to do both and not doing a good job in either\ncase is not a pretty picture.\n"},{"id":"191540","messageId":"4FB218AA.2060006@alum.mit.edu","threadId":"30385","inReplyTo":"7vobq06jt7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fetch/push: allow refs/*:refs/*","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-15T08:49:46Z","receivedAt":"2012-05-15T08:49:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2012 06:24 PM, Junio C Hamano wrote:\n> Michael Haggerty<mhagger@alum.mit.edu>  writes:\n>\n>> This combination \"!memcmp(ref->name, \"refs/\", 5)&&\n>> check_refname_format(ref->name, 0)\" is the reason that I suggested\n>> adding a REFNAME_FULL option [1], in which case it could be written\n>> \"check_refname_format(ref->name, REFNAME_FULL)\".\n>\n> I personally think that check-refname-format should lose its second\n> parameter and always check for a concrete full refs, so that we can\n> tighten the current allow-onelevel case a bit further (i.e. \".git/HEAD\" is\n> OK at the root level, but \".git/refs/heads/HEAD\" is asking for trouble,\n> and \".git/config\" is downright confusing. The function does not get a good\n> enough clue by only ONELEVEL).  That would mean REFNAME_FULL will not be\n> necessary---it should be the only mode of operation, and the callers need\n> to be adjusted accordingly.\n>\n> We should also introduce another function check-refspec-half-format to be\n> used for the checks for left and right halves of refspec (\"refs/heads/*\"\n> is OK but \"refs/heads*\" is not, perhaps).\n>\n> A single function trying to do both and not doing a good job in either\n> case is not a pretty picture.\n\nWhether the functionality is presented via one function with multiple \noptions or multiple functions with no options is of secondary \nimportance.  Let's first worry about the desired functionality, since \neven that is not clear.\n\n\nThere's a long weekend coming up in Germany so hopefully I'll have \nanother increment of time to work on this.  I'd like to get feedback on \nthe requirements.  First let me present a summary of what I know about \nrefnames in git.\n\n\nThere are a few categories of refname that come up in the git code:\n\n* Full refnames like \"refs/foo\" or \"refs/bar/baz\" (starting with \"refs/\").\n\n* Top-level special ALL_CAPS reference names like \"MERGE_HEAD\".\n\n* Refspec-style patterns with wildcards, like \"refs/heads/*\" or \n\"refs/foo/*/bar\".\n\n* Short branch names with an implied \"refs/heads/\" prefix omitted; e.g., \n\"master\" meaning \"refs/heads/master\".\n\n* Short tag names with an implied \"refs/tags/\" prefix omitted; e.g., \n\"v1.2.3\" meaning \"refs/tags/v1.2.3\".\n\n* Maybe others...?\n\n\nCurrently, I believe that code usually handles the short branch/tag \nnames via one of two mechanisms:\n\n* Explicitly prepend \"refs/heads/\" or \"refs/tags/\" to the short name to \nmake the corresponding full name, then work exclusively with the full name.\n\n* Use the ref_rev_parse_rules based functions like dwim_ref() to guess \nwhich prefix can be applied to a short refname to make it into a full \nrefname, then work exclusively with the full name.\n\nIf code will continue to work this way, then there might be no use for a \nfunction that can check short refnames; one would always convert the \nshort refname into a full refname and check the latter.  On the other \nhand, a short refname check might do special things like *reject* a name \nthat starts with \"refs/\".\n\n\nIn addition to the strict rules governing refnames, there are other \npolicies that are enforced at varying levels of strictness or that might \nbe helpful in the future; for example,\n\n* Refnames either have to start with \"refs/\" or have to be ALL_CAPS.\n\n* Refnames cannot be directly under \"refs/\" but should be in a namespace \nlike \"refs/heads/\".  (This is implied by the old check-ref-format \ndocumentation:\n\n> . They must contain at least one `/`. This enforces the presence of a\n>   category like `heads/`, `tags/` etc. but the actual names are not\n>   restricted.\n\nThis sentence also implies that check-ref-format was intended to be used \nfor refnames shortened by having the \"refs/\" prefix omitted, which is \ncurious.)\n\n* Unlikely refnames like \"refs/heads/HEAD\" could be handled with \nsuspicion, as you suggest.  I would also nominate constructs like \n\"refs/heads/refs/heads/mybranch\", which I've seen cause confusion at $WORK.\n\n\nIn terms of implementation:\n\n* We want to distinguish between what is acceptable for new refnames \nversus refnames that already exist in the local repository and/or \nrefnames received from a remote repo.  So there should be some kind of \n\"REFNAME_RELAXED\" option.\n\n* It is important for the \"old\" check_refname_format() and the \"new\" one \nto coexist during the transition.  This is simply because I don't want \nto have to analyze and rewrite all ~30 callers of check_refname_format() \nin one big bang.  I suggest retaining a copy as \ncheck_refname_format_legacy() that is deprecated and removed after all \ncallers have been rewritten.\n\n* The exact current behavior of \"git check-ref-format\" will probably not \nmap well to the new check_refname_format(), and is anyway inadequate and \nsomewhat inconsistent.  We need to decide how strict we need to be about \nbackwards-compatibility of this command.\n\n* I've been using \"git check-ref-format\" for testing \ncheck_refname_format().  But I think it would be a bad idea to expose \nall of the check_refname_format() options and variants in an installed \nplumbing command.  Therefore I think that there should be a program that \ncan be used to call check_refname_format() with all of its option \ncombinations to be used only for testing purposes.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"191551","messageId":"7vpqa5fp5m.fsf@alter.siamese.dyndns.org","threadId":"30385","inReplyTo":"4FB218AA.2060006@alum.mit.edu","subject":"Re: [PATCH] fetch/push: allow refs/*:refs/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-15T15:19:01Z","receivedAt":"2012-05-15T15:19:01Z","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> * Full refnames like \"refs/foo\" or \"refs/bar/baz\" (starting with \"refs/\").\n\nWhen reading any of these out of existing set of ref candidates (i.e. read\nfrom readdir() recursing down from \".git/refs\"), we shouldn't be rejecting\nwith format checks, but we may want to warn.\n\nWhen creating, we have full refnames in this case, so we can just check.\n\n> * Top-level special ALL_CAPS reference names like \"MERGE_HEAD\".\n\nSimilar.  These are \"full refname\" as far as ref format checking code is\nconcerned, and the format logic allows only ALL_CAPS at the top-level.\nWhen reading any single-level out of existing set of ref candidates\n(i.e. read from readdir() scanning \".git/\"), malformed candidates should\nbe ignored without even warning (otherwise you will incorrectly complain\nupon seeing \".git/config\").  When creating, we have full refnames in this\ncase, so we can just check.\n\n> * Refspec-style patterns with wildcards, like \"refs/heads/*\" or\n> \"refs/foo/*/bar\".\n\nAs I said in the message you are replying to, the format check for these\nshould be a separate function (which might happen to share some code with\nref, but that is an implementation detail).\n\n> * Short branch names with an implied \"refs/heads/\" prefix omitted;\n> e.g., \"master\" meaning \"refs/heads/master\".\n>\n> * Short tag names with an implied \"refs/tags/\" prefix omitted; e.g.,\n> \"v1.2.3\" meaning \"refs/tags/v1.2.3\".\n\nChecking for the above two classes should conceptually happen with the\nimplied prefix.  When creating (i.e. git branch/git checkout -b/git tag),\nyou know what full refname the thing will become if you allow it to be\ncreated.\n\nAs an optimization measure at the implementation level, being able to\ncheck \"frotz\", knowing that the calling code wants to know that the name\nis usable as a branch name, without having to do an allocation to hold\n\"refs/heads/frotz\" somewhere to call the format-check function, options\nlike allow-onelevel is sometimes useful, but at the conceptual level it is\nnot strictly necessary.\n\n> Currently, I believe that code usually handles the short branch/tag\n> names via one of two mechanisms:\n>\n> * Explicitly prepend \"refs/heads/\" or \"refs/tags/\" to the short name\n> to make the corresponding full name, then work exclusively with the\n> full name.\n\nConceptually everybody should be doing that.\n\n> * Use the ref_rev_parse_rules based functions like dwim_ref() to guess\n> which prefix can be applied to a short refname to make it into a full\n> refname, then work exclusively with the full name.\n\nThat is merely _matching_ with existing name, no?  Once you see an\nexisting name, you shouldn't be _rejecting_ anything.  If you see a\nquestionable name, you may warn.\n\nI am not sure if these \"two mechanisms\" is a proper characterization of\nthe problem space.  Shouldn't you be separating between reading and\ncreating instead?\n\n> * Refnames either have to start with \"refs/\" or have to be ALL_CAPS.\n\nSensible.\n\n> * Refnames cannot be directly under \"refs/\" but should be in a\n> namespace like \"refs/heads/\".  (This is implied by the old\n> check-ref-format documentation:\n\nAlmost, but beware of stashes.\n"}]}