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

[Jeff King <peff@peff.net>]

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

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