git submodule should honor "-c credential.helper" command line argument

git submodule should honor "-c credential.helper" command line argument

[Marc Strapetz]

git -c credential.helper=helper submodule update --init submodule

does not invoke "helper", but falls back to the default strategies.
When configuring in ~/.gitconfig:

[credential]
   helper=helper

git submodule update --init submodule

works fine. This behavior is somewhat unexpected -- is this a bug or by 
intention? In case intention, what's the recommended way to "inject" 
credentials helpers to work on submodules without modifying Git's config 
files?

Tested with Git 2.5.0 (Windows).

-Marc

Re: git submodule should honor "-c credential.helper" command line argument

[Jeff King]

On Tue, Feb 02, 2016 at 06:13:14PM +0100, Marc Strapetz wrote:

> git -c credential.helper=helper submodule update --init submodule
> 
> does not invoke "helper", but falls back to the default strategies.
> When configuring in ~/.gitconfig:
> 
> [credential]
>   helper=helper
> 
> git submodule update --init submodule
> 
> works fine. This behavior is somewhat unexpected -- is this a bug or by
> intention? In case intention, what's the recommended way to "inject"
> credentials helpers to work on submodules without modifying Git's config
> files?
> 
> Tested with Git 2.5.0 (Windows).

I think the problem is that when git "switches" to working in the
submodule repository, it clears the environment, which includes any "-c"
command switches. This makes sense for some situations, but not for
others. This thread shows a similar problem:

  http://thread.gmane.org/gmane.comp.version-control.git/264840

Jens suggested there adding an option to tell clone to pass specific
variables to the submodule, which I think makes sense. AFAIK, nobody has
done any work yet on that approach.

-Peff

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Tue, Feb 2, 2016 at 8:25 PM, Jeff King <peff@peff.net> wrote:
> I think the problem is that when git "switches" to working in the
> submodule repository, it clears the environment, which includes any "-c"
> command switches. This makes sense for some situations, but not for
> others. This thread shows a similar problem:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/264840
>
> Jens suggested there adding an option to tell clone to pass specific
> variables to the submodule, which I think makes sense. AFAIK, nobody has
> done any work yet on that approach.
>

This is something that I am also interested in, haven't had a chance
to look at it just yet though. I may have some time soon to take a
stab at this.

Regards,
Jake

Re: git submodule should honor "-c credential.helper" command line argument

[Marc Strapetz]

On 03.02.2016 08:35, Jacob Keller wrote:
> On Tue, Feb 2, 2016 at 8:25 PM, Jeff King <peff@peff.net> wrote:
>> I think the problem is that when git "switches" to working in the
>> submodule repository, it clears the environment, which includes any "-c"
>> command switches. This makes sense for some situations, but not for
>> others. This thread shows a similar problem:
>>
>>    http://thread.gmane.org/gmane.comp.version-control.git/264840
>>
>> Jens suggested there adding an option to tell clone to pass specific
>> variables to the submodule, which I think makes sense. AFAIK, nobody has
>> done any work yet on that approach.
>>
>
> This is something that I am also interested in, haven't had a chance
> to look at it just yet though. I may have some time soon to take a
> stab at this.

To have this issue addressed would be great. It's currently preventing 
me to switch our authentication-related code to credential helpers.

Jake, I'm happy to help, if I can (I don't have any experience with Git 
code though, except of browsing it a couple of times ... so patches from 
my side would need a lot of review I guess :). Just let me know.

-Marc

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Tue, Feb 2, 2016 at 8:25 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 02, 2016 at 06:13:14PM +0100, Marc Strapetz wrote:
>
>> git -c credential.helper=helper submodule update --init submodule
>>
>> does not invoke "helper", but falls back to the default strategies.
>> When configuring in ~/.gitconfig:
>>
>> [credential]
>>   helper=helper
>>
>> git submodule update --init submodule
>>
>> works fine. This behavior is somewhat unexpected -- is this a bug or by
>> intention? In case intention, what's the recommended way to "inject"
>> credentials helpers to work on submodules without modifying Git's config
>> files?
>>
>> Tested with Git 2.5.0 (Windows).
>
> I think the problem is that when git "switches" to working in the
> submodule repository, it clears the environment, which includes any "-c"
> command switches. This makes sense for some situations, but not for
> others. This thread shows a similar problem:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/264840
>
> Jens suggested there adding an option to tell clone to pass specific
> variables to the submodule, which I think makes sense. AFAIK, nobody has
> done any work yet on that approach.
>
> -Peff
> --

Ok so I am not sure we even really need to use "-c" option in
git-clone considering that we can just use the same flow we do for
setting core.worktree values. I'll propose a patch with you two Cc'ed,
which I think fixes the issue. There may actually be a set of
configuration we want to include though, and the main issue I see is
that it won't get updated correctly whenever the parent configuration
changes.

Thanks,
Jake

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> Ok so I am not sure we even really need to use "-c" option in
> git-clone considering that we can just use the same flow we do for
> setting core.worktree values. I'll propose a patch with you two Cc'ed,
> which I think fixes the issue. There may actually be a set of
> configuration we want to include though, and the main issue I see is
> that it won't get updated correctly whenever the parent configuration
> changes.
>
> Thanks,
> Jake

I tried adding the config as part of module_clone in
submodule--helper.c but it didn't pass the test I added. I haven't had
time to look at this in the last few days, but I am stuck as to why
submodule--helper.c appeared to not use module_clone as I thought.

Regards,
Jake

Re: git submodule should honor "-c credential.helper" command line argument

[Marc Strapetz]

On 07.02.2016 05:41, Jacob Keller wrote:
> On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> Ok so I am not sure we even really need to use "-c" option in
>> git-clone considering that we can just use the same flow we do for
>> setting core.worktree values. I'll propose a patch with you two Cc'ed,
>> which I think fixes the issue. There may actually be a set of
>> configuration we want to include though, and the main issue I see is
>> that it won't get updated correctly whenever the parent configuration
>> changes.
>>
>> Thanks,
>> Jake
>
> I tried adding the config as part of module_clone in
> submodule--helper.c but it didn't pass the test I added. I haven't had
> time to look at this in the last few days, but I am stuck as to why
> submodule--helper.c appeared to not use module_clone as I thought.

I've tried to just comment out clearing of environment variables in 
git-sh-setup.sh, clear_local_git_env(). I've noticed that "-c 
credentials-helper ..." is stored in $GIT_CONFIG_PARAMETERS and with 
existing code is reset there. If not clearing the environment variables, 
at least "git submodule init" is working properly. I didn't try with 
other commands nor to run tests.

-Marc

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Sun, Feb 7, 2016 at 5:48 AM, Marc Strapetz <marc.strapetz@syntevo.com> wrote:
> On 07.02.2016 05:41, Jacob Keller wrote:
>>
>> On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller <jacob.keller@gmail.com>
>> wrote:
>>>
>>> Ok so I am not sure we even really need to use "-c" option in
>>> git-clone considering that we can just use the same flow we do for
>>> setting core.worktree values. I'll propose a patch with you two Cc'ed,
>>> which I think fixes the issue. There may actually be a set of
>>> configuration we want to include though, and the main issue I see is
>>> that it won't get updated correctly whenever the parent configuration
>>> changes.
>>>
>>> Thanks,
>>> Jake
>>
>>
>> I tried adding the config as part of module_clone in
>> submodule--helper.c but it didn't pass the test I added. I haven't had
>> time to look at this in the last few days, but I am stuck as to why
>> submodule--helper.c appeared to not use module_clone as I thought.
>
>
> I've tried to just comment out clearing of environment variables in
> git-sh-setup.sh, clear_local_git_env(). I've noticed that "-c
> credentials-helper ..." is stored in $GIT_CONFIG_PARAMETERS and with
> existing code is reset there. If not clearing the environment variables, at
> least "git submodule init" is working properly. I didn't try with other
> commands nor to run tests.
>
> -Marc
>
>

I'll have to dig more into this next week.

Regards,
Jake

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Sun, Feb 7, 2016 at 7:44 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sun, Feb 7, 2016 at 5:48 AM, Marc Strapetz <marc.strapetz@syntevo.com> wrote:
>> On 07.02.2016 05:41, Jacob Keller wrote:
>>>
>>> On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller <jacob.keller@gmail.com>
>>> wrote:
>>>>
>>>> Ok so I am not sure we even really need to use "-c" option in
>>>> git-clone considering that we can just use the same flow we do for
>>>> setting core.worktree values. I'll propose a patch with you two Cc'ed,
>>>> which I think fixes the issue. There may actually be a set of
>>>> configuration we want to include though, and the main issue I see is
>>>> that it won't get updated correctly whenever the parent configuration
>>>> changes.
>>>>
>>>> Thanks,
>>>> Jake
>>>
>>>
>>> I tried adding the config as part of module_clone in
>>> submodule--helper.c but it didn't pass the test I added. I haven't had
>>> time to look at this in the last few days, but I am stuck as to why
>>> submodule--helper.c appeared to not use module_clone as I thought.
>>
>>
>> I've tried to just comment out clearing of environment variables in
>> git-sh-setup.sh, clear_local_git_env(). I've noticed that "-c
>> credentials-helper ..." is stored in $GIT_CONFIG_PARAMETERS and with
>> existing code is reset there. If not clearing the environment variables, at
>> least "git submodule init" is working properly. I didn't try with other
>> commands nor to run tests.
>>
>> -Marc
>>
>>
>
> I'll have to dig more into this next week.
>
> Regards,
> Jake

I am looking at this more and I am stuck as to how best to provide a test case.

I think the problem as stated above is pretty straight forward, we
just want to stop clearing GIT_CONFIG_PARAMETERS but I can't find an
easy way to test that we've done the right thing. There are no current
tests for using a credential helper with submodule update right now.

Regards,
Jake

Re: git submodule should honor "-c credential.helper" command line argument

[Jeff King]

On Thu, Feb 18, 2016 at 05:15:54PM -0800, Jacob Keller wrote:

> I am looking at this more and I am stuck as to how best to provide a
> test case.
> 
> I think the problem as stated above is pretty straight forward, we
> just want to stop clearing GIT_CONFIG_PARAMETERS but I can't find an
> easy way to test that we've done the right thing. There are no current
> tests for using a credential helper with submodule update right now.

If you just want to test that GIT_CONFIG_PARAMETERS is left untouched in
the submodule, you can tweak any config setting that would impact the
newly-cloned repo. E.g. this:

  unset GIT_COMMITTER_NAME
  git config --global user.name='Global Name'
  git -c user.name='Command-Line Name' clone repo-with-module foo
  head -1 foo/.git/logs/HEAD
  head -1 foo/.git/modules/sub/logs/HEAD

shows that the parent-level clone uses the "-c" name, but the submodule
does not.

That being said, I am not sure this is the right solution. In the thread
I linked earlier[1], Jens indicated he would prefer not to blindly share
config with the submodules, and I think I agree. Or are you proposing to
pick and choose the keys in GIT_CONFIG_PARAMETERS, and whitelist
credential.*?

In that case, obviously my test example would not work, though I think
that it might be fine to put "user.name" onto the whitelist (the things
we really would worry about is stuff like "core.worktree" that clearly
does not make sense to carry over into the submodule).

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/264840

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Thu, Feb 18, 2016 at 8:30 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 18, 2016 at 05:15:54PM -0800, Jacob Keller wrote:
>
>> I am looking at this more and I am stuck as to how best to provide a
>> test case.
>>
>> I think the problem as stated above is pretty straight forward, we
>> just want to stop clearing GIT_CONFIG_PARAMETERS but I can't find an
>> easy way to test that we've done the right thing. There are no current
>> tests for using a credential helper with submodule update right now.
>
> If you just want to test that GIT_CONFIG_PARAMETERS is left untouched in
> the submodule, you can tweak any config setting that would impact the
> newly-cloned repo. E.g. this:
>
>   unset GIT_COMMITTER_NAME
>   git config --global user.name='Global Name'
>   git -c user.name='Command-Line Name' clone repo-with-module foo
>   head -1 foo/.git/logs/HEAD
>   head -1 foo/.git/modules/sub/logs/HEAD
>
> shows that the parent-level clone uses the "-c" name, but the submodule
> does not.
>
> That being said, I am not sure this is the right solution. In the thread
> I linked earlier[1], Jens indicated he would prefer not to blindly share
> config with the submodules, and I think I agree. Or are you proposing to
> pick and choose the keys in GIT_CONFIG_PARAMETERS, and whitelist
> credential.*?
>
> In that case, obviously my test example would not work, though I think
> that it might be fine to put "user.name" onto the whitelist (the things
> we really would worry about is stuff like "core.worktree" that clearly
> does not make sense to carry over into the submodule).
>
> -Peff
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/264840
>

I would prefer to either.. blacklist stuff like core.worktree, or
whitelist a bunch of stuff that makes sense. In this case though, I
would prefer to have an explicit test of credential.helper, but I
don't know if any of our tests actually have a solid test case for
"credential.helper was used in a clone. There may not be test
infrastructure for this though, so your test might work well enough.

As for how to whitelist config to share with the submodule I am really
not 100% sure, since we just clear GIT_CONFIG_PARAMETERS, and I think
we'd need a specialized variant of clear_local_git_env_vars specific
to submodule then.

Thanks,
Jake

Re: git submodule should honor "-c credential.helper" command line argument

[Jeff King]

On Thu, Feb 18, 2016 at 11:29:09PM -0800, Jacob Keller wrote:

> I would prefer to either.. blacklist stuff like core.worktree, or
> whitelist a bunch of stuff that makes sense. In this case though, I
> would prefer to have an explicit test of credential.helper, but I
> don't know if any of our tests actually have a solid test case for
> "credential.helper was used in a clone. There may not be test
> infrastructure for this though, so your test might work well enough.

To trigger a credential fetch in actual use, you have to clone over
http. See the credential tests in t5550, for example.

> As for how to whitelist config to share with the submodule I am really
> not 100% sure, since we just clear GIT_CONFIG_PARAMETERS, and I think
> we'd need a specialized variant of clear_local_git_env_vars specific
> to submodule then.

Yeah, you'll have to parse, which is pretty painful. In C, you'd do
something like:

  int submodule_config_ok(const char *var)
  {
	if (starts_with(var, "credential."))
		return 1;
	return 0;
  }

  int filter_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, ' ');
		/* these actually probably need quoted all as * one string */
		sq_quote_buf(out, var);
		sq_quote_buf(out, "=");
		sq_quote_buf(out, value);
	}
	return 0;
  }

and then call it like:

  struct strbuf filtered_config = STRBUF_INIT;
  git_config_from_parameters(filter_submodule_config, &filtered_config);
  argv_array_pushf(&child_process.env, "%s=%s",
                   CONFIG_DATA_ENVIRONMENT, filtered_config.buf);

but right now git-submodule.sh is all in shell. You'd probably need a
special helper from git-submodule--helper, though it might simply make
sense to put this off until the submodule code is fully ported to C.

-Peff

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Thu, Feb 18, 2016 at 11:46 PM, Jeff King <peff@peff.net> wrote:
> To trigger a credential fetch in actual use, you have to clone over
> http. See the credential tests in t5550, for example.
>

I'll look at these.

>> As for how to whitelist config to share with the submodule I am really
>> not 100% sure, since we just clear GIT_CONFIG_PARAMETERS, and I think
>> we'd need a specialized variant of clear_local_git_env_vars specific
>> to submodule then.
>
> Yeah, you'll have to parse, which is pretty painful. In C, you'd do
> something like:

<snip>

>
> but right now git-submodule.sh is all in shell. You'd probably need a
> special helper from git-submodule--helper, though it might simply make
> sense to put this off until the submodule code is fully ported to C.
>

I think the best approach is probably to put this off since I am not
100% sure how we'd handle environment variables from swapping into
submoduler--helper and out. That seems really finicky and likely to go
away once the full port occurs so I am not sure it's worth it.

> -Peff

Re: git submodule should honor "-c credential.helper" command line argument

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That being said, I am not sure this is the right solution. In the thread
> I linked earlier[1], Jens indicated he would prefer not to blindly share
> config with the submodules, and I think I agree. Or are you proposing to
> pick and choose the keys in GIT_CONFIG_PARAMETERS, and whitelist
> credential.*?

Yes, I think it is sensible not to propagate by default, and pass
ones that are absolutely safe, sane and necessary by whitelisting.

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Fri, Feb 19, 2016 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> That being said, I am not sure this is the right solution. In the thread
>> I linked earlier[1], Jens indicated he would prefer not to blindly share
>> config with the submodules, and I think I agree. Or are you proposing to
>> pick and choose the keys in GIT_CONFIG_PARAMETERS, and whitelist
>> credential.*?
>
> Yes, I think it is sensible not to propagate by default, and pass
> ones that are absolutely safe, sane and necessary by whitelisting.

Yes I agree. I'm trying to think of a reasonable way to implement this
as part of a submodule--helper.. something like whitelist-config or
something, which would just output the value of GIT_CONFIG_PARAMETERS
after whitelisting. It would be up to the shell code to then use this
inside a subshell.

Regards,
Jake