Re: [PATCH 2/4] option-strings: use OPT_FILENAME

[Jeff King <peff@peff.net>]

On Mon, Feb 23, 2015 at 05:17:44PM +0100, Michael J Gruber wrote:

> diff --git a/archive.c b/archive.c
> index 94a9981..b4da255 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -419,7 +419,7 @@ static int parse_archive_args(int argc, const char **argv,
>  		OPT_STRING(0, "format", &format, N_("fmt"), N_("archive format")),
>  		OPT_STRING(0, "prefix", &base, N_("prefix"),
>  			N_("prepend prefix to each pathname in the archive")),
> -		OPT_STRING('o', "output", &output, N_("file"),
> +		OPT_FILENAME('o', "output", &output,
>  			N_("write the archive to this file")),
>  		OPT_BOOL(0, "worktree-attributes", &worktree_attributes,
>  			N_("read .gitattributes in working directory")),

This one is rather tricky...if you look down a few lines, you will see
that we just die() when the option is specified. The point is that
--output is not supposed to make it down to this level (it gets
intercepted by cmd_archive first), and we would not want to respect it
for a remote invocation (ditto for --exec and --remote options).

It's a little weird that we have it here at all (after all, if we
omitted it, we would _also_ complain here). But I guess the "-h" text is
generated by this parse_options invocation. That seems to be confirmed
by the commit messages of 4fac1d3a98 and 52e7787609.

So I think your change has literally no impact; we do not care about
prefixing or not here, and the help text remains the same (because it
already said "file"). I am not sure which is better from a readability
standpoint. On the one hand, it is clear that we should match the option
in cmd_archive, since we are printing the help for it here. But we would
never want to act in any way on the filename-properties of this string
(e.g., if we ever started looking at the filesystem as part of
fix_filename(), this would leak information if a malicious client sent
"--output=foo" to a git-upload-archive server). That's probably a rather
unlikely scenario, though.

> diff --git a/builtin/archive.c b/builtin/archive.c
> index a1e3b94..9c96681 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -85,7 +85,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>  	const char *output = NULL;
>  	const char *remote = NULL;
>  	struct option local_opts[] = {
> -		OPT_STRING('o', "output", &output, N_("file"),
> +		OPT_FILENAME('o', "output", &output,
>  			N_("write the archive to this file")),
>  		OPT_STRING(0, "remote", &remote, N_("repo"),
>  			N_("retrieve the archive from remote repository <repo>")),

So this one is the flip-side of the parse_archive_args() change above.
Any changes to the help-string here will _not_ get shown (we use
PARSE_OPT_KEEP_ALL, which implies PARSE_OPT_NO_INTERNAL_HELP, and the
"-h" gets passed down to parse_archive_args parser).

But it should be changing the behavior of "-o" to properly handle
relative paths, which is a good thing. Except it doesn't. :)

The entry in git.c:commands for cmd_archive does not mention
"RUN_SETUP", so we will not have actually chdir'd here. It already works
fine.

I do not think using OPT_FILENAME here is _hurting_ anything, though.
The "prefix" variable will always be NULL, and therefore fix_filename()
will be a noop. And the code is hopefully more obvious as a result. So
I'd be OK with this change, even though it technically does nothing.

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 303e217..9b14c7c 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2514,8 +2514,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
>  		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
>  		OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
> -		OPT_STRING('S', NULL, &revs_file, N_("file"), N_("Use revisions from <file> instead of calling git-rev-list")),
> -		OPT_STRING(0, "contents", &contents_from, N_("file"), N_("Use <file>'s contents as the final image")),
> +		OPT_FILENAME('S', NULL, &revs_file, N_("Use revisions from <file> instead of calling git-rev-list")),
> +		OPT_FILENAME(0, "contents", &contents_from, N_("Use <file>'s contents as the final image")),

These ones _are_ actually doing something, and it seems like a strict
improvement. E.g.:

  cd t &&
  git blame --contents=test-lib.sh test-lib.sh

does not work without your patch.

> diff --git a/builtin/config.c b/builtin/config.c
> index 8cc2604..b80922c 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -54,7 +54,7 @@ static struct option builtin_config_options[] = {
>  	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
>  	OPT_BOOL(0, "system", &use_system_config, N_("use system config file")),
>  	OPT_BOOL(0, "local", &use_local_config, N_("use repository config file")),
> -	OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
> +	OPT_FILENAME('f', "file", &given_config_source.file, N_("use given config file")),
>  	OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
>  	OPT_GROUP(N_("Action")),
>  	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),

This one introduces a bug. We already handle prepending the prefix to
the filename later in cmd_config(). Doing:

  cd t &&
  git config --file=../.git/config --list

works, but with your patch produces:

  fatal: unable to read config file 't/t/../.git/config': No such file or directory

In theory we can get rid of the manual handling in cmd_config in favor
of using OPT_FILENAME. There are some spots where we assign to
given_config_source.file after parsing options but before doing the
fixup, but I think they should all be absolute paths (they come from
home_config_paths(), so unless you have a relative home directory...).

It might be worth double-checking the test coverage before touching this
area too much, though.

> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index b8182c2..44f2600 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -983,9 +983,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
>  		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
>  			     N_("select handling of tags that tag filtered objects"),
>  			     parse_opt_tag_of_filtered_mode),
> -		OPT_STRING(0, "export-marks", &export_filename, N_("file"),
> +		OPT_FILENAME(0, "export-marks", &export_filename,
>  			     N_("Dump marks to this file")),
> -		OPT_STRING(0, "import-marks", &import_filename, N_("file"),
> +		OPT_FILENAME(0, "import-marks", &import_filename,
>  			     N_("Import marks from this file")),
>  		OPT_BOOL(0, "fake-missing-tagger", &fake_missing_tagger,
>  			 N_("Fake a tagger when tags lack one")),

These ones look like a straightforward improvement.

> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index 6158363..7b13940 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -98,7 +98,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL( 0 , "stdin-paths", &stdin_paths, N_("read file names from stdin")),
>  		OPT_BOOL( 0 , "no-filters", &no_filters, N_("store file as is without filters")),
>  		OPT_BOOL( 0, "literally", &literally, N_("just hash any random garbage to create corrupt objects for debugging Git")),
> -		OPT_STRING( 0 , "path", &vpath, N_("file"), N_("process file as it were from this path")),
> +		OPT_FILENAME( 0 , "path", &vpath, N_("process file as it were from this path")),
>  		OPT_END()

I'm not sure about this one. hash-object does not do SETUP_GIT
automatically, so at the time of the parsing, our prefix is empty. So
it's always a noop.

Later, we _do_ tack the prefix on here, but only after we have called
setup_git_directory (and only if we are writing out the object, since
otherwise we do not need to be in a .git directory at all).

So using OPT_FILENAME here doesn't produce any wrong behavior, but it is
potentially quite confusing.

> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 99cee20..0ddd3a8 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -489,7 +489,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>  		{ OPTION_CALLBACK, 'X', "exclude-from", &dir, N_("file"),
>  			N_("exclude patterns are read from <file>"),
>  			0, option_parse_exclude_from },
> -		OPT_STRING(0, "exclude-per-directory", &dir.exclude_per_dir, N_("file"),
> +		OPT_FILENAME(0, "exclude-per-directory", &dir.exclude_per_dir,
>  			N_("read additional per-directory exclude patterns in <file>")),
>  		{ OPTION_CALLBACK, 0, "exclude-standard", &dir, NULL,
>  			N_("add the standard git exclusions"),

I don't think this is right. The per-directory exclude path gets tacked
onto each directory we traverse. It is not a single path in itself, and
prepending our current prefix to it would definitely be the wrong thing
(if you were in "t/", we would not want to look for "Documentation/t/.gitignore").

-Peff