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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Apr 13, 2020 at 05:25:06PM -0400, Jeff King wrote:
> On Mon, Apr 13, 2020 at 01:39:34PM -0600, Taylor Blau wrote:
>
> > > > 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...
>
> Yes, the behavior there looks fine to me. Though you may want to catch
> the parse_oid_hex() separately and return its own error message. Telling
> the user "I don't understand non-hex object names" instead of just
> "invalid commit object" may be useful. I think it would also make the
> flow of the function easier to follow.
>
> If we were writing from scratch, I'd actually suggest that
> builtin/commit-graph.c do parse_oid_hex() call as we read lines, and
> then commit-graph could just be working with an oid_array or oidset,
> which would reduce overall memory usage. I don't know if that would
> cause other complications, but it could be worth looking into.

It actually improved the situation quite a bit, so thanks for the
suggestion. In addition refactoring it was quite a lot of fun. The
second-order benefit was that it moves these two failure modes into
separate parts of the code.

Converting the user input to an OID (and thus dealing with input like
"HEAD", "refs/tags/foo", and etc.) is a separate part from turning those
OIDs into 'struct commit *'s. So the "non-OID input" and "this OID
doesn't refer to a commit" checks can be moved into the builtin and
library machinery separately, which is quite handy.

The patch is too large to send here inline (there's a lot of noise from
renaming 'commit_hex' to 'commits'), but I'll include it in my
commit-graphs backlog series shortly.

> > +               if (ret || (ctx->check_oids && !result)) {
> >                         error(_("invalid commit object id: %s"),
> >                             commit_hex->items[i].string);
> >                         return -1;
>
> We could also take this a step further and just ditch check_oids
> entirely (under the assumption that nobody really wants it; they just
> wanted to catch bad names in the first place).

I'd rather let this shake out in a discussion of the patch once I send
it, since I'm not sure how people feel in general.

> -Peff

Thanks,
Taylor