[PATCH v3 0/8] commit-graph: drop CHECK_OIDS, peel in callers
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 13, 2020, 21:59 UTC
- Message-ID
- <cover.1589407014.git.me@ttaylorr.com>
- In-Reply-To
- <cover.1588641176.git.me@ttaylorr.com>
Hi,
Here is a tiny reroll to my series to drop the commit-graph's behavior of complaining about non-commit OIDs on input with '--stdin-commits'. I promised this reroll after Peff's review beginning in [1].
Not a great deal has changed, but I mostly took every suggestion that Peff put forward (in the sub-thread beginning at [1]). The "big deals" since last time are:
- 'add_ref_to_set()' got cleaned up to use 'oid_object_info'.
- progress meter updates moved after their action
- 'graph_write()' got cleaned up to break the loop into two loops
executed conditionally (based on which of '--stdin-packs' or
'--stdin-commits' was passed) , as well as a cleanup label was
introduced and some UNLEAK cruft was cleaned up.For convenience, a range-diff since v2 (with the changes on master applied since then elided out) is included below.
Thanks again for your review.
-Taylor
[1]: https://lore.kernel.org/git/20200507195441.GA29683@coredump.intra.peff.net/
Taylor Blau (8): commit-graph.c: extract 'refs_cb_data' commit-graph.c: show progress of finding reachable commits commit-graph.c: peel refs in 'add_ref_to_set' builtin/commit-graph.c: extract 'read_one_commit()' builtin/commit-graph.c: dereference tags in builtin commit-graph.c: simplify 'fill_oids_from_commits' t5318: reorder test below 'graph_read_expect' commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag
Documentation/git-commit-graph.txt | 6 ++- builtin/commit-graph.c | 73 +++++++++++++++++++----------- commit-graph.c | 62 +++++++++++-------------- commit-graph.h | 4 +- t/t5318-commit-graph.sh | 25 ++++++---- 5 files changed, 95 insertions(+), 75 deletions(-)
Range-diff against v2:
1: 43286c3c45 = 60: 0efbcfcf3a commit-graph.c: extract 'refs_cb_data'
2: cb56a9a73b ! 61: 773522c745 commit-graph.c: show progress of finding reachable commits
@@ commit-graph.c: static void compute_bloom_filters(struct write_commit_graph_cont static int add_ref_to_set(const char *refname,
@@ commit-graph.c: static int add_ref_to_set(const char *refname,
- {
struct refs_cb_data *data = (struct refs_cb_data *)cb_data; -+ display_progress(data->progress, oidset_size(data->commits) + 1);
-+
oidset_insert(data->commits, oid);
++
++ display_progress(data->progress, oidset_size(data->commits));
++
return 0;
}
+
@@ commit-graph.c: int write_commit_graph_reachable(struct object_directory *odb, memset(&data, 0, sizeof(data));
3: 85c388a077 < -: ---------- commit-graph.c: peel refs in 'add_ref_to_set'
-: ---------- > 62: 74b424f1ac commit-graph.c: peel refs in 'add_ref_to_set'
4: cef441b465 ! 63: c37e94907b builtin/commit-graph.c: extract 'read_one_commit()'
@@ builtin/commit-graph.c: static int write_option_parse_split(const struct option
return 0;
} -+static int read_one_commit(struct oidset *commits, char *hash)
++static int read_one_commit(struct oidset *commits, const char *hash)
+{
+ struct object_id oid;
+ const char *end;
+
-+ if (parse_oid_hex(hash, &oid, &end)) {
-+ error(_("unexpected non-hex object ID: %s"), hash);
-+ return 1;
-+ }
++ if (parse_oid_hex(hash, &oid, &end))
++ return error(_("unexpected non-hex object ID: %s"), hash);
+
+ oidset_insert(commits, &oid);
+ return 0;
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
} - string_list_init(&lines, 0);
-+ string_list_init(&pack_indexes, 0);
- if (opts.stdin_packs || opts.stdin_commits) {
- struct strbuf buf = STRBUF_INIT;
--
-- while (strbuf_getline(&buf, stdin) != EOF)
+- if (opts.stdin_packs || opts.stdin_commits) {
+- struct strbuf buf = STRBUF_INIT;
++ struct strbuf buf = STRBUF_INIT;
++ if (opts.stdin_packs) {
++ string_list_init(&pack_indexes, 0);
+
+ while (strbuf_getline(&buf, stdin) != EOF)
- string_list_append(&lines, strbuf_detach(&buf, NULL));
--
++ string_list_append(&pack_indexes,
++ strbuf_detach(&buf, NULL));
++ } else if (opts.stdin_commits) {
++ oidset_init(&commits, 0);
++ flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
+
- if (opts.stdin_packs)
- pack_indexes = &lines;
- if (opts.stdin_commits) {
+- if (opts.stdin_commits) {
- struct string_list_item *item;
- oidset_init(&commits, lines.nr);
- for_each_string_list_item(item, &lines) {
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
- }
-
- oidset_insert(&commits, &oid);
-- }
-+ oidset_init(&commits, 0);
- flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
- }
-
+ while (strbuf_getline(&buf, stdin) != EOF) {
-+ char *line = strbuf_detach(&buf, NULL);
-+ if (opts.stdin_commits) {
-+ int result = read_one_commit(&commits, line);
-+ if (result)
-+ return result;
-+ } else
-+ string_list_append(&pack_indexes, line);
-+ }
-+
- UNLEAK(buf);
++ if (read_one_commit(&commits, buf.buf)) {
++ result = 1;
++ goto cleanup;
+ }
+- flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
+ }
+-
+- UNLEAK(buf);
} if (write_commit_graph(odb,
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
result = 1; - UNLEAK(lines);
++cleanup:
+ UNLEAK(pack_indexes);
++ strbuf_release(&buf);
return result;
} 5: d449d83ce2 < -: ---------- builtin/commit-graph.c: dereference tags in builtin
-: ---------- > 64: 56403dd377 builtin/commit-graph.c: dereference tags in builtin
6: 61887870c7 = 65: 6afb08a927 commit-graph.c: simplify 'fill_oids_from_commits'
7: e393b16097 = 66: 8fdd2792df t5318: reorder test below 'graph_read_expect'
8: ad373f05ff ! 67: 3cb0bd306c commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag
@@ Commit message
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
+ 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 <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com> ## Documentation/git-commit-graph.txt ##
@@ Documentation/git-commit-graph.txt: with `--stdin-commits` or `--reachable`.)
commits starting at all refs. (Cannot be combined with `--stdin-commits` ## builtin/commit-graph.c ##
-@@ builtin/commit-graph.c: static int read_one_commit(struct oidset *commits, struct progress *progress,
+@@
+ #include "commit-graph.h"
+ #include "object-store.h"
+ #include "progress.h"
++#include "tag.h" - display_progress(progress, oidset_size(commits) + 1);
+ static char const * const builtin_commit_graph_usage[] = {
+ N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
+@@ builtin/commit-graph.c: 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 (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;
- }
+ 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)); @@ builtin/commit-graph.c: 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(
- _("Collecting commits from input"), 0);
+ 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); ## commit-graph.c ##
@@ commit-graph.c: struct write_commit_graph_context {
@@ commit-graph.c: int write_commit_graph(struct object_directory *odb,
ctx->total_bloom_filter_data_size = 0; ## commit-graph.h ##
-@@ commit-graph.h: 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),
+@@ commit-graph.h: 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)
++ COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),
}; enum commit_graph_split_flags {
@@ t/t5318-commit-graph.sh: graph_read_expect() {
+ # 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 &&
++ test_i18ngrep "invalid object" stderr &&
+ # valid commit and tree OID
+ git rev-parse HEAD HEAD^{tree} >in &&
+ git commit-graph write --stdin-commits <in &&
--
2.26.0.113.ge9739cdccc