[BUG] git-submodule has bash-ism?

[BUG] git-submodule has bash-ism?

[Junio C Hamano]

relative_path ()
{
	local target curdir result
	target=$1
	curdir=${2-$wt_prefix}

I am hoping that Stefan's "gradually rewrite things in C" will make
it unnecessary to worry about this one.  "git submodule" would not
work correctly on posixly correct shells in the meantime.

Re: [BUG] git-submodule has bash-ism?

[Stefan Beller]

On Tue, May 31, 2016 at 4:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> relative_path ()
> {
>         local target curdir result
>         target=$1
>         curdir=${2-$wt_prefix}
>
> I am hoping that Stefan's "gradually rewrite things in C" will make
> it unnecessary to worry about this one.  "git submodule" would not
> work correctly on posixly correct shells in the meantime.

noted.

Maybe as a smaller step we can expose the relative_path from the
submodule--helper
instead of rewriting all actual users first.

Thanks for pointing out,
Stefan


>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] submodule: remove bashism from shell script

[Stefan Beller]

Junio pointed out `relative_path` was using bashisms via the
local variables. As the longer term goal is to rewrite most of the
submodule code in C, do it now.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

* developed on top of origin/master + "[PATCH] submodule--helper: offer a
  consistent API" which I just sent.
* This fix looks amazingly simple (it even worked on the first try),
  so I temporarily did a s|printf("%s", relative_path(argv[1], argv[2], &sb));|printf("bogus");|
  to ensure we actually catch failures in the display path. And we do!
  
Thanks,
Stefan

 builtin/submodule--helper.c | 12 +++++++++++
 git-submodule.sh            | 51 +++++++--------------------------------------
 2 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f0b2c4f..926d205 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -831,6 +831,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int resolve_relative_path(int argc, const char **argv, const char *prefix)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (argc != 3)
+		die("submodule--helper relative_path takes exactly 2 arguments, got %d", argc);
+
+	printf("%s", relative_path(argv[1], argv[2], &sb));
+	strbuf_release(&sb);
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -841,6 +852,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name},
 	{"clone", module_clone},
 	{"update-clone", update_clone},
+	{"relative-path", resolve_relative_path},
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
 	{"init", module_init}
diff --git a/git-submodule.sh b/git-submodule.sh
index fadbe5d..7fe8a51 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -46,41 +46,6 @@ prefix=
 custom_name=
 depth=
 
-# Resolve a path to be relative to another path.  This is intended for
-# converting submodule paths when git-submodule is run in a subdirectory
-# and only handles paths where the directory separator is '/'.
-#
-# The output is the first argument as a path relative to the second argument,
-# which defaults to $wt_prefix if it is omitted.
-relative_path ()
-{
-	local target curdir result
-	target=$1
-	curdir=${2-$wt_prefix}
-	curdir=${curdir%/}
-	result=
-
-	while test -n "$curdir"
-	do
-		case "$target" in
-		"$curdir/"*)
-			target=${target#"$curdir"/}
-			break
-			;;
-		esac
-
-		result="${result}../"
-		if test "$curdir" = "${curdir%/*}"
-		then
-			curdir=
-		else
-			curdir="${curdir%/*}"
-		fi
-	done
-
-	echo "$result$target"
-}
-
 die_if_unmatched ()
 {
 	if test "$1" = "#unmatched"
@@ -354,14 +319,14 @@ cmd_foreach()
 		die_if_unmatched "$mode"
 		if test -e "$sm_path"/.git
 		then
-			displaypath=$(relative_path "$prefix$sm_path")
+			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 			say "$(eval_gettext "Entering '\$displaypath'")"
 			name=$(git submodule--helper name "$sm_path")
 			(
 				prefix="$prefix$sm_path/"
 				sanitize_submodule_env
 				cd "$sm_path" &&
-				sm_path=$(relative_path "$sm_path") &&
+				sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
 				if test $# -eq 1
@@ -465,7 +430,7 @@ cmd_deinit()
 		die_if_unmatched "$mode"
 		name=$(git submodule--helper name "$sm_path") || exit
 
-		displaypath=$(relative_path "$sm_path")
+		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
 
 		# Remove the submodule work tree (unless the user already did it)
 		if test -d "$sm_path"
@@ -629,7 +594,7 @@ cmd_update()
 			fi
 		fi
 
-		displaypath=$(relative_path "$prefix$sm_path")
+		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 
 		if test $just_cloned -eq 1
 		then
@@ -723,7 +688,7 @@ cmd_update()
 		if test -n "$recursive"
 		then
 			(
-				prefix=$(relative_path "$prefix$sm_path/")
+				prefix=$(git submodule--helper relative-path "$prefix$sm_path/" "$wt_prefix")
 				wt_prefix=
 				sanitize_submodule_env
 				cd "$sm_path" &&
@@ -907,7 +872,7 @@ cmd_summary() {
 		! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&
 		missing_dst=t
 
-		display_name=$(relative_path "$name")
+		display_name=$(git submodule--helper relative-path "$name" "$wt_prefix")
 
 		total_commits=
 		case "$missing_src,$missing_dst" in
@@ -1028,7 +993,7 @@ cmd_status()
 		die_if_unmatched "$mode"
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
-		displaypath=$(relative_path "$prefix$sm_path")
+		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 		if test "$stage" = U
 		then
 			say "U$sha1 $displaypath"
@@ -1131,7 +1096,7 @@ cmd_sync()
 
 		if git config "submodule.$name.url" >/dev/null 2>/dev/null
 		then
-			displaypath=$(relative_path "$prefix$sm_path")
+			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 			say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"
 			git config submodule."$name".url "$super_config_url"
 
-- 
2.9.0.rc1.2.ge49d2c8.dirty

Re: [BUG] git-submodule has bash-ism?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> relative_path ()
> {
> 	local target curdir result
> 	target=$1
> 	curdir=${2-$wt_prefix}
>
> I am hoping that Stefan's "gradually rewrite things in C" will make
> it unnecessary to worry about this one.  "git submodule" would not
> work correctly on posixly correct shells in the meantime.

These are two other offenders.

$ git grep '^[	 ]local[ 	]' \*.sh
t/t5500-fetch-pack.sh:	local diagport
t/t7403-submodule-sync.sh:	local root

The grep gives many other hits, but those in completion are OK; it
is designed to be specific to bash, and whose tests in t9902 is in
the same boat.  A few more near the end of t/test-lib-functions are
only for mingw where bash is the only supported shell at least for
running tests.

Re: [BUG] git-submodule has bash-ism?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> These are two other offenders.
>
> $ git grep '^[	 ]local[ 	]' \*.sh
> t/t5500-fetch-pack.sh:	local diagport
> t/t7403-submodule-sync.sh:	local root
>
> The grep gives many other hits, but those in completion are OK; it
> is designed to be specific to bash, and whose tests in t9902 is in
> the same boat.  A few more near the end of t/test-lib-functions are
> only for mingw where bash is the only supported shell at least for
> running tests.

I think this should be sufficient (extra sets of eyeballs are
appreciated).  For 5500, diagport is not a variable used elsewhere
and can simply lose the "local".  7403 overrides the "root" variable
used in the test framework for no good reason (its use is not about
temporarily relocating where the test repositories are created), but
they can be made not to clobber the varible by moving them into the
subshells it already uses.

 t/t5500-fetch-pack.sh     | 1 -
 t/t7403-submodule-sync.sh | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 9b9bec4..dc305d6 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -556,7 +556,6 @@ check_prot_path () {
 }
 
 check_prot_host_port_path () {
-	local diagport
 	case "$2" in
 		*ssh*)
 		pp=ssh
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 79bc135..5503ec0 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -62,13 +62,13 @@ test_expect_success 'change submodule' '
 '
 
 reset_submodule_urls () {
-	local root
-	root=$(pwd) &&
 	(
+		root=$(pwd) &&
 		cd super-clone/submodule &&
 		git config remote.origin.url "$root/submodule"
 	) &&
 	(
+		root=$(pwd) &&
 		cd super-clone/submodule/sub-submodule &&
 		git config remote.origin.url "$root/submodule"
 	)

Re: [BUG] git-submodule has bash-ism?

[Jeff King]

On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > These are two other offenders.
> >
> > $ git grep '^[	 ]local[ 	]' \*.sh
> > t/t5500-fetch-pack.sh:	local diagport
> > t/t7403-submodule-sync.sh:	local root
> >
> > The grep gives many other hits, but those in completion are OK; it
> > is designed to be specific to bash, and whose tests in t9902 is in
> > the same boat.  A few more near the end of t/test-lib-functions are
> > only for mingw where bash is the only supported shell at least for
> > running tests.
> 
> I think this should be sufficient (extra sets of eyeballs are
> appreciated).  For 5500, diagport is not a variable used elsewhere
> and can simply lose the "local".  7403 overrides the "root" variable
> used in the test framework for no good reason (its use is not about
> temporarily relocating where the test repositories are created), but
> they can be made not to clobber the varible by moving them into the
> subshells it already uses.

I peeked at these cases last night when looking at other shell stuff,
and I agree these are the only two spots which need attention (though I
find it interesting that they've been around for a while with nobody
complaining. "local" isn't in POSIX, but it _is_ supported in a lot of
shells. I wonder if we are being overly conservative in disallowing it,
as the impetus here seems to be ancient versions of ksh, which is having
other problems).

Anyway, I am OK with dropping these ones for now. They are not helping
anything, and they are the last two spots.

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 9b9bec4..dc305d6 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -556,7 +556,6 @@ check_prot_path () {
>  }
>  
>  check_prot_host_port_path () {
> -	local diagport
>  	case "$2" in
>  		*ssh*)
>  		pp=ssh

This one is particularly egregious because the function sets a bunch of
other variables and does not bother to "local" them.

> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> index 79bc135..5503ec0 100755
> --- a/t/t7403-submodule-sync.sh
> +++ b/t/t7403-submodule-sync.sh
> @@ -62,13 +62,13 @@ test_expect_success 'change submodule' '
>  '
>  
>  reset_submodule_urls () {
> -	local root
> -	root=$(pwd) &&
>  	(
> +		root=$(pwd) &&
>  		cd super-clone/submodule &&
>  		git config remote.origin.url "$root/submodule"
>  	) &&
>  	(
> +		root=$(pwd) &&
>  		cd super-clone/submodule/sub-submodule &&
>  		git config remote.origin.url "$root/submodule"

Hmm. Isn't $root always just going to be $TRASH_DIRECTORY here? There's
only one caller, which appears to pass an argument which is ignored (?).

It's probably worth doing the minimal thing now and leaving further
cleanup for a separate patch, though. Cc-ing John Keeping, the author of
091a6eb0feed, which added this code.

-Peff

Re: [BUG] git-submodule has bash-ism?

[John Keeping]

On Wed, Jun 01, 2016 at 12:37:47PM -0400, Jeff King wrote:
> On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:
> > diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> > index 79bc135..5503ec0 100755
> > --- a/t/t7403-submodule-sync.sh
> > +++ b/t/t7403-submodule-sync.sh
> > @@ -62,13 +62,13 @@ test_expect_success 'change submodule' '
> >  '
> >  
> >  reset_submodule_urls () {
> > -	local root
> > -	root=$(pwd) &&
> >  	(
> > +		root=$(pwd) &&
> >  		cd super-clone/submodule &&
> >  		git config remote.origin.url "$root/submodule"
> >  	) &&
> >  	(
> > +		root=$(pwd) &&
> >  		cd super-clone/submodule/sub-submodule &&
> >  		git config remote.origin.url "$root/submodule"
> 
> Hmm. Isn't $root always just going to be $TRASH_DIRECTORY here? There's
> only one caller, which appears to pass an argument which is ignored (?).
> 
> It's probably worth doing the minimal thing now and leaving further
> cleanup for a separate patch, though. Cc-ing John Keeping, the author of
> 091a6eb0feed, which added this code.

I can't shed any light on what this is trying to do; I had a look
through the mailing list and this arrived in the final version of the
series without any comment.

Looking at it now I can't see why this is a separate function (that is
called with a parameter it never uses).  I wonder if my original
approach was to call this via test_when_finished from the two tests
following this function definition, but that's pure speculation now.

Junio's change is obviously correct as a minimal fix.

I wonder if it's relevant that the "local root" line isn't &&-chained?
Is it possible that on some shells we ignore an error but everything
still works?

Re: [BUG] git-submodule has bash-ism?

[Jeff King]

On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:

> > >  reset_submodule_urls () {
> > > -	local root
> > > -	root=$(pwd) &&
> > >  	(
> > > +		root=$(pwd) &&
> > >  		cd super-clone/submodule &&
> > >  		git config remote.origin.url "$root/submodule"
> > >  	) &&
> > >  	(
> > > +		root=$(pwd) &&
> > >  		cd super-clone/submodule/sub-submodule &&
> > >  		git config remote.origin.url "$root/submodule"
> [...]
> I wonder if it's relevant that the "local root" line isn't &&-chained?
> Is it possible that on some shells we ignore an error but everything
> still works?

I don't think so. We're inside a function, so we wouldn't affect any
outer &&-chaining in the function (and there isn't any in the caller
anyway). I think it's a reasonable custom not to bother &&-chaining
"local" lines, as they come at the top of a function and can't fail.

-Peff

Re: [BUG] git-submodule has bash-ism?

[John Keeping]

On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:
> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:
> 
> > > >  reset_submodule_urls () {
> > > > -	local root
> > > > -	root=$(pwd) &&
> > > >  	(
> > > > +		root=$(pwd) &&
> > > >  		cd super-clone/submodule &&
> > > >  		git config remote.origin.url "$root/submodule"
> > > >  	) &&
> > > >  	(
> > > > +		root=$(pwd) &&
> > > >  		cd super-clone/submodule/sub-submodule &&
> > > >  		git config remote.origin.url "$root/submodule"
> > [...]
> > I wonder if it's relevant that the "local root" line isn't &&-chained?
> > Is it possible that on some shells we ignore an error but everything
> > still works?
> 
> I don't think so. We're inside a function, so we wouldn't affect any
> outer &&-chaining in the function (and there isn't any in the caller
> anyway). I think it's a reasonable custom not to bother &&-chaining
> "local" lines, as they come at the top of a function and can't fail.

Can't fail if the shell supports "local", but if we're in a shell that
doesn't support it, then the lack of "&&" may allow us to just carry on.

Re: [BUG] git-submodule has bash-ism?

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:
>> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:
>> 
>> > > >  reset_submodule_urls () {
>> > > > -	local root
>> > > > -	root=$(pwd) &&
>> > > >  	(
>> > > > +		root=$(pwd) &&
>> > > >  		cd super-clone/submodule &&
>> > > >  		git config remote.origin.url "$root/submodule"
>> > > >  	) &&
>> > > >  	(
>> > > > +		root=$(pwd) &&
>> > > >  		cd super-clone/submodule/sub-submodule &&
>> > > >  		git config remote.origin.url "$root/submodule"
>> > [...]
>> > I wonder if it's relevant that the "local root" line isn't &&-chained?
>> > Is it possible that on some shells we ignore an error but everything
>> > still works?
>> 
>> I don't think so. We're inside a function, so we wouldn't affect any
>> outer &&-chaining in the function (and there isn't any in the caller
>> anyway). I think it's a reasonable custom not to bother &&-chaining
>> "local" lines, as they come at the top of a function and can't fail.
>
> Can't fail if the shell supports "local", but if we're in a shell that
> doesn't support it, then the lack of "&&" may allow us to just carry on.

True, but if "to just carry on" were a correct behaviour, then
wouldn't that mean that "local" was unnecessary, i.e. the variable
did not have to get localized because stomping on the global name
would not affect later reference to the same variable made by the
caller?

If the clobbering of a global variable breaks the behaviour of the
script, wouldn't we rather want to catch that fact?

So either way, I do not think "local variable names" that breaks
&&-chain can be justified.  Either the variable must be localized
for the script to work correctly, in which case we want local with
&&-chaining, or it does not have to, in which case we do not want to
have "local" that is not necessary, no?

Re: [BUG] git-submodule has bash-ism?

[John Keeping]

On Wed, Jun 01, 2016 at 12:45:11PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:
> >> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:
> >> 
> >> > > >  reset_submodule_urls () {
> >> > > > -	local root
> >> > > > -	root=$(pwd) &&
> >> > > >  	(
> >> > > > +		root=$(pwd) &&
> >> > > >  		cd super-clone/submodule &&
> >> > > >  		git config remote.origin.url "$root/submodule"
> >> > > >  	) &&
> >> > > >  	(
> >> > > > +		root=$(pwd) &&
> >> > > >  		cd super-clone/submodule/sub-submodule &&
> >> > > >  		git config remote.origin.url "$root/submodule"
> >> > [...]
> >> > I wonder if it's relevant that the "local root" line isn't &&-chained?
> >> > Is it possible that on some shells we ignore an error but everything
> >> > still works?
> >> 
> >> I don't think so. We're inside a function, so we wouldn't affect any
> >> outer &&-chaining in the function (and there isn't any in the caller
> >> anyway). I think it's a reasonable custom not to bother &&-chaining
> >> "local" lines, as they come at the top of a function and can't fail.
> >
> > Can't fail if the shell supports "local", but if we're in a shell that
> > doesn't support it, then the lack of "&&" may allow us to just carry on.
> 
> True, but if "to just carry on" were a correct behaviour, then
> wouldn't that mean that "local" was unnecessary, i.e. the variable
> did not have to get localized because stomping on the global name
> would not affect later reference to the same variable made by the
> caller?
> 
> If the clobbering of a global variable breaks the behaviour of the
> script, wouldn't we rather want to catch that fact?
> 
> So either way, I do not think "local variable names" that breaks
> &&-chain can be justified.  Either the variable must be localized
> for the script to work correctly, in which case we want local with
> &&-chaining, or it does not have to, in which case we do not want to
> have "local" that is not necessary, no?

Absolutely, my original point should have been prefixed with: I wonder
if the reason we haven't had any problems reported is because ...

And we've got lucky because the clobbering of global variables happens
not to matter in these particular cases.

Re: [BUG] git-submodule has bash-ism?

[Jeff King]

On Wed, Jun 01, 2016 at 09:28:53PM +0100, John Keeping wrote:

> > So either way, I do not think "local variable names" that breaks
> > &&-chain can be justified.  Either the variable must be localized
> > for the script to work correctly, in which case we want local with
> > &&-chaining, or it does not have to, in which case we do not want to
> > have "local" that is not necessary, no?
> 
> Absolutely, my original point should have been prefixed with: I wonder
> if the reason we haven't had any problems reported is because ...
> 
> And we've got lucky because the clobbering of global variables happens
> not to matter in these particular cases.

Ah, OK, what you were saying makes much more sense to me now, then.

Even on a shell like ksh93 that does not grok local at all, there is a
good chance that nobody ever looked at the "-v" output for the test,
which would not have been failing, to see that it was complaining.

So I agree we can't really take "no problems reported" on these existing
cases as any kind of data point.

-Peff

Re: [BUG] git-submodule has bash-ism?

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Absolutely, my original point should have been prefixed with: I wonder
> if the reason we haven't had any problems reported is because ...
>
> And we've got lucky because the clobbering of global variables happens
> not to matter in these particular cases.

Ah, I did misunderstand why you were making that statement, and now
I fully agree with your conclusion (which is what Jeff spelled out
in the latest message) that the fact that we saw no breakage report
is not a datapoint that everybody's shell supports "local" at all.

Thanks for clarification.

Re: [BUG] git-submodule has bash-ism?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> John Keeping <john@keeping.me.uk> writes:
>
>> Absolutely, my original point should have been prefixed with: I wonder
>> if the reason we haven't had any problems reported is because ...
>>
>> And we've got lucky because the clobbering of global variables happens
>> not to matter in these particular cases.
>
> Ah, I did misunderstand why you were making that statement, and now
> I fully agree with your conclusion (which is what Jeff spelled out
> in the latest message) that the fact that we saw no breakage report
> is not a datapoint that everybody's shell supports "local" at all.
>
> Thanks for clarification.

So here is the final version with a log message.

-- >8 --
Subject: [PATCH] t5500 & t7403: lose bash-ism "local"

In t5500::check_prot_host_port_path(), diagport is not a variable
used elsewhere and the function is not recursively called so this
can simply lose the "local", which may not be supported by shell
(besides, the function liberally clobbers other variables without
making them "local").

t7403::reset_submodule_urls() overrides the "root" variable used
in the test framework for no good reason; its use is not about
temporarily relocating where the test repositories are created.
This assignment can be made not to clobber the varible by moving
them into the subshells it already uses.  Its value is always
$TRASH_DIRECTORY, so we could use it instead there, and this
function that is called only once and its two subshells may not be
necessary (instead, the caller can use "git -C $there config" and
set a value that is derived from $TRASH_DIRECTORY), but this is a
minimum fix that is needed to lose "local".

Helped-by: John Keeping <john@keeping.me.uk>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5500-fetch-pack.sh     | 1 -
 t/t7403-submodule-sync.sh | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 9b9bec4..dc305d6 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -556,7 +556,6 @@ check_prot_path () {
 }
 
 check_prot_host_port_path () {
-	local diagport
 	case "$2" in
 		*ssh*)
 		pp=ssh
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 79bc135..5503ec0 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -62,13 +62,13 @@ test_expect_success 'change submodule' '
 '
 
 reset_submodule_urls () {
-	local root
-	root=$(pwd) &&
 	(
+		root=$(pwd) &&
 		cd super-clone/submodule &&
 		git config remote.origin.url "$root/submodule"
 	) &&
 	(
+		root=$(pwd) &&
 		cd super-clone/submodule/sub-submodule &&
 		git config remote.origin.url "$root/submodule"
 	)
-- 
2.9.0-rc1-223-gb1e1500

Re: [BUG] git-submodule has bash-ism?

[Eric Sunshine]

On Wed, Jun 1, 2016 at 4:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] t5500 & t7403: lose bash-ism "local"
>
> In t5500::check_prot_host_port_path(), diagport is not a variable
> used elsewhere and the function is not recursively called so this
> can simply lose the "local", which may not be supported by shell
> (besides, the function liberally clobbers other variables without
> making them "local").
>
> t7403::reset_submodule_urls() overrides the "root" variable used
> in the test framework for no good reason; its use is not about
> temporarily relocating where the test repositories are created.
> This assignment can be made not to clobber the varible by moving

s/varible/variable/

> them into the subshells it already uses.  Its value is always
> $TRASH_DIRECTORY, so we could use it instead there, and this
> function that is called only once and its two subshells may not be
> necessary (instead, the caller can use "git -C $there config" and
> set a value that is derived from $TRASH_DIRECTORY), but this is a
> minimum fix that is needed to lose "local".
>
> Helped-by: John Keeping <john@keeping.me.uk>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [BUG] git-submodule has bash-ism?

[Jeff King]

On Wed, Jun 01, 2016 at 01:56:08PM -0700, Junio C Hamano wrote:

> So here is the final version with a log message.
> 
> -- >8 --
> Subject: [PATCH] t5500 & t7403: lose bash-ism "local"
> [...]

Looks good. Thanks.

-Peff

Re: [BUG] git-submodule has bash-ism?

[Stefan Beller]

On Wed, Jun 1, 2016 at 1:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> Absolutely, my original point should have been prefixed with: I wonder
>> if the reason we haven't had any problems reported is because ...
>>
>> And we've got lucky because the clobbering of global variables happens
>> not to matter in these particular cases.
>
> Ah, I did misunderstand why you were making that statement, and now
> I fully agree with your conclusion (which is what Jeff spelled out
> in the latest message) that the fact that we saw no breakage report
> is not a datapoint that everybody's shell supports "local" at all.
>
> Thanks for clarification.

I think both the use of submodules and the use of shells not supporting 'local'
is a minority in our current user base, so I am not surprised that nobody
complained about that, as the overlap between submodule users and
non-local shell users may even be zero.

The patch just sent, looks good to me for the minimal fix in the tests.

Thanks,
Stefan


> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html