Re: [PATCH 6/6] commit-graph: report incomplete chains during verification

From: Jeff King <peff@peff.net>
Date: Sep 28, 2023, 04:30 UTC
Message-ID: <20230928043058.GA57926@coredump.intra.peff.net>
In-Reply-To: <20230926060430.GF1341418@coredump.intra.peff.net>

On Tue, Sep 26, 2023 at 02:04:30AM -0400, Jeff King wrote:

Show 8 quoted lines

> @@ -608,6 +611,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
>  
>  		if (!valid) {
>  			warning(_("unable to find all commit-graph files"));
> +			*incomplete_chain = 1;
>  			break;
>  		}
>  	}

Reviewing my own patch since I noticed it misses a case. ;)

The whole function here looks like (abbreviated):

  int valid = 1;

  for (i = 0; i < count; i++) {
          if (get_oid_hex(line.buf, &oids[i])) {
                  warning(_("invalid commit-graph chain: line '%s' not a hash"),
                          line.buf);
                  valid = 0;
                  break;
          }
  
          valid = 0;
          for (odb = r->objects->odb; odb; odb = odb->next) {
                  struct commit_graph *g = load_commit_graph_one(r, graph_name, odb);
                  if (g) {
                          if (add_graph_to_chain(g, graph_chain, oids, i)) {
                                  graph_chain = g;
                                  valid = 1;
                          }
  
                          break;
                  }
          }
  
          if (!valid) {
                  warning(_("unable to find all commit-graph files"));
                  break;
          }
  }

My patch updates the final "if (!valid)" check, which covers anything that happened in the loop over the odb (i.e., finding and opening the file itself). But if we see a line which does not parse as an oid, we break out of the outer loop earlier. We set "valid", but nobody looks at it! So the caller would not be correctly notified that we had an error in that case.

The fix is simple: we can just check "valid" after leaving the outer loop, which covers both cases. And we'll want an extra test to check it. This is actually covered by the test modified earlier in patch 3, where sha1 and sha256 produced different results. I fixed it there by consistently corrupting the first line of the file, but we really want to check both cases.

I'll send a re-roll in a moment.

-Peff