Re: [PATCH] Fixes handling of --reference argument.
On Thu, Oct 25, 2012 at 11:32:29PM +0200, Jens Lehmann wrote: > >>> @@ -270,7 +270,6 @@ cmd_add() > >>> ;; > >>> --reference=*) > >>> reference="$1" > >>> - shift > >>> ;; > >> > >> Is that right? We'll unconditionally do a "shift" at the end of the > >> loop. If it were a two-part argument like "--reference foo", the extra > >> shift would make sense, but for "--reference=*", no extra shift should > >> be neccessary. Am I missing something? > > > > Both the patch and Jeff's analysis are right. You only need an > > in-case shift if you consume "$2", or you're on ‘--’ and you're > > breaking before the end-of-case shift. > > Right you are. The shift there is wrong, as there is no extra argument > to consume for "--reference=" (opposed to "--reference ", > also see cmd_update() where this is done right). Oh, the problem is that I'm an idiot, and for some reason read it as _adding_ the bogus shift, not removing it. Patch is clearly correct. > So tested and Acked-By me, but me thinks the subject should read: > >[PATCH] submodule add: Fix handling of the --reference= option > > and the commit message should begin with: > >Doing a shift there is wrong because there is no extra argument >to consume when "--reference=" is used (note the '=' instead >of a space). Yeah, I think it makes sense to explain why it is wrong in the commit message (I'll blame that for my lack of common sense above :) ). > Peff, is it ok for you to squash that in or do you want Stefan to resend? I can squash it in. Thanks all. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fixes handling of --reference argument.
Am 25.10.2012 12:45, schrieb W. Trevor King: > On Thu, Oct 25, 2012 at 04:36:26AM -0400, Jeff King wrote: >> On Wed, Oct 24, 2012 at 09:52:52PM -0700, sza...@google.com wrote: >>> diff --git a/git-submodule.sh b/git-submodule.sh >>> index ab6b110..dcceb43 100755 >>> --- a/git-submodule.sh >>> +++ b/git-submodule.sh >>> @@ -270,7 +270,6 @@ cmd_add() >>> ;; >>> --reference=*) >>> reference="$1" >>> - shift >>> ;; >> >> Is that right? We'll unconditionally do a "shift" at the end of the >> loop. If it were a two-part argument like "--reference foo", the extra >> shift would make sense, but for "--reference=*", no extra shift should >> be neccessary. Am I missing something? > > Both the patch and Jeff's analysis are right. You only need an > in-case shift if you consume "$2", or you're on ‘--’ and you're > breaking before the end-of-case shift. Right you are. The shift there is wrong, as there is no extra argument to consume for "--reference=" (opposed to "--reference ", also see cmd_update() where this is done right). So tested and Acked-By me, but me thinks the subject should read: [PATCH] submodule add: Fix handling of the --reference= option and the commit message should begin with: Doing a shift there is wrong because there is no extra argument to consume when "--reference=" is used (note the '=' instead of a space). Peff, is it ok for you to squash that in or do you want Stefan to resend? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fixes handling of --reference argument.
On Thu, Oct 25, 2012 at 04:36:26AM -0400, Jeff King wrote: > On Wed, Oct 24, 2012 at 09:52:52PM -0700, sza...@google.com wrote: > > diff --git a/git-submodule.sh b/git-submodule.sh > > index ab6b110..dcceb43 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > @@ -270,7 +270,6 @@ cmd_add() > > ;; > > --reference=*) > > reference="$1" > > - shift > > ;; > > Is that right? We'll unconditionally do a "shift" at the end of the > loop. If it were a two-part argument like "--reference foo", the extra > shift would make sense, but for "--reference=*", no extra shift should > be neccessary. Am I missing something? Both the patch and Jeff's analysis are right. You only need an in-case shift if you consume "$2", or you're on ‘--’ and you're breaking before the end-of-case shift. -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH] Fixes handling of --reference argument.
On Wed, Oct 24, 2012 at 09:52:52PM -0700, sza...@google.com wrote: > Signed-off-by: Stefan Zager > --- > git-submodule.sh |1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index ab6b110..dcceb43 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -270,7 +270,6 @@ cmd_add() > ;; > --reference=*) > reference="$1" > - shift > ;; Is that right? We'll unconditionally do a "shift" at the end of the loop. If it were a two-part argument like "--reference foo", the extra shift would make sense, but for "--reference=*", no extra shift should be neccessary. Am I missing something? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fixes handling of --reference argument.
Signed-off-by: Stefan Zager --- git-submodule.sh |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index ab6b110..dcceb43 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -270,7 +270,6 @@ cmd_add() ;; --reference=*) reference="$1" - shift ;; --) shift -- 1.7.7.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
