| Message ID | ec4804a99bf70f9a97d1faea60bd55aaa97d1b80.1725008898.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | environment: guard reliance on `the_repository` | expand |
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? > + > +/* > + * 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! :)
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
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); + +/* + * 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); /**
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(-)