On Thu, Aug 10, 2023 at 02:36:06PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > diff --git a/commit-graph.c b/commit-graph.c
> > index c68f5c6b3a..acca753ce8 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -2686,9 +2686,12 @@ static int verify_one_commit_graph(struct repository *r,
> > graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
> > oid_to_hex(&cur_oid));
> > generation_zero = GENERATION_ZERO_EXISTS;
> > - } else if (generation_zero == GENERATION_ZERO_EXISTS)
> > - graph_report(_("commit-graph has non-zero generation number for commit %s, but zero elsewhere"),
> > - oid_to_hex(&cur_oid));
> > + } else {
> > + if (generation_zero == GENERATION_ZERO_EXISTS)
> > + graph_report(_("commit-graph has non-zero generation number for commit %s, but zero elsewhere"),
> > + oid_to_hex(&cur_oid));
> > + generation_zero = GENERATION_NUMBER_EXISTS;
> > + }
>
> Hmph, doesn't this potentially cause us to emit the two reports
> alternating, if we are unlucky enough to see a commit with 0
> generation first (which will silently set gz to ZERO_EXISTS), then
> another commit with non-zero generation (which will complain we saw
> non-zero for the current one and earlier we saw zero elsewhere, and
> then set gz to NUM_EXISTS), and then another commit with 0
> generation (which will complain the other way, and set gz back again
> to ZERO_EXISTS)?
>
> I am tempted to say this gz business should be done with two bits
> (seen zero bit and seen non-zero bit), and immediately after we see
> both kinds, we should report once and stop making further reports,
> but ...
Yeah, I think you are right. It might be OK, in the sense that we would
show a different commit each time as we flip-flopped, but it's not clear
to me how valuable that is.
If the actual commit ids are not valuable, then we could just set bits
and then at the end of the loop produce one warning:
if (seen_zero && seen_non_zero) {
graph_report("oops, we saw both types");
}
Certainly that would make the code less confusing to me. :) But I really
don't know if marking the individual commit is useful or not (on the
other hand, it cannot be perfect, since when we see a mismatch we do not
know if it was _this_ commit that is wrong and the previous one is
right, or if the previous one was wrong and this one was right). I guess
we could also save an example of each type and report them (i.e., make
seen_zero and seen_non_zero pointers to commits/oids).
> > if (generation_zero == GENERATION_ZERO_EXISTS)
> > continue;
>
> ... as I do not see what this "continue" is doing, I'd stop at
> expressing my puzzlement ;-)
Yeah, I'm not sure on this bit. I had thought at first it was just
trying to avoid the rest of the loop for commits which are 0-generation.
But after Taylor's explanation that this is about whole files with
zero-generations, it makes sense that we would not do the rest of the
loop for any commit (it is already an error to have mixed zero/non-zero
entries, so the file fails verification).
In a "two bits" world, I think this just becomes:
if (seen_zero)
continue;
-Peff
