Re: [PATCH v3?] Add global and system-wide gitattributes

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Aug 30, 2010, 22:55 UTC
Message-ID: <vpqhbibbthi.fsf@bauges.imag.fr>
In-Reply-To: <7vbp8jerfq.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> I don't understand why this breaks the test. It seems blame
>>> --encoding=UTF-8 relies on the fact that the i18n section of the
>>> configuration is not loaded.
>>
>> That's interesting; I haven't traced the codepath involved, but I do not
>> think "configuration is not loaded" is the issue. "Reading either before
>> the main codepath is ready, or more likely overwriting/destroying what the
>> main codepath has read it by re-reading the configuration" may be.
>
> I think that hunch is correct.

Confirmed.

Show 10 quoted lines

> A typical way we default to hardcoded value, overridable by
> configuration file, and then further use command line to override
> that, is for the main codepath to do the following in this order:
>
>  - call git_config(git_appropriate_config); this changes the variables
>    (with possibly hardcoded default) defined in environment.c;
>
>  - parse command line options and override the variable;
>
>  - use the variable at runtime.

Yes, this is the problem, with git_log_output_encoding as you guessed.

> The correct solution would be twofold, but the latter is rather painful:

Not that much in the case of git_log_output_encoding, but other uses of the same pattern may exist.

>  - The call from the bootstrap_attr_stack should use a callback that reads
>    only the attribute file location configuration and _nothing else_.

[...]

>  - The way programs (this is not limited to blame and other rev-list
>    machinery users) implement the "use configured values but let command
>    line override them" need to be changed.

I think it's reasonable to do both. Having both git_config() and command-line parsing write to the same variable is fragile and should be avoided IMHO, but OTOH, arbitrary calls to git_config(git_default_config) may break other things, so ...

Show 7 quoted lines

>    One possibility is to copy the values determined by reading the config
>    and the command line to their own variables, so that later random call
>    to git_config() won't stomp on the actual values to be used.  This is
>    painful as environment.c variables are _meant_ to be easily usable as
>    global variables and copying them away (which means they now need to be
>    passed around throughout the callchain in the various APIs) defeats
>    the whole point of having them.

I just keep two global variables instead of two, and implement a straightforward accessor. Command-line option parsing already used to write to a global variable, so it doesn't change much.

New patch serie follows,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/