From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Stefan Beller <sbeller@google•com>,
Jacob Keller <jacob.keller@gmail•com>,
Johannes Schindelin <johannes.schindelin@gmx•de>,
"git\@vger.kernel.org" <git@vger•kernel.org>
Subject: Re: [PATCH] submodule: stop sanitizing config options
Date: Thu, 05 May 2016 09:59:15 -0700 [thread overview]
Message-ID: <xmqq60uscufw.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160505012219.GA15090@sigill.intra.peff.net> (Jeff King's message of "Wed, 4 May 2016 21:22:19 -0400")
Jeff King <peff@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 <peff@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, &sanitized_config);
> - if (sanitized_config.len)
> - printf("%s\n", sanitized_config.buf);
> -
> - strbuf_release(&sanitized_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
> + GIT_CONFIG_PARAMETERS=$save_config
> export GIT_CONFIG_PARAMETERS
> }
>
> diff --git a/submodule.c b/submodule.c
> index c18ab9b..d598881 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1098,50 +1098,13 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
> strbuf_release(&rel_path);
> free((void *)real_work_tree);
> }
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> - if (starts_with(var, "credential."))
> - return 1;
> - return 0;
> -}
> -
> -int sanitize_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, ' ');
> -
> - if (value)
> - sq_quotef(out, "%s=%s", var, value);
> - else
> - sq_quote_buf(out, var);
> - }
> -
> - return 0;
> -}
>
> void prepare_submodule_repo_env(struct argv_array *out)
> {
> const char * const *var;
>
> for (var = local_repo_env; *var; var++) {
> - if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> - struct strbuf sanitized_config = STRBUF_INIT;
> - git_config_from_parameters(sanitize_submodule_config,
> - &sanitized_config);
> - argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> - strbuf_release(&sanitized_config);
> - } else {
> + if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
> argv_array_push(out, *var);
> - }
> }
> -
> }
> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> deleted file mode 100755
> index 149d428..0000000
> --- a/t/t7412-submodule--helper.sh
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -#!/bin/sh
> -#
> -# Copyright (c) 2016 Jacob Keller
> -#
> -
> -test_description='Basic plumbing support of submodule--helper
> -
> -This test verifies the submodule--helper plumbing command used to implement
> -git-submodule.
> -'
> -
> -. ./test-lib.sh
> -
> -test_expect_success 'sanitize-config clears configuration' '
> - git -c user.name="Some User" submodule--helper sanitize-config >actual &&
> - test_must_be_empty actual
> -'
> -
> -sq="'"
> -test_expect_success 'sanitize-config keeps credential.helper' '
> - git -c credential.helper=helper submodule--helper sanitize-config >actual &&
> - echo "${sq}credential.helper=helper${sq}" >expect &&
> - test_cmp expect actual
> -'
> -
> -test_done
next prev parent reply other threads:[~2016-05-05 16:59 UTC|newest]
Thread overview: 119+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-25 13:13 [PATCH] http: Support sending custom HTTP headers Johannes Schindelin
2016-04-25 15:53 ` Shawn Pearce
2016-04-25 17:03 ` Jeff King
2016-04-26 15:37 ` Johannes Schindelin
2016-04-26 16:57 ` Jeff King
2016-04-25 18:43 ` Junio C Hamano
2016-04-26 15:33 ` Johannes Schindelin
2016-04-26 16:22 ` Junio C Hamano
2016-04-26 17:38 ` Jeff King
2016-04-27 6:31 ` Johannes Schindelin
2016-04-27 7:52 ` Jeff King
2016-04-27 11:56 ` Johannes Schindelin
2016-04-26 15:40 ` [PATCH v2] http: support " Johannes Schindelin
2016-04-26 17:03 ` Junio C Hamano
2016-04-26 17:12 ` Jeff King
2016-04-26 17:20 ` Junio C Hamano
2016-04-26 17:44 ` Jeff King
2016-04-27 6:08 ` Johannes Schindelin
2016-04-27 6:29 ` Johannes Schindelin
2016-04-26 19:05 ` Junio C Hamano
2016-04-27 6:29 ` [PATCH v3] " Johannes Schindelin
2016-04-27 12:20 ` [PATCH v4] " Johannes Schindelin
2016-04-27 19:30 ` Jeff King
2016-04-27 21:03 ` Junio C Hamano
2016-04-28 10:03 ` [PATCH v5 0/2] Add support for sending additional " Johannes Schindelin
2016-04-28 10:03 ` [PATCH v5 1/2] http: support sending custom " Johannes Schindelin
2016-04-28 10:03 ` [PATCH v5 2/2] submodule: pass on http.extraheader config settings Johannes Schindelin
2016-04-28 11:29 ` Jeff King
2016-04-28 12:19 ` Johannes Schindelin
2016-04-28 13:49 ` Jeff King
2016-04-28 15:37 ` Jacob Keller
2016-04-28 15:39 ` Jeff King
2016-04-28 16:09 ` Stefan Beller
2016-04-28 16:50 ` Jeff King
2016-04-28 19:06 ` Junio C Hamano
2016-04-28 19:10 ` Jeff King
2016-04-28 19:28 ` Junio C Hamano
2016-04-28 19:34 ` Stefan Beller
2016-04-28 19:52 ` Junio C Hamano
2016-04-28 19:53 ` Junio C Hamano
2016-04-28 20:01 ` Stefan Beller
2016-04-28 22:47 ` Junio C Hamano
2016-04-28 21:03 ` Jeff King
2016-04-28 21:12 ` Stefan Beller
2016-04-28 22:44 ` Junio C Hamano
2016-04-29 13:35 ` Jeff King
2016-04-28 21:00 ` Jeff King
2016-04-28 21:08 ` Stefan Beller
2016-04-28 21:20 ` Jeff King
2016-04-29 12:29 ` Johannes Schindelin
2016-04-29 13:26 ` Jeff King
2016-04-28 13:53 ` Jeff King
2016-04-28 19:41 ` Junio C Hamano
2016-04-29 12:35 ` Johannes Schindelin
2016-04-29 12:48 ` Johannes Schindelin
2016-04-29 13:10 ` Jeff King
2016-04-29 15:56 ` Johannes Schindelin
2016-05-04 6:14 ` [PATCH v6 0/2] Add support for sending additional HTTP headers Johannes Schindelin
2016-05-04 6:14 ` [PATCH v6 1/2] http: support sending custom " Johannes Schindelin
2016-05-05 19:10 ` Lars Schneider
2016-05-05 19:40 ` Junio C Hamano
2016-05-05 20:03 ` Jeff King
2016-05-04 6:14 ` [PATCH v6 2/2] submodule: pass on http.extraheader config settings Johannes Schindelin
2016-05-04 6:26 ` [PATCH v6 0/2] Add support for sending additional HTTP headers Jeff King
2016-05-04 7:36 ` Junio C Hamano
2016-05-04 11:20 ` Johannes Schindelin
2016-05-04 18:23 ` Junio C Hamano
2016-05-04 7:45 ` Jeff King
2016-05-04 8:00 ` [PATCH] submodule: stop sanitizing config options Jeff King
2016-05-04 8:17 ` Junio C Hamano
2016-05-04 11:25 ` Johannes Schindelin
2016-05-04 17:58 ` Stefan Beller
2016-05-04 19:04 ` Jeff King
2016-05-04 18:43 ` Junio C Hamano
2016-05-04 19:09 ` Jeff King
2016-05-04 22:53 ` Stefan Beller
2016-05-05 1:22 ` Jeff King
2016-05-05 16:59 ` Junio C Hamano [this message]
2016-05-05 20:14 ` Jeff King
2016-05-05 23:33 ` Junio C Hamano
2016-05-06 0:23 ` Stefan Beller
2016-05-06 1:00 ` Jeff King
2016-05-06 19:56 ` Junio C Hamano
2016-05-09 6:18 ` [PATCH v7 0/3] Add support for sending additional HTTP headers (part 2) Johannes Schindelin
2016-05-09 6:18 ` [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2 Johannes Schindelin
2016-05-09 8:03 ` Jeff King
2016-05-09 14:03 ` Johannes Schindelin
2016-05-09 14:27 ` Jeff King
2016-05-09 15:11 ` Johannes Schindelin
2016-05-09 16:42 ` Junio C Hamano
2016-05-09 16:51 ` Jeff King
2016-05-09 17:41 ` Junio C Hamano
2016-05-10 6:53 ` Johannes Schindelin
2016-05-10 7:13 ` Junio C Hamano
2016-05-09 16:23 ` Junio C Hamano
2016-05-10 6:37 ` Lars Schneider
2016-05-10 7:14 ` Junio C Hamano
2016-05-09 6:19 ` [PATCH v7 2/3] t5551: make the test for extra HTTP headers more robust Johannes Schindelin
2016-05-09 7:56 ` Lars Schneider
2016-05-09 8:05 ` Jeff King
2016-05-09 8:13 ` Johannes Schindelin
2016-05-09 8:20 ` Jeff King
2016-05-09 6:19 ` [PATCH v7 3/3] submodule: pass on http.extraheader config settings Johannes Schindelin
2016-05-10 7:08 ` [PATCH v8 0/3] Add support for sending additional HTTP headers (part 2) Johannes Schindelin
2016-05-10 7:08 ` [PATCH v8 1/3] tests: adjust the configuration for Apache 2.2 Johannes Schindelin
2016-05-10 17:31 ` Junio C Hamano
2016-05-10 7:08 ` [PATCH v8 2/3] t5551: make the test for extra HTTP headers more robust Johannes Schindelin
2016-05-10 17:34 ` Junio C Hamano
2016-05-11 17:13 ` t5551 hangs ? Torsten Bögershausen
2016-05-11 17:31 ` Jeff King
2016-05-11 20:03 ` Torsten Bögershausen
2016-05-12 3:16 ` Jeff King
2016-05-12 6:21 ` Torsten Bögershausen
2016-05-12 6:40 ` Jeff King
2016-05-12 7:29 ` Jeff King
2016-05-10 7:08 ` [PATCH v8 3/3] submodule: ensure that -c http.extraheader is heeded Johannes Schindelin
2016-05-10 17:38 ` Junio C Hamano
2016-05-11 6:57 ` Johannes Schindelin
[not found] ` <34DE0A16-F0B2-4379-8E02-5235D34FDD76@gmail.com>
2016-05-16 13:35 ` mail-patch-series.sh, was Re: [PATCH v7 0/3] Add support for sending additional HTTP headers (part 2) Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq60uscufw.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jacob.keller@gmail$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox