* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-06-30 7:19 UTC (permalink / raw)
To: Matt Hunter
Cc: phillip.wood, Patrick Steinhardt,
Harald Nordgren via GitGitGadget, git
In-Reply-To: <DJM1N17VMUM5.3V5Y6YMFLIFQJ@lfurio.us>
> This is probably a larger question, since (according to the man page) it
> affects the other 'git history' commands as well. When I run
> 'git history ...' and discover that I made a mistake after inspecting
> the results, is there a fool-proof way to undo the change and return to
> the previous state? My first thought was to run 'git reset --hard ...',
> but the default behavior of --update-refs (moving other branches) can
> make this more complicated.
This is a larger question: But I would love to have a reflog that is
more human-centered. When e.g. rebasing a series with N commits, it's
very tricky in the reflog to find what was the state before that.
I feel like branch switching is given too much space in the reflog,
since it's not a destructive action, I don't care about it.
And when handling multiple commits in on go (squashing, rebasing), I
would love to see a visual hierarchy (with indentation for sub-steps)
instead of treating each action as equally important when it isn't.
Harald
^ permalink raw reply
* Re: [PATCH 1/2] odb/source: generalize `reprepare()` callback
From: Toon Claes @ 2026-06-30 8:18 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <akINy-hP5EPD4Y4e@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Jun 26, 2026 at 02:10:32PM +0200, Toon Claes wrote:
>> > diff --git a/builtin/grep.c b/builtin/grep.c
>> > index 8080d1bf5e..7361bf071e 100644
>> > --- a/builtin/grep.c
>> > +++ b/builtin/grep.c
>> > @@ -1361,10 +1360,8 @@ int cmd_grep(int argc,
>> > struct odb_source *source;
>> >
>> > odb_prepare_alternates(the_repository->objects);
>> > - for (source = the_repository->objects->sources; source; source = source->next) {
>> > - struct odb_source_files *files = odb_source_files_downcast(source);
>> > - odb_source_packed_prepare(files->packed);
>> > + for (source = the_repository->objects->sources; source; source = source->next)
>> > + odb_source_prepare(source, 0);
>>
>> So you're downcasting inside the implementation by the backends itself.
>> That makes sense, but would it be worth to say something about that in
>> the commit message?
>
> Hm. Would that provide much value? I'm probably quite a bit biased here,
> but I think that it's implicit that the backends have to eventually cast
> the generic structure to their own backend.
>
> So I wouldn't really know how to clarify this. Did you have anything
> specific in mind?
Ah, I'm sorry, I misread that. I thought you changed the vtable function
to do the downcasting, but you're simply changing from calling a
`*_packed_*()` to the generic variant that goes through the vtable.
Anyhow, not worth mentioning in the commit message.
>> > diff --git a/odb/source-packed.c b/odb/source-packed.c
>> > index 42c28fba0e..fa5a072488 100644
>> > --- a/odb/source-packed.c
>> > +++ b/odb/source-packed.c
>> > @@ -15,7 +15,7 @@ static int find_pack_entry(struct odb_source_packed *store,
>> > {
>> > struct packfile_list_entry *l;
>> >
>> > - odb_source_packed_prepare(store);
>> > + odb_source_prepare(&store->base, 0);
>>
>> Why are you not using ODB_PREPARE_FLUSH_CACHES here? It used to do
>> before?
>
> Because this was calling `odb_source_packed_prepare()` before, not
> `odb_source_reprepare()`. So this was calling the non-flushing
> variant.
Again, confusion on my end.
>> > if (store->midx && fill_midx_entry(store->midx, oid, e))
>> > return 1;
>> >
>> > @@ -47,7 +47,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
>> > * been added since the last time we have prepared the packfile store.
>> > */
>> > if (flags & OBJECT_INFO_SECOND_READ)
>> > - odb_source_reprepare(source);
>> > + odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
>>
>> I think the new code is correct, but why wasn't `packed` used here in
>> the past? The old odb_source_reprepare() expected a downcasted, didn't
>> it?
>
> No, `odb_source_reprepare()` is the generic variant. The naming schema
> is typically:
>
> - `odb_source_frobnicate()` for the generic variants, which receive a
> `struct odb_source` as input.
>
> - `odb_source_<type>_frobnitcate()` for their backend-specific
> implementations, which cast down the generic `struct odb_source` to
> their backend-specific struct.
Yeah, I understand things better now. Thanks for clarifying.
--
Cheers,
Toon
^ permalink raw reply
* Re: What's cooking in git.git (Jun 2026, #10)
From: Toon Claes @ 2026-06-30 8:20 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqq5x36dtyf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> * ps/odb-generalize-prepare (2026-06-22) 3 commits
> - odb: introduce `odb_prepare()`
> - odb/source: generalize `reprepare()` callback
> - Merge branch 'ps/odb-source-packed' into ps/odb-generalize-prepare
> (this branch uses ps/odb-source-packed.)
>
> The `reprepare()` callback for object database sources has been
> generalized into a `prepare()` callback with an optional flush cache
> flag, and a new `odb_prepare()` wrapper has been introduced to
> allow pre-opening object database sources.
>
> Needs review.
> source: <20260622-b4-pks-odb-generalize-prepare-v1-0-d2a5c5d13144@pks.im>
I did have some questions/remarks, but Patrick answered them, and with
those answers I'm happy about this series.
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH 2/2] odb: introduce `odb_prepare()`
From: Patrick Steinhardt @ 2026-06-30 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Toon Claes, git
In-Reply-To: <xmqqa4sdt3e6.fsf@gitster.g>
On Mon, Jun 29, 2026 at 02:58:41PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> According to my grep results are there 17 callsites for odb_reprepare(),
> >> then I agree it makes sense to create this wrapper.
> >
> > Yeah, I was a bit torn myself whether or not to keep the wrapper. I
> > eventually decided to just keep it because it reduces churn, and it's a
> > trivial wrapper anyway.
>
> That sounds OK. Are we all happy with the current shape of the
> topic? I myself did not find anything iffy in these two patches.
Based on Toon's reply [1] it seems like this series is ready to go.
Thanks!
Patrick
[1]: <87ik704f1j.fsf@emacs.iotcl.com>
^ permalink raw reply
* Re: [PATCH 6/6] builtin/receive-pack: stage incoming objects via ODB transactions
From: Patrick Steinhardt @ 2026-06-30 8:45 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <akLLB_J-pvJ7iR7c@denethor>
On Mon, Jun 29, 2026 at 03:25:18PM -0500, Justin Tobler wrote:
> On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> > On Tue, Jun 23, 2026 at 11:19:20PM -0500, Justin Tobler wrote:
> > > Objects received by git-receive-pack(1) are quarantined in a temporary
> > > "incoming" directory and migrated into the object database prior to the
> > > reference updates. The quarantine is currently managed through
> > > `tmp_objdir` directly. In a pluggable ODB future, how exactly an object
> > > gets written to a transaction may vary for a given ODB source. Refactor
> > > git-receive-pack(1) to use the ODB transaction interfaces to manage the
> > > object staging area in a more agnostic manner accordingly.
> > >
> > > Note that the temporary directory created for git-receive-pack(1) is
> > > eagerly created and uses a different prefix name. This behavior is
> >
> > A different prefix name compared to what?
>
> Currently the temporary directories created for ODB transactions all use
> the prefix "bulk-fsync". The temp dir created by git-receive-pack(1) is
> expected to have the prefix "incoming".
>
> > > special cased in the "files" backend by having `odb_transaction_begin()`
> > > callers that require this behavior provide an `ODB_TRANSACTION_RECEIVE`
> > > flag.
> >
> > Okay. I guess this is to retain existing behaviour where the temporary
> > directory is created lazily everywhere else. Makes me wonder whether we
> > should eventually change this to just unconditionally create the
> > directory in all cases so that we can drop this new flag.
>
> It would be nice to not have to have a flag here, but if we want to also
> keep the existing temp dir prefixes, we would also need to keep the
> flags.
Fair. Makes me wonder whether we really need to keep the exact same
naming for this temporary directory. This is so deep into internals that
I'm not sure whether we really need to treat this as part of our
interface. I'm rather inclined to say it's not necessary.
In any case, I think it's fine to defer that discussion and keep this
as-is for now. But we might keep it in the back of our minds and maybe
simplify this in a subsequent patch series.
> > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > > index 19eb6a1b61..ee8e03e2ab 100644
> > > --- a/builtin/receive-pack.c
> > > +++ b/builtin/receive-pack.c
> > > @@ -2326,7 +2323,8 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
> > > ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
> > > }
> > >
> > > -static const char *unpack(int err_fd, struct shallow_info *si)
> > > +static const char *unpack(int err_fd, struct shallow_info *si,
> > > + struct odb_transaction *transaction)
> > > {
> > > struct pack_header hdr;
> > > const char *hdr_err;
> >
> > It feels a bit weird that we sometimes pass the transaction as
> > parameter, whereas othertimes we access it via `the_repository`.
>
> That's fair. I was trying to avoid the churn of wiring to all its
> callsites, but it's probably best to be consistent. Maybe it would be
> fine to just create a transaction global like we do for the reference
> transaction?
My first kneejerk reaction was "no", but then I noticed that the global
variable you're talking about is local to "builtin/receive-pack.c". So
that might be an okayish solution.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Patrick Steinhardt @ 2026-06-30 8:45 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <akK1roQJknYstX0u@denethor>
On Mon, Jun 29, 2026 at 01:58:54PM -0500, Justin Tobler wrote:
> On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> > On Tue, Jun 23, 2026 at 11:19:16PM -0500, Justin Tobler wrote:
> > > diff --git a/object-file.c b/object-file.c
> > > index a3eb8d71dd..18c2df75fb 100644
> > > --- a/object-file.c
> > > +++ b/object-file.c
> > > @@ -499,7 +499,7 @@ struct odb_transaction_files {
> > > struct transaction_packfile packfile;
> > > };
> > >
> > > -static void odb_transaction_files_prepare(struct odb_transaction *base)
> > > +static int odb_transaction_files_prepare(struct odb_transaction *base)
> > > {
> > > struct odb_transaction_files *transaction =
> > > container_of_or_null(base, struct odb_transaction_files, base);
> >
> > By the way, is there any reason why those functions are still hosted in
> > "object-file.c" instead of in "odb/source-files.c"? I should probably
> > know, but I forgot.
>
> There are currently a couple spots in the "files" object write path in
> "object-file.c" that still reach into some of these transaction function
> that are not part of the generic ODB transaction interface. I'm hoping
> in a future followup series to detangle this a bit and be able to get
> all the "files" ODB transaction related code moved into
> "odb/source-files.c".
Makes sense. I also revisited that code a couple days ago, and the
answer is "it's messy right now". Hopefully this will become easier to
detangle as we make progress on pluggifying transactions.
> > > @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> > > * added at the time they call odb_transaction_files_begin.
> > > */
> > > if (!transaction || transaction->objdir)
> > > - return;
> > > + return 0;
> > >
> > > transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
> > > - if (transaction->objdir)
> > > - tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> > > + if (!transaction->objdir)
> > > + return -1;
> >
> > Huh. So previously we just didn't handle this error at all and just
> > continued to tag along? Did that result in anything sensible or was this
> > just YOLOing it?
>
> Good question. Previously if there was an error, we wouldn't end up
> creating any tmpdir and would instead continue to use the primary ODB to
> write objects in. This change would make it a hard error if we fail to
> create the temp dir. This matches the behavior that git-receive-pack(1)
> expects, but I didn't consider that the existing callers could
> transparently handle there being no temp dir.
>
> I suspect we may want existing ODB transaction users to continue being
> resilient in the same manner. In the next version, I'll maintain the
> same behavior.
Honestly I'd say that the change is a good one. I cannot think of a
single reason to just blindly not create the transaction and proceed.
But it certainly is something that should be documented as part of the
commit message.
> > > @@ -1670,27 +1678,34 @@ int read_loose_object(struct repository *repo,
> > > return ret;
> > > }
> > >
> > > -static void odb_transaction_files_commit(struct odb_transaction *base)
> > > +static int odb_transaction_files_commit(struct odb_transaction *base)
> > > {
> > > struct odb_transaction_files *transaction =
> > > container_of(base, struct odb_transaction_files, base);
> > >
> > > - flush_loose_object_transaction(transaction);
> > > + if (flush_loose_object_transaction(transaction))
> > > + return -1;
> > > flush_packfile_transaction(transaction);
> > > +
> > > + return 0;
> > > }
> > >
> > > -struct odb_transaction *odb_transaction_files_begin(struct odb_source *source)
> > > +int odb_transaction_files_begin(struct odb_source *source,
> > > + struct odb_transaction **out)
> > > {
> > > struct odb_transaction_files *transaction;
> > > struct object_database *odb = source->odb;
> > >
> > > - if (odb->transaction)
> > > - return NULL;
> > > + if (odb->transaction) {
> > > + *out = NULL;
> > > + return 0;
> > > + }
> > >
> > > transaction = xcalloc(1, sizeof(*transaction));
> > > transaction->base.source = source;
> > > transaction->base.commit = odb_transaction_files_commit;
> > > transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
> > > + *out = &transaction->base;
> > >
> > > - return &transaction->base;
> > > + return 0;
> > > }
> >
> > It's still somewhat fishy that we have this ODB-level transaction, but
> > that's a preexisting issue and thus outside the scope of this patch
> > series. Ideally though, it would be possible for there to be multiple
> > transactions, and it would be the caller's responsibility for juggling
> > these transactions. Just as it happens with reference transactions.
>
> I completely agree. One of the current problems preventing this is that
> only a single instance of tmp_objdir is allowed and stored globally.
> This is done to keep atexit() cleanup simple.
>
> My plan is to eventually convert all existing tmp_objdir callsites to
> use ODB transactions which should hopefully allow us to remove the need
> for a separate tmp_objdir system. At that point, we can also work to
> change how temp dir cleanup is handled at exit.
Great.
> Another problem is that there are also a couple of ODB transaction
> callsites that require to know if there is already a pending transaction
> for the repository and the transaction has not been wired down to these
> callsites. My hope is that this can be addressed though as we expand ODB
> transaction usage for object writes.
Yeah, agreed. Making the use of transactions explicit feels sensible to
me.
Patrick
^ permalink raw reply
* Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Patrick Steinhardt @ 2026-06-30 8:45 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <akLBFaTfBEq8vHUr@denethor>
On Mon, Jun 29, 2026 at 02:04:08PM -0500, Justin Tobler wrote:
> On 26/06/29 01:58PM, Justin Tobler wrote:
> > On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> > > On Tue, Jun 23, 2026 at 11:19:16PM -0500, Justin Tobler wrote:
> > > > @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> > > > * added at the time they call odb_transaction_files_begin.
> > > > */
> > > > if (!transaction || transaction->objdir)
> > > > - return;
> > > > + return 0;
> > > >
> > > > transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
> > > > - if (transaction->objdir)
> > > > - tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> > > > + if (!transaction->objdir)
> > > > + return -1;
> > >
> > > Huh. So previously we just didn't handle this error at all and just
> > > continued to tag along? Did that result in anything sensible or was this
> > > just YOLOing it?
> >
> > Good question. Previously if there was an error, we wouldn't end up
> > creating any tmpdir and would instead continue to use the primary ODB to
> > write objects in. This change would make it a hard error if we fail to
> > create the temp dir. This matches the behavior that git-receive-pack(1)
> > expects, but I didn't consider that the existing callers could
> > transparently handle there being no temp dir.
> >
> > I suspect we may want existing ODB transaction users to continue being
> > resilient in the same manner. In the next version, I'll maintain the
> > same behavior.
>
> I think I got a bit ahead of myself. The existing callers of
> odb_transaction_files_prepare() still continue to ignore this error. So
> the behavior already does remain the same here.
Oh, well, okay. I think this behaviour is plain bad -- if the caller
wants to have a transaction, then we should bail in case we cannot
create one. But this doesn't need to be fixed in this patch series.
Patrick
^ permalink raw reply
* Re: [PATCH 0/3] fixing expensive http test timeouts
From: Patrick Steinhardt @ 2026-06-30 9:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Michael Montalbo, git
In-Reply-To: <xmqqik71xqtc.fsf@gitster.g>
On Mon, Jun 29, 2026 at 09:19:11AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> pushes only to "cast in stone" branches. If there are other
> >> branches that deserve to be tested with TEST_LONG upon other events
> >> that the existing GitHub Actions CI does not trigger, it may be good
> >> to have GitLab CI cover them, perhaps?
> >
> > I'm a bit hesitant to do such a split, mostly because the canonical
> > source of truth that the project typically uses is GitHub's CI. So I
> > want us at GitLab to be able to catch the same issues that GitHub would
> > flag. And if GitLab's CI stopped detecting everything that GitHub does,
> > then the result would likely be that we often create merge requests on
> > both platforms, which would only result in more wasted resources.
>
> I didn't suggest splitting them into two circles that overlap but
> each with area only it covers, though. GitLab's coverage can be
> superset to GitHub's and that would satify what I suggested.
Fair.
> FWIW, I do not consider GitHub's CI "the canonical source" at all.
> It is a very handy service to use to check how well we are doing,
> but from time to time it has its own hiccups ;-).
Well, GitLab of course has its own share of hiccups, like for example
the Chocolatey issues we've been facing.
> What can we do to make the visibility of GitLab's CI more prominent?
>
> I know where the CI jobs that are triggered when I push out the
> integration branches are found at GitHub's website[*], but I do not
> think I know the corresponding one at GitLab, for example, and I
> think that is a shame.
The pipelines of the official mirror can be found at [1]. We might for
example add something like the below patch to our README.md to make it
more discoverable.
Patrick
[1]: https://gitlab.com/git-scm/git/-/pipelines
diff --git a/README.md b/README.md
index d87bca1b8c..9ad77fdf7e 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,5 @@
-[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
+[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
+[](https://gitlab.com/git-scm/git/-/pipelines?ref=master)
Git - fast, scalable, distributed revision control system
=========================================================
^ permalink raw reply related
* Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Patrick Steinhardt @ 2026-06-30 9:05 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
In-Reply-To: <20260629203527.GA1895313@coredump.intra.peff.net>
On Mon, Jun 29, 2026 at 04:35:27PM -0400, Jeff King wrote:
> There's one other thing you might find interesting. While poking at the
> timings here the other day, I noticed that reftable is very eager to
> stat the tables.list file. Try this:
>
> git init --ref-format=reftable
> blob=$(echo foo | git hash-object -w --stdin)
> seq -f "create refs/tags/foo-%g $blob" 2000 |
> strace -c git update-ref --stdin
>
> We make 2000 fstat, which strace claims takes 85% of the time. I suspect
> this is over-emphasized because strace inherently makes syscalls slow,
> but running with perf also highlights it as a non-trivial cost.
Yeah, this rings a bell. If I remember correctly, this is because we
call `refs_resolve_ref_unsafe()` to verify whether the target already
exists. And as that function is generic, it wasn't easy to optimize it
by reusing the already-loaded reftable stack.
> It has been a long time since I've thought about reftable internals, but
> it feels like we ought to be able to take the lock and then trust that
> the stack has not been manipulated.
Yeah, that would certainly be an option to explore.
> It may not be worth digging into too much, though. I can make 50,000
> refs in 150ms on my system, which is probably good enough (especially
> compared to the files backend).
True, it's going to be much better compared to the "files" backend. But
that isn't enough reason to not optimize it even further -- doubly so if
it actually takes 85% of the time. Sounds like a low-hanging fruit to me
that can result in a significant speedup.
I'll probably not get to it anytime soon, but I'll create an issue to
keep track of it.
Patrick
^ permalink raw reply
* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Matt Hunter @ 2026-06-30 9:23 UTC (permalink / raw)
To: Harald Nordgren
Cc: phillip.wood, Patrick Steinhardt,
Harald Nordgren via GitGitGadget, git
In-Reply-To: <CAHwyqnVBEOm+FwD+i9Aa7edTvdnDPJom1zubcXgoExZnp--vWQ@mail.gmail.com>
On Tue Jun 30, 2026 at 3:19 AM EDT, Harald Nordgren wrote:
>> This is probably a larger question, since (according to the man page) it
>> affects the other 'git history' commands as well. When I run
>> 'git history ...' and discover that I made a mistake after inspecting
>> the results, is there a fool-proof way to undo the change and return to
>> the previous state? My first thought was to run 'git reset --hard ...',
>> but the default behavior of --update-refs (moving other branches) can
>> make this more complicated.
>
> This is a larger question: But I would love to have a reflog that is
> more human-centered. When e.g. rebasing a series with N commits, it's
> very tricky in the reflog to find what was the state before that.
>
> I feel like branch switching is given too much space in the reflog,
> since it's not a destructive action, I don't care about it.
I share these headaches to an extent. When dealing with the first
problem (seeing an atomic entry in the reflog), I usually look at the
branch's own reflog instead of HEAD's
But my question is about doing a comprehensive reset from a botched
operation. If any history operation updates branch refs besides the
current one. I don't think there's an obvious way to see which other
ones were affected, and a naive 'git reset --hard my-branch@{1}' leaves
them pointed at unwanted commits. Is this right?
tangent: I'm pretty sure that git-status relies on checkout / branch
switching reflog entries in order to know which tag your detached HEAD
started from, eg: when it says something like
HEAD detached from v2.55.0-rc2
nothing to commit, working tree clean
>
> And when handling multiple commits in on go (squashing, rebasing), I
> would love to see a visual hierarchy (with indentation for sub-steps)
> instead of treating each action as equally important when it isn't.
Sounds compelling!
^ permalink raw reply
* Re: [PATCH v4 3/3] replay: offer an option to linearize the commit topology
From: Johannes Schindelin @ 2026-06-30 9:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Toon Claes, git, Elijah Newren
In-Reply-To: <akInDBlyWbbRFcLH@pks.im>
Hi Patrick & Toon,
On Tue, 30 Jun 2026, Patrick Steinhardt wrote:
> On Fri, Jun 26, 2026 at 07:36:31AM +0200, Toon Claes wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > git-rebase(1) essentially knows about three different modes:
> > >
> > > - "--no-rebase-merges", which is the default and maps to your
> > > "--linearize".
> > >
> > > - "--rebase-merges", which by default doesn't rebase cousins by using
> > > "--ancestry-path" internally.
> > >
> > > - "--rebase-merges=rebase-cousins", which doesn't pass the above
> > > option.
> > >
> > > So it's not a simple boolean there, which makes me wonder whether we
> > > should mirror the same interface so that all of git-rebase(1)'s modes
> > > can be represented, as well.
> >
> > That's a valid question, although I don't know a good answer to that.
> >
> > Basically you're asking for what the command line options will look
> > like? Allow me to think out loud.
> >
> > In this series I'm adding --linearize to git-replay(1). As mentioned, I
> > don't think it makes sense to add it to git-history(1) as well. Without
> > this option, the process aborts when it encounters a merge.
> >
> > Dscho sent a patch series to properly replay (2-way) merges. I think
> > this should become the default for both git-replay(1) and
> > git-history(1).
> >
> > But then, do we want to have an option that brings back the current
> > behavior of aborting at merges? Maybe with --no-merges?
>
> I think that would be a sensible option to have.
I also think that we'll need a way to abort at merges because linearizing
commits is a relatively common operation.
> > Then there's the option of rebasing cousins left. That's something that
> > isn't covered by Dscho's series yet. Maybe --replay-cousins?
> >
> > To reiterate what the final design could look like:
> >
> > * <nothing>: replay merges preserving topology.
> > * "--linearize": flattens merges (only git-replay(1)).
> > * "--no-merges": dies when the process tries to replay a merge.
> > * "--replay-cousins": does what --rebase-merges=rebase-cousins does.
>
> Right. And if we tried to be consistent with git-rebase(1), then this
> could be done as:
>
> - "--rebase-merges" to replay merges preserving topology, which is the
> default once we support replaying them.
>
> - "--no-rebase-merges" to flatten commits.
>
> - "--rebase-merges=abort" to explicitly die when seeing merges.
>
> - "--rebase-merges=rebase-cousins"
The `git rebase` options are unlikely to be a good precedent to follow.
Their history is full of usability warts, and in hindsight, I would really
have loved a more steady hand in developing and maintaining a good UX. The
fact alone that this is called `rebase` speaks volumes about how hostile
of a user experience this command surfaces.
In any case, these options should use the much more natural term "replay"
instead of "rebase".
But then: you said that `--no-rebase-merges` should flatten the commits?
That's not what this option name conveys to me; It would convey to me that
the operation would _abort_ on encountering merge commits.
In other words, I do think that the --linearize option is conceptually
quite distinct from the different modes in which merge commits could be
handled. As such, this option should probably not be conflated with
the various `--replay-merges=<mode>` modes.
> > Now, all these options are (I think) mutually exclusive, so we could
> > consider an option "--replay-merges=<mode>", but personally I find
> > "--<option>=<value>" arguments harder to use than specifying separate
> > options.
> >
> > I think I'm avoiding your question, because the design of the command
> > line parameters doesn't need tot 1-on-1 correlate to the internal
> > datastructure. And I agree the mode isn't a boolean, but does that mean
> > we want to use an enum internally? Well, I don't know. And I also don't
> > think that matters right now. Code is easy to change, I think the
> > command line options should be designed with the future in mind, which I
> > believe we do with "--linearize".
> >
> > Sorry for this long-winded rambling, but bottom line I think it's fine
> > to add --linearize and in the future add more options and see how the
> > code should evolve to support those.
>
> Hm, I dunno. You basically reasoned that we potentially want to have all
> of the same options that git-rebase(1)'s "--rebase-merges=" already
> supports. So that begs the question why we need to reinvent the wheel
> then and not just use the same syntax.
I would strongly caution against repeating the same UX mistakes as `git
rebase` has to live with.
The _functionality_, yes, I think that'd be good to have in `git replay`.
But we can surface that functionality in much better ways, with option
names that reflect the concepts much more intuitively.
Ciao,
Johannes
> Note that I'm not arguing that we should support all of these options
> now. I'm merely arguing that we should try to be consistent, unless
> there is a good argument not to do that. I'm fine with the interface if
> there indeed is a good argument, but if so we should document why we
> think that the current interface in git-rebase(1) is not a good fit for
> this command.
>
> Thanks!
>
> Patrick
>
>
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Patrick Steinhardt @ 2026-06-30 10:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak
In-Reply-To: <20260630064301.GB3733961@coredump.intra.peff.net>
On Tue, Jun 30, 2026 at 02:43:01AM -0400, Jeff King wrote:
> In prepare_bases() we do a custom revision walk, separate from the main
> format-patch walk. After we finish, we fail to call release_revisions(),
> possibly leaking its contents.
>
> We failed to notice it so far because the revision machinery doesn't
> always allocate. But at least one case can trigger the leak: if a commit
> graph is present, then the topo-walk allocates revs.topo_walk_info and
> some associated data structures. You can see it in the test suite by
> running:
>
> make SANITIZE=leak
> cd t
> GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
>
> which yields many entries like:
>
> ==git==3687620==ERROR: LeakSanitizer: detected memory leaks
> Direct leak of 200 byte(s) in 1 object(s) allocated from:
> #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
> #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
> #2 0x55cd452cdd9d in xmalloc wrapper.c:76
> #3 0x55cd45255473 in init_topo_walk revision.c:3845
> #4 0x55cd45255bef in prepare_revision_walk revision.c:4017
> #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
> #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
Interesting. Makes me wonder whether we should modify linux-TEST-vars to
also run with the leak checker enabled. Ideally we'd of course just do
this for all jobs, but the overhead is probably way too high... yes,
doing a simple benchmark shows a ~3x hit.
So this is definitely nothing we want to do for all jobs. But for the
linux-TEST-vars job it might make sense, as it exercises a bunch of
non-default code paths.
> The un-released rev_info has been there since the code was added in
> fa2ab86d18 (format-patch: add '--base' option to record base tree info,
> 2016-04-26), but back then we didn't even have a way to release rev_info
> resources! The actual leak probably started around f0d9cc4196
> (revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's
> hard to bisect because there were so many other unrelated leaks back
> then.
>
> So I'm not sure exactly when the leak started beyond "long ago", but it
> is easy-ish to find now (since we've plugged all those other leaks) and
> the solution is clear.
>
> I didn't add a new test since we can demonstrate it with the existing
> ones, but it does require tweaking a test variable. We might consider
> ways to get more automatic leak-checking coverage there, but I think it
> should be done outside of this fix.
Yeah, agreed.
One thing worth noting: there are still six test suites that are failing
with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x
failures are because of the missing call to `repo_unuse_commit_buffer()`
in git-history(1), which we already noted elsewhere.
All of the remaining leaks in t0095, t4013 and t4211 seem to be related
to bloom filters.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/log.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index d027ce1e0b..350b35c556 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1888,6 +1888,7 @@ static void prepare_bases(struct base_tree_info *bases,
> bases->nr_patch_id++;
> }
> clear_commit_base(&commit_base);
> + release_revisions(&revs);
> }
The fix looks sensible to me. We always initialize `revs` before we take
this exit path here, and there is no other early return that we'd have
to adjust.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 0/5] builtin/refs: add ability to write references
From: Patrick Steinhardt @ 2026-06-30 10:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqik71ul0j.fsf@gitster.g>
On Mon, Jun 29, 2026 at 01:52:44PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Reference-related functionality in Git is currently spread across many
> > different commands: git-update-ref(1), git-for-each-ref(1),
> > git-show-ref(1), git-pack-refs(1) and git-symbolic-ref(1). This makes it
> > hard for users to discover what functionality we have available to work
> > with references.
> >
> > We have thus started to consolidate this functionality into git-refs(1),
> > which is a toolbox of everything related to references. Until now, the
> > command doesn't handle functionality of git-update-ref(1).
>
> This unfortunately hasn't heard any responses since June 17th, so I
> took a look at it again myself. All the things we discussed during
> the review of the initial round has been addressed, it seems.
>
> Shall we mark the topic ready for 'next' now?
Let me send one last reroll to fix the typo you pointed out. But other
than that I think this should be ready.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 4/5] builtin/refs: add "create" subcommand
From: Patrick Steinhardt @ 2026-06-30 10:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq5x31ukqv.fsf@gitster.g>
On Mon, Jun 29, 2026 at 01:58:32PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > + if (repo_get_oid_with_flags(repo, argv[1], &newoid, GET_OID_SKIP_AMBIGUITY_CHECK))
> > + die(_("invalid object ID: '%s'"), argv[1]);
> > + if (is_null_oid(&newoid))
> > + die(_("cannot create reference with null old object ID"));
>
> An apparent typo here, "with null old" -> "with null new object
> name".
>
> Other than that, I think this one is good.
Yes, indeed, good eyes.
Patrick
^ permalink raw reply
* git-blame vs. abbrev
From: Laszlo Ersek @ 2026-06-30 11:15 UTC (permalink / raw)
To: git
Hi,
when git-blame is passed the "-b" option ("Show blank SHA-1 for boundary
commits"), shouldn't git-blame *stop* reserving a commit hash nibble for
the caret that otherwise marks boundary commits?
More directly, I find it inconvenient that git-blame shows commit hashes
that are one nibble longer (13) than my "core.abbrev" (12) setting; that
makes cutting and pasting commit hashes from the git-blame output into a
git-rebase TODO list cumbersome. I briefly hoped that by setting
"blame.blankBoundary", I could get around that, but it doesn't seem to
work (I tried with Git 2.55). I now have an alias that passes
"--abbrev=11" explicitly, as a last resort, to git-blame. (Even a
potential "blame.abbrev" would be superior, but such a permanent setting
doesn't seem to exist.)
Thanks,
Laszlo Ersek
^ permalink raw reply
* Re: [PATCH 2/6] odb: make backend-specific fields optional
From: Patrick Steinhardt @ 2026-06-30 11:28 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <akKmwPGSAGEGKZjL@denethor>
On Mon, Jun 29, 2026 at 12:25:21PM -0500, Justin Tobler wrote:
> On 26/06/24 02:19PM, Patrick Steinhardt wrote:
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 8726485f1f..adc626ce30 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -269,32 +301,20 @@ struct object_info {
> > */
> > time_t *mtimep;
> >
> > + /*
> > + * Backend-specific information that tells the caller where exactly an
> > + * object was looked up from. This information should help disambiguate
> > + * object lookups in case the same object exists in multiple sources,
> > + * or multiple times in the same source.
> > + */
> > + struct object_info_source *sourcep;
>
> To me, the name `sourcep` makes me think a pointer to `struct
> odb_source`. This did confuse me slightly when initially reading, but
> I'm not sure it's worth it to be overly verbose here.
Yeah, good point. But as you say, I haven't been able to really come up
with a name that is not overly verbose. We could potentially rename the
structure itself to `odb_source_info` and then call the field itself
`source_infop`. Would that help?
Patrick
^ permalink raw reply
* Re: [PATCH 1/6] packfile: thread odb_source_packed through packed_object_info()
From: Patrick Steinhardt @ 2026-06-30 11:28 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <akKge0zmT3WSfdyz@denethor>
On Mon, Jun 29, 2026 at 12:01:47PM -0500, Justin Tobler wrote:
> On 26/06/24 02:19PM, Patrick Steinhardt wrote:
> > Add an optional `struct odb_source_packed *source` parameter to
> > `packed_object_info()` and `packed_object_info_with_index_pos()`. This
> > parameter is unused at this point in time, but it will be used in a
> > follow-up commit so that we can record the source of a specific object.
>
> Ok so `packed_object_info()` is responsible for populating `struct
> object_info` from the provided packfile and object offset. By
> additionally providing the object source, the ultimate goal is to store
> the this information in `struct object_info` or some equivalent
> structure.
>
> At first, I wondered if it would make more sense for `struct packed_git`
> to record the `struct odb_source_packed` it comes from, but maybe that
> wouldn't be the best layer to handle this bookkeeping?
Yeah, I was thinking about that, too. But I feel like that would be a
layering violation: a packfile can in theory live standalone without a
source. So tracking that information as part of the packfile itself just
feels wrong to me.
We could in theory adapt all callers of `packed_object_info()` to track
the origin of the packfiles. I _think_ that should be feasible at almost
all sites. But I'm just not sure myself whether that really buys us much
in the first place, because...
> > Note that callers in "odb/source-packed.c" pass the already-available
> > source, but all other callers pass `NULL` instead. This is fine though,
> > as we only care about populating this info when called via the packed
> > store.
>
> Hmmm, is this because knowing the ODB source the object comes from is
> only useful for callers from in "odb/source-packed.c"? Maybe this will
> become a bit more clear to me in subsequent patches.
... right now none none of the callers that call `packed_object_info()`
directly care about the source information at all. It's really only
callers of `odb_read_object_info()` that do.
So I understand that this feels a bit iffy. But arguably, the right way
to fix this is to stop using `packed_object_info()` altogether. It is an
internal implementation detail of the object source backend, and ideally
we shouldn't need to care about it.
I already have a patch series that fixes git-cat-file(1). The
commit-graph is a bigger building site, as I'm still not a 100% decided
on how to represent such auxiliary data structures with pluggable object
backends. And for the other commands I don't yet have a good answer.
Patrick
^ permalink raw reply
* Re: [PATCH 3/6] odb: add `source` field to struct object_info_source
From: Patrick Steinhardt @ 2026-06-30 11:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Justin Tobler, git
In-Reply-To: <xmqqmrwdul8y.fsf@gitster.g>
On Mon, Jun 29, 2026 at 01:47:41PM -0700, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
>
> >> @@ -1424,6 +1424,10 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source UNUSED,
> >> oi->whence = OI_PACKED;
> >>
> >> if (oi->sourcep) {
> >> + if (!source)
> >> + BUG("cannot request source without an owning source");
> >> + oi->sourcep->source = &source->base;
> >
> > And here it is set for the packed backend. Looks good.
> >
> > Naive question: I understand that some `packed_info_object()` callers
> > may not have the `struct odb_source` on hand, but when the `struct
> > packed_git` is intially setup, is it not always known the ODB source it
> > comes from? It makes me wonder if the ODB source should also be recorded
> > when `struct packed_git` is initialized.
I've addressed this comment on patch 1.
> As with your reaction to [PATCH 1/6], I do share this puzzlement: if
> the source can almost always be NULL, what is it good for and isn't
> it something that can be computed from the available information?
It's not almost always NULL, even though it looks like this because we
ended up adapting more callers to pass `NULL` than we adapted callers to
pass an actual source. But in the end it's rather the opposite: there
are very few low-level callers that don't have the source info
available, and everyone else instead uses `odb_read_object_info()`,
where we do have it available. But those callers don't need to be
adjusted, so they weren't visible in the diff.
> Perhaps it is the naming?
Yeah, as Justin pointed out, calling this `sourcep` is confusing.
> I am confused what the above quoted code actually is doing ("if you
> have a source, then grab its base and set it to .source member of
> the struct the out parameter points at", makes it sound like the out
> parameter sourcep should be pointing at a structure with .base
> member, not .source member, or perhaps the caller should be passing
> &oi->sourcep->source as *base to be assigned to, or something).
We have to return the generic source here, not the specialized source,
so that this interface can be used by every implementation. Other sites
would end up storing their own source, which of course would have a
different specialized backend.
So an alternative to write this would have been:
oi->sourcep->source = (struct odb_source *) source;
But by assigning the base we avoid having to cast.
Patrick
^ permalink raw reply
* Re: [PATCH v4 3/3] replay: offer an option to linearize the commit topology
From: Patrick Steinhardt @ 2026-06-30 11:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Toon Claes, git, Elijah Newren
In-Reply-To: <9e7d14c4-82f0-2b89-b07b-f219119a199b@gmx.de>
On Tue, Jun 30, 2026 at 11:44:47AM +0200, Johannes Schindelin wrote:
> On Tue, 30 Jun 2026, Patrick Steinhardt wrote:
> > On Fri, Jun 26, 2026 at 07:36:31AM +0200, Toon Claes wrote:
> > > Then there's the option of rebasing cousins left. That's something that
> > > isn't covered by Dscho's series yet. Maybe --replay-cousins?
> > >
> > > To reiterate what the final design could look like:
> > >
> > > * <nothing>: replay merges preserving topology.
> > > * "--linearize": flattens merges (only git-replay(1)).
> > > * "--no-merges": dies when the process tries to replay a merge.
> > > * "--replay-cousins": does what --rebase-merges=rebase-cousins does.
> >
> > Right. And if we tried to be consistent with git-rebase(1), then this
> > could be done as:
> >
> > - "--rebase-merges" to replay merges preserving topology, which is the
> > default once we support replaying them.
> >
> > - "--no-rebase-merges" to flatten commits.
> >
> > - "--rebase-merges=abort" to explicitly die when seeing merges.
> >
> > - "--rebase-merges=rebase-cousins"
>
> The `git rebase` options are unlikely to be a good precedent to follow.
> Their history is full of usability warts, and in hindsight, I would really
> have loved a more steady hand in developing and maintaining a good UX. The
> fact alone that this is called `rebase` speaks volumes about how hostile
> of a user experience this command surfaces.
>
> In any case, these options should use the much more natural term "replay"
> instead of "rebase".
>
> But then: you said that `--no-rebase-merges` should flatten the commits?
> That's not what this option name conveys to me; It would convey to me that
> the operation would _abort_ on encountering merge commits.
>
> In other words, I do think that the --linearize option is conceptually
> quite distinct from the different modes in which merge commits could be
> handled. As such, this option should probably not be conflated with
> the various `--replay-merges=<mode>` modes.
Fair enough. Arguments like this are basically what I want to read in
the commit message. As said in the below snippet: I'm not against
diverging from the git-rebase(1) interface, but if we do that we should
document why we think that the current interface is bad.
[snip]
> > Note that I'm not arguing that we should support all of these options
> > now. I'm merely arguing that we should try to be consistent, unless
> > there is a good argument not to do that. I'm fine with the interface if
> > there indeed is a good argument, but if so we should document why we
> > think that the current interface in git-rebase(1) is not a good fit for
> > this command.
Thanks!
Patrick
^ permalink raw reply
* [PATCH 00/13] setup: split up repository discovery and setup
From: Patrick Steinhardt @ 2026-06-30 11:47 UTC (permalink / raw)
To: git
Hi,
this patch series is the next set of refactorings to simplify how we
configure repositories in "setup.c".
The setup of the repository is essentially happening in two phases:
1. We discover the location of the repository as well as its format.
2. We then use this information to configure the repository.
So far so sensible. In our code base though these two phases are quite
intertwined with one another, as we continue to repeatedly call
`set_git_dir()` and `set_work_tree()` on the repository as we discover
its locations. This makes it hard to follow the logic, and it basically
leaves us with a partially-configured repository.
This patch series splits this up into two proper phases that are
completely separate from one another. The first phase now populates a
`struct repo_discovery` structure, without even having access to any
repository. The second phase then takes that structure and configures
the repository accordingly.
Ultimately, the motivation of this whole exercise is that eventually we
can unify configuration of the repository into `repo_init()` instead of
having bits and pieces thereof distributed across "repository.c" and
"setup.c".
This series is built on top of v2.55.0 with the following three branches
merged into it:
- ps/refs-onbranch-fixes at d6522d01df (refs: protect against
chicken-and-egg recursion, 2026-06-25).
- ps/setup-drop-global-state at 1ceee7431b (treewide: drop
USE_THE_REPOSITORY_VARIABLE, 2026-06-11).
- jk/repo-info-path-keys at 3ac28d832a (repo: add path.gitdir with
absolute and relative suffix formatting, 2026-06-24).
Thanks!
Patrick
---
Patrick Steinhardt (13):
setup: rename `check_repository_format_gently()`
setup: mark bogus worktree in `apply_repository_format()`
setup: unify setup of shallow file
setup: split up concerns of `setup_git_env_internal()`
setup: introduce explicit repository discovery
setup: embed repository format in discovery
setup: move prefix into repository
setup: drop static `cwd` variable
setup: propagate prefix via repository discovery
setup: make repository discovery self-contained
setup: drop redundant configuration of `startup_info->have_repository`
setup: pass worktree to `init_db()`
setup: mark `set_git_work_tree()` as file-local
builtin/clone.c | 8 +-
builtin/init-db.c | 34 ++--
builtin/repo.c | 8 +-
builtin/rev-parse.c | 5 +-
builtin/update-index.c | 4 +-
common-init.c | 20 +++
git.c | 2 +-
object-name.c | 4 +-
repository.c | 1 +
repository.h | 8 +
setup.c | 419 ++++++++++++++++++++++++++-----------------------
setup.h | 7 +-
trace.c | 4 +-
13 files changed, 283 insertions(+), 241 deletions(-)
---
base-commit: b340fc4c4f3850656b726ff757b42d2020215378
change-id: 20260618-pks-setup-split-discovery-and-setup-d7f23831803c
^ permalink raw reply
* [PATCH 01/13] setup: rename `check_repository_format_gently()`
From: Patrick Steinhardt @ 2026-06-30 11:47 UTC (permalink / raw)
To: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-0-13864eb5a032@pks.im>
The function `check_repository_format_gently()` receives a format as
input. An unknowing reader may thus suspect that this function actually
checks the passed-in format for consistency. While the function indeed
checks the repository format, it actually serves two purposes:
- It reads the repository's format and populates the passed-in format
with that information.
- It then indeed checks whether the format is consistent.
Rename the function to `read_and_verify_repository_format()` to clarify
its functionality. While at it, reorder the parameters so that the
format comes first to better match other functions that pass around the
format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/setup.c b/setup.c
index 951ab9eedb..118416e350 100644
--- a/setup.c
+++ b/setup.c
@@ -749,9 +749,9 @@ static int check_repo_format(const char *var, const char *value,
return read_worktree_config(var, value, ctx, vdata);
}
-static int check_repository_format_gently(const char *gitdir,
- struct repository_format *candidate,
- int *nongit_ok)
+static int read_and_verify_repository_format(struct repository_format *format,
+ const char *gitdir,
+ int *nongit_ok)
{
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -759,7 +759,7 @@ static int check_repository_format_gently(const char *gitdir,
has_common = get_common_dir(&sb, gitdir);
strbuf_addstr(&sb, "/config");
- read_repository_format(candidate, sb.buf);
+ read_repository_format(format, sb.buf);
strbuf_release(&sb);
/*
@@ -767,10 +767,10 @@ static int check_repository_format_gently(const char *gitdir,
* we treat a missing config as a silent "ok", even when nongit_ok
* is unset.
*/
- if (candidate->version < 0)
+ if (format->version < 0)
return 0;
- if (verify_repository_format(candidate, &err) < 0) {
+ if (verify_repository_format(format, &err) < 0) {
if (nongit_ok) {
warning("%s", err.buf);
strbuf_release(&err);
@@ -780,37 +780,37 @@ static int check_repository_format_gently(const char *gitdir,
die("%s", err.buf);
}
- string_list_clear(&candidate->unknown_extensions, 0);
- string_list_clear(&candidate->v1_only_extensions, 0);
+ string_list_clear(&format->unknown_extensions, 0);
+ string_list_clear(&format->v1_only_extensions, 0);
- if (candidate->worktree_config) {
+ if (format->worktree_config) {
/*
* pick up core.bare and core.worktree from per-worktree
* config if present
*/
strbuf_addf(&sb, "%s/config.worktree", gitdir);
- git_config_from_file(read_worktree_config, sb.buf, candidate);
+ git_config_from_file(read_worktree_config, sb.buf, format);
strbuf_release(&sb);
has_common = 0;
}
if (startup_info->force_bare_repository) {
- candidate->is_bare = 1;
- FREE_AND_NULL(candidate->work_tree);
+ format->is_bare = 1;
+ FREE_AND_NULL(format->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;
+ format->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);
+ FREE_AND_NULL(format->work_tree);
}
return 0;
@@ -1141,7 +1141,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
die(_("not a git repository: '%s'"), gitdirenv);
}
- if (check_repository_format_gently(gitdirenv, repo_fmt, nongit_ok)) {
+ if (read_and_verify_repository_format(repo_fmt, gitdirenv, nongit_ok)) {
free(gitfile);
return NULL;
}
@@ -1218,7 +1218,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
struct repository_format *repo_fmt,
int *nongit_ok)
{
- if (check_repository_format_gently(gitdir, repo_fmt, nongit_ok))
+ if (read_and_verify_repository_format(repo_fmt, gitdir, nongit_ok))
return NULL;
/* --work-tree is set without --git-dir; use discovered one */
@@ -1266,7 +1266,7 @@ static const char *setup_bare_git_dir(struct repository *repo,
{
int root_len;
- if (check_repository_format_gently(".", repo_fmt, nongit_ok))
+ if (read_and_verify_repository_format(repo_fmt, ".", nongit_ok))
return NULL;
setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
@@ -1874,7 +1874,7 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
struct strbuf err = STRBUF_INIT;
set_git_dir(repo, ".", 0);
- check_repository_format_gently(".", &fmt, NULL);
+ read_and_verify_repository_format(&fmt, ".", NULL);
if (apply_repository_format(repo, &fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
die("%s", err.buf);
startup_info->have_repository = 1;
@@ -2836,7 +2836,7 @@ int init_db(struct repository *repo,
* config file, so this will not fail. What we are catching
* is an attempt to reinitialize new repository with an old tool.
*/
- check_repository_format_gently(repo_get_git_dir(repo), &repo_fmt, NULL);
+ read_and_verify_repository_format(&repo_fmt, repo_get_git_dir(repo), NULL);
repository_format_configure(&repo_fmt, hash, ref_storage_format);
if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
die("%s", err.buf);
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH 02/13] setup: mark bogus worktree in `apply_repository_format()`
From: Patrick Steinhardt @ 2026-06-30 11:47 UTC (permalink / raw)
To: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-0-13864eb5a032@pks.im>
When a repository is configured to have both "core.worktree" and
"core.bare" we emit a warning and mark the worktree configuration as
bogus so that the next call to `setup_work_tree()` will cause us to die.
This allows us to still use the misconfigured repository, at least as
long as we don't try to use its worktree.
This condition is handled in `setup_explicit_git_dir()`. In a subsequent
commit we'll refactor this function so that it doesn't receive a repo as
input anymore though, and consequently we cannot set the "bogus" bit
anymore.
Move the logic into `apply_repository_format()` instead to prepare for
this. While at it, fix up formatting a bit.
Note that this change requires us to also explicitly unset the value of
"core.worktree" in case we have the "GIT_WORK_TREE" environment variable
set. This is because the environment variable overrides the repository's
configuration, and we don't want to warn or die in case the work tree
has been configured explicitly regardless of whether or not "core.bare"
is set.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/setup.c b/setup.c
index 118416e350..f54eac5e5a 100644
--- a/setup.c
+++ b/setup.c
@@ -1147,24 +1147,24 @@ static const char *setup_explicit_git_dir(struct repository *repo,
}
/* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */
- if (work_tree_env)
+ if (work_tree_env) {
+ /*
+ * The environment variable overrides "core.worktree". This
+ * also has the consequence that we don't want to flag cases as
+ * bogus where we have both "core.worktree" and "core.bare", so
+ * we have to exlicitly unset the configuration.
+ */
+ FREE_AND_NULL(repo_fmt->work_tree);
set_git_work_tree(repo, work_tree_env);
- 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");
- repo->worktree_config_is_bogus = true;
- }
-
+ } else if (repo_fmt->is_bare > 0) {
/* #18, #26 */
set_git_dir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
- }
- else if (repo_fmt->work_tree) { /* #6, #14 */
- if (is_absolute_path(repo_fmt->work_tree))
+ } 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 {
+ } else {
char *core_worktree;
if (chdir(gitdirenv))
die_errno(_("cannot chdir to '%s'"), gitdirenv);
@@ -1176,15 +1176,14 @@ static const char *setup_explicit_git_dir(struct repository *repo,
set_git_work_tree(repo, core_worktree);
free(core_worktree);
}
- }
- else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
+ } else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
/* #16d */
set_git_dir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
- }
- else /* #2, #10 */
+ } else { /* #2, #10 */
set_git_work_tree(repo, ".");
+ }
/* set_git_work_tree() must have been called by now */
worktree = repo_get_work_tree(repo);
@@ -1768,6 +1767,12 @@ int apply_repository_format(struct repository *repo,
if (verify_repository_format(format, err) < 0)
return -1;
+ if (format->is_bare > 0 && format->work_tree) {
+ /* #22.2, #30 */
+ warning("core.bare and core.worktree do not make sense");
+ repo->worktree_config_is_bogus = true;
+ }
+
if (flags & APPLY_REPOSITORY_FORMAT_HONOR_ENV) {
object_directory = xstrdup_or_null(getenv(DB_ENVIRONMENT));
alternate_object_directories = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH 03/13] setup: unify setup of shallow file
From: Patrick Steinhardt @ 2026-06-30 11:47 UTC (permalink / raw)
To: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-0-13864eb5a032@pks.im>
It is possible to configure an arbitrary "shallow" file via two
mechanisms, and the respective logic to handle these is split across two
locations:
- Via the "GIT_SHALLOW_FILE" environment variable, which is handled in
`setup_git_env_internal()`.
- Via the global "--shallow-file=" command line option, which is
handled in `handle_options()`.
We can rather easily unify this logic by not configuring the shallow
file in `handle_options()`, but instead overwriting the environment
variable. The environment variable itself is then handled inside of
`apply_repository_format()`, which is responsible for configuring a
discovered Git directory.
This new logic is similar in nature to how we handle the other global
options already, all of which end up setting an environment variable.
So for one this gives us more consistency. But more importantly, this
change means that `the_repository` will not contain any relevant state
anymore before we hit `apply_repository_format()` once we're at the end
of this patch series. Consequently, it will become possible for us to
completely discard `the_repository` and populate it anew.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
git.c | 2 +-
setup.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/git.c b/git.c
index 387eabe38c..e5f1811b6b 100644
--- a/git.c
+++ b/git.c
@@ -306,7 +306,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
} else if (!strcmp(cmd, "--shallow-file")) {
(*argv)++;
(*argc)--;
- set_alternate_shallow_file(the_repository, (*argv)[0], 1);
+ setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-C")) {
diff --git a/setup.c b/setup.c
index f54eac5e5a..5e6b959f68 100644
--- a/setup.c
+++ b/setup.c
@@ -1046,7 +1046,6 @@ static void setup_git_env_internal(struct repository *repo,
const char *git_dir)
{
char *git_replace_ref_base;
- const char *shallow_file;
const char *replace_ref_base;
struct set_gitdir_args args = { NULL };
struct strvec to_free = STRVEC_INIT;
@@ -1067,10 +1066,6 @@ static void setup_git_env_internal(struct repository *repo,
: "refs/replace/");
update_ref_namespace(NAMESPACE_REPLACE, git_replace_ref_base);
- shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
- if (shallow_file)
- set_alternate_shallow_file(repo, shallow_file, 0);
-
if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
fetch_if_missing = 0;
}
@@ -1774,8 +1769,13 @@ int apply_repository_format(struct repository *repo,
}
if (flags & APPLY_REPOSITORY_FORMAT_HONOR_ENV) {
+ const char *shallow_file;
+
object_directory = xstrdup_or_null(getenv(DB_ENVIRONMENT));
alternate_object_directories = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
+ shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
+ if (shallow_file)
+ set_alternate_shallow_file(repo, shallow_file, 0);
}
repo->bare_cfg = format->is_bare;
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH 04/13] setup: split up concerns of `setup_git_env_internal()`
From: Patrick Steinhardt @ 2026-06-30 11:47 UTC (permalink / raw)
To: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-0-13864eb5a032@pks.im>
The function `setup_git_env_internal()` does two completely unrelated
things:
- It configures the repository's gitdir and propagates environment
variables into it.
- It configures a couple of global parameters via environment
variables.
The function is called when we initialize the repository's path, but
it's also called via `chdir_notify_register()` whenever we change the
current working directory. While we indeed have to reconfigure the
gitdir in case it's a relative path, it doesn't make sense to reapply
the global environment variables.
Split up concerns of this function along the above delineation. Handling
of the global environment variables is moved into `init_git()`, as they
can be considered part of our setup procedure.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
common-init.c | 20 ++++++++++++++++
setup.c | 73 +++++++++++++++++++++++------------------------------------
2 files changed, 48 insertions(+), 45 deletions(-)
diff --git a/common-init.c b/common-init.c
index 5cc73f058c..d26c9c1f20 100644
--- a/common-init.c
+++ b/common-init.c
@@ -5,7 +5,10 @@
#include "exec-cmd.h"
#include "gettext.h"
#include "attr.h"
+#include "odb.h"
+#include "parse.h"
#include "repository.h"
+#include "replace-object.h"
#include "setup.h"
#include "strbuf.h"
#include "trace2.h"
@@ -31,6 +34,22 @@ static void restore_sigpipe_to_default(void)
signal(SIGPIPE, SIG_DFL);
}
+static void setup_environment(void)
+{
+ char *git_replace_ref_base;
+ const char *replace_ref_base;
+
+ if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
+ disable_replace_refs();
+ replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);
+ git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base
+ : "refs/replace/");
+ update_ref_namespace(NAMESPACE_REPLACE, git_replace_ref_base);
+
+ if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
+ fetch_if_missing = 0;
+}
+
void init_git(const char **argv)
{
struct strbuf tmp = STRBUF_INIT;
@@ -51,6 +70,7 @@ void init_git(const char **argv)
git_setup_gettext();
initialize_repository(the_repository);
+ setup_environment();
attr_start();
diff --git a/setup.c b/setup.c
index 5e6b959f68..dd8514b822 100644
--- a/setup.c
+++ b/setup.c
@@ -10,7 +10,6 @@
#include "object-file.h"
#include "object-name.h"
#include "refs.h"
-#include "replace-object.h"
#include "repository.h"
#include "config.h"
#include "dir.h"
@@ -1042,38 +1041,19 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
return error_code ? NULL : path;
}
-static void setup_git_env_internal(struct repository *repo,
- const char *git_dir)
+static void apply_gitdir_and_environment(struct repository *repo, const char *path)
{
- char *git_replace_ref_base;
- const char *replace_ref_base;
- struct set_gitdir_args args = { NULL };
struct strvec to_free = STRVEC_INIT;
+ struct set_gitdir_args args = {
+ .commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT),
+ .graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT),
+ .index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT),
+ .disable_ref_updates = !!getenv(GIT_QUARANTINE_ENVIRONMENT),
+ };
- args.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);
- args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
- args.index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT);
- if (getenv(GIT_QUARANTINE_ENVIRONMENT))
- args.disable_ref_updates = true;
+ repo_set_gitdir(repo, path, &args);
- repo_set_gitdir(repo, git_dir, &args);
strvec_clear(&to_free);
-
- if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
- disable_replace_refs();
- replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);
- git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base
- : "refs/replace/");
- update_ref_namespace(NAMESPACE_REPLACE, git_replace_ref_base);
-
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
- fetch_if_missing = 0;
-}
-
-static void set_git_dir_1(struct repository *repo, const char *path)
-{
- xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
- setup_git_env_internal(repo, path);
}
static void update_relative_gitdir(const char *name UNUSED,
@@ -1087,11 +1067,12 @@ static void update_relative_gitdir(const char *name UNUSED,
trace_printf_key(&trace_setup_key,
"setup: move $GIT_DIR to '%s'",
path);
- set_git_dir_1(repo, path);
+ apply_gitdir_and_environment(repo, path);
+ xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
free(path);
}
-static void set_git_dir(struct repository *repo, const char *path, int make_realpath)
+static void apply_and_export_relative_gitdir(struct repository *repo, const char *path, int make_realpath)
{
struct strbuf realpath = STRBUF_INIT;
@@ -1100,7 +1081,9 @@ static void set_git_dir(struct repository *repo, const char *path, int make_real
path = realpath.buf;
}
- set_git_dir_1(repo, path);
+ apply_gitdir_and_environment(repo, path);
+ xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
+
if (!is_absolute_path(path))
chdir_notify_register(NULL, update_relative_gitdir, repo);
@@ -1153,7 +1136,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
set_git_work_tree(repo, work_tree_env);
} else if (repo_fmt->is_bare > 0) {
/* #18, #26 */
- set_git_dir(repo, gitdirenv, 0);
+ apply_and_export_relative_gitdir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
} else if (repo_fmt->work_tree) { /* #6, #14 */
@@ -1173,7 +1156,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
}
} else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
/* #16d */
- set_git_dir(repo, gitdirenv, 0);
+ apply_and_export_relative_gitdir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
} else { /* #2, #10 */
@@ -1185,14 +1168,14 @@ static const char *setup_explicit_git_dir(struct repository *repo,
/* both repo_get_work_tree() and cwd are already normalized */
if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
- set_git_dir(repo, gitdirenv, 0);
+ apply_and_export_relative_gitdir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
}
offset = dir_inside_of(cwd->buf, worktree);
if (offset >= 0) { /* cwd inside worktree? */
- set_git_dir(repo, gitdirenv, 1);
+ apply_and_export_relative_gitdir(repo, gitdirenv, 1);
if (chdir(worktree))
die_errno(_("cannot chdir to '%s'"), worktree);
strbuf_addch(cwd, '/');
@@ -1201,7 +1184,7 @@ static const char *setup_explicit_git_dir(struct repository *repo,
}
/* cwd outside worktree */
- set_git_dir(repo, gitdirenv, 0);
+ apply_and_export_relative_gitdir(repo, gitdirenv, 0);
free(gitfile);
return NULL;
}
@@ -1231,7 +1214,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 (repo_fmt->is_bare > 0) {
- set_git_dir(repo, gitdir, (offset != cwd->len));
+ apply_and_export_relative_gitdir(repo, gitdir, (offset != cwd->len));
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
return NULL;
@@ -1240,7 +1223,7 @@ static const char *setup_discovered_git_dir(struct repository *repo,
/* #0, #1, #5, #8, #9, #12, #13 */
set_git_work_tree(repo, ".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
- set_git_dir(repo, gitdir, 0);
+ apply_and_export_relative_gitdir(repo, gitdir, 0);
if (offset >= cwd->len)
return NULL;
@@ -1280,10 +1263,10 @@ static const char *setup_bare_git_dir(struct repository *repo,
die_errno(_("cannot come back to cwd"));
root_len = offset_1st_component(cwd->buf);
strbuf_setlen(cwd, offset > root_len ? offset : root_len);
- set_git_dir(repo, cwd->buf, 0);
+ apply_and_export_relative_gitdir(repo, cwd->buf, 0);
}
else
- set_git_dir(repo, ".", 0);
+ apply_and_export_relative_gitdir(repo, ".", 0);
return NULL;
}
@@ -1878,7 +1861,7 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
struct repository_format fmt = REPOSITORY_FORMAT_INIT;
struct strbuf err = STRBUF_INIT;
- set_git_dir(repo, ".", 0);
+ apply_and_export_relative_gitdir(repo, ".", 0);
read_and_verify_repository_format(&fmt, ".", NULL);
if (apply_repository_format(repo, &fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
die("%s", err.buf);
@@ -2022,7 +2005,7 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
startup_info->have_repository = 1;
/*
- * Not all paths through the setup code will call 'set_git_dir()' (which
+ * Not all paths through the setup code will call 'apply_and_export_relative_gitdir()' (which
* directly sets up the environment) so in order to guarantee that the
* environment is in a consistent state after setup, explicitly setup
* the environment if we have a repository.
@@ -2040,7 +2023,7 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
if (!gitdir)
gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
- setup_git_env_internal(repo, gitdir);
+ apply_gitdir_and_environment(repo, gitdir);
}
if (startup_info->have_repository) {
@@ -2825,12 +2808,12 @@ int init_db(struct repository *repo,
if (!exist_ok && !stat(real_git_dir, &st))
die(_("%s already exists"), real_git_dir);
- set_git_dir(repo, real_git_dir, 1);
+ apply_and_export_relative_gitdir(repo, real_git_dir, 1);
git_dir = repo_get_git_dir(repo);
separate_git_dir(git_dir, original_git_dir);
}
else {
- set_git_dir(repo, git_dir, 1);
+ apply_and_export_relative_gitdir(repo, git_dir, 1);
git_dir = repo_get_git_dir(repo);
}
startup_info->have_repository = 1;
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH 05/13] setup: introduce explicit repository discovery
From: Patrick Steinhardt @ 2026-06-30 11:47 UTC (permalink / raw)
To: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-0-13864eb5a032@pks.im>
When setting up the global repository we intermix repository discovery
and repository configuration: we repeatedly call `set_git_work_tree()`
and `apply_and_export_relative_gitdir()` until we're happy with the
result. The result of this is then a partially-configured repository
that we use for further setup.
This process is quite hard to follow, as it's never quite clear which
parts of the repository have been configured already and which haven't.
Furthermore, it means that the repository configuration is distributed
across many different places instead of having it neatly contained in a
single location. Ultimately, this is the reason that we cannot use a
central function like `repo_init()`.
Refactor the logic so that we stop partially-configuring a repository
and instead populate a new `struct repo_discovery`. This allow us to
essentially split repository setup into two phases:
- The first phase only figures out parameters required to configure
the repository.
- The second phase then takes these parameters and applies them to the
repository.
Like this, we'll never end up with a partially-configured repository and
can eventually extend `repo_init()` to handle the full initialization
for us.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 155 ++++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 98 insertions(+), 57 deletions(-)
diff --git a/setup.c b/setup.c
index dd8514b822..06768de23f 100644
--- a/setup.c
+++ b/setup.c
@@ -1090,14 +1090,47 @@ static void apply_and_export_relative_gitdir(struct repository *repo, const char
strbuf_release(&realpath);
}
-static const char *setup_explicit_git_dir(struct repository *repo,
- const char *gitdirenv,
- struct strbuf *cwd,
- struct repository_format *repo_fmt,
- int *nongit_ok)
+struct repo_discovery {
+ char *gitdir;
+ char *worktree;
+};
+
+#define REPO_DISCOVERY_INIT { 0 }
+
+static void repo_discovery_release(struct repo_discovery *r)
+{
+ free(r->gitdir);
+ free(r->worktree);
+}
+
+static void repo_discovery_set_gitdir(struct repo_discovery *r,
+ const char *gitdir,
+ int make_realpath)
+{
+ free(r->gitdir);
+ if (make_realpath) {
+ struct strbuf realpath = STRBUF_INIT;
+ strbuf_realpath(&realpath, gitdir, 1);
+ r->gitdir = strbuf_detach(&realpath, NULL);
+ } else {
+ r->gitdir = xstrdup(gitdir);
+ }
+}
+
+static void repo_discovery_set_worktree(struct repo_discovery *r,
+ const char *worktree)
+{
+ free(r->worktree);
+ r->worktree = real_pathdup(worktree, 1);
+}
+
+static const char *repo_discover_explicit_gitdir(struct repo_discovery *discovery,
+ const char *gitdirenv,
+ struct strbuf *cwd,
+ struct repository_format *repo_fmt,
+ int *nongit_ok)
{
const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
- const char *worktree;
char *gitfile;
int offset;
@@ -1133,15 +1166,15 @@ static const char *setup_explicit_git_dir(struct repository *repo,
* we have to exlicitly unset the configuration.
*/
FREE_AND_NULL(repo_fmt->work_tree);
- set_git_work_tree(repo, work_tree_env);
+ repo_discovery_set_worktree(discovery, work_tree_env);
} else if (repo_fmt->is_bare > 0) {
/* #18, #26 */
- apply_and_export_relative_gitdir(repo, gitdirenv, 0);
+ repo_discovery_set_gitdir(discovery, gitdirenv, 0);
free(gitfile);
return NULL;
} 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);
+ repo_discovery_set_worktree(discovery, repo_fmt->work_tree);
} else {
char *core_worktree;
if (chdir(gitdirenv))
@@ -1151,49 +1184,46 @@ static const char *setup_explicit_git_dir(struct repository *repo,
core_worktree = xgetcwd();
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
- set_git_work_tree(repo, core_worktree);
+ repo_discovery_set_worktree(discovery, core_worktree);
free(core_worktree);
}
} else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
/* #16d */
- apply_and_export_relative_gitdir(repo, gitdirenv, 0);
+ repo_discovery_set_gitdir(discovery, gitdirenv, 0);
free(gitfile);
return NULL;
} else { /* #2, #10 */
- set_git_work_tree(repo, ".");
+ repo_discovery_set_worktree(discovery, ".");
}
- /* set_git_work_tree() must have been called by now */
- worktree = repo_get_work_tree(repo);
-
- /* both repo_get_work_tree() and cwd are already normalized */
- if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
- apply_and_export_relative_gitdir(repo, gitdirenv, 0);
+ /* both the worktree and cwd are already normalized */
+ if (!strcmp(cwd->buf, discovery->worktree)) { /* cwd == worktree */
+ repo_discovery_set_gitdir(discovery, gitdirenv, 0);
free(gitfile);
return NULL;
}
- offset = dir_inside_of(cwd->buf, worktree);
- if (offset >= 0) { /* cwd inside worktree? */
- apply_and_export_relative_gitdir(repo, gitdirenv, 1);
- if (chdir(worktree))
- die_errno(_("cannot chdir to '%s'"), worktree);
+ offset = dir_inside_of(cwd->buf, discovery->worktree);
+ if (offset >= 0) { /* cwd inside discovery->worktree? */
+ repo_discovery_set_gitdir(discovery, gitdirenv, 1);
+ if (chdir(discovery->worktree))
+ die_errno(_("cannot chdir to '%s'"), discovery->worktree);
strbuf_addch(cwd, '/');
free(gitfile);
return cwd->buf + offset;
}
/* cwd outside worktree */
- apply_and_export_relative_gitdir(repo, gitdirenv, 0);
+ repo_discovery_set_gitdir(discovery, gitdirenv, 0);
free(gitfile);
return NULL;
}
-static const char *setup_discovered_git_dir(struct repository *repo,
- const char *gitdir,
- struct strbuf *cwd, int offset,
- struct repository_format *repo_fmt,
- int *nongit_ok)
+static const char *repo_discover_implicit_gitdir(struct repo_discovery *discovery,
+ const char *gitdir,
+ struct strbuf *cwd, int offset,
+ struct repository_format *repo_fmt,
+ int *nongit_ok)
{
if (read_and_verify_repository_format(repo_fmt, gitdir, nongit_ok))
return NULL;
@@ -1207,23 +1237,24 @@ static const char *setup_discovered_git_dir(struct repository *repo,
gitdir = to_free = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
- ret = setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
+ ret = repo_discover_explicit_gitdir(discovery, gitdir, cwd,
+ repo_fmt, nongit_ok);
free(to_free);
return ret;
}
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
if (repo_fmt->is_bare > 0) {
- apply_and_export_relative_gitdir(repo, gitdir, (offset != cwd->len));
+ repo_discovery_set_gitdir(discovery, gitdir, (offset != cwd->len));
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
return NULL;
}
/* #0, #1, #5, #8, #9, #12, #13 */
- set_git_work_tree(repo, ".");
+ repo_discovery_set_worktree(discovery, ".");
if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
- apply_and_export_relative_gitdir(repo, gitdir, 0);
+ repo_discovery_set_gitdir(discovery, gitdir, 0);
if (offset >= cwd->len)
return NULL;
@@ -1236,10 +1267,10 @@ static const char *setup_discovered_git_dir(struct repository *repo,
}
/* #16.1, #17.1, #20.1, #21.1, #22.1 (see t1510) */
-static const char *setup_bare_git_dir(struct repository *repo,
- struct strbuf *cwd, int offset,
- struct repository_format *repo_fmt,
- int *nongit_ok)
+static const char *repo_discover_bare_gitdir(struct repo_discovery *discovery,
+ struct strbuf *cwd, int offset,
+ struct repository_format *repo_fmt,
+ int *nongit_ok)
{
int root_len;
@@ -1255,7 +1286,8 @@ static const char *setup_bare_git_dir(struct repository *repo,
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
if (chdir(cwd->buf))
die_errno(_("cannot come back to cwd"));
- return setup_explicit_git_dir(repo, gitdir, cwd, repo_fmt, nongit_ok);
+ return repo_discover_explicit_gitdir(discovery, gitdir, cwd,
+ repo_fmt, nongit_ok);
}
if (offset != cwd->len) {
@@ -1263,10 +1295,10 @@ static const char *setup_bare_git_dir(struct repository *repo,
die_errno(_("cannot come back to cwd"));
root_len = offset_1st_component(cwd->buf);
strbuf_setlen(cwd, offset > root_len ? offset : root_len);
- apply_and_export_relative_gitdir(repo, cwd->buf, 0);
+ repo_discovery_set_gitdir(discovery, cwd->buf, 0);
}
else
- apply_and_export_relative_gitdir(repo, ".", 0);
+ repo_discovery_set_gitdir(discovery, ".", 0);
return NULL;
}
@@ -1525,10 +1557,10 @@ static int is_implicit_bare_repo(const char *path)
* the discovered .git/ directory, if any. If `gitdir` is not absolute, it
* is relative to `dir` (i.e. *not* necessarily the cwd).
*/
-static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
- struct strbuf *gitdir,
- struct strbuf *report,
- int die_on_error)
+static enum discovery_result repo_discovery_find_dir(struct strbuf *dir,
+ struct strbuf *gitdir,
+ struct strbuf *report,
+ int die_on_error)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
@@ -1695,7 +1727,7 @@ enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
return GIT_DIR_CWD_FAILURE;
cwd_len = dir.len;
- result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);
+ result = repo_discovery_find_dir(&dir, gitdir, NULL, 0);
if (result <= 0) {
strbuf_release(&dir);
return result;
@@ -1902,6 +1934,7 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
{
static struct strbuf cwd = STRBUF_INIT;
struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT;
+ struct repo_discovery discovery = REPO_DISCOVERY_INIT;
const char *prefix = NULL;
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
@@ -1926,20 +1959,22 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
die_errno(_("Unable to read current working directory"));
strbuf_addbuf(&dir, &cwd);
- switch (setup_git_directory_gently_1(&dir, &gitdir, &report, 1)) {
+ switch (repo_discovery_find_dir(&dir, &gitdir, &report, 1)) {
case GIT_DIR_EXPLICIT:
- prefix = setup_explicit_git_dir(repo, gitdir.buf, &cwd, &repo_fmt, nongit_ok);
+ prefix = repo_discover_explicit_gitdir(&discovery, gitdir.buf, &cwd,
+ &repo_fmt, nongit_ok);
break;
case GIT_DIR_DISCOVERED:
if (dir.len < cwd.len && chdir(dir.buf))
die(_("cannot change to '%s'"), dir.buf);
- prefix = setup_discovered_git_dir(repo, gitdir.buf, &cwd, dir.len,
- &repo_fmt, nongit_ok);
+ prefix = repo_discover_implicit_gitdir(&discovery, gitdir.buf, &cwd, dir.len,
+ &repo_fmt, nongit_ok);
break;
case GIT_DIR_BARE:
if (dir.len < cwd.len && chdir(dir.buf))
die(_("cannot change to '%s'"), dir.buf);
- prefix = setup_bare_git_dir(repo, &cwd, dir.len, &repo_fmt, nongit_ok);
+ prefix = repo_discover_bare_gitdir(&discovery, &cwd, dir.len,
+ &repo_fmt, nongit_ok);
break;
case GIT_DIR_HIT_CEILING:
if (!nongit_ok)
@@ -1980,13 +2015,13 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
case GIT_DIR_CWD_FAILURE:
case GIT_DIR_INVALID_FORMAT:
/*
- * As a safeguard against setup_git_directory_gently_1 returning
+ * As a safeguard against repo_discovery_find_dir returning
* these values, fallthrough to BUG. Otherwise it is possible to
* set startup_info->have_repository to 1 when we did nothing to
* find a repository.
*/
default:
- BUG("unhandled setup_git_directory_gently_1() result");
+ BUG("unhandled repo_discovery_find_dir() result");
}
/*
@@ -2005,10 +2040,10 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
startup_info->have_repository = 1;
/*
- * Not all paths through the setup code will call 'apply_and_export_relative_gitdir()' (which
- * directly sets up the environment) so in order to guarantee that the
- * environment is in a consistent state after setup, explicitly setup
- * the environment if we have a repository.
+ * Not all paths through the setup code will have recorded a gitdir
+ * above, so in order to guarantee that the environment is in a
+ * consistent state after setup, explicitly set up the gitdir and
+ * environment if we have a repository.
*
* NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some
* code paths so we also need to explicitly setup the environment if
@@ -2019,7 +2054,12 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
startup_info->have_repository ||
/* GIT_DIR_EXPLICIT */
getenv(GIT_DIR_ENVIRONMENT)) {
- if (!repo->gitdir) {
+ if (discovery.worktree)
+ set_git_work_tree(repo, discovery.worktree);
+
+ if (discovery.gitdir) {
+ apply_and_export_relative_gitdir(repo, discovery.gitdir, 0);
+ } else {
const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
if (!gitdir)
gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
@@ -2074,6 +2114,7 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
setup_original_cwd(repo);
+ repo_discovery_release(&discovery);
strbuf_release(&dir);
strbuf_release(&gitdir);
strbuf_release(&report);
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
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;
as well as URLs for NNTP newsgroup(s).