Re: [PATCH v4 0/5] config: introduce discovery.bare and protected config

[Glen Choo <chooglen@google.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> Glen Choo (5):
>>   Documentation/git-config.txt: add SCOPES section
>>   Documentation: define protected configuration
>
> Forgot to mention when I was sending my comments on patch 2: we should
> standardize on "protected config" and not use "protected configuration"
> anywhere.

Makes sense.

> One alternative design would have been to have separate configsets for
> protected config and non-protected config (or even better, separate
> configsets for trace2 config, protected config minus trace2 config, and
> non-protected config) but that doesn't have to block the submission of
> this patch set.

I suppose that the idea behind this is that we only parse and store each
config file exactly once. It's a good goal, but the whole point of the
configset is that we can query a single struct to figure out the value
of a config variable. Having multiple configsets starts to shift more of
the burden to the callers because they now have to query multiple
configsets to find their desired config value, and we already start to
see some of this unpleasantness in this series.

An alternative that I'd been thinking about is to make a few changes to
the git_config_* + configset API to allow us to use a single configset
for all of our needs:

1. Keep track of what config we've read when reading into
   the_repository->config, i.e. instead of a boolean "all config has
   been [un]read", we can express "system and global config has been
   read, but not local or command config". Then, use this information to
   load config from sources as they become available. This will allow us
   to read incomplete config for trace2 and setup.c (discovery.bare and
   safe.directory), and only read what we need later on.

   This assumes that when Git reads config, that config is always valid
   later on. So this is broken if, e.g. we read global config file A
   during setup, but when we discover the repo, we discard A and read
   global config file B instead. I don't know if we do this or if we are
   planning to in the future.

2. Add an additional argument that specifies what scopes to respect when
   reading config (maybe as a set of flags). This gives us extra
   specificity when using the git_config*() functions, so we could get
   rid of git_protected_config() like so:

    /* Change enum config_scope into flags first... */

    #define WIP_SCOPES_PROTECTED = CONFIG_SCOPE_SYSTEM & \
      CONFIG_SCOPE_GLOBAL & CONFIG_SCOPE_COMMAND

    static enum discovery_bare_allowed get_discovery_bare(void)
    {
      enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
      git_config(discovery_bare_cb, &result, WIP_SCOPES_PROTECTED);
      return result;
    }

   And as an added bonus, this gives us an easy way to implement the
   constant time git_config_*() functions for protected config. We could
   even do this without doing 1. first. I haven't looked into whether
   we could turn the enum into flags, but otherwise, I think this is
   pretty feasible.