* [PATCH 1/5] t5550: fix typo in $HTTPD_URL
2016-04-28 13:35 ` [PATCH 0/5] fixes for sanitized submodule config Jeff King
@ 2016-04-28 13:36 ` Jeff King
2016-04-28 15:24 ` Jacob Keller
2016-04-28 13:37 ` [PATCH 2/5] t5550: break submodule config test into multiple sub-tests Jeff King
` (4 subsequent siblings)
5 siblings, 1 reply; 36+ messages in thread
From: Jeff King @ 2016-04-28 13:36 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jacob Keller, Stefan Beller, Git Users, Jens.Lehmann
Commit 14111fc (git: submodule honor -c credential.* from
command line, 2016-02-29) accidentally wrote $HTTP_URL. It
happened to work because we ended up with "credential..helper",
which we treat the same as "credential.helper", applying it
to all URLs.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5550-http-fetch-dumb.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 48e2ab6..81cc57f 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -103,7 +103,7 @@ test_expect_success 'cmdline credential config passes into submodules' '
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
'
--
2.8.1.617.gbdccc2d
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] t5550: fix typo in $HTTPD_URL
2016-04-28 13:36 ` [PATCH 1/5] t5550: fix typo in $HTTPD_URL Jeff King
@ 2016-04-28 15:24 ` Jacob Keller
2016-04-28 15:25 ` Jeff King
0 siblings, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2016-04-28 15:24 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Stefan Beller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 6:36 AM, Jeff King <peff@peff.net> wrote:
> Commit 14111fc (git: submodule honor -c credential.* from
> command line, 2016-02-29) accidentally wrote $HTTP_URL. It
> happened to work because we ended up with "credential..helper",
> which we treat the same as "credential.helper", applying it
> to all URLs.
You say "credential.helper" twice here? I think that's confusing. The
patch looks perfectly fine but I am having trouble parsing this
description. 'We end up with X which we treat the same as X"?
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5550-http-fetch-dumb.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index 48e2ab6..81cc57f 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -103,7 +103,7 @@ test_expect_success 'cmdline credential config passes into submodules' '
> 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
> '
> --
> 2.8.1.617.gbdccc2d
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] t5550: fix typo in $HTTPD_URL
2016-04-28 15:24 ` Jacob Keller
@ 2016-04-28 15:25 ` Jeff King
2016-04-28 15:26 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Jeff King @ 2016-04-28 15:25 UTC (permalink / raw)
To: Jacob Keller; +Cc: Lars Schneider, Stefan Beller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 08:24:05AM -0700, Jacob Keller wrote:
> On Thu, Apr 28, 2016 at 6:36 AM, Jeff King <peff@peff.net> wrote:
> > Commit 14111fc (git: submodule honor -c credential.* from
> > command line, 2016-02-29) accidentally wrote $HTTP_URL. It
> > happened to work because we ended up with "credential..helper",
> > which we treat the same as "credential.helper", applying it
> > to all URLs.
>
> You say "credential.helper" twice here? I think that's confusing. The
> patch looks perfectly fine but I am having trouble parsing this
> description. 'We end up with X which we treat the same as X"?
Note the two dots in the first one. There is no variable $HTTP_URL, so:
credential.$HTTP_URL.helper
becomes:
credential..helper
-Peff
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] t5550: fix typo in $HTTPD_URL
2016-04-28 15:25 ` Jeff King
@ 2016-04-28 15:26 ` Jacob Keller
0 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2016-04-28 15:26 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Stefan Beller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 8:25 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 08:24:05AM -0700, Jacob Keller wrote:
>
>> On Thu, Apr 28, 2016 at 6:36 AM, Jeff King <peff@peff.net> wrote:
>> > Commit 14111fc (git: submodule honor -c credential.* from
>> > command line, 2016-02-29) accidentally wrote $HTTP_URL. It
>> > happened to work because we ended up with "credential..helper",
>> > which we treat the same as "credential.helper", applying it
>> > to all URLs.
>>
>> You say "credential.helper" twice here? I think that's confusing. The
>> patch looks perfectly fine but I am having trouble parsing this
>> description. 'We end up with X which we treat the same as X"?
>
> Note the two dots in the first one. There is no variable $HTTP_URL, so:
>
> credential.$HTTP_URL.helper
>
> becomes:
>
> credential..helper
>
> -Peff
Ah yes, very tiny text and tired morning eyes you are right. Makes
more sense now.
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
2016-04-28 13:35 ` [PATCH 0/5] fixes for sanitized submodule config Jeff King
2016-04-28 13:36 ` [PATCH 1/5] t5550: fix typo in $HTTPD_URL Jeff King
@ 2016-04-28 13:37 ` Jeff King
2016-04-28 15:21 ` Stefan Beller
2016-04-28 13:37 ` [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS Jeff King
` (3 subsequent siblings)
5 siblings, 1 reply; 36+ messages in thread
From: Jeff King @ 2016-04-28 13:37 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jacob Keller, Stefan Beller, Git Users, Jens.Lehmann
Right now we test only the cloning case, but there are other
interesting cases (e.g., fetching). Let's pull the setup
bits into their own test, which will make things flow more
logically once we start adding more tests which use the
setup.
Let's also introduce some whitespace to the clone-test to
split the two parts: making sure it fails without our
cmdline config, and that it succeeds with it.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5550-http-fetch-dumb.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 81cc57f..e8e91bb 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -91,17 +91,21 @@ test_expect_success 'configured username does not override URL' '
expect_askpass pass user@host
'
-test_expect_success 'cmdline credential config passes into submodules' '
+test_expect_success 'set up repo with http submodules' '
git init super &&
set_askpass user@host pass@host &&
(
cd super &&
git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
git commit -m "add submodule"
- ) &&
+ )
+'
+
+test_expect_success 'cmdline credential config passes to submodule via clone' '
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.$HTTPD_URL.username=user@host" \
clone --recursive super super-clone &&
--
2.8.1.617.gbdccc2d
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
2016-04-28 13:37 ` [PATCH 2/5] t5550: break submodule config test into multiple sub-tests Jeff King
@ 2016-04-28 15:21 ` Stefan Beller
2016-04-28 15:25 ` Jeff King
2016-04-28 15:25 ` Jacob Keller
0 siblings, 2 replies; 36+ messages in thread
From: Stefan Beller @ 2016-04-28 15:21 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 6:37 AM, Jeff King <peff@peff.net> wrote:
> Right now we test only the cloning case, but there are other
> interesting cases (e.g., fetching). Let's pull the setup
> bits into their own test, which will make things flow more
> logically once we start adding more tests which use the
> setup.
>
> Let's also introduce some whitespace to the clone-test to
> split the two parts: making sure it fails without our
> cmdline config, and that it succeeds with it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5550-http-fetch-dumb.sh | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index 81cc57f..e8e91bb 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -91,17 +91,21 @@ test_expect_success 'configured username does not override URL' '
> expect_askpass pass user@host
> '
>
> -test_expect_success 'cmdline credential config passes into submodules' '
> +test_expect_success 'set up repo with http submodules' '
set up or setup?
$ grep -r "set up" |wc -l
69
$ grep -r "setup" |wc -l
1162
Apart from that nit, this patch looks good to me.
> git init super &&
> set_askpass user@host pass@host &&
> (
> cd super &&
> git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
> git commit -m "add submodule"
> - ) &&
> + )
> +'
> +
> +test_expect_success 'cmdline credential config passes to submodule via clone' '
> 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.$HTTPD_URL.username=user@host" \
> clone --recursive super super-clone &&
> --
> 2.8.1.617.gbdccc2d
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
2016-04-28 15:21 ` Stefan Beller
@ 2016-04-28 15:25 ` Jeff King
2016-04-28 15:25 ` Jacob Keller
1 sibling, 0 replies; 36+ messages in thread
From: Jeff King @ 2016-04-28 15:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 08:21:21AM -0700, Stefan Beller wrote:
> > -test_expect_success 'cmdline credential config passes into submodules' '
> > +test_expect_success 'set up repo with http submodules' '
>
> set up or setup?
>
> $ grep -r "set up" |wc -l
> 69
> $ grep -r "setup" |wc -l
> 1162
>
> Apart from that nit, this patch looks good to me.
"set up" is a verb phrase. "setup" is a noun or an adjective.
I think we quite often use the latter as a verb. Whether that is
grammatically depends on your philosophy, I think.
-Peff
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/5] t5550: break submodule config test into multiple sub-tests
2016-04-28 15:21 ` Stefan Beller
2016-04-28 15:25 ` Jeff King
@ 2016-04-28 15:25 ` Jacob Keller
1 sibling, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2016-04-28 15:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jeff King, Lars Schneider, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 8:21 AM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Apr 28, 2016 at 6:37 AM, Jeff King <peff@peff.net> wrote:
>> Right now we test only the cloning case, but there are other
>> interesting cases (e.g., fetching). Let's pull the setup
>> bits into their own test, which will make things flow more
>> logically once we start adding more tests which use the
>> setup.
>>
>> Let's also introduce some whitespace to the clone-test to
>> split the two parts: making sure it fails without our
>> cmdline config, and that it succeeds with it.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> t/t5550-http-fetch-dumb.sh | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
>> index 81cc57f..e8e91bb 100755
>> --- a/t/t5550-http-fetch-dumb.sh
>> +++ b/t/t5550-http-fetch-dumb.sh
>> @@ -91,17 +91,21 @@ test_expect_success 'configured username does not override URL' '
>> expect_askpass pass user@host
>> '
>>
>> -test_expect_success 'cmdline credential config passes into submodules' '
>> +test_expect_success 'set up repo with http submodules' '
>
> set up or setup?
>
> $ grep -r "set up" |wc -l
> 69
> $ grep -r "setup" |wc -l
> 1162
>
> Apart from that nit, this patch looks good to me.
>
Yes this looks quite a bit more readable.
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 13:35 ` [PATCH 0/5] fixes for sanitized submodule config Jeff King
2016-04-28 13:36 ` [PATCH 1/5] t5550: fix typo in $HTTPD_URL Jeff King
2016-04-28 13:37 ` [PATCH 2/5] t5550: break submodule config test into multiple sub-tests Jeff King
@ 2016-04-28 13:37 ` Jeff King
2016-04-28 15:25 ` Stefan Beller
2016-04-28 15:28 ` Jacob Keller
2016-04-28 13:38 ` [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c Jeff King
` (2 subsequent siblings)
5 siblings, 2 replies; 36+ messages in thread
From: Jeff King @ 2016-04-28 13:37 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jacob Keller, Stefan Beller, Git Users, Jens.Lehmann
Commit 14111fc (git: submodule honor -c credential.* from
command line, 2016-02-29) taught git-submodule.sh to save
the sanitized value of $GIT_CONFIG_PARAMETERS when clearing
the environment for a submodule. However, it failed to
export the result, meaning that it had no effect for any
sub-programs.
We didn't catch this in our initial tests because we checked
only the "clone" case, which does not go through the shell
script at all. Provoking "git submodule update" to do a
fetch demonstrates the bug.
Noticed-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
git-submodule.sh | 1 +
t/t5550-http-fetch-dumb.sh | 17 +++++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 2a84d7e..3a40d4b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -200,6 +200,7 @@ sanitize_submodule_env()
sanitized_config=$(git submodule--helper sanitize-config)
clear_local_git_env
GIT_CONFIG_PARAMETERS=$sanitized_config
+ export GIT_CONFIG_PARAMETERS
}
#
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index e8e91bb..13ac788 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -112,6 +112,23 @@ test_expect_success 'cmdline credential config passes to submodule via clone' '
expect_askpass pass user@host
'
+test_expect_success 'cmdline credential config passes submodule update' '
+ # advance the submodule HEAD so that a fetch is required
+ git commit --allow-empty -m foo &&
+ git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
+ sha1=$(git rev-parse HEAD) &&
+ git -C super-clone update-index --cacheinfo 160000,$sha1,sub &&
+
+ set_askpass wrong pass@host &&
+ test_must_fail git -C super-clone submodule update &&
+
+ set_askpass wrong pass@host &&
+ git -C super-clone \
+ -c "credential.$HTTPD_URL.username=user@host" \
+ submodule update &&
+ expect_askpass pass user@host
+'
+
test_expect_success 'fetch changes via http' '
echo content >>file &&
git commit -a -m two &&
--
2.8.1.617.gbdccc2d
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 13:37 ` [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS Jeff King
@ 2016-04-28 15:25 ` Stefan Beller
2016-04-28 15:28 ` Jeff King
2016-04-28 15:28 ` Jacob Keller
1 sibling, 1 reply; 36+ messages in thread
From: Stefan Beller @ 2016-04-28 15:25 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 6:37 AM, Jeff King <peff@peff.net> wrote:
> Commit 14111fc (git: submodule honor -c credential.* from
> command line, 2016-02-29) taught git-submodule.sh to save
> the sanitized value of $GIT_CONFIG_PARAMETERS when clearing
> the environment for a submodule. However, it failed to
> export the result, meaning that it had no effect for any
> sub-programs.
>
> We didn't catch this in our initial tests because we checked
> only the "clone" case, which does not go through the shell
> script at all. Provoking "git submodule update" to do a
> fetch demonstrates the bug.
>
> Noticed-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> git-submodule.sh | 1 +
> t/t5550-http-fetch-dumb.sh | 17 +++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2a84d7e..3a40d4b 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -200,6 +200,7 @@ sanitize_submodule_env()
> sanitized_config=$(git submodule--helper sanitize-config)
> clear_local_git_env
> GIT_CONFIG_PARAMETERS=$sanitized_config
> + export GIT_CONFIG_PARAMETERS
> }
>
> #
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index e8e91bb..13ac788 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -112,6 +112,23 @@ test_expect_success 'cmdline credential config passes to submodule via clone' '
> expect_askpass pass user@host
> '
>
> +test_expect_success 'cmdline credential config passes submodule update' '
> + # advance the submodule HEAD so that a fetch is required
> + git commit --allow-empty -m foo &&
> + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
> + sha1=$(git rev-parse HEAD) &&
> + git -C super-clone update-index --cacheinfo 160000,$sha1,sub &&
The use of update-index seems elegant to me, though different than
any submodule test I wrote so far. :)
> +
> + set_askpass wrong pass@host &&
> + test_must_fail git -C super-clone submodule update &&
> +
> + set_askpass wrong pass@host &&
> + git -C super-clone \
> + -c "credential.$HTTPD_URL.username=user@host" \
> + submodule update &&
> + expect_askpass pass user@host
> +'
> +
> test_expect_success 'fetch changes via http' '
> echo content >>file &&
> git commit -a -m two &&
> --
> 2.8.1.617.gbdccc2d
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 15:25 ` Stefan Beller
@ 2016-04-28 15:28 ` Jeff King
2016-04-28 15:35 ` Stefan Beller
2016-04-28 16:51 ` Johannes Schindelin
0 siblings, 2 replies; 36+ messages in thread
From: Jeff King @ 2016-04-28 15:28 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 08:25:29AM -0700, Stefan Beller wrote:
> > +test_expect_success 'cmdline credential config passes submodule update' '
> > + # advance the submodule HEAD so that a fetch is required
> > + git commit --allow-empty -m foo &&
> > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
> > + sha1=$(git rev-parse HEAD) &&
> > + git -C super-clone update-index --cacheinfo 160000,$sha1,sub &&
>
> The use of update-index seems elegant to me, though different than
> any submodule test I wrote so far. :)
Yeah, I actually wrestled with finding the shortest recipe to convince
git-submodule to actually call git-fetch. Suggestions welcome if there's
something more canonical.
But I think we have to advance the submodule pointer in some way to
convince it to want to fetch (I also tried deleting the refs in the
cloned module, but that seemed hacky).
I guess the way it would happen in real life is that the "origin" remote
("super" here, not "super-clone") would make the change and commit the
submodule, and then "super-clone" would pull it.
That seemed even more convoluted to me.
-Peff
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 15:28 ` Jeff King
@ 2016-04-28 15:35 ` Stefan Beller
2016-04-28 16:51 ` Johannes Schindelin
1 sibling, 0 replies; 36+ messages in thread
From: Stefan Beller @ 2016-04-28 15:35 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 8:28 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 08:25:29AM -0700, Stefan Beller wrote:
>
>> > +test_expect_success 'cmdline credential config passes submodule update' '
>> > + # advance the submodule HEAD so that a fetch is required
>> > + git commit --allow-empty -m foo &&
>> > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
>> > + sha1=$(git rev-parse HEAD) &&
>> > + git -C super-clone update-index --cacheinfo 160000,$sha1,sub &&
>>
>> The use of update-index seems elegant to me, though different than
>> any submodule test I wrote so far. :)
>
> Yeah, I actually wrestled with finding the shortest recipe to convince
> git-submodule to actually call git-fetch. Suggestions welcome if there's
> something more canonical.
>
> But I think we have to advance the submodule pointer in some way to
> convince it to want to fetch (I also tried deleting the refs in the
> cloned module, but that seemed hacky).
>
> I guess the way it would happen in real life is that the "origin" remote
> ("super" here, not "super-clone") would make the change and commit the
> submodule, and then "super-clone" would pull it.
>
> That seemed even more convoluted to me.
That's what I write in the other submodule tests, because I think we should
test closest to reality.
Instead of deleting the refs code, another hacky way would be
echo $newsha1 > .git/module/remote/origin/refs/heads/master
but the update-index is less hacky, so let's keep that.
I did not want to point out an issue with it, just that I was pleasantly
surprised to see such a short test.
Thanks,
Stefan
>
> -Peff
> --
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 15:28 ` Jeff King
2016-04-28 15:35 ` Stefan Beller
@ 2016-04-28 16:51 ` Johannes Schindelin
1 sibling, 0 replies; 36+ messages in thread
From: Johannes Schindelin @ 2016-04-28 16:51 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Lars Schneider, Jacob Keller, Git Users,
Jens Lehmann
Hi,
On Thu, 28 Apr 2016, Jeff King wrote:
> On Thu, Apr 28, 2016 at 08:25:29AM -0700, Stefan Beller wrote:
>
> > > +test_expect_success 'cmdline credential config passes submodule update' '
> > > + # advance the submodule HEAD so that a fetch is required
> > > + git commit --allow-empty -m foo &&
> > > + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD &&
> > > + sha1=$(git rev-parse HEAD) &&
> > > + git -C super-clone update-index --cacheinfo 160000,$sha1,sub &&
> >
> > The use of update-index seems elegant to me, though different than
> > any submodule test I wrote so far. :)
>
> Yeah, I actually wrestled with finding the shortest recipe to convince
> git-submodule to actually call git-fetch. Suggestions welcome if there's
> something more canonical.
FWIW that's exactly how I did things in
https://github.com/dscho/git/commit/89d0024450b0e6e9997ad9e3d681248bde1bafc0
:-)
Ciao,
Dscho
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 13:37 ` [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS Jeff King
2016-04-28 15:25 ` Stefan Beller
@ 2016-04-28 15:28 ` Jacob Keller
2016-04-28 15:36 ` Jeff King
1 sibling, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2016-04-28 15:28 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Stefan Beller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 6:37 AM, Jeff King <peff@peff.net> wrote:
> Commit 14111fc (git: submodule honor -c credential.* from
> command line, 2016-02-29) taught git-submodule.sh to save
> the sanitized value of $GIT_CONFIG_PARAMETERS when clearing
> the environment for a submodule. However, it failed to
> export the result, meaning that it had no effect for any
> sub-programs.
>
> We didn't catch this in our initial tests because we checked
> only the "clone" case, which does not go through the shell
> script at all. Provoking "git submodule update" to do a
> fetch demonstrates the bug.
>
> Noticed-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> git-submodule.sh | 1 +
> t/t5550-http-fetch-dumb.sh | 17 +++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2a84d7e..3a40d4b 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -200,6 +200,7 @@ sanitize_submodule_env()
> sanitized_config=$(git submodule--helper sanitize-config)
> clear_local_git_env
> GIT_CONFIG_PARAMETERS=$sanitized_config
> + export GIT_CONFIG_PARAMETERS
why not
export GIT_CONFIG_PARAMETERS=$santized_config
?
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 15:28 ` Jacob Keller
@ 2016-04-28 15:36 ` Jeff King
2016-04-28 15:40 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Jeff King @ 2016-04-28 15:36 UTC (permalink / raw)
To: Jacob Keller; +Cc: Lars Schneider, Stefan Beller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 08:28:29AM -0700, Jacob Keller wrote:
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 2a84d7e..3a40d4b 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -200,6 +200,7 @@ sanitize_submodule_env()
> > sanitized_config=$(git submodule--helper sanitize-config)
> > clear_local_git_env
> > GIT_CONFIG_PARAMETERS=$sanitized_config
> > + export GIT_CONFIG_PARAMETERS
>
> why not
>
> export GIT_CONFIG_PARAMETERS=$santized_config
Portability. Try:
$ dash -c 'one="foo bar"; export two=$one; echo $two'
foo
$ bash -c 'one="foo bar"; export two=$one; echo $two'
foo bar
I think:
export GIT_CONFIG_PARAMETERS="$sanitized_config"
solves that. Some antique shells do not like "export x=y" at all, but I
don't know if any of them are still relevant.
-Peff
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS
2016-04-28 15:36 ` Jeff King
@ 2016-04-28 15:40 ` Jacob Keller
0 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2016-04-28 15:40 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Stefan Beller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 8:36 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 08:28:29AM -0700, Jacob Keller wrote:
>
>> > diff --git a/git-submodule.sh b/git-submodule.sh
>> > index 2a84d7e..3a40d4b 100755
>> > --- a/git-submodule.sh
>> > +++ b/git-submodule.sh
>> > @@ -200,6 +200,7 @@ sanitize_submodule_env()
>> > sanitized_config=$(git submodule--helper sanitize-config)
>> > clear_local_git_env
>> > GIT_CONFIG_PARAMETERS=$sanitized_config
>> > + export GIT_CONFIG_PARAMETERS
>>
>> why not
>>
>> export GIT_CONFIG_PARAMETERS=$santized_config
>
> Portability. Try:
>
> $ dash -c 'one="foo bar"; export two=$one; echo $two'
> foo
>
> $ bash -c 'one="foo bar"; export two=$one; echo $two'
> foo bar
>
> I think:
>
> export GIT_CONFIG_PARAMETERS="$sanitized_config"
>
> solves that. Some antique shells do not like "export x=y" at all, but I
> don't know if any of them are still relevant.
>
> -Peff
Fair enough. quotes would most likely work and I know I am in the
habbit of using quotes a bit more liberally but this will work
correctly so I wouldn't change it.
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
2016-04-28 13:35 ` [PATCH 0/5] fixes for sanitized submodule config Jeff King
` (2 preceding siblings ...)
2016-04-28 13:37 ` [PATCH 3/5] submodule: export sanitized GIT_CONFIG_PARAMETERS Jeff King
@ 2016-04-28 13:38 ` Jeff King
2016-04-28 15:30 ` Stefan Beller
2016-04-28 16:28 ` Lars Schneider
2016-04-28 13:39 ` [PATCH 5/5] submodule: use prepare_submodule_repo_env consistently Jeff King
2016-04-28 14:02 ` [PATCH 0/5] fixes for sanitized submodule config Johannes Schindelin
5 siblings, 2 replies; 36+ messages in thread
From: Jeff King @ 2016-04-28 13:38 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jacob Keller, Stefan Beller, Git Users, Jens.Lehmann
These functions should be used by any code which spawns a
submodule process, which may happen in submodule.c (e.g.,
for spawning fetch). Let's move them there and make them
public so that submodule--helper can continue to use them.
Sine they're now public, let's also provide a basic overview
of their intended use.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/submodule--helper.c | 48 --------------------------------------------
submodule.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
submodule.h | 16 +++++++++++++++
3 files changed, 65 insertions(+), 48 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..de3ad5b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -119,54 +119,6 @@ static int module_name(int argc, const char **argv, const char *prefix)
return 0;
}
-/*
- * Rules to sanitize configuration variables that are Ok to be passed into
- * submodule operations from the parent project using "-c". Should only
- * include keys which are both (a) safe and (b) necessary for proper
- * operation.
- */
-static int submodule_config_ok(const char *var)
-{
- if (starts_with(var, "credential."))
- return 1;
- return 0;
-}
-
-static int sanitize_submodule_config(const char *var, const char *value, void *data)
-{
- struct strbuf *out = data;
-
- if (submodule_config_ok(var)) {
- if (out->len)
- strbuf_addch(out, ' ');
-
- if (value)
- sq_quotef(out, "%s=%s", var, value);
- else
- sq_quote_buf(out, var);
- }
-
- return 0;
-}
-
-static void prepare_submodule_repo_env(struct argv_array *out)
-{
- const char * const *var;
-
- for (var = local_repo_env; *var; var++) {
- if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
- struct strbuf sanitized_config = STRBUF_INIT;
- git_config_from_parameters(sanitize_submodule_config,
- &sanitized_config);
- argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
- strbuf_release(&sanitized_config);
- } else {
- argv_array_push(out, *var);
- }
- }
-
-}
-
static int clone_submodule(const char *path, const char *gitdir, const char *url,
const char *depth, const char *reference, int quiet)
{
diff --git a/submodule.c b/submodule.c
index 90825e1..02eaf0e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -13,6 +13,7 @@
#include "argv-array.h"
#include "blob.h"
#include "thread-utils.h"
+#include "quote.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
static int parallel_jobs = 1;
@@ -1129,3 +1130,51 @@ int parallel_submodules(void)
{
return parallel_jobs;
}
+
+/*
+ * Rules to sanitize configuration variables that are Ok to be passed into
+ * submodule operations from the parent project using "-c". Should only
+ * include keys which are both (a) safe and (b) necessary for proper
+ * operation.
+ */
+static int submodule_config_ok(const char *var)
+{
+ if (starts_with(var, "credential."))
+ return 1;
+ return 0;
+}
+
+int sanitize_submodule_config(const char *var, const char *value, void *data)
+{
+ struct strbuf *out = data;
+
+ if (submodule_config_ok(var)) {
+ if (out->len)
+ strbuf_addch(out, ' ');
+
+ if (value)
+ sq_quotef(out, "%s=%s", var, value);
+ else
+ sq_quote_buf(out, var);
+ }
+
+ return 0;
+}
+
+void prepare_submodule_repo_env(struct argv_array *out)
+{
+ const char * const *var;
+
+ for (var = local_repo_env; *var; var++) {
+ if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
+ struct strbuf sanitized_config = STRBUF_INIT;
+ git_config_from_parameters(sanitize_submodule_config,
+ &sanitized_config);
+ argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
+ strbuf_release(&sanitized_config);
+ } else {
+ argv_array_push(out, *var);
+ }
+ }
+
+}
diff --git a/submodule.h b/submodule.h
index 7ef3775..7577b3b 100644
--- a/submodule.h
+++ b/submodule.h
@@ -61,4 +61,20 @@ int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
+/*
+ * This function is intended as a callback for use with
+ * git_config_from_parameters(). It ignores any config options which
+ * are not suitable for passing along to a submodule, and accumulates the rest
+ * in "data", which must be a pointer to a strbuf. The end result can
+ * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
+ */
+int sanitize_submodule_config(const char *var, const char *value, void *data);
+
+/*
+ * Prepare the "env_array" parameter of a "struct child_process" for executing
+ * a submodule by clearing any repo-specific envirionment variables, but
+ * retaining any config approved by sanitize_submodule_config().
+ */
+void prepare_submodule_repo_env(struct argv_array *out);
+
#endif
--
2.8.1.617.gbdccc2d
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
2016-04-28 13:38 ` [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c Jeff King
@ 2016-04-28 15:30 ` Stefan Beller
2016-04-28 15:37 ` Jeff King
2016-04-28 16:28 ` Lars Schneider
1 sibling, 1 reply; 36+ messages in thread
From: Stefan Beller @ 2016-04-28 15:30 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 6:38 AM, Jeff King <peff@peff.net> wrote:
> These functions should be used by any code which spawns a
> submodule process, which may happen in submodule.c (e.g.,
> for spawning fetch). Let's move them there and make them
> public so that submodule--helper can continue to use them.
>
> Sine they're now public, let's also provide a basic overview
> of their intended use.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/submodule--helper.c | 48 --------------------------------------------
> submodule.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
> submodule.h | 16 +++++++++++++++
> 3 files changed, 65 insertions(+), 48 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3bd6883..de3ad5b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -119,54 +119,6 @@ static int module_name(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> - if (starts_with(var, "credential."))
> - return 1;
> - return 0;
> -}
> -
> -static int sanitize_submodule_config(const char *var, const char *value, void *data)
> -{
> - struct strbuf *out = data;
> -
> - if (submodule_config_ok(var)) {
> - if (out->len)
> - strbuf_addch(out, ' ');
> -
> - if (value)
> - sq_quotef(out, "%s=%s", var, value);
> - else
> - sq_quote_buf(out, var);
> - }
> -
> - return 0;
> -}
> -
> -static void prepare_submodule_repo_env(struct argv_array *out)
> -{
> - const char * const *var;
> -
> - for (var = local_repo_env; *var; var++) {
> - if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> - struct strbuf sanitized_config = STRBUF_INIT;
> - git_config_from_parameters(sanitize_submodule_config,
> - &sanitized_config);
> - argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> - strbuf_release(&sanitized_config);
> - } else {
> - argv_array_push(out, *var);
> - }
> - }
> -
> -}
> -
> static int clone_submodule(const char *path, const char *gitdir, const char *url,
> const char *depth, const char *reference, int quiet)
> {
> diff --git a/submodule.c b/submodule.c
> index 90825e1..02eaf0e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -13,6 +13,7 @@
> #include "argv-array.h"
> #include "blob.h"
> #include "thread-utils.h"
> +#include "quote.h"
>
> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
> static int parallel_jobs = 1;
> @@ -1129,3 +1130,51 @@ int parallel_submodules(void)
> {
> return parallel_jobs;
> }
> +
> +/*
> + * Rules to sanitize configuration variables that are Ok to be passed into
> + * submodule operations from the parent project using "-c". Should only
> + * include keys which are both (a) safe and (b) necessary for proper
> + * operation.
> + */
> +static int submodule_config_ok(const char *var)
> +{
> + if (starts_with(var, "credential."))
> + return 1;
> + return 0;
> +}
> +
> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> + struct strbuf *out = data;
> +
> + if (submodule_config_ok(var)) {
> + if (out->len)
> + strbuf_addch(out, ' ');
> +
> + if (value)
> + sq_quotef(out, "%s=%s", var, value);
> + else
> + sq_quote_buf(out, var);
> + }
> +
> + return 0;
> +}
> +
> +void prepare_submodule_repo_env(struct argv_array *out)
> +{
> + const char * const *var;
> +
> + for (var = local_repo_env; *var; var++) {
> + if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> + struct strbuf sanitized_config = STRBUF_INIT;
> + git_config_from_parameters(sanitize_submodule_config,
> + &sanitized_config);
> + argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> + strbuf_release(&sanitized_config);
> + } else {
> + argv_array_push(out, *var);
> + }
> + }
> +
> +}
> diff --git a/submodule.h b/submodule.h
> index 7ef3775..7577b3b 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -61,4 +61,20 @@ int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
> void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
> int parallel_submodules(void);
>
> +/*
> + * This function is intended as a callback for use with
> + * git_config_from_parameters(). It ignores any config options which
> + * are not suitable for passing along to a submodule, and accumulates the rest
> + * in "data", which must be a pointer to a strbuf.
So why is it a void* then? You could make it a strbuf* here, so you
would not have to document it?
Oh right, because of git_config_from_parameters(sanitize_submodule_config, ...
> The end result can
> + * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
s/sub-process/process operating on submodules/, maybe ?
While it is technically a sub-process, I started to have an aversion
against "sub"-things
unless strictly required. :)
> + */
> +int sanitize_submodule_config(const char *var, const char *value, void *data);
> +
> +/*
> + * Prepare the "env_array" parameter of a "struct child_process" for executing
> + * a submodule by clearing any repo-specific envirionment variables, but
> + * retaining any config approved by sanitize_submodule_config().
> + */
> +void prepare_submodule_repo_env(struct argv_array *out);
> +
> #endif
> --
> 2.8.1.617.gbdccc2d
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
2016-04-28 15:30 ` Stefan Beller
@ 2016-04-28 15:37 ` Jeff King
0 siblings, 0 replies; 36+ messages in thread
From: Jeff King @ 2016-04-28 15:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 08:30:45AM -0700, Stefan Beller wrote:
> > +/*
> > + * This function is intended as a callback for use with
> > + * git_config_from_parameters(). It ignores any config options which
> > + * are not suitable for passing along to a submodule, and accumulates the rest
> > + * in "data", which must be a pointer to a strbuf.
>
> So why is it a void* then? You could make it a strbuf* here, so you
> would not have to document it?
> Oh right, because of git_config_from_parameters(sanitize_submodule_config, ...
Exactly (and why it is all the more important do document it!).
> > The end result can
> > + * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
>
> s/sub-process/process operating on submodules/, maybe ?
>
> While it is technically a sub-process, I started to have an aversion
> against "sub"-things
> unless strictly required. :)
Technically it can be any process which then spawns a process operating
on a submodule. Maybe "another process" would be enough?
-Peff
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c
2016-04-28 13:38 ` [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c Jeff King
2016-04-28 15:30 ` Stefan Beller
@ 2016-04-28 16:28 ` Lars Schneider
1 sibling, 0 replies; 36+ messages in thread
From: Lars Schneider @ 2016-04-28 16:28 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Stefan Beller, Git Users, Jens.Lehmann
> On 28 Apr 2016, at 15:38, Jeff King <peff@peff.net> wrote:
>
> These functions should be used by any code which spawns a
> submodule process, which may happen in submodule.c (e.g.,
> for spawning fetch). Let's move them there and make them
> public so that submodule--helper can continue to use them.
>
> Sine they're now public, let's also
s/Sine/Since
Thanks for fixing this entire issue, Peff!
- Lars
> provide a basic overview
> of their intended use.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/submodule--helper.c | 48 --------------------------------------------
> submodule.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
> submodule.h | 16 +++++++++++++++
> 3 files changed, 65 insertions(+), 48 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3bd6883..de3ad5b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -119,54 +119,6 @@ static int module_name(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> - if (starts_with(var, "credential."))
> - return 1;
> - return 0;
> -}
> -
> -static int sanitize_submodule_config(const char *var, const char *value, void *data)
> -{
> - struct strbuf *out = data;
> -
> - if (submodule_config_ok(var)) {
> - if (out->len)
> - strbuf_addch(out, ' ');
> -
> - if (value)
> - sq_quotef(out, "%s=%s", var, value);
> - else
> - sq_quote_buf(out, var);
> - }
> -
> - return 0;
> -}
> -
> -static void prepare_submodule_repo_env(struct argv_array *out)
> -{
> - const char * const *var;
> -
> - for (var = local_repo_env; *var; var++) {
> - if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> - struct strbuf sanitized_config = STRBUF_INIT;
> - git_config_from_parameters(sanitize_submodule_config,
> - &sanitized_config);
> - argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> - strbuf_release(&sanitized_config);
> - } else {
> - argv_array_push(out, *var);
> - }
> - }
> -
> -}
> -
> static int clone_submodule(const char *path, const char *gitdir, const char *url,
> const char *depth, const char *reference, int quiet)
> {
> diff --git a/submodule.c b/submodule.c
> index 90825e1..02eaf0e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -13,6 +13,7 @@
> #include "argv-array.h"
> #include "blob.h"
> #include "thread-utils.h"
> +#include "quote.h"
>
> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
> static int parallel_jobs = 1;
> @@ -1129,3 +1130,51 @@ int parallel_submodules(void)
> {
> return parallel_jobs;
> }
> +
> +/*
> + * Rules to sanitize configuration variables that are Ok to be passed into
> + * submodule operations from the parent project using "-c". Should only
> + * include keys which are both (a) safe and (b) necessary for proper
> + * operation.
> + */
> +static int submodule_config_ok(const char *var)
> +{
> + if (starts_with(var, "credential."))
> + return 1;
> + return 0;
> +}
> +
> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> + struct strbuf *out = data;
> +
> + if (submodule_config_ok(var)) {
> + if (out->len)
> + strbuf_addch(out, ' ');
> +
> + if (value)
> + sq_quotef(out, "%s=%s", var, value);
> + else
> + sq_quote_buf(out, var);
> + }
> +
> + return 0;
> +}
> +
> +void prepare_submodule_repo_env(struct argv_array *out)
> +{
> + const char * const *var;
> +
> + for (var = local_repo_env; *var; var++) {
> + if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> + struct strbuf sanitized_config = STRBUF_INIT;
> + git_config_from_parameters(sanitize_submodule_config,
> + &sanitized_config);
> + argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> + strbuf_release(&sanitized_config);
> + } else {
> + argv_array_push(out, *var);
> + }
> + }
> +
> +}
> diff --git a/submodule.h b/submodule.h
> index 7ef3775..7577b3b 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -61,4 +61,20 @@ int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
> void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
> int parallel_submodules(void);
>
> +/*
> + * This function is intended as a callback for use with
> + * git_config_from_parameters(). It ignores any config options which
> + * are not suitable for passing along to a submodule, and accumulates the rest
> + * in "data", which must be a pointer to a strbuf. The end result can
> + * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
> + */
> +int sanitize_submodule_config(const char *var, const char *value, void *data);
> +
> +/*
> + * Prepare the "env_array" parameter of a "struct child_process" for executing
> + * a submodule by clearing any repo-specific envirionment variables, but
> + * retaining any config approved by sanitize_submodule_config().
> + */
> +void prepare_submodule_repo_env(struct argv_array *out);
> +
> #endif
> --
> 2.8.1.617.gbdccc2d
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 5/5] submodule: use prepare_submodule_repo_env consistently
2016-04-28 13:35 ` [PATCH 0/5] fixes for sanitized submodule config Jeff King
` (3 preceding siblings ...)
2016-04-28 13:38 ` [PATCH 4/5] submodule--helper: move config-sanitizing to submodule.c Jeff King
@ 2016-04-28 13:39 ` Jeff King
2016-04-28 14:02 ` [PATCH 0/5] fixes for sanitized submodule config Johannes Schindelin
5 siblings, 0 replies; 36+ messages in thread
From: Jeff King @ 2016-04-28 13:39 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jacob Keller, Stefan Beller, Git Users, Jens.Lehmann
Before 14111fc (git: submodule honor -c credential.* from
command line, 2016-02-29), it was sufficient for code which
spawned a process in a submodule to just set the child
process's "env" field to "local_repo_env" to clear the
environment of any repo-specific variables.
That commit introduced a more complicated procedure, in
which we clear most variables but allow through sanitized
config. For C code, we used that procedure only for cloning,
but not for any of the programs spawned by submodule.c. As a
result, things like "git fetch --recurse-submodules" behave
differently than "git clone --recursive"; the former will
not pass through the sanitized config.
We can fix this by using prepare_submodule_repo_env()
everywhere in submodule.c.
Signed-off-by: Jeff King <peff@peff.net>
---
submodule.c | 14 +++++++-------
t/t5550-http-fetch-dumb.sh | 11 +++++++++++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/submodule.c b/submodule.c
index 02eaf0e..4e76b98 100644
--- a/submodule.c
+++ b/submodule.c
@@ -394,7 +394,7 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20
argv[1] = sha1_to_hex(sha1);
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
@@ -481,7 +481,7 @@ static int push_submodule(const char *path)
const char *argv[] = {"push", NULL};
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.dir = path;
@@ -527,7 +527,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
argv[3] = sha1_to_hex(sha1);
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.dir = path;
@@ -710,7 +710,7 @@ static int get_next_submodule(struct child_process *cp,
if (is_directory(git_dir)) {
child_process_init(cp);
cp->dir = strbuf_detach(&submodule_path, NULL);
- cp->env = local_repo_env;
+ prepare_submodule_repo_env(&cp->env_array);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -825,7 +825,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
argv[2] = "-uno";
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
@@ -886,7 +886,7 @@ int submodule_uses_gitfile(const char *path)
/* Now test that all nested submodules use a gitfile too */
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.no_stderr = 1;
@@ -919,7 +919,7 @@ int ok_to_remove_submodule(const char *path)
return 0;
cp.argv = argv;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 13ac788..3484b6f 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -112,6 +112,17 @@ test_expect_success 'cmdline credential config passes to submodule via clone' '
expect_askpass pass user@host
'
+test_expect_success 'cmdline credential config passes submodule via fetch' '
+ 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
+'
+
test_expect_success 'cmdline credential config passes submodule update' '
# advance the submodule HEAD so that a fetch is required
git commit --allow-empty -m foo &&
--
2.8.1.617.gbdccc2d
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] fixes for sanitized submodule config
2016-04-28 13:35 ` [PATCH 0/5] fixes for sanitized submodule config Jeff King
` (4 preceding siblings ...)
2016-04-28 13:39 ` [PATCH 5/5] submodule: use prepare_submodule_repo_env consistently Jeff King
@ 2016-04-28 14:02 ` Johannes Schindelin
2016-04-28 15:56 ` Stefan Beller
5 siblings, 1 reply; 36+ messages in thread
From: Johannes Schindelin @ 2016-04-28 14:02 UTC (permalink / raw)
To: Jeff King
Cc: Lars Schneider, Jacob Keller, Stefan Beller, Git Users,
Jens.Lehmann
Hi Peff,
On Thu, 28 Apr 2016, Jeff King wrote:
> On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote:
>
> > So that case _is_ correct right now. It's just that t5550 isn't testing
> > the shell script part, which is broken. Probably running "git submodule
> > update" in the resulting clone would cover that.
> >
> > And for the fetch case, we probably just need to be calling
> > prepare_submodule_repo_env() there, too.
>
> So here's a series which fixes sanitizing in the "git-submodule" shell
> script, along with "git fetch". And cleans up a few things along the
> way.
Nice!
I reviewed those changes and they all look sensible to me (did not apply
them locally for lack of time, though).
Ciao,
Dscho
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] fixes for sanitized submodule config
2016-04-28 14:02 ` [PATCH 0/5] fixes for sanitized submodule config Johannes Schindelin
@ 2016-04-28 15:56 ` Stefan Beller
2016-04-28 16:03 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Stefan Beller @ 2016-04-28 15:56 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Lars Schneider, Jacob Keller, Git Users, Jens Lehmann
On Thu, Apr 28, 2016 at 7:02 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Peff,
>
> On Thu, 28 Apr 2016, Jeff King wrote:
>
>> On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote:
>>
>> > So that case _is_ correct right now. It's just that t5550 isn't testing
>> > the shell script part, which is broken. Probably running "git submodule
>> > update" in the resulting clone would cover that.
>> >
>> > And for the fetch case, we probably just need to be calling
>> > prepare_submodule_repo_env() there, too.
>>
>> So here's a series which fixes sanitizing in the "git-submodule" shell
>> script, along with "git fetch". And cleans up a few things along the
>> way.
>
> Nice!
>
> I reviewed those changes and they all look sensible to me (did not apply
> them locally for lack of time, though).
Same here.
>
> Ciao,
> Dscho
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] fixes for sanitized submodule config
2016-04-28 15:56 ` Stefan Beller
@ 2016-04-28 16:03 ` Jacob Keller
0 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2016-04-28 16:03 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, Jeff King, Lars Schneider, Git Users,
Jens Lehmann
On Thu, Apr 28, 2016 at 8:56 AM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Apr 28, 2016 at 7:02 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Peff,
>>
>> On Thu, 28 Apr 2016, Jeff King wrote:
>>
>>> On Thu, Apr 28, 2016 at 08:17:53AM -0400, Jeff King wrote:
>>>
>>> > So that case _is_ correct right now. It's just that t5550 isn't testing
>>> > the shell script part, which is broken. Probably running "git submodule
>>> > update" in the resulting clone would cover that.
>>> >
>>> > And for the fetch case, we probably just need to be calling
>>> > prepare_submodule_repo_env() there, too.
>>>
>>> So here's a series which fixes sanitizing in the "git-submodule" shell
>>> script, along with "git fetch". And cleans up a few things along the
>>> way.
>>
>> Nice!
>>
>> I reviewed those changes and they all look sensible to me (did not apply
>> them locally for lack of time, though).
>
> Same here.
>
Same as well. Looks good, and good catch finding the bug!
Thanks,
Jake
>>
>> Ciao,
>> Dscho
^ permalink raw reply [flat|nested] 36+ messages in thread