From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 06 Jan 2026 06:34:56 GMT
Subject: Re: [PATCH v2 3/7] packfile: extend `is_delta` field to allow for "unknown" state
Message-ID: <aVytEHdNHDHHNpLt@pks.im>
In-Reply-To: <87o6n8oyw2.fsf@iotcl.com>

On Mon, Jan 05, 2026 at 04:35:57PM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The `struct object_info::u::packed::is_delta` field determines whether
> > or not a specific object is stored as a delta. It only stores whether or
> > not the object is stored as delta, so it is treated as a boolean value.
> >
> > This boolean is insufficient though: when reading a packed object via
> > `packfile_store_read_object_info()` we know to skip parsing the actual
> > object when the user didn't request any object-specific data. In that
> > case we won't read the object itself, but will only look up its position
> > in the packfile. Consequently, we do not know whether it is a delta or
> > not.
> 
> This explains why you're introducing "unknown", but I'm having trouble
> understanding why we need distinction between ofs-delta and ref-delta?
> 
> (To any other reader: If you want to know what those two are, check
> "Deltified representation" in Documentation/gitformat-pack.adoc)

Good question, and indeed we don't need that information at any
callsite right now. We do discern the delta type in various locations,
but mostly do so internally at "packfile.c" so that we know how to read
the given object. And there we rely on the `OBJ_REF_DELTA` and
`OBJ_OFS_DELTA` types.

We could of course adapt this to only use `PACKED_OBJECT_TYPE_DELTA`.
But we already have the information readily available at our figertips,
and over time I'd ideally rather want to get rid of the `OBJ_*_DELTA`
values as they leak internal implementation details of the packfile
store into the generic object interfaces. That's way down the road, but
by keeping around the information now it makes such a later conversion
easier.

> > This isn't really an issue right now, as the check for an empty request
> > is broken. But a subsequent commit will fix it, and once we do we will
> > have the need to also represent an "unknown" delta state.
> >
> > Prepare for this change by introducing a new enum that encodes the
> > object type. We don't use the "unknown" state just yet, but will start
> > to do so in the next commit.
> 
> A little bit confusing this "next commit" is [PATCH 6/7], but that's a
> note to any other reader and not so much a nitpick worth addressing.

Fair. I've fixed this up locally to say "subsequent commit".

Thanks!

Patrick