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

From: Derrick Stolee <derrickstolee@github.com>
Date: Nov 7, 2022, 20:14 UTC
Message-ID: <8e4a8817-4fd5-d7f3-2c7d-bf23e61b9823@github.com>
In-Reply-To: <pull.1407.git.1667845501422.gitgitgadget@gmail.com>

On 11/7/22 1:25 PM, Johannes Schindelin via GitGitGadget wrote:

Show 6 quoted lines

> 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)?

Show 6 quoted 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.

Show 22 quoted lines

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

Show 10 quoted lines

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