Skip to contentSkip to conversation

scalar reconfigure -a: remove stale `scalar.repo` entries

PATCH In review Johannes Schindelin via GitGitGadget 3 participants 3 messages Nov 7, 2022

lore
Share & tools
lore

Johannes Schindelin via GitGitGadget

author
#0 lore raw

PATCH 1/1 scalar reconfigure -a: remove stale `scalar.repo` entries

From: Johannes Schindelin <[email protected]>

Every once in a while, a Git for Windows installation fails because the attempt to reconfigure a Scalar enlistment failed because it was deleted manually without removing the corresponding entries in the global Git config.

In f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment directory gracefully, 2021-12-03), we already taught scalar delete to handle the case of a manually deleted enlistment gracefully. This patch adds the same graceful handling to scalar reconfigure --all.

This patch is best viewed with --color-moved.

Signed-off-by Johannes Schindelin <[email protected]>

---
scalar reconfigure -a: remove stale scalar.repo entries

This has been on my TODO list for, like, forever.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1407%2Fdscho%2Fscalar-reconfigure-a-and-stale-config-entries-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1407/dscho/scalar-reconfigure-a-and-stale-config-entries-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1407

 scalar.c          | 54 +++++++++++++++++++++++++++++------------------
 t/t9210-scalar.sh | 11 ++++++++++
 2 files changed, 45 insertions(+), 20 deletions(-)
scalar.c +34 -20
index 6de9c0ee523..7f4bdb6c153 100644
@@ -599,6 +599,24 @@ static int get_scalar_repos(const char *key, const char *value, void *data)
 	return 0;
 }
 
+static int remove_deleted_enlistment(struct strbuf *path)
+{
+	int res = 0;
+	strbuf_realpath_forgiving(path, path->buf, 1);
+
+	if (run_git("config", "--global",
+		    "--unset", "--fixed-value",
+		    "scalar.repo", path->buf, NULL) < 0)
+		res = -1;
+
+	if (run_git("config", "--global",
+		    "--unset", "--fixed-value",
+		    "maintenance.repo", path->buf, NULL) < 0)
+		res = -1;
+
+	return res;
+}
+
 static int cmd_reconfigure(int argc, const char **argv)
 {
 	int all = 0;

@@ -638,8 +656,22 @@ static int cmd_reconfigure(int argc, const char **argv)
 		strbuf_reset(&gitdir);
 
 		if (chdir(dir) < 0) {
-			warning_errno(_("could not switch to '%s'"), dir);
-			res = -1;
+			struct strbuf buf = STRBUF_INIT;
+
+			if (errno != ENOENT) {
+				warning_errno(_("could not switch to '%s'"), dir);
+				res = -1;
+				continue;
+			}
+
+			strbuf_addstr(&buf, dir);
+			if (remove_deleted_enlistment(&buf))
+				res = error(_("could not remove stale "
+					      "scalar.repo '%s'"), dir);
+			else
+				warning(_("removing stale scalar.repo '%s'"),
+					dir);
+			strbuf_release(&buf);
 		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
 			warning_errno(_("git repository gone in '%s'"), dir);
 			res = -1;

@@ -725,24 +757,6 @@ static int cmd_run(int argc, const char **argv)
 	return 0;
 }
 
-static int remove_deleted_enlistment(struct strbuf *path)
-{
-	int res = 0;
-	strbuf_realpath_forgiving(path, path->buf, 1);
-
-	if (run_git("config", "--global",
-		    "--unset", "--fixed-value",
-		    "scalar.repo", path->buf, NULL) < 0)
-		res = -1;
-
-	if (run_git("config", "--global",
-		    "--unset", "--fixed-value",
-		    "maintenance.repo", path->buf, NULL) < 0)
-		res = -1;
-
-	return res;
-}
-
 static int cmd_unregister(int argc, const char **argv)
 {
 	struct option options[] = {
t/t9210-scalar.sh +11 -0
index be51a8bb7a4..c7f8a379108 100755
@@ -166,6 +166,17 @@ test_expect_success 'scalar reconfigure' '
 	test true = "$(git -C one/src config core.preloadIndex)"
 '
 
+test_expect_success '`reconfigure -a` removes stale config entries' '
+	git init stale/src &&
+	scalar register stale &&
+	scalar list >scalar.repos &&
+	grep stale scalar.repos &&
+	rm -rf stale &&
+	scalar reconfigure -a &&
+	scalar list >scalar.repos &&
+	! grep stale scalar.repos
+'
+
 test_expect_success 'scalar delete without enlistment shows a usage' '
 	test_expect_code 129 scalar delete
 '

base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 11 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Every once in a while, a Git for Windows installation fails because the attempt to reconfigure a Scalar enlistment failed because it was deleted manually without removing the corresponding entries in the global Git config.

In f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment directory gracefully, 2021-12-03), we already taught scalar delete to handle the case of a manually deleted enlistment gracefully. This patch adds the same graceful handling to scalar reconfigure --all.

Makes sense, thanks.

t/t9210-scalar.sh test_expect_success 'scalar reconfigure' ' · line 177

Quoted 13 lines
t/t9210-scalar.sh +9 -0
@@ -166,6 +166,17 @@ test_expect_success 'scalar reconfigure' '
 	test true = "$(git -C one/src config core.preloadIndex)"
 '
 
+test_expect_success '`reconfigure -a` removes stale config entries' '
+	git init stale/src &&
+	scalar register stale &&
+	scalar list >scalar.repos &&
+	grep stale scalar.repos &&
+	rm -rf stale &&
+	scalar reconfigure -a &&
+	scalar list >scalar.repos &&
+	! grep stale scalar.repos

Arguably this test would be slightly improved if we checked not only for '!grep stale scalar.repos', but that the correct set of repositories *does* appear in the list.

But presumably we have plenty of other tests which do so already, so we're not lacking any coverage here by omitting it.

This patch looks good to me, will queue.

Thanks,
Taylor

#2 lore raw
In reply to Johannes Schindelin via GitGitGadget
Quoted 6 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Every once in a while, a Git for Windows installation fails because the attempt to reconfigure a Scalar enlistment failed because it was deleted manually without removing the corresponding entries in the global Git config.

Not actually important to this patch: does it actually fail, or does it just report a warning message (which _looks_ like a failure)?

Quoted 6 lines

In f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment directory gracefully, 2021-12-03), we already taught scalar delete to handle the case of a manually deleted enlistment gracefully. This patch adds the same graceful handling to scalar reconfigure --all.

This patch is best viewed with --color-moved.

Thanks, that did help, even if it's a small change.

Quoted 12 lines
scalar.c +6 -2
@@ -638,8 +656,22 @@ static int cmd_reconfigure(int argc, const char **argv)
 		strbuf_reset(&gitdir);
 
 		if (chdir(dir) < 0) {
-			warning_errno(_("could not switch to '%s'"), dir);
-			res = -1;
+			struct strbuf buf = STRBUF_INIT;
+
+			if (errno != ENOENT) {
+				warning_errno(_("could not switch to '%s'"), dir);
+				res = -1;
+				continue;

scalar.c static int cmd_reconfigure(int argc, const char **argv) · line 674

Quoted 10 lines
scalar.c +10 -0
+			}
+
+			strbuf_addstr(&buf, dir);
+			if (remove_deleted_enlistment(&buf))
+				res = error(_("could not remove stale "
+					      "scalar.repo '%s'"), dir);
+			else
+				warning(_("removing stale scalar.repo '%s'"),
+					dir);
+			strbuf_release(&buf);

Looks good.

t/t9210-scalar.sh test_expect_success 'scalar reconfigure' ' · line 178

Quoted 10 lines
t/t9210-scalar.sh +10 -0
+test_expect_success '`reconfigure -a` removes stale config entries' '
+	git init stale/src &&
+	scalar register stale &&
+	scalar list >scalar.repos &&
+	grep stale scalar.repos &&
+	rm -rf stale &&
+	scalar reconfigure -a &&
+	scalar list >scalar.repos &&
+	! grep stale scalar.repos
+'

In his reply, Taylor mentioned it would be good to check that we still have the correct list of scalar.repos value. I think that might be critical since one "solution" to this problem of a stale repo could be to remove all scalar.repos values (and this test would currently pass).

Please use a test_cmp against scalar.repos. Something like this works:

test_expect_success 'reconfigure -a removes stale config entries' '

	git init stale/src &&
	scalar register stale &&
	scalar list >scalar.repos &&
	grep stale scalar.repos &&

	grep -v stale scalar.repos >expect &&

	rm -rf stale &&
	scalar reconfigure -a &&
	scalar list >scalar.repos &&
	test_cmp expect scalar.repos

'

Thanks,
-Stolee