Re: [PATCH 3/3] scalar reconfigure: help users remove buggy repos

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2023, 16:44 UTC
Message-ID: <xmqqcyzplgk1.fsf@gitster.g>
In-Reply-To: <907410f76c4a5d7ef325545f52696a9a0c00b6b3.1692025937.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> diff --git a/scalar.c b/scalar.c
> index 938bb73f3ce..7d87d7ea724 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)
>  	git_config(get_scalar_repos, &scalar_repos);
>  
>  	for (i = 0; i < scalar_repos.nr; i++) {
> +		int failed = 0;
>  		const char *dir = scalar_repos.items[i].string;

OK. You need a variable that lets you tell if the repository this round of the loop dealt with was good, and do not want to abort the loop, so you cannot reuse the "res" outside of the loop. Makes sort of sense.

I wonder if it makes it simpler to initialize "failed" to true and clear it when you see it succeeded, though.

Show 6 quoted lines

> @@ -674,30 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)
>  
>  			if (errno != ENOENT) {
>  				warning_errno(_("could not switch to '%s'"), dir);
> +				failed = -1;
> +				goto loop_end;

Such a change lets you drop this assignment ...

Show 6 quoted lines

>  			}
>  
>  			strbuf_addstr(&buf, dir);
>  			if (remove_deleted_enlistment(&buf))
> +				failed = error(_("could not remove stale "
> +						 "scalar.repo '%s'"), dir);

... and this one, but ...

>  			else
> -				warning(_("removing stale scalar.repo '%s'"),
> +				warning(_("removed stale scalar.repo '%s'"),
>  					dir);

... you'd need to drop, i.e. "failed = 0", here while you warn. It is a nice touch to update the message, by the way.

Show 12 quoted lines

>  			strbuf_release(&buf);
> +			goto loop_end;
> +		}
> +
> +		switch (discover_git_directory_reason(&commondir, &gitdir)) {
> +		case GIT_DIR_INVALID_OWNERSHIP:
> +			warning(_("repository at '%s' has different owner"), dir);
> +			failed = -1;
> +			goto loop_end;
> +
> +		case GIT_DIR_DISCOVERED:
> +			break;

... and you'd need to drop i.e. "failed = 0", here, too. but other assignments in the switch can go.

Show 14 quoted lines

> +		default:
> +			warning(_("repository not found in '%s'"), dir);
> +			failed = -1;
> +			break;
> +		}
> +
> +		git_config_clear();
> +
> +		the_repository = &r;
> +		r.commondir = commondir.buf;
> +		r.gitdir = gitdir.buf;
> +
> +		if (set_recommended_config(1) < 0)
> +			failed = -1;

And the polarity of the check and assignment here needs flipping.

> +loop_end:
> +		if (failed) {
> +			res = failed;

This assignment is a bit misleading, as if the value in "failed" actually matters, when it does not. It is merely a "did we not succeed this round, 0 or non-zero?" boolean. It would have been easier to see what was going on by saying

	if (failed) {
		res = -1;

here, I would think.

Show 5 quoted lines

> +			warning(_("to unregister this repository from Scalar, run\n"
> +				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
> +				dir);
>  		}
>  	}

Other than that, this step nicely justifies why the previous step [PATCH 2/3] is a good thing to do.

Thanks.