[PATCHv2 00/10] Prepare for alternative remote-tracking branch location

[PATCHv2 00/10] Prepare for alternative remote-tracking branch location

[Johan Herland]

Hi,

Here is the second iteration of the current series for teaching git to
work with remote ref namespaces. This iteration is pretty much a full
rework of the first iteration, based on Junio's comments to the first
iteration, and discussion with other Git developers at the Git Merge
conference in Berlin, particularily Jonathan Nieder has helped to work
out some of these issues.

Patches #1 - #2 are pretty much unchanged from v1, except that we now
put the remote refs in refs/peers/* instead of /refs/remotes/*.

Patches #3 - #5 gets us to the point where "git rev-parse origin/master"
will expand to "refs/peers/origin/heads/master", and vice versa for
shortening

Patches #6-#10 (except #7 which is a small unrelated test addition) is
about making "git branch -a" and "git branch -r" output remote-tracking
branches from the refs/peers/* hierarchy.

Note that this patch series is based on top of the jh/shorten-refname
series in 'pu'.


Have fun!

...Johan


Johan Herland (10):
  t7900: Start testing usability of namespaced remote refs
  t7900: Demonstrate failure to expand "$peer/$branch" according to refspecs
  refs.c: Refactor code for mapping between shorthand names and full refnames
  remote: Reject remote names containing '/'
  refs.c: Add support for expanding/shortening refs in refs/peers/*
  t7900: Test git branch -r/-a output w/remote-tracking branches in refs/peers/*
  t3203: Add testcase for fix in 1603ade81352a526ccb206f41ff81ecbc855df2d
  builtin/branch.c: Refactor ref_item.name and .dest into strbufs
  builtin/branch.c: Refactor "remotes/" prepending to remote-tracking branches
  branch: Fix display of remote branches in refs/peers/*

 builtin/branch.c                               | 114 +++++------
 builtin/remote.c                               |   4 +-
 cache.h                                        |   4 -
 refs.c                                         | 256 ++++++++++++++++++-------
 refs.h                                         |   6 +
 remote.c                                       |   6 +-
 t/t3203-branch-output.sh                       |  15 ++
 t/t5505-remote.sh                              |  12 ++
 t/t7900-working-with-namespaced-remote-refs.sh | 131 +++++++++++++
 t/t7901-multi-level-remote-name-failure.sh     |  20 ++
 10 files changed, 430 insertions(+), 138 deletions(-)
 create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh
 create mode 100755 t/t7901-multi-level-remote-name-failure.sh

-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 01/10] t7900: Start testing usability of namespaced remote refs

[Johan Herland]

Some users are interested in fetching remote refs into a separate namespace
in the local repo. E.g. instead of the usual remote config:

  [remote "origin"]
	fetch = +refs/heads/*:refs/remotes/origin/*
	url = ...

they want to keep remote tags separate from local tags, and they may also
want to fetch other ref types:

  [remote "origin"]
	fetch = +refs/heads/*:refs/peers/origin/heads/*
	fetch = +refs/tags/*:refs/peers/origin/tags/*
	fetch = +refs/notes/*:refs/peers/origin/notes/*
	fetch = +refs/replace/*:refs/peers/origin/replace/*
	tagopt = "--no-tags"
	url = ...

This configuration creates a separate namespace under refs/peers/origin/*
mirroring the structure of local refs (under refs/*) where all the relevant
refs from the 'origin' remote can be found. The reason for renaming the
refs/remotes/ prefix to refs/peers/ is to allow Git to treat the two
hierarchies somewhat differently with respect to how shorthand names are
expanded by the ref machinery.

This patch introduces a test whose main purpose is to verify that git will
work comfortably with this kind of setup. For now, we only verify that it
is possible (though not exactly easy) to establish a clone with the above
configuration, and that fetching into it yields the expected result.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t7900-working-with-namespaced-remote-refs.sh | 82 ++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
new file mode 100755
index 0000000..dfd916b
--- /dev/null
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='testing end-user usability of namespaced remote refs
+
+Set up a local repo with namespaced remote refs, like this:
+
+[remote "origin"]
+	fetch = +refs/heads/*:refs/peers/origin/heads/*
+	fetch = +refs/tags/*:refs/peers/origin/tags/*
+	fetch = +refs/notes/*:refs/peers/origin/notes/*
+	fetch = +refs/replace/*:refs/peers/origin/replace/*
+	tagopt = "--no-tags"
+	url = ...
+
+Test that the usual end-user operations work as expected with this setup.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup server repo' '
+	git init server &&
+	(
+		cd server &&
+		test_commit server_master_a &&
+		git checkout -b other &&
+		test_commit server_other_b &&
+		git checkout master &&
+		test_commit server_master_b
+	)
+'
+
+server_master_a=$(git --git-dir=server/.git rev-parse --verify server_master_a)
+server_master_b=$(git --git-dir=server/.git rev-parse --verify server_master_b)
+server_other_b=$(git --git-dir=server/.git rev-parse --verify server_other_b)
+
+cat >expect.refspecs << EOF
++refs/heads/*:refs/peers/origin/heads/*
++refs/tags/*:refs/peers/origin/tags/*
++refs/notes/*:refs/peers/origin/notes/*
++refs/replace/*:refs/peers/origin/replace/*
+EOF
+
+cat >expect.show-ref << EOF
+$server_master_b refs/heads/master
+$server_master_b refs/peers/origin/heads/master
+$server_other_b refs/peers/origin/heads/other
+$server_master_a refs/peers/origin/tags/server_master_a
+$server_master_b refs/peers/origin/tags/server_master_b
+$server_other_b refs/peers/origin/tags/server_other_b
+EOF
+
+test_clone() {
+	( cd $1 && git config --get-all remote.origin.fetch ) >actual.refspecs &&
+	test_cmp expect.refspecs actual.refspecs &&
+	( cd $1 && git show-ref ) >actual.show-ref &&
+	test_cmp expect.show-ref actual.show-ref
+}
+
+test_expect_failure 'clone with namespaced remote refs' '
+	git clone --layout=peers server client &&
+	test_clone client
+'
+
+# Work-around for the not-yet-existing clone option used above
+test_expect_success 'work-around "clone" with namespaced remote refs' '
+	rm -rf client &&
+	git init client &&
+	(
+		cd client &&
+		git config remote.origin.url ../server &&
+		git config --add remote.origin.fetch "+refs/heads/*:refs/peers/origin/heads/*" &&
+		git config --add remote.origin.fetch "+refs/tags/*:refs/peers/origin/tags/*" &&
+		git config --add remote.origin.fetch "+refs/notes/*:refs/peers/origin/notes/*" &&
+		git config --add remote.origin.fetch "+refs/replace/*:refs/peers/origin/replace/*" &&
+		git config remote.origin.tagopt "--no-tags" &&
+		git fetch &&
+		git checkout master
+	) &&
+	test_clone client
+'
+
+test_done
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 02/10] t7900: Demonstrate failure to expand "$peer/$branch" according to refspecs

[Johan Herland]

This test verifies that the following expressions all evaluate to the
full refname "refs/peers/origin/heads/master":

 - refs/peers/origin/heads/master
 - peers/origin/heads/master
 - origin/heads/master
 - origin/master

(We assume that there are no other conflicting refs for which the above
expressions would cause ambiguity.)

Currently none of these work, because the refs machinery don't know how
to expand shorthand names within the refs/peers/* hierarchy.

Mirroring the expansion of the above 4 expressions into the full refname,
the same 4 expressions should also be shortened into "origin/master" when
abbreviating them into their shortest unambiguous representation, e.g.
when running "git rev-parse --abbrev-ref" on them. A (currently failing)
test verifying this behavior is also added by this patch.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t7900-working-with-namespaced-remote-refs.sh | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
index dfd916b..109e9b8 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -79,4 +79,32 @@ test_expect_success 'work-around "clone" with namespaced remote refs' '
 	test_clone client
 '
 
+test_expect_success 'enter client repo' '
+	cd client
+'
+
+test_expect_failure 'short-hand notation expands correctly for remote-tracking branches' '
+	echo refs/peers/origin/heads/master >expect &&
+	git rev-parse --symbolic-full-name refs/peers/origin/heads/master >actual &&
+	test_cmp expect actual &&
+	git rev-parse --symbolic-full-name peers/origin/heads/master >actual &&
+	test_cmp expect actual &&
+	git rev-parse --symbolic-full-name origin/heads/master >actual &&
+	test_cmp expect actual &&
+	git rev-parse --symbolic-full-name origin/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'remote-tracking branches are shortened correctly' '
+	echo origin/master >expect &&
+	git rev-parse --abbrev-ref refs/peers/origin/heads/master >actual &&
+	test_cmp expect actual &&
+	git rev-parse --abbrev-ref peers/origin/heads/master >actual &&
+	test_cmp expect actual &&
+	git rev-parse --abbrev-ref origin/heads/master >actual &&
+	test_cmp expect actual &&
+	git rev-parse --abbrev-ref origin/master >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Johan Herland]

This patch is in preparation for extending the ways in which we expand
shorthand names into full refnames, and shorten full refnames into
unambiguous shorthand names.

We collect the logic for performing the expansion and shortening into two
functions: refname_expand() and refname_shorten(). refname_expand() takes
a shorthand name, and a pattern containing a wildcard (more on those
below), and substitutes the shorthand name for the wildcard. The result
is written into a strbuf provided by the caller. Correspondingly,
refname_shorten() takes a full refname and matches it against the given
pattern, extracting the portion of the full refname that matches the
pattern wildcard. The resulting shorthand name is written into a strbuf
provided by the caller.

The refname_expand() function no longer uses mkpath()/mksnpath() to
perform the pattern expansion. Instead, it uses strbuf_expand(), which
removes the need for using fixed-length buffers from the code. This also
means we no longer call cleanup_path() on the resulting refs, but AFAICS,
this is not really needed anyway (also it breaks no existing tests).

Since the pattern expansion is now done by strbuf_expand(), the syntax of
the pattern wildcard must change as well. Whereas a pattern used to look
like this:

    "refs/remotes/%.*s/HEAD"

it now looks like this:

    "refs/remotes/%*/HEAD"

where the "%*" wildcard is meant to be read as "the full shorthand name".
More wildcards will be added by future patches that require more complex
wildcard matching.

The list of patterns has been renamed from "ref_rev_parse_rules" to
"refname_patterns" (which should be more descriptive), and it has lost
the NULL terminator, which is no longer needed, since all iterations of
the list now uses ARRAY_SIZE(refname_patterns) to end iteration, instead
of testing each member against NULL.

Furthermore, refname_match() used to take the list of patterns as an
argument, but since all callers pass the one and only pattern list, we
can drop the argument altogether, and use refname_patterns directly in
refname_match(). This also enables us to make the pattern list static.

The code for shortening full refnames into shorthands, has been adjusted
to deal with the new "%*" patterns. It now uses strbuf_split_str() with
the '%' as terminator, to split the prefix part from the suffix part.
Then - as before - it matches the prefix from the start of the given
refname and the suffix from the end of the refname. If both matches are
successful, the middle portion is extracted as the resulting shorthand
name.

The end result of this refactoring is equivalent in behavior to the
existing code, but makes it easier for future patches to adjust how
expansion and shortening happens.

All of the existing users of ref_rev_parse_rules are converted to
refname_patterns by this patch, so ref_rev_parse_rules is removed
as well.

Signed-off-by: Johan Herland <johan@herland.net>
---
 cache.h  |   4 --
 refs.c   | 187 ++++++++++++++++++++++++++++++++++++++-------------------------
 refs.h   |   3 +
 remote.c |   6 +-
 4 files changed, 119 insertions(+), 81 deletions(-)

diff --git a/cache.h b/cache.h
index ac620b2..0cc43b2 100644
--- a/cache.h
+++ b/cache.h
@@ -874,10 +874,6 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 extern int interpret_branch_name(const char *str, struct strbuf *);
 extern int get_sha1_mb(const char *str, unsigned char *sha1);
 
-extern int refname_match(const char *abbrev_name, const char *full_name, const char **rules);
-extern const char *ref_rev_parse_rules[];
-#define ref_fetch_rules ref_rev_parse_rules
-
 extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
 extern int validate_headref(const char *ref);
 
diff --git a/refs.c b/refs.c
index 006f369..ab5e120 100644
--- a/refs.c
+++ b/refs.c
@@ -1783,27 +1783,91 @@ const char *prettify_refname(const char *name)
 		0);
 }
 
-const char *ref_rev_parse_rules[] = {
-	"%.*s",
-	"refs/%.*s",
-	"refs/tags/%.*s",
-	"refs/heads/%.*s",
-	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
-	NULL
+static const char *refname_patterns[] = {
+	"%*",
+	"refs/%*",
+	"refs/tags/%*",
+	"refs/heads/%*",
+	"refs/remotes/%*",
+	"refs/remotes/%*/HEAD"
 };
 
-int refname_match(const char *abbrev_name, const char *full_name, const char **rules)
+struct wildcard_data {
+	const char *s;
+	size_t len;
+	int done;
+};
+
+static size_t refname_expand_helper(struct strbuf *sb, const char *placeholder,
+				    void *context)
 {
-	const char **p;
-	const int abbrev_name_len = strlen(abbrev_name);
+	struct wildcard_data *cb = context;
+	if (*placeholder == '*' && !cb->done) {
+		strbuf_add(sb, cb->s, cb->len);
+		cb->done = 1;
+		return 1;
+	}
+	return 0;
+}
 
-	for (p = rules; *p; p++) {
-		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
+static int refname_expand(struct strbuf *dst, const char *pattern,
+			  const char *shortname, size_t shortname_len)
+{
+	struct wildcard_data cbdata = { shortname, shortname_len, 0 };
+	strbuf_reset(dst);
+	strbuf_expand(dst, pattern, refname_expand_helper, &cbdata);
+	if (!cbdata.done)
+		strbuf_reset(dst);
+	return !cbdata.done;
+}
+
+static int refname_shorten(struct strbuf *dst, const char *pattern,
+			   const char *refname, size_t refname_len)
+{
+	/*
+	 * Match refname against pattern, using "%*" as wildcard, and
+	 * extract the wildcard-matching portion of refname into dst.
+	 * Return 0 on success (positive match, wildcard-matching portion
+	 * copied into dst), and non-zero on failure (no match, dst empty).
+	 */
+	const char *match_end;
+	struct strbuf **fragments = strbuf_split_str(pattern, '%', 2);
+	struct strbuf *prefix = fragments[0], *suffix = fragments[1];
+	assert(!fragments[2]);
+	assert(prefix->len && prefix->buf[prefix->len - 1] == '%');
+	assert(suffix->len && suffix->buf[0] == '*');
+
+	strbuf_reset(dst);
+	match_end = refname + refname_len - (suffix->len - 1);
+	if (refname_len <= (prefix->len - 1) + (suffix->len - 1) ||
+	    memcmp(prefix->buf, refname, prefix->len - 1) ||
+	    memcmp(suffix->buf + 1, match_end, suffix->len - 1)) {
+		strbuf_list_free(fragments);
+		return 1; /* refname does not match pattern */
+	}
+
+	refname += prefix->len - 1;
+	strbuf_add(dst, refname, match_end - refname);
+	strbuf_list_free(fragments);
+	return 0;
+}
+
+int refname_match(const char *abbrev_name, const char *full_name)
+{
+	size_t i;
+	const size_t abbrev_len = strlen(abbrev_name);
+	struct strbuf exp = STRBUF_INIT;
+
+	for (i = 0; i < ARRAY_SIZE(refname_patterns); i++) {
+		if (!refname_expand(&exp, refname_patterns[i],
+				    abbrev_name, abbrev_len) &&
+		    !strcmp(full_name, exp.buf)) {
+			strbuf_release(&exp);
 			return 1;
 		}
 	}
 
+	strbuf_release(&exp);
 	return 0;
 }
 
@@ -1866,30 +1930,33 @@ static char *substitute_branch_name(const char **string, int *len)
 int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
 	char *last_branch = substitute_branch_name(&str, &len);
-	const char **p, *r;
+	struct strbuf exp = STRBUF_INIT;
 	int refs_found = 0;
+	size_t i;
 
 	*ref = NULL;
-	for (p = ref_rev_parse_rules; *p; p++) {
-		char fullref[PATH_MAX];
+	for (i = 0; i < ARRAY_SIZE(refname_patterns); i++) {
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
+		const char *r;
 		int flag;
 
 		this_result = refs_found ? sha1_from_ref : sha1;
-		mksnpath(fullref, sizeof(fullref), *p, len, str);
-		r = resolve_ref_unsafe(fullref, this_result, 1, &flag);
+		if (refname_expand(&exp, refname_patterns[i], str, len))
+			continue;
+		r = resolve_ref_unsafe(exp.buf, this_result, 1, &flag);
 		if (r) {
-			if (!refs_found++)
+			if ((!*ref || strcmp(*ref, r)) && !refs_found++)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
 				break;
-		} else if ((flag & REF_ISSYMREF) && strcmp(fullref, "HEAD")) {
-			warning("ignoring dangling symref %s.", fullref);
-		} else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {
-			warning("ignoring broken ref %s.", fullref);
+		} else if ((flag & REF_ISSYMREF) && strcmp(exp.buf, "HEAD")) {
+			warning("ignoring dangling symref %s.", exp.buf);
+		} else if ((flag & REF_ISBROKEN) && strchr(exp.buf, '/')) {
+			warning("ignoring broken ref %s.", exp.buf);
 		}
 	}
+	strbuf_release(&exp);
 	free(last_branch);
 	return refs_found;
 }
@@ -1897,24 +1964,25 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 {
 	char *last_branch = substitute_branch_name(&str, &len);
-	const char **p;
+	struct strbuf path = STRBUF_INIT;
 	int logs_found = 0;
+	size_t i;
 
 	*log = NULL;
-	for (p = ref_rev_parse_rules; *p; p++) {
+	for (i = 0; i < ARRAY_SIZE(refname_patterns); i++) {
 		struct stat st;
 		unsigned char hash[20];
-		char path[PATH_MAX];
 		const char *ref, *it;
 
-		mksnpath(path, sizeof(path), *p, len, str);
-		ref = resolve_ref_unsafe(path, hash, 1, NULL);
+		if (refname_expand(&path, refname_patterns[i], str, len))
+			continue;
+		ref = resolve_ref_unsafe(path.buf, hash, 1, NULL);
 		if (!ref)
 			continue;
-		if (!stat(git_path("logs/%s", path), &st) &&
+		if (!stat(git_path("logs/%s", path.buf), &st) &&
 		    S_ISREG(st.st_mode))
-			it = path;
-		else if (strcmp(ref, path) &&
+			it = path.buf;
+		else if (strcmp(ref, path.buf) &&
 			 !stat(git_path("logs/%s", ref), &st) &&
 			 S_ISREG(st.st_mode))
 			it = ref;
@@ -1927,6 +1995,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		if (!warn_ambiguous_refs)
 			break;
 	}
+	strbuf_release(&path);
 	free(last_branch);
 	return logs_found;
 }
@@ -3004,47 +3073,20 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)
 	return NULL;
 }
 
-static int shorten_ref(const char *refname, const char *pattern, char *short_name)
-{
-	/*
-	 * pattern must be of the form "[pre]%.*s[post]". If refname
-	 * starts with "[pre]" and ends with "[post]", extract the middle
-	 * part into short_name, and return the number of chars in the
-	 * middle part (not counting the added NUL-terminator). Otherwise,
-	 * if refname does not match pattern, return 0.
-	 */
-	int match_len;
-	const char *match_start, *sep = strstr(pattern, "%.*s");
-	if (!sep || strstr(sep + 4, "%.*s"))
-		die("invalid pattern in ref_rev_parse_rules: %s", pattern);
-	match_start = refname + (sep - pattern);
-	match_len = strlen(refname) - (strlen(pattern) - 4);
-	if (match_len <= 0 ||
-	    memcmp(refname, pattern, match_start - refname) ||
-	    strcmp(match_start + match_len, sep + 4))
-		return 0; /* refname does not match */
-	memcpy(short_name, match_start, match_len);
-	short_name[match_len] = '\0';
-	return match_len;
-}
-
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
+	size_t refname_len = strlen(refname);
 	int i;
-	char *short_name;
-
-	/* buffer for scanf result, at most refname must fit */
-	short_name = xstrdup(refname);
+	struct strbuf shortname = STRBUF_INIT;
+	struct strbuf expanded = STRBUF_INIT;
 
 	/* skip first rule, it will always match */
-	for (i = ARRAY_SIZE(ref_rev_parse_rules) - 2; i > 0; --i) {
+	for (i = ARRAY_SIZE(refname_patterns) - 1; i > 0; --i) {
 		int j;
 		int rules_to_fail = i;
-		int short_name_len;
 
-		if (!(short_name_len = shorten_ref(refname,
-						   ref_rev_parse_rules[i],
-						   short_name)))
+		if (refname_shorten(&shortname, refname_patterns[i],
+				    refname, refname_len))
 			continue;
 
 		/*
@@ -3052,16 +3094,13 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		 * must fail to resolve to a valid non-ambiguous ref
 		 */
 		if (strict)
-			rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules) - 1;
+			rules_to_fail = ARRAY_SIZE(refname_patterns);
 
 		/*
 		 * check if the short name resolves to a valid ref,
 		 * but use only rules prior to the matched one
 		 */
 		for (j = 0; j < rules_to_fail; j++) {
-			const char *rule = ref_rev_parse_rules[j];
-			char refname[PATH_MAX];
-
 			/* skip matched rule */
 			if (i == j)
 				continue;
@@ -3069,23 +3108,23 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 			/*
 			 * the short name is ambiguous, if it resolves
 			 * (with this previous rule) to a valid ref
-			 * read_ref() returns 0 on success
 			 */
-			mksnpath(refname, sizeof(refname),
-				 rule, short_name_len, short_name);
-			if (ref_exists(refname))
+			if (!refname_expand(&expanded, refname_patterns[j],
+					    shortname.buf, shortname.len) &&
+			    ref_exists(expanded.buf))
 				break;
 		}
+		strbuf_release(&expanded);
 
 		/*
 		 * short name is non-ambiguous if all previous rules
 		 * haven't resolved to a valid ref
 		 */
 		if (j == rules_to_fail)
-			return short_name;
+			return strbuf_detach(&shortname, NULL);
 	}
 
-	free(short_name);
+	strbuf_release(&shortname);
 	return xstrdup(refname);
 }
 
diff --git a/refs.h b/refs.h
index 8060ed8..e05c1f1 100644
--- a/refs.h
+++ b/refs.h
@@ -164,6 +164,9 @@ extern int for_each_reflog(each_ref_fn, void *);
 extern int check_refname_format(const char *refname, int flags);
 
 extern const char *prettify_refname(const char *refname);
+
+extern int refname_match(const char *abbrev_name, const char *full_name);
+
 extern char *shorten_unambiguous_ref(const char *refname, int strict);
 
 /** rename ref, return 0 on success **/
diff --git a/remote.c b/remote.c
index 68eb99b..8ca4823 100644
--- a/remote.c
+++ b/remote.c
@@ -981,7 +981,7 @@ static int count_refspec_match(const char *pattern,
 		char *name = refs->name;
 		int namelen = strlen(name);
 
-		if (!refname_match(pattern, name, ref_rev_parse_rules))
+		if (!refname_match(pattern, name))
 			continue;
 
 		/* A match is "weak" if it is with refs outside
@@ -1499,7 +1499,7 @@ int branch_merge_matches(struct branch *branch,
 {
 	if (!branch || i < 0 || i >= branch->merge_nr)
 		return 0;
-	return refname_match(branch->merge[i]->src, refname, ref_fetch_rules);
+	return refname_match(branch->merge[i]->src, refname);
 }
 
 static int ignore_symref_update(const char *refname)
@@ -1545,7 +1545,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c
 {
 	const struct ref *ref;
 	for (ref = refs; ref; ref = ref->next) {
-		if (refname_match(name, ref->name, ref_fetch_rules))
+		if (refname_match(name, ref->name))
 			return ref;
 	}
 	return NULL;
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> The refname_expand() function no longer uses mkpath()/mksnpath() to
> perform the pattern expansion. Instead, it uses strbuf_expand(), which
> removes the need for using fixed-length buffers from the code.

It is a brilliant idea to use strbuf_expand() for this. I like it.

I notice that you later introduce %1 (that is 'one', not 'el'), but
unless you are planning to introduce %2 and %3 that semantically
fall into a similar category as %1, I would rather see a different
letter used that is mnemonic to what the placeholder _means_.  

The choice of the letter is arbitrary and may not look like it
matters that much, because it is not exposed to the end user.  But
by switching from the sprintf() semantics that shows things given to
it in the order they were given, without knowing what they mean, and
introducing a strbuf_expand() machinery tailored for refnames (and
refnames only), the new code assigns meanings to each part of the
refname, and we can afford to be more descriptive.

The choice of '%*' is justifiable, "it is the closest to the '*' we
traditionally used to replace only one thing", but '%1' does not
look the best placeholder to use, at least to me.

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Johan Herland]

On Mon, May 13, 2013 at 6:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> The refname_expand() function no longer uses mkpath()/mksnpath() to
>> perform the pattern expansion. Instead, it uses strbuf_expand(), which
>> removes the need for using fixed-length buffers from the code.
>
> It is a brilliant idea to use strbuf_expand() for this. I like it.
>
> I notice that you later introduce %1 (that is 'one', not 'el'), but
> unless you are planning to introduce %2 and %3 that semantically
> fall into a similar category as %1, I would rather see a different
> letter used that is mnemonic to what the placeholder _means_.
>
> The choice of the letter is arbitrary and may not look like it
> matters that much, because it is not exposed to the end user.  But
> by switching from the sprintf() semantics that shows things given to
> it in the order they were given, without knowing what they mean, and
> introducing a strbuf_expand() machinery tailored for refnames (and
> refnames only), the new code assigns meanings to each part of the
> refname, and we can afford to be more descriptive.
>
> The choice of '%*' is justifiable, "it is the closest to the '*' we
> traditionally used to replace only one thing", but '%1' does not
> look the best placeholder to use, at least to me.

Obviously, I named it '%1' since it expands into the _first_ component
of the (slash-separated) shorthand. There is no further parsing or
verification that it actually corresponds to a remote (and as far as I
currently understand, we do not want to do such verification), so I
thought it better not to make such assumptions in the placeholder
name. That said, I could go with '%r' for "remote", although we have
plenty of other concepts in Git that use 'r' as the initial letter. I
could maybe use '%remote' instead?

Also, about the '%*': When used alone, it means "the entire
shorthand", but when preceded with a '%1' it subtly changes meaning
into 'the remainder of the shorthand after extracting the first
component'. I believe the two interpretations are compatible and
unambiguous, but if we want to be very explicit about what's
happening, we could use something like '%all' and '%the_rest' for the
two cases, respectively?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Obviously, I named it '%1' since it expands into the _first_ component
> of the (slash-separated) shorthand.

OK, I can buy something like

	%*
        refs/%*
        refs/heads/%*
        ...
        refs/remotes/%*/HEAD
        refs/remotes/%1/%2
        refs/peers/%1/heads/%2

that is, for a pattern that has %*, we feed the end-user string as a
whole, and for a pattern that has %1 thru %N, we find an appropriate
way to chop the end-user string into N pieces (e.g. nick/name would
be split into %1 = nick, %2 = name, while foo/bar/baz might have two
possibilities, <%1, %2> = <foo, bar/baz> or <foo/bar, baz>).  The
earlier ones on the above list can even be written with their %*
substituted with %1 if we go that route.

And that makes perfect sense, and is exactly the kind of "you plan
to have %2 and %3 that falls into the same category as %1" I was
asking you about in the message.

So, no more objection to %1 from me, if that is the direction you
are taking us.

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johan Herland <johan@herland.net> writes:
>
>> Obviously, I named it '%1' since it expands into the _first_ component
>> of the (slash-separated) shorthand.
>
> OK, I can buy something like
>
> 	%*
>         refs/%*
>         refs/heads/%*
>         ...
>         refs/remotes/%*/HEAD
>         refs/remotes/%1/%2
>         refs/peers/%1/heads/%2
>
> that is, for a pattern that has %*, we feed the end-user string as a
> whole, and for a pattern that has %1 thru %N, we find an appropriate
> way to chop the end-user string into N pieces (e.g. nick/name would
> be split into %1 = nick, %2 = name, while foo/bar/baz might have two
> possibilities, <%1, %2> = <foo, bar/baz> or <foo/bar, baz>).  The
> earlier ones on the above list can even be written with their %*
> substituted with %1 if we go that route.

Just to make sure.

Please do not let "two possibilities" stop you.  As I said in the
nearby thread, I do not necessarily insist that we must try all N
possibilities.  "We find an appropriate way" could be just "we
always chop at the first slash, and %1 is what comes before it, %2
is what comes after it".

That will close the possibility for us to use %1 thru %N (N is
limited to 2), but it still is "We have %1 and we have %2, both fall
into the same 'path components, numbered from left to right'
category", and justifies the use of %1 ("one", not "el").

So still no objection to %1 from me.

> And that makes perfect sense, and is exactly the kind of "you plan
> to have %2 and %3 that falls into the same category as %1" I was
> asking you about in the message.
>
> So, no more objection to %1 from me, if that is the direction you
> are taking us.

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Johan Herland]

On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Johan Herland <johan@herland.net> writes:
>>> Obviously, I named it '%1' since it expands into the _first_ component
>>> of the (slash-separated) shorthand.
>>
>> OK, I can buy something like
>>
>>       %*
>>         refs/%*
>>         refs/heads/%*
>>         ...
>>         refs/remotes/%*/HEAD
>>         refs/remotes/%1/%2
>>         refs/peers/%1/heads/%2
>>
>> that is, for a pattern that has %*, we feed the end-user string as a
>> whole, and for a pattern that has %1 thru %N, we find an appropriate
>> way to chop the end-user string into N pieces (e.g. nick/name would
>> be split into %1 = nick, %2 = name, while foo/bar/baz might have two
>> possibilities, <%1, %2> = <foo, bar/baz> or <foo/bar, baz>).  The
>> earlier ones on the above list can even be written with their %*
>> substituted with %1 if we go that route.
>
> Just to make sure.
>
> Please do not let "two possibilities" stop you.  As I said in the
> nearby thread, I do not necessarily insist that we must try all N
> possibilities.  "We find an appropriate way" could be just "we
> always chop at the first slash, and %1 is what comes before it, %2
> is what comes after it".
>
> That will close the possibility for us to use %1 thru %N (N is
> limited to 2), but it still is "We have %1 and we have %2, both fall
> into the same 'path components, numbered from left to right'
> category", and justifies the use of %1 ("one", not "el").
>
> So still no objection to %1 from me.

I think I like "refs/peers/%1/heads/%*" better than
"refs/peers/%1/heads/%2", since the latter sort of makes me wonder
whether the 3rd, 4th, etc. components would be discarded. That said,
having %* mean "the rest of the shorthand" means that we must adjust
the expansion of %* for every preceding %N, which prevents us from
simplifying the code by using strbuf_expand_dict_cb() with a static
dictionary [1].

I am not sure why we would want "refs/remotes/%1/%2" instead of
"refs/remote/%*". Maybe I've been staring at this for too long, but I
find the latter shorter and more descriptive and the "%1/%2" notation
needlessly cumbersome, especially if it's also supposed to match
"foo/bar/baz"

When it comes to multi-level remote names, I guess I could drop the
patch that disallows them, and still just have "%1" only map to the
first component (i.e. "foo/bar/baz" would always be interpreted as %1
= "foo", and never %1 = "foo/bar"). This would mean that the
"foo/bar/baz" shorthand notation would simply not work against
remote-tracking branch "baz" from remote "foo/bar", but we might say
that's ok, because (a) multi-level remote names are so rare, and (b)
the simple workaround of explicitly saying
"refs/peers/foo/bar/heads/baz" would always be available in any case.


...Johan


[1]: Maybe we could use '%N+' to mean "everything starting from
component #N"? Then we could construct the following dictionary the
shorthand "foo/bar/baz":

  "*" -> "foo/bar/baz"
  "1" -> "foo"
  "1+" -> "foo/bar/baz"
  "2" -> "bar"
  "2+" -> "bar/baz"
etc.

But I really think this is overkill. Avoiding having to write our own
expansion helper is not _that_ important.

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I think I like "refs/peers/%1/heads/%*" better than
> "refs/peers/%1/heads/%2", since the latter sort of makes me wonder
> whether the 3rd, 4th, etc. components would be discarded.

Makes sense.

> I am not sure why we would want "refs/remotes/%1/%2" instead of
> "refs/remote/%*".

The former way makes it easier to see what "refs/peers/%1/heads/%2"
means, I think, but otherwise aren't they equivalent?  I do not see
a strong reason to favor one over the other.

> remote-tracking branch "baz" from remote "foo/bar", but we might say
> that's ok, because (a) multi-level remote names are so rare, and (b)
> the simple workaround of explicitly saying
> "refs/peers/foo/bar/heads/baz" would always be available in any case.

Sounds sensible.

And if you limit things that way, "%1" again starts to make sense,
as you are representing "the first path component" with it.

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Michael Haggerty]

On 05/14/2013 04:24 PM, Johan Herland wrote:
> On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> Johan Herland <johan@herland.net> writes:
>>>> Obviously, I named it '%1' since it expands into the _first_ component
>>>> of the (slash-separated) shorthand.
>>>
>>> OK, I can buy something like
>>>
>>>       %*
>>>         refs/%*
>>>         refs/heads/%*
>>>         ...
>>>         refs/remotes/%*/HEAD
>>>         refs/remotes/%1/%2
>>>         refs/peers/%1/heads/%2
>>>
>>> that is, for a pattern that has %*, we feed the end-user string as a
>>> whole, and for a pattern that has %1 thru %N, we find an appropriate
>>> way to chop the end-user string into N pieces (e.g. nick/name would
>>> be split into %1 = nick, %2 = name, while foo/bar/baz might have two
>>> possibilities, <%1, %2> = <foo, bar/baz> or <foo/bar, baz>).  The
>>> earlier ones on the above list can even be written with their %*
>>> substituted with %1 if we go that route.
>>
>> Just to make sure.
>>
>> Please do not let "two possibilities" stop you.  As I said in the
>> nearby thread, I do not necessarily insist that we must try all N
>> possibilities.  "We find an appropriate way" could be just "we
>> always chop at the first slash, and %1 is what comes before it, %2
>> is what comes after it".
>>
>> That will close the possibility for us to use %1 thru %N (N is
>> limited to 2), but it still is "We have %1 and we have %2, both fall
>> into the same 'path components, numbered from left to right'
>> category", and justifies the use of %1 ("one", not "el").
>>
>> So still no objection to %1 from me.
> 
> I think I like "refs/peers/%1/heads/%*" better than
> "refs/peers/%1/heads/%2", since the latter sort of makes me wonder
> whether the 3rd, 4th, etc. components would be discarded. That said,
> having %* mean "the rest of the shorthand" means that we must adjust
> the expansion of %* for every preceding %N, which prevents us from
> simplifying the code by using strbuf_expand_dict_cb() with a static
> dictionary [1].
> 
> I am not sure why we would want "refs/remotes/%1/%2" instead of
> "refs/remote/%*". Maybe I've been staring at this for too long, but I
> find the latter shorter and more descriptive and the "%1/%2" notation
> needlessly cumbersome, especially if it's also supposed to match
> "foo/bar/baz"

"refs/remotes/%1/%2" (or "refs/remotes/%1/%*") might be a nice way to
imply that the rule should only be attempted if the input has at least
two components, whereas something like "refs/heads/%*" would be applied
even for inputs with no slashes.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Johan Herland]

On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/14/2013 04:24 PM, Johan Herland wrote:
>> I am not sure why we would want "refs/remotes/%1/%2" instead of
>> "refs/remote/%*". Maybe I've been staring at this for too long, but I
>> find the latter shorter and more descriptive and the "%1/%2" notation
>> needlessly cumbersome, especially if it's also supposed to match
>> "foo/bar/baz"
>
> "refs/remotes/%1/%2" (or "refs/remotes/%1/%*") might be a nice way to
> imply that the rule should only be attempted if the input has at least
> two components, whereas something like "refs/heads/%*" would be applied
> even for inputs with no slashes.

/me likes, at least for "refs/remotes/%1/%*".

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Johan Herland]

On Wed, May 15, 2013 at 9:39 AM, Johan Herland <johan@herland.net> wrote:
> On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> "refs/remotes/%1/%2" (or "refs/remotes/%1/%*") might be a nice way to
>> imply that the rule should only be attempted if the input has at least
>> two components, whereas something like "refs/heads/%*" would be applied
>> even for inputs with no slashes.
>
> /me likes, at least for "refs/remotes/%1/%*".

Unfortunately, using "refs/remotes/%1/%*" instead of "refs/remotes/%*"
breaks a number of git-svn tests which puts refs directly within
refs/remotes/, and then does things like "git reset --hard trunk"
(expecting trunk -> refs/remotes/trunk, which the refs/remotes/%1/%*
doesn't match).

I don't know if putting refs directly within refs/remotes/ is
something that git-svn does by default (which would prevent us from
changing "refs/remotes/%*" to "refs/remotes/%1/%*"), or whether it is
specific to the tests (in which case we should fix the tests).

Also, there might be too many other users of refs directly within
refs/remotes/ that expect "foo_without_slash" to expand to
"refs/remotes/foo_without_slash", which would prevent us from doing
this change.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Wed, May 15, 2013 at 9:39 AM, Johan Herland <johan@herland.net> wrote:
>> On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> "refs/remotes/%1/%2" (or "refs/remotes/%1/%*") might be a nice way to
>>> imply that the rule should only be attempted if the input has at least
>>> two components, whereas something like "refs/heads/%*" would be applied
>>> even for inputs with no slashes.
>>
>> /me likes, at least for "refs/remotes/%1/%*".
>
> Unfortunately, using "refs/remotes/%1/%*" instead of "refs/remotes/%*"
> breaks a number of git-svn tests which puts refs directly within
> refs/remotes/, and then does things like "git reset --hard trunk"
> (expecting trunk -> refs/remotes/trunk, which the refs/remotes/%1/%*
> doesn't match).

Oh, I never thought refs/remotes/%1/%* was a suggestion for a
serious "improvement", but was merely a thought experiment "if all
the remotes were at least two level names, we could express it like
this to stress that fact".

We already allow 'origin' to refer to refs/remotes/origin/HEAD, so
it is clear refs/remotes/%1/%* alone will not be able to replace
what we have; we need refs/remotes/%* and refs/remotes/%*/HEAD
anyway.

Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Eric Wong]

Johan Herland <johan@herland.net> wrote:
> Unfortunately, using "refs/remotes/%1/%*" instead of "refs/remotes/%*"
> breaks a number of git-svn tests which puts refs directly within
> refs/remotes/, and then does things like "git reset --hard trunk"
> (expecting trunk -> refs/remotes/trunk, which the refs/remotes/%1/%*
> doesn't match).
> 
> I don't know if putting refs directly within refs/remotes/ is
> something that git-svn does by default (which would prevent us from
> changing "refs/remotes/%*" to "refs/remotes/%1/%*"), or whether it is
> specific to the tests (in which case we should fix the tests).

Yes, git-svn uses refs/remotes/%* by default.

This was a design mistake on my part.  I think it's too late to change,
now, as existing users will encounter breakage and/or out-of-date
documentation (which is even more confusing, as git-svn is often the
first introduction to git for SVN users).

[PATCHv2 04/10] remote: Reject remote names containing '/'

[Johan Herland]

Although we definitely support and encourage use of multi-level branch
names, we have never conciously tried to give support for multi-level
remote names. Currently, they are allowed, but there is no evidence that
they are commonly used.

Now, they do provide a source of problems when trying to expand the
"$nick/$name" shorthand notation (where $nick matches a remote name)
into a full refname. Consider the shorthand "foo/bar/baz": Does this
parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?

Since we need to be unambiguous about these things, we hereby declare
that a remote name shall never contain a '/' character, and that the
only correct way to parse "foo/bar/baz" is $nick = foo, $name = bar/baz.

This patch teaches 'git remote' to reject remote names with slashes,
and adds tests verifying this.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/remote.c  |  4 ++--
 t/t5505-remote.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 5e54d36..6e7369d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -195,7 +195,7 @@ static int add(int argc, const char **argv)
 		die(_("remote %s already exists."), name);
 
 	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
-	if (!valid_fetch_refspec(buf2.buf))
+	if (!valid_fetch_refspec(buf2.buf) || strchr(name, '/'))
 		die(_("'%s' is not a valid remote name"), name);
 
 	strbuf_addf(&buf, "remote.%s.url", name);
@@ -646,7 +646,7 @@ static int mv(int argc, const char **argv)
 		die(_("remote %s already exists."), rename.new);
 
 	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
-	if (!valid_fetch_refspec(buf.buf))
+	if (!valid_fetch_refspec(buf.buf) || strchr(rename.new, '/'))
 		die(_("'%s' is not a valid remote name"), rename.new);
 
 	strbuf_reset(&buf);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dd10ff0..f7098fe 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -621,6 +621,12 @@ test_expect_success 'reject adding remote with an invalid name' '
 
 '
 
+test_expect_success 'reject adding remote with slash in name' '
+
+	test_must_fail git remote add multi/level .
+
+'
+
 # The first three test if the tracking branches are properly renamed,
 # the last two ones check if the config is updated.
 
@@ -668,6 +674,12 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 
 '
 
+test_expect_success 'rename a remote with slash in name' '
+
+	test_must_fail git remote rename upstream up/stream
+
+'
+
 cat > remotes_origin << EOF
 URL: $(pwd)/one
 Push: refs/heads/master:refs/heads/upstream
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Eric Sunshine]

On Sat, May 11, 2013 at 12:21 PM, Johan Herland <johan@herland.net> wrote:
> Although we definitely support and encourage use of multi-level branch
> names, we have never conciously tried to give support for multi-level

s/conciously/consciously/

> remote names. Currently, they are allowed, but there is no evidence that
> they are commonly used.
>
> Now, they do provide a source of problems when trying to expand the
> "$nick/$name" shorthand notation (where $nick matches a remote name)
> into a full refname. Consider the shorthand "foo/bar/baz": Does this
> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?
>
> Since we need to be unambiguous about these things, we hereby declare
> that a remote name shall never contain a '/' character, and that the
> only correct way to parse "foo/bar/baz" is $nick = foo, $name = bar/baz.
>
> This patch teaches 'git remote' to reject remote names with slashes,
> and adds tests verifying this.
>
> Signed-off-by: Johan Herland <johan@herland.net>

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Johan Herland]

On Mon, May 13, 2013 at 6:48 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, May 11, 2013 at 12:21 PM, Johan Herland <johan@herland.net> wrote:
>> Although we definitely support and encourage use of multi-level branch
>> names, we have never conciously tried to give support for multi-level
>
> s/conciously/consciously/

Thanks, will fix.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Although we definitely support and encourage use of multi-level branch
> names, we have never conciously tried to give support for multi-level
> remote names. Currently, they are allowed, but there is no evidence that
> they are commonly used.
>
> Now, they do provide a source of problems when trying to expand the
> "$nick/$name" shorthand notation (where $nick matches a remote name)
> into a full refname. Consider the shorthand "foo/bar/baz": Does this
> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?
>
> Since we need to be unambiguous about these things, we hereby declare
> that a remote name shall never contain a '/' character, and that the
> only correct way to parse "foo/bar/baz" is $nick = foo, $name = bar/baz.

I know I am guilty of hinting to go in this direction in the earlier
discussion, but I have to wonder how much more refactoring is needed
to see if there is only one unique possibility among many.

For a string with N slashes, you have only N possible ways to split
it into $nick and $name, and if you see a ref "bar/baz" copied from
remote "foo" but no ref "baz" copied from remote "foo/bar" (or you
may not even have a remote "foo/bar" in the first place), the user
is already very unambiguous. The declaration is merely being lazy.

I am not saying we must implement such a back-track to disambiguate
the user input better.  I am wondering how much more effort on top
of this series is needed if we want to get there (provided that the
disambiguation is a good thing to do in the first place).

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Johan Herland]

On Mon, May 13, 2013 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> Although we definitely support and encourage use of multi-level branch
>> names, we have never conciously tried to give support for multi-level
>> remote names. Currently, they are allowed, but there is no evidence that
>> they are commonly used.
>>
>> Now, they do provide a source of problems when trying to expand the
>> "$nick/$name" shorthand notation (where $nick matches a remote name)
>> into a full refname. Consider the shorthand "foo/bar/baz": Does this
>> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?
>>
>> Since we need to be unambiguous about these things, we hereby declare
>> that a remote name shall never contain a '/' character, and that the
>> only correct way to parse "foo/bar/baz" is $nick = foo, $name = bar/baz.
>
> I know I am guilty of hinting to go in this direction in the earlier
> discussion, but I have to wonder how much more refactoring is needed
> to see if there is only one unique possibility among many.
>
> For a string with N slashes, you have only N possible ways to split
> it into $nick and $name, and if you see a ref "bar/baz" copied from
> remote "foo" but no ref "baz" copied from remote "foo/bar" (or you
> may not even have a remote "foo/bar" in the first place), the user
> is already very unambiguous. The declaration is merely being lazy.
>
> I am not saying we must implement such a back-track to disambiguate
> the user input better.  I am wondering how much more effort on top
> of this series is needed if we want to get there (provided that the
> disambiguation is a good thing to do in the first place).

I feel the problem with multi-level remote names runs a little deeper
than merely disambiguation when resolving remote-tracking refs: If you
have two remotes "foo" and "foo/bar", and they have branches "bar/baz"
and "baz", respectively, then they will (in the default current
configuration) end up clobbering eachother due to the overlapping
remote-tracking branch (refs/remotes/foo/bar/baz). Although the remote
ref namespace coincidentally resolves this by mapping the two to
"refs/peers/foo/heads/bar/baz" and "refs/peers/foo/bar/heads/baz"
respectively, you can easily create a different (although probably
even more unlikely) case where the remote ref namespace causes the
same kind of overlap: One remote "foo" with branch "heads/bar", and
another remote "foo/heads" with branch "bar" will both end up
clobbering eachother at "refs/peers/foo/heads/heads/bar"...

The disambiguation can probably be resolved, although the resulting
code will obviously be somewhat more cumbersome and ugly (and IMHO the
current code is plenty of that already...). Combine this with the
problems of clobbering of the same remote-tracking ref (describe
above), and the fact that AFAIK a multi-level remote name has never
been observed "in the wild" (none of the people I asked at the Git
Merge conference had ever observed multi-level remote names, nor did
they directly oppose banning them), I'm not at all sure it's worth
bothering about this at all. Simply disallowing multi-level remote
names seems like the simpler and naturally ambiguity-resistant
approach.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Ramkumar Ramachandra]

Johan Herland wrote:
> The disambiguation can probably be resolved, although the resulting
> code will obviously be somewhat more cumbersome and ugly (and IMHO the
> current code is plenty of that already...). Combine this with the
> problems of clobbering of the same remote-tracking ref (describe
> above), and the fact that AFAIK a multi-level remote name has never
> been observed "in the wild" (none of the people I asked at the Git
> Merge conference had ever observed multi-level remote names, nor did
> they directly oppose banning them), I'm not at all sure it's worth
> bothering about this at all. Simply disallowing multi-level remote
> names seems like the simpler and naturally ambiguity-resistant
> approach.

The problem with multi-level remote names is that we use the same
delimiter as in the ref namespace: '/'.  So, obviously, there's a lot
of room for confusion.  I wonder if we should banish multi-level
remotes altogether though: is it possible that they will be useful to
someone in the future?

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Johan Herland]

On Thu, May 16, 2013 at 11:48 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Johan Herland wrote:
>> The disambiguation can probably be resolved, although the resulting
>> code will obviously be somewhat more cumbersome and ugly (and IMHO the
>> current code is plenty of that already...). Combine this with the
>> problems of clobbering of the same remote-tracking ref (describe
>> above), and the fact that AFAIK a multi-level remote name has never
>> been observed "in the wild" (none of the people I asked at the Git
>> Merge conference had ever observed multi-level remote names, nor did
>> they directly oppose banning them), I'm not at all sure it's worth
>> bothering about this at all. Simply disallowing multi-level remote
>> names seems like the simpler and naturally ambiguity-resistant
>> approach.
>
> The problem with multi-level remote names is that we use the same
> delimiter as in the ref namespace: '/'.  So, obviously, there's a lot
> of room for confusion.  I wonder if we should banish multi-level
> remotes altogether though: is it possible that they will be useful to
> someone in the future?

Technically, the problem is that we don't use a different delimiter
between the $remote and $ref parts. If we had used
"multi/level/remote:nested/ref" we would have been OK (on this issue
at least, probably not OK on other issues).

FWIW, I've abandoned this patch, and don't care much about multi-level
remote names anymore. They work in current git, and they will sort-of
work with remote ref namespaces as well, although you will have to use
full refnames when referring to their remote-tracking refs. If
multi-level remote names suddenly become popular, we can change the
code to try to resolve them unambiguously.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Ramkumar Ramachandra]

Johan Herland wrote:
> FWIW, I've abandoned this patch, and don't care much about multi-level
> remote names anymore. They work in current git, and they will sort-of
> work with remote ref namespaces as well, although you will have to use
> full refnames when referring to their remote-tracking refs. If
> multi-level remote names suddenly become popular, we can change the
> code to try to resolve them unambiguously.

Makes sense.  We can deal with the issue if and when multi-level
remotes become popular.

[PATCHv2 05/10] refs.c: Add support for expanding/shortening refs in refs/peers/*

[Johan Herland]

This patch adds the following patterns for expanding/shortening refs:

  "refs/peers/%*"
  "refs/peers/%1/tags/%*"
  "refs/peers/%1/heads/%*"

These allow shorthand names like "origin/master" to expand to refs in
the refs/peers/* hierarchy (in this case, the likely expansion would be
by the middle rule above, resulting in "refs/peers/origin/heads/master").

To accomplish this, we have added the new "%1" wildcard which shall
expand into the first component (i.e. up to the first '/') of the given
shorthand. The other wildcard ("%*") shall then expand into the remainder
of the shorthand (i.e. following the first '/').

Correspondingly, when shortening according to a pattern with "%1", a
single component (not including any '/' character) shall be extracted
from that point in the given refname, and shall be added to the resulting
shorthand, with a trailing '/'. Then, when hitting the "%*", the
remainder of the given refname (modulo a trailing match in the pattern)
shall be extracted and appended to the portion previously extracted by
the "%1" wildcard.

The need to split the "$remote/$ref" into its $remote and $ref parts is
the reason why multi-level remote names will no longer work (and hence
were disallowed in the previous patch). A testcase demonstrating how
multi-level remote names fail is therefore included in this patch.

Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c                                         | 101 +++++++++++++++++++++----
 t/t7900-working-with-namespaced-remote-refs.sh |   4 +-
 t/t7901-multi-level-remote-name-failure.sh     |  20 +++++
 3 files changed, 107 insertions(+), 18 deletions(-)
 create mode 100755 t/t7901-multi-level-remote-name-failure.sh

diff --git a/refs.c b/refs.c
index ab5e120..188a9eb 100644
--- a/refs.c
+++ b/refs.c
@@ -1789,7 +1789,10 @@ static const char *refname_patterns[] = {
 	"refs/tags/%*",
 	"refs/heads/%*",
 	"refs/remotes/%*",
-	"refs/remotes/%*/HEAD"
+	"refs/remotes/%*/HEAD",
+	"refs/peers/%*",
+	"refs/peers/%1/tags/%*",
+	"refs/peers/%1/heads/%*"
 };
 
 struct wildcard_data {
@@ -1806,6 +1809,15 @@ static size_t refname_expand_helper(struct strbuf *sb, const char *placeholder,
 		strbuf_add(sb, cb->s, cb->len);
 		cb->done = 1;
 		return 1;
+	} else if (*placeholder == '1' && !cb->done) {
+		const char *p = memchr(cb->s, '/', cb->len);
+		size_t copy_len = p ? p - cb->s : cb->len;
+		strbuf_add(sb, cb->s, copy_len);
+		if (copy_len < cb->len && cb->s[copy_len] == '/')
+			copy_len++;
+		cb->s += copy_len;
+		cb->len -= copy_len;
+		return 1;
 	}
 	return 0;
 }
@@ -1821,6 +1833,46 @@ static int refname_expand(struct strbuf *dst, const char *pattern,
 	return !cbdata.done;
 }
 
+static int handle_fragment(struct strbuf *dst, struct strbuf *fragment,
+			   int first, int last,
+			   const char *refname, size_t refname_len)
+{
+	const char *ref = refname, *p;
+	size_t ref_len = refname_len, trail_len;
+	if (!first) {
+		/* extract wildcard according to wildcard char */
+		switch (fragment->buf[0]) {
+		case '1':
+			/* extract up to next '/' from refname */
+			p = memchr(ref, '/', ref_len);
+			if (!p)
+				return -1;
+			strbuf_add(dst, ref, p - ref);
+			strbuf_addch(dst, '/');
+			ref_len -= p - ref;
+			ref += p - ref;
+			break;
+		case '*':
+			/* extract all up to matching trailer */
+			assert(last);
+			trail_len = fragment->len - 1;
+			if (trail_len > ref_len)
+				return -1;
+			strbuf_add(dst, ref, ref_len - trail_len);
+			ref += ref_len - trail_len;
+			ref_len -= ref_len - trail_len;
+			break;
+		}
+	}
+
+	/* match rest of fragment verbatim */
+	p = fragment->buf + (first ? 0 : 1); /* skip wildcard character */
+	trail_len = fragment->len - ((first ? 0 : 1) + (last ? 0 : 1));
+	if (trail_len > ref_len || memcmp(ref, p, trail_len))
+		return -1;
+	return (ref - refname) + trail_len;
+}
+
 static int refname_shorten(struct strbuf *dst, const char *pattern,
 			   const char *refname, size_t refname_len)
 {
@@ -1830,26 +1882,43 @@ static int refname_shorten(struct strbuf *dst, const char *pattern,
 	 * Return 0 on success (positive match, wildcard-matching portion
 	 * copied into dst), and non-zero on failure (no match, dst empty).
 	 */
-	const char *match_end;
-	struct strbuf **fragments = strbuf_split_str(pattern, '%', 2);
-	struct strbuf *prefix = fragments[0], *suffix = fragments[1];
-	assert(!fragments[2]);
-	assert(prefix->len && prefix->buf[prefix->len - 1] == '%');
-	assert(suffix->len && suffix->buf[0] == '*');
+	struct strbuf **fragments = strbuf_split_str(pattern, '%', 0);
+	struct strbuf **it;
+	int first = 1, last, ret = 1;
 
 	strbuf_reset(dst);
-	match_end = refname + refname_len - (suffix->len - 1);
-	if (refname_len <= (prefix->len - 1) + (suffix->len - 1) ||
-	    memcmp(prefix->buf, refname, prefix->len - 1) ||
-	    memcmp(suffix->buf + 1, match_end, suffix->len - 1)) {
-		strbuf_list_free(fragments);
-		return 1; /* refname does not match pattern */
+	for (it = fragments; *it; it++) {
+		int consumed;
+		struct strbuf *cur = *it;
+		last = *(it + 1) == NULL;
+
+		/* all but last ends with '%' */
+		assert(last || cur->buf[cur->len - 1] == '%');
+		/* all but first starts with '*' or '1' */
+		assert(first || cur->buf[0] == '*' || cur->buf[0] == '1');
+		/* only last starts with '*' */
+		assert((cur->buf[0] == '*' && last) ||
+		       (cur->buf[0] != '*' && !last));
+
+		consumed = handle_fragment(dst, cur, first, last,
+					   refname, refname_len);
+		if (consumed < 0)
+			goto cleanup;
+		else {
+			refname += consumed;
+			refname_len -= consumed;
+		}
+
+		first = 0;
 	}
+	if (refname_len == 0)
+		ret = 0;
 
-	refname += prefix->len - 1;
-	strbuf_add(dst, refname, match_end - refname);
+cleanup:
+	if (ret)
+		strbuf_reset(dst);
 	strbuf_list_free(fragments);
-	return 0;
+	return ret;
 }
 
 int refname_match(const char *abbrev_name, const char *full_name)
diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
index 109e9b8..33266e0 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -83,7 +83,7 @@ test_expect_success 'enter client repo' '
 	cd client
 '
 
-test_expect_failure 'short-hand notation expands correctly for remote-tracking branches' '
+test_expect_success 'short-hand notation expands correctly for remote-tracking branches' '
 	echo refs/peers/origin/heads/master >expect &&
 	git rev-parse --symbolic-full-name refs/peers/origin/heads/master >actual &&
 	test_cmp expect actual &&
@@ -95,7 +95,7 @@ test_expect_failure 'short-hand notation expands correctly for remote-tracking b
 	test_cmp expect actual
 '
 
-test_expect_failure 'remote-tracking branches are shortened correctly' '
+test_expect_success 'remote-tracking branches are shortened correctly' '
 	echo origin/master >expect &&
 	git rev-parse --abbrev-ref refs/peers/origin/heads/master >actual &&
 	test_cmp expect actual &&
diff --git a/t/t7901-multi-level-remote-name-failure.sh b/t/t7901-multi-level-remote-name-failure.sh
new file mode 100755
index 0000000..8d2a617
--- /dev/null
+++ b/t/t7901-multi-level-remote-name-failure.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='Show why multi-level remote names can no longer be used'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit a &&
+	git config remote.multi/level.url . &&
+	git config remote.multi/level.fetch "+refs/heads/*:refs/peers/multi/level/heads/*" &&
+	git fetch multi/level
+'
+
+test_expect_failure 'Fail to use shorthand notation: "$remote/$branch"' '
+	git rev-parse --verify a >expect &&
+	git rev-parse --verify multi/level/master >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 06/10] t7900: Test git branch -r/-a output w/remote-tracking branches in refs/peers/*

[Johan Herland]

These tests will be made to pass by subsequent patches.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t7900-working-with-namespaced-remote-refs.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
index 33266e0..279664c 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -107,4 +107,25 @@ test_expect_success 'remote-tracking branches are shortened correctly' '
 	test_cmp expect actual
 '
 
+cat >expect.branch-r << EOF
+  origin/master
+  origin/other
+EOF
+
+test_expect_failure 'git branch -r should show remote-tracking branches' '
+	git branch -r >actual.branch-r &&
+	test_cmp expect.branch-r actual.branch-r
+'
+
+cat >expect.branch-a << EOF
+* master
+  peers/origin/heads/master
+  peers/origin/heads/other
+EOF
+
+test_expect_failure 'git branch -a should also show remote-tracking branches' '
+	git branch -a >actual.branch-a &&
+	test_cmp expect.branch-a actual.branch-a
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 07/10] t3203: Add testcase for fix in 1603ade81352a526ccb206f41ff81ecbc855df2d

[Johan Herland]

This adds a testcase for some behavior that I very nearly broke while
refactoring some stuff in builtin/branch.c.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3203-branch-output.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index ba4f98e..c312d3a 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -106,4 +106,19 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch -a --merged handles broken refs gracefully' '
+	cat >expect <<EOF &&
+  branch-one
+  branch-two
+* master
+  remotes/origin/HEAD -> origin/branch-one
+  remotes/origin/branch-one
+  remotes/origin/branch-two
+EOF
+	git checkout master &&
+	echo "broken ref" > .git/refs/heads/broken &&
+	test_must_fail git branch -a --merged >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 08/10] builtin/branch.c: Refactor ref_item.name and .dest into strbufs

[Johan Herland]

In preparation of a future patch which reorganizes how the display of the
ref_list is done.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/branch.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0836890..c8b49e3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -276,8 +276,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 }
 
 struct ref_item {
-	char *name;
-	char *dest;
+	struct strbuf name;
+	struct strbuf dest;
 	unsigned int kind, width;
 	struct commit *commit;
 };
@@ -380,11 +380,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 
 	/* Record the new item */
 	newitem = &(ref_list->list[ref_list->index++]);
-	newitem->name = xstrdup(refname);
+	strbuf_init(&newitem->name, 0);
+	strbuf_addstr(&newitem->name, refname);
 	newitem->kind = kind;
 	newitem->commit = commit;
 	newitem->width = utf8_strwidth(refname);
-	newitem->dest = resolve_symref(orig_refname, prefix);
+	strbuf_init(&newitem->dest, 0);
+	orig_refname = resolve_symref(orig_refname, prefix);
+	if (orig_refname)
+		strbuf_addstr(&newitem->dest, orig_refname);
 	/* adjust for "remotes/" */
 	if (newitem->kind == REF_REMOTE_BRANCH &&
 	    ref_list->kinds != REF_REMOTE_BRANCH)
@@ -400,8 +404,8 @@ static void free_ref_list(struct ref_list *ref_list)
 	int i;
 
 	for (i = 0; i < ref_list->index; i++) {
-		free(ref_list->list[i].name);
-		free(ref_list->list[i].dest);
+		strbuf_release(&ref_list->list[i].name);
+		strbuf_release(&ref_list->list[i].dest);
 	}
 	free(ref_list->list);
 }
@@ -413,7 +417,7 @@ static int ref_cmp(const void *r1, const void *r2)
 
 	if (c1->kind != c2->kind)
 		return c1->kind - c2->kind;
-	return strcmp(c1->name, c2->name);
+	return strbuf_cmp(&c1->name, &c2->name);
 }
 
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
@@ -496,7 +500,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 	}
 
 	if (item->kind == REF_LOCAL_BRANCH)
-		fill_tracking_info(&stat, item->name, verbose > 1);
+		fill_tracking_info(&stat, item->name.buf, verbose > 1);
 
 	strbuf_addf(out, " %s %s%s",
 		find_unique_abbrev(item->commit->object.sha1, abbrev),
@@ -534,7 +538,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		color = BRANCH_COLOR_CURRENT;
 	}
 
-	strbuf_addf(&name, "%s%s", prefix, item->name);
+	strbuf_addf(&name, "%s%s", prefix, item->name.buf);
 	if (verbose) {
 		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
 		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
@@ -544,8 +548,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
 			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
-	if (item->dest)
-		strbuf_addf(&out, " -> %s", item->dest);
+	if (item->dest.len)
+		strbuf_addf(&out, " -> %s", item->dest.buf);
 	else if (verbose)
 		/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
 		add_verbose_info(&out, item, verbose, abbrev);
@@ -601,15 +605,16 @@ static void show_detached(struct ref_list *ref_list)
 
 	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
 		struct ref_item item;
-		item.name = get_head_description();
-		item.width = utf8_strwidth(item.name);
+		strbuf_init(&item.name, 0);
+		strbuf_addstr(&item.name, get_head_description());
+		item.width = utf8_strwidth(item.name.buf);
 		item.kind = REF_LOCAL_BRANCH;
-		item.dest = NULL;
+		strbuf_init(&item.dest, 0);
 		item.commit = head_commit;
 		if (item.width > ref_list->maxwidth)
 			ref_list->maxwidth = item.width;
 		print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
-		free(item.name);
+		strbuf_release(&item.name);
 	}
 }
 
@@ -655,7 +660,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	for (i = 0; i < ref_list.index; i++) {
 		int current = !detached &&
 			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
-			!strcmp(ref_list.list[i].name, head);
+			!strcmp(ref_list.list[i].name.buf, head);
 		char *prefix = (kinds != REF_REMOTE_BRANCH &&
 				ref_list.list[i].kind == REF_REMOTE_BRANCH)
 				? "remotes/" : "";
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 09/10] builtin/branch.c: Refactor "remotes/" prepending to remote-tracking branches

[Johan Herland]

When running "git branch -a" (instead of "git branch -r"), we prefix the
remote-tracking branches with "remotes/" to allow the user to more easily
tell them apart from the local branches.

The code that prepended "remotes/" to remote-tracking branches was
located in print_ref_item(), while the code that adjusted the
ref_item.width (to account for the prepended "remotes/") was located in
append_ref().

This code moves the prepending of "remotes/" up into append_ref(), which
is a nice cleanup of the code, as well as a preparation for changing the
logic when remote-tracking branches start showing up in refs/peers/*.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/branch.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index c8b49e3..4480be2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -384,7 +384,6 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	strbuf_addstr(&newitem->name, refname);
 	newitem->kind = kind;
 	newitem->commit = commit;
-	newitem->width = utf8_strwidth(refname);
 	strbuf_init(&newitem->dest, 0);
 	orig_refname = resolve_symref(orig_refname, prefix);
 	if (orig_refname)
@@ -392,7 +391,8 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	/* adjust for "remotes/" */
 	if (newitem->kind == REF_REMOTE_BRANCH &&
 	    ref_list->kinds != REF_REMOTE_BRANCH)
-		newitem->width += 8;
+		strbuf_insert(&newitem->name, 0, "remotes/", 8);
+	newitem->width = utf8_strwidth(newitem->name.buf);
 	if (newitem->width > ref_list->maxwidth)
 		ref_list->maxwidth = newitem->width;
 
@@ -510,12 +510,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 }
 
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int current, char *prefix)
+			   int abbrev, int current)
 {
 	char c;
 	int color;
 	struct commit *commit = item->commit;
-	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
 
 	if (!matches_merge_filter(commit))
 		return;
@@ -538,15 +538,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		color = BRANCH_COLOR_CURRENT;
 	}
 
-	strbuf_addf(&name, "%s%s", prefix, item->name.buf);
 	if (verbose) {
-		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
+		int utf8_compensation = strlen(item->name.buf) - utf8_strwidth(item->name.buf);
 		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
-			    maxwidth + utf8_compensation, name.buf,
+			    maxwidth + utf8_compensation, item->name.buf,
 			    branch_get_color(BRANCH_COLOR_RESET));
 	} else
 		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
-			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
+			    item->name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
 	if (item->dest.len)
 		strbuf_addf(&out, " -> %s", item->dest.buf);
@@ -559,7 +558,6 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	} else {
 		printf("%s\n", out.buf);
 	}
-	strbuf_release(&name);
 	strbuf_release(&out);
 }
 
@@ -613,7 +611,7 @@ static void show_detached(struct ref_list *ref_list)
 		item.commit = head_commit;
 		if (item.width > ref_list->maxwidth)
 			ref_list->maxwidth = item.width;
-		print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
+		print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1);
 		strbuf_release(&item.name);
 	}
 }
@@ -661,11 +659,8 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		int current = !detached &&
 			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
 			!strcmp(ref_list.list[i].name.buf, head);
-		char *prefix = (kinds != REF_REMOTE_BRANCH &&
-				ref_list.list[i].kind == REF_REMOTE_BRANCH)
-				? "remotes/" : "";
 		print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
-			       abbrev, current, prefix);
+			       abbrev, current);
 	}
 
 	free_ref_list(&ref_list);
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*

[Johan Herland]

The current branch display code assumes all remote-tracking branches live
inside refs/remotes/*, and that their appropriate shorthand names can be
retrieved by simply stripping off the "refs/remotes/" prefix.

When we add remote-tracking branches in refs/peers/$remote/heads/*, the
code must not only be taught how to list refs from the new location, but
also how to appropriately derive shorthand names for the remote-tracking
branches there (instead of stripping off a hardcoded prefix, we must use
the refname_shorten() function introduced in an earlier patch.)

There is already a good set of tests for the expected output from git
branch, and this patch does not break any of them. However, we do fix
the two tests with remote-tracking branches in refs/peers/* introduced
in a previous patch.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/branch.c                               | 66 ++++++++++++++------------
 refs.c                                         |  4 +-
 refs.h                                         |  3 ++
 t/t7900-working-with-namespaced-remote-refs.sh |  4 +-
 4 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4480be2..9a6bce8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -290,18 +290,16 @@ struct ref_list {
 	int kinds;
 };
 
-static char *resolve_symref(const char *src, const char *prefix)
+static void resolve_symref(struct strbuf *shortname, const char *pattern, const char *src)
 {
 	unsigned char sha1[20];
 	int flag;
-	const char *dst, *cp;
+	const char *dst;
 
 	dst = resolve_ref_unsafe(src, sha1, 0, &flag);
-	if (!(dst && (flag & REF_ISSYMREF)))
-		return NULL;
-	if (prefix && (cp = skip_prefix(dst, prefix)))
-		dst = cp;
-	return xstrdup(dst);
+	if (dst && (flag & REF_ISSYMREF) &&
+	    refname_shorten(shortname, pattern, dst, strlen(dst)))
+		strbuf_addstr(shortname, dst);
 }
 
 struct append_ref_cb {
@@ -328,74 +326,80 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	struct ref_list *ref_list = cb->ref_list;
 	struct ref_item *newitem;
 	struct commit *commit;
+	struct strbuf ref = STRBUF_INIT;
 	int kind, i;
-	const char *prefix, *orig_refname = refname;
+	const char *pattern;
+	size_t refname_len = strlen(refname);
 
 	static struct {
 		int kind;
-		const char *prefix;
-		int pfxlen;
+		const char *pattern;
 	} ref_kind[] = {
-		{ REF_LOCAL_BRANCH, "refs/heads/", 11 },
-		{ REF_REMOTE_BRANCH, "refs/remotes/", 13 },
+		{ REF_LOCAL_BRANCH, "refs/heads/%*" },
+		{ REF_REMOTE_BRANCH, "refs/remotes/%*" },
+		{ REF_REMOTE_BRANCH, "refs/peers/%1/heads/%*" },
 	};
 
 	/* Detect kind */
 	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
-		prefix = ref_kind[i].prefix;
-		if (strncmp(refname, prefix, ref_kind[i].pfxlen))
+		if (refname_shorten(&ref, ref_kind[i].pattern, refname, refname_len))
 			continue;
 		kind = ref_kind[i].kind;
-		refname += ref_kind[i].pfxlen;
+		pattern = ref_kind[i].pattern;
 		break;
 	}
 	if (ARRAY_SIZE(ref_kind) <= i)
-		return 0;
+		goto out;
 
 	/* Don't add types the caller doesn't want */
 	if ((kind & ref_list->kinds) == 0)
-		return 0;
+		goto out;
 
-	if (!match_patterns(cb->pattern, refname))
-		return 0;
+	if (!match_patterns(cb->pattern, ref.buf))
+		goto out;
 
 	commit = NULL;
 	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
 		commit = lookup_commit_reference_gently(sha1, 1);
 		if (!commit) {
-			cb->ret = error(_("branch '%s' does not point at a commit"), refname);
-			return 0;
+			cb->ret = error(_("branch '%s' does not point at a commit"), ref.buf);
+			goto out;
 		}
 
 		/* Filter with with_commit if specified */
 		if (!is_descendant_of(commit, ref_list->with_commit))
-			return 0;
+			goto out;
 
 		if (merge_filter != NO_FILTER)
 			add_pending_object(&ref_list->revs,
-					   (struct object *)commit, refname);
+					   (struct object *)commit, ref.buf);
 	}
 
+	/*
+	 * When displaying more then just remote-tracking branches, make the
+	 * remote-tracking branches more explicit, e.g. instead of printing
+	 * "origin/master", we should print "remote/origin/master" (or
+	 * "peers/origin/heads/master").
+	 */
+	if (kind == REF_REMOTE_BRANCH && ref_list->kinds != REF_REMOTE_BRANCH)
+		refname_shorten(&ref, "refs/%*", refname, refname_len);
+
 	ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
 
 	/* Record the new item */
 	newitem = &(ref_list->list[ref_list->index++]);
 	strbuf_init(&newitem->name, 0);
-	strbuf_addstr(&newitem->name, refname);
+	strbuf_addbuf(&newitem->name, &ref);
 	newitem->kind = kind;
 	newitem->commit = commit;
 	strbuf_init(&newitem->dest, 0);
-	orig_refname = resolve_symref(orig_refname, prefix);
-	if (orig_refname)
-		strbuf_addstr(&newitem->dest, orig_refname);
-	/* adjust for "remotes/" */
-	if (newitem->kind == REF_REMOTE_BRANCH &&
-	    ref_list->kinds != REF_REMOTE_BRANCH)
-		strbuf_insert(&newitem->name, 0, "remotes/", 8);
+	resolve_symref(&newitem->dest, pattern, refname);
 	newitem->width = utf8_strwidth(newitem->name.buf);
 	if (newitem->width > ref_list->maxwidth)
 		ref_list->maxwidth = newitem->width;
 
+out:
+	strbuf_release(&ref);
 	return 0;
 }
 
diff --git a/refs.c b/refs.c
index 188a9eb..a78199a 100644
--- a/refs.c
+++ b/refs.c
@@ -1873,8 +1873,8 @@ static int handle_fragment(struct strbuf *dst, struct strbuf *fragment,
 	return (ref - refname) + trail_len;
 }
 
-static int refname_shorten(struct strbuf *dst, const char *pattern,
-			   const char *refname, size_t refname_len)
+int refname_shorten(struct strbuf *dst, const char *pattern,
+		    const char *refname, size_t refname_len)
 {
 	/*
 	 * Match refname against pattern, using "%*" as wildcard, and
diff --git a/refs.h b/refs.h
index e05c1f1..245d53d 100644
--- a/refs.h
+++ b/refs.h
@@ -165,6 +165,9 @@ extern int check_refname_format(const char *refname, int flags);
 
 extern const char *prettify_refname(const char *refname);
 
+extern int refname_shorten(struct strbuf *dst, const char *pattern,
+			   const char *refname, size_t refname_len);
+
 extern int refname_match(const char *abbrev_name, const char *full_name);
 
 extern char *shorten_unambiguous_ref(const char *refname, int strict);
diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
index 279664c..450b193 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -112,7 +112,7 @@ cat >expect.branch-r << EOF
   origin/other
 EOF
 
-test_expect_failure 'git branch -r should show remote-tracking branches' '
+test_expect_success 'git branch -r should show remote-tracking branches' '
 	git branch -r >actual.branch-r &&
 	test_cmp expect.branch-r actual.branch-r
 '
@@ -123,7 +123,7 @@ cat >expect.branch-a << EOF
   peers/origin/heads/other
 EOF
 
-test_expect_failure 'git branch -a should also show remote-tracking branches' '
+test_expect_success 'git branch -a should also show remote-tracking branches' '
 	git branch -a >actual.branch-a &&
 	test_cmp expect.branch-a actual.branch-a
 '
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*

[Eric Sunshine]

On Sat, May 11, 2013 at 12:21 PM, Johan Herland <johan@herland.net> wrote:
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4480be2..9a6bce8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -328,74 +326,80 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
>         struct ref_list *ref_list = cb->ref_list;
>         struct ref_item *newitem;
>         struct commit *commit;
> +       struct strbuf ref = STRBUF_INIT;
>         int kind, i;
> -       const char *prefix, *orig_refname = refname;
> +       const char *pattern;
> +       size_t refname_len = strlen(refname);
>
>         static struct {
>                 int kind;
> -               const char *prefix;
> -               int pfxlen;
> +               const char *pattern;
>         } ref_kind[] = {
> -               { REF_LOCAL_BRANCH, "refs/heads/", 11 },
> -               { REF_REMOTE_BRANCH, "refs/remotes/", 13 },
> +               { REF_LOCAL_BRANCH, "refs/heads/%*" },
> +               { REF_REMOTE_BRANCH, "refs/remotes/%*" },
> +               { REF_REMOTE_BRANCH, "refs/peers/%1/heads/%*" },
>         };
>
>         /* Detect kind */
>         for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
> -               prefix = ref_kind[i].prefix;
> -               if (strncmp(refname, prefix, ref_kind[i].pfxlen))
> +               if (refname_shorten(&ref, ref_kind[i].pattern, refname, refname_len))
>                         continue;
>                 kind = ref_kind[i].kind;
> -               refname += ref_kind[i].pfxlen;
> +               pattern = ref_kind[i].pattern;
>                 break;
>         }
>         if (ARRAY_SIZE(ref_kind) <= i)
> -               return 0;
> +               goto out;
>
>         /* Don't add types the caller doesn't want */
>         if ((kind & ref_list->kinds) == 0)
> -               return 0;
> +               goto out;
>
> -       if (!match_patterns(cb->pattern, refname))
> -               return 0;
> +       if (!match_patterns(cb->pattern, ref.buf))
> +               goto out;
>
>         commit = NULL;
>         if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
>                 commit = lookup_commit_reference_gently(sha1, 1);
>                 if (!commit) {
> -                       cb->ret = error(_("branch '%s' does not point at a commit"), refname);
> -                       return 0;
> +                       cb->ret = error(_("branch '%s' does not point at a commit"), ref.buf);
> +                       goto out;
>                 }
>
>                 /* Filter with with_commit if specified */
>                 if (!is_descendant_of(commit, ref_list->with_commit))
> -                       return 0;
> +                       goto out;
>
>                 if (merge_filter != NO_FILTER)
>                         add_pending_object(&ref_list->revs,
> -                                          (struct object *)commit, refname);
> +                                          (struct object *)commit, ref.buf);
>         }
>
> +       /*
> +        * When displaying more then just remote-tracking branches, make the

s/then/than/

> +        * remote-tracking branches more explicit, e.g. instead of printing
> +        * "origin/master", we should print "remote/origin/master" (or
> +        * "peers/origin/heads/master").
> +        */
> +       if (kind == REF_REMOTE_BRANCH && ref_list->kinds != REF_REMOTE_BRANCH)
> +               refname_shorten(&ref, "refs/%*", refname, refname_len);
> +
>         ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
>
>         /* Record the new item */
>         newitem = &(ref_list->list[ref_list->index++]);
>         strbuf_init(&newitem->name, 0);
> -       strbuf_addstr(&newitem->name, refname);
> +       strbuf_addbuf(&newitem->name, &ref);
>         newitem->kind = kind;
>         newitem->commit = commit;
>         strbuf_init(&newitem->dest, 0);
> -       orig_refname = resolve_symref(orig_refname, prefix);
> -       if (orig_refname)
> -               strbuf_addstr(&newitem->dest, orig_refname);
> -       /* adjust for "remotes/" */
> -       if (newitem->kind == REF_REMOTE_BRANCH &&
> -           ref_list->kinds != REF_REMOTE_BRANCH)
> -               strbuf_insert(&newitem->name, 0, "remotes/", 8);
> +       resolve_symref(&newitem->dest, pattern, refname);
>         newitem->width = utf8_strwidth(newitem->name.buf);
>         if (newitem->width > ref_list->maxwidth)
>                 ref_list->maxwidth = newitem->width;
>
> +out:
> +       strbuf_release(&ref);
>         return 0;
>  }
>
> diff --git a/refs.c b/refs.c
> index 188a9eb..a78199a 100644
> --- a/refs.c
> +++ b/refs.c

Re: [PATCHv2 10/10] branch: Fix display of remote branches in refs/peers/*

[Johan Herland]

On Mon, May 13, 2013 at 7:19 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, May 11, 2013 at 12:21 PM, Johan Herland <johan@herland.net> wrote:
>> +        * When displaying more then just remote-tracking branches, make the
> s/then/than/

Thanks, will fix.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net