Re: [BUG] git-submodule has bash-ism?
On Tue, May 31, 2016 at 4:08 PM, Junio C Hamano 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
Junio C Hamano 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
Junio C Hamano 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" ) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote: > Junio C Hamano 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
John Keeping 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
On Wed, Jun 01, 2016 at 12:45:11PM -0700, Junio C Hamano wrote: > John Keeping 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
John Keeping 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
Junio C Hamano writes: > John Keeping 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 Helped-by: Jeff King Signed-off-by: Junio C Hamano --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
On Wed, Jun 1, 2016 at 1:48 PM, Junio C Hamano wrote: > John Keeping 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
On Wed, Jun 1, 2016 at 4:56 PM, Junio C Hamano 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 > Helped-by: Jeff King > Signed-off-by: Junio C Hamano -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git-submodule has bash-ism?
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html