{"thread":{"id":"10719","subject":"[PATCH] git-sh-setup: fix parseopt `eval`.","startedAt":"2007-11-08T07:09:29Z","lastAt":"2007-11-08T09:35:15Z","messageCount":4,"participants":["Junio C Hamano","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58804","messageId":"7vr6j15i3a.fsf@gitster.siamese.dyndns.org","threadId":"10719","inReplyTo":null,"subject":"[PATCH] git-sh-setup: fix parseopt `eval`.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-08T07:09:29Z","receivedAt":"2007-11-08T07:09:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The 'automagic parseopt' support corrupted non option parameters\nthat had IFS characters in them. The worst case can be seen\nwhen it has a non option parameter like this:\n\n\t$1=\" * some string blech\"\n\nSigned-off-by: Junio C Hamano \n---\n\n * I had \"git pull -n . to/pic-branch\" in Meta/PU which was\n affected by this bug, expanding the \" * \" bullet before the\n merge message that is passed from git-pull to git-merge.\n\n I am a bit upset because I _knew_ that the eval was wrong\n when I first saw it, but somehow I forgot about it when I\n made it land on 'next'. My fault.\n\n git-sh-setup.sh | 11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex e1cf885..f1c4839 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -21,11 +21,12 @@ if test -n \"$OPTIONS_SPEC\"; then\n \t\texec \"$0\" -h\n \t}\n \n-\tparseopt_extra=\n-\t[ -n \"$OPTIONS_KEEPDASHDASH\" ] &&\n-\t\tparseopt_extra=\"$parseopt_extra --keep-dashdash\"\n-\n-\teval `echo \"$OPTIONS_SPEC\" | git rev-parse --parseopt $parseopt_extra -- \"$@\" || echo exit $?`\n+\t[ -n \"$OPTIONS_KEEPDASHDASH\" ] && parseopt_extra=\"--keep-dashdash\"\n+\tparsed=$(\n+\t\techo \"$OPTIONS_SPEC\" |\n+\t\tgit rev-parse --parseopt $parseopt_extra -- \"$@\"\n+\t) &&\n+\teval \"$parsed\" || exit\n else\n \tusage() {\n \t\tdie \"Usage: $0 $USAGE\"\n-- \n1.5.3.5.1617.g65b5b\n"},{"id":"58822","messageId":"20071108081556.GB6528@artemis.corp","threadId":"10719","inReplyTo":"7vr6j15i3a.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-sh-setup: fix parseopt `eval`.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-08T08:15:56Z","receivedAt":"2007-11-08T08:15:56Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Nov 08, 2007 at 07:09:29AM +0000, Junio C Hamano wrote:\n> The 'automagic parseopt' support corrupted non option parameters\n> that had IFS characters in them. The worst case can be seen\n> when it has a non option parameter like this:\n\nhu sorry about that, I should have put \"\" around the ``. I knew it also\nbut it slipped my mind too. I believe this works as well:\n\neval \"$(echo \"$OPTIONS_SPEC\" | git rev-parse --parseopt $parseopt_extra -- \"$@\" || echo exit $?)\"\n\nI like it better because you will then exit with an exit 129 wich is\nwhat we want (and what I documented would work too :P)\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"58835","messageId":"20071108091402.GA7391@artemis.corp","threadId":"10719","inReplyTo":"7vr6j15i3a.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-sh-setup: fix parseopt `eval`.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-08T09:14:02Z","receivedAt":"2007-11-08T09:14:02Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Nov 08, 2007 at 07:09:29AM +0000, Junio C Hamano wrote:\n> The 'automagic parseopt' support corrupted non option parameters\n> that had IFS characters in them. The worst case can be seen\n> when it has a non option parameter like this:\n> \n> \t$1=\" * some string blech\"\n> \n> Signed-off-by: Junio C Hamano \n\n> -\tparseopt_extra=\n> -\t[ -n \"$OPTIONS_KEEPDASHDASH\" ] &&\n> -\t\tparseopt_extra=\"$parseopt_extra --keep-dashdash\"\n> +\t[ -n \"$OPTIONS_KEEPDASHDASH\" ] && parseopt_extra=\"--keep-dashdash\"\n\n oh and this part is wrong because you're affected by $parseopt_extra\nenvironment poisonning. And you have to fix git-clone.sh that uses\ngit-rev-parse --parsopt directly with the same call too (as it doesn't\nuse git-sh-setup).\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"58842","messageId":"20071108093515.GC7391@artemis.corp","threadId":"10719","inReplyTo":"20071108091402.GA7391@artemis.corp","subject":"Re: [PATCH] git-sh-setup: fix parseopt `eval`.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-08T09:35:15Z","receivedAt":"2007-11-08T09:35:15Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Nov 08, 2007 at 09:14:02AM +0000, Pierre Habouzit wrote:\n> On Thu, Nov 08, 2007 at 07:09:29AM +0000, Junio C Hamano wrote:\n> > The 'automagic parseopt' support corrupted non option parameters\n> > that had IFS characters in them. The worst case can be seen\n> > when it has a non option parameter like this:\n> > \n> > \t$1=\" * some string blech\"\n> > \n> > Signed-off-by: Junio C Hamano \n> \n> > -\tparseopt_extra=\n> > -\t[ -n \"$OPTIONS_KEEPDASHDASH\" ] &&\n> > -\t\tparseopt_extra=\"$parseopt_extra --keep-dashdash\"\n> > +\t[ -n \"$OPTIONS_KEEPDASHDASH\" ] && parseopt_extra=\"--keep-dashdash\"\n> \n> oh and this part is wrong because you're affected by $parseopt_extra\n> environment poisonning. And you have to fix git-clone.sh that uses\n> git-rev-parse --parsopt directly with the same call too (as it doesn't\n> use git-sh-setup).\n\n Here is a patch that should fix all those issues at once, replace\nyours. I tested it with this minimal test:\n\n $ cat parseopt.sh\n #!/bin/sh\n\n OPTIONS_KEEPDASHDASH=\n OPTIONS_SPEC=\"\\\n foo\n --\n \"\n . git-sh-setup\n for i in \"$@\"; do echo \"$i\"; done\n $ ./parseopt.sh \" * hahahah\tbleh\"\n --\n * hahahah bleh\n $ ./parseopt.sh -asd \" * hahahah bleh\"\n error: unknown switch `a'\n usage: foo\n\n\n $ echo $?\n 129\n\nwhich fix your bug, and still behaves as advertised.\n\n\nFrom 3c2095533094ff6d82272dc36d9f576b0e81d135 Mon Sep 17 00:00:00 2001\nFrom: Pierre Habouzit \nDate: Thu, 8 Nov 2007 10:32:11 +0100\nSubject: [PATCH] Prevent eval of $(git-rev-parse --parseopt) output to be shell-expansed.\n\nThanks to Junio for having spotted this.\nUse the preferred $(...) form rather than ``\n\nSigned-off-by: Pierre Habouzit \n---\n git-clone.sh | 2 +-\n git-sh-setup.sh | 8 ++++++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/git-clone.sh b/git-clone.sh\nindex f216f03..24ad179 100755\n--- a/git-clone.sh\n+++ b/git-clone.sh\n@@ -36,7 +36,7 @@ usage() {\n \texec \"$0\" -h\n }\n \n-eval `echo \"$OPTIONS_SPEC\" | git rev-parse --parseopt -- \"$@\" || echo exit $?`\n+eval \"$(echo \"$OPTIONS_SPEC\" | git rev-parse --parseopt -- \"$@\" || echo exit $?)\"\n \n get_repo_base() {\n \t(\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex e1cf885..5aa62dd 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -23,9 +23,13 @@ if test -n \"$OPTIONS_SPEC\"; then\n \n \tparseopt_extra=\n \t[ -n \"$OPTIONS_KEEPDASHDASH\" ] &&\n-\t\tparseopt_extra=\"$parseopt_extra --keep-dashdash\"\n+\t\tparseopt_extra=\"--keep-dashdash\"\n \n-\teval `echo \"$OPTIONS_SPEC\" | git rev-parse --parseopt $parseopt_extra -- \"$@\" || echo exit $?`\n+\teval \"$(\n+\t\techo \"$OPTIONS_SPEC\" |\n+\t\t\tgit rev-parse --parseopt $parseopt_extra -- \"$@\" ||\n+\t\techo exit $?\n+\t)\"\n else\n \tusage() {\n \t\tdie \"Usage: $0 $USAGE\"\n-- \n1.5.3.5.1598.gdef4e-dirty\n\n"}]}