Re: Buggy handling of non-canonical ref names

Junio C Hamano [off-list ref] writes:

Michael Haggerty [off-list ref] writes:

quoted

What is the policy about reference names and their canonicalization?

The overall policy has been that we care about well-formed input, and
everything else is "undefined", even though as you found out some of them
try to work sensibly.

quoted

    $ git check-ref-format /foo/bar ; echo $?
    0

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

I think these are bogus. Patches welcome.

I actually think the former is correct and the latter should strip the
leading slash. Essentially what "check-ref-format" (and the underlying
check_ref_format() function) validates is if the given user string can be
used under $GIT_DIR/refs/ to name a ref, and $GIT_DIR/refs//foo/bar is
(because we "tolerate duplicated slashes" cf. comment in the function) the
same as $GIT_DIR/refs/foo/bar and is allowed.

I do not know what the original motivation behind --print was, but it
seems to want to show it canonicalized, so it should strip the leading
slash as well.

I think what is missing is a unified way to canonicalize the refnames
(which led to the inconsistencies you observed), and I strongly suspect
that check_ref_format() should learn to return the canonicalized format
(if asked by the caller) and the caller should use the canonicalized
version after feeding end-user input to it.

Then the plumbing "check-ref-format --print" can use it just like any
other caller that should be (or already are) using check_ref_format()
to validate the end-user input.

Yes, such a change will update the overall policy I stated earlier and
narrow the scope of "undefined" down a bit, by uniformly codifying that
leading and duplicate slashes are removed to be nice to the user.

On 08/25/2011 12:27 AM, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Michael Haggerty [off-list ref] writes:

quoted

What is the policy about reference names and their canonicalization?

The overall policy has been that we care about well-formed input, and
everything else is "undefined", even though as you found out some of them
try to work sensibly.

quoted

    $ git check-ref-format /foo/bar ; echo $?
    0

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

I think these are bogus. Patches welcome.

I actually think the former is correct and the latter should strip the
leading slash. Essentially what "check-ref-format" (and the underlying
check_ref_format() function) validates is if the given user string can be
used under $GIT_DIR/refs/ to name a ref, and $GIT_DIR/refs//foo/bar is
(because we "tolerate duplicated slashes" cf. comment in the function) the
same as $GIT_DIR/refs/foo/bar and is allowed.

I can live with either way, but I should point out that such an extra
slash can be problematic when used naively in conjunction with Python's
standard glue-together-pathname function, os.path.join() [1]:

    $ python
    >>> import os
    >>> os.path.join('.git', '/foo/bar')
    '/foo/bar'
    >>>

Maybe there are other examples of libraries with these semantics.

I think what is missing is a unified way to canonicalize the refnames
(which led to the inconsistencies you observed), and I strongly suspect
that check_ref_format() should learn to return the canonicalized format
(if asked by the caller) and the caller should use the canonicalized
version after feeding end-user input to it.

Then the plumbing "check-ref-format --print" can use it just like any
other caller that should be (or already are) using check_ref_format()
to validate the end-user input.

Indeed, regardless of the policy about leading slashes, this is a good
plan.  I will try to find time to work on it.

Yes, such a change will update the overall policy I stated earlier and
narrow the scope of "undefined" down a bit, by uniformly codifying that
leading and duplicate slashes are removed to be nice to the user.

Michael

[1] http://docs.python.org/library/os.path.html#os.path.join

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

Previously, refnames with leading slashes were handled inconsistently.
So forbid them altogether.

Signed-off-by: Michael Haggerty <redacted>
---
This patch chooses the alternative of forbidding leading slashes,
because I feel that it is the safer option.  It is like Carlos's patch
but with documentation changes, test-suite additions, and an
corresponding change to git.py.  If anybody knows of other
implementations of the ref format checking code, please let me know.

This patch applies to either master or maint.  Since it restricts the
DWIM behavior in some cases, master is probably the better choice.

 Documentation/git-check-ref-format.txt |    7 ++++---
 git_remote_helpers/git/git.py          |    3 ++-
 refs.c                                 |    3 +++
 t/t1402-check-ref-format.sh            |    4 ++++
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index c9fdf84..238b185 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt

@@ -34,15 +34,16 @@ git imposes the following rules on how references are named:
   category like `heads/`, `tags/` etc. but the actual names are not
   restricted.
 
-. They cannot have two consecutive dots `..` anywhere.
+. They must not start or end with slash `/`.
+
+. They cannot have two consecutive dots `..` anywhere or end with a
+  dot `.`.
 
 . They cannot have ASCII control characters (i.e. bytes whose
   values are lower than \040, or \177 `DEL`), space, tilde `~`,
   caret `{caret}`, colon `:`, question-mark `?`, asterisk `*`,
   or open bracket `[` anywhere.
 
-. They cannot end with a slash `/` nor a dot `.`.
-
 . They cannot end with the sequence `.lock`.
 
 . They cannot contain a sequence `@{`.

diff --git a/git_remote_helpers/git/git.py b/git_remote_helpers/git/git.py
index a383e6c..6df53aa 100644
--- a/git_remote_helpers/git/git.py
+++ b/git_remote_helpers/git/git.py

@@ -55,7 +55,8 @@ def valid_git_ref (ref_name):
     # command.  The rules were derived from the git check-ref-format(1)
     # manual page.  This code should be replaced by a call to
     # check_ref_format() in the git library, when such is available.
-    if ref_name.endswith('/') or \
+    if ref_name.startswith('/') or \
+       ref_name.endswith('/') or \
        ref_name.startswith('.') or \
        ref_name.count('/.') or \
        ref_name.count('..') or \

diff --git a/refs.c b/refs.c
index 6f313a9..84c5af3 100644
--- a/refs.c
+++ b/refs.c

@@ -880,6 +880,9 @@ int check_ref_format(const char *ref)
 	const char *cp = ref;
 
 	level = 0;
+	if (*cp == '/')
+		/* no leading slashes */
+		return CHECK_REF_FORMAT_ERROR;
 	while (1) {
 		while ((ch = *cp++) == '/')
 			; /* tolerate duplicated slashes */

diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 1b0f82f..b05ca26 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh

@@ -18,6 +18,10 @@ invalid_ref 'foo'
 valid_ref 'foo/bar/baz'
 valid_ref 'refs///heads/foo'
 invalid_ref 'heads/foo/'
+invalid_ref '/foo'
+invalid_ref 'foo/'
+invalid_ref '/foo/bar'
+invalid_ref 'foo/bar/'
 invalid_ref './foo'
 invalid_ref '.refs/foo'
 invalid_ref 'heads/foo..bar'

-- 
1.7.6.8.gd2879