Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

From: Jeff King <peff@peff.net>
Date: Apr 29, 2016, 13:26 UTC
Message-ID: <20160429132617.GB30931@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1604291417580.9313@virtualbox>

On Fri, Apr 29, 2016 at 02:29:25PM +0200, Johannes Schindelin wrote:

Show 8 quoted lines

> The more I think about it, I actually think that we do the user a *really*
> great disservice by filtering the CONFIG_DATA_ENVIRONMENT. If I call
> 
> 	git -c ... $cmd
> 
> and that configuration is *not* picked up, it is much worse than letting
> users shoot themselves in their own feet by specifying config settings
> that are *prone* to wreak havoc.

That's a good point. For every "whoops, I didn't mean for this to kick in for the submodule!" there is an equal and opposite "whoops, this needed to kick in for the submodule!" case (e.g., instead of over-reaching turning http.sslverify off, you might be trying to turn it _on_ and fail to apply it in all cases).

And making the rule "-c applies to all sub-processes, period" is much simpler to explain.

(Though of course it is still not entirely true. We clear the config
 when accessing another repository as a remote for fetching or pushing.
 But a good chunk of that is simply for consistency of all remotes, as
 we do not pass the environment across TCP sessions).

Show 18 quoted lines

> > So I think the only two cases worth filtering are:
> > 
> >   1. Ones where we _know_ that the config is nonsense to pass along,
> >      _and_ where a user might conceivably make use of the
> >      just-the-top-level version of it (core.worktree
> >      comes to mind, though of course they are probably better served by
> >      "--work-tree" in such a case).
> >   2. An option where we think there may be some security implication.
> >      Setting "http.sslverify" to false does have some security
> >      implications ("oops, I only meant to turn off verification for the
> >      root repo, and I got MiTM-attacked for the submodules!"). But it's
> >      so obscure and unlikely that I think the benefit outweighs it.
> 
> I can see that happening when somebody calls an alias with `git -c ...`
> and that alias performs actions in the top-level project as well as all
> submodules.
> 
> But. Do we really have to be "Big Daddy" for users who do that?

I have more sympathy for cases that full under (1) above, just because they currently work _now_. So it's possible somebody is currently doing a thing that makes sense under the current rules, but will involve foot-shooting under the new version.

Show 5 quoted lines

> > I am OK staying with a whitelist.
> 
> Me, too. But I am even more in favor of abandoning this "we know what is
> good for you" approach, i.e. that whitelist that filters
> CONFIG_DATA_ENVIRONMENT.

I could live with that, too. I've added Jens to the cc; his preference for not blindly sharing config to submodules is part of what influenced me in the original discussion.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/264840

    I think that's the right link, which I dug out of my
    <20160219043019.GA14764@sigill.intra.peff.net> from a few months
    ago. Gmane seems to be down.