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

Junio C Hamano [off-list ref] 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)

quoted

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.

quoted

+	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.

quoted

+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.

quoted

@@ -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.

quoted

+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.

quoted

+	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/