Re: [PATCH] scalar reconfigure -a: remove stale `scalar.repo` entries

[Derrick Stolee <derrickstolee@github.com>]

On 11/7/22 1:25 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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