{"thread":{"id":"33796","subject":"[PATCHv2 00/10] Prepare for alternative remote-tracking branch location","startedAt":"2013-05-11T16:21:10Z","lastAt":"2013-05-16T12:14:40Z","messageCount":31,"participants":["Johan Herland","Eric Sunshine","Junio C Hamano","Michael Haggerty","Eric Wong","Ramkumar Ramachandra"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"217013","messageId":"1368289280-30337-1-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":null,"subject":"[PATCHv2 00/10] Prepare for alternative remote-tracking branch location","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:10Z","receivedAt":"2013-05-11T16:21:10Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Hi,\n\nHere is the second iteration of the current series for teaching git to\nwork with remote ref namespaces. This iteration is pretty much a full\nrework of the first iteration, based on Junio's comments to the first\niteration, and discussion with other Git developers at the Git Merge\nconference in Berlin, particularily Jonathan Nieder has helped to work\nout some of these issues.\n\nPatches #1 - #2 are pretty much unchanged from v1, except that we now\nput the remote refs in refs/peers/* instead of /refs/remotes/*.\n\nPatches #3 - #5 gets us to the point where \"git rev-parse origin/master\"\nwill expand to \"refs/peers/origin/heads/master\", and vice versa for\nshortening\n\nPatches #6-#10 (except #7 which is a small unrelated test addition) is\nabout making \"git branch -a\" and \"git branch -r\" output remote-tracking\nbranches from the refs/peers/* hierarchy.\n\nNote that this patch series is based on top of the jh/shorten-refname\nseries in 'pu'.\n\n\nHave fun!\n\n...Johan\n\n\nJohan Herland (10):\n t7900: Start testing usability of namespaced remote refs\n t7900: Demonstrate failure to expand \"$peer/$branch\" according to refspecs\n refs.c: Refactor code for mapping between shorthand names and full refnames\n remote: Reject remote names containing '/'\n refs.c: Add support for expanding/shortening refs in refs/peers/*\n t7900: Test git branch -r/-a output w/remote-tracking branches in refs/peers/*\n t3203: Add testcase for fix in 1603ade81352a526ccb206f41ff81ecbc855df2d\n builtin/branch.c: Refactor ref_item.name and .dest into strbufs\n builtin/branch.c: Refactor \"remotes/\" prepending to remote-tracking branches\n branch: Fix display of remote branches in refs/peers/*\n\n builtin/branch.c | 114 +++++------\n builtin/remote.c | 4 +-\n cache.h | 4 -\n refs.c | 256 ++++++++++++++++++-------\n refs.h | 6 +\n remote.c | 6 +-\n t/t3203-branch-output.sh | 15 ++\n t/t5505-remote.sh | 12 ++\n t/t7900-working-with-namespaced-remote-refs.sh | 131 +++++++++++++\n t/t7901-multi-level-remote-name-failure.sh | 20 ++\n 10 files changed, 430 insertions(+), 138 deletions(-)\n create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh\n create mode 100755 t/t7901-multi-level-remote-name-failure.sh\n\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217014","messageId":"1368289280-30337-2-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 01/10] t7900: Start testing usability of namespaced remote refs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:11Z","receivedAt":"2013-05-11T16:21:11Z","isPatch":false,"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/peers/origin/heads/*\n\tfetch = +refs/tags/*:refs/peers/origin/tags/*\n\tfetch = +refs/notes/*:refs/peers/origin/notes/*\n\tfetch = +refs/replace/*:refs/peers/origin/replace/*\n\ttagopt = \"--no-tags\"\n\turl = ...\n\nThis configuration creates a separate namespace under refs/peers/origin/*\nmirroring the structure of local refs (under refs/*) where all the relevant\nrefs from the 'origin' remote can be found. The reason for renaming the\nrefs/remotes/ prefix to refs/peers/ is to allow Git to treat the two\nhierarchies somewhat differently with respect to how shorthand names are\nexpanded by the ref machinery.\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 | 82 ++++++++++++++++++++++++++\n 1 file changed, 82 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..dfd916b\n--- /dev/null\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -0,0 +1,82 @@\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/peers/origin/heads/*\n+\tfetch = +refs/tags/*:refs/peers/origin/tags/*\n+\tfetch = +refs/notes/*:refs/peers/origin/notes/*\n+\tfetch = +refs/replace/*:refs/peers/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/peers/origin/heads/*\n++refs/tags/*:refs/peers/origin/tags/*\n++refs/notes/*:refs/peers/origin/notes/*\n++refs/replace/*:refs/peers/origin/replace/*\n+EOF\n+\n+cat >expect.show-ref << EOF\n+$server_master_b refs/heads/master\n+$server_master_b refs/peers/origin/heads/master\n+$server_other_b refs/peers/origin/heads/other\n+$server_master_a refs/peers/origin/tags/server_master_a\n+$server_master_b refs/peers/origin/tags/server_master_b\n+$server_other_b refs/peers/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 --layout=peers server client &&\n+\ttest_clone client\n+'\n+\n+# Work-around for the not-yet-existing clone option used above\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 config remote.origin.url ../server &&\n+\t\tgit config --add remote.origin.fetch \"+refs/heads/*:refs/peers/origin/heads/*\" &&\n+\t\tgit config --add remote.origin.fetch \"+refs/tags/*:refs/peers/origin/tags/*\" &&\n+\t\tgit config --add remote.origin.fetch \"+refs/notes/*:refs/peers/origin/notes/*\" &&\n+\t\tgit config --add remote.origin.fetch \"+refs/replace/*:refs/peers/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":"217015","messageId":"1368289280-30337-3-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 02/10] t7900: Demonstrate failure to expand \"$peer/$branch\" according to refspecs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:12Z","receivedAt":"2013-05-11T16:21:12Z","isPatch":false,"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/peers/origin/heads/master\":\n\n - refs/peers/origin/heads/master\n - peers/origin/heads/master\n - origin/heads/master\n - origin/master\n\n(We assume that there are no other conflicting refs for which the above\nexpressions would cause ambiguity.)\n\nCurrently none of these work, because the refs machinery don't know how\nto expand shorthand names within the refs/peers/* hierarchy.\n\nMirroring the expansion of the above 4 expressions into the full refname,\nthe same 4 expressions 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 dfd916b..109e9b8 100755\n--- a/t/t7900-working-with-namespaced-remote-refs.sh\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -79,4 +79,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/peers/origin/heads/master >expect &&\n+\tgit rev-parse --symbolic-full-name refs/peers/origin/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --symbolic-full-name peers/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/peers/origin/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse --abbrev-ref peers/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":"217023","messageId":"1368289280-30337-4-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:13Z","receivedAt":"2013-05-11T16:21:13Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This patch is in preparation for extending the ways in which we expand\nshorthand names into full refnames, and shorten full refnames into\nunambiguous shorthand names.\n\nWe collect the logic for performing the expansion and shortening into two\nfunctions: refname_expand() and refname_shorten(). refname_expand() takes\na shorthand name, and a pattern containing a wildcard (more on those\nbelow), and substitutes the shorthand name for the wildcard. The result\nis written into a strbuf provided by the caller. Correspondingly,\nrefname_shorten() takes a full refname and matches it against the given\npattern, extracting the portion of the full refname that matches the\npattern wildcard. The resulting shorthand name is written into a strbuf\nprovided by the caller.\n\nThe refname_expand() function no longer uses mkpath()/mksnpath() to\nperform the pattern expansion. Instead, it uses strbuf_expand(), which\nremoves the need for using fixed-length buffers from the code. This also\nmeans we no longer call cleanup_path() on the resulting refs, but AFAICS,\nthis is not really needed anyway (also it breaks no existing tests).\n\nSince the pattern expansion is now done by strbuf_expand(), the syntax of\nthe pattern wildcard must change as well. Whereas a pattern used to look\nlike this:\n\n \"refs/remotes/%.*s/HEAD\"\n\nit now looks like this:\n\n \"refs/remotes/%*/HEAD\"\n\nwhere the \"%*\" wildcard is meant to be read as \"the full shorthand name\".\nMore wildcards will be added by future patches that require more complex\nwildcard matching.\n\nThe list of patterns has been renamed from \"ref_rev_parse_rules\" to\n\"refname_patterns\" (which should be more descriptive), and it has lost\nthe NULL terminator, which is no longer needed, since all iterations of\nthe list now uses ARRAY_SIZE(refname_patterns) to end iteration, instead\nof testing each member against NULL.\n\nFurthermore, refname_match() used to take the list of patterns as an\nargument, but since all callers pass the one and only pattern list, we\ncan drop the argument altogether, and use refname_patterns directly in\nrefname_match(). This also enables us to make the pattern list static.\n\nThe code for shortening full refnames into shorthands, has been adjusted\nto deal with the new \"%*\" patterns. It now uses strbuf_split_str() with\nthe '%' as terminator, to split the prefix part from the suffix part.\nThen - as before - it matches the prefix from the start of the given\nrefname and the suffix from the end of the refname. If both matches are\nsuccessful, the middle portion is extracted as the resulting shorthand\nname.\n\nThe end result of this refactoring is equivalent in behavior to the\nexisting code, but makes it easier for future patches to adjust how\nexpansion and shortening happens.\n\nAll of the existing users of ref_rev_parse_rules are converted to\nrefname_patterns by this patch, so ref_rev_parse_rules is removed\nas well.\n\nSigned-off-by: Johan Herland \n---\n cache.h | 4 --\n refs.c | 187 ++++++++++++++++++++++++++++++++++++++-------------------------\n refs.h | 3 +\n remote.c | 6 +-\n 4 files changed, 119 insertions(+), 81 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ac620b2..0cc43b2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -874,10 +874,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 006f369..ab5e120 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1783,27 +1783,91 @@ const char *prettify_refname(const char *name)\n \t\t0);\n }\n \n-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+static const char *refname_patterns[] = {\n+\t\"%*\",\n+\t\"refs/%*\",\n+\t\"refs/tags/%*\",\n+\t\"refs/heads/%*\",\n+\t\"refs/remotes/%*\",\n+\t\"refs/remotes/%*/HEAD\"\n };\n \n-int refname_match(const char *abbrev_name, const char *full_name, const char **rules)\n+struct wildcard_data {\n+\tconst char *s;\n+\tsize_t len;\n+\tint done;\n+};\n+\n+static size_t refname_expand_helper(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n {\n-\tconst char **p;\n-\tconst int abbrev_name_len = strlen(abbrev_name);\n+\tstruct wildcard_data *cb = context;\n+\tif (*placeholder == '*' && !cb->done) {\n+\t\tstrbuf_add(sb, cb->s, cb->len);\n+\t\tcb->done = 1;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n \n-\tfor (p = rules; *p; p++) {\n-\t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {\n+static int refname_expand(struct strbuf *dst, const char *pattern,\n+\t\t\t const char *shortname, size_t shortname_len)\n+{\n+\tstruct wildcard_data cbdata = { shortname, shortname_len, 0 };\n+\tstrbuf_reset(dst);\n+\tstrbuf_expand(dst, pattern, refname_expand_helper, &cbdata);\n+\tif (!cbdata.done)\n+\t\tstrbuf_reset(dst);\n+\treturn !cbdata.done;\n+}\n+\n+static int refname_shorten(struct strbuf *dst, const char *pattern,\n+\t\t\t const char *refname, size_t refname_len)\n+{\n+\t/*\n+\t * Match refname against pattern, using \"%*\" as wildcard, and\n+\t * extract the wildcard-matching portion of refname into dst.\n+\t * Return 0 on success (positive match, wildcard-matching portion\n+\t * copied into dst), and non-zero on failure (no match, dst empty).\n+\t */\n+\tconst char *match_end;\n+\tstruct strbuf **fragments = strbuf_split_str(pattern, '%', 2);\n+\tstruct strbuf *prefix = fragments[0], *suffix = fragments[1];\n+\tassert(!fragments[2]);\n+\tassert(prefix->len && prefix->buf[prefix->len - 1] == '%');\n+\tassert(suffix->len && suffix->buf[0] == '*');\n+\n+\tstrbuf_reset(dst);\n+\tmatch_end = refname + refname_len - (suffix->len - 1);\n+\tif (refname_len <= (prefix->len - 1) + (suffix->len - 1) ||\n+\t memcmp(prefix->buf, refname, prefix->len - 1) ||\n+\t memcmp(suffix->buf + 1, match_end, suffix->len - 1)) {\n+\t\tstrbuf_list_free(fragments);\n+\t\treturn 1; /* refname does not match pattern */\n+\t}\n+\n+\trefname += prefix->len - 1;\n+\tstrbuf_add(dst, refname, match_end - refname);\n+\tstrbuf_list_free(fragments);\n+\treturn 0;\n+}\n+\n+int refname_match(const char *abbrev_name, const char *full_name)\n+{\n+\tsize_t i;\n+\tconst size_t abbrev_len = strlen(abbrev_name);\n+\tstruct strbuf exp = STRBUF_INIT;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(refname_patterns); i++) {\n+\t\tif (!refname_expand(&exp, refname_patterns[i],\n+\t\t\t\t abbrev_name, abbrev_len) &&\n+\t\t !strcmp(full_name, exp.buf)) {\n+\t\t\tstrbuf_release(&exp);\n \t\t\treturn 1;\n \t\t}\n \t}\n \n+\tstrbuf_release(&exp);\n \treturn 0;\n }\n \n@@ -1866,30 +1930,33 @@ 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+\tstruct strbuf exp = STRBUF_INIT;\n \tint refs_found = 0;\n+\tsize_t i;\n \n \t*ref = NULL;\n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tchar fullref[PATH_MAX];\n+\tfor (i = 0; i < ARRAY_SIZE(refname_patterns); i++) {\n \t\tunsigned char sha1_from_ref[20];\n \t\tunsigned char *this_result;\n+\t\tconst char *r;\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\tr = resolve_ref_unsafe(fullref, this_result, 1, &flag);\n+\t\tif (refname_expand(&exp, refname_patterns[i], str, len))\n+\t\t\tcontinue;\n+\t\tr = resolve_ref_unsafe(exp.buf, 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-\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\")) {\n-\t\t\twarning(\"ignoring dangling symref %s.\", fullref);\n-\t\t} else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {\n-\t\t\twarning(\"ignoring broken ref %s.\", fullref);\n+\t\t} else if ((flag & REF_ISSYMREF) && strcmp(exp.buf, \"HEAD\")) {\n+\t\t\twarning(\"ignoring dangling symref %s.\", exp.buf);\n+\t\t} else if ((flag & REF_ISBROKEN) && strchr(exp.buf, '/')) {\n+\t\t\twarning(\"ignoring broken ref %s.\", exp.buf);\n \t\t}\n \t}\n+\tstrbuf_release(&exp);\n \tfree(last_branch);\n \treturn refs_found;\n }\n@@ -1897,24 +1964,25 @@ 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+\tstruct strbuf path = STRBUF_INIT;\n \tint logs_found = 0;\n+\tsize_t i;\n \n \t*log = NULL;\n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n+\tfor (i = 0; i < ARRAY_SIZE(refname_patterns); i++) {\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\tref = resolve_ref_unsafe(path, hash, 1, NULL);\n+\t\tif (refname_expand(&path, refname_patterns[i], str, len))\n+\t\t\tcontinue;\n+\t\tref = resolve_ref_unsafe(path.buf, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-\t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n+\t\tif (!stat(git_path(\"logs/%s\", path.buf), &st) &&\n \t\t S_ISREG(st.st_mode))\n-\t\t\tit = path;\n-\t\telse if (strcmp(ref, path) &&\n+\t\t\tit = path.buf;\n+\t\telse if (strcmp(ref, path.buf) &&\n \t\t\t !stat(git_path(\"logs/%s\", ref), &st) &&\n \t\t\t S_ISREG(st.st_mode))\n \t\t\tit = ref;\n@@ -1927,6 +1995,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tif (!warn_ambiguous_refs)\n \t\t\tbreak;\n \t}\n+\tstrbuf_release(&path);\n \tfree(last_branch);\n \treturn logs_found;\n }\n@@ -3004,47 +3073,20 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)\n \treturn NULL;\n }\n \n-static 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]\". 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 memcmp(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+\tsize_t refname_len = strlen(refname);\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+\tstruct strbuf shortname = STRBUF_INIT;\n+\tstruct strbuf expanded = STRBUF_INIT;\n \n \t/* skip first rule, it will always match */\n-\tfor (i = ARRAY_SIZE(ref_rev_parse_rules) - 2; i > 0; --i) {\n+\tfor (i = ARRAY_SIZE(refname_patterns) - 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 (!(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 (refname_shorten(&shortname, refname_patterns[i],\n+\t\t\t\t refname, refname_len))\n \t\t\tcontinue;\n \n \t\t/*\n@@ -3052,16 +3094,13 @@ 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_rev_parse_rules) - 1;\n+\t\t\trules_to_fail = ARRAY_SIZE(refname_patterns);\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-\n \t\t\t/* skip matched rule */\n \t\t\tif (i == j)\n \t\t\t\tcontinue;\n@@ -3069,23 +3108,23 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\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 */\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\tif (!refname_expand(&expanded, refname_patterns[j],\n+\t\t\t\t\t shortname.buf, shortname.len) &&\n+\t\t\t ref_exists(expanded.buf))\n \t\t\t\tbreak;\n \t\t}\n+\t\tstrbuf_release(&expanded);\n \n \t\t/*\n \t\t * short name is non-ambiguous if all previous rules\n \t\t * haven't resolved to a valid ref\n \t\t */\n \t\tif (j == rules_to_fail)\n-\t\t\treturn short_name;\n+\t\t\treturn strbuf_detach(&shortname, NULL);\n \t}\n \n-\tfree(short_name);\n+\tstrbuf_release(&shortname);\n \treturn xstrdup(refname);\n }\n \ndiff --git a/refs.h b/refs.h\nindex 8060ed8..e05c1f1 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -164,6 +164,9 @@ 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+extern int refname_match(const char *abbrev_name, const char *full_name);\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..8ca4823 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))\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);\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))\n \t\t\treturn ref;\n \t}\n \treturn NULL;\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217016","messageId":"1368289280-30337-5-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:14Z","receivedAt":"2013-05-11T16:21:14Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Although we definitely support and encourage use of multi-level branch\nnames, we have never conciously tried to give support for multi-level\nremote names. Currently, they are allowed, but there is no evidence that\nthey are commonly used.\n\nNow, they do provide a source of problems when trying to expand the\n\"$nick/$name\" shorthand notation (where $nick matches a remote name)\ninto a full refname. Consider the shorthand \"foo/bar/baz\": Does this\nparse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?\n\nSince we need to be unambiguous about these things, we hereby declare\nthat a remote name shall never contain a '/' character, and that the\nonly correct way to parse \"foo/bar/baz\" is $nick = foo, $name = bar/baz.\n\nThis patch teaches 'git remote' to reject remote names with slashes,\nand adds tests verifying this.\n\nSigned-off-by: Johan Herland \n---\n builtin/remote.c | 4 ++--\n t/t5505-remote.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5e54d36..6e7369d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -195,7 +195,7 @@ static int add(int argc, const char **argv)\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n \tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n-\tif (!valid_fetch_refspec(buf2.buf))\n+\tif (!valid_fetch_refspec(buf2.buf) || strchr(name, '/'))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n@@ -646,7 +646,7 @@ static int mv(int argc, const char **argv)\n \t\tdie(_(\"remote %s already exists.\"), rename.new);\n \n \tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n-\tif (!valid_fetch_refspec(buf.buf))\n+\tif (!valid_fetch_refspec(buf.buf) || strchr(rename.new, '/'))\n \t\tdie(_(\"'%s' is not a valid remote name\"), rename.new);\n \n \tstrbuf_reset(&buf);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex dd10ff0..f7098fe 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -621,6 +621,12 @@ test_expect_success 'reject adding remote with an invalid name' '\n \n '\n \n+test_expect_success 'reject adding remote with slash in name' '\n+\n+\ttest_must_fail git remote add multi/level .\n+\n+'\n+\n # The first three test if the tracking branches are properly renamed,\n # the last two ones check if the config is updated.\n \n@@ -668,6 +674,12 @@ test_expect_success 'rename a remote with name prefix of other remote' '\n \n '\n \n+test_expect_success 'rename a remote with slash in name' '\n+\n+\ttest_must_fail git remote rename upstream up/stream\n+\n+'\n+\n cat > remotes_origin << EOF\n URL: $(pwd)/one\n Push: refs/heads/master:refs/heads/upstream\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217022","messageId":"1368289280-30337-6-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 05/10] refs.c: Add support for expanding/shortening refs in refs/peers/*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:15Z","receivedAt":"2013-05-11T16:21:15Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This patch adds the following patterns for expanding/shortening refs:\n\n \"refs/peers/%*\"\n \"refs/peers/%1/tags/%*\"\n \"refs/peers/%1/heads/%*\"\n\nThese allow shorthand names like \"origin/master\" to expand to refs in\nthe refs/peers/* hierarchy (in this case, the likely expansion would be\nby the middle rule above, resulting in \"refs/peers/origin/heads/master\").\n\nTo accomplish this, we have added the new \"%1\" wildcard which shall\nexpand into the first component (i.e. up to the first '/') of the given\nshorthand. The other wildcard (\"%*\") shall then expand into the remainder\nof the shorthand (i.e. following the first '/').\n\nCorrespondingly, when shortening according to a pattern with \"%1\", a\nsingle component (not including any '/' character) shall be extracted\nfrom that point in the given refname, and shall be added to the resulting\nshorthand, with a trailing '/'. Then, when hitting the \"%*\", the\nremainder of the given refname (modulo a trailing match in the pattern)\nshall be extracted and appended to the portion previously extracted by\nthe \"%1\" wildcard.\n\nThe need to split the \"$remote/$ref\" into its $remote and $ref parts is\nthe reason why multi-level remote names will no longer work (and hence\nwere disallowed in the previous patch). A testcase demonstrating how\nmulti-level remote names fail is therefore included in this patch.\n\nSigned-off-by: Johan Herland \n---\n refs.c | 101 +++++++++++++++++++++----\n t/t7900-working-with-namespaced-remote-refs.sh | 4 +-\n t/t7901-multi-level-remote-name-failure.sh | 20 +++++\n 3 files changed, 107 insertions(+), 18 deletions(-)\n create mode 100755 t/t7901-multi-level-remote-name-failure.sh\n\ndiff --git a/refs.c b/refs.c\nindex ab5e120..188a9eb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1789,7 +1789,10 @@ static const char *refname_patterns[] = {\n \t\"refs/tags/%*\",\n \t\"refs/heads/%*\",\n \t\"refs/remotes/%*\",\n-\t\"refs/remotes/%*/HEAD\"\n+\t\"refs/remotes/%*/HEAD\",\n+\t\"refs/peers/%*\",\n+\t\"refs/peers/%1/tags/%*\",\n+\t\"refs/peers/%1/heads/%*\"\n };\n \n struct wildcard_data {\n@@ -1806,6 +1809,15 @@ static size_t refname_expand_helper(struct strbuf *sb, const char *placeholder,\n \t\tstrbuf_add(sb, cb->s, cb->len);\n \t\tcb->done = 1;\n \t\treturn 1;\n+\t} else if (*placeholder == '1' && !cb->done) {\n+\t\tconst char *p = memchr(cb->s, '/', cb->len);\n+\t\tsize_t copy_len = p ? p - cb->s : cb->len;\n+\t\tstrbuf_add(sb, cb->s, copy_len);\n+\t\tif (copy_len < cb->len && cb->s[copy_len] == '/')\n+\t\t\tcopy_len++;\n+\t\tcb->s += copy_len;\n+\t\tcb->len -= copy_len;\n+\t\treturn 1;\n \t}\n \treturn 0;\n }\n@@ -1821,6 +1833,46 @@ static int refname_expand(struct strbuf *dst, const char *pattern,\n \treturn !cbdata.done;\n }\n \n+static int handle_fragment(struct strbuf *dst, struct strbuf *fragment,\n+\t\t\t int first, int last,\n+\t\t\t const char *refname, size_t refname_len)\n+{\n+\tconst char *ref = refname, *p;\n+\tsize_t ref_len = refname_len, trail_len;\n+\tif (!first) {\n+\t\t/* extract wildcard according to wildcard char */\n+\t\tswitch (fragment->buf[0]) {\n+\t\tcase '1':\n+\t\t\t/* extract up to next '/' from refname */\n+\t\t\tp = memchr(ref, '/', ref_len);\n+\t\t\tif (!p)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_add(dst, ref, p - ref);\n+\t\t\tstrbuf_addch(dst, '/');\n+\t\t\tref_len -= p - ref;\n+\t\t\tref += p - ref;\n+\t\t\tbreak;\n+\t\tcase '*':\n+\t\t\t/* extract all up to matching trailer */\n+\t\t\tassert(last);\n+\t\t\ttrail_len = fragment->len - 1;\n+\t\t\tif (trail_len > ref_len)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_add(dst, ref, ref_len - trail_len);\n+\t\t\tref += ref_len - trail_len;\n+\t\t\tref_len -= ref_len - trail_len;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\t/* match rest of fragment verbatim */\n+\tp = fragment->buf + (first ? 0 : 1); /* skip wildcard character */\n+\ttrail_len = fragment->len - ((first ? 0 : 1) + (last ? 0 : 1));\n+\tif (trail_len > ref_len || memcmp(ref, p, trail_len))\n+\t\treturn -1;\n+\treturn (ref - refname) + trail_len;\n+}\n+\n static int refname_shorten(struct strbuf *dst, const char *pattern,\n \t\t\t const char *refname, size_t refname_len)\n {\n@@ -1830,26 +1882,43 @@ static int refname_shorten(struct strbuf *dst, const char *pattern,\n \t * Return 0 on success (positive match, wildcard-matching portion\n \t * copied into dst), and non-zero on failure (no match, dst empty).\n \t */\n-\tconst char *match_end;\n-\tstruct strbuf **fragments = strbuf_split_str(pattern, '%', 2);\n-\tstruct strbuf *prefix = fragments[0], *suffix = fragments[1];\n-\tassert(!fragments[2]);\n-\tassert(prefix->len && prefix->buf[prefix->len - 1] == '%');\n-\tassert(suffix->len && suffix->buf[0] == '*');\n+\tstruct strbuf **fragments = strbuf_split_str(pattern, '%', 0);\n+\tstruct strbuf **it;\n+\tint first = 1, last, ret = 1;\n \n \tstrbuf_reset(dst);\n-\tmatch_end = refname + refname_len - (suffix->len - 1);\n-\tif (refname_len <= (prefix->len - 1) + (suffix->len - 1) ||\n-\t memcmp(prefix->buf, refname, prefix->len - 1) ||\n-\t memcmp(suffix->buf + 1, match_end, suffix->len - 1)) {\n-\t\tstrbuf_list_free(fragments);\n-\t\treturn 1; /* refname does not match pattern */\n+\tfor (it = fragments; *it; it++) {\n+\t\tint consumed;\n+\t\tstruct strbuf *cur = *it;\n+\t\tlast = *(it + 1) == NULL;\n+\n+\t\t/* all but last ends with '%' */\n+\t\tassert(last || cur->buf[cur->len - 1] == '%');\n+\t\t/* all but first starts with '*' or '1' */\n+\t\tassert(first || cur->buf[0] == '*' || cur->buf[0] == '1');\n+\t\t/* only last starts with '*' */\n+\t\tassert((cur->buf[0] == '*' && last) ||\n+\t\t (cur->buf[0] != '*' && !last));\n+\n+\t\tconsumed = handle_fragment(dst, cur, first, last,\n+\t\t\t\t\t refname, refname_len);\n+\t\tif (consumed < 0)\n+\t\t\tgoto cleanup;\n+\t\telse {\n+\t\t\trefname += consumed;\n+\t\t\trefname_len -= consumed;\n+\t\t}\n+\n+\t\tfirst = 0;\n \t}\n+\tif (refname_len == 0)\n+\t\tret = 0;\n \n-\trefname += prefix->len - 1;\n-\tstrbuf_add(dst, refname, match_end - refname);\n+cleanup:\n+\tif (ret)\n+\t\tstrbuf_reset(dst);\n \tstrbuf_list_free(fragments);\n-\treturn 0;\n+\treturn ret;\n }\n \n int refname_match(const char *abbrev_name, const char *full_name)\ndiff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh\nindex 109e9b8..33266e0 100755\n--- a/t/t7900-working-with-namespaced-remote-refs.sh\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -83,7 +83,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/peers/origin/heads/master >expect &&\n \tgit rev-parse --symbolic-full-name refs/peers/origin/heads/master >actual &&\n \ttest_cmp expect actual &&\n@@ -95,7 +95,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/peers/origin/heads/master >actual &&\n \ttest_cmp expect actual &&\ndiff --git a/t/t7901-multi-level-remote-name-failure.sh b/t/t7901-multi-level-remote-name-failure.sh\nnew file mode 100755\nindex 0000000..8d2a617\n--- /dev/null\n+++ b/t/t7901-multi-level-remote-name-failure.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+\n+test_description='Show why multi-level remote names can no longer be used'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit a &&\n+\tgit config remote.multi/level.url . &&\n+\tgit config remote.multi/level.fetch \"+refs/heads/*:refs/peers/multi/level/heads/*\" &&\n+\tgit fetch multi/level\n+'\n+\n+test_expect_failure 'Fail to use shorthand notation: \"$remote/$branch\"' '\n+\tgit rev-parse --verify a >expect &&\n+\tgit rev-parse --verify multi/level/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217017","messageId":"1368289280-30337-7-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 06/10] t7900: Test git branch -r/-a output w/remote-tracking branches in refs/peers/*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:16Z","receivedAt":"2013-05-11T16:21:16Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"These tests will be made to pass by subsequent patches.\n\nSigned-off-by: Johan Herland \n---\n t/t7900-working-with-namespaced-remote-refs.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh\nindex 33266e0..279664c 100755\n--- a/t/t7900-working-with-namespaced-remote-refs.sh\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -107,4 +107,25 @@ test_expect_success 'remote-tracking branches are shortened correctly' '\n \ttest_cmp expect actual\n '\n \n+cat >expect.branch-r << EOF\n+ origin/master\n+ origin/other\n+EOF\n+\n+test_expect_failure 'git branch -r should show remote-tracking branches' '\n+\tgit branch -r >actual.branch-r &&\n+\ttest_cmp expect.branch-r actual.branch-r\n+'\n+\n+cat >expect.branch-a << EOF\n+* master\n+ peers/origin/heads/master\n+ peers/origin/heads/other\n+EOF\n+\n+test_expect_failure 'git branch -a should also show remote-tracking branches' '\n+\tgit branch -a >actual.branch-a &&\n+\ttest_cmp expect.branch-a actual.branch-a\n+'\n+\n test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217018","messageId":"1368289280-30337-8-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 07/10] t3203: Add testcase for fix in 1603ade81352a526ccb206f41ff81ecbc855df2d","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:17Z","receivedAt":"2013-05-11T16:21:17Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This adds a testcase for some behavior that I very nearly broke while\nrefactoring some stuff in builtin/branch.c.\n\nSigned-off-by: Johan Herland \n---\n t/t3203-branch-output.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex ba4f98e..c312d3a 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -106,4 +106,19 @@ EOF\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'git branch -a --merged handles broken refs gracefully' '\n+\tcat >expect < origin/branch-one\n+ remotes/origin/branch-one\n+ remotes/origin/branch-two\n+EOF\n+\tgit checkout master &&\n+\techo \"broken ref\" > .git/refs/heads/broken &&\n+\ttest_must_fail git branch -a --merged >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217019","messageId":"1368289280-30337-9-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 08/10] builtin/branch.c: Refactor ref_item.name and .dest into strbufs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:18Z","receivedAt":"2013-05-11T16:21:18Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"In preparation of a future patch which reorganizes how the display of the\nref_list is done.\n\nSigned-off-by: Johan Herland \n---\n builtin/branch.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0836890..c8b49e3 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -276,8 +276,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n }\n \n struct ref_item {\n-\tchar *name;\n-\tchar *dest;\n+\tstruct strbuf name;\n+\tstruct strbuf dest;\n \tunsigned int kind, width;\n \tstruct commit *commit;\n };\n@@ -380,11 +380,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \n \t/* Record the new item */\n \tnewitem = &(ref_list->list[ref_list->index++]);\n-\tnewitem->name = xstrdup(refname);\n+\tstrbuf_init(&newitem->name, 0);\n+\tstrbuf_addstr(&newitem->name, refname);\n \tnewitem->kind = kind;\n \tnewitem->commit = commit;\n \tnewitem->width = utf8_strwidth(refname);\n-\tnewitem->dest = resolve_symref(orig_refname, prefix);\n+\tstrbuf_init(&newitem->dest, 0);\n+\torig_refname = resolve_symref(orig_refname, prefix);\n+\tif (orig_refname)\n+\t\tstrbuf_addstr(&newitem->dest, orig_refname);\n \t/* adjust for \"remotes/\" */\n \tif (newitem->kind == REF_REMOTE_BRANCH &&\n \t ref_list->kinds != REF_REMOTE_BRANCH)\n@@ -400,8 +404,8 @@ static void free_ref_list(struct ref_list *ref_list)\n \tint i;\n \n \tfor (i = 0; i < ref_list->index; i++) {\n-\t\tfree(ref_list->list[i].name);\n-\t\tfree(ref_list->list[i].dest);\n+\t\tstrbuf_release(&ref_list->list[i].name);\n+\t\tstrbuf_release(&ref_list->list[i].dest);\n \t}\n \tfree(ref_list->list);\n }\n@@ -413,7 +417,7 @@ static int ref_cmp(const void *r1, const void *r2)\n \n \tif (c1->kind != c2->kind)\n \t\treturn c1->kind - c2->kind;\n-\treturn strcmp(c1->name, c2->name);\n+\treturn strbuf_cmp(&c1->name, &c2->name);\n }\n \n static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n@@ -496,7 +500,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n \t}\n \n \tif (item->kind == REF_LOCAL_BRANCH)\n-\t\tfill_tracking_info(&stat, item->name, verbose > 1);\n+\t\tfill_tracking_info(&stat, item->name.buf, verbose > 1);\n \n \tstrbuf_addf(out, \" %s %s%s\",\n \t\tfind_unique_abbrev(item->commit->object.sha1, abbrev),\n@@ -534,7 +538,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\tcolor = BRANCH_COLOR_CURRENT;\n \t}\n \n-\tstrbuf_addf(&name, \"%s%s\", prefix, item->name);\n+\tstrbuf_addf(&name, \"%s%s\", prefix, item->name.buf);\n \tif (verbose) {\n \t\tint utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);\n \t\tstrbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(color),\n@@ -544,8 +548,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\tstrbuf_addf(&out, \"%c %s%s%s\", c, branch_get_color(color),\n \t\t\t name.buf, branch_get_color(BRANCH_COLOR_RESET));\n \n-\tif (item->dest)\n-\t\tstrbuf_addf(&out, \" -> %s\", item->dest);\n+\tif (item->dest.len)\n+\t\tstrbuf_addf(&out, \" -> %s\", item->dest.buf);\n \telse if (verbose)\n \t\t/* \" f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h\" */\n \t\tadd_verbose_info(&out, item, verbose, abbrev);\n@@ -601,15 +605,16 @@ static void show_detached(struct ref_list *ref_list)\n \n \tif (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n \t\tstruct ref_item item;\n-\t\titem.name = get_head_description();\n-\t\titem.width = utf8_strwidth(item.name);\n+\t\tstrbuf_init(&item.name, 0);\n+\t\tstrbuf_addstr(&item.name, get_head_description());\n+\t\titem.width = utf8_strwidth(item.name.buf);\n \t\titem.kind = REF_LOCAL_BRANCH;\n-\t\titem.dest = NULL;\n+\t\tstrbuf_init(&item.dest, 0);\n \t\titem.commit = head_commit;\n \t\tif (item.width > ref_list->maxwidth)\n \t\t\tref_list->maxwidth = item.width;\n \t\tprint_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n-\t\tfree(item.name);\n+\t\tstrbuf_release(&item.name);\n \t}\n }\n \n@@ -655,7 +660,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tfor (i = 0; i < ref_list.index; i++) {\n \t\tint current = !detached &&\n \t\t\t(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&\n-\t\t\t!strcmp(ref_list.list[i].name, head);\n+\t\t\t!strcmp(ref_list.list[i].name.buf, head);\n \t\tchar *prefix = (kinds != REF_REMOTE_BRANCH &&\n \t\t\t\tref_list.list[i].kind == REF_REMOTE_BRANCH)\n \t\t\t\t? \"remotes/\" : \"\";\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217021","messageId":"1368289280-30337-10-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 09/10] builtin/branch.c: Refactor \"remotes/\" prepending to remote-tracking branches","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:19Z","receivedAt":"2013-05-11T16:21:19Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When running \"git branch -a\" (instead of \"git branch -r\"), we prefix the\nremote-tracking branches with \"remotes/\" to allow the user to more easily\ntell them apart from the local branches.\n\nThe code that prepended \"remotes/\" to remote-tracking branches was\nlocated in print_ref_item(), while the code that adjusted the\nref_item.width (to account for the prepended \"remotes/\") was located in\nappend_ref().\n\nThis code moves the prepending of \"remotes/\" up into append_ref(), which\nis a nice cleanup of the code, as well as a preparation for changing the\nlogic when remote-tracking branches start showing up in refs/peers/*.\n\nSigned-off-by: Johan Herland \n---\n builtin/branch.c | 23 +++++++++--------------\n 1 file changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c8b49e3..4480be2 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -384,7 +384,6 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \tstrbuf_addstr(&newitem->name, refname);\n \tnewitem->kind = kind;\n \tnewitem->commit = commit;\n-\tnewitem->width = utf8_strwidth(refname);\n \tstrbuf_init(&newitem->dest, 0);\n \torig_refname = resolve_symref(orig_refname, prefix);\n \tif (orig_refname)\n@@ -392,7 +391,8 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \t/* adjust for \"remotes/\" */\n \tif (newitem->kind == REF_REMOTE_BRANCH &&\n \t ref_list->kinds != REF_REMOTE_BRANCH)\n-\t\tnewitem->width += 8;\n+\t\tstrbuf_insert(&newitem->name, 0, \"remotes/\", 8);\n+\tnewitem->width = utf8_strwidth(newitem->name.buf);\n \tif (newitem->width > ref_list->maxwidth)\n \t\tref_list->maxwidth = newitem->width;\n \n@@ -510,12 +510,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n }\n \n static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n-\t\t\t int abbrev, int current, char *prefix)\n+\t\t\t int abbrev, int current)\n {\n \tchar c;\n \tint color;\n \tstruct commit *commit = item->commit;\n-\tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n \n \tif (!matches_merge_filter(commit))\n \t\treturn;\n@@ -538,15 +538,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\tcolor = BRANCH_COLOR_CURRENT;\n \t}\n \n-\tstrbuf_addf(&name, \"%s%s\", prefix, item->name.buf);\n \tif (verbose) {\n-\t\tint utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);\n+\t\tint utf8_compensation = strlen(item->name.buf) - utf8_strwidth(item->name.buf);\n \t\tstrbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(color),\n-\t\t\t maxwidth + utf8_compensation, name.buf,\n+\t\t\t maxwidth + utf8_compensation, item->name.buf,\n \t\t\t branch_get_color(BRANCH_COLOR_RESET));\n \t} else\n \t\tstrbuf_addf(&out, \"%c %s%s%s\", c, branch_get_color(color),\n-\t\t\t name.buf, branch_get_color(BRANCH_COLOR_RESET));\n+\t\t\t item->name.buf, branch_get_color(BRANCH_COLOR_RESET));\n \n \tif (item->dest.len)\n \t\tstrbuf_addf(&out, \" -> %s\", item->dest.buf);\n@@ -559,7 +558,6 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t} else {\n \t\tprintf(\"%s\\n\", out.buf);\n \t}\n-\tstrbuf_release(&name);\n \tstrbuf_release(&out);\n }\n \n@@ -613,7 +611,7 @@ static void show_detached(struct ref_list *ref_list)\n \t\titem.commit = head_commit;\n \t\tif (item.width > ref_list->maxwidth)\n \t\t\tref_list->maxwidth = item.width;\n-\t\tprint_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n+\t\tprint_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1);\n \t\tstrbuf_release(&item.name);\n \t}\n }\n@@ -661,11 +659,8 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \t\tint current = !detached &&\n \t\t\t(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&\n \t\t\t!strcmp(ref_list.list[i].name.buf, head);\n-\t\tchar *prefix = (kinds != REF_REMOTE_BRANCH &&\n-\t\t\t\tref_list.list[i].kind == REF_REMOTE_BRANCH)\n-\t\t\t\t? \"remotes/\" : \"\";\n \t\tprint_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,\n-\t\t\t abbrev, current, prefix);\n+\t\t\t abbrev, current);\n \t}\n \n \tfree_ref_list(&ref_list);\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217020","messageId":"1368289280-30337-11-git-send-email-johan@herland.net","threadId":"33796","inReplyTo":"1368289280-30337-1-git-send-email-johan@herland.net","subject":"[PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-11T16:21:20Z","receivedAt":"2013-05-11T16:21:20Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The current branch display code assumes all remote-tracking branches live\ninside refs/remotes/*, and that their appropriate shorthand names can be\nretrieved by simply stripping off the \"refs/remotes/\" prefix.\n\nWhen we add remote-tracking branches in refs/peers/$remote/heads/*, the\ncode must not only be taught how to list refs from the new location, but\nalso how to appropriately derive shorthand names for the remote-tracking\nbranches there (instead of stripping off a hardcoded prefix, we must use\nthe refname_shorten() function introduced in an earlier patch.)\n\nThere is already a good set of tests for the expected output from git\nbranch, and this patch does not break any of them. However, we do fix\nthe two tests with remote-tracking branches in refs/peers/* introduced\nin a previous patch.\n\nSigned-off-by: Johan Herland \n---\n builtin/branch.c | 66 ++++++++++++++------------\n refs.c | 4 +-\n refs.h | 3 ++\n t/t7900-working-with-namespaced-remote-refs.sh | 4 +-\n 4 files changed, 42 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 4480be2..9a6bce8 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -290,18 +290,16 @@ struct ref_list {\n \tint kinds;\n };\n \n-static char *resolve_symref(const char *src, const char *prefix)\n+static void resolve_symref(struct strbuf *shortname, const char *pattern, const char *src)\n {\n \tunsigned char sha1[20];\n \tint flag;\n-\tconst char *dst, *cp;\n+\tconst char *dst;\n \n \tdst = resolve_ref_unsafe(src, sha1, 0, &flag);\n-\tif (!(dst && (flag & REF_ISSYMREF)))\n-\t\treturn NULL;\n-\tif (prefix && (cp = skip_prefix(dst, prefix)))\n-\t\tdst = cp;\n-\treturn xstrdup(dst);\n+\tif (dst && (flag & REF_ISSYMREF) &&\n+\t refname_shorten(shortname, pattern, dst, strlen(dst)))\n+\t\tstrbuf_addstr(shortname, dst);\n }\n \n struct append_ref_cb {\n@@ -328,74 +326,80 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \tstruct ref_list *ref_list = cb->ref_list;\n \tstruct ref_item *newitem;\n \tstruct commit *commit;\n+\tstruct strbuf ref = STRBUF_INIT;\n \tint kind, i;\n-\tconst char *prefix, *orig_refname = refname;\n+\tconst char *pattern;\n+\tsize_t refname_len = strlen(refname);\n \n \tstatic struct {\n \t\tint kind;\n-\t\tconst char *prefix;\n-\t\tint pfxlen;\n+\t\tconst char *pattern;\n \t} ref_kind[] = {\n-\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\", 11 },\n-\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\", 13 },\n+\t\t{ REF_LOCAL_BRANCH, \"refs/heads/%*\" },\n+\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/%*\" },\n+\t\t{ REF_REMOTE_BRANCH, \"refs/peers/%1/heads/%*\" },\n \t};\n \n \t/* Detect kind */\n \tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n-\t\tprefix = ref_kind[i].prefix;\n-\t\tif (strncmp(refname, prefix, ref_kind[i].pfxlen))\n+\t\tif (refname_shorten(&ref, ref_kind[i].pattern, refname, refname_len))\n \t\t\tcontinue;\n \t\tkind = ref_kind[i].kind;\n-\t\trefname += ref_kind[i].pfxlen;\n+\t\tpattern = ref_kind[i].pattern;\n \t\tbreak;\n \t}\n \tif (ARRAY_SIZE(ref_kind) <= i)\n-\t\treturn 0;\n+\t\tgoto out;\n \n \t/* Don't add types the caller doesn't want */\n \tif ((kind & ref_list->kinds) == 0)\n-\t\treturn 0;\n+\t\tgoto out;\n \n-\tif (!match_patterns(cb->pattern, refname))\n-\t\treturn 0;\n+\tif (!match_patterns(cb->pattern, ref.buf))\n+\t\tgoto out;\n \n \tcommit = NULL;\n \tif (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {\n \t\tcommit = lookup_commit_reference_gently(sha1, 1);\n \t\tif (!commit) {\n-\t\t\tcb->ret = error(_(\"branch '%s' does not point at a commit\"), refname);\n-\t\t\treturn 0;\n+\t\t\tcb->ret = error(_(\"branch '%s' does not point at a commit\"), ref.buf);\n+\t\t\tgoto out;\n \t\t}\n \n \t\t/* Filter with with_commit if specified */\n \t\tif (!is_descendant_of(commit, ref_list->with_commit))\n-\t\t\treturn 0;\n+\t\t\tgoto out;\n \n \t\tif (merge_filter != NO_FILTER)\n \t\t\tadd_pending_object(&ref_list->revs,\n-\t\t\t\t\t (struct object *)commit, refname);\n+\t\t\t\t\t (struct object *)commit, ref.buf);\n \t}\n \n+\t/*\n+\t * When displaying more then just remote-tracking branches, make the\n+\t * remote-tracking branches more explicit, e.g. instead of printing\n+\t * \"origin/master\", we should print \"remote/origin/master\" (or\n+\t * \"peers/origin/heads/master\").\n+\t */\n+\tif (kind == REF_REMOTE_BRANCH && ref_list->kinds != REF_REMOTE_BRANCH)\n+\t\trefname_shorten(&ref, \"refs/%*\", refname, refname_len);\n+\n \tALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);\n \n \t/* Record the new item */\n \tnewitem = &(ref_list->list[ref_list->index++]);\n \tstrbuf_init(&newitem->name, 0);\n-\tstrbuf_addstr(&newitem->name, refname);\n+\tstrbuf_addbuf(&newitem->name, &ref);\n \tnewitem->kind = kind;\n \tnewitem->commit = commit;\n \tstrbuf_init(&newitem->dest, 0);\n-\torig_refname = resolve_symref(orig_refname, prefix);\n-\tif (orig_refname)\n-\t\tstrbuf_addstr(&newitem->dest, orig_refname);\n-\t/* adjust for \"remotes/\" */\n-\tif (newitem->kind == REF_REMOTE_BRANCH &&\n-\t ref_list->kinds != REF_REMOTE_BRANCH)\n-\t\tstrbuf_insert(&newitem->name, 0, \"remotes/\", 8);\n+\tresolve_symref(&newitem->dest, pattern, refname);\n \tnewitem->width = utf8_strwidth(newitem->name.buf);\n \tif (newitem->width > ref_list->maxwidth)\n \t\tref_list->maxwidth = newitem->width;\n \n+out:\n+\tstrbuf_release(&ref);\n \treturn 0;\n }\n \ndiff --git a/refs.c b/refs.c\nindex 188a9eb..a78199a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1873,8 +1873,8 @@ static int handle_fragment(struct strbuf *dst, struct strbuf *fragment,\n \treturn (ref - refname) + trail_len;\n }\n \n-static int refname_shorten(struct strbuf *dst, const char *pattern,\n-\t\t\t const char *refname, size_t refname_len)\n+int refname_shorten(struct strbuf *dst, const char *pattern,\n+\t\t const char *refname, size_t refname_len)\n {\n \t/*\n \t * Match refname against pattern, using \"%*\" as wildcard, and\ndiff --git a/refs.h b/refs.h\nindex e05c1f1..245d53d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -165,6 +165,9 @@ extern int check_refname_format(const char *refname, int flags);\n \n extern const char *prettify_refname(const char *refname);\n \n+extern int refname_shorten(struct strbuf *dst, const char *pattern,\n+\t\t\t const char *refname, size_t refname_len);\n+\n extern int refname_match(const char *abbrev_name, const char *full_name);\n \n extern char *shorten_unambiguous_ref(const char *refname, int strict);\ndiff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh\nindex 279664c..450b193 100755\n--- a/t/t7900-working-with-namespaced-remote-refs.sh\n+++ b/t/t7900-working-with-namespaced-remote-refs.sh\n@@ -112,7 +112,7 @@ cat >expect.branch-r << EOF\n origin/other\n EOF\n \n-test_expect_failure 'git branch -r should show remote-tracking branches' '\n+test_expect_success 'git branch -r should show remote-tracking branches' '\n \tgit branch -r >actual.branch-r &&\n \ttest_cmp expect.branch-r actual.branch-r\n '\n@@ -123,7 +123,7 @@ cat >expect.branch-a << EOF\n peers/origin/heads/other\n EOF\n \n-test_expect_failure 'git branch -a should also show remote-tracking branches' '\n+test_expect_success 'git branch -a should also show remote-tracking branches' '\n \tgit branch -a >actual.branch-a &&\n \ttest_cmp expect.branch-a actual.branch-a\n '\n-- \n1.8.1.3.704.g33f7d4f\n"},{"id":"217111","messageId":"CAPig+cR-FkOtGvn+A+JdQ5ia+eUXw6iVzEmOpxCrUgkPttZkVQ@mail.gmail.com","threadId":"33796","inReplyTo":"1368289280-30337-5-git-send-email-johan@herland.net","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-13T04:48:42Z","receivedAt":"2013-05-13T04:48:42Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 11, 2013 at 12:21 PM, Johan Herland wrote:\n> Although we definitely support and encourage use of multi-level branch\n> names, we have never conciously tried to give support for multi-level\n\ns/conciously/consciously/\n\n> remote names. Currently, they are allowed, but there is no evidence that\n> they are commonly used.\n>\n> Now, they do provide a source of problems when trying to expand the\n> \"$nick/$name\" shorthand notation (where $nick matches a remote name)\n> into a full refname. Consider the shorthand \"foo/bar/baz\": Does this\n> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?\n>\n> Since we need to be unambiguous about these things, we hereby declare\n> that a remote name shall never contain a '/' character, and that the\n> only correct way to parse \"foo/bar/baz\" is $nick = foo, $name = bar/baz.\n>\n> This patch teaches 'git remote' to reject remote names with slashes,\n> and adds tests verifying this.\n>\n> Signed-off-by: Johan Herland \n"},{"id":"217112","messageId":"7vtxm7scn5.fsf@alter.siamese.dyndns.org","threadId":"33796","inReplyTo":"1368289280-30337-5-git-send-email-johan@herland.net","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T04:54:22Z","receivedAt":"2013-05-13T04:54:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Although we definitely support and encourage use of multi-level branch\n> names, we have never conciously tried to give support for multi-level\n> remote names. Currently, they are allowed, but there is no evidence that\n> they are commonly used.\n>\n> Now, they do provide a source of problems when trying to expand the\n> \"$nick/$name\" shorthand notation (where $nick matches a remote name)\n> into a full refname. Consider the shorthand \"foo/bar/baz\": Does this\n> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?\n>\n> Since we need to be unambiguous about these things, we hereby declare\n> that a remote name shall never contain a '/' character, and that the\n> only correct way to parse \"foo/bar/baz\" is $nick = foo, $name = bar/baz.\n\nI know I am guilty of hinting to go in this direction in the earlier\ndiscussion, but I have to wonder how much more refactoring is needed\nto see if there is only one unique possibility among many.\n\nFor a string with N slashes, you have only N possible ways to split\nit into $nick and $name, and if you see a ref \"bar/baz\" copied from\nremote \"foo\" but no ref \"baz\" copied from remote \"foo/bar\" (or you\nmay not even have a remote \"foo/bar\" in the first place), the user\nis already very unambiguous. The declaration is merely being lazy.\n\nI am not saying we must implement such a back-track to disambiguate\nthe user input better. I am wondering how much more effort on top\nof this series is needed if we want to get there (provided that the\ndisambiguation is a good thing to do in the first place).\n"},{"id":"217113","messageId":"7vmwrzsck1.fsf@alter.siamese.dyndns.org","threadId":"33796","inReplyTo":"1368289280-30337-4-git-send-email-johan@herland.net","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T04:56:14Z","receivedAt":"2013-05-13T04:56:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> The refname_expand() function no longer uses mkpath()/mksnpath() to\n> perform the pattern expansion. Instead, it uses strbuf_expand(), which\n> removes the need for using fixed-length buffers from the code.\n\nIt is a brilliant idea to use strbuf_expand() for this. I like it.\n\nI notice that you later introduce %1 (that is 'one', not 'el'), but\nunless you are planning to introduce %2 and %3 that semantically\nfall into a similar category as %1, I would rather see a different\nletter used that is mnemonic to what the placeholder _means_. \n\nThe choice of the letter is arbitrary and may not look like it\nmatters that much, because it is not exposed to the end user. But\nby switching from the sprintf() semantics that shows things given to\nit in the order they were given, without knowing what they mean, and\nintroducing a strbuf_expand() machinery tailored for refnames (and\nrefnames only), the new code assigns meanings to each part of the\nrefname, and we can afford to be more descriptive.\n\nThe choice of '%*' is justifiable, \"it is the closest to the '*' we\ntraditionally used to replace only one thing\", but '%1' does not\nlook the best placeholder to use, at least to me.\n"},{"id":"217120","messageId":"CAPig+cSRnsiYRQhX8Gz41D43005zgF9ysNTNSNn-C27nB9HftQ@mail.gmail.com","threadId":"33796","inReplyTo":"1368289280-30337-11-git-send-email-johan@herland.net","subject":"Re: [PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-13T05:19:49Z","receivedAt":"2013-05-13T05:19:49Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 11, 2013 at 12:21 PM, Johan Herland wrote:\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 4480be2..9a6bce8 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -328,74 +326,80 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n> struct ref_list *ref_list = cb->ref_list;\n> struct ref_item *newitem;\n> struct commit *commit;\n> + struct strbuf ref = STRBUF_INIT;\n> int kind, i;\n> - const char *prefix, *orig_refname = refname;\n> + const char *pattern;\n> + size_t refname_len = strlen(refname);\n>\n> static struct {\n> int kind;\n> - const char *prefix;\n> - int pfxlen;\n> + const char *pattern;\n> } ref_kind[] = {\n> - { REF_LOCAL_BRANCH, \"refs/heads/\", 11 },\n> - { REF_REMOTE_BRANCH, \"refs/remotes/\", 13 },\n> + { REF_LOCAL_BRANCH, \"refs/heads/%*\" },\n> + { REF_REMOTE_BRANCH, \"refs/remotes/%*\" },\n> + { REF_REMOTE_BRANCH, \"refs/peers/%1/heads/%*\" },\n> };\n>\n> /* Detect kind */\n> for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n> - prefix = ref_kind[i].prefix;\n> - if (strncmp(refname, prefix, ref_kind[i].pfxlen))\n> + if (refname_shorten(&ref, ref_kind[i].pattern, refname, refname_len))\n> continue;\n> kind = ref_kind[i].kind;\n> - refname += ref_kind[i].pfxlen;\n> + pattern = ref_kind[i].pattern;\n> break;\n> }\n> if (ARRAY_SIZE(ref_kind) <= i)\n> - return 0;\n> + goto out;\n>\n> /* Don't add types the caller doesn't want */\n> if ((kind & ref_list->kinds) == 0)\n> - return 0;\n> + goto out;\n>\n> - if (!match_patterns(cb->pattern, refname))\n> - return 0;\n> + if (!match_patterns(cb->pattern, ref.buf))\n> + goto out;\n>\n> commit = NULL;\n> if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {\n> commit = lookup_commit_reference_gently(sha1, 1);\n> if (!commit) {\n> - cb->ret = error(_(\"branch '%s' does not point at a commit\"), refname);\n> - return 0;\n> + cb->ret = error(_(\"branch '%s' does not point at a commit\"), ref.buf);\n> + goto out;\n> }\n>\n> /* Filter with with_commit if specified */\n> if (!is_descendant_of(commit, ref_list->with_commit))\n> - return 0;\n> + goto out;\n>\n> if (merge_filter != NO_FILTER)\n> add_pending_object(&ref_list->revs,\n> - (struct object *)commit, refname);\n> + (struct object *)commit, ref.buf);\n> }\n>\n> + /*\n> + * When displaying more then just remote-tracking branches, make the\n\ns/then/than/\n\n> + * remote-tracking branches more explicit, e.g. instead of printing\n> + * \"origin/master\", we should print \"remote/origin/master\" (or\n> + * \"peers/origin/heads/master\").\n> + */\n> + if (kind == REF_REMOTE_BRANCH && ref_list->kinds != REF_REMOTE_BRANCH)\n> + refname_shorten(&ref, \"refs/%*\", refname, refname_len);\n> +\n> ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);\n>\n> /* Record the new item */\n> newitem = &(ref_list->list[ref_list->index++]);\n> strbuf_init(&newitem->name, 0);\n> - strbuf_addstr(&newitem->name, refname);\n> + strbuf_addbuf(&newitem->name, &ref);\n> newitem->kind = kind;\n> newitem->commit = commit;\n> strbuf_init(&newitem->dest, 0);\n> - orig_refname = resolve_symref(orig_refname, prefix);\n> - if (orig_refname)\n> - strbuf_addstr(&newitem->dest, orig_refname);\n> - /* adjust for \"remotes/\" */\n> - if (newitem->kind == REF_REMOTE_BRANCH &&\n> - ref_list->kinds != REF_REMOTE_BRANCH)\n> - strbuf_insert(&newitem->name, 0, \"remotes/\", 8);\n> + resolve_symref(&newitem->dest, pattern, refname);\n> newitem->width = utf8_strwidth(newitem->name.buf);\n> if (newitem->width > ref_list->maxwidth)\n> ref_list->maxwidth = newitem->width;\n>\n> +out:\n> + strbuf_release(&ref);\n> return 0;\n> }\n>\n> diff --git a/refs.c b/refs.c\n> index 188a9eb..a78199a 100644\n> --- a/refs.c\n> +++ b/refs.c\n"},{"id":"217123","messageId":"CALKQrgf0m8r-Ofb+Ss1OpEF67dPS73b8nB+usVxH=Y=h3441Wg@mail.gmail.com","threadId":"33796","inReplyTo":"7vmwrzsck1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-13T06:31:23Z","receivedAt":"2013-05-13T06:31:23Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 13, 2013 at 6:56 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>\n>> The refname_expand() function no longer uses mkpath()/mksnpath() to\n>> perform the pattern expansion. Instead, it uses strbuf_expand(), which\n>> removes the need for using fixed-length buffers from the code.\n>\n> It is a brilliant idea to use strbuf_expand() for this. I like it.\n>\n> I notice that you later introduce %1 (that is 'one', not 'el'), but\n> unless you are planning to introduce %2 and %3 that semantically\n> fall into a similar category as %1, I would rather see a different\n> letter used that is mnemonic to what the placeholder _means_.\n>\n> The choice of the letter is arbitrary and may not look like it\n> matters that much, because it is not exposed to the end user. But\n> by switching from the sprintf() semantics that shows things given to\n> it in the order they were given, without knowing what they mean, and\n> introducing a strbuf_expand() machinery tailored for refnames (and\n> refnames only), the new code assigns meanings to each part of the\n> refname, and we can afford to be more descriptive.\n>\n> The choice of '%*' is justifiable, \"it is the closest to the '*' we\n> traditionally used to replace only one thing\", but '%1' does not\n> look the best placeholder to use, at least to me.\n\nObviously, I named it '%1' since it expands into the _first_ component\nof the (slash-separated) shorthand. There is no further parsing or\nverification that it actually corresponds to a remote (and as far as I\ncurrently understand, we do not want to do such verification), so I\nthought it better not to make such assumptions in the placeholder\nname. That said, I could go with '%r' for \"remote\", although we have\nplenty of other concepts in Git that use 'r' as the initial letter. I\ncould maybe use '%remote' instead?\n\nAlso, about the '%*': When used alone, it means \"the entire\nshorthand\", but when preceded with a '%1' it subtly changes meaning\ninto 'the remainder of the shorthand after extracting the first\ncomponent'. I believe the two interpretations are compatible and\nunambiguous, but if we want to be very explicit about what's\nhappening, we could use something like '%all' and '%the_rest' for the\ntwo cases, respectively?\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217124","messageId":"CALKQrgeirzRuto4Q==5WY8-idwj+zspcE2OBLO2=0tHMTXuvhg@mail.gmail.com","threadId":"33796","inReplyTo":"CAPig+cR-FkOtGvn+A+JdQ5ia+eUXw6iVzEmOpxCrUgkPttZkVQ@mail.gmail.com","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-13T06:32:37Z","receivedAt":"2013-05-13T06:32:37Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 13, 2013 at 6:48 AM, Eric Sunshine wrote:\n> On Sat, May 11, 2013 at 12:21 PM, Johan Herland wrote:\n>> Although we definitely support and encourage use of multi-level branch\n>> names, we have never conciously tried to give support for multi-level\n>\n> s/conciously/consciously/\n\nThanks, will fix.\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217125","messageId":"7v8v3jqsy7.fsf@alter.siamese.dyndns.org","threadId":"33796","inReplyTo":"CALKQrgf0m8r-Ofb+Ss1OpEF67dPS73b8nB+usVxH=Y=h3441Wg@mail.gmail.com","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T06:45:04Z","receivedAt":"2013-05-13T06:45:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Obviously, I named it '%1' since it expands into the _first_ component\n> of the (slash-separated) shorthand.\n\nOK, I can buy something like\n\n\t%*\n refs/%*\n refs/heads/%*\n ...\n refs/remotes/%*/HEAD\n refs/remotes/%1/%2\n refs/peers/%1/heads/%2\n\nthat is, for a pattern that has %*, we feed the end-user string as a\nwhole, and for a pattern that has %1 thru %N, we find an appropriate\nway to chop the end-user string into N pieces (e.g. nick/name would\nbe split into %1 = nick, %2 = name, while foo/bar/baz might have two\npossibilities, <%1, %2> = or ). The\nearlier ones on the above list can even be written with their %*\nsubstituted with %1 if we go that route.\n\nAnd that makes perfect sense, and is exactly the kind of \"you plan\nto have %2 and %3 that falls into the same category as %1\" I was\nasking you about in the message.\n\nSo, no more objection to %1 from me, if that is the direction you\nare taking us.\n"},{"id":"217126","messageId":"CALKQrgcry9bwmonaeWA4M7a3k36S_Q3ZQLmv7Ui5r+tdzdMr_A@mail.gmail.com","threadId":"33796","inReplyTo":"7vtxm7scn5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-13T06:53:10Z","receivedAt":"2013-05-13T06:53:10Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 13, 2013 at 6:54 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>\n>> Although we definitely support and encourage use of multi-level branch\n>> names, we have never conciously tried to give support for multi-level\n>> remote names. Currently, they are allowed, but there is no evidence that\n>> they are commonly used.\n>>\n>> Now, they do provide a source of problems when trying to expand the\n>> \"$nick/$name\" shorthand notation (where $nick matches a remote name)\n>> into a full refname. Consider the shorthand \"foo/bar/baz\": Does this\n>> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?\n>>\n>> Since we need to be unambiguous about these things, we hereby declare\n>> that a remote name shall never contain a '/' character, and that the\n>> only correct way to parse \"foo/bar/baz\" is $nick = foo, $name = bar/baz.\n>\n> I know I am guilty of hinting to go in this direction in the earlier\n> discussion, but I have to wonder how much more refactoring is needed\n> to see if there is only one unique possibility among many.\n>\n> For a string with N slashes, you have only N possible ways to split\n> it into $nick and $name, and if you see a ref \"bar/baz\" copied from\n> remote \"foo\" but no ref \"baz\" copied from remote \"foo/bar\" (or you\n> may not even have a remote \"foo/bar\" in the first place), the user\n> is already very unambiguous. The declaration is merely being lazy.\n>\n> I am not saying we must implement such a back-track to disambiguate\n> the user input better. I am wondering how much more effort on top\n> of this series is needed if we want to get there (provided that the\n> disambiguation is a good thing to do in the first place).\n\nI feel the problem with multi-level remote names runs a little deeper\nthan merely disambiguation when resolving remote-tracking refs: If you\nhave two remotes \"foo\" and \"foo/bar\", and they have branches \"bar/baz\"\nand \"baz\", respectively, then they will (in the default current\nconfiguration) end up clobbering eachother due to the overlapping\nremote-tracking branch (refs/remotes/foo/bar/baz). Although the remote\nref namespace coincidentally resolves this by mapping the two to\n\"refs/peers/foo/heads/bar/baz\" and \"refs/peers/foo/bar/heads/baz\"\nrespectively, you can easily create a different (although probably\neven more unlikely) case where the remote ref namespace causes the\nsame kind of overlap: One remote \"foo\" with branch \"heads/bar\", and\nanother remote \"foo/heads\" with branch \"bar\" will both end up\nclobbering eachother at \"refs/peers/foo/heads/heads/bar\"...\n\nThe disambiguation can probably be resolved, although the resulting\ncode will obviously be somewhat more cumbersome and ugly (and IMHO the\ncurrent code is plenty of that already...). Combine this with the\nproblems of clobbering of the same remote-tracking ref (describe\nabove), and the fact that AFAIK a multi-level remote name has never\nbeen observed \"in the wild\" (none of the people I asked at the Git\nMerge conference had ever observed multi-level remote names, nor did\nthey directly oppose banning them), I'm not at all sure it's worth\nbothering about this at all. Simply disallowing multi-level remote\nnames seems like the simpler and naturally ambiguity-resistant\napproach.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217127","messageId":"CALKQrgcMj4-YD-tEgirmAWFKSB6s7yzK=zigaR+FyjdSRPjvrA@mail.gmail.com","threadId":"33796","inReplyTo":"CAPig+cSRnsiYRQhX8Gz41D43005zgF9ysNTNSNn-C27nB9HftQ@mail.gmail.com","subject":"Re: [PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-13T06:55:21Z","receivedAt":"2013-05-13T06:55:21Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 13, 2013 at 7:19 AM, Eric Sunshine wrote:\n> On Sat, May 11, 2013 at 12:21 PM, Johan Herland wrote:\n>> + * When displaying more then just remote-tracking branches, make the\n> s/then/than/\n\nThanks, will fix.\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217228","messageId":"7vwqr2liv6.fsf@alter.siamese.dyndns.org","threadId":"33796","inReplyTo":"7v8v3jqsy7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T20:34:05Z","receivedAt":"2013-05-13T20:34:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johan Herland writes:\n>\n>> Obviously, I named it '%1' since it expands into the _first_ component\n>> of the (slash-separated) shorthand.\n>\n> OK, I can buy something like\n>\n> \t%*\n> refs/%*\n> refs/heads/%*\n> ...\n> refs/remotes/%*/HEAD\n> refs/remotes/%1/%2\n> refs/peers/%1/heads/%2\n>\n> that is, for a pattern that has %*, we feed the end-user string as a\n> whole, and for a pattern that has %1 thru %N, we find an appropriate\n> way to chop the end-user string into N pieces (e.g. nick/name would\n> be split into %1 = nick, %2 = name, while foo/bar/baz might have two\n> possibilities, <%1, %2> = or ). The\n> earlier ones on the above list can even be written with their %*\n> substituted with %1 if we go that route.\n\nJust to make sure.\n\nPlease do not let \"two possibilities\" stop you. As I said in the\nnearby thread, I do not necessarily insist that we must try all N\npossibilities. \"We find an appropriate way\" could be just \"we\nalways chop at the first slash, and %1 is what comes before it, %2\nis what comes after it\".\n\nThat will close the possibility for us to use %1 thru %N (N is\nlimited to 2), but it still is \"We have %1 and we have %2, both fall\ninto the same 'path components, numbered from left to right'\ncategory\", and justifies the use of %1 (\"one\", not \"el\").\n\nSo still no objection to %1 from me.\n\n> And that makes perfect sense, and is exactly the kind of \"you plan\n> to have %2 and %3 that falls into the same category as %1\" I was\n> asking you about in the message.\n>\n> So, no more objection to %1 from me, if that is the direction you\n> are taking us.\n"},{"id":"217329","messageId":"CALKQrgcDBMPeXPzTnpGyeosipR6Ln_zLh4Q_i1A7-eFUnTnBcA@mail.gmail.com","threadId":"33796","inReplyTo":"7vwqr2liv6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-14T14:24:03Z","receivedAt":"2013-05-14T14:24:03Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>> Johan Herland writes:\n>>> Obviously, I named it '%1' since it expands into the _first_ component\n>>> of the (slash-separated) shorthand.\n>>\n>> OK, I can buy something like\n>>\n>> %*\n>> refs/%*\n>> refs/heads/%*\n>> ...\n>> refs/remotes/%*/HEAD\n>> refs/remotes/%1/%2\n>> refs/peers/%1/heads/%2\n>>\n>> that is, for a pattern that has %*, we feed the end-user string as a\n>> whole, and for a pattern that has %1 thru %N, we find an appropriate\n>> way to chop the end-user string into N pieces (e.g. nick/name would\n>> be split into %1 = nick, %2 = name, while foo/bar/baz might have two\n>> possibilities, <%1, %2> = or ). The\n>> earlier ones on the above list can even be written with their %*\n>> substituted with %1 if we go that route.\n>\n> Just to make sure.\n>\n> Please do not let \"two possibilities\" stop you. As I said in the\n> nearby thread, I do not necessarily insist that we must try all N\n> possibilities. \"We find an appropriate way\" could be just \"we\n> always chop at the first slash, and %1 is what comes before it, %2\n> is what comes after it\".\n>\n> That will close the possibility for us to use %1 thru %N (N is\n> limited to 2), but it still is \"We have %1 and we have %2, both fall\n> into the same 'path components, numbered from left to right'\n> category\", and justifies the use of %1 (\"one\", not \"el\").\n>\n> So still no objection to %1 from me.\n\nI think I like \"refs/peers/%1/heads/%*\" better than\n\"refs/peers/%1/heads/%2\", since the latter sort of makes me wonder\nwhether the 3rd, 4th, etc. components would be discarded. That said,\nhaving %* mean \"the rest of the shorthand\" means that we must adjust\nthe expansion of %* for every preceding %N, which prevents us from\nsimplifying the code by using strbuf_expand_dict_cb() with a static\ndictionary [1].\n\nI am not sure why we would want \"refs/remotes/%1/%2\" instead of\n\"refs/remote/%*\". Maybe I've been staring at this for too long, but I\nfind the latter shorter and more descriptive and the \"%1/%2\" notation\nneedlessly cumbersome, especially if it's also supposed to match\n\"foo/bar/baz\"\n\nWhen it comes to multi-level remote names, I guess I could drop the\npatch that disallows them, and still just have \"%1\" only map to the\nfirst component (i.e. \"foo/bar/baz\" would always be interpreted as %1\n= \"foo\", and never %1 = \"foo/bar\"). This would mean that the\n\"foo/bar/baz\" shorthand notation would simply not work against\nremote-tracking branch \"baz\" from remote \"foo/bar\", but we might say\nthat's ok, because (a) multi-level remote names are so rare, and (b)\nthe simple workaround of explicitly saying\n\"refs/peers/foo/bar/heads/baz\" would always be available in any case.\n\n\n...Johan\n\n\n[1]: Maybe we could use '%N+' to mean \"everything starting from\ncomponent #N\"? Then we could construct the following dictionary the\nshorthand \"foo/bar/baz\":\n\n \"*\" -> \"foo/bar/baz\"\n \"1\" -> \"foo\"\n \"1+\" -> \"foo/bar/baz\"\n \"2\" -> \"bar\"\n \"2+\" -> \"bar/baz\"\netc.\n\nBut I really think this is overkill. Avoiding having to write our own\nexpansion helper is not _that_ important.\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217335","messageId":"7vobcdjvsh.fsf@alter.siamese.dyndns.org","threadId":"33796","inReplyTo":"CALKQrgcDBMPeXPzTnpGyeosipR6Ln_zLh4Q_i1A7-eFUnTnBcA@mail.gmail.com","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-14T17:50:06Z","receivedAt":"2013-05-14T17:50:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> I think I like \"refs/peers/%1/heads/%*\" better than\n> \"refs/peers/%1/heads/%2\", since the latter sort of makes me wonder\n> whether the 3rd, 4th, etc. components would be discarded.\n\nMakes sense.\n\n> I am not sure why we would want \"refs/remotes/%1/%2\" instead of\n> \"refs/remote/%*\".\n\nThe former way makes it easier to see what \"refs/peers/%1/heads/%2\"\nmeans, I think, but otherwise aren't they equivalent? I do not see\na strong reason to favor one over the other.\n\n> remote-tracking branch \"baz\" from remote \"foo/bar\", but we might say\n> that's ok, because (a) multi-level remote names are so rare, and (b)\n> the simple workaround of explicitly saying\n> \"refs/peers/foo/bar/heads/baz\" would always be available in any case.\n\nSounds sensible.\n\nAnd if you limit things that way, \"%1\" again starts to make sense,\nas you are representing \"the first path component\" with it.\n"},{"id":"217413","messageId":"51932F1B.9000200@alum.mit.edu","threadId":"33796","inReplyTo":"CALKQrgcDBMPeXPzTnpGyeosipR6Ln_zLh4Q_i1A7-eFUnTnBcA@mail.gmail.com","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-15T06:45:47Z","receivedAt":"2013-05-15T06:45:47Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/14/2013 04:24 PM, Johan Herland wrote:\n> On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>> Johan Herland writes:\n>>>> Obviously, I named it '%1' since it expands into the _first_ component\n>>>> of the (slash-separated) shorthand.\n>>>\n>>> OK, I can buy something like\n>>>\n>>> %*\n>>> refs/%*\n>>> refs/heads/%*\n>>> ...\n>>> refs/remotes/%*/HEAD\n>>> refs/remotes/%1/%2\n>>> refs/peers/%1/heads/%2\n>>>\n>>> that is, for a pattern that has %*, we feed the end-user string as a\n>>> whole, and for a pattern that has %1 thru %N, we find an appropriate\n>>> way to chop the end-user string into N pieces (e.g. nick/name would\n>>> be split into %1 = nick, %2 = name, while foo/bar/baz might have two\n>>> possibilities, <%1, %2> = or ). The\n>>> earlier ones on the above list can even be written with their %*\n>>> substituted with %1 if we go that route.\n>>\n>> Just to make sure.\n>>\n>> Please do not let \"two possibilities\" stop you. As I said in the\n>> nearby thread, I do not necessarily insist that we must try all N\n>> possibilities. \"We find an appropriate way\" could be just \"we\n>> always chop at the first slash, and %1 is what comes before it, %2\n>> is what comes after it\".\n>>\n>> That will close the possibility for us to use %1 thru %N (N is\n>> limited to 2), but it still is \"We have %1 and we have %2, both fall\n>> into the same 'path components, numbered from left to right'\n>> category\", and justifies the use of %1 (\"one\", not \"el\").\n>>\n>> So still no objection to %1 from me.\n> \n> I think I like \"refs/peers/%1/heads/%*\" better than\n> \"refs/peers/%1/heads/%2\", since the latter sort of makes me wonder\n> whether the 3rd, 4th, etc. components would be discarded. That said,\n> having %* mean \"the rest of the shorthand\" means that we must adjust\n> the expansion of %* for every preceding %N, which prevents us from\n> simplifying the code by using strbuf_expand_dict_cb() with a static\n> dictionary [1].\n> \n> I am not sure why we would want \"refs/remotes/%1/%2\" instead of\n> \"refs/remote/%*\". Maybe I've been staring at this for too long, but I\n> find the latter shorter and more descriptive and the \"%1/%2\" notation\n> needlessly cumbersome, especially if it's also supposed to match\n> \"foo/bar/baz\"\n\n\"refs/remotes/%1/%2\" (or \"refs/remotes/%1/%*\") might be a nice way to\nimply that the rule should only be attempted if the input has at least\ntwo components, whereas something like \"refs/heads/%*\" would be applied\neven for inputs with no slashes.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"217418","messageId":"CALKQrgdsBzcdtyej=qvaL-2rr-5t_UzXdOL-ZZ3a-rGW5V_i7Q@mail.gmail.com","threadId":"33796","inReplyTo":"51932F1B.9000200@alum.mit.edu","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-15T07:39:55Z","receivedAt":"2013-05-15T07:39:55Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty wrote:\n> On 05/14/2013 04:24 PM, Johan Herland wrote:\n>> I am not sure why we would want \"refs/remotes/%1/%2\" instead of\n>> \"refs/remote/%*\". Maybe I've been staring at this for too long, but I\n>> find the latter shorter and more descriptive and the \"%1/%2\" notation\n>> needlessly cumbersome, especially if it's also supposed to match\n>> \"foo/bar/baz\"\n>\n> \"refs/remotes/%1/%2\" (or \"refs/remotes/%1/%*\") might be a nice way to\n> imply that the rule should only be attempted if the input has at least\n> two components, whereas something like \"refs/heads/%*\" would be applied\n> even for inputs with no slashes.\n\n/me likes, at least for \"refs/remotes/%1/%*\".\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217436","messageId":"CALKQrgdroLDSf=Xc58NiHSf7fp8cRMbrR0mGrfKY7Rg0FpPFJg@mail.gmail.com","threadId":"33796","inReplyTo":"CALKQrgdsBzcdtyej=qvaL-2rr-5t_UzXdOL-ZZ3a-rGW5V_i7Q@mail.gmail.com","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-15T13:53:52Z","receivedAt":"2013-05-15T13:53:52Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, May 15, 2013 at 9:39 AM, Johan Herland wrote:\n> On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty wrote:\n>> \"refs/remotes/%1/%2\" (or \"refs/remotes/%1/%*\") might be a nice way to\n>> imply that the rule should only be attempted if the input has at least\n>> two components, whereas something like \"refs/heads/%*\" would be applied\n>> even for inputs with no slashes.\n>\n> /me likes, at least for \"refs/remotes/%1/%*\".\n\nUnfortunately, using \"refs/remotes/%1/%*\" instead of \"refs/remotes/%*\"\nbreaks a number of git-svn tests which puts refs directly within\nrefs/remotes/, and then does things like \"git reset --hard trunk\"\n(expecting trunk -> refs/remotes/trunk, which the refs/remotes/%1/%*\ndoesn't match).\n\nI don't know if putting refs directly within refs/remotes/ is\nsomething that git-svn does by default (which would prevent us from\nchanging \"refs/remotes/%*\" to \"refs/remotes/%1/%*\"), or whether it is\nspecific to the tests (in which case we should fix the tests).\n\nAlso, there might be too many other users of refs directly within\nrefs/remotes/ that expect \"foo_without_slash\" to expand to\n\"refs/remotes/foo_without_slash\", which would prevent us from doing\nthis change.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217441","messageId":"7v38toe0n3.fsf@alter.siamese.dyndns.org","threadId":"33796","inReplyTo":"CALKQrgdroLDSf=Xc58NiHSf7fp8cRMbrR0mGrfKY7Rg0FpPFJg@mail.gmail.com","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-15T15:14:08Z","receivedAt":"2013-05-15T15:14:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Wed, May 15, 2013 at 9:39 AM, Johan Herland wrote:\n>> On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty wrote:\n>>> \"refs/remotes/%1/%2\" (or \"refs/remotes/%1/%*\") might be a nice way to\n>>> imply that the rule should only be attempted if the input has at least\n>>> two components, whereas something like \"refs/heads/%*\" would be applied\n>>> even for inputs with no slashes.\n>>\n>> /me likes, at least for \"refs/remotes/%1/%*\".\n>\n> Unfortunately, using \"refs/remotes/%1/%*\" instead of \"refs/remotes/%*\"\n> breaks a number of git-svn tests which puts refs directly within\n> refs/remotes/, and then does things like \"git reset --hard trunk\"\n> (expecting trunk -> refs/remotes/trunk, which the refs/remotes/%1/%*\n> doesn't match).\n\nOh, I never thought refs/remotes/%1/%* was a suggestion for a\nserious \"improvement\", but was merely a thought experiment \"if all\nthe remotes were at least two level names, we could express it like\nthis to stress that fact\".\n\nWe already allow 'origin' to refer to refs/remotes/origin/HEAD, so\nit is clear refs/remotes/%1/%* alone will not be able to replace\nwhat we have; we need refs/remotes/%* and refs/remotes/%*/HEAD\nanyway.\n"},{"id":"217473","messageId":"20130515194959.GB28720@dcvr.yhbt.net","threadId":"33796","inReplyTo":"CALKQrgdroLDSf=Xc58NiHSf7fp8cRMbrR0mGrfKY7Rg0FpPFJg@mail.gmail.com","subject":"Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-05-15T19:49:59Z","receivedAt":"2013-05-15T19:49:59Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Johan Herland wrote:\n> Unfortunately, using \"refs/remotes/%1/%*\" instead of \"refs/remotes/%*\"\n> breaks a number of git-svn tests which puts refs directly within\n> refs/remotes/, and then does things like \"git reset --hard trunk\"\n> (expecting trunk -> refs/remotes/trunk, which the refs/remotes/%1/%*\n> doesn't match).\n> \n> I don't know if putting refs directly within refs/remotes/ is\n> something that git-svn does by default (which would prevent us from\n> changing \"refs/remotes/%*\" to \"refs/remotes/%1/%*\"), or whether it is\n> specific to the tests (in which case we should fix the tests).\n\nYes, git-svn uses refs/remotes/%* by default.\n\nThis was a design mistake on my part. I think it's too late to change,\nnow, as existing users will encounter breakage and/or out-of-date\ndocumentation (which is even more confusing, as git-svn is often the\nfirst introduction to git for SVN users).\n"},{"id":"217554","messageId":"CALkWK0k=P265MCmvj9C7MFiF=tkhCZ__3oF8FWmWd4sNx3VcTw@mail.gmail.com","threadId":"33796","inReplyTo":"CALKQrgcry9bwmonaeWA4M7a3k36S_Q3ZQLmv7Ui5r+tdzdMr_A@mail.gmail.com","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-16T09:48:37Z","receivedAt":"2013-05-16T09:48:37Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Johan Herland wrote:\n> The disambiguation can probably be resolved, although the resulting\n> code will obviously be somewhat more cumbersome and ugly (and IMHO the\n> current code is plenty of that already...). Combine this with the\n> problems of clobbering of the same remote-tracking ref (describe\n> above), and the fact that AFAIK a multi-level remote name has never\n> been observed \"in the wild\" (none of the people I asked at the Git\n> Merge conference had ever observed multi-level remote names, nor did\n> they directly oppose banning them), I'm not at all sure it's worth\n> bothering about this at all. Simply disallowing multi-level remote\n> names seems like the simpler and naturally ambiguity-resistant\n> approach.\n\nThe problem with multi-level remote names is that we use the same\ndelimiter as in the ref namespace: '/'. So, obviously, there's a lot\nof room for confusion. I wonder if we should banish multi-level\nremotes altogether though: is it possible that they will be useful to\nsomeone in the future?\n"},{"id":"217581","messageId":"CALKQrgewHCrkf0kP57Mm+werT9yAWnLZP7CJn-k3g85gfPRspA@mail.gmail.com","threadId":"33796","inReplyTo":"CALkWK0k=P265MCmvj9C7MFiF=tkhCZ__3oF8FWmWd4sNx3VcTw@mail.gmail.com","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-16T11:17:03Z","receivedAt":"2013-05-16T11:17:03Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thu, May 16, 2013 at 11:48 AM, Ramkumar Ramachandra\n wrote:\n> Johan Herland wrote:\n>> The disambiguation can probably be resolved, although the resulting\n>> code will obviously be somewhat more cumbersome and ugly (and IMHO the\n>> current code is plenty of that already...). Combine this with the\n>> problems of clobbering of the same remote-tracking ref (describe\n>> above), and the fact that AFAIK a multi-level remote name has never\n>> been observed \"in the wild\" (none of the people I asked at the Git\n>> Merge conference had ever observed multi-level remote names, nor did\n>> they directly oppose banning them), I'm not at all sure it's worth\n>> bothering about this at all. Simply disallowing multi-level remote\n>> names seems like the simpler and naturally ambiguity-resistant\n>> approach.\n>\n> The problem with multi-level remote names is that we use the same\n> delimiter as in the ref namespace: '/'. So, obviously, there's a lot\n> of room for confusion. I wonder if we should banish multi-level\n> remotes altogether though: is it possible that they will be useful to\n> someone in the future?\n\nTechnically, the problem is that we don't use a different delimiter\nbetween the $remote and $ref parts. If we had used\n\"multi/level/remote:nested/ref\" we would have been OK (on this issue\nat least, probably not OK on other issues).\n\nFWIW, I've abandoned this patch, and don't care much about multi-level\nremote names anymore. They work in current git, and they will sort-of\nwork with remote ref namespaces as well, although you will have to use\nfull refnames when referring to their remote-tracking refs. If\nmulti-level remote names suddenly become popular, we can change the\ncode to try to resolve them unambiguously.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"217585","messageId":"CALkWK0kRzADXq8ePTkDLg76-0du7+5Uvbn7pXu3Tw0d7VLsPow@mail.gmail.com","threadId":"33796","inReplyTo":"CALKQrgewHCrkf0kP57Mm+werT9yAWnLZP7CJn-k3g85gfPRspA@mail.gmail.com","subject":"Re: [PATCHv2 04/10] remote: Reject remote names containing '/'","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-16T12:14:40Z","receivedAt":"2013-05-16T12:14:40Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Johan Herland wrote:\n> FWIW, I've abandoned this patch, and don't care much about multi-level\n> remote names anymore. They work in current git, and they will sort-of\n> work with remote ref namespaces as well, although you will have to use\n> full refnames when referring to their remote-tracking refs. If\n> multi-level remote names suddenly become popular, we can change the\n> code to try to resolve them unambiguously.\n\nMakes sense. We can deal with the issue if and when multi-level\nremotes become popular.\n"}]}