From: Justin Tobler <jltobler@gmail.com>
Date: Mon, 06 Apr 2026 20:38:05 GMT
Subject: Re: [PATCH] object-file: don't use object database without a repository
Message-ID: <adQYS_ThpOzxCLTi@denethor>
In-Reply-To: <20260406200651.GA26091@coredump.intra.peff.net>

On 26/04/06 04:06PM, Jeff King wrote:
> On Mon, Apr 06, 2026 at 01:17:17PM -0500, Justin Tobler wrote:
> 
> > I agree with Peff here that the correct fix should continue to use the
> > object streaming mechanisms. To avoid this segfault, we really should
> > avoid using ODB transactions when there isn't an ODB in the first place.
> > 
> > I replied in another thread[1] with how we could go about fixing. To
> > summarize, it just so happens that I already have a patch[2] out on the
> > list that appears to resolve this issue.
> > 
> > For the use case here, git-diff(1) is only interested in generating the
> > hash for the "large" blobs and not actually writing anything to the ODB.
> > This patch introduces a separate "hash-only" variant of
> > `index_blob_packfile_transaction()` and is used to bypass creating an
> > ODB transaction when object writes are not needed.
> > 
> > If this is the route we want to go down, I can extract this patch from
> > the current series and send it as a separate fix. :)
> 
> Yeah, I think this is a good path forward. I took a look at making the
> transaction begin/end conditional, but that's not nearly enough anymore.
> The transaction object stores state which is used under the hood by
> index_blob_packfile_transaction(). So we'd really need some kind of fake
> noop transaction that understands how to stream.
> 
> Just having the caller divert to a "hash this without having an odb"
> interface is way simpler (especially since this is the only spot that
> needs it, so we are only paying the price once either way).
> 
> I gave a cursory look at the patch you linked. For a maint fix like this
> I think we could probably slim it down a bit: introduce the new
> hash-only helper but _don't_ actually rip flag support out of
> index_blob_packfile_transaction(), so we know that we can't accidentally
> break it. Though maybe that is being overly cautious; it only has one
> caller, and that caller would no longer be passing in any meaningful
> flags.

Ya, I think slimming down the patch probably makes sense. I'll start
working on it and make sure to include some tests too. :)

Thanks,
-Justin