Re: [PATCH v2 2/3] setup: add discover_git_directory_reason()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 22, 2023, 19:30 UTC
- Message-ID
- <xmqqttsqlvsd.fsf@gitster.g>
- In-Reply-To
- <787af0f9744e2f18c9ab680886650278a9d2f8d0.1692725056.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 63 quoted lines
> 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.