threads / patch / 31937

patchFixes handling of --reference argument.

Subject: [PATCH] Fixes handling of --reference argument.

## tl;dr

5 messages between Oct 25, 2012 and Oct 26, 2012. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

szager@google.com· Oct 25, 2012, 04:52 UTC · lore

Signed-off-by: Stefan Zager <szager@google.com>
---
 git-submodule.sh |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

Show changes to git-submodule.sh +0 −1

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· Oct 25, 2012, 08:36 UTC · re: szager@google.com · lore

Re: [PATCH] Fixes handling of --reference argument.

On Wed, Oct 24, 2012 at 09:52:52PM -0700, szager@google.com wrote:

Show 15 quoted lines

> Signed-off-by: Stefan Zager <szager@google.com>
> ---
>  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· Oct 25, 2012, 10:45 UTC · re: Jeff King · lore

Re: [PATCH] Fixes handling of --reference argument.

On Thu, Oct 25, 2012 at 04:36:26AM -0400, Jeff King wrote:

Show 16 quoted lines

> 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· Oct 25, 2012, 21:32 UTC · re: W. Trevor King · lore

Re: [PATCH] Fixes handling of --reference argument.

Am 25.10.2012 12:45, schrieb W. Trevor King:

Show 21 quoted lines

> 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=<repo>" (opposed to "--reference <repo>", 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=<repo> option

and the commit message should begin with:

   Doing a shift there is wrong because there is no extra argument
   to consume when "--reference=<repo>" 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· Oct 26, 2012, 00:39 UTC · re: Jens Lehmann · lore

Re: [PATCH] Fixes handling of --reference argument.

On Thu, Oct 25, 2012 at 11:32:29PM +0200, Jens Lehmann wrote:

Show 19 quoted lines

> >>> @@ -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=<repo>" (opposed to "--reference <repo>",
> 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.

Show 9 quoted lines

> So tested and Acked-By me, but me thinks the subject should read:
> 
>    [PATCH] submodule add: Fix handling of the --reference=<repo> option
> 
> and the commit message should begin with:
> 
>    Doing a shift there is wrong because there is no extra argument
>    to consume when "--reference=<repo>" 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