git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: "Taylor Blau" <me@ttaylorr.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'
Date: Mon, 13 Apr 2020 20:04:10 -0600	[thread overview]
Message-ID: <20200414020410.GC71440@syl.local> (raw)
In-Reply-To: <20200413212506.GA28407@coredump.intra.peff.net>

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

  reply	other threads:[~2020-04-14  2:04 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-05  8:02 [PATCH 0/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits' SZEDER Gábor
2019-08-05  8:02 ` [PATCH 1/3] t5318-commit-graph: use 'test_expect_code' SZEDER Gábor
2019-08-05  8:02 ` [PATCH 2/3] commit-graph: turn a group of write-related macro flags into an enum SZEDER Gábor
2019-08-05  8:02 ` [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits' SZEDER Gábor
2019-08-05 13:57   ` Derrick Stolee
2019-08-05 17:57     ` SZEDER Gábor
2020-04-03 18:30   ` Jeff King
2020-04-03 18:49     ` Taylor Blau
2020-04-03 19:38       ` SZEDER Gábor
2020-04-03 19:51         ` Jeff King
2020-04-03 20:40           ` SZEDER Gábor
2020-04-03 23:10             ` Jeff King
2020-04-13 19:39               ` Taylor Blau
2020-04-13 21:25                 ` Jeff King
2020-04-14  2:04                   ` Taylor Blau [this message]
2020-04-03 19:55         ` Taylor Blau
2020-04-03 19:47       ` Junio C Hamano
2020-04-03 19:57         ` Taylor Blau
2019-08-05 10:14 ` [PATCH 0/3] " SZEDER Gábor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200414020410.GC71440@syl.local \
    --to=me@ttaylorr.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).