threads / discuss / 3947

lstat() call in rev-parse.c

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

## tl;dr

8 messages between Apr 23, 2006 and Apr 26, 2006.

replies: 7people: 4as markdown or json

Paul Mackerras· Apr 23, 2006, 12:03 UTC · lore

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.
Linus Torvalds· Apr 23, 2006, 16:19 UTC · re: Paul Mackerras · lore

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

On Sun, 23 Apr 2006, Paul Mackerras wrote:
Show 8 quoted lines
>
> 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
Paul Mackerras· Apr 24, 2006, 23:23 UTC · re: Linus Torvalds · lore

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

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.
Matthias Lederhofer· Apr 26, 2006, 15:28 UTC · re: Linus Torvalds · lore

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

> 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 --.

Linus Torvalds· Apr 26, 2006, 15:43 UTC · re: Matthias Lederhofer · lore

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

On Wed, 26 Apr 2006, Matthias Lederhofer wrote:
Show 8 quoted lines
> > 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
Linus Torvalds· Apr 26, 2006, 17:15 UTC · re: Linus Torvalds · lore

[PATCH] Fix filename verification when in a subdirectory

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:
Show 15 quoted lines
> > 
> > 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;
Timo Hirvonen· Apr 26, 2006, 18:05 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@osdl.org> wrote:
Show 11 quoted lines
> +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/
Linus Torvalds· Apr 26, 2006, 18:14 UTC · re: Timo Hirvonen · lore

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

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

← back to recent threads