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

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Aug 23, 2023, 09:58 UTC
Message-ID: <ZOXYOWf/Vqfslmv4@ugly>
In-Reply-To: <787af0f9744e2f18c9ab680886650278a9d2f8d0.1692725056.git.gitgitgadget@gmail.com>

On Tue, Aug 22, 2023 at 05:24:14PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 10 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
>

"by moving it"

Show 16 quoted lines

>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