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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 3, 2020, 19:47 UTC
Message-ID: <xmqqd08oz70o.fsf@gitster.c.googlers.com>
In-Reply-To: <20200403184933.GA57202@syl.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 24 quoted lines

> 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).

Show 11 quoted lines

> 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.