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

[Jeff King <peff@peff.net>]

On Mon, Feb 23, 2015 at 01:07:13PM -0800, Junio C Hamano wrote:

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

Oh, I agree that annotating the option with more meaning is not a bad
thing. It may help readability, and as you note, we may grow new
features on those annotations over time. I mostly just got tired of
writing things along those lines by the time I finished with the
OPT_FILENAME patch (I guess OPT_PATH is the one more likely to grow new
features, though).

The one exception here is the thing I mentioned in parse_archive_args:
that one can be controlled by a remote caller and we would not want to
leak any information about which files exist, where the user's home
directory is, etc.

-Peff