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
