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

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

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?