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

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

On Tue, Aug 22, 2023 at 05:24:14PM +0000, Derrick Stolee via GitGitGadget wrote:
>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
>
"by moving it"

>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.
>
i have no clue wha this means. what is "it"?

>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.
>
is this really worth it, given that there are just two callers, and the 
adjustment is trivial?
if you insist, note that the function name can be easily misread as 
"discover_git_(directory_reason)", which is unhelpful. i typically use 
an _impl suffix in such "thin wrapper" scenarios.

regards