get_sha1_basic(): corner case ambiguity fix

When .git/refs/heads/frotz and .git/refs/tags/frotz existed, and
the object name stored in .git/refs/heads/frotz were corrupt, we
ended up picking tags/frotz without complaining.  Worse yet, if
the corrupt .git/refs/heads/frotz was more than 40 bytes and
began with hexadecimal characters, it silently overwritten the
initial part of the returned result.

This commit adds a couple of tests to demonstrate these cases,
with a fix.

Signed-off-by: Junio C Hamano <junkio@cox.net>

diff --git a/sha1_name.c b/sha1_name.c
index faac158..bf8f0f0 100644 (file)
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -203,11 +203,12 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
        return NULL;
 }
 
-static int ambiguous_path(const char *path)
+static int ambiguous_path(const char *path, int len)
 {
        int slash = 1;
+       int cnt;
 
-       for (;;) {
+       for (cnt = 0; cnt < len; cnt++) {
                switch (*path++) {
                case '\0':
                        break;
@@ -224,6 +225,7 @@ static int ambiguous_path(const char *path)
                }
                return slash;
        }
+       return slash;
 }
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
@@ -242,26 +244,41 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
                return 0;
 
        /* Accept only unambiguous ref paths. */
-       if (ambiguous_path(str))
+       if (ambiguous_path(str, len))
                return -1;
 
        for (p = prefix; *p; p++) {
                char *pathname = git_path("%s/%.*s", *p, len, str);
+
                if (!read_ref(pathname, sha1)) {
                        /* Must be unique; i.e. when heads/foo and
                         * tags/foo are both present, reject "foo".
-                        * Note that read_ref() eventually calls
-                        * get_sha1_hex() which can smudge initial
-                        * part of the buffer even if what is read
-                        * is found to be invalid halfway.
                         */
                        if (1 < found++)
                                return -1;
                }
+
+               /* We want to allow .git/description file and
+                * "description" branch to exist at the same time.
+                * "git-rev-parse description" should silently skip
+                * .git/description file as a candidate for
+                * get_sha1().  However, having garbage file anywhere
+                * under refs/ is not OK, and we would not have caught
+                * ambiguous heads and tags with the above test.
+                */
+               else if (**p && !access(pathname, F_OK)) {
+                       /* Garbage exists under .git/refs */
+                       return error("garbage ref found '%s'", pathname);
+               }
        }
-       if (found == 1)
+       switch (found) {
+       case 0:
+               return -1;
+       case 1:
                return 0;
-       return -1;
+       default:
+               return error("ambiguous refname '%.*s'", len, str);
+       }
 }
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1);

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index bc3e711..ffa723e 100755 (executable)
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -205,4 +205,52 @@ test_expect_success \
     'no diff after checkout and git-update-index --refresh.' \
     'git-diff-files >current && cmp -s current /dev/null'
 
+
+# extended sha1 parsing and ambiguity resolution
+
+GIT_AUTHOR_DATE='1995-01-29T16:00:00 -0800'
+GIT_AUTHOR_EMAIL=a.u.thor@example.com
+GIT_AUTHOR_NAME='A U Thor'
+GIT_COMMITTER_DATE='1995-01-29T16:00:00 -0800'
+GIT_COMMITTER_EMAIL=c.o.mmitter@example.com
+GIT_COMMITTER_NAME='C O Mmitter'
+export GIT_AUTHOR_DATE
+export GIT_AUTHOR_EMAIL
+export GIT_AUTHOR_NAME
+export GIT_COMMITTER_DATE
+export GIT_COMMITTER_EMAIL
+export GIT_COMMITTER_NAME
+
+test_expect_success \
+       'initial commit.' \
+       'commit=$(echo Initial commit | git-commit-tree $tree) &&
+        echo "$commit" >.git/refs/heads/master &&
+        git-ls-tree HEAD &&
+        test "$commit" = 51a092e9ef6cbbe66d258acd17599d3f80be6162'
+
+test_expect_success \
+       'Ambiguous' \
+       'echo "$commit" >.git/refs/heads/nasty &&
+        echo "$commit" >.git/refs/tags/nasty &&
+        if git-rev-parse --verify nasty
+        then
+               echo "should have barfed"
+               false
+        else
+               :
+        fi &&
+        # names directly underneath .git/ should not interfere
+        echo "$commit" >.git/refs/heads/description &&
+        git-rev-parse --verify description &&
+        # broken object name
+        echo fffffffffffffffffffffffffffffffffffffffg \
+               >.git/refs/heads/nasty &&
+        if git-rev-parse --verify nasty
+        then
+               echo "should have barfed"
+               false
+        else
+               :
+        fi'
+
 test_done

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2819bef..a97d259 100755 (executable)
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -18,6 +18,7 @@ unset GIT_ALTERNATE_OBJECT_DIRECTORIES
 unset GIT_AUTHOR_DATE
 unset GIT_AUTHOR_EMAIL
 unset GIT_AUTHOR_NAME
+unset GIT_COMMITTER_DATE
 unset GIT_COMMITTER_EMAIL
 unset GIT_COMMITTER_NAME
 unset GIT_DIFF_OPTS