git-1.7.3 breakage: "git stash show xxx" doesn't show anything

git-1.7.3 breakage: "git stash show xxx" doesn't show anything

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Downstream bug: http://bugs.gentoo.org/338586

telling git-stash to show a specific stash no longer works with git-1.7.3:
  git stash show stash@{0}
  <no output!?>

Downgrading to dev-vcs/git-1.7.2.3 and it works fine.
Noticed on two stable amd64 systems.

Reproduction:
$ rm -rf foo && mkdir foo && cd foo
$ git init
Initialized empty Git repository in /home/vapier/foo/.git/
$ echo f > f && git add f && git commit -qmm
$ > f
$ git stash
Saved working directory and index state WIP on master: d287dea m
HEAD is now at d287dea m
$ git stash list | cat
stash@{0}: WIP on master: d287dea m
$ git stash show | cat
 f |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
$ git stash show stash@{0} | cat
<nothing!>

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]

Re: git-1.7.3 breakage: "git stash show xxx" doesn't show anything

[Brandon Casey]

On 09/24/2010 02:19 PM, Robin H. Johnson wrote:
> Downstream bug: http://bugs.gentoo.org/338586
> 
> telling git-stash to show a specific stash no longer works with git-1.7.3:
>   git stash show stash@{0}
>   <no output!?>
> 
> Downgrading to dev-vcs/git-1.7.2.3 and it works fine.
> Noticed on two stable amd64 systems.
> 
> Reproduction:
> $ rm -rf foo && mkdir foo && cd foo
> $ git init
> Initialized empty Git repository in /home/vapier/foo/.git/
> $ echo f > f && git add f && git commit -qmm
> $ > f
> $ git stash
> Saved working directory and index state WIP on master: d287dea m
> HEAD is now at d287dea m
> $ git stash list | cat
> stash@{0}: WIP on master: d287dea m
> $ git stash show | cat
>  f |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> $ git stash show stash@{0} | cat
> <nothing!>
>

Probably,

diff --git a/git-stash.sh b/git-stash.sh
index 7ce818b..4fbfb62 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -265,7 +265,7 @@ parse_flags_and_rev()
        i_tree=
 
        REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
-       FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
+       FLAGS=$(git rev-parse --no-revs --flags "$@" 2>/dev/null)
 
        set -- $FLAGS
 

Re: git-1.7.3 breakage: "git stash show xxx" doesn't show anything

[Brian Gernhardt]

On Sep 24, 2010, at 4:01 PM, Brandon Casey wrote:

> Probably,
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index 7ce818b..4fbfb62 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -265,7 +265,7 @@ parse_flags_and_rev()
>        i_tree=
> 
>        REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
> -       FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
> +       FLAGS=$(git rev-parse --no-revs --flags "$@" 2>/dev/null)
> 
>        set -- $FLAGS

I bisected the issue to a9bf09e (detached-stash: simplify git stash show), which is when "git stash show" started using parse_flags_and_rev (via assert_stash_like()).

More worrying to me is that the tests for "git stash show" don't bother to test the output.  I'll be working on that now.

~~ Brian

[PATCH] t/t3903-stash: improve testing of git-stash show

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Recently, the 'stash show' functionality was broken for the case when a
stash-like argument was supplied.  Since, commit 9bf09e, 'stash show' when
supplied a stash-like argument prints nothing and still exists with a zero
status.  Unfortunately, the flaw slipped through the test suite cracks
since the output of 'stash show' was not verified to be correct.

Improve and expand on the existing tests so that this flaws is detected.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


On 09/24/2010 03:27 PM, Brian Gernhardt wrote:
> I bisected the issue to a9bf09e (detached-stash: simplify git stash show),
> which is when "git stash show" started using parse_flags_and_rev (via
> assert_stash_like()).
> 
> More worrying to me is that the tests for "git stash show" don't bother
> to test the output.  I'll be working on that now.

I was preparing these tests when your email came in.

hth,
Brandon


 t/t3903-stash.sh |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a283dca..e8a7338 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -406,7 +406,7 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '
 	test $(git ls-files --modified | wc -l) -eq 1
 '
 
-test_expect_success 'stash show - stashes on stack, stash-like argument' '
+test_expect_failure 'stash show - stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -416,16 +416,70 @@ test_expect_success 'stash show - stashes on stack, stash-like argument' '
 	echo bar >> file &&
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
-	git stash show ${STASH_ID}
+	cat >expected <<-EOF &&
+	 file |    1 +
+	 1 files changed, 1 insertions(+), 0 deletions(-)
+	EOF
+	git stash show ${STASH_ID} >actual &&
+	test_cmp expected actual
 '
-test_expect_success 'stash show - no stashes on stack, stash-like argument' '
+
+test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	git stash &&
+	test_when_finished "git stash drop" &&
+	echo bar >> file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..935fbd3 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+bar
+	EOF
+	git stash show -p ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	 file |    1 +
+	 1 files changed, 1 insertions(+), 0 deletions(-)
+	EOF
+	git stash show ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'stash show -p - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
 	echo foo >> file &&
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
-	git stash show ${STASH_ID}
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..71b52c4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show -p ${STASH_ID} >actual &&
+	test_cmp expected actual
 '
 
 test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
-- 
1.7.3

Re: [PATCH] t/t3903-stash: improve testing of git-stash show

[Brian Gernhardt]

On Sep 24, 2010, at 4:40 PM, Brandon Casey wrote:

> From: Brandon Casey <drafnel@gmail.com>
> 
> Recently, the 'stash show' functionality was broken for the case when a
> stash-like argument was supplied.  Since, commit 9bf09e, 'stash show' when
> supplied a stash-like argument prints nothing and still exists with a zero
> status.  Unfortunately, the flaw slipped through the test suite cracks
> since the output of 'stash show' was not verified to be correct.
> 
> Improve and expand on the existing tests so that this flaws is detected.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
> 
> 
> On 09/24/2010 03:27 PM, Brian Gernhardt wrote:
>> I bisected the issue to a9bf09e (detached-stash: simplify git stash show),
>> which is when "git stash show" started using parse_flags_and_rev (via
>> assert_stash_like()).
>> 
>> More worrying to me is that the tests for "git stash show" don't bother
>> to test the output.  I'll be working on that now.
> 
> I was preparing these tests when your email came in.

I spent longer finding the source of the problem than you did.  I was just putting the finishing touches on it when I got this.  Yours is more through than mine, so I won't bother finishing.

~~ Brian

Re: [PATCH] t/t3903-stash: improve testing of git-stash show

[Brandon Casey]

On 09/24/2010 03:43 PM, Brian Gernhardt wrote:

>> On 09/24/2010 03:27 PM, Brian Gernhardt wrote:
>>> I bisected the issue to a9bf09e (detached-stash: simplify git stash show),
>>> which is when "git stash show" started using parse_flags_and_rev (via
>>> assert_stash_like()).
>>>
>>> More worrying to me is that the tests for "git stash show" don't bother
>>> to test the output.  I'll be working on that now.
>>
>> I was preparing these tests when your email came in.
> 
> I spent longer finding the source of the problem than you did.  I was just
> putting the finishing touches on it when I got this.  Yours is more through
> than mine, so I won't bother finishing.

I hope you're just abandoning the tests you were creating, and _not_
abandoning the search for a fix.  The "solution" I offered is flawed
and breaks some of the other tests. :)

-Brandon

Re: [PATCH] t/t3903-stash: improve testing of git-stash show

[Brian Gernhardt]

On Sep 24, 2010, at 4:50 PM, Brandon Casey wrote:

> I hope you're just abandoning the tests you were creating, and _not_
> abandoning the search for a fix.  The "solution" I offered is flawed
> and breaks some of the other tests. :)

Actually, I think the solution you offered exposed what could be considered a bug in git-rev-parse.  The fact that it worked before was just an happy accident, I think...

$ ARGS="-q --index stash@{0}"
$ # Get only the revision arguments
$ git rev-parse --no-flags --symbolic $ARGS
stash@{0}
$ # What git-stash currently uses to get flags
$ git rev-parse --no-revs -- $ARGS
--
-q
--index
stash@{0}
$ # That was a lot more than just the flags
$ # What git-stash "should" use to get flags
$ git rev-parse --no-revs --flags $ARGS
--index
$ # Huh, it ate -q, let's try --
$ git rev-parse --no-revs --flags -- $ARGS
$ # No, that's not right either...

git-stash's current code "FLAGS=$(git rev-parse --no-revs -- "$@")" simply returns all of the arguments including a starting --.  The issue is that git-rev-parse eats a -q parameter.  There's no way to distinguish between arguments for rev-parse and arguments it's supposed to parse.  Generally this isn't an issue.

The simple way to deal with this is to check for -q before using rev-parse.  The better way is to either get rev-parse to stop eating the -q somehow or to switch git-stash to parseopts.

Simple patch coming soon.

~~ Brian

[PATCH] git-stash: fix flag parsing

[Brian Gernhardt]

Currently git-stash uses `git rev-parse --no-revs -- "$@"` to set its
FLAGS variable.  This is the same as `FLAGS="-- $@"`.  It should use
`git rev-parse --no-revs --flags "$@"`, but that eats any "-q" or
"--quiet" argument.  So move the check for quiet before rev-parse.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 Not the most elegant solution, but it works.

 I think we want to add the ability for git rev-parse to understand
 `git rev-parse --no-revs --flags -- "$@"`, but I'm not sure if that
 would break anything else and don't have the time to do it right now.

 git-stash.sh     |   15 +++++++++++----
 t/t3903-stash.sh |    8 ++++----
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 7ce818b..b44da41 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -264,8 +264,18 @@ parse_flags_and_rev()
 	b_tree=
 	i_tree=
 
+	# Work around rev-parse --flags eating -q
+	for opt
+	do
+		case "$opt" in
+			-q|--quiet)
+				GIT_QUIET=t
+			;;
+		esac
+	done
+
 	REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
-	FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
+	FLAGS=$(git rev-parse --no-revs --flags "$@" 2>/dev/null)
 
 	set -- $FLAGS
 
@@ -273,9 +283,6 @@ parse_flags_and_rev()
 	while test $# -ne 0
 	do
 		case "$1" in
-			-q|--quiet)
-				GIT_QUIET=-t
-			;;
 			--index)
 				INDEX_OPTION=--index
 			;;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e8a7338..9ed2396 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -406,7 +406,7 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '
 	test $(git ls-files --modified | wc -l) -eq 1
 '
 
-test_expect_failure 'stash show - stashes on stack, stash-like argument' '
+test_expect_success 'stash show - stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -424,7 +424,7 @@ test_expect_failure 'stash show - stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
+test_expect_success 'stash show -p - stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -447,7 +447,7 @@ test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
+test_expect_success 'stash show - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -462,7 +462,7 @@ test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'stash show -p - no stashes on stack, stash-like argument' '
+test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
-- 
1.7.3.234.g7bba3

[PATCH] git-stash: fix flag parsing

[Brian Gernhardt]

Currently git-stash uses `git rev-parse --no-revs -- "$@"` to set its
FLAGS variable.  This is the same as `FLAGS="-- $@"`.  It should use
`git rev-parse --no-revs --flags "$@"`, but that eats any "-q" or
"--quiet" argument.  So move the check for quiet before rev-parse.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 Not the most elegant solution, but it works.

 I think we want to add the ability for git rev-parse to understand
 `git rev-parse --no-revs --flags -- "$@"`, but I'm not sure if that
 would break anything else and don't have the time to do it right now.

 (This time with the right CC list.)

 git-stash.sh     |   15 +++++++++++----
 t/t3903-stash.sh |    8 ++++----
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 7ce818b..b44da41 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -264,8 +264,18 @@ parse_flags_and_rev()
 	b_tree=
 	i_tree=
 
+	# Work around rev-parse --flags eating -q
+	for opt
+	do
+		case "$opt" in
+			-q|--quiet)
+				GIT_QUIET=t
+			;;
+		esac
+	done
+
 	REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
-	FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
+	FLAGS=$(git rev-parse --no-revs --flags "$@" 2>/dev/null)
 
 	set -- $FLAGS
 
@@ -273,9 +283,6 @@ parse_flags_and_rev()
 	while test $# -ne 0
 	do
 		case "$1" in
-			-q|--quiet)
-				GIT_QUIET=-t
-			;;
 			--index)
 				INDEX_OPTION=--index
 			;;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e8a7338..9ed2396 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -406,7 +406,7 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '
 	test $(git ls-files --modified | wc -l) -eq 1
 '
 
-test_expect_failure 'stash show - stashes on stack, stash-like argument' '
+test_expect_success 'stash show - stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -424,7 +424,7 @@ test_expect_failure 'stash show - stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
+test_expect_success 'stash show -p - stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -447,7 +447,7 @@ test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
+test_expect_success 'stash show - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -462,7 +462,7 @@ test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'stash show -p - no stashes on stack, stash-like argument' '
+test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
-- 
1.7.3.234.g7bba3

Re: [PATCH] git-stash: fix flag parsing

[Jon Seymour]

Thanks Brian. I've submitted what I hope is the simplest possible
rectification of my regression - apologies for not catching this
myself!

jon.

On Sat, Sep 25, 2010 at 8:15 AM, Brian Gernhardt
<brian@gernhardtsoftware.com> wrote:
> Currently git-stash uses `git rev-parse --no-revs -- "$@"` to set its
> FLAGS variable.  This is the same as `FLAGS="-- $@"`.  It should use
> `git rev-parse --no-revs --flags "$@"`, but that eats any "-q" or
> "--quiet" argument.  So move the check for quiet before rev-parse.
>
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>
>  Not the most elegant solution, but it works.
>
>  I think we want to add the ability for git rev-parse to understand
>  `git rev-parse --no-revs --flags -- "$@"`, but I'm not sure if that
>  would break anything else and don't have the time to do it right now.
>
>  (This time with the right CC list.)
>
>  git-stash.sh     |   15 +++++++++++----
>  t/t3903-stash.sh |    8 ++++----
>  2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 7ce818b..b44da41 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -264,8 +264,18 @@ parse_flags_and_rev()
>        b_tree=
>        i_tree=
>
> +       # Work around rev-parse --flags eating -q
> +       for opt
> +       do
> +               case "$opt" in
> +                       -q|--quiet)
> +                               GIT_QUIET=t
> +                       ;;
> +               esac
> +       done
> +
>        REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
> -       FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
> +       FLAGS=$(git rev-parse --no-revs --flags "$@" 2>/dev/null)
>
>        set -- $FLAGS
>
> @@ -273,9 +283,6 @@ parse_flags_and_rev()
>        while test $# -ne 0
>        do
>                case "$1" in
> -                       -q|--quiet)
> -                               GIT_QUIET=-t
> -                       ;;
>                        --index)
>                                INDEX_OPTION=--index
>                        ;;
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index e8a7338..9ed2396 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -406,7 +406,7 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '
>        test $(git ls-files --modified | wc -l) -eq 1
>  '
>
> -test_expect_failure 'stash show - stashes on stack, stash-like argument' '
> +test_expect_success 'stash show - stashes on stack, stash-like argument' '
>        git stash clear &&
>        test_when_finished "git reset --hard HEAD" &&
>        git reset --hard &&
> @@ -424,7 +424,7 @@ test_expect_failure 'stash show - stashes on stack, stash-like argument' '
>        test_cmp expected actual
>  '
>
> -test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
> +test_expect_success 'stash show -p - stashes on stack, stash-like argument' '
>        git stash clear &&
>        test_when_finished "git reset --hard HEAD" &&
>        git reset --hard &&
> @@ -447,7 +447,7 @@ test_expect_failure 'stash show -p - stashes on stack, stash-like argument' '
>        test_cmp expected actual
>  '
>
> -test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
> +test_expect_success 'stash show - no stashes on stack, stash-like argument' '
>        git stash clear &&
>        test_when_finished "git reset --hard HEAD" &&
>        git reset --hard &&
> @@ -462,7 +462,7 @@ test_expect_failure 'stash show - no stashes on stack, stash-like argument' '
>        test_cmp expected actual
>  '
>
> -test_expect_failure 'stash show -p - no stashes on stack, stash-like argument' '
> +test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
>        git stash clear &&
>        test_when_finished "git reset --hard HEAD" &&
>        git reset --hard &&
> --
> 1.7.3.234.g7bba3
>
>

Re: [PATCH] git-stash: fix flag parsing

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> Currently git-stash uses `git rev-parse --no-revs -- "$@"` to set its
> FLAGS variable.  This is the same as `FLAGS="-- $@"`.  It should use
> `git rev-parse --no-revs --flags "$@"`, but that eats any "-q" or
> "--quiet" argument.  So move the check for quiet before rev-parse.
>
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>
>  Not the most elegant solution, but it works.

Thanks.

rev-parse as a flag parser was useful hack when most of the log family was
"rev-list | diff-tree" with various options, but the recent push to make
flags consistent across commands inevitably has made it fundamentally
impossible for the function to function sensibly in all cases, as it
itself has to share some options, like "-q", with others.

After the push of rewriting everything as C builtin, combined with the
improvement of parse-options, I have to say that rev-parse flag parser
hack has outlived its usefulness.  The former means there is less reason
to script end-user commands as a "rev-list | diff-tree" pipeline (I am not
talking about special purpose statistics commands that use rev-list here),
and the latter means that it has become easier for a Porcelain script to
parse _its_ command line options and come up with a set of sensible
options to underlying rev-list and the command at the downstream of the
pipe, without following the original hacky pattern of "we let random
options and args and without interpreting them we guess which ones to
throw at rev-list and which ones to throw at diff-tree".

[PATCH] stash: simplify parsing fixes

[Jon Seymour]

This patch simplifies Brian's fix for the recent regression by:

* eliminating the extra loop
* eliminating use of git rev-parse for parsing flags
* making use of the for opt idiom for the retained loop
* eliminating the redundant -- case

The patch has been tested with the tests in current maint.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-stash.sh |   28 +++++++---------------------
 1 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 57f36ce..23a9ab5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -264,36 +264,22 @@ parse_flags_and_rev()
 	b_tree=
 	i_tree=
 
-	# Work around rev-parse --flags eating -q
-	for opt
-	do
-		case "$opt" in
-		-q|--quiet)
-			GIT_QUIET=t
-			;;
-		esac
-	done
-
 	REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
-	FLAGS=$(git rev-parse --no-revs --flags "$@" 2>/dev/null)
-
-	set -- $FLAGS
 
 	FLAGS=
-	while test $# -ne 0
+	for opt
 	do
-		case "$1" in
+		case "$opt" in
+			-q|--quiet)
+				GIT_QUIET=-t
+			;;
 			--index)
 				INDEX_OPTION=--index
 			;;
-			--)
-				:
-			;;
-			*)
-				FLAGS="${FLAGS}${FLAGS:+ }$1"
+			-*)
+				FLAGS="${FLAGS}${FLAGS:+ }$opt"
 			;;
 		esac
-		shift
 	done
 
 	set -- $REV
-- 
1.7.3.2.g9027fa.dirty

[PATCH] stash show: fix breakage in 1.7.3

[Jon Seymour]

The detached-stash series regressed support for
   git stash show stash@{0}

due to a faulty assumption that
git rev-parse --no-revs stash@{0} would treat
stash@{0} as a revision reference and thus
not display it.

This patch restores the behaviour of git stash show
so that only flag like options are assigned
to the FLAGS variable and thus does not
depend git rev-parse behaviour so strongly.

It has been tested with Brandon Casey's improved t3903 tests.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 Apologies for the breakage.
 Brandon: thanks for improving the tests.

 git-stash.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 7ce818b..c2f1b2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -282,7 +282,7 @@ parse_flags_and_rev()
 			--)
 				:
 			;;
-			*)
+			-*)
 				FLAGS="${FLAGS}${FLAGS:+ }$1"
 			;;
 		esac
-- 
1.7.2.12.g8788e.dirty

[PATCH v1] stash show: fix breakage in 1.7.3

[Jon Seymour]

The detached-stash series regressed support for
   git stash show stash@{0}

due to a faulty assumption that:
   git rev-parse --no-revs -- stash@{0}

would treat stash@{0} as a revision reference and
thus not output it.

This patch restores the behaviour of git stash show
so that git rev-parse is not used for parsing flags
and only flag like options are assigned to
the FLAGS variable.

It has been tested with Brandon Casey's improved t3903 tests.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-stash.sh |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

This revision further simplifies the parsing code
by removing use of git rev-parse for FLAGS parsing
altogether.

diff --git a/git-stash.sh b/git-stash.sh
index 7ce818b..8b18bb5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -265,9 +265,6 @@ parse_flags_and_rev()
 	i_tree=
 
 	REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
-	FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
-
-	set -- $FLAGS
 
 	FLAGS=
 	while test $# -ne 0
@@ -282,7 +279,7 @@ parse_flags_and_rev()
 			--)
 				:
 			;;
-			*)
+			-*)
 				FLAGS="${FLAGS}${FLAGS:+ }$1"
 			;;
 		esac
-- 
1.7.2.14.g132f5.dirty

Re: [PATCH v1] stash show: fix breakage in 1.7.3

[Brian Gernhardt]

On Sep 24, 2010, at 11:32 PM, Jon Seymour wrote:

> due to a faulty assumption that:
>   git rev-parse --no-revs -- stash@{0}

This assumption is faulty, it should be "git rev-parse --no-revs --flags stash@{0}", which works properly for all revision arguments and flags _except_ -q and --quiet.

> This revision further simplifies the parsing code
> by removing use of git rev-parse for FLAGS parsing
> altogether.

That is simpler, and does fix this specific issue.  However, I would strongly argue that "git rev-parse --no-revs --flags" is broken.  I really don't have the time tonight or probably this weekend to work on it, but git-rev-parse should only take "-q" and "--quiet" for itself if "--verify" was passed.  (Since that is the only mode in which rev-parse uses quiet, AFAIK.)

Possibly rev-parse should also (or instead) separate "arguments for rev-parse" and "arguments rev-parse is parsing" using the standard "--".  I don't know if this will affect any current users.

~~ Brian

Re: [PATCH v1] stash show: fix breakage in 1.7.3

[Jon Seymour]

On Sat, Sep 25, 2010 at 2:45 PM, Brian Gernhardt
<brian@gernhardtsoftware.com> wrote:
>
> On Sep 24, 2010, at 11:32 PM, Jon Seymour wrote:
>
>> due to a faulty assumption that:
>>   git rev-parse --no-revs -- stash@{0}
>
> This assumption is faulty, it should be "git rev-parse --no-revs --flags stash@{0}", which works properly for all revision arguments and flags _except_ -q and --quiet.

Agreed. If I recall the evolution of the code, I originally had
--no-revs --flags, but then found -q was being eaten. The only way to
prevent it being eaten was to remove --flags and add --, which "fixed"
the -q problem but created the git stash show xxx problem. Of course,
had my unit tests been more thorough, I would have picked this, but
alas they were not.

>
>> This revision further simplifies the parsing code
>> by removing use of git rev-parse for FLAGS parsing
>> altogether.
>
> That is simpler, and does fix this specific issue.  However, I would strongly argue that "git rev-parse --no-revs --flags" is broken.  I really don't have the time tonight or probably this weekend to work on it, but git-rev-parse should only take "-q" and "--quiet" for itself if "--verify" was passed.  (Since that is the only mode in which rev-parse uses quiet, AFAIK.)
>

This seems like a reasonable compromise to me.

> Possibly rev-parse should also (or instead) separate "arguments for rev-parse" and "arguments rev-parse is parsing" using the standard "--".  I don't know if this will affect any current users.
>

I suspect this could be problematic.

Current behaviour:

   $ git rev-parse --flags -X -- Y -Z
   -X

Here -- is being used to say, don't interpret anything after -- as
being flag-like even if it could be flag-like.

So, if a command foo uses git rev-parse to parse its own arguments:

    foo -X -- Y -Z

then I suspect, it would only want to treat -X as being flag-like,
from the user of foo's point of view, both Y and -Z are not meant to
be interpreted by foo.

It might make sense for --flags to be an instruction to git rev-parse
not to interpret any subsequent arguments as git rev-parse options so
that:

  $ git rev-parse --flags -q --no-flags -- --revs-only

would output:

  -q --no-flags

That is, prevent -q being eaten by git rev-parse, but preserve
existing interpretation of --.

> ~~ Brian
>
>

Re: [PATCH v1] stash show: fix breakage in 1.7.3

[Jon Seymour]

| Added Brandon to distribution - apologies for earlier omission.

jon.

On Sat, Sep 25, 2010 at 1:32 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> The detached-stash series regressed support for
>   git stash show stash@{0}
>
> due to a faulty assumption that:
>   git rev-parse --no-revs -- stash@{0}
>
> would treat stash@{0} as a revision reference and
> thus not output it.
>
> This patch restores the behaviour of git stash show
> so that git rev-parse is not used for parsing flags
> and only flag like options are assigned to
> the FLAGS variable.
>
> It has been tested with Brandon Casey's improved t3903 tests.
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  git-stash.sh |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> This revision further simplifies the parsing code
> by removing use of git rev-parse for FLAGS parsing
> altogether.
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 7ce818b..8b18bb5 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -265,9 +265,6 @@ parse_flags_and_rev()
>        i_tree=
>
>        REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
> -       FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
> -
> -       set -- $FLAGS
>
>        FLAGS=
>        while test $# -ne 0
> @@ -282,7 +279,7 @@ parse_flags_and_rev()
>                        --)
>                                :
>                        ;;
> -                       *)
> +                       -*)
>                                FLAGS="${FLAGS}${FLAGS:+ }$1"
>                        ;;
>                esac
> --
> 1.7.2.14.g132f5.dirty
>
>

Re: [PATCH v1] stash show: fix breakage in 1.7.3

[Jon Seymour]

This patch has been superceded by:

    [PATCH] stash: simplify parsing fixes

which applies on the Brian's fix that has been applied to maint.

On Sat, Sep 25, 2010 at 1:32 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> The detached-stash series regressed support for
>   git stash show stash@{0}
>
> due to a faulty assumption that:
>   git rev-parse --no-revs -- stash@{0}
>
> would treat stash@{0} as a revision reference and
> thus not output it.
>
> This patch restores the behaviour of git stash show
> so that git rev-parse is not used for parsing flags
> and only flag like options are assigned to
> the FLAGS variable.
>
> It has been tested with Brandon Casey's improved t3903 tests.
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  git-stash.sh |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> This revision further simplifies the parsing code
> by removing use of git rev-parse for FLAGS parsing
> altogether.
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 7ce818b..8b18bb5 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -265,9 +265,6 @@ parse_flags_and_rev()
>        i_tree=
>
>        REV=$(git rev-parse --no-flags --symbolic "$@" 2>/dev/null)
> -       FLAGS=$(git rev-parse --no-revs -- "$@" 2>/dev/null)
> -
> -       set -- $FLAGS
>
>        FLAGS=
>        while test $# -ne 0
> @@ -282,7 +279,7 @@ parse_flags_and_rev()
>                        --)
>                                :
>                        ;;
> -                       *)
> +                       -*)
>                                FLAGS="${FLAGS}${FLAGS:+ }$1"
>                        ;;
>                esac
> --
> 1.7.2.14.g132f5.dirty
>
>

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

[Jon Seymour]

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.

Tests will follow in subsequent iterations of this patch, if the consensus
is that the approach is correct.

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..9e340c7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -497,8 +497,15 @@ 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)) {
+				 show_flag(arg);
+				 continue;
+			}
 			if (!strcmp(arg, "--default")) {
 				def = argv[i+1];
 				i++;
-- 
1.7.2.14.gbde03.dirty

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

[Jon Seymour]

On Sat, Sep 25, 2010 at 5:19 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> Current git rev-parse behaviour makes --flags hard to use if the remaining
> ...

Note that I haven't run the test suite yet to see if it breaks
anything. I'd also
expect to write additional tests and update the documentation.

jon.

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

[Jon Seymour]

On Sat, Sep 25, 2010 at 5:19 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Sat, Sep 25, 2010 at 5:19 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
>> Current git rev-parse behaviour makes --flags hard to use if the remaining
>> ...
>
> Note that I haven't run the test suite yet to see if it breaks
> anything. I'd also
> expect to write additional tests and update the documentation.
>
> jon.
>

Mmmm...almost certainly not going to regress anything, since there
does not seem to be a test or script that uses --flags. Ahem.

jon.

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

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> 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

[Jon Seymour]

On Sun, Sep 26, 2010 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> 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).
>

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

[Jon Seymour]

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

[Jon Seymour]

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

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 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.

[Jon Seymour]

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 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

[Jon Seymour]

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 <jon.seymour@gmail.com>
---
 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