Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Junio C Hamano]

Jonathan Nieder  writes:

> I should have mentioned this is
>
> Reported-by: Shin Fan 
>
>> Signed-off-by: Jeff King 
>> ---
>> I just did this on master, and it is standalone. But for the reasons
>> above I think it would also be fine to stick on the tip of the
>> jk/submodule-c-credential topic.
>>
>>  config.c   |  2 +-
>>  t/t1300-repo-config.sh | 14 ++
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder 
> Tested-by: Jonathan Nieder 
>
> Thanks for the quick fix.
>
> Sincerely,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551

Thanks for keeping an eye on the 'next' front.

It is much more pleasant to find and fix problems before a new topic
hits 'master', and I wish there were more people like you who use
Git that is newer than 'master' regularly and report issues.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Jacob Keller]

On Tue, Mar 22, 2016 at 2:43 PM, Jonathan Nieder  wrote:
> Jeff King wrote[1]:
>
>> Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
>>
>> The "git -c var=value" option stuffs the config value into
>> $GIT_CONFIG_PARAMETERS, so that sub-processes can see it.
>> When the config is later read via git_config() or similar,
>> we parse it back out of that variable.  The parsing end is a
>> little bit picky; it assumes that each entry was generated
>> with sq_quote_buf(), and that there is no extraneous
>> whitespace.
>>
>> On the generating end, we are careful to append to an
>> existing $GIT_CONFIG_PARAMETERS variable if it exists.
>> However, our test for "should we add a space separator" is
>> too liberal: it will add one even if the environment
>> variable exists but is empty. As a result, you might end up
>> with:
>>
>>GIT_CONFIG_PARAMETERS=" 'core.foo=bar'"
>>
>> which the parser will choke on.
>>
>> This was hard to trigger in older versions of git, since we
>> only set the variable when we had something to put into it
>> (though you could certainly trigger it manually). But since
>> 14111fc (git: submodule honor -c credential.* from command
>> line, 2016-02-29), the submodule code will unconditionally
>> put the $GIT_CONFIG_PARAMETERS variable into the environment
>> of any operation in the submodule, whether it is empty or
>> not. So any of those operations which themselves use "git
>> -c" will generate the unparseable value and fail.
>>
>> We can easily fix it by catching this case on the generating
>> side. While we're adding a test, let's also check that
>> multiple layers of "git -c" work, which was previously not
>> tested at all.
>>
>> Reported-by: Jonathan Nieder 
>
> I should have mentioned this is
>
> Reported-by: Shin Fan 
>
>> Signed-off-by: Jeff King 
>> ---
>> I just did this on master, and it is standalone. But for the reasons
>> above I think it would also be fine to stick on the tip of the
>> jk/submodule-c-credential topic.
>>
>>  config.c   |  2 +-
>>  t/t1300-repo-config.sh | 14 ++
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder 
> Tested-by: Jonathan Nieder 
>
> Thanks for the quick fix.
>
> Sincerely,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551


Yep, thanks Jeff. This looked fine to me.

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Jonathan Nieder]

Jeff King wrote[1]:

> Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
>
> The "git -c var=value" option stuffs the config value into
> $GIT_CONFIG_PARAMETERS, so that sub-processes can see it.
> When the config is later read via git_config() or similar,
> we parse it back out of that variable.  The parsing end is a
> little bit picky; it assumes that each entry was generated
> with sq_quote_buf(), and that there is no extraneous
> whitespace.
>
> On the generating end, we are careful to append to an
> existing $GIT_CONFIG_PARAMETERS variable if it exists.
> However, our test for "should we add a space separator" is
> too liberal: it will add one even if the environment
> variable exists but is empty. As a result, you might end up
> with:
>
>GIT_CONFIG_PARAMETERS=" 'core.foo=bar'"
>
> which the parser will choke on.
>
> This was hard to trigger in older versions of git, since we
> only set the variable when we had something to put into it
> (though you could certainly trigger it manually). But since
> 14111fc (git: submodule honor -c credential.* from command
> line, 2016-02-29), the submodule code will unconditionally
> put the $GIT_CONFIG_PARAMETERS variable into the environment
> of any operation in the submodule, whether it is empty or
> not. So any of those operations which themselves use "git
> -c" will generate the unparseable value and fail.
>
> We can easily fix it by catching this case on the generating
> side. While we're adding a test, let's also check that
> multiple layers of "git -c" work, which was previously not
> tested at all.
>
> Reported-by: Jonathan Nieder 

I should have mentioned this is

Reported-by: Shin Fan 

> Signed-off-by: Jeff King 
> ---
> I just did this on master, and it is standalone. But for the reasons
> above I think it would also be fine to stick on the tip of the
> jk/submodule-c-credential topic.
>
>  config.c   |  2 +-
>  t/t1300-repo-config.sh | 14 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder 
Tested-by: Jonathan Nieder 

Thanks for the quick fix.

Sincerely,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Junio C Hamano]

Jeff King  writes:

> Obviously another option would be to make the parsing side more liberal
> (which has the added effect that if anybody _else_ ever wants to
> generate $GIT_CONFIG_PARAMETERS, they will not get annoyed). But I took
> this route for now because it's the simplest way to fix the regression.
> And even if we do later make the parser more liberal, it's still a good
> idea to keep the output from the generating side as clean as possible.

Sounds sensible.  Will queue.  Thanks.  

> I just did this on master, and it is standalone. But for the reasons
> above I think it would also be fine to stick on the tip of the
> jk/submodule-c-credential topic.

Yup, I think that is fine.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Jeff King]

On Tue, Mar 22, 2016 at 03:23:09PM -0400, Jeff King wrote:

> On Tue, Mar 22, 2016 at 11:56:28AM -0700, Jonathan Nieder wrote:
> 
> > This is failing for me when I use "git submodule add" with a remote
> > helper I whitelisted with GIT_ALLOW_PROTOCOL, with current "next":
> > 
> >  $ bin-wrappers/git submodule add 
> > persistent-https://kernel.googlesource.com/pub/scm/git/git sm
> >  Cloning into 'sm'...
> >  error: bogus format in GIT_CONFIG_PARAMETERS
> >  fatal: unable to parse command-line config
> >  fatal: clone of 
> > 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule 
> > path 'sm' failed
> > 
> > sq_dequote_to_argv doesn't like the space at the beginning of
> > $GIT_CONFIG_PARAMETERS.  Reverting 14111fc4 fixes it.  Known
> > problem?
> 
> It's known that the parsing end is excessively picky, but not this
> particular bug. I found the problem; I'll have a patch out in a few
> minute after I write a test.

Here it is.

Obviously another option would be to make the parsing side more liberal
(which has the added effect that if anybody _else_ ever wants to
generate $GIT_CONFIG_PARAMETERS, they will not get annoyed). But I took
this route for now because it's the simplest way to fix the regression.
And even if we do later make the parser more liberal, it's still a good
idea to keep the output from the generating side as clean as possible.

-- >8 --
Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

The "git -c var=value" option stuffs the config value into
$GIT_CONFIG_PARAMETERS, so that sub-processes can see it.
When the config is later read via git_config() or similar,
we parse it back out of that variable.  The parsing end is a
little bit picky; it assumes that each entry was generated
with sq_quote_buf(), and that there is no extraneous
whitespace.

On the generating end, we are careful to append to an
existing $GIT_CONFIG_PARAMETERS variable if it exists.
However, our test for "should we add a space separator" is
too liberal: it will add one even if the environment
variable exists but is empty. As a result, you might end up
with:

   GIT_CONFIG_PARAMETERS=" 'core.foo=bar'"

which the parser will choke on.

This was hard to trigger in older versions of git, since we
only set the variable when we had something to put into it
(though you could certainly trigger it manually). But since
14111fc (git: submodule honor -c credential.* from command
line, 2016-02-29), the submodule code will unconditionally
put the $GIT_CONFIG_PARAMETERS variable into the environment
of any operation in the submodule, whether it is empty or
not. So any of those operations which themselves use "git
-c" will generate the unparseable value and fail.

We can easily fix it by catching this case on the generating
side. While we're adding a test, let's also check that
multiple layers of "git -c" work, which was previously not
tested at all.

Reported-by: Jonathan Nieder 
Signed-off-by: Jeff King 
---
I just did this on master, and it is standalone. But for the reasons
above I think it would also be fine to stick on the tip of the
jk/submodule-c-credential topic.

 config.c   |  2 +-
 t/t1300-repo-config.sh | 14 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 9ba40bc..8f66519 100644
--- a/config.c
+++ b/config.c
@@ -162,7 +162,7 @@ void git_config_push_parameter(const char *text)
 {
struct strbuf env = STRBUF_INIT;
const char *old = getenv(CONFIG_DATA_ENVIRONMENT);
-   if (old) {
+   if (old && *old) {
strbuf_addstr(, old);
strbuf_addch(, ' ');
}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..2b58312 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1091,6 +1091,20 @@ test_expect_success 'git -c complains about empty key 
and value' '
test_must_fail git -c "" rev-parse
 '
 
+test_expect_success 'multiple git -c appends config' '
+   test_config alias.x "!git -c x.two=2 config --get-regexp ^x\.*" &&
+   cat >expect <<-\EOF &&
+   x.one 1
+   x.two 2
+   EOF
+   git -c x.one=1 x >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'git -c is not confused by empty environment' '
+   GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
+'
+
 test_expect_success 'git config --edit works' '
git config -f tmp test.value no &&
echo test.value=yes >expect &&
-- 
2.8.0.rc4.388.gdee5eca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html