git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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.

Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee
Message 18 of 25 in “scalar: two downstream improvements”
  1. 0/3 scalar: two downstream improvementsDerrick Stolee via GitGitGadget, Aug 14, 2023
  2. 1/3 scalar: add --[no-]src optionDerrick Stolee via GitGitGadget, Aug 14, 2023
  3. Junio C HamanoAug 14, 2023
  4. Derrick StoleeAug 14, 2023
  5. 3/3 scalar reconfigure: help users remove buggy reposDerrick Stolee via GitGitGadget, Aug 14, 2023
  6. Junio C HamanoAug 14, 2023
  7. 2/3 setup: add discover_git_directory_reason()Derrick Stolee via GitGitGadget, Aug 14, 2023
  8. Junio C HamanoAug 14, 2023
  9. Junio C HamanoAug 14, 2023
  10. 0/3 scalar: two downstream improvementsDerrick Stolee via GitGitGadget, Aug 22, 2023
  11. 1/3 scalar: add --[no-]src optionDerrick Stolee via GitGitGadget, Aug 22, 2023
  12. Oswald BuddenhagenAug 23, 2023
  13. 3/3 scalar reconfigure: help users remove buggy reposDerrick Stolee via GitGitGadget, Aug 22, 2023
  14. Junio C HamanoAug 22, 2023
  15. Derrick StoleeAug 25, 2023
  16. Junio C HamanoAug 25, 2023
  17. 2/3 setup: add discover_git_directory_reason()Derrick Stolee via GitGitGadget, Aug 22, 2023
  18. Junio C HamanoAug 22, 2023
  19. Derrick StoleeAug 22, 2023
  20. Oswald BuddenhagenAug 23, 2023
  21. 0/3 scalar: two downstream improvementsDerrick Stolee via GitGitGadget, Aug 28, 2023
  22. 1/3 scalar: add --[no-]src optionDerrick Stolee via GitGitGadget, Aug 28, 2023
  23. 3/3 scalar reconfigure: help users remove buggy reposDerrick Stolee via GitGitGadget, Aug 28, 2023
  24. 2/3 setup: add discover_git_directory_reason()Derrick Stolee via GitGitGadget, Aug 28, 2023
  25. Junio C HamanoAug 28, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.