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

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@imag.fr> 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.

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

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

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

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

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