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

From: Jens Lehmann <jens.lehmann@web.de>
Date: Oct 25, 2012, 21:32 UTC
Message-ID: <5089AFED.7060404@web.de>
In-Reply-To: <20121025104519.GA3816@odin.tremily.us>

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?