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

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

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

On Tue, Feb 2, 2016 at 8:25 PM, Jeff King [off-list ref] 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

On 03.02.2016 08:35, Jacob Keller wrote:

On Tue, Feb 2, 2016 at 8:25 PM, Jeff King [off-list ref] wrote:

quoted

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

On Tue, Feb 2, 2016 at 8:25 PM, Jeff King [off-list ref] wrote:

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

quoted

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

On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller [off-list ref] 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

On 07.02.2016 05:41, Jacob Keller wrote:

On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller [off-list ref] wrote:

quoted

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

On Sun, Feb 7, 2016 at 5:48 AM, Marc Strapetz [off-list ref] wrote:

On 07.02.2016 05:41, Jacob Keller wrote:

quoted

On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller [off-list ref]
wrote:

quoted

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

On Sun, Feb 7, 2016 at 7:44 PM, Jacob Keller [off-list ref] wrote:

On Sun, Feb 7, 2016 at 5:48 AM, Marc Strapetz [off-list ref] wrote:

quoted

On 07.02.2016 05:41, Jacob Keller wrote:

quoted

On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller [off-list ref]
wrote:

quoted

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

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

On Thu, Feb 18, 2016 at 8:30 PM, Jeff King [off-list ref] wrote:

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

quoted

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

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

On Thu, Feb 18, 2016 at 11:46 PM, Jeff King [off-list ref] 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.

quoted

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