lstat() call in rev-parse.c

lstat() call in rev-parse.c

[Paul Mackerras]

Why does git-rev-parse do an lstat on some of its arguments, at line
345 of rev-parse.c, and die if the lstat fails?  It doesn't seem to do
anything with the result.

The effect is that if you do "gitk a b", it works as long as a and b
exist (as files or directories), but fails if they don't, and some
users have found this confusing.  Yes they should put in a --, but
it's not obvious to users why this should make it work in the case
when a or b doesn't exist.

(And yes I just took out the git-rev-parse call from gitk, but I'm
going to need to do git-rev-parse --no-refs --no-flags for some
changes I'm doing at the moment.)

Paul.

Re: lstat() call in rev-parse.c

[Linus Torvalds]

On Sun, 23 Apr 2006, Paul Mackerras wrote:
>
> Why does git-rev-parse do an lstat on some of its arguments, at line
> 345 of rev-parse.c, and die if the lstat fails?  It doesn't seem to do
> anything with the result.
> 
> The effect is that if you do "gitk a b", it works as long as a and b
> exist (as files or directories), but fails if they don't, and some
> users have found this confusing.

Because it's even _more_ common to do

	gitk v2.6.16

(in the git directory) and be very confused when that returns an empty 
history. 

So the rule is: if you don't give that "--", then we have to be able to 
confirm that the filenames are really files. Not a misspelled revision 
name, or a revision name that was correctly spelled, but for the wrong 
project, because you were in the wrong subdirectory ;)

And yes, this actually happened to me. I was demonstrating git before we 
did that fix, and I used the wrong tag-name, and gitk would think for a 
minute and them show "No commits selected" with no error, because 
"v2.6.16" didn't exist at that time, and it used the "tag-name" as a 
filename, and that filename didn't actually exist, so the number of 
commits that changed it was exactly zero.

So now, if you do "gitk v2.6.16" in the git directory, it will return a 
nice and understandable 

	Error reading commits: fatal: 'v2.6.16': No such file or directory

which is _exactly_ what you want. The only problem is that gitk will 
actually open the big window too, and the error window is small and can be 
non-obvious if the window manager hides it in some corner. So I actually 
think you should try to make the error window come up smack dab in front 
of the main gitk window and make the error clearer.

So the rules are simple:
 - the filenames are _always_ separated by "--"
 - we have a short-hand, which allows the "--" to be dropped iff it is 
   unambiguous

Where "unambiguous" means that
 - the revision name cannot possible be interpreted as a filename (we 
   don't check this part yet, but I think we should)
 - all filenames are obviously not revision names, since they not 
   only aren't valid revisions, they actually exist as files.

Otherwise, misspellings, typos, and thinkos result in total confusion.

		Linus

Re: lstat() call in rev-parse.c

[Paul Mackerras]

Linus Torvalds writes:

> So the rule is: if you don't give that "--", then we have to be able to 
> confirm that the filenames are really files. Not a misspelled revision 
> name, or a revision name that was correctly spelled, but for the wrong 
> project, because you were in the wrong subdirectory ;)

OK, fair enough.  In that case we need a better error message, so I
don't get people complaining that gitk can't show the history of files
that don't exist any more.  How about something like:

Argument "foo" is ambiguous - revision name or file/directory name?
Please put "--" before the list of filenames.

I'll hack up a patch to this effect.

Paul.

Re: lstat() call in rev-parse.c

[Matthias Lederhofer]

> So the rule is: if you don't give that "--", then we have to be able to 
> confirm that the filenames are really files. Not a misspelled revision 
> name, or a revision name that was correctly spelled, but for the wrong 
> project, because you were in the wrong subdirectory ;)

Shouldn't git rev-parse try to stat the file (additionally?) in the
current directory instead of the top git directory? git (diff|log|..)
seem to fail everytime in a subdirectory without --.

Re: lstat() call in rev-parse.c

[Linus Torvalds]

On Wed, 26 Apr 2006, Matthias Lederhofer wrote:

> > So the rule is: if you don't give that "--", then we have to be able 
> > to confirm that the filenames are really files. Not a misspelled 
> > revision name, or a revision name that was correctly spelled, but for 
> > the wrong project, because you were in the wrong subdirectory ;)
> 
> Shouldn't git rev-parse try to stat the file (additionally?) in the 
> current directory instead of the top git directory? git (diff|log|..) 
> seem to fail everytime in a subdirectory without --.

Good point. However, the reason for that is that it actually _does_ stat 
the file in the current directory, but it has done the 

	revs->prefix = setup_git_directory();

in the init path (and it does need to do that, since that's what figures 
out where the .git directory is, so that we can parse the revisions 
correctly).

And that "setup_git_directory()" will chdir() to the root of the project.

So the "lstat()" should probably take "revs->prefix" into account, the 
way get_pathspec() does. Ie we should probably use

	char *name = argv[i];
	if (rev->prefix)
		name = prefix_filename(rev->prefix, strlen(rev->prefix), name);
	if (lstat(name, ..) < 0)
		die(...)

instead of just a plain lstat().

Probably worth doing as a small helper funtion of its own (and get rid of 
the current "die_badfile()" - and do all of that inside the helper 
function).

Somebody?

		Linus

[PATCH] Fix filename verification when in a subdirectory

[Linus Torvalds]

When we are in a subdirectory of a git archive, we need to take the prefix 
of that subdirectory into accoung when we verify filename arguments.

Noted by Matthias Lederhofer

This also uses the improved error reporting for all the other git commands 
that use the revision parsing interfaces, not just git-rev-parse. Also, it 
makes the error reporting for mixed filenames and argument flags clearer 
(you cannot put flags after the start of the pathname list).

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---
On Wed, 26 Apr 2006, Linus Torvalds wrote:
> > 
> > Shouldn't git rev-parse try to stat the file (additionally?) in the 
> > current directory instead of the top git directory? git (diff|log|..) 
> > seem to fail everytime in a subdirectory without --.
> 
> Good point. However, the reason for that is that it actually _does_ stat 
> the file in the current directory, but it has done the 
> 
> 	revs->prefix = setup_git_directory();
> 
> in the init path (and it does need to do that, since that's what figures 
> out where the .git directory is, so that we can parse the revisions 
> correctly).
> 
> And that "setup_git_directory()" will chdir() to the root of the project.

diff --git a/cache.h b/cache.h
index 69801b0..4d8fabc 100644
--- a/cache.h
+++ b/cache.h
@@ -134,6 +134,7 @@ extern const char *setup_git_directory_g
 extern const char *setup_git_directory(void);
 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);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
diff --git a/rev-parse.c b/rev-parse.c
index 7f66ae2..62e16af 100644
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -160,14 +160,6 @@ static int show_file(const char *arg)
 	return 0;
 }
 
-static void die_badfile(const char *arg)
-{
-	if (errno != ENOENT)
-		die("'%s': %s", arg, strerror(errno));
-	die("'%s' is ambiguous - revision name or file/directory name?\n"
-	    "Please put '--' before the list of filenames.", arg);
-}
-
 int main(int argc, char **argv)
 {
 	int i, as_is = 0, verify = 0;
@@ -177,14 +169,12 @@ int main(int argc, char **argv)
 	git_config(git_default_config);
 
 	for (i = 1; i < argc; i++) {
-		struct stat st;
 		char *arg = argv[i];
 		char *dotdot;
 
 		if (as_is) {
 			if (show_file(arg) && as_is < 2)
-				if (lstat(arg, &st) < 0)
-					die_badfile(arg);
+				verify_filename(prefix, arg);
 			continue;
 		}
 		if (!strcmp(arg,"-n")) {
@@ -350,8 +340,7 @@ int main(int argc, char **argv)
 			continue;
 		if (verify)
 			die("Needed a single revision");
-		if (lstat(arg, &st) < 0)
-			die_badfile(arg);
+		verify_filename(prefix, arg);
 	}
 	show_default();
 	if (verify && revs_count != 1)
diff --git a/revision.c b/revision.c
index f9c7d15..f2a9f25 100644
--- a/revision.c
+++ b/revision.c
@@ -752,17 +752,15 @@ 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 */
-			for (j = i; j < argc; j++) {
-				if (lstat(argv[j], &st) < 0)
-					die("'%s': %s", argv[j], strerror(errno));
-			}
+			for (j = i; j < argc; j++)
+				verify_filename(revs->prefix, argv[j]);
+
 			revs->prune_data = get_pathspec(revs->prefix, argv + i);
 			break;
 		}
diff --git a/setup.c b/setup.c
index 36ede3d..119ef7d 100644
--- a/setup.c
+++ b/setup.c
@@ -62,6 +62,29 @@ const char *prefix_filename(const char *
 	return path;
 }
 
+/*
+ * Verify a filename that we got as an argument for a pathspec
+ * entry. Note that a filename that begins with "-" never verifies
+ * as true, because even if such a filename were to exist, we want
+ * it to be preceded by the "--" marker (or we want the user to
+ * use a format like "./-filename")
+ */
+void verify_filename(const char *prefix, const char *arg)
+{
+	const char *name;
+	struct stat st;
+
+	if (*arg == '-')
+		die("bad flag '%s' used after filename", arg);
+	name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
+	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("'%s': %s", arg, strerror(errno));
+}
+
 const char **get_pathspec(const char *prefix, const char **pathspec)
 {
 	const char *entry = *pathspec;

Re: [PATCH] Fix filename verification when in a subdirectory

[Timo Hirvonen]

Linus Torvalds <torvalds@osdl.org> wrote:

> +void verify_filename(const char *prefix, const char *arg)
> +{
> +	const char *name;
> +	struct stat st;
> +
> +	if (*arg == '-')
> +		die("bad flag '%s' used after filename", arg);
> +	name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
> +	if (!lstat(name, &st))
> +		return;
> +	if (errno == ENOENT);

Extra semicolon.

-- 
http://onion.dynserv.net/~timo/

Re: [PATCH] Fix filename verification when in a subdirectory

[Linus Torvalds]

On Wed, 26 Apr 2006, Timo Hirvonen wrote:
> 
> Extra semicolon.

Duh, indeed. It just didn't show up in any of the normal cases.

Junio, just apply without that stupid semicolon..

		Linus