Re: [PATCH] submodule: stop sanitizing config options

[Jeff King <peff@peff.net>]

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