Git development
 help / color / mirror / Atom feed
* [PATCH] submodule absorbgitdirs tests: use test_* helper functions
From: Bryan B. Lima @ 2026-06-30  2:02 UTC (permalink / raw)
  To: git
  Cc: bblima, gustavoscorrea, Ævar Arnfjörð Bjarmason,
	Junio C Hamano

Use modern helper functions from test-lib-functions.sh to provide nice error messages.

Signed-off-by: Bryan B. Lima <bblima@usp.br>
Co-authored-by: Gustavo S. Correa <gustavoscorrea@usp.br>
Signed-off-by: Gustavo S. Correa <gustavoscorrea@usp.br>
---
 t/t7412-submodule-absorbgitdirs.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index 0490499573..bd1c684480 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -34,8 +34,8 @@ test_expect_success 'absorb the git dir' '
 	git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
 	git fsck &&
-	test -f sub1/.git &&
-	test -d .git/modules/sub1 &&
+	test_path_is_file sub1/.git &&
+	test_path_is_dir .git/modules/sub1 &&
 	git status >actual.1 &&
 	git -C sub1 rev-parse HEAD >actual.2 &&
 	test_cmp expect.1 actual.1 &&
@@ -47,9 +47,9 @@ test_expect_success 'absorbing does not fail for deinitialized submodules' '
 	git submodule deinit --all &&
 	git submodule absorbgitdirs 2>err &&
 	test_must_be_empty err &&
-	test -d .git/modules/sub1 &&
-	test -d sub1 &&
-	! test -e sub1/.git
+	test_path_is_dir .git/modules/sub1 &&
+	test_path_is_dir sub1 &&
+	test_path_is_missing sub1/.git
 '
 
 test_expect_success 'setup nested submodule' '
@@ -72,8 +72,8 @@ test_expect_success 'absorb the git dir in a nested submodule' '
 	EOF
 	git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
-	test -f sub1/nested/.git &&
-	test -d .git/modules/sub1/modules/nested &&
+	test_path_is_file sub1/nested/.git &&
+	test_path_is_dir .git/modules/sub1/modules/nested &&
 	git status >actual.1 &&
 	git -C sub1/nested rev-parse HEAD >actual.2 &&
 	test_cmp expect.1 actual.1 &&
@@ -109,9 +109,9 @@ test_expect_success 'absorb the git dir in a nested submodule' '
 	EOF
 	git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
-	test -f sub1/.git &&
-	test -f sub1/nested/.git &&
-	test -d .git/modules/sub1/modules/nested &&
+	test_path_is_file sub1/.git &&
+	test_path_is_file sub1/nested/.git &&
+	test_path_is_dir .git/modules/sub1/modules/nested &&
 	git status >actual.1 &&
 	git -C sub1/nested rev-parse HEAD >actual.2 &&
 	test_cmp expect.1 actual.1 &&
@@ -155,7 +155,7 @@ test_expect_success 'absorbing the git dir fails for incomplete submodules' '
 	test_must_fail git submodule absorbgitdirs 2>actual &&
 	test_cmp expect actual &&
 	git -C sub2 fsck &&
-	test -d sub2/.git &&
+	test_path_is_dir sub2/.git &&
 	git status >actual &&
 	git -C sub2 rev-parse HEAD >actual.2 &&
 	test_cmp expect.1 actual.1 &&

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
2.54.0


^ permalink raw reply related

* Re: [PATCH v6 2/3] revision: add peek functions for lookahead
From: Pablo Sabater @ 2026-06-29 23:29 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Kristofer Karlsson, git, ayu.chandekar, chandrapratap3519,
	christian.couder, jltobler, karthik.188, peff, phillip.wood,
	siddharthasthana31
In-Reply-To: <xmqqechpt3i9.fsf@gitster.g>

El lun, 29 jun 2026 a las 23:56, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Kristofer Karlsson <krka@spotify.com> writes:
>
> > The solution is to skip peeking entirely and instead call
> > get_revision_internal() to populate a small lookahead buffer -
> > it only needs two slots.
> >
> >     struct git_graph {
> >         // ...
> >         struct commit *lookahead[2];
> >         int lookahead_nr;
> >     }
> >
> >     while (revs->graph->lookahead_nr < 2) {
> >         struct commit *next = get_revision_internal(revs);
> >         if (!next)
> >             break;
> >         graph_push_lookahead(revs->graph, next);
> >     }
> >
> > After prototyping this locally, the three test_expect_failure
> > cases in t4218 went away (though I had to do some minor tweaks
> > to ensure it become fully deterministic by ticking the commit
> > timestamps.
> >
> > One subtlety worth mentioning: get_revision_internal() sets
> > SHOWN on commits, so lookahead commits are marked SHOWN before
> > graph_update() processes them. This makes graph_is_interesting()
> > think they are already displayed. The fix is a small check in
> > graph_is_interesting() that recognizes commits in the lookahead
> > buffer as interesting regardless of their SHOWN flag.
> >
> >     for (i = 0; i < graph->lookahead_nr; i++)
> >         if (graph->lookahead[i] == commit)
> >             return 1;
> >     // other checks after this ...
> >
> > This approach ultimately removes the need for
> > revision_peek_next_commit() and revision_has_commits_after()
> > entirely - the graph code no longer needs to peek
> > at rev_info internals.
>
> Sorry I lost track, but I think the message I am responding to is
> one of the latest messages in the thread.  Whose court is the
> ball in right now?

Hi!

It's still mine :).
Sorry I haven't worked on this patch these last days, I tested this
morning what Kristofer told me to do, but I didn't finish it. I think
I'll have it by tomorrow.

So, the next step is a reroll from me.

>
> Thanks.
>

Thanks for the patience,
Pablo.

^ permalink raw reply

* Re: [PATCH 2/2] odb: introduce `odb_prepare()`
From: Junio C Hamano @ 2026-06-29 21:58 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Toon Claes, git
In-Reply-To: <akIN0CxVxhaHnvJ0@pks.im>

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.

Thanks.

^ permalink raw reply

* Re: [PATCH v6 2/3] revision: add peek functions for lookahead
From: Junio C Hamano @ 2026-06-29 21:56 UTC (permalink / raw)
  To: Kristofer Karlsson
  Cc: Pablo Sabater, git, ayu.chandekar, chandrapratap3519,
	christian.couder, jltobler, karthik.188, peff, phillip.wood,
	siddharthasthana31, Kristofer Karlsson
In-Reply-To: <CAL71e4OQ_kGb+UwHgikHG236-8BVtc7P9OdpV4i4UzYRCoPczw@mail.gmail.com>

Kristofer Karlsson <krka@spotify.com> writes:

> The solution is to skip peeking entirely and instead call
> get_revision_internal() to populate a small lookahead buffer -
> it only needs two slots.
>
>     struct git_graph {
>         // ...
>         struct commit *lookahead[2];
>         int lookahead_nr;
>     }
>
>     while (revs->graph->lookahead_nr < 2) {
>         struct commit *next = get_revision_internal(revs);
>         if (!next)
>             break;
>         graph_push_lookahead(revs->graph, next);
>     }
>
> After prototyping this locally, the three test_expect_failure
> cases in t4218 went away (though I had to do some minor tweaks
> to ensure it become fully deterministic by ticking the commit
> timestamps.
>
> One subtlety worth mentioning: get_revision_internal() sets
> SHOWN on commits, so lookahead commits are marked SHOWN before
> graph_update() processes them. This makes graph_is_interesting()
> think they are already displayed. The fix is a small check in
> graph_is_interesting() that recognizes commits in the lookahead
> buffer as interesting regardless of their SHOWN flag.
>
>     for (i = 0; i < graph->lookahead_nr; i++)
>         if (graph->lookahead[i] == commit)
>             return 1;
>     // other checks after this ...
>
> This approach ultimately removes the need for
> revision_peek_next_commit() and revision_has_commits_after()
> entirely - the graph code no longer needs to peek
> at rev_info internals.

Sorry I lost track, but I think the message I am responding to is
one of the latest messages in the thread.  Whose court is the
ball in right now?

Thanks.


^ permalink raw reply

* Re: [PATCH v3 0/2] completion: hide dotfiles for selected path completion
From: Junio C Hamano @ 2026-06-29 21:52 UTC (permalink / raw)
  To: Zakariyah Ali via GitGitGadget; +Cc: git, Zakariyah Ali
In-Reply-To: <pull.2311.v3.git.git.1781978156.gitgitgadget@gmail.com>

"Zakariyah Ali via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The completion helper for index paths uses git ls-files rather than shell
> filename completion. As a result, leading-dot paths such as a tracked
> .gitignore were offered even when the user had not started the path with ..

Will we see a hopefully small and final update (v4) to conclude this
topic anytime soom?  No rush, but just wondering...

^ permalink raw reply

* Re: [PATCH] meson: wire up USE_NSEC build knob
From: Junio C Hamano @ 2026-06-29 21:38 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: Jeff King, D. Ben Knoble, git, brian m . carlson, Ramsay Jones
In-Reply-To: <akIL6oJgUv8J8SB2@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> Hm. That makes me wonder whether it is the completely wrong approach to
> make this a build option then. If it works on some systems and only on
> some filesystems, then a build option is just too coarse-grained. A
> distro wouldn't really be able to ever enable the option, unless it knew
> that repositories will only ever exist on a filesystem that works. Which
> I guess is an assumption that no distro can make.

Yes and no.  Build options are not only for distro packagers who aim
for widest audience.  If you know the target box with its
filesystems happen to be OK with the option, flipping the switch to
turn it on is totally a sensible thing to do.  It is true that this
one is much less flexible (because the situation you must be in to
enable it is much narrower).

> So instead, I wonder whether we should treat this the same as for
> example "core.ignoreCase", where we only use nanosecond resolution when
> opted in by the user. Ideally, if we had a way to detect brokenness, we
> could even make git-init(1) set it automatically.

I like the line of thought.

The ignoreCase MUST be set for correct operation if your filesystem
is incapable of case sensitive operation, and if your filesystem is
case sensitive, building with ignoreCase set may limit what you can
do, and give you some performace hits, but also the code can make
assumptions like "ah, we saw 'Makefile' in this directory so there
wouldn't be makefile at the same time" and misbehave).  In other
words, it is not something you set by choice.

On the other hand, nanosecond timestamp does not have to be enabled
even if your filesystem and operating system is capable of keeping
the timestamp always down to nanosecond resolution, even though it
has to be disabled if your filesystem and operating system randomly
loses precision due to buffer cache getting flushed.  So there is a
slight difference between it and the ignoreCase situation.

^ permalink raw reply

* [ANNOUNCE] Git Merge 2026 CFP deadline extended
From: Taylor Blau @ 2026-06-29 21:36 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Scott Chacon, Emily Shaffer, Patrick Steinhardt

[GMail web client, please excuse any formatting errors.]

Hi,

In [1], the dates and deadlines for Git Merge 2026 were announced, and
the CFP deadline was set for June 30, 2026.

**The deadline has been extended to July 14, 2026 at 11:59 PM (UTC-7).**

The primary reason for extending the deadline is to collect more
submissions from folks who have not yet submitted a talk, but indicated
that they plan on doing so. (A number of folks I'm thinking of are
currently out of office, and I want to make sure that they have a chance
to submit their talk proposals.)

If you have any questions about the talk submission process, or about
Git Merge in general, please feel free to reach out to me on- or
off-list. See you at Git Merge!

Thanks,
Taylor

[1]: https://lore.kernel.org/git/agJVZhTbA%2FhFUKG%2F@nand.local/

^ permalink raw reply

* Re: [PATCH v2 5/6] t: convert grep assertions to test_grep
From: Junio C Hamano @ 2026-06-29 21:21 UTC (permalink / raw)
  To: Michael Montalbo via GitGitGadget, SZEDER Gábor
  Cc: git, D. Ben Knoble, Eric Sunshine, Michael Montalbo
In-Reply-To: <xmqq4iin4e1i.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Ah, of course.  Michael sidesteps this mechanism by not using
> "test_grep !", with
>
>        ! grep dirty file3 && # lint-ok: file may not exist after --quit
>
> and if we realize that "may not exist" is actually "never exists",
> then your other patch from 5 years ago would become the most
> sensible fix for this line.
>
> It may not be a bad idea to go through "# lint-ok:" introduced by
> Michael's series with finer toothed comb (there are only a handful
> of them) and see if there are similar "look, the file we are
> grepping in never exists with correctly running Git" gotchas.

In any case, I think SZEDER's fix to stop grepping in the file but
instead insisting on its absense does make sense and it is now in
'next'.  So perhaps this topic can have a small and final reroll v3
that omits change to this particular line (and possibly fix other
lines that punts with "# lint-ok" if needed) and we can declare
victory after that?

Thanks, all.

^ permalink raw reply

* Re: [PATCH v2] prio-queue: use cascade-down for faster extract-min
From: Junio C Hamano @ 2026-06-29 21:16 UTC (permalink / raw)
  To: Kristofer Karlsson
  Cc: René Scharfe, Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <CAL71e4MYNiScZjTwkApjDAjRh2LM0_SP59h5HCTywV-Pua03tw@mail.gmail.com>

Kristofer Karlsson <krka@spotify.com> writes:

> Now I am thinking it would be easier to reason about this if the other
> patch lands first, since the cascade change becomes simpler to evaluate
> when replace is already gone and only the unfused paths remain.

Sorry, I should have noticed this message and responded earlier.
Let's make sure that the "other patch" is ready then and merge it
down.

Since June 8th, nothing seemed to have happened to the thread for
the "other patch".

  https://lore.kernel.org/git/pull.2140.v4.git.1780945851.gitgitgadget@gmail.com/

Is everybody happy with these two patches?

Thanks.

^ permalink raw reply

* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-06-29 21:13 UTC (permalink / raw)
  To: phillip.wood; +Cc: Patrick Steinhardt, Harald Nordgren via GitGitGadget, git
In-Reply-To: <dff9378a-267f-4b49-bee4-615b4bf75abb@gmail.com>

> You've trimmed the line where I said
>
>  >> Possibly with a comment before each message saying where it came
>  >> from.

Sorry about that! Can you show me the example of what it would look
like? It's much easier for me to reason about it then.


Harald

^ permalink raw reply

* Re: [PATCH v2 4/5] builtin/refs: add "create" subcommand
From: Junio C Hamano @ 2026-06-29 20:58 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260617-pks-refs-writing-subcommands-v2-4-07f3d18336f9@pks.im>

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.



^ permalink raw reply

* Re: [PATCH v4 0/3] environment: migrate 'trust_executable_bit' into 'repo_config_values'
From: Junio C Hamano @ 2026-06-29 20:55 UTC (permalink / raw)
  To: Tian Yuchen
  Cc: git, ps, Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260619162105.648495-1-cat@malon.dev>

Tian Yuchen <cat@malon.dev> writes:

> The 'core.filemode' (stored as 'trust_executable_bit') configuration
> act as a core filesystem capability flag.

This unfortunately hasn't heard any responses since June 19th.  Are
there remaining issues with it?  Or do people fundamentally have
objections against this change?  Or things are too busy in general
that there are more patches than there are folks willing to review
them?

^ permalink raw reply

* Re: [PATCH v2 0/5] builtin/refs: add ability to write references
From: Junio C Hamano @ 2026-06-29 20:52 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260617-pks-refs-writing-subcommands-v2-0-07f3d18336f9@pks.im>

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?

^ permalink raw reply

* Re: [PATCH 3/6] odb: add `source` field to struct object_info_source
From: Junio C Hamano @ 2026-06-29 20:47 UTC (permalink / raw)
  To: Justin Tobler; +Cc: Patrick Steinhardt, git
In-Reply-To: <akKtc4ybxFRVJmNv@denethor>

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.

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?

Perhaps it is the naming?

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).

^ permalink raw reply

* Re: [PATCH v4 0/3] Reuse --contains traversal results
From: Junio C Hamano @ 2026-06-29 20:40 UTC (permalink / raw)
  To: Tamir Duberstein
  Cc: git, Jeff King, Karthik Nayak, Victoria Dye, Derrick Stolee,
	Elijah Newren, Kristofer Karlsson
In-Reply-To: <20260612-ref-filter-memoized-contains-v4-0-5ed39fd001dd@gmail.com>

Tamir Duberstein <tamird@gmail.com> writes:

> git tag uses a memoized traversal for --contains, while git branch
> and git for-each-ref repeat a reachability walk for each ref. Reuse
> the memoized traversal when generation numbers can bound the walk.
>
> The first patch makes the memoized traversal reject cyclic replacement
> histories. The last makes the non-memoized path report reachability
> errors.

This unfortunately hasn't heard any responses since June 12th.  Are
there remaining issues with it?  Or do people fundamentally have
objections against this change?  Or things are too busy in general
that there are more patches than there are folks willing to review
them?


^ permalink raw reply

* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
From: Jeff King @ 2026-06-29 20:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Montalbo, Patrick Steinhardt, git
In-Reply-To: <xmqqh5mlz9uw.fsf@gitster.g>

On Mon, Jun 29, 2026 at 07:42:31AM -0700, Junio C Hamano wrote:

> > Ah, yeah. It should work either way, but it is slightly confusing for it
> > to be non-bare. I'll wait to re-send (though if nothing else comes up,
> > it may be simpler for you to just amend on your side).
> 
> OK.  It seems both Patrick and you are in favor of using only [1/3]
> & [2/3] but dropping [3/3]?  If that is the concensus I can just
> tweak this one and apply before 2.55 final.

Yep, that sounds great. Looks like it already happened. :)

-Peff

^ permalink raw reply

* Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Jeff King @ 2026-06-29 20:35 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
In-Reply-To: <akIJQbOUbdBbkTef@pks.im>

On Mon, Jun 29, 2026 at 07:57:21AM +0200, Patrick Steinhardt wrote:

> > It would be nice if we had a way to generate all of these refs without
> > writing so many individual files. But even if we taught the ref code to
> > write large cases directly to the packed-refs file, we'd still need to
> > take individual locks. The real solution is a backend like reftable,
> > which shaves ~30% off of the test runtime.
> 
> We kind of already have this with the `REF_TRANSACTION_FLAG_INITIAL`
> flag, but right now it is only used when performing a clone or when
> migrating references. Also, it requires an empty repository that has no
> references yet.
> 
> It raises the question whether we could also extend git-fast-import(1)
> to use it, as it would typically be run on an almost-empty repository.
> It's the "almost" that kills it though, as we already do have at least
> the HEAD reference. So it could be feasible, but it's not as trivial as
> just setting the flag and then we're magically faster.
> 
> And besides, in this particular test here we run git-fast-import(1)
> multiple times in the same repository, so it wouldn't help us.
> 
> We could of course extend all of this so that Git is able to write into
> the packed-refs directly, even with preexisting refs. But I agree with
> your sentiment: it doesn't feel worth it as the reftable backend fixes
> scenarios like this anyway.

Yup. In the past I've pondered exposing this via update-ref, but I think
it's too weird and/or dangerous to do so. Especially because you are
still stuck creating all of the .lock files, so the performance is not
even that much better (though it does save you doing so _twice_ when you
then pack the refs).

So the performance option you really want is "YOLO, just write some
packed-refs without locking". But that is not something I think we want
to expose to users. ;)

We could do it ad-hoc within this test like so:

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index dcff0bc7d4..276c7ac002 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -389,11 +389,15 @@ create_tags () {
 		echo "from :$1"
 	done | git fast-import --export-marks=marks &&
 
+	# should be mostly a noop, but makes sure we have the right header
+	git pack-refs &&
+
 	# now assign tags to all the dangling commits we created above
+	# It is OK to write directly to the packed-refs file because we know
+	# that our entries are sorted by refname, and that they all
+	# come after what we wrote earlier.
 	tag=$(perl -e "print \"bla\" x 30") &&
-	sed -e "s|^:\([^ ]*\) \(.*\)$|create refs/tags/$tag-\1 \2|" <marks >input &&
-	git update-ref --stdin <input &&
-	rm input
+	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
 }
 
 test_expect_success 'create 2,000 tags in the repo' '

That gives us the same ~30% speedup that using reftables does, but it
still is quite gross and fragile. And it is not even strictly correct,
because we don't zero-pad the numbered tags (so our file is subtly out
of order).  Plus it would need to be conditional on the ref backend
being used. Yuck.


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.

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.

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).

-Peff

^ permalink raw reply related

* Re: [PATCH 6/6] builtin/receive-pack: stage incoming objects via ODB transactions
From: Justin Tobler @ 2026-06-29 20:25 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aju_AmlKVi5UZaiQ@pks.im>

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.

> It might've also made sense to split this commit up into two: one to
> introduce the flag parameter, and then one to do the changes to
> git-receive-pack(1).

Ya, I think you are right. I actually originally had it this way but
squashed these two commits together for some reason. I'll split them
back up in the next version.

> > 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
> > @@ -112,8 +112,6 @@ static enum {
> >  } use_keepalive;
> >  static int keepalive_in_sec = 5;
> >  
> > -static struct tmp_objdir *tmp_objdir;
> > -
> >  static struct proc_receive_ref {
> >  	unsigned int want_add:1,
> >  		     want_delete:1,
> 
> I assume the goal is that we convert all other users of the tmp-objdir
> subsystem to also use transactions eventually, so that this becomes an
> implementation detail fo the files transaction?

Yes, that is exactly my plan :)

> > @@ -2106,14 +2104,13 @@ static void execute_commands(struct command *commands,
> >  	 * Now we'll start writing out refs, which means the objects need
> >  	 * to be in their final positions so that other processes can see them.
> >  	 */
> > -	if (tmp_objdir_migrate(tmp_objdir) < 0) {
> > +	if (odb_transaction_commit(the_repository->objects->transaction)) {
> >  		for (cmd = commands; cmd; cmd = cmd->next) {
> >  			if (!cmd->error_string)
> >  				cmd->error_string = "unable to migrate objects to permanent storage";
> >  		}
> >  		return;
> >  	}
> > -	tmp_objdir = NULL;
> 
> We don't need to unset the transaction because that's what
> `odb_transaction_commit()` already does for us, I assume?

That is correct. The tmp_objdir global is actually removed and now
competely managed by the ODB transaction. We can probably actuall remove
the "tmp-objdir" include now too. I'll do that in the next version.

> > @@ -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?

> > @@ -2351,20 +2349,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
> >  		strvec_push(&child.args, alt_shallow_file);
> >  	}
> >  
> > -	tmp_objdir = tmp_objdir_create(the_repository, "incoming");
> > -	if (!tmp_objdir) {
> > -		if (err_fd > 0)
> > -			close(err_fd);
> > -		return "unable to create temporary object directory";
> > -	}
> > -	strvec_pushv(&child.env, tmp_objdir_env(tmp_objdir));
> > -
> > -	/*
> > -	 * Normally we just pass the tmp_objdir environment to the child
> > -	 * processes that do the heavy lifting, but we may need to see these
> > -	 * objects ourselves to set up shallow information.
> > -	 */
> > -	tmp_objdir_add_as_alternate(tmp_objdir);
> > +	strvec_pushv(&child.env, odb_transaction_env(transaction));
> 
> Interesting, this here seems like a change in behaviour. Previously we
> added the transactions as an alternate, but now we only propagate it via
> the environment. I didn't see this mentioned in the commit message.

That's not quite correct. By using the ODB transaction interface, the
underlying tmp_objdir is managed transparently. When we begin the ODB
transaction, the temp directory that gets created is automatically
created and applied as the primary ODB. This ultimately produces an
equivalent result, its just now set up when the transaction is started
(which happens a little bit earlier).

The only behavior change here is that the temp directory is being
applied as the primary instead of an alternate in the main
git-receive-pack(1) process. Since all writes though are handled by the
child processes that get spawned, this shouldn't really matter though.

This is certainly rather confusing though, and should be mentioned in
the commit message. I'll do this in the next version.

> > @@ -2707,7 +2694,10 @@ int cmd_receive_pack(int argc,
> >  		if (!si.nr_ours && !si.nr_theirs)
> >  			shallow_update = 0;
> >  		if (!delete_only(commands)) {
> > -			unpack_status = unpack_with_sideband(&si);
> > +			if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE))
> > +				unpack_status = "unable to start ODB transaction";
> 
> s/ODB/object/
> 
> This may be visible to the user, and "ODB" may mean nothing to them.

Will change in the next version.

> > diff --git a/object-file.c b/object-file.c
> > index 14064d188a..e7958753ec 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1702,7 +1703,8 @@ static const char **odb_transaction_files_env(struct odb_transaction *base)
> >  }
> >  
> >  int odb_transaction_files_begin(struct odb_source *source,
> > -				struct odb_transaction **out)
> > +				struct odb_transaction **out,
> > +				enum odb_transaction_flags flags)
> >  {
> >  	struct odb_transaction_files *transaction;
> >  	struct object_database *odb = source->odb;
> > @@ -1717,6 +1719,20 @@ int odb_transaction_files_begin(struct odb_source *source,
> >  	transaction->base.commit = odb_transaction_files_commit;
> >  	transaction->base.write_object_stream = odb_transaction_files_write_object_stream;
> >  	transaction->base.env = odb_transaction_files_env;
> > +
> > +	transaction->prefix = "bulk-fsync";
> > +	if (flags & ODB_TRANSACTION_RECEIVE) {
> > +		/*
> > +		 * ODB transactions for git-receive-pack(1) eagerly create a
> > +		 * temporary directory and use a different prefix.
> > +		 */
> > +		transaction->prefix = "incoming";
> > +		if (odb_transaction_files_prepare(&transaction->base)) {
> > +			free(transaction);
> > +			return -1;
> > +		}
> > +	}
> > +
> 
> Okay, makes sense. I really wonder whether we need to insist this much
> on the exact name used by this, but better be safe than sorry for now I
> guess.
> 
> And as mentioned before, I also wonder whether it really makes sense to
> have the lazy creation of the tmp-objdir. Maybe add a NEEDSWORK item
> here that mentions we want to investigate whether this is even needed at
> all?

Ya, I was worried that there would be some reason the temp dir prefix
should remain the same or there would be some performance reason to
lazily create directories. I didn't investigate too deeply though. I'll
add a NEEDSWORK comment to make note of this.

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index 536458297b..78392ff13d 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -44,6 +43,10 @@ struct odb_transaction {
> >  	const char **(*env)(struct odb_transaction *transaction);
> >  };
> >  
> > +enum odb_transaction_flags {
> > +	ODB_TRANSACTION_RECEIVE = (1 << 0),
> > +};
> 
> It's not clear at all what this flag does based on its name, so we
> should have documentation for it.

Will update.

Thanks,
-Justin

^ permalink raw reply

* Re: [PATCH v7 11/11] builtin/history: implement "drop" subcommand
From: Junio C Hamano @ 2026-06-29 19:49 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: git, Pablo Sabater, Kristoffer Haugsbakk, Phillip Wood,
	Christian Couder
In-Reply-To: <20260629-b4-pks-history-drop-v7-11-6e9392a957d8@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> +static int find_head_tree_change(struct repository *repo,
> +				 const struct replay_result *result,
> +				 struct commit **old_head,
> +				 struct commit **new_head,
> +				 bool *changed)
> +{
> +	const struct replay_ref_update *head_update = NULL;
> +	struct commit *old_head_commit, *new_head_commit;
> +	struct tree *old_head_tree, *new_head_tree;
> +	const char *head_target;
> +	int head_flags;
> +
> +	*changed = false;
> +
> +	head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo),
> +					      "HEAD", RESOLVE_REF_NO_RECURSE,
> +					      NULL, &head_flags);
> +	if (!head_target)
> +		return error(_("cannot look up HEAD"));

Here head_target would be something like "refs/heads/master", or
whatever the "HEAD" happens to point at.

> +	if (!(head_flags & REF_ISSYMREF))
> +		head_target = "HEAD";

But if it is not a symref, then it is a detached HEAD.  We manually
set it to "HEAD" again.  We know head_target was not NULL, so what
did we receive in it from refs_resolve_ref_unsafe() call, before we
overwrite it here?

> +	for (size_t i = 0; i < result->updates_nr; i++) {
> +		if (!strcmp(result->updates[i].refname, head_target)) {
> +			head_update = &result->updates[i];
> +			break;
> +		}
> +	}

We see if we are going to update the currently checked out branch.

The .updates[] array was earlier populated by the caller that called
compute_pending_ref_updates() before it called us.

> +	if (!head_update)
> +		return 0;

And we return if not.  That is a simple and happy case.  Otherwise
we'd tell the caller about the updated commit and tree (strictly
speaking that would be redundant, but since we have it already, it
is better to give them to the caller instead of forcing the caller
to unwrap the commit).

> +static int cmd_history_drop(int argc,
> +			    const char **argv,
> +			    const char *prefix,
> +			    struct repository *repo)
> +{
> +...
> +	struct option options[] = {
> +		OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
> +			       N_("control which refs should be updated"),
> +			       PARSE_OPT_NONEG, parse_ref_action),
> ...
> +		OPT_END(),
> +	};
> ...
> +	ret = compute_pending_ref_updates(&revs, action, original, rewritten,
> +					  empty, &result);

Here we call the function.  When action is "--update-refs=head",
doesn't this code in compute_pending_ref_updates() ... 

		if (action == REF_ACTION_HEAD &&
		    decoration->type != DECORATION_REF_HEAD)
			continue;

... skip any reference name (loaded by load_ref_decorations() lazily
by calling get_name_decoration()) that is not "HEAD", which would
mean we end up not finding any hits in the find_head_tree_change()
function call we make later ...

> +	if (ret) {
> +		ret = error(_("failed replaying descendants"));
> +		goto out;
> +	}
> + ...
> +	if (!is_bare_repository()) {
> +		ret = find_head_tree_change(repo, &result, &old_head,
> +					    &new_head, &head_moves);

... here?

^ permalink raw reply

* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Phillip Wood @ 2026-06-29 19:48 UTC (permalink / raw)
  To: Harald Nordgren
  Cc: Patrick Steinhardt, phillip.wood,
	Harald Nordgren via GitGitGadget, git
In-Reply-To: <CAHwyqnWQmObWr3N81_EU6F13iyKp3FfY8KSNFfoAjS4r_0qJrQ@mail.gmail.com>

On 29/06/2026 19:03, Harald Nordgren wrote:
>> So instead of
>>
>>       # This is the combination of 4 commits
>>       # This is the first commit message
>>       Base subject
>>
>>       Base body
>>
>>       # This is the second commit message
>>       # Another subject
>>
>>       # Another body
>>
>>       # This is the third commit message
>>       # fixup! Base subject
>>
>>       # This is the fourth commit message
>>       # amend! Another subject
>>       A better subject
>>
>>       A better body
>>
>> We'd have
>>
>>       # This is the combination of 4 commits
>>       # 123 Base subject
>>       # 456 Another subject
>>       # 789 fixup! Base subject
>>       # abc amend! Another subject
>>
>>       Base Subject
>>
>>       Base Body
>>
>>       Another subject
>>
>>       Another Body
> 
> I think this makes it a lot harder to read. If every commit body was
> always just a single paragraph it could make sense, but it's generally
> not. Look at the commits in this series, with no delimiter of where
> one commit message ends and the next one starts, it would be very
> confusing.

You've trimmed the line where I said

 >> Possibly with a comment before each message saying where it came
 >> from.

So I'm not against adding a comment before each message, but I do think 
we should omit any messages that would be commented out completely. If 
you look at the rebase example above you can see there is a mass of 
comments between the two pieces of text that make up the new message. 
That makes it hard to see what is actually going to be included in the 
new message.

Thanks

Phillip

>> It would be good to error out if the user tries squash a fixup! style
>> commit and range does not contain its target commit.
> 
> Good point, I don't see a good reason to allow this.
> 
>> There does seem to be some support for merges in this patch series which
>> I think behaves pretty sensibly. If we have
>>
>>             C - D
>>            /     \
>>     - A - B - E - F - G
>>
>> Then squashing A..G should be fine because the parents of F are in the
>> range and it looks like we support that. Squashing should B..G without
>> --ancestry-path should be safe as well because B ends up as the parent
>> of the squashed commit but we don't have a way to disable
>> --ancestry-path (and maybe we don't want to add one). Squashing F^@..G
>> might be useful to fixup a merge (though perhaps amending F rather than
>> creating G is a simpler way to fix a broken merge). Squashing E..G does
>> not make sense because the range does not include one of the merge parents.
> 
> Thanks for a good explanation here!
> 
> 
> Harald
> 


^ permalink raw reply

* Re: [PATCH 5/6] odb/transaction: add transaction env interface
From: Justin Tobler @ 2026-06-29 19:20 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aju-_Nf3kmoIidue@pks.im>

On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> On Tue, Jun 23, 2026 at 11:19:19PM -0500, Justin Tobler wrote:
> > The ODB transaction backend is responsible for creating/managing its own
> > staging area for writing objects. Other child processes spawned by Git
> > may need to access to uncommitted objects or write new objects in the
> 
> s/may need to access to/may need access to/

Will fix.

> > staging area though.
> > 
> > Introduce `odb_transaction_env()` which is expected to provide the set
> > of environment variables needed by a child process to access the
> > transaction staging area.
> 
> Possessive s is missing, I think.

Yes. :)

[snip]
> > +
> > +	/*
> > +	 * This callback is expected to return a NULL-terminated array of
> > +	 * environment variables that a child process should inherit so
> > +	 * that its object writes participate in the transaction. The
> > +	 * returned array is owned by the backend and remains valid until
> > +	 * the transaction ends. May return NULL when the backend does not
> > +	 * need to expose any state to child processes.
> > +	 */
> > +	const char **(*env)(struct odb_transaction *transaction);
> 
> Would it make more sense to adapt this function so that:
> 
>   - It receives a `struct strvec` as input that the environment
>     variables are to be amended to.
> 
>   - It returns a normal error code to indicate errors?

Ya, that is probably a more sensible interface as it would be nice to be
able to signal an error. Will do in the next version.

Thanks,
-Justin

^ permalink raw reply

* Re: [PATCH 4/6] odb/transaction: propagate commit errors
From: Justin Tobler @ 2026-06-29 19:16 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aju-90Uayxwsevm7@pks.im>

On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> On Tue, Jun 23, 2026 at 11:19:18PM -0500, Justin Tobler wrote:
> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index cd6d50f2e5..7898770071 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -54,7 +54,7 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
> >   * Commits an ODB transaction making the written objects visible. If the
> >   * specified transaction is NULL, the function is a no-op.
> >   */
> > -void odb_transaction_commit(struct odb_transaction *transaction);
> > +int odb_transaction_commit(struct odb_transaction *transaction);
> 
> Should the function comment be amended, as well? We should definitely
> point out that calling this with a NULL transaction also returns
> success.

Will do in the next version.

Thanks,
-Justin

^ permalink raw reply

* Re: [PATCH 3/6] odb/transaction: propagate begin errors
From: Justin Tobler @ 2026-06-29 19:15 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aju-8hUeuyL6gnNU@pks.im>

On 26/06/24 01:26PM, Patrick Steinhardt wrote:
> On Tue, Jun 23, 2026 at 11:19:17PM -0500, Justin Tobler wrote:
> > diff --git a/odb/transaction.c b/odb/transaction.c
> > index b16e07aebf..d3de01db50 100644
> > --- a/odb/transaction.c
> > +++ b/odb/transaction.c
> > @@ -2,14 +2,20 @@
> >  #include "odb/source.h"
> >  #include "odb/transaction.h"
> >  
> > -struct odb_transaction *odb_transaction_begin(struct object_database *odb)
> > +int odb_transaction_begin(struct object_database *odb,
> > +			  struct odb_transaction **out)
> >  {
> > -	if (odb->transaction)
> > -		return NULL;
> > +	int ret;
> >  
> > -	odb_source_begin_transaction(odb->sources, &odb->transaction);
> > +	if (odb->transaction) {
> > +		*out = NULL;
> > +		return 0;
> > +	}
> 
> Hm. So we may return successful, but not set the `out` pointer to a
> transaction. And...
> 
> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index f4c1ebfaaa..cd6d50f2e5 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -33,11 +35,20 @@ struct odb_transaction {
> >  };
> >  
> >  /*
> > - * Starts an ODB transaction. Subsequent objects are written to the transaction
> > - * and not committed until odb_transaction_commit() is invoked on the
> > - * transaction. If the ODB already has a pending transaction, NULL is returned.
> > + * Starts an ODB transaction and returns it via `out`. Subsequent objects are
> > + * written to the transaction and not committed until odb_transaction_commit()
> > + * is invoked on the transaction. Returns 0 on success and a negative value on
> > + * error. If the ODB already has a pending transaction, `out` is set to NULL.
> >   */
> > -struct odb_transaction *odb_transaction_begin(struct object_database *odb);
> > +int odb_transaction_begin(struct object_database *odb,
> > +			  struct odb_transaction **out);
> > +
> > +static inline void odb_transaction_begin_or_die(struct object_database *odb,
> > +						struct odb_transaction **out)
> > +{
> > +	if (odb_transaction_begin(odb, out))
> > +		die(_("failed to start ODB transaction"));
> > +}
> 
> ... we don't special-case that here, either. So a caller may invoke the
> function, not die, but it might still not have a valid transaction. That
> feels wrong to me.

Ya, the original idea for a NULL transaction to signal that there is
already a transaction in flight and nested ODB transaction users to use
that to determine whether they needed to start a new transaction or not.
I completely agree thought that this is rather ackward.

Instead we could just make the callers that are worried about
potentially nested transactions handle this explicitly. I'll do that in
the next version.

-Justin

^ permalink raw reply

* Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Justin Tobler @ 2026-06-29 19:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqqjyrniy6r.fsf@gitster.g>

On 26/06/24 11:35AM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > The "files" transaction backend may encounter errors related to managing
> > the temporary directory used to stage objects, but silently ignores
> > these errors. Instead return errors encountered in the
> > `odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
> > callers to handle as needed.
> 
> "handle them as needed", perhaps.

Will fix, thanks

[snip]
> The caller of this function does react to a failure of it, ...
> 
> > @@ -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;
> >  }
> 
> ... like this, which is good.  Do we need an explicit "abort-transaction",
> or is that implicit?

So this is currently handled implicitly via
`tmp-objdir.c:remove_tmp_objdir()` which gets registered as an atexit()
handler. As long as the tmp_objdir global remains set, it will
automatically get cleaned up.

In a subsequent series, I do plan to add `odb_transaction_abort()` to
the transaction interface. It may make sense to also use that here to
make the cleanup a bit more explicit though.

-Justin

^ permalink raw reply

* Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Justin Tobler @ 2026-06-29 19:04 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <akK1roQJknYstX0u@denethor>

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.

-Justin

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox