{"thread":{"id":"21849","subject":"Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.","startedAt":"2009-12-02T20:01:01Z","lastAt":"2010-02-15T14:16:05Z","messageCount":9,"participants":["Junio C Hamano","Matthieu Moy","Sverre Rabbelier","y@imag.fr"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"299218","messageId":"1259784061-25143-1-git-send-email-y","threadId":"21849","inReplyTo":null,"subject":"[PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"","fromEmail":"y@imag.fr","sentAt":"2009-12-02T20:01:01Z","receivedAt":"2009-12-02T20:01:01Z","isPatch":true,"sender":{"key":"y@imag.fr","avatar":null},"body":"From: Matthieu Moy <Matthieu.Moy@imag.fr>\n\nThe previous error message was the same in many situations (unknown\nrevision or path not in the working tree). We try to help the user as\nmuch as possible to understand the error, especially with the\nsha1:filename notation. In this case, we say whether the sha1 or the\nfilename is problematic, and diagnose the confusion between\nrelative-to-root and relative-to-$PWD confusion precisely.\n\nThe 6 new error messages are tested.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nChanges since v1:\n\n* Fixed a segfault with\n\n+\tif (!prefix)\n+\t\tprefix = \"\";\n\n* Added testcases.\n\n cache.h                        |    6 ++-\n setup.c                        |   15 +++++-\n sha1_name.c                    |   95 ++++++++++++++++++++++++++++++++++++++--\n t/t1506-rev-parse-diagnosis.sh |   67 ++++++++++++++++++++++++++++\n 4 files changed, 176 insertions(+), 7 deletions(-)\n create mode 100755 t/t1506-rev-parse-diagnosis.sh\n\ndiff --git a/cache.h b/cache.h\nindex 0e69384..5c8cb5f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)\n #define DEFAULT_ABBREV 7\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);\n+static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\n+{\n+\treturn get_sha1_with_mode_1(str, sha1, mode, 0, NULL);\n+}\n+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);\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! */\n extern int read_ref(const char *filename, unsigned char *sha1);\ndiff --git a/setup.c b/setup.c\nindex f67250b..3094e8b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)\n \tdie_errno(\"failed to stat '%s'\", arg);\n }\n \n+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+\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+\n+}\n+\n /*\n  * Verify a filename that we got as an argument for a pathspec\n  * entry. Note that a filename that begins with \"-\" never verifies\n@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)\n \t\tdie(\"bad flag '%s' used after filename\", arg);\n \tif (check_filename(prefix, arg))\n \t\treturn;\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+\tdie_verify_filename(prefix, arg);\n }\n \n /*\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 44bb62d..030e2ac 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -804,7 +804,77 @@ int get_sha1(const char *name, unsigned char *sha1)\n \treturn get_sha1_with_mode(name, sha1, &unused);\n }\n \n-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n+static void diagnose_invalid_sha1_path(const char *prefix,\n+\t\t\t\t       const char *filename,\n+\t\t\t\t       const char *tree_sha1,\n+\t\t\t\t       const char *object_name)\n+{\n+\tstruct stat st;\n+\tunsigned char sha1[20];\n+\tunsigned mode;\n+\n+\tif (!prefix)\n+\t\tprefix = \"\";\n+\n+\tif (!lstat(filename, &st))\n+\t\tdie(\"Path '%s' exists on disk, but not in '%s'.\",\n+\t\t    filename, object_name);\n+\tif (errno == ENOENT || errno == ENOTDIR) {\n+\t\tchar *fullname = malloc(strlen(filename)\n+\t\t\t\t\t     + strlen(prefix) + 1);\n+\t\tstrcpy(fullname, prefix);\n+\t\tstrcat(fullname, filename);\n+\n+\t\tif (!get_tree_entry(tree_sha1, fullname,\n+\t\t\t\t    sha1, &mode)) {\n+\t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n+\t\t\t    \"Did you mean '%s:%s'?\",\n+\t\t\t    fullname,\n+\t\t\t    filename,\n+\t\t\t    object_name,\n+\t\t\t    fullname);\n+\t\t}\n+\t\tdie(\"Path '%s' does not exist in '%s'\",\n+\t\t    filename, object_name);\n+\t}\n+}\n+\n+static void diagnose_invalid_index_path(int stage,\n+\t\t\t\t\tconst char *prefix,\n+\t\t\t\t\tconst char *filename)\n+{\n+\tstruct stat st;\n+\n+\tif (!prefix)\n+\t\tprefix = \"\";\n+\n+\tif (!lstat(filename, &st))\n+\t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n+\tif (errno == ENOENT || errno == ENOTDIR) {\n+\t\tstruct cache_entry *ce;\n+\t\tint pos;\n+\t\tint namelen = strlen(filename) + strlen(prefix);\n+\t\tchar *fullname = malloc(namelen + 1);\n+\t\tstrcpy(fullname, prefix);\n+\t\tstrcat(fullname, filename);\n+\t\tpos = cache_name_pos(fullname, namelen);\n+\t\tif (pos < 0)\n+\t\t\tpos = -pos - 1;\n+\t\tce = active_cache[pos];\n+\t\tif (ce_namelen(ce) == namelen &&\n+\t\t    !memcmp(ce->name, fullname, namelen))\n+\t\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n+\t\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t\t    fullname, filename,\n+\t\t\t    stage, fullname);\n+\n+\t\tdie(\"Path '%s' does not exist (neither on disk nor in the index).\",\n+\t\t    filename);\n+\t}\n+}\n+\n+\n+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix)\n {\n \tint ret, bracket_depth;\n \tint namelen = strlen(name);\n@@ -850,6 +920,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t\t\t}\n \t\t\tpos++;\n \t\t}\n+\t\tif (fatal)\n+\t\t\tdiagnose_invalid_index_path(stage, prefix, cp);\n \t\treturn -1;\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n@@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t}\n \tif (*cp == ':') {\n \t\tunsigned char tree_sha1[20];\n-\t\tif (!get_sha1_1(name, cp-name, tree_sha1))\n-\t\t\treturn get_tree_entry(tree_sha1, cp+1, sha1,\n-\t\t\t\t\t      mode);\n+\t\tchar *object_name;\n+\t\tif (fatal) {\n+\t\t\tobject_name = malloc(cp-name+1);\n+\t\t\tstrncpy(object_name, name, cp-name);\n+\t\t\tobject_name[cp-name] = '\\0';\n+\t\t}\n+\t\tif (!get_sha1_1(name, cp-name, tree_sha1)) {\n+\t\t\tconst char *filename = cp+1;\n+\t\t\tret = get_tree_entry(tree_sha1, filename, sha1, mode);\n+\t\t\tif (fatal)\n+\t\t\t\tdiagnose_invalid_sha1_path(prefix, filename,\n+\t\t\t\t\t\t\t   tree_sha1, object_name);\n+\n+\t\t\treturn ret;\n+\t\t} else {\n+\t\t\tif (fatal)\n+\t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n+\t\t}\n \t}\n \treturn ret;\n }\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nnew file mode 100755\nindex 0000000..8112d56\n--- /dev/null\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -0,0 +1,67 @@\n+#!/bin/sh\n+\n+test_description='test git rev-parse diagnosis for invalid argument'\n+\n+exec </dev/null\n+\n+. ./test-lib.sh\n+\n+HASH_file=\n+\n+test_expect_success 'set up basic repo' '\n+\techo one > file.txt &&\n+\tmkdir subdir &&\n+\techo two > subdir/file.txt &&\n+\techo three > subdir/file2.txt &&\n+\tgit add . &&\n+\tgit commit -m init &&\n+\techo four > index-only.txt &&\n+\tgit add index-only.txt &&\n+\techo five > disk-only.txt\n+'\n+\n+test_expect_success 'correct file objects' '\n+\tHASH_file=$(git rev-parse HEAD:file.txt) &&\n+\tgit rev-parse HEAD:subdir/file.txt &&\n+\tgit rev-parse :index-only.txt &&\n+\tcd subdir &&\n+\tgit rev-parse HEAD:file.txt &&\n+\tgit rev-parse HEAD:subdir/file2.txt &&\n+\ttest $HASH_file = $(git rev-parse HEAD:file.txt) &&\n+\ttest $HASH_file = $(git rev-parse :file.txt) &&\n+\ttest $HASH_file = $(git rev-parse :0:file.txt) &&\n+\tcd ..\n+'\n+\n+test_expect_success 'incorrect revision id' '\n+\ttest_must_fail git rev-parse foobar:file.txt 2>&1 |\n+\t\tgrep \"Invalid object name '\"'\"'foobar'\"'\"'.\" &&\n+\ttest_must_fail git rev-parse foobar 2>&1 |\n+\t\tgrep \"unknown revision or path not in the working tree.\"\n+'\n+\n+test_expect_success 'incorrect file in sha1:path' '\n+\ttest_must_fail git rev-parse HEAD:nothing.txt 2>&1 |\n+\t\tgrep \"fatal: Path '\"'\"'nothing.txt'\"'\"' does not exist in '\"'\"'HEAD'\"'\"'\" &&\n+\ttest_must_fail git rev-parse HEAD:index-only.txt 2>&1 |\n+\t\tgrep \"fatal: Path '\"'\"'index-only.txt'\"'\"' exists on disk, but not in '\"'\"'HEAD'\"'\"'.\" &&\n+\tcd subdir &&\n+\ttest_must_fail git rev-parse HEAD:file2.txt 2>&1 |\n+\t\tgrep \"Did you mean '\"'\"'HEAD:subdir/file2.txt'\"'\"'?\" &&\n+\tcd ..\n+'\n+\n+test_expect_success 'incorrect file in :path and :0:path' '\n+\ttest_must_fail git rev-parse :nothing.txt 2>&1 |\n+\t\tgrep \"fatal: Path '\"'\"'nothing.txt'\"'\"' does not exist (neither on disk nor in the index).\" &&\n+\ttest_must_fail git rev-parse :1:nothing.txt 2>&1 |\n+\t\tgrep \"Path '\"'\"'nothing.txt'\"'\"' does not exist (neither on disk nor in the index).\" &&\n+\tcd subdir &&\n+\ttest_must_fail git rev-parse :file2.txt 2>&1 |\n+\t\tgrep \"Did you mean '\"'\"':0:subdir/file2.txt'\"'\"'?\" &&\n+\tcd .. &&\n+\ttest_must_fail git rev-parse :disk-only.txt 2>&1 |\n+\t\tgrep \"fatal: Path '\"'\"'disk-only.txt'\"'\"' exists on disk, but not in the index.\"\n+'\n+\n+test_done\n-- \n1.6.6.rc0.256.g6060\n\n"},{"id":"129142","messageId":"7vljhj4wv0.fsf@alter.siamese.dyndns.org","threadId":"21849","inReplyTo":"1259784061-25143-1-git-send-email-y","subject":"Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-03T20:37:55Z","receivedAt":"2009-12-03T20:37:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"y@imag.fr writes:\n\n> diff --git a/cache.h b/cache.h\n> index 0e69384..5c8cb5f 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)\n>  #define DEFAULT_ABBREV 7\n>  \n>  extern int get_sha1(const char *str, unsigned char *sha1);\n> -extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);\n> +static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\n> +{\n> +\treturn get_sha1_with_mode_1(str, sha1, mode, 0, NULL);\n> +}\n> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);\n\nDo I understand correctly that \"fatal\" here is the same as \"!gently\"\nelsewhere in the API?\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 44bb62d..030e2ac 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -804,7 +804,77 @@ int get_sha1(const char *name, unsigned char *sha1)\n>  \treturn get_sha1_with_mode(name, sha1, &unused);\n>  }\n>  \n> -int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n> +static void diagnose_invalid_sha1_path(const char *prefix,\n> +\t\t\t\t       const char *filename,\n> +\t\t\t\t       const char *tree_sha1,\n> +\t\t\t\t       const char *object_name)\n> +{\n> +\tstruct stat st;\n> +\tunsigned char sha1[20];\n> +\tunsigned mode;\n> +\n> +\tif (!prefix)\n> +\t\tprefix = \"\";\n> +\n> +\tif (!lstat(filename, &st))\n> +\t\tdie(\"Path '%s' exists on disk, but not in '%s'.\",\n> +\t\t    filename, object_name);\n> +\tif (errno == ENOENT || errno == ENOTDIR) {\n> +\t\tchar *fullname = malloc(strlen(filename)\n> +\t\t\t\t\t     + strlen(prefix) + 1);\n> +\t\tstrcpy(fullname, prefix);\n> +\t\tstrcat(fullname, filename);\n\nWhat if malloc fails here (and elsewhere in your patch)?\n\n> +\t\tif (!get_tree_entry(tree_sha1, fullname,\n> +\t\t\t\t    sha1, &mode)) {\n> +\t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n> +\t\t\t    \"Did you mean '%s:%s'?\",\n> +\t\t\t    fullname,\n> +\t\t\t    filename,\n> +\t\t\t    object_name,\n> +\t\t\t    fullname);\n> +\t\t}\n> +\t\tdie(\"Path '%s' does not exist in '%s'\",\n> +\t\t    filename, object_name);\n> +\t}\n> +}\n> +\n> +static void diagnose_invalid_index_path(int stage,\n> +\t\t\t\t\tconst char *prefix,\n> +\t\t\t\t\tconst char *filename)\n> +{\n> +\tstruct stat st;\n> +\n> +\tif (!prefix)\n> +\t\tprefix = \"\";\n> +\n> +\tif (!lstat(filename, &st))\n> +\t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n> +\tif (errno == ENOENT || errno == ENOTDIR) {\n> +\t\tstruct cache_entry *ce;\n> +\t\tint pos;\n> +\t\tint namelen = strlen(filename) + strlen(prefix);\n> +\t\tchar *fullname = malloc(namelen + 1);\n> +\t\tstrcpy(fullname, prefix);\n> +\t\tstrcat(fullname, filename);\n> +\t\tpos = cache_name_pos(fullname, namelen);\n> +\t\tif (pos < 0)\n> +\t\t\tpos = -pos - 1;\n> +\t\tce = active_cache[pos];\n> +\t\tif (ce_namelen(ce) == namelen &&\n> +\t\t    !memcmp(ce->name, fullname, namelen))\n> +\t\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n> +\t\t\t    \"Did you mean ':%d:%s'?\",\n> +\t\t\t    fullname, filename,\n> +\t\t\t    stage, fullname);\n\nWhat happens if the user asked for \":2:Makefile\" while in directory \"t/\",\nand there is \":1:t/Makefile\" but not \":2:t/Makefile\" in the index?\n\nWhat should happen if the user asked for \":2:t/Makefile\" in such a case?\n\n> @@ -850,6 +920,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n>  \t\t\t}\n>  \t\t\tpos++;\n>  \t\t}\n> +\t\tif (fatal)\n> +\t\t\tdiagnose_invalid_index_path(stage, prefix, cp);\n>  \t\treturn -1;\n>  \t}\n>  \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n> @@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n>  \t}\n>  \tif (*cp == ':') {\n>  \t\tunsigned char tree_sha1[20];\n> -\t\tif (!get_sha1_1(name, cp-name, tree_sha1))\n> -\t\t\treturn get_tree_entry(tree_sha1, cp+1, sha1,\n> -\t\t\t\t\t      mode);\n> +\t\tchar *object_name;\n> +\t\tif (fatal) {\n> +\t\t\tobject_name = malloc(cp-name+1);\n\nWhere is this freed?\n\nInstead of doing a leaky allocation, it may make sense to pass the tree\nobject name as <const char *, size_t> pair, and print it with \"%.*s\" in\nthe error reporting codepath.  After all, object_name is used only for\nthat purpose in diagnose_invalid_sha1_path(), no?\n\n> +\t\t\tstrncpy(object_name, name, cp-name);\n> +\t\t\tobject_name[cp-name] = '\\0';\n> +\t\t}\n> +\t\tif (!get_sha1_1(name, cp-name, tree_sha1)) {\n> +\t\t\tconst char *filename = cp+1;\n> +\t\t\tret = get_tree_entry(tree_sha1, filename, sha1, mode);\n> +\t\t\tif (fatal)\n> +\t\t\t\tdiagnose_invalid_sha1_path(prefix, filename,\n> +\t\t\t\t\t\t\t   tree_sha1, object_name);\n> +\n> +\t\t\treturn ret;\n> +\t\t} else {\n> +\t\t\tif (fatal)\n> +\t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n> +\t\t}\n>  \t}\n>  \treturn ret;\n>  }\n> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\n> new file mode 100755\n> index 0000000..8112d56\n> --- /dev/null\n> +++ b/t/t1506-rev-parse-diagnosis.sh\n> @@ -0,0 +1,67 @@\n> +#!/bin/sh\n> +\n> +test_description='test git rev-parse diagnosis for invalid argument'\n> +\n> +exec </dev/null\n> +\n> +. ./test-lib.sh\n> +\n> +HASH_file=\n> +\n> +test_expect_success 'set up basic repo' '\n> +\techo one > file.txt &&\n> +\tmkdir subdir &&\n> +\techo two > subdir/file.txt &&\n> +\techo three > subdir/file2.txt &&\n> +\tgit add . &&\n> +\tgit commit -m init &&\n> +\techo four > index-only.txt &&\n> +\tgit add index-only.txt &&\n> +\techo five > disk-only.txt\n> +'\n> +\n> +test_expect_success 'correct file objects' '\n> +\tHASH_file=$(git rev-parse HEAD:file.txt) &&\n> +\tgit rev-parse HEAD:subdir/file.txt &&\n> +\tgit rev-parse :index-only.txt &&\n> +\tcd subdir &&\n> +\tgit rev-parse HEAD:file.txt &&\n> +\tgit rev-parse HEAD:subdir/file2.txt &&\n> +\ttest $HASH_file = $(git rev-parse HEAD:file.txt) &&\n> +\ttest $HASH_file = $(git rev-parse :file.txt) &&\n> +\ttest $HASH_file = $(git rev-parse :0:file.txt) &&\n> +\tcd ..\n> +'\n\nPlease make it a habit of not doing \"cd\" without forcing a subprocess\nusing ().  If 'rev-parse HEAD:file.txt' fails after \"cd subdir\", the next\ntest will start running from that directory.\n\n> +test_expect_success 'incorrect revision id' '\n> +\ttest_must_fail git rev-parse foobar:file.txt 2>&1 |\n> +\t\tgrep \"Invalid object name '\"'\"'foobar'\"'\"'.\" &&\n\nIt always is better to write this in separate steps, because exit status\nof the upstream of pipe is discarded by the shell.\n\nIf you expect an error exit and want to make sure a particular error\nmessage is given, do this:\n\n\ttest_must_fail git rev-parse foobar:file.txt 2>error &&\n        grep \"Invalid ...\" error \n\nIf you expect an error exit and want to make sure an incorrect error\nmessage is not produced, do this:\n\n\ttest_must_fail git rev-parse foobar:file.txt 2>error &&\n        ! grep \"Invalid ...\" error \n"},{"id":"129351","messageId":"vpqhbs4dkjr.fsf@bauges.imag.fr","threadId":"21849","inReplyTo":"7vljhj4wv0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2009-12-06T18:28:08Z","receivedAt":"2009-12-06T18:28:08Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> y@imag.fr writes:\n\n(sorry for the bad email, my fingers tried to anwser \"yes\" when\ngit-send-email asked me to confirm my address)\n\n>> diff --git a/cache.h b/cache.h\n>> index 0e69384..5c8cb5f 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)\n>>  #define DEFAULT_ABBREV 7\n>>  \n>>  extern int get_sha1(const char *str, unsigned char *sha1);\n>> -extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);\n>> +static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\n>> +{\n>> +\treturn get_sha1_with_mode_1(str, sha1, mode, 0, NULL);\n>> +}\n>> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);\n>\n> Do I understand correctly that \"fatal\" here is the same as \"!gently\"\n> elsewhere in the API?\n\nIt seems it is. I renamed it.\n\n>> +\tif (errno == ENOENT || errno == ENOTDIR) {\n>> +\t\tchar *fullname = malloc(strlen(filename)\n>> +\t\t\t\t\t     + strlen(prefix) + 1);\n>> +\t\tstrcpy(fullname, prefix);\n>> +\t\tstrcat(fullname, filename);\n>\n> What if malloc fails here (and elsewhere in your patch)?\n\nShould have been an xmalloc. Fixed.\n\n>> +static void diagnose_invalid_index_path(int stage,\n>> +\t\t\t\t\tconst char *prefix,\n>> +\t\t\t\t\tconst char *filename)\n>> +{\n>> +\tstruct stat st;\n>> +\n>> +\tif (!prefix)\n>> +\t\tprefix = \"\";\n>> +\n>> +\tif (!lstat(filename, &st))\n>> +\t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n>> +\tif (errno == ENOENT || errno == ENOTDIR) {\n>> +\t\tstruct cache_entry *ce;\n>> +\t\tint pos;\n>> +\t\tint namelen = strlen(filename) + strlen(prefix);\n>> +\t\tchar *fullname = malloc(namelen + 1);\n>> +\t\tstrcpy(fullname, prefix);\n>> +\t\tstrcat(fullname, filename);\n>> +\t\tpos = cache_name_pos(fullname, namelen);\n>> +\t\tif (pos < 0)\n>> +\t\t\tpos = -pos - 1;\n>> +\t\tce = active_cache[pos];\n>> +\t\tif (ce_namelen(ce) == namelen &&\n>> +\t\t    !memcmp(ce->name, fullname, namelen))\n>> +\t\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n>> +\t\t\t    \"Did you mean ':%d:%s'?\",\n>> +\t\t\t    fullname, filename,\n>> +\t\t\t    stage, fullname);\n                            ^^^^^\nThis one should have been ce_stage(ce), otherwise, we\nsuggest :0:t/Makefile as you probably guessed:\n\n> What happens if the user asked for \":2:Makefile\" while in directory \"t/\",\n> and there is \":1:t/Makefile\" but not \":2:t/Makefile\" in the index?\n>\n> What should happen if the user asked for \":2:t/Makefile\" in such a\n> case?\n\nI originally didn't handle the case \"it's in the index, but not at the\nstage you requested\" (since most users asking :N:blah will be fluent\nenough in Git not to need a special case of error messages). But I\nfinally implemented this test.\n\n>> @@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n>>  \t}\n>>  \tif (*cp == ':') {\n>>  \t\tunsigned char tree_sha1[20];\n>> -\t\tif (!get_sha1_1(name, cp-name, tree_sha1))\n>> -\t\t\treturn get_tree_entry(tree_sha1, cp+1, sha1,\n>> -\t\t\t\t\t      mode);\n>> +\t\tchar *object_name;\n>> +\t\tif (fatal) {\n>> +\t\t\tobject_name = malloc(cp-name+1);\n>\n> Where is this freed?\n\nNowhere. In the present state, it's not serious since all codepath\ngoing through this malloc reach a die() right after. But a free()\ndoesn't harm, I've added it.\n\n> Instead of doing a leaky allocation, it may make sense to pass the tree\n> object name as <const char *, size_t> pair, and print it with \"%.*s\" in\n> the error reporting codepath.  After all, object_name is used only for\n> that purpose in diagnose_invalid_sha1_path(), no?\n\nmatter of taste, but I prefer doing one \"complicated\" thing (copying\nthe string) once, and avoid having to deal with two variables instead\nof one later.\n\n>> +test_expect_success 'correct file objects' '\n>> +\tHASH_file=$(git rev-parse HEAD:file.txt) &&\n>> +\tgit rev-parse HEAD:subdir/file.txt &&\n>> +\tgit rev-parse :index-only.txt &&\n>> +\tcd subdir &&\n>> +\tgit rev-parse HEAD:file.txt &&\n>> +\tgit rev-parse HEAD:subdir/file2.txt &&\n>> +\ttest $HASH_file = $(git rev-parse HEAD:file.txt) &&\n>> +\ttest $HASH_file = $(git rev-parse :file.txt) &&\n>> +\ttest $HASH_file = $(git rev-parse :0:file.txt) &&\n>> +\tcd ..\n>> +'\n>\n> Please make it a habit of not doing \"cd\" without forcing a subprocess\n> using ().\n\nFixed.\n\n>> +\ttest_must_fail git rev-parse foobar:file.txt 2>&1 |\n>> +\t\tgrep \"Invalid object name '\"'\"'foobar'\"'\"'.\" &&\n>\n> It always is better to write this in separate steps, because exit status\n> of the upstream of pipe is discarded by the shell.\n\nFixed.\n\nI'll resubmit soon.\n\n--\nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"129353","messageId":"1260125128-17857-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"21849","inReplyTo":"vpqhbs4dkjr.fsf@bauges.imag.fr","subject":"[PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2009-12-06T18:45:28Z","receivedAt":"2009-12-06T18:45:28Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The previous error message was the same in many situations (unknown\nrevision or path not in the working tree). We try to help the user as\nmuch as possible to understand the error, especially with the\nsha1:filename notation. In this case, we say whether the sha1 or the\nfilename is problematic, and diagnose the confusion between\nrelative-to-root and relative-to-$PWD confusion precisely.\n\nThe 7 new error messages are tested.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nThis should address Junio's comments (BTW, I forgot to say \"thanks for\nthe review\" :-) ).\n\n\n cache.h                        |    6 ++-\n setup.c                        |   15 +++++-\n sha1_name.c                    |  115 ++++++++++++++++++++++++++++++++++++++--\n t/t1506-rev-parse-diagnosis.sh |   69 ++++++++++++++++++++++++\n 4 files changed, 198 insertions(+), 7 deletions(-)\n create mode 100755 t/t1506-rev-parse-diagnosis.sh\n\ndiff --git a/cache.h b/cache.h\nindex 0e69384..563862c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)\n #define DEFAULT_ABBREV 7\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);\n+static inline 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+}\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_hex(const char *hex, unsigned char *sha1);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref(const char *filename, unsigned char *sha1);\ndiff --git a/setup.c b/setup.c\nindex f67250b..5792eb7 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)\n \tdie_errno(\"failed to stat '%s'\", arg);\n }\n \n+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, 0, 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);\n+\n+}\n+\n /*\n  * Verify a filename that we got as an argument for a pathspec\n  * entry. Note that a filename that begins with \"-\" never verifies\n@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)\n \t\tdie(\"bad flag '%s' used after filename\", arg);\n \tif (check_filename(prefix, arg))\n \t\treturn;\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+\tdie_verify_filename(prefix, arg);\n }\n \n /*\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 44bb62d..1bc09ac 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -804,7 +804,96 @@ int get_sha1(const char *name, unsigned char *sha1)\n \treturn get_sha1_with_mode(name, sha1, &unused);\n }\n \n-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n+/* Must be called only when object_name:filename doesn't exist. */\n+static void diagnose_invalid_sha1_path(const char *prefix,\n+\t\t\t\t       const char *filename,\n+\t\t\t\t       const char *tree_sha1,\n+\t\t\t\t       const char *object_name)\n+{\n+\tstruct stat st;\n+\tunsigned char sha1[20];\n+\tunsigned mode;\n+\n+\tif (!prefix)\n+\t\tprefix = \"\";\n+\n+\tif (!lstat(filename, &st))\n+\t\tdie(\"Path '%s' exists on disk, but not in '%s'.\",\n+\t\t    filename, object_name);\n+\tif (errno == ENOENT || errno == ENOTDIR) {\n+\t\tchar *fullname = xmalloc(strlen(filename)\n+\t\t\t\t\t     + strlen(prefix) + 1);\n+\t\tstrcpy(fullname, prefix);\n+\t\tstrcat(fullname, filename);\n+\n+\t\tif (!get_tree_entry(tree_sha1, fullname,\n+\t\t\t\t    sha1, &mode)) {\n+\t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n+\t\t\t    \"Did you mean '%s:%s'?\",\n+\t\t\t    fullname,\n+\t\t\t    filename,\n+\t\t\t    object_name,\n+\t\t\t    fullname);\n+\t\t}\n+\t\tdie(\"Path '%s' does not exist in '%s'\",\n+\t\t    filename, object_name);\n+\t}\n+}\n+\n+/* Must be called only when :stage:filename doesn't exist. */\n+static void diagnose_invalid_index_path(int stage,\n+\t\t\t\t\tconst char *prefix,\n+\t\t\t\t\tconst char *filename)\n+{\n+\tstruct stat st;\n+\tstruct cache_entry *ce;\n+\tint pos;\n+\tint namelen = strlen(filename);\n+\tint fullnamelen;\n+\tchar *fullname;\n+\n+\tif (!prefix)\n+\t\tprefix = \"\";\n+\n+\t/* Wrong stage number? */\n+\tpos = cache_name_pos(filename, namelen);\n+\tif (pos < 0)\n+\t\tpos = -pos - 1;\n+\tce = active_cache[pos];\n+\tif (ce_namelen(ce) == namelen &&\n+\t    !memcmp(ce->name, filename, namelen))\n+\t\tdie(\"Path '%s' is in the index, but not at stage %d.\\n\"\n+\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t    filename, stage,\n+\t\t    ce_stage(ce), filename);\n+\n+\t/* Confusion between relative and absolute filenames? */\n+\tfullnamelen = namelen + strlen(prefix);\n+\tfullname = xmalloc(fullnamelen + 1);\n+\tstrcpy(fullname, prefix);\n+\tstrcat(fullname, filename);\n+\tpos = cache_name_pos(fullname, fullnamelen);\n+\tif (pos < 0)\n+\t\tpos = -pos - 1;\n+\tce = active_cache[pos];\n+\tif (ce_namelen(ce) == fullnamelen &&\n+\t    !memcmp(ce->name, fullname, fullnamelen))\n+\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n+\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t    fullname, filename,\n+\t\t    ce_stage(ce), fullname);\n+\n+\tif (!lstat(filename, &st))\n+\t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n+\tif (errno == ENOENT || errno == ENOTDIR)\n+\t\tdie(\"Path '%s' does not exist (neither on disk nor in the index).\",\n+\t\t    filename);\n+\n+\tfree(fullname);\n+}\n+\n+\n+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)\n {\n \tint ret, bracket_depth;\n \tint namelen = strlen(name);\n@@ -850,6 +939,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t\t\t}\n \t\t\tpos++;\n \t\t}\n+\t\tif (!gently)\n+\t\t\tdiagnose_invalid_index_path(stage, prefix, cp);\n \t\treturn -1;\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n@@ -862,9 +953,25 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t}\n \tif (*cp == ':') {\n \t\tunsigned char tree_sha1[20];\n-\t\tif (!get_sha1_1(name, cp-name, tree_sha1))\n-\t\t\treturn get_tree_entry(tree_sha1, cp+1, sha1,\n-\t\t\t\t\t      mode);\n+\t\tchar *object_name;\n+\t\tif (!gently) {\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+\t\t}\n+\t\tif (!get_sha1_1(name, cp-name, tree_sha1)) {\n+\t\t\tconst char *filename = cp+1;\n+\t\t\tret = get_tree_entry(tree_sha1, filename, sha1, mode);\n+\t\t\tif (!gently) {\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+\t\t\t}\n+\t\t\treturn ret;\n+\t\t} else {\n+\t\t\tif (!gently)\n+\t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n+\t\t}\n \t}\n \treturn ret;\n }\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nnew file mode 100755\nindex 0000000..af721f9\n--- /dev/null\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+test_description='test git rev-parse diagnosis for invalid argument'\n+\n+exec </dev/null\n+\n+. ./test-lib.sh\n+\n+HASH_file=\n+\n+test_expect_success 'set up basic repo' '\n+\techo one > file.txt &&\n+\tmkdir subdir &&\n+\techo two > subdir/file.txt &&\n+\techo three > subdir/file2.txt &&\n+\tgit add . &&\n+\tgit commit -m init &&\n+\techo four > index-only.txt &&\n+\tgit add index-only.txt &&\n+\techo five > disk-only.txt\n+'\n+\n+test_expect_success 'correct file objects' '\n+\tHASH_file=$(git rev-parse HEAD:file.txt) &&\n+\tgit rev-parse HEAD:subdir/file.txt &&\n+\tgit rev-parse :index-only.txt &&\n+\t(cd subdir &&\n+\t git rev-parse HEAD:subdir/file2.txt &&\n+\t test $HASH_file = $(git rev-parse HEAD:file.txt) &&\n+\t test $HASH_file = $(git rev-parse :file.txt) &&\n+\t test $HASH_file = $(git rev-parse :0:file.txt) )\n+'\n+\n+test_expect_success 'incorrect revision id' '\n+\ttest_must_fail git rev-parse foobar:file.txt 2>error &&\n+\tgrep \"Invalid object name '\"'\"'foobar'\"'\"'.\" error &&\n+\ttest_must_fail git rev-parse foobar 2> error &&\n+\tgrep \"unknown revision or path not in the working tree.\" error\n+'\n+\n+test_expect_success 'incorrect file in sha1:path' '\n+\ttest_must_fail git rev-parse HEAD:nothing.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'nothing.txt'\"'\"' does not exist in '\"'\"'HEAD'\"'\"'\" error &&\n+\ttest_must_fail git rev-parse HEAD:index-only.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'index-only.txt'\"'\"' exists on disk, but not in '\"'\"'HEAD'\"'\"'.\" error &&\n+\t(cd subdir &&\n+\t test_must_fail git rev-parse HEAD:file2.txt 2> error &&\n+\t grep \"Did you mean '\"'\"'HEAD:subdir/file2.txt'\"'\"'?\" error )\n+'\n+\n+test_expect_success 'incorrect file in :path and :N:path' '\n+\ttest_must_fail git rev-parse :nothing.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'nothing.txt'\"'\"' does not exist (neither on disk nor in the index).\" error &&\n+\ttest_must_fail git rev-parse :1:nothing.txt 2> error &&\n+\tgrep \"Path '\"'\"'nothing.txt'\"'\"' does not exist (neither on disk nor in the index).\" error &&\n+\ttest_must_fail git rev-parse :1:file.txt 2> error &&\n+\tgrep \"Did you mean '\"'\"':0:file.txt'\"'\"'?\" error &&\n+\t(cd subdir &&\n+\t test_must_fail git rev-parse :1:file.txt 2> error &&\n+\t grep \"Did you mean '\"'\"':0:file.txt'\"'\"'?\" error &&\n+\t test_must_fail git rev-parse :file2.txt 2> error &&\n+\t grep \"Did you mean '\"'\"':0:subdir/file2.txt'\"'\"'?\" error &&\n+\t test_must_fail git rev-parse :2:file2.txt 2> error &&\n+\t grep \"Did you mean '\"'\"':0:subdir/file2.txt'\"'\"'?\" error) &&\n+\ttest_must_fail git rev-parse :disk-only.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'disk-only.txt'\"'\"' exists on disk, but not in the index.\" error\n+'\n+\n+test_done\n-- \n1.6.6.rc0.355.gcb53a.dirty\n"},{"id":"129368","messageId":"7vtyw3bp7r.fsf@alter.siamese.dyndns.org","threadId":"21849","inReplyTo":"vpqhbs4dkjr.fsf@bauges.imag.fr","subject":"Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-07T00:30:16Z","receivedAt":"2009-12-07T00:30:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>>> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);\n>>\n>> Do I understand correctly that \"fatal\" here is the same as \"!gently\"\n>> elsewhere in the API?\n>\n> It seems it is. I renamed it.\n\nThis was a pure question, not a suggestion to change it (we do name a\nfunction do_foo_gently() when there is do_foo() that does the same but\nreports errors more noisily, though).  I found the name \"fatal\" a bit\nconfusing as I at first couldn't tell if the caller was telling the\nfunction that it already detected a \"fatal\" error (and telling the\nfunction to report the fatalness) and didn't realize that the caller is\ninstead saying \"if you find an error, treat it as a fatal one\" until I\nread it again.\n\nI am Ok with the new \"gently\" name with the negative semantics as well (I\nsee no need to change it back to \"error_is_fatal\").\n\nThanks.\n"},{"id":"129385","messageId":"7vk4wz9rbt.fsf@alter.siamese.dyndns.org","threadId":"21849","inReplyTo":"vpqhbs4dkjr.fsf@bauges.imag.fr","subject":"Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-07T07:27:34Z","receivedAt":"2009-12-07T07:27:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\nThe second round still had a few compilation warnings (turned to errors in\nmy environment) so I fixed them up somewhat.  I stopped before getting a\nclean compile, though---you will still get:\n\n  sha1_name.c: In function 'get_sha1_with_mode_1':\n  sha1_name.c:956: error: 'object_name' may be used uninitialized in this function\n\neven with this fix-up.\n\n>> Instead of doing a leaky allocation, it may make sense to pass the tree\n>> object name as <const char *, size_t> pair, and print it with \"%.*s\" in\n>> the error reporting codepath.  After all, object_name is used only for\n>> that purpose in diagnose_invalid_sha1_path(), no?\n>\n> matter of taste,...\n\nI thought generally it is accepted that I had a better taste on this list?\n;-)\n\n-- squashed --\n\n cache.h     |    4 ++--\n sha1_name.c |    2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 952bd51..3f9ee86 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -702,11 +702,11 @@ static inline unsigned int hexval(unsigned char c)\n #define DEFAULT_ABBREV 7\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n-static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\n+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, 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 }\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_hex(const char *hex, unsigned char *sha1);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref(const char *filename, unsigned char *sha1);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 1bc09ac..025244a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -807,7 +807,7 @@ int get_sha1(const char *name, unsigned char *sha1)\n /* Must be called only when object_name:filename doesn't exist. */\n static void diagnose_invalid_sha1_path(const char *prefix,\n \t\t\t\t       const char *filename,\n-\t\t\t\t       const char *tree_sha1,\n+\t\t\t\t       const unsigned char *tree_sha1,\n \t\t\t\t       const char *object_name)\n {\n \tstruct stat st;\n"},{"id":"129397","messageId":"vpqws0zcd1c.fsf@bauges.imag.fr","threadId":"21849","inReplyTo":"7vk4wz9rbt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2009-12-07T10:07:59Z","receivedAt":"2009-12-07T10:07:59Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n> The second round still had a few compilation warnings (turned to errors in\n> my environment) so I fixed them up somewhat.\n\nThanks. For some reasons, I had CFLAGS=-g and I didn't see the\nwarnings :-(\n\n> I stopped before getting a\n> clean compile, though---you will still get:\n>\n>   sha1_name.c: In function 'get_sha1_with_mode_1':\n>   sha1_name.c:956: error: 'object_name' may be used uninitialized in this function\n\nThis one is strange. I don't git it with gcc 4.4, even with -Wall\n-Werror. And indeed, object_name is initialized when !gently, which is\nprecisely when we use it.\n\nI did\n\n-               char *object_name;\n+               char *object_name = NULL;\n\nto make sure the warning goes away on your machine, but I don't\nunderstand what's going on.\n\n>  cache.h     |    4 ++--\n>  sha1_name.c |    2 +-\n\nSquashed into v3, together with initialisation of object_name, and two\ns/int/unsigned/ to please gcc -Wextra.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"129398","messageId":"1260180650-613-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"21849","inReplyTo":"vpqws0zcd1c.fsf@bauges.imag.fr","subject":"[PATCH v3] Detailed diagnosis when parsing an object name fails.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2009-12-07T10:10:50Z","receivedAt":"2009-12-07T10:10:50Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The previous error message was the same in many situations (unknown\nrevision or path not in the working tree). We try to help the user as\nmuch as possible to understand the error, especially with the\nsha1:filename notation. In this case, we say whether the sha1 or the\nfilename is problematic, and diagnose the confusion between\nrelative-to-root and relative-to-$PWD confusion precisely.\n\nThe 7 new error messages are tested.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n cache.h                        |    6 ++-\n setup.c                        |   15 +++++-\n sha1_name.c                    |  115 ++++++++++++++++++++++++++++++++++++++--\n t/t1506-rev-parse-diagnosis.sh |   69 ++++++++++++++++++++++++\n 4 files changed, 198 insertions(+), 7 deletions(-)\n create mode 100755 t/t1506-rev-parse-diagnosis.sh\n\ndiff --git a/cache.h b/cache.h\nindex 0e69384..c122bfa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)\n #define DEFAULT_ABBREV 7\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);\n+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, 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+}\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! */\n extern int read_ref(const char *filename, unsigned char *sha1);\ndiff --git a/setup.c b/setup.c\nindex f67250b..5792eb7 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)\n \tdie_errno(\"failed to stat '%s'\", arg);\n }\n \n+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, 0, 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);\n+\n+}\n+\n /*\n  * Verify a filename that we got as an argument for a pathspec\n  * entry. Note that a filename that begins with \"-\" never verifies\n@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)\n \t\tdie(\"bad flag '%s' used after filename\", arg);\n \tif (check_filename(prefix, arg))\n \t\treturn;\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+\tdie_verify_filename(prefix, arg);\n }\n \n /*\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 44bb62d..ca8f9db 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -804,7 +804,96 @@ int get_sha1(const char *name, unsigned char *sha1)\n \treturn get_sha1_with_mode(name, sha1, &unused);\n }\n \n-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n+/* Must be called only when object_name:filename doesn't exist. */\n+static void diagnose_invalid_sha1_path(const char *prefix,\n+\t\t\t\t       const char *filename,\n+\t\t\t\t       const unsigned char *tree_sha1,\n+\t\t\t\t       const char *object_name)\n+{\n+\tstruct stat st;\n+\tunsigned char sha1[20];\n+\tunsigned mode;\n+\n+\tif (!prefix)\n+\t\tprefix = \"\";\n+\n+\tif (!lstat(filename, &st))\n+\t\tdie(\"Path '%s' exists on disk, but not in '%s'.\",\n+\t\t    filename, object_name);\n+\tif (errno == ENOENT || errno == ENOTDIR) {\n+\t\tchar *fullname = xmalloc(strlen(filename)\n+\t\t\t\t\t     + strlen(prefix) + 1);\n+\t\tstrcpy(fullname, prefix);\n+\t\tstrcat(fullname, filename);\n+\n+\t\tif (!get_tree_entry(tree_sha1, fullname,\n+\t\t\t\t    sha1, &mode)) {\n+\t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n+\t\t\t    \"Did you mean '%s:%s'?\",\n+\t\t\t    fullname,\n+\t\t\t    filename,\n+\t\t\t    object_name,\n+\t\t\t    fullname);\n+\t\t}\n+\t\tdie(\"Path '%s' does not exist in '%s'\",\n+\t\t    filename, object_name);\n+\t}\n+}\n+\n+/* Must be called only when :stage:filename doesn't exist. */\n+static void diagnose_invalid_index_path(int stage,\n+\t\t\t\t\tconst char *prefix,\n+\t\t\t\t\tconst char *filename)\n+{\n+\tstruct stat st;\n+\tstruct cache_entry *ce;\n+\tint pos;\n+\tunsigned namelen = strlen(filename);\n+\tunsigned fullnamelen;\n+\tchar *fullname;\n+\n+\tif (!prefix)\n+\t\tprefix = \"\";\n+\n+\t/* Wrong stage number? */\n+\tpos = cache_name_pos(filename, namelen);\n+\tif (pos < 0)\n+\t\tpos = -pos - 1;\n+\tce = active_cache[pos];\n+\tif (ce_namelen(ce) == namelen &&\n+\t    !memcmp(ce->name, filename, namelen))\n+\t\tdie(\"Path '%s' is in the index, but not at stage %d.\\n\"\n+\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t    filename, stage,\n+\t\t    ce_stage(ce), filename);\n+\n+\t/* Confusion between relative and absolute filenames? */\n+\tfullnamelen = namelen + strlen(prefix);\n+\tfullname = xmalloc(fullnamelen + 1);\n+\tstrcpy(fullname, prefix);\n+\tstrcat(fullname, filename);\n+\tpos = cache_name_pos(fullname, fullnamelen);\n+\tif (pos < 0)\n+\t\tpos = -pos - 1;\n+\tce = active_cache[pos];\n+\tif (ce_namelen(ce) == fullnamelen &&\n+\t    !memcmp(ce->name, fullname, fullnamelen))\n+\t\tdie(\"Path '%s' is in the index, but not '%s'.\\n\"\n+\t\t    \"Did you mean ':%d:%s'?\",\n+\t\t    fullname, filename,\n+\t\t    ce_stage(ce), fullname);\n+\n+\tif (!lstat(filename, &st))\n+\t\tdie(\"Path '%s' exists on disk, but not in the index.\", filename);\n+\tif (errno == ENOENT || errno == ENOTDIR)\n+\t\tdie(\"Path '%s' does not exist (neither on disk nor in the index).\",\n+\t\t    filename);\n+\n+\tfree(fullname);\n+}\n+\n+\n+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)\n {\n \tint ret, bracket_depth;\n \tint namelen = strlen(name);\n@@ -850,6 +939,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t\t\t}\n \t\t\tpos++;\n \t\t}\n+\t\tif (!gently)\n+\t\t\tdiagnose_invalid_index_path(stage, prefix, cp);\n \t\treturn -1;\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n@@ -862,9 +953,25 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t}\n \tif (*cp == ':') {\n \t\tunsigned char tree_sha1[20];\n-\t\tif (!get_sha1_1(name, cp-name, tree_sha1))\n-\t\t\treturn get_tree_entry(tree_sha1, cp+1, sha1,\n-\t\t\t\t\t      mode);\n+\t\tchar *object_name = NULL;\n+\t\tif (!gently) {\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+\t\t}\n+\t\tif (!get_sha1_1(name, cp-name, tree_sha1)) {\n+\t\t\tconst char *filename = cp+1;\n+\t\t\tret = get_tree_entry(tree_sha1, filename, sha1, mode);\n+\t\t\tif (!gently) {\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+\t\t\t}\n+\t\t\treturn ret;\n+\t\t} else {\n+\t\t\tif (!gently)\n+\t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n+\t\t}\n \t}\n \treturn ret;\n }\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nnew file mode 100755\nindex 0000000..af721f9\n--- /dev/null\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+test_description='test git rev-parse diagnosis for invalid argument'\n+\n+exec </dev/null\n+\n+. ./test-lib.sh\n+\n+HASH_file=\n+\n+test_expect_success 'set up basic repo' '\n+\techo one > file.txt &&\n+\tmkdir subdir &&\n+\techo two > subdir/file.txt &&\n+\techo three > subdir/file2.txt &&\n+\tgit add . &&\n+\tgit commit -m init &&\n+\techo four > index-only.txt &&\n+\tgit add index-only.txt &&\n+\techo five > disk-only.txt\n+'\n+\n+test_expect_success 'correct file objects' '\n+\tHASH_file=$(git rev-parse HEAD:file.txt) &&\n+\tgit rev-parse HEAD:subdir/file.txt &&\n+\tgit rev-parse :index-only.txt &&\n+\t(cd subdir &&\n+\t git rev-parse HEAD:subdir/file2.txt &&\n+\t test $HASH_file = $(git rev-parse HEAD:file.txt) &&\n+\t test $HASH_file = $(git rev-parse :file.txt) &&\n+\t test $HASH_file = $(git rev-parse :0:file.txt) )\n+'\n+\n+test_expect_success 'incorrect revision id' '\n+\ttest_must_fail git rev-parse foobar:file.txt 2>error &&\n+\tgrep \"Invalid object name '\"'\"'foobar'\"'\"'.\" error &&\n+\ttest_must_fail git rev-parse foobar 2> error &&\n+\tgrep \"unknown revision or path not in the working tree.\" error\n+'\n+\n+test_expect_success 'incorrect file in sha1:path' '\n+\ttest_must_fail git rev-parse HEAD:nothing.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'nothing.txt'\"'\"' does not exist in '\"'\"'HEAD'\"'\"'\" error &&\n+\ttest_must_fail git rev-parse HEAD:index-only.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'index-only.txt'\"'\"' exists on disk, but not in '\"'\"'HEAD'\"'\"'.\" error &&\n+\t(cd subdir &&\n+\t test_must_fail git rev-parse HEAD:file2.txt 2> error &&\n+\t grep \"Did you mean '\"'\"'HEAD:subdir/file2.txt'\"'\"'?\" error )\n+'\n+\n+test_expect_success 'incorrect file in :path and :N:path' '\n+\ttest_must_fail git rev-parse :nothing.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'nothing.txt'\"'\"' does not exist (neither on disk nor in the index).\" error &&\n+\ttest_must_fail git rev-parse :1:nothing.txt 2> error &&\n+\tgrep \"Path '\"'\"'nothing.txt'\"'\"' does not exist (neither on disk nor in the index).\" error &&\n+\ttest_must_fail git rev-parse :1:file.txt 2> error &&\n+\tgrep \"Did you mean '\"'\"':0:file.txt'\"'\"'?\" error &&\n+\t(cd subdir &&\n+\t test_must_fail git rev-parse :1:file.txt 2> error &&\n+\t grep \"Did you mean '\"'\"':0:file.txt'\"'\"'?\" error &&\n+\t test_must_fail git rev-parse :file2.txt 2> error &&\n+\t grep \"Did you mean '\"'\"':0:subdir/file2.txt'\"'\"'?\" error &&\n+\t test_must_fail git rev-parse :2:file2.txt 2> error &&\n+\t grep \"Did you mean '\"'\"':0:subdir/file2.txt'\"'\"'?\" error) &&\n+\ttest_must_fail git rev-parse :disk-only.txt 2> error &&\n+\tgrep \"fatal: Path '\"'\"'disk-only.txt'\"'\"' exists on disk, but not in the index.\" error\n+'\n+\n+test_done\n-- \n1.6.6.rc0.355.gcb53a.dirty\n"},{"id":"134641","messageId":"fabb9a1e1002150616h6a747f58q41eecf84dac736fb@mail.gmail.com","threadId":"21849","inReplyTo":"1260180650-613-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH v3] Detailed diagnosis when parsing an object name fails.","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-02-15T14:16:05Z","receivedAt":"2010-02-15T14:16:05Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Dec 7, 2009 at 11:10, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:\n> The previous error message was the same in many situations (unknown\n> revision or path not in the working tree). We try to help the user as\n> much as possible to understand the error, especially with the\n> sha1:filename notation. In this case, we say whether the sha1 or the\n> filename is problematic, and diagnose the confusion between\n> relative-to-root and relative-to-$PWD confusion precisely.\n\nThank you, I just got helped by your patch, and was very pleasantly\nsurprised to see git being so unusually helpful! We'll make a\nuser-friendly scm out of git yet :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"}]}