# [PATCH] Fixes handling of --reference argument. 5 messages from 2012-10-25 to 2012-10-26. Participants: szager@google.com, Jeff King, W. Trevor King, Jens Lehmann. Thread: https://gitlist.dev/t/31937 ## szager@google.com, 2012-10-25 04:52 Subject: [PATCH] Fixes handling of --reference argument. Message-ID: <5088c5a4.L25tOcUVCSwBRpYF%szager@google.com> URL: https://gitlist.dev/e/5088c5a4.L25tOcUVCSwBRpYF%25szager%40google.com ``` 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 ``` ## Jeff King, 2012-10-25 08:36 Subject: Re: [PATCH] Fixes handling of --reference argument. Message-ID: <20121025083625.GA8390@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121025083625.GA8390%40sigill.intra.peff.net In-Reply-To: <5088c5a4.L25tOcUVCSwBRpYF%szager@google.com> ``` On Wed, Oct 24, 2012 at 09:52:52PM -0700, szager@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 ``` ## W. Trevor King, 2012-10-25 10:45 Subject: Re: [PATCH] Fixes handling of --reference argument. Message-ID: <20121025104519.GA3816@odin.tremily.us> URL: https://gitlist.dev/e/20121025104519.GA3816%40odin.tremily.us In-Reply-To: <20121025083625.GA8390@sigill.intra.peff.net> ``` On Thu, Oct 25, 2012 at 04:36:26AM -0400, Jeff King wrote: > On Wed, Oct 24, 2012 at 09:52:52PM -0700, szager@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 ``` ## Jens Lehmann, 2012-10-25 21:32 Subject: Re: [PATCH] Fixes handling of --reference argument. Message-ID: <5089AFED.7060404@web.de> URL: https://gitlist.dev/e/5089AFED.7060404%40web.de In-Reply-To: <20121025104519.GA3816@odin.tremily.us> ``` 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, szager@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? ``` ## Jeff King, 2012-10-26 00:39 Subject: Re: [PATCH] Fixes handling of --reference argument. Message-ID: <20121026003934.GA20064@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121026003934.GA20064%40sigill.intra.peff.net In-Reply-To: <5089AFED.7060404@web.de> ``` 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 ```