On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html