[PATCH] fix overstrict :<path> diagnosis

[PATCH] fix overstrict :<path> diagnosis

[Junio C Hamano]

Given "git log :", we get a disambiguation message that tries to be
helpful and yet totally misses the point, i.e.

    $ git log :
    fatal: Path '' does not exist (neither on disk nor in the index).
    $ git log :/
    fatal: Path '/' exists on disk, but not in the index.

An empty path nor anything that begins with '/' cannot possibly in the
index, and it is wrong to guess that the user might have meant to access
such an index entry.

It should yield the same error message as "git log '*.c'", i.e.

    $ git log '*.c'
    fatal: ambiguous argument '*.c': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I actually think the message should say

 	fatal: ambiguous argument '*.c': neither a rev nor a path.
        Use '--' to separate pathspec from revisions.

   for brevity, but that is a separate topic.

 sha1_name.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index faea58d..90d8bfa 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1173,7 +1173,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			}
 			pos++;
 		}
-		if (!gently)
+		if (!gently && name[1] && name[1] != '/')
 			diagnose_invalid_index_path(stage, prefix, cp);
 		free(new_path);
 		return -1;

[PATCH] fix overslow :/no-such-string-ever-existed diagnostics

[Junio C Hamano]

"git cmd :/no-such-string-ever-existed" runs an extra round of get_sha1()
since 009fee4 (Detailed diagnosis when parsing an object name fails.,
2009-12-07).  Once without error diagnosis to see there is no commit with
such a string in the log message (hence "it cannot be a ref"), and after
seeing that :/no-such-string-ever-existed is not a filename (hence "it
cannot be a path, either"), another time to give "better diagnosis".

The thing is, the second time it runs, we already know that traversing the
history all the way down to the root will _not_ find any matching commit.

Rename misguided "gently" parameter, which is turned off _only_ when the
"detailed diagnosis" codepath knows that it cannot be a ref and making the
call only for the caller to die with a message.  Flip its meaning (and
adjust the callers) and call it "only_to_die", which is not a great name,
but it describes far more clearly what the codepaths that switches their
behaviour based on this variable do.

On my box, the command spends ~1.8 seconds without the patch to make the
report; with the patch it spends ~1.12 seconds.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Yes, we often say "this is an error path and paying the price in
   performance for a better diagnosis is worth", and it is correct.
   But it is not an excuse to spend unnecessary cycles.

   We may want to add a guard at the beginning of die_verify_filename() to
   omit the extra call to get_sha1_with_mode_1(only_to_die=1) when arg
   looks like a magic pathspec, i.e. ":" followed by anything !isalnum().

 cache.h     |    8 ++++----
 setup.c     |    2 +-
 sha1_name.c |   17 +++++++++--------
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index be6ce72..a9e6419 100644
--- a/cache.h
+++ b/cache.h
@@ -785,15 +785,15 @@ struct object_context {
 };
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int only_to_die, 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);
+	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
 }
-extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int gently, const char *prefix);
+extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int only_to_die, const char *prefix);
 static inline int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)
 {
-	return get_sha1_with_context_1(str, sha1, orc, 1, NULL);
+	return get_sha1_with_context_1(str, sha1, orc, 0, NULL);
 }
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
diff --git a/setup.c b/setup.c
index 5048252..50e8548 100644
--- a/setup.c
+++ b/setup.c
@@ -86,7 +86,7 @@ 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);
+	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);
diff --git a/sha1_name.c b/sha1_name.c
index faea58d..ec83611 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1080,11 +1080,12 @@ static void diagnose_invalid_index_path(int stage,
 }
 
 
-int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
+			 int only_to_die, const char *prefix)
 {
 	struct object_context oc;
 	int ret;
-	ret = get_sha1_with_context_1(name, sha1, &oc, gently, prefix);
+	ret = get_sha1_with_context_1(name, sha1, &oc, only_to_die, prefix);
 	*mode = oc.mode;
 	return ret;
 }
@@ -1108,7 +1109,7 @@ static char *resolve_relative_path(const char *rel)
 
 int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			    struct object_context *oc,
-			    int gently, const char *prefix)
+			    int only_to_die, const char *prefix)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
@@ -1130,7 +1131,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 		struct cache_entry *ce;
 		char *new_path = NULL;
 		int pos;
-		if (namelen > 2 && name[1] == '/') {
+		if (!only_to_die && namelen > 2 && name[1] == '/') {
 			struct commit_list *list = NULL;
 			for_each_ref(handle_one_ref, &list);
 			return get_sha1_oneline(name + 2, sha1, list);
@@ -1173,7 +1174,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			}
 			pos++;
 		}
-		if (!gently)
+		if (only_to_die && name[1] && name[1] != '/')
 			diagnose_invalid_index_path(stage, prefix, cp);
 		free(new_path);
 		return -1;
@@ -1189,7 +1190,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
 		char *object_name = NULL;
-		if (!gently) {
+		if (only_to_die) {
 			object_name = xmalloc(cp-name+1);
 			strncpy(object_name, name, cp-name);
 			object_name[cp-name] = '\0';
@@ -1202,7 +1203,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			if (new_filename)
 				filename = new_filename;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
-			if (!gently) {
+			if (only_to_die) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);
@@ -1215,7 +1216,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			free(new_filename);
 			return ret;
 		} else {
-			if (!gently)
+			if (only_to_die)
 				die("Invalid object name '%s'.", object_name);
 		}
 	}
   

Re* [PATCH] fix overslow :/no-such-string-ever-existed diagnostics

[Junio C Hamano]

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

>    We may want to add a guard at the beginning of die_verify_filename() to
>    omit the extra call to get_sha1_with_mode_1(only_to_die=1) when arg
>    looks like a magic pathspec, i.e. ":" followed by anything !isalnum().

... which would look like this.

diff --git a/setup.c b/setup.c
index 50e8548..fd4ce59 100644
--- a/setup.c
+++ b/setup.c
@@ -85,8 +85,17 @@ 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);
+
+	/*
+	 * Saying "'(icase)foo' does not exist in the index" when the
+	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec
+	 * begins with a colon and is followed by a non-alnum; do not
+	 * let get_sha1_with_mode_1(only_to_die=1) to even trigger.
+	 */
+	if (!(arg[0] == ':' && !isalnum(arg[1])))
+		/* 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);

Re: [PATCH] fix overslow :/no-such-string-ever-existed diagnostics

[Matthieu Moy]

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

> On my box, the command spends ~1.8 seconds without the patch to make the
> report; with the patch it spends ~1.12 seconds.

Nice improvement.

>  * Yes, we often say "this is an error path and paying the price in
>    performance for a better diagnosis is worth", and it is correct.
>    But it is not an excuse to spend unnecessary cycles.

Right. When I introduced the second call with gently=0, I was thinking
of cases where the function is fast, but :/blah has to walk through
history, and we don't want that twice (especially since the conclusion
is to give generic and not-so-helpfull message!).

>    We may want to add a guard at the beginning of die_verify_filename() to
>    omit the extra call to get_sha1_with_mode_1(only_to_die=1) when arg
>    looks like a magic pathspec, i.e. ":" followed by anything !isalnum().

Is this a complement or an alternative? It seems to me that your other
patch makes this first one useless (in the sense that the second call is
always cheap), and avoids complexifying the code of get_sha1_with_mode_1
for the first call.

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

Re: [PATCH] fix overslow :/no-such-string-ever-existed diagnostics

[Junio C Hamano]

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

> Is this a complement or an alternative? It seems to me that your other
> patch makes this first one useless (in the sense that the second call is
> always cheap), and avoids complexifying the code of get_sha1_with_mode_1
> for the first call.

I think that the get_sha1_with_mode_1() that is sprinkled with the
"gently" conditional everywhere _is_ the primary source of complexity.

I suspect that if 009fee4 did a proper refactoring, get_sha1_with_mode()
wouldn't have that "gently" option at all, and the die_verify_filename()
wouldn't call get_sha1_with_mode().  Instead, they would share same set of
helper functions and the latter would make a few calls to them, only to
deal with :<path> and <tree>:<path> cases.