Re: [PATCH 1/9] refs: make _advance() check struct repo, not flag

From: Jonathan Tan <jonathantanmy@google.com>
To: peff@peff.net
Cc: jonathantanmy@google.com, git@vger.kernel.org
Subject: Re: [PATCH 1/9] refs: make _advance() check struct repo, not flag
Date: Fri, 24 Sep 2021 11:28:37 -0700	[thread overview]
Message-ID: <20210924182837.2923447-1-jonathantanmy@google.com> (raw)
In-Reply-To: <YU4VUF17tyG2AZi3@coredump.intra.peff.net>

> 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@google.com/