From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 06 Feb 2026 18:03:15 GMT
Subject: Re: [PATCH] [RFC][GSoC 2026] builtin/repo: avoid global state in get_layout_bare
Message-ID: <xmqqfr7dg36k.fsf@gitster.g>
In-Reply-To: <20260206152002.1244-1-kumarayushjha123@gmail.com>

Ayush Jha <kumarayushjha123@gmail.com> writes:

> The get_layout_bare() function accepts a struct repository *repo
> argument but marks it UNUSED and instead relies on
> is_bare_repository(), which depends on global state.
>
> As bareness is a per-repository property, this causes the function
> to always report the status of the global repository, even when a
> specific repository instance is provided.
>
> This change computes the bare status using the passed-in repository
> instance (based on core.bare and the absence of a worktree),
> thereby removing the dependency on global state.
>
> This patch is sent as an RFC to solicit feedback on whether using
> repository-local state here is the preferred approach.
>
> Signed-off-by: Ayush Jha <kumarayushjha123@gmail.com>
> ---

I'll let others to suggest improvements above the three-dash line.

> diff --git a/builtin/repo.c b/builtin/repo.c
> index 0ea045abc1..b2619cc77c 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -35,9 +35,12 @@ struct field {
>  	get_value_fn *get_value;
>  };
>  
> -static int get_layout_bare(struct repository *repo UNUSED, struct strbuf *buf)
> +static int get_layout_bare(struct repository *repo, struct strbuf *buf)
>  {
> -	strbuf_addstr(buf, is_bare_repository() ? "true" : "false");
> +	int is_bare_cfg = -1;
> +	repo_config_get_bool(repo, "core.bare", &is_bare_cfg);
> +
> +	strbuf_addstr(buf, is_bare_cfg && !repo_get_work_tree(repo) ? "true" : "false");
>  	return 0;
>  }


As git.c:commands[] lists "repo" with RUN_SETUP, we know that in
cmd_repo(), after passing parse_options(), repo is guaranteed to
be a non-NULL pointer that is the_repository.  So this new version
of the function that assumes repo can be safely passed to
repo_config_get_bool() and repo_get_work_tree() would probably be
OK.  I didn't dig to find out is_bare_repository() does exactly the
same thing as what the new code does when the_repository is given to
the "repo" parameter, though.

Having said all that.

In general, anything in builtin/soething.c is an implementation of
"git something" command and cannot be reused outside the context of
that "something" command, so it is much less interesting to get rid
of dependencies on the_repository from these files.  The design
choice to work with a single repository at a time is with the
"something" command, and as long as that design choice stays, there
isn't point in hiding the fact that we work only on the_repository.

If we were moving the logic implemented by the helper functions that
are used by cmd_repo() function to a shared and reusable library-ish
part of the system (perhaps in repo.c at the top-level), that is when
it becomes much more interesting and releavnt to make sure that a
function that takes "repo" as its parameter does work on that
repository and not the_repository.

But until then, ...

Thanks.