Make git-rev-list and git-rev-parse argument parsing stricter

Linus Torvalds· Jan 25, 2006, 22:00 UTC · lore

If you pass it a filename without the "--" marker to separate it from revision information and flags, we now require that the file in question actually exists. This makes mis-typed revision information not be silently just considered a strange filename.

With the "--" marker, you can continue to pass in filenames that do not actually exists - useful for querying what happened to a file that you no longer have in the repository.

[ All scripts should use the "--" format regardless, to make things
  unambiguous. So this change should not affect any existing tools ]

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
----

I hit this the hard way when I was showing off git at Linux.Conf.Au yesterday, and I asked for

	git log v2.6.14..v2.6.16

which silently did nothing at all. The reason? I don't actually have a v2.6.16, the most recent version is 2.6.16-rc1. So it thought v2.6.14..v2.6.16 was a _filename_.

Now, normally, if you use the "proper" format (with the -- delimeter to mark where filenames start), you'd never have this ambiguity. But without the "--", the way I decided that we should disambiguate things is to verify that any filename given with the ambiguous shorthand version actually exists.

Comments?

diff --git a/rev-list.c b/rev-list.c
index d060966..e00e6fc 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -844,8 +844,12 @@ int main(int argc, const char **argv)
 			arg++;
 			limited = 1;
 		}
-		if (get_sha1(arg, sha1) < 0)
+		if (get_sha1(arg, sha1) < 0) {
+			struct stat st;
+			if (lstat(arg, &st) < 0)
+				die("'%s': %s", arg, strerror(errno));
 			break;
+		}
 		commit = get_commit_reference(arg, sha1, flags);
 		handle_one_commit(commit, &list);
 	}
diff --git a/rev-parse.c b/rev-parse.c
index 0c951af..7abad35 100644
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -154,6 +154,7 @@ int main(int argc, char **argv)
 	const char *prefix = setup_git_directory();
 	
 	for (i = 1; i < argc; i++) {
+		struct stat st;
 		char *arg = argv[i];
 		char *dotdot;
 	
@@ -293,6 +294,8 @@ int main(int argc, char **argv)
 		}
 		if (verify)
 			die("Needed a single revision");
+		if (lstat(arg, &st) < 0)
+			die("'%s': %s", arg, strerror(errno));
 		as_is = 1;
 		show_file(arg);
 	}

Linus Torvalds· Jan 25, 2006, 22:02 UTC · re: Linus Torvalds · lore

git-whatchanged: exit out early on errors

If we get an error parsing the arguments, exit.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This goes together with the previous diff - it just makes git-whatchanged exit gracefully when git-rev-parse errors out.

diff --git a/git-whatchanged.sh b/git-whatchanged.sh
index 80e2500..d4f985b 100755
--- a/git-whatchanged.sh
+++ b/git-whatchanged.sh
@@ -4,7 +4,7 @@ USAGE='[-p] [--max-count=<n>] [<since>..
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
-diff_tree_flags=$(git-rev-parse --sq --no-revs --flags "$@")
+diff_tree_flags=$(git-rev-parse --sq --no-revs --flags "$@") || exit
 test -z "$diff_tree_flags" &&
 	diff_tree_flags=$(git-repo-config --get whatchanged.difftree)
 test -z "$diff_tree_flags" &&

Junio C Hamano· Jan 25, 2006, 22:40 UTC · re: Linus Torvalds · lore

Re: Make git-rev-list and git-rev-parse argument parsing stricter

Linus Torvalds <torvalds@osdl.org> writes:

Show 6 quoted lines

> If you pass it a filename without the "--" marker to separate it from
> revision information and flags, we now require that the file in question
> actually exists. This makes mis-typed revision information not be silently
> just considered a strange filename.
>...
> Comments?

I think it is a good safety measure. People has to say "git rev-list -- git-commit-script" if they are interested in historical paths, but that is less common. We could even go stronger and always require '--', but that is probably too much for normal use. I like the balance you struck here.