{"thread":{"id":"42501","subject":"[BUG] git-submodule has bash-ism?","startedAt":"2016-05-31T23:08:03Z","lastAt":"2016-06-01T21:08:32Z","messageCount":17,"participants":["Junio C Hamano","Stefan Beller","Jeff King","John Keeping","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"287954","messageId":"xmqq1t4h3jxo.fsf@gitster.mtv.corp.google.com","threadId":"42501","inReplyTo":null,"subject":"[BUG] git-submodule has bash-ism?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-31T23:08:03Z","receivedAt":"2016-05-31T23:08:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"relative_path ()\n{\n\tlocal target curdir result\n\ttarget=$1\n\tcurdir=${2-$wt_prefix}\n\nI am hoping that Stefan's \"gradually rewrite things in C\" will make\nit unnecessary to worry about this one.  \"git submodule\" would not\nwork correctly on posixly correct shells in the meantime.\n"},{"id":"287956","messageId":"CAGZ79kYoZfwWfigUZJM9ryTSNv-WE-0owxF=iUSHsc_nQ9rWVA@mail.gmail.com","threadId":"42501","inReplyTo":"xmqq1t4h3jxo.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-31T23:16:20Z","receivedAt":"2016-05-31T23:16:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 31, 2016 at 4:08 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> relative_path ()\n> {\n>         local target curdir result\n>         target=$1\n>         curdir=${2-$wt_prefix}\n>\n> I am hoping that Stefan's \"gradually rewrite things in C\" will make\n> it unnecessary to worry about this one.  \"git submodule\" would not\n> work correctly on posixly correct shells in the meantime.\n\nnoted.\n\nMaybe as a smaller step we can expose the relative_path from the\nsubmodule--helper\ninstead of rewriting all actual users first.\n\nThanks for pointing out,\nStefan\n\n\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"287965","messageId":"20160601002759.11592-1-sbeller@google.com","threadId":"42501","inReplyTo":"xmqq1t4h3jxo.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] submodule: remove bashism from shell script","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-01T00:27:59Z","receivedAt":"2016-06-01T00:27:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Junio pointed out `relative_path` was using bashisms via the\nlocal variables. As the longer term goal is to rewrite most of the\nsubmodule code in C, do it now.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n* developed on top of origin/master + \"[PATCH] submodule--helper: offer a\n  consistent API\" which I just sent.\n* This fix looks amazingly simple (it even worked on the first try),\n  so I temporarily did a s|printf(\"%s\", relative_path(argv[1], argv[2], &sb));|printf(\"bogus\");|\n  to ensure we actually catch failures in the display path. And we do!\n  \nThanks,\nStefan\n\n builtin/submodule--helper.c | 12 +++++++++++\n git-submodule.sh            | 51 +++++++--------------------------------------\n 2 files changed, 20 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f0b2c4f..926d205 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -831,6 +831,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tif (argc != 3)\n+\t\tdie(\"submodule--helper relative_path takes exactly 2 arguments, got %d\", argc);\n+\n+\tprintf(\"%s\", relative_path(argv[1], argv[2], &sb));\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -841,6 +852,7 @@ static struct cmd_struct commands[] = {\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n \t{\"update-clone\", update_clone},\n+\t{\"relative-path\", resolve_relative_path},\n \t{\"resolve-relative-url\", resolve_relative_url},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test},\n \t{\"init\", module_init}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex fadbe5d..7fe8a51 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -46,41 +46,6 @@ prefix=\n custom_name=\n depth=\n \n-# Resolve a path to be relative to another path.  This is intended for\n-# converting submodule paths when git-submodule is run in a subdirectory\n-# and only handles paths where the directory separator is '/'.\n-#\n-# The output is the first argument as a path relative to the second argument,\n-# which defaults to $wt_prefix if it is omitted.\n-relative_path ()\n-{\n-\tlocal target curdir result\n-\ttarget=$1\n-\tcurdir=${2-$wt_prefix}\n-\tcurdir=${curdir%/}\n-\tresult=\n-\n-\twhile test -n \"$curdir\"\n-\tdo\n-\t\tcase \"$target\" in\n-\t\t\"$curdir/\"*)\n-\t\t\ttarget=${target#\"$curdir\"/}\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\n-\t\tresult=\"${result}../\"\n-\t\tif test \"$curdir\" = \"${curdir%/*}\"\n-\t\tthen\n-\t\t\tcurdir=\n-\t\telse\n-\t\t\tcurdir=\"${curdir%/*}\"\n-\t\tfi\n-\tdone\n-\n-\techo \"$result$target\"\n-}\n-\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n@@ -354,14 +319,14 @@ cmd_foreach()\n \t\tdie_if_unmatched \"$mode\"\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n-\t\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n+\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n \t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n \t\t\tname=$(git submodule--helper name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(relative_path \"$sm_path\") &&\n+\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n \t\t\t\t# we make $path available to scripts ...\n \t\t\t\tpath=$sm_path &&\n \t\t\t\tif test $# -eq 1\n@@ -465,7 +430,7 @@ cmd_deinit()\n \t\tdie_if_unmatched \"$mode\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \n-\t\tdisplaypath=$(relative_path \"$sm_path\")\n+\t\tdisplaypath=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\")\n \n \t\t# Remove the submodule work tree (unless the user already did it)\n \t\tif test -d \"$sm_path\"\n@@ -629,7 +594,7 @@ cmd_update()\n \t\t\tfi\n \t\tfi\n \n-\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n+\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n \n \t\tif test $just_cloned -eq 1\n \t\tthen\n@@ -723,7 +688,7 @@ cmd_update()\n \t\tif test -n \"$recursive\"\n \t\tthen\n \t\t\t(\n-\t\t\t\tprefix=$(relative_path \"$prefix$sm_path/\")\n+\t\t\t\tprefix=$(git submodule--helper relative-path \"$prefix$sm_path/\" \"$wt_prefix\")\n \t\t\t\twt_prefix=\n \t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n@@ -907,7 +872,7 @@ cmd_summary() {\n \t\t! GIT_DIR=\"$name/.git\" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n \t\tmissing_dst=t\n \n-\t\tdisplay_name=$(relative_path \"$name\")\n+\t\tdisplay_name=$(git submodule--helper relative-path \"$name\" \"$wt_prefix\")\n \n \t\ttotal_commits=\n \t\tcase \"$missing_src,$missing_dst\" in\n@@ -1028,7 +993,7 @@ cmd_status()\n \t\tdie_if_unmatched \"$mode\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n-\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n+\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n \t\tif test \"$stage\" = U\n \t\tthen\n \t\t\tsay \"U$sha1 $displaypath\"\n@@ -1131,7 +1096,7 @@ cmd_sync()\n \n \t\tif git config \"submodule.$name.url\" >/dev/null 2>/dev/null\n \t\tthen\n-\t\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n+\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n \t\t\tsay \"$(eval_gettext \"Synchronizing submodule url for '\\$displaypath'\")\"\n \t\t\tgit config submodule.\"$name\".url \"$super_config_url\"\n \n-- \n2.9.0.rc1.2.ge49d2c8.dirty\n"},{"id":"288018","messageId":"xmqqoa7kzy3u.fsf@gitster.mtv.corp.google.com","threadId":"42501","inReplyTo":"xmqq1t4h3jxo.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T16:13:09Z","receivedAt":"2016-06-01T16:13:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> relative_path ()\n> {\n> \tlocal target curdir result\n> \ttarget=$1\n> \tcurdir=${2-$wt_prefix}\n>\n> I am hoping that Stefan's \"gradually rewrite things in C\" will make\n> it unnecessary to worry about this one.  \"git submodule\" would not\n> work correctly on posixly correct shells in the meantime.\n\nThese are two other offenders.\n\n$ git grep '^[\t ]local[ \t]' \\*.sh\nt/t5500-fetch-pack.sh:\tlocal diagport\nt/t7403-submodule-sync.sh:\tlocal root\n\nThe grep gives many other hits, but those in completion are OK; it\nis designed to be specific to bash, and whose tests in t9902 is in\nthe same boat.  A few more near the end of t/test-lib-functions are\nonly for mingw where bash is the only supported shell at least for\nrunning tests.\n"},{"id":"288019","messageId":"xmqqk2i8zxtx.fsf@gitster.mtv.corp.google.com","threadId":"42501","inReplyTo":"xmqqoa7kzy3u.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T16:19:06Z","receivedAt":"2016-06-01T16:19:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> These are two other offenders.\n>\n> $ git grep '^[\t ]local[ \t]' \\*.sh\n> t/t5500-fetch-pack.sh:\tlocal diagport\n> t/t7403-submodule-sync.sh:\tlocal root\n>\n> The grep gives many other hits, but those in completion are OK; it\n> is designed to be specific to bash, and whose tests in t9902 is in\n> the same boat.  A few more near the end of t/test-lib-functions are\n> only for mingw where bash is the only supported shell at least for\n> running tests.\n\nI think this should be sufficient (extra sets of eyeballs are\nappreciated).  For 5500, diagport is not a variable used elsewhere\nand can simply lose the \"local\".  7403 overrides the \"root\" variable\nused in the test framework for no good reason (its use is not about\ntemporarily relocating where the test repositories are created), but\nthey can be made not to clobber the varible by moving them into the\nsubshells it already uses.\n\n t/t5500-fetch-pack.sh     | 1 -\n t/t7403-submodule-sync.sh | 4 ++--\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 9b9bec4..dc305d6 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -556,7 +556,6 @@ check_prot_path () {\n }\n \n check_prot_host_port_path () {\n-\tlocal diagport\n \tcase \"$2\" in\n \t\t*ssh*)\n \t\tpp=ssh\ndiff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\nindex 79bc135..5503ec0 100755\n--- a/t/t7403-submodule-sync.sh\n+++ b/t/t7403-submodule-sync.sh\n@@ -62,13 +62,13 @@ test_expect_success 'change submodule' '\n '\n \n reset_submodule_urls () {\n-\tlocal root\n-\troot=$(pwd) &&\n \t(\n+\t\troot=$(pwd) &&\n \t\tcd super-clone/submodule &&\n \t\tgit config remote.origin.url \"$root/submodule\"\n \t) &&\n \t(\n+\t\troot=$(pwd) &&\n \t\tcd super-clone/submodule/sub-submodule &&\n \t\tgit config remote.origin.url \"$root/submodule\"\n \t)\n"},{"id":"288020","messageId":"20160601163747.GA10721@sigill.intra.peff.net","threadId":"42501","inReplyTo":"xmqqk2i8zxtx.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T16:37:47Z","receivedAt":"2016-06-01T16:37:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > These are two other offenders.\n> >\n> > $ git grep '^[\t ]local[ \t]' \\*.sh\n> > t/t5500-fetch-pack.sh:\tlocal diagport\n> > t/t7403-submodule-sync.sh:\tlocal root\n> >\n> > The grep gives many other hits, but those in completion are OK; it\n> > is designed to be specific to bash, and whose tests in t9902 is in\n> > the same boat.  A few more near the end of t/test-lib-functions are\n> > only for mingw where bash is the only supported shell at least for\n> > running tests.\n> \n> I think this should be sufficient (extra sets of eyeballs are\n> appreciated).  For 5500, diagport is not a variable used elsewhere\n> and can simply lose the \"local\".  7403 overrides the \"root\" variable\n> used in the test framework for no good reason (its use is not about\n> temporarily relocating where the test repositories are created), but\n> they can be made not to clobber the varible by moving them into the\n> subshells it already uses.\n\nI peeked at these cases last night when looking at other shell stuff,\nand I agree these are the only two spots which need attention (though I\nfind it interesting that they've been around for a while with nobody\ncomplaining. \"local\" isn't in POSIX, but it _is_ supported in a lot of\nshells. I wonder if we are being overly conservative in disallowing it,\nas the impetus here seems to be ancient versions of ksh, which is having\nother problems).\n\nAnyway, I am OK with dropping these ones for now. They are not helping\nanything, and they are the last two spots.\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 9b9bec4..dc305d6 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -556,7 +556,6 @@ check_prot_path () {\n>  }\n>  \n>  check_prot_host_port_path () {\n> -\tlocal diagport\n>  \tcase \"$2\" in\n>  \t\t*ssh*)\n>  \t\tpp=ssh\n\nThis one is particularly egregious because the function sets a bunch of\nother variables and does not bother to \"local\" them.\n\n> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\n> index 79bc135..5503ec0 100755\n> --- a/t/t7403-submodule-sync.sh\n> +++ b/t/t7403-submodule-sync.sh\n> @@ -62,13 +62,13 @@ test_expect_success 'change submodule' '\n>  '\n>  \n>  reset_submodule_urls () {\n> -\tlocal root\n> -\troot=$(pwd) &&\n>  \t(\n> +\t\troot=$(pwd) &&\n>  \t\tcd super-clone/submodule &&\n>  \t\tgit config remote.origin.url \"$root/submodule\"\n>  \t) &&\n>  \t(\n> +\t\troot=$(pwd) &&\n>  \t\tcd super-clone/submodule/sub-submodule &&\n>  \t\tgit config remote.origin.url \"$root/submodule\"\n\nHmm. Isn't $root always just going to be $TRASH_DIRECTORY here? There's\nonly one caller, which appears to pass an argument which is ignored (?).\n\nIt's probably worth doing the minimal thing now and leaving further\ncleanup for a separate patch, though. Cc-ing John Keeping, the author of\n091a6eb0feed, which added this code.\n\n-Peff\n"},{"id":"288068","messageId":"20160601183100.GN1355@john.keeping.me.uk","threadId":"42501","inReplyTo":"20160601163747.GA10721@sigill.intra.peff.net","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-06-01T18:31:00Z","receivedAt":"2016-06-01T18:31:00Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jun 01, 2016 at 12:37:47PM -0400, Jeff King wrote:\n> On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:\n> > diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\n> > index 79bc135..5503ec0 100755\n> > --- a/t/t7403-submodule-sync.sh\n> > +++ b/t/t7403-submodule-sync.sh\n> > @@ -62,13 +62,13 @@ test_expect_success 'change submodule' '\n> >  '\n> >  \n> >  reset_submodule_urls () {\n> > -\tlocal root\n> > -\troot=$(pwd) &&\n> >  \t(\n> > +\t\troot=$(pwd) &&\n> >  \t\tcd super-clone/submodule &&\n> >  \t\tgit config remote.origin.url \"$root/submodule\"\n> >  \t) &&\n> >  \t(\n> > +\t\troot=$(pwd) &&\n> >  \t\tcd super-clone/submodule/sub-submodule &&\n> >  \t\tgit config remote.origin.url \"$root/submodule\"\n> \n> Hmm. Isn't $root always just going to be $TRASH_DIRECTORY here? There's\n> only one caller, which appears to pass an argument which is ignored (?).\n> \n> It's probably worth doing the minimal thing now and leaving further\n> cleanup for a separate patch, though. Cc-ing John Keeping, the author of\n> 091a6eb0feed, which added this code.\n\nI can't shed any light on what this is trying to do; I had a look\nthrough the mailing list and this arrived in the final version of the\nseries without any comment.\n\nLooking at it now I can't see why this is a separate function (that is\ncalled with a parameter it never uses).  I wonder if my original\napproach was to call this via test_when_finished from the two tests\nfollowing this function definition, but that's pure speculation now.\n\nJunio's change is obviously correct as a minimal fix.\n\nI wonder if it's relevant that the \"local root\" line isn't &&-chained?\nIs it possible that on some shells we ignore an error but everything\nstill works?\n"},{"id":"288071","messageId":"20160601190759.GB12496@sigill.intra.peff.net","threadId":"42501","inReplyTo":"20160601183100.GN1355@john.keeping.me.uk","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T19:07:59Z","receivedAt":"2016-06-01T19:07:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:\n\n> > >  reset_submodule_urls () {\n> > > -\tlocal root\n> > > -\troot=$(pwd) &&\n> > >  \t(\n> > > +\t\troot=$(pwd) &&\n> > >  \t\tcd super-clone/submodule &&\n> > >  \t\tgit config remote.origin.url \"$root/submodule\"\n> > >  \t) &&\n> > >  \t(\n> > > +\t\troot=$(pwd) &&\n> > >  \t\tcd super-clone/submodule/sub-submodule &&\n> > >  \t\tgit config remote.origin.url \"$root/submodule\"\n> [...]\n> I wonder if it's relevant that the \"local root\" line isn't &&-chained?\n> Is it possible that on some shells we ignore an error but everything\n> still works?\n\nI don't think so. We're inside a function, so we wouldn't affect any\nouter &&-chaining in the function (and there isn't any in the caller\nanyway). I think it's a reasonable custom not to bother &&-chaining\n\"local\" lines, as they come at the top of a function and can't fail.\n\n-Peff\n"},{"id":"288072","messageId":"20160601191621.GO1355@john.keeping.me.uk","threadId":"42501","inReplyTo":"20160601190759.GB12496@sigill.intra.peff.net","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-06-01T19:16:21Z","receivedAt":"2016-06-01T19:16:21Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:\n> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:\n> \n> > > >  reset_submodule_urls () {\n> > > > -\tlocal root\n> > > > -\troot=$(pwd) &&\n> > > >  \t(\n> > > > +\t\troot=$(pwd) &&\n> > > >  \t\tcd super-clone/submodule &&\n> > > >  \t\tgit config remote.origin.url \"$root/submodule\"\n> > > >  \t) &&\n> > > >  \t(\n> > > > +\t\troot=$(pwd) &&\n> > > >  \t\tcd super-clone/submodule/sub-submodule &&\n> > > >  \t\tgit config remote.origin.url \"$root/submodule\"\n> > [...]\n> > I wonder if it's relevant that the \"local root\" line isn't &&-chained?\n> > Is it possible that on some shells we ignore an error but everything\n> > still works?\n> \n> I don't think so. We're inside a function, so we wouldn't affect any\n> outer &&-chaining in the function (and there isn't any in the caller\n> anyway). I think it's a reasonable custom not to bother &&-chaining\n> \"local\" lines, as they come at the top of a function and can't fail.\n\nCan't fail if the shell supports \"local\", but if we're in a shell that\ndoesn't support it, then the lack of \"&&\" may allow us to just carry on.\n"},{"id":"288074","messageId":"xmqqinxsy9q0.fsf@gitster.mtv.corp.google.com","threadId":"42501","inReplyTo":"20160601191621.GO1355@john.keeping.me.uk","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T19:45:11Z","receivedAt":"2016-06-01T19:45:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:\n>> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:\n>> \n>> > > >  reset_submodule_urls () {\n>> > > > -\tlocal root\n>> > > > -\troot=$(pwd) &&\n>> > > >  \t(\n>> > > > +\t\troot=$(pwd) &&\n>> > > >  \t\tcd super-clone/submodule &&\n>> > > >  \t\tgit config remote.origin.url \"$root/submodule\"\n>> > > >  \t) &&\n>> > > >  \t(\n>> > > > +\t\troot=$(pwd) &&\n>> > > >  \t\tcd super-clone/submodule/sub-submodule &&\n>> > > >  \t\tgit config remote.origin.url \"$root/submodule\"\n>> > [...]\n>> > I wonder if it's relevant that the \"local root\" line isn't &&-chained?\n>> > Is it possible that on some shells we ignore an error but everything\n>> > still works?\n>> \n>> I don't think so. We're inside a function, so we wouldn't affect any\n>> outer &&-chaining in the function (and there isn't any in the caller\n>> anyway). I think it's a reasonable custom not to bother &&-chaining\n>> \"local\" lines, as they come at the top of a function and can't fail.\n>\n> Can't fail if the shell supports \"local\", but if we're in a shell that\n> doesn't support it, then the lack of \"&&\" may allow us to just carry on.\n\nTrue, but if \"to just carry on\" were a correct behaviour, then\nwouldn't that mean that \"local\" was unnecessary, i.e. the variable\ndid not have to get localized because stomping on the global name\nwould not affect later reference to the same variable made by the\ncaller?\n\nIf the clobbering of a global variable breaks the behaviour of the\nscript, wouldn't we rather want to catch that fact?\n\nSo either way, I do not think \"local variable names\" that breaks\n&&-chain can be justified.  Either the variable must be localized\nfor the script to work correctly, in which case we want local with\n&&-chaining, or it does not have to, in which case we do not want to\nhave \"local\" that is not necessary, no?\n"},{"id":"288078","messageId":"20160601202852.GP1355@john.keeping.me.uk","threadId":"42501","inReplyTo":"xmqqinxsy9q0.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-06-01T20:28:53Z","receivedAt":"2016-06-01T20:28:53Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jun 01, 2016 at 12:45:11PM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:\n> >> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:\n> >> \n> >> > > >  reset_submodule_urls () {\n> >> > > > -\tlocal root\n> >> > > > -\troot=$(pwd) &&\n> >> > > >  \t(\n> >> > > > +\t\troot=$(pwd) &&\n> >> > > >  \t\tcd super-clone/submodule &&\n> >> > > >  \t\tgit config remote.origin.url \"$root/submodule\"\n> >> > > >  \t) &&\n> >> > > >  \t(\n> >> > > > +\t\troot=$(pwd) &&\n> >> > > >  \t\tcd super-clone/submodule/sub-submodule &&\n> >> > > >  \t\tgit config remote.origin.url \"$root/submodule\"\n> >> > [...]\n> >> > I wonder if it's relevant that the \"local root\" line isn't &&-chained?\n> >> > Is it possible that on some shells we ignore an error but everything\n> >> > still works?\n> >> \n> >> I don't think so. We're inside a function, so we wouldn't affect any\n> >> outer &&-chaining in the function (and there isn't any in the caller\n> >> anyway). I think it's a reasonable custom not to bother &&-chaining\n> >> \"local\" lines, as they come at the top of a function and can't fail.\n> >\n> > Can't fail if the shell supports \"local\", but if we're in a shell that\n> > doesn't support it, then the lack of \"&&\" may allow us to just carry on.\n> \n> True, but if \"to just carry on\" were a correct behaviour, then\n> wouldn't that mean that \"local\" was unnecessary, i.e. the variable\n> did not have to get localized because stomping on the global name\n> would not affect later reference to the same variable made by the\n> caller?\n> \n> If the clobbering of a global variable breaks the behaviour of the\n> script, wouldn't we rather want to catch that fact?\n> \n> So either way, I do not think \"local variable names\" that breaks\n> &&-chain can be justified.  Either the variable must be localized\n> for the script to work correctly, in which case we want local with\n> &&-chaining, or it does not have to, in which case we do not want to\n> have \"local\" that is not necessary, no?\n\nAbsolutely, my original point should have been prefixed with: I wonder\nif the reason we haven't had any problems reported is because ...\n\nAnd we've got lucky because the clobbering of global variables happens\nnot to matter in these particular cases.\n"},{"id":"288080","messageId":"20160601203243.GA15490@sigill.intra.peff.net","threadId":"42501","inReplyTo":"20160601202852.GP1355@john.keeping.me.uk","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T20:32:43Z","receivedAt":"2016-06-01T20:32:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 09:28:53PM +0100, John Keeping wrote:\n\n> > So either way, I do not think \"local variable names\" that breaks\n> > &&-chain can be justified.  Either the variable must be localized\n> > for the script to work correctly, in which case we want local with\n> > &&-chaining, or it does not have to, in which case we do not want to\n> > have \"local\" that is not necessary, no?\n> \n> Absolutely, my original point should have been prefixed with: I wonder\n> if the reason we haven't had any problems reported is because ...\n> \n> And we've got lucky because the clobbering of global variables happens\n> not to matter in these particular cases.\n\nAh, OK, what you were saying makes much more sense to me now, then.\n\nEven on a shell like ksh93 that does not grok local at all, there is a\ngood chance that nobody ever looked at the \"-v\" output for the test,\nwhich would not have been failing, to see that it was complaining.\n\nSo I agree we can't really take \"no problems reported\" on these existing\ncases as any kind of data point.\n\n-Peff\n"},{"id":"288081","messageId":"xmqq8tyoy6se.fsf@gitster.mtv.corp.google.com","threadId":"42501","inReplyTo":"20160601202852.GP1355@john.keeping.me.uk","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T20:48:33Z","receivedAt":"2016-06-01T20:48:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Absolutely, my original point should have been prefixed with: I wonder\n> if the reason we haven't had any problems reported is because ...\n>\n> And we've got lucky because the clobbering of global variables happens\n> not to matter in these particular cases.\n\nAh, I did misunderstand why you were making that statement, and now\nI fully agree with your conclusion (which is what Jeff spelled out\nin the latest message) that the fact that we saw no breakage report\nis not a datapoint that everybody's shell supports \"local\" at all.\n\nThanks for clarification.\n"},{"id":"288082","messageId":"xmqq37owy6fr.fsf@gitster.mtv.corp.google.com","threadId":"42501","inReplyTo":"xmqq8tyoy6se.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T20:56:08Z","receivedAt":"2016-06-01T20:56:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> John Keeping <john@keeping.me.uk> writes:\n>\n>> Absolutely, my original point should have been prefixed with: I wonder\n>> if the reason we haven't had any problems reported is because ...\n>>\n>> And we've got lucky because the clobbering of global variables happens\n>> not to matter in these particular cases.\n>\n> Ah, I did misunderstand why you were making that statement, and now\n> I fully agree with your conclusion (which is what Jeff spelled out\n> in the latest message) that the fact that we saw no breakage report\n> is not a datapoint that everybody's shell supports \"local\" at all.\n>\n> Thanks for clarification.\n\nSo here is the final version with a log message.\n\n-- >8 --\nSubject: [PATCH] t5500 & t7403: lose bash-ism \"local\"\n\nIn t5500::check_prot_host_port_path(), diagport is not a variable\nused elsewhere and the function is not recursively called so this\ncan simply lose the \"local\", which may not be supported by shell\n(besides, the function liberally clobbers other variables without\nmaking them \"local\").\n\nt7403::reset_submodule_urls() overrides the \"root\" variable used\nin the test framework for no good reason; its use is not about\ntemporarily relocating where the test repositories are created.\nThis assignment can be made not to clobber the varible by moving\nthem into the subshells it already uses.  Its value is always\n$TRASH_DIRECTORY, so we could use it instead there, and this\nfunction that is called only once and its two subshells may not be\nnecessary (instead, the caller can use \"git -C $there config\" and\nset a value that is derived from $TRASH_DIRECTORY), but this is a\nminimum fix that is needed to lose \"local\".\n\nHelped-by: John Keeping <john@keeping.me.uk>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t5500-fetch-pack.sh     | 1 -\n t/t7403-submodule-sync.sh | 4 ++--\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 9b9bec4..dc305d6 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -556,7 +556,6 @@ check_prot_path () {\n }\n \n check_prot_host_port_path () {\n-\tlocal diagport\n \tcase \"$2\" in\n \t\t*ssh*)\n \t\tpp=ssh\ndiff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\nindex 79bc135..5503ec0 100755\n--- a/t/t7403-submodule-sync.sh\n+++ b/t/t7403-submodule-sync.sh\n@@ -62,13 +62,13 @@ test_expect_success 'change submodule' '\n '\n \n reset_submodule_urls () {\n-\tlocal root\n-\troot=$(pwd) &&\n \t(\n+\t\troot=$(pwd) &&\n \t\tcd super-clone/submodule &&\n \t\tgit config remote.origin.url \"$root/submodule\"\n \t) &&\n \t(\n+\t\troot=$(pwd) &&\n \t\tcd super-clone/submodule/sub-submodule &&\n \t\tgit config remote.origin.url \"$root/submodule\"\n \t)\n-- \n2.9.0-rc1-223-gb1e1500\n"},{"id":"288083","messageId":"CAGZ79kaUHm==ZjRwD3MTnCXKAWUHszb4PPF=S4Y-TD2e4eBsTw@mail.gmail.com","threadId":"42501","inReplyTo":"xmqq8tyoy6se.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-01T20:59:06Z","receivedAt":"2016-06-01T20:59:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 1, 2016 at 1:48 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> John Keeping <john@keeping.me.uk> writes:\n>\n>> Absolutely, my original point should have been prefixed with: I wonder\n>> if the reason we haven't had any problems reported is because ...\n>>\n>> And we've got lucky because the clobbering of global variables happens\n>> not to matter in these particular cases.\n>\n> Ah, I did misunderstand why you were making that statement, and now\n> I fully agree with your conclusion (which is what Jeff spelled out\n> in the latest message) that the fact that we saw no breakage report\n> is not a datapoint that everybody's shell supports \"local\" at all.\n>\n> Thanks for clarification.\n\nI think both the use of submodules and the use of shells not supporting 'local'\nis a minority in our current user base, so I am not surprised that nobody\ncomplained about that, as the overlap between submodule users and\nnon-local shell users may even be zero.\n\nThe patch just sent, looks good to me for the minimal fix in the tests.\n\nThanks,\nStefan\n\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"288084","messageId":"CAPig+cSERmvSRhLYfNpTUu-SfFwhAzxfUh4XTZKUbARAu5rpdg@mail.gmail.com","threadId":"42501","inReplyTo":"xmqq37owy6fr.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-01T20:59:34Z","receivedAt":"2016-06-01T20:59:34Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 1, 2016 at 4:56 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Subject: [PATCH] t5500 & t7403: lose bash-ism \"local\"\n>\n> In t5500::check_prot_host_port_path(), diagport is not a variable\n> used elsewhere and the function is not recursively called so this\n> can simply lose the \"local\", which may not be supported by shell\n> (besides, the function liberally clobbers other variables without\n> making them \"local\").\n>\n> t7403::reset_submodule_urls() overrides the \"root\" variable used\n> in the test framework for no good reason; its use is not about\n> temporarily relocating where the test repositories are created.\n> This assignment can be made not to clobber the varible by moving\n\ns/varible/variable/\n\n> them into the subshells it already uses.  Its value is always\n> $TRASH_DIRECTORY, so we could use it instead there, and this\n> function that is called only once and its two subshells may not be\n> necessary (instead, the caller can use \"git -C $there config\" and\n> set a value that is derived from $TRASH_DIRECTORY), but this is a\n> minimum fix that is needed to lose \"local\".\n>\n> Helped-by: John Keeping <john@keeping.me.uk>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"288087","messageId":"20160601210832.GB18118@sigill.intra.peff.net","threadId":"42501","inReplyTo":"xmqq37owy6fr.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] git-submodule has bash-ism?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T21:08:32Z","receivedAt":"2016-06-01T21:08:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 01:56:08PM -0700, Junio C Hamano wrote:\n\n> So here is the final version with a log message.\n> \n> -- >8 --\n> Subject: [PATCH] t5500 & t7403: lose bash-ism \"local\"\n> [...]\n\nLooks good. Thanks.\n\n-Peff\n"}]}