disallow refs containing successive slashes

When creating branches using names starting with '/' or containing a '//', a leading slash would just vanish and successive slashes were 'compressed' into just one slash.

This behaviour comes from the refs being stored as files in subdirectories where the filesystem compresses each '//' in a pathname to a '/' . So disallowing refs to contain '//' fixes these problems and prohibits branch names to start with a '/' or to contain '//'.

---

I became aware of this issue while looking into problems occuring when a user created a branch starting with a '/' in git gui (e.g. "/foo"). Strange things happen, while git gui shows the current branch as "/foo" under the hood a branch "foo" (without the slash) had been created. But then you can't delete "/foo" from git gui, because a branch of that name doesn't exist.

IMHO branch names should not be allowed to start with a '/' or contain a '//' (branches with trailing slashes are not allowed since commit 03feddd6). So this implies that refs should never contain a '//' in the first place.

This behaviour has been changed the last time by commit 03feddd. The version before that commit didn't allow '//' to be part of a ref (but still allowed a trailing '/'). The newer version explicitly states "/* tolerate duplicated slashes */" in the added code. But i couldn't find any hints on the list or on the interweb *why* these were allowed. So this patch might break something i can't see right now, even though the test suite runs fine ...

Thoughts? Objections?

 Documentation/git-check-ref-format.txt |    2 ++
 refs.c                                 |    7 +++++--
 t/t1600-check-ref-format.sh            |   19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100755 t/t1600-check-ref-format.sh

index 0b7982e..180ffbb 100644
@@ -38,6 +38,8 @@ imposes the following rules on how references are named:
 
 . They cannot end with a slash `/` nor a dot `.`.
 
+. They cannot contain two successive slashes `//`.
+
 . They cannot end with the sequence `.lock`.
 
 . They cannot contain a sequence `@{`.

index 808f56b..cedd768 100644
@@ -687,6 +687,7 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
  * - it has double dots "..", or
  * - it has ASCII control character, "~", "^", ":" or SP, anywhere, or
  * - it ends with a "/".
+ * - it contains '//'
  * - it ends with ".lock"
  * - it contains a "\" (backslash)
  */

@@ -712,8 +713,10 @@ int check_ref_format(const char *ref)
 
 	level = 0;
 	while (1) {
-		while ((ch = *cp++) == '/')
-			; /* tolerate duplicated slashes */
+		if ((ch = *cp++) == '/')
+			/* Don't tolerate successive slashes */
+			return CHECK_REF_FORMAT_ERROR;
+
 		if (!ch)
 			/* should not end with slashes */
 			return CHECK_REF_FORMAT_ERROR;

new file mode 100755
index 0000000..05fb9f9
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='git check-ref-format'
+
+. ./test-lib.sh
+
+test_expect_success 'refs/heads/master is ok' '
+	git check-ref-format refs/heads/master
+'
+
+test_expect_success 'no successive slashes allowed' '
+	test_must_fail git check-ref-format a//b
+'
+
+test_expect_success 'no trailing slashes allowed' '
+	test_must_fail git check-ref-format a/b/
+'
+
+test_done

1.6.5.rc2.205.g1185a

Hmm. We already do that without your patch.

    $ git branch /foo//bar
    $ git for-each-ref --format='%(refname)'
    refs/heads/foo/bar
    refs/heads/master
    $ git branch -d /foo//bar
    Deleted branch /foo//bar (was deadbeef)
    $ git for-each-ref --format='%(refname)'
    refs/heads/master

Perhaps an interface to give a cleaned-up version, e.g.

    $ git check-ref-format --print refs/heads//foo/bar
    refs/heads/foo/bar

is what you want in order to fix git-gui? I dunno.

The following packages do exactly that. I tried to use it to fix git gui, but my Tcl-fu was not up to it, at least for tonight.

It is not obvious to me that this interface is the one that would be most helpful for git gui: probably a command to just munge the branch name would be more convenient, though less general.

Is the --print option useful? Right now, it seems that its main use is documentation. But in the future, it would be very nice if this command could be used to transform Unicode ref names to some appropriate normalization form, to make Unicode ref names usable in Mac OS X and less confusing everywhere.

I look forward to your thoughts.

Jonathan Nieder (4):
Add tests for git check-ref-format
Documentation: describe check-ref-format --branch in more detail
check-ref-format: add option to print canonical name
check-ref-format: Simplify --print implementation

 Documentation/git-check-ref-format.txt |   32 ++++++++++++----
 builtin-check-ref-format.c             |   31 ++++++++++++++++
 t/t1402-check-ref-format.sh            |   61 ++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 8 deletions(-)
 create mode 100644 t/t1402-check-ref-format.sh

The "git check-ref-format" command is a basic command various porcelains rely on. Test its functionality to make sure it does not unintentionally change.

---
 t/t1402-check-ref-format.sh |   44 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100644 t/t1402-check-ref-format.sh

new file mode 100644
index 0000000..382bc6e
@@ -0,0 +1,44 @@
+#!/bin/sh
+
+test_description='Test git check-ref-format'
+
+. ./test-lib.sh
+
+valid_ref() {
+	test_expect_success "ref name '$1' is valid" \
+		"git check-ref-format '$1'"
+}
+invalid_ref() {
+	test_expect_success "ref name '$1' is not valid" \
+		"test_must_fail git check-ref-format '$1'"
+}
+
+valid_ref 'heads/foo'
+invalid_ref 'foo'
+valid_ref 'foo/bar/baz'
+valid_ref 'refs///heads/foo'
+invalid_ref 'heads/foo/'
+invalid_ref './foo'
+invalid_ref '.refs/foo'
+invalid_ref 'heads/foo..bar'
+invalid_ref 'heads/foo?bar'
+valid_ref 'foo./bar'
+invalid_ref 'heads/foo.lock'
+valid_ref 'heads/foo@bar'
+invalid_ref 'heads/v@{ation'
+invalid_ref 'heads/foo\bar'
+
+test_expect_success "check-ref-format --branch @{-1}" '
+	T=$(git write-tree) &&
+	sha1=$(echo A | git commit-tree $T) &&
+	git update-ref refs/heads/master $sha1 &&
+	git update-ref refs/remotes/origin/master $sha1
+	git checkout master &&
+	git checkout origin/master &&
+	git checkout master &&
+	refname=$(git check-ref-format --branch @{-1}) &&
+	test "$refname" = "$sha1" &&
+	refname2=$(git check-ref-format --branch @{-2}) &&
+	test "$refname2" = master'
+
+test_done

1.6.5.rc1.199.g596ec

Unless one already knew, it was not obvious what sort of shorthand "git check-ref-format --branch" expands. Explain it.

The --branch argument is not optional.

---
 Documentation/git-check-ref-format.txt |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

index 0b7982e..e9b3b40 100644
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git check-ref-format' <refname>
-'git check-ref-format' [--branch] <branchname-shorthand>
+'git check-ref-format' --branch <branchname-shorthand>
 
 DESCRIPTION
 -----------

@@ -63,8 +63,11 @@ reference name expressions (see linkgit:git-rev-parse[1]):
 
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
-With the `--branch` option, it expands a branch name shorthand and
-prints the name of the branch the shorthand refers to.
+With the `--branch` option, it expands the ``previous branch syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
+were on.  This option should be used by porcelains to accept this
+syntax anywhere a branch name is expected, so they can act as if you
+typed the branch name.
 
 EXAMPLE
 -------

1.6.5.rc1.199.g596ec

Tolerating empty path components in ref names means each ref does not have a unique name. This creates difficulty for porcelains that want to see if two branches are equal. Add a helper associating to each ref a canonical name.

If a user asks a porcelain to create a ref "refs/heads//master", the porcelain can run "git check-ref-format --print refs/heads//master" and only deal with "refs/heads/master" from then on.

In the future, it would be very nice if this command could be modified to transform Unicode ref names to some appropriate normalization form, to make Unicode ref names usable in Mac OS X, too, and less confusing everywhere.

---
 Documentation/git-check-ref-format.txt |   25 +++++++++++++++++++------
 builtin-check-ref-format.c             |   10 ++++++++++
 t/t1402-check-ref-format.sh            |   17 +++++++++++++++++
 3 files changed, 46 insertions(+), 6 deletions(-)

index e9b3b40..0aeef24 100644
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git check-ref-format' <refname>
+'git check-ref-format' --print <refname>
 'git check-ref-format' --branch <branchname-shorthand>
 
 DESCRIPTION

@@ -63,19 +64,31 @@ reference name expressions (see linkgit:git-rev-parse[1]):
 
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
+With the `--print` option, if 'refname' is acceptable, it prints the
+canonicalized name of a hypothetical reference with that name.  That is,
+it prints 'refname' with any extra `/` characters removed.
+
 With the `--branch` option, it expands the ``previous branch syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
 were on.  This option should be used by porcelains to accept this
 syntax anywhere a branch name is expected, so they can act as if you
 typed the branch name.
 
-EXAMPLE
--------
-
-git check-ref-format --branch @{-1}::
-
-Print the name of the previous branch.
+EXAMPLES
+--------
 
+* Print the name of the previous branch:
++
+------------
+$ git check-ref-format --branch @{-1}
+------------
+
+* Determine the reference name to use for a new branch:
++
+------------
+$ ref=$(git check-ref-format --print "refs/heads/$newbranch") ||
+die "we do not like '$newbranch' as a branch name."
+------------
 
 GIT
 ---

index f9381e0..b97b61a 100644
@@ -17,6 +17,16 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 		printf("%s\n", sb.buf + 11);
 		exit(0);
 	}
+	if (argc == 3 && !strcmp(argv[1], "--print")) {
+		char *refname = xmalloc(strlen(argv[2]) + 1);
+
+		if (check_ref_format(argv[2]))
+			exit(1);
+		if (normalize_path_copy(refname, argv[2]))
+			die("Could not normalize ref name '%s'", argv[2]);
+		printf("%s\n", refname);
+		exit(0);
+	}
 	if (argc != 2)
 		usage("git check-ref-format refname");
 	return !!check_ref_format(argv[1]);

index 382bc6e..eb45afb 100644
@@ -41,4 +41,21 @@ test_expect_success "check-ref-format --branch @{-1}" '
 	refname2=$(git check-ref-format --branch @{-2}) &&
 	test "$refname2" = master'
 
+valid_ref_normalized() {
+	test_expect_success "ref name '$1' simplifies to '$2'" "
+		refname=\$(git check-ref-format --print '$1') &&
+		test \"\$refname\" = '$2'"
+}
+invalid_ref_normalized() {
+	test_expect_success "check-ref-format --print rejects '$1'" "
+		test_must_fail git check-ref-format --print '$1'"
+}
+
+valid_ref_normalized 'heads/foo' 'heads/foo'
+valid_ref_normalized 'refs///heads/foo' 'refs/heads/foo'
+invalid_ref_normalized 'foo'
+invalid_ref_normalized 'heads/foo/../bar'
+invalid_ref_normalized 'heads/./foo'
+invalid_ref_normalized 'heads\foo'
+
 test_done

1.6.5.rc1.199.g596ec

I understand that you prefer the latter between "there is no tool; the caller is responsibile to make sure it feeds us canonical representation" and "there is a tool that makes a slightly malformed string into canonical form for the callers to use before calling us." And that would be my preference between these two as well.

But that is based on the current behaviour of accepting slightly malformed and silently making it canonical. If we throw a third alternative, Jonathan's original patch, that did "we reject malformed string", in the mix, what would be your preference?

I moderately favour the "tool to canonicalize is given, and it would be a bug for the caller not to use it" approach this series takes primarily because that approach won't break scripts that do something like this to make a new branch, optionally grouped by the owner (e.g. 'sp/smart-http' or 'next'):

	owner=$1 topic=$2
	branch=$owner/$topic
        git branch "$branch"

This currently works as expected as long as it does not later try to compare for-each-ref output with "refs/heads/$branch" and expects a match. With the third approach, the optional username grouping becomes mandatory for such a script, as 'git branch' will error out. To keep the grouping still optinal, such a script needs to be written perhaps like:

	if test -z "$2"
        then
		branch="$1"
	else
        	branch="$1/$2"
	fi

and that would be a hassle for the scripters, but this _could_ be a kind of backward incompatible tightening we might want to consider for 1.7.0, as somebody suggested earlier.

But now I have spelled this out, I do not see much upside for rejecting, and more importantly, I think it would be an independent issue. We can reject or just keep normalizing silently, and a tool to show the normalized name would be useful and necessary regardless of that.

...

I agree with the last paragraph here, we shouldn't reject, but instead keep the current state but encourage tools to use the new canonical print tool to clean up a name if they want to hang onto the string the user entered and it needs to exactly match for-each-ref sort of output.

Shawn.

I do not disagree with a desire to help fixing the unicode insanity on that platform, but I suspect that check-ref-format is a wrong place to tackle the issue. You would need a similar filter for outputs from the likes of ls-files and "diff --name-only", iow, anything that deal with pathnames, no?

It would have be something like "check-ref-format --print | iconv ..." pipeline (conceptually, if not forcing the pipeline to the end users, that is).

Also are people happy with "--print"? I was waiting for others to reword it to "--normalize" or something like that. In your documentation you explain it to "canonicalize", and in your tests you name the output "normalized". I am fine with either wording, but would like to see us using only one, not both.

Thanks.

GNU iconv does not write the various Unicode normalization forms, so it would have to be something like "check-ref-format --print | charlint ..." instead. Regardless of the filesystem, it seems reasonable to consider é (U+00e9) and é (U+0065 + U+0301) the same character when it appears in a ref name, and one way to achieve this would be to pick one normalization form and stick to it. This does not seem so different from stripping out empty path components.

As a side effect, that would deal with OS X’s strange handling of unicode filenames for .git/refs/*. Now that I think about it, if fighting OS X were the only problem that needed to be solved, I don’t think I’d like this solution so much. The analogous solution to the also unsolved issue of case insensitive filesystems is to force all ref names to lowercase. Do we want to do that? (The case insensitivity issue might not be as bad, since the relevant filesystems will at least _preserve_ the case of filenames in .git/refs. Should we copy them and smudge new ref names to match known ones that differ only in case? Just thinking about the problem makes me cringe.)

Coping with the unicode filename craziness in the working tree is a separate issue, though probably a more important one. I think Linus set up a framework for solving it in
<http://thread.gmane.org/gmane.comp.version-control.git/77827>.

Regards,
Jonathan

normalize_path_copy() is a complicated function, but most of its functionality will never apply to a ref name that has been checked with check_ref_format().

---
 builtin-check-ref-format.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

index b97b61a..e439136 100644
@@ -7,6 +7,28 @@
 #include "builtin.h"
 #include "strbuf.h"
 
+/*
+ * Replace each run of adjacent slashes in src with a single slash,
+ * and write the result to dst.
+ *
+ * This function is similar to normalize_path_copy(), but stripped down
+ * to meet check_ref_format's simpler needs.
+ */
+static void collapse_slashes(char *dst, const char *src)
+{
+	char ch;
+	char prev = '\0';
+
+	while (ch = *src++) {
+		if (prev == '/' && ch == prev)
+			continue;
+
+		*dst++ = ch;
+		prev = ch;
+	}
+	*dst = '\0';
+}
+
 int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
 	if (argc == 3 && !strcmp(argv[1], "--branch")) {

@@ -22,8 +44,7 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 
 		if (check_ref_format(argv[2]))
 			exit(1);
-		if (normalize_path_copy(refname, argv[2]))
-			die("Could not normalize ref name '%s'", argv[2]);
+		collapse_slashes(refname, argv[2]);
 		printf("%s\n", refname);
 		exit(0);
 	}

1.6.5.rc1.199.g596ec

s/packages/patches/

I better sleep...

Sorry for the confusion,
Jonathan

Yes, but git changes the name silently from '/foo//bar' to 'foo/bar'!

The automagical removal of some slashes leads to strange behaviour when using such names:

    $ git checkout -b foo/bar
    Switched to a new branch 'foo/bar'
    $ git checkout -b /foo//bar
    fatal: A branch named '/foo//bar' already exists.
    $ git for-each-ref --format='%(refname)'
    refs/heads/foo/bar
    refs/heads/master

git claims wrongly that "A branch named '/foo//bar' already exists.".

And that same happens to tag and repo names, leading to other strange effects:

    $ git tag /foo//bar
    $ git tag
    foo/bar

That is not the tagname the user provided.

    $ git remote add /foo//bar git://git.kernel.org/pub/scm/git/git.git
    $ git remote show
    warning: Config remote shorthand cannot begin with '/': /foo//bar.url
    warning: Config remote shorthand cannot begin with '/': /foo//bar.fetch
    $ git fetch /foo//bar
    warning: Config remote shorthand cannot begin with '/': /foo//bar.url
    warning: Config remote shorthand cannot begin with '/': /foo//bar.fetch
    fatal: 'foo/bar' does not appear to be a git repository
    fatal: The remote end hung up unexpectedly

Note: git fetch uses both versions of the remote name in its output.

Yes, one solution could be to fix every application handling branch, tag or repo names to mimic the namechange done in the bowels of git. But i think it is not worth the hassle. Every application i tested (git gui, gitk and a handful of git commands) just assumes - and i think rightfully so - that refnames will not change while they are being created (And they do call "git check-ref-format" or strbuf_check_branch_ref() and friends to make sure they have a valid refname, but it gets changed nonetheless).

With this patch you get an error every time you try to create such a refname in the first place and the strange effects and the ambiguity of refnames ('/foo//bar' is a synonym for 'foo/bar' right now) just go away.

The motivation for this patch was a confused user at my dayjob. IMHO most people don't use '/foo//bar' on purpose but the extra slashes get there through typos, copy & paste errors and such. Then generating an error might just be the Right Thing to do to avoid the problems with the changed refname afterwards.

That cuts both ways.

When the users make typoes (e.g. /foo//bar) you can accept the only sane correction (e.g. foo/bar) instead of rejecting, since the only thing the user can do after getting such a rejection is to correct it to that corrected name (e.g. foo/bar) himself and re-issue the command anyway. You can push the "hassle" down to the user, or you can fix the tool to remove the hassle from the user.

Besides, by rejecting what we used to accept you are breaking people's expectations. So I am moderately negative, unless you can say your "every application" is literally _tons_.

Yes, making check-ref-format stricter without changing its users to massage their input would be a regression.

The problem Jens described is a git-gui bug. In lib/branch_delete.tcl, line 57, the list of deletable branches is populated as follows:

	foreach h [load_all_heads] {
		if {$h ne $current_branch} {
			$w_heads insert end $h
		}
	}

Since slashes coalesce, a user-supplied new branch name is not canonical and checking "$h eq $current_branch" does not actually check if they are the same branch. git-gui should be using the branch name as output by "git check-ref-format --branch" after the branch is created.

But what about other scripts that assume each branch has only one possible name? Maybe they could be forced to fix up the name early on by making check-ref-format reject names with "//" in them and providing a "git check-ref-format --print" to help.

Upside: scripts would complain loudly instead of failing subtly on input with extra slashes if they forget to use "git check-ref-format --print" when appropriate. Downside: users, including the 22 callers of check_ref_format() in git, would have to be checked and probably changed to avoid regressions.

Never having made that typo, I find it hard to consider such a change worth the trouble, but it sounds doable.

Regards,
Jonathan

Isn't 1.7.0 a good release to break such people's expectations?

Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/