public inbox for git@vger.kernel.org
 help / color / mirror / Atom feed
* [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