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

From: Jeff King <peff@peff.net>
Date: Oct 26, 2012, 00:39 UTC
Message-ID: <20121026003934.GA20064@sigill.intra.peff.net>
In-Reply-To: <5089AFED.7060404@web.de>

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