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

Commit Message

Derrick Stolee Aug. 22, 2023, 5:24 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 | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Aug. 22, 2023, 7:45 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> @@ -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 succeeded = 0;
>  		const char *dir = scalar_repos.items[i].string;
>  
>  		strbuf_reset(&commondir);
> @@ -674,27 +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;
> +				goto loop_end;

This is after seeing chdir(dir) failed.  If the user manually
removed the enlisted directory, ENOENT would be one of the most
likely errors.  If the user dropped a file to the place after it was
vacated, we may get ENOTDIR, which is also not so bad.

In any case, is it desirable to keep the enlistment still configured
by jumping to loop_end in these "other" error conditions?  If the
reason why we cannot chdir() into it is because of some tentative
glitch that may resolve by itself, retaining the enlistment data may
have value, because it can be reused without the user having to
recreate the enlistment when the "tentatively unavailable" directory
comes back online, I guess, but how realistic would such an error
be?

>  			}
>  
>  			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'"),
> +				error(_("could not remove stale "
> +					"scalar.repo '%s'"), dir);
> +			else {
> +				warning(_("removed stale scalar.repo '%s'"),
>  					dir);
> +				succeeded = 1;
> +			}
>  			strbuf_release(&buf);
> -		} else {
> -			git_config_clear();
> +			goto loop_end;
> +		}

So the above is "what if we fail to chdir()", which looked sensible.
Then comes the "what if we don't have a usable repository there?", which
was lost in [PATCH 2/3].

> +		switch (discover_git_directory_reason(&commondir, &gitdir)) {
> +		case GIT_DIR_INVALID_OWNERSHIP:
> +			warning(_("repository at '%s' has different owner"), dir);
> +			goto loop_end;
> +
> +		case GIT_DIR_DISCOVERED:
> +			succeeded = 1;
> +			break;
> +
> +		default:
> +			warning(_("repository not found in '%s'"), dir);
> +			break;

Among the error cases, INVALID_OWNERSHIP is one of the possibilities
that merits specialized message to the end-user.  I wonder if others
also deserve to be explained, though.

 - HIT_CEILING and HIT_MOUNT_POINT will happen when there is no
   usable repository between "dir" and the specified ceiling.

 - INVALID_GITFILE and INVALID_FORMAT are signs of some repository
   corruption.

 - DISALLOWED_BARE is unlikely to happen in the scalar context.

> +		}
> +
> +		git_config_clear();
> +
> +		the_repository = &r;
> +		r.commondir = commondir.buf;
> +		r.gitdir = gitdir.buf;
>  
> -			the_repository = &r;
> -			r.commondir = commondir.buf;
> -			r.gitdir = gitdir.buf;
> +		if (set_recommended_config(1) >= 0)
> +			succeeded = 1;
>  
> -			if (set_recommended_config(1) < 0)
> -				res = -1;
> +loop_end:
> +		if (!succeeded) {
> +			res = -1;
> +			warning(_("to unregister this repository from Scalar, run\n"
> +				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
> +				dir);

Ah, OK.  So the strategy is to punt on accepting the responsibility
for removing an inaccessible directory; rather, we just report that
we had trouble chdir() and let the user decide.  Which makes sense.

Thanks.

>  		}
>  	}

Derrick Stolee Aug. 25, 2023, 5:21 p.m. UTC | #2

On 8/22/2023 3:45 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:

>> +		switch (discover_git_directory_reason(&commondir, &gitdir)) {
>> +		case GIT_DIR_INVALID_OWNERSHIP:
>> +			warning(_("repository at '%s' has different owner"), dir);
>> +			goto loop_end;
>> +
>> +		case GIT_DIR_DISCOVERED:
>> +			succeeded = 1;
>> +			break;
>> +
>> +		default:
>> +			warning(_("repository not found in '%s'"), dir);
>> +			break;
> 
> Among the error cases, INVALID_OWNERSHIP is one of the possibilities
> that merits specialized message to the end-user.  I wonder if others
> also deserve to be explained, though.

The specific choice of GIT_DIR_INVALID_OWNERSHIP is singled out
because it's a new-ish reason and is the most confusing to users
when things fail for this reason.
 
>  - HIT_CEILING and HIT_MOUNT_POINT will happen when there is no
>    usable repository between "dir" and the specified ceiling.

These are basically "didn't find a Git repo" but there are different
reasons why Git stopped looking. I'm not sure there is something more
valuable to indicate here than the "repository not found" message
that already exists.

>  - INVALID_GITFILE and INVALID_FORMAT are signs of some repository
>    corruption.

I can add a message for this kind of error, which seems helpful to
point out to a user.

>  - DISALLOWED_BARE is unlikely to happen in the scalar context.

I agree.

Thanks,
-Stolee

Junio C Hamano Aug. 25, 2023, 6:05 p.m. UTC | #3

Derrick Stolee <[e-mail]> writes:

> On 8/22/2023 3:45 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
>
>>> +		switch (discover_git_directory_reason(&commondir, &gitdir)) {
>>> +		case GIT_DIR_INVALID_OWNERSHIP:
>>> +			warning(_("repository at '%s' has different owner"), dir);
>>> +			goto loop_end;
>>> +
>>> +		case GIT_DIR_DISCOVERED:
>>> +			succeeded = 1;
>>> +			break;
>>> +
>>> +		default:
>>> +			warning(_("repository not found in '%s'"), dir);
>>> +			break;
>> 
>> Among the error cases, INVALID_OWNERSHIP is one of the possibilities
>> that merits specialized message to the end-user.  I wonder if others
>> also deserve to be explained, though.
>
> The specific choice of GIT_DIR_INVALID_OWNERSHIP is singled out
> because it's a new-ish reason and is the most confusing to users
> when things fail for this reason.
>  
>>  - HIT_CEILING and HIT_MOUNT_POINT will happen when there is no
>>    usable repository between "dir" and the specified ceiling.
>
> These are basically "didn't find a Git repo" but there are different
> reasons why Git stopped looking. I'm not sure there is something more
> valuable to indicate here than the "repository not found" message
> that already exists.

OK.  I just know that "not found" will be greeted by "stupid Git, if
you go one level up, there is a .git/ directory!", now we have many
users than we used to have and people forget what they configured.

But I think it would apply much less to users who see "repository
not found" in the "scalar reconfigure" than ones who manually
created a repository, and then forgot that they shuffled the disks
around with cross mounting.  So I agree with you that it is not
essential to mention these reasons in this codepath (and
setup_git_directory() does have a reasonable message for at least
the mount-point case that covers the more general case).

>>  - INVALID_GITFILE and INVALID_FORMAT are signs of some repository
>>    corruption.
>
> I can add a message for this kind of error, which seems helpful to
> point out to a user.

Maybe.  We can do that in a follow-up topic separately.

Thanks.

Patch

diff --git a/scalar.c b/scalar.c
index 02a38e845e1..54df0fdbb9f 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 succeeded = 0;
 		const char *dir = scalar_repos.items[i].string;
 
 		strbuf_reset(&commondir);
@@ -674,27 +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;
+				goto loop_end;
 			}
 
 			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'"),
+				error(_("could not remove stale "
+					"scalar.repo '%s'"), dir);
+			else {
+				warning(_("removed stale scalar.repo '%s'"),
 					dir);
+				succeeded = 1;
+			}
 			strbuf_release(&buf);
-		} else {
-			git_config_clear();
+			goto loop_end;
+		}
+
+		switch (discover_git_directory_reason(&commondir, &gitdir)) {
+		case GIT_DIR_INVALID_OWNERSHIP:
+			warning(_("repository at '%s' has different owner"), dir);
+			goto loop_end;
+
+		case GIT_DIR_DISCOVERED:
+			succeeded = 1;
+			break;
+
+		default:
+			warning(_("repository not found in '%s'"), dir);
+			break;
+		}
+
+		git_config_clear();
+
+		the_repository = &r;
+		r.commondir = commondir.buf;
+		r.gitdir = gitdir.buf;
 
-			the_repository = &r;
-			r.commondir = commondir.buf;
-			r.gitdir = gitdir.buf;
+		if (set_recommended_config(1) >= 0)
+			succeeded = 1;
 
-			if (set_recommended_config(1) < 0)
-				res = -1;
+loop_end:
+		if (!succeeded) {
+			res = -1;
+			warning(_("to unregister this repository from Scalar, run\n"
+				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
+				dir);
 		}
 	}