| Message ID | 1e2376a4b998b5b182cc5f72afc7282134bcdf2c.1697660181.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | maintenance: use XDG config if it exists | expand |
On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote: > > `git maintenance register` registers the repository in the user's global > config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if > `~/.gitconfig` does not exist. However, this command creates a > `~/.gitconfig` file and writes to that one even though the XDG variant > exists. > > This used to work correctly until 50a044f1e4 (gc: replace config > subprocesses with API calls, 2022-09-27), when the command started calling > the config API instead of git-config(1). > > Also change `unregister` accordingly. > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > --- > builtin/gc.c | 23 +++++------------------ > t/t7900-maintenance.sh | 21 +++++++++++++++++++++ > 2 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index 17fc031f63a..7b780f2ab38 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) > > if (!found) { > int rc; > - char *user_config = NULL, *xdg_config = NULL; > > - if (!config_file) { > - git_global_config_paths(&user_config, &xdg_config); > - config_file = user_config; > - if (!user_config) > - die(_("$HOME not set")); > - } > + if (!config_file) > + config_file = git_global_config(); > rc = git_config_set_multivar_in_file_gently( > config_file, "maintenance.repo", maintpath, > CONFIG_REGEX_NONE, 0); > - free(user_config); > - free(xdg_config); Don't we have to free `config_file` now? > if (rc) > die(_("unable to add '%s' value of '%s'"), > @@ -1595,18 +1588,12 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi > > if (found) { > int rc; > - char *user_config = NULL, *xdg_config = NULL; > - if (!config_file) { > - git_global_config_paths(&user_config, &xdg_config); > - config_file = user_config; > - if (!user_config) > - die(_("$HOME not set")); > - } > + > + if (!config_file) > + config_file = git_global_config(); > rc = git_config_set_multivar_in_file_gently( > config_file, key, NULL, maintpath, > CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); > - free(user_config); > - free(xdg_config); Same here. > if (rc && > (!force || rc == CONFIG_NOTHING_SET)) > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 487e326b3fa..a11e6c61520 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' ' > test_subcommand ! git maintenance run --auto --quiet <false > ' > > +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' > + XDG_CONFIG_HOME=.config && > + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config && > + export "XDG_CONFIG_HOME" && Style: there is no need to quote here. Also, I think we need to unset this variable at the end of this test as tests don't run in a subshell. In theory, we should also be able to leave this variable unset completely as it will be derived from HOME automatically anyway. > + mkdir -p "$XDG_CONFIG_HOME"/git && > + touch "$XDG_CONFIG_HOME"/git/config && > + git maintenance register && > + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual && > + pwd >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' ' > + test_when_finished git maintenance unregister && > + test_path_is_missing "$XDG_CONFIG_HOME"/git/config && > + git maintenance register && > + git config --global --get maintenance.repo >actual && > + pwd >expect && > + test_cmp expect actual > +' > + As you also change the code of `git maintenance unregister`, should we test its behaviour, as well? Patrick > test_expect_success 'maintenance.<task>.enabled' ' > git config maintenance.gc.enabled false && > git config maintenance.commit-graph.enabled true && > -- > 2.42.0.2.g879ad04204 >
On Mon, Oct 23, 2023 at 5:58 AM Patrick Steinhardt <ps@pks.im> wrote: > On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote: > > `git maintenance register` registers the repository in the user's global > > config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if > > `~/.gitconfig` does not exist. However, this command creates a > > `~/.gitconfig` file and writes to that one even though the XDG variant > > exists. > > > > This used to work correctly until 50a044f1e4 (gc: replace config > > subprocesses with API calls, 2022-09-27), when the command started calling > > the config API instead of git-config(1). > > > > Also change `unregister` accordingly. > > > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > --- > > +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' > > + XDG_CONFIG_HOME=.config && > > + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config && > > + export "XDG_CONFIG_HOME" && > > Also, I think we need to unset this variable at the end of this test as > tests don't run in a subshell. [...] Yup, well spotted. Almost the entire body of this test should be in a subshell to ensure that the environment variable does not live beyond the end of this test. But test_when_finished() can't be used in a subshell, so a little care is needed: test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' test_when_finished rm -r .config/git/config && ( XDG_CONFIG_HOME=.config && ... ) ' > > + mkdir -p "$XDG_CONFIG_HOME"/git && > > + touch "$XDG_CONFIG_HOME"/git/config && If the timestamp of the file is not significant, then we use `>` to create it rather than `touch`: >"$XDG_CONFIG_HOME"/git/config && > > + git maintenance register && > > + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual && > > + pwd >expect && > > + test_cmp expect actual > > +'
diff --git a/builtin/gc.c b/builtin/gc.c index 17fc031f63a..7b780f2ab38 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) if (!found) { int rc; - char *user_config = NULL, *xdg_config = NULL; - if (!config_file) { - git_global_config_paths(&user_config, &xdg_config); - config_file = user_config; - if (!user_config) - die(_("$HOME not set")); - } + if (!config_file) + config_file = git_global_config(); rc = git_config_set_multivar_in_file_gently( config_file, "maintenance.repo", maintpath, CONFIG_REGEX_NONE, 0); - free(user_config); - free(xdg_config); if (rc) die(_("unable to add '%s' value of '%s'"), @@ -1595,18 +1588,12 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi if (found) { int rc; - char *user_config = NULL, *xdg_config = NULL; - if (!config_file) { - git_global_config_paths(&user_config, &xdg_config); - config_file = user_config; - if (!user_config) - die(_("$HOME not set")); - } + + if (!config_file) + config_file = git_global_config(); rc = git_config_set_multivar_in_file_gently( config_file, key, NULL, maintpath, CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); - free(user_config); - free(xdg_config); if (rc && (!force || rc == CONFIG_NOTHING_SET)) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 487e326b3fa..a11e6c61520 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' ' test_subcommand ! git maintenance run --auto --quiet <false ' +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' + XDG_CONFIG_HOME=.config && + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config && + export "XDG_CONFIG_HOME" && + mkdir -p "$XDG_CONFIG_HOME"/git && + touch "$XDG_CONFIG_HOME"/git/config && + git maintenance register && + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +' + +test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' ' + test_when_finished git maintenance unregister && + test_path_is_missing "$XDG_CONFIG_HOME"/git/config && + git maintenance register && + git config --global --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +' + test_expect_success 'maintenance.<task>.enabled' ' git config maintenance.gc.enabled false && git config maintenance.commit-graph.enabled true &&
`git maintenance register` registers the repository in the user's global config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if `~/.gitconfig` does not exist. However, this command creates a `~/.gitconfig` file and writes to that one even though the XDG variant exists. This used to work correctly until 50a044f1e4 (gc: replace config subprocesses with API calls, 2022-09-27), when the command started calling the config API instead of git-config(1). Also change `unregister` accordingly. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- builtin/gc.c | 23 +++++------------------ t/t7900-maintenance.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 18 deletions(-)