Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'

[Taylor Blau <me@ttaylorr.com>]

On Fri, Apr 03, 2020 at 07:10:21PM -0400, Jeff King wrote:
> On Fri, Apr 03, 2020 at 10:40:13PM +0200, SZEDER Gábor wrote:
>
> > > Do you care about complaining about:
> > >
> > >   git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> > >
> > > ? That's the case that's much more interesting, I think.
> >
> > Hm, are you trying to go in the direction where '--stdin-commits'
> > would keep erroring out on any non-full-hex-oid, but would accept and
> > silently ignore any hex oids that are not commits (perhaps even when
> > there is no such object, dunno)?  I think that would support the use
> > cases you mentioned, while it would still save me when I do the 'echo
> > <ref>' thing (somehow I regularly do that, remember doing it the day
> > before yesterday!).
>
> Yes, exactly. The case you care about and the case I care about are
> different ones, so there's no inherent conflict between them.

I was looking back again at this today, and I think we need something
more or less like the following on top. I'll send it out later today or
early tomorrow...

diff --git a/commit-graph.c b/commit-graph.c
index 2d436907cd..58e7890590 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1221,17 +1221,24 @@ static int fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,
                                        commit_hex->nr);
        }
        for (i = 0; i < commit_hex->nr; i++) {
+               int ret;
                const char *end;
                struct object_id oid;
                struct commit *result;

                display_progress(ctx->progress, i + 1);
-               if (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&
-                   (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {
+
+               ret = parse_oid_hex(commit_hex->items[i].string, &oid, &end);
+               if (!ret) {
+                   result = lookup_commit_reference_gently(ctx->r, &oid, 1);
+                   if (result) {
                        ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
                        oidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));
                        ctx->oids.nr++;
-               } else if (ctx->check_oids) {
+                   }
+               }
+
+               if (ret || (ctx->check_oids && !result)) {
                        error(_("invalid commit object id: %s"),
                            commit_hex->items[i].string);
                        return -1;

> -Peff

Thanks,
Taylor