Re: [PATCH 6/6] builtin/receive-pack: stage incoming objects via ODB transactions

[Patrick Steinhardt <ps@pks.im>]

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