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>

3 files changed, 75 insertions, 9 deletions

diff --git a/sha1_name.c b/sha1_name.c
index faac158b1..bf8f0f0e1 100644
--- 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 bc3e711a5..ffa723ea8 100755
--- 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 2819bef1c..a97d259e2 100755
--- 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