Hi,
On Wed, Feb 24, 2016 at 3:27 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Wed, Feb 24, 2016 at 12:09 PM, Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
>> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>> Add a git submodule--helper function which can be used to sanitize the
>> GIT_CONFIG_PARAMETERS value to only allow certain settings. For now,
>> restrict this to only credential.* settings.
>
> I guess for now that subset is fine and will be expanded over time?
>
Yes, I figured that we would expand it as there became a reason to use
it.. If we instead blacklist things we may introduce many more issues
in the future.
>>
>> Replace all the calls to clear_local_git_env with a wrapped function
>> that filters GIT_CONFIG_PARAMETERS using the new helper and then
>> restores it to the filtered subset after clearing the rest of the
>> environment.
>
> The patch looks good to me, we should have introduced the submodule--helper
> test file earlier. I may migrate the test for the fix I send earlier
> today to that
> file eventually.
>
Ya, I didn't see any other tests for submodule--helper in master
branch yet, so I just added one.
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index f4c3eff179b5..8194d3b3d1d5 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -255,6 +255,56 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>> return 0;
>> }
>>
>
> Probably this will cause easy to resolve merge conflicts with
> origin/sb/submodule-parallel-update
>
Yea I figured there would be some conflicts, but I based it on master
so it should be relatively easy to fix things up.
>> @@ -264,6 +314,7 @@ static struct cmd_struct commands[] = {
>> {"list", module_list},
>> {"name", module_name},
>> {"clone", module_clone},
>> + {"sanitize-config", module_sanitize_config},
>
> same here.
>
>> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
>> new file mode 100755
>> index 000000000000..376f58afe967
>> --- /dev/null
>> +++ b/t/t7412-submodule--helper.sh
>
> Thanks for introducing such a file. I did not do it as I though it was
> "too small"
> and would not be enough to test to justify its own file.
>
I think that it makes more sense, since none of the other files really
fit the submodule--helper. In addition, I figure that "eventually" it
would go away after all of git-submodule.sh was re-written in C, and
git-submodule--helper is obsolete. That meant that having only a
single location for these tests would be easier to isolate and remove
them when we need to.
>> +
>> +test_expect_success 'sanitize-config clears configuration' '
>> + git -c user.name="Some User" submodule--helper sanitize-config >actual &&
>> + test_must_be_empty actual
>
> I usually keep my user.name in the global config, so no need to pass
> it around like that,
> but for testing purposes this looks good.
I just picked a simple key. This could really be anything, but I
figured that user.name is unlikely to be ever allowed to pass through
to submodules though it is probably harmless to do so. I don't believe
this is necessarily a valid case but rather just a way to make sure
that all configuration was not passed through.
Thanks,
Jake
--
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