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

Commit Message

Derrick Stolee Aug. 14, 2023, 3:12 p.m. UTC

From: Derrick Stolee <[e-mail]>

When running 'scalar reconfigure -a', Scalar has warning messages about
the repository missing (or not containing a .git directory). Failures
can also happen while trying to modify the repository-local config for
that repository.

These warnings may seem confusing to users who don't understand what
they mean or how to stop them.

Add a warning that instructs the user how to remove the warning in
future installations.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 scalar.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 17 deletions(-)

Comments

Junio C Hamano Aug. 14, 2023, 4:44 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> 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.

Patch

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;
 
 		strbuf_reset(&commondir);
@@ -674,30 +675,51 @@  static int cmd_reconfigure(int argc, const char **argv)
 
 			if (errno != ENOENT) {
 				warning_errno(_("could not switch to '%s'"), dir);
-				res = -1;
-				continue;
+				failed = -1;
+				goto loop_end;
 			}
 
 			strbuf_addstr(&buf, dir);
 			if (remove_deleted_enlistment(&buf))
-				res = error(_("could not remove stale "
-					      "scalar.repo '%s'"), dir);
+				failed = error(_("could not remove stale "
+						 "scalar.repo '%s'"), dir);
 			else
-				warning(_("removing stale scalar.repo '%s'"),
+				warning(_("removed 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;
-		} else {
-			git_config_clear();
-
-			the_repository = &r;
-			r.commondir = commondir.buf;
-			r.gitdir = gitdir.buf;
-
-			if (set_recommended_config(1) < 0)
-				res = -1;
+			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;
+
+		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;
+
+loop_end:
+		if (failed) {
+			res = failed;
+			warning(_("to unregister this repository from Scalar, run\n"
+				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
+				dir);
 		}
 	}