| Message ID | 34bdbc27d618d7467d2caf6844d8c06bdcb8545b.1618297711.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 1b3d1f8f72dbef73f7d667ccc8f2e171736ca043 |
| Headers | show |
| Series | config: allow overriding global/system config | expand |
On Tue, Apr 13, 2021 at 09:11:44AM +0200, Patrick Steinhardt wrote: > While at it, the function is also refactored to pass memory ownership to > the caller. This is done to better match semantics of > `git_global_config()`, which is going to be introduced in the next > commit. I think this turned out nicely. There are only two callers, one of which is already handling freeing the global config. In the other: > diff --git a/builtin/config.c b/builtin/config.c > index f71fa39b38..02ed0b3fe7 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) > } > } > else if (use_system_config) { > - given_config_source.file = git_etc_gitconfig(); > + given_config_source.file = git_system_config(); > given_config_source.scope = CONFIG_SCOPE_SYSTEM; > } else if (use_local_config) { > given_config_source.file = git_pathdup("config"); We "leak" the result, but I think it is actually an improvement. As you can see in the context, we are sometimes allocating the field in other code paths, so this is takes us closer to a world where we can actually call free(given_config_source.file) to fix the leak in all of the other code paths. :) (I don't think any of that needs to be dealt with in this series, of course). -Peff
On Tue, Apr 13, 2021 at 09:11:44AM +0200, Patrick Steinhardt wrote: > The `git_etc_gitconfig()` function retrieves the system-level path of > the configuration file. We're about to introduce a way to override it > via an environment variable, at which point the name of this function > would start to become misleading. > > Rename the function to `git_system_config()` as a preparatory step. > While at it, the function is also refactored to pass memory ownership to > the caller. This is done to better match semantics of > `git_global_config()`, which is going to be introduced in the next > commit. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/config.c | 2 +- > config.c | 18 ++++++++---------- > config.h | 3 ++- > 3 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/builtin/config.c b/builtin/config.c > index f71fa39b38..02ed0b3fe7 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) > } > } > else if (use_system_config) { > - given_config_source.file = git_etc_gitconfig(); > + given_config_source.file = git_system_config(); > given_config_source.scope = CONFIG_SCOPE_SYSTEM; > } else if (use_local_config) { > given_config_source.file = git_pathdup("config"); > diff --git a/config.c b/config.c > index 6428393a41..8c83669cce 100644 > --- a/config.c > +++ b/config.c > @@ -1844,12 +1844,9 @@ static int git_config_from_blob_ref(config_fn_t fn, > return git_config_from_blob_oid(fn, name, &oid, data); > } > > -const char *git_etc_gitconfig(void) > +char *git_system_config(void) > { > - static const char *system_wide; > - if (!system_wide) > - system_wide = system_path(ETC_GITCONFIG); > - return system_wide; > + return system_path(ETC_GITCONFIG); > } > > /* > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts, > config_fn_t fn, void *data) > { > int ret = 0; > + char *system_config = git_system_config(); > char *xdg_config = xdg_config_home("config"); > char *user_config = expand_user_path("~/.gitconfig", 0); > char *repo_config; > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts, > repo_config = NULL; > > current_parsing_scope = CONFIG_SCOPE_SYSTEM; > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, Removing git_config_system() from the condition breaks GIT_CONFIG_NOSYSTEM: expecting success of 9999.1 'test': cat /usr/local/etc/gitconfig && git config --list --show-origin --show-scope + cat /usr/local/etc/gitconfig [foo] bar = baz + git config --list --show-origin --show-scope system file:/usr/local/etc/gitconfig foo.bar=baz local file:.git/config core.repositoryformatversion=0 local file:.git/config core.filemode=true local file:.git/config core.bare=false local file:.git/config core.logallrefupdates=true ok 1 - test And breaks just about everything the Linux32 job on Travis CI: https://travis-ci.org/github/git/git/jobs/767207687#L1218 > - opts->system_gently ? > - ACCESS_EACCES_OK : 0)) > - ret += git_config_from_file(fn, git_etc_gitconfig(), > - data); > + if (system_config && !access_or_die(system_config, R_OK, > + opts->system_gently ? > + ACCESS_EACCES_OK : 0)) > + ret += git_config_from_file(fn, system_config, data); > > current_parsing_scope = CONFIG_SCOPE_GLOBAL; > if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) > @@ -1927,6 +1924,7 @@ static int do_git_config_sequence(const struct config_options *opts, > die(_("unable to parse command-line config")); > > current_parsing_scope = prev_parsing_scope; > + free(system_config); > free(xdg_config); > free(user_config); > free(repo_config); > diff --git a/config.h b/config.h > index 19a9adbaa9..2be8fa1880 100644 > --- a/config.h > +++ b/config.h > @@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *); > int git_config_rename_section_in_file(const char *, const char *, const char *); > int git_config_copy_section(const char *, const char *); > int git_config_copy_section_in_file(const char *, const char *, const char *); > -const char *git_etc_gitconfig(void); > int git_env_bool(const char *, int); > unsigned long git_env_ulong(const char *, unsigned long); > int git_config_system(void); > @@ -327,6 +326,8 @@ int config_error_nonbool(const char *); > #define config_error_nonbool(s) (config_error_nonbool(s), const_error()) > #endif > > +char *git_system_config(void); > + > int git_config_parse_parameter(const char *, config_fn_t fn, void *data); > > enum config_scope current_config_scope(void); > -- > 2.31.1 >
On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote: > > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts, > > config_fn_t fn, void *data) > > { > > int ret = 0; > > + char *system_config = git_system_config(); > > char *xdg_config = xdg_config_home("config"); > > char *user_config = expand_user_path("~/.gitconfig", 0); > > char *repo_config; > > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts, > > repo_config = NULL; > > > > current_parsing_scope = CONFIG_SCOPE_SYSTEM; > > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, > > Removing git_config_system() from the condition breaks > GIT_CONFIG_NOSYSTEM: Good catch. My gut feeling is that the new git_system_config() should check NOSYSTEM and return NULL if it's set, and then we can get rid of git_config_system() entirely. That is slightly different than the old behavior; right now GIT_CONFIG_NOSYSTEM only prevents reading during the normal sequence, and not reading (or writing!) via "git config --system". But I think it would be an improvement to prevent those (the whole point of the feature was to avoid the test suite accidentally accessing the larger environment). -Peff
Jeff King <peff@peff.net> writes: > On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote: > >> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts, >> > config_fn_t fn, void *data) >> > { >> > int ret = 0; >> > + char *system_config = git_system_config(); >> > char *xdg_config = xdg_config_home("config"); >> > char *user_config = expand_user_path("~/.gitconfig", 0); >> > char *repo_config; >> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts, >> > repo_config = NULL; >> > >> > current_parsing_scope = CONFIG_SCOPE_SYSTEM; >> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, >> >> Removing git_config_system() from the condition breaks >> GIT_CONFIG_NOSYSTEM: > > Good catch. My gut feeling is that the new git_system_config() should > check NOSYSTEM and return NULL if it's set, and then we can get rid of > git_config_system() entirely. NULL -> /dev/null I guess? > That is slightly different than the old behavior; right now > GIT_CONFIG_NOSYSTEM only prevents reading during the normal sequence, > and not reading (or writing!) via "git config --system". But I think it > would be an improvement to prevent those (the whole point of the feature > was to avoid the test suite accidentally accessing the larger > environment). Yup.
On Sat, Apr 17, 2021 at 02:37:39PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote: > > > >> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts, > >> > config_fn_t fn, void *data) > >> > { > >> > int ret = 0; > >> > + char *system_config = git_system_config(); > >> > char *xdg_config = xdg_config_home("config"); > >> > char *user_config = expand_user_path("~/.gitconfig", 0); > >> > char *repo_config; > >> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts, > >> > repo_config = NULL; > >> > > >> > current_parsing_scope = CONFIG_SCOPE_SYSTEM; > >> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, > >> > >> Removing git_config_system() from the condition breaks > >> GIT_CONFIG_NOSYSTEM: > > > > Good catch. My gut feeling is that the new git_system_config() should > > check NOSYSTEM and return NULL if it's set, and then we can get rid of > > git_config_system() entirely. > > NULL -> /dev/null I guess? I was thinking NULL to catch this line from the post-image of Patrick's series: if (system_config && !access_or_die(system_config, R_OK, opts->system_gently ? ACCESS_EACCES_OK : 0)) ret += git_config_from_file(fn, system_config, data); which would see that we have no file at all. But that may be unexpected for other callers (right now git_etc_gitconfig() can never return NULL, and I'm not sure what would happen with "git config --system"; I suspect it would do the regular config sequence instead, which is wrong). So yeah, probably returning /dev/null is more sensible (and makes it a true alias for GIT_CONFIG_SYSTEM=/dev/null). -Peff
On Sun, Apr 18, 2021 at 01:39:31AM -0400, Jeff King wrote: > On Sat, Apr 17, 2021 at 02:37:39PM -0700, Junio C Hamano wrote: > > > Jeff King <peff@peff.net> writes: > > > > > On Fri, Apr 16, 2021 at 11:14:51PM +0200, SZEDER Gábor wrote: > > > > > >> > @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts, > > >> > config_fn_t fn, void *data) > > >> > { > > >> > int ret = 0; > > >> > + char *system_config = git_system_config(); > > >> > char *xdg_config = xdg_config_home("config"); > > >> > char *user_config = expand_user_path("~/.gitconfig", 0); > > >> > char *repo_config; > > >> > @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts, > > >> > repo_config = NULL; > > >> > > > >> > current_parsing_scope = CONFIG_SCOPE_SYSTEM; > > >> > - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, > > >> > > >> Removing git_config_system() from the condition breaks > > >> GIT_CONFIG_NOSYSTEM: > > > > > > Good catch. My gut feeling is that the new git_system_config() should > > > check NOSYSTEM and return NULL if it's set, and then we can get rid of > > > git_config_system() entirely. > > > > NULL -> /dev/null I guess? > > I was thinking NULL to catch this line from the post-image of Patrick's > series: > > if (system_config && !access_or_die(system_config, R_OK, > opts->system_gently ? > ACCESS_EACCES_OK : 0)) > ret += git_config_from_file(fn, system_config, data); > > which would see that we have no file at all. But that may be unexpected > for other callers (right now git_etc_gitconfig() can never return NULL, > and I'm not sure what would happen with "git config --system"; I suspect > it would do the regular config sequence instead, which is wrong). > > So yeah, probably returning /dev/null is more sensible (and makes it a > true alias for GIT_CONFIG_SYSTEM=/dev/null). > > -Peff It's only by accident that I dropped the call to `git_config_system()`, must've happened when resolving conflicts somewhere. The issue with just returning `/dev/null` from `git_system_config()` is that git-config(1) would be broken, as you hint at. We do not honor GIT_CONFIG_NOSYSTEM there if the "--system" flag was given. So yes, we could change it to return `/dev/null`, but that would change semantics of GIT_CONFIG_NOSYSTEM. I'm not sure doing this in the same series is a good idea. Even more so because with returning `/dev/null`, the conversion would be silent: whereas previous versions simply wrote to or read from the system-level config, we now pretend to have read or written something even though we didn't. Patrick
On Mon, Apr 19, 2021 at 01:03:56PM +0200, Patrick Steinhardt wrote: > It's only by accident that I dropped the call to `git_config_system()`, > must've happened when resolving conflicts somewhere. The issue with just > returning `/dev/null` from `git_system_config()` is that git-config(1) > would be broken, as you hint at. We do not honor GIT_CONFIG_NOSYSTEM > there if the "--system" flag was given. > > So yes, we could change it to return `/dev/null`, but that would change > semantics of GIT_CONFIG_NOSYSTEM. I'm not sure doing this in the same > series is a good idea. Even more so because with returning `/dev/null`, > the conversion would be silent: whereas previous versions simply wrote > to or read from the system-level config, we now pretend to have read or > written something even though we didn't. I'm OK being more cautious here, and leaving NOSYSTEM as it is. My original thinking was that returning /dev/null would be an improvement even for the existing call in "git config --system". And I do think it would be for reading. But it actually gets pretty weird for writing, because of our atomic-rename strategy. And that is actually true of your new variable too. Doing this: GIT_CONFIG_SYSTEM=/dev/null git config --system foo.bar would return nothing, which makes sense to me. But this: GIT_CONFIG_SYSTEM=/dev/null git config --system foo.bar value would try to write /dev/null.lock, and then rename it into place over the real /dev/null! That may be slightly surprising, but it is not really any different than any other non-regular-file entry. The stakes are slightly higher here, as it breaks all sorts of things that can be hard to recover from (even if you know the correct mknod incantation, important things like bash seem to get crabby if they can't open /dev/null for writing). And it may be more likely to happen by mistake (as opposed to "git config --file=/dev/null", which behaves the same). But unless you are root, you are likely to just get an error in the first place, so it seems like an unlikely mistake in practice. So I think it may fall into "if it hurts, don't do it". -Peff
diff --git a/builtin/config.c b/builtin/config.c index f71fa39b38..02ed0b3fe7 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) } } else if (use_system_config) { - given_config_source.file = git_etc_gitconfig(); + given_config_source.file = git_system_config(); given_config_source.scope = CONFIG_SCOPE_SYSTEM; } else if (use_local_config) { given_config_source.file = git_pathdup("config"); diff --git a/config.c b/config.c index 6428393a41..8c83669cce 100644 --- a/config.c +++ b/config.c @@ -1844,12 +1844,9 @@ static int git_config_from_blob_ref(config_fn_t fn, return git_config_from_blob_oid(fn, name, &oid, data); } -const char *git_etc_gitconfig(void) +char *git_system_config(void) { - static const char *system_wide; - if (!system_wide) - system_wide = system_path(ETC_GITCONFIG); - return system_wide; + return system_path(ETC_GITCONFIG); } /* @@ -1883,6 +1880,7 @@ static int do_git_config_sequence(const struct config_options *opts, config_fn_t fn, void *data) { int ret = 0; + char *system_config = git_system_config(); char *xdg_config = xdg_config_home("config"); char *user_config = expand_user_path("~/.gitconfig", 0); char *repo_config; @@ -1896,11 +1894,10 @@ static int do_git_config_sequence(const struct config_options *opts, repo_config = NULL; current_parsing_scope = CONFIG_SCOPE_SYSTEM; - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, - opts->system_gently ? - ACCESS_EACCES_OK : 0)) - ret += git_config_from_file(fn, git_etc_gitconfig(), - data); + if (system_config && !access_or_die(system_config, R_OK, + opts->system_gently ? + ACCESS_EACCES_OK : 0)) + ret += git_config_from_file(fn, system_config, data); current_parsing_scope = CONFIG_SCOPE_GLOBAL; if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) @@ -1927,6 +1924,7 @@ static int do_git_config_sequence(const struct config_options *opts, die(_("unable to parse command-line config")); current_parsing_scope = prev_parsing_scope; + free(system_config); free(xdg_config); free(user_config); free(repo_config); diff --git a/config.h b/config.h index 19a9adbaa9..2be8fa1880 100644 --- a/config.h +++ b/config.h @@ -318,7 +318,6 @@ int git_config_rename_section(const char *, const char *); int git_config_rename_section_in_file(const char *, const char *, const char *); int git_config_copy_section(const char *, const char *); int git_config_copy_section_in_file(const char *, const char *, const char *); -const char *git_etc_gitconfig(void); int git_env_bool(const char *, int); unsigned long git_env_ulong(const char *, unsigned long); int git_config_system(void); @@ -327,6 +326,8 @@ int config_error_nonbool(const char *); #define config_error_nonbool(s) (config_error_nonbool(s), const_error()) #endif +char *git_system_config(void); + int git_config_parse_parameter(const char *, config_fn_t fn, void *data); enum config_scope current_config_scope(void);
The `git_etc_gitconfig()` function retrieves the system-level path of the configuration file. We're about to introduce a way to override it via an environment variable, at which point the name of this function would start to become misleading. Rename the function to `git_system_config()` as a preparatory step. While at it, the function is also refactored to pass memory ownership to the caller. This is done to better match semantics of `git_global_config()`, which is going to be introduced in the next commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/config.c | 2 +- config.c | 18 ++++++++---------- config.h | 3 ++- 3 files changed, 11 insertions(+), 12 deletions(-)