Re: [PATCH v3 4/8] builtin/commit-graph.c: extract 'read_one_commit()'
From: Taylor Blau <hidden>
Date: 2020-05-14 18:02:04
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, May 14, 2020 at 01:56:46PM -0400, Jeff King wrote:
On Wed, May 13, 2020 at 03:59:41PM -0600, Taylor Blau wrote:quoted
@@ -209,44 +220,35 @@ static int graph_write(int argc, const char **argv) return 0; } - string_list_init(&lines, 0); - if (opts.stdin_packs || opts.stdin_commits) { - struct strbuf buf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + if (opts.stdin_packs) {This is a decl-after-statement, isn't it? It should fail with DEVELOPER=1.
Whoops. I'm not sure how I missed this.
The strbuf could be local in each block of the conditional, though that makes your cleanup label trickier. Only the stdin_commits side needs to clean up the buf, so we could just do it there. Or I'd be OK with having the strbuf declared at the top of the function, and using the whole-function cleanup label.
Thankfully, fixing this by moving the declaration/initialization up to the top of the function didn't cause any conflicts with later patches. Junio: if you're queuing this reroll, please use the below version instead:
--- >8 ---
Subject: [PATCH] builtin/commit-graph.c: extract 'read_one_commit()' With either '--stdin-commits' or '--stdin-packs', the commit-graph builtin will read line-delimited input, and interpret it either as a series of commit OIDs, or pack names. In a subsequent commit, we will begin handling '--stdin-commits' differently by processing each line as it comes in, instead of in one shot at the end. To make adequate room for this additional logic, split the '--stdin-commits' case from '--stdin-packs' by only storing the input when '--stdin-packs' is given. In the case of '--stdin-commits', feed each line to a new 'read_one_commit' helper, which (for now) will merely call 'parse_oid_hex'. Signed-off-by: Taylor Blau <redacted> --- builtin/commit-graph.c | 56 ++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 15fe60317c..aaa1d0c738 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c@@ -138,12 +138,24 @@ static int write_option_parse_split(const struct option *opt, const char *arg, return 0; } +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)) + return error(_("unexpected non-hex object ID: %s"), hash); + + oidset_insert(commits, &oid); + return 0; +} + static int graph_write(int argc, const char **argv) { - struct string_list *pack_indexes = NULL; + struct strbuf buf = STRBUF_INIT; + struct string_list pack_indexes; struct oidset commits = OIDSET_INIT; struct object_directory *odb = NULL; - struct string_list lines; int result = 0; enum commit_graph_write_flags flags = 0;
@@ -209,44 +221,34 @@ static int graph_write(int argc, const char **argv) return 0; } - string_list_init(&lines, 0); - if (opts.stdin_packs || opts.stdin_commits) { - 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) { - struct string_list_item *item; - oidset_init(&commits, lines.nr); - for_each_string_list_item(item, &lines) { - struct object_id oid; - const char *end; - - if (parse_oid_hex(item->string, &oid, &end)) { - error(_("unexpected non-hex object ID: " - "%s"), item->string); - return 1; - } - - oidset_insert(&commits, &oid); + while (strbuf_getline(&buf, stdin) != EOF) { + if (read_one_commit(&commits, buf.buf)) { + result = 1; + goto cleanup; } - flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS; } - - UNLEAK(buf); } if (write_commit_graph(odb, - pack_indexes, + opts.stdin_packs ? &pack_indexes : NULL, opts.stdin_commits ? &commits : NULL, flags, &split_opts)) result = 1; - UNLEAK(lines); +cleanup: + UNLEAK(pack_indexes); + strbuf_release(&buf); return result; } --
2.26.0.113.ge9739cdccc