[RFC] Loosening path argument check a little bit in revision.c

[RFC] Loosening path argument check a little bit in revision.c

[Junio C Hamano]

The argument parser revision.c::setup_revisions() insists on
path arguments that do not follow the double-dash marker to be
paths (either files or directories) that exist in the working
tree.  As the fact that diff-files and update-index have
explicit options to ignore "missing" files reminds us,
traditionally we allowed a sparsely populated working tree, so
this check is not very user friendly.

This patch allows non-existing paths to be given without the
double-dash marker as long as they exist in the index, or they
are leading paths of blobs that exist in the index.

A misspelled tag v2.6.16 is not a very likely name to have only
in the index and not in the working tree, so this should not
break the case the original code wanted to fix ;-).

---

 * I noticed a breakage in out test scripts while libifying
   diff-files.  The two-tree fast-forward merge test stuffs an
   entry DF/DF in the index while having a working tree file DF,
   and run "diff-files DF/DF" to make sure two-way read-tree
   left the index dirty.  The libified diff-files naturally uses
   revision infrastructure to get its parameters, and the path
   argument check causes the test to fail without this patch.

   This is just an RFC.  We can easily rewrite the particular
   test to use the double-dash marker, but I suspect existing
   porcelains share the same problem, expecting to be able to do
   something like this:

   	$ names=`git diff-files --name-only`
        $ git diff-index HEAD $names

   Of course, the kosher way is to always use double-dash, like
   this:

	$ git diff-files -z --name-only |
          xargs -0 git diff-index HEAD --

   So I am not pushing this too strongly.  Comments?

 revision.c |   48 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 113dd5a..9f6dd24 100644
--- a/revision.c
+++ b/revision.c
@@ -504,6 +504,43 @@ void init_revisions(struct rev_info *rev
 	diff_setup(&revs->diffopt);
 }
 
+static int check_path_arg(const char *path)
+{
+	struct stat st;
+	int tmp = lstat(path, &st);
+	if (!tmp)
+		return 0;
+	tmp = errno;
+	if (!active_cache)
+		read_cache();
+	if (active_cache) {
+		int namelen = strlen(path);
+		int pos = cache_name_pos(path, namelen);
+		if (pos < 0)
+			pos = -pos - 1;
+		if (pos < active_nr) {
+			struct cache_entry *ce = active_cache[pos];
+			if (ce_namelen(ce) == namelen &&
+			    !memcmp(ce->name, path, namelen))
+				return 0;
+		}
+		/* Try it as a directory name - "foo/" */
+		while (++pos < active_nr) {
+			struct cache_entry *ce = active_cache[pos];
+			if (namelen < ce_namelen(ce) &&
+			    !memcmp(ce->name, path, namelen)) {
+				/* prefix still matches */
+				if (ce->name[namelen] == '/')
+					return 0;
+			}
+			else
+				break; /* prefix does not match anymore */
+		}
+	}
+	errno = tmp;
+	return -1;
+}
+
 /*
  * Parse revision information, filling in the "rev_info" structure,
  * and removing the used arguments from the argument list.
@@ -752,16 +789,19 @@ int setup_revisions(int argc, const char
 			arg++;
 		}
 		if (get_sha1(arg, sha1) < 0) {
-			struct stat st;
 			int j;
 
 			if (seen_dashdash || local_flags)
 				die("bad revision '%s'", arg);
 
-			/* If we didn't have a "--", all filenames must exist */
+			/* If we didn't have a "--", all filenames must
+			 * exist, either in the working tree or in the
+			 * cache.
+			 */
 			for (j = i; j < argc; j++) {
-				if (lstat(argv[j], &st) < 0)
-					die("'%s': %s", argv[j], strerror(errno));
+				if (check_path_arg(argv[j]))
+					die("'%s': %s", argv[j],
+					    strerror(errno));
 			}
 			revs->prune_data = get_pathspec(revs->prefix, argv + i);
 			break;

Re: [RFC] Loosening path argument check a little bit in revision.c

[Linus Torvalds]

On Sat, 22 Apr 2006, Junio C Hamano wrote:
> 
> This patch allows non-existing paths to be given without the
> double-dash marker as long as they exist in the index, or they
> are leading paths of blobs that exist in the index.

The only objection I have to this (and it's not a very strong one) is that 
it can cause the index to be populated for processes that really don't 
care.

We could work around that with a flag in "struct rev_info", so that 
programs that call "setup_revisions()" can specify before-hand if they are 
interested in the index or not.

The reason I somewhat care was that I was actually going to make the 
checks a bit _stronger_: I was going to say that if you have a pathspec, 
but don't have a "--" marker, then we'd additionally check:

 - none of the arguments we parsed as revisions could be interpreted as a 
   filename.

 - none of the paths are "--" 

so that there really isn't any possibility of accidental confusion in case 
somebody does have a revision that looks like a pathname too.

But I don't actually care _that_ deeply.

		Linus

[PATCH] revision parsing: make "rev -- paths" checks stronger.

[Junio C Hamano]

If you don't have a "--" marker, then:

 - all of the arguments we are going to assume are pathspecs
   must exist in the working tree.

 - none of the arguments we parsed as revisions could be
   interpreted as a filename.

so that there really isn't any possibility of confusion in case
somebody does have a revision that looks like a pathname too.

The former rule has been in effect; this implements the latter.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * I did not understand 'if you have a pathspec, but' part, and
   'none of the paths are "--"' part of your rules, so this
   might probably be a bit different from what you had in
   mind.

   The patch, lacking the "if you have a pathspec" part of the
   rule, would make this complain:

   	$ >v1.0.0
        $ git show v1.0.0

   BTW, this would silently do what you would not find
   interesting, with or without the patch:

       $ >v2.9.6
	 ... long time later after you forget you had such a
	 ... bogus file
       $ git show v2.9.6

  Linus Torvalds <torvalds@osdl.org> writes:

  > ... I was going to say that if you have a pathspec, 
  > but don't have a "--" marker, then we'd additionally check:
  >
  >  - none of the arguments we parsed as revisions could be interpreted as a 
  >    filename.
  >
  >  - none of the paths are "--" 
  >
  > so that there really isn't...



 cache.h    |    1 +
 revision.c |   14 +++++++++++++-
 setup.c    |   24 ++++++++++++++++++++++--
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 4d8fabc..a4f253e 100644
--- a/cache.h
+++ b/cache.h
@@ -135,6 +135,7 @@ extern const char *setup_git_directory(v
 extern const char *prefix_path(const char *prefix, int len, const char *path);
 extern const char *prefix_filename(const char *prefix, int len, const char *path);
 extern void verify_filename(const char *prefix, const char *name);
+extern void verify_non_filename(const char *prefix, const char *name);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
diff --git a/revision.c b/revision.c
index f2a9f25..b6ed014 100644
--- a/revision.c
+++ b/revision.c
@@ -740,6 +740,11 @@ int setup_revisions(int argc, const char
 				include = get_reference(revs, next, sha1, flags);
 				if (!exclude || !include)
 					die("Invalid revision range %s..%s", arg, next);
+
+				if (!seen_dashdash) {
+					*dotdot = '.';
+					verify_non_filename(revs->prefix, arg);
+				}
 				add_pending_object(revs, exclude, this);
 				add_pending_object(revs, include, next);
 				continue;
@@ -757,13 +762,20 @@ int setup_revisions(int argc, const char
 			if (seen_dashdash || local_flags)
 				die("bad revision '%s'", arg);
 
-			/* If we didn't have a "--", all filenames must exist */
+			/* If we didn't have a "--":
+			 * (1) all filenames must exist;
+			 * (2) all rev-args must not be interpretable
+			 *     as a valid filename.
+			 * but the latter we have checked in the main loop.
+			 */
 			for (j = i; j < argc; j++)
 				verify_filename(revs->prefix, argv[j]);
 
 			revs->prune_data = get_pathspec(revs->prefix, argv + i);
 			break;
 		}
+		if (!seen_dashdash)
+			verify_non_filename(revs->prefix, arg);
 		object = get_reference(revs, arg, sha1, flags ^ local_flags);
 		add_pending_object(revs, object, arg);
 	}
diff --git a/setup.c b/setup.c
index cce9bb8..fe7f884 100644
--- a/setup.c
+++ b/setup.c
@@ -80,11 +80,31 @@ void verify_filename(const char *prefix,
 	if (!lstat(name, &st))
 		return;
 	if (errno == ENOENT)
-		die("ambiguous argument '%s': unknown revision or filename\n"
-		    "Use '--' to separate filenames from revisions", arg);
+		die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
+		    "Use '--' to separate paths from revisions", arg);
 	die("'%s': %s", arg, strerror(errno));
 }
 
+/*
+ * Opposite of the above: the command line did not have -- marker
+ * and we parsed the arg as a refname.  It should not be interpretable
+ * as a filename.
+ */
+void verify_non_filename(const char *prefix, const char *arg)
+{
+	const char *name;
+	struct stat st;
+
+	if (*arg == '-')
+		return; /* flag */
+	name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
+	if (!lstat(name, &st))
+		die("ambiguous argument '%s': both revision and filename\n"
+		    "Use '--' to separate filenames from revisions", arg);
+	if (errno != ENOENT)
+		die("'%s': %s", arg, strerror(errno));
+}
+
 const char **get_pathspec(const char *prefix, const char **pathspec)
 {
 	const char *entry = *pathspec;
-- 
1.3.1.ga6c7