Re: [PATCH 5/6] odb/source: introduce generic object counting

[Patrick Steinhardt <ps@pks.im>]

On Tue, Mar 10, 2026 at 10:51:33AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +static int odb_source_files_count_objects(struct odb_source *source,
> > +					  enum odb_count_objects_flags flags,
> > +					  unsigned long *out)
> > +{
> > +	struct odb_source_files *files = odb_source_files_downcast(source);
> > +	unsigned long count;
> > +	int ret;
> > +
> > +	ret = packfile_store_count_objects(files->packed, flags, &count);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (!(flags & ODB_COUNT_OBJECTS_APPROXIMATE)) {
> > +		unsigned long loose_count;
> > +
> > +		ret = odb_source_loose_count_objects(source, flags, &loose_count);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		count += loose_count;
> > +	}
> > +
> > +	*out = count;
> > +	ret = 0;
> > +
> > +out:
> > +	return ret;
> > +}
> 
> The design to assume that the majority of objects should be in the
> packfiles and the number of loose objects can be ignored when we are
> getting approximation is inherited from the world before this
> series, I think, which is a valid choice for this series to make.
> 
> As your "get an approximate count of loose objects" counts a single
> shared fully, instead of punting as soon as the limit is hit, we
> could ask that function and add it in when the APPROXIMATE flag is
> passed, and get a bit more accurate number cheaply even when we are
> approximating.  I am not sure what the pros and cons of doing so
> myself, but you may already have thought about it and rejected it,
> perhaps?

Yeah, I was very torn on this myself, and I switched back and forth
multiple times. I don't really think there is a large downside if we
started to also count loose objects here. The performance overhead
should be negligible, and it may arrive at a result that is closer to
the real world.

But the loose object counting approximation is somewhat vague overall
with the way we extrapolate the object count, and as you mentioned it
matches the old semantics to not include loose objects. So that's why I
decided against including it, so that we retain semantics.

That being said, with the current set of users it doesn't really matter
too much which of both approaches we pick.

Patrick