[PATCH v2] Detailed diagnosis when parsing an object name fails.

[PATCH v2] Detailed diagnosis when parsing an object name fails.

[y]

From: Matthieu Moy <Matthieu.Moy@imag.fr>

The previous error message was the same in many situations (unknown
revision or path not in the working tree). We try to help the user as
much as possible to understand the error, especially with the
sha1:filename notation. In this case, we say whether the sha1 or the
filename is problematic, and diagnose the confusion between
relative-to-root and relative-to-$PWD confusion precisely.

The 6 new error messages are tested.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Changes since v1:

* Fixed a segfault with

+	if (!prefix)
+		prefix = "";

* Added testcases.

 cache.h                        |    6 ++-
 setup.c                        |   15 +++++-
 sha1_name.c                    |   95 ++++++++++++++++++++++++++++++++++++++--
 t/t1506-rev-parse-diagnosis.sh |   67 ++++++++++++++++++++++++++++
 4 files changed, 176 insertions(+), 7 deletions(-)
 create mode 100755 t/t1506-rev-parse-diagnosis.sh

diff --git a/cache.h b/cache.h
index 0e69384..5c8cb5f 100644
--- a/cache.h
+++ b/cache.h
@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
 #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+{
+	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
+}
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/setup.c b/setup.c
index f67250b..3094e8b 100644
--- a/setup.c
+++ b/setup.c
@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
+static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+{
+	unsigned char sha1[20];
+	unsigned mode;
+	/* try a detailed diagnostic ... */
+	get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
+	/* ... or fall back the most general message. */
+	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
+	    "Use '--' to separate paths from revisions", arg);
+
+}
+
 /*
  * Verify a filename that we got as an argument for a pathspec
  * entry. Note that a filename that begins with "-" never verifies
@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
-	    "Use '--' to separate paths from revisions", arg);
+	die_verify_filename(prefix, arg);
 }
 
 /*
diff --git a/sha1_name.c b/sha1_name.c
index 44bb62d..030e2ac 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -804,7 +804,77 @@ int get_sha1(const char *name, unsigned char *sha1)
 	return get_sha1_with_mode(name, sha1, &unused);
 }
 
-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
+static void diagnose_invalid_sha1_path(const char *prefix,
+				       const char *filename,
+				       const char *tree_sha1,
+				       const char *object_name)
+{
+	struct stat st;
+	unsigned char sha1[20];
+	unsigned mode;
+
+	if (!prefix)
+		prefix = "";
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in '%s'.",
+		    filename, object_name);
+	if (errno == ENOENT || errno == ENOTDIR) {
+		char *fullname = malloc(strlen(filename)
+					     + strlen(prefix) + 1);
+		strcpy(fullname, prefix);
+		strcat(fullname, filename);
+
+		if (!get_tree_entry(tree_sha1, fullname,
+				    sha1, &mode)) {
+			die("Path '%s' exists, but not '%s'.\n"
+			    "Did you mean '%s:%s'?",
+			    fullname,
+			    filename,
+			    object_name,
+			    fullname);
+		}
+		die("Path '%s' does not exist in '%s'",
+		    filename, object_name);
+	}
+}
+
+static void diagnose_invalid_index_path(int stage,
+					const char *prefix,
+					const char *filename)
+{
+	struct stat st;
+
+	if (!prefix)
+		prefix = "";
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in the index.", filename);
+	if (errno == ENOENT || errno == ENOTDIR) {
+		struct cache_entry *ce;
+		int pos;
+		int namelen = strlen(filename) + strlen(prefix);
+		char *fullname = malloc(namelen + 1);
+		strcpy(fullname, prefix);
+		strcat(fullname, filename);
+		pos = cache_name_pos(fullname, namelen);
+		if (pos < 0)
+			pos = -pos - 1;
+		ce = active_cache[pos];
+		if (ce_namelen(ce) == namelen &&
+		    !memcmp(ce->name, fullname, namelen))
+			die("Path '%s' is in the index, but not '%s'.\n"
+			    "Did you mean ':%d:%s'?",
+			    fullname, filename,
+			    stage, fullname);
+
+		die("Path '%s' does not exist (neither on disk nor in the index).",
+		    filename);
+	}
+}
+
+
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
@@ -850,6 +920,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 			}
 			pos++;
 		}
+		if (fatal)
+			diagnose_invalid_index_path(stage, prefix, cp);
 		return -1;
 	}
 	for (cp = name, bracket_depth = 0; *cp; cp++) {
@@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 	}
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
-		if (!get_sha1_1(name, cp-name, tree_sha1))
-			return get_tree_entry(tree_sha1, cp+1, sha1,
-					      mode);
+		char *object_name;
+		if (fatal) {
+			object_name = malloc(cp-name+1);
+			strncpy(object_name, name, cp-name);
+			object_name[cp-name] = '\0';
+		}
+		if (!get_sha1_1(name, cp-name, tree_sha1)) {
+			const char *filename = cp+1;
+			ret = get_tree_entry(tree_sha1, filename, sha1, mode);
+			if (fatal)
+				diagnose_invalid_sha1_path(prefix, filename,
+							   tree_sha1, object_name);
+
+			return ret;
+		} else {
+			if (fatal)
+				die("Invalid object name '%s'.", object_name);
+		}
 	}
 	return ret;
 }
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
new file mode 100755
index 0000000..8112d56
--- /dev/null
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='test git rev-parse diagnosis for invalid argument'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+HASH_file=
+
+test_expect_success 'set up basic repo' '
+	echo one > file.txt &&
+	mkdir subdir &&
+	echo two > subdir/file.txt &&
+	echo three > subdir/file2.txt &&
+	git add . &&
+	git commit -m init &&
+	echo four > index-only.txt &&
+	git add index-only.txt &&
+	echo five > disk-only.txt
+'
+
+test_expect_success 'correct file objects' '
+	HASH_file=$(git rev-parse HEAD:file.txt) &&
+	git rev-parse HEAD:subdir/file.txt &&
+	git rev-parse :index-only.txt &&
+	cd subdir &&
+	git rev-parse HEAD:file.txt &&
+	git rev-parse HEAD:subdir/file2.txt &&
+	test $HASH_file = $(git rev-parse HEAD:file.txt) &&
+	test $HASH_file = $(git rev-parse :file.txt) &&
+	test $HASH_file = $(git rev-parse :0:file.txt) &&
+	cd ..
+'
+
+test_expect_success 'incorrect revision id' '
+	test_must_fail git rev-parse foobar:file.txt 2>&1 |
+		grep "Invalid object name '"'"'foobar'"'"'." &&
+	test_must_fail git rev-parse foobar 2>&1 |
+		grep "unknown revision or path not in the working tree."
+'
+
+test_expect_success 'incorrect file in sha1:path' '
+	test_must_fail git rev-parse HEAD:nothing.txt 2>&1 |
+		grep "fatal: Path '"'"'nothing.txt'"'"' does not exist in '"'"'HEAD'"'"'" &&
+	test_must_fail git rev-parse HEAD:index-only.txt 2>&1 |
+		grep "fatal: Path '"'"'index-only.txt'"'"' exists on disk, but not in '"'"'HEAD'"'"'." &&
+	cd subdir &&
+	test_must_fail git rev-parse HEAD:file2.txt 2>&1 |
+		grep "Did you mean '"'"'HEAD:subdir/file2.txt'"'"'?" &&
+	cd ..
+'
+
+test_expect_success 'incorrect file in :path and :0:path' '
+	test_must_fail git rev-parse :nothing.txt 2>&1 |
+		grep "fatal: Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." &&
+	test_must_fail git rev-parse :1:nothing.txt 2>&1 |
+		grep "Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." &&
+	cd subdir &&
+	test_must_fail git rev-parse :file2.txt 2>&1 |
+		grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" &&
+	cd .. &&
+	test_must_fail git rev-parse :disk-only.txt 2>&1 |
+		grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index."
+'
+
+test_done
-- 
1.6.6.rc0.256.g6060

Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.

[Junio C Hamano]

y@imag.fr writes:

> diff --git a/cache.h b/cache.h
> index 0e69384..5c8cb5f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
>  #define DEFAULT_ABBREV 7
>  
>  extern int get_sha1(const char *str, unsigned char *sha1);
> -extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
> +static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
> +{
> +	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
> +}
> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);

Do I understand correctly that "fatal" here is the same as "!gently"
elsewhere in the API?

> diff --git a/sha1_name.c b/sha1_name.c
> index 44bb62d..030e2ac 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -804,7 +804,77 @@ int get_sha1(const char *name, unsigned char *sha1)
>  	return get_sha1_with_mode(name, sha1, &unused);
>  }
>  
> -int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
> +static void diagnose_invalid_sha1_path(const char *prefix,
> +				       const char *filename,
> +				       const char *tree_sha1,
> +				       const char *object_name)
> +{
> +	struct stat st;
> +	unsigned char sha1[20];
> +	unsigned mode;
> +
> +	if (!prefix)
> +		prefix = "";
> +
> +	if (!lstat(filename, &st))
> +		die("Path '%s' exists on disk, but not in '%s'.",
> +		    filename, object_name);
> +	if (errno == ENOENT || errno == ENOTDIR) {
> +		char *fullname = malloc(strlen(filename)
> +					     + strlen(prefix) + 1);
> +		strcpy(fullname, prefix);
> +		strcat(fullname, filename);

What if malloc fails here (and elsewhere in your patch)?

> +		if (!get_tree_entry(tree_sha1, fullname,
> +				    sha1, &mode)) {
> +			die("Path '%s' exists, but not '%s'.\n"
> +			    "Did you mean '%s:%s'?",
> +			    fullname,
> +			    filename,
> +			    object_name,
> +			    fullname);
> +		}
> +		die("Path '%s' does not exist in '%s'",
> +		    filename, object_name);
> +	}
> +}
> +
> +static void diagnose_invalid_index_path(int stage,
> +					const char *prefix,
> +					const char *filename)
> +{
> +	struct stat st;
> +
> +	if (!prefix)
> +		prefix = "";
> +
> +	if (!lstat(filename, &st))
> +		die("Path '%s' exists on disk, but not in the index.", filename);
> +	if (errno == ENOENT || errno == ENOTDIR) {
> +		struct cache_entry *ce;
> +		int pos;
> +		int namelen = strlen(filename) + strlen(prefix);
> +		char *fullname = malloc(namelen + 1);
> +		strcpy(fullname, prefix);
> +		strcat(fullname, filename);
> +		pos = cache_name_pos(fullname, namelen);
> +		if (pos < 0)
> +			pos = -pos - 1;
> +		ce = active_cache[pos];
> +		if (ce_namelen(ce) == namelen &&
> +		    !memcmp(ce->name, fullname, namelen))
> +			die("Path '%s' is in the index, but not '%s'.\n"
> +			    "Did you mean ':%d:%s'?",
> +			    fullname, filename,
> +			    stage, fullname);

What happens if the user asked for ":2:Makefile" while in directory "t/",
and there is ":1:t/Makefile" but not ":2:t/Makefile" in the index?

What should happen if the user asked for ":2:t/Makefile" in such a case?

> @@ -850,6 +920,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
>  			}
>  			pos++;
>  		}
> +		if (fatal)
> +			diagnose_invalid_index_path(stage, prefix, cp);
>  		return -1;
>  	}
>  	for (cp = name, bracket_depth = 0; *cp; cp++) {
> @@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
>  	}
>  	if (*cp == ':') {
>  		unsigned char tree_sha1[20];
> -		if (!get_sha1_1(name, cp-name, tree_sha1))
> -			return get_tree_entry(tree_sha1, cp+1, sha1,
> -					      mode);
> +		char *object_name;
> +		if (fatal) {
> +			object_name = malloc(cp-name+1);

Where is this freed?

Instead of doing a leaky allocation, it may make sense to pass the tree
object name as <const char *, size_t> pair, and print it with "%.*s" in
the error reporting codepath.  After all, object_name is used only for
that purpose in diagnose_invalid_sha1_path(), no?

> +			strncpy(object_name, name, cp-name);
> +			object_name[cp-name] = '\0';
> +		}
> +		if (!get_sha1_1(name, cp-name, tree_sha1)) {
> +			const char *filename = cp+1;
> +			ret = get_tree_entry(tree_sha1, filename, sha1, mode);
> +			if (fatal)
> +				diagnose_invalid_sha1_path(prefix, filename,
> +							   tree_sha1, object_name);
> +
> +			return ret;
> +		} else {
> +			if (fatal)
> +				die("Invalid object name '%s'.", object_name);
> +		}
>  	}
>  	return ret;
>  }
> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> new file mode 100755
> index 0000000..8112d56
> --- /dev/null
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -0,0 +1,67 @@
> +#!/bin/sh
> +
> +test_description='test git rev-parse diagnosis for invalid argument'
> +
> +exec </dev/null
> +
> +. ./test-lib.sh
> +
> +HASH_file=
> +
> +test_expect_success 'set up basic repo' '
> +	echo one > file.txt &&
> +	mkdir subdir &&
> +	echo two > subdir/file.txt &&
> +	echo three > subdir/file2.txt &&
> +	git add . &&
> +	git commit -m init &&
> +	echo four > index-only.txt &&
> +	git add index-only.txt &&
> +	echo five > disk-only.txt
> +'
> +
> +test_expect_success 'correct file objects' '
> +	HASH_file=$(git rev-parse HEAD:file.txt) &&
> +	git rev-parse HEAD:subdir/file.txt &&
> +	git rev-parse :index-only.txt &&
> +	cd subdir &&
> +	git rev-parse HEAD:file.txt &&
> +	git rev-parse HEAD:subdir/file2.txt &&
> +	test $HASH_file = $(git rev-parse HEAD:file.txt) &&
> +	test $HASH_file = $(git rev-parse :file.txt) &&
> +	test $HASH_file = $(git rev-parse :0:file.txt) &&
> +	cd ..
> +'

Please make it a habit of not doing "cd" without forcing a subprocess
using ().  If 'rev-parse HEAD:file.txt' fails after "cd subdir", the next
test will start running from that directory.

> +test_expect_success 'incorrect revision id' '
> +	test_must_fail git rev-parse foobar:file.txt 2>&1 |
> +		grep "Invalid object name '"'"'foobar'"'"'." &&

It always is better to write this in separate steps, because exit status
of the upstream of pipe is discarded by the shell.

If you expect an error exit and want to make sure a particular error
message is given, do this:

	test_must_fail git rev-parse foobar:file.txt 2>error &&
        grep "Invalid ..." error 

If you expect an error exit and want to make sure an incorrect error
message is not produced, do this:

	test_must_fail git rev-parse foobar:file.txt 2>error &&
        ! grep "Invalid ..." error 

Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.

[Matthieu Moy]

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

> y@imag.fr writes:

(sorry for the bad email, my fingers tried to anwser "yes" when
git-send-email asked me to confirm my address)

>> diff --git a/cache.h b/cache.h
>> index 0e69384..5c8cb5f 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
>>  #define DEFAULT_ABBREV 7
>>  
>>  extern int get_sha1(const char *str, unsigned char *sha1);
>> -extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
>> +static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
>> +{
>> +	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
>> +}
>> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);
>
> Do I understand correctly that "fatal" here is the same as "!gently"
> elsewhere in the API?

It seems it is. I renamed it.

>> +	if (errno == ENOENT || errno == ENOTDIR) {
>> +		char *fullname = malloc(strlen(filename)
>> +					     + strlen(prefix) + 1);
>> +		strcpy(fullname, prefix);
>> +		strcat(fullname, filename);
>
> What if malloc fails here (and elsewhere in your patch)?

Should have been an xmalloc. Fixed.

>> +static void diagnose_invalid_index_path(int stage,
>> +					const char *prefix,
>> +					const char *filename)
>> +{
>> +	struct stat st;
>> +
>> +	if (!prefix)
>> +		prefix = "";
>> +
>> +	if (!lstat(filename, &st))
>> +		die("Path '%s' exists on disk, but not in the index.", filename);
>> +	if (errno == ENOENT || errno == ENOTDIR) {
>> +		struct cache_entry *ce;
>> +		int pos;
>> +		int namelen = strlen(filename) + strlen(prefix);
>> +		char *fullname = malloc(namelen + 1);
>> +		strcpy(fullname, prefix);
>> +		strcat(fullname, filename);
>> +		pos = cache_name_pos(fullname, namelen);
>> +		if (pos < 0)
>> +			pos = -pos - 1;
>> +		ce = active_cache[pos];
>> +		if (ce_namelen(ce) == namelen &&
>> +		    !memcmp(ce->name, fullname, namelen))
>> +			die("Path '%s' is in the index, but not '%s'.\n"
>> +			    "Did you mean ':%d:%s'?",
>> +			    fullname, filename,
>> +			    stage, fullname);
                            ^^^^^
This one should have been ce_stage(ce), otherwise, we
suggest :0:t/Makefile as you probably guessed:

> What happens if the user asked for ":2:Makefile" while in directory "t/",
> and there is ":1:t/Makefile" but not ":2:t/Makefile" in the index?
>
> What should happen if the user asked for ":2:t/Makefile" in such a
> case?

I originally didn't handle the case "it's in the index, but not at the
stage you requested" (since most users asking :N:blah will be fluent
enough in Git not to need a special case of error messages). But I
finally implemented this test.

>> @@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
>>  	}
>>  	if (*cp == ':') {
>>  		unsigned char tree_sha1[20];
>> -		if (!get_sha1_1(name, cp-name, tree_sha1))
>> -			return get_tree_entry(tree_sha1, cp+1, sha1,
>> -					      mode);
>> +		char *object_name;
>> +		if (fatal) {
>> +			object_name = malloc(cp-name+1);
>
> Where is this freed?

Nowhere. In the present state, it's not serious since all codepath
going through this malloc reach a die() right after. But a free()
doesn't harm, I've added it.

> Instead of doing a leaky allocation, it may make sense to pass the tree
> object name as <const char *, size_t> pair, and print it with "%.*s" in
> the error reporting codepath.  After all, object_name is used only for
> that purpose in diagnose_invalid_sha1_path(), no?

matter of taste, but I prefer doing one "complicated" thing (copying
the string) once, and avoid having to deal with two variables instead
of one later.

>> +test_expect_success 'correct file objects' '
>> +	HASH_file=$(git rev-parse HEAD:file.txt) &&
>> +	git rev-parse HEAD:subdir/file.txt &&
>> +	git rev-parse :index-only.txt &&
>> +	cd subdir &&
>> +	git rev-parse HEAD:file.txt &&
>> +	git rev-parse HEAD:subdir/file2.txt &&
>> +	test $HASH_file = $(git rev-parse HEAD:file.txt) &&
>> +	test $HASH_file = $(git rev-parse :file.txt) &&
>> +	test $HASH_file = $(git rev-parse :0:file.txt) &&
>> +	cd ..
>> +'
>
> Please make it a habit of not doing "cd" without forcing a subprocess
> using ().

Fixed.

>> +	test_must_fail git rev-parse foobar:file.txt 2>&1 |
>> +		grep "Invalid object name '"'"'foobar'"'"'." &&
>
> It always is better to write this in separate steps, because exit status
> of the upstream of pipe is discarded by the shell.

Fixed.

I'll resubmit soon.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2] Detailed diagnosis when parsing an object name fails.

[Matthieu Moy]

The previous error message was the same in many situations (unknown
revision or path not in the working tree). We try to help the user as
much as possible to understand the error, especially with the
sha1:filename notation. In this case, we say whether the sha1 or the
filename is problematic, and diagnose the confusion between
relative-to-root and relative-to-$PWD confusion precisely.

The 7 new error messages are tested.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This should address Junio's comments (BTW, I forgot to say "thanks for
the review" :-) ).


 cache.h                        |    6 ++-
 setup.c                        |   15 +++++-
 sha1_name.c                    |  115 ++++++++++++++++++++++++++++++++++++++--
 t/t1506-rev-parse-diagnosis.sh |   69 ++++++++++++++++++++++++
 4 files changed, 198 insertions(+), 7 deletions(-)
 create mode 100755 t/t1506-rev-parse-diagnosis.sh

diff --git a/cache.h b/cache.h
index 0e69384..563862c 100644
--- a/cache.h
+++ b/cache.h
@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
 #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+{
+	return get_sha1_with_mode_1(str, sha1, mode, 1, NULL);
+}
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/setup.c b/setup.c
index f67250b..5792eb7 100644
--- a/setup.c
+++ b/setup.c
@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
+static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+{
+	unsigned char sha1[20];
+	unsigned mode;
+	/* try a detailed diagnostic ... */
+	get_sha1_with_mode_1(arg, sha1, &mode, 0, prefix);
+	/* ... or fall back the most general message. */
+	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
+	    "Use '--' to separate paths from revisions", arg);
+
+}
+
 /*
  * Verify a filename that we got as an argument for a pathspec
  * entry. Note that a filename that begins with "-" never verifies
@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
-	    "Use '--' to separate paths from revisions", arg);
+	die_verify_filename(prefix, arg);
 }
 
 /*
diff --git a/sha1_name.c b/sha1_name.c
index 44bb62d..1bc09ac 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -804,7 +804,96 @@ int get_sha1(const char *name, unsigned char *sha1)
 	return get_sha1_with_mode(name, sha1, &unused);
 }
 
-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
+/* Must be called only when object_name:filename doesn't exist. */
+static void diagnose_invalid_sha1_path(const char *prefix,
+				       const char *filename,
+				       const char *tree_sha1,
+				       const char *object_name)
+{
+	struct stat st;
+	unsigned char sha1[20];
+	unsigned mode;
+
+	if (!prefix)
+		prefix = "";
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in '%s'.",
+		    filename, object_name);
+	if (errno == ENOENT || errno == ENOTDIR) {
+		char *fullname = xmalloc(strlen(filename)
+					     + strlen(prefix) + 1);
+		strcpy(fullname, prefix);
+		strcat(fullname, filename);
+
+		if (!get_tree_entry(tree_sha1, fullname,
+				    sha1, &mode)) {
+			die("Path '%s' exists, but not '%s'.\n"
+			    "Did you mean '%s:%s'?",
+			    fullname,
+			    filename,
+			    object_name,
+			    fullname);
+		}
+		die("Path '%s' does not exist in '%s'",
+		    filename, object_name);
+	}
+}
+
+/* Must be called only when :stage:filename doesn't exist. */
+static void diagnose_invalid_index_path(int stage,
+					const char *prefix,
+					const char *filename)
+{
+	struct stat st;
+	struct cache_entry *ce;
+	int pos;
+	int namelen = strlen(filename);
+	int fullnamelen;
+	char *fullname;
+
+	if (!prefix)
+		prefix = "";
+
+	/* Wrong stage number? */
+	pos = cache_name_pos(filename, namelen);
+	if (pos < 0)
+		pos = -pos - 1;
+	ce = active_cache[pos];
+	if (ce_namelen(ce) == namelen &&
+	    !memcmp(ce->name, filename, namelen))
+		die("Path '%s' is in the index, but not at stage %d.\n"
+		    "Did you mean ':%d:%s'?",
+		    filename, stage,
+		    ce_stage(ce), filename);
+
+	/* Confusion between relative and absolute filenames? */
+	fullnamelen = namelen + strlen(prefix);
+	fullname = xmalloc(fullnamelen + 1);
+	strcpy(fullname, prefix);
+	strcat(fullname, filename);
+	pos = cache_name_pos(fullname, fullnamelen);
+	if (pos < 0)
+		pos = -pos - 1;
+	ce = active_cache[pos];
+	if (ce_namelen(ce) == fullnamelen &&
+	    !memcmp(ce->name, fullname, fullnamelen))
+		die("Path '%s' is in the index, but not '%s'.\n"
+		    "Did you mean ':%d:%s'?",
+		    fullname, filename,
+		    ce_stage(ce), fullname);
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in the index.", filename);
+	if (errno == ENOENT || errno == ENOTDIR)
+		die("Path '%s' does not exist (neither on disk nor in the index).",
+		    filename);
+
+	free(fullname);
+}
+
+
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
@@ -850,6 +939,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 			}
 			pos++;
 		}
+		if (!gently)
+			diagnose_invalid_index_path(stage, prefix, cp);
 		return -1;
 	}
 	for (cp = name, bracket_depth = 0; *cp; cp++) {
@@ -862,9 +953,25 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 	}
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
-		if (!get_sha1_1(name, cp-name, tree_sha1))
-			return get_tree_entry(tree_sha1, cp+1, sha1,
-					      mode);
+		char *object_name;
+		if (!gently) {
+			object_name = xmalloc(cp-name+1);
+			strncpy(object_name, name, cp-name);
+			object_name[cp-name] = '\0';
+		}
+		if (!get_sha1_1(name, cp-name, tree_sha1)) {
+			const char *filename = cp+1;
+			ret = get_tree_entry(tree_sha1, filename, sha1, mode);
+			if (!gently) {
+				diagnose_invalid_sha1_path(prefix, filename,
+							   tree_sha1, object_name);
+				free(object_name);
+			}
+			return ret;
+		} else {
+			if (!gently)
+				die("Invalid object name '%s'.", object_name);
+		}
 	}
 	return ret;
 }
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
new file mode 100755
index 0000000..af721f9
--- /dev/null
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='test git rev-parse diagnosis for invalid argument'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+HASH_file=
+
+test_expect_success 'set up basic repo' '
+	echo one > file.txt &&
+	mkdir subdir &&
+	echo two > subdir/file.txt &&
+	echo three > subdir/file2.txt &&
+	git add . &&
+	git commit -m init &&
+	echo four > index-only.txt &&
+	git add index-only.txt &&
+	echo five > disk-only.txt
+'
+
+test_expect_success 'correct file objects' '
+	HASH_file=$(git rev-parse HEAD:file.txt) &&
+	git rev-parse HEAD:subdir/file.txt &&
+	git rev-parse :index-only.txt &&
+	(cd subdir &&
+	 git rev-parse HEAD:subdir/file2.txt &&
+	 test $HASH_file = $(git rev-parse HEAD:file.txt) &&
+	 test $HASH_file = $(git rev-parse :file.txt) &&
+	 test $HASH_file = $(git rev-parse :0:file.txt) )
+'
+
+test_expect_success 'incorrect revision id' '
+	test_must_fail git rev-parse foobar:file.txt 2>error &&
+	grep "Invalid object name '"'"'foobar'"'"'." error &&
+	test_must_fail git rev-parse foobar 2> error &&
+	grep "unknown revision or path not in the working tree." error
+'
+
+test_expect_success 'incorrect file in sha1:path' '
+	test_must_fail git rev-parse HEAD:nothing.txt 2> error &&
+	grep "fatal: Path '"'"'nothing.txt'"'"' does not exist in '"'"'HEAD'"'"'" error &&
+	test_must_fail git rev-parse HEAD:index-only.txt 2> error &&
+	grep "fatal: Path '"'"'index-only.txt'"'"' exists on disk, but not in '"'"'HEAD'"'"'." error &&
+	(cd subdir &&
+	 test_must_fail git rev-parse HEAD:file2.txt 2> error &&
+	 grep "Did you mean '"'"'HEAD:subdir/file2.txt'"'"'?" error )
+'
+
+test_expect_success 'incorrect file in :path and :N:path' '
+	test_must_fail git rev-parse :nothing.txt 2> error &&
+	grep "fatal: Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
+	test_must_fail git rev-parse :1:nothing.txt 2> error &&
+	grep "Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
+	test_must_fail git rev-parse :1:file.txt 2> error &&
+	grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	(cd subdir &&
+	 test_must_fail git rev-parse :1:file.txt 2> error &&
+	 grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	 test_must_fail git rev-parse :file2.txt 2> error &&
+	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error &&
+	 test_must_fail git rev-parse :2:file2.txt 2> error &&
+	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error) &&
+	test_must_fail git rev-parse :disk-only.txt 2> error &&
+	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
+'
+
+test_done
-- 
1.6.6.rc0.355.gcb53a.dirty

Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>>> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);
>>
>> Do I understand correctly that "fatal" here is the same as "!gently"
>> elsewhere in the API?
>
> It seems it is. I renamed it.

This was a pure question, not a suggestion to change it (we do name a
function do_foo_gently() when there is do_foo() that does the same but
reports errors more noisily, though).  I found the name "fatal" a bit
confusing as I at first couldn't tell if the caller was telling the
function that it already detected a "fatal" error (and telling the
function to report the fatalness) and didn't realize that the caller is
instead saying "if you find an error, treat it as a fatal one" until I
read it again.

I am Ok with the new "gently" name with the negative semantics as well (I
see no need to change it back to "error_is_fatal").

Thanks.

Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

The second round still had a few compilation warnings (turned to errors in
my environment) so I fixed them up somewhat.  I stopped before getting a
clean compile, though---you will still get:

  sha1_name.c: In function 'get_sha1_with_mode_1':
  sha1_name.c:956: error: 'object_name' may be used uninitialized in this function

even with this fix-up.

>> Instead of doing a leaky allocation, it may make sense to pass the tree
>> object name as <const char *, size_t> pair, and print it with "%.*s" in
>> the error reporting codepath.  After all, object_name is used only for
>> that purpose in diagnose_invalid_sha1_path(), no?
>
> matter of taste,...

I thought generally it is accepted that I had a better taste on this list?
;-)

-- squashed --

 cache.h     |    4 ++--
 sha1_name.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 952bd51..3f9ee86 100644
--- a/cache.h
+++ b/cache.h
@@ -702,11 +702,11 @@ static inline unsigned int hexval(unsigned char c)
 #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
+static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
 {
 	return get_sha1_with_mode_1(str, sha1, mode, 1, NULL);
 }
-extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/sha1_name.c b/sha1_name.c
index 1bc09ac..025244a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -807,7 +807,7 @@ int get_sha1(const char *name, unsigned char *sha1)
 /* Must be called only when object_name:filename doesn't exist. */
 static void diagnose_invalid_sha1_path(const char *prefix,
 				       const char *filename,
-				       const char *tree_sha1,
+				       const unsigned char *tree_sha1,
 				       const char *object_name)
 {
 	struct stat st;

Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> The second round still had a few compilation warnings (turned to errors in
> my environment) so I fixed them up somewhat.

Thanks. For some reasons, I had CFLAGS=-g and I didn't see the
warnings :-(

> I stopped before getting a
> clean compile, though---you will still get:
>
>   sha1_name.c: In function 'get_sha1_with_mode_1':
>   sha1_name.c:956: error: 'object_name' may be used uninitialized in this function

This one is strange. I don't git it with gcc 4.4, even with -Wall
-Werror. And indeed, object_name is initialized when !gently, which is
precisely when we use it.

I did

-               char *object_name;
+               char *object_name = NULL;

to make sure the warning goes away on your machine, but I don't
understand what's going on.

>  cache.h     |    4 ++--
>  sha1_name.c |    2 +-

Squashed into v3, together with initialisation of object_name, and two
s/int/unsigned/ to please gcc -Wextra.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v3] Detailed diagnosis when parsing an object name fails.

[Matthieu Moy]

The previous error message was the same in many situations (unknown
revision or path not in the working tree). We try to help the user as
much as possible to understand the error, especially with the
sha1:filename notation. In this case, we say whether the sha1 or the
filename is problematic, and diagnose the confusion between
relative-to-root and relative-to-$PWD confusion precisely.

The 7 new error messages are tested.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 cache.h                        |    6 ++-
 setup.c                        |   15 +++++-
 sha1_name.c                    |  115 ++++++++++++++++++++++++++++++++++++++--
 t/t1506-rev-parse-diagnosis.sh |   69 ++++++++++++++++++++++++
 4 files changed, 198 insertions(+), 7 deletions(-)
 create mode 100755 t/t1506-rev-parse-diagnosis.sh

diff --git a/cache.h b/cache.h
index 0e69384..c122bfa 100644
--- a/cache.h
+++ b/cache.h
@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
 #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
+static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+{
+	return get_sha1_with_mode_1(str, sha1, mode, 1, NULL);
+}
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/setup.c b/setup.c
index f67250b..5792eb7 100644
--- a/setup.c
+++ b/setup.c
@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
+static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+{
+	unsigned char sha1[20];
+	unsigned mode;
+	/* try a detailed diagnostic ... */
+	get_sha1_with_mode_1(arg, sha1, &mode, 0, prefix);
+	/* ... or fall back the most general message. */
+	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
+	    "Use '--' to separate paths from revisions", arg);
+
+}
+
 /*
  * Verify a filename that we got as an argument for a pathspec
  * entry. Note that a filename that begins with "-" never verifies
@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
-	    "Use '--' to separate paths from revisions", arg);
+	die_verify_filename(prefix, arg);
 }
 
 /*
diff --git a/sha1_name.c b/sha1_name.c
index 44bb62d..ca8f9db 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -804,7 +804,96 @@ int get_sha1(const char *name, unsigned char *sha1)
 	return get_sha1_with_mode(name, sha1, &unused);
 }
 
-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
+/* Must be called only when object_name:filename doesn't exist. */
+static void diagnose_invalid_sha1_path(const char *prefix,
+				       const char *filename,
+				       const unsigned char *tree_sha1,
+				       const char *object_name)
+{
+	struct stat st;
+	unsigned char sha1[20];
+	unsigned mode;
+
+	if (!prefix)
+		prefix = "";
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in '%s'.",
+		    filename, object_name);
+	if (errno == ENOENT || errno == ENOTDIR) {
+		char *fullname = xmalloc(strlen(filename)
+					     + strlen(prefix) + 1);
+		strcpy(fullname, prefix);
+		strcat(fullname, filename);
+
+		if (!get_tree_entry(tree_sha1, fullname,
+				    sha1, &mode)) {
+			die("Path '%s' exists, but not '%s'.\n"
+			    "Did you mean '%s:%s'?",
+			    fullname,
+			    filename,
+			    object_name,
+			    fullname);
+		}
+		die("Path '%s' does not exist in '%s'",
+		    filename, object_name);
+	}
+}
+
+/* Must be called only when :stage:filename doesn't exist. */
+static void diagnose_invalid_index_path(int stage,
+					const char *prefix,
+					const char *filename)
+{
+	struct stat st;
+	struct cache_entry *ce;
+	int pos;
+	unsigned namelen = strlen(filename);
+	unsigned fullnamelen;
+	char *fullname;
+
+	if (!prefix)
+		prefix = "";
+
+	/* Wrong stage number? */
+	pos = cache_name_pos(filename, namelen);
+	if (pos < 0)
+		pos = -pos - 1;
+	ce = active_cache[pos];
+	if (ce_namelen(ce) == namelen &&
+	    !memcmp(ce->name, filename, namelen))
+		die("Path '%s' is in the index, but not at stage %d.\n"
+		    "Did you mean ':%d:%s'?",
+		    filename, stage,
+		    ce_stage(ce), filename);
+
+	/* Confusion between relative and absolute filenames? */
+	fullnamelen = namelen + strlen(prefix);
+	fullname = xmalloc(fullnamelen + 1);
+	strcpy(fullname, prefix);
+	strcat(fullname, filename);
+	pos = cache_name_pos(fullname, fullnamelen);
+	if (pos < 0)
+		pos = -pos - 1;
+	ce = active_cache[pos];
+	if (ce_namelen(ce) == fullnamelen &&
+	    !memcmp(ce->name, fullname, fullnamelen))
+		die("Path '%s' is in the index, but not '%s'.\n"
+		    "Did you mean ':%d:%s'?",
+		    fullname, filename,
+		    ce_stage(ce), fullname);
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in the index.", filename);
+	if (errno == ENOENT || errno == ENOTDIR)
+		die("Path '%s' does not exist (neither on disk nor in the index).",
+		    filename);
+
+	free(fullname);
+}
+
+
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
@@ -850,6 +939,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 			}
 			pos++;
 		}
+		if (!gently)
+			diagnose_invalid_index_path(stage, prefix, cp);
 		return -1;
 	}
 	for (cp = name, bracket_depth = 0; *cp; cp++) {
@@ -862,9 +953,25 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 	}
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
-		if (!get_sha1_1(name, cp-name, tree_sha1))
-			return get_tree_entry(tree_sha1, cp+1, sha1,
-					      mode);
+		char *object_name = NULL;
+		if (!gently) {
+			object_name = xmalloc(cp-name+1);
+			strncpy(object_name, name, cp-name);
+			object_name[cp-name] = '\0';
+		}
+		if (!get_sha1_1(name, cp-name, tree_sha1)) {
+			const char *filename = cp+1;
+			ret = get_tree_entry(tree_sha1, filename, sha1, mode);
+			if (!gently) {
+				diagnose_invalid_sha1_path(prefix, filename,
+							   tree_sha1, object_name);
+				free(object_name);
+			}
+			return ret;
+		} else {
+			if (!gently)
+				die("Invalid object name '%s'.", object_name);
+		}
 	}
 	return ret;
 }
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
new file mode 100755
index 0000000..af721f9
--- /dev/null
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='test git rev-parse diagnosis for invalid argument'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+HASH_file=
+
+test_expect_success 'set up basic repo' '
+	echo one > file.txt &&
+	mkdir subdir &&
+	echo two > subdir/file.txt &&
+	echo three > subdir/file2.txt &&
+	git add . &&
+	git commit -m init &&
+	echo four > index-only.txt &&
+	git add index-only.txt &&
+	echo five > disk-only.txt
+'
+
+test_expect_success 'correct file objects' '
+	HASH_file=$(git rev-parse HEAD:file.txt) &&
+	git rev-parse HEAD:subdir/file.txt &&
+	git rev-parse :index-only.txt &&
+	(cd subdir &&
+	 git rev-parse HEAD:subdir/file2.txt &&
+	 test $HASH_file = $(git rev-parse HEAD:file.txt) &&
+	 test $HASH_file = $(git rev-parse :file.txt) &&
+	 test $HASH_file = $(git rev-parse :0:file.txt) )
+'
+
+test_expect_success 'incorrect revision id' '
+	test_must_fail git rev-parse foobar:file.txt 2>error &&
+	grep "Invalid object name '"'"'foobar'"'"'." error &&
+	test_must_fail git rev-parse foobar 2> error &&
+	grep "unknown revision or path not in the working tree." error
+'
+
+test_expect_success 'incorrect file in sha1:path' '
+	test_must_fail git rev-parse HEAD:nothing.txt 2> error &&
+	grep "fatal: Path '"'"'nothing.txt'"'"' does not exist in '"'"'HEAD'"'"'" error &&
+	test_must_fail git rev-parse HEAD:index-only.txt 2> error &&
+	grep "fatal: Path '"'"'index-only.txt'"'"' exists on disk, but not in '"'"'HEAD'"'"'." error &&
+	(cd subdir &&
+	 test_must_fail git rev-parse HEAD:file2.txt 2> error &&
+	 grep "Did you mean '"'"'HEAD:subdir/file2.txt'"'"'?" error )
+'
+
+test_expect_success 'incorrect file in :path and :N:path' '
+	test_must_fail git rev-parse :nothing.txt 2> error &&
+	grep "fatal: Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
+	test_must_fail git rev-parse :1:nothing.txt 2> error &&
+	grep "Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
+	test_must_fail git rev-parse :1:file.txt 2> error &&
+	grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	(cd subdir &&
+	 test_must_fail git rev-parse :1:file.txt 2> error &&
+	 grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	 test_must_fail git rev-parse :file2.txt 2> error &&
+	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error &&
+	 test_must_fail git rev-parse :2:file2.txt 2> error &&
+	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error) &&
+	test_must_fail git rev-parse :disk-only.txt 2> error &&
+	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
+'
+
+test_done
-- 
1.6.6.rc0.355.gcb53a.dirty

Re: [PATCH v3] Detailed diagnosis when parsing an object name fails.

[Sverre Rabbelier]

Heya,

On Mon, Dec 7, 2009 at 11:10, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> The previous error message was the same in many situations (unknown
> revision or path not in the working tree). We try to help the user as
> much as possible to understand the error, especially with the
> sha1:filename notation. In this case, we say whether the sha1 or the
> filename is problematic, and diagnose the confusion between
> relative-to-root and relative-to-$PWD confusion precisely.

Thank you, I just got helped by your patch, and was very pleasantly
surprised to see git being so unusually helpful! We'll make a
user-friendly scm out of git yet :).

-- 
Cheers,

Sverre Rabbelier