Re: [PATCH 3/6] connected: refactor iterator to return next object ID directly

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

> On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:
>> The object ID iterator used by the connectivity checks returns the next
>> object ID via an out-parameter and then uses a return code to indicate
>> whether an item was found. This is a bit roundabout: instead of a
>> separate error code, we can just retrun the next object ID directly and
>> use `NULL` pointers as indicator that the iterator got no items left.
>> Furthermore, this avoids a copy of the object ID.
>> 
>> Refactor the iterator and all its implementations to return object IDs
>> directly. While I was honestly hoping for a small speedup given that we
>> can now avoid a copy, both versions perform the same. Still, the end
>> result is easier to understand and thus it makes sense to keep this
>> refactoring regardless.
>
> It's too bad about the lack of measurable performance gains, but the
> new code _is_ doing less, it's just not enough.
>
> I agree that the new code organization is better.

The only potential downside I can think of is the loss of ability to
convey the reason why it failed to return one by adding new return
codes from the function, which I do not immediately see all that
useful future extension anyway, so I agree that "we find what we
found, or NULL if we don't find" is much straight-forward and easier
to understand.

Nicely done.

Thanks.