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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Junio C Hamano <gitster@pobox.com> writes:

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

verify_filename will check the same thing in both cases. If the caller
looks like

if (name is not a valid object name) {
        verify_filename(name);
}

then it should ask for a detailed diagnosis. If the caller knows that an
object name would not be accepted anyway, it should not.

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

In general, this is the case, but that's a consequence of "an object
name would not be accepted anyway". I don't think there is any such call
in Git's code source right now, but we could imagine a caller trying to
verify that something is actually a file, and "verify_filename" would be
a correct way to do it, provided you pass diagnose_rev == 0.

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

I buy that.

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

Yes, and this is what we are doing already. This verify_filename is only
called for the first argument. We have exactly the right pattern here:

		/*
		 * Otherwise, argv[i] could be either <rev> or <paths> and
		 * has to be unambiguous.
		 */
		else if (!get_sha1(argv[i], sha1)) {
			verify_non_filename(prefix, argv[i]);
		} else {
			/* Otherwise we treat this as a filename */
			verify_filename(prefix, argv[i], 1);
		}

Clearly, if "argv[i]" is a filename, it's OK and we take it as it is,
but if it is not, then the failure is due to both "verify_filename" and
"git_sha1" failures, and we should take that into account in the
diagnosis. To me, the fact that this is called for the first non-rev
argument is a detail, the real reason to pass 1 here is that we wouldn't
have called verify_filename if it was a revision.

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

I considered "could_have_been_a_rev" or
"would_have_been_ok_if_it_was_a_rev" ;-).

I think it would be better to document that as a comment, like this in
cache.h:

   * In most cases, the caller will want diagnose_rev == 1 when
   * verifying the first non_rev argument, and diagnose_rev == 0 for the
   * next ones (because we already saw a filename, there's not ambiguity
   * anymore).
   */
  extern void verify_filename(const char *prefix, const char *name, int diagnose_rev);
  
but keep a param name that is more general.

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