{"thread":{"id":"33726","subject":"[PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","startedAt":"2013-05-04T23:55:42Z","lastAt":"2013-05-07T22:37:19Z","messageCount":39,"participants":["Johan Herland","Junio C Hamano","Bert Wesarg","Santi Béjar"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"216428","messageId":"1367711749-8812-1-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":null,"subject":"[PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:42Z","receivedAt":"2013-05-04T23:55:42Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The \"$remote/$branch\" syntax can be interpreted in two subtly different\nways:\n\n 1. A shorthand name for the remote-tracking branch corresponding to a\n specific $branch from a specific $remote.\n\n 2. A refname fragment, which - when appended to \"refs/remotes/\" -\n yields the remote-tracking branch corresponding to a specific\n $branch from a specific $remote.\n\nFor the current default/conventional refspecs, both interpretations are\ntrue and equally valid. However, when configuring non-default refspecs\n(such as the +refs/heads/*:refs/remotes/origin/heads/*), it becomes\nobvious that the current code follows the latter interpretation: The\n\"$remote/$branch\" shorthand will no longer work, and you are forced to\nuse \"$remote/heads/$branch\" instead.\n\nI argue that the former interpretation is what most users intuitively\nexpect, and that we should work towards making Git follow the same\ninterpretation:\n\nInstead of prepending \"refs/remotes/\" to convert \"$remote/$branch\" into\na full refname, we should find \"$remote\" in the repo config, and then\nmap \"refs/heads/$branch\" through $remote's fetch refspec(s), to find\nthe remote-tracking branch that properly corresponds to the specific\n$branch in the $remote repo.\n\nThis goal is achieved by the final patch in this series, and most of\nthe preceding patches serve as preliminary changes and refactoring to\nsupport this.\n\nPatch #1 is the exception in that it is a self-contained bugfix for a\nscanf-related problem I ran across while working on the patch series.\n\nPatches #2 and #3 introduce a new test intended to verify Git's\nusability when working with remote ref namespaces (which rely on\nsetting up unconventional refspecs). For now, this test is fairly\nthin, but it should be expanded as we find more problems with these\nkinds of setups.\n\nPatches #4 and #5 are pure refactorings to reorganize the code that\nexpands shorthand names to full refnames and vice versa. The idea\nis to associate the patterns that are used to expand/shorten ref\nnames with the actual function that does the expansion/shortening,\nso that we can later add patterns that uses different expand/shorten\nfunctions.\n\nPatch #6 teaches Git to realize when - in the context of communication\nwith a remote repo - it's expanding shorthand refs into either local\nrefnames, or remote refnames. It is important that any expansion rules\nrelying on local repo configuration are not allowed to expand shorthand\nnames on behalf of the remote repo.\n\nFinally, patch #7 introduces a new rule and associated expand/shorten\nfunctions mapping \"$remote/$branch\"-type shorthand names to/from their\nremote-tracking branch counterparts, by using the configured refspecs\nas described above. This rule is obviously only applied to local refs,\nas it would be wrong for a repo to use its local config to dictate a\nref expansion in a remote repo.\n\nThe series has been build on recent 'next', and although it also\napplies cleanly to v1.8.3-rc1, it will cause a test failure in\nt7900, since it depends on the jh/checkout-auto-tracking topic, which\nis currently cooking.\n\n\nHave fun! :)\n\n...Johan\n\n\nJohan Herland (7):\n shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin\n t7900: Start testing usability of namespaced remote refs\n t7900: Demonstrate failure to expand \"$remote/$branch\" according to refspecs\n refs.c: Refactor rules for expanding shorthand names into full refnames\n refs.c: Refactor code for shortening full refnames into shorthand names\n refname_match(): Caller must declare if we're matching local or remote refs\n refs.c: Add rules for resolving refs using remote refspecs\n\n cache.h | 4 -\n refs.c | 260 +++++++++++++++++--------\n refs.h | 14 ++\n remote.c | 15 +-\n t/t6300-for-each-ref.sh | 12 ++\n t/t7900-working-with-namespaced-remote-refs.sh | 133 +++++++++++++\n 6 files changed, 342 insertions(+), 96 deletions(-)\n create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh\n\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216430","messageId":"1367711749-8812-2-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:43Z","receivedAt":"2013-05-04T23:55:43Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When expanding shorthand refs to full ref names (e.g. in dwim_ref()),\nwe use the ref_rev_parse_rules list of expansion patterns. This list\nallows \"origin\" to be expanded into \"refs/remotes/origin/HEAD\", by\nusing the \"refs/remotes/%.*s/HEAD\" pattern from that list.\n\nshorten_unambiguous_ref() exists to provide the reverse operation:\nturning a full ref name into a shorter (but still unambiguous) name.\nIt does so by matching the given refname against each pattern from\nthe ref_rev_parse_rules list (in reverse), and extracting the short-\nhand name from the matching rule.\n\nHowever, when given \"refs/remotes/origin/HEAD\" it fails to shorten it\ninto \"origin\", because we misuse the sscanf() function when matching\n\"refs/remotes/origin/HEAD\" against \"refs/remotes/%.*s/HEAD\": We end\nup calling sscanf like this:\n\n sscanf(\"refs/remotes/origin/HEAD\", \"refs/remotes/%s/HEAD\", short_name)\n\nIn this case, sscanf() will match the initial \"refs/remotes/\" part, and\nthen match the remainder of the refname against the \"%s\", and place it\n(\"origin/HEAD\") into short_name. The part of the pattern following the\n\"%s\" format is never verified, because sscanf() apparently does not\nneed to do that (it has performed the one expected format extraction,\nand will return 1 correspondingly; see [1] for more details).\n\nThis patch replaces the misuse of sscanf() with a fairly simple function\nthat manually matches the refname against patterns, and extracts the\nshorthand name.\n\nAlso a testcase verifying \"refs/remotes/origin/HEAD\" -> \"origin\" has\nbeen added.\n\n[1]: If we assume that sscanf() does not do a verification pass prior\nto format extraction, there is AFAICS _no_ way for sscanf() - having\nalready done one or more format extractions - to indicate to its caller\nthat the input fails to match the trailing part of the format string.\nIn other words, AFAICS, the scanf() family of function will only verify\nmatching input up to and including the last format specifier in the\nformat string. Any data following the last format specifier will not be\nverified. Yet another reason to consider the scanf functions harmful...\n\nCc: Bert Wesarg \nSigned-off-by: Johan Herland \n---\n refs.c | 82 +++++++++++++++++++------------------------------\n t/t6300-for-each-ref.sh | 12 ++++++++\n 2 files changed, 43 insertions(+), 51 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d17931a..7231f54 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2945,80 +2945,60 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n \treturn NULL;\n }\n \n-/*\n- * generate a format suitable for scanf from a ref_rev_parse_rules\n- * rule, that is replace the \"%.*s\" spec with a \"%s\" spec\n- */\n-static void gen_scanf_fmt(char *scanf_fmt, const char *rule)\n+int shorten_ref(const char *refname, const char *pattern, char *short_name)\n {\n-\tchar *spec;\n-\n-\tspec = strstr(rule, \"%.*s\");\n-\tif (!spec || strstr(spec + 4, \"%.*s\"))\n-\t\tdie(\"invalid rule in ref_rev_parse_rules: %s\", rule);\n-\n-\t/* copy all until spec */\n-\tstrncpy(scanf_fmt, rule, spec - rule);\n-\tscanf_fmt[spec - rule] = '\\0';\n-\t/* copy new spec */\n-\tstrcat(scanf_fmt, \"%s\");\n-\t/* copy remaining rule */\n-\tstrcat(scanf_fmt, spec + 4);\n-\n-\treturn;\n+\t/*\n+\t * pattern must be of the form \"[pre]%.*s[post]\". Check if refname\n+\t * starts with \"[pre]\" and ends with \"[post]\". If so, write the\n+\t * middle part into short_name, and return the number of chars\n+\t * written (not counting the added NUL-terminator). Otherwise,\n+\t * if refname does not match pattern, return 0.\n+\t */\n+\tsize_t pre_len, post_start, post_len, match_len;\n+\tsize_t ref_len = strlen(refname);\n+\tchar *sep = strstr(pattern, \"%.*s\");\n+\tif (!sep || strstr(sep + 4, \"%.*s\"))\n+\t\tdie(\"invalid pattern in ref_rev_parse_rules: %s\", pattern);\n+\tpre_len = sep - pattern;\n+\tpost_start = pre_len + 4;\n+\tpost_len = strlen(pattern + post_start);\n+\tif (pre_len + post_len >= ref_len)\n+\t\treturn 0; /* refname too short */\n+\tmatch_len = ref_len - (pre_len + post_len);\n+\tif (strncmp(refname, pattern, pre_len) ||\n+\t strncmp(refname + ref_len - post_len, pattern + post_start, post_len))\n+\t\treturn 0; /* refname does not match */\n+\tmemcpy(short_name, refname + pre_len, match_len);\n+\tshort_name[match_len] = '\\0';\n+\treturn match_len;\n }\n \n char *shorten_unambiguous_ref(const char *refname, int strict)\n {\n \tint i;\n-\tstatic char **scanf_fmts;\n-\tstatic int nr_rules;\n \tchar *short_name;\n \n-\t/* pre generate scanf formats from ref_rev_parse_rules[] */\n-\tif (!nr_rules) {\n-\t\tsize_t total_len = 0;\n-\n-\t\t/* the rule list is NULL terminated, count them first */\n-\t\tfor (; ref_rev_parse_rules[nr_rules]; nr_rules++)\n-\t\t\t/* no +1 because strlen(\"%s\") < strlen(\"%.*s\") */\n-\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]);\n-\n-\t\tscanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n-\n-\t\ttotal_len = 0;\n-\t\tfor (i = 0; i < nr_rules; i++) {\n-\t\t\tscanf_fmts[i] = (char *)&scanf_fmts[nr_rules]\n-\t\t\t\t\t+ total_len;\n-\t\t\tgen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);\n-\t\t\ttotal_len += strlen(ref_rev_parse_rules[i]);\n-\t\t}\n-\t}\n-\n-\t/* bail out if there are no rules */\n-\tif (!nr_rules)\n-\t\treturn xstrdup(refname);\n-\n \t/* buffer for scanf result, at most refname must fit */\n \tshort_name = xstrdup(refname);\n \n \t/* skip first rule, it will always match */\n-\tfor (i = nr_rules - 1; i > 0 ; --i) {\n+\tfor (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n \t\tint j;\n \t\tint rules_to_fail = i;\n \t\tint short_name_len;\n \n-\t\tif (1 != sscanf(refname, scanf_fmts[i], short_name))\n+\t\tif (!ref_rev_parse_rules[i] ||\n+\t\t !(short_name_len = shorten_ref(refname,\n+\t\t\t\t\t\t ref_rev_parse_rules[i],\n+\t\t\t\t\t\t short_name)))\n \t\t\tcontinue;\n \n-\t\tshort_name_len = strlen(short_name);\n-\n \t\t/*\n \t\t * in strict mode, all (except the matched one) rules\n \t\t * must fail to resolve to a valid non-ambiguous ref\n \t\t */\n \t\tif (strict)\n-\t\t\trules_to_fail = nr_rules;\n+\t\t\trules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);\n \n \t\t/*\n \t\t * check if the short name resolves to a valid ref,\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 752f5cb..57e3109 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -466,4 +466,16 @@ test_expect_success 'Verify sort with multiple keys' '\n \t\trefs/tags/bogo refs/tags/master > actual &&\n \ttest_cmp expected actual\n '\n+\n+cat >expected <<\\EOF\n+origin\n+origin/master\n+EOF\n+\n+test_expect_success 'Check refs/remotes/origin/HEAD shortens to origin' '\n+\tgit remote set-head origin master &&\n+\tgit for-each-ref --format=\"%(refname:short)\" refs/remotes >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216429","messageId":"1367711749-8812-3-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 2/7] t7900: Start testing usability of namespaced remote refs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:44Z","receivedAt":"2013-05-04T23:55:44Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Some users are interested in fetching remote refs into a separate namespace\nin the local repo. E.g. instead of the usual remote config:\n\n [remote \"origin\"]\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n\turl = ...\n\nthey want to keep remote tags separate from local tags, and they may also\nwant to fetch other ref types:\n\n [remote \"origin\"]\n\tfetch = +refs/heads/*:refs/remotes/origin/heads/*\n\tfetch = +refs/tags/*:refs/remotes/origin/tags/*\n\tfetch = +refs/notes/*:refs/remotes/origin/notes/*\n\tfetch = +refs/replace/*:refs/remotes/origin/replace/*\n\ttagopt = \"--no-tags\"\n\turl = ...\n\nThis configuration creates a separate namespace under refs/remotes/origin/*\nmirroring the structure of local refs (under refs/*) where all the relevant\nrefs from the 'origin' remote can be found.\n\nThis patch introduces a test whose main purpose is to verify that git will\nwork comfortably with this kind of setup. For now, we only verify that it\nis possible (though not exactly easy) to establish a clone with the above\nconfiguration, and that fetching into it yields the expected result.\n\nSigned-off-by: Johan Herland \n---\n t/t7900-working-with-namespaced-remote-refs.sh | 88 ++++++++++++++++++++++++++\n 1 file changed, 88 insertions(+)\n create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh\n\ndiff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh\nnew file mode 100755\nindex 0000000..af03ac9\n--- /dev/null\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -0,0 +1,88 @@\n+#!/bin/sh\n+\n+test_description='testing end-user usability of namespaced remote refs\n+\n+Set up a local repo with namespaced remote refs, like this:\n+\n+[remote \"origin\"]\n+\tfetch = +refs/heads/*:refs/remotes/origin/heads/*\n+\tfetch = +refs/tags/*:refs/remotes/origin/tags/*\n+\tfetch = +refs/notes/*:refs/remotes/origin/notes/*\n+\tfetch = +refs/replace/*:refs/remotes/origin/replace/*\n+\ttagopt = \"--no-tags\"\n+\turl = ...\n+\n+Test that the usual end-user operations work as expected with this setup.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup server repo' '\n+\tgit init server &&\n+\t(\n+\t\tcd server &&\n+\t\ttest_commit server_master_a &&\n+\t\tgit checkout -b other &&\n+\t\ttest_commit server_other_b &&\n+\t\tgit checkout master &&\n+\t\ttest_commit server_master_b\n+\t)\n+'\n+\n+server_master_a=$(git --git-dir=server/.git rev-parse --verify server_master_a)\n+server_master_b=$(git --git-dir=server/.git rev-parse --verify server_master_b)\n+server_other_b=$(git --git-dir=server/.git rev-parse --verify server_other_b)\n+\n+cat > expect.refspecs << EOF\n++refs/heads/*:refs/remotes/origin/heads/*\n++refs/tags/*:refs/remotes/origin/tags/*\n++refs/notes/*:refs/remotes/origin/notes/*\n++refs/replace/*:refs/remotes/origin/replace/*\n+EOF\n+\n+cat > expect.show-ref << EOF\n+$server_master_b refs/heads/master\n+$server_master_b refs/remotes/origin/heads/master\n+$server_other_b refs/remotes/origin/heads/other\n+$server_master_a refs/remotes/origin/tags/server_master_a\n+$server_master_b refs/remotes/origin/tags/server_master_b\n+$server_other_b refs/remotes/origin/tags/server_other_b\n+EOF\n+\n+test_clone() {\n+\t( cd $1 && git config --get-all remote.origin.fetch ) > actual.refspecs &&\n+\ttest_cmp expect.refspecs actual.refspecs &&\n+\t( cd $1 && git show-ref ) > actual.show-ref &&\n+\ttest_cmp expect.show-ref actual.show-ref\n+}\n+\n+test_expect_failure 'clone with namespaced remote refs' '\n+\tgit clone server client \\\n+\t\t--config remote.origin.fetch=\"+refs/heads/*:refs/remotes/origin/heads/*\" \\\n+\t\t--config remote.origin.fetch=\"+refs/tags/*:refs/remotes/origin/tags/*\" \\\n+\t\t--config remote.origin.fetch=\"+refs/notes/*:refs/remotes/origin/notes/*\" \\\n+\t\t--config remote.origin.fetch=\"+refs/replace/*:refs/remotes/origin/replace/*\" \\\n+\t\t--config remote.origin.tagopt \"--no-tags\" &&\n+\ttest_clone client\n+'\n+\n+# Work-around for the above failure\n+test_expect_success 'work-around \"clone\" with namespaced remote refs' '\n+\trm -rf client &&\n+\tgit init client &&\n+\t(\n+\t\tcd client &&\n+\t\tgit remote add origin ../server &&\n+\t\tgit config --unset-all remote.origin.fetch &&\n+\t\tgit config --add remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/heads/*\" &&\n+\t\tgit config --add remote.origin.fetch \"+refs/tags/*:refs/remotes/origin/tags/*\" &&\n+\t\tgit config --add remote.origin.fetch \"+refs/notes/*:refs/remotes/origin/notes/*\" &&\n+\t\tgit config --add remote.origin.fetch \"+refs/replace/*:refs/remotes/origin/replace/*\" &&\n+\t\tgit config remote.origin.tagopt \"--no-tags\" &&\n+\t\tgit fetch &&\n+\t\tgit checkout master\n+\t) &&\n+\ttest_clone client\n+'\n+\n+test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216432","messageId":"1367711749-8812-4-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 3/7] t7900: Demonstrate failure to expand \"$remote/$branch\" according to refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:45Z","receivedAt":"2013-05-04T23:55:45Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This test verifies that the following expressions all evaluate to the\nfull refname \"refs/remotes/origin/heads/master\":\n\n - refs/remotes/origin/heads/master\n - remotes/origin/heads/master\n - origin/heads/master\n - origin/master\n\nCurrently the last of these fail, because the \"$remote/$branch\" syntax only\nworks for remotes with conventional (refs/heads/*:refs/remotes/origin/*)\nrefspecs.\n\nIn order for users of namespaced remote refs (or any other unconventional\nrefspec configuration) to be able to use the \"$remote/$branch\" syntax, we\nneed to extend the parsing of \"$remote/$branch\" expressions to take the\nconfigured refspecs into account (i.e. look up the fetch refspecs for\n$remote, and map \"refs/heads/$branch\" through the refspecs to find the\ncorresponding remote-tracking branch name).\n\nMirroring the expansion of the above 4 expressions into the full refname,\nthe same 4 expression should also be shortened into \"origin/master\" when\nabbreviating them into their shortest unambiguous representation, e.g.\nwhen running \"git rev-parse --abbrev-ref\" on them. A (currently failing)\ntest verifying this behavior is also added by this patch.\n\nSigned-off-by: Johan Herland \n---\n t/t7900-working-with-namespaced-remote-refs.sh | 28 ++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh\nindex af03ac9..cc25e76 100755\n--- a/t/t7900-working-with-namespaced-remote-refs.sh\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -85,4 +85,32 @@ test_expect_success 'work-around \"clone\" with namespaced remote refs' '\n \ttest_clone client\n '\n \n+test_expect_success 'enter client repo' '\n+\tcd client\n+'\n+\n+test_expect_failure 'short-hand notation expands correctly for remote-tracking branches' '\n+\techo refs/remotes/origin/heads/master > expect &&\n+\tgit rev-parse --symbolic-full-name refs/remotes/origin/heads/master > actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --symbolic-full-name remotes/origin/heads/master > actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --symbolic-full-name origin/heads/master > actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --symbolic-full-name origin/master > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'remote-tracking branches are shortened correctly' '\n+\techo origin/master > expect &&\n+\tgit rev-parse --abbrev-ref refs/remotes/origin/heads/master > actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --abbrev-ref remotes/origin/heads/master > actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --abbrev-ref origin/heads/master > actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --abbrev-ref origin/master > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216434","messageId":"1367711749-8812-5-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 4/7] refs.c: Refactor rules for expanding shorthand names into full refnames","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:46Z","receivedAt":"2013-05-04T23:55:46Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"In preparation for allowing alternative ways of expanding shorthand refs\n(like \"master\") into full refnames (like \"refs/heads/master\"): Expand\nthe current ref_rev_parse_rules list into ref_expand_rules, a list of\nstruct ref_expand_rule objects that encode both an expansion pattern\n(e.g. \"refs/heads/%.*s\") and an associated expansion function\n(e.g. the code that applies \"master\" to \"refs/heads/%.*s\" to produce\n\"refs/heads/master\"). This allows us to later add expansion rules that\ndo something other than the current purely textual expansion.\n\nThe current expansion behavior is encoded in the new ref_expand_txtly()\nhelper function, which does the mksnpath() call that were previously\nperformed by all users of ref_rev_parse_rules.\n\nThe end result is identical in behavior to the existing code, but makes\nit easier to adjust the way ref expansion happens for remote-tracking\nbranches in future patches\n\nMost of the existing code that uses ref_rev_parse_rules to expand\nshorthand refs are converted to use ref_expand_rules instead.\n\nSigned-off-by: Johan Herland \n---\n cache.h | 4 ----\n refs.c | 46 +++++++++++++++++++++++++++++++++-------------\n refs.h | 11 +++++++++++\n remote.c | 6 +++---\n 4 files changed, 47 insertions(+), 20 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7ce9061..6adab04 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -875,10 +875,6 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\n extern int interpret_branch_name(const char *str, struct strbuf *);\n extern int get_sha1_mb(const char *str, unsigned char *sha1);\n \n-extern int refname_match(const char *abbrev_name, const char *full_name, const char **rules);\n-extern const char *ref_rev_parse_rules[];\n-#define ref_fetch_rules ref_rev_parse_rules\n-\n extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);\n extern int validate_headref(const char *ref);\n \ndiff --git a/refs.c b/refs.c\nindex 7231f54..8b02140 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1724,7 +1724,24 @@ const char *prettify_refname(const char *name)\n \t\t0);\n }\n \n-const char *ref_rev_parse_rules[] = {\n+static void ref_expand_txtly(const struct ref_expand_rule *rule,\n+\t\t\t char *dst, size_t dst_len,\n+\t\t\t const char *shortname, size_t shortname_len)\n+{\n+\tmksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);\n+}\n+\n+const struct ref_expand_rule ref_expand_rules[] = {\n+\t{ ref_expand_txtly, \"%.*s\" },\n+\t{ ref_expand_txtly, \"refs/%.*s\" },\n+\t{ ref_expand_txtly, \"refs/tags/%.*s\" },\n+\t{ ref_expand_txtly, \"refs/heads/%.*s\" },\n+\t{ ref_expand_txtly, \"refs/remotes/%.*s\" },\n+\t{ ref_expand_txtly, \"refs/remotes/%.*s/HEAD\" },\n+\t{ NULL, NULL }\n+};\n+\n+static const char *ref_rev_parse_rules[] = {\n \t\"%.*s\",\n \t\"refs/%.*s\",\n \t\"refs/tags/%.*s\",\n@@ -1734,15 +1751,17 @@ const char *ref_rev_parse_rules[] = {\n \tNULL\n };\n \n-int refname_match(const char *abbrev_name, const char *full_name, const char **rules)\n+int refname_match(const char *abbrev_name, const char *full_name,\n+\t\t const struct ref_expand_rule *rules)\n {\n-\tconst char **p;\n+\tconst struct ref_expand_rule *p;\n \tconst int abbrev_name_len = strlen(abbrev_name);\n+\tchar n[PATH_MAX];\n \n-\tfor (p = rules; *p; p++) {\n-\t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {\n+\tfor (p = rules; p->expand; p++) {\n+\t\tp->expand(p, n, sizeof(n), abbrev_name, abbrev_name_len);\n+\t\tif (!strcmp(full_name, n))\n \t\t\treturn 1;\n-\t\t}\n \t}\n \n \treturn 0;\n@@ -1807,21 +1826,22 @@ static char *substitute_branch_name(const char **string, int *len)\n int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n {\n \tchar *last_branch = substitute_branch_name(&str, &len);\n-\tconst char **p, *r;\n+\tconst struct ref_expand_rule *p;\n+\tconst char *r;\n \tint refs_found = 0;\n \n \t*ref = NULL;\n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n+\tfor (p = ref_expand_rules; p->expand; p++) {\n \t\tchar fullref[PATH_MAX];\n \t\tunsigned char sha1_from_ref[20];\n \t\tunsigned char *this_result;\n \t\tint flag;\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n-\t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n+\t\tp->expand(p, fullref, sizeof(fullref), str, len);\n \t\tr = resolve_ref_unsafe(fullref, this_result, 1, &flag);\n \t\tif (r) {\n-\t\t\tif (!refs_found++)\n+\t\t\tif ((!*ref || strcmp(*ref, r)) && !refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n \t\t\tif (!warn_ambiguous_refs)\n \t\t\t\tbreak;\n@@ -1838,17 +1858,17 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n {\n \tchar *last_branch = substitute_branch_name(&str, &len);\n-\tconst char **p;\n+\tconst struct ref_expand_rule *p;\n \tint logs_found = 0;\n \n \t*log = NULL;\n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n+\tfor (p = ref_expand_rules; p->expand; p++) {\n \t\tstruct stat st;\n \t\tunsigned char hash[20];\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n-\t\tmksnpath(path, sizeof(path), *p, len, str);\n+\t\tp->expand(p, path, sizeof(path), str, len);\n \t\tref = resolve_ref_unsafe(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\ndiff --git a/refs.h b/refs.h\nindex 8060ed8..85710cb 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -164,6 +164,17 @@ extern int for_each_reflog(each_ref_fn, void *);\n extern int check_refname_format(const char *refname, int flags);\n \n extern const char *prettify_refname(const char *refname);\n+\n+struct ref_expand_rule {\n+\tvoid (*expand)(const struct ref_expand_rule *rule,\n+\t\t char *dst, size_t dst_len,\n+\t\t const char *shortname, size_t shortname_len);\n+\tconst char *pattern;\n+};\n+extern const struct ref_expand_rule ref_expand_rules[];\n+extern int refname_match(const char *abbrev_name, const char *full_name,\n+\t\t\t const struct ref_expand_rule *rules);\n+\n extern char *shorten_unambiguous_ref(const char *refname, int strict);\n \n /** rename ref, return 0 on success **/\ndiff --git a/remote.c b/remote.c\nindex 68eb99b..5ef34c9 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -981,7 +981,7 @@ static int count_refspec_match(const char *pattern,\n \t\tchar *name = refs->name;\n \t\tint namelen = strlen(name);\n \n-\t\tif (!refname_match(pattern, name, ref_rev_parse_rules))\n+\t\tif (!refname_match(pattern, name, ref_expand_rules))\n \t\t\tcontinue;\n \n \t\t/* A match is \"weak\" if it is with refs outside\n@@ -1499,7 +1499,7 @@ int branch_merge_matches(struct branch *branch,\n {\n \tif (!branch || i < 0 || i >= branch->merge_nr)\n \t\treturn 0;\n-\treturn refname_match(branch->merge[i]->src, refname, ref_fetch_rules);\n+\treturn refname_match(branch->merge[i]->src, refname, ref_expand_rules);\n }\n \n static int ignore_symref_update(const char *refname)\n@@ -1545,7 +1545,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c\n {\n \tconst struct ref *ref;\n \tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (refname_match(name, ref->name, ref_fetch_rules))\n+\t\tif (refname_match(name, ref->name, ref_expand_rules))\n \t\t\treturn ref;\n \t}\n \treturn NULL;\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216433","messageId":"1367711749-8812-6-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 5/7] refs.c: Refactor code for shortening full refnames into shorthand names","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:47Z","receivedAt":"2013-05-04T23:55:47Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"shorten_unambiguous_ref() provides the reverse functionality to the\nexpansion of shorthand names into full refnames, i.e. it takes a full\nrefname (like \"refs/heads/master\"), and matches it against a pattern\n(like \"refs/heads/%.*s\") to produce a shorthand name (like \"master\").\n\nBeing the last remaining user of ref_rev_parse_rules list, this patch\nconverts it to use ref_expand_rules instead. However, as we associated\nan expansion function with each expansion rule - to allow for alternative\nexpansion behavior in the future - we also need to associate a\n\"shortening\" function with each rule, to allow for the reverse operation\nto be customized as well.\n\nTherefore, we add a \"shorten\" function to struct ref_expand_rule, to\nencode the shortening of a full refname into a shorthand name. The\nrelevant rule and the full refname is passed as arguments, and the\nresulting shorthand name is returned as an allocated string. If the\nrefname could not be shortened according to the given rule, NULL is\nreturned.\n\nThe reason for moving the allocation of the shorthand name into the\nshortening function, is that one assumes the shortening function itself\nwill best know exactly how much memory is needed to hold the shorthand\nstring.\n\nNaturally, we provide a shortening function that encodes the current\ntextual shortening algorithm - called ref_shorten_txtly() - which is\nmerely a slight refactoring of the former shorten_ref() function.\n\nThis patch removes the only remaining user of ref_rev_parse_rules.\nIt has now been fully replaced by ref_expand_rules. Hence this patch\nalso removes ref_rev_parse_rules.\n\nSigned-off-by: Johan Herland \n---\n refs.c | 110 ++++++++++++++++++++++++++++-------------------------------------\n refs.h | 2 ++\n 2 files changed, 50 insertions(+), 62 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8b02140..a866489 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1731,24 +1731,41 @@ static void ref_expand_txtly(const struct ref_expand_rule *rule,\n \tmksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);\n }\n \n-const struct ref_expand_rule ref_expand_rules[] = {\n-\t{ ref_expand_txtly, \"%.*s\" },\n-\t{ ref_expand_txtly, \"refs/%.*s\" },\n-\t{ ref_expand_txtly, \"refs/tags/%.*s\" },\n-\t{ ref_expand_txtly, \"refs/heads/%.*s\" },\n-\t{ ref_expand_txtly, \"refs/remotes/%.*s\" },\n-\t{ ref_expand_txtly, \"refs/remotes/%.*s/HEAD\" },\n-\t{ NULL, NULL }\n-};\n+static char *ref_shorten_txtly(const struct ref_expand_rule *rule,\n+\t\t\t const char *refname)\n+{\n+\t/*\n+\t * rule->pattern must be of the form \"[pre]%.*s[post]\". Check if\n+\t * refname starts with \"[pre]\" and ends with \"[post]\". If so,\n+\t * extract the middle part into a newly-allocated buffer, and\n+\t * return it. Else - if refname does not match rule->pattern -\n+\t * return NULL.\n+\t */\n+\tsize_t pre_len, post_start, post_len, match_len;\n+\tsize_t ref_len = strlen(refname);\n+\tchar *sep = strstr(rule->pattern, \"%.*s\");\n+\tif (!sep || strstr(sep + 4, \"%.*s\"))\n+\t\tdie(\"invalid pattern in ref_rev_parse_rules_alt: %s\", rule->pattern);\n+\tpre_len = sep - rule->pattern;\n+\tpost_start = pre_len + 4;\n+\tpost_len = strlen(rule->pattern + post_start);\n+\tif (pre_len + post_len >= ref_len)\n+\t\treturn NULL; /* refname too short */\n+\tmatch_len = ref_len - (pre_len + post_len);\n+\tif (strncmp(refname, rule->pattern, pre_len) ||\n+\t strncmp(refname + ref_len - post_len, rule->pattern + post_start, post_len))\n+\t\treturn NULL; /* refname does not match */\n+\treturn xstrndup(refname + pre_len, match_len);\n+}\n \n-static const char *ref_rev_parse_rules[] = {\n-\t\"%.*s\",\n-\t\"refs/%.*s\",\n-\t\"refs/tags/%.*s\",\n-\t\"refs/heads/%.*s\",\n-\t\"refs/remotes/%.*s\",\n-\t\"refs/remotes/%.*s/HEAD\",\n-\tNULL\n+const struct ref_expand_rule ref_expand_rules[] = {\n+\t{ ref_expand_txtly, NULL, \"%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/tags/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/heads/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s/HEAD\" },\n+\t{ NULL, NULL, NULL }\n };\n \n int refname_match(const char *abbrev_name, const char *full_name,\n@@ -2965,68 +2982,35 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n \treturn NULL;\n }\n \n-int shorten_ref(const char *refname, const char *pattern, char *short_name)\n-{\n-\t/*\n-\t * pattern must be of the form \"[pre]%.*s[post]\". Check if refname\n-\t * starts with \"[pre]\" and ends with \"[post]\". If so, write the\n-\t * middle part into short_name, and return the number of chars\n-\t * written (not counting the added NUL-terminator). Otherwise,\n-\t * if refname does not match pattern, return 0.\n-\t */\n-\tsize_t pre_len, post_start, post_len, match_len;\n-\tsize_t ref_len = strlen(refname);\n-\tchar *sep = strstr(pattern, \"%.*s\");\n-\tif (!sep || strstr(sep + 4, \"%.*s\"))\n-\t\tdie(\"invalid pattern in ref_rev_parse_rules: %s\", pattern);\n-\tpre_len = sep - pattern;\n-\tpost_start = pre_len + 4;\n-\tpost_len = strlen(pattern + post_start);\n-\tif (pre_len + post_len >= ref_len)\n-\t\treturn 0; /* refname too short */\n-\tmatch_len = ref_len - (pre_len + post_len);\n-\tif (strncmp(refname, pattern, pre_len) ||\n-\t strncmp(refname + ref_len - post_len, pattern + post_start, post_len))\n-\t\treturn 0; /* refname does not match */\n-\tmemcpy(short_name, refname + pre_len, match_len);\n-\tshort_name[match_len] = '\\0';\n-\treturn match_len;\n-}\n-\n char *shorten_unambiguous_ref(const char *refname, int strict)\n {\n \tint i;\n \tchar *short_name;\n \n-\t/* buffer for scanf result, at most refname must fit */\n-\tshort_name = xstrdup(refname);\n-\n-\t/* skip first rule, it will always match */\n-\tfor (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n+\tfor (i = ARRAY_SIZE(ref_expand_rules) - 1; i >= 0 ; --i) {\n \t\tint j;\n \t\tint rules_to_fail = i;\n \t\tint short_name_len;\n+\t\tconst struct ref_expand_rule *p = ref_expand_rules + i;\n \n-\t\tif (!ref_rev_parse_rules[i] ||\n-\t\t !(short_name_len = shorten_ref(refname,\n-\t\t\t\t\t\t ref_rev_parse_rules[i],\n-\t\t\t\t\t\t short_name)))\n+\t\tif (!p->shorten || !(short_name = p->shorten(p, refname)))\n \t\t\tcontinue;\n+\t\tshort_name_len = strlen(short_name);\n \n \t\t/*\n \t\t * in strict mode, all (except the matched one) rules\n \t\t * must fail to resolve to a valid non-ambiguous ref\n \t\t */\n \t\tif (strict)\n-\t\t\trules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);\n+\t\t\trules_to_fail = ARRAY_SIZE(ref_expand_rules);\n \n \t\t/*\n \t\t * check if the short name resolves to a valid ref,\n \t\t * but use only rules prior to the matched one\n \t\t */\n \t\tfor (j = 0; j < rules_to_fail; j++) {\n-\t\t\tconst char *rule = ref_rev_parse_rules[j];\n-\t\t\tchar refname[PATH_MAX];\n+\t\t\tconst struct ref_expand_rule *q = ref_expand_rules + j;\n+\t\t\tchar resolved[PATH_MAX];\n \n \t\t\t/* skip matched rule */\n \t\t\tif (i == j)\n@@ -3037,10 +3021,12 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\t\t * (with this previous rule) to a valid ref\n \t\t\t * read_ref() returns 0 on success\n \t\t\t */\n-\t\t\tmksnpath(refname, sizeof(refname),\n-\t\t\t\t rule, short_name_len, short_name);\n-\t\t\tif (ref_exists(refname))\n-\t\t\t\tbreak;\n+\t\t\tif (q->expand) {\n+\t\t\t\tq->expand(q, resolved, sizeof(resolved),\n+\t\t\t\t\t short_name, short_name_len);\n+\t\t\t\tif (ref_exists(resolved))\n+\t\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \n \t\t/*\n@@ -3049,9 +3035,9 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\t */\n \t\tif (j == rules_to_fail)\n \t\t\treturn short_name;\n+\t\tfree(short_name);\n \t}\n \n-\tfree(short_name);\n \treturn xstrdup(refname);\n }\n \ndiff --git a/refs.h b/refs.h\nindex 85710cb..245af6f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -169,6 +169,8 @@ struct ref_expand_rule {\n \tvoid (*expand)(const struct ref_expand_rule *rule,\n \t\t char *dst, size_t dst_len,\n \t\t const char *shortname, size_t shortname_len);\n+\tchar *(*shorten)(const struct ref_expand_rule *rule,\n+\t\t\t const char *refname);\n \tconst char *pattern;\n };\n extern const struct ref_expand_rule ref_expand_rules[];\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216435","messageId":"1367711749-8812-7-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 6/7] refname_match(): Caller must declare if we're matching local or remote refs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:48Z","receivedAt":"2013-05-04T23:55:48Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"refname_match() is used to check whether a given shorthand name matches a\ngiven full refname, but that full refname does not always belong in the\nlocal repo, rather it is sometimes taken from list of refs sent over from\na remote repo.\n\nIn preparation for adding alternative ways of expanding refspecs, we\nneed to take into account that such alternative ways may depend on\nlocal repo configuration, and it would therefore be wrong to use them\nwhen expanding refnames for a remote repo.\n\nTo resolve this, we split the ref_expand_rules list into a local and a\nremote variant, and teach the callers of refname_match() to pass the\ncorrect list as third argument to refname_match().\n\nFor now, the remote and local lists are identical, but this will change\nin a subsequent patch.\n\nThe other functions that use ref_expand_rules, all use it in a local\ncontext, so they hardcode the use of the local variant.\n\nSigned-off-by: Johan Herland \n---\n refs.c | 24 +++++++++++++++++-------\n refs.h | 3 ++-\n remote.c | 15 +++++++++------\n 3 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a866489..98997c4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1758,7 +1758,17 @@ static char *ref_shorten_txtly(const struct ref_expand_rule *rule,\n \treturn xstrndup(refname + pre_len, match_len);\n }\n \n-const struct ref_expand_rule ref_expand_rules[] = {\n+const struct ref_expand_rule ref_expand_rules_local[] = {\n+\t{ ref_expand_txtly, NULL, \"%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/tags/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/heads/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s\" },\n+\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s/HEAD\" },\n+\t{ NULL, NULL, NULL }\n+};\n+\n+const struct ref_expand_rule ref_expand_rules_remote[] = {\n \t{ ref_expand_txtly, NULL, \"%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/tags/%.*s\" },\n@@ -1848,7 +1858,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \tint refs_found = 0;\n \n \t*ref = NULL;\n-\tfor (p = ref_expand_rules; p->expand; p++) {\n+\tfor (p = ref_expand_rules_local; p->expand; p++) {\n \t\tchar fullref[PATH_MAX];\n \t\tunsigned char sha1_from_ref[20];\n \t\tunsigned char *this_result;\n@@ -1879,7 +1889,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \tint logs_found = 0;\n \n \t*log = NULL;\n-\tfor (p = ref_expand_rules; p->expand; p++) {\n+\tfor (p = ref_expand_rules_local; p->expand; p++) {\n \t\tstruct stat st;\n \t\tunsigned char hash[20];\n \t\tchar path[PATH_MAX];\n@@ -2987,11 +2997,11 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \tint i;\n \tchar *short_name;\n \n-\tfor (i = ARRAY_SIZE(ref_expand_rules) - 1; i >= 0 ; --i) {\n+\tfor (i = ARRAY_SIZE(ref_expand_rules_local) - 1; i >= 0 ; --i) {\n \t\tint j;\n \t\tint rules_to_fail = i;\n \t\tint short_name_len;\n-\t\tconst struct ref_expand_rule *p = ref_expand_rules + i;\n+\t\tconst struct ref_expand_rule *p = ref_expand_rules_local + i;\n \n \t\tif (!p->shorten || !(short_name = p->shorten(p, refname)))\n \t\t\tcontinue;\n@@ -3002,14 +3012,14 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\t * must fail to resolve to a valid non-ambiguous ref\n \t\t */\n \t\tif (strict)\n-\t\t\trules_to_fail = ARRAY_SIZE(ref_expand_rules);\n+\t\t\trules_to_fail = ARRAY_SIZE(ref_expand_rules_local);\n \n \t\t/*\n \t\t * check if the short name resolves to a valid ref,\n \t\t * but use only rules prior to the matched one\n \t\t */\n \t\tfor (j = 0; j < rules_to_fail; j++) {\n-\t\t\tconst struct ref_expand_rule *q = ref_expand_rules + j;\n+\t\t\tconst struct ref_expand_rule *q = ref_expand_rules_local + j;\n \t\t\tchar resolved[PATH_MAX];\n \n \t\t\t/* skip matched rule */\ndiff --git a/refs.h b/refs.h\nindex 245af6f..15b5ec2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -173,7 +173,8 @@ struct ref_expand_rule {\n \t\t\t const char *refname);\n \tconst char *pattern;\n };\n-extern const struct ref_expand_rule ref_expand_rules[];\n+extern const struct ref_expand_rule ref_expand_rules_local[];\n+extern const struct ref_expand_rule ref_expand_rules_remote[];\n extern int refname_match(const char *abbrev_name, const char *full_name,\n \t\t\t const struct ref_expand_rule *rules);\n \ndiff --git a/remote.c b/remote.c\nindex 5ef34c9..379577c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -969,7 +969,8 @@ void sort_ref_list(struct ref **l, int (*cmp)(const void *, const void *))\n \n static int count_refspec_match(const char *pattern,\n \t\t\t struct ref *refs,\n-\t\t\t struct ref **matched_ref)\n+\t\t\t struct ref **matched_ref,\n+\t\t\t int refs_from_remote)\n {\n \tint patlen = strlen(pattern);\n \tstruct ref *matched_weak = NULL;\n@@ -981,7 +982,9 @@ static int count_refspec_match(const char *pattern,\n \t\tchar *name = refs->name;\n \t\tint namelen = strlen(name);\n \n-\t\tif (!refname_match(pattern, name, ref_expand_rules))\n+\t\tif (!refname_match(pattern, name, refs_from_remote ?\n+\t\t\t\t ref_expand_rules_remote :\n+\t\t\t\t ref_expand_rules_local))\n \t\t\tcontinue;\n \n \t\t/* A match is \"weak\" if it is with refs outside\n@@ -1091,7 +1094,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\treturn 0;\n \n \tmatched_src = matched_dst = NULL;\n-\tswitch (count_refspec_match(rs->src, src, &matched_src)) {\n+\tswitch (count_refspec_match(rs->src, src, &matched_src, 0)) {\n \tcase 1:\n \t\tcopy_src = 1;\n \t\tbreak;\n@@ -1121,7 +1124,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\t\t matched_src->name);\n \t}\n \n-\tswitch (count_refspec_match(dst_value, dst, &matched_dst)) {\n+\tswitch (count_refspec_match(dst_value, dst, &matched_dst, 1)) {\n \tcase 1:\n \t\tbreak;\n \tcase 0:\n@@ -1499,7 +1502,7 @@ int branch_merge_matches(struct branch *branch,\n {\n \tif (!branch || i < 0 || i >= branch->merge_nr)\n \t\treturn 0;\n-\treturn refname_match(branch->merge[i]->src, refname, ref_expand_rules);\n+\treturn refname_match(branch->merge[i]->src, refname, ref_expand_rules_local);\n }\n \n static int ignore_symref_update(const char *refname)\n@@ -1545,7 +1548,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c\n {\n \tconst struct ref *ref;\n \tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (refname_match(name, ref->name, ref_expand_rules))\n+\t\tif (refname_match(name, ref->name, ref_expand_rules_remote))\n \t\t\treturn ref;\n \t}\n \treturn NULL;\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216431","messageId":"1367711749-8812-8-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"[PATCH 7/7] refs.c: Add rules for resolving refs using remote refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-04T23:55:49Z","receivedAt":"2013-05-04T23:55:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The \"$remote/$branch\" expression is often used as a shorthand with the\nintention of referring to the remote-tracking branch corresponding to\nthe given $branch in the $remote repo.\n\nCurrently, Git resolves this by prepending \"refs/remotes/\" to the given\n\"$remote/$branch\" expression, resulting in \"refs/remotes/$remote/$branch\",\nwhich is equivalent to the above intention only when conventional\nrefspecs are being used (e.g. refs/heads/*:refs/remotes/origin/*).\nCorrespondingly, a full remote-tracking branch name, can only be\nshortened to the \"$remote/$branch\" form if it textually matches the\n\"refs/remotes/$remote/$branch\" format.\n\nIf unconventional refspecs (e.g. refs/heads/*:refs/remotes/origin/heads/*)\nare being used, then the current expansion (\"refs/remotes/$remote/$branch\")\nfails to match the remote-tracking branches from the configured remotes.\nDitto for the corresponding shortening.\n\nInstead of doing pure textual expansion (from \"$remote/$branch\" to\n\"refs/remotes/$remote/$branch\"), we should expand by finding all remotes\nmatching \"$remote\", look up their fetch refspecs, and map\n\"refs/heads/$branch\" through them to find the appropriate remote-tracking\nbranch name corresponding to \"$remote/$branch\". This would yield the\ncorrect remote-tracking branch in all cases, both for conventional and\nunconventional refspecs.\n\nLikewise, when shortening full refnames for remote-tracking branches into\ntheir shorthand form (\"$remote/$branch\"), we should find the refspec with\nthe RHS that matches the full remote-tracking branch name, and map\nthrough that refspec to produce the corresponding LHS (minus the leading\n\"refs/heads/\" part), and from that construct the corresponding\n\"$remote/$branch\" shorthand.\n\nThis patch adds a new expansion method - ref_expand_refspec() - and a\ncorresponding shortening method - ref_shorten_refspec() - that implements\nthe remote refspec traversal and expanding/shortening logic described\nabove. These expand/shorten methods complement the existing\nref_expand_txtly() and ref_shorten_txtly() methods that implement the\ncurrent textual expanding/shortening logic.\n\nImplementing the proper expanding/shortening of \"$remote/$branch\" is now\na simple matter of adding another entry to the ref_expand_rules list,\nusing the new ref_expand_refspec() and ref_shorten_refspec() functions.\n\nNote that the existing \"refs/remotes/*\" textual expansion/shortening rule\nis kept to preserve backwards compatibility for refs under refs/remotes/*\nthat are not covered by any configured refspec.\n\nSigned-off-by: Johan Herland \n---\n refs.c | 98 +++++++++++++++++++++++++-\n t/t7900-working-with-namespaced-remote-refs.sh | 21 +++++-\n 2 files changed, 114 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 98997c4..18d7188 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4,6 +4,7 @@\n #include \"tag.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \"remote.h\"\n \n /*\n * Make sure \"ref\" is something reasonable to have under \".git/refs/\";\n@@ -1758,12 +1759,102 @@ static char *ref_shorten_txtly(const struct ref_expand_rule *rule,\n \treturn xstrndup(refname + pre_len, match_len);\n }\n \n+struct ref_expand_refspec_helper_data {\n+\tconst struct ref_expand_rule *rule;\n+\tchar *dst;\n+\tsize_t dst_len;\n+\tconst char *src;\n+\tsize_t src_len;\n+};\n+\n+static int ref_expand_refspec_helper(struct remote *remote, void *cb_data)\n+{\n+\tstruct ref_expand_refspec_helper_data *cb = cb_data;\n+\tstruct refspec query;\n+\tchar refspec_src[PATH_MAX];\n+\tsize_t ref_start = strlen(remote->name) + 1;\n+\tif (prefixcmp(cb->src, remote->name) ||\n+\t cb->src_len <= ref_start ||\n+\t cb->src[ref_start - 1] != '/')\n+\t\treturn 0;\n+\n+\tmksnpath(refspec_src, sizeof(refspec_src), cb->rule->pattern,\n+\t\t cb->src_len - ref_start, cb->src + ref_start);\n+\n+\tmemset(&query, 0, sizeof(struct refspec));\n+\tquery.src = refspec_src;\n+\tif ((!remote_find_tracking(remote, &query)) &&\n+\t strlen(query.dst) < cb->dst_len) {\n+\t\tstrcpy(cb->dst, query.dst);\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static void ref_expand_refspec(const struct ref_expand_rule *rule,\n+\t\t\t char *dst, size_t dst_len,\n+\t\t\t const char *shortname, size_t shortname_len)\n+{\n+\t/*\n+\t * Given shortname of the form \"$remote/$ref\", see if there is a\n+\t * fetch refspec configured for $remote whose lhs matches\n+\t * rule->pattern % $ref, and use the corresponding rhs of that\n+\t * mapping as the expanded result of \"$remote/$ref\".\n+\t */\n+\tconst void *has_slash = memchr(shortname, '/', shortname_len);\n+\tstruct ref_expand_refspec_helper_data cb = {\n+\t\trule, dst, dst_len, shortname, shortname_len };\n+\tdst[0] = '\\0';\n+\tif (has_slash)\n+\t\tfor_each_remote(ref_expand_refspec_helper, &cb);\n+}\n+\n+static int ref_shorten_refspec_helper(struct remote *remote, void *cb_data)\n+{\n+\tstruct ref_expand_refspec_helper_data *cb = cb_data;\n+\tstruct refspec query;\n+\tchar *lhs_ref;\n+\tint ret = 0;\n+\n+\tmemset(&query, 0, sizeof(struct refspec));\n+\tquery.dst = (char *) cb->src;\n+\tif (!remote_find_tracking(remote, &query) &&\n+\t (lhs_ref = ref_shorten_txtly(cb->rule, query.src))) {\n+\t\t/* refname matches rhs and rule->pattern matches lhs */\n+\t\tcb->dst_len = strlen(remote->name) + 1 + strlen(lhs_ref);\n+\t\t/* \"$remote/$lhs_ref\" should be shorter than src */\n+\t\tif (cb->dst_len < cb->src_len) {\n+\t\t\tcb->dst = xmalloc(cb->dst_len + 1);\n+\t\t\tsnprintf(cb->dst, cb->dst_len + 1,\n+\t\t\t\t \"%s/%s\", remote->name, lhs_ref);\n+\t\t\tret = 1;\n+\t\t}\n+\t\tfree(lhs_ref);\n+\t}\n+\treturn ret;\n+}\n+\n+static char *ref_shorten_refspec(const struct ref_expand_rule *rule,\n+\t\t\t\t const char *refname)\n+{\n+\t/*\n+\t * See if there is a $remote with a fetch refspec that matches the\n+\t * given refname on rhs, and will produce refs/heads/$ref on the\n+\t * lhs. If so, construct \"$remote/$ref\" as the shorthand.\n+\t */\n+\tstruct ref_expand_refspec_helper_data cb = {\n+\t\trule, NULL, 0, refname, strlen(refname) };\n+\tfor_each_remote(ref_shorten_refspec_helper, &cb);\n+\treturn cb.dst;\n+}\n+\n const struct ref_expand_rule ref_expand_rules_local[] = {\n \t{ ref_expand_txtly, NULL, \"%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/tags/%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/heads/%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s\" },\n+\t{ ref_expand_refspec, ref_shorten_refspec, \"refs/heads/%.*s\" },\n \t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s/HEAD\" },\n \t{ NULL, NULL, NULL }\n };\n@@ -3028,13 +3119,14 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \n \t\t\t/*\n \t\t\t * the short name is ambiguous, if it resolves\n-\t\t\t * (with this previous rule) to a valid ref\n-\t\t\t * read_ref() returns 0 on success\n+\t\t\t * (with this previous rule) to a valid\n+\t\t\t * (but different) ref\n \t\t\t */\n \t\t\tif (q->expand) {\n \t\t\t\tq->expand(q, resolved, sizeof(resolved),\n \t\t\t\t\t short_name, short_name_len);\n-\t\t\t\tif (ref_exists(resolved))\n+\t\t\t\tif (strcmp(refname, resolved) &&\n+\t\t\t\t ref_exists(resolved))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh\nindex cc25e76..302083e 100755\n--- a/t/t7900-working-with-namespaced-remote-refs.sh\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -89,7 +89,7 @@ test_expect_success 'enter client repo' '\n \tcd client\n '\n \n-test_expect_failure 'short-hand notation expands correctly for remote-tracking branches' '\n+test_expect_success 'short-hand notation expands correctly for remote-tracking branches' '\n \techo refs/remotes/origin/heads/master > expect &&\n \tgit rev-parse --symbolic-full-name refs/remotes/origin/heads/master > actual &&\n \ttest_cmp expect actual &&\n@@ -101,7 +101,7 @@ test_expect_failure 'short-hand notation expands correctly for remote-tracking b\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'remote-tracking branches are shortened correctly' '\n+test_expect_success 'remote-tracking branches are shortened correctly' '\n \techo origin/master > expect &&\n \tgit rev-parse --abbrev-ref refs/remotes/origin/heads/master > actual &&\n \ttest_cmp expect actual &&\n@@ -113,4 +113,21 @@ test_expect_failure 'remote-tracking branches are shortened correctly' '\n \ttest_cmp expect actual\n '\n \n+cat > expect.origin_master << EOF\n+$server_master_b\n+$server_master_a\n+EOF\n+\n+cat > expect.origin_other << EOF\n+$server_other_b\n+$server_master_a\n+EOF\n+\n+test_expect_success 'rev-list machinery should work with $remote/$branch' '\n+\tgit rev-list origin/master > actual.origin_master &&\n+\ttest_cmp expect.origin_master actual.origin_master &&\n+\tgit rev-list origin/other > actual.origin_other &&\n+\ttest_cmp expect.origin_other actual.origin_other\n+'\n+\n test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216437","messageId":"7vr4hmuk20.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-1-git-send-email-johan@herland.net","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-05T04:28:39Z","receivedAt":"2013-05-05T04:28:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> The \"$remote/$branch\" syntax can be interpreted in two subtly different\n> ways:\n>\n> 1. A shorthand name for the remote-tracking branch corresponding to a\n> specific $branch from a specific $remote.\n>\n> 2. A refname fragment, which - when appended to \"refs/remotes/\" -\n> yields the remote-tracking branch corresponding to a specific\n> $branch from a specific $remote.\n\nI think both of the above are somewhat distorted views and they go\nagainst all the documentation we have so far. The real definition\nis:\n\n 3. $string (which may happen to have one or more slashes) is used\n by prepending a few common prefixes to see if the result forms\n a full refname, and refs/remotes/ is one of the prefixes.\n origin/master ends up referring refs/remotes/origin/master\n because of this.\n\n> However, when configuring non-default refspecs\n> (such as the +refs/heads/*:refs/remotes/origin/heads/*), it becomes\n> obvious that the current code follows the latter interpretation: The\n> \"$remote/$branch\" shorthand will no longer work, and you are forced to\n> use \"$remote/heads/$branch\" instead.\n\nWhile I do _not_ think it is _wrong_ to use remotes/origin/heads/*\nas a namespace for branches you copy from the 'origin' remote, my\ngut feeling is that it is myopic to redefine that origin/master\nresolves to refs/remotes/origin/heads/master [*1*].\n\nStep back a bit.\n\nThere must be a reason why somebody wants remotes/origin/heads/*\ninstead of the traditional remotes/origin/* to keep the copies of\nbranches taken from the origin.\n\nIt is because she wants to use the parts of remotes/origin/ that are\noutside remote/origin/heads/ to store other things taken from that\nremote, no? They may be \"changes\", \"pull-requests\", \"notes\", etc.\n\nIf origin/master were to map to refs/remotes/origin/heads/master and\norigin/jh/rtrack were to map to refs/remotes/origin/heads/jh/rtrack,\n[*2*] what short-hands hierarchies in refs/remotes/origin/ other\nthan \"heads/\" would have?\n\nIf you do not special case \"heads/\",\n\n $ git merge origin/pull-requests/4\n\nis very straightforward to understand and explain when you use the\ndefinition #3 above. But if you do, then the above may refer to\norigin/heads/pull-requests/4, or perhaps there is no pull-requests/4\nbranch in the origin and the resolution may have to error out.\n\nWhile I do not reject refs/remotes/origin/heads/* layout as a\npossibility, I am somewhat skeptical that any \"solution\" that starts\nfrom the \"two interpretations\" above (both of which are flawed, that\nonly consider what happens to the branches) will yield a generally\nuseful result.\n\nIf the final end result you are shooting for is to introduce an\nextra level between the remote name and the branch names, i.e.\n\"heads/\", any solution needs to at least have a plan (not necessarily\na detailed design or implementation) for the other hierarchies. The\npossibility to have these other hierarchies per remote is the true\nprogress that the \"heads/\" at that level can give us; there is not\nmuch point to have heads/ after refs/remotes/origin/, if heads/ is\nthe only thing that can come there.\n\n\n[Footnotes]\n\n*1* Unlike the usual cautions from me, this does not have anything\n to do with backward compatibility; it is more about forward\n thinking.\n\n*2* Wait.\n Does origin/jh/rtrack map to refs/remotes/origin/jh/heads/rtrack\n which is rtrack branch taken from the origin/jh remote?\n"},{"id":"216443","messageId":"CALKQrgdp9DVDBLNwCAmQHbEfZDvhdsmSW3sh1BRo1XEnyqPPaA@mail.gmail.com","threadId":"33726","inReplyTo":"7vr4hmuk20.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-05T09:59:46Z","receivedAt":"2013-05-05T09:59:46Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sun, May 5, 2013 at 6:28 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>\n>> The \"$remote/$branch\" syntax can be interpreted in two subtly different\n>> ways:\n>>\n>> 1. A shorthand name for the remote-tracking branch corresponding to a\n>> specific $branch from a specific $remote.\n>>\n>> 2. A refname fragment, which - when appended to \"refs/remotes/\" -\n>> yields the remote-tracking branch corresponding to a specific\n>> $branch from a specific $remote.\n>\n> I think both of the above are somewhat distorted views and they go\n> against all the documentation we have so far. The real definition\n> is:\n>\n> 3. $string (which may happen to have one or more slashes) is used\n> by prepending a few common prefixes to see if the result forms\n> a full refname, and refs/remotes/ is one of the prefixes.\n> origin/master ends up referring refs/remotes/origin/master\n> because of this.\n\nTrue, but from the POV of remote-tracking branches (which is what this\nseries is concerned about, more on that below), #2 and #3 are equivalent.\n\n>> However, when configuring non-default refspecs\n>> (such as the +refs/heads/*:refs/remotes/origin/heads/*), it becomes\n>> obvious that the current code follows the latter interpretation: The\n>> \"$remote/$branch\" shorthand will no longer work, and you are forced to\n>> use \"$remote/heads/$branch\" instead.\n>\n> While I do _not_ think it is _wrong_ to use remotes/origin/heads/*\n> as a namespace for branches you copy from the 'origin' remote, my\n> gut feeling is that it is myopic to redefine that origin/master\n> resolves to refs/remotes/origin/heads/master [*1*].\n\nI guess my point is to \"raise\" the origin/master notation to a higher\nlevel of abstraction. Instead of it being purely a string that is\nappended to some common prefixes to yielf a (hopefully unambiguous) match,\nI want Git to support the idea that it refers to the \"master\" shorthand\nfrom the \"origin\" remote. In other words, just as you can write\n\"$something\" to refer to a ref in the local repo, you can also write\n\"$remote/$something\" to refer to a remote-tracking ref from another repo.\n\n> Step back a bit.\n>\n> There must be a reason why somebody wants remotes/origin/heads/*\n> instead of the traditional remotes/origin/* to keep the copies of\n> branches taken from the origin.\n>\n> It is because she wants to use the parts of remotes/origin/ that are\n> outside remote/origin/heads/ to store other things taken from that\n> remote, no? They may be \"changes\", \"pull-requests\", \"notes\", etc.\n\nIndeed.\n\n> If origin/master were to map to refs/remotes/origin/heads/master and\n> origin/jh/rtrack were to map to refs/remotes/origin/heads/jh/rtrack,\n> [*2*] what short-hands hierarchies in refs/remotes/origin/ other\n> than \"heads/\" would have?\n\nThe same. Today, you can use \"eggs\" to refer to either \"refs/heads/eggs\"\nor \"refs/tags/eggs\", depending on which exists in your repo. (Having both\nexist is also possible, but then you must express more explicitly which of\nthem you mean). In the same way, I want \"origin/eggs\" to refer to either\n\"refs/remotes/origin/heads/eggs\" or \"refs/remotes/origin/tags/eggs\",\ndepending on which is available (with the same behavior when both exist).\n\nThis series only gets us halfway there, the other half consists of making\nGit work properly with \"remote tags\" (i.e. teaching Git to handle\n\"refs/remotes/$remote/tags/*\" as an extension of \"refs/tags/*\").\n\nPart of that would surely be to add another rule that maps \"$remote/$tag\"\nthrough $remote's fetch refspec for tags to yield the corresponding\nremote-tracking tag (e.g. \"refs/remotes/$remote/tags/$tag\" if you're\nusing namespaced remote refs). This rule would should cause problems for\ncurrent users since it would map to _nothing_ if you're using the default\nrefspec, alternatively \"refs/tags/$tag\" if you have explicitly set up\nmirroring of tags from the remote (refs/tags/*:refs/tags/*).\n\n> If you do not special case \"heads/\",\n>\n> $ git merge origin/pull-requests/4\n>\n> is very straightforward to understand and explain when you use the\n> definition #3 above. But if you do, then the above may refer to\n> origin/heads/pull-requests/4, or perhaps there is no pull-requests/4\n> branch in the origin and the resolution may have to error out.\n\nLet's say that in the origin repo there may be either refs/pull-requests/4\nor refs/heads/pull-requests/4, and they are fetched into our repo, and\nplaced at either refs/remotes/origin/pull-requests/4 or\nrefs/remotes/origin/heads/pull-requests/4, respectively. (If both are\npresent, and fetched, we will get both in our local repo).\n\nWhen doing your\n\n $ git merge origin/pull-requests/4\n\nwe would either expand to refs/remotes/origin/pull-requests/4 (with the\nexisting \"refs/remotes/%.*s\" rule) if that exists, or alternatively expand\nto refs/remotes/origin/heads/pull-requests/4 (with the new refspec-mapping\nrule from this series) if that exists. (If both exist, you would have to\nbe more explicit to resolve the ambiguity - the same ambiguity that would\nhave to be resolved if you did \"git merge pull-requests/4\" in the origin\nrepo.\n\nThe rules that expand shorthand names into full refnames, are all about\nbalancing convenience against ambiguity. We want users to be able use as\nshort names as possible, as long as they are unambiguous. As part of that\nwe have determined that \"foo\" can be auto-completed into any of\n\n foo\n refs/foo\n refs/tags/foo\n refs/heads/foo\n\nwithout causing ambiguity in the common case (obviously all of these could\nexist, and the user would have to be more explicit, but in most repos, at\nmost one of these exist, so we are fine).\n\nI want to extend the same reasoning to remote-tracking refs, i.e.\n\"$remote/$name\" could be auto-completed into any of\n\n refs/remotes/$remote/$name\n refs/remotes/$remote/tags/$name\n refs/remotes/$remote/heads/$name\n\nwithout causing ambiguity in the common case. When there is ambiguity, we\nwould resolve that in the same manner as for local refs.\n\n> While I do not reject refs/remotes/origin/heads/* layout as a\n> possibility, I am somewhat skeptical that any \"solution\" that starts\n> from the \"two interpretations\" above (both of which are flawed, that\n> only consider what happens to the branches) will yield a generally\n> useful result.\n\nThe current series only concerns itself with the branches, but the larger\nintention is to make it work for tags and other refs as well.\n\n> If the final end result you are shooting for is to introduce an\n> extra level between the remote name and the branch names, i.e.\n> \"heads/\", any solution needs to at least have a plan (not necessarily\n> a detailed design or implementation) for the other hierarchies. The\n> possibility to have these other hierarchies per remote is the true\n> progress that the \"heads/\" at that level can give us; there is not\n> much point to have heads/ after refs/remotes/origin/, if heads/ is\n> the only thing that can come there.\n\nI fully agree. This series was meant as the first step in that direction\n(sorry for not describing my intentions more clearly).\n\nI believe \"the plan\" that you request might have been largely described in\nthe large \"[1.8.0] Provide proper remote ref namespaces\" thread, but this\nthread is now more than 2 years old, and although I re-read some of it to\nprepare for this series, it was stupid of me to assume that it would still\nbe fresh in the minds of everyone else... I hope my answers above have\nclarified the plan somewhat.\n\n> [Footnotes]\n>\n> *1* Unlike the usual cautions from me, this does not have anything\n> to do with backward compatibility; it is more about forward\n> thinking.\n>\n> *2* Wait.\n> Does origin/jh/rtrack map to refs/remotes/origin/jh/heads/rtrack\n> which is rtrack branch taken from the origin/jh remote?\n\nThere is an inherent ambiguity when interpreting \"foo/bar/baz\" as\n\"$remote/$ref\", whether to do\n\n $remote = foo/bar and $ref = baz\n\nor\n\n $remote = foo and $ref = bar/baz\n\nMy opinion on this is:\n\n 1. Having both remotes \"foo\" and \"foo/bar\" in the same repo is _insane_,\n and this scenario would cause problems for current Git as well.\n\n 2. Remote ref namespaces marginally improves the situation by forcing the\n middle fragment (\"bar\" in the above example) to be one of \"heads\",\n \"tags\", etc.; otherwise the refs won't clobber eachother.\n\n 3. Currently, this series does not explicitly handle this insanity.\n The code in patch 7/7 (refs.c:ref_expand_refspec()) stops at the\n first remote that matches a prefix of the given shorthand, so both\n alternatives won't be tested. This could be fixed by moving the ref\n verification (does the refname actually exist/resolve?) into the\n expansion, but I'm not sure it's worth it.\n\n 4. The proper way to deal with this is probably to detect when a new\n remote name is a prefix of another remote name (or vice versa) in\n \"git remote add\" and \"git remote rename\", and warn or error out.\n\n\nHope this helps,\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216444","messageId":"CAKPyHN0fyrZm=PBZSLhiWzcWO8w1Jjgh439pYVYZ_ViqMVO8Cw@mail.gmail.com","threadId":"33726","inReplyTo":"1367711749-8812-2-git-send-email-johan@herland.net","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2013-05-05T11:56:11Z","receivedAt":"2013-05-05T11:56:11Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Sun, May 5, 2013 at 1:55 AM, Johan Herland wrote:\n> When expanding shorthand refs to full ref names (e.g. in dwim_ref()),\n> we use the ref_rev_parse_rules list of expansion patterns. This list\n> allows \"origin\" to be expanded into \"refs/remotes/origin/HEAD\", by\n> using the \"refs/remotes/%.*s/HEAD\" pattern from that list.\n>\n> shorten_unambiguous_ref() exists to provide the reverse operation:\n> turning a full ref name into a shorter (but still unambiguous) name.\n> It does so by matching the given refname against each pattern from\n> the ref_rev_parse_rules list (in reverse), and extracting the short-\n> hand name from the matching rule.\n>\n> However, when given \"refs/remotes/origin/HEAD\" it fails to shorten it\n> into \"origin\", because we misuse the sscanf() function when matching\n> \"refs/remotes/origin/HEAD\" against \"refs/remotes/%.*s/HEAD\": We end\n> up calling sscanf like this:\n>\n> sscanf(\"refs/remotes/origin/HEAD\", \"refs/remotes/%s/HEAD\", short_name)\n>\n> In this case, sscanf() will match the initial \"refs/remotes/\" part, and\n> then match the remainder of the refname against the \"%s\", and place it\n> (\"origin/HEAD\") into short_name. The part of the pattern following the\n> \"%s\" format is never verified, because sscanf() apparently does not\n> need to do that (it has performed the one expected format extraction,\n> and will return 1 correspondingly; see [1] for more details).\n>\n> This patch replaces the misuse of sscanf() with a fairly simple function\n> that manually matches the refname against patterns, and extracts the\n> shorthand name.\n>\n> Also a testcase verifying \"refs/remotes/origin/HEAD\" -> \"origin\" has\n> been added.\n>\n> [1]: If we assume that sscanf() does not do a verification pass prior\n> to format extraction, there is AFAICS _no_ way for sscanf() - having\n> already done one or more format extractions - to indicate to its caller\n> that the input fails to match the trailing part of the format string.\n> In other words, AFAICS, the scanf() family of function will only verify\n> matching input up to and including the last format specifier in the\n> format string. Any data following the last format specifier will not be\n> verified. Yet another reason to consider the scanf functions harmful...\n>\n> Cc: Bert Wesarg \n\nLooks good, thanks.\n\nReviewed-by: Bert Wesarg \n\n> Signed-off-by: Johan Herland \n> ---\n> refs.c | 82 +++++++++++++++++++------------------------------\n> t/t6300-for-each-ref.sh | 12 ++++++++\n> 2 files changed, 43 insertions(+), 51 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index d17931a..7231f54 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2945,80 +2945,60 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n> return NULL;\n> }\n>\n> -/*\n> - * generate a format suitable for scanf from a ref_rev_parse_rules\n> - * rule, that is replace the \"%.*s\" spec with a \"%s\" spec\n> - */\n> -static void gen_scanf_fmt(char *scanf_fmt, const char *rule)\n> +int shorten_ref(const char *refname, const char *pattern, char *short_name)\n> {\n> - char *spec;\n> -\n> - spec = strstr(rule, \"%.*s\");\n> - if (!spec || strstr(spec + 4, \"%.*s\"))\n> - die(\"invalid rule in ref_rev_parse_rules: %s\", rule);\n> -\n> - /* copy all until spec */\n> - strncpy(scanf_fmt, rule, spec - rule);\n> - scanf_fmt[spec - rule] = '\\0';\n> - /* copy new spec */\n> - strcat(scanf_fmt, \"%s\");\n> - /* copy remaining rule */\n> - strcat(scanf_fmt, spec + 4);\n> -\n> - return;\n> + /*\n> + * pattern must be of the form \"[pre]%.*s[post]\". Check if refname\n> + * starts with \"[pre]\" and ends with \"[post]\". If so, write the\n> + * middle part into short_name, and return the number of chars\n> + * written (not counting the added NUL-terminator). Otherwise,\n> + * if refname does not match pattern, return 0.\n> + */\n> + size_t pre_len, post_start, post_len, match_len;\n> + size_t ref_len = strlen(refname);\n> + char *sep = strstr(pattern, \"%.*s\");\n> + if (!sep || strstr(sep + 4, \"%.*s\"))\n> + die(\"invalid pattern in ref_rev_parse_rules: %s\", pattern);\n> + pre_len = sep - pattern;\n> + post_start = pre_len + 4;\n> + post_len = strlen(pattern + post_start);\n> + if (pre_len + post_len >= ref_len)\n> + return 0; /* refname too short */\n> + match_len = ref_len - (pre_len + post_len);\n> + if (strncmp(refname, pattern, pre_len) ||\n> + strncmp(refname + ref_len - post_len, pattern + post_start, post_len))\n> + return 0; /* refname does not match */\n> + memcpy(short_name, refname + pre_len, match_len);\n> + short_name[match_len] = '\\0';\n> + return match_len;\n> }\n>\n> char *shorten_unambiguous_ref(const char *refname, int strict)\n> {\n> int i;\n> - static char **scanf_fmts;\n> - static int nr_rules;\n> char *short_name;\n>\n> - /* pre generate scanf formats from ref_rev_parse_rules[] */\n> - if (!nr_rules) {\n> - size_t total_len = 0;\n> -\n> - /* the rule list is NULL terminated, count them first */\n> - for (; ref_rev_parse_rules[nr_rules]; nr_rules++)\n> - /* no +1 because strlen(\"%s\") < strlen(\"%.*s\") */\n> - total_len += strlen(ref_rev_parse_rules[nr_rules]);\n> -\n> - scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n> -\n> - total_len = 0;\n> - for (i = 0; i < nr_rules; i++) {\n> - scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]\n> - + total_len;\n> - gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);\n> - total_len += strlen(ref_rev_parse_rules[i]);\n> - }\n> - }\n> -\n> - /* bail out if there are no rules */\n> - if (!nr_rules)\n> - return xstrdup(refname);\n> -\n> /* buffer for scanf result, at most refname must fit */\n> short_name = xstrdup(refname);\n>\n> /* skip first rule, it will always match */\n> - for (i = nr_rules - 1; i > 0 ; --i) {\n> + for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n> int j;\n> int rules_to_fail = i;\n> int short_name_len;\n>\n> - if (1 != sscanf(refname, scanf_fmts[i], short_name))\n> + if (!ref_rev_parse_rules[i] ||\n> + !(short_name_len = shorten_ref(refname,\n> + ref_rev_parse_rules[i],\n> + short_name)))\n> continue;\n>\n> - short_name_len = strlen(short_name);\n> -\n> /*\n> * in strict mode, all (except the matched one) rules\n> * must fail to resolve to a valid non-ambiguous ref\n> */\n> if (strict)\n> - rules_to_fail = nr_rules;\n> + rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);\n>\n> /*\n> * check if the short name resolves to a valid ref,\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 752f5cb..57e3109 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -466,4 +466,16 @@ test_expect_success 'Verify sort with multiple keys' '\n> refs/tags/bogo refs/tags/master > actual &&\n> test_cmp expected actual\n> '\n> +\n> +cat >expected <<\\EOF\n> +origin\n> +origin/master\n> +EOF\n> +\n> +test_expect_success 'Check refs/remotes/origin/HEAD shortens to origin' '\n> + git remote set-head origin master &&\n> + git for-each-ref --format=\"%(refname:short)\" refs/remotes >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> test_done\n> --\n> 1.8.1.3.704.g33f7d4f\n>\n"},{"id":"216460","messageId":"7v8v3tuu6i.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgdp9DVDBLNwCAmQHbEfZDvhdsmSW3sh1BRo1XEnyqPPaA@mail.gmail.com","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-05T19:02:13Z","receivedAt":"2013-05-05T19:02:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> I want to extend the same reasoning to remote-tracking refs, i.e.\n> \"$remote/$name\" could be auto-completed into any of\n>\n> refs/remotes/$remote/$name\n> refs/remotes/$remote/tags/$name\n> refs/remotes/$remote/heads/$name\n>\n> without causing ambiguity in the common case. When there is ambiguity, we\n> would resolve that in the same manner as for local refs.\n>\n> The current series only concerns itself with the branches, but the larger\n> intention is to make it work for tags and other refs as well.\n\nGood ;-).\n\nSo another issue that remains is the following, I think.\n\nWhen interpreting $nick/$name, assuming that we can tell where $nick\nfor a remote ends and $name for the ref we take from the remote\nbegins [*1*], how would we determine which refs/remotes/$remote/ is\nused for $nick?\n\nMy gut feeling is that we should ignore any \"remote.$nick.fetch\"\nwildcard mapping, e.g.\n\n [remote \"foo\"]\n fetch = +refs/heads/*:refs/remotes/bar/heads/*\n fetch = +refs/tags/*:refs/remotes/baz/tags/*\n\nso that we look always in refs/remotes/$nick/ somewhere, for at\nleast two reasons:\n\n * For sane people, \"bar\" and \"baz\" in the above example are both\n \"foo\", so ignoring remote.foo.fetch mapping is a no-op for them.\n\n * For people who deliberately wanted to move \"foo\"'s refs to\n different hierarchies depending on the hierarchies at the origin\n (i.e. branches to \"bar\", tags to \"baz\"), they wanted to do so for\n a reason to group related things in \"bar\" (and \"baz\") [*2*]. For\n them, mapping with remote.$nick.fetch\" means not allowing them to\n use the real name of the group (i.e. \"bar\") they chose to name\n their refs.\n\n>> If the final end result you are shooting for is to introduce an\n>> extra level between the remote name and the branch names, i.e.\n>> \"heads/\", any solution needs to at least have a plan (not necessarily\n>> a detailed design or implementation) for the other hierarchies. The\n>> possibility to have these other hierarchies per remote is the true\n>> progress that the \"heads/\" at that level can give us; there is not\n>> much point to have heads/ after refs/remotes/origin/, if heads/ is\n>> the only thing that can come there.\n>\n> I fully agree. This series was meant as the first step in that direction\n> (sorry for not describing my intentions more clearly).\n\nAnd I do not think we mind terribly if we extend the ref_rev_parse_rules[]\nused in dwim_ref() to also look at these\n\n\trefs/remotes/$nick/$name\n\trefs/remotes/$nick/tags/$name\n\trefs/remotes/$nick/heads/$name\n\n(the first of the above is existing \"refs/remotes/%.*s\"). I think\nit is going too far if you extend it further to\n\n\trefs/remotes/$nick/*/$name\n\nwhere the code does not control what an acceptable match for '*' is\n(i.e. origin/foo matching origin/changes/foo might be OK, but\nmatching it with origin/randomstring/foo is not, unless the canned\nref_rev_parse_rules[] knows about the \"randomstring\", or there is a\nconfiguration mechanism for the user to tell us she cares about the\n\"randomstring\" hierarchy in her project).\n\n\n[Footnotes]\n\n*1* I offhand do not remember if we even allow multi-level remote\n nicks, but I do know we support multi-level branch names, so it\n may turn out that the only valid split of origin/jh/rbranch is\n topic 'jh/rbranch' from remote 'origin' and not topic 'rbranch'\n from remote 'origin/jh'.\n\n*2* Perhaps \"bar\" in the above is spelled \"topics\", and the\n hierarchy may be used to collect non-integration single topic\n branches from more than one remote. An example that is more in\n line with such a usage might be:\n\n [remote \"jh\"]\n fetch = +refs/heads/*:refs/remotes/topics/heads/jh/*\n [remote \"jk\"]\n fetch = +refs/heads/*:refs/remotes/topics/heads/jk/*\n [remote \"fc\"]\n fetch = +refs/heads/*:refs/remotes/topics/heads/fc/*\n\n and I would expect \"git merge topics/jh/rbranch\" to merge the\n \"refs/remotes/topics/heads/jh/rbranch\" topic branch.\n"},{"id":"216465","messageId":"CALKQrgf6NcT2tEGMTczxR2WspOi4NjrN_kxmKN-QyE2Py3iSaQ@mail.gmail.com","threadId":"33726","inReplyTo":"7v8v3tuu6i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-05T22:26:40Z","receivedAt":"2013-05-05T22:26:40Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sun, May 5, 2013 at 9:02 PM, Junio C Hamano wrote:\n> So another issue that remains is the following, I think.\n>\n> When interpreting $nick/$name, assuming that we can tell where $nick\n> for a remote ends and $name for the ref we take from the remote\n> begins [*1*], how would we determine which refs/remotes/$remote/ is\n> used for $nick?\n\nMy code currently iterates for_each_remote() until we find a remote whose\nname == $nick. After all, the remote name is what the user interacts with\nwhen doing things like \"git fetch $remote\" and \"git push $remote ...\".\nHence, I believe that the remote name is the most natural name to use for\nthe \"$nick/$name\" shorthand.\n\n> My gut feeling is that we should ignore any \"remote.$nick.fetch\"\n> wildcard mapping, e.g.\n>\n> [remote \"foo\"]\n> fetch = +refs/heads/*:refs/remotes/bar/heads/*\n> fetch = +refs/tags/*:refs/remotes/baz/tags/*\n>\n> so that we look always in refs/remotes/$nick/ somewhere, for at\n> least two reasons:\n>\n> * For sane people, \"bar\" and \"baz\" in the above example are both\n> \"foo\", so ignoring remote.foo.fetch mapping is a no-op for them.\n\nAgreed.\n\n> * For people who deliberately wanted to move \"foo\"'s refs to\n> different hierarchies depending on the hierarchies at the origin\n> (i.e. branches to \"bar\", tags to \"baz\"), they wanted to do so for\n> a reason to group related things in \"bar\" (and \"baz\") [*2*]. For\n> them, mapping with remote.$nick.fetch\" means not allowing them to\n> use the real name of the group (i.e. \"bar\") they chose to name\n> their refs.\n\nActually, this series currently accepts _both_. Observe (using your config\nfrom above):\n\n Shorthand -> Expansion Matching rule\n --------------------------------------------------------------------\n foo/$branch -> refs/remotes/bar/heads/$branch foo's refspec\n foo/$tag -> refs/remotes/baz/tags/$branch foo's refspec\n bar/heads/$branch -> refs/remotes/bar/heads/$branch refs/remotes/%.*s\n baz/tags/$tag -> refs/remotes/baz/tags/$tag refs/remotes/%.*s\n\n(If you want to lose the \"heads/\" from the \"bar/heads/$branch\" shorthand\n (or lose \"tags/\" from \"baz/tags/$tag\"), use this config instead:\n\n [remote \"foo\"]\n fetch = +refs/heads/*:refs/remotes/bar/*\n fetch = +refs/tags/*:refs/remotes/baz/tags/*\n)\n\nNow, IINM you seem to mean that we shouldn't look at refspecs (or the\nconfig file) at all, but should instead require that $nick matches a\nsubdirectory of \"refs/remotes\", and then allow for some flexibility as\nto what comes between \"refs/remotes/$nick\" and \"$name\" (for example, one\nof \"/\", \"/heads/\", and \"/tags/\").\n\nThis would not allow the user to use the relevant $remote_name for $nick,\nwhich I argue might be the more natural name for the user to use, since\nit's the same name that is used for otherwise interacting with the remote.\n\n>>> If the final end result you are shooting for is to introduce an\n>>> extra level between the remote name and the branch names, i.e.\n>>> \"heads/\", any solution needs to at least have a plan (not necessarily\n>>> a detailed design or implementation) for the other hierarchies. The\n>>> possibility to have these other hierarchies per remote is the true\n>>> progress that the \"heads/\" at that level can give us; there is not\n>>> much point to have heads/ after refs/remotes/origin/, if heads/ is\n>>> the only thing that can come there.\n>>\n>> I fully agree. This series was meant as the first step in that direction\n>> (sorry for not describing my intentions more clearly).\n>\n> And I do not think we mind terribly if we extend the ref_rev_parse_rules[]\n> used in dwim_ref() to also look at these\n>\n> refs/remotes/$nick/$name\n> refs/remotes/$nick/tags/$name\n> refs/remotes/$nick/heads/$name\n>\n> (the first of the above is existing \"refs/remotes/%.*s\").\n\nThis is not what I have in mind with this series (although the behavior\nwill be identical for the refspecs suggested in the remote ref namespace\ndiscussion). Rather I would have the following three rules:\n\n refs/remotes/%.*s\n (which is - as you say - identical to your first rule)\n RHS from remote $nick's refspec matching LHS = refs/tags/$name\n RHS from remote $nick's refspec matching LHS = refs/heads/$name\n\nand indeed, my series implements the last of these (in addition to\nretaining the first). The middle one (for tags) would be added by a future\npatch series.\n\nThe whole point of mapping through the refspecs, is that we DON'T hardcode\nany preference as to where remote-tracking branches (or tags, or whatever)\nshould be located. It's _purely_ up to how the user has configured her\nrefspecs, and any expansion for remote-tracking refs should obey those\nrefspecs.\n\n> I think it is going too far if you extend it further to\n>\n> refs/remotes/$nick/*/$name\n>\n> where the code does not control what an acceptable match for '*' is\n> (i.e. origin/foo matching origin/changes/foo might be OK, but\n> matching it with origin/randomstring/foo is not, unless the canned\n> ref_rev_parse_rules[] knows about the \"randomstring\", or there is a\n> configuration mechanism for the user to tell us she cares about the\n> \"randomstring\" hierarchy in her project).\n\nFully agreed, although this is somewhat irrelevant to my objective.\nI don't want a list of \"blessed\" strings that will be automatically\ntraversed upon expansion at all (much less a configuration mechanism\nto customize it). I want the _refspec_ to be the king of deciding where\nremote-tracking branches and tags are\n\nLet me try to summarize my views on how refnames should work in Git, to\nsee if we can identify where we differ on the principles (or if we, in\nfact, differ at all):\n\n0. refnames must obviously follow the check-ref-format syntactic rules.\n\n1. refs should generally be placed within the refs/ hierarchy. Anything\n outside refs/ is only for Git's own special use (e.g. HEAD, FETCH_HEAD,\n etc.).\n\n2. refs may in general be placed anywhere within refs/, but there are some\n places that are interpreted specially by Git:\n - refs/heads/* hold local branches\n - refs/tags/* hold local tags\n - refs/replace/* hold local replace refs\n - refs/notes/* hold local notes\n - (there may be more here, for other ref types, e.g. refs/original/)\n\n3. refs that originate from a remote repository (a.k.a. remote-tracking\n refs) are typically placed within refs/remotes/*, however there are NO\n _hardcoded_ rules dictating how remote-tracking refs are to be\n organized.\n\n4. The organization of remote-tracking refs is determined by the\n configured (fetch) refspecs, which map remote refs to their remote-\n tracking counterparts (typically - but not necessarily - located\n within refs/remotes/*).\n\n5. Ideally, all refs within refs/remotes/* should correspond to exactly\n one refspec (i.e. be matched by the RHS side of that refspec) in one\n remote, although this is not a requirement.\n\n6. The user should be able to use shorthand notation for local refs when\n doing so is natural and unambiguous in the current context. For example\n in a branch context, \"$name\" should automatically be expanded into\n \"refs/heads/$name\", if doing so is unambiguous. Likewise for tags,\n notes and other contexts corresponding to the local ref types mentioned\n in #2.\n\n7. When there is no context limiting us to a specific ref type, we should\n still allow the shorthands from #6, as long as they are unambiguous.\n\n8. For remote-tracking refs, there is no _hardcoded_ structure along which\n we can expand shorthand notations. However, since the structure is\n defined by the configured (fetch) refspecs, we can use those same\n refspecs to expand the \"$nick/$name\" shorthand notation into the\n remote-tracking ref from remote \"$nick\" that (unambiguously) matches\n \"$name\". (When matching \"$name\" against $nick's refspecs, we should use\n the same expansions as in #6 to expand \"$name\" into something matching\n the LHS of a refspec (e.g. \"refs/heads/$name\", \"refs/tags/$name\"), and\n then use the corresponding RHS as the resulting expansion.)\n\n9. In addition to the refspec-sensitive expansion of shorthand notations\n described in #8, we probably also want to allow a blanket expansion of\n \"$anything\" into \"refs/remotes/$anything\", since that seems generally\n useful.\n\nSome notes:\n\n- Both the current default refspecs, and the refspecs suggested in the\n remote ref namespace discussion follow the above principles. Also, your\n non-integration single topic hierarchy mentioned below should work with\n these principles.\n\n- My rationale for #3 is that hardcoding a structure within refs/remotes/*\n will likely resist future inventions for how to deal with remote refs,\n and the likely result will be to create ad hoc solutions outside the\n refs/remotes/* hierarchy, like we have already seen with e.g.\n \"refs/remote-notes/*\". By leaving the organizing of refs/remotes/* up to\n the refspecs (#4), we are infinitely more customizable and future-proof\n when it comes to new ways of organizing remote refs.\n\n> [Footnotes]\n>\n> *1* I offhand do not remember if we even allow multi-level remote\n> nicks, but I do know we support multi-level branch names, so it\n> may turn out that the only valid split of origin/jh/rbranch is\n> topic 'jh/rbranch' from remote 'origin' and not topic 'rbranch'\n> from remote 'origin/jh'.\n\nWe do currently allow this. Observe:\n\n$ git init clobbering_remotes\nInitialized empty Git repository in ./clobbering_remotes/.git/\n$ cd clobbering_remotes/\n$ echo foo > foo\n$ git add foo\n$ git commit -m foo\n[master (root-commit) 5b5db04] foo\n 1 file changed, 1 insertion(+)\n create mode 100644 foo\n$ git checkout -b bar/master\nSwitched to a new branch 'bar/master'\n$ echo bar > foo\n$ git commit -am bar\n[bar/master 03f1d10] bar\n 1 file changed, 1 insertion(+), 1 deletion(-)\n$ git remote add foo .\n$ git fetch foo\nFrom .\n * [new branch] bar/master -> foo/bar/master\n * [new branch] master -> foo/master\n$ git rev-parse refs/remotes/foo/bar/master\n03f1d10c1456aec8b42e2432cb7726e92d0dc17a\n$ git remote add foo/bar .\n$ git fetch foo/bar\nFrom .\n * [new branch] bar/master -> foo/bar/bar/master\n + 03f1d10...5b5db04 master -> foo/bar/master (forced update)\n$ git rev-parse refs/remotes/foo/bar/master\n5b5db042e0af6f10bef7a3c82ce53e5c0ac9ab8a\n\nI would support disallowing multi-level remote names, although I don't\nknow if it is commonly used, and would break many existing users.\n\n> *2* Perhaps \"bar\" in the above is spelled \"topics\", and the\n> hierarchy may be used to collect non-integration single topic\n> branches from more than one remote. An example that is more in\n> line with such a usage might be:\n>\n> [remote \"jh\"]\n> fetch = +refs/heads/*:refs/remotes/topics/heads/jh/*\n> [remote \"jk\"]\n> fetch = +refs/heads/*:refs/remotes/topics/heads/jk/*\n> [remote \"fc\"]\n> fetch = +refs/heads/*:refs/remotes/topics/heads/fc/*\n>\n> and I would expect \"git merge topics/jh/rbranch\" to merge the\n> \"refs/remotes/topics/heads/jh/rbranch\" topic branch.\n\nI like the use case, but not necessarily your expectation. ;-)\n\nWith the above configuration, and my series as-is, you could simply do\n\"git merge jh/rbranch\" to merge the \"refs/remotes/topics/heads/jh/rbranch\"\ntopic branch. Furthermore, I don't see why you want/need the extra\n\"heads/\" level in the refspec. If you do this instead:\n\n [remote \"jh\"]\n fetch = +refs/heads/*:refs/remotes/topics/jh/*\n [remote \"jk\"]\n fetch = +refs/heads/*:refs/remotes/topics/jk/*\n [remote \"fc\"]\n fetch = +refs/heads/*:refs/remotes/topics/fc/*\n\nyour \"git merge topics/jh/rbranch\" should work with current Git. With my\npatch series on top, it would also be equivalent to \"git merge jh/rbranch\".\n\n\n...Johan\n\n--\nJohan Herland, \nwww.herland.net\n"},{"id":"216466","messageId":"7vvc6xt5ov.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgf6NcT2tEGMTczxR2WspOi4NjrN_kxmKN-QyE2Py3iSaQ@mail.gmail.com","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-05T22:36:32Z","receivedAt":"2013-05-05T22:36:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> This would not allow the user to use the relevant $remote_name for $nick,\n> which I argue might be the more natural name for the user to use, since\n> it's the same name that is used for otherwise interacting with the remote.\n\nThat is where we differ.\n\nThe thing is, when you name a local ref (be it \"refs/heads/master\"\nor \"refs/remotes/origin/master\") with a short-hand, you are still\ndealing with a refname, not interacting with the remote at all.\n\nTaking notice of remote.$nick.fetch mappings only to complicate the\nrefname resolution logic is absolutely unacceptable, at least to\nsomebody who comes from the \"we are interacting with refs, not with\nremotes\" school, like me.\n"},{"id":"216472","messageId":"CA+gHt1Aq+Hi5Uf-s+q5WaigHXP1Qyq100N=C4x4pwFf8-Q=GcA@mail.gmail.com","threadId":"33726","inReplyTo":"7vvc6xt5ov.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2013-05-06T01:02:31Z","receivedAt":"2013-05-06T01:02:31Z","isPatch":true,"sender":{"key":"santi@agolina.net","avatar":null},"body":"El 06/05/2013 00:36, \"Junio C Hamano\" escribió:\n>\n> Johan Herland writes:\n>\n> > This would not allow the user to use the relevant $remote_name for $nick,\n> > which I argue might be the more natural name for the user to use, since\n> > it's the same name that is used for otherwise interacting with the remote.\n>\n> That is where we differ.\n>\n> The thing is, when you name a local ref (be it \"refs/heads/master\"\n> or \"refs/remotes/origin/master\") with a short-hand, you are still\n> dealing with a refname, not interacting with the remote at all.\n>\n> Taking notice of remote.$nick.fetch mappings only to complicate the\n> refname resolution logic is absolutely unacceptable, at least to\n> somebody who comes from the \"we are interacting with refs, not with\n> remotes\" school, like me.\n\nMaybe we could mark it explicity with a double slash: \"$remote//$branch\",\nor similar. And it even allows a slash in the remote nick: \"bar/baz//foo\".\n\nSee you,\nSanti\n\nP.D: Resend because the list rejected it, sorry for the duplicate.\n"},{"id":"216473","messageId":"CA+gHt1DAy+OF-A8PiANM8k3=HdpsH8B-EWV5a3Dqv9svxCbZfA@mail.gmail.com","threadId":"33726","inReplyTo":"CA+gHt1Aq+Hi5Uf-s+q5WaigHXP1Qyq100N=C4x4pwFf8-Q=GcA@mail.gmail.com","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2013-05-06T01:04:29Z","receivedAt":"2013-05-06T01:04:29Z","isPatch":true,"sender":{"key":"santi@agolina.net","avatar":null},"body":"2013/5/6 Santi Béjar :\n> El 06/05/2013 00:36, \"Junio C Hamano\" escribió:\n>>\n>> Johan Herland writes:\n>>\n>> > This would not allow the user to use the relevant $remote_name for $nick,\n>> > which I argue might be the more natural name for the user to use, since\n>> > it's the same name that is used for otherwise interacting with the remote.\n>>\n>> That is where we differ.\n>>\n>> The thing is, when you name a local ref (be it \"refs/heads/master\"\n>> or \"refs/remotes/origin/master\") with a short-hand, you are still\n>> dealing with a refname, not interacting with the remote at all.\n>>\n>> Taking notice of remote.$nick.fetch mappings only to complicate the\n>> refname resolution logic is absolutely unacceptable, at least to\n>> somebody who comes from the \"we are interacting with refs, not with\n>> remotes\" school, like me.\n>\n> Maybe we could mark it explicity with a double slash: \"$remote//$branch\",\n> or similar. And it even allows a slash in the remote nick: \"bar/baz//foo\".\n\nThe next question could be: why not make it work with $url too? As in:\n\n$ git merge git://git.kernel.org/pub/scm/git/git.git//master\n\nBut I don't know if it can be problematic...\n\nI remember that there was a discussion about the remote#branch\nnotation used in cogito, and at the end it was rejected.\n\nSee you,\nSanti\n"},{"id":"216519","messageId":"7vhaigrqay.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgf6NcT2tEGMTczxR2WspOi4NjrN_kxmKN-QyE2Py3iSaQ@mail.gmail.com","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T17:06:29Z","receivedAt":"2013-05-06T17:06:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Let me try to summarize my views on how refnames should work in Git, to\n> see if we can identify where we differ on the principles (or if we, in\n> fact, differ at all):\n\nThanks; I think I already said where I think we differ in a separate\nmessage, but a short version is that the point of remote.$nick.fetch\nmapping is to solve \"The remote may call a ref $this, which is not\nthe refname I want to or can use in my repository, so here is the\nrule to use when importing it in my local namespace. With the\nmapping, I can name the ref in my local namespace conveniently.\"\nE.g. their \"refs/heads/master\" cannot be our \"refs/heads/master\" at\nthe same time, so use \"refs/heads/origin/master\".\n\nThe result of the above mapping, be it remotes/origin/master or\nremotes/origin/heads/master, should be designed to be useful for the\nlocal use of the ref in question. If you further need to remap it\nwhen using it locally, there is something wrong in the mapping you\ndefined in your remote.$nick.fetch mapping in the first place.\n\nWe do not force any structure under refs/remotes/; it is left\nentirely up to the user, even though we would like to suggest the\nbest current practice by teaching \"clone\" and \"remote add\" to lay\nthem out in a certain way.\n\nAnother thing is that refs/remotes/ is not special at all. If notes\nhierarchies taken from a remote need to be somewhere other than\nrefs/notes/, it is perfectly fine to introduce refs/remote-notes/ if\nthat is the best layout when using them locally. What is special is\nrefs/heads/ in that they are the _only_ refs you can check out to\nthe working tree and directly advance them by working on the working\ntree files.\n\n> I would support disallowing multi-level remote names, although I don't\n> know if it is commonly used, and would break many existing users.\n\nI somewhat doubt it.\n\nWe very much anticipated the use of multi-level branch names from\nthe very beginning and have support (e.g. in \"for-each-ref\" and\n\"branch --list\") to group/filter them according to prefixes, but I\ndo not think there is anywhere we consciously try to give support\nfor multi-level remote names to treat groups of remotes that share\nthe same prefix.\n\n>> *2* Perhaps \"bar\" in the above is spelled \"topics\", and the\n>> hierarchy may be used to collect non-integration single topic\n>> branches from more than one remote. An example that is more in\n>> line with such a usage might be:\n>>\n>> [remote \"jh\"]\n>> fetch = +refs/heads/*:refs/remotes/topics/heads/jh/*\n>> [remote \"jk\"]\n>> fetch = +refs/heads/*:refs/remotes/topics/heads/jk/*\n>> [remote \"fc\"]\n>> fetch = +refs/heads/*:refs/remotes/topics/heads/fc/*\n>>\n>> and I would expect \"git merge topics/jh/rbranch\" to merge the\n>> \"refs/remotes/topics/heads/jh/rbranch\" topic branch.\n>\n> I like the use case, but not necessarily your expectation. ;-)\n>\n> With the above configuration, and my series as-is, you could simply do\n> \"git merge jh/rbranch\" to merge the \"refs/remotes/topics/heads/jh/rbranch\"\n> topic branch.\n\nThat dropping of 'topics/' is the issue. The user wanted to group\nthem under 'topics/' hierarchy and made a conscous effort to set up\nthe fetch refspec to map these refs there. These are done all for\nconvenience when she deals with refs in her namespace in the\nrepository. What justification do we have to second guess the user\nand force her to drop it when naming these refs?\n\n> Furthermore, I don't see why you want/need the extra\n> \"heads/\" level in the refspec.\n\nJust like you wanted to have separate kinds of refs under a single\nremote, the layout is grouping kinds of refs other than branch heads\nrelated to the \"topics\" (as opposed to \"integration branches\").\n"},{"id":"216520","messageId":"7vd2t4rq2p.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CA+gHt1DAy+OF-A8PiANM8k3=HdpsH8B-EWV5a3Dqv9svxCbZfA@mail.gmail.com","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T17:11:26Z","receivedAt":"2013-05-06T17:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santi Béjar writes:\n\n> The next question could be: why not make it work with $url too? As in:\n>\n> $ git merge git://git.kernel.org/pub/scm/git/git.git//master\n\nYou need to remember \"merge\" is a local operation, working in your\nlocal repository. Like it or not, the UI of Git makes distinction\nbetween operations that are local and those that go to other\nrepositories (e.g. \"git pull\").\n\nThat of course does _not_ prevent you from writing an alternative UI\n\"got merge \" that interprets part and invokes\n\"git merge\" or \"git pull\" as its helper (after all, Git is designed\nto be scripted).\n"},{"id":"216523","messageId":"7v4negrpn1.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"7vhaigrqay.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T17:20:50Z","receivedAt":"2013-05-06T17:20:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\nJust a few typofixes...\n\n> Johan Herland writes:\n>\n>> Let me try to summarize my views on how refnames should work in Git, to\n>> see if we can identify where we differ on the principles (or if we, in\n>> fact, differ at all):\n>\n> Thanks; I think I already said where I think we differ in a separate\n> message, but a short version is that the point of remote.$nick.fetch\n> mapping is to solve \"The remote may call a ref $this, which is not\n> the refname I want to or can use in my repository, so here is the\n> rule to use when importing it in my local namespace. With the\n> mapping, I can name the ref in my local namespace conveniently.\"\n> E.g. their \"refs/heads/master\" cannot be our \"refs/heads/master\" at\n> the same time, so use \"refs/heads/origin/master\".\n\nThe last should be \"refs/remotes/origin/master\".\n\n>\n> The result of the above mapping, be it remotes/origin/master or\n> remotes/origin/heads/master, should be designed to be useful for the\n> local use of the ref in question. If you further need to remap it\n> when using it locally, there is something wrong in the mapping you\n> defined in your remote.$nick.fetch mapping in the first place.\n>\n> We do not force any structure under refs/remotes/; it is left\n> entirely up to the user, even though we would like to suggest the\n> best current practice by teaching \"clone\" and \"remote add\" to lay\n> them out in a certain way.\n>\n> Another thing is that refs/remotes/ is not special at all. If notes\n> hierarchies taken from a remote need to be somewhere other than\n> refs/notes/, it is perfectly fine to introduce refs/remote-notes/ if\n> that is the best layout when using them locally. What is special is\n\ns/the best/the most convenient/;\n\n> refs/heads/ in that they are the _only_ refs you can check out to\n> the working tree and directly advance them by working on the working\n> tree files.\n"},{"id":"216525","messageId":"7vy5bsq9m9.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-2-git-send-email-johan@herland.net","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T17:52:14Z","receivedAt":"2013-05-06T17:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> ... there is AFAICS _no_ way for sscanf() - having\n> already done one or more format extractions - to indicate to its caller\n> that the input fails to match the trailing part of the format string.\n\nYeah, we can detect when we did not have enough, but we cannot tell\nwhere it stopped matching.\n\nIt is interesting that this bug has stayed so long with us, which\nmay indicate that nobody actually uses the feature at all.\n\nGood eyes.\n\n>\n> Cc: Bert Wesarg \n> Signed-off-by: Johan Herland \n> ---\n> refs.c | 82 +++++++++++++++++++------------------------------\n> t/t6300-for-each-ref.sh | 12 ++++++++\n> 2 files changed, 43 insertions(+), 51 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index d17931a..7231f54 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2945,80 +2945,60 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n> \treturn NULL;\n> }\n> \n> +int shorten_ref(const char *refname, const char *pattern, char *short_name)\n\nDoes this need to be an extern?\n\n> {\n> +\t/*\n> +\t * pattern must be of the form \"[pre]%.*s[post]\". Check if refname\n> +\t * starts with \"[pre]\" and ends with \"[post]\". If so, write the\n> +\t * middle part into short_name, and return the number of chars\n> +\t * written (not counting the added NUL-terminator). Otherwise,\n> +\t * if refname does not match pattern, return 0.\n> +\t */\n> +\tsize_t pre_len, post_start, post_len, match_len;\n> +\tsize_t ref_len = strlen(refname);\n> +\tchar *sep = strstr(pattern, \"%.*s\");\n> +\tif (!sep || strstr(sep + 4, \"%.*s\"))\n> +\t\tdie(\"invalid pattern in ref_rev_parse_rules: %s\", pattern);\n> +\tpre_len = sep - pattern;\n> +\tpost_start = pre_len + 4;\n> +\tpost_len = strlen(pattern + post_start);\n> +\tif (pre_len + post_len >= ref_len)\n> +\t\treturn 0; /* refname too short */\n> +\tmatch_len = ref_len - (pre_len + post_len);\n> +\tif (strncmp(refname, pattern, pre_len) ||\n> +\t strncmp(refname + ref_len - post_len, pattern + post_start, post_len))\n> +\t\treturn 0; /* refname does not match */\n> +\tmemcpy(short_name, refname + pre_len, match_len);\n> +\tshort_name[match_len] = '\\0';\n> +\treturn match_len;\n> }\n\nOK. Looks correct, even though I suspect some people might come up\nwith a more concise way to express the above.\n\n> char *shorten_unambiguous_ref(const char *refname, int strict)\n> {\n> \tint i;\n> \tchar *short_name;\n> \n> \t/* skip first rule, it will always match */\n> -\tfor (i = nr_rules - 1; i > 0 ; --i) {\n> +\tfor (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n> \t\tint j;\n> \t\tint rules_to_fail = i;\n> \t\tint short_name_len;\n> \n> +\t\tif (!ref_rev_parse_rules[i] ||\n\nWhat is this skippage about? Isn't it what you already compensated\naway by starting from \"ARRAY_SIZE() - 1\"?\n\nAhh, no. But wait. Isn't there a larger issue here?\n\n> +\t\t !(short_name_len = shorten_ref(refname,\n> +\t\t\t\t\t\t ref_rev_parse_rules[i],\n> +\t\t\t\t\t\t short_name)))\n> \t\t\tcontinue;\n> \n> -\t\tshort_name_len = strlen(short_name);\n> -\n> \t\t/*\n> \t\t * in strict mode, all (except the matched one) rules\n> \t\t * must fail to resolve to a valid non-ambiguous ref\n> \t\t */\n> \t\tif (strict)\n> -\t\t\trules_to_fail = nr_rules;\n> +\t\t\trules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);\n\nIsn't nr_rules in the original is \"ARRAY_SIZE()-1\"?\n\n> \n> \t\t/*\n> \t\t * check if the short name resolves to a valid ref,\n\nCould you add a test to trigger the \"strict\" codepath?\n\nThanks.\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 752f5cb..57e3109 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -466,4 +466,16 @@ test_expect_success 'Verify sort with multiple keys' '\n> \t\trefs/tags/bogo refs/tags/master > actual &&\n> \ttest_cmp expected actual\n> '\n> +\n> +cat >expected <<\\EOF\n> +origin\n> +origin/master\n> +EOF\n> +\n> +test_expect_success 'Check refs/remotes/origin/HEAD shortens to origin' '\n> +\tgit remote set-head origin master &&\n> +\tgit for-each-ref --format=\"%(refname:short)\" refs/remotes >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n"},{"id":"216535","messageId":"CA+gHt1AiFgzjN0ZjEhQNUsQ1XfES36ChUNCHjGyUJsEWTSfpCQ@mail.gmail.com","threadId":"33726","inReplyTo":"7vd2t4rq2p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2013-05-06T19:17:11Z","receivedAt":"2013-05-06T19:17:11Z","isPatch":true,"sender":{"key":"santi@agolina.net","avatar":null},"body":"El 06/05/2013 19:11, \"Junio C Hamano\" va escriure:\n>\n> Santi Béjar writes:\n>\n> > The next question could be: why not make it work with $url too? As in:\n> >\n> > $ git merge git://git.kernel.org/pub/scm/git/git.git//master\n>\n> You need to remember \"merge\" is a local operation, working in your\n> local repository. Like it or not, the UI of Git makes distinction\n> between operations that are local and those that go to other\n> repositories (e.g. \"git pull\").\n>\n\nOf course! In fact I wanted to say \"git pull\".\n\nSanti\n"},{"id":"216560","messageId":"CALKQrgeegzzJ-2QNvdmeeugS0Aw7jrE4SM8S7zk+qPdfgRCMyg@mail.gmail.com","threadId":"33726","inReplyTo":"7vhaigrqay.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-06T23:42:58Z","receivedAt":"2013-05-06T23:42:58Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 6, 2013 at 7:06 PM, Junio C Hamano wrote:\n> Johan Herland writes:\n>> Let me try to summarize my views on how refnames should work in Git, to\n>> see if we can identify where we differ on the principles (or if we, in\n>> fact, differ at all):\n>\n> Thanks; I think I already said where I think we differ in a separate\n> message, but a short version is that the point of remote.$nick.fetch\n> mapping is to solve \"The remote may call a ref $this, which is not\n> the refname I want to or can use in my repository, so here is the\n> rule to use when importing it in my local namespace. With the\n> mapping, I can name the ref in my local namespace conveniently.\"\n> E.g. their \"refs/heads/master\" cannot be our \"refs/heads/master\" at\n> the same time, so use \"refs/remotes/origin/master\".\n>\n> The result of the above mapping, be it remotes/origin/master or\n> remotes/origin/heads/master, should be designed to be useful for the\n> local use of the ref in question. If you further need to remap it\n> when using it locally, there is something wrong in the mapping you\n> defined in your remote.$nick.fetch mapping in the first place.\n\nOk, so whereas I consider the refspec to be \"king\", and that the expansion\nfrom convenient shorthands to full remote-tracking refnames should be\nderived from the chosen refspec, you would (if I understand you correctly)\nrather have a constant (i.e. independent of remotes and refspecs) set of\nrules for expanding shorthands to full refnames, and if the user chooses\nrefspecs that don't mesh well with those rules, then that is the user's\nproblem, and not Git's.\n\n> We do not force any structure under refs/remotes/; it is left\n> entirely up to the user, even though we would like to suggest the\n> best current practice by teaching \"clone\" and \"remote add\" to lay\n> them out in a certain way.\n\nIf we were to suggest +refs/heads/*:refs/remotes/origin/heads/* as the\nbest practice, I assume you do want \"origin/master\" to keep working. And\nsince you do not want to use the configured refspec when expanding\n\"origin/master\" into \"refs/remotes/origin/heads/master\", then I assume\nyou would rather add a hardcoded (what I call a \"textual expansion\" in\nmy patches) rule that would map \"$nick/$name\" into\n\n /refs/remotes/$nick/heads/$name\n\nBut isn't the existence of such a rule evidence of us trying to impose\n(or at least hint) at a certain structure for refs/remotes/*?\n\nIn light of this, I'm interested in your thoughts about the following\nrelated problem that I've just started looking at:\n\ngit branch -r shows the remote-tracking branches in this repo. Currently,\nAFAICS, this just spits out all refs under refs/remotes/*. This behavior\nmust clearly be modified if we are to allow remote-tracking tags at\nrefs/remotes/$remote/tags/* (they currently show up in \"git branch -r\",\nbut shouldn't). One could say that the filter should merely change from\nrefs/remotes/* to refs/remotes/*/heads/*, but this would break for\nexisting (old-style) remotes. Should we add a heuristic for detecting when\nto use refs/remotes/* vs. refs/remotes/*/heads/* as a filter?\n\nMy approach would be to iterate through the configured remotes, and for\neach remote list all refs that match the RHS of the refspec whose LHS is\nrefs/heads/*. This would work for both old- and new-style remotes with\nno heuristics.\n\nIf you agree that my approach is correct for enumerating remote-tracking\nbranches, then what is different about using the refspec when expanding\nremote-tracking refs in general?\n\nIn other words, given the following configuration:\n\n [remote \"origin\"]\n +refs/heads/*:refs/foo/bar/baz/*\n [remote \"foo\"]\n +refs/heads/*:refs/remotes/origin/heads/*\n\n1. In your opininon, is refs/foo/bar/baz/master a remote-tracking branch?\n\n2. Should refs/foo/bar/baz/master be listed by \"git branch -r\"?\n\n3. Should the \"origin/master\" shorthand notation expand to\n refs/remotes/origin/heads/master from remote foo, or\n refs/foo/bar/baz/master from remote origin?\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216562","messageId":"7va9o7pogo.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-3-git-send-email-johan@herland.net","subject":"Re: [PATCH 2/7] t7900: Start testing usability of namespaced remote refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T01:29:11Z","receivedAt":"2013-05-07T01:29:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> +test_expect_success 'work-around \"clone\" with namespaced remote refs' '\n> +\trm -rf client &&\n> +\tgit init client &&\n> +\t(\n> +\t\tcd client &&\n> +\t\tgit remote add origin ../server &&\n> +\t\tgit config --unset-all remote.origin.fetch &&\n> +\t\tgit config --add remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/heads/*\" &&\n\nIf you were to do this, I think you should drop the \"remote add\norigin\" step and illustrate what configuration variables should be\nprepared (at least minimally---the final implementation of \"git\nclone --separate-remote-layout\" may add some other configuration\nvariable as a hint to say \"this remote is using the new layout\" or\nsomesuch) in this \"client\" repository.\n\nThat would make the test more self documenting.\n\nI am not convinced that it is a good idea to reuse \"remotes/origin\"\nhierarchy which traditionally has been branches-only like this,\nthough. It may be better to use\n\n\trefs/$remotes_new_layout/origin/{heads,tags,...}/*\n\nfor a value of $remotes_new_layout that is different from \"remote\",\nand teach the dwim_ref() machinery to pay attention to it, to avoid\nconfusion. Otherwise, you wouldn't be able to tell between a topic\nbranch that works on tags named \"tags/refactor\" under the old layout,\nand a tag that marks a good point in a refactoring effort \"refactor\"\nunder the new layout.\n\n> +\t\tgit config --add remote.origin.fetch \"+refs/tags/*:refs/remotes/origin/tags/*\" &&\n> +\t\tgit config --add remote.origin.fetch \"+refs/notes/*:refs/remotes/origin/notes/*\" &&\n> +\t\tgit config --add remote.origin.fetch \"+refs/replace/*:refs/remotes/origin/replace/*\" &&\n> +\t\tgit config remote.origin.tagopt \"--no-tags\" &&\n> +\t\tgit fetch &&\n> +\t\tgit checkout master\n> +\t) &&\n> +\ttest_clone client\n> +'\n> +\n> +test_done\n"},{"id":"216563","messageId":"7v61yvpof3.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-4-git-send-email-johan@herland.net","subject":"Re: [PATCH 3/7] t7900: Demonstrate failure to expand \"$remote/$branch\" according to refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T01:30:08Z","receivedAt":"2013-05-07T01:30:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> This test verifies that the following expressions all evaluate to the\n> full refname \"refs/remotes/origin/heads/master\":\n\nAs I've aleady said, I am not convinced that local refname\nresolution should pay attention to refspec mapping, so I won't look\nat this step.\n"},{"id":"216564","messageId":"7v1u9jpo4m.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-5-git-send-email-johan@herland.net","subject":"Re: [PATCH 4/7] refs.c: Refactor rules for expanding shorthand names into full refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T01:36:25Z","receivedAt":"2013-05-07T01:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> diff --git a/refs.c b/refs.c\n> index 7231f54..8b02140 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1724,7 +1724,24 @@ const char *prettify_refname(const char *name)\n> \t\t0);\n> }\n> \n> -const char *ref_rev_parse_rules[] = {\n> +static void ref_expand_txtly(const struct ref_expand_rule *rule,\n> +\t\t\t char *dst, size_t dst_len,\n> +\t\t\t const char *shortname, size_t shortname_len)\n> +{\n> +\tmksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);\n> +}\n> +\n> +const struct ref_expand_rule ref_expand_rules[] = {\n> +\t{ ref_expand_txtly, \"%.*s\" },\n> +\t{ ref_expand_txtly, \"refs/%.*s\" },\n> +\t{ ref_expand_txtly, \"refs/tags/%.*s\" },\n> +\t{ ref_expand_txtly, \"refs/heads/%.*s\" },\n> +\t{ ref_expand_txtly, \"refs/remotes/%.*s\" },\n> +\t{ ref_expand_txtly, \"refs/remotes/%.*s/HEAD\" },\n> +\t{ NULL, NULL }\n> +};\n> +\n> +static const char *ref_rev_parse_rules[] = {\n> \t\"%.*s\",\n> \t\"refs/%.*s\",\n> \t\"refs/tags/%.*s\",\n> @@ -1734,15 +1751,17 @@ const char *ref_rev_parse_rules[] = {\n> \tNULL\n> };\n> \n> -int refname_match(const char *abbrev_name, const char *full_name, const char **rules)\n> +int refname_match(const char *abbrev_name, const char *full_name,\n> +\t\t const struct ref_expand_rule *rules)\n> {\n> -\tconst char **p;\n> +\tconst struct ref_expand_rule *p;\n> \tconst int abbrev_name_len = strlen(abbrev_name);\n> +\tchar n[PATH_MAX];\n\nHmmm, is it too much to ask to do this without a fixed length\nbuffer? I think we have long learned the value of using strbuf to\navoid having to worry about buffer overruns.\n\nI am OK with the idea to make ref_expand_rules[] customizable, and\nthe overall strategy taken by this step to refactor the current code\nlooks reasonably sensible.\n"},{"id":"216565","messageId":"7vwqrbo97g.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-6-git-send-email-johan@herland.net","subject":"Re: [PATCH 5/7] refs.c: Refactor code for shortening full refnames into shorthand names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T01:44:03Z","receivedAt":"2013-05-07T01:44:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> This patch removes the only remaining user of ref_rev_parse_rules.\n> It has now been fully replaced by ref_expand_rules. Hence this patch\n> also removes ref_rev_parse_rules.\n\nYeah, I was wondering when you would do this while reading [4/7], as\nremoving the parse_rules[] would break shortener side, and leaving\nit in for long would risk allowing parse_rules[] and expand_rules[]\nto drift apart.\n\n> -const struct ref_expand_rule ref_expand_rules[] = {\n> -\t{ ref_expand_txtly, \"%.*s\" },\n> -\t{ ref_expand_txtly, \"refs/%.*s\" },\n> -\t{ ref_expand_txtly, \"refs/tags/%.*s\" },\n> -\t{ ref_expand_txtly, \"refs/heads/%.*s\" },\n> -\t{ ref_expand_txtly, \"refs/remotes/%.*s\" },\n> -\t{ ref_expand_txtly, \"refs/remotes/%.*s/HEAD\" },\n> -\t{ NULL, NULL }\n> -};\n\nI wonder if you planned the previous step a bit better, this removal\nof a large block of text could have come next to the replacement of\nit we see after the addition of ref_shorten_txtly() function.\n\n> +static char *ref_shorten_txtly(const struct ref_expand_rule *rule,\n> +\t\t\t const char *refname)\n> +{\n> +...\n> +}\n> \n> -static const char *ref_rev_parse_rules[] = {\n> -\t\"%.*s\",\n> -\t\"refs/%.*s\",\n> -\t\"refs/tags/%.*s\",\n> -\t\"refs/heads/%.*s\",\n> -\t\"refs/remotes/%.*s\",\n> -\t\"refs/remotes/%.*s/HEAD\",\n> -\tNULL\n> +const struct ref_expand_rule ref_expand_rules[] = {\n> +\t{ ref_expand_txtly, NULL, \"%.*s\" },\n> +\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/%.*s\" },\n> +\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/tags/%.*s\" },\n> +\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/heads/%.*s\" },\n> +\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s\" },\n> +\t{ ref_expand_txtly, ref_shorten_txtly, \"refs/remotes/%.*s/HEAD\" },\n> +\t{ NULL, NULL, NULL }\n> };\n> \n> int refname_match(const char *abbrev_name, const char *full_name,\n\n> char *shorten_unambiguous_ref(const char *refname, int strict)\n> {\n> \tint i;\n> \tchar *short_name;\n> \n> -\t/* buffer for scanf result, at most refname must fit */\n> -\tshort_name = xstrdup(refname);\n> -\n> -\t/* skip first rule, it will always match */\n> -\tfor (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n> +\tfor (i = ARRAY_SIZE(ref_expand_rules) - 1; i >= 0 ; --i) {\n> ...\n> \t\t/*\n> \t\t * in strict mode, all (except the matched one) rules\n> \t\t * must fail to resolve to a valid non-ambiguous ref\n> \t\t */\n> \t\tif (strict)\n> -\t\t\trules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);\n> +\t\t\trules_to_fail = ARRAY_SIZE(ref_expand_rules);\n\nThis part obviously depends on 1/7; do we still have an off-by-one\nchange from the original, or did I miscount when I reviewed 1/7?\n\nAgain, the overall strategy to refactor sounds sound.\n\nIt may be a lot simpler if you have ref_expand/shorten_append() and\nref_expand/shortn_append_with_HEAD() built-in helper functions.\nThen you can perform the expansion and contraction without \"%.*s\" at\nall.\n"},{"id":"216566","messageId":"7vsj1zo8zu.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"1367711749-8812-7-git-send-email-johan@herland.net","subject":"Re: [PATCH 6/7] refname_match(): Caller must declare if we're matching local or remote refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T01:48:37Z","receivedAt":"2013-05-07T01:48:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> refname_match() is used to check whether a given shorthand name matches a\n> given full refname, but that full refname does not always belong in the\n> local repo, rather it is sometimes taken from list of refs sent over from\n> a remote repo.\n\nThat \"local vs remote\" is a wrong way to think about things.\n\nAll refs you can feed to resolve_ref() and dwim_ref() are local, and\n\"remotes\" is not all that special. It is just a convention to store\nmany different kind of things per different hierarchy, and the only\nspecial hierarchy we have is refs/heads/* that can be updated by\nmaking new commits through the index and the working tree.\n"},{"id":"216569","messageId":"7vip2vo7wz.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgeegzzJ-2QNvdmeeugS0Aw7jrE4SM8S7zk+qPdfgRCMyg@mail.gmail.com","subject":"Re: [PATCH 0/7] Make \"$remote/$branch\" work with unconventional refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T02:11:56Z","receivedAt":"2013-05-07T02:11:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Ok, so whereas I consider the refspec to be \"king\", and that the expansion\n> from convenient shorthands to full remote-tracking refnames should be\n> derived from the chosen refspec, you would (if I understand you correctly)\n> rather have a constant (i.e. independent of remotes and refspecs) set of\n> rules for expanding shorthands to full refnames, and if the user chooses\n> refspecs that don't mesh well with those rules, then that is the user's\n> problem, and not Git's.\n\nYou need to dig your rhetoric from the other end of the tunnel. I\nwould consider the local namespace for the refs to be the \"king\",\nand we would design how they are resolved to be useful for user's\nlocal usage pattern. How the remote refs are copied by fetch\nfollowing refspecs is one of the many components (e.g. the dwimming\nmachinery \"checkout foo\" uses to guess that the user may want to\nfork from and integrate later with one of the refs under refs/remotes\nis one of them and it is not \"fetch\") to complement the refname\nresolution rule to support the local usage of refs.\n\n> In light of this, I'm interested in your thoughts about the following\n> related problem that I've just started looking at:\n>\n> git branch -r shows the remote-tracking branches in this repo. Currently,\n> .... Should we add a heuristic for detecting when\n> to use refs/remotes/* vs. refs/remotes/*/heads/* as a filter?\n\nDidn't I already said that I do not think repurposing refs/remotes/\nfor these \"unified\" copies is the best approach?\n\nA change that I think is a good thing to add on top of your [45]/7\nrefactoring is to allow the user to add custom expansion/contraction\nrules. Then the user can group refs regardless of \"remotes\" and\ngive meaningful shortening.\n\nAs I said in a very early review, viewing \"fetch refspec\" to be\n\"king\" and refspecs are the only way the user may want to group\nthings locally is myopic. The every-day usage of the local names\nought to be the king, and everything else should serve to make it\neasier to use.\n"},{"id":"216634","messageId":"CALKQrgcoz-+5Kb-Y1Ui9LhE=+pvcRUdAS+iRWXAfsYnV6+k34w@mail.gmail.com","threadId":"33726","inReplyTo":"7vy5bsq9m9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T18:49:34Z","receivedAt":"2013-05-07T18:49:34Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano wrote:\n> Johan Herland writes:\n>\n>> ... there is AFAICS _no_ way for sscanf() - having\n>> already done one or more format extractions - to indicate to its caller\n>> that the input fails to match the trailing part of the format string.\n>\n> Yeah, we can detect when we did not have enough, but we cannot tell\n> where it stopped matching.\n>\n> It is interesting that this bug has stayed so long with us, which\n> may indicate that nobody actually uses the feature at all.\n\nI don't know if people really care about whether\n\"refs/remotes/origin/HEAD\" shortens to \"origin/HEAD\" or \"origin\". I'm\nguessing that people _do_ depend on the reverse - having \"origin\"\nexpand into \"refs/remotes/origin/HEAD\", so we probably cannot rip out\nthe \"refs/remotes/%.*s/HEAD\" rule altogether...\n\n> Good eyes.\n>\n>> Cc: Bert Wesarg \n>> Signed-off-by: Johan Herland \n>> ---\n>> refs.c | 82 +++++++++++++++++++------------------------------\n>> t/t6300-for-each-ref.sh | 12 ++++++++\n>> 2 files changed, 43 insertions(+), 51 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index d17931a..7231f54 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2945,80 +2945,60 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n>> return NULL;\n>> }\n>>\n>> +int shorten_ref(const char *refname, const char *pattern, char *short_name)\n>\n> Does this need to be an extern?\n\nNope, it should be static. Will fix.\n\n>> {\n>> + /*\n>> + * pattern must be of the form \"[pre]%.*s[post]\". Check if refname\n>> + * starts with \"[pre]\" and ends with \"[post]\". If so, write the\n>> + * middle part into short_name, and return the number of chars\n>> + * written (not counting the added NUL-terminator). Otherwise,\n>> + * if refname does not match pattern, return 0.\n>> + */\n>> + size_t pre_len, post_start, post_len, match_len;\n>> + size_t ref_len = strlen(refname);\n>> + char *sep = strstr(pattern, \"%.*s\");\n>> + if (!sep || strstr(sep + 4, \"%.*s\"))\n>> + die(\"invalid pattern in ref_rev_parse_rules: %s\", pattern);\n>> + pre_len = sep - pattern;\n>> + post_start = pre_len + 4;\n>> + post_len = strlen(pattern + post_start);\n>> + if (pre_len + post_len >= ref_len)\n>> + return 0; /* refname too short */\n>> + match_len = ref_len - (pre_len + post_len);\n>> + if (strncmp(refname, pattern, pre_len) ||\n>> + strncmp(refname + ref_len - post_len, pattern + post_start, post_len))\n>> + return 0; /* refname does not match */\n>> + memcpy(short_name, refname + pre_len, match_len);\n>> + short_name[match_len] = '\\0';\n>> + return match_len;\n>> }\n>\n> OK. Looks correct, even though I suspect some people might come up\n> with a more concise way to express the above.\n\nYeah, I made it sort of explicit to convince myself I'd gotten it\nright. I'm sure the same can be expressed in fewer lines of code.\n\n>> char *shorten_unambiguous_ref(const char *refname, int strict)\n>> {\n>> int i;\n>> char *short_name;\n>>\n>> /* skip first rule, it will always match */\n>> - for (i = nr_rules - 1; i > 0 ; --i) {\n>> + for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n>> int j;\n>> int rules_to_fail = i;\n>> int short_name_len;\n>>\n>> + if (!ref_rev_parse_rules[i] ||\n>\n> What is this skippage about? Isn't it what you already compensated\n> away by starting from \"ARRAY_SIZE() - 1\"?\n\nThere are various things being skipped at various points... The\nref_rev_parse_rules array looks like this:\n\nconst char *ref_rev_parse_rules[] = {\n\t\"%.*s\",\n\t\"refs/%.*s\",\n\t\"refs/tags/%.*s\",\n\t\"refs/heads/%.*s\",\n\t\"refs/remotes/%.*s\",\n\t\"refs/remotes/%.*s/HEAD\",\n\tNULL\n};\n\nObviously we want to skip looking at the last (sentinel) entry. But\nthere's also no point in looking at the first, since it trivially\n\"shortens\" to itself.\n\nThe for loop in this function:\n>> - for (i = nr_rules - 1; i > 0 ; --i) {\n>> + for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {\n\nis about skipping the _first_ array entry (we start at the last index,\nand stop _before_ we reach 0).\n\nThe current line:\n>> + if (!ref_rev_parse_rules[i] ||\n\nis about skipping the last (sentinel) entry. The previous code did\nthis by doing a pre-pass where nr_rules is set to\nARRAY_SIZE(ref_rev_parse_rules) - 1. I should have obviously done the\nsame by initializing i to ARRAY_SIZE(ref_rev_parse_rules) - 2 in the\nabove for loop.\n\n> Ahh, no. But wait. Isn't there a larger issue here?\n>\n>> + !(short_name_len = shorten_ref(refname,\n>> + ref_rev_parse_rules[i],\n>> + short_name)))\n>> continue;\n>>\n>> - short_name_len = strlen(short_name);\n>> -\n>> /*\n>> * in strict mode, all (except the matched one) rules\n>> * must fail to resolve to a valid non-ambiguous ref\n>> */\n>> if (strict)\n>> - rules_to_fail = nr_rules;\n>> + rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);\n>\n> Isn't nr_rules in the original is \"ARRAY_SIZE()-1\"?\n\nTrue. Good catch.\n\n>>\n>> /*\n>> * check if the short name resolves to a valid ref,\n>\n> Could you add a test to trigger the \"strict\" codepath?\n\nI imagined the strict codepath was already being tested by the\naddition to t6300, seeing as core.warnAmbiguousRef defaults to true.\nObviously I will have to add some more tests to make sure I'm not\nscrewing things up.\n\nNew version coming up. I'm going to rip this patch out of the\nsurrounding series, since it doesn't really belong there anyway.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216635","messageId":"1367952856-30729-1-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"CALKQrgcoz-+5Kb-Y1Ui9LhE=+pvcRUdAS+iRWXAfsYnV6+k34w@mail.gmail.com","subject":"[PATCHv2 1/3] t1514: Add tests of shortening refnames in strict/loose mode","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T18:54:14Z","receivedAt":"2013-05-07T18:54:14Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"These tests verify the correct behavior of \"git rev-parse --abbrev-ref\"\nin both \"strict\" and \"loose\" modes. Really, it tests the correct behavior\nof refs.c:shorten_unambiguous_ref() with its 'strict' argument set to\neither true of false.\n\nSigned-off-by: Johan Herland \n---\n t/t1514-rev-parse-shorten_unambiguous_ref.sh | 75 ++++++++++++++++++++++++++++\n 1 file changed, 75 insertions(+)\n create mode 100755 t/t1514-rev-parse-shorten_unambiguous_ref.sh\n\ndiff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh\nnew file mode 100755\nindex 0000000..41e0162\n--- /dev/null\n+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh\n@@ -0,0 +1,75 @@\n+#!/bin/sh\n+\n+test_description='short refname disambiguation\n+\n+Create refs that share the same name, and make sure\n+\"git rev-parse --abbrev-ref\" can present them all with as short a name\n+as possible, while still being unambiguous.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit master_a &&\n+\tgit remote add origin . &&\n+\tgit fetch origin &&\n+\ttest_commit master_b &&\n+\tgit branch origin/master &&\n+\ttest_commit master_c &&\n+\tgit tag master &&\n+\ttest_commit master_d &&\n+\tgit update-ref refs/master master_d &&\n+\ttest_commit master_e\n+\ttest_commit master_f\n+'\n+\n+cat > expect.show-ref << EOF\n+$(git rev-parse master_f) refs/heads/master\n+$(git rev-parse master_b) refs/heads/origin/master\n+$(git rev-parse master_d) refs/master\n+$(git rev-parse master_a) refs/remotes/origin/master\n+$(git rev-parse master_c) refs/tags/master\n+$(git rev-parse master_a) refs/tags/master_a\n+$(git rev-parse master_b) refs/tags/master_b\n+$(git rev-parse master_c) refs/tags/master_c\n+$(git rev-parse master_d) refs/tags/master_d\n+$(git rev-parse master_e) refs/tags/master_e\n+$(git rev-parse master_f) refs/tags/master_f\n+EOF\n+\n+test_expect_success 'we have the expected ref layout' '\n+\tgit show-ref > actual.show-ref &&\n+\ttest_cmp expect.show-ref actual.show-ref\n+'\n+\n+test_shortname () {\n+\trefname=$1\n+\tmode=$2\n+\texpect_shortname=$3\n+\texpect_tag=$4\n+\techo \"$expect_shortname\" > expect.shortname &&\n+\tactual_shortname=\"$(git rev-parse --abbrev-ref=\"$mode\" \"$refname\")\" &&\n+\techo \"$actual_shortname\" > actual.shortname &&\n+\ttest_cmp expect.shortname actual.shortname &&\n+\tgit rev-parse --verify \"$expect_tag\" > expect.sha1 &&\n+\tgit rev-parse --verify \"$actual_shortname\" > actual.sha1 &&\n+\ttest_cmp expect.sha1 actual.sha1\n+}\n+\n+test_expect_success 'shortening refnames in strict mode' '\n+\ttest_shortname refs/heads/master strict heads/master master_f &&\n+\ttest_shortname refs/heads/origin/master strict heads/origin/master master_b &&\n+\ttest_shortname refs/master strict refs/master master_d &&\n+\ttest_shortname refs/remotes/origin/master strict remotes/origin/master master_a &&\n+\ttest_shortname refs/tags/master strict tags/master master_c\n+'\n+\n+test_expect_success 'shortening refnames in loose mode' '\n+\ttest_shortname refs/heads/master loose heads/master master_f &&\n+\ttest_shortname refs/heads/origin/master loose origin/master master_b &&\n+\ttest_shortname refs/master loose master master_d &&\n+\ttest_shortname refs/remotes/origin/master loose remotes/origin/master master_a &&\n+\ttest_shortname refs/tags/master loose tags/master master_c\n+'\n+\n+test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216636","messageId":"1367952856-30729-2-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367952856-30729-1-git-send-email-johan@herland.net","subject":"[PATCHv2 2/3] t1514: Demonstrate failure to correctly shorten \"refs/remotes/origin/HEAD\"","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T18:54:15Z","receivedAt":"2013-05-07T18:54:15Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"There is currently a bug in refs.c:shorten_unambiguous_ref() that causes\n\"refs/remotes/origin/HEAD\" to be shortened to \"origin/HEAD\" instead of\n\"origin\" (which is expected from matching against the \"refs/remotes/%.*s\"\npattern from refs.c:ref_rev_parse_rules).\n\nSigned-off-by: Johan Herland \n---\n t/t1514-rev-parse-shorten_unambiguous_ref.sh | 8 ++++++--\n t/t6300-for-each-ref.sh | 12 ++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh\nindex 41e0162..ad75436 100755\n--- a/t/t1514-rev-parse-shorten_unambiguous_ref.sh\n+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh\n@@ -20,6 +20,7 @@ test_expect_success 'setup' '\n \ttest_commit master_d &&\n \tgit update-ref refs/master master_d &&\n \ttest_commit master_e\n+\tgit update-ref refs/remotes/origin/HEAD master_e &&\n \ttest_commit master_f\n '\n \n@@ -27,6 +28,7 @@ cat > expect.show-ref << EOF\n $(git rev-parse master_f) refs/heads/master\n $(git rev-parse master_b) refs/heads/origin/master\n $(git rev-parse master_d) refs/master\n+$(git rev-parse master_e) refs/remotes/origin/HEAD\n $(git rev-parse master_a) refs/remotes/origin/master\n $(git rev-parse master_c) refs/tags/master\n $(git rev-parse master_a) refs/tags/master_a\n@@ -56,18 +58,20 @@ test_shortname () {\n \ttest_cmp expect.sha1 actual.sha1\n }\n \n-test_expect_success 'shortening refnames in strict mode' '\n+test_expect_failure 'shortening refnames in strict mode' '\n \ttest_shortname refs/heads/master strict heads/master master_f &&\n \ttest_shortname refs/heads/origin/master strict heads/origin/master master_b &&\n \ttest_shortname refs/master strict refs/master master_d &&\n+\ttest_shortname refs/remotes/origin/HEAD strict origin master_e &&\n \ttest_shortname refs/remotes/origin/master strict remotes/origin/master master_a &&\n \ttest_shortname refs/tags/master strict tags/master master_c\n '\n \n-test_expect_success 'shortening refnames in loose mode' '\n+test_expect_failure 'shortening refnames in loose mode' '\n \ttest_shortname refs/heads/master loose heads/master master_f &&\n \ttest_shortname refs/heads/origin/master loose origin/master master_b &&\n \ttest_shortname refs/master loose master master_d &&\n+\ttest_shortname refs/remotes/origin/HEAD loose origin master_e &&\n \ttest_shortname refs/remotes/origin/master loose remotes/origin/master master_a &&\n \ttest_shortname refs/tags/master loose tags/master master_c\n '\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 752f5cb..5d716c8 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -466,4 +466,16 @@ test_expect_success 'Verify sort with multiple keys' '\n \t\trefs/tags/bogo refs/tags/master > actual &&\n \ttest_cmp expected actual\n '\n+\n+cat >expected <<\\EOF\n+origin\n+origin/master\n+EOF\n+\n+test_expect_failure 'Check refs/remotes/origin/HEAD shortens to origin' '\n+\tgit remote set-head origin master &&\n+\tgit for-each-ref --format=\"%(refname:short)\" refs/remotes >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216637","messageId":"1367952856-30729-3-git-send-email-johan@herland.net","threadId":"33726","inReplyTo":"1367952856-30729-1-git-send-email-johan@herland.net","subject":"[PATCHv2 3/3] shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to origin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T18:54:16Z","receivedAt":"2013-05-07T18:54:16Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When expanding shorthand refs to full ref names (e.g. in dwim_ref()),\nwe use the ref_rev_parse_rules list of expansion patterns. This list\nallows \"origin\" to be expanded into \"refs/remotes/origin/HEAD\", by\nusing the \"refs/remotes/%.*s/HEAD\" pattern from that list.\n\nshorten_unambiguous_ref() exists to provide the reverse operation:\nturning a full ref name into a shorter (but still unambiguous) name.\nIt does so by matching the given refname against each pattern from\nthe ref_rev_parse_rules list (in reverse), and extracting the short-\nhand name from the matching rule.\n\nHowever, when given \"refs/remotes/origin/HEAD\" it fails to shorten it\ninto \"origin\", because we misuse the sscanf() function when matching\n\"refs/remotes/origin/HEAD\" against \"refs/remotes/%.*s/HEAD\": We end\nup calling sscanf like this:\n\n sscanf(\"refs/remotes/origin/HEAD\", \"refs/remotes/%s/HEAD\", short_name)\n\nIn this case, sscanf() will match the initial \"refs/remotes/\" part, and\nthen match the remainder of the refname against the \"%s\", and place it\n(\"origin/HEAD\") into short_name. The part of the pattern following the\n\"%s\" format is never verified, because sscanf() apparently does not\nneed to do that (it has performed the one expected format extraction,\nand will return 1 correspondingly; see [1] for more details).\n\nThis patch replaces the misuse of sscanf() with a fairly simple function\nthat manually matches the refname against patterns, and extracts the\nshorthand name.\n\n[1]: If we assume that sscanf() does not do a verification pass prior\nto format extraction, there is AFAICS _no_ way for sscanf() - having\nalready done one or more format extractions - to indicate to its caller\nthat the input fails to match the trailing part of the format string.\nIn other words, AFAICS, the scanf() family of function will only verify\nmatching input up to and including the last format specifier in the\nformat string. Any data following the last format specifier will not be\nverified. Yet another reason to consider the scanf functions harmful...\n\nCc: Bert Wesarg \nImproved-by: Junio C Hamano \nSigned-off-by: Johan Herland \n---\n refs.c | 77 ++++++++++------------------\n t/t1514-rev-parse-shorten_unambiguous_ref.sh | 4 +-\n t/t6300-for-each-ref.sh | 2 +-\n 3 files changed, 29 insertions(+), 54 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d17931a..a0ba2fd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2945,80 +2945,55 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n \treturn NULL;\n }\n \n-/*\n- * generate a format suitable for scanf from a ref_rev_parse_rules\n- * rule, that is replace the \"%.*s\" spec with a \"%s\" spec\n- */\n-static void gen_scanf_fmt(char *scanf_fmt, const char *rule)\n+static int shorten_ref(const char *refname, const char *pattern, char *short_name)\n {\n-\tchar *spec;\n-\n-\tspec = strstr(rule, \"%.*s\");\n-\tif (!spec || strstr(spec + 4, \"%.*s\"))\n-\t\tdie(\"invalid rule in ref_rev_parse_rules: %s\", rule);\n-\n-\t/* copy all until spec */\n-\tstrncpy(scanf_fmt, rule, spec - rule);\n-\tscanf_fmt[spec - rule] = '\\0';\n-\t/* copy new spec */\n-\tstrcat(scanf_fmt, \"%s\");\n-\t/* copy remaining rule */\n-\tstrcat(scanf_fmt, spec + 4);\n-\n-\treturn;\n+\t/*\n+\t * pattern must be of the form \"[pre]%.*s[post]\". If refname\n+\t * starts with \"[pre]\" and ends with \"[post]\", extract the middle\n+\t * part into short_name, and return the number of chars in the\n+\t * middle part (not counting the added NUL-terminator). Otherwise,\n+\t * if refname does not match pattern, return 0.\n+\t */\n+\tint match_len;\n+\tconst char *match_start, *sep = strstr(pattern, \"%.*s\");\n+\tif (!sep || strstr(sep + 4, \"%.*s\"))\n+\t\tdie(\"invalid pattern in ref_rev_parse_rules: %s\", pattern);\n+\tmatch_start = refname + (sep - pattern);\n+\tmatch_len = strlen(refname) - (strlen(pattern) - 4);\n+\tif (match_len <= 0 ||\n+\t strncmp(refname, pattern, match_start - refname) ||\n+\t strcmp(match_start + match_len, sep + 4))\n+\t\treturn 0; /* refname does not match */\n+\tmemcpy(short_name, match_start, match_len);\n+\tshort_name[match_len] = '\\0';\n+\treturn match_len;\n }\n \n char *shorten_unambiguous_ref(const char *refname, int strict)\n {\n \tint i;\n-\tstatic char **scanf_fmts;\n-\tstatic int nr_rules;\n \tchar *short_name;\n \n-\t/* pre generate scanf formats from ref_rev_parse_rules[] */\n-\tif (!nr_rules) {\n-\t\tsize_t total_len = 0;\n-\n-\t\t/* the rule list is NULL terminated, count them first */\n-\t\tfor (; ref_rev_parse_rules[nr_rules]; nr_rules++)\n-\t\t\t/* no +1 because strlen(\"%s\") < strlen(\"%.*s\") */\n-\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]);\n-\n-\t\tscanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n-\n-\t\ttotal_len = 0;\n-\t\tfor (i = 0; i < nr_rules; i++) {\n-\t\t\tscanf_fmts[i] = (char *)&scanf_fmts[nr_rules]\n-\t\t\t\t\t+ total_len;\n-\t\t\tgen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);\n-\t\t\ttotal_len += strlen(ref_rev_parse_rules[i]);\n-\t\t}\n-\t}\n-\n-\t/* bail out if there are no rules */\n-\tif (!nr_rules)\n-\t\treturn xstrdup(refname);\n-\n \t/* buffer for scanf result, at most refname must fit */\n \tshort_name = xstrdup(refname);\n \n \t/* skip first rule, it will always match */\n-\tfor (i = nr_rules - 1; i > 0 ; --i) {\n+\tfor (i = ARRAY_SIZE(ref_rev_parse_rules) - 2; i > 0 ; --i) {\n \t\tint j;\n \t\tint rules_to_fail = i;\n \t\tint short_name_len;\n \n-\t\tif (1 != sscanf(refname, scanf_fmts[i], short_name))\n+\t\tif (!(short_name_len = shorten_ref(refname,\n+\t\t\t\t\t\t ref_rev_parse_rules[i],\n+\t\t\t\t\t\t short_name)))\n \t\t\tcontinue;\n \n-\t\tshort_name_len = strlen(short_name);\n-\n \t\t/*\n \t\t * in strict mode, all (except the matched one) rules\n \t\t * must fail to resolve to a valid non-ambiguous ref\n \t\t */\n \t\tif (strict)\n-\t\t\trules_to_fail = nr_rules;\n+\t\t\trules_to_fail = ARRAY_SIZE(ref_rev_parse_rules) - 1;\n \n \t\t/*\n \t\t * check if the short name resolves to a valid ref,\ndiff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh\nindex ad75436..fd87ce3 100755\n--- a/t/t1514-rev-parse-shorten_unambiguous_ref.sh\n+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh\n@@ -58,7 +58,7 @@ test_shortname () {\n \ttest_cmp expect.sha1 actual.sha1\n }\n \n-test_expect_failure 'shortening refnames in strict mode' '\n+test_expect_success 'shortening refnames in strict mode' '\n \ttest_shortname refs/heads/master strict heads/master master_f &&\n \ttest_shortname refs/heads/origin/master strict heads/origin/master master_b &&\n \ttest_shortname refs/master strict refs/master master_d &&\n@@ -67,7 +67,7 @@ test_expect_failure 'shortening refnames in strict mode' '\n \ttest_shortname refs/tags/master strict tags/master master_c\n '\n \n-test_expect_failure 'shortening refnames in loose mode' '\n+test_expect_success 'shortening refnames in loose mode' '\n \ttest_shortname refs/heads/master loose heads/master master_f &&\n \ttest_shortname refs/heads/origin/master loose origin/master master_b &&\n \ttest_shortname refs/master loose master master_d &&\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 5d716c8..57e3109 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -472,7 +472,7 @@ origin\n origin/master\n EOF\n \n-test_expect_failure 'Check refs/remotes/origin/HEAD shortens to origin' '\n+test_expect_success 'Check refs/remotes/origin/HEAD shortens to origin' '\n \tgit remote set-head origin master &&\n \tgit for-each-ref --format=\"%(refname:short)\" refs/remotes >actual &&\n \ttest_cmp expected actual\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"216642","messageId":"7v61yujye5.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgcoz-+5Kb-Y1Ui9LhE=+pvcRUdAS+iRWXAfsYnV6+k34w@mail.gmail.com","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T21:03:30Z","receivedAt":"2013-05-07T21:03:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> New version coming up. I'm going to rip this patch out of the\n> surrounding series, since it doesn't really belong there anyway.\n\nThanks; will queue.\n"},{"id":"216643","messageId":"7vy5bqiij3.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgcoz-+5Kb-Y1Ui9LhE=+pvcRUdAS+iRWXAfsYnV6+k34w@mail.gmail.com","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T21:31:28Z","receivedAt":"2013-05-07T21:31:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano wrote:\n>> Johan Herland writes:\n>>\n>>> ... there is AFAICS _no_ way for sscanf() - having\n>>> already done one or more format extractions - to indicate to its caller\n>>> that the input fails to match the trailing part of the format string.\n>>\n>> Yeah, we can detect when we did not have enough, but we cannot tell\n>> where it stopped matching.\n>>\n>> It is interesting that this bug has stayed so long with us, which\n>> may indicate that nobody actually uses the feature at all.\n>\n> I don't know if people really care about whether\n> \"refs/remotes/origin/HEAD\" shortens to \"origin/HEAD\" or \"origin\". I'm\n> guessing that people _do_ depend on the reverse - having \"origin\"\n> expand into \"refs/remotes/origin/HEAD\", so we probably cannot rip out\n> the \"refs/remotes/%.*s/HEAD\" rule altogether...\n\nOh, no doubt about that reverse conversion.\n\nThe real reason nobody cared about refs/remotes/origin/HEAD is that\nnobody sane has anything but non-symbolic ref there. Your t1514\ndoes this:\n\n\t...\n\tgit update-ref refs/master master_d &&\n\ttest_commit master_e\n\tgit update-ref refs/remotes/origin/HEAD master_e &&\n\t...\n\nNowhere in the set-up sequence, you see anything that does\n\n\tgit symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master\n\nor any other branch we copied from the remote.\n\nAnd the shortening is done after dereferencing the synbolic ref.\nBecause of this, refs/remotes/origin/HEAD usually resolves to\norigin/master, not origin.\n\n t/t1514-rev-parse-shorten-unambiguous-ref.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t1514-rev-parse-shorten-unambiguous-ref.sh b/t/t1514-rev-parse-shorten-unambiguous-ref.sh\nindex fd87ce3..556ad16 100755\n--- a/t/t1514-rev-parse-shorten-unambiguous-ref.sh\n+++ b/t/t1514-rev-parse-shorten-unambiguous-ref.sh\n@@ -76,4 +76,11 @@ test_expect_success 'shortening refnames in loose mode' '\n \ttest_shortname refs/tags/master loose tags/master master_c\n '\n \n+test_expect_success 'shortening is done after dereferencing a symref' '\n+\tgit update-ref refs/remotes/frotz/master master_e &&\n+\tgit symbolic-ref refs/remotes/frotz/HEAD refs/remotes/frotz/master &&\n+\ttest_shortname refs/remotes/frotz/HEAD strict frotz/master master_e &&\n+\ttest_shortname refs/remotes/frotz/HEAD loose frotz/master master_e\n+'\n+\n test_done\n"},{"id":"216644","messageId":"CALKQrgctyZGf2z+=+qjcW-s0uyVCqw01pv6X2NG+8yyC3FoTvQ@mail.gmail.com","threadId":"33726","inReplyTo":"7va9o7pogo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/7] t7900: Start testing usability of namespaced remote refs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T21:52:59Z","receivedAt":"2013-05-07T21:52:59Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, May 7, 2013 at 3:29 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>> +test_expect_success 'work-around \"clone\" with namespaced remote refs' '\n>> + rm -rf client &&\n>> + git init client &&\n>> + (\n>> + cd client &&\n>> + git remote add origin ../server &&\n>> + git config --unset-all remote.origin.fetch &&\n>> + git config --add remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/heads/*\" &&\n>\n> If you were to do this, I think you should drop the \"remote add\n> origin\" step and illustrate what configuration variables should be\n> prepared (at least minimally---the final implementation of \"git\n> clone --separate-remote-layout\" may add some other configuration\n> variable as a hint to say \"this remote is using the new layout\" or\n> somesuch) in this \"client\" repository.\n\nSure, I can change the test into doing:\n\n\tcd client &&\n\tgit config remote.origin.url ../server &&\n\tgit config --add remote.origin.fetch\n\"+refs/heads/*:refs/remotes/origin/heads/*\" &&\n\tgit config --add remote.origin.fetch\n\"+refs/tags/*:refs/remotes/origin/tags/*\" &&\n\tgit config --add remote.origin.fetch\n\"+refs/notes/*:refs/remotes/origin/notes/*\" &&\n\tgit config --add remote.origin.fetch\n\"+refs/replace/*:refs/remotes/origin/replace/*\" &&\n\tgit config remote.origin.tagopt \"--no-tags\" &&\n\tgit fetch &&\n\tgit checkout master\n\n> That would make the test more self documenting.\n>\n> I am not convinced that it is a good idea to reuse \"remotes/origin\"\n> hierarchy which traditionally has been branches-only like this,\n> though. It may be better to use\n>\n> refs/$remotes_new_layout/origin/{heads,tags,...}/*\n>\n> for a value of $remotes_new_layout that is different from \"remote\",\n> and teach the dwim_ref() machinery to pay attention to it, to avoid\n> confusion. Otherwise, you wouldn't be able to tell between a topic\n> branch that works on tags named \"tags/refactor\" under the old layout,\n> and a tag that marks a good point in a refactoring effort \"refactor\"\n> under the new layout.\n\nI see your point, although I'm not convinced it is common among users\nto have branch names of the \"tags/*\" form (or tag names of the\n\"heads/*\" form, for that matter). I'm also not sure it's worth messing\nwith the \"remotes\" name which has had a long time to work its way into\nour brains and into git's user interface.\n\nThat said, I could have a go at using \"refs/peers/*\" instead of\n\"refs/remotes/*\", and see how that works out.\n\nIf it sticks, how pervasive do we want this renaming to be? I guess we\ndon't want to rename the \"git remote\" command to \"git peer\" just\nyet... What about the config? Do we rename \"remote.origin.url\" to\n\"peer.origin.url\" for new-style remotes? For how long do you\nanticipate having \"peers\" and \"remotes\" living side-by-side as\nconcepts in git?\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216657","messageId":"CALKQrgdaXOjXFeWSpGtgqKhALqpRN0L7VEMbNf+93UJEBTD9ig@mail.gmail.com","threadId":"33726","inReplyTo":"7vy5bqiij3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T22:03:35Z","receivedAt":"2013-05-07T22:03:35Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, May 7, 2013 at 11:31 PM, Junio C Hamano wrote:\n> Johan Herland writes:\n>> On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano wrote:\n>>> It is interesting that this bug has stayed so long with us, which\n>>> may indicate that nobody actually uses the feature at all.\n>>\n>> I don't know if people really care about whether\n>> \"refs/remotes/origin/HEAD\" shortens to \"origin/HEAD\" or \"origin\". I'm\n>> guessing that people _do_ depend on the reverse - having \"origin\"\n>> expand into \"refs/remotes/origin/HEAD\", so we probably cannot rip out\n>> the \"refs/remotes/%.*s/HEAD\" rule altogether...\n>\n> Oh, no doubt about that reverse conversion.\n>\n> The real reason nobody cared about refs/remotes/origin/HEAD is that\n> nobody sane has anything but non-symbolic ref there. Your t1514\n> does this:\n>\n> ...\n> git update-ref refs/master master_d &&\n> test_commit master_e\n\n...oops, I see I forgot the trailing && on this line. Do you want a\nresend, or fix up yourself?\n\n> git update-ref refs/remotes/origin/HEAD master_e &&\n> ...\n>\n> Nowhere in the set-up sequence, you see anything that does\n>\n> git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master\n>\n> or any other branch we copied from the remote.\n\nCorrect. I first did a \"git remote set-head origin master\", but\nquickly discovered that rev-parse resolved the symref as part of\n--abbrev-ref, so I had to fake up a non-symref to trigger the\nshortening logic I wanted to test.\n\n> And the shortening is done after dereferencing the symbolic ref.\n> Because of this, refs/remotes/origin/HEAD usually resolves to\n> origin/master, not origin.\n>\n> t/t1514-rev-parse-shorten-unambiguous-ref.sh | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/t/t1514-rev-parse-shorten-unambiguous-ref.sh b/t/t1514-rev-parse-shorten-unambiguous-ref.sh\n> index fd87ce3..556ad16 100755\n> --- a/t/t1514-rev-parse-shorten-unambiguous-ref.sh\n> +++ b/t/t1514-rev-parse-shorten-unambiguous-ref.sh\n> @@ -76,4 +76,11 @@ test_expect_success 'shortening refnames in loose mode' '\n> test_shortname refs/tags/master loose tags/master master_c\n> '\n>\n> +test_expect_success 'shortening is done after dereferencing a symref' '\n> + git update-ref refs/remotes/frotz/master master_e &&\n> + git symbolic-ref refs/remotes/frotz/HEAD refs/remotes/frotz/master &&\n> + test_shortname refs/remotes/frotz/HEAD strict frotz/master master_e &&\n> + test_shortname refs/remotes/frotz/HEAD loose frotz/master master_e\n> +'\n> +\n> test_done\n\nTrue. I'm not sure whether that's a feature or a bug in --abbrev-ref,\nprobably a feature.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216661","messageId":"7vtxmeigw1.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgdaXOjXFeWSpGtgqKhALqpRN0L7VEMbNf+93UJEBTD9ig@mail.gmail.com","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T22:06:54Z","receivedAt":"2013-05-07T22:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> ...oops, I see I forgot the trailing && on this line. Do you want a\n> resend, or fix up yourself?\n\nI've pushed out a heavily fixed-up version on 'pu', mostly for\nstyles and some log message changes to describe \"when it is not a\nsymref\".\n"},{"id":"216664","messageId":"7vppx2ig8y.fsf@alter.siamese.dyndns.org","threadId":"33726","inReplyTo":"CALKQrgctyZGf2z+=+qjcW-s0uyVCqw01pv6X2NG+8yyC3FoTvQ@mail.gmail.com","subject":"Re: [PATCH 2/7] t7900: Start testing usability of namespaced remote refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T22:20:45Z","receivedAt":"2013-05-07T22:20:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> That said, I could have a go at using \"refs/peers/*\" instead of\n> \"refs/remotes/*\", and see how that works out.\n\nHmm, I had \"refs/track/\" in mind. Perhaps \"peers\" may work as well.\n\n> If it sticks, how pervasive do we want this renaming to be? I guess we\n> don't want to rename the \"git remote\" command to \"git peer\" just\n> yet.\n\nIf we were to do this, I would expect that the transition would be\nsimilar to the way we introduced the separate remote layout. The\neffort was started at around v1.3.0 era and we allowed the users to\nchoose the layout when they make a new clone for quite some time,\nuntil we made it the default at v1.5.0 boundary, IIRC. Let the user\nopt into using the new layout first, and then if the new layout\nturns out to be vastly more useful than the current one, then the\nuserbase will welcome it as the new default (and otherwise, it won't\nbecome the new default).\n\nWe _should_ be able to tell the layout being used by checking which\nof refs/peers/ or refs/remotes/ is populated, but I do not mind if\nwe added core.remoteLayout configuration variable that explicitly\ntells us which, if such an explicit clue turns out necessary.\n"},{"id":"216666","messageId":"CALKQrgeFZPM3xczxyiN_vjjhaJ_tevqPRPoAh7BBSWpCp1F0=w@mail.gmail.com","threadId":"33726","inReplyTo":"7vtxmeigw1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-07T22:37:19Z","receivedAt":"2013-05-07T22:37:19Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, May 8, 2013 at 12:06 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>\n>> ...oops, I see I forgot the trailing && on this line. Do you want a\n>> resend, or fix up yourself?\n>\n> I've pushed out a heavily fixed-up version on 'pu', mostly for\n> styles and some log message changes to describe \"when it is not a\n> symref\".\n\nLooks good to me.\n\nThanks!\n\n-- \nJohan Herland, \nwww.herland.net\n"}]}