* Re: [PATCH 5/7] environment: split up concerns of `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-11 9:19 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <ainesoUOuhspKxHF@denethor>
On Wed, Jun 10, 2026 at 05:22:04PM -0500, Justin Tobler wrote:
> On 26/06/10 08:56AM, Patrick Steinhardt wrote:
> > diff --git a/builtin/init-db.c b/builtin/init-db.c
> > index 52aa92fb0a..566732c9f4 100644
> > --- a/builtin/init-db.c
> > +++ b/builtin/init-db.c
> > @@ -81,7 +81,7 @@ int cmd_init_db(int argc,
> > const char *template_dir = NULL;
> > char *template_dir_to_free = NULL;
> > unsigned int flags = 0;
> > - int bare = is_bare_repository_cfg;
> > + int bare = startup_info->force_bare_repository ? 1 : -1;
>
> Any particular reason to continue mapping `force_bare_repository=false`
> to -1? Or was this to just minimize changes?
The `-1` value doesn't mean "false", but it rather means "undecided".
The effect of this is that "core.bare" will eventually override this.
> > diff --git a/setup.c b/setup.c
> > index 71fc6b33da..2b690da8ca 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -795,10 +795,16 @@ static int check_repository_format_gently(const char *gitdir,
> > has_common = 0;
> > }
> >
> > - if (!has_common) {
> > - if (candidate->is_bare != -1)
> > - is_bare_repository_cfg = candidate->is_bare;
> > - } else {
> > + if (startup_info->force_bare_repository) {
> > + candidate->is_bare = 1;
> > + FREE_AND_NULL(candidate->work_tree);
> > + } else if (has_common) {
> > + /*
> > + * When sharing a common dir with another repository (e.g. a
> > + * linked worktree), do not let this repository's config
> > + * dictate bareness; it is inherited from the main worktree.
> > + */
> > + candidate->is_bare = -1;
> > FREE_AND_NULL(candidate->work_tree);
>
> Previously, when there was a common dir, `candidate->work_tree` was left
> untouched, but now we are expclicitly setting it. I'm not sure I fully
> understand this change.
I cannot blame you. All of this logic is so unbelievably tangled and
hard to follow.
In any case, I think you might have missed the fact that we `else if`
branch is now `has_common` as compared to `!has_common`? To explain the
different cases a bit:
- When we have `force_bare_repository` we are being told that the
repository should be treated as bare. So we set `is_bare` and also
clear the work tree that may have been discovered.
- When we have a commondir we know that we're in a worktree.
Previously we did nothing in this case, and that had the implicit
effect that `is_bare_repository_cfg` would remain at `-1`. So to
match that behaviour we have to also reset the candidate's bareness
to `-1`, so that we parse it via the repository's configuration at a
later point in time.
The other part here is that we also reset `candidate->work_tree`.
This is because the expectation is that `$GIT_DIR/common` should
override any "core.worktree" settings. Quoting git-config(1):
If GIT_COMMON_DIR environment variable is set, core.worktree is
ignored and not used for determining the root of working tree.
- When we don't have a commondir we previosuly had to also adapt the
global `is_bare_repository_cfg` variable. This part is not necessary
anymore, so we basically just drop this case altogether.
Patrick
^ permalink raw reply
* Re: git-diff in a worktree is an order of magnitude slower?
From: Jeff King @ 2026-06-11 8:55 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: Git
In-Reply-To: <CALnO6CD+3sE1xQUnRsCFfWrZTsq2Edw7BWseLzasgT3dgtaq_Q@mail.gmail.com>
On Tue, Jun 09, 2026 at 01:15:11PM -0400, D. Ben Knoble wrote:
> > Which implies that the entries are stat dirty. And indeed, if I run:
> >
> > git -C linux update-index --refresh
> >
> > now they both take ~20ms.
>
> Ah, TIL about --refresh. I suppose it could be nice if "git diff"
> updated the index in this way, but that sounds like a band-aid. Maybe
> creating a fresh worktree should do the equivalent to make sure it's
> considered "fresh"?
I think "git diff" _does_ refresh the index internally (that's what
takes so long!). I thought we then wrote out the result, but maybe we
don't notice that it needs an update for some reason?
I'm pretty sure "git status" does something similar, though running it
in a slow working tree _does_ seem to make things faster. Maybe it's
more aggressive about doing the update.
I don't think that refreshing after making a worktree would help. The
problem is one of timestamps: we just wrote an index (so it _should_ be
totally up to date), but we err on the side of caution for some entries
because the file timestamps and the index timestamp are the same. So
what makes it "work" is that one second passed between writing those
files and running "update-index". If you ran it from the worktree
command automatically, it might all still happen in the same second.
And of course, it's not just worktrees. Any time we checkout we may
suffer from this problem, though initial clones and worktree creation
will write more files than most.
> At $DAYJOB, I _think_ some version of "git restore <stuff>" ended up
> also updating the index.
Yep, that would make sense. Any index write (after the second-hand
ticks) will make it go away, since it means updating the mtime of the
index.
> > I'd have thought USE_NSEC was the default these days, but looks like it
> > isn't? Try building with that and I'll bet it goes away entirely.
>
> Thanks, I'll take a look.
>
> I can see on my Macbook that at least Meson does automatically set
> either USE_ST_TIMESPEC or NO_NSEC automatically, but has no option to
> enabled USE_NSEC and try that. I can probably write that patch (which
> I'll do to test), and I can send it along with the "worktree add
> should refresh the index" if you think that's an appropriate thing to
> do.
I think NO_NSEC is about not looking at the nsec fields of stat structs
(since they might not exist). But we don't actually use them for stat
matching unless USE_NSEC is set.
I guess the distinction goes back to c06ff4908b (Record ns-timestamps if
possible, but do not use it without USE_NSEC, 2009-03-04), which details
some reasons you might not want USE_NSEC. Feels like it ought to be a
run-time config, though, and maybe even something that gets auto-probed
by git-init.
Definitely not an area I have looked at much, though, nor thought hard
about. So there might be gotchas. :)
-Peff
^ permalink raw reply
* Re: [PATCH v2] ls-files: filter pathspec before lstat
From: Jeff King @ 2026-06-11 8:41 UTC (permalink / raw)
To: Tamir Duberstein
Cc: git, René Scharfe, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <CAJ-ks9mJk-=xp1hW77hAoZwwQAfpMukYO8OvvkLx646-2Z3_kg@mail.gmail.com>
On Tue, Jun 09, 2026 at 04:15:41PM -0700, Tamir Duberstein wrote:
> On Tue, Jun 9, 2026 at 3:41 AM Jeff King <peff@peff.net> wrote:
> >
> > On Mon, Jun 08, 2026 at 07:37:15PM -0700, Tamir Duberstein wrote:
> >
> > > + /*
> > > + * match_pathspec() is linear in pathspec.nr, so prefilter only
> > > + * the single-pathspec case. Only entries shown by show_ce()
> > > + * satisfy --error-unmatch.
> > > + */
> > > + if (pathspec.nr == 1 &&
> > > + !match_pathspec(repo->index, &pathspec, fullname.buf,
> > > + fullname.len, max_prefix_len, NULL,
> > > + S_ISDIR(ce->ce_mode) ||
> > > + S_ISGITLINK(ce->ce_mode)))
> > > + continue;
> >
> > This feels...kind of arbitrary, no? Surely it's also faster with
> > pathspec.nr == 2, and so on up to some nr closer to the size of the
> > total index. It feels weird to be making an arbitrary cutoff based on
> > pathspec performance in calling code like this.
> >
> > It is not wrong, per se, as you are optimizing your case without trying
> > to hurt any others. But what do we do when somebody profiles it and
> > comes along trying to bump the number to 2, or 10?
> >
> > I dunno.
>
> Yeah, absolutely it's arbitrary. The simplest answer is that others
> are welcome to bump this, provided they make the case for it.
OK. I can live with, I suppose, but I am tempted to say that it should
just kick in always (i.e., removing the pathspec.nr check).
Though I did show a case where the performance regresses, it was pretty
made-up and not something I'd expect in the real world. And you'd see
that same crappy performance with "git ls-files -- $(git ls-files)",
without the "-m". The real solution is making the pathspec code less
crappy.
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] config: allow disabling config includes
From: Jeff King @ 2026-06-11 8:39 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git, gitster
In-Reply-To: <4d7834c0-d8ab-4dcd-8a7f-ed62c30cbe43@gmail.com>
On Tue, Jun 09, 2026 at 08:59:22AM -0400, Derrick Stolee wrote:
> > So I dunno. From the described motivation, this feels like a band-aid
> > that fixes only one narrow instance of a greater problem.
> >
> > The notion of enabling/disabling includes per-command is itself a
> > flexible building block. So it's possible that it has other uses in
> > general. But it's also a fairly broad hammer that covers more than your
> > use case. If you're planning to use "git --no-includes" in some script,
> > then it breaks the config of anybody who uses includes in their
> > user-level ~/.gitconfig file.
> >
> > So you may need some more directed limiting.
>
> Are you suggesting some kind of internal sandbox to limit Git from
> accessing repository paths from config includes and other config-set keys?
> That would be a more complete solution, but I'm not sure how we could plug
> all of those holes at once. I'll think on it, though.
I don't know that I'm really suggesting anything, but more just thinking
out loud. My concern would be that we plug one such hole, and then later
find we need more. And then we are stuck with the solution for plugging
that hole, even if it may later become redundant.
I'm not sure I entirely understand the problematic case, though. The
user points to in-repo config (which we already tell people is a bad
idea), and then that config breaks for some reason? Because the include
is relative and git is run from another directory?
If you are going to make such an include, I'd hope you'd at least do it
from .git/config, which should reliably resolve relative paths based on
the source include file, not the current working directory.
> This is exactly the kind of case I was worried about. This specific case
> only impacts write operations, but some tools do those things. And this
> email case is a common one that users do in their global config to isolate
> personal and professional identities.
Yeah, I picked it because I suspect it is a very common use of includes.
But really it is up to users to do whatever they want with includes, and
they'd probably expect them to always work.
> I'm trying to think if there's a place where we'd have some config that is
> critical to the repo functioning not in its local config (like the repo
> format version or extensions). Perhaps borrowing from your work/personal
> example, a user could use a different credential helper for work than they
> use for personal repositories.
It depends what you mean by critical, I suppose. If I happen to prefer
shoving all of my diff.*.textconv commands into ~/.gitconfig-diff, then
including it with include.path=.gitconfig-diff in ~/.gitconfig, I'd
expect it to just work everywhere. But disabling includes would violate
that assumption. Probably not _critical_, but it could be annoying and
surprising.
> Or: are we venturing into territory where we don't even want to create a
> new foot-gun? If there were another way to solve the situation that I'm
> facing without these risks, then I'd be open to it. Any ideas?
Yeah, the more I think on it, the more it seems like a foot-gun. Like I
said, I'm not sure I entirely understand the use-case. If you could
flesh out an example, that might help.
-Peff
^ permalink raw reply
* Re: [PATCH v3] transport-helper: fix TSAN race in transfer_debug()
From: Jeff King @ 2026-06-11 8:33 UTC (permalink / raw)
To: Pushkar Singh; +Cc: git, gitster
In-Reply-To: <20260609134741.4727-2-pushkarkumarsingh1970@gmail.com>
On Tue, Jun 09, 2026 at 01:47:42PM +0000, Pushkar Singh wrote:
> Changes since v2:
> - Treat an uninitialized transfer_debug_enabled as a BUG()
> instead of silently treating it as disabled.
> - Follow Jeff King's suggestion to distinguish an
> uninitialized state from a disabled state.
Thanks, this looks OK to me.
> + if (transfer_debug_enabled < 0)
> + BUG("somebody forgot to check GIT_TRANSLOOP_DEBUG!");
That is a somewhat silly message, but the point is that nobody is
supposed to see it ever. And it does lead them to roughly the right
spot, so I think it's sufficient. (Yes, I know it came from my earlier
response).
-Peff
^ permalink raw reply
* Re: [PATCH v4] git-gui: silence install recipes under "make -s"
From: Kristoffer Haugsbakk @ 2026-06-11 8:32 UTC (permalink / raw)
To: Harald Nordgren, Johannes Sixt; +Cc: git, Koji Nakamaru
In-Reply-To: <CAHwyqnVSnf9K50xgUjeHFM395Rvj_uTVvZ1U8EZayNDZeMP4Bg@mail.gmail.com>
On Thu, Jun 11, 2026, at 10:26, Harald Nordgren wrote:
> On Thu, Jun 11, 2026 at 7:37 AM Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> Am 10.06.26 um 15:19 schrieb Harald Nordgren:
>> > What does it mean for it to be queued here, should I expect it to show
>> > up on seen or next?
>> It means that I'll arrange that it will appear in the next Git release.
>> Until then you can find the commit in
>> https://github.com/j6t/git-gui/tree/hn/silence-make-s .
>
> Thanks! So does that mean that 'seen' and 'next' are branches that are
> added to only by Junio Hamano?
Yes. Because the git/git repository (on GitHub) is a mirror of his own
gitster/git repo (for these branches but not topic branches).
When he is away and there is an interim maintainer, they may push to
these branches on their own repos and use git/git as a mirror. So I
guess they all have commit access to git/git.
That is my understanding of the matter and what I have observed.
^ permalink raw reply
* Re: [PATCH v2 3/3] doc: git-config: escape erroneous highlight markup
From: Jeff King @ 2026-06-11 8:31 UTC (permalink / raw)
To: Tuomas Ahola
Cc: git, Kristoffer Haugsbakk, Junio C Hamano, Jean-Noël Avila
In-Reply-To: <20260611080242.lqXwi%taahol@utu.fi>
On Thu, Jun 11, 2026 at 11:02:42AM +0300, Tuomas Ahola wrote:
> > > If _<message>_ begins with one or more whitespaces followed
> > > -by "#", it is used as-is. If it begins with "#", a space is
> > > +by "\#", it is used as-is. If it begins with "\#", a space is
> > > prepended before it is used. Otherwise, a string " # " (a
> > > space followed by a hash followed by a space) is prepended
> >
> > I saw the comment on round 1 about this second "#" on the line. But
> > while we are here, should we be doing the one in the context, too?
> >
> > -Peff
>
> It seems adding that second backslash was already too much, as doc-diff (which
> I neglected to run before submitting V2) shows:
>
> ```
> $ ./doc-diff V1 V2
>
> If <message> begins with one or more whitespaces followed by "#",
> - it is used as-is. If it begins with "#", a space is prepended
> + it is used as-is. If it begins with "\#", a space is prepended
> before it is used. Otherwise, a string " # " (a space followed by a
> hash followed by a space) is prepended to it. The resulting string
> is placed immediately after the value defined for the variable. The
> ```
Heh, it would not be the first time I am baffled by asciidoc's parsing. :)
Adding a backslash to the third instance "fixes" the second one to me,
but I wouldn't want to rely on that (plus it breaks the third instance).
Using backticks does work, though it always opens a typographical
question. When reading the source, you see `#`, so you get a punctuation
delimiter but no typographical one. In the rendered output, you'll see
it in a typewriter font (assuming we fix the config issue), but we'd
lose the visible punctuation. I could live with that.
But for " # ", it gets weirder. We need punctuation to call out the
spaces, but what should happen to the quotes? They are not really part
of the literal string, so should they go inside or outside the
backticks? I think it may be a moot point as "` # `" is not parsed as
you might hope by asciidoc. Doing `" # "` does work, and is probably OK
enough here.
-Peff
^ permalink raw reply
* Re: [PATCH v4] git-gui: silence install recipes under "make -s"
From: Harald Nordgren @ 2026-06-11 8:26 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <fdf7f988-d345-4107-845f-e089d7829c16@kdbg.org>
On Thu, Jun 11, 2026 at 7:37 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 10.06.26 um 15:19 schrieb Harald Nordgren:
> > What does it mean for it to be queued here, should I expect it to show
> > up on seen or next?
> It means that I'll arrange that it will appear in the next Git release.
> Until then you can find the commit in
> https://github.com/j6t/git-gui/tree/hn/silence-make-s .
Thanks! So does that mean that 'seen' and 'next' are branches that are
added to only by Junio Hamano?
Harald
^ permalink raw reply
* Re: [PATCH v2 2/2] ref-filter: memoize --contains with generations
From: Jeff King @ 2026-06-11 8:22 UTC (permalink / raw)
To: Tamir Duberstein
Cc: git, Karthik Nayak, Junio C Hamano, Victoria Dye, Derrick Stolee,
Elijah Newren
In-Reply-To: <20260608-ref-filter-memoized-contains-v2-2-e72720344a7c@gmail.com>
On Mon, Jun 08, 2026 at 07:36:35PM -0700, Tamir Duberstein wrote:
> The wall-time standard deviations were 11.356 seconds and 133.8
> milliseconds, respectively. Separate runs without redirection produced
> the same output with SHA-256
> 2466f6e2b72aa16b1a2126eddb81c8a1b2764ee251204ac034c191a925aa896f.
Heh. Without the original repo, this sha256 hash is meaningless to us,
isn't it? Ditto for the sha1 the earlier command.
> int commit_contains(struct ref_filter *filter, struct commit *commit,
> struct commit_list *list, struct contains_cache *cache)
> {
> - if (filter->with_commit_tag_algo)
> + int result;
> +
> + if (!list)
> + return 1;
> + if (filter->with_commit_tag_algo ||
> + generation_numbers_enabled(the_repository))
> return contains_tag_algo(commit, list, cache) == CONTAINS_YES;
> - return repo_is_descendant_of(the_repository, commit, list);
> +
> + result = repo_is_descendant_of(the_repository, commit, list);
> + if (result < 0)
> + exit(128);
> + return result;
There's a little more going on here than I expected from the commit
message. Is it important for us to short-circuit the empty list and just
return 1? Or did the existing helper functions already handle that?
Looking at contains_tag_algo(), I think it would actually return
CONTAINS_NO here (though I didn't test it). So this is actually a change
in behavior for "git tag" if that's correct. I doubt it is triggerable
in practice, though, as we would simply never call commit_contains() in
the first place with an empty list. But if we are going to add in this
logic, I think it makes sense to do so as a separate commit (describing
what it is doing and why it's not (yet) a triggerable bug).
Checking the result of repo_is_descendant_of() makes sense, as discussed
earlier. But probably that should come as its own patch, since it's an
independent bug-fix. I'm also tempted to say it should call die()
instead of a direct exit, though it does look like the error exit paths
from repo_is_descendant_of() would all have produced their own messages.
And one side note. While looking at the implementation of
repo_is_descendant_of(), I did notice something curious: it also
switches algorithms based on the presence of generation numbers! So it
should also be cutting off the traversal early when possible. But I
guess its main problem is that we call it independently for each
candidate, so it may traverse the same (useful) stretch of history
multiple times.
So probably an alternative approach to this patch would be feeding all
of the candidates at once, the way we do with reach_filter() via
filter_refs(). I'm not sure if we have the right functions available for
that (naively, --contains and --merged are inversions of each other, so
swapping the arguments to tips_reachable_from_bases() might work, but I
didn't think very hard on it).
I wonder if that might perform better or worse. I'm content to leave it
for another day, though, as switching to the memoizing depth-first algo
here is a pretty easy change.
> - commit=$(git commit-tree $(git rev-parse HEAD^{tree})) &&
> + git rev-list --first-parent --max-count=8192 HEAD >contains-commits &&
> + test_file_not_empty contains-commits &&
> + git update-ref refs/contains-perf-base "$(tail -n 1 contains-commits)" &&
> + awk "{
> + printf \"update refs/contains-perf/%04d %s\\n\", NR, \$1
> + }" contains-commits |
> + git update-ref --stdin &&
> + git pack-refs --include "refs/contains-perf/*" &&
My head almost exploded reading the embedded quoting in that awk
invocation. But I can't think offhand of a better way to do it. You
can't use test_seq because it needs both the number and the original
string. You can do it with sed, but it probably ends up even more
unreadable.
But OK, we are making a bunch of refs based on first-parent history.
> + tree=$(git rev-parse HEAD^{tree}) &&
> + base=$(git rev-parse HEAD) &&
> + target=$(echo target | git commit-tree "$tree" -p "$base") &&
> + git update-ref refs/contains-diverged/target "$target" &&
> + for i in $(test_seq 1 4)
> + do
> + commit=$(echo candidate-$i |
> + git commit-tree "$tree" -p "$base") &&
> + git update-ref refs/contains-diverged/candidate-$i "$commit" ||
> + return 1
> + done &&
And then a few candidate refs that are not reachable from other refs, or
from each other. OK.
I think you could just write:
git commit-tree HEAD^{tree} -p HEAD
instead of doing separate rev-parses, but it's probably not a big deal
either way.
> +test_expect_success 'verify contains results' '
> + git for-each-ref --contains=refs/contains-perf-base \
> + refs/contains-perf/ >actual &&
> + test_line_count = $(wc -l <contains-commits) actual &&
> +
> + echo refs/contains-diverged/target >expect &&
> + GIT_TEST_COMMIT_GRAPH=0 \
> + git -c core.commitGraph=false for-each-ref \
> + --format="%(refname)" \
> + --contains=refs/contains-diverged/target \
> + refs/contains-diverged/ >actual &&
> + test_cmp expect actual
> +'
This is a funny test to have in the middle of a perf script (which
hardly anybody ever runs). If we are concerned about the correctness,
should this be in a non-perf test script? Though I'd imagine something
like it is already covered there.
There's a lot of subtlety in what we're verifying, too. In the first
half, we are checking that all of the commits in contains-perf contain
the base. And that base is the final element of the contains-commits
list. Which made me wonder what happens in a branch history, since that
list is linearized. But because we used --first-parent to generate it,
it _is_ linear, and the results work out. So OK, I don't think it's
wrong, but I am struggling to understand the meaning of the test.
The second half is just checking that...the other refs which are not
contained in "target" are not mentioned? OK, but why do it only with
commit graphs off. Why not both off and on? Again, I'm not sure I
understand what we're trying to focus on here.
> +test_perf 'contains: git for-each-ref --contains' '
> + git for-each-ref --contains=refs/contains-perf-base \
> + refs/contains-perf/ >/dev/null
> +'
Yay, actual perf tests. Here we have a ton of matches, and they all walk
over the same chunk of history. Should get much faster, though it's
mostly a synthetic test.
For --merged, we already have separate tests with each of for-each-ref,
branch, and tag. Should we have the same here for --contains? And should
we be using the input repo data, rather than our synthetic test? It is
nice to show off the performance with the synthetic test, but ultimately
the point of the perf suite is feeding it real workloads and looking for
regressions.
> +test_perf 'contains without generations: divergent refs' '
> + GIT_TEST_COMMIT_GRAPH=0 \
> + git -c core.commitGraph=false for-each-ref \
> + --contains=refs/contains-diverged/target \
> + refs/contains-diverged/ >/dev/null
> +'
OK, and this one should find that most of them are not contained, but
the depth-first algorithm could walk all the way down to the roots. But
we don't run it at all, since we disable commit graphs!
So what are we trying to measure here? If it left commit graphs enabled,
I think we could demonstrate that using the depth-first algorithm with
generation numbers does not make anything _worse_. I.e., that
for-each-ref and branch did not regress from the change.
> +test_expect_success 'missing ancestors are reported by contains filters' '
> + test_when_finished "git update-ref -d refs/heads/missing-parent" &&
> + {
> + echo "tree $(git rev-parse HEAD^{tree})" &&
> + echo "parent $MISSING" &&
> + git cat-file commit HEAD |
> + sed -n -e "/^author /p" -e "/^committer /p" &&
> + echo &&
> + echo "missing parent"
> + } >commit &&
> + broken=$(git hash-object -t commit -w commit) &&
> + git update-ref refs/heads/missing-parent "$broken" &&
> + for option in --contains --no-contains
> + do
> + test_must_fail git for-each-ref "$option=HEAD" \
> + refs/heads/missing-parent >out 2>err &&
> + test_must_be_empty out &&
> + test_grep "parse commit $MISSING" err ||
> + return 1
> + done
> +'
This is a great thing to test, but probably should be pulled out into
a separate patch along with the fix to check the return code.
The commit construction looks OK, and is nicer than corrupting the
repository by deleting a real object. Given that we are pulling the
idents from an existing commit, it might be simpler to just use the
whole commit as a template, like:
git cat-file commit HEAD |
sed "s/^parent /parent $MISSING/"
but it may be a matter of taste.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/2] ref-filter: memoize --contains with generations
From: Karthik Nayak @ 2026-06-11 8:16 UTC (permalink / raw)
To: Tamir Duberstein
Cc: git, Jeff King, Junio C Hamano, Victoria Dye, Derrick Stolee,
Elijah Newren
In-Reply-To: <CAJ-ks9ku=-675naKESOJJxOo0b5BmoH7=76aKZXXmUHM+=ZV0w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5749 bytes --]
Tamir Duberstein <tamird@gmail.com> writes:
> On Wed, Jun 10, 2026 at 4:47 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Tamir Duberstein <tamird@gmail.com> writes:
>>
>> > git branch and git for-each-ref call repo_is_descendant_of() for
>> > each candidate selected by --contains or --no-contains. Each call
>> > starts a new graph walk, so refs with shared history repeatedly
>> > traverse the same commits.
>> >
>> > ffc4b8012d (tag: speed up --contains calculation, 2011-06-11)
>> > introduced a depth-first walk for git tag that caches positive and
>> > negative answers across candidates. ee2bd06b0f (ref-filter: implement
>> > '--contains' option, 2015-07-07) preserved both implementations when
>> > ref-filter learned --contains.
>> >
>> > The memoized walk is not always faster. Without generation numbers,
>> > a negative check can walk to the root even when the breadth-first
>> > merge-base walk finds a nearby divergence. With generation numbers,
>> > the depth-first walk can stop below the oldest target while still
>> > reusing answers across candidates.
>> >
>> > Keep the existing memoized selection for git tag. Select it for other
>> > ref-filter callers when generation numbers are enabled, and retain
>> > the breadth-first walk otherwise.
>> >
>> > When generation numbers are unavailable, repo_is_descendant_of() can
>> > return -1 if ancestry cannot be read. The ref-filter Boolean interface
>> > treated that error as a match. Check it and exit instead. The memoized
>> > path already dies on the same parse failure, so both selected paths now
>> > fail rather than return a result.
>> >
>> > Add p1500 cases for up to 8,192 packed refs along one first-parent
>> > history and for sibling refs near the tip with generation numbers
>> > forced off.
>> >
>> > On a checkout with 62,174 remote-tracking refs and generation numbers
>> > enabled, I ran:
>> >
>> > hyperfine --warmup 0 --runs 3 \
>> > --command-name parent \
>> > '"$parent" branch -r --contains c78ae85f3ce7e >/dev/null' \
>> > --command-name this-commit \
>> > '"$this" branch -r --contains c78ae85f3ce7e >/dev/null'
>> >
>> > The results were:
>> >
>> > parent this commit
>> > elapsed 104.365 s 467.7 ms
>> > user 93.702 s 220.2 ms
>> > system 0.723 s 182.7 ms
>> >
>> > The wall-time standard deviations were 11.356 seconds and 133.8
>> > milliseconds, respectively. Separate runs without redirection produced
>> > the same output with SHA-256
>> > 2466f6e2b72aa16b1a2126eddb81c8a1b2764ee251204ac034c191a925aa896f.
>> >
>> > Both revisions were built with the default -O2 flags using Apple
>> > clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)
>> > with a 16-core Apple M4 Max (12 performance and four efficiency
>> > cores) and 128 GB RAM.
>> >
>> > Link: https://lore.kernel.org/git/1445163904-24611-1-git-send-email-Karthik.188@gmail.com/
>> > Link: https://lore.kernel.org/r/20230324191009.GA536967@coredump.intra.peff.net
>> > Link: https://lore.kernel.org/git/20260527070510.3510836-1-krka@spotify.com/
>> > Link: https://lore.kernel.org/r/20260608223430.GA340696@coredump.intra.peff.net
>> > Suggested-by: Jeff King <peff@peff.net>
>> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>> > ---
>> > commit-reach.c | 13 +++++++++--
>> > commit-reach.h | 7 ++++++
>> > t/perf/p1500-graph-walks.sh | 49 +++++++++++++++++++++++++++++++++++++++++-
>> > t/t6301-for-each-ref-errors.sh | 22 +++++++++++++++++++
>> > 4 files changed, 88 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/commit-reach.c b/commit-reach.c
>> > index 65b618959b..83a48004ef 100644
>> > --- a/commit-reach.c
>> > +++ b/commit-reach.c
>> > @@ -821,9 +821,18 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
>> > int commit_contains(struct ref_filter *filter, struct commit *commit,
>> > struct commit_list *list, struct contains_cache *cache)
>> > {
>> > - if (filter->with_commit_tag_algo)
>> > + int result;
>> > +
>> > + if (!list)
>> > + return 1;
>> > + if (filter->with_commit_tag_algo ||
>> > + generation_numbers_enabled(the_repository))
>>
>> What's stopping us from dropping `filter->with_commit_tag_algo`
>> completely and then doing?
>>
>> if (generation_numbers_enabled(the_repository))
>> return contains_algo(commit, list, cache) == CONTAINS_YES;
>> return repo_is_descendant_of(the_repository, commit, list);
>
> Jeff raised this distinction during the v1 review:
>
> https://lore.kernel.org/r/20260608223430.GA340696@coredump.intra.peff.net/
>
> `with_commit_tag_algo` preserves the existing behavior of `git tag` when
> generation numbers are unavailable. `git tag --contains` has used the
> memoized walk since ffc4b8012d (tag: speed up --contains calculation,
> 2011-06-11). Dropping the flag would send it back through repeated
> `repo_is_descendant_of()` walks in repositories without usable generation
> numbers.
>
I did read that, my question is on top of that. Do we also want to use
the non-memoized walk for 'git tag' when there are no generation numbers
available or does that not work? If not, we should mention that too in
the commit message.
> The condition in v2 implements the rule discussed there: retain the
> existing memoized path for `git tag`, and use it for other ref-filter
> callers when generation numbers make the depth-first walk reliably
> advantageous.
>
> This is probably my fault for breaking the threading between this and
> v1. Sorry about that.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/3] doc: git-config: escape erroneous highlight markup
From: Tuomas Ahola @ 2026-06-11 8:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, Kristoffer Haugsbakk, Junio C Hamano, Jean-Noël Avila
In-Reply-To: <20260611061156.GC2187173@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Thu, Jun 11, 2026 at 01:55:13AM +0300, Tuomas Ahola wrote:
>
> > Paired octothorpes are used in AsciiDoc to mark highlighted text,
> > <mark> being the equivalent HTML tag. To use the symbol as a literal
> > character, it can be escaped with a backslash.
> >
> > Do so in git-config.adoc.
>
> I think this works OK, but in general I think most uses of backslash for
> metacharacters should consider using literal backticks. That shields it
> from the special meaning for asciidoc, but also will render it
> differently for the user (usually with a typewriter font, which becomes
> bold in roff output).
>
> Though curiously the case of `#` in git-fast-import seems not to get
> marked as <code> in the html output (even though the nearby `LF` does).
> I wonder if there is some special treatment of `#` or something.
>
> > If _<message>_ begins with one or more whitespaces followed
> > -by "#", it is used as-is. If it begins with "#", a space is
> > +by "\#", it is used as-is. If it begins with "\#", a space is
> > prepended before it is used. Otherwise, a string " # " (a
> > space followed by a hash followed by a space) is prepended
>
> I saw the comment on round 1 about this second "#" on the line. But
> while we are here, should we be doing the one in the context, too?
>
> -Peff
It seems adding that second backslash was already too much, as doc-diff (which
I neglected to run before submitting V2) shows:
```
$ ./doc-diff V1 V2
If <message> begins with one or more whitespaces followed by "#",
- it is used as-is. If it begins with "#", a space is prepended
+ it is used as-is. If it begins with "\#", a space is prepended
before it is used. Otherwise, a string " # " (a space followed by a
hash followed by a space) is prepended to it. The resulting string
is placed immediately after the value defined for the variable. The
```
--Tuomas
^ permalink raw reply
* Re: [PATCH v2 1/2] commit-reach: handle cycles in contains walk
From: Jeff King @ 2026-06-11 7:29 UTC (permalink / raw)
To: Tamir Duberstein
Cc: git, Karthik Nayak, Junio C Hamano, Victoria Dye, Derrick Stolee,
Elijah Newren
In-Reply-To: <20260608-ref-filter-memoized-contains-v2-1-e72720344a7c@gmail.com>
On Mon, Jun 08, 2026 at 07:36:34PM -0700, Tamir Duberstein wrote:
> @@ -744,7 +745,7 @@ static void push_to_contains_stack(struct commit *candidate, struct contains_sta
> }
>
> static enum contains_result contains_tag_algo(struct commit *candidate,
> - const struct commit_list *want,
> + struct commit_list *want,
> struct contains_cache *cache)
OK, we must lose the const here because repo_is_descendant_of() does not
have it. We could add const to that function, though that cascades down
to a few other helpers (see below). I'm not sure if that is making the
world a better place, or if it is just const pedantry.
diff --git a/commit-reach.c b/commit-reach.c
index 5df471a313..8cede01f01 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -563,7 +563,7 @@ int repo_get_merge_bases(struct repository *r,
*/
int repo_is_descendant_of(struct repository *r,
struct commit *commit,
- struct commit_list *with_commit)
+ const struct commit_list *with_commit)
{
if (!with_commit)
return 1;
@@ -955,11 +955,12 @@ int can_all_from_reach_with_flag(struct object_array *from,
return result;
}
-int can_all_from_reach(struct commit_list *from, struct commit_list *to,
+int can_all_from_reach(const struct commit_list *from,
+ const struct commit_list *to,
int cutoff_by_min_date)
{
struct object_array from_objs = OBJECT_ARRAY_INIT;
- struct commit_list *from_iter = from, *to_iter = to;
+ const struct commit_list *from_iter = from, *to_iter = to;
int result;
timestamp_t min_commit_date = cutoff_by_min_date ? from->item->date : 0;
timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
diff --git a/commit-reach.h b/commit-reach.h
index 3f3a563d8a..76e82f827e 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -37,7 +37,7 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)
int repo_is_descendant_of(struct repository *r,
struct commit *commit,
- struct commit_list *with_commit);
+ const struct commit_list *with_commit);
int repo_in_merge_bases(struct repository *r,
struct commit *commit,
struct commit *reference);
@@ -93,7 +93,8 @@ int can_all_from_reach_with_flag(struct object_array *from,
unsigned int assign_flag,
timestamp_t min_commit_date,
timestamp_t min_generation);
-int can_all_from_reach(struct commit_list *from, struct commit_list *to,
+int can_all_from_reach(const struct commit_list *from,
+ const struct commit_list *to,
int commit_date_cutoff);
> +cycle:
> + free(contains_stack.contains_stack);
> + clear_contains_cache(cache);
> + init_contains_cache(cache);
> +
> + result = repo_is_descendant_of(the_repository, candidate, want);
> + if (result < 0)
> + exit(128);
We are feeding the whole initial "want" list, so we should get a correct
answer regardless of how far we got into the cycle, which would run into
problems (e.g., if the cycle existed only on some branch of the
history). But going back to the initial list will always be correct.
Good.
Two small points, though.
One, the call to init_contains_cache() is redundant here; the clear
function is documented as making things ready for use (it's a little
hard to grep for, due to macros, but the docs are in commit-slab.h).
It's probably not hurting anything.
Two, the call to exit(128) is unusual for our code base (I'd guess it
was cribbed off of the top-level exits in builtin/pull.c). We'd usually
die() instead. Even if repo_is_descendant_of() produced its own error
message, it may be useful to mention that we were falling back to it due
to a cycle.
But even better is if we can return the error up the stack. We do not
return errors from contains_tag_algo() currently, but it has only one
caller. And that caller may also directly return the result of
repo_is_descendant_of(). So could we just pass that along?
Perhaps not. Looking at the callers of commit_contains(), they treat the
result as a pure boolean. So probably calling die() is reasonable, and
we already do so via parse_commit_or_die() elsewhere in the algorithm.
That does leave a potential lurking bug for the non-tag-algo code path.
> + *contains_cache_at(cache, candidate) =
> + result ? CONTAINS_YES : CONTAINS_NO;
> + return result ? CONTAINS_YES : CONTAINS_NO;
So we actually cache our discovered value. Cute, and it might save us
from hitting the cycle again, though not always. E.g., two candidates A
and B share a parent P, and the cycle starts at P but does not include A
or B. We discover the cycle and cache the value for A, but discover it
again for B.
We do lose all of the existing non-cycle cached values when we call
clear_contains_cache(). But we have to at least clear out all of the
IN_PROGRESS commits. It is hard to care too much about optimizing the
outcome for this case which we expect to happen approximately never.
So I think doing the simplest correct thing is OK.
> +test_expect_success 'tag --contains handles cyclic replacement histories' '
> + first=$(git rev-parse HEAD~2) &&
> + second=$(git rev-parse HEAD~) &&
> + third=$(git rev-parse HEAD) &&
> + test_when_finished "
> + git replace -d $first
> + git replace -d $third
> + git tag -d cycle-a cycle-b
> + " &&
We usually &&-chain the commands inside test_when_finished. If they
fail, the test harness will note this and complain (if the test was not
otherwise failing). It's usually not a big deal either way, though
sometimes it can catch silly mistakes (e.g., if you wrote $second
instead of $third and the "replace -d" is quietly doing nothing at all).
I'm a little surprised that the chainlint checker doesn't catch this,
but I guess it doesn't know to recurse into the snippet handed to
test_when_finished. It probably is not really worth the trouble to teach
it to do so.
Otherwise the test looks good to me.
-Peff
^ permalink raw reply related
* Re: trailers: --only-trailers normalizes URLs to trailers
From: Kristoffer Haugsbakk @ 2026-06-11 7:03 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260611065616.GE2191159@coredump.intra.peff.net>
On Thu, Jun 11, 2026, at 08:56, Jeff King wrote:
>>[snip]
>>
>> And again I don’t think that is likely to ever happen (with a knock
>> on wood).
>
> I didn't spend much effort on the patch I showed beyond running it once.
> It would probably need tests and a doc update. I wasn't planning to run
> with it, but if you feel like doing so, please feel free to use it as
> you like.
Yeah, I want to add some tests on top
and make a sumbission (but sent a
message to first confirm that you weren't
cooking anything more ;) ). Thanks.
--
Sent from mobile
^ permalink raw reply
* Re: [PATCH v3] index-pack: retain child bases in delta cache
From: Jeff King @ 2026-06-11 6:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Arijit Banerjee, Arijit Banerjee via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Derrick Stolee,
Arijit Banerjee
In-Reply-To: <xmqqldcmxxco.fsf@gitster.g>
On Wed, Jun 10, 2026 at 07:51:19AM -0700, Junio C Hamano wrote:
> Arijit Banerjee <arijit91@gmail.com> writes:
>
> > Apologies, my earlier replies were sent through GitHub's notification
> > emails and appeared only as PR comments, so they did not reach the mailing
> > list.
> >
> > On Thu, Jun 4, 2026, Jeff King wrote:
> >> So I am happy with either v2 or v3.
> >
> > I also did not see a meaningful performance difference between v2 and v3.
> > I am happy with either direction and defer to the maintainers on whether
> > v3's more precise release is worth the added complexity.
>
> I have no strong preference either way.
Nor me. I'd probably go with v2 simply because it is shorter and less
code. If there is an optimization whose effect we cannot measure, it is
probably not worth even the few lines to have it. It could always be
resurrected if somebody finds a case where it matters.
-Peff
^ permalink raw reply
* Re: trailers: --only-trailers normalizes URLs to trailers
From: Jeff King @ 2026-06-11 6:56 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <d8f7f827-27da-41fc-af8d-72d383b24fff@app.fastmail.com>
On Wed, Jun 10, 2026 at 04:21:29PM +0200, Kristoffer Haugsbakk wrote:
> > Yeah, though if you'll allow me to nitpick your subject a moment: I
> > don't think --only-trailers is really the culprit here. It demonstrates
> > the problem because it normalizes the "trailer" it found. But the loose
> > trailer matching is the more fundamental issue. For example:
> >
> >[snip]
>
> Yeah, this is more precise. I focused a ton on the normalized output
> because that’s what makes it obvious. But the fundamental problem is
> interpreting URLs like trailers.
That makes sense. As the author of --only-trailers I immediately
wondered if I had introduced a bug in it, so I was partially motivated
by exonerating myself. ;) I agree that using it is the simplest way to
demonstrate the problem.
> > Agreed, though I think a rule like: ":// (with no whitespace)" is not a
> > valid separator. Something like this:
>
> Yes, matching on `://` strictly is a better proposal. No need to care
> about `http`, `https`, `file`, etc. And both of these would *still* have
> to be true for this change to be a false negative w.r.t. the user’s
> intentions:
>
> • They really input a trailer that looks like a URL, but it’s not meant
> to be a URL
> • They really wanted the value to start with `//`
>
> And again I don’t think that is likely to ever happen (with a knock
> on wood).
I didn't spend much effort on the patch I showed beyond running it once.
It would probably need tests and a doc update. I wasn't planning to run
with it, but if you feel like doing so, please feel free to use it as
you like.
-Peff
^ permalink raw reply
* Re: [PATCH 0/9] refs: stop using `chdir_notify_reparent()`
From: Jeff King @ 2026-06-11 6:53 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
On Wed, Jun 10, 2026 at 04:57:06PM +0200, Patrick Steinhardt wrote:
> this patch series is a follow-up of the discussion at [1]. It converts
> the reference backends to always use absolute paths internally, which
> then allows us to drop the calls to `chdir_notify_reparent()`.
We added chdir-notify to suport set_work_tree(). Commit 8500e0de3f
(set_work_tree: use chdir_notify, 2018-03-30) mentions an optimization
from 044bbbcb63 (Make git_dir a path relative to work_tree in
setup_work_tree(), 2008-06-19). That commit demonstrates some measurable
speedup from using relative versus absolute paths.
If we move to a world of all absolute paths where chdir-notify is not
necessary, will we lose that optimization?
I'm not sure how much it matters in practice these days, or if those
timings could be repeated. And they weren't all _that_ big to start
with. I guess it may depend on how deep your repo is within your
filesystem, too.
-Peff
^ permalink raw reply
* Re: [PATCH v3] describe: limit default ref iteration to tags
From: Jeff King @ 2026-06-11 6:49 UTC (permalink / raw)
To: Tamir Duberstein; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <20260610-describe-tag-ref-scope-v3-1-5aa63ab279f7@gmail.com>
On Wed, Jun 10, 2026 at 11:50:01AM -0700, Tamir Duberstein wrote:
> The runtime drops from 0.03(0.01+0.01) to 0.02(0.00+0.00). In a
> repository with 120,532 refs but only 330 tags, the same command went
> from 171.7 ms to 9.9 ms.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> Changes in v3:
> - Pack the synthetic refs to better match repositories with many refs.
> - Generate update-ref input with test_seq -f.
> - Shorten the commit message and report the p6100.6 result.
> - Link to v2: https://patch.msgid.link/20260608-describe-tag-ref-scope-v2-1-256fd36dca32@gmail.com
Thanks, this looks fine to me. I am still puzzled that your 120k ref
repo is so slow in the before case. Even bumping the perf test to 100k
refs, it ~10ms on my machine. But maybe it's a combination of not being
well packed, plus a slower filesystem.
At any rate, I think it is obvious that doing less work is always going
to be better, so there's not much need to dig too deeply into the
differences for information that is probably not that useful.
-Peff
^ permalink raw reply
* [PATCH v2 7/7] treewide: drop USE_THE_REPOSITORY_VARIABLE
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
Adapt a couple of trivial callers of `is_bare_repository()` to instead
use a repository available via the caller's context so that we can drop
the `USE_THE_REPOSITORY_VARIABLE` macro.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/repack.c | 3 +--
mailmap.c | 6 ++----
refs/reftable-backend.c | 4 +---
setup.c | 3 +--
4 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bbc6f51639..d0465fb4f5 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "builtin.h"
@@ -265,7 +264,7 @@ int cmd_repack(int argc,
if (write_bitmaps < 0) {
if (write_midx == REPACK_WRITE_MIDX_NONE &&
- (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository(the_repository)))
+ (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository(repo)))
write_bitmaps = 0;
}
if (po_args.pack_kept_objects < 0)
diff --git a/mailmap.c b/mailmap.c
index 7d8590cdd6..2d5514f833 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "git-compat-util.h"
#include "environment.h"
#include "string-list.h"
@@ -219,10 +217,10 @@ int read_mailmap(struct repository *repo, struct string_list *map)
map->strdup_strings = 1;
map->cmp = namemap_cmp;
- if (!mailmap_blob && is_bare_repository(the_repository))
+ if (!mailmap_blob && is_bare_repository(repo))
mailmap_blob = xstrdup("HEAD:.mailmap");
- if (!startup_info->have_repository || !is_bare_repository(the_repository))
+ if (!startup_info->have_repository || !is_bare_repository(repo))
err |= read_mailmap_file(map, ".mailmap",
startup_info->have_repository ?
MAILMAP_NOFOLLOW : 0);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 101ef29ac8..c151d331e7 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "../git-compat-util.h"
#include "../abspath.h"
#include "../chdir-notify.h"
@@ -288,7 +286,7 @@ static int should_write_log(struct reftable_ref_store *refs, const char *refname
{
enum log_refs_config log_refs_cfg = refs->log_all_ref_updates;
if (log_refs_cfg == LOG_REFS_UNSET)
- log_refs_cfg = is_bare_repository(the_repository) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+ log_refs_cfg = is_bare_repository(refs->base.repo) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
switch (log_refs_cfg) {
case LOG_REFS_NONE:
diff --git a/setup.c b/setup.c
index e6db80ab07..65f4ac95a8 100644
--- a/setup.c
+++ b/setup.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "git-compat-util.h"
@@ -2610,7 +2609,7 @@ static int create_default_files(struct repository *repo,
}
repo_config_set(repo, "core.filemode", filemode ? "true" : "false");
- if (is_bare_repository(the_repository))
+ if (is_bare_repository(repo))
repo_config_set(repo, "core.bare", "true");
else {
repo_config_set(repo, "core.bare", "false");
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 6/7] environment: stop using `the_repository` in `is_bare_repository()`
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
Refactor `is_bare_repository()` to take in a repository parameter so
that we no longer depend on `the_repository`. Adjust callers
accordingly.
Furthermore, move the function outside of the declarations that are only
available when `USE_THE_REPOSITORY_VARIABLE` is set, as it no longer
depends on that variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
attr.c | 4 ++--
builtin/bisect.c | 2 +-
builtin/blame.c | 2 +-
builtin/check-attr.c | 2 +-
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
builtin/history.c | 2 +-
builtin/repack.c | 2 +-
builtin/repo.c | 2 +-
builtin/reset.c | 2 +-
builtin/rev-parse.c | 2 +-
environment.c | 4 ++--
environment.h | 4 ++--
mailmap.c | 4 ++--
refs/files-backend.c | 2 +-
refs/reftable-backend.c | 2 +-
setup.c | 2 +-
transport.c | 4 ++--
worktree.c | 2 +-
19 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/attr.c b/attr.c
index 75369547b3..04cb284954 100644
--- a/attr.c
+++ b/attr.c
@@ -681,7 +681,7 @@ static enum git_attr_direction direction;
void git_attr_set_direction(enum git_attr_direction new_direction)
{
- if (is_bare_repository() && new_direction != GIT_ATTR_INDEX)
+ if (is_bare_repository(the_repository) && new_direction != GIT_ATTR_INDEX)
BUG("non-INDEX attr direction in a bare repo");
if (new_direction != direction)
@@ -848,7 +848,7 @@ static struct attr_stack *read_attr(struct index_state *istate,
res = read_attr_from_index(istate, path, flags);
} else if (tree_oid) {
res = read_attr_from_blob(istate, tree_oid, path, flags);
- } else if (!is_bare_repository()) {
+ } else if (!is_bare_repository(the_repository)) {
if (direction == GIT_ATTR_CHECKOUT) {
res = read_attr_from_index(istate, path, flags);
if (!res)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index e7c2d2f3bb..798e28f501 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -724,7 +724,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
struct object_id oid;
const char *head;
- if (is_bare_repository())
+ if (is_bare_repository(the_repository))
no_checkout = 1;
/*
diff --git a/builtin/blame.c b/builtin/blame.c
index ffbd3ce5c5..553f4cb780 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1163,7 +1163,7 @@ int cmd_blame(int argc,
revs.disable_stdin = 1;
setup_revisions(argc, argv, &revs, NULL);
- if (!revs.pending.nr && is_bare_repository()) {
+ if (!revs.pending.nr && is_bare_repository(the_repository)) {
struct commit *head_commit;
struct object_id head_oid;
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 98f64d5b92..217d83ea7d 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -116,7 +116,7 @@ int cmd_check_attr(int argc,
struct object_id initialized_oid;
int cnt, i, doubledash, filei;
- if (!is_bare_repository())
+ if (!is_bare_repository(the_repository))
setup_work_tree(the_repository);
repo_config(the_repository, git_default_config, NULL);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c1d7c672f4..44b8c70da1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1764,7 +1764,7 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
if (!head_name)
goto cleanup;
- baremirror = is_bare_repository() && remote->mirror;
+ baremirror = is_bare_repository(the_repository) && remote->mirror;
create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror;
if (baremirror) {
strbuf_addstr(&b_head, "HEAD");
diff --git a/builtin/gc.c b/builtin/gc.c
index 84a66d3240..61da30de9f 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -902,7 +902,7 @@ int cmd_gc(int argc,
die(_("failed to parse gc.logExpiry value %s"), cfg.gc_log_expire);
if (cfg.pack_refs < 0)
- cfg.pack_refs = !is_bare_repository();
+ cfg.pack_refs = !is_bare_repository(the_repository);
argc = parse_options(argc, argv, prefix, builtin_gc_options,
builtin_gc_usage, 0);
diff --git a/builtin/history.c b/builtin/history.c
index 091465a59e..fd83de8265 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -525,7 +525,7 @@ static int cmd_history_fixup(int argc,
if (action == REF_ACTION_DEFAULT)
action = REF_ACTION_BRANCHES;
- if (is_bare_repository()) {
+ if (is_bare_repository(repo)) {
ret = error(_("cannot run fixup in a bare repository"));
goto out;
}
diff --git a/builtin/repack.c b/builtin/repack.c
index 1524a9c13a..bbc6f51639 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -265,7 +265,7 @@ int cmd_repack(int argc,
if (write_bitmaps < 0) {
if (write_midx == REPACK_WRITE_MIDX_NONE &&
- (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
+ (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository(the_repository)))
write_bitmaps = 0;
}
if (po_args.pack_kept_objects < 0)
diff --git a/builtin/repo.c b/builtin/repo.c
index 71a5c1c29c..34e96514bc 100644
--- a/builtin/repo.c
+++ b/builtin/repo.c
@@ -58,7 +58,7 @@ struct repo_info_field {
static int get_layout_bare(struct repository *repo UNUSED, struct strbuf *buf)
{
- strbuf_addstr(buf, is_bare_repository() ? "true" : "false");
+ strbuf_addstr(buf, is_bare_repository(the_repository) ? "true" : "false");
return 0;
}
diff --git a/builtin/reset.c b/builtin/reset.c
index 3be6bd0121..78e69bd84b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -470,7 +470,7 @@ int cmd_reset(int argc,
if (reset_type != SOFT && (reset_type != MIXED || repo_get_work_tree(the_repository)))
setup_work_tree(the_repository);
- if (reset_type == MIXED && is_bare_repository())
+ if (reset_type == MIXED && is_bare_repository(the_repository))
die(_("%s reset is not allowed in a bare repository"),
_(reset_type_names[reset_type]));
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index bb882678fe..090e5cfbb0 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -1084,7 +1084,7 @@ int cmd_rev_parse(int argc,
continue;
}
if (!strcmp(arg, "--is-bare-repository")) {
- printf("%s\n", is_bare_repository() ? "true"
+ printf("%s\n", is_bare_repository(the_repository) ? "true"
: "false");
continue;
}
diff --git a/environment.c b/environment.c
index 9d7c908c55..bf20953415 100644
--- a/environment.c
+++ b/environment.c
@@ -132,10 +132,10 @@ const char *getenv_safe(struct strvec *argv, const char *name)
return argv->v[argv->nr - 1];
}
-int is_bare_repository(void)
+int is_bare_repository(struct repository *repo)
{
/* if core.bare is not 'false', let's see if there is a work tree */
- return the_repository->bare_cfg && !repo_get_work_tree(the_repository);
+ return repo->bare_cfg && !repo_get_work_tree(repo);
}
int have_git_dir(void)
diff --git a/environment.h b/environment.h
index afb5bcf197..164a55df2c 100644
--- a/environment.h
+++ b/environment.h
@@ -125,6 +125,8 @@ int git_default_core_config(const char *var, const char *value,
void repo_config_values_init(struct repo_config_values *cfg);
+int is_bare_repository(struct repository *repo);
+
/*
* TODO: All the below state either explicitly or implicitly relies on
* `the_repository`. We should eventually get rid of these and make the
@@ -147,8 +149,6 @@ void repo_config_values_init(struct repo_config_values *cfg);
*/
int have_git_dir(void);
-int is_bare_repository(void);
-
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
extern int trust_ctime;
diff --git a/mailmap.c b/mailmap.c
index 3b2691781d..7d8590cdd6 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -219,10 +219,10 @@ int read_mailmap(struct repository *repo, struct string_list *map)
map->strdup_strings = 1;
map->cmp = namemap_cmp;
- if (!mailmap_blob && is_bare_repository())
+ if (!mailmap_blob && is_bare_repository(the_repository))
mailmap_blob = xstrdup("HEAD:.mailmap");
- if (!startup_info->have_repository || !is_bare_repository())
+ if (!startup_info->have_repository || !is_bare_repository(the_repository))
err |= read_mailmap_file(map, ".mailmap",
startup_info->have_repository ?
MAILMAP_NOFOLLOW : 0);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4c7858787..2b27091484 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1865,7 +1865,7 @@ static int log_ref_setup(struct files_ref_store *refs,
char *logfile;
if (log_refs_cfg == LOG_REFS_UNSET)
- log_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+ log_refs_cfg = is_bare_repository(the_repository) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
files_reflog_path(refs, &logfile_sb, refname);
logfile = strbuf_detach(&logfile_sb, NULL);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4ae22922de..101ef29ac8 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -288,7 +288,7 @@ static int should_write_log(struct reftable_ref_store *refs, const char *refname
{
enum log_refs_config log_refs_cfg = refs->log_all_ref_updates;
if (log_refs_cfg == LOG_REFS_UNSET)
- log_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+ log_refs_cfg = is_bare_repository(the_repository) ? LOG_REFS_NONE : LOG_REFS_NORMAL;
switch (log_refs_cfg) {
case LOG_REFS_NONE:
diff --git a/setup.c b/setup.c
index 32f14a8688..e6db80ab07 100644
--- a/setup.c
+++ b/setup.c
@@ -2610,7 +2610,7 @@ static int create_default_files(struct repository *repo,
}
repo_config_set(repo, "core.filemode", filemode ? "true" : "false");
- if (is_bare_repository())
+ if (is_bare_repository(the_repository))
repo_config_set(repo, "core.bare", "true");
else {
repo_config_set(repo, "core.bare", "false");
diff --git a/transport.c b/transport.c
index 0f5ec30247..fc144f0aed 100644
--- a/transport.c
+++ b/transport.c
@@ -1482,7 +1482,7 @@ int transport_push(struct repository *r,
if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
- !is_bare_repository()) {
+ !is_bare_repository(the_repository)) {
struct ref *ref = remote_refs;
struct oid_array commits = OID_ARRAY_INIT;
@@ -1509,7 +1509,7 @@ int transport_push(struct repository *r,
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
- !pretend)) && !is_bare_repository()) {
+ !pretend)) && !is_bare_repository(the_repository)) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct oid_array commits = OID_ARRAY_INIT;
diff --git a/worktree.c b/worktree.c
index 7d70f2c1da..30125827fd 100644
--- a/worktree.c
+++ b/worktree.c
@@ -124,7 +124,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->is_current = is_current_worktree(worktree);
worktree->is_bare = (the_repository->bare_cfg == 1) ||
- is_bare_repository() ||
+ is_bare_repository(the_repository) ||
/*
* When in a secondary worktree we have to also verify if the main
* worktree is bare in $commondir/config.worktree.
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 5/7] environment: split up concerns of `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
The `is_bare_repository_cfg` variable tracks two different pieces of
information:
- It tracks whether the user has invoked git with the "--bare" flag,
which makes us treat any discovered Git repository as if it was a
bare repository.
- Otherwise it tracks whether the discovered `the_repository` is bare.
This makes the flag extremely confusing and creates a bit of a challenge
when handling multiple repositories in the same process.
Split up the concerns of this variable into two pieces:
- `startup_info.force_bare_repository` tracks whether the user has
passed the "--bare" flag. This is used as a hint to treat newly set
up repositories as bare regardless of whether or not they have a
worktree.
- `struct repository::bare_cfg` tracks whether or not a repository is
considered bare. This takes into account both whether the user has
passed "--bare" and the discovered state of the repository itself.
Whether or not a repository is bare is now resolved when checking the
repository's format, and is then later applied to the repository itself
via `apply_repository_format()`.
This enables a subsequent change where we make `is_bare_repository()`
not depend on global state anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 2 +-
environment.c | 5 ++---
environment.h | 1 -
git.c | 2 +-
repository.c | 1 +
repository.h | 7 +++++++
setup.c | 27 ++++++++++++++++++++-------
setup.h | 6 ++++++
worktree.c | 2 +-
9 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 52aa92fb0a..566732c9f4 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -81,7 +81,7 @@ int cmd_init_db(int argc,
const char *template_dir = NULL;
char *template_dir_to_free = NULL;
unsigned int flags = 0;
- int bare = is_bare_repository_cfg;
+ int bare = startup_info->force_bare_repository ? 1 : -1;
const char *object_format = NULL;
const char *ref_format = NULL;
const char *initial_branch = NULL;
diff --git a/environment.c b/environment.c
index 4e86335f25..9d7c908c55 100644
--- a/environment.c
+++ b/environment.c
@@ -48,7 +48,6 @@ int has_symlinks = 1;
int minimum_abbrev = 4, default_abbrev = -1;
int ignore_case;
int assume_unchanged;
-int is_bare_repository_cfg = -1; /* unspecified */
int warn_on_object_refname_ambiguity = 1;
char *git_commit_encoding;
char *git_log_output_encoding;
@@ -136,7 +135,7 @@ const char *getenv_safe(struct strvec *argv, const char *name)
int is_bare_repository(void)
{
/* if core.bare is not 'false', let's see if there is a work tree */
- return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
+ return the_repository->bare_cfg && !repo_get_work_tree(the_repository);
}
int have_git_dir(void)
@@ -342,7 +341,7 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.bare")) {
- is_bare_repository_cfg = git_config_bool(var, value);
+ the_repository->bare_cfg = git_config_bool(var, value);
return 0;
}
diff --git a/environment.h b/environment.h
index 5d6e4e6c1b..afb5bcf197 100644
--- a/environment.h
+++ b/environment.h
@@ -147,7 +147,6 @@ void repo_config_values_init(struct repo_config_values *cfg);
*/
int have_git_dir(void);
-extern int is_bare_repository_cfg;
int is_bare_repository(void);
/* Environment bits from configuration mechanism */
diff --git a/git.c b/git.c
index 36f08891ef..387eabe38c 100644
--- a/git.c
+++ b/git.c
@@ -255,7 +255,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
char *cwd = xgetcwd();
- is_bare_repository_cfg = 1;
+ startup_info->force_bare_repository = true;
setenv(GIT_DIR_ENVIRONMENT, cwd, 0);
free(cwd);
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
diff --git a/repository.c b/repository.c
index 187dd471c4..c1e91eb0da 100644
--- a/repository.c
+++ b/repository.c
@@ -73,6 +73,7 @@ void initialize_repository(struct repository *repo)
ALLOC_ARRAY(repo->index, 1);
index_state_init(repo->index, repo);
repo->check_deprecated_config = true;
+ repo->bare_cfg = -1;
repo_config_values_init(&repo->config_values_private_);
/*
diff --git a/repository.h b/repository.h
index 36e2db2633..7d649e32e7 100644
--- a/repository.h
+++ b/repository.h
@@ -117,6 +117,13 @@ struct repository {
bool worktree_initialized;
bool worktree_config_is_bogus;
+ /*
+ * Whether the repository is bare, as set by "core.bare" config or
+ * inferred during repository discovery. -1 means unset/unknown, 0
+ * means non-bare, 1 means bare.
+ */
+ int bare_cfg;
+
/*
* Path from the root of the top-level superproject down to this
* repository. This is only non-NULL if the repository is initialized
diff --git a/setup.c b/setup.c
index 71fc6b33da..32f14a8688 100644
--- a/setup.c
+++ b/setup.c
@@ -795,10 +795,22 @@ static int check_repository_format_gently(const char *gitdir,
has_common = 0;
}
- if (!has_common) {
- if (candidate->is_bare != -1)
- is_bare_repository_cfg = candidate->is_bare;
- } else {
+ if (startup_info->force_bare_repository) {
+ candidate->is_bare = 1;
+ FREE_AND_NULL(candidate->work_tree);
+ } else if (has_common) {
+ /*
+ * When sharing a common dir with another repository (e.g. a
+ * linked worktree), do not let this repository's config
+ * dictate bareness; it is inherited from the main worktree.
+ */
+ candidate->is_bare = -1;
+
+ /*
+ * Furthermore, "core.worktree" is supposed to be ignored when
+ * we have a commondir configured, unless it comes from the
+ * per-worktree configuration.
+ */
FREE_AND_NULL(candidate->work_tree);
}
@@ -1138,7 +1150,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
/* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */
if (work_tree_env)
set_git_work_tree(repo, work_tree_env);
- else if (is_bare_repository_cfg > 0) {
+ else if (repo_fmt->is_bare > 0) {
if (repo_fmt->work_tree) {
/* #22.2, #30 */
warning("core.bare and core.worktree do not make sense");
@@ -1225,7 +1237,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
- if (is_bare_repository_cfg > 0) {
+ if (repo_fmt->is_bare > 0) {
set_git_dir(repo, gitdir, (offset != cwd->len));
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
@@ -1762,6 +1774,7 @@ int apply_repository_format(struct repository *repo,
alternate_object_directories = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
}
+ repo->bare_cfg = format->is_bare;
repo_set_hash_algo(repo, format->hash_algo);
repo->objects = odb_new(repo, object_directory,
alternate_object_directories);
@@ -2571,7 +2584,7 @@ static int create_default_files(struct repository *repo,
repo_settings_set_shared_repository(repo,
init_shared_repository);
- is_bare_repository_cfg = !work_tree;
+ repo->bare_cfg = !work_tree;
/*
* We would have created the above under user's umask -- under
diff --git a/setup.h b/setup.h
index 705d1d6ff7..b9fd96bea6 100644
--- a/setup.h
+++ b/setup.h
@@ -292,6 +292,12 @@ enum sharedrepo {
int git_config_perm(const char *var, const char *value);
struct startup_info {
+ /*
+ * Whether the user is asking us to treat the repository as bare via
+ * `git --bare`, even if it's not.
+ */
+ bool force_bare_repository;
+
int have_repository;
const char *prefix;
const char *original_cwd;
diff --git a/worktree.c b/worktree.c
index 97eddc3916..7d70f2c1da 100644
--- a/worktree.c
+++ b/worktree.c
@@ -123,7 +123,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
worktree->repo = the_repository;
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->is_current = is_current_worktree(worktree);
- worktree->is_bare = (is_bare_repository_cfg == 1) ||
+ worktree->is_bare = (the_repository->bare_cfg == 1) ||
is_bare_repository() ||
/*
* When in a secondary worktree we have to also verify if the main
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 4/7] builtin/init: stop modifying `is_bare_repository_cfg`
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
We're modifying `is_bare_repository_cfg` in "builtin/init.c" to indicate
whether the newly created repository is supposed to be a bare repository
or not.
This is ultimately unnecessary though: when initializing the repository
in `init_db()` we eventually set `is_bare_repository_cfg = !work_tree`,
so all that matters is whether or not we have a working tree configured,
and the working tree is set up in the non-bare in "builtin/init.c".
Stop modifying the global variable in "builtin/init.c" in favor of a
local variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index b4343c2804..52aa92fb0a 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -81,6 +81,7 @@ int cmd_init_db(int argc,
const char *template_dir = NULL;
char *template_dir_to_free = NULL;
unsigned int flags = 0;
+ int bare = is_bare_repository_cfg;
const char *object_format = NULL;
const char *ref_format = NULL;
const char *initial_branch = NULL;
@@ -90,7 +91,7 @@ int cmd_init_db(int argc,
const struct option init_db_options[] = {
OPT_STRING(0, "template", &template_dir, N_("template-directory"),
N_("directory from which templates will be used")),
- OPT_SET_INT(0, "bare", &is_bare_repository_cfg,
+ OPT_SET_INT(0, "bare", &bare,
N_("create a bare repository"), 1),
{
.type = OPTION_CALLBACK,
@@ -116,7 +117,7 @@ int cmd_init_db(int argc,
argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
- if (real_git_dir && is_bare_repository_cfg == 1)
+ if (real_git_dir && bare == 1)
die(_("options '%s' and '%s' cannot be used together"), "--separate-git-dir", "--bare");
if (real_git_dir && !is_absolute_path(real_git_dir))
@@ -160,7 +161,7 @@ int cmd_init_db(int argc,
} else if (0 < argc) {
usage(init_db_usage[0]);
}
- if (is_bare_repository_cfg == 1) {
+ if (bare == 1) {
char *cwd = xgetcwd();
setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
free(cwd);
@@ -187,7 +188,7 @@ int cmd_init_db(int argc,
*/
git_dir = xstrdup_or_null(getenv(GIT_DIR_ENVIRONMENT));
work_tree = xstrdup_or_null(getenv(GIT_WORK_TREE_ENVIRONMENT));
- if ((!git_dir || is_bare_repository_cfg == 1) && work_tree)
+ if ((!git_dir || bare == 1) && work_tree)
die(_("%s (or --work-tree=<directory>) not allowed without "
"specifying %s (or --git-dir=<directory>)"),
GIT_WORK_TREE_ENVIRONMENT,
@@ -224,10 +225,10 @@ int cmd_init_db(int argc,
strbuf_release(&sb);
}
- if (is_bare_repository_cfg < 0)
- is_bare_repository_cfg = guess_repository_type(git_dir);
+ if (bare < 0)
+ bare = guess_repository_type(git_dir);
- if (!is_bare_repository_cfg) {
+ if (!bare) {
const char *git_dir_parent = strrchr(git_dir, '/');
if (work_tree) {
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 3/7] setup: remove global `git_work_tree_cfg` variable
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
The global `git_work_tree_cfg` variable used to be modified by both
"setup.c" and by "builtin/init-db.c". We have refactored the latter user
to not use that variable at all anymore in a preceding commit, which
makes "setup.c" the only remaining user.
Even for "setup.c" it is unnecessary though, as we only ever set it to
the value we have stored in the discovered repository format. The
consequence is that we only ever set it in case we already have it set
to the same value in our discovered repository format, which makes it
redundant.
Refactor the code so that we instead use the worktree configuration as
discovered via the repository format. Drop the global variable.
Note that in `check_repository_format_gently()` we now have to free the
candidate work tree variable. This change is required to retain previous
semantics: before we essentially had an implicit `else` branch where we
set `git_work_tree_cfg = NULL`, but we were able to elide that branch
because we already knew that it would be `NULL` anyway. Now that we use
the candidate work tree directly to populate the repository's work tree
though we have to clear it to retain those semantics.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/setup.c b/setup.c
index 52228b42a1..71fc6b33da 100644
--- a/setup.c
+++ b/setup.c
@@ -31,9 +31,6 @@ enum allowed_bare_repo {
ALLOWED_BARE_REPO_ALL,
};
-/* This is set by setup_git_directory_gently() and/or git_default_config() */
-static char *git_work_tree_cfg;
-
static struct startup_info the_startup_info;
struct startup_info *startup_info = &the_startup_info;
const char *tmp_original_cwd;
@@ -799,13 +796,10 @@ static int check_repository_format_gently(const char *gitdir,
}
if (!has_common) {
- if (candidate->is_bare != -1) {
+ if (candidate->is_bare != -1)
is_bare_repository_cfg = candidate->is_bare;
- }
- if (candidate->work_tree) {
- free(git_work_tree_cfg);
- git_work_tree_cfg = xstrdup(candidate->work_tree);
- }
+ } else {
+ FREE_AND_NULL(candidate->work_tree);
}
return 0;
@@ -1145,7 +1139,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
if (work_tree_env)
set_git_work_tree(repo, work_tree_env);
else if (is_bare_repository_cfg > 0) {
- if (git_work_tree_cfg) {
+ if (repo_fmt->work_tree) {
/* #22.2, #30 */
warning("core.bare and core.worktree do not make sense");
repo->worktree_config_is_bogus = true;
@@ -1156,15 +1150,15 @@ static const char *setup_explicit_git_dir(struct repository *repo,
free(gitfile);
return NULL;
}
- else if (git_work_tree_cfg) { /* #6, #14 */
- if (is_absolute_path(git_work_tree_cfg))
- set_git_work_tree(repo, git_work_tree_cfg);
+ else if (repo_fmt->work_tree) { /* #6, #14 */
+ if (is_absolute_path(repo_fmt->work_tree))
+ set_git_work_tree(repo, repo_fmt->work_tree);
else {
char *core_worktree;
if (chdir(gitdirenv))
die_errno(_("cannot chdir to '%s'"), gitdirenv);
- if (chdir(git_work_tree_cfg))
- die_errno(_("cannot chdir to '%s'"), git_work_tree_cfg);
+ if (chdir(repo_fmt->work_tree))
+ die_errno(_("cannot chdir to '%s'"), repo_fmt->work_tree);
core_worktree = xgetcwd();
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
@@ -1217,7 +1211,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
return NULL;
/* --work-tree is set without --git-dir; use discovered one */
- if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || repo_fmt->work_tree) {
char *to_free = NULL;
const char *ret;
@@ -1267,7 +1261,7 @@ static const char *setup_bare_git_dir(struct repository *repo,
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
/* --work-tree is set without --git-dir; use discovered one */
- if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || repo_fmt->work_tree) {
static const char *gitdir;
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 2/7] builtin/init: simplify logic to configure worktree
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
In the preceding commit we have stopped modifying the global
`git_work_tree_cfg` variable. With this change there's now some code
paths where we end up setting the local `git_work_tree_cfg` variable,
but without actually using the value for anything.
Refactor the code a bit so that we only set the worktree configuration
in case it's actually needed. Furthermore, reflow it a bit to make the
code easier to follow.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 01bc27904e..b4343c2804 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -229,24 +229,29 @@ int cmd_init_db(int argc,
if (!is_bare_repository_cfg) {
const char *git_dir_parent = strrchr(git_dir, '/');
- char *git_work_tree_cfg = NULL;
- if (git_dir_parent) {
- char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
- git_work_tree_cfg = real_pathdup(rel, 1);
- free(rel);
- }
- if (!git_work_tree_cfg)
- git_work_tree_cfg = xgetcwd();
- if (work_tree)
+ if (work_tree) {
set_git_work_tree(the_repository, work_tree);
- else
- set_git_work_tree(the_repository, git_work_tree_cfg);
+ } else {
+ char *work_tree_cfg = NULL;
+
+ if (git_dir_parent) {
+ char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
+ work_tree_cfg = real_pathdup(rel, 1);
+ free(rel);
+ }
+
+ if (!work_tree_cfg)
+ work_tree_cfg = xgetcwd();
+
+ set_git_work_tree(the_repository, work_tree_cfg);
+
+ free(work_tree_cfg);
+ }
+
if (access(repo_get_work_tree(the_repository), X_OK))
die_errno (_("Cannot access work tree '%s'"),
repo_get_work_tree(the_repository));
-
- free(git_work_tree_cfg);
}
else {
if (real_git_dir)
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 1/7] builtin/init: stop modifying global `git_work_tree_cfg` variable
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260611-b4-pks-setup-drop-global-state-v2-0-a6f7269c841d@pks.im>
When executing git-init(1) we need to figure out the final location of
the worktree. This location can be configured in a couple of ways: via
an environment variable, via the preexisting "core.worktree" config in
case we're reinitializing, or implicitly when reinitializing a non-bare
repository.
When checking for the worktree location in "builtin/init-db.c" we
populate any potentially-discovered value both by setting the global
`git_work_tree_cfg` variable and via `set_git_work_tree()`, which
ultimately ends up modifying `struct repository::worktree`.
Modifying `git_work_tree_cfg` is unnecessary though: we configure the
worktree in `create_default_files()`, and that function derives the
worktree location via `repo_get_work_tree()`. Consequently, propagating
the worktree via `set_git_work_tree()` is sufficient.
Stop munging `git_work_tree_cfg` and make it file-local to "setup.c" and
function-local to `cmd_init_db()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/init-db.c | 4 ++++
environment.c | 3 ---
environment.h | 1 -
setup.c | 3 +++
4 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index c55517ad94..01bc27904e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -229,6 +229,8 @@ int cmd_init_db(int argc,
if (!is_bare_repository_cfg) {
const char *git_dir_parent = strrchr(git_dir, '/');
+ char *git_work_tree_cfg = NULL;
+
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
git_work_tree_cfg = real_pathdup(rel, 1);
@@ -243,6 +245,8 @@ int cmd_init_db(int argc,
if (access(repo_get_work_tree(the_repository), X_OK))
die_errno (_("Cannot access work tree '%s'"),
repo_get_work_tree(the_repository));
+
+ free(git_work_tree_cfg);
}
else {
if (real_git_dir)
diff --git a/environment.c b/environment.c
index fc3ed8bb1c..4e86335f25 100644
--- a/environment.c
+++ b/environment.c
@@ -100,9 +100,6 @@ int auto_comment_line_char;
bool warn_on_auto_comment_char;
#endif /* !WITH_BREAKING_CHANGES */
-/* This is set by setup_git_directory_gently() and/or git_default_config() */
-char *git_work_tree_cfg;
-
/*
* Repository-local GIT_* environment variables; see environment.h for details.
*/
diff --git a/environment.h b/environment.h
index ccfcf37bfb..5d6e4e6c1b 100644
--- a/environment.h
+++ b/environment.h
@@ -149,7 +149,6 @@ int have_git_dir(void);
extern int is_bare_repository_cfg;
int is_bare_repository(void);
-extern char *git_work_tree_cfg;
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
diff --git a/setup.c b/setup.c
index b4652651df..52228b42a1 100644
--- a/setup.c
+++ b/setup.c
@@ -31,6 +31,9 @@ enum allowed_bare_repo {
ALLOWED_BARE_REPO_ALL,
};
+/* This is set by setup_git_directory_gently() and/or git_default_config() */
+static char *git_work_tree_cfg;
+
static struct startup_info the_startup_info;
struct startup_info *startup_info = &the_startup_info;
const char *tmp_original_cwd;
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH v2 0/7] setup: drop global state
From: Patrick Steinhardt @ 2026-06-11 6:44 UTC (permalink / raw)
To: git; +Cc: Justin Tobler
In-Reply-To: <20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im>
Hi,
this patch series continues to refactor "setup.c", where the focus is to
drop remaining global state that we have in "setup.c". The most
important consequence of this is that we don't need to rely on
`the_repository` in `is_bare_repository()` anymore.
This series is built on top of 1ff279f340 (The 13th batch, 2026-06-09)
with ps/setup-centralize-odb-creation at 42b9d3dc9d (setup: construct
object database in `apply_repository_format()`, 2026-06-04) merged into
it.
Changes in v2:
- Improve documentation for some aspects of `check_repository_format_gently()`.
- Link to v1: https://patch.msgid.link/20260610-b4-pks-setup-drop-global-state-v1-0-5dff3eec8f06@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (7):
builtin/init: stop modifying global `git_work_tree_cfg` variable
builtin/init: simplify logic to configure worktree
setup: remove global `git_work_tree_cfg` variable
builtin/init: stop modifying `is_bare_repository_cfg`
environment: split up concerns of `is_bare_repository_cfg`
environment: stop using `the_repository` in `is_bare_repository()`
treewide: drop USE_THE_REPOSITORY_VARIABLE
attr.c | 4 ++--
builtin/bisect.c | 2 +-
builtin/blame.c | 2 +-
builtin/check-attr.c | 2 +-
builtin/fetch.c | 2 +-
builtin/gc.c | 2 +-
builtin/history.c | 2 +-
builtin/init-db.c | 44 +++++++++++++++++++++++++-----------------
builtin/repack.c | 3 +--
builtin/repo.c | 2 +-
builtin/reset.c | 2 +-
builtin/rev-parse.c | 2 +-
environment.c | 10 +++-------
environment.h | 6 ++----
git.c | 2 +-
mailmap.c | 6 ++----
refs/files-backend.c | 2 +-
refs/reftable-backend.c | 4 +---
repository.c | 1 +
repository.h | 7 +++++++
setup.c | 51 +++++++++++++++++++++++++++++--------------------
setup.h | 6 ++++++
transport.c | 4 ++--
worktree.c | 4 ++--
24 files changed, 97 insertions(+), 75 deletions(-)
Range-diff versus v1:
1: 0281a4bca9 = 1: 96b71f5223 builtin/init: stop modifying global `git_work_tree_cfg` variable
2: 6fdc8d77e8 = 2: a51c0ff79d builtin/init: simplify logic to configure worktree
3: ce31595ff5 ! 3: e06393ddc5 setup: remove global `git_work_tree_cfg` variable
@@ Commit message
Refactor the code so that we instead use the worktree configuration as
discovered via the repository format. Drop the global variable.
+ Note that in `check_repository_format_gently()` we now have to free the
+ candidate work tree variable. This change is required to retain previous
+ semantics: before we essentially had an implicit `else` branch where we
+ set `git_work_tree_cfg = NULL`, but we were able to elide that branch
+ because we already knew that it would be `NULL` anyway. Now that we use
+ the candidate work tree directly to populate the repository's work tree
+ though we have to clear it to retain those semantics.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## setup.c ##
4: 6a69dc853c = 4: 628ed54c8c builtin/init: stop modifying `is_bare_repository_cfg`
5: afa2d8bbda ! 5: 02ceaf4a20 environment: split up concerns of `is_bare_repository_cfg`
@@ setup.c: static int check_repository_format_gently(const char *gitdir,
+ * dictate bareness; it is inherited from the main worktree.
+ */
+ candidate->is_bare = -1;
++
++ /*
++ * Furthermore, "core.worktree" is supposed to be ignored when
++ * we have a commondir configured, unless it comes from the
++ * per-worktree configuration.
++ */
FREE_AND_NULL(candidate->work_tree);
}
6: 04849a2cb5 = 6: a08aef5685 environment: stop using `the_repository` in `is_bare_repository()`
7: 78191c7557 = 7: f93f6599df treewide: drop USE_THE_REPOSITORY_VARIABLE
---
base-commit: f5a08a09a0fdf0fc2a355eba7979e2cfd65659e5
change-id: 20260422-b4-pks-setup-drop-global-state-6b1374aed5db
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox