{"thread":{"id":"27316","subject":"[PATCH] fix overstrict : diagnosis","startedAt":"2011-05-10T18:10:30Z","lastAt":"2011-05-10T23:11:22Z","messageCount":5,"participants":["Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"167608","messageId":"7vhb92jujt.fsf@alter.siamese.dyndns.org","threadId":"27316","inReplyTo":null,"subject":"[PATCH] fix overstrict : diagnosis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-10T18:10:30Z","receivedAt":"2011-05-10T18:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Given \"git log :\", we get a disambiguation message that tries to be\nhelpful and yet totally misses the point, i.e.\n\n $ git log :\n fatal: Path '' does not exist (neither on disk nor in the index).\n $ git log :/\n fatal: Path '/' exists on disk, but not in the index.\n\nAn empty path nor anything that begins with '/' cannot possibly in the\nindex, and it is wrong to guess that the user might have meant to access\nsuch an index entry.\n\nIt should yield the same error message as \"git log '*.c'\", i.e.\n\n $ git log '*.c'\n fatal: ambiguous argument '*.c': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions\n\nSigned-off-by: Junio C Hamano \n---\n\n * I actually think the message should say\n\n \tfatal: ambiguous argument '*.c': neither a rev nor a path.\n Use '--' to separate pathspec from revisions.\n\n for brevity, but that is a separate topic.\n\n sha1_name.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex faea58d..90d8bfa 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1173,7 +1173,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\t}\n \t\t\tpos++;\n \t\t}\n-\t\tif (!gently)\n+\t\tif (!gently && name[1] && name[1] != '/')\n \t\t\tdiagnose_invalid_index_path(stage, prefix, cp);\n \t\tfree(new_path);\n \t\treturn -1;\n"},{"id":"167610","messageId":"7vzkmuidk1.fsf@alter.siamese.dyndns.org","threadId":"27316","inReplyTo":"7vhb92jujt.fsf@alter.siamese.dyndns.org","subject":"[PATCH] fix overslow :/no-such-string-ever-existed diagnostics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-10T19:02:54Z","receivedAt":"2011-05-10T19:02:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git cmd :/no-such-string-ever-existed\" runs an extra round of get_sha1()\nsince 009fee4 (Detailed diagnosis when parsing an object name fails.,\n2009-12-07). Once without error diagnosis to see there is no commit with\nsuch a string in the log message (hence \"it cannot be a ref\"), and after\nseeing that :/no-such-string-ever-existed is not a filename (hence \"it\ncannot be a path, either\"), another time to give \"better diagnosis\".\n\nThe thing is, the second time it runs, we already know that traversing the\nhistory all the way down to the root will _not_ find any matching commit.\n\nRename misguided \"gently\" parameter, which is turned off _only_ when the\n\"detailed diagnosis\" codepath knows that it cannot be a ref and making the\ncall only for the caller to die with a message. Flip its meaning (and\nadjust the callers) and call it \"only_to_die\", which is not a great name,\nbut it describes far more clearly what the codepaths that switches their\nbehaviour based on this variable do.\n\nOn my box, the command spends ~1.8 seconds without the patch to make the\nreport; with the patch it spends ~1.12 seconds.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Yes, we often say \"this is an error path and paying the price in\n performance for a better diagnosis is worth\", and it is correct.\n But it is not an excuse to spend unnecessary cycles.\n\n We may want to add a guard at the beginning of die_verify_filename() to\n omit the extra call to get_sha1_with_mode_1(only_to_die=1) when arg\n looks like a magic pathspec, i.e. \":\" followed by anything !isalnum().\n\n cache.h | 8 ++++----\n setup.c | 2 +-\n sha1_name.c | 17 +++++++++--------\n 3 files changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex be6ce72..a9e6419 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -785,15 +785,15 @@ struct object_context {\n };\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n-extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);\n+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int only_to_die, const char *prefix);\n static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\n {\n-\treturn get_sha1_with_mode_1(str, sha1, mode, 1, NULL);\n+\treturn get_sha1_with_mode_1(str, sha1, mode, 0, NULL);\n }\n-extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int gently, const char *prefix);\n+extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int only_to_die, const char *prefix);\n static inline int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)\n {\n-\treturn get_sha1_with_context_1(str, sha1, orc, 1, NULL);\n+\treturn get_sha1_with_context_1(str, sha1, orc, 0, NULL);\n }\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\ndiff --git a/setup.c b/setup.c\nindex 5048252..50e8548 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -86,7 +86,7 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)\n \tunsigned char sha1[20];\n \tunsigned mode;\n \t/* try a detailed diagnostic ... */\n-\tget_sha1_with_mode_1(arg, sha1, &mode, 0, prefix);\n+\tget_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);\n \t/* ... or fall back the most general message. */\n \tdie(\"ambiguous argument '%s': unknown revision or path not in the working tree.\\n\"\n \t \"Use '--' to separate paths from revisions\", arg);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex faea58d..ec83611 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1080,11 +1080,12 @@ static void diagnose_invalid_index_path(int stage,\n }\n \n \n-int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)\n+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,\n+\t\t\t int only_to_die, const char *prefix)\n {\n \tstruct object_context oc;\n \tint ret;\n-\tret = get_sha1_with_context_1(name, sha1, &oc, gently, prefix);\n+\tret = get_sha1_with_context_1(name, sha1, &oc, only_to_die, prefix);\n \t*mode = oc.mode;\n \treturn ret;\n }\n@@ -1108,7 +1109,7 @@ static char *resolve_relative_path(const char *rel)\n \n int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\t struct object_context *oc,\n-\t\t\t int gently, const char *prefix)\n+\t\t\t int only_to_die, const char *prefix)\n {\n \tint ret, bracket_depth;\n \tint namelen = strlen(name);\n@@ -1130,7 +1131,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\tstruct cache_entry *ce;\n \t\tchar *new_path = NULL;\n \t\tint pos;\n-\t\tif (namelen > 2 && name[1] == '/') {\n+\t\tif (!only_to_die && namelen > 2 && name[1] == '/') {\n \t\t\tstruct commit_list *list = NULL;\n \t\t\tfor_each_ref(handle_one_ref, &list);\n \t\t\treturn get_sha1_oneline(name + 2, sha1, list);\n@@ -1173,7 +1174,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\t}\n \t\t\tpos++;\n \t\t}\n-\t\tif (!gently)\n+\t\tif (only_to_die && name[1] && name[1] != '/')\n \t\t\tdiagnose_invalid_index_path(stage, prefix, cp);\n \t\tfree(new_path);\n \t\treturn -1;\n@@ -1189,7 +1190,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \tif (*cp == ':') {\n \t\tunsigned char tree_sha1[20];\n \t\tchar *object_name = NULL;\n-\t\tif (!gently) {\n+\t\tif (only_to_die) {\n \t\t\tobject_name = xmalloc(cp-name+1);\n \t\t\tstrncpy(object_name, name, cp-name);\n \t\t\tobject_name[cp-name] = '\\0';\n@@ -1202,7 +1203,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\tif (new_filename)\n \t\t\t\tfilename = new_filename;\n \t\t\tret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);\n-\t\t\tif (!gently) {\n+\t\t\tif (only_to_die) {\n \t\t\t\tdiagnose_invalid_sha1_path(prefix, filename,\n \t\t\t\t\t\t\t tree_sha1, object_name);\n \t\t\t\tfree(object_name);\n@@ -1215,7 +1216,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\tfree(new_filename);\n \t\t\treturn ret;\n \t\t} else {\n-\t\t\tif (!gently)\n+\t\t\tif (only_to_die)\n \t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n \t\t}\n \t}\n \n"},{"id":"167611","messageId":"7vvcxiidgi.fsf_-_@alter.siamese.dyndns.org","threadId":"27316","inReplyTo":"7vzkmuidk1.fsf@alter.siamese.dyndns.org","subject":"Re* [PATCH] fix overslow :/no-such-string-ever-existed diagnostics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-10T19:05:01Z","receivedAt":"2011-05-10T19:05:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We may want to add a guard at the beginning of die_verify_filename() to\n> omit the extra call to get_sha1_with_mode_1(only_to_die=1) when arg\n> looks like a magic pathspec, i.e. \":\" followed by anything !isalnum().\n\n... which would look like this.\n\ndiff --git a/setup.c b/setup.c\nindex 50e8548..fd4ce59 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -85,8 +85,17 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)\n {\n \tunsigned char sha1[20];\n \tunsigned mode;\n-\t/* try a detailed diagnostic ... */\n-\tget_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);\n+\n+\t/*\n+\t * Saying \"'(icase)foo' does not exist in the index\" when the\n+\t * user gave us \":(icase)foo\" is just stupid. A magic pathspec\n+\t * begins with a colon and is followed by a non-alnum; do not\n+\t * let get_sha1_with_mode_1(only_to_die=1) to even trigger.\n+\t */\n+\tif (!(arg[0] == ':' && !isalnum(arg[1])))\n+\t\t/* try a detailed diagnostic ... */\n+\t\tget_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);\n+\n \t/* ... or fall back the most general message. */\n \tdie(\"ambiguous argument '%s': unknown revision or path not in the working tree.\\n\"\n \t \"Use '--' to separate paths from revisions\", arg);\n"},{"id":"167625","messageId":"vpqfwomffuy.fsf@bauges.imag.fr","threadId":"27316","inReplyTo":"7vzkmuidk1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix overslow :/no-such-string-ever-existed diagnostics","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-05-10T20:41:25Z","receivedAt":"2011-05-10T20:41:25Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> On my box, the command spends ~1.8 seconds without the patch to make the\n> report; with the patch it spends ~1.12 seconds.\n\nNice improvement.\n\n> * Yes, we often say \"this is an error path and paying the price in\n> performance for a better diagnosis is worth\", and it is correct.\n> But it is not an excuse to spend unnecessary cycles.\n\nRight. When I introduced the second call with gently=0, I was thinking\nof cases where the function is fast, but :/blah has to walk through\nhistory, and we don't want that twice (especially since the conclusion\nis to give generic and not-so-helpfull message!).\n\n> We may want to add a guard at the beginning of die_verify_filename() to\n> omit the extra call to get_sha1_with_mode_1(only_to_die=1) when arg\n> looks like a magic pathspec, i.e. \":\" followed by anything !isalnum().\n\nIs this a complement or an alternative? It seems to me that your other\npatch makes this first one useless (in the sense that the second call is\nalways cheap), and avoids complexifying the code of get_sha1_with_mode_1\nfor the first call.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"167645","messageId":"7v4o52i21x.fsf@alter.siamese.dyndns.org","threadId":"27316","inReplyTo":"vpqfwomffuy.fsf@bauges.imag.fr","subject":"Re: [PATCH] fix overslow :/no-such-string-ever-existed diagnostics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-10T23:11:22Z","receivedAt":"2011-05-10T23:11:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Is this a complement or an alternative? It seems to me that your other\n> patch makes this first one useless (in the sense that the second call is\n> always cheap), and avoids complexifying the code of get_sha1_with_mode_1\n> for the first call.\n\nI think that the get_sha1_with_mode_1() that is sprinkled with the\n\"gently\" conditional everywhere _is_ the primary source of complexity.\n\nI suspect that if 009fee4 did a proper refactoring, get_sha1_with_mode()\nwouldn't have that \"gently\" option at all, and the die_verify_filename()\nwouldn't call get_sha1_with_mode(). Instead, they would share same set of\nhelper functions and the latter would make a few calls to them, only to\ndeal with : and : cases.\n"}]}