rev-parse(1): logically group options

RFC/PATCH In review John Keeping 2 participants 6 messages Jun 15, 2016 Documentation

Share & tools

John Keeping

author

2016-06-15 22:58 UTC #0 lore raw

The options section of the git-rev-parse manual page has grown organically so that there now does not seem to be much logic behind the ordering of the options. It also does not make it clear that certain options must appear first on the command line.

Address this by reorganising the options into groups with subheadings. The text of option descriptions does not change.

Signed-off-by John Keeping <[email protected]>

---
 Documentation/git-rev-parse.txt | 104 +++++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 43 deletions(-)

Documentation/git-rev-parse.txt +61 ~~-43~~

index 993903c..fa284b0 100644
@@ -24,9 +24,35 @@ distinguish between them.
 
 OPTIONS
 -------
+
+Operation Modes
+~~~~~~~~~~~~~~~
+
+Each of these options must appear first on the command line.
+
+--local-env-vars::
+	List the GIT_* environment variables that are local to the
+	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
+	Only the names of the variables are listed, not their value,
+	even if they are set.
+
 --parseopt::
 	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
 
+--resolve-git-dir <path>::
+	Check if <path> is a valid repository or a gitfile that
+	points at a valid repository, and print the location of the
+	repository.  If <path> is a gitfile then the resolved path
+	to the real repository is printed.
+
+--sq-quote::
+	Use 'git rev-parse' in shell quoting mode (see SQ-QUOTE
+	section below). In contrast to the `--sq` option below, this
+	mode does only quoting. Nothing else is done to command input.
+
+Options for --parseopt
+~~~~~~~~~~~~~~~~~~~~~~
+
 --keep-dashdash::
 	Only meaningful in `--parseopt` mode. Tells the option parser to echo
 	out the first `--` met instead of skipping it.

@@ -36,10 +62,8 @@ OPTIONS
 	the first non-option argument.  This can be used to parse sub-commands
 	that take options themselves.
 
---sq-quote::
-	Use 'git rev-parse' in shell quoting mode (see SQ-QUOTE
-	section below). In contrast to the `--sq` option below, this
-	mode does only quoting. Nothing else is done to command input.
+Options for Filtering
+~~~~~~~~~~~~~~~~~~~~~
 
 --revs-only::
 	Do not output flags and parameters not meant for

@@ -55,6 +79,9 @@ OPTIONS
 --no-flags::
 	Do not output flag parameters.
 
+Options for Output
+~~~~~~~~~~~~~~~~~~
+
 --default <arg>::
 	If there is no parameter given by the user, use `<arg>`
 	instead.

@@ -110,6 +137,17 @@ can be used.
 	strip '{caret}' prefix from the object names that already have
 	one.
 
+--abbrev-ref[=(strict|loose)]::
+	A non-ambiguous short name of the objects name.
+	The option core.warnAmbiguousRefs is used to select the strict
+	abbreviation mode.
+
+--short::
+--short=number::
+	Instead of outputting the full SHA-1 values of object names try to
+	abbreviate them to a shorter unique name. When no length is specified
+	7 is used. The minimum length is 4.
+
 --symbolic::
 	Usually the object names are output in SHA-1 form (with
 	possible '{caret}' prefix); this option makes them output in a

@@ -123,16 +161,8 @@ can be used.
 	unfortunately named tag "master"), and show them as full
 	refnames (e.g. "refs/heads/master").
 
---abbrev-ref[=(strict|loose)]::
-	A non-ambiguous short name of the objects name.
-	The option core.warnAmbiguousRefs is used to select the strict
-	abbreviation mode.
-
---disambiguate=<prefix>::
-	Show every object whose name begins with the given prefix.
-	The <prefix> must be at least 4 hexadecimal digits long to
-	avoid listing each and every object in the repository by
-	mistake.
+Options for Input
+~~~~~~~~~~~~~~~~~
 
 --all::
 	Show all refs found in `refs/`.

@@ -155,18 +185,11 @@ shown.  If the pattern does not contain a globbing character (`?`,
 	character (`?`, `*`, or `[`), it is turned into a prefix
 	match by appending `/*`.
 
---show-toplevel::
-	Show the absolute path of the top-level directory.
-
---show-prefix::
-	When the command is invoked from a subdirectory, show the
-	path of the current directory relative to the top-level
-	directory.
-
---show-cdup::
-	When the command is invoked from a subdirectory, show the
-	path of the top-level directory relative to the current
-	directory (typically a sequence of "../", or an empty string).
+--disambiguate=<prefix>::
+	Show every object whose name begins with the given prefix.
+	The <prefix> must be at least 4 hexadecimal digits long to
+	avoid listing each and every object in the repository by
+	mistake.
 
 --git-dir::
 	Show `$GIT_DIR` if defined. Otherwise show the path to

@@ -177,6 +200,14 @@ If `$GIT_DIR` is not defined and the current directory
 is not detected to lie in a Git repository or work tree
 print a message to stderr and exit with nonzero status.
 
+--show-toplevel::
+	Show the absolute path of the top-level directory.
+
+--show-cdup::
+	When the command is invoked from a subdirectory, show the
+	path of the top-level directory relative to the current
+	directory (typically a sequence of "../", or an empty string).
+
 --is-inside-git-dir::
 	When the current working directory is below the repository
 	directory print "true", otherwise "false".

@@ -188,17 +219,10 @@ print a message to stderr and exit with nonzero status.
 --is-bare-repository::
 	When the repository is bare print "true", otherwise "false".
 
---local-env-vars::
-	List the GIT_* environment variables that are local to the
-	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
-	Only the names of the variables are listed, not their value,
-	even if they are set.
-
---short::
---short=number::
-	Instead of outputting the full SHA-1 values of object names try to
-	abbreviate them to a shorter unique name. When no length is specified
-	7 is used. The minimum length is 4.
+--show-prefix::
+	When the command is invoked from a subdirectory, show the
+	path of the current directory relative to the top-level
+	directory.
 
 --since=datestring::
 --after=datestring::

@@ -213,12 +237,6 @@ print a message to stderr and exit with nonzero status.
 <args>...::
 	Flags and parameters to be parsed.
 
---resolve-git-dir <path>::
-	Check if <path> is a valid repository or a gitfile that
-	points at a valid repository, and print the location of the
-	repository.  If <path> is a gitfile then the resolved path
-	to the real repository is printed.
-
 
 include::revisions.txt[]

Signature · 1 line

1.8.3.3.972.gc83849e.dirty

Junio C Hamano

2016-06-15 22:58 UTC #1 lore raw

Quoted 9 lines · John Keeping

The options section of the git-rev-parse manual page has grown organically so that there now does not seem to be much logic behind the ordering of the options. It also does not make it clear that certain options must appear first on the command line.
Address this by reorganising the options into groups with subheadings. The text of option descriptions does not change.
Signed-off-by John Keeping <[email protected]>

The idea to introduce a general grouping makes a lot of sense, I think.

Documentation/git-rev-parse.txt distinguish between them. · line 37

Quoted 10 lines

Documentation/git-rev-parse.txt +10 -0

+Operation Modes
+~~~~~~~~~~~~~~~
+
+Each of these options must appear first on the command line.
+
+--local-env-vars::
+	List the GIT_* environment variables that are local to the
+	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
+	Only the names of the variables are listed, not their value,
+	even if they are set.

Honestly speaking, "must appear first" for "--local-env-vars" is a bug in implementations of this option, I think. It does not make sense to ask

	git rev-parse --local-env-vars -- Makefile

and the command operates on "--" and "Makefile" in the normal operation mode, not "local-env-vars" mode.

Quoted 30 lines

Documentation/git-rev-parse.txt +21 -0

+
 --parseopt::
 	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
 
+--resolve-git-dir <path>::
+	Check if <path> is a valid repository or a gitfile that
+	points at a valid repository, and print the location of the
+	repository.  If <path> is a gitfile then the resolved path
+	to the real repository is printed.
+
+--sq-quote::
+	Use 'git rev-parse' in shell quoting mode (see SQ-QUOTE
+	section below). In contrast to the `--sq` option below, this
+	mode does only quoting. Nothing else is done to command input.
 
+Options for Input
+~~~~~~~~~~~~~~~~~
 
+--show-toplevel::
+	Show the absolute path of the top-level directory.
+
+--show-cdup::
+	When the command is invoked from a subdirectory, show the
+	path of the top-level directory relative to the current
+	directory (typically a sequence of "../", or an empty string).
+
 --is-inside-git-dir::
 	When the current working directory is below the repository
 	directory print "true", otherwise "false".

Documentation/git-rev-parse.txt print a message to stderr and exit with nonzero status. · line 225

Quoted 7 lines

Documentation/git-rev-parse.txt +4 -0

@@ -188,17 +219,10 @@ print a message to stderr and exit with nonzero status.
 --is-bare-repository::
 	When the repository is bare print "true", otherwise "false".
 
+--show-prefix::
+	When the command is invoked from a subdirectory, show the
+	path of the current directory relative to the top-level
+	directory.

I am not sure if --show-*, --is-*, and --git-dir belongs to "options for input". They are truly kitchen sink options to ask for various aspects of the repository and directory, and it may be equally valid (or even more valid) to consider them as separate operation modes.

John Keeping

author

2016-06-15 22:58 UTC #2 lore raw

v2 cover Improvements to rev-parse option handling

This adds a new patch to remove the restrictions on --local-env-var and --resolve-git-dir so that they do not need to appear first on the command line.

The other patch is update to reflect this as well as to split up the catch-all "Options for Input" subsection a bit.

John Keeping (2):
rev-parse: remove restrictions on some options
rev-parse(1): logically group options

 Documentation/git-rev-parse.txt | 104 ++++++++++++++++++++++++----------------
 builtin/rev-parse.c             |  28 +++++------
 2 files changed, 77 insertions(+), 55 deletions(-)

Signature · 1 line

1.8.3.3.972.gc83849e.dirty

John Keeping

author

2016-06-15 22:58 UTC #3 lore raw

PATCH v2 1/2 rev-parse: remove restrictions on some options

The "--local-env-vars" and "--resolve-git-dir" arguments to git-rev-parse are currently only handled if they appear first on the command line (in the case of "--local-env-vars", only if it is the only argument). While it may not make sense to use these options when any others are specified, there is no reason for this restriction and it might confuse users if these arguments appear to be ignored.

There is no need for any documentation change here as the restrictions on these options are not documented.

Signed-off-by John Keeping <[email protected]>

---
 builtin/rev-parse.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

builtin/rev-parse.c +13 ~~-15~~

index de894c7..c9aa28f 100644
@@ -486,21 +486,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	if (argc > 1 && !strcmp("--sq-quote", argv[1]))
 		return cmd_sq_quote(argc - 2, argv + 2);
 
-	if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {
-		int i;
-		for (i = 0; local_repo_env[i]; i++)
-			printf("%s\n", local_repo_env[i]);
-		return 0;
-	}
-
-	if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {
-		const char *gitdir = resolve_gitdir(argv[2]);
-		if (!gitdir)
-			die("not a gitdir '%s'", argv[2]);
-		puts(gitdir);
-		return 0;
-	}
-
 	if (argc > 1 && !strcmp("-h", argv[1]))
 		usage(builtin_rev_parse_usage);

@@ -661,6 +646,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				for_each_remote_ref(show_reference, NULL);
 				continue;
 			}
+			if (!strcmp(arg, "--local-env-vars")) {
+				int i;
+				for (i = 0; local_repo_env[i]; i++)
+					printf("%s\n", local_repo_env[i]);
+				continue;
+			}
 			if (!strcmp(arg, "--show-toplevel")) {
 				const char *work_tree = get_git_work_tree();
 				if (work_tree)

@@ -711,6 +702,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				printf("%s%s.git\n", cwd, len && cwd[len-1] != '/' ? "/" : "");
 				continue;
 			}
+			if (!strcmp(arg, "--resolve-git-dir")) {
+				const char *gitdir = resolve_gitdir(argv[i+1]);
+				if (!gitdir)
+					die("not a gitdir '%s'", argv[i+1]);
+				puts(gitdir);
+				continue;
+			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
 				printf("%s\n", is_inside_git_dir() ? "true"
 						: "false");

Signature · 1 line

1.8.3.3.972.gc83849e.dirty

John Keeping

author

2016-06-15 22:58 UTC #4 lore raw

PATCH v2 2/2 rev-parse(1): logically group options

In reply to John Keeping

Address this by reorganising the options into groups with subheadings. The text of option descriptions does not change.

Signed-off-by John Keeping <[email protected]>

---
 Documentation/git-rev-parse.txt | 104 ++++++++++++++++++++++++----------------
 1 file changed, 64 insertions(+), 40 deletions(-)

Documentation/git-rev-parse.txt +64 ~~-40~~

index 993903c..34c55a7 100644
@@ -24,9 +24,23 @@ distinguish between them.
 
 OPTIONS
 -------
+
+Operation Modes
+~~~~~~~~~~~~~~~
+
+Each of these options must appear first on the command line.
+
 --parseopt::
 	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
 
+--sq-quote::
+	Use 'git rev-parse' in shell quoting mode (see SQ-QUOTE
+	section below). In contrast to the `--sq` option below, this
+	mode does only quoting. Nothing else is done to command input.
+
+Options for --parseopt
+~~~~~~~~~~~~~~~~~~~~~~
+
 --keep-dashdash::
 	Only meaningful in `--parseopt` mode. Tells the option parser to echo
 	out the first `--` met instead of skipping it.

@@ -36,10 +50,8 @@ OPTIONS
 	the first non-option argument.  This can be used to parse sub-commands
 	that take options themselves.
 
---sq-quote::
-	Use 'git rev-parse' in shell quoting mode (see SQ-QUOTE
-	section below). In contrast to the `--sq` option below, this
-	mode does only quoting. Nothing else is done to command input.
+Options for Filtering
+~~~~~~~~~~~~~~~~~~~~~
 
 --revs-only::
 	Do not output flags and parameters not meant for

@@ -55,6 +67,9 @@ OPTIONS
 --no-flags::
 	Do not output flag parameters.
 
+Options for Output
+~~~~~~~~~~~~~~~~~~
+
 --default <arg>::
 	If there is no parameter given by the user, use `<arg>`
 	instead.

@@ -110,6 +125,17 @@ can be used.
 	strip '{caret}' prefix from the object names that already have
 	one.
 
+--abbrev-ref[=(strict|loose)]::
+	A non-ambiguous short name of the objects name.
+	The option core.warnAmbiguousRefs is used to select the strict
+	abbreviation mode.
+
+--short::
+--short=number::
+	Instead of outputting the full SHA-1 values of object names try to
+	abbreviate them to a shorter unique name. When no length is specified
+	7 is used. The minimum length is 4.
+
 --symbolic::
 	Usually the object names are output in SHA-1 form (with
 	possible '{caret}' prefix); this option makes them output in a

@@ -123,16 +149,8 @@ can be used.
 	unfortunately named tag "master"), and show them as full
 	refnames (e.g. "refs/heads/master").
 
---abbrev-ref[=(strict|loose)]::
-	A non-ambiguous short name of the objects name.
-	The option core.warnAmbiguousRefs is used to select the strict
-	abbreviation mode.
-
---disambiguate=<prefix>::
-	Show every object whose name begins with the given prefix.
-	The <prefix> must be at least 4 hexadecimal digits long to
-	avoid listing each and every object in the repository by
-	mistake.
+Options for Objects
+~~~~~~~~~~~~~~~~~~~
 
 --all::
 	Show all refs found in `refs/`.

@@ -155,18 +173,20 @@ shown.  If the pattern does not contain a globbing character (`?`,
 	character (`?`, `*`, or `[`), it is turned into a prefix
 	match by appending `/*`.
 
---show-toplevel::
-	Show the absolute path of the top-level directory.
+--disambiguate=<prefix>::
+	Show every object whose name begins with the given prefix.
+	The <prefix> must be at least 4 hexadecimal digits long to
+	avoid listing each and every object in the repository by
+	mistake.
 
---show-prefix::
-	When the command is invoked from a subdirectory, show the
-	path of the current directory relative to the top-level
-	directory.
+Options for Files
+~~~~~~~~~~~~~~~~~
 
---show-cdup::
-	When the command is invoked from a subdirectory, show the
-	path of the top-level directory relative to the current
-	directory (typically a sequence of "../", or an empty string).
+--local-env-vars::
+	List the GIT_* environment variables that are local to the
+	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
+	Only the names of the variables are listed, not their value,
+	even if they are set.
 
 --git-dir::
 	Show `$GIT_DIR` if defined. Otherwise show the path to

@@ -188,17 +208,27 @@ print a message to stderr and exit with nonzero status.
 --is-bare-repository::
 	When the repository is bare print "true", otherwise "false".
 
---local-env-vars::
-	List the GIT_* environment variables that are local to the
-	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
-	Only the names of the variables are listed, not their value,
-	even if they are set.
+--resolve-git-dir <path>::
+	Check if <path> is a valid repository or a gitfile that
+	points at a valid repository, and print the location of the
+	repository.  If <path> is a gitfile then the resolved path
+	to the real repository is printed.
 
---short::
---short=number::
-	Instead of outputting the full SHA-1 values of object names try to
-	abbreviate them to a shorter unique name. When no length is specified
-	7 is used. The minimum length is 4.
+--show-cdup::
+	When the command is invoked from a subdirectory, show the
+	path of the top-level directory relative to the current
+	directory (typically a sequence of "../", or an empty string).
+
+--show-prefix::
+	When the command is invoked from a subdirectory, show the
+	path of the current directory relative to the top-level
+	directory.
+
+--show-toplevel::
+	Show the absolute path of the top-level directory.
+
+Other Options
+~~~~~~~~~~~~~
 
 --since=datestring::
 --after=datestring::

@@ -213,12 +243,6 @@ print a message to stderr and exit with nonzero status.
 <args>...::
 	Flags and parameters to be parsed.
 
---resolve-git-dir <path>::
-	Check if <path> is a valid repository or a gitfile that
-	points at a valid repository, and print the location of the
-	repository.  If <path> is a gitfile then the resolved path
-	to the real repository is printed.
-
 
 include::revisions.txt[]

Signature · 1 line

1.8.3.3.972.gc83849e.dirty

Junio C Hamano

2016-06-15 22:58 UTC #5 lore raw

Quoted 24 lines · John Keeping

The options section of the git-rev-parse manual page has grown organically so that there now does not seem to be much logic behind the ordering of the options. It also does not make it clear that certain options must appear first on the command line.
Address this by reorganising the options into groups with subheadings. The text of option descriptions does not change.
Signed-off-by John Keeping <[email protected]>
---
 Documentation/git-rev-parse.txt | 104 ++++++++++++++++++++++++----------------
 1 file changed, 64 insertions(+), 40 deletions(-)
Documentation/git-rev-parse.txt +3 -0
index 993903c..34c55a7 100644
@@ -24,9 +24,23 @@ distinguish between them.
 
 OPTIONS
 -------
+
+Operation Modes
+~~~~~~~~~~~~~~~

Documentation/git-rev-parse.txt distinguish between them. · line 39

Quoted 10 lines

Documentation/git-rev-parse.txt +7 -0

+
+Each of these options must appear first on the command line.
+
 --parseopt::
 	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
 
+--sq-quote::
+	Use 'git rev-parse' in shell quoting mode (see SQ-QUOTE
+	section below). In contrast to the `--sq` option below, this
+	mode does only quoting. Nothing else is done to command input.

Much nicer ;-)

The rest also looked good to me. Thanks.