Re: [PATCH/RFC] rev-parse: stop interpreting flags as options to rev-parse once --flags is specified

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH/RFC] rev-parse: stop interpreting flags as options to rev-parse once --flags is specified

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:37 

Jon Seymour [off-list ref] writes:

Mmmm...almost certainly not going to regress anything, since there
does not seem to be a test or script that uses --flags. Ahem.
I've already explained the historical background in a separate message; I
realize that my message was missing the important part: conclusion.

If there weren't rev-parse before and we were about to invent the command,
I would agree that --flags should suppress output of HEAD.  Also I doubt
anybody relies on --flags for the purpose of removing non-revision
arguments.  So in that sense, your change would not hurt people.

But I do not think encouraging the use of rev-parse to pick "flags" is a
good idea in the longer term anyway, so I do not care too much about this
issue.  Unless you will teach "rev-parse --flags" about all the options
all other git command take (e.g. it should know --ignore-submodule takes
an optional option argument and be able to parse "--ignore-submodule all"
out), which is fundamentally impossible (e.g. for some commands "-n" does
not take argument, for some other "-n" takes an integer argument, and the
rev-parse command fundamentally cannot decide if it should report what
follows "-n" as part of its "--flags" output).

Re: [PATCH/RFC] rev-parse: stop interpreting flags as options to rev-parse once --flags is specified

From: Jon Seymour <hidden>
Date: 2016-06-15 22:49:37 

On Sun, Sep 26, 2010 at 5:11 PM, Junio C Hamano [off-list ref] wrote:
Jon Seymour [off-list ref] writes:
quoted
Mmmm...almost certainly not going to regress anything, since there
does not seem to be a test or script that uses --flags. Ahem.
I've already explained the historical background in a separate message; I
realize that my message was missing the important part: conclusion.

If there weren't rev-parse before and we were about to invent the command,
I would agree that --flags should suppress output of HEAD.  Also I doubt
anybody relies on --flags for the purpose of removing non-revision
arguments.  So in that sense, your change would not hurt people.

But I do not think encouraging the use of rev-parse to pick "flags" is a
good idea in the longer term anyway, so I do not care too much about this
issue.  Unless you will teach "rev-parse --flags" about all the options
all other git command take (e.g. it should know --ignore-submodule takes
an optional option argument and be able to parse "--ignore-submodule all"
out), which is fundamentally impossible (e.g. for some commands "-n" does
not take argument, for some other "-n" takes an integer argument, and the
rev-parse command fundamentally cannot decide if it should report what
follows "-n" as part of its "--flags" output).
Ok, so I have withdrawn the patch that makes --flags imply --no-revs.

v7 has 3 commits. The first commit documents existing behaviour more
accurately. The second commit adds a test suite for existing
behaviour.

I am offering the 3rd commit for discussion since your comments to
date have not directly addressed this commit. To recap, this commit
causes rev-parse to stop interpreting options once --flags is
interpreted.

For example, this would allow:

$ git rev-parse --flags --all
--all
$

whereas currently:

$ git rev-parse --flags --all
 .. list of revisions ..
$

As it stands, --flags really can't be used for any useful purpose. It
can't be used to output arbitrary flag or rev-like arguments
compatible with rev-list because it eats --all for itself. it can't be
used with any option that it also recognises (such as -q).

This being the case, one has to wonder whether we shouldn't just
deprecate --flags so that people don't waste time trying to use it.

jon.

[PATCH v7 0/3] rev-parse: allow --flags to output rev-parse-like flags

From: Jon Seymour <hidden>
Date: 2016-06-15 22:49:37 

This series allows git rev-parse --flags to output remaining flag-like arguments
even if such arguments are valid options to git rev-parse itself.

Previously:
  $ git rev-parse --flags -q -X --no-flags -- Y -Z
  -X
  $

Now:
  $ git rev-parse --flags -q -X --no-flags -- Y -Z
  -q -X --no-flags
  $

Aevar's feedback on v2 and v4 of this series has been incorporated.

The first commit modifies the documentation so that it accurately
reflects the current implementation.

The second commit introduces tests that document existing behaviour.

The third commit changes the way git rev-parse interprets option-like
arguments after the first --flags option is processed.

The first and second commits should be non-controversial since they
merely document and test existing behaviour. The third commit
is offered for discussion.

v7 removes the patch that made --flags imply --no-revs.

Jon Seymour (3):
  rev-parse: update Documentation of --flags
  rev-parse: add tests for git rev-parse --flags.
  rev-parse: stop interpreting flags as options to rev-parse once
    --flags is specified

 Documentation/git-rev-parse.txt |   12 +++-
 builtin/rev-parse.c             |    8 ++
 t/t1510-rev-parse-flags.sh      |  172 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 1 deletions(-)
 create mode 100755 t/t1510-rev-parse-flags.sh

-- 
1.7.3.3.g9129b6

[PATCH v7 1/3] rev-parse: update Documentation of --flags

From: Jon Seymour <hidden>
Date: 2016-06-15 22:49:37 

This change updates the documentation of git rev-parse --flags
so that the documentation accurately matches the current
implementation.

Signed-off-by: Jon Seymour <redacted>
---
 Documentation/git-rev-parse.txt |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 341ca90..27d15b0 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -49,7 +49,10 @@ OPTIONS
 	'git rev-list' command.
 
 --flags::
-	Do not output non-flag parameters.
+	Output any flag and revision-like values in the remaining parameters.
++
+Note that any parameter which is also a valid 'git rev-parse' option
+will be interpreted as an option to 'git rev-parse' and thus will not be output.
 
 --no-flags::
 	Do not output flag parameters.
-- 
1.7.3.3.g9129b6

[PATCH v7 2/3] rev-parse: add tests for git rev-parse --flags.

From: Jon Seymour <hidden>
Date: 2016-06-15 22:49:37 

Signed-off-by: Jon Seymour <redacted>
---
 t/t1510-rev-parse-flags.sh |  174 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100755 t/t1510-rev-parse-flags.sh

diff --git a/t/t1510-rev-parse-flags.sh b/t/t1510-rev-parse-flags.sh
new file mode 100755
index 0000000..e327b96
--- /dev/null
+++ b/t/t1510-rev-parse-flags.sh
@@ -0,0 +1,174 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Jon Seymour
+#
+
+test_description='test git rev-parse --flags'
+. ./test-lib.sh
+
+test_commit "A"
+
+test_expect_success 'git rev-parse --flags -> ""' \
+'
+	>expected &&
+	git rev-parse --flags >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags X -> ""' \
+'
+	>expected &&
+	git rev-parse --flags X >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --no-revs --flags HEAD -> ""' \
+'
+	>expected &&
+	git rev-parse --no-revs --flags HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags HEAD -> sha1 of HEAD' \
+'
+	git rev-parse HEAD > expected &&
+	git rev-parse --flags HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -- -> ""' \
+'
+	>expected &&
+	git rev-parse --flags -- >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -- X -> ""' \
+'
+	>expected &&
+	git rev-parse --flags -- X >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -- -X -> ""' \
+'
+	>expected &&
+	git rev-parse --flags -- -X >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -- -q --> ""' \
+'
+	>expected &&
+	git rev-parse --flags -- -q >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -X -> "-X"' \
+'
+	printf "%s\n" -X > expected &&
+	git rev-parse --flags -X >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -X -- Y -Z -> "-X"' \
+'
+	printf "%s\n" -X > expected &&
+	git rev-parse --flags -X -- Y -Z >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --no-flags --flags -X -> ""' \
+'
+	>expected &&
+	git rev-parse --no-flags --flags -X >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --symbolic --no-flags --flags HEAD -> "HEAD"' \
+'
+	echo HEAD >expected &&
+	git rev-parse --symbolic --no-flags --flags HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -q -> ""' \
+'
+	>expected &&
+	git rev-parse --flags -q >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags --no-flags -> ""' \
+'
+	>expected &&
+	git rev-parse --flags --no-flags >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --no-revs file -> "file"' \
+'
+	echo foo >file &&
+	echo file >expected &&
+	git rev-parse --no-revs file >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags -X file -> "-X"' \
+'
+	echo foo >file &&
+	printf "%s\n" "-X" >expected &&
+	git rev-parse --flags -X file >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --no-revs -- not-a-file -> "-- not-a-file"' \
+'
+	cat >expected <<-EOF &&
+--
+not-a-file
+	EOF
+	git rev-parse --no-revs -- not-a-file >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags --all -> list of revs' \
+'
+	cat >expected <<-EOF &&
+commit
+	EOF
+	git cat-file -t $(git rev-parse --flags --all | head -1) >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'git rev-parse --no-revs --all -> list of revs' \
+'
+	cat >expected <<-EOF &&
+commit
+	EOF
+	git cat-file -t $(git rev-parse --no-revs --all | head -1) >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --no-revs --min-age=20100203 -> ""' \
+'
+	>expected &&
+	git rev-parse --no-revs --min-age=20100203 > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --flags --min-age=20100203 -> "--min-age=20100203" ' \
+'
+	printf "%s\n" "--min-age=20100203" >expected &&
+	git rev-parse --flags --min-age=20100203 > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git rev-parse --no-revs --flags --all -> ""' \
+'
+	>expected &&
+	git rev-parse --no-revs --flags --all >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.3.3.g9129b6

[PATCH v7 3/3] rev-parse: stop interpreting flags as options to rev-parse once --flags is specified

From: Jon Seymour <hidden>
Date: 2016-06-15 22:49:37 

Current git rev-parse behaviour makes --flags hard to use if the remaining
arguments to git rev-parse contain an option that would otherwise be interpreted
as an option by git rev-parse itself.

So, for example:

  $> git rev-parse --flags -q -X
  -X

Normally one might expect to use -- to prevent -q being interpreted:

  $> git rev-parse --flags -- -q -X
  -q -X

But we can't really use -- in this way, because commands that use
git rev-parse might reasonably expect:

  $> git rev-parse --flags -Y -- -q -X
  -Y

That is, -Y to be regarded as a flag but everything after -- to be uninterpreted.

This proposed change modifies git rev-parse so that git rev-parse stops
interpreting flag arguments as options to git rev-parse once --flags is
interpreted. We also exit early once -- is found.

Previously:
 $> git rev-parse --flags --all
 {list of sha1 hashes}
 $>

Now:
 $> git rev-parse --flags --all
 --all
 $>

Signed-off-by: Jon Seymour <redacted>
---
 Documentation/git-rev-parse.txt |   11 +++++++++--
 builtin/rev-parse.c             |    8 ++++++++
 t/t1510-rev-parse-flags.sh      |   14 ++++++--------
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 27d15b0..3eac735 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -51,11 +51,18 @@ OPTIONS
 --flags::
 	Output any flag and revision-like values in the remaining parameters.
 +
-Note that any parameter which is also a valid 'git rev-parse' option
-will be interpreted as an option to 'git rev-parse' and thus will not be output.
+If specified, this option causes 'git rev-parse' to stop
+interpreting remaining arguments as options for its own
+consumption. As such, this option should be specified
+after all other options that 'git rev-parse' is expected
+to interpret.
 
 --no-flags::
 	Do not output flag parameters.
++
+If both `--flags` and `--no-flags` are specified, the first
+option specified wins and the other option is treated like
+a non-option argument.
 
 --default <arg>::
 	If there is no parameter given by the user, use `<arg>`
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..2ad269a 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -497,8 +497,16 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				/* Pass on the "--" if we show anything but files.. */
 				if (filter & (DO_FLAGS | DO_REVS))
 					show_file(arg);
+				if (!(filter & DO_NONFLAGS)) {
+					return 0;
+				}
 				continue;
 			}
+			if (!(filter & DO_NONFLAGS)) {
+				/* once we see --flags, we stop interpreting other flags */
+				 show_flag(arg);
+				 continue;
+			}
 			if (!strcmp(arg, "--default")) {
 				def = argv[i+1];
 				i++;
diff --git a/t/t1510-rev-parse-flags.sh b/t/t1510-rev-parse-flags.sh
index e327b96..1e8311e 100755
--- a/t/t1510-rev-parse-flags.sh
+++ b/t/t1510-rev-parse-flags.sh
@@ -92,16 +92,16 @@ test_expect_success 'git rev-parse --symbolic --no-flags --flags HEAD -> "HEAD"'
 	test_cmp expected actual
 '
 
-test_expect_success 'git rev-parse --flags -q -> ""' \
+test_expect_success 'git rev-parse --flags -q -> "-q"' \
 '
-	>expected &&
+	printf "%s\n" -q > expected &&
 	git rev-parse --flags -q >actual &&
 	test_cmp expected actual
 '
 
-test_expect_success 'git rev-parse --flags --no-flags -> ""' \
+test_expect_success 'git rev-parse --flags --no-flags -> "--no-flags"' \
 '
-	>expected &&
+	printf "%s\n" --no-flags > expected &&
 	git rev-parse --flags --no-flags >actual &&
 	test_cmp expected actual
 '
@@ -134,10 +134,8 @@ not-a-file
 
 test_expect_success 'git rev-parse --flags --all -> list of revs' \
 '
-	cat >expected <<-EOF &&
-commit
-	EOF
-	git cat-file -t $(git rev-parse --flags --all | head -1) >actual &&
+	printf "%s\n" "--all" >expected &&
+	git rev-parse --flags --all >actual &&
 	test_cmp expected actual
 '
 
-- 
1.7.3.3.g9129b6
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help