threads / bug / 42501

[BUG] git-submodule has bash-ism?

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

## tl;dr

17 messages between May 31, 2016 and Jun 1, 2016.

replies: 16people: 5as markdown or json

Junio C Hamano· May 31, 2016, 23:08 UTC · lore
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.

Stefan Beller· May 31, 2016, 23:16 UTC · re: Junio C Hamano · lore

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

On Tue, May 31, 2016 at 4:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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

Show 5 quoted lines
>
> --
> 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
Stefan Beller· Jun 1, 2016, 00:27 UTC · re: Junio C Hamano · lore

[PATCH] submodule: remove bashism from shell script

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
Junio C Hamano· Jun 1, 2016, 16:13 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> 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.

Junio C Hamano· Jun 1, 2016, 16:19 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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"
 	)
Jeff King· Jun 1, 2016, 16:37 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> 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.

Show 12 quoted lines
> 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.

Show 19 quoted lines
> 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
John Keeping· Jun 1, 2016, 18:31 UTC · re: Jeff King · lore

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

On Wed, Jun 01, 2016 at 12:37:47PM -0400, Jeff King wrote:
Show 27 quoted lines
> 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?

Jeff King· Jun 1, 2016, 19:07 UTC · re: John Keeping · lore

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

On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:
Show 16 quoted lines
> > >  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
John Keeping· Jun 1, 2016, 19:16 UTC · re: Jeff King · lore

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

On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:
Show 23 quoted lines
> 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.

Junio C Hamano· Jun 1, 2016, 19:45 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 27 quoted lines
> 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?

John Keeping· Jun 1, 2016, 20:28 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 01, 2016 at 12:45:11PM -0700, Junio C Hamano wrote:
Show 44 quoted lines
> 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.

Jeff King· Jun 1, 2016, 20:32 UTC · re: John Keeping · lore

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

On Wed, Jun 01, 2016 at 09:28:53PM +0100, John Keeping wrote:
Show 11 quoted lines
> > 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
Junio C Hamano· Jun 1, 2016, 20:48 UTC · re: John Keeping · lore

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

John Keeping <john@keeping.me.uk> writes:
Show 5 quoted lines
> 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.
Junio C Hamano· Jun 1, 2016, 20:56 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> 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
Eric Sunshine· Jun 1, 2016, 20:59 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 1, 2016 at 4:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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/
Show 10 quoted lines
> 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>
Jeff King· Jun 1, 2016, 21:08 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 01, 2016 at 01:56:08PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> So here is the final version with a log message.
> 
> -- >8 --
> Subject: [PATCH] t5500 & t7403: lose bash-ism "local"
> [...]
Looks good. Thanks.
-Peff
Stefan Beller· Jun 1, 2016, 20:59 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 1, 2016 at 1:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> 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

← back to recent threads