Re: [PATCH] submodule: stop sanitizing config options

2016-05-06 Thread Junio C Hamano 
Stefan Beller  writes:

>> Hmph, Stefan, do you want to keep this (if you want to resurrect the
>> function in some future, for example)?
>
> IMHO it is easier to revert or rewrite than to carry unused code?
> Unused code is not tested and untested code is broken code.
> And relying on broken code in the future will guarantee bugs.
> (Sure new code may also have bugs, but I just think that bugs
> in newly written code can be fixed easier)
>
> I would prefer to get rid of it, i.e. taking that patch.

OK.
--
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] submodule: stop sanitizing config options

2016-05-05 Thread Jeff King 
On Thu, May 05, 2016 at 05:23:51PM -0700, Stefan Beller wrote:

> >> -/*
> >> - * This function is intended as a callback for use with
> >> - * git_config_from_parameters(). It ignores any config options which
> >> - * are not suitable for passing along to a submodule, and accumulates the 
> >> rest
> >> - * in "data", which must be a pointer to a strbuf. The end result can
> >> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
> >> - */
> >> -int sanitize_submodule_config(const char *var, const char *value, void 
> >> *data);
> >> -
> >>  /*
> >>   * Prepare the "env_array" parameter of a "struct child_process" for 
> >> executing
> >>   * a submodule by clearing any repo-specific envirionment variables, but
> >> - * retaining any config approved by sanitize_submodule_config().
> >> + * retaining any config in the environment.
> >>   */
> >>  void prepare_submodule_repo_env(struct argv_array *out);
> >>
> >>
> >> -Peff
> >
> > Hmph, Stefan, do you want to keep this (if you want to resurrect the
> > function in some future, for example)?
> 
> IMHO it is easier to revert or rewrite than to carry unused code?
> Unused code is not tested and untested code is broken code.
> And relying on broken code in the future will guarantee bugs.
> (Sure new code may also have bugs, but I just think that bugs
> in newly written code can be fixed easier)
> 
> I would prefer to get rid of it, i.e. taking that patch.

Keep in mind this squash is just dropping the _declaration_. The code
itself was dropped in the earlier patch. (And I agree there isn't a good
reason to keep it when we can easily "git revert" later).

-Peff
--
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] submodule: stop sanitizing config options

2016-05-05 Thread Stefan Beller 
On Thu, May 5, 2016 at 4:33 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> I had originally thought after the final one that we could further clean
>> up by turning prepare_submodule_repo_env() into a static function. But
>> we can't; it gets called in one spot from submodule--helper. However,
>> while looking at it, I did notice that we probably want to squash this
>> into the final patch (since sanitize_submodule_config went away
>> completely):
>>
>> diff --git a/submodule.h b/submodule.h
>> index 48690b1..869d259 100644
>> --- a/submodule.h
>> +++ b/submodule.h
>> @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
>> const char *remotes_nam
>>  int push_unpushed_submodules(unsigned char new_sha1[20], const char 
>> *remotes_name);
>>  void connect_work_tree_and_git_dir(const char *work_tree, const char 
>> *git_dir);
>>
>> -/*
>> - * This function is intended as a callback for use with
>> - * git_config_from_parameters(). It ignores any config options which
>> - * are not suitable for passing along to a submodule, and accumulates the 
>> rest
>> - * in "data", which must be a pointer to a strbuf. The end result can
>> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
>> - */
>> -int sanitize_submodule_config(const char *var, const char *value, void 
>> *data);
>> -
>>  /*
>>   * Prepare the "env_array" parameter of a "struct child_process" for 
>> executing
>>   * a submodule by clearing any repo-specific envirionment variables, but
>> - * retaining any config approved by sanitize_submodule_config().
>> + * retaining any config in the environment.
>>   */
>>  void prepare_submodule_repo_env(struct argv_array *out);
>>
>>
>> -Peff
>
> Hmph, Stefan, do you want to keep this (if you want to resurrect the
> function in some future, for example)?

IMHO it is easier to revert or rewrite than to carry unused code?
Unused code is not tested and untested code is broken code.
And relying on broken code in the future will guarantee bugs.
(Sure new code may also have bugs, but I just think that bugs
in newly written code can be fixed easier)

I would prefer to get rid of it, i.e. taking that patch.

Thanks,
Stefan
--
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] submodule: stop sanitizing config options

2016-05-05 Thread Junio C Hamano 
Jeff King  writes:

> I had originally thought after the final one that we could further clean
> up by turning prepare_submodule_repo_env() into a static function. But
> we can't; it gets called in one spot from submodule--helper. However,
> while looking at it, I did notice that we probably want to squash this
> into the final patch (since sanitize_submodule_config went away
> completely):
>
> diff --git a/submodule.h b/submodule.h
> index 48690b1..869d259 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
> const char *remotes_nam
>  int push_unpushed_submodules(unsigned char new_sha1[20], const char 
> *remotes_name);
>  void connect_work_tree_and_git_dir(const char *work_tree, const char 
> *git_dir);
>  
> -/*
> - * This function is intended as a callback for use with
> - * git_config_from_parameters(). It ignores any config options which
> - * are not suitable for passing along to a submodule, and accumulates the 
> rest
> - * in "data", which must be a pointer to a strbuf. The end result can
> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
> - */
> -int sanitize_submodule_config(const char *var, const char *value, void 
> *data);
> -
>  /*
>   * Prepare the "env_array" parameter of a "struct child_process" for 
> executing
>   * a submodule by clearing any repo-specific envirionment variables, but
> - * retaining any config approved by sanitize_submodule_config().
> + * retaining any config in the environment.
>   */
>  void prepare_submodule_repo_env(struct argv_array *out);
>  
>
> -Peff

Hmph, Stefan, do you want to keep this (if you want to resurrect the
function in some future, for example)?
--
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] submodule: stop sanitizing config options

2016-05-05 Thread Jeff King 
On Thu, May 05, 2016 at 09:59:15AM -0700, Junio C Hamano wrote:

> Sorry for the trouble.
> 
> I queued jk/submodule-c-credential to be mergeable to 'maint', as it
> could be argued two ways.  We can say that not propagating -c down
> is a bug and the series is a bugfix.  Or it is merely a known
> limitation and the series is a new feature.  I was leaning towards
> the former, but I was also willing to declare that is a known bug
> that will left unfixed in the maintenance track.

Ah, OK, that makes perfect sense.

> It probably is a good time to merge jk/submodule-config-sanitize-fix
> into jk/submodule-c-credential (i.e. a mere fast-forward), remove
> that "-fix" branch, and apply this patch directly on top of the
> resulting jk/submodule-c-credential.  That will make the whole thing
> a 13-patch series, consisting of:
> 
>  7 patches up to the current jk/submodule-c-credential
>   d1f8849 git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
>   14111fc git: submodule honor -c credential.* from command line
>   e70986d quote: implement sq_quotef()
>   7dad263 submodule: fix segmentation fault in submodule--helper clone
>   717416c submodule: fix submodule--helper clone usage
>   08e0970 submodule: check argc count for git submodule--helper clone
>   d10e3b4 submodule: don't pass empty string arguments to submodule--helper 
> clone
>  
>  5 patches up to the current jk/submodule-config-sanitize-fix
>   c12e865 submodule: use prepare_submodule_repo_env consistently
>   4638728 submodule--helper: move config-sanitizing to submodule.c
>   860cba6 submodule: export sanitized GIT_CONFIG_PARAMETERS
>   455d22c t5550: break submodule config test into multiple sub-tests
>   1149ee2 t5550: fix typo in $HTTPD_URL
> 
>  1 patch (this one)
>   4e6706a submodule: stop sanitizing config options

That sounds reasonable. Note that the later patches drop the only caller
of the newly-introduced sq_quotef(). So we could revert e70986d
(omitting it from the series doesn't make sense, as it would leave a
broken state in the middle). I am also fine with leaving it. It seems
like a potentially useful addition.

I had originally thought after the final one that we could further clean
up by turning prepare_submodule_repo_env() into a static function. But
we can't; it gets called in one spot from submodule--helper. However,
while looking at it, I did notice that we probably want to squash this
into the final patch (since sanitize_submodule_config went away
completely):

diff --git a/submodule.h b/submodule.h
index 48690b1..869d259 100644
--- a/submodule.h
+++ b/submodule.h
@@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
const char *remotes_nam
 int push_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 
-/*
- * This function is intended as a callback for use with
- * git_config_from_parameters(). It ignores any config options which
- * are not suitable for passing along to a submodule, and accumulates the rest
- * in "data", which must be a pointer to a strbuf. The end result can
- * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
- */
-int sanitize_submodule_config(const char *var, const char *value, void *data);
-
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific envirionment variables, but
- * retaining any config approved by sanitize_submodule_config().
+ * retaining any config in the environment.
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 

-Peff
--
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] submodule: stop sanitizing config options

2016-05-05 Thread Junio C Hamano 
Jeff King <p...@peff.net> writes:

> Here's a version of my patch that should apply for you (no
> semantic changes, just differences in the surrounding context):

Sorry for the trouble.

I queued jk/submodule-c-credential to be mergeable to 'maint', as it
could be argued two ways.  We can say that not propagating -c down
is a bug and the series is a bugfix.  Or it is merely a known
limitation and the series is a new feature.  I was leaning towards
the former, but I was also willing to declare that is a known bug
that will left unfixed in the maintenance track.

If I knew the 5 patches on jk/submodule-config-sanitize-fix was what
we would eventually agree to be the right change, I would have
applied them directly on top of jk/submodule-c-credential instead of
forking a separate branch for them.  That way, either the "bugfix"
would all go to 'maint', or the "feature" wouldn't, and we do not
have to worry about making a mistake to merge only half-done state
(i.e. jk/submodule-c-credential that passes only the credential.*
configuration) that nobody would want to 'maint'.

But when I picked up jk/submodule-config-sanitize-fix, I didn't have
enough time to think things through or carefully read the discussion
to convince myself that we already had a list concensus, so I queued
it separately only to make sure I won't lose track of it--I can come
back to it later that way.

It probably is a good time to merge jk/submodule-config-sanitize-fix
into jk/submodule-c-credential (i.e. a mere fast-forward), remove
that "-fix" branch, and apply this patch directly on top of the
resulting jk/submodule-c-credential.  That will make the whole thing
a 13-patch series, consisting of:

 7 patches up to the current jk/submodule-c-credential
  d1f8849 git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
  14111fc git: submodule honor -c credential.* from command line
  e70986d quote: implement sq_quotef()
  7dad263 submodule: fix segmentation fault in submodule--helper clone
  717416c submodule: fix submodule--helper clone usage
  08e0970 submodule: check argc count for git submodule--helper clone
  d10e3b4 submodule: don't pass empty string arguments to submodule--helper 
clone
 
 5 patches up to the current jk/submodule-config-sanitize-fix
  c12e865 submodule: use prepare_submodule_repo_env consistently
  4638728 submodule--helper: move config-sanitizing to submodule.c
  860cba6 submodule: export sanitized GIT_CONFIG_PARAMETERS
  455d22c t5550: break submodule config test into multiple sub-tests
  1149ee2 t5550: fix typo in $HTTPD_URL

 1 patch (this one)
  4e6706a submodule: stop sanitizing config options

whose early 7 patches have already been merged to 'master' (and none
has been merged to 'maint').

> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
>   1. It prevented obvious nonsense like using core.worktree
>  for multiple repos.
>
>   2. It could prevent surprise when the user did not mean
>  for the options to leak to the submodules (e.g.,
>  http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.
>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).
>
> We can do this easily from within the submodule shell
> script, which lets us drop the submodule--helper option
> entirely (and it's OK to do so because as a "--" program, it
> is entirely a private implementation detail).
>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
>  builtin/submodule--helper.c  | 17 -
>  git-submodule.sh |  4 ++--
>  submodule.c  | 39 +--
>  t/t7412-submodule--helper.sh | 26 --
>  4 files changed, 3 insertions(+), 83 deletions(-)
>  delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 16d6432..89250f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -260,22 +260,6 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
>

Re: [PATCH] submodule: stop sanitizing config options

2016-05-04 Thread Jeff King 
On Wed, May 04, 2016 at 03:53:26PM -0700, Stefan Beller wrote:

> > I think we'd actually do it all in one, and that patch looks something
> > like the one below (on top of jk/submodule-config-sanitize-fix).
> 
> $ git checkout origin/jk/submodule-config-sanitize-fix
> $ git am p
> Applying: submodule: stop sanitizing config options
> error: patch failed: builtin/submodule--helper.c:246
> error: builtin/submodule--helper.c: patch does not apply
> error: patch failed: submodule.c:1131
> error: submodule.c: patch does not apply
> Patch failed at 0001 submodule: stop sanitizing config options
> 
> So if you want some documentation on top of that, where would I base it on?

I build the patches for jk/submodule-config-sanitize-fix on top of
master as of the other day, and then built this most recent patch on top
of that.

Looks like Junio applied them directly on the tip of
jk/submodule-c-credential, and had to wiggle the code in submodule.c,
which conflicted with the parallel-process stuff that was merged in
between. Since the new patch updates that code, it will likewise run
into conflicts.

I don't think there's a strict right answer here; if the original buggy
submodule-c-credential code had been released, we would definitely want
to build off of it for "maint" releases. But it wasn't, so master is
"just as good" in a sense. But I think Junio makes it a habit to apply
fixes as far back as the introduced bug, even when it's not going to
maint.

So since that's what published, it makes sense to build on that. Here's
a version of my patch that should apply for you (no semantic changes,
just differences in the surrounding context):

-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options

The point of having a whitelist of command-line config
options to pass to submodules was two-fold:

  1. It prevented obvious nonsense like using core.worktree
 for multiple repos.

  2. It could prevent surprise when the user did not mean
 for the options to leak to the submodules (e.g.,
 http.sslverify=false).

For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).

So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.

Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).

We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).

Signed-off-by: Jeff King <p...@peff.net>
---
 builtin/submodule--helper.c  | 17 -
 git-submodule.sh |  4 ++--
 submodule.c  | 39 +--
 t/t7412-submodule--helper.sh | 26 --
 4 files changed, 3 insertions(+), 83 deletions(-)
 delete mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d6432..89250f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -260,22 +260,6 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static int module_sanitize_config(int argc, const char **argv, const char 
*prefix)
-{
-   struct strbuf sanitized_config = STRBUF_INIT;
-
-   if (argc > 1)
-   usage(_("git submodule--helper sanitize-config"));
-
-   git_config_from_parameters(sanitize_submodule_config, 
_config);
-   if (sanitized_config.len)
-   printf("%s\n", sanitized_config.buf);
-
-   strbuf_release(_config);
-
-   return 0;
-}
-
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
@@ -285,7 +269,6 @@ static struct cmd_struct commands[] = {
{"list", module_list},
{"name", module_name},
{"clone", module_clone},
-   {"sanitize-config", module_sanitize_config},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 91f5856..b1c056c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -197,9 +197,9 @@ isnumber()
 # of the settings from GIT_CONFIG_PARAMETERS.
 sanitize_submodule_env()
 {
-   sanitized_config=$(git submodule--helper sanitize-config)
+   save_config=$GIT_CONFIG_PARAMETERS
clear_local_git_env
-   GIT_CONFIG_PARAMETERS=$sanitized_config
+   GI

Re: [PATCH] submodule: stop sanitizing config options

2016-05-04 Thread Stefan Beller 
> I don't think there was any documentation for the _old_ behavior, and
> certainly jk/submodule-c-credential didn't add any. But it probably is
> worth document, maybe as part of "-c"? Care to roll a patch on top?

Sure.

>
> I think we'd actually do it all in one, and that patch looks something
> like the one below (on top of jk/submodule-config-sanitize-fix).

$ git checkout origin/jk/submodule-config-sanitize-fix
$ git am p
Applying: submodule: stop sanitizing config options
error: patch failed: builtin/submodule--helper.c:246
error: builtin/submodule--helper.c: patch does not apply
error: patch failed: submodule.c:1131
error: submodule.c: patch does not apply
Patch failed at 0001 submodule: stop sanitizing config options

So if you want some documentation on top of that, where would I base it on?
--
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] submodule: stop sanitizing config options

2016-05-04 Thread Jeff King 
On Wed, May 04, 2016 at 11:43:47AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 3a40d4b..c9d53e1 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -197,9 +197,9 @@ isnumber()
> >  # of the settings from GIT_CONFIG_PARAMETERS.
> >  sanitize_submodule_env()
> >  {
> > -   sanitized_config=$(git submodule--helper sanitize-config)
> > +   save_config=$GIT_CONFIG_PARAMETERS
> > clear_local_git_env
> > -   GIT_CONFIG_PARAMETERS=$sanitized_config
> > +   GIT_CONFIG_PARAMETERS=$save_config
> > export GIT_CONFIG_PARAMETERS
> >  }
> 
> This does "clear the obviously per-repository stuff, but add back in
> anything that came from -c".  If it is easy to do "add anything that
> came from -c, and then clear the obviously per-repository stuff", we
> don't even have to say "exporting core.worktree down may hurt; do
> not do it then", which may be the best of both worlds?

It's not easy at this level. clear_local_git_env() is just clearing
whole variables like $GIT_WORK_TREE. But if you say "-c core.worktree",
that is still live in $GIT_CONFIG_PARAMETERS, and gets fed to
sub-processes on each invocation. Nobody ever removes individual content
from $GIT_CONFIG_PARAMETERS once something is in it.

> Or have we decided that even sharing core.worktree may have a valid
> use case and it is better not to filter them?

I do not personally consider it a valid use case. The sentiment just
seemed to be that it was better to make the rule simple and easy to
explain ("everything gets passed through; if it hurts, don't do it")
than to keep sanitizing.

If you want "pass through everything except core.worktree", then that is
the blacklist approach (which is easy to do if we scrap this patch and
just teak submodule_config_ok() from a whitelist into a blacklist).

-Peff
--
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] submodule: stop sanitizing config options

2016-05-04 Thread Jeff King 
On Wed, May 04, 2016 at 10:58:26AM -0700, Stefan Beller wrote:

> > So this whitelist is probably not giving us any benefit, and
> > is already creating a hassle as people propose things to put
> > on it. Let's just drop it entirely.
> 
> Just to recap:
> Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
> we passed nothing down to the commands operating on submodules.
> 
> Then we decided to pass on some of it based on a curated list.
> 
> Curating the list is too hard, so we pass on everything now, because
> it is easy to maintain and easy to explain. And when the user is hurt,
> they're holding it wrong?

Yes, I think that sums it up (the last paragraph is what's under
discussion, so it's up for debate).

> > Note that we still need to keep a special code path for
> > "prepare the submodule environment", because we still have
> > to take care to pass through $GIT_CONFIG_PARAMETERS (and
> > block the rest of the repo-specific environment variables).
> 
> So when running `git -c foo=bar command --recurse-submodules`
> the `-c` parsing calls git_config_push_parameter, which
> exports that string `foo=baz` into the environment variable
> GIT_CONFIG_PARAMETERS.
> 
> When the submodule command is called, sanitize_submodule_env
> just wipes all the Git related configurations except those in
> GIT_CONFIG_PARAMETERS as they are set again after
> clear_local_git_env wiped it.

Right.

> I wonder about the implementation detail, if we rather want to introduce
> a `git rev-parse --repo-only-local-env-vars` which is
> `git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
> such that clear_local_git_env does the right thing and we don't
> have to have 2 functions for it (i.e. clear_local_git_env and
> sanitize_submodule_env, which is the newer not as strict version of it)

I don't think that really buys you much. The policy is technically
duplicated in prepare_submodule_repo_env() in submodule.c and
sanitize_submodule_env in submodule.h. But without the whitelist, it's
such a simple policy that I'm not sure it's worth the boilerplate of
having the shell function call into the C code.

If we did want to go that route, the more appropriate interface would
probably be:

  git submodule--helper sanitize-env

or something. That avoids making it a public interface, and makes it
clear that we are invoking the submodule rules.

> Do we want to add documentation for the new behavior though?
> Before: pass not -c arguments to submodules
> Now: Pass all the -c arguments to submodules

I don't think there was any documentation for the _old_ behavior, and
certainly jk/submodule-c-credential didn't add any. But it probably is
worth document, maybe as part of "-c"? Care to roll a patch on top?

-Peff
--
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] submodule: stop sanitizing config options

2016-05-04 Thread Junio C Hamano 
Jeff King  writes:

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3a40d4b..c9d53e1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -197,9 +197,9 @@ isnumber()
>  # of the settings from GIT_CONFIG_PARAMETERS.
>  sanitize_submodule_env()
>  {
> - sanitized_config=$(git submodule--helper sanitize-config)
> + save_config=$GIT_CONFIG_PARAMETERS
>   clear_local_git_env
> - GIT_CONFIG_PARAMETERS=$sanitized_config
> + GIT_CONFIG_PARAMETERS=$save_config
>   export GIT_CONFIG_PARAMETERS
>  }

This does "clear the obviously per-repository stuff, but add back in
anything that came from -c".  If it is easy to do "add anything that
came from -c, and then clear the obviously per-repository stuff", we
don't even have to say "exporting core.worktree down may hurt; do
not do it then", which may be the best of both worlds?

Or have we decided that even sharing core.worktree may have a valid
use case and it is better not to filter them?

--
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] submodule: stop sanitizing config options

2016-05-04 Thread Stefan Beller 
On Wed, May 4, 2016 at 1:00 AM, Jeff King <p...@peff.net> wrote:
> [+cc Stefan and Jacob since this is really resuming that earlier thread]
>
> On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:
>
>> On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
>>
>> > >   submodule: pass on http.extraheader config settings
>> >
>> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
>> > surprised at first that your test worked at all, but that is because it
>> > is using "clone", which is the one code path that works).
>> >
>> > But I think we are waiting on going one of two paths:
>> >
>> >   1. drop sanitizing entirely
>> >
>> >   2. fix sanitizing and add more variables to it
>> >
>> > If we go the route of (2), then we'd want my fix topic and this patch.
>> > And if not, then we don't need any of it (just a patch dropping the
>> > filtering, which AFAIK nobody has written yet).
>>
>> Actually, I think this last bit is not quite true. If we want to go back
>> to "nothing gets passed to submodules", we can drop all of my patches,
>> but I don't think anybody wants to do that.
>>
>> But if we want "everything gets passed to submodules", then we do need
>> something like my patch series, because every use of local_repo_env
>> needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
>> don't think we want to simply drop that variable from local_repo_env
>> (which would also mean that it would be propagated to a local
>> git-upload-pack, for example, along with any third-party scripts that
>> use rev-parse --local-env-vars).
>>
>> So I think we'd actually want my series as a preliminary fix, followed
>> by dropping the whitelist entirely on top of that, and then probably
>> simplifying the shell sanitize_submodule_env() on top of that (it would
>> be correct without the whitelist, but you can also trivially implement
>> it without having to call submodule--helper at all).
>
> I think we'd actually do it all in one, and that patch looks something
> like the one below (on top of jk/submodule-config-sanitize-fix).
>
> I don't feel that strongly about going either direction with this, but I
> figure it doesn't hurt to make the patch so we know what the actual
> option looks like.
>
> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
>   1. It prevented obvious nonsense like using core.worktree
>  for multiple repos.
>
>   2. It could prevent surprise when the user did not mean
>  for the options to leak to the submodules (e.g.,
>  http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.

Just to recap:
Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
we passed nothing down to the commands operating on submodules.

Then we decided to pass on some of it based on a curated list.

Curating the list is too hard, so we pass on everything now, because
it is easy to maintain and easy to explain. And when the user is hurt,
they're holding it wrong?

>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).

So when running `git -c foo=bar command --recurse-submodules`
the `-c` parsing calls git_config_push_parameter, which
exports that string `foo=baz` into the environment variable
GIT_CONFIG_PARAMETERS.

When the submodule command is called, sanitize_submodule_env
just wipes all the Git related configurations except those in
GIT_CONFIG_PARAMETERS as they are set again after
clear_local_git_env wiped it.

I wonder about the implementation detail, if we rather want to introduce
a `git rev-parse --repo-only-local-env-vars` which is
`git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
such that clear_local_git_env does the right thing and we don't
have to have 2 functions for it (i.e. clear_local_git_env and
sanitize_submodule_env, which is the newer not as strict version of it)

But as Jeff once put it, rev-parse is already a

Re: [PATCH] submodule: stop sanitizing config options

2016-05-04 Thread Johannes Schindelin 
Hi,

On Wed, 4 May 2016, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > [+cc Stefan and Jacob since this is really resuming that earlier thread]
> > ...
> >> 
> >> So I think we'd actually want my series as a preliminary fix, followed
> >> by dropping the whitelist entirely on top of that, and then probably
> >> simplifying the shell sanitize_submodule_env() on top of that (it would
> >> be correct without the whitelist, but you can also trivially implement
> >> it without having to call submodule--helper at all).
> >
> > I think we'd actually do it all in one, and that patch looks something
> > like the one below (on top of jk/submodule-config-sanitize-fix).
> >
> > I don't feel that strongly about going either direction with this, but I
> > figure it doesn't hurt to make the patch so we know what the actual
> > option looks like.
> 
> I do not feel strongly either, but I suspect "we do not filter"
> would be a lot easier to explain than "we pass these selected
> things, each with such and such justification why it has to be
> passed down".

I really like the simplicity of the rationale.

> Nice code reduction is very attractive, too, but that is secondary.

Me, too. It just makes things simpler.

Ciao,
Dscho
--
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] submodule: stop sanitizing config options

2016-05-04 Thread Junio C Hamano 
Jeff King <p...@peff.net> writes:

> [+cc Stefan and Jacob since this is really resuming that earlier thread]
> ...
>> 
>> So I think we'd actually want my series as a preliminary fix, followed
>> by dropping the whitelist entirely on top of that, and then probably
>> simplifying the shell sanitize_submodule_env() on top of that (it would
>> be correct without the whitelist, but you can also trivially implement
>> it without having to call submodule--helper at all).
>
> I think we'd actually do it all in one, and that patch looks something
> like the one below (on top of jk/submodule-config-sanitize-fix).
>
> I don't feel that strongly about going either direction with this, but I
> figure it doesn't hurt to make the patch so we know what the actual
> option looks like.

I do not feel strongly either, but I suspect "we do not filter"
would be a lot easier to explain than "we pass these selected
things, each with such and such justification why it has to be
passed down".

Nice code reduction is very attractive, too, but that is secondary.

> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
>   1. It prevented obvious nonsense like using core.worktree
>  for multiple repos.
>
>   2. It could prevent surprise when the user did not mean
>  for the options to leak to the submodules (e.g.,
>  http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.
>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).
>
> We can do this easily from within the submodule shell
> script, which lets us drop the submodule--helper option
> entirely (and it's OK to do so because as a "--" program, it
> is entirely a private implementation detail).
>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
>  builtin/submodule--helper.c  | 17 -
>  git-submodule.sh |  4 ++--
>  submodule.c  | 40 +---
>  t/t7412-submodule--helper.sh | 26 --
>  4 files changed, 3 insertions(+), 84 deletions(-)
>  delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index de3ad5b..48cfc48 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> -static int module_sanitize_config(int argc, const char **argv, const char 
> *prefix)
> -{
> - struct strbuf sanitized_config = STRBUF_INIT;
> -
> - if (argc > 1)
> - usage(_("git submodule--helper sanitize-config"));
> -
> - git_config_from_parameters(sanitize_submodule_config, 
> _config);
> - if (sanitized_config.len)
> - printf("%s\n", sanitized_config.buf);
> -
> - strbuf_release(_config);
> -
> - return 0;
> -}
> -
>  struct submodule_update_clone {
>   /* index into 'list', the list of submodules to look into for cloning */
>   int current;
> @@ -522,7 +506,6 @@ static struct cmd_struct commands[] = {
>   {"list", module_list},
>   {"name", module_name},
>   {"clone", module_clone},
> - {"sanitize-config", module_sanitize_config},
>   {"update-clone", update_clone}
>  };
>  
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3a40d4b..c9d53e1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -197,9 +197,9 @@ isnumber()
>  # of the settings from GIT_CONFIG_PARAMETERS.
>  sanitize_submodule_env()
>  {
> - sanitized_config=$(git submodule--helper sanitize-config)
> + save_config=$GIT_CONFIG_PARAMETERS
>   clear_local_git_env
> - GIT_CONFIG_PARAMETERS=$sanitized_config
> + GIT_CONFIG_PARAMETERS=$save_config
>   export GIT_CONFIG_PARAMETERS
>  }
>  
> diff --git a/submodule.c b/submodule.c
> index 4e76b98..072ea82 1

[PATCH] submodule: stop sanitizing config options

2016-05-04 Thread Jeff King 
[+cc Stefan and Jacob since this is really resuming that earlier thread]

On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:

> On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
> 
> > >   submodule: pass on http.extraheader config settings
> > 
> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
> > surprised at first that your test worked at all, but that is because it
> > is using "clone", which is the one code path that works).
> > 
> > But I think we are waiting on going one of two paths:
> > 
> >   1. drop sanitizing entirely
> > 
> >   2. fix sanitizing and add more variables to it
> > 
> > If we go the route of (2), then we'd want my fix topic and this patch.
> > And if not, then we don't need any of it (just a patch dropping the
> > filtering, which AFAIK nobody has written yet).
> 
> Actually, I think this last bit is not quite true. If we want to go back
> to "nothing gets passed to submodules", we can drop all of my patches,
> but I don't think anybody wants to do that.
> 
> But if we want "everything gets passed to submodules", then we do need
> something like my patch series, because every use of local_repo_env
> needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
> don't think we want to simply drop that variable from local_repo_env
> (which would also mean that it would be propagated to a local
> git-upload-pack, for example, along with any third-party scripts that
> use rev-parse --local-env-vars).
> 
> So I think we'd actually want my series as a preliminary fix, followed
> by dropping the whitelist entirely on top of that, and then probably
> simplifying the shell sanitize_submodule_env() on top of that (it would
> be correct without the whitelist, but you can also trivially implement
> it without having to call submodule--helper at all).

I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).

I don't feel that strongly about going either direction with this, but I
figure it doesn't hurt to make the patch so we know what the actual
option looks like.

-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options

The point of having a whitelist of command-line config
options to pass to submodules was two-fold:

  1. It prevented obvious nonsense like using core.worktree
 for multiple repos.

  2. It could prevent surprise when the user did not mean
 for the options to leak to the submodules (e.g.,
 http.sslverify=false).

For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).

So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.

Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).

We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).

Signed-off-by: Jeff King <p...@peff.net>
---
 builtin/submodule--helper.c  | 17 -
 git-submodule.sh |  4 ++--
 submodule.c  | 40 +---
 t/t7412-submodule--helper.sh | 26 --
 4 files changed, 3 insertions(+), 84 deletions(-)
 delete mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index de3ad5b..48cfc48 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static int module_sanitize_config(int argc, const char **argv, const char 
*prefix)
-{
-   struct strbuf sanitized_config = STRBUF_INIT;
-
-   if (argc > 1)
-   usage(_("git submodule--helper sanitize-config"));
-
-   git_config_from_parameters(sanitize_submodule_config, 
_config);
-   if (sanitized_config.len)
-   printf("%s\n", sanitized_config.buf);
-
-   strbuf_release(_config);
-
-   return 0;
-}
-
 struct submodule_update_clone {
/* index into 'list', the list of submodules to look into for cloning */
int current;
@@ -522,7 +506,6 @@ static struct cmd_struct commands[] = {
{"list", module_list},