threads / patch / 25242

v5, 4 partsrev-parse: allow --flags to output rev-parse-like flags

Subject: [PATCH v5 0/4] rev-parse: allow --flags to output rev-parse-like flags

## tl;dr

7 messages between Sep 25, 2010 and Sep 26, 2010. Diffs are folded; open one to read it.

replies: 6people: 1as markdown or json

Jon Seymour· Sep 25, 2010, 16:18 UTC · lore

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
  $

This series also changes the interpretation of --flags so that specification of this option implies --no-revs.

Previously:
  $ git rev-parse --flags HEAD
  HEAD
  $

Now:
  $ git rev-parse --flags HEAD
  $

Reviewers attention is drawn to the following behaviour which, while unchanged from current behaviour, differs from the behaviour specified by the documentation. Specifically:

  $ git rev-parse -X --no-flags
  -X
  $ git rev-parse --no-flags -X
  $

In other words, a git rev-parse option only ever affects the interpretation of succeeding options, never preceding options.

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

v5 fixes a breakage in support of --no-revs option introduced by v4 and updates the test to add tests that would have prevented that breakage being introduced.

Jon Seymour (4):
  rev-parse: stop interpreting flags as options to rev-parse once
    --flags is specified
  rev-parse: add tests for git rev-parse --flags.
  rev-parse: update documentation of --flags and --no-flags options
  rev-parse: make --flags imply --no-revs for remaining arguments.

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

-- 
1.7.3.4.g9eb38

Jon Seymour· Sep 25, 2010, 16:18 UTC · re: Jon Seymour · lore

[PATCH v5 1/4] rev-parse: stop interpreting flags as options to rev-parse once --flags is specified

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.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 builtin/rev-parse.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

Show changes to builtin/rev-parse.c +8 −0

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++;

-- 
1.7.3.4.g73371.dirty

Jon Seymour· Sep 25, 2010, 16:18 UTC · re: Jon Seymour · lore

[PATCH v5 2/4] rev-parse: add tests for git rev-parse --flags.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t1510-rev-parse-flags.sh |  109 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 109 insertions(+), 0 deletions(-)
 create mode 100755 t/t1510-rev-parse-flags.sh

Show changes to t/t1510-rev-parse-flags.sh +109 −0

diff --git a/t/t1510-rev-parse-flags.sh b/t/t1510-rev-parse-flags.sh
new file mode 100755
index 0000000..ef0b4ad
--- /dev/null
+++ b/t/t1510-rev-parse-flags.sh
@@ -0,0 +1,109 @@
+#!/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 --symbolic --flags HEAD -> "HEAD"' \
+'
+	echo HEAD > expected &&
+	git rev-parse --symbolic --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 -q -> "-q"' \
+'
+	printf "%s\n" -q > expected &&
+	git rev-parse --flags -q >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 --flags --no-flags -> "--no-flags"' \
+'
+	printf "%s\n" --no-flags > expected &&
+	git rev-parse --flags --no-flags >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_done

-- 
1.7.3.4.g73371.dirty

Jon Seymour· Sep 25, 2010, 16:18 UTC · re: Jon Seymour · lore

[PATCH v5 3/4] rev-parse: update documentation of --flags and --no-flags options

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 Documentation/git-rev-parse.txt |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

Show changes to Documentation/git-rev-parse.txt +11 −1

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 341ca90..f5e6637 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -49,10 +49,20 @@ OPTIONS
 	'git rev-list' command.
 
 --flags::
-	Do not output non-flag parameters.
+	Do not output non-flag parameters which are not also revisions.
+	+
+	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>`

-- 
1.7.3.4.g73371.dirty

Jon Seymour· Sep 25, 2010, 16:18 UTC · re: Jon Seymour · lore

[PATCH v5 4/4] rev-parse: make --flags imply --no-revs for remaining arguments.

This change ensures that git rev-parse --flags complies with its documentation, namely:

   "Do not output non-flag parameters".

Previously:
   $ git rev-parse --flags HEAD
   <sha1 hash of HEAD>
   $

Now:
   $ git rev-parse --flags HEAD
   $

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 Documentation/git-rev-parse.txt |   24 +++++++++++++-----------
 builtin/rev-parse.c             |    6 +++++-
 t/t1510-rev-parse-flags.sh      |   24 +++++++++++++++++++++---
 3 files changed, 39 insertions(+), 15 deletions(-)

Show changes to 3 files +39 −15

Documentation/git-rev-parse.txt, builtin/rev-parse.c, t/t1510-rev-parse-flags.sh

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index f5e6637..f26fc7b 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -49,20 +49,22 @@ OPTIONS
 	'git rev-list' command.
 
 --flags::
-	Do not output non-flag parameters which are not also revisions.
-	+
-	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.
+	Do not output non-flag parameters.
++
+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.
++
+If `--flags` is specified, `--no-revs` is implied.
 
 --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.
++
+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 2ad269a..0655424 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -521,7 +521,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--flags")) {
-				filter &= ~DO_NONFLAGS;
+				if (!(filter & DO_FLAGS)) {
+					/* prevent --flags being interpreted if --no-flags has been seen */
+					continue;
+				}
+				filter &= ~(DO_NONFLAGS|DO_REVS);
 				continue;
 			}
 			if (!strcmp(arg, "--no-flags")) {
diff --git a/t/t1510-rev-parse-flags.sh b/t/t1510-rev-parse-flags.sh
index ef0b4ad..7df2081 100755
--- a/t/t1510-rev-parse-flags.sh
+++ b/t/t1510-rev-parse-flags.sh
@@ -29,10 +29,10 @@ test_expect_success 'git rev-parse --no-revs --flags HEAD -> ""' \
 	test_cmp expected actual
 '
 
-test_expect_success 'git rev-parse --symbolic --flags HEAD -> "HEAD"' \
+test_expect_success 'git rev-parse --flags HEAD -> ""' \
 '
-	echo HEAD > expected &&
-	git rev-parse --symbolic --flags HEAD >actual &&
+	: > expected &&
+	git rev-parse --flags HEAD >actual &&
 	test_cmp expected actual
 '
 
@@ -106,4 +106,22 @@ test_expect_success 'git rev-parse --symbolic --no-flags --flags HEAD -> "HEAD"'
 	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 --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_done

-- 
1.7.3.4.g73371.dirty

Jon Seymour· Sep 25, 2010, 16:21 UTC · re: Jon Seymour · lore

Re: [PATCH v5 4/4] rev-parse: make --flags imply --no-revs for remaining arguments.

Apologies - it appears I missed on : > case. Will fix in next iteration...

jon.

On Sun, Sep 26, 2010 at 2:18 AM, Jon Seymour <jon.seymour@gmail.com> wrote:

Show 121 quoted lines

> This change ensures that git rev-parse --flags complies with its documentation,
> namely:
>
>   "Do not output non-flag parameters".
>
> Previously:
>   $ git rev-parse --flags HEAD
>   <sha1 hash of HEAD>
>   $
>
> Now:
>   $ git rev-parse --flags HEAD
>   $
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  Documentation/git-rev-parse.txt |   24 +++++++++++++-----------
>  builtin/rev-parse.c             |    6 +++++-
>  t/t1510-rev-parse-flags.sh      |   24 +++++++++++++++++++++---
>  3 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index f5e6637..f26fc7b 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -49,20 +49,22 @@ OPTIONS
>        'git rev-list' command.
>
>  --flags::
> -       Do not output non-flag parameters which are not also revisions.
> -       +
> -       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.
> +       Do not output non-flag parameters.
> ++
> +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.
> ++
> +If `--flags` is specified, `--no-revs` is implied.
>
>  --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.
> ++
> +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 2ad269a..0655424 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -521,7 +521,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                                continue;
>                        }
>                        if (!strcmp(arg, "--flags")) {
> -                               filter &= ~DO_NONFLAGS;
> +                               if (!(filter & DO_FLAGS)) {
> +                                       /* prevent --flags being interpreted if --no-flags has been seen */
> +                                       continue;
> +                               }
> +                               filter &= ~(DO_NONFLAGS|DO_REVS);
>                                continue;
>                        }
>                        if (!strcmp(arg, "--no-flags")) {
> diff --git a/t/t1510-rev-parse-flags.sh b/t/t1510-rev-parse-flags.sh
> index ef0b4ad..7df2081 100755
> --- a/t/t1510-rev-parse-flags.sh
> +++ b/t/t1510-rev-parse-flags.sh
> @@ -29,10 +29,10 @@ test_expect_success 'git rev-parse --no-revs --flags HEAD -> ""' \
>        test_cmp expected actual
>  '
>
> -test_expect_success 'git rev-parse --symbolic --flags HEAD -> "HEAD"' \
> +test_expect_success 'git rev-parse --flags HEAD -> ""' \
>  '
> -       echo HEAD > expected &&
> -       git rev-parse --symbolic --flags HEAD >actual &&
> +       : > expected &&
> +       git rev-parse --flags HEAD >actual &&
>        test_cmp expected actual
>  '
>
> @@ -106,4 +106,22 @@ test_expect_success 'git rev-parse --symbolic --no-flags --flags HEAD -> "HEAD"'
>        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 --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_done
> --
> 1.7.3.4.g73371.dirty
>
>

Jon Seymour· Sep 26, 2010, 01:24 UTC · re: Jon Seymour · lore

[RE: v6 0/4] rev-parse: allow --flags to output rev-parse-like flags

Those interested in the latest version of this series can poll the stop-interpreting-after-flags branch on git://github.com/jonseymour/git.git

I have tagged v6 which is re-organized slightly w.r.t v5 so that tests and documentation of existing behaviour are cleanly separated from proposed changes.

I will post another version of this series to the list if/when I receive additional feedback or a final ack.

Regards,

jon seymour.