Re: [PATCH 2/3] don't write to git_log_output_encoding outside git_config()

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I'm just raising attention on this patch. It didn't receive any
> comment and didn't find its way to pu.
>
> I think it makes the code more robust, and would prevent accidental
> bugs like the one I introduced patching the gitattributes patch, but
> it's not critical.

If we didn't have the fix to po/etc-gitattributes topic, the patch is
absolutely necessary as it stops bleeding.

Two points and a bit of discussion.

(1) The patch claims that the existing "have a variable in environment.c,
    with possibly hardcoded default, update that variable by reading
    config file at the startup, further override the same variable by
    reading the command line, and use the final result from the variable"
    pattern is fragile, _if_ configuration files can be re-read at random
    places outside the control of the main program.

    The above claim _is_ correct, but is it the existing pattern that is
    broken, or the uncontrolled reading of configuration file, that is the
    real culprit (iow, you said "X if Y" but if Y is false then what X
    claims does not matter)?

    I don't think we have a satisfactory answer to that question yet.

(2) If the answer to the question (1) is that the existing pattern is bad,
    I agree that a mechanism to protect the value that the main program
    decided to use (after taking environment.c default, config and its
    command line processing) _is_ necessary, and the patch shows one
    possible way to do so by replacing a single environment.c variable
    with a pair of variables and an accessor macro.

    (a) Is that the best solution to the problem, though?

    (b) Does the solution show us a pattern that is easy to follow to
        avoid the same problem with other environment.c variables?

One minor potential flaw I see is that while this solution can be applied
to a variable of a type with an obvious "unset" value (e.g. pointer with
NULL), we cannot use it for a variable of type "int" if it can truly take
any value, because get_git_frotz() macro needs to be able to check if
git_fortz_cli has that "unset" value in order to decide which one of the
variables to use.  My gut feeling is that it wouldn't matter in real-life
(most int-type knobs actually take uint values), but I didn't check.

I care about (2-b) more than anything else.  For example, we see
"git_commit_encoding" variable in cache.h in the context of your patch.
This particular patch does not have to (and I do not want it to) fix
potential problems with that or any other variable, but it would be nice
if the patch shows us an obvious and uniform way to apply the same fix
when/if it becomes necessary.  IOW, we would want to make sure that this
is a generic enough solution, not an ad-hoc workaround.

Is there something we can do to make it easier to apply to other cases?
Perhaps something along the lines of...

    #define git_declare_var_pair(type, name, unset) \
    extern type name; \
    extern type name ## _cli; \
    static inline type get_ ## name (void) { \
    	return (name ## _cli != (unset)) \
        	? name ## _cli \
                : name; \
    }

    git_declare_var_pair(const char *, git_log_output_encoding, NULL)

Or is it a way-premature generalization?

By the way, do we have a case where the main codepath reads an environment
variable and updates a variable in environment.c to be used with that
value?  Then the existing "fragile" pattern for that case may look like:

    - there is a variable with default value in environment.c;
    - it is overwritten by reading configuration file;
    - it is further overwritten by reading environ[];
    - it is further overwritten by reading argv[];
    - and then it is used.

Does the solution presented by this patch show us a pattern that is
applicable to such a case as well?

>> diff --git a/cache.h b/cache.h
>> index eb77e1d..7e10a39 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1005,7 +1005,25 @@ extern int user_ident_explicitly_given;
>>  extern int user_ident_sufficiently_given(void);
>>  
>>  extern const char *git_commit_encoding;
>> +
>> +/* Value found in config file */
>>  extern const char *git_log_output_encoding;
>> +
>> +/* Value given in command line with --encoding */
>> +extern const char *git_log_output_encoding_cli;
>> +
>> +/* 
>> + * Prioritize the value given by the command-line over the value found
>> + * in the config file.
>> + */
>> +static inline
>> +const char *get_git_log_output_encoding()
>> +{
>> +	return git_log_output_encoding_cli ?
>> +		git_log_output_encoding_cli :
>> +		git_log_output_encoding;
>> +}
>> +
>>  extern const char *git_mailmap_file;