| Message ID | 20221108194930.25805-3-ronan@rjp.ie (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-maintenance quality-of-life improvements | expand |
Hi Ronan I think both these patches are useful improvements, thanks for working on them. I've left one comment below On 08/11/2022 19:49, Ronan Pigott wrote: > diff --git a/builtin/gc.c b/builtin/gc.c > index 24ea85c7af..1709355bce 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) > > if (!found) { > int rc; > - char *user_config, *xdg_config; > - git_global_config(&user_config, &xdg_config); > - if (!user_config) > - die(_("$HOME not set")); > + char *user_config = NULL, *xdg_config = NULL; > + > + if (!config_file) { > + git_global_config(&user_config, &xdg_config); > + config_file = user_config; Here we need to decide whether to use user_config or xdg_config as the config file. In builtin/config.c we do this with if (access_or_warn(user_config, R_OK, 0) && xdg_config && !access_or_warn(xdg_config, R_OK, 0)) { given_config_source.file = xdg_config; free(user_config); } else { given_config_source.file = user_config; free(xdg_config); } We need something similar here (maybe we should create a helper function to find the appropriate file) > + if (!user_config) > + die(_("$HOME not set")); This check needs to come before deciding which config file to use Best Wishes Phillip > + } > rc = git_config_set_multivar_in_file_gently( > - user_config, "maintenance.repo", maintpath, > + config_file, "maintenance.repo", maintpath, > CONFIG_REGEX_NONE, 0); > free(user_config); > free(xdg_config); > @@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) > } > > static char const * const builtin_maintenance_unregister_usage[] = { > - "git maintenance unregister [--force]", > + "git maintenance unregister [--config-file <path>] [--force]", > NULL > }; > > static int maintenance_unregister(int argc, const char **argv, const char *prefix) > { > int force = 0; > + char *config_file = NULL; > struct option options[] = { > + OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")), > OPT__FORCE(&force, > N_("return success even if repository was not registered"), > PARSE_OPT_NOCOMPLETE), > @@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi > usage_with_options(builtin_maintenance_unregister_usage, > options); > > - list = git_config_get_value_multi(key); > - if (list) { > - for_each_string_list_item(item, list) { > - if (!strcmp(maintpath, item->string)) { > - found = 1; > - break; > + if (!config_file) { > + list = git_config_get_value_multi(key); > + if (list) { > + for_each_string_list_item(item, list) { > + if (!strcmp(maintpath, item->string)) { > + found = 1; > + break; > + } > } > } > } > > - if (found) { > + if (found || config_file) { > int rc; > - char *user_config, *xdg_config; > - git_global_config(&user_config, &xdg_config); > - if (!user_config) > - die(_("$HOME not set")); > + char *user_config = NULL, *xdg_config = NULL; > + if (!config_file) { > + git_global_config(&user_config, &xdg_config); > + config_file = user_config; > + if (!user_config) > + die(_("$HOME not set")); > + } > rc = git_config_set_multivar_in_file_gently( > - user_config, key, NULL, maintpath, > + config_file, key, NULL, maintpath, > CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); > free(user_config); > free(xdg_config); > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 96bdd42045..091da683a8 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' ' > git config --global --get-all maintenance.repo >actual && > test_cmp before actual && > > + git config --file ./other --add maintenance.repo /existing1 && > + git config --file ./other --add maintenance.repo /existing2 && > + git config --file ./other --get-all maintenance.repo >before && > + > + git maintenance register --config-file ./other && > + test_cmp_config false maintenance.auto && > + git config --file ./other --get-all maintenance.repo >between && > + cp before expect && > + pwd >>expect && > + test_cmp expect between && > + > + git maintenance unregister --config-file ./other && > + git config --file ./other --get-all maintenance.repo >actual && > + test_cmp before actual && > + > test_must_fail git maintenance unregister 2>err && > grep "is not registered" err && > git maintenance unregister --force
On Tue, Nov 08, 2022 at 08:38:51PM +0000, Phillip Wood wrote: > > diff --git a/builtin/gc.c b/builtin/gc.c > > index 24ea85c7af..1709355bce 100644 > > --- a/builtin/gc.c > > +++ b/builtin/gc.c > > @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) > > if (!found) { > > int rc; > > - char *user_config, *xdg_config; > > - git_global_config(&user_config, &xdg_config); > > - if (!user_config) > > - die(_("$HOME not set")); > > + char *user_config = NULL, *xdg_config = NULL; > > + > > + if (!config_file) { > > + git_global_config(&user_config, &xdg_config); > > + config_file = user_config; > > Here we need to decide whether to use user_config or xdg_config as the > config file. In builtin/config.c we do this with > > if (access_or_warn(user_config, R_OK, 0) && > xdg_config && !access_or_warn(xdg_config, R_OK, 0)) { > given_config_source.file = xdg_config; > free(user_config); > } else { > given_config_source.file = user_config; > free(xdg_config); > } > > We need something similar here (maybe we should create a helper function to > find the appropriate file) > > > + if (!user_config) > > + die(_("$HOME not set")); > > This check needs to come before deciding which config file to use True, but that problem existed before this series, too. So the new behavior is no worse with respect to the XDG config stuff, and any improvements to that behavior can be done independently on top. Thanks, Taylor
Hi Taylor On 08/11/2022 20:57, Taylor Blau wrote: > On Tue, Nov 08, 2022 at 08:38:51PM +0000, Phillip Wood wrote: >>> diff --git a/builtin/gc.c b/builtin/gc.c >>> index 24ea85c7af..1709355bce 100644 >>> --- a/builtin/gc.c >>> +++ b/builtin/gc.c >>> @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) >>> if (!found) { >>> int rc; >>> - char *user_config, *xdg_config; >>> - git_global_config(&user_config, &xdg_config); >>> - if (!user_config) >>> - die(_("$HOME not set")); >>> + char *user_config = NULL, *xdg_config = NULL; >>> + >>> + if (!config_file) { >>> + git_global_config(&user_config, &xdg_config); >>> + config_file = user_config; >> >> Here we need to decide whether to use user_config or xdg_config as the >> config file. In builtin/config.c we do this with >> >> if (access_or_warn(user_config, R_OK, 0) && >> xdg_config && !access_or_warn(xdg_config, R_OK, 0)) { >> given_config_source.file = xdg_config; >> free(user_config); >> } else { >> given_config_source.file = user_config; >> free(xdg_config); >> } >> >> We need something similar here (maybe we should create a helper function to >> find the appropriate file) >> >>> + if (!user_config) >>> + die(_("$HOME not set")); >> >> This check needs to come before deciding which config file to use > > True, but that problem existed before this series, too. Oh yes, sorry I'd missed that Phillip > So the new > behavior is no worse with respect to the XDG config stuff, and any > improvements to that behavior can be done independently on top. >> Thanks, > Taylor
Yes, I saw that the xdg_config was ignored but did not try to change it. If we add a helper function for this in the future it might be worth considering some of the lesser supported parts of the xdg spec, specifically XDG_CONFIG_DIRS which I use with neovim, for example, to achieve some more host-specific configurations. The xdg spec isn't totally clear on how these additional configuration directories should be used imo, so it would also be acceptable to just ignore them as is currently done.
On 11/8/2022 2:49 PM, Ronan Pigott wrote: > maintenance register currently records the maintenance repo exclusively > within the user's global configuration, but other configuration files > may be relevant when running maintenance if they are included from the > global config. This option allows the user to choose where maintenance > repos are recorded. > + Initialize Git config values so any scheduled maintenance will start > + running on this repository. This adds the repository to the > + `maintenance.repo` config variable in the current user's global config, > + or the config specified by --config option, and enables some This still says --config... > + recommended configuration values for `maintenance.<task>.schedule`. The > + tasks that are enabled are safe for running in the background without > + disrupting foreground processes. > + "git maintenance register [--config-file <path>]", > + char *config_file = NULL; > struct option options[] = { > + OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")), > static char const * const builtin_maintenance_unregister_usage[] = { > - "git maintenance unregister [--force]", > + "git maintenance unregister [--config-file <path>] [--force]", > + char *config_file = NULL; > struct option options[] = { > + OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")), ...but these have been updated. Easy to miss that doc update. > - list = git_config_get_value_multi(key); > - if (list) { > - for_each_string_list_item(item, list) { > - if (!strcmp(maintpath, item->string)) { > - found = 1; > - break; > + if (!config_file) { > + list = git_config_get_value_multi(key); > + if (list) { > + for_each_string_list_item(item, list) { > + if (!strcmp(maintpath, item->string)) { > + found = 1; > + break; > + } > } > } > } > > - if (found) { > + if (found || config_file) { This seems like it will attempt to update the given config file regardless of its contents. This might result in failures when the file does not have that value, which is different than the typical case. I think the right thing to do would be to update the first "if (!config_file)" block to have an else. if (!config_file) list = git_config_get_value_multi(key); else list = <get multi-value from the file> if (list) { for_each_string_list_item(item, list) { ... Then the "if (found)" case can be the same. > int rc; > - char *user_config, *xdg_config; > - git_global_config(&user_config, &xdg_config); > - if (!user_config) > - die(_("$HOME not set")); > + char *user_config = NULL, *xdg_config = NULL; > + if (!config_file) { > + git_global_config(&user_config, &xdg_config); > + config_file = user_config; > + if (!user_config) > + die(_("$HOME not set")); > + } > rc = git_config_set_multivar_in_file_gently( > - user_config, key, NULL, maintpath, > + config_file, key, NULL, maintpath, > CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); at least until this part that handles the config write. > free(user_config); > free(xdg_config); > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 96bdd42045..091da683a8 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' ' > git config --global --get-all maintenance.repo >actual && > test_cmp before actual && > > + git config --file ./other --add maintenance.repo /existing1 && > + git config --file ./other --add maintenance.repo /existing2 && > + git config --file ./other --get-all maintenance.repo >before && > + > + git maintenance register --config-file ./other && > + test_cmp_config false maintenance.auto && > + git config --file ./other --get-all maintenance.repo >between && > + cp before expect && > + pwd >>expect && > + test_cmp expect between && > + > + git maintenance unregister --config-file ./other && > + git config --file ./other --get-all maintenance.repo >actual && > + test_cmp before actual && > + Please add a case of `git maintenance unregister --config-file` where the given file does not already have that repo listed to make sure we cover that case. Thanks, -Stolee
On Wed, Nov 09, 2022 at 10:05:32AM -0500, Derrick Stolee wrote: > On 11/8/2022 2:49 PM, Ronan Pigott wrote: > > maintenance register currently records the maintenance repo exclusively > > within the user's global configuration, but other configuration files > > may be relevant when running maintenance if they are included from the > > global config. This option allows the user to choose where maintenance > > repos are recorded. > > > + Initialize Git config values so any scheduled maintenance will start > > + running on this repository. This adds the repository to the > > + `maintenance.repo` config variable in the current user's global config, > > + or the config specified by --config option, and enables some > > This still says --config... Thanks for spotting. I graduated this topic too early, it seems. Let's hold it in 'next' while we wait for a patch or two on top, or I can see about reverting what's in 'next' and replacing it with a new round. Thanks, Taylor
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index bb888690e4..eb3ae9fbd5 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -50,13 +50,13 @@ stop:: the background maintenance is restarted later. register:: - Initialize Git config values so any scheduled maintenance will - start running on this repository. This adds the repository to the - `maintenance.repo` config variable in the current user's global - config and enables some recommended configuration values for - `maintenance.<task>.schedule`. The tasks that are enabled are safe - for running in the background without disrupting foreground - processes. + Initialize Git config values so any scheduled maintenance will start + running on this repository. This adds the repository to the + `maintenance.repo` config variable in the current user's global config, + or the config specified by --config option, and enables some + recommended configuration values for `maintenance.<task>.schedule`. The + tasks that are enabled are safe for running in the background without + disrupting foreground processes. + The `register` subcommand will also set the `maintenance.strategy` config value to `incremental`, if this value is not previously set. The diff --git a/builtin/gc.c b/builtin/gc.c index 24ea85c7af..1709355bce 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1454,13 +1454,15 @@ static char *get_maintpath(void) } static char const * const builtin_maintenance_register_usage[] = { - "git maintenance register", + "git maintenance register [--config-file <path>]", NULL }; static int maintenance_register(int argc, const char **argv, const char *prefix) { + char *config_file = NULL; struct option options[] = { + OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")), OPT_END(), }; int found = 0; @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) if (!found) { int rc; - char *user_config, *xdg_config; - git_global_config(&user_config, &xdg_config); - if (!user_config) - die(_("$HOME not set")); + char *user_config = NULL, *xdg_config = NULL; + + if (!config_file) { + git_global_config(&user_config, &xdg_config); + config_file = user_config; + if (!user_config) + die(_("$HOME not set")); + } rc = git_config_set_multivar_in_file_gently( - user_config, "maintenance.repo", maintpath, + config_file, "maintenance.repo", maintpath, CONFIG_REGEX_NONE, 0); free(user_config); free(xdg_config); @@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) } static char const * const builtin_maintenance_unregister_usage[] = { - "git maintenance unregister [--force]", + "git maintenance unregister [--config-file <path>] [--force]", NULL }; static int maintenance_unregister(int argc, const char **argv, const char *prefix) { int force = 0; + char *config_file = NULL; struct option options[] = { + OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")), OPT__FORCE(&force, N_("return success even if repository was not registered"), PARSE_OPT_NOCOMPLETE), @@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi usage_with_options(builtin_maintenance_unregister_usage, options); - list = git_config_get_value_multi(key); - if (list) { - for_each_string_list_item(item, list) { - if (!strcmp(maintpath, item->string)) { - found = 1; - break; + if (!config_file) { + list = git_config_get_value_multi(key); + if (list) { + for_each_string_list_item(item, list) { + if (!strcmp(maintpath, item->string)) { + found = 1; + break; + } } } } - if (found) { + if (found || config_file) { int rc; - char *user_config, *xdg_config; - git_global_config(&user_config, &xdg_config); - if (!user_config) - die(_("$HOME not set")); + char *user_config = NULL, *xdg_config = NULL; + if (!config_file) { + git_global_config(&user_config, &xdg_config); + config_file = user_config; + if (!user_config) + die(_("$HOME not set")); + } rc = git_config_set_multivar_in_file_gently( - user_config, key, NULL, maintpath, + config_file, key, NULL, maintpath, CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); free(user_config); free(xdg_config); diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 96bdd42045..091da683a8 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' ' git config --global --get-all maintenance.repo >actual && test_cmp before actual && + git config --file ./other --add maintenance.repo /existing1 && + git config --file ./other --add maintenance.repo /existing2 && + git config --file ./other --get-all maintenance.repo >before && + + git maintenance register --config-file ./other && + test_cmp_config false maintenance.auto && + git config --file ./other --get-all maintenance.repo >between && + cp before expect && + pwd >>expect && + test_cmp expect between && + + git maintenance unregister --config-file ./other && + git config --file ./other --get-all maintenance.repo >actual && + test_cmp before actual && + test_must_fail git maintenance unregister 2>err && grep "is not registered" err && git maintenance unregister --force
maintenance register currently records the maintenance repo exclusively within the user's global configuration, but other configuration files may be relevant when running maintenance if they are included from the global config. This option allows the user to choose where maintenance repos are recorded. Signed-off-by: Ronan Pigott <ronan@rjp.ie> --- Documentation/git-maintenance.txt | 14 ++++----- builtin/gc.c | 51 +++++++++++++++++++------------ t/t7900-maintenance.sh | 15 +++++++++ 3 files changed, 54 insertions(+), 26 deletions(-)