On Thu, Apr 28, 2016 at 07:25:11AM -0400, Jeff King wrote:
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 2a84d7e..b02f5b9 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -199,7 +199,7 @@ sanitize_submodule_env()
> > {
> > sanitized_config=$(git submodule--helper sanitize-config)
> > clear_local_git_env
> > - GIT_CONFIG_PARAMETERS=$sanitized_config
> > + export GIT_CONFIG_PARAMETERS=$sanitized_config
> > }
>
> If you already have $GIT_CONFIG_PARAMETERS exported when we enter the
> function, then we should not need to re-export it when changing the
> value in the final line (the export bit is retained by the shell). But
> if you don't have it set already, then $sanitized_config must by
> definition be empty.
>
> So it should do the right thing without the export.
>
> At the same time, clear_local_git_env() will call "unset" on
> GIT_CONFIG_PARAMETERS. Which would clear the export bit, meaning the
> final line doesn't ever have any impact on sub-programs, and the whole
> thing is totally broken. But then, why does the test in t5550 pass?
>
> Confused...
Ah. t5550 passes because it does not exercise this code path at all. We
try a recursive clone, which calls "git submodule update --init", which
does not seem to clear the config at all. So it works even without
14111fc49.
I tried to improve the test by adding git-fetch (note that I also fixed
a bug where we use $HTTP_URL instead of $HTTPD_URL, and added some
whitespace to make the result more readable):
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 69ef388..6ec3ba3 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -103,12 +103,23 @@ test_expect_success 'cmdline credential config passes into submodules' '
git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
git commit -m "add submodule"
) &&
+
set_askpass wrong pass@host &&
test_must_fail git clone --recursive super super-clone &&
rm -rf super-clone &&
+
set_askpass wrong pass@host &&
- git -c "credential.$HTTP_URL.username=user@host" \
+ git -c "credential.$HTTPD_URL.username=user@host" \
clone --recursive super super-clone &&
+ expect_askpass pass user@host &&
+
+ set_askpass wrong pass@host &&
+ test_must_fail git -C super-clone fetch --recurse-submodules &&
+
+ set_askpass wrong pass@host &&
+ git -C super-clone \
+ -c "credential.$HTTPD_URL.username=user@host" \
+ fetch --recurse-submodules &&
expect_askpass pass user@host
'
but that doesn't pass, even with the export fix! That's because fetch
doesn't go through git-submodule at all; it calls "git fetch" itself,
and uses local_repo_env, which clears the config. It needs to learn to
use the same mechanism that sanitize_submodule_env() does.
So AFAICT 14111fc49 is totally broken. It doesn't actually work for
git-submodule (because of the missing export), nor for git-fetch
(because that skips the shell script), and the one case we are testing
already worked without it (but probably _should_ be sanitizing the
config, so is buggy, too).
-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