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=<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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html