[PATCH] checkout: --to requires directory

[PATCH] checkout: --to requires directory

[Michael J Gruber]

--to requires a directory, not a file. Say so in the usage string.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8b2bf20..8cdcd07 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1341,7 +1341,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			 N_("do not limit pathspecs to sparse entries only")),
 		OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
 				N_("second guess 'git checkout no-such-branch'")),
-		OPT_FILENAME(0, "to", &opts.new_worktree,
+		OPT_STRING(0, "to", &opts.new_worktree, N_("dir"),
 			   N_("check a branch out in a separate working directory")),
 		OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
 			 N_("do not check if another worktree is holding the given ref")),
-- 
2.3.0.296.g32c87e1

Re: [PATCH] checkout: --to requires directory

[Jeff King]

On Mon, Feb 23, 2015 at 03:16:59PM +0100, Michael J Gruber wrote:

> --to requires a directory, not a file. Say so in the usage string.

Sounds like a good goal, but...

> -		OPT_FILENAME(0, "to", &opts.new_worktree,
> +		OPT_STRING(0, "to", &opts.new_worktree, N_("dir"),
>  			   N_("check a branch out in a separate working directory")),

OPT_FILENAME also calls fix_filename(), which munges the filename so
that relative paths given by the user will work, even though git has
chdir'd to the root of the working tree.

So you need to handle that somewhere. I think it might be less painful
to teach OPT_FILENAME to be more flexible in the usage message it
prints.

-Peff

Re: [PATCH] checkout: --to requires directory

[Michael J Gruber]

Jeff King venit, vidit, dixit 23.02.2015 15:42:
> On Mon, Feb 23, 2015 at 03:16:59PM +0100, Michael J Gruber wrote:
> 
>> --to requires a directory, not a file. Say so in the usage string.
> 
> Sounds like a good goal, but...
> 
>> -		OPT_FILENAME(0, "to", &opts.new_worktree,
>> +		OPT_STRING(0, "to", &opts.new_worktree, N_("dir"),
>>  			   N_("check a branch out in a separate working directory")),
> 
> OPT_FILENAME also calls fix_filename(), which munges the filename so
> that relative paths given by the user will work, even though git has
> chdir'd to the root of the working tree.

Ooops, I wasn't aware of that. We do use OPT_STRING in quite a few
places for directories, though.

> So you need to handle that somewhere. I think it might be less painful
> to teach OPT_FILENAME to be more flexible in the usage message it
> prints.

We have at least path, template-dir dir which can benefit from that.

After Junio's call, I'm trying to look a bit at list-files and multiple
workdirs. I guess I should collect those bits.

BTW: multiple workdirs commit messages and doc talk about a hard link
named "link" in worktrees/<id>/, but I don't get any from "checkout --to".

Instead, and in addition to the expected "gitdir", I get a file
"gitfile" whose content is a relative path (*not* prefixed with "gitdir:
"), probably to the worktree .git gitfile (relative from the base
worktree). But "gitdir" "points to" the same, using (i.e. it contains)
an absolute path.

I can't quite make sense of this. Maybe the doc is behind? But still.
"gitdir" and "gitfile" pointing to the same file.

Michael

[PATCH 0/4] OPT_{FILENAME,PATH}

[Michael J Gruber]

Here's a "mini-war" on file-type arguments being specified as OPT_STRING
(in order to repent for doing so myself).

1/4 introduces OPT_PATH which is like OPT_FILENAME, but takes an additional
argument which names the option argument.
2/4 converts OPT_STRING to OPT_FILENAME where it makes sense
3/4 converts OPT_STRING to OPT_PATH where it makes sense
4/4 converts OPT_FILENAME to OPT_PATH for "checkout --to"

1/4, 2/4 should apply on top of origin/next independently.
3/4 needs 1/4.
4/4 needs 1/4 and is on top of nd/multiple-worktrees.

Michael J Gruber (4):
  parse-options: introduce OPT_PATH
  option-strings: use OPT_FILENAME
  option-strings: use OPT_PATH
  checkout: --to requires directory

 Documentation/technical/api-parse-options.txt | 5 +++++
 archive.c                                     | 2 +-
 builtin/archive.c                             | 2 +-
 builtin/blame.c                               | 4 ++--
 builtin/checkout.c                            | 2 +-
 builtin/clone.c                               | 4 ++--
 builtin/config.c                              | 2 +-
 builtin/fast-export.c                         | 4 ++--
 builtin/hash-object.c                         | 2 +-
 builtin/init-db.c                             | 4 ++--
 builtin/ls-files.c                            | 2 +-
 parse-options.h                               | 2 ++
 12 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.3.0.296.g32c87e1

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

[Michael J Gruber]

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.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/technical/api-parse-options.txt | 5 +++++
 parse-options.h                               | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 1f2db31..109903c 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -187,6 +187,11 @@ There are some macros to easily define options:
 	The filename will be prefixed by passing the filename along with
 	the prefix argument of `parse_options()` to `prefix_filename()`.
 
+`OPT_PATH(short, long, &var, arg_str, description)`::
+	Introduce an option with a path argument named arg_str.
+	The filename will be prefixed by passing the filename along with
+	the prefix argument of `parse_options()` to `prefix_filename()`.
+
 `OPT_ARGUMENT(long, description)`::
 	Introduce a long-option argument that will be kept in `argv[]`.
 
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) }
 #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" }
-- 
2.3.0.296.g32c87e1

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

[Junio C Hamano]

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.

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 23.02.2015 20:23:
> 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.

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

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

[Philip Oakley]

From: "Michael J Gruber" <git@drmicha.warpmail.net>
> 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.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> Documentation/technical/api-parse-options.txt | 5 +++++
> parse-options.h                               | 2 ++
> 2 files changed, 7 insertions(+)
>
> diff --git a/Documentation/technical/api-parse-options.txt 
> b/Documentation/technical/api-parse-options.txt
> index 1f2db31..109903c 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -187,6 +187,11 @@ There are some macros to easily define options:
>  The filename will be prefixed by passing the filename along with
>  the prefix argument of `parse_options()` to `prefix_filename()`.
>
> +`OPT_PATH(short, long, &var, arg_str, description)`::
> + Introduce an option with a path argument named arg_str.
> + The filename will be prefixed by passing the filename along with

I couldn't understand this. Should it be s/filename/pathname/ ? Or am I 
way off topic.

> + the prefix argument of `parse_options()` to `prefix_filename()`.
> +
> `OPT_ARGUMENT(long, description)`::
>  Introduce a long-option argument that will be kept in `argv[]`.
>
> 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) }
> #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" }
> -- 
> 2.3.0.296.g32c87e1
>
> --
Philip 

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

[Michael J Gruber]

Some commands use OPT_STRING to specify a <file> argument. Let them use
OPT_FILENAME so that they can profit from path prefixing.

This excludes low-level commands like the credential helpers.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
This could be before 1/4 but seemed more logical here.

 archive.c             | 2 +-
 builtin/archive.c     | 2 +-
 builtin/blame.c       | 4 ++--
 builtin/config.c      | 2 +-
 builtin/fast-export.c | 4 ++--
 builtin/hash-object.c | 2 +-
 builtin/ls-files.c    | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

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")),
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>")),
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")),
 		{ OPTION_CALLBACK, 'C', NULL, &opt, N_("score"), N_("Find line copies within and across files"), PARSE_OPT_OPTARG, blame_copy_callback },
 		{ OPTION_CALLBACK, 'M', NULL, &opt, N_("score"), N_("Find line movements within and across files"), PARSE_OPT_OPTARG, blame_move_callback },
 		OPT_STRING_LIST('L', NULL, &range_list, N_("n,m"), N_("Process only line range n,m, counting from 1")),
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),
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")),
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()
 	};
 	int i;
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"),
-- 
2.3.0.296.g32c87e1

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

[Jeff King]

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

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

[Junio C Hamano]

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

> Some commands use OPT_STRING to specify a <file> argument. Let them use
> OPT_FILENAME so that they can profit from path prefixing.

If the existing code that takes string actually already works
correctly, they must be doing the necessary prefixing on their own,
but I do not see in this patch a hunk that removes such a custom
prefixing.  Puzzled...

It may be that some of them do not work correctly from subdirectory
and this change fixes bugs, but are all of them like that, or is
this patch fixing some of them while breaking some others?
>
>  archive.c             | 2 +-
>  builtin/archive.c     | 2 +-
>  builtin/blame.c       | 4 ++--
>  builtin/config.c      | 2 +-
>  builtin/fast-export.c | 4 ++--
>  builtin/hash-object.c | 2 +-
>  builtin/ls-files.c    | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> Some commands use OPT_STRING to specify a <file> argument. Let them use
>> OPT_FILENAME so that they can profit from path prefixing.
>
> If the existing code that takes string actually already works
> correctly, they must be doing the necessary prefixing on their own,
> but I do not see in this patch a hunk that removes such a custom
> prefixing.  Puzzled...
>
> It may be that some of them do not work correctly from subdirectory
> and this change fixes bugs, but are all of them like that, or is
> this patch fixing some of them while breaking some others?

Ahh, I see Peff already went over the changes in the patch and
sifted them into good, questionable and bad bins.

Thanks; I'll shut up and await for a reroll ;-)

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

[Michael J Gruber]

Some commands use OPT_STRING to specify a path type argument. Let them
use OPT_PATH so that they can profit from path prefixing.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/clone.c   | 4 ++--
 builtin/init-db.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

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),
@@ -94,7 +94,7 @@ static struct option builtin_clone_options[] = {
 		    N_("create a shallow clone of that depth")),
 	OPT_BOOL(0, "single-branch", &option_single_branch,
 		    N_("clone only one branch, HEAD or --branch")),
-	OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
+	OPT_PATH(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
 		   N_("separate git dir from working tree")),
 	OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
 			N_("set config inside the new repository")),
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 6b7fa5f..262c9ae 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -477,7 +477,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
 	const struct option init_db_options[] = {
-		OPT_STRING(0, "template", &template_dir, N_("template-directory"),
+		OPT_PATH(0, "template", &template_dir, N_("template-directory"),
 				N_("directory from which templates will be used")),
 		OPT_SET_INT(0, "bare", &is_bare_repository_cfg,
 				N_("create a bare repository"), 1),
@@ -486,7 +486,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			N_("specify that the git repository is to be shared amongst several users"),
 			PARSE_OPT_OPTARG | PARSE_OPT_NONEG, shared_callback, 0},
 		OPT_BIT('q', "quiet", &flags, N_("be quiet"), INIT_DB_QUIET),
-		OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
+		OPT_PATH(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
 			   N_("separate git dir from working tree")),
 		OPT_END()
 	};
-- 
2.3.0.296.g32c87e1

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

[Jeff King]

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 think the same goes for the rest of the instances in this patch.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

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

[PATCH 4/4] checkout: --to requires directory

[Michael J Gruber]

--to requires a directory, not a file. Say so in the usage string.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I did not spot any other "misuses" of OPT_FILENAME (for non-files).

 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8b2bf20..4256560 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1341,7 +1341,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			 N_("do not limit pathspecs to sparse entries only")),
 		OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
 				N_("second guess 'git checkout no-such-branch'")),
-		OPT_FILENAME(0, "to", &opts.new_worktree,
+		OPT_PATH(0, "to", &opts.new_worktree, N_("dir"),
 			   N_("check a branch out in a separate working directory")),
 		OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
 			 N_("do not check if another worktree is holding the given ref")),
-- 
2.3.0.296.g32c87e1

Re: [PATCH] checkout: --to requires directory

[Duy Nguyen]

On Mon, Feb 23, 2015 at 9:16 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> --to requires a directory, not a file. Say so in the usage string.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  builtin/checkout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 8b2bf20..8cdcd07 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1341,7 +1341,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>                          N_("do not limit pathspecs to sparse entries only")),
>                 OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
>                                 N_("second guess 'git checkout no-such-branch'")),
> -               OPT_FILENAME(0, "to", &opts.new_worktree,
> +               OPT_STRING(0, "to", &opts.new_worktree, N_("dir"),

Nack. OPT_FILENAME has hidden magic, see fix_filename(). If you want
to change the text, you'll have to fall back to using OPTION_FILENAME
directly.

>                            N_("check a branch out in a separate working directory")),
>                 OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
>                          N_("do not check if another worktree is holding the given ref")),



-- 
Duy