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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2015, 19:23 UTC
Message-ID: <xmqqbnkktoti.fsf@gitster.dls.corp.google.com>
In-Reply-To: <cccf3d14e5582996e5be1467849121a262f9c9f1.1424707497.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 13 quoted lines

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

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

>  #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" }