{"thread":{"id":"55192","subject":"[PATCH] maintenance: fix incorrect `maintenance.repo` path with bare repository","startedAt":"2021-02-23T07:33:24Z","lastAt":"2021-02-23T16:22:56Z","messageCount":3,"participants":["Eric Sunshine","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417543","messageId":"20210223073107.40675-1-sunshine@sunshineco.com","threadId":"55192","inReplyTo":null,"subject":"[PATCH] maintenance: fix incorrect `maintenance.repo` path with bare repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-23T07:31:07Z","receivedAt":"2021-02-23T07:33:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The periodic maintenance tasks configured by `git maintenance start`\ninvoke `git for-each-repo` to run `git maintenance run` on each path\nspecified by the multi-value global configuration variable\n`maintenance.repo`. Because `git for-each-repo` will likely be run\noutside of the repositories which require periodic maintenance, it is\nmandatory that the repository paths specified by `maintenance.repo` are\nabsolute.\n\nUnfortunately, however, `git maintenance register` does nothing to\nensure that the paths it assigns to `maintenance.repo` are indeed\nabsolute, and may in fact -- especially in the case of a bare repository\n-- assign a relative path to `maintenance.repo` instead. Fix this\nproblem by converting all paths to absolute before assigning them to\n`maintenance.repo`.\n\nWhile at it, also fix `git maintenance unregister` to convert paths to\nabsolute, as well, in order to ensure that it can correctly remove from\n`maintenance.repo` a path assigned via `git maintenance register`.\n\nReported-by: Clement Moyroud <clement.moyroud@gmail.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nReported by Clement here:\nhttps://lore.kernel.org/git/CABXAcUzRhkeQhVtwtx-NBR0hbkoo=aCTwN464Dsj8680GPMDxw@mail.gmail.com/\n\nbuiltin/gc.c           | 50 ++++++++++++++++++++++++++++--------------\n t/t7900-maintenance.sh | 13 +++++++++++\n 2 files changed, 46 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6db9cb39e6..ef7226d7bc 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1461,11 +1461,23 @@ static int maintenance_run(int argc, const char **argv, const char *prefix)\n \treturn maintenance_run_tasks(&opts);\n }\n \n+static char *get_maintpath(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *p = the_repository->worktree ?\n+\t\tthe_repository->worktree : the_repository->gitdir;\n+\n+\tstrbuf_realpath(&sb, p, 1);\n+\treturn strbuf_detach(&sb, NULL);\n+}\n+\n static int maintenance_register(void)\n {\n+\tint rc;\n \tchar *config_value;\n \tstruct child_process config_set = CHILD_PROCESS_INIT;\n \tstruct child_process config_get = CHILD_PROCESS_INIT;\n+\tchar *maintpath = get_maintpath();\n \n \t/* Disable foreground maintenance */\n \tgit_config_set(\"maintenance.auto\", \"false\");\n@@ -1478,40 +1490,44 @@ static int maintenance_register(void)\n \n \tconfig_get.git_cmd = 1;\n \tstrvec_pushl(&config_get.args, \"config\", \"--global\", \"--get\",\n-\t\t     \"--fixed-value\", \"maintenance.repo\",\n-\t\t     the_repository->worktree ? the_repository->worktree\n-\t\t\t\t\t      : the_repository->gitdir,\n-\t\t\t NULL);\n+\t\t     \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n \tconfig_get.out = -1;\n \n-\tif (start_command(&config_get))\n-\t\treturn error(_(\"failed to run 'git config'\"));\n+\tif (start_command(&config_get)) {\n+\t\trc = error(_(\"failed to run 'git config'\"));\n+\t\tgoto done;\n+\t}\n \n \t/* We already have this value in our config! */\n-\tif (!finish_command(&config_get))\n-\t\treturn 0;\n+\tif (!finish_command(&config_get)) {\n+\t\trc = 0;\n+\t\tgoto done;\n+\t}\n \n \tconfig_set.git_cmd = 1;\n \tstrvec_pushl(&config_set.args, \"config\", \"--add\", \"--global\", \"maintenance.repo\",\n-\t\t     the_repository->worktree ? the_repository->worktree\n-\t\t\t\t\t      : the_repository->gitdir,\n-\t\t     NULL);\n+\t\t     maintpath, NULL);\n+\n+\trc = run_command(&config_set);\n \n-\treturn run_command(&config_set);\n+done:\n+\tfree(maintpath);\n+\treturn rc;\n }\n \n static int maintenance_unregister(void)\n {\n+\tint rc;\n \tstruct child_process config_unset = CHILD_PROCESS_INIT;\n+\tchar *maintpath = get_maintpath();\n \n \tconfig_unset.git_cmd = 1;\n \tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t     \"--fixed-value\", \"maintenance.repo\",\n-\t\t     the_repository->worktree ? the_repository->worktree\n-\t\t\t\t\t      : the_repository->gitdir,\n-\t\t     NULL);\n+\t\t     \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n \n-\treturn run_command(&config_unset);\n+\trc = run_command(&config_unset);\n+\tfree(maintpath);\n+\treturn rc;\n }\n \n static const char *get_frequency(enum schedule_priority schedule)\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 286b18db3c..2412d8c5c0 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -632,4 +632,17 @@ test_expect_success 'fails when running outside of a repository' '\n \tnongit test_must_fail git maintenance unregister\n '\n \n+test_expect_success 'register and unregister bare repo' '\n+\ttest_when_finished \"git config --global --unset-all maintenance.repo || :\" &&\n+\ttest_might_fail git config --global --unset-all maintenance.repo &&\n+\tgit init --bare barerepo &&\n+\t(\n+\t\tcd barerepo &&\n+\t\tgit maintenance register &&\n+\t\tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n+\t\tgit maintenance unregister &&\n+\t\ttest_must_fail git config --global --get-all maintenance.repo\n+\t)\n+'\n+\n test_done\n-- \n2.30.1.766.gb4fecdf3b7\n\n"},{"id":"417561","messageId":"5653230a-20c2-321f-7ea0-93ce59858532@gmail.com","threadId":"55192","inReplyTo":"20210223073107.40675-1-sunshine@sunshineco.com","subject":"Re: [PATCH] maintenance: fix incorrect `maintenance.repo` path with bare repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-23T12:25:57Z","receivedAt":"2021-02-23T12:26:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/23/2021 2:31 AM, Eric Sunshine wrote:\n> The periodic maintenance tasks configured by `git maintenance start`\n> invoke `git for-each-repo` to run `git maintenance run` on each path\n> specified by the multi-value global configuration variable\n> `maintenance.repo`. Because `git for-each-repo` will likely be run\n> outside of the repositories which require periodic maintenance, it is\n> mandatory that the repository paths specified by `maintenance.repo` are\n> absolute.\n> \n> Unfortunately, however, `git maintenance register` does nothing to\n> ensure that the paths it assigns to `maintenance.repo` are indeed\n> absolute, and may in fact -- especially in the case of a bare repository\n> -- assign a relative path to `maintenance.repo` instead. Fix this\n> problem by converting all paths to absolute before assigning them to\n> `maintenance.repo`.\n> \n> While at it, also fix `git maintenance unregister` to convert paths to\n> absolute, as well, in order to ensure that it can correctly remove from\n> `maintenance.repo` a path assigned via `git maintenance register`.\n\nThanks for the report and the fix!\n\n> +static char *get_maintpath(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tconst char *p = the_repository->worktree ?\n> +\t\tthe_repository->worktree : the_repository->gitdir;\n> +\n> +\tstrbuf_realpath(&sb, p, 1);\n> +\treturn strbuf_detach(&sb, NULL);\n> +}\n> +\n\nThis looks right.\n\n>  static int maintenance_register(void)\n>  {\n> +\tint rc;\n>  \tchar *config_value;\n>  \tstruct child_process config_set = CHILD_PROCESS_INIT;\n>  \tstruct child_process config_get = CHILD_PROCESS_INIT;\n> +\tchar *maintpath = get_maintpath();\n\nI'm sorry that this diff looks more complicated than seems necessary,\nbut your creation of the \"done:\" label is important for cleaning up\nthe string. Thanks.\n\n> +test_expect_success 'register and unregister bare repo' '\n> +\ttest_when_finished \"git config --global --unset-all maintenance.repo || :\" &&\n> +\ttest_might_fail git config --global --unset-all maintenance.repo &&\n> +\tgit init --bare barerepo &&\n> +\t(\n> +\t\tcd barerepo &&\n> +\t\tgit maintenance register &&\n> +\t\tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n\nI'm concerned about this test passing on Windows, but if the CI build is happy,\nthen I'm happy.\n\n> +\t\tgit maintenance unregister &&\n> +\t\ttest_must_fail git config --global --get-all maintenance.repo\n> +\t)\n> +'\n\nThanks!\n\nReviewed-by: Derrick Stolee <dstolee@microsoft.com>\n"},{"id":"417565","messageId":"CAPig+cTWZSe+e5JnTLMDt+F7NxdLG0fFqqSavkyo_YN7B7Affg@mail.gmail.com","threadId":"55192","inReplyTo":"5653230a-20c2-321f-7ea0-93ce59858532@gmail.com","subject":"Re: [PATCH] maintenance: fix incorrect `maintenance.repo` path with bare repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-23T16:21:46Z","receivedAt":"2021-02-23T16:22:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 23, 2021 at 7:26 AM Derrick Stolee <stolee@gmail.com> wrote:\n> On 2/23/2021 2:31 AM, Eric Sunshine wrote:\n> > +test_expect_success 'register and unregister bare repo' '\n> > +     test_when_finished \"git config --global --unset-all maintenance.repo || :\" &&\n> > +     test_might_fail git config --global --unset-all maintenance.repo &&\n> > +     git init --bare barerepo &&\n> > +     (\n> > +             cd barerepo &&\n> > +             git maintenance register &&\n> > +             git config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n>\n> I'm concerned about this test passing on Windows, but if the CI build is happy,\n> then I'm happy.\n\nI had a slight worry about Windows, as well, but (1) this is copied\ndirectly from an earlier test in this script which does pass on\nWindows, and (2) I tested on Windows, and it works fine. So, all is\ngood.\n\n> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>\n\nThanks.\n"}]}