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

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

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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.

>  		}
>  	}