Re: "Detailed diagnosis" perhaps broken

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: "Detailed diagnosis" perhaps broken

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:54:07

Junio C Hamano [off-list ref] writes:

	$ git log COPYING HEAD^:COPYING
	fatal: Path 'COPYING' exists on disk, but not in 'HEAD^'.

Oops!

quoted hunk

--- a/sha1_name.c
+++ b/sha1_name.c

@@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			if (new_filename)
 				filename = new_filename;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
-			if (only_to_die) {
+			if (only_to_die && ret) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);

This is one obvious thing to do. We should never call
diagnose_invalid_sha1_path if the search done by get_tree_entry
succeeded. A patch follows with a proper test-case.

But that isn't sufficient unfortunately. The other question here is: why
did we even try calling get_tree_entry, if we're not looking for an
object at all? Indeed, if get_tree_entry fails, we get:

  $ git log COPYING HEAD:foo
  fatal: Path 'foo' does not exist in 'HEAD'

At least, the message is correct in that foo does not exist in HEAD, but
not accurate in the sense that it is not the reason for the error.

So, the other fix should be to distinguish from the caller of
verify_filename whether we should try the detailed diagnosis including
sha1_name_*, or just die and complain about path not being in the
working tree. Another patch follows doing that.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:54:07

diagnose_invalid_sha1_path is normally meant to be called to diagnose
<treeish>:<pathname> when <pathname> does not exist in <treeish>.
However, the current code may call it if <treeish>:<pathname> is invalid
(which triggers another call with only_to_die == 1), but for another
reason. This happens when calling e.g.

  git log existing-file HEAD:existing-file

(because existing-file is a file and not a revision, the next arguments
are assumed to be files too), leading to incorrect message like
"existing-file does not exist in HEAD".

Check that the search for <pathname> in <treeish> fails before triggering
the diagnosis.

Bug report and code fix by: Junio C Hamano [off-list ref]
Test by: Matthieu Moy [off-list ref]

Signed-off-by: Matthieu Moy <redacted>
---

This patch is very simple and should be rather uncontroversial.

 sha1_name.c                    |  2 +-
 t/t1506-rev-parse-diagnosis.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index c633113..5d81ea0 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -1127,7 +1127,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			if (new_filename)
 				filename = new_filename;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
-			if (only_to_die) {
+			if (ret && only_to_die) {
 				diagnose_invalid_sha1_path(prefix, filename,
 							   tree_sha1, object_name);
 				free(object_name);

diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 0843a1c..4a39ac5 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh

@@ -171,4 +171,15 @@ test_expect_success 'relative path when startup_info is NULL' '
 	grep "BUG: startup_info struct is not initialized." error
 '
 
+test_expect_success '<commit>:file correctly diagnosed after a pathname' '
+	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
+	test_i18ngrep ! "exists on disk" error &&
+	test_i18ngrep "unknown revision or path not in the working tree" error &&
+	cat >expect <<EOF &&
+file.txt
+HEAD:file.txt
+EOF
+	test_cmp expect actual
+'
+
 test_done

-- 
1.7.11.rc0.57.g84a04c7

[PATCH 2/2 RFC] verify_filename: ask the caller to chose the kind of diagnosis

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:54:07

verify_filename can be called in two different contexts. Either we just
tried to interpret a string as an object name, and it fails, so we try
looking for a working tree file as a fallback, or we _know_ that we are
looking for a filename, and shouldn't even try interpreting the string as
an object name.

For example, with this change, we get:

  $ git log COPYING HEAD:inexistant
  fatal: HEAD:inexistant: no such path in the working tree.
  $ git log HEAD:inexistant
  fatal: Path 'inexistant' does not exist in 'HEAD'

Signed-off-by: Matthieu Moy <redacted>
---

This one is much less straightforward, hence the RFC. I did check all
the call sites of verify_filename, but I may not have fully understood
the logic at each call site.

A mistake in the diagnose_rev argument is not _that_ serious however,
since it does not change the potential failures, but only the error
message.

 builtin/grep.c                 | 7 +++++--
 builtin/reset.c                | 2 +-
 builtin/rev-parse.c            | 4 ++--
 cache.h                        | 9 ++++++++-
 revision.c                     | 5 +++--
 setup.c                        | 8 +++++---
 t/t1506-rev-parse-diagnosis.sh | 2 +-
 7 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index fe1726f..41924dc 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -927,8 +927,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	/* The rest are paths */
 	if (!seen_dashdash) {
 		int j;
-		for (j = i; j < argc; j++)
-			verify_filename(prefix, argv[j]);
+		if (i < argc) {
+			verify_filename(prefix, argv[i], 1);
+			for (j = i + 1; j < argc; j++)
+				verify_filename(prefix, argv[j], 0);
+		}
 	}
 
 	paths = get_pathspec(prefix, argv + i);

diff --git a/builtin/reset.c b/builtin/reset.c
index 8c2c1d5..4cc34c9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -285,7 +285,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			rev = argv[i++];
 		} else {
 			/* Otherwise we treat this as a filename */
-			verify_filename(prefix, argv[i]);
+			verify_filename(prefix, argv[i], 1);
 		}
 	}

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 733f626..13495b8 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c

@@ -486,7 +486,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 
 		if (as_is) {
 			if (show_file(arg) && as_is < 2)
-				verify_filename(prefix, arg);
+				verify_filename(prefix, arg, 0);
 			continue;
 		}
 		if (!strcmp(arg,"-n")) {

@@ -734,7 +734,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		as_is = 1;
 		if (!show_file(arg))
 			continue;
-		verify_filename(prefix, arg);
+		verify_filename(prefix, arg, 1);
 	}
 	if (verify) {
 		if (revs_count == 1) {

diff --git a/cache.h b/cache.h
index 06413e1..d1a4d9e 100644
--- a/cache.h
+++ b/cache.h

@@ -409,7 +409,14 @@ extern const char *setup_git_directory(void);
 extern 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 int check_filename(const char *prefix, const char *name);
-extern void verify_filename(const char *prefix, const char *name);
+/*
+ * Verify that "name" is a filename.
+ * The "diagnose_rev" is used to provide a user-friendly diagnosis. If
+ * 0, the diagnosis will try to diagnose "name" as an invalid object
+ * name (e.g. HEAD:foo). If non-zero, the diagnosis will only complain
+ * about an inexisting file.
+ */
+extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);
 extern void verify_non_filename(const char *prefix, const char *name);
 
 #define INIT_DB_QUIET 0x0001

diff --git a/revision.c b/revision.c
index 935e7a7..756196a 100644
--- a/revision.c
+++ b/revision.c

@@ -1780,8 +1780,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			 *     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]);
+			verify_filename(revs->prefix, arg, 1);			
+			for (j = i + 1; j < argc; j++)
+				verify_filename(revs->prefix, argv[j], 0);
 
 			append_prune_data(&prune_data, argv + i);
 			break;

diff --git a/setup.c b/setup.c
index 731851a..ca33092 100644
--- a/setup.c
+++ b/setup.c

@@ -53,11 +53,13 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
-static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+static void NORETURN die_verify_filename(const char *prefix, const char *arg, int diagnose_rev)
 {
 	unsigned char sha1[20];
 	unsigned mode;
 
+	if (!diagnose_rev)
+		die("%s: no such path in the working tree.", arg);
 	/*
 	 * Saying "'(icase)foo' does not exist in the index" when the
 	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec

@@ -81,13 +83,13 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
  * 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)
+void verify_filename(const char *prefix, const char *arg, int diagnose_rev)
 {
 	if (*arg == '-')
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die_verify_filename(prefix, arg);
+	die_verify_filename(prefix, arg, diagnose_rev);
 }
 
 /*

diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4a39ac5..a3f11e8 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh

@@ -174,7 +174,7 @@ test_expect_success 'relative path when startup_info is NULL' '
 test_expect_success '<commit>:file correctly diagnosed after a pathname' '
 	test_must_fail git rev-parse file.txt HEAD:file.txt 1>actual 2>error &&
 	test_i18ngrep ! "exists on disk" error &&
-	test_i18ngrep "unknown revision or path not in the working tree" error &&
+	test_i18ngrep "no such path in the working tree" error &&
 	cat >expect <<EOF &&
 file.txt
 HEAD:file.txt

-- 
1.7.11.rc0.57.g84a04c7

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help