{"thread":{"id":"58770","subject":"[PATCH v2 0/2] git-maintenance quality-of-life improvements","startedAt":"2022-11-08T19:50:01Z","lastAt":"2022-11-10T02:38:49Z","messageCount":10,"participants":["Ronan Pigott","Taylor Blau","Phillip Wood","ronan@rjp.ie","Derrick Stolee"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"466887","messageId":"20221108194930.25805-1-ronan@rjp.ie","threadId":"58770","inReplyTo":null,"subject":"[PATCH v2 0/2] git-maintenance quality-of-life improvements","fromName":"Ronan Pigott","fromEmail":"ronan@rjp.ie","sentAt":"2022-11-08T19:49:28Z","receivedAt":"2022-11-08T19:50:01Z","isPatch":true,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"This is a pair of quality-of-life patches for git-maintenance that have\nhelped me in my workflow. PTAL.\n\nChanges from v1:\n - Added a test case for path interpolation in for-each-repo\n - Changed [--config <file>] to [--config-file <path>] in\n   register/unregister\n - Dropped the short-name option, -c\n - Avoid finding the global config when the user has supplied the config\n   on the command line\n\nRonan Pigott (2):\n  for-each-repo: interpolate repo path arguments\n  maintenance: add option to register in a specific config\n\n Documentation/git-maintenance.txt | 14 ++++-----\n builtin/for-each-repo.c           |  5 ++-\n builtin/gc.c                      | 51 +++++++++++++++++++------------\n t/t0068-for-each-repo.sh          |  6 ++++\n t/t7900-maintenance.sh            | 15 +++++++++\n 5 files changed, 64 insertions(+), 27 deletions(-)\n\n-- \n2.38.1\n\n"},{"id":"466888","messageId":"20221108194930.25805-2-ronan@rjp.ie","threadId":"58770","inReplyTo":"20221108194930.25805-1-ronan@rjp.ie","subject":"[PATCH v2 1/2] for-each-repo: interpolate repo path arguments","fromName":"Ronan Pigott","fromEmail":"ronan@rjp.ie","sentAt":"2022-11-08T19:49:29Z","receivedAt":"2022-11-08T19:50:03Z","isPatch":true,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"This is a quality of life change for git-maintenance, so repos can be\nrecorded with the tilde syntax. The register subcommand will not record\nrepos in this format by default.\n\nSigned-off-by: Ronan Pigott <ronan@rjp.ie>\n---\n builtin/for-each-repo.c  | 5 ++++-\n t/t0068-for-each-repo.sh | 6 ++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex d45d873f57..6aeac37148 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -14,13 +14,16 @@ static int run_command_on_repo(const char *path, int argc, const char ** argv)\n {\n \tint i;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n+\tchar *abspath = interpolate_path(path, 0);\n \n \tchild.git_cmd = 1;\n-\tstrvec_pushl(&child.args, \"-C\", path, NULL);\n+\tstrvec_pushl(&child.args, \"-C\", abspath, NULL);\n \n \tfor (i = 0; i < argc; i++)\n \t\tstrvec_push(&child.args, argv[i]);\n \n+\tfree(abspath);\n+\n \treturn run_command(&child);\n }\n \ndiff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\nindex 4675e85251..c6e0d65563 100755\n--- a/t/t0068-for-each-repo.sh\n+++ b/t/t0068-for-each-repo.sh\n@@ -8,9 +8,11 @@ test_expect_success 'run based on configured value' '\n \tgit init one &&\n \tgit init two &&\n \tgit init three &&\n+\tgit init ~/four &&\n \tgit -C two commit --allow-empty -m \"DID NOT RUN\" &&\n \tgit config run.key \"$TRASH_DIRECTORY/one\" &&\n \tgit config --add run.key \"$TRASH_DIRECTORY/three\" &&\n+\tgit config --add run.key \"~/four\" &&\n \tgit for-each-repo --config=run.key commit --allow-empty -m \"ran\" &&\n \tgit -C one log -1 --pretty=format:%s >message &&\n \tgrep ran message &&\n@@ -18,12 +20,16 @@ test_expect_success 'run based on configured value' '\n \t! grep ran message &&\n \tgit -C three log -1 --pretty=format:%s >message &&\n \tgrep ran message &&\n+\tgit -C ~/four log -1 --pretty=format:%s >message &&\n+\tgrep ran message &&\n \tgit for-each-repo --config=run.key -- commit --allow-empty -m \"ran again\" &&\n \tgit -C one log -1 --pretty=format:%s >message &&\n \tgrep again message &&\n \tgit -C two log -1 --pretty=format:%s >message &&\n \t! grep again message &&\n \tgit -C three log -1 --pretty=format:%s >message &&\n+\tgrep again message &&\n+\tgit -C ~/four log -1 --pretty=format:%s >message &&\n \tgrep again message\n '\n \n-- \n2.38.1\n\n"},{"id":"466889","messageId":"20221108194930.25805-3-ronan@rjp.ie","threadId":"58770","inReplyTo":"20221108194930.25805-1-ronan@rjp.ie","subject":"[PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"Ronan Pigott","fromEmail":"ronan@rjp.ie","sentAt":"2022-11-08T19:49:30Z","receivedAt":"2022-11-08T19:50:04Z","isPatch":true,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"maintenance register currently records the maintenance repo exclusively\nwithin the user's global configuration, but other configuration files\nmay be relevant when running maintenance if they are included from the\nglobal config. This option allows the user to choose where maintenance\nrepos are recorded.\n\nSigned-off-by: Ronan Pigott <ronan@rjp.ie>\n---\n Documentation/git-maintenance.txt | 14 ++++-----\n builtin/gc.c                      | 51 +++++++++++++++++++------------\n t/t7900-maintenance.sh            | 15 +++++++++\n 3 files changed, 54 insertions(+), 26 deletions(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex bb888690e4..eb3ae9fbd5 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -50,13 +50,13 @@ stop::\n \tthe background maintenance is restarted later.\n \n register::\n-\tInitialize Git config values so any scheduled maintenance will\n-\tstart running on this repository. This adds the repository to the\n-\t`maintenance.repo` config variable in the current user's global\n-\tconfig and enables some recommended configuration values for\n-\t`maintenance.<task>.schedule`. The tasks that are enabled are safe\n-\tfor running in the background without disrupting foreground\n-\tprocesses.\n+\tInitialize Git config values so any scheduled maintenance will start\n+\trunning on this repository. This adds the repository to the\n+\t`maintenance.repo` config variable in the current user's global config,\n+\tor the config specified by --config option, and enables some\n+\trecommended configuration values for `maintenance.<task>.schedule`. The\n+\ttasks that are enabled are safe for running in the background without\n+\tdisrupting foreground processes.\n +\n The `register` subcommand will also set the `maintenance.strategy` config\n value to `incremental`, if this value is not previously set. The\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 24ea85c7af..1709355bce 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1454,13 +1454,15 @@ static char *get_maintpath(void)\n }\n \n static char const * const builtin_maintenance_register_usage[] = {\n-\t\"git maintenance register\",\n+\t\"git maintenance register [--config-file <path>]\",\n \tNULL\n };\n \n static int maintenance_register(int argc, const char **argv, const char *prefix)\n {\n+\tchar *config_file = NULL;\n \tstruct option options[] = {\n+\t\tOPT_STRING(0, \"config-file\", &config_file, N_(\"file\"), N_(\"use given config file\")),\n \t\tOPT_END(),\n \t};\n \tint found = 0;\n@@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \n \tif (!found) {\n \t\tint rc;\n-\t\tchar *user_config, *xdg_config;\n-\t\tgit_global_config(&user_config, &xdg_config);\n-\t\tif (!user_config)\n-\t\t\tdie(_(\"$HOME not set\"));\n+\t\tchar *user_config = NULL, *xdg_config = NULL;\n+\n+\t\tif (!config_file) {\n+\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tconfig_file = user_config;\n+\t\t\tif (!user_config)\n+\t\t\t\tdie(_(\"$HOME not set\"));\n+\t\t}\n \t\trc = git_config_set_multivar_in_file_gently(\n-\t\t\tuser_config, \"maintenance.repo\", maintpath,\n+\t\t\tconfig_file, \"maintenance.repo\", maintpath,\n \t\t\tCONFIG_REGEX_NONE, 0);\n \t\tfree(user_config);\n \t\tfree(xdg_config);\n@@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n }\n \n static char const * const builtin_maintenance_unregister_usage[] = {\n-\t\"git maintenance unregister [--force]\",\n+\t\"git maintenance unregister [--config-file <path>] [--force]\",\n \tNULL\n };\n \n static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n {\n \tint force = 0;\n+\tchar *config_file = NULL;\n \tstruct option options[] = {\n+\t\tOPT_STRING(0, \"config-file\", &config_file, N_(\"file\"), N_(\"use given config file\")),\n \t\tOPT__FORCE(&force,\n \t\t\t   N_(\"return success even if repository was not registered\"),\n \t\t\t   PARSE_OPT_NOCOMPLETE),\n@@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tusage_with_options(builtin_maintenance_unregister_usage,\n \t\t\t\t   options);\n \n-\tlist = git_config_get_value_multi(key);\n-\tif (list) {\n-\t\tfor_each_string_list_item(item, list) {\n-\t\t\tif (!strcmp(maintpath, item->string)) {\n-\t\t\t\tfound = 1;\n-\t\t\t\tbreak;\n+\tif (!config_file) {\n+\t\tlist = git_config_get_value_multi(key);\n+\t\tif (list) {\n+\t\t\tfor_each_string_list_item(item, list) {\n+\t\t\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t}\n \n-\tif (found) {\n+\tif (found || config_file) {\n \t\tint rc;\n-\t\tchar *user_config, *xdg_config;\n-\t\tgit_global_config(&user_config, &xdg_config);\n-\t\tif (!user_config)\n-\t\t\tdie(_(\"$HOME not set\"));\n+\t\tchar *user_config = NULL, *xdg_config = NULL;\n+\t\tif (!config_file) {\n+\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tconfig_file = user_config;\n+\t\t\tif (!user_config)\n+\t\t\t\tdie(_(\"$HOME not set\"));\n+\t\t}\n \t\trc = git_config_set_multivar_in_file_gently(\n-\t\t\tuser_config, key, NULL, maintpath,\n+\t\t\tconfig_file, key, NULL, maintpath,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n \t\tfree(user_config);\n \t\tfree(xdg_config);\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 96bdd42045..091da683a8 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '\n \tgit config --global --get-all maintenance.repo >actual &&\n \ttest_cmp before actual &&\n \n+\tgit config --file ./other --add maintenance.repo /existing1 &&\n+\tgit config --file ./other --add maintenance.repo /existing2 &&\n+\tgit config --file ./other --get-all maintenance.repo >before &&\n+\n+\tgit maintenance register --config-file ./other &&\n+\ttest_cmp_config false maintenance.auto &&\n+\tgit config --file ./other --get-all maintenance.repo >between &&\n+\tcp before expect &&\n+\tpwd >>expect &&\n+\ttest_cmp expect between &&\n+\n+\tgit maintenance unregister --config-file ./other &&\n+\tgit config --file ./other --get-all maintenance.repo >actual &&\n+\ttest_cmp before actual &&\n+\n \ttest_must_fail git maintenance unregister 2>err &&\n \tgrep \"is not registered\" err &&\n \tgit maintenance unregister --force\n-- \n2.38.1\n\n"},{"id":"466890","messageId":"Y2qz6yxvJeiEQyb7@nand.local","threadId":"58770","inReplyTo":"20221108194930.25805-1-ronan@rjp.ie","subject":"Re: [PATCH v2 0/2] git-maintenance quality-of-life improvements","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T19:54:19Z","receivedAt":"2022-11-08T19:54:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Ronan,\n\nOn Tue, Nov 08, 2022 at 12:49:28PM -0700, Ronan Pigott wrote:\n> Ronan Pigott (2):\n>   for-each-repo: interpolate repo path arguments\n>   maintenance: add option to register in a specific config\n\nThanks, this round looks good to me. Will queue.\n\nThanks,\nTaylor\n"},{"id":"466895","messageId":"448cc6ed-c441-85a3-2780-0c07e56f53f8@dunelm.org.uk","threadId":"58770","inReplyTo":"20221108194930.25805-3-ronan@rjp.ie","subject":"Re: [PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-08T20:38:51Z","receivedAt":"2022-11-08T20:39:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ronan\n\nI think both these patches are useful improvements, thanks for working \non them. I've left one comment below\n\nOn 08/11/2022 19:49, Ronan Pigott wrote:\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 24ea85c7af..1709355bce 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n>   \n>   \tif (!found) {\n>   \t\tint rc;\n> -\t\tchar *user_config, *xdg_config;\n> -\t\tgit_global_config(&user_config, &xdg_config);\n> -\t\tif (!user_config)\n> -\t\t\tdie(_(\"$HOME not set\"));\n> +\t\tchar *user_config = NULL, *xdg_config = NULL;\n> +\n> +\t\tif (!config_file) {\n> +\t\t\tgit_global_config(&user_config, &xdg_config);\n> +\t\t\tconfig_file = user_config;\n\nHere we need to decide whether to use user_config or xdg_config as the \nconfig file. In builtin/config.c we do this with\n\n\t\tif (access_or_warn(user_config, R_OK, 0) &&\n\t\t    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n\t\t\tgiven_config_source.file = xdg_config;\n\t\t\tfree(user_config);\n\t\t} else {\n\t\t\tgiven_config_source.file = user_config;\n\t\t\tfree(xdg_config);\n\t\t}\n\nWe need something similar here (maybe we should create a helper function \nto find the appropriate file)\n\n> +\t\t\tif (!user_config)\n> +\t\t\t\tdie(_(\"$HOME not set\"));\n\nThis check needs to come before deciding which config file to use\n\nBest Wishes\n\nPhillip\n\n> +\t\t}\n>   \t\trc = git_config_set_multivar_in_file_gently(\n> -\t\t\tuser_config, \"maintenance.repo\", maintpath,\n> +\t\t\tconfig_file, \"maintenance.repo\", maintpath,\n>   \t\t\tCONFIG_REGEX_NONE, 0);\n>   \t\tfree(user_config);\n>   \t\tfree(xdg_config);\n> @@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n>   }\n>   \n>   static char const * const builtin_maintenance_unregister_usage[] = {\n> -\t\"git maintenance unregister [--force]\",\n> +\t\"git maintenance unregister [--config-file <path>] [--force]\",\n>   \tNULL\n>   };\n>   \n>   static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n>   {\n>   \tint force = 0;\n> +\tchar *config_file = NULL;\n>   \tstruct option options[] = {\n> +\t\tOPT_STRING(0, \"config-file\", &config_file, N_(\"file\"), N_(\"use given config file\")),\n>   \t\tOPT__FORCE(&force,\n>   \t\t\t   N_(\"return success even if repository was not registered\"),\n>   \t\t\t   PARSE_OPT_NOCOMPLETE),\n> @@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n>   \t\tusage_with_options(builtin_maintenance_unregister_usage,\n>   \t\t\t\t   options);\n>   \n> -\tlist = git_config_get_value_multi(key);\n> -\tif (list) {\n> -\t\tfor_each_string_list_item(item, list) {\n> -\t\t\tif (!strcmp(maintpath, item->string)) {\n> -\t\t\t\tfound = 1;\n> -\t\t\t\tbreak;\n> +\tif (!config_file) {\n> +\t\tlist = git_config_get_value_multi(key);\n> +\t\tif (list) {\n> +\t\t\tfor_each_string_list_item(item, list) {\n> +\t\t\t\tif (!strcmp(maintpath, item->string)) {\n> +\t\t\t\t\tfound = 1;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n>   \t\t\t}\n>   \t\t}\n>   \t}\n>   \n> -\tif (found) {\n> +\tif (found || config_file) {\n>   \t\tint rc;\n> -\t\tchar *user_config, *xdg_config;\n> -\t\tgit_global_config(&user_config, &xdg_config);\n> -\t\tif (!user_config)\n> -\t\t\tdie(_(\"$HOME not set\"));\n> +\t\tchar *user_config = NULL, *xdg_config = NULL;\n> +\t\tif (!config_file) {\n> +\t\t\tgit_global_config(&user_config, &xdg_config);\n> +\t\t\tconfig_file = user_config;\n> +\t\t\tif (!user_config)\n> +\t\t\t\tdie(_(\"$HOME not set\"));\n> +\t\t}\n>   \t\trc = git_config_set_multivar_in_file_gently(\n> -\t\t\tuser_config, key, NULL, maintpath,\n> +\t\t\tconfig_file, key, NULL, maintpath,\n>   \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n>   \t\tfree(user_config);\n>   \t\tfree(xdg_config);\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 96bdd42045..091da683a8 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '\n>   \tgit config --global --get-all maintenance.repo >actual &&\n>   \ttest_cmp before actual &&\n>   \n> +\tgit config --file ./other --add maintenance.repo /existing1 &&\n> +\tgit config --file ./other --add maintenance.repo /existing2 &&\n> +\tgit config --file ./other --get-all maintenance.repo >before &&\n> +\n> +\tgit maintenance register --config-file ./other &&\n> +\ttest_cmp_config false maintenance.auto &&\n> +\tgit config --file ./other --get-all maintenance.repo >between &&\n> +\tcp before expect &&\n> +\tpwd >>expect &&\n> +\ttest_cmp expect between &&\n> +\n> +\tgit maintenance unregister --config-file ./other &&\n> +\tgit config --file ./other --get-all maintenance.repo >actual &&\n> +\ttest_cmp before actual &&\n> +\n>   \ttest_must_fail git maintenance unregister 2>err &&\n>   \tgrep \"is not registered\" err &&\n>   \tgit maintenance unregister --force\n"},{"id":"466898","messageId":"Y2rCsq5y9TuCfrKC@nand.local","threadId":"58770","inReplyTo":"448cc6ed-c441-85a3-2780-0c07e56f53f8@dunelm.org.uk","subject":"Re: [PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T20:57:22Z","receivedAt":"2022-11-08T20:57:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 08, 2022 at 08:38:51PM +0000, Phillip Wood wrote:\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index 24ea85c7af..1709355bce 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n> >   \tif (!found) {\n> >   \t\tint rc;\n> > -\t\tchar *user_config, *xdg_config;\n> > -\t\tgit_global_config(&user_config, &xdg_config);\n> > -\t\tif (!user_config)\n> > -\t\t\tdie(_(\"$HOME not set\"));\n> > +\t\tchar *user_config = NULL, *xdg_config = NULL;\n> > +\n> > +\t\tif (!config_file) {\n> > +\t\t\tgit_global_config(&user_config, &xdg_config);\n> > +\t\t\tconfig_file = user_config;\n>\n> Here we need to decide whether to use user_config or xdg_config as the\n> config file. In builtin/config.c we do this with\n>\n> \t\tif (access_or_warn(user_config, R_OK, 0) &&\n> \t\t    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n> \t\t\tgiven_config_source.file = xdg_config;\n> \t\t\tfree(user_config);\n> \t\t} else {\n> \t\t\tgiven_config_source.file = user_config;\n> \t\t\tfree(xdg_config);\n> \t\t}\n>\n> We need something similar here (maybe we should create a helper function to\n> find the appropriate file)\n>\n> > +\t\t\tif (!user_config)\n> > +\t\t\t\tdie(_(\"$HOME not set\"));\n>\n> This check needs to come before deciding which config file to use\n\nTrue, but that problem existed before this series, too. So the new\nbehavior is no worse with respect to the XDG config stuff, and any\nimprovements to that behavior can be done independently on top.\n\nThanks,\nTaylor\n"},{"id":"466902","messageId":"7730f942-7332-4408-a2c0-f6ccde3cdb3a@dunelm.org.uk","threadId":"58770","inReplyTo":"Y2rCsq5y9TuCfrKC@nand.local","subject":"Re: [PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-08T21:28:25Z","receivedAt":"2022-11-08T21:28:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Taylor\n\nOn 08/11/2022 20:57, Taylor Blau wrote:\n> On Tue, Nov 08, 2022 at 08:38:51PM +0000, Phillip Wood wrote:\n>>> diff --git a/builtin/gc.c b/builtin/gc.c\n>>> index 24ea85c7af..1709355bce 100644\n>>> --- a/builtin/gc.c\n>>> +++ b/builtin/gc.c\n>>> @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n>>>    \tif (!found) {\n>>>    \t\tint rc;\n>>> -\t\tchar *user_config, *xdg_config;\n>>> -\t\tgit_global_config(&user_config, &xdg_config);\n>>> -\t\tif (!user_config)\n>>> -\t\t\tdie(_(\"$HOME not set\"));\n>>> +\t\tchar *user_config = NULL, *xdg_config = NULL;\n>>> +\n>>> +\t\tif (!config_file) {\n>>> +\t\t\tgit_global_config(&user_config, &xdg_config);\n>>> +\t\t\tconfig_file = user_config;\n>>\n>> Here we need to decide whether to use user_config or xdg_config as the\n>> config file. In builtin/config.c we do this with\n>>\n>> \t\tif (access_or_warn(user_config, R_OK, 0) &&\n>> \t\t    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n>> \t\t\tgiven_config_source.file = xdg_config;\n>> \t\t\tfree(user_config);\n>> \t\t} else {\n>> \t\t\tgiven_config_source.file = user_config;\n>> \t\t\tfree(xdg_config);\n>> \t\t}\n>>\n>> We need something similar here (maybe we should create a helper function to\n>> find the appropriate file)\n>>\n>>> +\t\t\tif (!user_config)\n>>> +\t\t\t\tdie(_(\"$HOME not set\"));\n>>\n>> This check needs to come before deciding which config file to use\n> \n> True, but that problem existed before this series, too.\n\nOh yes, sorry I'd missed that\n\nPhillip\n\n> So the new\n> behavior is no worse with respect to the XDG config stuff, and any\n> improvements to that behavior can be done independently on top. >> Thanks,\n> Taylor\n"},{"id":"466918","messageId":"3f2efad81c08dce739ece667331738bb@rjp.ie","threadId":"58770","inReplyTo":"7730f942-7332-4408-a2c0-f6ccde3cdb3a@dunelm.org.uk","subject":"Re: [PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"","fromEmail":"ronan@rjp.ie","sentAt":"2022-11-08T22:45:29Z","receivedAt":"2022-11-08T22:45:36Z","isPatch":true,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"Yes, I saw that the xdg_config was ignored but did not try to change it.\n\nIf we add a helper function for this in the future it might be worth considering\nsome of the lesser supported parts of the xdg spec, specifically XDG_CONFIG_DIRS\nwhich I use with neovim, for example, to achieve some more host-specific configurations.\n\nThe xdg spec isn't totally clear on how these additional configuration directories should\nbe used imo, so it would also be acceptable to just ignore them as is currently done.\n"},{"id":"466959","messageId":"df1d3485-3913-c132-83b5-09ca2408000b@github.com","threadId":"58770","inReplyTo":"20221108194930.25805-3-ronan@rjp.ie","subject":"Re: [PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-09T15:05:32Z","receivedAt":"2022-11-09T15:05:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/8/2022 2:49 PM, Ronan Pigott wrote:\n> maintenance register currently records the maintenance repo exclusively\n> within the user's global configuration, but other configuration files\n> may be relevant when running maintenance if they are included from the\n> global config. This option allows the user to choose where maintenance\n> repos are recorded.\n\n> +\tInitialize Git config values so any scheduled maintenance will start\n> +\trunning on this repository. This adds the repository to the\n> +\t`maintenance.repo` config variable in the current user's global config,\n> +\tor the config specified by --config option, and enables some\n\nThis still says --config...\n\n> +\trecommended configuration values for `maintenance.<task>.schedule`. The\n> +\ttasks that are enabled are safe for running in the background without\n> +\tdisrupting foreground processes.\n\n> +\t\"git maintenance register [--config-file <path>]\",\n\n> +\tchar *config_file = NULL;\n>  \tstruct option options[] = {\n> +\t\tOPT_STRING(0, \"config-file\", &config_file, N_(\"file\"), N_(\"use given config file\")),\n\n>  static char const * const builtin_maintenance_unregister_usage[] = {\n> -\t\"git maintenance unregister [--force]\",\n> +\t\"git maintenance unregister [--config-file <path>] [--force]\",\n\n> +\tchar *config_file = NULL;\n>  \tstruct option options[] = {\n> +\t\tOPT_STRING(0, \"config-file\", &config_file, N_(\"file\"), N_(\"use given config file\")),\n\n...but these have been updated. Easy to miss that doc update.\n\n> -\tlist = git_config_get_value_multi(key);\n> -\tif (list) {\n> -\t\tfor_each_string_list_item(item, list) {\n> -\t\t\tif (!strcmp(maintpath, item->string)) {\n> -\t\t\t\tfound = 1;\n> -\t\t\t\tbreak;\n> +\tif (!config_file) {\n> +\t\tlist = git_config_get_value_multi(key);\n> +\t\tif (list) {\n> +\t\t\tfor_each_string_list_item(item, list) {\n> +\t\t\t\tif (!strcmp(maintpath, item->string)) {\n> +\t\t\t\t\tfound = 1;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n>  \t\t\t}\n>  \t\t}\n>  \t}\n>  \n> -\tif (found) {\n> +\tif (found || config_file) {\n\nThis seems like it will attempt to update the given config file\nregardless of its contents. This might result in failures when the\nfile does not have that value, which is different than the typical\ncase.\n\nI think the right thing to do would be to update the first\n\"if (!config_file)\" block to have an else.\n\n\n\tif (!config_file)\n\t\tlist = git_config_get_value_multi(key);\n\telse\n\t\tlist = <get multi-value from the file>\n\n\tif (list) {\n\t\tfor_each_string_list_item(item, list) {\n\t\t\t...\n\nThen the \"if (found)\" case can be the same.\n\n>  \t\tint rc;\n> -\t\tchar *user_config, *xdg_config;\n> -\t\tgit_global_config(&user_config, &xdg_config);\n> -\t\tif (!user_config)\n> -\t\t\tdie(_(\"$HOME not set\"));\n> +\t\tchar *user_config = NULL, *xdg_config = NULL;\n> +\t\tif (!config_file) {\n> +\t\t\tgit_global_config(&user_config, &xdg_config);\n> +\t\t\tconfig_file = user_config;\n> +\t\t\tif (!user_config)\n> +\t\t\t\tdie(_(\"$HOME not set\"));\n> +\t\t}\n>  \t\trc = git_config_set_multivar_in_file_gently(\n> -\t\t\tuser_config, key, NULL, maintpath,\n> +\t\t\tconfig_file, key, NULL, maintpath,\n>  \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n\nat least until this part that handles the config write.\n\n>  \t\tfree(user_config);\n>  \t\tfree(xdg_config);\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 96bdd42045..091da683a8 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '\n>  \tgit config --global --get-all maintenance.repo >actual &&\n>  \ttest_cmp before actual &&\n>  \n> +\tgit config --file ./other --add maintenance.repo /existing1 &&\n> +\tgit config --file ./other --add maintenance.repo /existing2 &&\n> +\tgit config --file ./other --get-all maintenance.repo >before &&\n> +\n> +\tgit maintenance register --config-file ./other &&\n> +\ttest_cmp_config false maintenance.auto &&\n> +\tgit config --file ./other --get-all maintenance.repo >between &&\n> +\tcp before expect &&\n> +\tpwd >>expect &&\n> +\ttest_cmp expect between &&\n> +\n> +\tgit maintenance unregister --config-file ./other &&\n> +\tgit config --file ./other --get-all maintenance.repo >actual &&\n> +\ttest_cmp before actual &&\n> +\n\nPlease add a case of `git maintenance unregister --config-file` where\nthe given file does not already have that repo listed to make sure we\ncover that case.\n\nThanks,\n-Stolee\n"},{"id":"467026","messageId":"Y2xkMnQQ4w8rhSwD@nand.local","threadId":"58770","inReplyTo":"df1d3485-3913-c132-83b5-09ca2408000b@github.com","subject":"Re: [PATCH v2 2/2] maintenance: add option to register in a specific config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-10T02:38:42Z","receivedAt":"2022-11-10T02:38:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 09, 2022 at 10:05:32AM -0500, Derrick Stolee wrote:\n> On 11/8/2022 2:49 PM, Ronan Pigott wrote:\n> > maintenance register currently records the maintenance repo exclusively\n> > within the user's global configuration, but other configuration files\n> > may be relevant when running maintenance if they are included from the\n> > global config. This option allows the user to choose where maintenance\n> > repos are recorded.\n>\n> > +\tInitialize Git config values so any scheduled maintenance will start\n> > +\trunning on this repository. This adds the repository to the\n> > +\t`maintenance.repo` config variable in the current user's global config,\n> > +\tor the config specified by --config option, and enables some\n>\n> This still says --config...\n\nThanks for spotting. I graduated this topic too early, it seems. Let's\nhold it in 'next' while we wait for a patch or two on top, or I can see\nabout reverting what's in 'next' and replacing it with a new round.\n\nThanks,\nTaylor\n"}]}