{"thread":{"id":"3941","subject":"Re: [RFC] Loosening path argument check a little bit in revision.c","startedAt":"2006-04-22T10:15:05Z","lastAt":"2006-04-26T22:22:14Z","messageCount":3,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"19008","messageId":"7vzmidkjbq.fsf@assigned-by-dhcp.cox.net","threadId":"3941","inReplyTo":null,"subject":"[RFC] Loosening path argument check a little bit in revision.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-22T10:15:05Z","receivedAt":"2006-04-22T10:15:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The argument parser revision.c::setup_revisions() insists on\npath arguments that do not follow the double-dash marker to be\npaths (either files or directories) that exist in the working\ntree.  As the fact that diff-files and update-index have\nexplicit options to ignore \"missing\" files reminds us,\ntraditionally we allowed a sparsely populated working tree, so\nthis check is not very user friendly.\n\nThis patch allows non-existing paths to be given without the\ndouble-dash marker as long as they exist in the index, or they\nare leading paths of blobs that exist in the index.\n\nA misspelled tag v2.6.16 is not a very likely name to have only\nin the index and not in the working tree, so this should not\nbreak the case the original code wanted to fix ;-).\n\n---\n\n * I noticed a breakage in out test scripts while libifying\n   diff-files.  The two-tree fast-forward merge test stuffs an\n   entry DF/DF in the index while having a working tree file DF,\n   and run \"diff-files DF/DF\" to make sure two-way read-tree\n   left the index dirty.  The libified diff-files naturally uses\n   revision infrastructure to get its parameters, and the path\n   argument check causes the test to fail without this patch.\n\n   This is just an RFC.  We can easily rewrite the particular\n   test to use the double-dash marker, but I suspect existing\n   porcelains share the same problem, expecting to be able to do\n   something like this:\n\n   \t$ names=`git diff-files --name-only`\n        $ git diff-index HEAD $names\n\n   Of course, the kosher way is to always use double-dash, like\n   this:\n\n\t$ git diff-files -z --name-only |\n          xargs -0 git diff-index HEAD --\n\n   So I am not pushing this too strongly.  Comments?\n\n revision.c |   48 ++++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 44 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 113dd5a..9f6dd24 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -504,6 +504,43 @@ void init_revisions(struct rev_info *rev\n \tdiff_setup(&revs->diffopt);\n }\n \n+static int check_path_arg(const char *path)\n+{\n+\tstruct stat st;\n+\tint tmp = lstat(path, &st);\n+\tif (!tmp)\n+\t\treturn 0;\n+\ttmp = errno;\n+\tif (!active_cache)\n+\t\tread_cache();\n+\tif (active_cache) {\n+\t\tint namelen = strlen(path);\n+\t\tint pos = cache_name_pos(path, namelen);\n+\t\tif (pos < 0)\n+\t\t\tpos = -pos - 1;\n+\t\tif (pos < active_nr) {\n+\t\t\tstruct cache_entry *ce = active_cache[pos];\n+\t\t\tif (ce_namelen(ce) == namelen &&\n+\t\t\t    !memcmp(ce->name, path, namelen))\n+\t\t\t\treturn 0;\n+\t\t}\n+\t\t/* Try it as a directory name - \"foo/\" */\n+\t\twhile (++pos < active_nr) {\n+\t\t\tstruct cache_entry *ce = active_cache[pos];\n+\t\t\tif (namelen < ce_namelen(ce) &&\n+\t\t\t    !memcmp(ce->name, path, namelen)) {\n+\t\t\t\t/* prefix still matches */\n+\t\t\t\tif (ce->name[namelen] == '/')\n+\t\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tbreak; /* prefix does not match anymore */\n+\t\t}\n+\t}\n+\terrno = tmp;\n+\treturn -1;\n+}\n+\n /*\n  * Parse revision information, filling in the \"rev_info\" structure,\n  * and removing the used arguments from the argument list.\n@@ -752,16 +789,19 @@ 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\t/* If we didn't have a \"--\", all filenames must\n+\t\t\t * exist, either in the working tree or in the\n+\t\t\t * cache.\n+\t\t\t */\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\tif (check_path_arg(argv[j]))\n+\t\t\t\t\tdie(\"'%s': %s\", argv[j],\n+\t\t\t\t\t    strerror(errno));\n \t\t\t}\n \t\t\trevs->prune_data = get_pathspec(revs->prefix, argv + i);\n \t\t\tbreak;\n"},{"id":"18987","messageId":"Pine.LNX.4.64.0604220921430.3701@g5.osdl.org","threadId":"3941","inReplyTo":"7vzmidkjbq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] Loosening path argument check a little bit in revision.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-22T16:30:29Z","receivedAt":"2006-04-22T16:30:29Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 22 Apr 2006, Junio C Hamano wrote:\n> \n> This patch allows non-existing paths to be given without the\n> double-dash marker as long as they exist in the index, or they\n> are leading paths of blobs that exist in the index.\n\nThe only objection I have to this (and it's not a very strong one) is that \nit can cause the index to be populated for processes that really don't \ncare.\n\nWe could work around that with a flag in \"struct rev_info\", so that \nprograms that call \"setup_revisions()\" can specify before-hand if they are \ninterested in the index or not.\n\nThe reason I somewhat care was that I was actually going to make the \nchecks a bit _stronger_: I was going to say that if you have a pathspec, \nbut don't have a \"--\" marker, then we'd additionally check:\n\n - none of the arguments we parsed as revisions could be interpreted as a \n   filename.\n\n - none of the paths are \"--\" \n\nso that there really isn't any possibility of accidental confusion in case \nsomebody does have a revision that looks like a pathname too.\n\nBut I don't actually care _that_ deeply.\n\n\t\tLinus\n"},{"id":"19169","messageId":"7vmze8m0yx.fsf_-_@assigned-by-dhcp.cox.net","threadId":"3941","inReplyTo":"Pine.LNX.4.64.0604220921430.3701@g5.osdl.org","subject":"[PATCH] revision parsing: make \"rev -- paths\" checks stronger.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-26T22:22:14Z","receivedAt":"2006-04-26T22:22:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you don't have a \"--\" marker, then:\n\n - all of the arguments we are going to assume are pathspecs\n   must exist in the working tree.\n\n - none of the arguments we parsed as revisions could be\n   interpreted as a filename.\n\nso that there really isn't any possibility of confusion in case\nsomebody does have a revision that looks like a pathname too.\n\nThe former rule has been in effect; this implements the latter.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n * I did not understand 'if you have a pathspec, but' part, and\n   'none of the paths are \"--\"' part of your rules, so this\n   might probably be a bit different from what you had in\n   mind.\n\n   The patch, lacking the \"if you have a pathspec\" part of the\n   rule, would make this complain:\n\n   \t$ >v1.0.0\n        $ git show v1.0.0\n\n   BTW, this would silently do what you would not find\n   interesting, with or without the patch:\n\n       $ >v2.9.6\n\t ... long time later after you forget you had such a\n\t ... bogus file\n       $ git show v2.9.6\n\n  Linus Torvalds <torvalds@osdl.org> writes:\n\n  > ... I was going to say that if you have a pathspec, \n  > but don't have a \"--\" marker, then we'd additionally check:\n  >\n  >  - none of the arguments we parsed as revisions could be interpreted as a \n  >    filename.\n  >\n  >  - none of the paths are \"--\" \n  >\n  > so that there really isn't...\n\n\n\n cache.h    |    1 +\n revision.c |   14 +++++++++++++-\n setup.c    |   24 ++++++++++++++++++++++--\n 3 files changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d8fabc..a4f253e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -135,6 +135,7 @@ extern const char *setup_git_directory(v\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+extern void verify_non_filename(const char *prefix, const char *name);\n \n #define alloc_nr(x) (((x)+16)*3/2)\n \ndiff --git a/revision.c b/revision.c\nindex f2a9f25..b6ed014 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -740,6 +740,11 @@ int setup_revisions(int argc, const char\n \t\t\t\tinclude = get_reference(revs, next, sha1, flags);\n \t\t\t\tif (!exclude || !include)\n \t\t\t\t\tdie(\"Invalid revision range %s..%s\", arg, next);\n+\n+\t\t\t\tif (!seen_dashdash) {\n+\t\t\t\t\t*dotdot = '.';\n+\t\t\t\t\tverify_non_filename(revs->prefix, arg);\n+\t\t\t\t}\n \t\t\t\tadd_pending_object(revs, exclude, this);\n \t\t\t\tadd_pending_object(revs, include, next);\n \t\t\t\tcontinue;\n@@ -757,13 +762,20 @@ int setup_revisions(int argc, const char\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\t/* If we didn't have a \"--\":\n+\t\t\t * (1) all filenames must exist;\n+\t\t\t * (2) all rev-args must not be interpretable\n+\t\t\t *     as a valid filename.\n+\t\t\t * but the latter we have checked in the main loop.\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}\n+\t\tif (!seen_dashdash)\n+\t\t\tverify_non_filename(revs->prefix, arg);\n \t\tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n \t\tadd_pending_object(revs, object, arg);\n \t}\ndiff --git a/setup.c b/setup.c\nindex cce9bb8..fe7f884 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -80,11 +80,31 @@ void verify_filename(const char *prefix,\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+\t\tdie(\"ambiguous argument '%s': unknown revision or path not in the working tree.\\n\"\n+\t\t    \"Use '--' to separate paths from revisions\", arg);\n \tdie(\"'%s': %s\", arg, strerror(errno));\n }\n \n+/*\n+ * Opposite of the above: the command line did not have -- marker\n+ * and we parsed the arg as a refname.  It should not be interpretable\n+ * as a filename.\n+ */\n+void verify_non_filename(const char *prefix, const char *arg)\n+{\n+\tconst char *name;\n+\tstruct stat st;\n+\n+\tif (*arg == '-')\n+\t\treturn; /* flag */\n+\tname = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;\n+\tif (!lstat(name, &st))\n+\t\tdie(\"ambiguous argument '%s': both revision and filename\\n\"\n+\t\t    \"Use '--' to separate filenames from revisions\", arg);\n+\tif (errno != ENOENT)\n+\t\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-- \n1.3.1.ga6c7\n"}]}