From: Junio C Hamano <firstname.lastname@example.org> To: Taylor Blau <email@example.com> Cc: "Jeff King" <firstname.lastname@example.org>, "SZEDER Gábor" <email@example.com>, "Derrick Stolee" <firstname.lastname@example.org>, email@example.com Subject: Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits' Date: Fri, 03 Apr 2020 12:47:03 -0700 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <20200403184933.GA57202@syl.local> (Taylor Blau's message of "Fri, 3 Apr 2020 12:49:33 -0600") Taylor Blau <email@example.com> writes: > On Fri, Apr 03, 2020 at 02:30:57PM -0400, Jeff King wrote: >> On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote: >> >> > Check that all input records are indeed valid commit object ids and >> > return with error otherwise, the same way '--stdin-packs' handles >> > invalid input; see e103f7276f (commit-graph: return with errors during >> > write, 2019-06-12). >> >> Can you explain more why the old behavior is a problem? For reasons (see >> below), we want to do something like: >> >> git for-each-ref --format='%(objectname)' | >> git commit-graph write --stdin-commits >> ... >> - you're generating an incremental graph update. You know somehow that >> a few refs were updated, and you want to feed those tips to generate >> the incremental, but not the rest of the refs (not because it would >> be wrong to do so, but in the name of keeping it O(size of change) >> and not O(number of refs in the repo). >> ... >> Normally I'm in favor of more error checking instead of less, but in >> this case it feels like it's putting scripted use at a disadvantage >> versus the internal code (e.g., the auto-write for git-fetch uses the >> "--reachable" semantics for its internal invocation). I think the "incremental from the tip of refs" is a valid and useful use case. I am not sure if the rationale given in the original to compare the (stricter) check done here and what e103f7276f did (which does not seem to get any input, valid or invalid, from the end users) was a meaningful comparison, and regardless of Gábor's answer to Peff's question, I think we should have an easy way to let the machinery itself filter non-commit, so "--[no-]check-oids" that optionally turns the stricter checking off would be an easy way out. I do not have a strong opinion on which way the default should be (at least not yet), but given that this was already in two major releases ago, I'd assume that stricter-by-default-with-escape-hatch would be the way to go (at least for now). > For what it's worth, (and in case it wasn't obvious) this came about > because we feed '--stdin-commits' at GitHub, and observed exactly this > error case. I wasn't sure what approach would be more palatable, so I > prepared both in my fork at https://github.com/ttaylorr/git: > > - Branch 'tb/commit-graph-dont-check-oids' drops this checking > entirely. > > - Branch 'tb/commit-graph-check-oids-option' adds a > '--[no-]check-oids', in case that this is generally desirable > behavior, by offering an opt-out of this OID checking. Thanks.
next prev parent reply other threads:[~2020-04-03 19:47 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 2020-04-03 19:55 ` Taylor Blau 2020-04-03 19:47 ` Junio C Hamano [this message] 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 * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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
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 NNTP newsgroup(s).