| Message ID | 6c2d130b0cd4b6a8a541b362ae7dd44d4c282e3f.1588641176.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: drop CHECK_OIDS, peel in callers | expand |
On 5/4/2020 9:14 PM, Taylor Blau wrote: > Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in > 'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on > receiving non-commit OIDs as input to '--stdin-commits'. > > This behavior can be cumbersome to work around in, say, the case of > piping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if > the caller does not want to cull out non-commits themselves. In this > situation, it would be ideal if 'git commit-graph write' wrote the graph > containing the inputs that did pertain to commits, and silently ignored > the remainder of the input. > > Some options have been proposed to the effect of '--[no-]check-oids' > which would allow callers to have the commit-graph builtin do just that. > After some discussion, it is difficult to imagine a caller who wouldn't > want to pass '--no-check-oids', suggesting that we should get rid of the > behavior of complaining about non-commit inputs altogether. > > If callers do wish to retain this behavior, they can easily work around > this change by doing the following: > > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | > awk '/commit/ { print $1 }' | > git commit-graph write --stdin-commits I appreciate that you included the workaround here for posterity. That allows anyone complaining to bisect to these instructions. > To make it so that valid OIDs that refer to non-existent objects are > indeed an error after loosening the error handling, perform an extra > lookup to make sure that object indeed exists before sending it to the > commit-graph internals. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > Documentation/git-commit-graph.txt | 6 ++++-- > builtin/commit-graph.c | 10 +++++----- > commit-graph.c | 2 -- > commit-graph.h | 10 ++++------ > t/t5318-commit-graph.sh | 15 +++++++++++---- > 5 files changed, 24 insertions(+), 19 deletions(-) > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > index 53a650225a..fcac7d12e1 100644 > --- a/Documentation/git-commit-graph.txt > +++ b/Documentation/git-commit-graph.txt > @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.) > + > With the `--stdin-commits` option, generate the new commit graph by > walking commits starting at the commits specified in stdin as a list > -of OIDs in hex, one OID per line. (Cannot be combined with > -`--stdin-packs` or `--reachable`.) > +of OIDs in hex, one OID per line. OIDs that resolve to non-commits > +(either directly, or by peeling tags) are silently ignored. OIDs that > +are malformed, or do not exist generate an error. (Cannot be combined > +with `--stdin-packs` or `--reachable`.) > + > With the `--reachable` option, generate the new commit graph by walking > commits starting at all refs. (Cannot be combined with `--stdin-commits` > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 9eec68572f..3637d079fb 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress, > > display_progress(progress, oidset_size(commits) + 1); > > + if (oid_object_info(the_repository, &oid, NULL) < 0) { > + error(_("object %s does not exist"), hash); > + return 1; > + } > + If we get a non-existent object, then this will cause us to fail the command, right? > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > index 89020d3d44..74f93f0a17 100755 > --- a/t/t5318-commit-graph.sh > +++ b/t/t5318-commit-graph.sh > @@ -84,11 +84,18 @@ graph_read_expect() { > > test_expect_success 'exit with correct error on bad input to --stdin-commits' ' > cd "$TRASH_DIRECTORY/full" && > - echo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr && > + # invalid, non-hex OID > + echo HEAD >in && > + test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr && > test_i18ngrep "unexpected non-hex object ID: HEAD" stderr && > - # valid tree OID, but not a commit OID > - git rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr && > - test_i18ngrep "invalid commit object id" stderr > + # non-existent OID > + echo $ZERO_OID >in && > + test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr && > + test_i18ngrep "does not exist" stderr && And here you verify that it fails at that point. Excellent! > + # valid commit and tree OID > + git rev-parse HEAD HEAD^{tree} >in && > + git commit-graph write --stdin-commits <in && > + graph_read_expect 3 > ' This is an excellent series! Thanks. Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
On Tue, May 05, 2020 at 08:10:40AM -0400, Derrick Stolee wrote: > On 5/4/2020 9:14 PM, Taylor Blau wrote: > > Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in > > 'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on > > receiving non-commit OIDs as input to '--stdin-commits'. > > > > This behavior can be cumbersome to work around in, say, the case of > > piping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if > > the caller does not want to cull out non-commits themselves. In this > > situation, it would be ideal if 'git commit-graph write' wrote the graph > > containing the inputs that did pertain to commits, and silently ignored > > the remainder of the input. > > > > Some options have been proposed to the effect of '--[no-]check-oids' > > which would allow callers to have the commit-graph builtin do just that. > > After some discussion, it is difficult to imagine a caller who wouldn't > > want to pass '--no-check-oids', suggesting that we should get rid of the > > behavior of complaining about non-commit inputs altogether. > > > > If callers do wish to retain this behavior, they can easily work around > > this change by doing the following: > > > > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | > > awk '/commit/ { print $1 }' | > > git commit-graph write --stdin-commits > > I appreciate that you included the workaround here for posterity. That > allows anyone complaining to bisect to these instructions. Well, now that I linked provided a link to [2] in the cover letter, you'll see that it was Peff's suggestion ;). But, I wrote something similar to this in one of GitHub's post-processing jobs before we had this series, so I think I can take some of the credit! > > To make it so that valid OIDs that refer to non-existent objects are > > indeed an error after loosening the error handling, perform an extra > > lookup to make sure that object indeed exists before sending it to the > > commit-graph internals. > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > Documentation/git-commit-graph.txt | 6 ++++-- > > builtin/commit-graph.c | 10 +++++----- > > commit-graph.c | 2 -- > > commit-graph.h | 10 ++++------ > > t/t5318-commit-graph.sh | 15 +++++++++++---- > > 5 files changed, 24 insertions(+), 19 deletions(-) > > > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > > index 53a650225a..fcac7d12e1 100644 > > --- a/Documentation/git-commit-graph.txt > > +++ b/Documentation/git-commit-graph.txt > > @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.) > > + > > With the `--stdin-commits` option, generate the new commit graph by > > walking commits starting at the commits specified in stdin as a list > > -of OIDs in hex, one OID per line. (Cannot be combined with > > -`--stdin-packs` or `--reachable`.) > > +of OIDs in hex, one OID per line. OIDs that resolve to non-commits > > +(either directly, or by peeling tags) are silently ignored. OIDs that > > +are malformed, or do not exist generate an error. (Cannot be combined > > +with `--stdin-packs` or `--reachable`.) > > + > > With the `--reachable` option, generate the new commit graph by walking > > commits starting at all refs. (Cannot be combined with `--stdin-commits` > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > > index 9eec68572f..3637d079fb 100644 > > --- a/builtin/commit-graph.c > > +++ b/builtin/commit-graph.c > > @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress, > > > > display_progress(progress, oidset_size(commits) + 1); > > > > + if (oid_object_info(the_repository, &oid, NULL) < 0) { > > + error(_("object %s does not exist"), hash); > > + return 1; > > + } > > + > > If we get a non-existent object, then this will cause us to fail > the command, right? > > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > > index 89020d3d44..74f93f0a17 100755 > > --- a/t/t5318-commit-graph.sh > > +++ b/t/t5318-commit-graph.sh > > @@ -84,11 +84,18 @@ graph_read_expect() { > > > > test_expect_success 'exit with correct error on bad input to --stdin-commits' ' > > cd "$TRASH_DIRECTORY/full" && > > - echo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr && > > + # invalid, non-hex OID > > + echo HEAD >in && > > + test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr && > > test_i18ngrep "unexpected non-hex object ID: HEAD" stderr && > > - # valid tree OID, but not a commit OID > > - git rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr && > > - test_i18ngrep "invalid commit object id" stderr > > + # non-existent OID > > + echo $ZERO_OID >in && > > + test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr && > > + test_i18ngrep "does not exist" stderr && > > And here you verify that it fails at that point. Excellent! > > > + # valid commit and tree OID > > + git rev-parse HEAD HEAD^{tree} >in && > > + git commit-graph write --stdin-commits <in && > > + graph_read_expect 3 > > ' > > This is an excellent series! Thanks. > > Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Thanks for your review. I applied your suggested changes locally (which came down to tweaking the progress meter's messages in the 2nd and 5th patches), but I'll sit on them for a day or so to make sure that nobody else has any thoughts before sending a v2. Thanks, Taylor
On Mon, May 04, 2020 at 07:14:03PM -0600, Taylor Blau wrote: > If callers do wish to retain this behavior, they can easily work around > this change by doing the following: > > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | > awk '/commit/ { print $1 }' | > git commit-graph write --stdin-commits I know this came from my earlier email, but I think that recipe actually shows how to make your input work even if --check-oids were the default. If you really want the --check-oids behavior, you'd want the opposite: to complain about those ones. So it's something like: git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | awk ' !/commit/ { print "not-a-commit:"$1 } /commit/ { print $1 } ' | git commit-graph write --stdin-commits > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > index 53a650225a..fcac7d12e1 100644 > --- a/Documentation/git-commit-graph.txt > +++ b/Documentation/git-commit-graph.txt > @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.) > + > With the `--stdin-commits` option, generate the new commit graph by > walking commits starting at the commits specified in stdin as a list > -of OIDs in hex, one OID per line. (Cannot be combined with > -`--stdin-packs` or `--reachable`.) > +of OIDs in hex, one OID per line. OIDs that resolve to non-commits > +(either directly, or by peeling tags) are silently ignored. OIDs that > +are malformed, or do not exist generate an error. (Cannot be combined > +with `--stdin-packs` or `--reachable`.) Yeah, I think these semantics are good. > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 9eec68572f..3637d079fb 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress, > > display_progress(progress, oidset_size(commits) + 1); > > + if (oid_object_info(the_repository, &oid, NULL) < 0) { > + error(_("object %s does not exist"), hash); > + return 1; > + } > + > result = lookup_commit_reference_gently(the_repository, &oid, 1); > if (result) > oidset_insert(commits, &result->object.oid); > - else { > - error(_("invalid commit object id: %s"), hash); > - return 1; > - } > return 0; > } We can avoid the object-existence check entirely if lookup_commit_reference_gently() gives us an answer. And we'd expect that to be the common path. Also, using has_object_file() is cheaper than oid_object_info(), since it doesn't have to resolve the type for deltas. So perhaps: result = lookup_commit_reference_gently(...); if (result) oidset_insert(...); else if (has_object_file(&oid)) ; /* not a commit; quietly ignore; else return error(no such object...); That said, I think this technique misses some cases of corruption. You're checking that the outer-most object exists, but not any intermediate peeled objects. I.e., lookup_commit_reference_gently() might have failed for two reasons: - an object it peeled to didn't exist - an object it peeled to wasn't a commit To do it thoroughly, I think you'd have to call deref_tag() yourself and distinguish NULL there (an error) from a result where obj->type isn't OBJ_COMMIT (quietly ignore). > enum commit_graph_write_flags { > - COMMIT_GRAPH_WRITE_APPEND = (1 << 0), > - COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), > - COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), > - /* Make sure that each OID in the input is a valid commit OID. */ > - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3), > - COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4), > + COMMIT_GRAPH_WRITE_APPEND = (1 << 0), > + COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), > + COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), > + COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3) As much as I love looking at matched-indentation lists, I think this diff is a good example of why it's not worth doing. It's much easier to see what's going on if the first three items aren't touched. I'd actually even leave BLOOM_FILTERS where it is, and accept the hole which could be refilled later. Your patch also loses the trailing comma after the final BLOOM_FILTERS entry. -Peff
On Thu, May 07, 2020 at 04:40:05PM -0400, Jeff King wrote: > On Mon, May 04, 2020 at 07:14:03PM -0600, Taylor Blau wrote: > > > If callers do wish to retain this behavior, they can easily work around > > this change by doing the following: > > > > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | > > awk '/commit/ { print $1 }' | > > git commit-graph write --stdin-commits > > I know this came from my earlier email, but I think that recipe actually > shows how to make your input work even if --check-oids were the default. > If you really want the --check-oids behavior, you'd want the opposite: > to complain about those ones. So it's something like: > > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | > awk ' > !/commit/ { print "not-a-commit:"$1 } > /commit/ { print $1 } > ' | > git commit-graph write --stdin-commits > > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > > index 53a650225a..fcac7d12e1 100644 > > --- a/Documentation/git-commit-graph.txt > > +++ b/Documentation/git-commit-graph.txt > > @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.) > > + > > With the `--stdin-commits` option, generate the new commit graph by > > walking commits starting at the commits specified in stdin as a list > > -of OIDs in hex, one OID per line. (Cannot be combined with > > -`--stdin-packs` or `--reachable`.) > > +of OIDs in hex, one OID per line. OIDs that resolve to non-commits > > +(either directly, or by peeling tags) are silently ignored. OIDs that > > +are malformed, or do not exist generate an error. (Cannot be combined > > +with `--stdin-packs` or `--reachable`.) > > Yeah, I think these semantics are good. > > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > > index 9eec68572f..3637d079fb 100644 > > --- a/builtin/commit-graph.c > > +++ b/builtin/commit-graph.c > > @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress, > > > > display_progress(progress, oidset_size(commits) + 1); > > > > + if (oid_object_info(the_repository, &oid, NULL) < 0) { > > + error(_("object %s does not exist"), hash); > > + return 1; > > + } > > + > > result = lookup_commit_reference_gently(the_repository, &oid, 1); > > if (result) > > oidset_insert(commits, &result->object.oid); > > - else { > > - error(_("invalid commit object id: %s"), hash); > > - return 1; > > - } > > return 0; > > } > > We can avoid the object-existence check entirely if > lookup_commit_reference_gently() gives us an answer. And we'd expect > that to be the common path. > > Also, using has_object_file() is cheaper than oid_object_info(), since > it doesn't have to resolve the type for deltas. > > So perhaps: > > result = lookup_commit_reference_gently(...); > if (result) > oidset_insert(...); > else if (has_object_file(&oid)) > ; /* not a commit; quietly ignore; > else > return error(no such object...); > > That said, I think this technique misses some cases of corruption. > You're checking that the outer-most object exists, but not any > intermediate peeled objects. I.e., lookup_commit_reference_gently() > might have failed for two reasons: > > - an object it peeled to didn't exist > > - an object it peeled to wasn't a commit > > To do it thoroughly, I think you'd have to call deref_tag() yourself and > distinguish NULL there (an error) from a result where obj->type isn't > OBJ_COMMIT (quietly ignore). Makes sense. I initially worried a little bit about what to call the error message (i.e., is is "this object doesn't exist" or "this object exists but peels to a non-commit, or an otherwise missing object"). But, I think just saying "invalid object: <hash>" is good enough here. > > enum commit_graph_write_flags { > > - COMMIT_GRAPH_WRITE_APPEND = (1 << 0), > > - COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), > > - COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), > > - /* Make sure that each OID in the input is a valid commit OID. */ > > - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3), > > - COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4), > > + COMMIT_GRAPH_WRITE_APPEND = (1 << 0), > > + COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), > > + COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), > > + COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3) > > As much as I love looking at matched-indentation lists, I think this > diff is a good example of why it's not worth doing. It's much easier to > see what's going on if the first three items aren't touched. I'd > actually even leave BLOOM_FILTERS where it is, and accept the hole which > could be refilled later. > > Your patch also loses the trailing comma after the final BLOOM_FILTERS > entry. Fixed both, thanks. I'll build these eight new patches and send them shortly, thanks for your review. > -Peff Thanks, Taylor
diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt index 53a650225a..fcac7d12e1 100644 --- a/Documentation/git-commit-graph.txt +++ b/Documentation/git-commit-graph.txt @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.) + With the `--stdin-commits` option, generate the new commit graph by walking commits starting at the commits specified in stdin as a list -of OIDs in hex, one OID per line. (Cannot be combined with -`--stdin-packs` or `--reachable`.) +of OIDs in hex, one OID per line. OIDs that resolve to non-commits +(either directly, or by peeling tags) are silently ignored. OIDs that +are malformed, or do not exist generate an error. (Cannot be combined +with `--stdin-packs` or `--reachable`.) + With the `--reachable` option, generate the new commit graph by walking commits starting at all refs. (Cannot be combined with `--stdin-commits` diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 9eec68572f..3637d079fb 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress, display_progress(progress, oidset_size(commits) + 1); + if (oid_object_info(the_repository, &oid, NULL) < 0) { + error(_("object %s does not exist"), hash); + return 1; + } + result = lookup_commit_reference_gently(the_repository, &oid, 1); if (result) oidset_insert(commits, &result->object.oid); - else { - error(_("invalid commit object id: %s"), hash); - return 1; - } return 0; } @@ -239,7 +240,6 @@ static int graph_write(int argc, const char **argv) struct strbuf buf = STRBUF_INIT; if (opts.stdin_commits) { oidset_init(&commits, 0); - flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS; if (opts.progress) progress = start_delayed_progress( _("Analyzing commits from stdin"), 0); diff --git a/commit-graph.c b/commit-graph.c index 24c2d80935..4e5539bac5 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -880,7 +880,6 @@ struct write_commit_graph_context { unsigned append:1, report_progress:1, split:1, - check_oids:1, changed_paths:1, order_by_pack:1; @@ -2001,7 +2000,6 @@ int write_commit_graph(struct object_directory *odb, 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; ctx->changed_paths = flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS ? 1 : 0; ctx->total_bloom_filter_data_size = 0; diff --git a/commit-graph.h b/commit-graph.h index 4212766a4f..659393e07f 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -88,12 +88,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size); int generation_numbers_enabled(struct repository *r); enum commit_graph_write_flags { - COMMIT_GRAPH_WRITE_APPEND = (1 << 0), - COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), - COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), - /* Make sure that each OID in the input is a valid commit OID. */ - COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3), - COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4), + COMMIT_GRAPH_WRITE_APPEND = (1 << 0), + COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), + COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), + COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3) }; enum commit_graph_split_flags { diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 89020d3d44..74f93f0a17 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -84,11 +84,18 @@ graph_read_expect() { test_expect_success 'exit with correct error on bad input to --stdin-commits' ' cd "$TRASH_DIRECTORY/full" && - echo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr && + # invalid, non-hex OID + echo HEAD >in && + test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr && test_i18ngrep "unexpected non-hex object ID: HEAD" stderr && - # valid tree OID, but not a commit OID - git rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr && - test_i18ngrep "invalid commit object id" stderr + # non-existent OID + echo $ZERO_OID >in && + test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr && + test_i18ngrep "does not exist" stderr && + # valid commit and tree OID + git rev-parse HEAD HEAD^{tree} >in && + git commit-graph write --stdin-commits <in && + graph_read_expect 3 ' test_expect_success 'write graph' '
Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in 'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on receiving non-commit OIDs as input to '--stdin-commits'. This behavior can be cumbersome to work around in, say, the case of piping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if the caller does not want to cull out non-commits themselves. In this situation, it would be ideal if 'git commit-graph write' wrote the graph containing the inputs that did pertain to commits, and silently ignored the remainder of the input. Some options have been proposed to the effect of '--[no-]check-oids' which would allow callers to have the commit-graph builtin do just that. After some discussion, it is difficult to imagine a caller who wouldn't want to pass '--no-check-oids', suggesting that we should get rid of the behavior of complaining about non-commit inputs altogether. If callers do wish to retain this behavior, they can easily work around this change by doing the following: git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' | awk '/commit/ { print $1 }' | git commit-graph write --stdin-commits To make it so that valid OIDs that refer to non-existent objects are indeed an error after loosening the error handling, perform an extra lookup to make sure that object indeed exists before sending it to the commit-graph internals. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-commit-graph.txt | 6 ++++-- builtin/commit-graph.c | 10 +++++----- commit-graph.c | 2 -- commit-graph.h | 10 ++++------ t/t5318-commit-graph.sh | 15 +++++++++++---- 5 files changed, 24 insertions(+), 19 deletions(-)