{"thread":{"id":"3947","subject":"lstat() call in rev-parse.c","startedAt":"2006-04-23T12:03:46Z","lastAt":"2006-04-26T18:14:00Z","messageCount":8,"participants":["Paul Mackerras","Linus Torvalds","Matthias Lederhofer","Timo Hirvonen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"19017","messageId":"17483.27938.890830.375324@cargo.ozlabs.ibm.com","threadId":"3947","inReplyTo":null,"subject":"lstat() call in rev-parse.c","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2006-04-23T12:03:46Z","receivedAt":"2006-04-23T12:03:46Z","isPatch":false,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Why does git-rev-parse do an lstat on some of its arguments, at line\n345 of rev-parse.c, and die if the lstat fails?  It doesn't seem to do\nanything with the result.\n\nThe effect is that if you do \"gitk a b\", it works as long as a and b\nexist (as files or directories), but fails if they don't, and some\nusers have found this confusing.  Yes they should put in a --, but\nit's not obvious to users why this should make it work in the case\nwhen a or b doesn't exist.\n\n(And yes I just took out the git-rev-parse call from gitk, but I'm\ngoing to need to do git-rev-parse --no-refs --no-flags for some\nchanges I'm doing at the moment.)\n\nPaul.\n"},{"id":"19018","messageId":"Pine.LNX.4.64.0604230906370.3701@g5.osdl.org","threadId":"3947","inReplyTo":"17483.27938.890830.375324@cargo.ozlabs.ibm.com","subject":"Re: lstat() call in rev-parse.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-23T16:19:30Z","receivedAt":"2006-04-23T16:19:30Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 23 Apr 2006, Paul Mackerras wrote:\n>\n> Why does git-rev-parse do an lstat on some of its arguments, at line\n> 345 of rev-parse.c, and die if the lstat fails?  It doesn't seem to do\n> anything with the result.\n> \n> The effect is that if you do \"gitk a b\", it works as long as a and b\n> exist (as files or directories), but fails if they don't, and some\n> users have found this confusing.\n\nBecause it's even _more_ common to do\n\n\tgitk v2.6.16\n\n(in the git directory) and be very confused when that returns an empty \nhistory. \n\nSo the rule is: if you don't give that \"--\", then we have to be able to \nconfirm that the filenames are really files. Not a misspelled revision \nname, or a revision name that was correctly spelled, but for the wrong \nproject, because you were in the wrong subdirectory ;)\n\nAnd yes, this actually happened to me. I was demonstrating git before we \ndid that fix, and I used the wrong tag-name, and gitk would think for a \nminute and them show \"No commits selected\" with no error, because \n\"v2.6.16\" didn't exist at that time, and it used the \"tag-name\" as a \nfilename, and that filename didn't actually exist, so the number of \ncommits that changed it was exactly zero.\n\nSo now, if you do \"gitk v2.6.16\" in the git directory, it will return a \nnice and understandable \n\n\tError reading commits: fatal: 'v2.6.16': No such file or directory\n\nwhich is _exactly_ what you want. The only problem is that gitk will \nactually open the big window too, and the error window is small and can be \nnon-obvious if the window manager hides it in some corner. So I actually \nthink you should try to make the error window come up smack dab in front \nof the main gitk window and make the error clearer.\n\nSo the rules are simple:\n - the filenames are _always_ separated by \"--\"\n - we have a short-hand, which allows the \"--\" to be dropped iff it is \n   unambiguous\n\nWhere \"unambiguous\" means that\n - the revision name cannot possible be interpreted as a filename (we \n   don't check this part yet, but I think we should)\n - all filenames are obviously not revision names, since they not \n   only aren't valid revisions, they actually exist as files.\n\nOtherwise, misspellings, typos, and thinkos result in total confusion.\n\n\t\tLinus\n"},{"id":"19066","messageId":"17485.24053.35997.868256@cargo.ozlabs.ibm.com","threadId":"3947","inReplyTo":"Pine.LNX.4.64.0604230906370.3701@g5.osdl.org","subject":"Re: lstat() call in rev-parse.c","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2006-04-24T23:23:33Z","receivedAt":"2006-04-24T23:23:33Z","isPatch":false,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Linus Torvalds writes:\n\n> So the rule is: if you don't give that \"--\", then we have to be able to \n> confirm that the filenames are really files. Not a misspelled revision \n> name, or a revision name that was correctly spelled, but for the wrong \n> project, because you were in the wrong subdirectory ;)\n\nOK, fair enough.  In that case we need a better error message, so I\ndon't get people complaining that gitk can't show the history of files\nthat don't exist any more.  How about something like:\n\nArgument \"foo\" is ambiguous - revision name or file/directory name?\nPlease put \"--\" before the list of filenames.\n\nI'll hack up a patch to this effect.\n\nPaul.\n"},{"id":"19157","messageId":"E1FYlwn-0005mf-CL@moooo.ath.cx","threadId":"3947","inReplyTo":"Pine.LNX.4.64.0604230906370.3701@g5.osdl.org","subject":"Re: lstat() call in rev-parse.c","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2006-04-26T15:28:57Z","receivedAt":"2006-04-26T15:28:57Z","isPatch":false,"sender":{"key":"matled@gmx.net","avatar":null},"body":"> So the rule is: if you don't give that \"--\", then we have to be able to \n> confirm that the filenames are really files. Not a misspelled revision \n> name, or a revision name that was correctly spelled, but for the wrong \n> project, because you were in the wrong subdirectory ;)\n\nShouldn't git rev-parse try to stat the file (additionally?) in the\ncurrent directory instead of the top git directory? git (diff|log|..)\nseem to fail everytime in a subdirectory without --.\n"},{"id":"19158","messageId":"Pine.LNX.4.64.0604260832240.3701@g5.osdl.org","threadId":"3947","inReplyTo":"E1FYlwn-0005mf-CL@moooo.ath.cx","subject":"Re: lstat() call in rev-parse.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-26T15:43:47Z","receivedAt":"2006-04-26T15:43:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 26 Apr 2006, Matthias Lederhofer wrote:\n\n> > So the rule is: if you don't give that \"--\", then we have to be able \n> > to confirm that the filenames are really files. Not a misspelled \n> > revision name, or a revision name that was correctly spelled, but for \n> > the wrong project, because you were in the wrong subdirectory ;)\n> \n> Shouldn't git rev-parse try to stat the file (additionally?) in the \n> current directory instead of the top git directory? git (diff|log|..) \n> seem to fail everytime in a subdirectory without --.\n\nGood point. However, the reason for that is that it actually _does_ stat \nthe file in the current directory, but it has done the \n\n\trevs->prefix = setup_git_directory();\n\nin the init path (and it does need to do that, since that's what figures \nout where the .git directory is, so that we can parse the revisions \ncorrectly).\n\nAnd that \"setup_git_directory()\" will chdir() to the root of the project.\n\nSo the \"lstat()\" should probably take \"revs->prefix\" into account, the \nway get_pathspec() does. Ie we should probably use\n\n\tchar *name = argv[i];\n\tif (rev->prefix)\n\t\tname = prefix_filename(rev->prefix, strlen(rev->prefix), name);\n\tif (lstat(name, ..) < 0)\n\t\tdie(...)\n\ninstead of just a plain lstat().\n\nProbably worth doing as a small helper funtion of its own (and get rid of \nthe current \"die_badfile()\" - and do all of that inside the helper \nfunction).\n\nSomebody?\n\n\t\tLinus\n"},{"id":"19162","messageId":"Pine.LNX.4.64.0604261010390.3701@g5.osdl.org","threadId":"3947","inReplyTo":"Pine.LNX.4.64.0604260832240.3701@g5.osdl.org","subject":"[PATCH] Fix filename verification when in a subdirectory","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-26T17:15:54Z","receivedAt":"2006-04-26T17:15:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWhen we are in a subdirectory of a git archive, we need to take the prefix \nof that subdirectory into accoung when we verify filename arguments.\n\nNoted by Matthias Lederhofer\n\nThis also uses the improved error reporting for all the other git commands \nthat use the revision parsing interfaces, not just git-rev-parse. Also, it \nmakes the error reporting for mixed filenames and argument flags clearer \n(you cannot put flags after the start of the pathname list).\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\nOn Wed, 26 Apr 2006, Linus Torvalds wrote:\n> > \n> > Shouldn't git rev-parse try to stat the file (additionally?) in the \n> > current directory instead of the top git directory? git (diff|log|..) \n> > seem to fail everytime in a subdirectory without --.\n> \n> Good point. However, the reason for that is that it actually _does_ stat \n> the file in the current directory, but it has done the \n> \n> \trevs->prefix = setup_git_directory();\n> \n> in the init path (and it does need to do that, since that's what figures \n> out where the .git directory is, so that we can parse the revisions \n> correctly).\n> \n> And that \"setup_git_directory()\" will chdir() to the root of the project.\n\ndiff --git a/cache.h b/cache.h\nindex 69801b0..4d8fabc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -134,6 +134,7 @@ extern const char *setup_git_directory_g\n extern const char *setup_git_directory(void);\n extern const char *prefix_path(const char *prefix, int len, const char *path);\n extern const char *prefix_filename(const char *prefix, int len, const char *path);\n+extern void verify_filename(const char *prefix, const char *name);\n \n #define alloc_nr(x) (((x)+16)*3/2)\n \ndiff --git a/rev-parse.c b/rev-parse.c\nindex 7f66ae2..62e16af 100644\n--- a/rev-parse.c\n+++ b/rev-parse.c\n@@ -160,14 +160,6 @@ static int show_file(const char *arg)\n \treturn 0;\n }\n \n-static void die_badfile(const char *arg)\n-{\n-\tif (errno != ENOENT)\n-\t\tdie(\"'%s': %s\", arg, strerror(errno));\n-\tdie(\"'%s' is ambiguous - revision name or file/directory name?\\n\"\n-\t    \"Please put '--' before the list of filenames.\", arg);\n-}\n-\n int main(int argc, char **argv)\n {\n \tint i, as_is = 0, verify = 0;\n@@ -177,14 +169,12 @@ int main(int argc, char **argv)\n \tgit_config(git_default_config);\n \n \tfor (i = 1; i < argc; i++) {\n-\t\tstruct stat st;\n \t\tchar *arg = argv[i];\n \t\tchar *dotdot;\n \n \t\tif (as_is) {\n \t\t\tif (show_file(arg) && as_is < 2)\n-\t\t\t\tif (lstat(arg, &st) < 0)\n-\t\t\t\t\tdie_badfile(arg);\n+\t\t\t\tverify_filename(prefix, arg);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg,\"-n\")) {\n@@ -350,8 +340,7 @@ int main(int argc, char **argv)\n \t\t\tcontinue;\n \t\tif (verify)\n \t\t\tdie(\"Needed a single revision\");\n-\t\tif (lstat(arg, &st) < 0)\n-\t\t\tdie_badfile(arg);\n+\t\tverify_filename(prefix, arg);\n \t}\n \tshow_default();\n \tif (verify && revs_count != 1)\ndiff --git a/revision.c b/revision.c\nindex f9c7d15..f2a9f25 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -752,17 +752,15 @@ int setup_revisions(int argc, const char\n \t\t\targ++;\n \t\t}\n \t\tif (get_sha1(arg, sha1) < 0) {\n-\t\t\tstruct stat st;\n \t\t\tint j;\n \n \t\t\tif (seen_dashdash || local_flags)\n \t\t\t\tdie(\"bad revision '%s'\", arg);\n \n \t\t\t/* If we didn't have a \"--\", all filenames must exist */\n-\t\t\tfor (j = i; j < argc; j++) {\n-\t\t\t\tif (lstat(argv[j], &st) < 0)\n-\t\t\t\t\tdie(\"'%s': %s\", argv[j], strerror(errno));\n-\t\t\t}\n+\t\t\tfor (j = i; j < argc; j++)\n+\t\t\t\tverify_filename(revs->prefix, argv[j]);\n+\n \t\t\trevs->prune_data = get_pathspec(revs->prefix, argv + i);\n \t\t\tbreak;\n \t\t}\ndiff --git a/setup.c b/setup.c\nindex 36ede3d..119ef7d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -62,6 +62,29 @@ const char *prefix_filename(const char *\n \treturn path;\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+ * as true, because even if such a filename were to exist, we want\n+ * it to be preceded by the \"--\" marker (or we want the user to\n+ * use a format like \"./-filename\")\n+ */\n+void verify_filename(const char *prefix, const char *arg)\n+{\n+\tconst char *name;\n+\tstruct stat st;\n+\n+\tif (*arg == '-')\n+\t\tdie(\"bad flag '%s' used after filename\", arg);\n+\tname = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;\n+\tif (!lstat(name, &st))\n+\t\treturn;\n+\tif (errno == ENOENT);\n+\t\tdie(\"ambiguous argument '%s': unknown revision or filename\\n\"\n+\t\t    \"Use '--' to separate filenames from revisions\", arg);\n+\tdie(\"'%s': %s\", arg, strerror(errno));\n+}\n+\n const char **get_pathspec(const char *prefix, const char **pathspec)\n {\n \tconst char *entry = *pathspec;\n"},{"id":"19164","messageId":"20060426210541.5e145e88.tihirvon@gmail.com","threadId":"3947","inReplyTo":"Pine.LNX.4.64.0604261010390.3701@g5.osdl.org","subject":"Re: [PATCH] Fix filename verification when in a subdirectory","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2006-04-26T18:05:41Z","receivedAt":"2006-04-26T18:05:41Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"Linus Torvalds <torvalds@osdl.org> wrote:\n\n> +void verify_filename(const char *prefix, const char *arg)\n> +{\n> +\tconst char *name;\n> +\tstruct stat st;\n> +\n> +\tif (*arg == '-')\n> +\t\tdie(\"bad flag '%s' used after filename\", arg);\n> +\tname = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;\n> +\tif (!lstat(name, &st))\n> +\t\treturn;\n> +\tif (errno == ENOENT);\n\nExtra semicolon.\n\n-- \nhttp://onion.dynserv.net/~timo/\n"},{"id":"19165","messageId":"Pine.LNX.4.64.0604261112240.3701@g5.osdl.org","threadId":"3947","inReplyTo":"20060426210541.5e145e88.tihirvon@gmail.com","subject":"Re: [PATCH] Fix filename verification when in a subdirectory","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-26T18:14:00Z","receivedAt":"2006-04-26T18:14:00Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 26 Apr 2006, Timo Hirvonen wrote:\n> \n> Extra semicolon.\n\nDuh, indeed. It just didn't show up in any of the normal cases.\n\nJunio, just apply without that stupid semicolon..\n\n\t\tLinus\n"}]}