threads / patch / 38633

patchcheckout: --to requires directory

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

## tl;dr

18 messages between Feb 23, 2015 and Feb 24, 2015. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

Michael J Gruber· Feb 23, 2015, 14:16 UTC · lore
--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(-)
Show changes to builtin/checkout.c +1 −1
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
Jeff King· Feb 23, 2015, 14:42 UTC · re: Michael J Gruber · lore

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

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
Michael J Gruber· Feb 23, 2015, 15:01 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 23.02.2015 15:42:
Show 13 quoted lines
> 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
Michael J Gruber· Feb 23, 2015, 16:17 UTC · re: Jeff King · lore

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

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
Michael J Gruber· Feb 23, 2015, 16:17 UTC · re: Jeff King · lore

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

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(+)
Show changes to 2 files +7 −0

Documentation/technical/api-parse-options.txt, parse-options.h

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
Junio C Hamano· Feb 23, 2015, 19:23 UTC · re: Michael J Gruber · lore

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

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" }
Michael J Gruber· Feb 24, 2015, 15:49 UTC · re: Junio C Hamano · lore

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

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" }
Philip Oakley· Feb 23, 2015, 19:45 UTC · re: Michael J Gruber · lore

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

From: "Michael J Gruber" <git@drmicha.warpmail.net>
Show 22 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.
>
> 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.

Show 25 quoted lines
> + 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
Michael J Gruber· Feb 23, 2015, 16:17 UTC · re: Jeff King · lore

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

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(-)
Show changes to 7 files +9 −9

archive.c, builtin/archive.c, builtin/blame.c, builtin/config.c, builtin/fast-export.c, builtin/hash-object.c, builtin/ls-files.c

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
Jeff King· Feb 23, 2015, 17:44 UTC · re: Michael J Gruber · lore

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

On Mon, Feb 23, 2015 at 05:17:44PM +0100, Michael J Gruber wrote:
Show 13 quoted lines
> 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.

Show 13 quoted lines
> 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.

Show 12 quoted lines
> 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.
Show 13 quoted lines
> 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.

Show 16 quoted lines
> 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.
Show 11 quoted lines
> 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.

Show 13 quoted lines
> 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
Junio C Hamano· Feb 23, 2015, 19:08 UTC · re: Michael J Gruber · lore

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

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?

Show 9 quoted lines
>
>  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(-)
Junio C Hamano· Feb 23, 2015, 20:31 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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 ;-)
Michael J Gruber· Feb 23, 2015, 16:17 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +4 −4

builtin/clone.c, builtin/init-db.c

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
Jeff King· Feb 23, 2015, 18:26 UTC · re: Michael J Gruber · lore

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

On Mon, Feb 23, 2015 at 05:17:45PM +0100, Michael J Gruber wrote:
Show 13 quoted lines
> 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
Junio C Hamano· Feb 23, 2015, 21:07 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> 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.

Jeff King· Feb 23, 2015, 21:12 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 23, 2015 at 01:07:13PM -0800, Junio C Hamano wrote:
Show 26 quoted lines
> >> -	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
Michael J Gruber· Feb 23, 2015, 16:17 UTC · re: Jeff King · lore

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

--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(-)
Show changes to builtin/checkout.c +1 −1
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
Duy Nguyen· Feb 24, 2015, 00:34 UTC · re: Michael J Gruber · lore

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

On Mon, Feb 23, 2015 at 9:16 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 17 quoted lines
> --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

← back to recent threads