Re: [PATCH 2/4] commit-graph: verify swapped zero/non-zero generation cases

From: Jeff King <peff@peff.net>
Date: Aug 11, 2023, 15:01 UTC
Message-ID: <20230811150114.GC2303200@coredump.intra.peff.net>
In-Reply-To: <xmqqleeir35l.fsf@gitster.g>

On Thu, Aug 10, 2023 at 02:36:06PM -0700, Junio C Hamano wrote:

Show 33 quoted lines

> Taylor Blau <me@ttaylorr.com> 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).

Show 5 quoted lines

> >  		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