Re: [PATCH] git: submodule honor -c credential.* from command line

From: Jacob Keller <jacob.keller@gmail.com>
Date: Feb 24, 2016, 23:50 UTC
Message-ID: <CA+P7+xp=EOUHbn5hp0YijAW6sfmZeiD2yp72jVero4_WB-skkg@mail.gmail.com>
In-Reply-To: <CAGZ79kbSd4TRdgdcj2NUrc+pL7ATrGQNcfTYd57TuJtyZ5QBwg@mail.gmail.com>

Hi,

On Wed, Feb 24, 2016 at 3:27 PM, Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines

> On Wed, Feb 24, 2016 at 12:09 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>> 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.

Show 11 quoted lines

>>
>> 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.

Show 12 quoted lines

>> 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.

Show 18 quoted lines

>> @@ -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.

Show 8 quoted lines

>> +
>> +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