On Thu, Sep 08, 2022 at 04:35:16PM +0200, Omar Polo wrote:
> this is what i had in mind when i was trying to fix the memleak in the
> commit graph, i.e. make iter_next write the id to a user-provided
> storage.  I'm still a bit unsure though.
> 
> One the one hand I like many parts of diff below: it makes the
> management of the id a bit more explicit instead of the current rule
> of the 'pointer valid up until next call'.
> 
> On the other hand however returning a pointer (or NULL when reaching
> the end and returning GOT_ERR_ITER_COMPLETED) was nice.

It should be enough to return GOT_ERR_ITER_COMPLETED if there are no
more IDs, correct? Callers should assume that 'id' is only valid if NULL
was returned from the most recent call, i.e. no error occurred.

> Diff below
> makes blame_open need an extra variable `id_seen' to keep track of
> whether `id' is valid or not.  meh :/
> 
> What do you think?

In most cases, checking the returned error for NULL vs non-NULL should
be good enough. But using a separate variable like 'id_seen' to keep
such state is fine, too. You could name it 'have_id' instead of 'id_seen'.
We already use the have_* naming convention elsewhere for such cases.