From 1fe10844cad39db18de18412c928a0e69b0067ae Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 4 May 2020 19:13:35 -0600 Subject: commit-graph.c: extract 'refs_cb_data' In subsequent patches, we are going to update a progress meter when 'add_ref_to_set()' is called, and need a convenient way to pass a 'struct progress *' in from the caller. Introduce 'refs_cb_data' as a catch-all for parameters that 'add_ref_to_set' may need, and wrap the existing single parameter in that struct. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- commit-graph.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'commit-graph.c') diff --git a/commit-graph.c b/commit-graph.c index 6dc777e2f3..00da281f39 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1318,13 +1318,17 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx) stop_progress(&progress); } +struct refs_cb_data { + struct oidset *commits; +}; + static int add_ref_to_set(const char *refname, const struct object_id *oid, int flags, void *cb_data) { - struct oidset *commits = (struct oidset *)cb_data; + struct refs_cb_data *data = (struct refs_cb_data *)cb_data; - oidset_insert(commits, oid); + oidset_insert(data->commits, oid); return 0; } @@ -1333,9 +1337,13 @@ int write_commit_graph_reachable(struct object_directory *odb, const struct split_commit_graph_opts *split_opts) { struct oidset commits = OIDSET_INIT; + struct refs_cb_data data; int result; - for_each_ref(add_ref_to_set, &commits); + memset(&data, 0, sizeof(data)); + data.commits = &commits; + + for_each_ref(add_ref_to_set, &data); result = write_commit_graph(odb, NULL, &commits, flags, split_opts); -- cgit 1.2.3-korg From d335ce8f24e335ffda911f01a9569f7132e64cdb Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 13 May 2020 15:59:33 -0600 Subject: commit-graph.c: show progress of finding reachable commits When 'git commit-graph write --reachable' is invoked, the commit-graph machinery calls 'for_each_ref()' to discover the set of reachable commits. Right now the 'add_ref_to_set' callback is not doing anything other than adding an OID to the set of known-reachable OIDs. In a subsequent commit, 'add_ref_to_set' will presumptively peel references. This operation should be fast for repositories with an up-to-date '$GIT_DIR/packed-refs', but may be slow in the general case. So that it doesn't appear that 'git commit-graph write' is idling with '--reachable' in the slow case, add a progress meter to provide some output in the meantime. In general, we don't expect a progress meter to appear at all, since peeling references with a 'packed-refs' file is quick. If it's slow and we do show a progress meter, the subsequent 'fill_oids_from_commits()' will be fast, since all of the calls to 'lookup_commit_reference_gently()' will be no-ops. Both progress meters are delayed, so it is unlikely that more than one will appear. In either case, this intermediate state will go away in a handful of patches, at which point there will be at most one progress meter. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- commit-graph.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'commit-graph.c') diff --git a/commit-graph.c b/commit-graph.c index 00da281f39..bd421c4f06 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1320,6 +1320,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx) struct refs_cb_data { struct oidset *commits; + struct progress *progress; }; static int add_ref_to_set(const char *refname, @@ -1329,6 +1330,9 @@ static int add_ref_to_set(const char *refname, struct refs_cb_data *data = (struct refs_cb_data *)cb_data; oidset_insert(data->commits, oid); + + display_progress(data->progress, oidset_size(data->commits)); + return 0; } @@ -1342,12 +1346,17 @@ int write_commit_graph_reachable(struct object_directory *odb, memset(&data, 0, sizeof(data)); data.commits = &commits; + if (flags & COMMIT_GRAPH_WRITE_PROGRESS) + data.progress = start_delayed_progress( + _("Collecting referenced commits"), 0); for_each_ref(add_ref_to_set, &data); result = write_commit_graph(odb, NULL, &commits, flags, split_opts); oidset_clear(&commits); + if (data.progress) + stop_progress(&data.progress); return result; } -- cgit 1.2.3-korg From 630cd5194e6fcf928290a5c0f85e4094559197c8 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 13 May 2020 15:59:37 -0600 Subject: commit-graph.c: peel refs in 'add_ref_to_set' While iterating references (to discover the set of commits to write to the commit-graph with 'git commit-graph write --reachable'), 'add_ref_to_set' can save 'fill_oids_from_commits()' some time by peeling the references beforehand. Move peeling out of 'fill_oids_from_commits()' and into 'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing so allows the commit-graph machinery to use the peeled value from '$GIT_DIR/packed-refs' instead of having to load and parse tags. While we're at it, discard non-commit objects reachable from ref tips. This would be done automatically by 'fill_oids_from_commits()', but such functionality will be removed in a subsequent patch after the call to 'lookup_commit_reference_gently' is dropped (at which point a non-commit object in the commits oidset will become an error). Suggested-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- commit-graph.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'commit-graph.c') diff --git a/commit-graph.c b/commit-graph.c index bd421c4f06..6098ecd575 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1327,9 +1327,13 @@ static int add_ref_to_set(const char *refname, const struct object_id *oid, int flags, void *cb_data) { + struct object_id peeled; struct refs_cb_data *data = (struct refs_cb_data *)cb_data; - oidset_insert(data->commits, oid); + if (!peel_ref(refname, &peeled)) + oid = &peeled; + if (oid_object_info(the_repository, oid, NULL) == OBJ_COMMIT) + oidset_insert(data->commits, oid); display_progress(data->progress, oidset_size(data->commits)); -- cgit 1.2.3-korg From 0ec2d0ff07f125b7dcf0fde2b508fa8d6d35e939 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 13 May 2020 15:59:47 -0600 Subject: commit-graph.c: simplify 'fill_oids_from_commits' In the previous handful of commits, both 'git commit-graph write --reachable' and '--stdin-commits' learned to peel tags down to the commits which they refer to before passing them into the commit-graph internals. This makes the call to 'lookup_commit_reference_gently()' inside of 'fill_oids_from_commits()' a noop, since all OIDs are commits by that point. As such, remove the call entirely, as well as the progress meter, which has been split and moved out to the callers in the aforementioned earlier commits. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- commit-graph.c | 33 +++------------------------------ 1 file changed, 3 insertions(+), 30 deletions(-) (limited to 'commit-graph.c') diff --git a/commit-graph.c b/commit-graph.c index 6098ecd575..aed03f4b2f 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1412,46 +1412,19 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx, static int fill_oids_from_commits(struct write_commit_graph_context *ctx, struct oidset *commits) { - uint32_t i = 0; - struct strbuf progress_title = STRBUF_INIT; struct oidset_iter iter; struct object_id *oid; if (!oidset_size(commits)) return 0; - if (ctx->report_progress) { - strbuf_addf(&progress_title, - Q_("Finding commits for commit graph from %d ref", - "Finding commits for commit graph from %d refs", - oidset_size(commits)), - oidset_size(commits)); - ctx->progress = start_delayed_progress( - progress_title.buf, - oidset_size(commits)); - } - oidset_iter_init(commits, &iter); while ((oid = oidset_iter_next(&iter))) { - struct commit *result; - - display_progress(ctx->progress, ++i); - - result = lookup_commit_reference_gently(ctx->r, oid, 1); - if (result) { - 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"), - oid_to_hex(oid)); - return -1; - } + ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc); + oidcpy(&ctx->oids.list[ctx->oids.nr], oid); + ctx->oids.nr++; } - stop_progress(&ctx->progress); - strbuf_release(&progress_title); - return 0; } -- cgit 1.2.3-korg From 2f00c355cb79ee86bddc9f2fef91ac380a6023fc Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 13 May 2020 15:59:55 -0600 Subject: commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag 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 "not-a-commit:"$1 } /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. Helped-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- Documentation/git-commit-graph.txt | 6 ++++-- builtin/commit-graph.c | 15 ++++++++------- commit-graph.c | 2 -- commit-graph.h | 4 +--- t/t5318-commit-graph.sh | 15 +++++++++++---- 5 files changed, 24 insertions(+), 18 deletions(-) (limited to 'commit-graph.c') 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 97eb3b72d6..75455da138 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -7,6 +7,7 @@ #include "commit-graph.h" #include "object-store.h" #include "progress.h" +#include "tag.h" static char const * const builtin_commit_graph_usage[] = { N_("git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]"), @@ -142,18 +143,19 @@ static int write_option_parse_split(const struct option *opt, const char *arg, static int read_one_commit(struct oidset *commits, struct progress *progress, const char *hash) { - struct commit *result; + struct object *result; struct object_id oid; const char *end; if (parse_oid_hex(hash, &oid, &end)) return error(_("unexpected non-hex object ID: %s"), hash); - result = lookup_commit_reference_gently(the_repository, &oid, 1); - if (result) - oidset_insert(commits, &result->object.oid); - else - return error(_("invalid commit object id: %s"), hash); + result = deref_tag(the_repository, parse_object(the_repository, &oid), + NULL, 0); + if (!result) + return error(_("invalid object: %s"), hash); + else if (object_as_type(the_repository, result, OBJ_COMMIT, 1)) + oidset_insert(commits, &result->oid); display_progress(progress, oidset_size(commits)); @@ -238,7 +240,6 @@ static int graph_write(int argc, const char **argv) strbuf_detach(&buf, NULL)); } else if (opts.stdin_commits) { oidset_init(&commits, 0); - flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS; if (opts.progress) progress = start_delayed_progress( _("Collecting commits from input"), 0); diff --git a/commit-graph.c b/commit-graph.c index aed03f4b2f..5df3e08718 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; @@ -2002,7 +2001,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..3ba0da1e5f 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -91,9 +91,7 @@ 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_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..e77244c39b 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 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 stderr && + test_i18ngrep "invalid object" stderr && + # valid commit and tree OID + git rev-parse HEAD HEAD^{tree} >in && + git commit-graph write --stdin-commits