Re: [PATCH v2 2/3] setup: add discover_git_directory_reason()

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

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

> From: Derrick Stolee <derrickstolee@github.com>
>
> There are many reasons why discovering a Git directory may fail. In
> particular, 8959555cee7 (setup_git_directory(): add an owner check for
> the top-level directory, 2022-03-02) added ownership checks as a
> security precaution.
>
> Callers attempting to set up a Git directory may want to inform the user
> about the reason for the failure. For that, expose the enum
> discovery_result from within setup.c and into cache.h where
> discover_git_directory() is defined.
>
> I initially wanted to change the return type of discover_git_directory()
> to be this enum, but several callers rely upon the "zero means success".
> The two problems with this are:
>
> 1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive
>    results are errors.
>
> 2. There are multiple successful states; positive results are
>    successful.
>
> It is worth noting that GIT_DIR_NONE is not returned, so we remove this
> option from the enum. We must be careful to keep the successful reasons
> as positive values, so they are given explicit positive values.
> Further, a use in scalar.c was previously impossible, so it is removed.
>
> Instead of updating all callers immediately, add a new method,
> discover_git_directory_reason(), and convert discover_git_directory() to
> be a thin shim on top of it.
>
> One thing that is important to note is that discover_git_directory()
> previously returned -1 on error, so let's continue that into the future.
> There is only one caller (in scalar.c) that depends on that signedness
> instead of a non-zero check, so clean that up, too.
>
> Because there are extra checks that discover_git_directory_reason() does
> after setup_git_directory_gently_1(), there are other modes that can be
> returned for failure states. Add these modes to the enum, but be sure to
> explicitly add them as BUG() states in the switch of
> setup_git_directory_gently().
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  scalar.c |  3 ---
>  setup.c  | 34 ++++++++++++----------------------
>  setup.h  | 35 ++++++++++++++++++++++++++++++++---
>  3 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/scalar.c b/scalar.c
> index 938bb73f3ce..02a38e845e1 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -686,9 +686,6 @@ static int cmd_reconfigure(int argc, const char **argv)
>  				warning(_("removing 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();
>  

In the original before this series, and also after applying [PATCH
3/3], the reconfiguration is a three-step process:

 - what if we cannot go to that directory?
 - what if the directory is not a usable repository?
 - now we are in a usable repository, let's reconfigure.

and this seems to lose the second step tentatively?  It is
resurrected in a more enhanced form in [PATCH 3/3] so it may not be
a huge deal, but it looks like it is not intended lossage.