{"thread":{"id":"31937","subject":"[PATCH] Fixes handling of --reference argument.","startedAt":"2012-10-25T04:52:52Z","lastAt":"2012-10-26T00:39:34Z","messageCount":5,"participants":["szager@google.com","Jeff King","W. Trevor King","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"201841","messageId":"5088c5a4.L25tOcUVCSwBRpYF%szager@google.com","threadId":"31937","inReplyTo":null,"subject":"[PATCH] Fixes handling of --reference argument.","fromName":"","fromEmail":"szager@google.com","sentAt":"2012-10-25T04:52:52Z","receivedAt":"2012-10-25T04:52:52Z","isPatch":true,"sender":{"key":"szager@google.com","avatar":null},"body":"Signed-off-by: Stefan Zager \n---\n git-submodule.sh | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ab6b110..dcceb43 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -270,7 +270,6 @@ cmd_add()\n \t\t\t;;\n \t\t--reference=*)\n \t\t\treference=\"$1\"\n-\t\t\tshift\n \t\t\t;;\n \t\t--)\n \t\t\tshift\n-- \n1.7.7.3\n"},{"id":"201867","messageId":"20121025083625.GA8390@sigill.intra.peff.net","threadId":"31937","inReplyTo":"5088c5a4.L25tOcUVCSwBRpYF%szager@google.com","subject":"Re: [PATCH] Fixes handling of --reference argument.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-25T08:36:26Z","receivedAt":"2012-10-25T08:36:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 24, 2012 at 09:52:52PM -0700, szager@google.com wrote:\n\n> Signed-off-by: Stefan Zager \n> ---\n> git-submodule.sh | 1 -\n> 1 files changed, 0 insertions(+), 1 deletions(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index ab6b110..dcceb43 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -270,7 +270,6 @@ cmd_add()\n> \t\t\t;;\n> \t\t--reference=*)\n> \t\t\treference=\"$1\"\n> -\t\t\tshift\n> \t\t\t;;\n\nIs that right? We'll unconditionally do a \"shift\" at the end of the\nloop. If it were a two-part argument like \"--reference foo\", the extra\nshift would make sense, but for \"--reference=*\", no extra shift should\nbe neccessary. Am I missing something?\n\n-Peff\n"},{"id":"201896","messageId":"20121025104519.GA3816@odin.tremily.us","threadId":"31937","inReplyTo":"20121025083625.GA8390@sigill.intra.peff.net","subject":"Re: [PATCH] Fixes handling of --reference argument.","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2012-10-25T10:45:19Z","receivedAt":"2012-10-25T10:45:19Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Oct 25, 2012 at 04:36:26AM -0400, Jeff King wrote:\n> On Wed, Oct 24, 2012 at 09:52:52PM -0700, szager@google.com wrote:\n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index ab6b110..dcceb43 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -270,7 +270,6 @@ cmd_add()\n> > \t\t\t;;\n> > \t\t--reference=*)\n> > \t\t\treference=\"$1\"\n> > -\t\t\tshift\n> > \t\t\t;;\n> \n> Is that right? We'll unconditionally do a \"shift\" at the end of the\n> loop. If it were a two-part argument like \"--reference foo\", the extra\n> shift would make sense, but for \"--reference=*\", no extra shift should\n> be neccessary. Am I missing something?\n\nBoth the patch and Jeff's analysis are right. You only need an\nin-case shift if you consume \"$2\", or you're on ‘--’ and you're\nbreaking before the end-of-case shift.\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"201920","messageId":"5089AFED.7060404@web.de","threadId":"31937","inReplyTo":"20121025104519.GA3816@odin.tremily.us","subject":"Re: [PATCH] Fixes handling of --reference argument.","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-10-25T21:32:29Z","receivedAt":"2012-10-25T21:32:29Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 25.10.2012 12:45, schrieb W. Trevor King:\n> On Thu, Oct 25, 2012 at 04:36:26AM -0400, Jeff King wrote:\n>> On Wed, Oct 24, 2012 at 09:52:52PM -0700, szager@google.com wrote:\n>>> diff --git a/git-submodule.sh b/git-submodule.sh\n>>> index ab6b110..dcceb43 100755\n>>> --- a/git-submodule.sh\n>>> +++ b/git-submodule.sh\n>>> @@ -270,7 +270,6 @@ cmd_add()\n>>> \t\t\t;;\n>>> \t\t--reference=*)\n>>> \t\t\treference=\"$1\"\n>>> -\t\t\tshift\n>>> \t\t\t;;\n>>\n>> Is that right? We'll unconditionally do a \"shift\" at the end of the\n>> loop. If it were a two-part argument like \"--reference foo\", the extra\n>> shift would make sense, but for \"--reference=*\", no extra shift should\n>> be neccessary. Am I missing something?\n> \n> Both the patch and Jeff's analysis are right. You only need an\n> in-case shift if you consume \"$2\", or you're on ‘--’ and you're\n> breaking before the end-of-case shift.\n\nRight you are. The shift there is wrong, as there is no extra argument\nto consume for \"--reference=\" (opposed to \"--reference \",\nalso see cmd_update() where this is done right).\n\nSo tested and Acked-By me, but me thinks the subject should read:\n\n [PATCH] submodule add: Fix handling of the --reference= option\n\nand the commit message should begin with:\n\n Doing a shift there is wrong because there is no extra argument\n to consume when \"--reference=\" is used (note the '=' instead\n of a space).\n\nPeff, is it ok for you to squash that in or do you want Stefan to resend?\n"},{"id":"201929","messageId":"20121026003934.GA20064@sigill.intra.peff.net","threadId":"31937","inReplyTo":"5089AFED.7060404@web.de","subject":"Re: [PATCH] Fixes handling of --reference argument.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-26T00:39:34Z","receivedAt":"2012-10-26T00:39:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 25, 2012 at 11:32:29PM +0200, Jens Lehmann wrote:\n\n> >>> @@ -270,7 +270,6 @@ cmd_add()\n> >>> \t\t\t;;\n> >>> \t\t--reference=*)\n> >>> \t\t\treference=\"$1\"\n> >>> -\t\t\tshift\n> >>> \t\t\t;;\n> >>\n> >> Is that right? We'll unconditionally do a \"shift\" at the end of the\n> >> loop. If it were a two-part argument like \"--reference foo\", the extra\n> >> shift would make sense, but for \"--reference=*\", no extra shift should\n> >> be neccessary. Am I missing something?\n> > \n> > Both the patch and Jeff's analysis are right. You only need an\n> > in-case shift if you consume \"$2\", or you're on ‘--’ and you're\n> > breaking before the end-of-case shift.\n> \n> Right you are. The shift there is wrong, as there is no extra argument\n> to consume for \"--reference=\" (opposed to \"--reference \",\n> also see cmd_update() where this is done right).\n\nOh, the problem is that I'm an idiot, and for some reason read it as\n_adding_ the bogus shift, not removing it. Patch is clearly correct.\n\n> So tested and Acked-By me, but me thinks the subject should read:\n> \n> [PATCH] submodule add: Fix handling of the --reference= option\n> \n> and the commit message should begin with:\n> \n> Doing a shift there is wrong because there is no extra argument\n> to consume when \"--reference=\" is used (note the '=' instead\n> of a space).\n\nYeah, I think it makes sense to explain why it is wrong in the commit\nmessage (I'll blame that for my lack of common sense above :) ).\n\n> Peff, is it ok for you to squash that in or do you want Stefan to resend?\n\nI can squash it in. Thanks all.\n\n-Peff\n"}]}