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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2012, 16:27 UTC
Message-ID: <7vvcio4mbe.fsf@alter.siamese.dyndns.org>
In-Reply-To: <vpq62apt92f.fsf@bauges.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 11 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

While I do not think that is likely to happen, I did think about that, but then it started to feel somewhat troublesome that we do not have symmetry "syntactically this place we have to have rev but it could be a misspelled pathname" and I stopped.

If I mentally rephrase diagnose_rev to could_have_been_a_rev as you later explained, it sort-of makes sense, and "diagnose_rev" hints that, so probably it is not too bad. Maybe "diagnose_misspelt_rev" would have given a stronger hint, though.

Show 9 quoted lines

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

I agree a better documentation is called for. It is not "in most cases", but "in all cases where the caller has to guess the boundary between revs (that come earlier on the command line) and paths (that come later), pass 1 in diagnose_misspelt_rev when checking the first argument that was not a rev, because it is not a path but is a rev that the user misspelled" or something like that.

I also thought that verify_filename_or_diagnose_misspelt_rev() as a separate helper function might be cleaner, but by changing the signature of an existing function you made sure that all the exicting calls appear in the same patch and get eyeballed for the correctness of the converison, which was good.