From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: dstolee@microsoft.com, gitster@pobox.com, peff@peff.net,
szeder.dev@gmail.com
Subject: [PATCH v3 0/8] commit-graph: drop CHECK_OIDS, peel in callers
Date: Wed, 13 May 2020 15:59:25 -0600 [thread overview]
Message-ID: <cover.1589407014.git.me@ttaylorr.com> (raw)
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
next prev parent reply other threads:[~2020-05-13 21:59 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-05 1:13 [PATCH 0/8] commit-graph: drop CHECK_OIDS, peel in callers Taylor Blau
2020-05-05 1:13 ` [PATCH 1/8] commit-graph.c: extract 'refs_cb_data' Taylor Blau
2020-05-05 1:13 ` [PATCH 2/8] commit-graph.c: show progress of finding reachable commits Taylor Blau
2020-05-05 11:50 ` Derrick Stolee
2020-05-05 16:13 ` Taylor Blau
2020-05-05 1:13 ` [PATCH 3/8] commit-graph.c: peel refs in 'add_ref_to_set' Taylor Blau
2020-05-07 19:54 ` Jeff King
2020-05-13 19:48 ` Taylor Blau
2020-05-13 20:17 ` Jeff King
2020-05-05 1:13 ` [PATCH 4/8] builtin/commit-graph.c: extract 'read_one_commit()' Taylor Blau
2020-05-07 20:03 ` Jeff King
2020-05-13 20:01 ` Taylor Blau
2020-05-05 1:13 ` [PATCH 5/8] builtin/commit-graph.c: dereference tags in builtin Taylor Blau
2020-05-05 12:01 ` Derrick Stolee
2020-05-05 16:14 ` Taylor Blau
2020-05-07 20:14 ` Jeff King
2020-05-05 1:13 ` [PATCH 6/8] commit-graph.c: simplify 'fill_oids_from_commits' Taylor Blau
2020-05-05 12:05 ` Derrick Stolee
2020-05-07 20:21 ` Jeff King
2020-05-05 1:14 ` [PATCH 7/8] t5318: reorder test below 'graph_read_expect' Taylor Blau
2020-05-05 1:14 ` [PATCH 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag Taylor Blau
2020-05-05 12:10 ` Derrick Stolee
2020-05-05 16:16 ` Taylor Blau
2020-05-07 20:40 ` Jeff King
2020-05-13 21:32 ` Taylor Blau
2020-05-05 11:35 ` [PATCH 0/8] commit-graph: drop CHECK_OIDS, peel in callers Derrick Stolee
2020-05-05 16:11 ` Taylor Blau
2020-05-06 0:07 ` [PATCH v2 " Taylor Blau
2020-05-06 0:07 ` [PATCH v2 1/8] commit-graph.c: extract 'refs_cb_data' Taylor Blau
2020-05-06 0:07 ` [PATCH v2 2/8] commit-graph.c: show progress of finding reachable commits Taylor Blau
2020-05-06 0:07 ` [PATCH v2 3/8] commit-graph.c: peel refs in 'add_ref_to_set' Taylor Blau
2020-05-06 0:07 ` [PATCH v2 4/8] builtin/commit-graph.c: extract 'read_one_commit()' Taylor Blau
2020-05-06 0:07 ` [PATCH v2 5/8] builtin/commit-graph.c: dereference tags in builtin Taylor Blau
2020-05-06 0:07 ` [PATCH v2 6/8] commit-graph.c: simplify 'fill_oids_from_commits' Taylor Blau
2020-05-06 0:07 ` [PATCH v2 7/8] t5318: reorder test below 'graph_read_expect' Taylor Blau
2020-05-06 0:07 ` [PATCH v2 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag Taylor Blau
2020-05-07 20:42 ` [PATCH v2 0/8] commit-graph: drop CHECK_OIDS, peel in callers Jeff King
2020-05-13 21:59 ` Taylor Blau [this message]
2020-05-13 21:59 ` [PATCH v3 1/8] commit-graph.c: extract 'refs_cb_data' Taylor Blau
2020-05-13 21:59 ` [PATCH v3 2/8] commit-graph.c: show progress of finding reachable commits Taylor Blau
2020-05-13 21:59 ` [PATCH v3 3/8] commit-graph.c: peel refs in 'add_ref_to_set' Taylor Blau
2020-05-13 21:59 ` [PATCH v3 4/8] builtin/commit-graph.c: extract 'read_one_commit()' Taylor Blau
2020-05-14 17:56 ` Jeff King
2020-05-14 18:02 ` Taylor Blau
2020-05-14 18:27 ` Junio C Hamano
2020-05-18 19:27 ` Taylor Blau
2020-05-13 21:59 ` [PATCH v3 5/8] builtin/commit-graph.c: dereference tags in builtin Taylor Blau
2020-05-14 18:01 ` Jeff King
2020-05-14 18:04 ` Taylor Blau
2020-07-10 19:02 ` [PATCH] commit-graph: fix "Collecting commits from input" progress line SZEDER Gábor
2020-07-10 19:17 ` Taylor Blau
2020-07-15 18:33 ` SZEDER Gábor
2020-07-15 18:43 ` Derrick Stolee
2020-07-15 18:58 ` Junio C Hamano
2020-05-13 21:59 ` [PATCH v3 6/8] commit-graph.c: simplify 'fill_oids_from_commits' Taylor Blau
2020-05-13 21:59 ` [PATCH v3 7/8] t5318: reorder test below 'graph_read_expect' Taylor Blau
2020-05-13 21:59 ` [PATCH v3 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag Taylor Blau
2020-05-14 18:09 ` Jeff King
2020-05-14 18:12 ` [PATCH v3 0/8] commit-graph: drop CHECK_OIDS, peel in callers Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1589407014.git.me@ttaylorr.com \
--to=me@ttaylorr.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.