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
next prev parent 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] " 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 \ --subject='Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in '\''write --stdin-commits'\''' \ /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
Code repositories for project(s) associated with this 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).