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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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