Re: [PATCH 2/2 RFC] verify_filename: ask the caller to chose the kind of diagnosis
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:07
Matthieu Moy [off-list ref] writes:
+/* + * 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);
The whole point of verify_filename() is to make sure, because the user did not have disambiguating "--" on the command line, that the first non-rev argument is a path and also it cannot be interpreted as a valid rev. It somehow feels wrong to make it also responsible, for a possibly misspelled rev. The caller can mistakenly throw 0 or 1 at random but the _only_ right value for this parameter is to set it to true only for the first non-rev, no? Let's look at the patched sites.
quoted hunk
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); + }
This is exactly verify_filename(prefix, argv[j], j == first_non_rev)
quoted hunk
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);
This is also checking the first non-rev, too. We just saw
"florbl^{triee}" in "git reset florbl^{triee} hello.c" is not a
valid rev. If "florbl^{triee}" is indeed a file, we shouldn't
complain and die with "This may be a misspelled rev", but take it as
a path.
quoted hunk
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); }
These are, too.
quoted hunk
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);
Likewise.
quoted hunk
@@ -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);
And this implements the "if it is path, don't complain, but otherwise diagnose misspelled rev if the caller asked us to". I think the patch is not wrong per-se, but diagnose_rev is probably misnamed. It tells the callee what to do, but gives little hint to the caller when to set it. s/diagnose_rev/first_non_rev/ or something might make it easier to understand for future callers. Thanks.