{"thread":{"id":"51711","subject":"[PATCH] git-submodule: fix expansion of depth for cmd_update","startedAt":"2019-08-22T20:31:20Z","lastAt":"2019-08-22T22:38:03Z","messageCount":3,"participants":["Jacob Keller","Junio C Hamano","Keller, Jacob E"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380975","messageId":"20190822203114.18805-1-jacob.e.keller@intel.com","threadId":"51711","inReplyTo":null,"subject":"[PATCH] git-submodule: fix expansion of depth for cmd_update","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2019-08-22T20:31:14Z","receivedAt":"2019-08-22T20:31:20Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe depth variable already contains \"--depth=\", so expanding it with an\nadditional --depth when invoking the update-clone git submodule--helper\nis incorrect.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n\nI'm *reasonably* sure this is correct, but I am not sure how to test it.\nIt's possible that it expands to \"--depth --depth=N\" and somehow this gets\nhandled properly?\n\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c7f58c5756f7..4e7fc8bf3652 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -547,7 +547,7 @@ cmd_update()\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+\"$reference\"} \\\n \t\t${dissociate:+\"--dissociate\"} \\\n-\t\t${depth:+--depth \"$depth\"} \\\n+\t\t${depth:+\"$depth\"} \\\n \t\t$recommend_shallow \\\n \t\t$jobs \\\n \t\t-- \\\n-- \n2.23.0.245.gf157bbb9169d\n\n"},{"id":"380977","messageId":"xmqqh868oqxv.fsf@gitster-ct.c.googlers.com","threadId":"51711","inReplyTo":"20190822203114.18805-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] git-submodule: fix expansion of depth for cmd_update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-22T21:01:16Z","receivedAt":"2019-08-22T21:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> The depth variable already contains \"--depth=\", so expanding it with an\n> additional --depth when invoking the update-clone git submodule--helper\n> is incorrect.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n>\n> I'm *reasonably* sure this is correct, but I am not sure how to test it.\n> It's possible that it expands to \"--depth --depth=N\" and somehow this gets\n> handled properly?\n\nI agree with your eyeballing of all the assignments to the variable,\nand other references to $depth take either one of these two forms:\n\n\tgit submodule--helper ... ${depth:+\"$depth\"} ...\n\tgit submodule--helper ... $depth ...\n\nAs long as \"git submodule ... --depth <depth> ...\" gets called with\n<depth> that does not have $IFS, either would work fine, but the\nformer is correct even when <depth> has problematic characters in it\nand your patch uses that form, too).\n\nHowever.\n\nThe command line parser for update_clone() stuffs --depth as a\nstring to suc.depth, and then the machinery ends up calling \nprepare_to_clone_next_submodule() with such an instance of suc\n(struct submodule_update_clone).  Then that function just pushes the\nsuc->depth to an argv array used to spawn a \"submodule--helper clone\".\n\nSo passing \"--depth --depth=23\" would be \"correct\", sadly, in that\ncodepath (I am not saying other codepaths would not call the same\nprepare_to_clone_next_submodule() with \"--depth 23\", as I didn't\ncheck, and if there is such a codepath, it would break).  \n\nWe may need to clean the mess up X-<.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index c7f58c5756f7..4e7fc8bf3652 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -547,7 +547,7 @@ cmd_update()\n>  \t\t${update:+--update \"$update\"} \\\n>  \t\t${reference:+\"$reference\"} \\\n>  \t\t${dissociate:+\"--dissociate\"} \\\n> -\t\t${depth:+--depth \"$depth\"} \\\n> +\t\t${depth:+\"$depth\"} \\\n>  \t\t$recommend_shallow \\\n>  \t\t$jobs \\\n>  \t\t-- \\\n"},{"id":"380989","messageId":"02874ECE860811409154E81DA85FBB5896860344@ORSMSX121.amr.corp.intel.com","threadId":"51711","inReplyTo":"xmqqh868oqxv.fsf@gitster-ct.c.googlers.com","subject":"RE: [PATCH] git-submodule: fix expansion of depth for cmd_update","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2019-08-22T22:38:00Z","receivedAt":"2019-08-22T22:38:03Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"> -----Original Message-----\n> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of\n> Junio C Hamano\n> Sent: Thursday, August 22, 2019 2:01 PM\n> To: Keller, Jacob E <jacob.e.keller@intel.com>\n> Cc: git@vger.kernel.org; Jacob Keller <jacob.keller@gmail.com>\n> Subject: Re: [PATCH] git-submodule: fix expansion of depth for cmd_update\n> \n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n> > From: Jacob Keller <jacob.keller@gmail.com>\n> >\n> > The depth variable already contains \"--depth=\", so expanding it with an\n> > additional --depth when invoking the update-clone git submodule--helper\n> > is incorrect.\n> >\n> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> > ---\n> >\n> > I'm *reasonably* sure this is correct, but I am not sure how to test it.\n> > It's possible that it expands to \"--depth --depth=N\" and somehow this gets\n> > handled properly?\n> \n> I agree with your eyeballing of all the assignments to the variable,\n> and other references to $depth take either one of these two forms:\n> \n> \tgit submodule--helper ... ${depth:+\"$depth\"} ...\n> \tgit submodule--helper ... $depth ...\n> \n> As long as \"git submodule ... --depth <depth> ...\" gets called with\n> <depth> that does not have $IFS, either would work fine, but the\n> former is correct even when <depth> has problematic characters in it\n> and your patch uses that form, too).\n> \n> However.\n> \n> The command line parser for update_clone() stuffs --depth as a\n> string to suc.depth, and then the machinery ends up calling\n> prepare_to_clone_next_submodule() with such an instance of suc\n> (struct submodule_update_clone).  Then that function just pushes the\n> suc->depth to an argv array used to spawn a \"submodule--helper clone\".\n> \n> So passing \"--depth --depth=23\" would be \"correct\", sadly, in that\n> codepath (I am not saying other codepaths would not call the same\n> prepare_to_clone_next_submodule() with \"--depth 23\", as I didn't\n> check, and if there is such a codepath, it would break).\n> \n\nOk, so it's technically correct, but weird. I had trouble understanding it.\n\n> We may need to clean the mess up X-<.\n> \n\nYea, I got confused looking at this code. Though if I recall, we were working towards rewriting it in C anyways (which is what the submodule--helper is doing).\n\nThanks,\nJake\n\n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index c7f58c5756f7..4e7fc8bf3652 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -547,7 +547,7 @@ cmd_update()\n> >  \t\t${update:+--update \"$update\"} \\\n> >  \t\t${reference:+\"$reference\"} \\\n> >  \t\t${dissociate:+\"--dissociate\"} \\\n> > -\t\t${depth:+--depth \"$depth\"} \\\n> > +\t\t${depth:+\"$depth\"} \\\n> >  \t\t$recommend_shallow \\\n> >  \t\t$jobs \\\n> >  \t\t-- \\\n"}]}