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

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;

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