patch, 3 partscommit-graph: error out on invalid commit oids in 'write --stdin-commits'

> While 'git commit-graph write --stdin-commits' expects commit object
> ids as input, it accepts and silently skips over any invalid commit
> object ids, and still exits with success:
> 
>   # nonsense
>   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   # sometimes I forgot that refs are not good...
>   $ echo HEAD | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   # valid tree OID, but not a commit OID
>   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   $ ls -l .git/objects/info/commit-graph
>   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> 
> 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).

> Note that it should only return with error when encountering an
> invalid commit object id coming from standard input.  However,
> '--reachable' uses the same code path to process object ids pointed to
> by all refs, and that includes tag object ids as well, which should
> still be skipped over.  Therefore add a new flag to 'enum
> commit_graph_write_flags' and a corresponding field to 'struct
> write_commit_graph_context', so we can differentiate between those two
> cases.

> @@ -1215,20 +1216,21 @@ static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,
>  		struct commit *result;
>  
>  		display_progress(ctx->progress, i + 1);
> -		if (commit_hex->items[i].string &&
> -		    parse_oid_hex(commit_hex->items[i].string, &oid, &end))
> -			continue;
> -
> -		result = lookup_commit_reference_gently(ctx->r, &oid, 1);
> -
> -		if (result) {
> +		if (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&
> +		    (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {
>  			ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
>  			oidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));
>  			ctx->oids.nr++;
> +		} else if (ctx->check_oids) {
> +			error(_("invalid commit object id: %s"),
> +			    commit_hex->items[i].string);
> +			return -1;
>  		}
>  	}
>  	stop_progress(&ctx->progress);
>  	strbuf_release(&progress_title);
> +
> +	return 0;
>  }

> @@ -1775,6 +1777,7 @@ int write_commit_graph(const char *obj_dir,
>  	ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;
>  	ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;
>  	ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;
> +	ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;
>  	ctx->split_opts = split_opts;

> @@ -1829,8 +1832,10 @@ int write_commit_graph(const char *obj_dir,
>  			goto cleanup;
>  	}
>  
> -	if (commit_hex)
> -		fill_oids_from_commit_hex(ctx, commit_hex);
> +	if (commit_hex) {
> +		if ((res = fill_oids_from_commit_hex(ctx, commit_hex)))
> +			goto cleanup;
> +	}

> On 8/5/2019 4:02 AM, SZEDER Gábor wrote:
> > While 'git commit-graph write --stdin-commits' expects commit object
> > ids as input, it accepts and silently skips over any invalid commit
> > object ids, and still exits with success:
> > 
> >   # nonsense
> >   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
> >   $ echo $?
> >   0
> >   # sometimes I forgot that refs are not good...
> >   $ echo HEAD | git commit-graph write --stdin-commits
> >   $ echo $?
> >   0
> >   # valid tree OID, but not a commit OID
> >   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> >   $ echo $?
> >   0
> >   $ ls -l .git/objects/info/commit-graph
> >   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> > 
> > 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).
> 
> Consistency is good. We should definitely make these modes match.

> > Note that it should only return with error when encountering an
> > invalid commit object id coming from standard input.  However,
> > '--reachable' uses the same code path to process object ids pointed to
> > by all refs, and that includes tag object ids as well, which should
> > still be skipped over.  Therefore add a new flag to 'enum
> > commit_graph_write_flags' and a corresponding field to 'struct
> > write_commit_graph_context', so we can differentiate between those two
> > cases.
> 
> Thank you for the care here.

> While 'git commit-graph write --stdin-commits' expects commit object
> ids as input, it accepts and silently skips over any invalid commit
> object ids, and still exits with success:
> 
>   # nonsense
>   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   # sometimes I forgot that refs are not good...
>   $ echo HEAD | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   # valid tree OID, but not a commit OID
>   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   $ ls -l .git/objects/info/commit-graph
>   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> 
> 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).

> On Mon, Aug 05, 2019 at 10:02:40AM +0200, SZEDER Gábor wrote:
>
> > While 'git commit-graph write --stdin-commits' expects commit object
> > ids as input, it accepts and silently skips over any invalid commit
> > object ids, and still exits with success:
> >
> >   # nonsense
> >   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
> >   $ echo $?
> >   0
> >   # sometimes I forgot that refs are not good...
> >   $ echo HEAD | git commit-graph write --stdin-commits
> >   $ echo $?
> >   0
> >   # valid tree OID, but not a commit OID
> >   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> >   $ echo $?
> >   0
> >   $ ls -l .git/objects/info/commit-graph
> >   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> >
> > 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
>
> In v2.23 and earlier, that worked exactly like --reachable, but now it
> will blow up if there are any refs that point to a non-commit (e.g., a
> tag of a blob).
>
> It can be worked around by asking for %(objecttype) and %(*objecttype)
> and grepping the result, but that's awkward and much less efficient
> (especially if you have a lot of annotated tags, as we may have to open
> and parse each one).
>
> Now obviously you could just use --reachable for the code above. But
> here are two plausible cases where you might not want to do that:
>
>  - you're limiting the graph to only a subset of refs (e.g., you want to
>    graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).
>
>  - 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).
>
> The latter is the actual case that bit us. I suppose one could do
> something like:
>
>   git rev-list --no-walk <maybe-commits |
>   git commit-graph write --stdin-commits
>
> to use rev-list as a filter, but that feels kind of baroque.
>
> 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).

> -Peff
>
> PS As an aside, I think the internal git-fetch write could benefit from
>    this same trick: feed the set of newly-stored ref tips to the
>    commit-graph machinery, rather than using for_each_ref().

> 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:
> >
> > > While 'git commit-graph write --stdin-commits' expects commit object
> > > ids as input, it accepts and silently skips over any invalid commit
> > > object ids, and still exits with success:
> > >
> > >   # nonsense
> > >   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
> > >   $ echo $?
> > >   0
> > >   # sometimes I forgot that refs are not good...
> > >   $ echo HEAD | git commit-graph write --stdin-commits
> > >   $ echo $?
> > >   0
> > >   # valid tree OID, but not a commit OID
> > >   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> > >   $ echo $?
> > >   0
> > >   $ ls -l .git/objects/info/commit-graph
> > >   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> > >
> > > 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
> >
> > In v2.23 and earlier, that worked exactly like --reachable, but now it
> > will blow up if there are any refs that point to a non-commit (e.g., a
> > tag of a blob).
> >
> > It can be worked around by asking for %(objecttype) and %(*objecttype)
> > and grepping the result, but that's awkward and much less efficient
> > (especially if you have a lot of annotated tags, as we may have to open
> > and parse each one).
> >
> > Now obviously you could just use --reachable for the code above. But
> > here are two plausible cases where you might not want to do that:
> >
> >  - you're limiting the graph to only a subset of refs (e.g., you want to
> >    graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).
> >
> >  - 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).
> >
> > The latter is the actual case that bit us. I suppose one could do
> > something like:
> >
> >   git rev-list --no-walk <maybe-commits |
> >   git commit-graph write --stdin-commits
> >
> > to use rev-list as a filter, but that feels kind of baroque.
> >
> > 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).
> 
> 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.
> 
> Please let me know if you find either of these to be good options, and
> I'll happily send one of them to the list. Thanks.

> > -Peff
> >
> > PS As an aside, I think the internal git-fetch write could benefit from
> >    this same trick: feed the set of newly-stored ref tips to the
> >    commit-graph machinery, rather than using for_each_ref().
> 
> Thanks,
> Taylor

> > > Can you explain more why the old behavior is a problem?
> 
> Because when I do:
> 
>    # sometimes I forgot that refs are not good...
>    $ echo HEAD | git commit-graph write --stdin-commits
> 
> then I get _nothing_: neither an error, nor a commit-graph.

> On Fri, Apr 03, 2020 at 09:38:42PM +0200, SZEDER Gábor wrote:
> 
> > > > Can you explain more why the old behavior is a problem?
> > 
> > Because when I do:
> > 
> >    # sometimes I forgot that refs are not good...
> >    $ echo HEAD | git commit-graph write --stdin-commits
> > 
> > then I get _nothing_: neither an error, nor a commit-graph.
> 
> OK, that makes more sense: it's an input format error, because we only
> take hex oids.
> 
> Do you care about complaining about:
> 
>   git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> 
> ? That's the case that's much more interesting, I think.

> > Or introduce 'git commit-graph write --stdin-refs'?  Or teach
> > '--stdin-commits' to DWIM and accept and parse refs?  Though the
> > question still remains what to do with refs that can't be peeled back
> > to commits
> 
> Right. I think there are two orthogonal questions:
> 
>   - whether to resolve arbitrary names to objects and how to handle such
>     input if we don't
> 
>   - what to do with an oid (whether given as hex or resolved from a
>     name) that isn't a commit-ish
> 
> -Peff

> > Do you care about complaining about:
> > 
> >   git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> > 
> > ? That's the case that's much more interesting, I think.
> 
> 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!).

> On Fri, Apr 03, 2020 at 10:40:13PM +0200, SZEDER Gábor wrote:
>
> > > Do you care about complaining about:
> > >
> > >   git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> > >
> > > ? That's the case that's much more interesting, I think.
> >
> > 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.

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

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

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

> On Fri, Apr 03, 2020 at 12:49:33PM -0600, Taylor Blau wrote:
> > 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:
> > >
> > > > While 'git commit-graph write --stdin-commits' expects commit object
> > > > ids as input, it accepts and silently skips over any invalid commit
> > > > object ids, and still exits with success:
> > > >
> > > >   # nonsense
> > > >   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
> > > >   $ echo $?
> > > >   0
> > > >   # sometimes I forgot that refs are not good...
> > > >   $ echo HEAD | git commit-graph write --stdin-commits
> > > >   $ echo $?
> > > >   0
> > > >   # valid tree OID, but not a commit OID
> > > >   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
> > > >   $ echo $?
> > > >   0
> > > >   $ ls -l .git/objects/info/commit-graph
> > > >   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> > > >
> > > > 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?
>
> Because when I do:
>
>    # sometimes I forgot that refs are not good...
>    $ echo HEAD | git commit-graph write --stdin-commits
>
> then I get _nothing_: neither an error, nor a commit-graph.
>
> > > For reasons (see
> > > below), we want to do something like:
> > >
> > >   git for-each-ref --format='%(objectname)' |
> > >   git commit-graph write --stdin-commits
> > >
> > > In v2.23 and earlier, that worked exactly like --reachable, but now it
> > > will blow up if there are any refs that point to a non-commit (e.g., a
> > > tag of a blob).
> > >
> > > It can be worked around by asking for %(objecttype) and %(*objecttype)
> > > and grepping the result, but that's awkward and much less efficient
> > > (especially if you have a lot of annotated tags, as we may have to open
> > > and parse each one).
> > >
> > > Now obviously you could just use --reachable for the code above. But
> > > here are two plausible cases where you might not want to do that:
> > >
> > >  - you're limiting the graph to only a subset of refs (e.g., you want to
> > >    graph refs/heads/ and refs/tags, but not refs/some-other-weird-area/).
> > >
> > >  - 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).
> > >
> > > The latter is the actual case that bit us. I suppose one could do
> > > something like:
> > >
> > >   git rev-list --no-walk <maybe-commits |
> > >   git commit-graph write --stdin-commits
> > >
> > > to use rev-list as a filter, but that feels kind of baroque.
> > >
> > > 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).
> >
> > 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.
>
> Oh, no :)

> >   - 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.
> >
> > Please let me know if you find either of these to be good options, and
> > I'll happily send one of them to the list. Thanks.
>
> Or introduce 'git commit-graph write --stdin-refs'?  Or teach
> '--stdin-commits' to DWIM and accept and parse refs?  Though the
> question still remains what to do with refs that can't be peeled back
> to commits

> > > -Peff
> > >
> > > PS As an aside, I think the internal git-fetch write could benefit from
> > >    this same trick: feed the set of newly-stored ref tips to the
> > >    commit-graph machinery, rather than using for_each_ref().
> >
> > Thanks,
> > Taylor

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

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

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

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

> While 'git commit-graph write --stdin-commits' expects commit object
> ids as input, it accepts and silently skips over any invalid commit
> object ids, and still exits with success:
> 
>   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
>   $ echo $?