Re: [PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`

From: Jeff King <peff@peff.net>
Date: Nov 22, 2024, 15:41 UTC
Message-ID: <20241122154116.GA1738455@coredump.intra.peff.net>
In-Reply-To: <xmqqiksgdrv8.fsf@gitster.g>

On Fri, Nov 22, 2024 at 09:39:07AM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
> 
> > Fix that by correctly assigning the value of 'revs->tag_objects' to the
> > 'tmp_tags' field.
> 
> Makes sense.  This breakage would make no difference in practice
> right now, as {type}_objects members of the rev_info structure have
> always been all flipped on together since their inception at
> 3c90f03d (Prepare git-rev-list for tracking tag objects too,
> 2005-06-29), so the original value of the tag_objects member is
> always the same as that of the blob_objects member.

Yep, I concur.

Show 8 quoted lines

> A possible alternative "fix" for this typo could be to unify these
> {type}_objects members into a single .non_commit_objects member in
> the rev_info structure; given that we (as far as I remember) never
> utilized the "feature" that, say, we can ask only for trees but not
> blobs for the past ~20 years, nobody knows if the apparent "support"
> for that feature is subtly broken in multiple ways (and one of them
> you just fixed with this patch) and the "feature" may not be worth
> keeping.

I have been tempted to do that, too. But FWIW, I do remember implementing something that set some but not all of the fields in a series. Digging in my old branches, I think it may have been for a reachability check for remote git-archive, where we want to dig only as deep as the object we are looking for (so if we are finding out if a tree is reachable, we do not need to ask about blobs, and looking for a tag needs neither trees nor blobs).

If no such code made it into the project in those 20 years, it may not worth worrying about too much. But I wanted to point it out as a plausible use case.

> But neverhteless, this is a correct thing to do unless we decide to
> rip out the support for individual types.  Will queue.

Agreed.

-Peff