Re: [PATCH 1/4] parse-options: introduce OPT_PATH

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Feb 24, 2015, 15:49 UTC
Message-ID: <54EC9D85.3010301@drmicha.warpmail.net>
In-Reply-To: <xmqqbnkktoti.fsf@gitster.dls.corp.google.com>

Junio C Hamano venit, vidit, dixit 23.02.2015 20:23:

Show 20 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Many options are paths, but not files. Introduce OPT_PATH which does
>> the same path processing as OPT_FILENAME but allows to name the argument.
>> ...
>> diff --git a/parse-options.h b/parse-options.h
>> index 7940bc7..5127a5d 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -149,6 +149,8 @@ struct option {
>>  	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }
>>  #define OPT_FILENAME(s, l, v, h)    { OPTION_FILENAME, (s), (l), (v), \
>>  				       N_("file"), (h) }
>> +#define OPT_PATH(s, l, v, a, h)    { OPTION_FILENAME, (s), (l), (v), \
>> +				       (a), (h) }
> 
> I am somewhat disappointed to see this implementation.
> 
>  - I expected that OPT_FILENAME will be re-implemented in terms of
>    OPT_PATH(), as a thin wrapper.

Right now they are just two macros. Would

#define OPT_FILENAME(s, l, v, h) OPT_PATH((s), (l), (v), N_("file"), (h))

be more what you expect? I don't consider that thinner but don't mind either way.

Show 22 quoted lines

>  - As the original complaint was "checkout --to requires a
>    directory, and a file would not work", I expected this to give
>    the programmer finer controls, such as:
> 
>     - The name must refer to an existing entity on the filesystem,
>       or an existing entity on the filesystem must not exist, or
>       anything is fine (tristate).
> 
>     - The name refers to a diretory, or the name refers to a regular
>       file, or the name refers to a symbolic link, or anything goes.
> 
> That is merely "somewhat", as the latter _can_ be coded (e.g. if you
> care that the given path already exists as a directory, stat() it
> and see if it is, or if you want that the given path does not exist
> yet, stat() it to make sure you get ENOENT) after the option is
> parsed by the program that uses the parser.
> 
> But the infrastructure to allow the latter would free you from
> having to say N_("file") or N_("directory"); if a parameter can
> refer to either a file or a directory, the parse-options library
> could give you N_("file or directory") because you are already
> telling what kind(s) of paths are allowed.

So, do you suggest to extend OPTION_FILENAME, and introduce several macros using it, or a macro taking a bitfield to be filled with PATH_OPT_FILE, PATH_OPT_DIR, PATH_OPT_EXISTS, PATH_OPT_ABSENT, PATH_OPT_MASK, PATH_OPT_MODE (require (mode & MASK == MODE))?

Sounds like the big solution to a small problem I had with the word "file" for a dir...

>>  #define OPT_COLOR_FLAG(s, l, v, h) \
>>  	{ OPTION_CALLBACK, (s), (l), (v), N_("when"), (h), PARSE_OPT_OPTARG, \
>>  		parse_opt_color_flag_cb, (intptr_t)"always" }