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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2023, 21:36 UTC
Message-ID: <xmqqleeir35l.fsf@gitster.g>
In-Reply-To: <9b9483893c072961c5871bd0bae17a7098d73c06.1691699851.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 17 quoted lines

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

>  		if (generation_zero == GENERATION_ZERO_EXISTS)
>  			continue;

... as I do not see what this "continue" is doing, I'd stop at expressing my puzzlement ;-)

Thanks.