git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: "Jeff King" <peff@peff.net>,
	"SZEDER Gábor" <szeder.dev@gmail.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: Fri, 03 Apr 2020 12:47:03 -0700	[thread overview]
Message-ID: <xmqqd08oz70o.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200403184933.GA57202@syl.local> (Taylor Blau's message of "Fri, 3 Apr 2020 12:49:33 -0600")

Taylor Blau <me@ttaylorr.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.

  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

  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=xmqqd08oz70o.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.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).