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

Commit Message

Johannes Schindelin Nov. 7, 2022, 6:25 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
    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(-)


base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193

Comments

Taylor Blau Nov. 7, 2022, 7:03 p.m. UTC | #1

On Mon, Nov 07, 2022 at 06:25:01PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
>
> 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.

> @@ -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

Derrick Stolee Nov. 7, 2022, 8:14 p.m. UTC | #2

On 11/7/22 1:25 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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)?
 
> 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.

> @@ -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);

Looks good.

> +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

Patch

diff --git a/scalar.c b/scalar.c
index 6de9c0ee523..7f4bdb6c153 100644
--- a/scalar.c
+++ b/scalar.c
@@ -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[] = {
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index be51a8bb7a4..c7f8a379108 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -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
 '