Re: [PATCH 2/5] refs: introduce a "ref paranoia" flag

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2015, 21:31 UTC
Message-ID: <xmqqpp84hdxg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150319210019.GB7775@peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

>> > +	if (ref_paranoia < 0)
>> > +		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
>> > +	if (ref_paranoia)
>> > +		data.flags |= DO_FOR_EACH_INCLUDE_BROKEN;
>> 
>> I am not a big fan of proliferation of interfaces based on
>> environment variables, but hopefully this is isolated enough to
>> become an issue in the future.
>
> I'm not sure which part you don't like.
> 
> We do have to have this variable cross some process boundaries. Only
> "repack" knows whether to turn on paranoia, but "pack-objects" is the
> one that must act on it.
>
> Or is there something else I'm missing?

In general, I do not like the pattern of program A setting an environment only because it wants to tell program B it spawns something, because we cannot tell program B that the environment should be dropped when it calls something else (e.g. user defined hooks, merge drivers, textconvs, etc.) to prevent end user invocation of Git commands from honoring it. Setting GIT_DIR or GIT_WORK_TREE and having to know when to drop them is not very pleasant, for example.

I think the use of this pattern is OK in this codepath in which repack calls pack-objects, and I think I can be persuaded to buy an argument that there is no harm, or it may even be a good thing, to run such an end-user program under paranoia mode, if pack-objects wants to spawn one.