Re: [PATCH v2 07/21] config: document `read_early_config()` and `read_very_early_config()`

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 11, 2024 at 10:59:42AM -0500, Justin Tobler wrote:
> On 24/08/30 11:09AM, Patrick Steinhardt wrote:
> > It's not clear what `read_early_config()` and `read_very_early_config()`
> > do differently compared to `repo_read_config()` from just looking at
> > their names. Document both of these in the header file to clarify their
> > intent.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  config.c |  4 ----
> >  config.h | 11 +++++++++++
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/config.c b/config.c
> > index 0b87f0f9050..a8357ea9544 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2234,10 +2234,6 @@ void read_early_config(config_fn_t cb, void *data)
> >  	strbuf_release(&gitdir);
> >  }
> >  
> > -/*
> > - * Read config but only enumerate system and global settings.
> > - * Omit any repo-local, worktree-local, or command-line settings.
> > - */
> >  void read_very_early_config(config_fn_t cb, void *data)
> >  {
> >  	struct config_options opts = { 0 };
> > diff --git a/config.h b/config.h
> > index d0497157c52..f5fa833cb98 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -192,7 +192,18 @@ int git_config_from_blob_oid(config_fn_t fn, const char *name,
> >  void git_config_push_parameter(const char *text);
> >  void git_config_push_env(const char *spec);
> >  int git_config_from_parameters(config_fn_t fn, void *data);
> > +
> > +/*
> > + * Read config when the Git directory has not yet been set up. In case
> > + * `the_repository` has not yet been set up, try to discover the Git
> > + * directory to read the configuration from.
> > + */
> >  void read_early_config(config_fn_t cb, void *data);
> 
> To restate in my own words, `read_early_config()` allows a config to be
> read before `the_repository` is setup by discovering the git dir itself.
> Out of curiousity, what prevents us from just ensuring `the_repository`
> is properly setup first?

This function is mostly called when we may or may not have a repository.
This is for example important for alias handling: we want aliases to
work when outside a repository, and they are not yet set up at the point
in time where we need to resolve such an alias. If you happen to be in a
repository, you also want to make its aliases available. If you aren't,
you only want to make aliases in your global and system configuration
available.

> > +
> > +/*
> > + * Read config but only enumerate system and global settings.
> > + * Omit any repo-local, worktree-local, or command-line settings.
> > + */
> >  void read_very_early_config(config_fn_t cb, void *data);
> 
> Here `read_very_early_config()` looks like it only cares about system
> and global configuration so it doesn't require a git dir or
> `the_repository` to be set up. Makes sense.
> 
> Not really related to this change, but it would be nice if the name of
> the function itself was more descript. Something like
> `config_read_system_and_global()`.
> 
> Overall, I find these new comments to be very helpful. Thanks! :)

Agreed, the names aren't great. But as you say, I'd rather not fix them
as part of this patch series.

Patrick