[PATCH] git-sh-setup: fix parseopt `eval`.

[PATCH] git-sh-setup: fix parseopt `eval`.

[Junio C Hamano]

The 'automagic parseopt' support corrupted non option parameters
that had IFS characters in them.  The worst case can be seen
when it has a non option parameter like this:

	$1=" * some string   blech"

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I had "git pull -n . to/pic-branch" in Meta/PU which was
   affected by this bug, expanding the " * " bullet before the
   merge message that is passed from git-pull to git-merge.

   I am a bit upset because I _knew_ that the eval was wrong
   when I first saw it, but somehow I forgot about it when I
   made it land on 'next'.  My fault.

 git-sh-setup.sh |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index e1cf885..f1c4839 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -21,11 +21,12 @@ if test -n "$OPTIONS_SPEC"; then
 		exec "$0" -h
 	}
 
-	parseopt_extra=
-	[ -n "$OPTIONS_KEEPDASHDASH" ] &&
-		parseopt_extra="$parseopt_extra --keep-dashdash"
-
-	eval `echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@" || echo exit $?`
+	[ -n "$OPTIONS_KEEPDASHDASH" ] && parseopt_extra="--keep-dashdash"
+	parsed=$(
+		echo "$OPTIONS_SPEC" |
+		git rev-parse --parseopt $parseopt_extra -- "$@"
+	) &&
+	eval "$parsed" || exit
 else
 	usage() {
 		die "Usage: $0 $USAGE"
-- 
1.5.3.5.1617.g65b5b

Re: [PATCH] git-sh-setup: fix parseopt `eval`.

[Pierre Habouzit]

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

On Thu, Nov 08, 2007 at 07:09:29AM +0000, Junio C Hamano wrote:
> The 'automagic parseopt' support corrupted non option parameters
> that had IFS characters in them.  The worst case can be seen
> when it has a non option parameter like this:

hu sorry about that, I should have put "" around the ``. I knew it also
but it slipped my mind too.  I believe this works as well:

eval "$(echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@" || echo exit $?)"

I like it better because you will then exit with an exit 129 wich is
what we want (and what I documented would work too :P)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [PATCH] git-sh-setup: fix parseopt `eval`.

[Pierre Habouzit]

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

On Thu, Nov 08, 2007 at 07:09:29AM +0000, Junio C Hamano wrote:
> The 'automagic parseopt' support corrupted non option parameters
> that had IFS characters in them.  The worst case can be seen
> when it has a non option parameter like this:
> 
> 	$1=" * some string   blech"
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

> -	parseopt_extra=
> -	[ -n "$OPTIONS_KEEPDASHDASH" ] &&
> -		parseopt_extra="$parseopt_extra --keep-dashdash"
> +	[ -n "$OPTIONS_KEEPDASHDASH" ] && parseopt_extra="--keep-dashdash"

  oh and this part is wrong because you're affected by $parseopt_extra
environment poisonning. And you have to fix git-clone.sh that uses
git-rev-parse --parsopt directly with the same call too (as it doesn't
use git-sh-setup).

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [PATCH] git-sh-setup: fix parseopt `eval`.

[Pierre Habouzit]

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

On Thu, Nov 08, 2007 at 09:14:02AM +0000, Pierre Habouzit wrote:
> On Thu, Nov 08, 2007 at 07:09:29AM +0000, Junio C Hamano wrote:
> > The 'automagic parseopt' support corrupted non option parameters
> > that had IFS characters in them.  The worst case can be seen
> > when it has a non option parameter like this:
> > 
> > 	$1=" * some string   blech"
> > 
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> > -	parseopt_extra=
> > -	[ -n "$OPTIONS_KEEPDASHDASH" ] &&
> > -		parseopt_extra="$parseopt_extra --keep-dashdash"
> > +	[ -n "$OPTIONS_KEEPDASHDASH" ] && parseopt_extra="--keep-dashdash"
> 
>   oh and this part is wrong because you're affected by $parseopt_extra
> environment poisonning. And you have to fix git-clone.sh that uses
> git-rev-parse --parsopt directly with the same call too (as it doesn't
> use git-sh-setup).

  Here is a patch that should fix all those issues at once, replace
yours.  I tested it with this minimal test:

    $ cat parseopt.sh
    #!/bin/sh

    OPTIONS_KEEPDASHDASH=
    OPTIONS_SPEC="\
    foo
    --
    "
    . git-sh-setup
    for i in "$@"; do echo "$i"; done
    $ ./parseopt.sh " * hahahah	bleh"
    --
     * hahahah     bleh
    $ ./parseopt.sh -asd " * hahahah     bleh"
    error: unknown switch `a'
    usage: foo


    $ echo $?
    129

which fix your bug, and still behaves as advertised.


From 3c2095533094ff6d82272dc36d9f576b0e81d135 Mon Sep 17 00:00:00 2001
From: Pierre Habouzit <madcoder@debian.org>
Date: Thu, 8 Nov 2007 10:32:11 +0100
Subject: [PATCH] Prevent eval of $(git-rev-parse --parseopt) output to be shell-expansed.

Thanks to Junio for having spotted this.
Use the preferred $(...) form rather than ``

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-clone.sh    |    2 +-
 git-sh-setup.sh |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index f216f03..24ad179 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -36,7 +36,7 @@ usage() {
 	exec "$0" -h
 }
 
-eval `echo "$OPTIONS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?`
+eval "$(echo "$OPTIONS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
 
 get_repo_base() {
 	(
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index e1cf885..5aa62dd 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -23,9 +23,13 @@ if test -n "$OPTIONS_SPEC"; then
 
 	parseopt_extra=
 	[ -n "$OPTIONS_KEEPDASHDASH" ] &&
-		parseopt_extra="$parseopt_extra --keep-dashdash"
+		parseopt_extra="--keep-dashdash"
 
-	eval `echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@" || echo exit $?`
+	eval "$(
+		echo "$OPTIONS_SPEC" |
+			git rev-parse --parseopt $parseopt_extra -- "$@" ||
+		echo exit $?
+	)"
 else
 	usage() {
 		die "Usage: $0 $USAGE"
-- 
1.5.3.5.1598.gdef4e-dirty


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