Re: [PATCH 3/4] option-strings: use OPT_PATH

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2015, 21:07 UTC
Message-ID: <xmqqd250s5fi.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150223182653.GB19904@peff.net>

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

> On Mon, Feb 23, 2015 at 05:17:45PM +0100, Michael J Gruber wrote:
>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index a9af3f2..15941c5 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -80,7 +80,7 @@ static struct option builtin_clone_options[] = {
>>  		    N_("initialize submodules in the clone")),
>>  	OPT_BOOL(0, "recurse-submodules", &option_recursive,
>>  		    N_("initialize submodules in the clone")),
>> -	OPT_STRING(0, "template", &option_template, N_("template-directory"),
>> +	OPT_PATH(0, "template", &option_template, N_("template-directory"),
>>  		   N_("directory from which templates will be used")),
>>  	OPT_CALLBACK(0 , "reference", &option_reference, N_("repo"),
>>  		     N_("reference repository"), &opt_parse_reference),
>
> I'm not sure if this one is doing anything. Clone cannot use SETUP_GIT
> for obvious reasons, so we should have a NULL prefix here. But that also
> means we should be doing the right thing already.

I somehow thought that OPT_FILENAME already used expand_user_path() but apparently it does not. It may want to.

And then this change will start to matter, as a good enhancement.

Of course, if OPT_PATH() is introduced in such a way that the program that uses the API can ask for "existing" and/or "directory",

    git clone --template=existing-file $URL
    git clone --template=no-such-directory $URL

can be diagnosed as an error without the program having to code very much.

So, I agree that this change does not do anything in the current codebase, but it goes in a right direction.