checkout: --to requires directory

PATCH In review Michael J Gruber 5 participants 18 messages Jun 15, 2016 Documentation

lore

Share & tools

lore

Michael J Gruber

author

2016-06-15 23:03 UTC #0 lore raw

PATCH 1/1 checkout: --to requires directory

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

Signed-off-by Michael J Gruber <[email protected]>

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

builtin/checkout.c +1 -1

index 8b2bf20..8cdcd07 100644
@@ -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")),

Signature · 1 line

2.3.0.296.g32c87e1

Jeff King

2016-06-15 23:03 UTC #1 lore raw

Quoted 1 line · Michael J Gruber

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

Sounds like a good goal, but...

builtin/checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 1345

Quoted 3 lines

builtin/checkout.c +1 -1

-		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

author

2016-06-15 23:03 UTC #2 lore raw

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 <[email protected]>

---
 Documentation/technical/api-parse-options.txt | 5 +++++
 parse-options.h                               | 2 ++
 2 files changed, 7 insertions(+)

Documentation/technical/api-parse-options.txt +5 -0

index 1f2db31..109903c 100644
@@ -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[]`.

parse-options.h +2 -0

index 7940bc7..5127a5d 100644
@@ -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" }

Signature · 1 line

2.3.0.296.g32c87e1

Philip Oakley

2016-06-15 23:03 UTC #3 lore raw

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

From: "Michael J Gruber" <[email protected]>

Quoted 14 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 <[email protected]>
---
Documentation/technical/api-parse-options.txt | 5 +++++
parse-options.h | 2 ++
2 files changed, 7 insertions(+)
Documentation/technical/api-parse-options.txt +0 -0
index 1f2db31..109903c 100644

Documentation/technical/api-parse-options.txt There are some macros to easily define options: · line 192

Quoted 6 lines

Documentation/technical/api-parse-options.txt +3 -0

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

Quoted 18 lines

Documentation/technical/api-parse-options.txt +2 -0

+ the prefix argument of `parse_options()` to `prefix_filename()`.
+

OPT_ARGUMENT(long, description)::
Introduce a long-option argument that will be kept in argv[].

parse-options.h +2 -0

index 7940bc7..5127a5d 100644
@@ -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,

Quoted 6 lines

\
parse_opt_color_flag_cb, (intptr_t)"always" }
Signature · 3 lines
2.3.0.296.g32c87e1

--

Philip

Junio C Hamano

2016-06-15 23:03 UTC #4 lore raw

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

In reply to Michael J Gruber

parse-options.h struct option { · line 153

Quoted 13 lines · 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.
...
parse-options.h +2 -0
index 7940bc7..5127a5d 100644
@@ -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.

Quoted 3 lines

parse-options.h +0 -0

 #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

author

2016-06-15 23:03 UTC #5 lore raw

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

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

Quoted 18 lines

parse-options.h struct option { · line 153
Quoted 13 lines · 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.
...
parse-options.h +2 -0
index 7940bc7..5127a5d 100644
@@ -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.

Quoted 2 lines

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

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

Quoted 5 lines

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

Quoted 3 lines

parse-options.h +0 -0

 #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

author

2016-06-15 23:03 UTC #6 lore raw

PATCH 2/4 option-strings: use OPT_FILENAME

In reply to Jeff King

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 <[email protected]>

---
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(-)

archive.c +1 -1

index 94a9981..b4da255 100644
@@ -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")),

builtin/archive.c +1 -1

index a1e3b94..9c96681 100644
@@ -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>")),

builtin/blame.c +2 -2

index 303e217..9b14c7c 100644
@@ -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")),

builtin/config.c +1 -1

index 8cc2604..b80922c 100644
@@ -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),

builtin/fast-export.c +2 -2

index b8182c2..44f2600 100644
@@ -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")),

builtin/hash-object.c +1 -1

index 6158363..7b13940 100644
@@ -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;

builtin/ls-files.c +1 -1

index 99cee20..0ddd3a8 100644
@@ -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"),

Signature · 1 line

2.3.0.296.g32c87e1

Jeff King

2016-06-15 23:03 UTC #7 lore raw

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

archive.c static int parse_archive_args(int argc, const char **argv, · line 425

Quoted 13 lines · Michael J Gruber

archive.c +1 -1

index 94a9981..b4da255 100644
@@ -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.

builtin/archive.c int cmd_archive(int argc, const char **argv, const char *prefix) · line 91

Quoted 13 lines

builtin/archive.c +1 -1

index a1e3b94..9c96681 100644
@@ -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.

builtin/blame.c int cmd_blame(int argc, const char **argv, const char *prefix) · line 2518

Quoted 12 lines

builtin/blame.c +2 -2

index 303e217..9b14c7c 100644
@@ -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.

builtin/config.c static struct option builtin_config_options[] = { · line 60

Quoted 13 lines

builtin/config.c +1 -1

index 8cc2604..b80922c 100644
@@ -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.

Quoted 6 lines

builtin/fast-export.c +0 -0

index b8182c2..44f2600 100644
@@ -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"),

builtin/fast-export.c int cmd_fast_export(int argc, const char **argv, const char *prefix) · line 991

Quoted 10 lines

builtin/fast-export.c +2 -2

 			     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.

builtin/hash-object.c int cmd_hash_object(int argc, const char **argv, const char *prefix) · line 102

Quoted 11 lines

builtin/hash-object.c +1 -1

index 6158363..7b13940 100644
@@ -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.

builtin/ls-files.c int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) · line 495

Quoted 13 lines

builtin/ls-files.c +1 -1

index 99cee20..0ddd3a8 100644
@@ -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

2016-06-15 23:03 UTC #8 lore raw

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

In reply to Michael J Gruber

Quoted 2 lines · 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.

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?

Quoted 9 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

2016-06-15 23:03 UTC #9 lore raw

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

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · 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.

Quoted 8 lines

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

author

2016-06-15 23:03 UTC #10 lore raw

In reply to Jeff King

Jeff King venit, vidit, dixit 23.02.2015 15:42:

Quoted 6 lines · Michael J Gruber

Quoted 2 lines
--to requires a directory, not a file. Say so in the usage string.
Sounds like a good goal, but...

Quoted 7 lines

builtin/checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 1345
Quoted 3 lines
builtin/checkout.c +1 -1
-		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.

Quoted 3 lines

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

author

2016-06-15 23:03 UTC #11 lore raw

PATCH 3/4 option-strings: use OPT_PATH

In reply to Jeff King

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 <[email protected]>

---
 builtin/clone.c   | 4 ++--
 builtin/init-db.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

builtin/clone.c +2 -2

index a9af3f2..15941c5 100644
@@ -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")),

builtin/init-db.c +2 -2

index 6b7fa5f..262c9ae 100644
@@ -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()
 	};

Signature · 1 line

2.3.0.296.g32c87e1

Jeff King

2016-06-15 23:03 UTC #12 lore raw

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

builtin/clone.c static struct option builtin_clone_options[] = { · line 86

Quoted 13 lines · Michael J Gruber

builtin/clone.c +1 -1

index a9af3f2..15941c5 100644
@@ -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

2016-06-15 23:03 UTC #13 lore raw

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

Quoted 11 lines · Jeff King

Quoted 9 lines · Michael J Gruber

builtin/clone.c +0 -1

index a9af3f2..15941c5 100644
@@ -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"),

Quoted 8 lines

builtin/clone.c static struct option builtin_clone_options[] = { · line 86
Quoted 4 lines
builtin/clone.c +1 -0
+	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

2016-06-15 23:03 UTC #14 lore raw

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

Quoted 18 lines · Junio C Hamano

Quoted 9 lines
builtin/clone.c static struct option builtin_clone_options[] = { · line 86
Quoted 5 lines
builtin/clone.c +1 -1
-	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",

Quoted 8 lines

    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

author

2016-06-15 23:03 UTC #15 lore raw

PATCH 4/4 checkout: --to requires directory

In reply to Jeff King

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

Signed-off-by Michael J Gruber <[email protected]>

---
I did not spot any other "misuses" of OPT_FILENAME (for non-files).

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

builtin/checkout.c +1 -1

index 8b2bf20..4256560 100644
@@ -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")),

Signature · 1 line

2.3.0.296.g32c87e1

Michael J Gruber

author

2016-06-15 23:03 UTC #16 lore raw

cover OPT_{FILENAME,PATH}

In reply to Jeff King

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(-)

Signature · 1 line

2.3.0.296.g32c87e1

Duy Nguyen

2016-06-15 23:03 UTC #17 lore raw

In reply to Michael J Gruber

Quoted 12 lines · Michael J Gruber

--to requires a directory, not a file. Say so in the usage string.
Signed-off-by Michael J Gruber <[email protected]>
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/checkout.c +0 -0
index 8b2bf20..8cdcd07 100644

builtin/checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 1344

Quoted 5 lines

builtin/checkout.c +1 -1

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

Quoted 3 lines

builtin/checkout.c +0 -0

                           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")),

Signature · 1 line

Duy