> I think your goal here of passing around a repository object is good. > But rolling the meaning of DO_FOR_EACH_INCLUDE_BROKEN into an implicit > "do we have a non-NULL repository" makes things awkward, I think. > > As you noticed, we can't get rid of the flags parameter entirely. We > still have DO_FOR_EACH_PER_WORKTREE_ONLY. But I also have a series which > adds another flag which pairs with INCLUDE_BROKEN. Having half of the > logic implicit in the repository pointer and half in a flag would be > weird. > > I'll post that series in a moment, but what I'm wondering here is: would > it be that big a deal to just pass the repository object around, and it > is simply not used if INCLUDE_BROKEN is passed? Quoting my response to Junio (sent a few minutes ago, so you might have not seen it) [1]: so that callers that don't want to access the ODB (all the callers that currently use DO_FOR_EACH_INCLUDE_BROKEN) can be assured that the iterator won't do that. If we had a non-NULL "struct repository *repository" parameter, a future code change might inadvertently use it, thus causing a bug. I'll take a look at your series when it comes out, but from what you say, it looks like we should pass a non-nullable repository and keep the DO_FOR_EACH_INCLUDE_BROKEN flag. I'll update this patch set to do that. [1] https://lore.kernel.org/git/20210924175651.2918488-1-jonathantanmy@xxxxxxxxxx/
