{"thread":{"id":"58757","subject":"[PATCH] scalar reconfigure -a: remove stale `scalar.repo` entries","startedAt":"2022-11-07T18:25:44Z","lastAt":"2022-11-07T20:15:27Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"466682","messageId":"pull.1407.git.1667845501422.gitgitgadget@gmail.com","threadId":"58757","inReplyTo":null,"subject":"[PATCH] scalar reconfigure -a: remove stale `scalar.repo` entries","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-07T18:25:01Z","receivedAt":"2022-11-07T18:25:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nEvery once in a while, a Git for Windows installation fails because the\nattempt to reconfigure a Scalar enlistment failed because it was deleted\nmanually without removing the corresponding entries in the global Git\nconfig.\n\nIn f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment\ndirectory gracefully, 2021-12-03), we already taught `scalar delete` to\nhandle the case of a manually deleted enlistment gracefully. This patch\nadds the same graceful handling to `scalar reconfigure --all`.\n\nThis patch is best viewed with `--color-moved`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    scalar reconfigure -a: remove stale scalar.repo entries\n    \n    This has been on my TODO list for, like, forever.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1407%2Fdscho%2Fscalar-reconfigure-a-and-stale-config-entries-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1407/dscho/scalar-reconfigure-a-and-stale-config-entries-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1407\n\n scalar.c          | 54 +++++++++++++++++++++++++++++------------------\n t/t9210-scalar.sh | 11 ++++++++++\n 2 files changed, 45 insertions(+), 20 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex 6de9c0ee523..7f4bdb6c153 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -599,6 +599,24 @@ static int get_scalar_repos(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n+static int remove_deleted_enlistment(struct strbuf *path)\n+{\n+\tint res = 0;\n+\tstrbuf_realpath_forgiving(path, path->buf, 1);\n+\n+\tif (run_git(\"config\", \"--global\",\n+\t\t    \"--unset\", \"--fixed-value\",\n+\t\t    \"scalar.repo\", path->buf, NULL) < 0)\n+\t\tres = -1;\n+\n+\tif (run_git(\"config\", \"--global\",\n+\t\t    \"--unset\", \"--fixed-value\",\n+\t\t    \"maintenance.repo\", path->buf, NULL) < 0)\n+\t\tres = -1;\n+\n+\treturn res;\n+}\n+\n static int cmd_reconfigure(int argc, const char **argv)\n {\n \tint all = 0;\n@@ -638,8 +656,22 @@ static int cmd_reconfigure(int argc, const char **argv)\n \t\tstrbuf_reset(&gitdir);\n \n \t\tif (chdir(dir) < 0) {\n-\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n-\t\t\tres = -1;\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\t\tif (errno != ENOENT) {\n+\t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n+\t\t\t\tres = -1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\tstrbuf_addstr(&buf, dir);\n+\t\t\tif (remove_deleted_enlistment(&buf))\n+\t\t\t\tres = error(_(\"could not remove stale \"\n+\t\t\t\t\t      \"scalar.repo '%s'\"), dir);\n+\t\t\telse\n+\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n+\t\t\t\t\tdir);\n+\t\t\tstrbuf_release(&buf);\n \t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n \t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n \t\t\tres = -1;\n@@ -725,24 +757,6 @@ static int cmd_run(int argc, const char **argv)\n \treturn 0;\n }\n \n-static int remove_deleted_enlistment(struct strbuf *path)\n-{\n-\tint res = 0;\n-\tstrbuf_realpath_forgiving(path, path->buf, 1);\n-\n-\tif (run_git(\"config\", \"--global\",\n-\t\t    \"--unset\", \"--fixed-value\",\n-\t\t    \"scalar.repo\", path->buf, NULL) < 0)\n-\t\tres = -1;\n-\n-\tif (run_git(\"config\", \"--global\",\n-\t\t    \"--unset\", \"--fixed-value\",\n-\t\t    \"maintenance.repo\", path->buf, NULL) < 0)\n-\t\tres = -1;\n-\n-\treturn res;\n-}\n-\n static int cmd_unregister(int argc, const char **argv)\n {\n \tstruct option options[] = {\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex be51a8bb7a4..c7f8a379108 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -166,6 +166,17 @@ test_expect_success 'scalar reconfigure' '\n \ttest true = \"$(git -C one/src config core.preloadIndex)\"\n '\n \n+test_expect_success '`reconfigure -a` removes stale config entries' '\n+\tgit init stale/src &&\n+\tscalar register stale &&\n+\tscalar list >scalar.repos &&\n+\tgrep stale scalar.repos &&\n+\trm -rf stale &&\n+\tscalar reconfigure -a &&\n+\tscalar list >scalar.repos &&\n+\t! grep stale scalar.repos\n+'\n+\n test_expect_success 'scalar delete without enlistment shows a usage' '\n \ttest_expect_code 129 scalar delete\n '\n\nbase-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\n-- \ngitgitgadget\n"},{"id":"466715","messageId":"Y2lWc2gmPaTfi4k5@nand.local","threadId":"58757","inReplyTo":"pull.1407.git.1667845501422.gitgitgadget@gmail.com","subject":"Re: [PATCH] scalar reconfigure -a: remove stale `scalar.repo` entries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T19:03:15Z","receivedAt":"2022-11-07T19:03:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 06:25:01PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Every once in a while, a Git for Windows installation fails because the\n> attempt to reconfigure a Scalar enlistment failed because it was deleted\n> manually without removing the corresponding entries in the global Git\n> config.\n>\n> In f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment\n> directory gracefully, 2021-12-03), we already taught `scalar delete` to\n> handle the case of a manually deleted enlistment gracefully. This patch\n> adds the same graceful handling to `scalar reconfigure --all`.\n\nMakes sense, thanks.\n\n> @@ -166,6 +166,17 @@ test_expect_success 'scalar reconfigure' '\n>  \ttest true = \"$(git -C one/src config core.preloadIndex)\"\n>  '\n>\n> +test_expect_success '`reconfigure -a` removes stale config entries' '\n> +\tgit init stale/src &&\n> +\tscalar register stale &&\n> +\tscalar list >scalar.repos &&\n> +\tgrep stale scalar.repos &&\n> +\trm -rf stale &&\n> +\tscalar reconfigure -a &&\n> +\tscalar list >scalar.repos &&\n> +\t! grep stale scalar.repos\n\nArguably this test would be slightly improved if we checked not only for\n'!grep stale scalar.repos', but that the correct set of repositories\n*does* appear in the list.\n\nBut presumably we have plenty of other tests which do so already, so\nwe're not lacking any coverage here by omitting it.\n\nThis patch looks good to me, will queue.\n\nThanks,\nTaylor\n"},{"id":"466732","messageId":"8e4a8817-4fd5-d7f3-2c7d-bf23e61b9823@github.com","threadId":"58757","inReplyTo":"pull.1407.git.1667845501422.gitgitgadget@gmail.com","subject":"Re: [PATCH] scalar reconfigure -a: remove stale `scalar.repo` entries","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T20:14:41Z","receivedAt":"2022-11-07T20:15:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/7/22 1:25 PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> Every once in a while, a Git for Windows installation fails because the\n> attempt to reconfigure a Scalar enlistment failed because it was deleted\n> manually without removing the corresponding entries in the global Git\n> config.\n\nNot actually important to this patch: does it actually fail, or does it\njust report a warning message (which _looks_ like a failure)?\n \n> In f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment\n> directory gracefully, 2021-12-03), we already taught `scalar delete` to\n> handle the case of a manually deleted enlistment gracefully. This patch\n> adds the same graceful handling to `scalar reconfigure --all`.\n> \n> This patch is best viewed with `--color-moved`.\n \nThanks, that did help, even if it's a small change.\n\n> @@ -638,8 +656,22 @@ static int cmd_reconfigure(int argc, const char **argv)\n>  \t\tstrbuf_reset(&gitdir);\n>  \n>  \t\tif (chdir(dir) < 0) {\n> -\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n> -\t\t\tres = -1;\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\t\tif (errno != ENOENT) {\n> +\t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n> +\t\t\t\tres = -1;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tstrbuf_addstr(&buf, dir);\n> +\t\t\tif (remove_deleted_enlistment(&buf))\n> +\t\t\t\tres = error(_(\"could not remove stale \"\n> +\t\t\t\t\t      \"scalar.repo '%s'\"), dir);\n> +\t\t\telse\n> +\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n> +\t\t\t\t\tdir);\n> +\t\t\tstrbuf_release(&buf);\n\nLooks good.\n\n> +test_expect_success '`reconfigure -a` removes stale config entries' '\n> +\tgit init stale/src &&\n> +\tscalar register stale &&\n> +\tscalar list >scalar.repos &&\n> +\tgrep stale scalar.repos &&\n> +\trm -rf stale &&\n> +\tscalar reconfigure -a &&\n> +\tscalar list >scalar.repos &&\n> +\t! grep stale scalar.repos\n> +'\n\nIn his reply, Taylor mentioned it would be good to check that we\nstill have the correct list of scalar.repos value. I think that\nmight be critical since one \"solution\" to this problem of a stale\nrepo could be to remove all scalar.repos values (and this test\nwould currently pass).\n\nPlease use a `test_cmp` against scalar.repos. Something like\nthis works:\n\ntest_expect_success '`reconfigure -a` removes stale config entries' '\n\tgit init stale/src &&\n\tscalar register stale &&\n\tscalar list >scalar.repos &&\n\tgrep stale scalar.repos &&\n\n\tgrep -v stale scalar.repos >expect &&\n\n\trm -rf stale &&\n\tscalar reconfigure -a &&\n\tscalar list >scalar.repos &&\n\ttest_cmp expect scalar.repos\n'\n\nThanks,\n-Stolee\n"}]}