* [PATCH] [RFC][GSoC 2026] builtin/repo: avoid global state in get_layout_bare
@ 2026-02-06 15:20 Ayush Jha
2026-02-06 18:03 ` Junio C Hamano
0 siblings, 1 reply; 2+ messages in thread
From: Ayush Jha @ 2026-02-06 15:20 UTC (permalink / raw)
To: git
Cc: Christian Couder, Karthik Nayak, Justin Tobler, Ayush Chandekar,
Siddharth Asthana, Lucas Seiki Oshiro, Chandra Pratap, Ayush Jha
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>
---
builtin/repo.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
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;
}
--
2.53.0.windows.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] [RFC][GSoC 2026] builtin/repo: avoid global state in get_layout_bare
2026-02-06 15:20 [PATCH] [RFC][GSoC 2026] builtin/repo: avoid global state in get_layout_bare Ayush Jha
@ 2026-02-06 18:03 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2026-02-06 18:03 UTC (permalink / raw)
To: Ayush Jha
Cc: git, Christian Couder, Karthik Nayak, Justin Tobler,
Ayush Chandekar, Siddharth Asthana, Lucas Seiki Oshiro,
Chandra Pratap
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-02-06 18:03 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-06 15:20 [PATCH] [RFC][GSoC 2026] builtin/repo: avoid global state in get_layout_bare Ayush Jha
2026-02-06 18:03 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox