{"thread":{"id":"53395","subject":"[PATCH 0/8] commit-graph: drop CHECK_OIDS, peel in callers","startedAt":"2020-05-05T01:13:37Z","lastAt":"2020-07-15T18:58:40Z","messageCount":59,"participants":["Taylor Blau","Derrick Stolee","Jeff King","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"397029","messageId":"cover.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":null,"subject":"[PATCH 0/8] commit-graph: drop CHECK_OIDS, peel in callers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:31Z","receivedAt":"2020-05-05T01:13:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nThese are a few patches to address [1] by getting rid of the\n'COMMIT_GRAPH_WRITE_CHECK_OIDS' flag. A more complete discussion about\nwhy we might want to get rid of that flag can be found in the second\npatch, as well as in [1], but the basic gist is as follows:\n\n - It is often convenient to pipe the output of 'git for-each-ref' into\n 'git commit-graph write --stdin-commits', but handling multi-layer\n tags is frustrating (at the very least, you have to peel them\n yourself in 'for-each-ref', incurring a higher-than-necessary cost\n outside of the commit-graph invocation)\n\n - It would be much more convenient if 'git commit-graph write\n --stdin-commits' ignored OIDs that peel down to non-commit objects\n silently, while still warning the caller about objects that (a)\n don't exist, or (b) invalid hash inputs (for e.g., \"HEAD\", or\n \"refs/heads/foo\").\n\n - By making this change, we are allowing *more* possible behaviors\n than in previous versions, and the only case that we are breaking\n (that 'git commit-graph write --stdin-commits' complains on\n non-commit OIDs) can be addressed by peeling outside of\n 'commit-graph write' [2].\n\nThe first six patches move peeling refs out from the commit-graph\ninternals, and into the callers.\n\nThis has no impact on the '--stdin-commits' case, but is a potential\nimprovement for '--reachable', where it is more efficient to call\n'peel_ref()' than 'lookup_commit_reference_gently()'. There is an\nintermediate state (that is resolved in the final patch) where we can\nhave more than one progress meter (since the progress meter moved with\nthe perhaps-expensive operations out of the commit-graph internals,\ntoo), but the final patch addresses this.\n\nThe last two patches lift the restriction on input to '--stdin-commits'\nresolving to commit objects, instead making the behavior to silently\ndrop non-commit OIDs, and continue complain about invalid OIDs, and\nvalid-but-missing OIDs.\n\nFinally, this topic isn't based on anything that isn't already in\n'master', so I think the days of many tangle commit-graph topics are\nbehind us.\n\nThanks in advance for your review.\n\nThanks,\nTaylor\n\nTaylor Blau (8):\n commit-graph.c: extract 'refs_cb_data'\n commit-graph.c: show progress of finding reachable commits\n commit-graph.c: peel refs in 'add_ref_to_set'\n builtin/commit-graph.c: extract 'read_one_commit()'\n builtin/commit-graph.c: dereference tags in builtin\n commit-graph.c: simplify 'fill_oids_from_commits'\n t5318: reorder test below 'graph_read_expect'\n commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag\n\n Documentation/git-commit-graph.txt | 6 ++-\n builtin/commit-graph.c | 73 ++++++++++++++++++++----------\n commit-graph.c | 61 +++++++++++--------------\n commit-graph.h | 10 ++--\n t/t5318-commit-graph.sh | 25 ++++++----\n 5 files changed, 98 insertions(+), 77 deletions(-)\n\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"397030","messageId":"43286c3c45b2dcef8d9d39b4998c17f364eb1060.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 1/8] commit-graph.c: extract 'refs_cb_data'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:35Z","receivedAt":"2020-05-05T01:13:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In subsequent patches, we are going to update a progress meter when\n'add_ref_to_set()' is called, and need a convenient way to pass a\n'struct progress *' in from the caller.\n\nIntroduce 'refs_cb_data' as a catch-all for parameters that\n'add_ref_to_set' may need, and wrap the existing single parameter in\nthat struct.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6dc777e2f3..00da281f39 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1318,13 +1318,17 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \tstop_progress(&progress);\n }\n \n+struct refs_cb_data {\n+\tstruct oidset *commits;\n+};\n+\n static int add_ref_to_set(const char *refname,\n \t\t\t const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n-\tstruct oidset *commits = (struct oidset *)cb_data;\n+\tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n-\toidset_insert(commits, oid);\n+\toidset_insert(data->commits, oid);\n \treturn 0;\n }\n \n@@ -1333,9 +1337,13 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n \tstruct oidset commits = OIDSET_INIT;\n+\tstruct refs_cb_data data;\n \tint result;\n \n-\tfor_each_ref(add_ref_to_set, &commits);\n+\tmemset(&data, 0, sizeof(data));\n+\tdata.commits = &commits;\n+\n+\tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397031","messageId":"5bdbeaf374b6050670f800fcdd3b54ddd0750754.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 2/8] commit-graph.c: show progress of finding reachable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:39Z","receivedAt":"2020-05-05T01:13:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When 'git commit-graph write --reachable' is invoked, the commit-graph\nmachinery calls 'for_each_ref()' to discover the set of reachable\ncommits.\n\nRight now the 'add_ref_to_set' callback is not doing anything other than\nadding an OID to the set of known-reachable OIDs. In a subsequent\ncommit, 'add_ref_to_set' will presumptively peel references. This\noperation should be fast for repositories with an up-to-date\n'$GIT_DIR/packed-refs', but may be slow in the general case.\n\nSo that it doesn't appear that 'git commit-graph write' is idling with\n'--reachable' in the slow case, add a progress meter to provide some\noutput in the meantime.\n\nIn general, we don't expect a progress meter to appear at all, since\npeeling references with a 'packed-refs' file is quick. If it's slow and\nwe do show a progress meter, the subsequent 'fill_oids_from_commits()'\nwill be fast, since all of the calls to\n'lookup_commit_reference_gently()' will be no-ops.\n\nBoth progress meters are delayed, so it is unlikely that more than one\nwill appear. In either case, this intermediate state will go away in a\nhandful of patches, at which point there will be at most one progress\nmeter.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 00da281f39..8f61256b0a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1320,6 +1320,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \n struct refs_cb_data {\n \tstruct oidset *commits;\n+\tstruct progress *progress;\n };\n \n static int add_ref_to_set(const char *refname,\n@@ -1328,6 +1329,8 @@ static int add_ref_to_set(const char *refname,\n {\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n+\tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n+\n \toidset_insert(data->commits, oid);\n \treturn 0;\n }\n@@ -1342,12 +1345,17 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.commits = &commits;\n+\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n+\t\tdata.progress = start_delayed_progress(\n+\t\t\t_(\"Finding reachable commits\"), 0);\n \n \tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n \toidset_clear(&commits);\n+\tif (data.progress)\n+\t\tstop_progress(&data.progress);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397032","messageId":"5ff56feab55b005b4a4d9559909ce7a08e5fa81e.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 3/8] commit-graph.c: peel refs in 'add_ref_to_set'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:43Z","receivedAt":"2020-05-05T01:13:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While iterating references (to discover the set of commits to write to\nthe commit-graph with 'git commit-graph write --reachable'),\n'add_ref_to_set' can save 'fill_oids_from_commits()' some time by\npeeling the references beforehand.\n\nMove peeling out of 'fill_oids_from_commits()' and into\n'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing\nso allows the commit-graph machinery to use the peeled value from\n'$GIT_DIR/packed-refs' instead of having to load and parse tags.\n\nWhile we're at it, discard non-commit objects reachable from ref tips.\nThis would be done automatically by 'fill_oids_from_commits()', but such\nfunctionality will be removed in a subsequent patch after the call to\n'lookup_commit_reference_gently' is dropped (at which point a non-commit\nobject in the commits oidset will become an error).\n\nSuggested-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 8f61256b0a..5c3fad0dd7 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1327,11 +1327,15 @@ static int add_ref_to_set(const char *refname,\n \t\t\t const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n+\tstruct object_id peeled;\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n \tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n \n-\toidset_insert(data->commits, oid);\n+\tif (peel_ref(refname, &peeled))\n+\t\tpeeled = *oid;\n+\tif (oid_object_info(the_repository, &peeled, NULL) == OBJ_COMMIT)\n+\t\toidset_insert(data->commits, &peeled);\n \treturn 0;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397033","messageId":"9ae8745dc090de37af0475ab12b79d541a52713d.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:46Z","receivedAt":"2020-05-05T01:13:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"With either '--stdin-commits' or '--stdin-packs', the commit-graph\nbuiltin will read line-delimited input, and interpret it either as a\nseries of commit OIDs, or pack names.\n\nIn a subsequent commit, we will begin handling '--stdin-commits'\ndifferently by processing each line as it comes in, instead of in one\nshot at the end. To make adequate room for this additional logic, split\nthe '--stdin-commits' case from '--stdin-packs' by only storing the\ninput when '--stdin-packs' is given.\n\nIn the case of '--stdin-commits', feed each line to a new\n'read_one_commit' helper, which (for now) will merely call\n'parse_oid_hex'.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 54 +++++++++++++++++++++++-------------------\n 1 file changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 15fe60317c..f550d8489a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -138,12 +138,25 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static int read_one_commit(struct oidset *commits, char *hash)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\n+\tif (parse_oid_hex(hash, &oid, &end)) {\n+\t\terror(_(\"unexpected non-hex object ID: %s\"), hash);\n+\t\treturn 1;\n+\t}\n+\n+\toidset_insert(commits, &oid);\n+\treturn 0;\n+}\n+\n static int graph_write(int argc, const char **argv)\n {\n-\tstruct string_list *pack_indexes = NULL;\n+\tstruct string_list pack_indexes;\n \tstruct oidset commits = OIDSET_INIT;\n \tstruct object_directory *odb = NULL;\n-\tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n \n@@ -209,44 +222,35 @@ static int graph_write(int argc, const char **argv)\n \t\treturn 0;\n \t}\n \n-\tstring_list_init(&lines, 0);\n+\tstring_list_init(&pack_indexes, 0);\n \tif (opts.stdin_packs || opts.stdin_commits) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\twhile (strbuf_getline(&buf, stdin) != EOF)\n-\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n-\n-\t\tif (opts.stdin_packs)\n-\t\t\tpack_indexes = &lines;\n \t\tif (opts.stdin_commits) {\n-\t\t\tstruct string_list_item *item;\n-\t\t\toidset_init(&commits, lines.nr);\n-\t\t\tfor_each_string_list_item(item, &lines) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tconst char *end;\n-\n-\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n-\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n-\t\t\t\t\t\t\"%s\"), item->string);\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\toidset_insert(&commits, &oid);\n-\t\t\t}\n+\t\t\toidset_init(&commits, 0);\n \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n \n+\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\t\tchar *line = strbuf_detach(&buf, NULL);\n+\t\t\tif (opts.stdin_commits) {\n+\t\t\t\tint result = read_one_commit(&commits, line);\n+\t\t\t\tif (result)\n+\t\t\t\t\treturn result;\n+\t\t\t} else\n+\t\t\t\tstring_list_append(&pack_indexes, line);\n+\t\t}\n+\n \t\tUNLEAK(buf);\n \t}\n \n \tif (write_commit_graph(odb,\n-\t\t\t pack_indexes,\n+\t\t\t opts.stdin_packs ? &pack_indexes : NULL,\n \t\t\t opts.stdin_commits ? &commits : NULL,\n \t\t\t flags,\n \t\t\t &split_opts))\n \t\tresult = 1;\n \n-\tUNLEAK(lines);\n+\tUNLEAK(pack_indexes);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397034","messageId":"513a634f14e90ec0c2e80a6aaf8cb66bbedf8966.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:49Z","receivedAt":"2020-05-05T01:13:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When given a list of commits, the commit-graph machinery calls\n'lookup_commit_reference_gently()' on each element in the set and treats\nthe resulting set of OIDs as the base over which to close for\nreachability.\n\nIn an earlier collection of commits, the 'git commit-graph write\n--reachable' case made the inner-most call to\n'lookup_commit_reference_gently()' by peeling references before they\nwere passed over to the commit-graph internals.\n\nDo the analog for 'git commit-graph write --stdin-commits' by calling\n'lookup_commit_reference_gently()' outside of the commit-graph\nmachinery, making the inner-most call a noop.\n\nSince this may incur additional processing time, surround\n'read_one_commit' with a progress meter to provide output to the caller.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex f550d8489a..9eec68572f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -6,6 +6,7 @@\n #include \"repository.h\"\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n+#include \"progress.h\"\n \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n@@ -138,8 +139,10 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int read_one_commit(struct oidset *commits, char *hash)\n+static int read_one_commit(struct oidset *commits, struct progress *progress,\n+\t\t\t char *hash)\n {\n+\tstruct commit *result;\n \tstruct object_id oid;\n \tconst char *end;\n \n@@ -148,7 +151,15 @@ static int read_one_commit(struct oidset *commits, char *hash)\n \t\treturn 1;\n \t}\n \n-\toidset_insert(commits, &oid);\n+\tdisplay_progress(progress, oidset_size(commits) + 1);\n+\n+\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n+\tif (result)\n+\t\toidset_insert(commits, &result->object.oid);\n+\telse {\n+\t\terror(_(\"invalid commit object id: %s\"), hash);\n+\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n@@ -159,6 +170,7 @@ static int graph_write(int argc, const char **argv)\n \tstruct object_directory *odb = NULL;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n+\tstruct progress *progress = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -228,18 +240,25 @@ static int graph_write(int argc, const char **argv)\n \t\tif (opts.stdin_commits) {\n \t\t\toidset_init(&commits, 0);\n \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\t\tif (opts.progress)\n+\t\t\t\tprogress = start_delayed_progress(\n+\t\t\t\t\t_(\"Analyzing commits from stdin\"), 0);\n \t\t}\n \n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\t\tchar *line = strbuf_detach(&buf, NULL);\n \t\t\tif (opts.stdin_commits) {\n-\t\t\t\tint result = read_one_commit(&commits, line);\n+\t\t\t\tint result = read_one_commit(&commits, progress,\n+\t\t\t\t\t\t\t line);\n \t\t\t\tif (result)\n \t\t\t\t\treturn result;\n \t\t\t} else\n \t\t\t\tstring_list_append(&pack_indexes, line);\n \t\t}\n \n+\t\tif (progress)\n+\t\t\tstop_progress(&progress);\n+\n \t\tUNLEAK(buf);\n \t}\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397035","messageId":"7e9d8c1f1a124171ebb5b4d874718053d1c9064a.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 6/8] commit-graph.c: simplify 'fill_oids_from_commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:13:54Z","receivedAt":"2020-05-05T01:13:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous handful of commits, both 'git commit-graph write\n--reachable' and '--stdin-commits' learned to peel tags down to the\ncommits which they refer to before passing them into the commit-graph\ninternals.\n\nThis makes the call to 'lookup_commit_reference_gently()' inside of\n'fill_oids_from_commits()' a noop, since all OIDs are commits by that\npoint.\n\nAs such, remove the call entirely, as well as the progress meter, which\nhas been split and moved out to the callers in the aforementioned\nearlier commits.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 33 +++------------------------------\n 1 file changed, 3 insertions(+), 30 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5c3fad0dd7..24c2d80935 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1411,46 +1411,19 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n static int fill_oids_from_commits(struct write_commit_graph_context *ctx,\n \t\t\t\t struct oidset *commits)\n {\n-\tuint32_t i = 0;\n-\tstruct strbuf progress_title = STRBUF_INIT;\n \tstruct oidset_iter iter;\n \tstruct object_id *oid;\n \n \tif (!oidset_size(commits))\n \t\treturn 0;\n \n-\tif (ctx->report_progress) {\n-\t\tstrbuf_addf(&progress_title,\n-\t\t\t Q_(\"Finding commits for commit graph from %d ref\",\n-\t\t\t \"Finding commits for commit graph from %d refs\",\n-\t\t\t oidset_size(commits)),\n-\t\t\t oidset_size(commits));\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\tprogress_title.buf,\n-\t\t\t\t\toidset_size(commits));\n-\t}\n-\n \toidset_iter_init(commits, &iter);\n \twhile ((oid = oidset_iter_next(&iter))) {\n-\t\tstruct commit *result;\n-\n-\t\tdisplay_progress(ctx->progress, ++i);\n-\n-\t\tresult = lookup_commit_reference_gently(ctx->r, oid, 1);\n-\t\tif (result) {\n-\t\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n-\t\t\toidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n-\t\t\tctx->oids.nr++;\n-\t\t} else if (ctx->check_oids) {\n-\t\t\terror(_(\"invalid commit object id: %s\"),\n-\t\t\t oid_to_hex(oid));\n-\t\t\treturn -1;\n-\t\t}\n+\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n+\t\toidcpy(&ctx->oids.list[ctx->oids.nr], oid);\n+\t\tctx->oids.nr++;\n \t}\n \n-\tstop_progress(&ctx->progress);\n-\tstrbuf_release(&progress_title);\n-\n \treturn 0;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397036","messageId":"f2f018b54c742856fa7283fad1439bfb3ce81570.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 7/8] t5318: reorder test below 'graph_read_expect'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:14:00Z","receivedAt":"2020-05-05T01:14:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the subsequent commit, we will introduce a dependency on\n'graph_read_expect' from t5318.7. Preemptively move it below\n'graph_read_expect()'s definition so that the test can call it.\n\nSigned-off-by: Taylor Blau \n---\n t/t5318-commit-graph.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 39e2918a32..89020d3d44 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -42,15 +42,6 @@ test_expect_success 'create commits and repack' '\n \tgit repack\n '\n \n-test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n-\tcd \"$TRASH_DIRECTORY/full\" &&\n-\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n-\t# valid tree OID, but not a commit OID\n-\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"invalid commit object id\" stderr\n-'\n-\n graph_git_two_modes() {\n \tgit -c core.commitGraph=true $1 >output\n \tgit -c core.commitGraph=false $1 >expect\n@@ -91,6 +82,15 @@ graph_read_expect() {\n \ttest_cmp expect output\n }\n \n+test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n+\t# valid tree OID, but not a commit OID\n+\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"invalid commit object id\" stderr\n+'\n+\n test_expect_success 'write graph' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write &&\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397037","messageId":"6c2d130b0cd4b6a8a541b362ae7dd44d4c282e3f.1588641176.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T01:14:03Z","receivedAt":"2020-05-05T01:14:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in\n'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on\nreceiving non-commit OIDs as input to '--stdin-commits'.\n\nThis behavior can be cumbersome to work around in, say, the case of\npiping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if\nthe caller does not want to cull out non-commits themselves. In this\nsituation, it would be ideal if 'git commit-graph write' wrote the graph\ncontaining the inputs that did pertain to commits, and silently ignored\nthe remainder of the input.\n\nSome options have been proposed to the effect of '--[no-]check-oids'\nwhich would allow callers to have the commit-graph builtin do just that.\nAfter some discussion, it is difficult to imagine a caller who wouldn't\nwant to pass '--no-check-oids', suggesting that we should get rid of the\nbehavior of complaining about non-commit inputs altogether.\n\nIf callers do wish to retain this behavior, they can easily work around\nthis change by doing the following:\n\n git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n awk '/commit/ { print $1 }' |\n git commit-graph write --stdin-commits\n\nTo make it so that valid OIDs that refer to non-existent objects are\nindeed an error after loosening the error handling, perform an extra\nlookup to make sure that object indeed exists before sending it to the\ncommit-graph internals.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 6 ++++--\n builtin/commit-graph.c | 10 +++++-----\n commit-graph.c | 2 --\n commit-graph.h | 10 ++++------\n t/t5318-commit-graph.sh | 15 +++++++++++----\n 5 files changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 53a650225a..fcac7d12e1 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n +\n With the `--stdin-commits` option, generate the new commit graph by\n walking commits starting at the commits specified in stdin as a list\n-of OIDs in hex, one OID per line. (Cannot be combined with\n-`--stdin-packs` or `--reachable`.)\n+of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n+(either directly, or by peeling tags) are silently ignored. OIDs that\n+are malformed, or do not exist generate an error. (Cannot be combined\n+with `--stdin-packs` or `--reachable`.)\n +\n With the `--reachable` option, generate the new commit graph by walking\n commits starting at all refs. (Cannot be combined with `--stdin-commits`\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 9eec68572f..3637d079fb 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress,\n \n \tdisplay_progress(progress, oidset_size(commits) + 1);\n \n+\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n+\t\terror(_(\"object %s does not exist\"), hash);\n+\t\treturn 1;\n+\t}\n+\n \tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n \tif (result)\n \t\toidset_insert(commits, &result->object.oid);\n-\telse {\n-\t\terror(_(\"invalid commit object id: %s\"), hash);\n-\t\treturn 1;\n-\t}\n \treturn 0;\n }\n \n@@ -239,7 +240,6 @@ static int graph_write(int argc, const char **argv)\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tif (opts.stdin_commits) {\n \t\t\toidset_init(&commits, 0);\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t\tif (opts.progress)\n \t\t\t\tprogress = start_delayed_progress(\n \t\t\t\t\t_(\"Analyzing commits from stdin\"), 0);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 24c2d80935..4e5539bac5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -880,7 +880,6 @@ struct write_commit_graph_context {\n \tunsigned append:1,\n \t\t report_progress:1,\n \t\t split:1,\n-\t\t check_oids:1,\n \t\t changed_paths:1,\n \t\t order_by_pack:1;\n \n@@ -2001,7 +2000,6 @@ int write_commit_graph(struct object_directory *odb,\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n-\tctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;\n \tctx->split_opts = split_opts;\n \tctx->changed_paths = flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS ? 1 : 0;\n \tctx->total_bloom_filter_data_size = 0;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 4212766a4f..659393e07f 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -88,12 +88,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n int generation_numbers_enabled(struct repository *r);\n \n enum commit_graph_write_flags {\n-\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n-\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n-\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n-\t/* Make sure that each OID in the input is a valid commit OID. */\n-\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),\n-\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),\n+\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n+\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n+\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n+\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3)\n };\n \n enum commit_graph_split_flags {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 89020d3d44..74f93f0a17 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -84,11 +84,18 @@ graph_read_expect() {\n \n test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\t# invalid, non-hex OID\n+\techo HEAD >in &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n \ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n-\t# valid tree OID, but not a commit OID\n-\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"invalid commit object id\" stderr\n+\t# non-existent OID\n+\techo $ZERO_OID >in &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n+\ttest_i18ngrep \"does not exist\" stderr &&\n+\t# valid commit and tree OID\n+\tgit rev-parse HEAD HEAD^{tree} >in &&\n+\tgit commit-graph write --stdin-commits Hi,\n> \n> These are a few patches to address [1]\n\nWe seem to be missing links [1] and [2].\n\n> - It is often convenient to pipe the output of 'git for-each-ref' into\n> 'git commit-graph write --stdin-commits', but handling multi-layer\n> tags is frustrating (at the very least, you have to peel them\n> yourself in 'for-each-ref', incurring a higher-than-necessary cost\n> outside of the commit-graph invocation)\n> \n> - It would be much more convenient if 'git commit-graph write\n> --stdin-commits' ignored OIDs that peel down to non-commit objects\n> silently, while still warning the caller about objects that (a)\n> don't exist, or (b) invalid hash inputs (for e.g., \"HEAD\", or\n> \"refs/heads/foo\").\n> \n> - By making this change, we are allowing *more* possible behaviors\n> than in previous versions, and the only case that we are breaking\n> (that 'git commit-graph write --stdin-commits' complains on\n> non-commit OIDs) can be addressed by peeling outside of\n> 'commit-graph write' [2].\n\nI agree that these are worthy goals. If someone was _depending_ on\nthe \"git commit-graph write\" command to _fail_, then they chose a\nstrange way to discover a ref that points to a non-commit.\n\n> The first six patches move peeling refs out from the commit-graph\n> internals, and into the callers.\n> \n> This has no impact on the '--stdin-commits' case, but is a potential\n> improvement for '--reachable', where it is more efficient to call\n> 'peel_ref()' than 'lookup_commit_reference_gently()'. There is an\n> intermediate state (that is resolved in the final patch) where we can\n> have more than one progress meter (since the progress meter moved with\n> the perhaps-expensive operations out of the commit-graph internals,\n> too), but the final patch addresses this.\n\nInteresting. I'll be sure to look carefully at this progress handling\nas this has potential to lose our progress options. I expect that you\nwere careful about this.\n\n> The last two patches lift the restriction on input to '--stdin-commits'\n> resolving to commit objects, instead making the behavior to silently\n> drop non-commit OIDs, and continue complain about invalid OIDs, and\n> valid-but-missing OIDs.\n> \n> Finally, this topic isn't based on anything that isn't already in\n> 'master', so I think the days of many tangle commit-graph topics are\n> behind us.\n\nWoo!\n\n-Stolee\n\n"},{"id":"397079","messageId":"b8f393fa-fec8-205e-0987-f4f6ff9b3531@gmail.com","threadId":"53395","inReplyTo":"5bdbeaf374b6050670f800fcdd3b54ddd0750754.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 2/8] commit-graph.c: show progress of finding reachable commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-05T11:50:50Z","receivedAt":"2020-05-05T11:50:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2020 9:13 PM, Taylor Blau wrote:\n> When 'git commit-graph write --reachable' is invoked, the commit-graph\n> machinery calls 'for_each_ref()' to discover the set of reachable\n> commits.\n> \n> Right now the 'add_ref_to_set' callback is not doing anything other than\n> adding an OID to the set of known-reachable OIDs. In a subsequent\n> commit, 'add_ref_to_set' will presumptively peel references. This\n> operation should be fast for repositories with an up-to-date\n> '$GIT_DIR/packed-refs', but may be slow in the general case.\n> \n> So that it doesn't appear that 'git commit-graph write' is idling with\n> '--reachable' in the slow case, add a progress meter to provide some\n> output in the meantime.\n> \n> In general, we don't expect a progress meter to appear at all, since\n> peeling references with a 'packed-refs' file is quick. If it's slow and\n> we do show a progress meter, the subsequent 'fill_oids_from_commits()'\n> will be fast, since all of the calls to\n> 'lookup_commit_reference_gently()' will be no-ops.\n> \n> Both progress meters are delayed, so it is unlikely that more than one\n> will appear. In either case, this intermediate state will go away in a\n> handful of patches, at which point there will be at most one progress\n> meter.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> commit-graph.c | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 00da281f39..8f61256b0a 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1320,6 +1320,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n> \n> struct refs_cb_data {\n> \tstruct oidset *commits;\n> +\tstruct progress *progress;\n> };\n> \n> static int add_ref_to_set(const char *refname,\n> @@ -1328,6 +1329,8 @@ static int add_ref_to_set(const char *refname,\n> {\n> \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n> \n> +\tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n> +\n> \toidset_insert(data->commits, oid);\n> \treturn 0;\n> }\n> @@ -1342,12 +1345,17 @@ int write_commit_graph_reachable(struct object_directory *odb,\n> \n> \tmemset(&data, 0, sizeof(data));\n> \tdata.commits = &commits;\n> +\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n> +\t\tdata.progress = start_delayed_progress(\n> +\t\t\t_(\"Finding reachable commits\"), 0);\n\nIs this the right phrase to use? This seems too close to the progress\nindicator \"Expanding reachable commits in commit graph\" which is walking\ncommits.\n\nAn alternative could be: \"Collecting referenced commits\"\n\nOutside of this string, I have no complaints. I also don't feel too\nstrongly about the string, either.\n\nThanks,\n-Stolee\n\n \n\n"},{"id":"397081","messageId":"1b0bf610-640e-de4e-ab31-ca8719d533f7@gmail.com","threadId":"53395","inReplyTo":"513a634f14e90ec0c2e80a6aaf8cb66bbedf8966.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-05T12:01:29Z","receivedAt":"2020-05-05T12:01:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2020 9:13 PM, Taylor Blau wrote:\n> @@ -228,18 +240,25 @@ static int graph_write(int argc, const char **argv)\n> \t\tif (opts.stdin_commits) {\n> \t\t\toidset_init(&commits, 0);\n> \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n> +\t\t\tif (opts.progress)\n> +\t\t\t\tprogress = start_delayed_progress(\n> +\t\t\t\t\t_(\"Analyzing commits from stdin\"), 0);\nThe code functions as you intend and is an improvement. Similar to my\nearlier suggestion to use something like \"Collecting referenced commits\"\nfor the --reachable case, perhaps this could be \"Collecting commits from input\"?\n\nThanks,\n-Stolee\n"},{"id":"397083","messageId":"b36f9210-bc26-5ca8-b163-62d75349b381@gmail.com","threadId":"53395","inReplyTo":"7e9d8c1f1a124171ebb5b4d874718053d1c9064a.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 6/8] commit-graph.c: simplify 'fill_oids_from_commits'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-05T12:05:24Z","receivedAt":"2020-05-05T12:05:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2020 9:13 PM, Taylor Blau wrote:\n> In the previous handful of commits, both 'git commit-graph write\n> --reachable' and '--stdin-commits' learned to peel tags down to the\n> commits which they refer to before passing them into the commit-graph\n> internals.\n> \n> This makes the call to 'lookup_commit_reference_gently()' inside of\n> 'fill_oids_from_commits()' a noop, since all OIDs are commits by that\n> point.\n> \n> As such, remove the call entirely, as well as the progress meter, which\n> has been split and moved out to the callers in the aforementioned\n> earlier commits.\n...\n> \toidset_iter_init(commits, &iter);\n> \twhile ((oid = oidset_iter_next(&iter))) {\n[snip removed code]\n> +\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n> +\t\toidcpy(&ctx->oids.list[ctx->oids.nr], oid);\n> +\t\tctx->oids.nr++;\n> \t}\n\nAfter looking at this loop, I wondered why we can't just use the\noidset inside the context struct. But then I realized that the\noids list expands with reachable commits and then is sorted\nlexicographically. Thus, this is the best time to copy from the\noidset to the commit list.\n\nI agree that dropping the progress is valuable here, since this\n_should_ be a very fast operation even for enormous commit sets.\n\nThanks,\n-Stolee\n\n\n"},{"id":"397084","messageId":"81fb0f5e-e7e1-7541-d6c7-36828190c95b@gmail.com","threadId":"53395","inReplyTo":"6c2d130b0cd4b6a8a541b362ae7dd44d4c282e3f.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-05T12:10:40Z","receivedAt":"2020-05-05T12:10:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2020 9:14 PM, Taylor Blau wrote:\n> Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in\n> 'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on\n> receiving non-commit OIDs as input to '--stdin-commits'.\n> \n> This behavior can be cumbersome to work around in, say, the case of\n> piping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if\n> the caller does not want to cull out non-commits themselves. In this\n> situation, it would be ideal if 'git commit-graph write' wrote the graph\n> containing the inputs that did pertain to commits, and silently ignored\n> the remainder of the input.\n> \n> Some options have been proposed to the effect of '--[no-]check-oids'\n> which would allow callers to have the commit-graph builtin do just that.\n> After some discussion, it is difficult to imagine a caller who wouldn't\n> want to pass '--no-check-oids', suggesting that we should get rid of the\n> behavior of complaining about non-commit inputs altogether.\n> \n> If callers do wish to retain this behavior, they can easily work around\n> this change by doing the following:\n> \n> git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> awk '/commit/ { print $1 }' |\n> git commit-graph write --stdin-commits\n\nI appreciate that you included the workaround here for posterity. That\nallows anyone complaining to bisect to these instructions.\n\n> To make it so that valid OIDs that refer to non-existent objects are\n> indeed an error after loosening the error handling, perform an extra\n> lookup to make sure that object indeed exists before sending it to the\n> commit-graph internals.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> Documentation/git-commit-graph.txt | 6 ++++--\n> builtin/commit-graph.c | 10 +++++-----\n> commit-graph.c | 2 --\n> commit-graph.h | 10 ++++------\n> t/t5318-commit-graph.sh | 15 +++++++++++----\n> 5 files changed, 24 insertions(+), 19 deletions(-)\n> \n> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> index 53a650225a..fcac7d12e1 100644\n> --- a/Documentation/git-commit-graph.txt\n> +++ b/Documentation/git-commit-graph.txt\n> @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n> +\n> With the `--stdin-commits` option, generate the new commit graph by\n> walking commits starting at the commits specified in stdin as a list\n> -of OIDs in hex, one OID per line. (Cannot be combined with\n> -`--stdin-packs` or `--reachable`.)\n> +of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n> +(either directly, or by peeling tags) are silently ignored. OIDs that\n> +are malformed, or do not exist generate an error. (Cannot be combined\n> +with `--stdin-packs` or `--reachable`.)\n> +\n> With the `--reachable` option, generate the new commit graph by walking\n> commits starting at all refs. (Cannot be combined with `--stdin-commits`\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 9eec68572f..3637d079fb 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress,\n> \n> \tdisplay_progress(progress, oidset_size(commits) + 1);\n> \n> +\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n> +\t\terror(_(\"object %s does not exist\"), hash);\n> +\t\treturn 1;\n> +\t}\n> +\n\nIf we get a non-existent object, then this will cause us to fail\nthe command, right?\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 89020d3d44..74f93f0a17 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -84,11 +84,18 @@ graph_read_expect() {\n> \n> test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> -\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n> +\t# invalid, non-hex OID\n> +\techo HEAD >in &&\n> +\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n> \ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n> -\t# valid tree OID, but not a commit OID\n> -\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n> -\ttest_i18ngrep \"invalid commit object id\" stderr\n> +\t# non-existent OID\n> +\techo $ZERO_OID >in &&\n> +\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n> +\ttest_i18ngrep \"does not exist\" stderr &&\n\nAnd here you verify that it fails at that point. Excellent!\n\n> +\t# valid commit and tree OID\n> +\tgit rev-parse HEAD HEAD^{tree} >in &&\n> +\tgit commit-graph write --stdin-commits +\tgraph_read_expect 3\n> '\n\nThis is an excellent series! Thanks.\n\nReviewed-by: Derrick Stolee \n\n\n\n"},{"id":"397106","messageId":"20200505161142.GD69300@syl.local","threadId":"53395","inReplyTo":"bc41469c-a55a-49a4-5e2e-096bbdd52751@gmail.com","subject":"Re: [PATCH 0/8] commit-graph: drop CHECK_OIDS, peel in callers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T16:11:42Z","receivedAt":"2020-05-05T16:11:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 05, 2020 at 07:35:29AM -0400, Derrick Stolee wrote:\n> On 5/4/2020 9:13 PM, Taylor Blau wrote:\n> > Hi,\n> >\n> > These are a few patches to address [1]\n>\n> We seem to be missing links [1] and [2].\n\nWhoops. I wrote the cover letter and then postponed it for an hour or\ntwo while I shuffled (what are now) the last two patches around. It\nlooks like I dropped the hyperlinks in the meantime, but here they are\nnow:\n\n[1]: https://lore.kernel.org/git/20200501223848.GH41612@syl.local/\n[2]: https://lore.kernel.org/git/20200504145937.GA11373@coredump.intra.peff.net/\n\nThanks,\nTaylor\n"},{"id":"397107","messageId":"20200505161323.GE69300@syl.local","threadId":"53395","inReplyTo":"b8f393fa-fec8-205e-0987-f4f6ff9b3531@gmail.com","subject":"Re: [PATCH 2/8] commit-graph.c: show progress of finding reachable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T16:13:23Z","receivedAt":"2020-05-05T16:13:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 05, 2020 at 07:50:50AM -0400, Derrick Stolee wrote:\n> On 5/4/2020 9:13 PM, Taylor Blau wrote:\n> > When 'git commit-graph write --reachable' is invoked, the commit-graph\n> > machinery calls 'for_each_ref()' to discover the set of reachable\n> > commits.\n> >\n> > Right now the 'add_ref_to_set' callback is not doing anything other than\n> > adding an OID to the set of known-reachable OIDs. In a subsequent\n> > commit, 'add_ref_to_set' will presumptively peel references. This\n> > operation should be fast for repositories with an up-to-date\n> > '$GIT_DIR/packed-refs', but may be slow in the general case.\n> >\n> > So that it doesn't appear that 'git commit-graph write' is idling with\n> > '--reachable' in the slow case, add a progress meter to provide some\n> > output in the meantime.\n> >\n> > In general, we don't expect a progress meter to appear at all, since\n> > peeling references with a 'packed-refs' file is quick. If it's slow and\n> > we do show a progress meter, the subsequent 'fill_oids_from_commits()'\n> > will be fast, since all of the calls to\n> > 'lookup_commit_reference_gently()' will be no-ops.\n> >\n> > Both progress meters are delayed, so it is unlikely that more than one\n> > will appear. In either case, this intermediate state will go away in a\n> > handful of patches, at which point there will be at most one progress\n> > meter.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > commit-graph.c | 8 ++++++++\n> > 1 file changed, 8 insertions(+)\n> >\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 00da281f39..8f61256b0a 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -1320,6 +1320,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n> >\n> > struct refs_cb_data {\n> > \tstruct oidset *commits;\n> > +\tstruct progress *progress;\n> > };\n> >\n> > static int add_ref_to_set(const char *refname,\n> > @@ -1328,6 +1329,8 @@ static int add_ref_to_set(const char *refname,\n> > {\n> > \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n> >\n> > +\tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n> > +\n> > \toidset_insert(data->commits, oid);\n> > \treturn 0;\n> > }\n> > @@ -1342,12 +1345,17 @@ int write_commit_graph_reachable(struct object_directory *odb,\n> >\n> > \tmemset(&data, 0, sizeof(data));\n> > \tdata.commits = &commits;\n> > +\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n> > +\t\tdata.progress = start_delayed_progress(\n> > +\t\t\t_(\"Finding reachable commits\"), 0);\n>\n> Is this the right phrase to use? This seems too close to the progress\n> indicator \"Expanding reachable commits in commit graph\" which is walking\n> commits.\n>\n> An alternative could be: \"Collecting referenced commits\"\n\nThat's a great suggestion. I'll apply that locally. For what it's worth,\nI don't feel that strongly about it either, but it's not hard to change\n;).\n\n> Outside of this string, I have no complaints. I also don't feel too\n> strongly about the string, either.\n>\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"397108","messageId":"20200505161441.GF69300@syl.local","threadId":"53395","inReplyTo":"1b0bf610-640e-de4e-ab31-ca8719d533f7@gmail.com","subject":"Re: [PATCH 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T16:14:41Z","receivedAt":"2020-05-05T16:14:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 05, 2020 at 08:01:29AM -0400, Derrick Stolee wrote:\n> On 5/4/2020 9:13 PM, Taylor Blau wrote:\n> > @@ -228,18 +240,25 @@ static int graph_write(int argc, const char **argv)\n> > \t\tif (opts.stdin_commits) {\n> > \t\t\toidset_init(&commits, 0);\n> > \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n> > +\t\t\tif (opts.progress)\n> > +\t\t\t\tprogress = start_delayed_progress(\n> > +\t\t\t\t\t_(\"Analyzing commits from stdin\"), 0);\n> The code functions as you intend and is an improvement. Similar to my\n> earlier suggestion to use something like \"Collecting referenced commits\"\n> for the --reachable case, perhaps this could be \"Collecting commits from input\"?\n\nYep, making these consistent with one another is a good thing to do,\nthanks.\n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"397109","messageId":"20200505161649.GG69300@syl.local","threadId":"53395","inReplyTo":"81fb0f5e-e7e1-7541-d6c7-36828190c95b@gmail.com","subject":"Re: [PATCH 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T16:16:49Z","receivedAt":"2020-05-05T16:16:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 05, 2020 at 08:10:40AM -0400, Derrick Stolee wrote:\n> On 5/4/2020 9:14 PM, Taylor Blau wrote:\n> > Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in\n> > 'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on\n> > receiving non-commit OIDs as input to '--stdin-commits'.\n> >\n> > This behavior can be cumbersome to work around in, say, the case of\n> > piping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if\n> > the caller does not want to cull out non-commits themselves. In this\n> > situation, it would be ideal if 'git commit-graph write' wrote the graph\n> > containing the inputs that did pertain to commits, and silently ignored\n> > the remainder of the input.\n> >\n> > Some options have been proposed to the effect of '--[no-]check-oids'\n> > which would allow callers to have the commit-graph builtin do just that.\n> > After some discussion, it is difficult to imagine a caller who wouldn't\n> > want to pass '--no-check-oids', suggesting that we should get rid of the\n> > behavior of complaining about non-commit inputs altogether.\n> >\n> > If callers do wish to retain this behavior, they can easily work around\n> > this change by doing the following:\n> >\n> > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> > awk '/commit/ { print $1 }' |\n> > git commit-graph write --stdin-commits\n>\n> I appreciate that you included the workaround here for posterity. That\n> allows anyone complaining to bisect to these instructions.\n\nWell, now that I linked provided a link to [2] in the cover letter,\nyou'll see that it was Peff's suggestion ;).\n\nBut, I wrote something similar to this in one of GitHub's\npost-processing jobs before we had this series, so I think I can take\nsome of the credit!\n\n> > To make it so that valid OIDs that refer to non-existent objects are\n> > indeed an error after loosening the error handling, perform an extra\n> > lookup to make sure that object indeed exists before sending it to the\n> > commit-graph internals.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > Documentation/git-commit-graph.txt | 6 ++++--\n> > builtin/commit-graph.c | 10 +++++-----\n> > commit-graph.c | 2 --\n> > commit-graph.h | 10 ++++------\n> > t/t5318-commit-graph.sh | 15 +++++++++++----\n> > 5 files changed, 24 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> > index 53a650225a..fcac7d12e1 100644\n> > --- a/Documentation/git-commit-graph.txt\n> > +++ b/Documentation/git-commit-graph.txt\n> > @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n> > +\n> > With the `--stdin-commits` option, generate the new commit graph by\n> > walking commits starting at the commits specified in stdin as a list\n> > -of OIDs in hex, one OID per line. (Cannot be combined with\n> > -`--stdin-packs` or `--reachable`.)\n> > +of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n> > +(either directly, or by peeling tags) are silently ignored. OIDs that\n> > +are malformed, or do not exist generate an error. (Cannot be combined\n> > +with `--stdin-packs` or `--reachable`.)\n> > +\n> > With the `--reachable` option, generate the new commit graph by walking\n> > commits starting at all refs. (Cannot be combined with `--stdin-commits`\n> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> > index 9eec68572f..3637d079fb 100644\n> > --- a/builtin/commit-graph.c\n> > +++ b/builtin/commit-graph.c\n> > @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress,\n> >\n> > \tdisplay_progress(progress, oidset_size(commits) + 1);\n> >\n> > +\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n> > +\t\terror(_(\"object %s does not exist\"), hash);\n> > +\t\treturn 1;\n> > +\t}\n> > +\n>\n> If we get a non-existent object, then this will cause us to fail\n> the command, right?\n>\n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index 89020d3d44..74f93f0a17 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -84,11 +84,18 @@ graph_read_expect() {\n> >\n> > test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > -\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n> > +\t# invalid, non-hex OID\n> > +\techo HEAD >in &&\n> > +\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n> > \ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n> > -\t# valid tree OID, but not a commit OID\n> > -\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n> > -\ttest_i18ngrep \"invalid commit object id\" stderr\n> > +\t# non-existent OID\n> > +\techo $ZERO_OID >in &&\n> > +\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n> > +\ttest_i18ngrep \"does not exist\" stderr &&\n>\n> And here you verify that it fails at that point. Excellent!\n>\n> > +\t# valid commit and tree OID\n> > +\tgit rev-parse HEAD HEAD^{tree} >in &&\n> > +\tgit commit-graph write --stdin-commits > +\tgraph_read_expect 3\n> > '\n>\n> This is an excellent series! Thanks.\n>\n> Reviewed-by: Derrick Stolee \n\nThanks for your review. I applied your suggested changes locally (which\ncame down to tweaking the progress meter's messages in the 2nd and 5th\npatches), but I'll sit on them for a day or so to make sure that nobody\nelse has any thoughts before sending a v2.\n\nThanks,\nTaylor\n"},{"id":"397151","messageId":"cover.1588723543.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH v2 0/8] commit-graph: drop CHECK_OIDS, peel in callers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:09Z","receivedAt":"2020-05-06T00:07:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere's a small re-roll of my series to move peeling outside of the\ncommit-graph internals, and drop the CHECK_OIDS flag. This re-roll was\npromised in [1], and only updates the messages used in the new progress\nmeters. For convenience, a range-diff from v1 is included below.\n\nThanks in advance for another review.\n\n[1]: https://lore.kernel.org/git/20200505161649.GG69300@syl.local/\n\nTaylor Blau (8):\n commit-graph.c: extract 'refs_cb_data'\n commit-graph.c: show progress of finding reachable commits\n commit-graph.c: peel refs in 'add_ref_to_set'\n builtin/commit-graph.c: extract 'read_one_commit()'\n builtin/commit-graph.c: dereference tags in builtin\n commit-graph.c: simplify 'fill_oids_from_commits'\n t5318: reorder test below 'graph_read_expect'\n commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag\n\n Documentation/git-commit-graph.txt | 6 ++-\n builtin/commit-graph.c | 73 ++++++++++++++++++++----------\n commit-graph.c | 61 +++++++++++--------------\n commit-graph.h | 10 ++--\n t/t5318-commit-graph.sh | 25 ++++++----\n 5 files changed, 98 insertions(+), 77 deletions(-)\n\nRange-diff against v1:\n1: 5bdbeaf374 ! 1: cb56a9a73b commit-graph.c: show progress of finding reachable commits\n @@ commit-graph.c: int write_commit_graph_reachable(struct object_directory *odb,\n \tdata.commits = &commits;\n +\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n +\t\tdata.progress = start_delayed_progress(\n -+\t\t\t_(\"Finding reachable commits\"), 0);\n ++\t\t\t_(\"Collecting referenced commits\"), 0);\n\n \tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n2: 5ff56feab5 = 2: 85c388a077 commit-graph.c: peel refs in 'add_ref_to_set'\n3: 9ae8745dc0 = 3: cef441b465 builtin/commit-graph.c: extract 'read_one_commit()'\n4: 513a634f14 ! 4: d449d83ce2 builtin/commit-graph.c: dereference tags in builtin\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n +\t\t\tif (opts.progress)\n +\t\t\t\tprogress = start_delayed_progress(\n -+\t\t\t\t\t_(\"Analyzing commits from stdin\"), 0);\n ++\t\t\t\t\t_(\"Collecting commits from input\"), 0);\n \t\t}\n\n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n5: 7e9d8c1f1a = 5: 61887870c7 commit-graph.c: simplify 'fill_oids_from_commits'\n6: f2f018b54c = 6: e393b16097 t5318: reorder test below 'graph_read_expect'\n7: 6c2d130b0c ! 7: ad373f05ff commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n -\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t\tif (opts.progress)\n \t\t\t\tprogress = start_delayed_progress(\n - \t\t\t\t\t_(\"Analyzing commits from stdin\"), 0);\n + \t\t\t\t\t_(\"Collecting commits from input\"), 0);\n\n ## commit-graph.c ##\n @@ commit-graph.c: struct write_commit_graph_context {\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"397152","messageId":"43286c3c45b2dcef8d9d39b4998c17f364eb1060.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 1/8] commit-graph.c: extract 'refs_cb_data'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:31Z","receivedAt":"2020-05-06T00:07:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In subsequent patches, we are going to update a progress meter when\n'add_ref_to_set()' is called, and need a convenient way to pass a\n'struct progress *' in from the caller.\n\nIntroduce 'refs_cb_data' as a catch-all for parameters that\n'add_ref_to_set' may need, and wrap the existing single parameter in\nthat struct.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6dc777e2f3..00da281f39 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1318,13 +1318,17 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \tstop_progress(&progress);\n }\n \n+struct refs_cb_data {\n+\tstruct oidset *commits;\n+};\n+\n static int add_ref_to_set(const char *refname,\n \t\t\t const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n-\tstruct oidset *commits = (struct oidset *)cb_data;\n+\tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n-\toidset_insert(commits, oid);\n+\toidset_insert(data->commits, oid);\n \treturn 0;\n }\n \n@@ -1333,9 +1337,13 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n \tstruct oidset commits = OIDSET_INIT;\n+\tstruct refs_cb_data data;\n \tint result;\n \n-\tfor_each_ref(add_ref_to_set, &commits);\n+\tmemset(&data, 0, sizeof(data));\n+\tdata.commits = &commits;\n+\n+\tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397153","messageId":"cb56a9a73b961c7d61b4c7dc6f49bd07f05d66e4.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 2/8] commit-graph.c: show progress of finding reachable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:35Z","receivedAt":"2020-05-06T00:07:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When 'git commit-graph write --reachable' is invoked, the commit-graph\nmachinery calls 'for_each_ref()' to discover the set of reachable\ncommits.\n\nRight now the 'add_ref_to_set' callback is not doing anything other than\nadding an OID to the set of known-reachable OIDs. In a subsequent\ncommit, 'add_ref_to_set' will presumptively peel references. This\noperation should be fast for repositories with an up-to-date\n'$GIT_DIR/packed-refs', but may be slow in the general case.\n\nSo that it doesn't appear that 'git commit-graph write' is idling with\n'--reachable' in the slow case, add a progress meter to provide some\noutput in the meantime.\n\nIn general, we don't expect a progress meter to appear at all, since\npeeling references with a 'packed-refs' file is quick. If it's slow and\nwe do show a progress meter, the subsequent 'fill_oids_from_commits()'\nwill be fast, since all of the calls to\n'lookup_commit_reference_gently()' will be no-ops.\n\nBoth progress meters are delayed, so it is unlikely that more than one\nwill appear. In either case, this intermediate state will go away in a\nhandful of patches, at which point there will be at most one progress\nmeter.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 00da281f39..d0397f2a23 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1320,6 +1320,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \n struct refs_cb_data {\n \tstruct oidset *commits;\n+\tstruct progress *progress;\n };\n \n static int add_ref_to_set(const char *refname,\n@@ -1328,6 +1329,8 @@ static int add_ref_to_set(const char *refname,\n {\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n+\tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n+\n \toidset_insert(data->commits, oid);\n \treturn 0;\n }\n@@ -1342,12 +1345,17 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.commits = &commits;\n+\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n+\t\tdata.progress = start_delayed_progress(\n+\t\t\t_(\"Collecting referenced commits\"), 0);\n \n \tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n \toidset_clear(&commits);\n+\tif (data.progress)\n+\t\tstop_progress(&data.progress);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397154","messageId":"85c388a077ed9b43ad4876e6b3efaa0358c78715.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 3/8] commit-graph.c: peel refs in 'add_ref_to_set'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:39Z","receivedAt":"2020-05-06T00:07:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While iterating references (to discover the set of commits to write to\nthe commit-graph with 'git commit-graph write --reachable'),\n'add_ref_to_set' can save 'fill_oids_from_commits()' some time by\npeeling the references beforehand.\n\nMove peeling out of 'fill_oids_from_commits()' and into\n'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing\nso allows the commit-graph machinery to use the peeled value from\n'$GIT_DIR/packed-refs' instead of having to load and parse tags.\n\nWhile we're at it, discard non-commit objects reachable from ref tips.\nThis would be done automatically by 'fill_oids_from_commits()', but such\nfunctionality will be removed in a subsequent patch after the call to\n'lookup_commit_reference_gently' is dropped (at which point a non-commit\nobject in the commits oidset will become an error).\n\nSuggested-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex d0397f2a23..2c3b5fd09d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1327,11 +1327,15 @@ static int add_ref_to_set(const char *refname,\n \t\t\t const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n+\tstruct object_id peeled;\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n \tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n \n-\toidset_insert(data->commits, oid);\n+\tif (peel_ref(refname, &peeled))\n+\t\tpeeled = *oid;\n+\tif (oid_object_info(the_repository, &peeled, NULL) == OBJ_COMMIT)\n+\t\toidset_insert(data->commits, &peeled);\n \treturn 0;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397155","messageId":"cef441b465b4129746fd9e4a04fb92d2440a3a32.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:43Z","receivedAt":"2020-05-06T00:07:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"With either '--stdin-commits' or '--stdin-packs', the commit-graph\nbuiltin will read line-delimited input, and interpret it either as a\nseries of commit OIDs, or pack names.\n\nIn a subsequent commit, we will begin handling '--stdin-commits'\ndifferently by processing each line as it comes in, instead of in one\nshot at the end. To make adequate room for this additional logic, split\nthe '--stdin-commits' case from '--stdin-packs' by only storing the\ninput when '--stdin-packs' is given.\n\nIn the case of '--stdin-commits', feed each line to a new\n'read_one_commit' helper, which (for now) will merely call\n'parse_oid_hex'.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 54 +++++++++++++++++++++++-------------------\n 1 file changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 15fe60317c..f550d8489a 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -138,12 +138,25 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static int read_one_commit(struct oidset *commits, char *hash)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\n+\tif (parse_oid_hex(hash, &oid, &end)) {\n+\t\terror(_(\"unexpected non-hex object ID: %s\"), hash);\n+\t\treturn 1;\n+\t}\n+\n+\toidset_insert(commits, &oid);\n+\treturn 0;\n+}\n+\n static int graph_write(int argc, const char **argv)\n {\n-\tstruct string_list *pack_indexes = NULL;\n+\tstruct string_list pack_indexes;\n \tstruct oidset commits = OIDSET_INIT;\n \tstruct object_directory *odb = NULL;\n-\tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n \n@@ -209,44 +222,35 @@ static int graph_write(int argc, const char **argv)\n \t\treturn 0;\n \t}\n \n-\tstring_list_init(&lines, 0);\n+\tstring_list_init(&pack_indexes, 0);\n \tif (opts.stdin_packs || opts.stdin_commits) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\twhile (strbuf_getline(&buf, stdin) != EOF)\n-\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n-\n-\t\tif (opts.stdin_packs)\n-\t\t\tpack_indexes = &lines;\n \t\tif (opts.stdin_commits) {\n-\t\t\tstruct string_list_item *item;\n-\t\t\toidset_init(&commits, lines.nr);\n-\t\t\tfor_each_string_list_item(item, &lines) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tconst char *end;\n-\n-\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n-\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n-\t\t\t\t\t\t\"%s\"), item->string);\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\toidset_insert(&commits, &oid);\n-\t\t\t}\n+\t\t\toidset_init(&commits, 0);\n \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n \n+\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\t\tchar *line = strbuf_detach(&buf, NULL);\n+\t\t\tif (opts.stdin_commits) {\n+\t\t\t\tint result = read_one_commit(&commits, line);\n+\t\t\t\tif (result)\n+\t\t\t\t\treturn result;\n+\t\t\t} else\n+\t\t\t\tstring_list_append(&pack_indexes, line);\n+\t\t}\n+\n \t\tUNLEAK(buf);\n \t}\n \n \tif (write_commit_graph(odb,\n-\t\t\t pack_indexes,\n+\t\t\t opts.stdin_packs ? &pack_indexes : NULL,\n \t\t\t opts.stdin_commits ? &commits : NULL,\n \t\t\t flags,\n \t\t\t &split_opts))\n \t\tresult = 1;\n \n-\tUNLEAK(lines);\n+\tUNLEAK(pack_indexes);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397156","messageId":"d449d83ce2efd4f8f3b15c0cfda8217ffa710116.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:47Z","receivedAt":"2020-05-06T00:07:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When given a list of commits, the commit-graph machinery calls\n'lookup_commit_reference_gently()' on each element in the set and treats\nthe resulting set of OIDs as the base over which to close for\nreachability.\n\nIn an earlier collection of commits, the 'git commit-graph write\n--reachable' case made the inner-most call to\n'lookup_commit_reference_gently()' by peeling references before they\nwere passed over to the commit-graph internals.\n\nDo the analog for 'git commit-graph write --stdin-commits' by calling\n'lookup_commit_reference_gently()' outside of the commit-graph\nmachinery, making the inner-most call a noop.\n\nSince this may incur additional processing time, surround\n'read_one_commit' with a progress meter to provide output to the caller.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex f550d8489a..6537e9acef 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -6,6 +6,7 @@\n #include \"repository.h\"\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n+#include \"progress.h\"\n \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n@@ -138,8 +139,10 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int read_one_commit(struct oidset *commits, char *hash)\n+static int read_one_commit(struct oidset *commits, struct progress *progress,\n+\t\t\t char *hash)\n {\n+\tstruct commit *result;\n \tstruct object_id oid;\n \tconst char *end;\n \n@@ -148,7 +151,15 @@ static int read_one_commit(struct oidset *commits, char *hash)\n \t\treturn 1;\n \t}\n \n-\toidset_insert(commits, &oid);\n+\tdisplay_progress(progress, oidset_size(commits) + 1);\n+\n+\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n+\tif (result)\n+\t\toidset_insert(commits, &result->object.oid);\n+\telse {\n+\t\terror(_(\"invalid commit object id: %s\"), hash);\n+\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n@@ -159,6 +170,7 @@ static int graph_write(int argc, const char **argv)\n \tstruct object_directory *odb = NULL;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n+\tstruct progress *progress = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -228,18 +240,25 @@ static int graph_write(int argc, const char **argv)\n \t\tif (opts.stdin_commits) {\n \t\t\toidset_init(&commits, 0);\n \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\t\tif (opts.progress)\n+\t\t\t\tprogress = start_delayed_progress(\n+\t\t\t\t\t_(\"Collecting commits from input\"), 0);\n \t\t}\n \n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\t\tchar *line = strbuf_detach(&buf, NULL);\n \t\t\tif (opts.stdin_commits) {\n-\t\t\t\tint result = read_one_commit(&commits, line);\n+\t\t\t\tint result = read_one_commit(&commits, progress,\n+\t\t\t\t\t\t\t line);\n \t\t\t\tif (result)\n \t\t\t\t\treturn result;\n \t\t\t} else\n \t\t\t\tstring_list_append(&pack_indexes, line);\n \t\t}\n \n+\t\tif (progress)\n+\t\t\tstop_progress(&progress);\n+\n \t\tUNLEAK(buf);\n \t}\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397157","messageId":"61887870c722d0934b02d7e2a94972a147dc4d3b.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 6/8] commit-graph.c: simplify 'fill_oids_from_commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:51Z","receivedAt":"2020-05-06T00:07:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous handful of commits, both 'git commit-graph write\n--reachable' and '--stdin-commits' learned to peel tags down to the\ncommits which they refer to before passing them into the commit-graph\ninternals.\n\nThis makes the call to 'lookup_commit_reference_gently()' inside of\n'fill_oids_from_commits()' a noop, since all OIDs are commits by that\npoint.\n\nAs such, remove the call entirely, as well as the progress meter, which\nhas been split and moved out to the callers in the aforementioned\nearlier commits.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 33 +++------------------------------\n 1 file changed, 3 insertions(+), 30 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2c3b5fd09d..56a4a43b30 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1411,46 +1411,19 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n static int fill_oids_from_commits(struct write_commit_graph_context *ctx,\n \t\t\t\t struct oidset *commits)\n {\n-\tuint32_t i = 0;\n-\tstruct strbuf progress_title = STRBUF_INIT;\n \tstruct oidset_iter iter;\n \tstruct object_id *oid;\n \n \tif (!oidset_size(commits))\n \t\treturn 0;\n \n-\tif (ctx->report_progress) {\n-\t\tstrbuf_addf(&progress_title,\n-\t\t\t Q_(\"Finding commits for commit graph from %d ref\",\n-\t\t\t \"Finding commits for commit graph from %d refs\",\n-\t\t\t oidset_size(commits)),\n-\t\t\t oidset_size(commits));\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\tprogress_title.buf,\n-\t\t\t\t\toidset_size(commits));\n-\t}\n-\n \toidset_iter_init(commits, &iter);\n \twhile ((oid = oidset_iter_next(&iter))) {\n-\t\tstruct commit *result;\n-\n-\t\tdisplay_progress(ctx->progress, ++i);\n-\n-\t\tresult = lookup_commit_reference_gently(ctx->r, oid, 1);\n-\t\tif (result) {\n-\t\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n-\t\t\toidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n-\t\t\tctx->oids.nr++;\n-\t\t} else if (ctx->check_oids) {\n-\t\t\terror(_(\"invalid commit object id: %s\"),\n-\t\t\t oid_to_hex(oid));\n-\t\t\treturn -1;\n-\t\t}\n+\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n+\t\toidcpy(&ctx->oids.list[ctx->oids.nr], oid);\n+\t\tctx->oids.nr++;\n \t}\n \n-\tstop_progress(&ctx->progress);\n-\tstrbuf_release(&progress_title);\n-\n \treturn 0;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397158","messageId":"e393b16097dfdf9a136f6726486561074f5f3b1c.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 7/8] t5318: reorder test below 'graph_read_expect'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:54Z","receivedAt":"2020-05-06T00:07:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the subsequent commit, we will introduce a dependency on\n'graph_read_expect' from t5318.7. Preemptively move it below\n'graph_read_expect()'s definition so that the test can call it.\n\nSigned-off-by: Taylor Blau \n---\n t/t5318-commit-graph.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 39e2918a32..89020d3d44 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -42,15 +42,6 @@ test_expect_success 'create commits and repack' '\n \tgit repack\n '\n \n-test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n-\tcd \"$TRASH_DIRECTORY/full\" &&\n-\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n-\t# valid tree OID, but not a commit OID\n-\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"invalid commit object id\" stderr\n-'\n-\n graph_git_two_modes() {\n \tgit -c core.commitGraph=true $1 >output\n \tgit -c core.commitGraph=false $1 >expect\n@@ -91,6 +82,15 @@ graph_read_expect() {\n \ttest_cmp expect output\n }\n \n+test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n+\t# valid tree OID, but not a commit OID\n+\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"invalid commit object id\" stderr\n+'\n+\n test_expect_success 'write graph' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write &&\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397159","messageId":"ad373f05fff5512b4242ba097b0d12c0107606e9.1588723544.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"[PATCH v2 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-06T00:07:58Z","receivedAt":"2020-05-06T00:08:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in\n'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on\nreceiving non-commit OIDs as input to '--stdin-commits'.\n\nThis behavior can be cumbersome to work around in, say, the case of\npiping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if\nthe caller does not want to cull out non-commits themselves. In this\nsituation, it would be ideal if 'git commit-graph write' wrote the graph\ncontaining the inputs that did pertain to commits, and silently ignored\nthe remainder of the input.\n\nSome options have been proposed to the effect of '--[no-]check-oids'\nwhich would allow callers to have the commit-graph builtin do just that.\nAfter some discussion, it is difficult to imagine a caller who wouldn't\nwant to pass '--no-check-oids', suggesting that we should get rid of the\nbehavior of complaining about non-commit inputs altogether.\n\nIf callers do wish to retain this behavior, they can easily work around\nthis change by doing the following:\n\n git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n awk '/commit/ { print $1 }' |\n git commit-graph write --stdin-commits\n\nTo make it so that valid OIDs that refer to non-existent objects are\nindeed an error after loosening the error handling, perform an extra\nlookup to make sure that object indeed exists before sending it to the\ncommit-graph internals.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 6 ++++--\n builtin/commit-graph.c | 10 +++++-----\n commit-graph.c | 2 --\n commit-graph.h | 10 ++++------\n t/t5318-commit-graph.sh | 15 +++++++++++----\n 5 files changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 53a650225a..fcac7d12e1 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n +\n With the `--stdin-commits` option, generate the new commit graph by\n walking commits starting at the commits specified in stdin as a list\n-of OIDs in hex, one OID per line. (Cannot be combined with\n-`--stdin-packs` or `--reachable`.)\n+of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n+(either directly, or by peeling tags) are silently ignored. OIDs that\n+are malformed, or do not exist generate an error. (Cannot be combined\n+with `--stdin-packs` or `--reachable`.)\n +\n With the `--reachable` option, generate the new commit graph by walking\n commits starting at all refs. (Cannot be combined with `--stdin-commits`\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 6537e9acef..a4685a1553 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress,\n \n \tdisplay_progress(progress, oidset_size(commits) + 1);\n \n+\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n+\t\terror(_(\"object %s does not exist\"), hash);\n+\t\treturn 1;\n+\t}\n+\n \tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n \tif (result)\n \t\toidset_insert(commits, &result->object.oid);\n-\telse {\n-\t\terror(_(\"invalid commit object id: %s\"), hash);\n-\t\treturn 1;\n-\t}\n \treturn 0;\n }\n \n@@ -239,7 +240,6 @@ static int graph_write(int argc, const char **argv)\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tif (opts.stdin_commits) {\n \t\t\toidset_init(&commits, 0);\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t\tif (opts.progress)\n \t\t\t\tprogress = start_delayed_progress(\n \t\t\t\t\t_(\"Collecting commits from input\"), 0);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 56a4a43b30..3c3ada29b1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -880,7 +880,6 @@ struct write_commit_graph_context {\n \tunsigned append:1,\n \t\t report_progress:1,\n \t\t split:1,\n-\t\t check_oids:1,\n \t\t changed_paths:1,\n \t\t order_by_pack:1;\n \n@@ -2001,7 +2000,6 @@ int write_commit_graph(struct object_directory *odb,\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n-\tctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;\n \tctx->split_opts = split_opts;\n \tctx->changed_paths = flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS ? 1 : 0;\n \tctx->total_bloom_filter_data_size = 0;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 4212766a4f..659393e07f 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -88,12 +88,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n int generation_numbers_enabled(struct repository *r);\n \n enum commit_graph_write_flags {\n-\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n-\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n-\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n-\t/* Make sure that each OID in the input is a valid commit OID. */\n-\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),\n-\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),\n+\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n+\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n+\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n+\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3)\n };\n \n enum commit_graph_split_flags {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 89020d3d44..74f93f0a17 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -84,11 +84,18 @@ graph_read_expect() {\n \n test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\t# invalid, non-hex OID\n+\techo HEAD >in &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n \ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n-\t# valid tree OID, but not a commit OID\n-\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"invalid commit object id\" stderr\n+\t# non-existent OID\n+\techo $ZERO_OID >in &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n+\ttest_i18ngrep \"does not exist\" stderr &&\n+\t# valid commit and tree OID\n+\tgit rev-parse HEAD HEAD^{tree} >in &&\n+\tgit commit-graph write --stdin-commits While iterating references (to discover the set of commits to write to\n> the commit-graph with 'git commit-graph write --reachable'),\n> 'add_ref_to_set' can save 'fill_oids_from_commits()' some time by\n> peeling the references beforehand.\n> \n> Move peeling out of 'fill_oids_from_commits()' and into\n> 'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing\n> so allows the commit-graph machinery to use the peeled value from\n> '$GIT_DIR/packed-refs' instead of having to load and parse tags.\n\nOr having to load and parse commits only to find out that they're not\ntags. :)\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 8f61256b0a..5c3fad0dd7 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1327,11 +1327,15 @@ static int add_ref_to_set(const char *refname,\n> \t\t\t const struct object_id *oid,\n> \t\t\t int flags, void *cb_data)\n> {\n> +\tstruct object_id peeled;\n> \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n> \n> \tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n> \n> -\toidset_insert(data->commits, oid);\n> +\tif (peel_ref(refname, &peeled))\n> +\t\tpeeled = *oid;\n\nIt may be the old-timer C programmer in me, but I always look slightly\nsuspicious at struct assignments. We know that object_id doesn't need a\ndeep copy, so it's obviously OK here. But should we use oidcpy() as a\nstyle thing?\n\nAlternatively, you could do this without a struct copy at all with:\n\n if (!peel_ref(...))\n oid = peeled;\n oidset_insert(..., oid);\n\nwhich is actually a bit cheaper.\n\n> +\tif (oid_object_info(the_repository, &peeled, NULL) == OBJ_COMMIT)\n> +\t\toidset_insert(data->commits, &peeled);\n\nI probably would have left adding this \"if\" until a later step, but I\nthink it's OK here.\n\n-Peff\n"},{"id":"397346","messageId":"20200507200305.GB29683@coredump.intra.peff.net","threadId":"53395","inReplyTo":"9ae8745dc090de37af0475ab12b79d541a52713d.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:03:05Z","receivedAt":"2020-05-07T20:03:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 07:13:46PM -0600, Taylor Blau wrote:\n\n> In the case of '--stdin-commits', feed each line to a new\n> 'read_one_commit' helper, which (for now) will merely call\n> 'parse_oid_hex'.\n\nMakes sense.\n\n> +static int read_one_commit(struct oidset *commits, char *hash)\n\nThis could be \"const char *hash\", couldn't it?\n\n> +\tstruct object_id oid;\n> +\tconst char *end;\n> +\n> +\tif (parse_oid_hex(hash, &oid, &end)) {\n> +\t\terror(_(\"unexpected non-hex object ID: %s\"), hash);\n> +\t\treturn 1;\n> +\t}\n\nReturning \"-1\" for error is more idiomatic in our code base (though I\nknow some of the commit-graph code doesn't follow that, I think we\nshould slowly try to move it back in the other direction.\n\n> +\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n> +\t\t\tchar *line = strbuf_detach(&buf, NULL);\n> +\t\t\tif (opts.stdin_commits) {\n> +\t\t\t\tint result = read_one_commit(&commits, line);\n> +\t\t\t\tif (result)\n> +\t\t\t\t\treturn result;\n> +\t\t\t} else\n> +\t\t\t\tstring_list_append(&pack_indexes, line);\n> +\t\t}\n\nThis leaks \"line\" for each commit in stdin_commits mode (it used to get\nadded to a string list). I think you want:\n\n while (strbuf_getline(&buf, stdin) != EOF) {\n if (opts.stdin_commits) {\n\t if (read_one_commit(&commits, buf.buf)) {\n\t\t\tstrbuf_release(&buf);\n\t\t\treturn 1;\n\t\t}\n\t} else {\n\t string_list_append(&pack_indexes, strbuf_detach(&buf));\n\t}\n }\n\nThough I think it might be easier to follow if each mode simply has its\nown while loop.\n\n> +\n> \t\tUNLEAK(buf);\n\nNot new in your patch, but this UNLEAK() has always bugged me. ;) Why\nnot just strbuf_release() it?\n\n-Peff\n"},{"id":"397347","messageId":"20200507201419.GC29683@coredump.intra.peff.net","threadId":"53395","inReplyTo":"513a634f14e90ec0c2e80a6aaf8cb66bbedf8966.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:14:19Z","receivedAt":"2020-05-07T20:14:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 07:13:49PM -0600, Taylor Blau wrote:\n\n> When given a list of commits, the commit-graph machinery calls\n> 'lookup_commit_reference_gently()' on each element in the set and treats\n> the resulting set of OIDs as the base over which to close for\n> reachability.\n> \n> In an earlier collection of commits, the 'git commit-graph write\n> --reachable' case made the inner-most call to\n> 'lookup_commit_reference_gently()' by peeling references before they\n> were passed over to the commit-graph internals.\n> \n> Do the analog for 'git commit-graph write --stdin-commits' by calling\n> 'lookup_commit_reference_gently()' outside of the commit-graph\n> machinery, making the inner-most call a noop.\n\nYep, I think this is a good direction.\n\n> @@ -148,7 +151,15 @@ static int read_one_commit(struct oidset *commits, char *hash)\n> \t\treturn 1;\n> \t}\n> \n> -\toidset_insert(commits, &oid);\n> +\tdisplay_progress(progress, oidset_size(commits) + 1);\n\nMost of our meters increment progress _after_ doing work.\n\nThis is especially important for meters with percentages (if we knew we\nhad 1 commit, we'd print \"100%\" and _then_ start to peel it, which is\nsilly).\n\nFor this instance we don't know the total number, so we're just counting\nup. But I think we should be consistent about when we update meters.\nPlus it's shorter to say:\n\n display_progress(progress, oidset_size(commits));\n\nafter having done the work. ;)\n\n> +\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n\nWould we want to pass quiet==0 here? If we see an error we're going to\nbail loudly below, so getting more details from the low-level functions\nis helpful (I think the only one you'd really get is \"I'm looking for a\ncommit, but it's a tree\" or similar).\n\nlookup_commit_reference_gently() is pretty aggressive about parsing\nobjects. We'll have to parse commits eventually, but we could possibly\ndo so using their graph representations. It may not be worth optimizing,\nbecause it would only matter if you fed a lot of --stdin-commits inputs\nthat were already graphed. (And if it is worth optimizing, it should\nprobably come in a separate commit anyway; this is just about moving the\nexisting peeling).\n\n> +\tif (result)\n> +\t\toidset_insert(commits, &result->object.oid);\n> +\telse {\n> +\t\terror(_(\"invalid commit object id: %s\"), hash);\n> +\t\treturn 1;\n> +\t}\n\nIf you follow my \"return -1\" suggestion from earlier, this would need\nit, too.\n\n> \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n> \t\t\tchar *line = strbuf_detach(&buf, NULL);\n> \t\t\tif (opts.stdin_commits) {\n> -\t\t\t\tint result = read_one_commit(&commits, line);\n> +\t\t\t\tint result = read_one_commit(&commits, progress,\n> +\t\t\t\t\t\t\t line);\n> \t\t\t\tif (result)\n> \t\t\t\t\treturn result;\n> \t\t\t} else\n> \t\t\t\tstring_list_append(&pack_indexes, line);\n> \t\t}\n> \n> +\t\tif (progress)\n> +\t\t\tstop_progress(&progress);\n\nIf we return early in the loop, we'd leave this progress meter hanging.\nIt might be worth converting that return to a break or goto that handles\ncleanup (it also needs to handle releasing the strbuf).\n\n-Peff\n"},{"id":"397348","messageId":"20200507202148.GD29683@coredump.intra.peff.net","threadId":"53395","inReplyTo":"7e9d8c1f1a124171ebb5b4d874718053d1c9064a.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 6/8] commit-graph.c: simplify 'fill_oids_from_commits'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:21:48Z","receivedAt":"2020-05-07T20:21:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 07:13:54PM -0600, Taylor Blau wrote:\n\n> In the previous handful of commits, both 'git commit-graph write\n> --reachable' and '--stdin-commits' learned to peel tags down to the\n> commits which they refer to before passing them into the commit-graph\n> internals.\n> \n> This makes the call to 'lookup_commit_reference_gently()' inside of\n> 'fill_oids_from_commits()' a noop, since all OIDs are commits by that\n> point.\n> \n> As such, remove the call entirely, as well as the progress meter, which\n> has been split and moved out to the callers in the aforementioned\n> earlier commits.\n\nYep, all this makes sense. I agree with Stolee that it's unfortunate we\ncan't just reuse the oidset now, but we do need the flattened array view\nhere. We could perhaps create such an array from the beginning (perhaps\nusing an oid_array), but we do need to care about de-duping the input.\nThat can be done easily with the sorted list, but there are pathological\ncorner cases where the performance is worse (e.g., if you have a ton of\nrefs all pointing to the same tags, like if you happened to be storing\nthe refs for 20,000 forks of the kernel all in one giant repo).\n\nI think we'd eventually turn all these into \"struct commit\" (and indeed,\nwe already do in --stdin-commits when we try to peel). So another\nalternative would be passing in an object_array(). But I guess that\nwould require surgery to the rest of the commit-graph code to work with\nthat instead of the oid list.\n\n> \twhile ((oid = oidset_iter_next(&iter))) {\n> -\t\tstruct commit *result;\n> -\n> -\t\tdisplay_progress(ctx->progress, ++i);\n> -\n> -\t\tresult = lookup_commit_reference_gently(ctx->r, oid, 1);\n> -\t\tif (result) {\n> -\t\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n> -\t\t\toidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n> -\t\t\tctx->oids.nr++;\n> -\t\t} else if (ctx->check_oids) {\n> -\t\t\terror(_(\"invalid commit object id: %s\"),\n> -\t\t\t oid_to_hex(oid));\n> -\t\t\treturn -1;\n> -\t\t}\n> +\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n> +\t\toidcpy(&ctx->oids.list[ctx->oids.nr], oid);\n> +\t\tctx->oids.nr++;\n> \t}\n\nI wondered if it's worth asserting that everything we got here is a\ncommit. But it's not cheap to make that check, and anyway we'd\npresumably just barf later on when we try to resolve the oids to\ncommits. So there's little point in spending cycles to catch it here.\n\n-Peff\n"},{"id":"397349","messageId":"20200507204005.GE29683@coredump.intra.peff.net","threadId":"53395","inReplyTo":"6c2d130b0cd4b6a8a541b362ae7dd44d4c282e3f.1588641176.git.me@ttaylorr.com","subject":"Re: [PATCH 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:40:05Z","receivedAt":"2020-05-07T20:40:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 07:14:03PM -0600, Taylor Blau wrote:\n\n> If callers do wish to retain this behavior, they can easily work around\n> this change by doing the following:\n> \n> git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> awk '/commit/ { print $1 }' |\n> git commit-graph write --stdin-commits\n\nI know this came from my earlier email, but I think that recipe actually\nshows how to make your input work even if --check-oids were the default.\nIf you really want the --check-oids behavior, you'd want the opposite:\nto complain about those ones. So it's something like:\n\n git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n awk '\n !/commit/ { print \"not-a-commit:\"$1 }\n /commit/ { print $1 }\n ' |\n git commit-graph write --stdin-commits\n\n> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> index 53a650225a..fcac7d12e1 100644\n> --- a/Documentation/git-commit-graph.txt\n> +++ b/Documentation/git-commit-graph.txt\n> @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n> +\n> With the `--stdin-commits` option, generate the new commit graph by\n> walking commits starting at the commits specified in stdin as a list\n> -of OIDs in hex, one OID per line. (Cannot be combined with\n> -`--stdin-packs` or `--reachable`.)\n> +of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n> +(either directly, or by peeling tags) are silently ignored. OIDs that\n> +are malformed, or do not exist generate an error. (Cannot be combined\n> +with `--stdin-packs` or `--reachable`.)\n\nYeah, I think these semantics are good.\n\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 9eec68572f..3637d079fb 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress,\n> \n> \tdisplay_progress(progress, oidset_size(commits) + 1);\n> \n> +\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n> +\t\terror(_(\"object %s does not exist\"), hash);\n> +\t\treturn 1;\n> +\t}\n> +\n> \tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n> \tif (result)\n> \t\toidset_insert(commits, &result->object.oid);\n> -\telse {\n> -\t\terror(_(\"invalid commit object id: %s\"), hash);\n> -\t\treturn 1;\n> -\t}\n> \treturn 0;\n> }\n\nWe can avoid the object-existence check entirely if\nlookup_commit_reference_gently() gives us an answer. And we'd expect\nthat to be the common path.\n\nAlso, using has_object_file() is cheaper than oid_object_info(), since\nit doesn't have to resolve the type for deltas.\n\nSo perhaps:\n\n result = lookup_commit_reference_gently(...);\n if (result)\n oidset_insert(...);\n else if (has_object_file(&oid))\n ; /* not a commit; quietly ignore;\n else\n return error(no such object...);\n\nThat said, I think this technique misses some cases of corruption.\nYou're checking that the outer-most object exists, but not any\nintermediate peeled objects. I.e., lookup_commit_reference_gently()\nmight have failed for two reasons:\n\n - an object it peeled to didn't exist\n\n - an object it peeled to wasn't a commit\n\nTo do it thoroughly, I think you'd have to call deref_tag() yourself and\ndistinguish NULL there (an error) from a result where obj->type isn't\nOBJ_COMMIT (quietly ignore).\n\n> enum commit_graph_write_flags {\n> -\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n> -\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n> -\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n> -\t/* Make sure that each OID in the input is a valid commit OID. */\n> -\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),\n> -\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),\n> +\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n> +\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n> +\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n> +\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3)\n\nAs much as I love looking at matched-indentation lists, I think this\ndiff is a good example of why it's not worth doing. It's much easier to\nsee what's going on if the first three items aren't touched. I'd\nactually even leave BLOOM_FILTERS where it is, and accept the hole which\ncould be refilled later.\n\nYour patch also loses the trailing comma after the final BLOOM_FILTERS\nentry.\n\n-Peff\n"},{"id":"397350","messageId":"20200507204204.GF29683@coredump.intra.peff.net","threadId":"53395","inReplyTo":"cover.1588723543.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/8] commit-graph: drop CHECK_OIDS, peel in callers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:42:04Z","receivedAt":"2020-05-07T20:42:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 05, 2020 at 06:07:09PM -0600, Taylor Blau wrote:\n\n> Here's a small re-roll of my series to move peeling outside of the\n> commit-graph internals, and drop the CHECK_OIDS flag. This re-roll was\n> promised in [1], and only updates the messages used in the new progress\n> meters. For convenience, a range-diff from v1 is included below.\n\nI just reviewed v1. :) But I think the revised progress messages in v2\nare much better.\n\nThe overall direction looks very good to me. I left a couple of\ncomments. Nothing too major, but I think there are some things worth\naddressing (leaks, and more robust error checking in the final patch).\n\n-Peff\n"},{"id":"397750","messageId":"20200513194848.GA24173@syl.local","threadId":"53395","inReplyTo":"20200507195441.GA29683@coredump.intra.peff.net","subject":"Re: [PATCH 3/8] commit-graph.c: peel refs in 'add_ref_to_set'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T19:48:48Z","receivedAt":"2020-05-13T19:48:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 07, 2020 at 03:54:41PM -0400, Jeff King wrote:\n> On Mon, May 04, 2020 at 07:13:43PM -0600, Taylor Blau wrote:\n>\n> > While iterating references (to discover the set of commits to write to\n> > the commit-graph with 'git commit-graph write --reachable'),\n> > 'add_ref_to_set' can save 'fill_oids_from_commits()' some time by\n> > peeling the references beforehand.\n> >\n> > Move peeling out of 'fill_oids_from_commits()' and into\n> > 'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing\n> > so allows the commit-graph machinery to use the peeled value from\n> > '$GIT_DIR/packed-refs' instead of having to load and parse tags.\n>\n> Or having to load and parse commits only to find out that they're not\n> tags. :)\n>\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 8f61256b0a..5c3fad0dd7 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -1327,11 +1327,15 @@ static int add_ref_to_set(const char *refname,\n> > \t\t\t const struct object_id *oid,\n> > \t\t\t int flags, void *cb_data)\n> > {\n> > +\tstruct object_id peeled;\n> > \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n> >\n> > \tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n> >\n> > -\toidset_insert(data->commits, oid);\n> > +\tif (peel_ref(refname, &peeled))\n> > +\t\tpeeled = *oid;\n>\n> It may be the old-timer C programmer in me, but I always look slightly\n> suspicious at struct assignments. We know that object_id doesn't need a\n> deep copy, so it's obviously OK here. But should we use oidcpy() as a\n> style thing?\n>\n> Alternatively, you could do this without a struct copy at all with:\n>\n> if (!peel_ref(...))\n> oid = peeled;\n> oidset_insert(..., oid);\n>\n> which is actually a bit cheaper.\n\nMakes sense, I think this version is the better of the two that you\nsuggested here. I noticed one small thing which is that since peeled is\nonly on the stack, I think we actually want 'oid = &peeled', but\notherwise I took this as-is.\n\n> > +\tif (oid_object_info(the_repository, &peeled, NULL) == OBJ_COMMIT)\n> > +\t\toidset_insert(data->commits, &peeled);\n>\n> I probably would have left adding this \"if\" until a later step, but I\n> think it's OK here.\n\nYeah, I did it here to avoid having to add a seemingly-unrelated change\nlater on. I agree it doesn't matter much, so in the interest of leaving\nthe series alone, I'll leave it where it is here.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"397755","messageId":"20200513200103.GB24173@syl.local","threadId":"53395","inReplyTo":"20200507200305.GB29683@coredump.intra.peff.net","subject":"Re: [PATCH 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T20:01:03Z","receivedAt":"2020-05-13T20:01:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 07, 2020 at 04:03:05PM -0400, Jeff King wrote:\n> On Mon, May 04, 2020 at 07:13:46PM -0600, Taylor Blau wrote:\n>\n> > In the case of '--stdin-commits', feed each line to a new\n> > 'read_one_commit' helper, which (for now) will merely call\n> > 'parse_oid_hex'.\n>\n> Makes sense.\n>\n> > +static int read_one_commit(struct oidset *commits, char *hash)\n>\n> This could be \"const char *hash\", couldn't it?\n\nYep, thanks.\n\n> > +\tstruct object_id oid;\n> > +\tconst char *end;\n> > +\n> > +\tif (parse_oid_hex(hash, &oid, &end)) {\n> > +\t\terror(_(\"unexpected non-hex object ID: %s\"), hash);\n> > +\t\treturn 1;\n> > +\t}\n>\n> Returning \"-1\" for error is more idiomatic in our code base (though I\n> know some of the commit-graph code doesn't follow that, I think we\n> should slowly try to move it back in the other direction.\n\nYeah, I know the -1 is more idiomatic than what I had written here. This\nwas done so that I could use the return value from 'read_one_commit' as\nan exit code from 'graph_write()', but I don't mind switching this to\n'return error(...)' and then checking at the caller for a non-zero\nreturn value and returning 1 there instead.\n\n> > +\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n> > +\t\t\tchar *line = strbuf_detach(&buf, NULL);\n> > +\t\t\tif (opts.stdin_commits) {\n> > +\t\t\t\tint result = read_one_commit(&commits, line);\n> > +\t\t\t\tif (result)\n> > +\t\t\t\t\treturn result;\n> > +\t\t\t} else\n> > +\t\t\t\tstring_list_append(&pack_indexes, line);\n> > +\t\t}\n>\n> This leaks \"line\" for each commit in stdin_commits mode (it used to get\n> added to a string list). I think you want:\n>\n> while (strbuf_getline(&buf, stdin) != EOF) {\n> if (opts.stdin_commits) {\n> \t if (read_one_commit(&commits, buf.buf)) {\n> \t\t\tstrbuf_release(&buf);\n> \t\t\treturn 1;\n> \t\t}\n> \t} else {\n> \t string_list_append(&pack_indexes, strbuf_detach(&buf));\n> \t}\n> }\n>\n> Though I think it might be easier to follow if each mode simply has its\n> own while loop.\n\nYeah, it's much clearer as two separate cases. I'll send something like\nthat shortly once I get to the rest of your review.\n\n> > +\n> > \t\tUNLEAK(buf);\n>\n> Not new in your patch, but this UNLEAK() has always bugged me. ;) Why\n> not just strbuf_release() it?\n\nI snuck it in! ;)\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"397760","messageId":"20200513201724.GA1823254@coredump.intra.peff.net","threadId":"53395","inReplyTo":"20200513194848.GA24173@syl.local","subject":"Re: [PATCH 3/8] commit-graph.c: peel refs in 'add_ref_to_set'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-13T20:17:24Z","receivedAt":"2020-05-13T20:17:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 01:48:48PM -0600, Taylor Blau wrote:\n\n> > Alternatively, you could do this without a struct copy at all with:\n> >\n> > if (!peel_ref(...))\n> > oid = peeled;\n> > oidset_insert(..., oid);\n> >\n> > which is actually a bit cheaper.\n> \n> Makes sense, I think this version is the better of the two that you\n> suggested here. I noticed one small thing which is that since peeled is\n> only on the stack, I think we actually want 'oid = &peeled', but\n> otherwise I took this as-is.\n\nYes, definitely. That's what I get for writing code in my email editor. :)\n\n-Peff\n"},{"id":"397775","messageId":"20200513213228.GC24173@syl.local","threadId":"53395","inReplyTo":"20200507204005.GE29683@coredump.intra.peff.net","subject":"Re: [PATCH 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:32:28Z","receivedAt":"2020-05-13T21:32:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 07, 2020 at 04:40:05PM -0400, Jeff King wrote:\n> On Mon, May 04, 2020 at 07:14:03PM -0600, Taylor Blau wrote:\n>\n> > If callers do wish to retain this behavior, they can easily work around\n> > this change by doing the following:\n> >\n> > git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> > awk '/commit/ { print $1 }' |\n> > git commit-graph write --stdin-commits\n>\n> I know this came from my earlier email, but I think that recipe actually\n> shows how to make your input work even if --check-oids were the default.\n> If you really want the --check-oids behavior, you'd want the opposite:\n> to complain about those ones. So it's something like:\n>\n> git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n> awk '\n> !/commit/ { print \"not-a-commit:\"$1 }\n> /commit/ { print $1 }\n> ' |\n> git commit-graph write --stdin-commits\n>\n> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\n> > index 53a650225a..fcac7d12e1 100644\n> > --- a/Documentation/git-commit-graph.txt\n> > +++ b/Documentation/git-commit-graph.txt\n> > @@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n> > +\n> > With the `--stdin-commits` option, generate the new commit graph by\n> > walking commits starting at the commits specified in stdin as a list\n> > -of OIDs in hex, one OID per line. (Cannot be combined with\n> > -`--stdin-packs` or `--reachable`.)\n> > +of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n> > +(either directly, or by peeling tags) are silently ignored. OIDs that\n> > +are malformed, or do not exist generate an error. (Cannot be combined\n> > +with `--stdin-packs` or `--reachable`.)\n>\n> Yeah, I think these semantics are good.\n>\n> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> > index 9eec68572f..3637d079fb 100644\n> > --- a/builtin/commit-graph.c\n> > +++ b/builtin/commit-graph.c\n> > @@ -153,13 +153,14 @@ static int read_one_commit(struct oidset *commits, struct progress *progress,\n> >\n> > \tdisplay_progress(progress, oidset_size(commits) + 1);\n> >\n> > +\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n> > +\t\terror(_(\"object %s does not exist\"), hash);\n> > +\t\treturn 1;\n> > +\t}\n> > +\n> > \tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n> > \tif (result)\n> > \t\toidset_insert(commits, &result->object.oid);\n> > -\telse {\n> > -\t\terror(_(\"invalid commit object id: %s\"), hash);\n> > -\t\treturn 1;\n> > -\t}\n> > \treturn 0;\n> > }\n>\n> We can avoid the object-existence check entirely if\n> lookup_commit_reference_gently() gives us an answer. And we'd expect\n> that to be the common path.\n>\n> Also, using has_object_file() is cheaper than oid_object_info(), since\n> it doesn't have to resolve the type for deltas.\n>\n> So perhaps:\n>\n> result = lookup_commit_reference_gently(...);\n> if (result)\n> oidset_insert(...);\n> else if (has_object_file(&oid))\n> ; /* not a commit; quietly ignore;\n> else\n> return error(no such object...);\n>\n> That said, I think this technique misses some cases of corruption.\n> You're checking that the outer-most object exists, but not any\n> intermediate peeled objects. I.e., lookup_commit_reference_gently()\n> might have failed for two reasons:\n>\n> - an object it peeled to didn't exist\n>\n> - an object it peeled to wasn't a commit\n>\n> To do it thoroughly, I think you'd have to call deref_tag() yourself and\n> distinguish NULL there (an error) from a result where obj->type isn't\n> OBJ_COMMIT (quietly ignore).\n\nMakes sense. I initially worried a little bit about what to call the\nerror message (i.e., is is \"this object doesn't exist\" or \"this object\nexists but peels to a non-commit, or an otherwise missing object\"). But,\nI think just saying \"invalid object: \" is good enough here.\n\n> > enum commit_graph_write_flags {\n> > -\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n> > -\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n> > -\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n> > -\t/* Make sure that each OID in the input is a valid commit OID. */\n> > -\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),\n> > -\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),\n> > +\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n> > +\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n> > +\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n> > +\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3)\n>\n> As much as I love looking at matched-indentation lists, I think this\n> diff is a good example of why it's not worth doing. It's much easier to\n> see what's going on if the first three items aren't touched. I'd\n> actually even leave BLOOM_FILTERS where it is, and accept the hole which\n> could be refilled later.\n>\n> Your patch also loses the trailing comma after the final BLOOM_FILTERS\n> entry.\n\nFixed both, thanks. I'll build these eight new patches and send them\nshortly, thanks for your review.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"397778","messageId":"cover.1589407014.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1588641176.git.me@ttaylorr.com","subject":"[PATCH v3 0/8] commit-graph: drop CHECK_OIDS, peel in callers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:25Z","receivedAt":"2020-05-13T21:59:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere is a tiny reroll to my series to drop the commit-graph's behavior\nof complaining about non-commit OIDs on input with '--stdin-commits'. I\npromised this reroll after Peff's review beginning in [1].\n\nNot a great deal has changed, but I mostly took every suggestion that\nPeff put forward (in the sub-thread beginning at [1]). The \"big deals\"\nsince last time are:\n\n - 'add_ref_to_set()' got cleaned up to use 'oid_object_info'.\n\n - progress meter updates moved after their action\n\n - 'graph_write()' got cleaned up to break the loop into two loops\n executed conditionally (based on which of '--stdin-packs' or\n '--stdin-commits' was passed) , as well as a cleanup label was\n introduced and some UNLEAK cruft was cleaned up.\n\nFor convenience, a range-diff since v2 (with the changes on master\napplied since then elided out) is included below.\n\nThanks again for your review.\n\n-Taylor\n\n[1]: https://lore.kernel.org/git/20200507195441.GA29683@coredump.intra.peff.net/\n\nTaylor Blau (8):\n commit-graph.c: extract 'refs_cb_data'\n commit-graph.c: show progress of finding reachable commits\n commit-graph.c: peel refs in 'add_ref_to_set'\n builtin/commit-graph.c: extract 'read_one_commit()'\n builtin/commit-graph.c: dereference tags in builtin\n commit-graph.c: simplify 'fill_oids_from_commits'\n t5318: reorder test below 'graph_read_expect'\n commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag\n\n Documentation/git-commit-graph.txt | 6 ++-\n builtin/commit-graph.c | 73 +++++++++++++++++++-----------\n commit-graph.c | 62 +++++++++++--------------\n commit-graph.h | 4 +-\n t/t5318-commit-graph.sh | 25 ++++++----\n 5 files changed, 95 insertions(+), 75 deletions(-)\n\nRange-diff against v2:\n 1: 43286c3c45 = 60: 0efbcfcf3a commit-graph.c: extract 'refs_cb_data'\n 2: cb56a9a73b ! 61: 773522c745 commit-graph.c: show progress of finding reachable commits\n @@ commit-graph.c: static void compute_bloom_filters(struct write_commit_graph_cont\n\n static int add_ref_to_set(const char *refname,\n @@ commit-graph.c: static int add_ref_to_set(const char *refname,\n - {\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n\n -+\tdisplay_progress(data->progress, oidset_size(data->commits) + 1);\n -+\n \toidset_insert(data->commits, oid);\n ++\n ++\tdisplay_progress(data->progress, oidset_size(data->commits));\n ++\n \treturn 0;\n }\n +\n @@ commit-graph.c: int write_commit_graph_reachable(struct object_directory *odb,\n\n \tmemset(&data, 0, sizeof(data));\n 3: 85c388a077 < -: ---------- commit-graph.c: peel refs in 'add_ref_to_set'\n -: ---------- > 62: 74b424f1ac commit-graph.c: peel refs in 'add_ref_to_set'\n 4: cef441b465 ! 63: c37e94907b builtin/commit-graph.c: extract 'read_one_commit()'\n @@ builtin/commit-graph.c: static int write_option_parse_split(const struct option\n \treturn 0;\n }\n\n -+static int read_one_commit(struct oidset *commits, char *hash)\n ++static int read_one_commit(struct oidset *commits, const char *hash)\n +{\n +\tstruct object_id oid;\n +\tconst char *end;\n +\n -+\tif (parse_oid_hex(hash, &oid, &end)) {\n -+\t\terror(_(\"unexpected non-hex object ID: %s\"), hash);\n -+\t\treturn 1;\n -+\t}\n ++\tif (parse_oid_hex(hash, &oid, &end))\n ++\t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n +\n +\toidset_insert(commits, &oid);\n +\treturn 0;\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t}\n\n -\tstring_list_init(&lines, 0);\n -+\tstring_list_init(&pack_indexes, 0);\n - \tif (opts.stdin_packs || opts.stdin_commits) {\n - \t\tstruct strbuf buf = STRBUF_INIT;\n --\n --\t\twhile (strbuf_getline(&buf, stdin) != EOF)\n +-\tif (opts.stdin_packs || opts.stdin_commits) {\n +-\t\tstruct strbuf buf = STRBUF_INIT;\n ++\tstruct strbuf buf = STRBUF_INIT;\n ++\tif (opts.stdin_packs) {\n ++\t\tstring_list_init(&pack_indexes, 0);\n +\n + \t\twhile (strbuf_getline(&buf, stdin) != EOF)\n -\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n --\n ++\t\t\tstring_list_append(&pack_indexes,\n ++\t\t\t\t\t strbuf_detach(&buf, NULL));\n ++\t} else if (opts.stdin_commits) {\n ++\t\toidset_init(&commits, 0);\n ++\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n +\n -\t\tif (opts.stdin_packs)\n -\t\t\tpack_indexes = &lines;\n - \t\tif (opts.stdin_commits) {\n +-\t\tif (opts.stdin_commits) {\n -\t\t\tstruct string_list_item *item;\n -\t\t\toidset_init(&commits, lines.nr);\n -\t\t\tfor_each_string_list_item(item, &lines) {\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n -\t\t\t\t}\n -\n -\t\t\t\toidset_insert(&commits, &oid);\n --\t\t\t}\n -+\t\t\toidset_init(&commits, 0);\n - \t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n - \t\t}\n -\n +\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n -+\t\t\tchar *line = strbuf_detach(&buf, NULL);\n -+\t\t\tif (opts.stdin_commits) {\n -+\t\t\t\tint result = read_one_commit(&commits, line);\n -+\t\t\t\tif (result)\n -+\t\t\t\t\treturn result;\n -+\t\t\t} else\n -+\t\t\t\tstring_list_append(&pack_indexes, line);\n -+\t\t}\n -+\n - \t\tUNLEAK(buf);\n ++\t\t\tif (read_one_commit(&commits, buf.buf)) {\n ++\t\t\t\tresult = 1;\n ++\t\t\t\tgoto cleanup;\n + \t\t\t}\n +-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n + \t\t}\n +-\n +-\t\tUNLEAK(buf);\n \t}\n\n \tif (write_commit_graph(odb,\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t\tresult = 1;\n\n -\tUNLEAK(lines);\n ++cleanup:\n +\tUNLEAK(pack_indexes);\n ++\tstrbuf_release(&buf);\n \treturn result;\n }\n\n 5: d449d83ce2 < -: ---------- builtin/commit-graph.c: dereference tags in builtin\n -: ---------- > 64: 56403dd377 builtin/commit-graph.c: dereference tags in builtin\n 6: 61887870c7 = 65: 6afb08a927 commit-graph.c: simplify 'fill_oids_from_commits'\n 7: e393b16097 = 66: 8fdd2792df t5318: reorder test below 'graph_read_expect'\n 8: ad373f05ff ! 67: 3cb0bd306c commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag\n @@ Commit message\n If callers do wish to retain this behavior, they can easily work around\n this change by doing the following:\n\n - git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n - awk '/commit/ { print $1 }' |\n - git commit-graph write --stdin-commits\n + git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n + awk '\n + !/commit/ { print \"not-a-commit:\"$1 }\n + /commit/ { print $1 }\n + ' |\n + git commit-graph write --stdin-commits\n\n To make it so that valid OIDs that refer to non-existent objects are\n indeed an error after loosening the error handling, perform an extra\n lookup to make sure that object indeed exists before sending it to the\n commit-graph internals.\n\n + Helped-by: Jeff King \n Signed-off-by: Taylor Blau \n\n ## Documentation/git-commit-graph.txt ##\n @@ Documentation/git-commit-graph.txt: with `--stdin-commits` or `--reachable`.)\n commits starting at all refs. (Cannot be combined with `--stdin-commits`\n\n ## builtin/commit-graph.c ##\n -@@ builtin/commit-graph.c: static int read_one_commit(struct oidset *commits, struct progress *progress,\n +@@\n + #include \"commit-graph.h\"\n + #include \"object-store.h\"\n + #include \"progress.h\"\n ++#include \"tag.h\"\n\n - \tdisplay_progress(progress, oidset_size(commits) + 1);\n + static char const * const builtin_commit_graph_usage[] = {\n + \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n +@@ builtin/commit-graph.c: static int write_option_parse_split(const struct option *opt, const char *arg,\n + static int read_one_commit(struct oidset *commits, struct progress *progress,\n + \t\t\t const char *hash)\n + {\n +-\tstruct commit *result;\n ++\tstruct object *result;\n + \tstruct object_id oid;\n + \tconst char *end;\n\n -+\tif (oid_object_info(the_repository, &oid, NULL) < 0) {\n -+\t\terror(_(\"object %s does not exist\"), hash);\n -+\t\treturn 1;\n -+\t}\n -+\n - \tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n - \tif (result)\n - \t\toidset_insert(commits, &result->object.oid);\n --\telse {\n --\t\terror(_(\"invalid commit object id: %s\"), hash);\n --\t\treturn 1;\n --\t}\n - \treturn 0;\n - }\n + \tif (parse_oid_hex(hash, &oid, &end))\n + \t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n +\n +-\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n +-\tif (result)\n +-\t\toidset_insert(commits, &result->object.oid);\n +-\telse\n +-\t\treturn error(_(\"invalid commit object id: %s\"), hash);\n ++\tresult = deref_tag(the_repository, parse_object(the_repository, &oid),\n ++\t\t\t NULL, 0);\n ++\tif (!result)\n ++\t\treturn error(_(\"invalid object: %s\"), hash);\n ++\telse if (object_as_type(the_repository, result, OBJ_COMMIT, 1))\n ++\t\toidset_insert(commits, &result->oid);\n +\n + \tdisplay_progress(progress, oidset_size(commits));\n\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n - \t\tstruct strbuf buf = STRBUF_INIT;\n - \t\tif (opts.stdin_commits) {\n - \t\t\toidset_init(&commits, 0);\n --\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n - \t\t\tif (opts.progress)\n - \t\t\t\tprogress = start_delayed_progress(\n - \t\t\t\t\t_(\"Collecting commits from input\"), 0);\n + \t\t\t\t\t strbuf_detach(&buf, NULL));\n + \t} else if (opts.stdin_commits) {\n + \t\toidset_init(&commits, 0);\n +-\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n + \t\tif (opts.progress)\n + \t\t\tprogress = start_delayed_progress(\n + \t\t\t\t_(\"Collecting commits from input\"), 0);\n\n ## commit-graph.c ##\n @@ commit-graph.c: struct write_commit_graph_context {\n @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n \tctx->total_bloom_filter_data_size = 0;\n\n ## commit-graph.h ##\n -@@ commit-graph.h: struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n - int generation_numbers_enabled(struct repository *r);\n -\n - enum commit_graph_write_flags {\n --\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n --\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n --\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n +@@ commit-graph.h: enum commit_graph_write_flags {\n + \tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n + \tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n + \tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n -\t/* Make sure that each OID in the input is a valid commit OID. */\n -\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),\n -\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),\n -+\tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n -+\tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n -+\tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n -+\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3)\n ++\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),\n };\n\n enum commit_graph_split_flags {\n @@ t/t5318-commit-graph.sh: graph_read_expect() {\n +\t# non-existent OID\n +\techo $ZERO_OID >in &&\n +\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n -+\ttest_i18ngrep \"does not exist\" stderr &&\n ++\ttest_i18ngrep \"invalid object\" stderr &&\n +\t# valid commit and tree OID\n +\tgit rev-parse HEAD HEAD^{tree} >in &&\n +\tgit commit-graph write --stdin-commits \n---\n commit-graph.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e3420ddcbf..9693112063 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1319,13 +1319,17 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \tstop_progress(&progress);\n }\n \n+struct refs_cb_data {\n+\tstruct oidset *commits;\n+};\n+\n static int add_ref_to_set(const char *refname,\n \t\t\t const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n-\tstruct oidset *commits = (struct oidset *)cb_data;\n+\tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n-\toidset_insert(commits, oid);\n+\toidset_insert(data->commits, oid);\n \treturn 0;\n }\n \n@@ -1334,9 +1338,13 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \t\t\t\t const struct split_commit_graph_opts *split_opts)\n {\n \tstruct oidset commits = OIDSET_INIT;\n+\tstruct refs_cb_data data;\n \tint result;\n \n-\tfor_each_ref(add_ref_to_set, &commits);\n+\tmemset(&data, 0, sizeof(data));\n+\tdata.commits = &commits;\n+\n+\tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397781","messageId":"773522c745c23a4c09f5c198bcb8f33c143c095e.1589407014.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 2/8] commit-graph.c: show progress of finding reachable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:33Z","receivedAt":"2020-05-13T21:59:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When 'git commit-graph write --reachable' is invoked, the commit-graph\nmachinery calls 'for_each_ref()' to discover the set of reachable\ncommits.\n\nRight now the 'add_ref_to_set' callback is not doing anything other than\nadding an OID to the set of known-reachable OIDs. In a subsequent\ncommit, 'add_ref_to_set' will presumptively peel references. This\noperation should be fast for repositories with an up-to-date\n'$GIT_DIR/packed-refs', but may be slow in the general case.\n\nSo that it doesn't appear that 'git commit-graph write' is idling with\n'--reachable' in the slow case, add a progress meter to provide some\noutput in the meantime.\n\nIn general, we don't expect a progress meter to appear at all, since\npeeling references with a 'packed-refs' file is quick. If it's slow and\nwe do show a progress meter, the subsequent 'fill_oids_from_commits()'\nwill be fast, since all of the calls to\n'lookup_commit_reference_gently()' will be no-ops.\n\nBoth progress meters are delayed, so it is unlikely that more than one\nwill appear. In either case, this intermediate state will go away in a\nhandful of patches, at which point there will be at most one progress\nmeter.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9693112063..f855911320 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1321,6 +1321,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \n struct refs_cb_data {\n \tstruct oidset *commits;\n+\tstruct progress *progress;\n };\n \n static int add_ref_to_set(const char *refname,\n@@ -1330,6 +1331,9 @@ static int add_ref_to_set(const char *refname,\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n \toidset_insert(data->commits, oid);\n+\n+\tdisplay_progress(data->progress, oidset_size(data->commits));\n+\n \treturn 0;\n }\n \n@@ -1343,12 +1347,17 @@ int write_commit_graph_reachable(struct object_directory *odb,\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.commits = &commits;\n+\tif (flags & COMMIT_GRAPH_WRITE_PROGRESS)\n+\t\tdata.progress = start_delayed_progress(\n+\t\t\t_(\"Collecting referenced commits\"), 0);\n \n \tfor_each_ref(add_ref_to_set, &data);\n \tresult = write_commit_graph(odb, NULL, &commits,\n \t\t\t\t flags, split_opts);\n \n \toidset_clear(&commits);\n+\tif (data.progress)\n+\t\tstop_progress(&data.progress);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397780","messageId":"74b424f1ac8168f599c73364e42581961ab2c6fd.1589407014.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 3/8] commit-graph.c: peel refs in 'add_ref_to_set'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:37Z","receivedAt":"2020-05-13T21:59:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While iterating references (to discover the set of commits to write to\nthe commit-graph with 'git commit-graph write --reachable'),\n'add_ref_to_set' can save 'fill_oids_from_commits()' some time by\npeeling the references beforehand.\n\nMove peeling out of 'fill_oids_from_commits()' and into\n'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing\nso allows the commit-graph machinery to use the peeled value from\n'$GIT_DIR/packed-refs' instead of having to load and parse tags.\n\nWhile we're at it, discard non-commit objects reachable from ref tips.\nThis would be done automatically by 'fill_oids_from_commits()', but such\nfunctionality will be removed in a subsequent patch after the call to\n'lookup_commit_reference_gently' is dropped (at which point a non-commit\nobject in the commits oidset will become an error).\n\nSuggested-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f855911320..e7de91f0f1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1328,9 +1328,13 @@ static int add_ref_to_set(const char *refname,\n \t\t\t const struct object_id *oid,\n \t\t\t int flags, void *cb_data)\n {\n+\tstruct object_id peeled;\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n-\toidset_insert(data->commits, oid);\n+\tif (!peel_ref(refname, &peeled))\n+\t\toid = &peeled;\n+\tif (oid_object_info(the_repository, oid, NULL) == OBJ_COMMIT)\n+\t\toidset_insert(data->commits, oid);\n \n \tdisplay_progress(data->progress, oidset_size(data->commits));\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397782","messageId":"c37e94907b140f3d2e5a44293f2c1faa6d473712.1589407014.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:41Z","receivedAt":"2020-05-13T21:59:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"With either '--stdin-commits' or '--stdin-packs', the commit-graph\nbuiltin will read line-delimited input, and interpret it either as a\nseries of commit OIDs, or pack names.\n\nIn a subsequent commit, we will begin handling '--stdin-commits'\ndifferently by processing each line as it comes in, instead of in one\nshot at the end. To make adequate room for this additional logic, split\nthe '--stdin-commits' case from '--stdin-packs' by only storing the\ninput when '--stdin-packs' is given.\n\nIn the case of '--stdin-commits', feed each line to a new\n'read_one_commit' helper, which (for now) will merely call\n'parse_oid_hex'.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 56 ++++++++++++++++++++++--------------------\n 1 file changed, 29 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 15fe60317c..f6647449ed 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -138,12 +138,23 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static int read_one_commit(struct oidset *commits, const char *hash)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\n+\tif (parse_oid_hex(hash, &oid, &end))\n+\t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n+\n+\toidset_insert(commits, &oid);\n+\treturn 0;\n+}\n+\n static int graph_write(int argc, const char **argv)\n {\n-\tstruct string_list *pack_indexes = NULL;\n+\tstruct string_list pack_indexes;\n \tstruct oidset commits = OIDSET_INIT;\n \tstruct object_directory *odb = NULL;\n-\tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n \n@@ -209,44 +220,35 @@ static int graph_write(int argc, const char **argv)\n \t\treturn 0;\n \t}\n \n-\tstring_list_init(&lines, 0);\n-\tif (opts.stdin_packs || opts.stdin_commits) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tif (opts.stdin_packs) {\n+\t\tstring_list_init(&pack_indexes, 0);\n \n \t\twhile (strbuf_getline(&buf, stdin) != EOF)\n-\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n+\t\t\tstring_list_append(&pack_indexes,\n+\t\t\t\t\t strbuf_detach(&buf, NULL));\n+\t} else if (opts.stdin_commits) {\n+\t\toidset_init(&commits, 0);\n+\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \n-\t\tif (opts.stdin_packs)\n-\t\t\tpack_indexes = &lines;\n-\t\tif (opts.stdin_commits) {\n-\t\t\tstruct string_list_item *item;\n-\t\t\toidset_init(&commits, lines.nr);\n-\t\t\tfor_each_string_list_item(item, &lines) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tconst char *end;\n-\n-\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n-\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n-\t\t\t\t\t\t\"%s\"), item->string);\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\toidset_insert(&commits, &oid);\n+\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\t\tif (read_one_commit(&commits, buf.buf)) {\n+\t\t\t\tresult = 1;\n+\t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n-\n-\t\tUNLEAK(buf);\n \t}\n \n \tif (write_commit_graph(odb,\n-\t\t\t pack_indexes,\n+\t\t\t opts.stdin_packs ? &pack_indexes : NULL,\n \t\t\t opts.stdin_commits ? &commits : NULL,\n \t\t\t flags,\n \t\t\t &split_opts))\n \t\tresult = 1;\n \n-\tUNLEAK(lines);\n+cleanup:\n+\tUNLEAK(pack_indexes);\n+\tstrbuf_release(&buf);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397783","messageId":"56403dd377b996d21a4da1e01ffd6e691ac120bd.1589407014.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:44Z","receivedAt":"2020-05-13T21:59:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When given a list of commits, the commit-graph machinery calls\n'lookup_commit_reference_gently()' on each element in the set and treats\nthe resulting set of OIDs as the base over which to close for\nreachability.\n\nIn an earlier collection of commits, the 'git commit-graph write\n--reachable' case made the inner-most call to\n'lookup_commit_reference_gently()' by peeling references before they\nwere passed over to the commit-graph internals.\n\nDo the analog for 'git commit-graph write --stdin-commits' by calling\n'lookup_commit_reference_gently()' outside of the commit-graph\nmachinery, making the inner-most call a noop.\n\nSince this may incur additional processing time, surround\n'read_one_commit' with a progress meter to provide output to the caller.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex f6647449ed..83c55d9227 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -6,6 +6,7 @@\n #include \"repository.h\"\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n+#include \"progress.h\"\n \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n@@ -138,15 +139,24 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static int read_one_commit(struct oidset *commits, const char *hash)\n+static int read_one_commit(struct oidset *commits, struct progress *progress,\n+\t\t\t const char *hash)\n {\n+\tstruct commit *result;\n \tstruct object_id oid;\n \tconst char *end;\n \n \tif (parse_oid_hex(hash, &oid, &end))\n \t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n \n-\toidset_insert(commits, &oid);\n+\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n+\tif (result)\n+\t\toidset_insert(commits, &result->object.oid);\n+\telse\n+\t\treturn error(_(\"invalid commit object id: %s\"), hash);\n+\n+\tdisplay_progress(progress, oidset_size(commits));\n+\n \treturn 0;\n }\n \n@@ -157,6 +167,7 @@ static int graph_write(int argc, const char **argv)\n \tstruct object_directory *odb = NULL;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n+\tstruct progress *progress = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n \t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n@@ -230,13 +241,18 @@ static int graph_write(int argc, const char **argv)\n \t} else if (opts.stdin_commits) {\n \t\toidset_init(&commits, 0);\n \t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n+\t\tif (opts.progress)\n+\t\t\tprogress = start_delayed_progress(\n+\t\t\t\t_(\"Collecting commits from input\"), 0);\n \n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\t\tif (read_one_commit(&commits, buf.buf)) {\n+\t\t\tif (read_one_commit(&commits, progress, buf.buf)) {\n \t\t\t\tresult = 1;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n+\n+\n \t}\n \n \tif (write_commit_graph(odb,\n@@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv)\n cleanup:\n \tUNLEAK(pack_indexes);\n \tstrbuf_release(&buf);\n+\tif (progress)\n+\t\tstop_progress(&progress);\n \treturn result;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397784","messageId":"6afb08a927b89e8f9cc570126d50669436abcae9.1589407015.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 6/8] commit-graph.c: simplify 'fill_oids_from_commits'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:47Z","receivedAt":"2020-05-13T21:59:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous handful of commits, both 'git commit-graph write\n--reachable' and '--stdin-commits' learned to peel tags down to the\ncommits which they refer to before passing them into the commit-graph\ninternals.\n\nThis makes the call to 'lookup_commit_reference_gently()' inside of\n'fill_oids_from_commits()' a noop, since all OIDs are commits by that\npoint.\n\nAs such, remove the call entirely, as well as the progress meter, which\nhas been split and moved out to the callers in the aforementioned\nearlier commits.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 33 +++------------------------------\n 1 file changed, 3 insertions(+), 30 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e7de91f0f1..3ea70e6676 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1413,46 +1413,19 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n static int fill_oids_from_commits(struct write_commit_graph_context *ctx,\n \t\t\t\t struct oidset *commits)\n {\n-\tuint32_t i = 0;\n-\tstruct strbuf progress_title = STRBUF_INIT;\n \tstruct oidset_iter iter;\n \tstruct object_id *oid;\n \n \tif (!oidset_size(commits))\n \t\treturn 0;\n \n-\tif (ctx->report_progress) {\n-\t\tstrbuf_addf(&progress_title,\n-\t\t\t Q_(\"Finding commits for commit graph from %d ref\",\n-\t\t\t \"Finding commits for commit graph from %d refs\",\n-\t\t\t oidset_size(commits)),\n-\t\t\t oidset_size(commits));\n-\t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\tprogress_title.buf,\n-\t\t\t\t\toidset_size(commits));\n-\t}\n-\n \toidset_iter_init(commits, &iter);\n \twhile ((oid = oidset_iter_next(&iter))) {\n-\t\tstruct commit *result;\n-\n-\t\tdisplay_progress(ctx->progress, ++i);\n-\n-\t\tresult = lookup_commit_reference_gently(ctx->r, oid, 1);\n-\t\tif (result) {\n-\t\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n-\t\t\toidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));\n-\t\t\tctx->oids.nr++;\n-\t\t} else if (ctx->check_oids) {\n-\t\t\terror(_(\"invalid commit object id: %s\"),\n-\t\t\t oid_to_hex(oid));\n-\t\t\treturn -1;\n-\t\t}\n+\t\tALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);\n+\t\toidcpy(&ctx->oids.list[ctx->oids.nr], oid);\n+\t\tctx->oids.nr++;\n \t}\n \n-\tstop_progress(&ctx->progress);\n-\tstrbuf_release(&progress_title);\n-\n \treturn 0;\n }\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397785","messageId":"8fdd2792dfd52bb304762a0faf7849bd55954d68.1589407015.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 7/8] t5318: reorder test below 'graph_read_expect'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:51Z","receivedAt":"2020-05-13T21:59:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the subsequent commit, we will introduce a dependency on\n'graph_read_expect' from t5318.7. Preemptively move it below\n'graph_read_expect()'s definition so that the test can call it.\n\nSigned-off-by: Taylor Blau \n---\n t/t5318-commit-graph.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 424599959c..255e3bb1c9 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -46,15 +46,6 @@ test_expect_success 'create commits and repack' '\n \tgit repack\n '\n \n-test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n-\tcd \"$TRASH_DIRECTORY/full\" &&\n-\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n-\t# valid tree OID, but not a commit OID\n-\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"invalid commit object id\" stderr\n-'\n-\n graph_git_two_modes() {\n \tgit -c core.commitGraph=true $1 >output\n \tgit -c core.commitGraph=false $1 >expect\n@@ -95,6 +86,15 @@ graph_read_expect() {\n \ttest_cmp expect output\n }\n \n+test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n+\t# valid tree OID, but not a commit OID\n+\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\ttest_i18ngrep \"invalid commit object id\" stderr\n+'\n+\n test_expect_success 'write graph' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write &&\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"397786","messageId":"3cb0bd306cbb971715f84db07a8d1acac658f2d0.1589407015.git.me@ttaylorr.com","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"[PATCH v3 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-13T21:59:55Z","receivedAt":"2020-05-13T21:59:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since 7c5c9b9c57 (commit-graph: error out on invalid commit oids in\n'write --stdin-commits', 2019-08-05), the commit-graph builtin dies on\nreceiving non-commit OIDs as input to '--stdin-commits'.\n\nThis behavior can be cumbersome to work around in, say, the case of\npiping 'git for-each-ref' to 'git commit-graph write --stdin-commits' if\nthe caller does not want to cull out non-commits themselves. In this\nsituation, it would be ideal if 'git commit-graph write' wrote the graph\ncontaining the inputs that did pertain to commits, and silently ignored\nthe remainder of the input.\n\nSome options have been proposed to the effect of '--[no-]check-oids'\nwhich would allow callers to have the commit-graph builtin do just that.\nAfter some discussion, it is difficult to imagine a caller who wouldn't\nwant to pass '--no-check-oids', suggesting that we should get rid of the\nbehavior of complaining about non-commit inputs altogether.\n\nIf callers do wish to retain this behavior, they can easily work around\nthis change by doing the following:\n\n git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |\n awk '\n !/commit/ { print \"not-a-commit:\"$1 }\n /commit/ { print $1 }\n ' |\n git commit-graph write --stdin-commits\n\nTo make it so that valid OIDs that refer to non-existent objects are\nindeed an error after loosening the error handling, perform an extra\nlookup to make sure that object indeed exists before sending it to the\ncommit-graph internals.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/git-commit-graph.txt | 6 ++++--\n builtin/commit-graph.c | 15 ++++++++-------\n commit-graph.c | 2 --\n commit-graph.h | 4 +---\n t/t5318-commit-graph.sh | 15 +++++++++++----\n 5 files changed, 24 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt\nindex 53a650225a..fcac7d12e1 100644\n--- a/Documentation/git-commit-graph.txt\n+++ b/Documentation/git-commit-graph.txt\n@@ -47,8 +47,10 @@ with `--stdin-commits` or `--reachable`.)\n +\n With the `--stdin-commits` option, generate the new commit graph by\n walking commits starting at the commits specified in stdin as a list\n-of OIDs in hex, one OID per line. (Cannot be combined with\n-`--stdin-packs` or `--reachable`.)\n+of OIDs in hex, one OID per line. OIDs that resolve to non-commits\n+(either directly, or by peeling tags) are silently ignored. OIDs that\n+are malformed, or do not exist generate an error. (Cannot be combined\n+with `--stdin-packs` or `--reachable`.)\n +\n With the `--reachable` option, generate the new commit graph by walking\n commits starting at all refs. (Cannot be combined with `--stdin-commits`\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 83c55d9227..d3bd1c31c9 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -7,6 +7,7 @@\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n #include \"progress.h\"\n+#include \"tag.h\"\n \n static char const * const builtin_commit_graph_usage[] = {\n \tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n@@ -142,18 +143,19 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n static int read_one_commit(struct oidset *commits, struct progress *progress,\n \t\t\t const char *hash)\n {\n-\tstruct commit *result;\n+\tstruct object *result;\n \tstruct object_id oid;\n \tconst char *end;\n \n \tif (parse_oid_hex(hash, &oid, &end))\n \t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n \n-\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n-\tif (result)\n-\t\toidset_insert(commits, &result->object.oid);\n-\telse\n-\t\treturn error(_(\"invalid commit object id: %s\"), hash);\n+\tresult = deref_tag(the_repository, parse_object(the_repository, &oid),\n+\t\t\t NULL, 0);\n+\tif (!result)\n+\t\treturn error(_(\"invalid object: %s\"), hash);\n+\telse if (object_as_type(the_repository, result, OBJ_COMMIT, 1))\n+\t\toidset_insert(commits, &result->oid);\n \n \tdisplay_progress(progress, oidset_size(commits));\n \n@@ -240,7 +242,6 @@ static int graph_write(int argc, const char **argv)\n \t\t\t\t\t strbuf_detach(&buf, NULL));\n \t} else if (opts.stdin_commits) {\n \t\toidset_init(&commits, 0);\n-\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\tif (opts.progress)\n \t\t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Collecting commits from input\"), 0);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 3ea70e6676..2ff042fbf4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -881,7 +881,6 @@ struct write_commit_graph_context {\n \tunsigned append:1,\n \t\t report_progress:1,\n \t\t split:1,\n-\t\t check_oids:1,\n \t\t changed_paths:1,\n \t\t order_by_pack:1;\n \n@@ -2011,7 +2010,6 @@ int write_commit_graph(struct object_directory *odb,\n \tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n \tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n-\tctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;\n \tctx->split_opts = split_opts;\n \tctx->changed_paths = flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS ? 1 : 0;\n \tctx->total_bloom_filter_data_size = 0;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 4212766a4f..3ba0da1e5f 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -91,9 +91,7 @@ enum commit_graph_write_flags {\n \tCOMMIT_GRAPH_WRITE_APPEND = (1 << 0),\n \tCOMMIT_GRAPH_WRITE_PROGRESS = (1 << 1),\n \tCOMMIT_GRAPH_WRITE_SPLIT = (1 << 2),\n-\t/* Make sure that each OID in the input is a valid commit OID. */\n-\tCOMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),\n-\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),\n+\tCOMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),\n };\n \n enum commit_graph_split_flags {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 255e3bb1c9..a79c624875 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -88,11 +88,18 @@ graph_read_expect() {\n \n test_expect_success 'exit with correct error on bad input to --stdin-commits' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-\techo HEAD | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n+\t# invalid, non-hex OID\n+\techo HEAD >in &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n \ttest_i18ngrep \"unexpected non-hex object ID: HEAD\" stderr &&\n-\t# valid tree OID, but not a commit OID\n-\tgit rev-parse HEAD^{tree} | test_expect_code 1 git commit-graph write --stdin-commits 2>stderr &&\n-\ttest_i18ngrep \"invalid commit object id\" stderr\n+\t# non-existent OID\n+\techo $ZERO_OID >in &&\n+\ttest_expect_code 1 git commit-graph write --stdin-commits stderr &&\n+\ttest_i18ngrep \"invalid object\" stderr &&\n+\t# valid commit and tree OID\n+\tgit rev-parse HEAD HEAD^{tree} >in &&\n+\tgit commit-graph write --stdin-commits @@ -209,44 +220,35 @@ static int graph_write(int argc, const char **argv)\n> \t\treturn 0;\n> \t}\n> \n> -\tstring_list_init(&lines, 0);\n> -\tif (opts.stdin_packs || opts.stdin_commits) {\n> -\t\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tif (opts.stdin_packs) {\n\nThis is a decl-after-statement, isn't it? It should fail with\nDEVELOPER=1.\n\nThe strbuf could be local in each block of the conditional, though that\nmakes your cleanup label trickier. Only the stdin_commits side needs to\nclean up the buf, so we could just do it there. Or I'd be OK with having\nthe strbuf declared at the top of the function, and using the\nwhole-function cleanup label.\n\n-Peff\n"},{"id":"397811","messageId":"20200514180150.GB2430834@coredump.intra.peff.net","threadId":"53395","inReplyTo":"56403dd377b996d21a4da1e01ffd6e691ac120bd.1589407014.git.me@ttaylorr.com","subject":"Re: [PATCH v3 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-14T18:01:50Z","receivedAt":"2020-05-14T18:01:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 03:59:44PM -0600, Taylor Blau wrote:\n\n> -static int read_one_commit(struct oidset *commits, const char *hash)\n> +static int read_one_commit(struct oidset *commits, struct progress *progress,\n> +\t\t\t const char *hash)\n> {\n> +\tstruct commit *result;\n> \tstruct object_id oid;\n> \tconst char *end;\n> \n> \tif (parse_oid_hex(hash, &oid, &end))\n> \t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n> \n> -\toidset_insert(commits, &oid);\n> +\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n> +\tif (result)\n> +\t\toidset_insert(commits, &result->object.oid);\n> +\telse\n> +\t\treturn error(_(\"invalid commit object id: %s\"), hash);\n> +\n> +\tdisplay_progress(progress, oidset_size(commits));\n> +\n\nI expected this to switch to deref_tag() here, but it looks like you do\nit in the final commit. That makes sense, since this step is really just\ncopying the existing logic.\n\n> @@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv)\n> cleanup:\n> \tUNLEAK(pack_indexes);\n> \tstrbuf_release(&buf);\n> +\tif (progress)\n> +\t\tstop_progress(&progress);\n> \treturn result;\n\nReally minor nit, but stop_progress(), like display_progress(), handles\nNULL already. So you can lose the \"if\" here.\n\n-Peff\n"},{"id":"397812","messageId":"20200514180200.GA86181@syl.local","threadId":"53395","inReplyTo":"20200514175646.GA2430834@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-14T18:02:00Z","receivedAt":"2020-05-14T18:02:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 14, 2020 at 01:56:46PM -0400, Jeff King wrote:\n> On Wed, May 13, 2020 at 03:59:41PM -0600, Taylor Blau wrote:\n>\n> > @@ -209,44 +220,35 @@ static int graph_write(int argc, const char **argv)\n> > \t\treturn 0;\n> > \t}\n> >\n> > -\tstring_list_init(&lines, 0);\n> > -\tif (opts.stdin_packs || opts.stdin_commits) {\n> > -\t\tstruct strbuf buf = STRBUF_INIT;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tif (opts.stdin_packs) {\n>\n> This is a decl-after-statement, isn't it? It should fail with\n> DEVELOPER=1.\n\nWhoops. I'm not sure how I missed this.\n\n> The strbuf could be local in each block of the conditional, though that\n> makes your cleanup label trickier. Only the stdin_commits side needs to\n> clean up the buf, so we could just do it there. Or I'd be OK with having\n> the strbuf declared at the top of the function, and using the\n> whole-function cleanup label.\n\nThankfully, fixing this by moving the declaration/initialization up to\nthe top of the function didn't cause any conflicts with later patches.\n\nJunio: if you're queuing this reroll, please use the below version\ninstead:\n\n--- >8 ---\n\nSubject: [PATCH] builtin/commit-graph.c: extract 'read_one_commit()'\n\nWith either '--stdin-commits' or '--stdin-packs', the commit-graph\nbuiltin will read line-delimited input, and interpret it either as a\nseries of commit OIDs, or pack names.\n\nIn a subsequent commit, we will begin handling '--stdin-commits'\ndifferently by processing each line as it comes in, instead of in one\nshot at the end. To make adequate room for this additional logic, split\nthe '--stdin-commits' case from '--stdin-packs' by only storing the\ninput when '--stdin-packs' is given.\n\nIn the case of '--stdin-commits', feed each line to a new\n'read_one_commit' helper, which (for now) will merely call\n'parse_oid_hex'.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 56 ++++++++++++++++++++++--------------------\n 1 file changed, 29 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 15fe60317c..aaa1d0c738 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -138,12 +138,24 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n\n+static int read_one_commit(struct oidset *commits, const char *hash)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\n+\tif (parse_oid_hex(hash, &oid, &end))\n+\t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n+\n+\toidset_insert(commits, &oid);\n+\treturn 0;\n+}\n+\n static int graph_write(int argc, const char **argv)\n {\n-\tstruct string_list *pack_indexes = NULL;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list pack_indexes;\n \tstruct oidset commits = OIDSET_INIT;\n \tstruct object_directory *odb = NULL;\n-\tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n\n@@ -209,44 +221,34 @@ static int graph_write(int argc, const char **argv)\n \t\treturn 0;\n \t}\n\n-\tstring_list_init(&lines, 0);\n-\tif (opts.stdin_packs || opts.stdin_commits) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n+\tif (opts.stdin_packs) {\n+\t\tstring_list_init(&pack_indexes, 0);\n\n \t\twhile (strbuf_getline(&buf, stdin) != EOF)\n-\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n+\t\t\tstring_list_append(&pack_indexes,\n+\t\t\t\t\t strbuf_detach(&buf, NULL));\n+\t} else if (opts.stdin_commits) {\n+\t\toidset_init(&commits, 0);\n+\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n\n-\t\tif (opts.stdin_packs)\n-\t\t\tpack_indexes = &lines;\n-\t\tif (opts.stdin_commits) {\n-\t\t\tstruct string_list_item *item;\n-\t\t\toidset_init(&commits, lines.nr);\n-\t\t\tfor_each_string_list_item(item, &lines) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tconst char *end;\n-\n-\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n-\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n-\t\t\t\t\t\t\"%s\"), item->string);\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\toidset_insert(&commits, &oid);\n+\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\t\tif (read_one_commit(&commits, buf.buf)) {\n+\t\t\t\tresult = 1;\n+\t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n-\n-\t\tUNLEAK(buf);\n \t}\n\n \tif (write_commit_graph(odb,\n-\t\t\t pack_indexes,\n+\t\t\t opts.stdin_packs ? &pack_indexes : NULL,\n \t\t\t opts.stdin_commits ? &commits : NULL,\n \t\t\t flags,\n \t\t\t &split_opts))\n \t\tresult = 1;\n\n-\tUNLEAK(lines);\n+cleanup:\n+\tUNLEAK(pack_indexes);\n+\tstrbuf_release(&buf);\n \treturn result;\n }\n\n--\n2.26.0.113.ge9739cdccc\n\n"},{"id":"397813","messageId":"20200514180424.GB86181@syl.local","threadId":"53395","inReplyTo":"20200514180150.GB2430834@coredump.intra.peff.net","subject":"Re: [PATCH v3 5/8] builtin/commit-graph.c: dereference tags in builtin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-14T18:04:24Z","receivedAt":"2020-05-14T18:04:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 14, 2020 at 02:01:50PM -0400, Jeff King wrote:\n> On Wed, May 13, 2020 at 03:59:44PM -0600, Taylor Blau wrote:\n>\n> > -static int read_one_commit(struct oidset *commits, const char *hash)\n> > +static int read_one_commit(struct oidset *commits, struct progress *progress,\n> > +\t\t\t const char *hash)\n> > {\n> > +\tstruct commit *result;\n> > \tstruct object_id oid;\n> > \tconst char *end;\n> >\n> > \tif (parse_oid_hex(hash, &oid, &end))\n> > \t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n> >\n> > -\toidset_insert(commits, &oid);\n> > +\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n> > +\tif (result)\n> > +\t\toidset_insert(commits, &result->object.oid);\n> > +\telse\n> > +\t\treturn error(_(\"invalid commit object id: %s\"), hash);\n> > +\n> > +\tdisplay_progress(progress, oidset_size(commits));\n> > +\n>\n> I expected this to switch to deref_tag() here, but it looks like you do\n> it in the final commit. That makes sense, since this step is really just\n> copying the existing logic.\n\nI went back and forth on this. I figure that the behavior change should\ncome in the final commit, so this is keeping things consistent at this\npoint in the series.\n\n> > @@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv)\n> > cleanup:\n> > \tUNLEAK(pack_indexes);\n> > \tstrbuf_release(&buf);\n> > +\tif (progress)\n> > +\t\tstop_progress(&progress);\n> > \treturn result;\n>\n> Really minor nit, but stop_progress(), like display_progress(), handles\n> NULL already. So you can lose the \"if\" here.\n\nMm. If there's nothing else (besides the review of 4/8 which I already\nsent a replacement patch for), I'd prefer to leave this as-is to avoid\nanother reroll. Hopefully since this isn't hurting anything with the\nextra if statement, it shouldn't matter too much.\n\nIf you feel strongly, I'm happy to re-send this series, or this patch.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"397814","messageId":"20200514180947.GC2430834@coredump.intra.peff.net","threadId":"53395","inReplyTo":"3cb0bd306cbb971715f84db07a8d1acac658f2d0.1589407015.git.me@ttaylorr.com","subject":"Re: [PATCH v3 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-14T18:09:47Z","receivedAt":"2020-05-14T18:09:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 03:59:55PM -0600, Taylor Blau wrote:\n\n> -\tresult = lookup_commit_reference_gently(the_repository, &oid, 1);\n> -\tif (result)\n> -\t\toidset_insert(commits, &result->object.oid);\n> -\telse\n> -\t\treturn error(_(\"invalid commit object id: %s\"), hash);\n> +\tresult = deref_tag(the_repository, parse_object(the_repository, &oid),\n> +\t\t\t NULL, 0);\n> +\tif (!result)\n> +\t\treturn error(_(\"invalid object: %s\"), hash);\n\nOK. As you noted earlier, this is really \"we don't have that object\"\n_or_ \"we don't have an object it points to\". But since either is a\ncorruption, and I'd expect parse_object() to produce a more detailed\nmessage anyway, I don't think it's worth trying to get more specific\nhere.\n\n> +\telse if (object_as_type(the_repository, result, OBJ_COMMIT, 1))\n> +\t\toidset_insert(commits, &result->oid);\n\nI suspect this could just be \"if (result->type == OBJ_COMMIT)\", as we'd\nnever see OBJ_NONE from a tag deref (which would have required the\ncasting behavior), but I don't think it hurts to use this function to be\non the safe side.\n\n-Peff\n"},{"id":"397815","messageId":"20200514181226.GD2430834@coredump.intra.peff.net","threadId":"53395","inReplyTo":"cover.1589407014.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/8] commit-graph: drop CHECK_OIDS, peel in callers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-14T18:12:26Z","receivedAt":"2020-05-14T18:12:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2020 at 03:59:25PM -0600, Taylor Blau wrote:\n\n> Here is a tiny reroll to my series to drop the commit-graph's behavior\n> of complaining about non-commit OIDs on input with '--stdin-commits'. I\n> promised this reroll after Peff's review beginning in [1].\n> \n> Not a great deal has changed, but I mostly took every suggestion that\n> Peff put forward (in the sub-thread beginning at [1]). The \"big deals\"\n> since last time are:\n\nThanks. With the exception of the decl-after-statement (and the tiny\nif() nit which we could take or leave), this looks good to me.\n\nThe fix for the decl-after-statement should be easy, but you might want\nto double check your build setup (\"make DEVELOPER=1\" does complain about\nit, and I think that means our CI should, too; if it doesn't, we should\nfix that).\n\n-Peff\n"},{"id":"397816","messageId":"xmqq8shuqsnf.fsf@gitster.c.googlers.com","threadId":"53395","inReplyTo":"20200514180200.GA86181@syl.local","subject":"Re: [PATCH v3 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-14T18:27:00Z","receivedAt":"2020-05-14T18:27:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> static int graph_write(int argc, const char **argv)\n> {\n> -\tstruct string_list *pack_indexes = NULL;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct string_list pack_indexes;\n\nI would strongly prefer this to be initialized here, not ...\n\n> \tstruct oidset commits = OIDSET_INIT;\n> \tstruct object_directory *odb = NULL;\n> -\tstruct string_list lines;\n> \tint result = 0;\n> \tenum commit_graph_write_flags flags = 0;\n>\n> @@ -209,44 +221,34 @@ static int graph_write(int argc, const char **argv)\n> \t\treturn 0;\n> \t}\n>\n> -\tstring_list_init(&lines, 0);\n> -\tif (opts.stdin_packs || opts.stdin_commits) {\n> -\t\tstruct strbuf buf = STRBUF_INIT;\n> +\tif (opts.stdin_packs) {\n> +\t\tstring_list_init(&pack_indexes, 0);\n\n... down here. I know that the use of it is guarded with a ternary\n(opts.stdin_packs ? &pack_indexes : NULL), but that is why you need\nto UNLEAK() it, intead of unconditionally releasing the strbuf, at\nthe end of this function, no?\n\n> \t\twhile (strbuf_getline(&buf, stdin) != EOF)\n> -\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n> +\t\t\tstring_list_append(&pack_indexes,\n> +\t\t\t\t\t strbuf_detach(&buf, NULL));\n> +\t} else if (opts.stdin_commits) {\n> +\t\toidset_init(&commits, 0);\n> +\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n>\n> -\t\tif (opts.stdin_packs)\n> -\t\t\tpack_indexes = &lines;\n> -\t\tif (opts.stdin_commits) {\n> -\t\t\tstruct string_list_item *item;\n> -\t\t\toidset_init(&commits, lines.nr);\n> -\t\t\tfor_each_string_list_item(item, &lines) {\n> -\t\t\t\tstruct object_id oid;\n> -\t\t\t\tconst char *end;\n> -\n> -\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n> -\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n> -\t\t\t\t\t\t\"%s\"), item->string);\n> -\t\t\t\t\treturn 1;\n> -\t\t\t\t}\n> -\n> -\t\t\t\toidset_insert(&commits, &oid);\n> +\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n> +\t\t\tif (read_one_commit(&commits, buf.buf)) {\n> +\t\t\t\tresult = 1;\n> +\t\t\t\tgoto cleanup;\n> \t\t\t}\n> -\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n> \t\t}\n> -\n> -\t\tUNLEAK(buf);\n> \t}\n>\n> \tif (write_commit_graph(odb,\n> -\t\t\t pack_indexes,\n> +\t\t\t opts.stdin_packs ? &pack_indexes : NULL,\n> \t\t\t opts.stdin_commits ? &commits : NULL,\n> \t\t\t flags,\n> \t\t\t &split_opts))\n> \t\tresult = 1;\n>\n> -\tUNLEAK(lines);\n> +cleanup:\n> +\tUNLEAK(pack_indexes);\n> +\tstrbuf_release(&buf);\n> \treturn result;\n> }\n>\n> --\n> 2.26.0.113.ge9739cdccc\n"},{"id":"398133","messageId":"20200518192709.GA3906@syl.local","threadId":"53395","inReplyTo":"xmqq8shuqsnf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 4/8] builtin/commit-graph.c: extract 'read_one_commit()'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-18T19:27:09Z","receivedAt":"2020-05-18T19:27:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Junio,\n\nSorry for a delayed response. I have been working on something that I\nhope to send to the list shortly to discuss. In the meantime...\n\nOn Thu, May 14, 2020 at 11:27:00AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > static int graph_write(int argc, const char **argv)\n> > {\n> > -\tstruct string_list *pack_indexes = NULL;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tstruct string_list pack_indexes;\n>\n> I would strongly prefer this to be initialized here, not ...\n\nThanks for the suggestion. Your approach is much more favorable than\nwhat I had written here, since it does allow me to unconditionally call\n'string_list_clear(...)' instead of 'UNLEAK', which all three of us\ndislike :).\n\nBelow is an amended patch; please use this as 4/8 instead of the\noriginal when queuing. I'd send you a re-rolled series, but the rest of\nthe patches apply cleanly on top, so I don't think there is a need\nunless it would make things more convenient for you.\n\n-- >8 --\n\nSubject: [PATCH] builtin/commit-graph.c: extract 'read_one_commit()'\n\nWith either '--stdin-commits' or '--stdin-packs', the commit-graph\nbuiltin will read line-delimited input, and interpret it either as a\nseries of commit OIDs, or pack names.\n\nIn a subsequent commit, we will begin handling '--stdin-commits'\ndifferently by processing each line as it comes in, instead of in one\nshot at the end. To make adequate room for this additional logic, split\nthe '--stdin-commits' case from '--stdin-packs' by only storing the\ninput when '--stdin-packs' is given.\n\nIn the case of '--stdin-commits', feed each line to a new\n'read_one_commit' helper, which (for now) will merely call\n'parse_oid_hex'.\n\nSigned-off-by: Taylor Blau \n---\n builtin/commit-graph.c | 56 +++++++++++++++++++++---------------------\n 1 file changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 15fe60317c..a5c2332a86 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -138,12 +138,24 @@ static int write_option_parse_split(const struct option *opt, const char *arg,\n \treturn 0;\n }\n\n+static int read_one_commit(struct oidset *commits, const char *hash)\n+{\n+\tstruct object_id oid;\n+\tconst char *end;\n+\n+\tif (parse_oid_hex(hash, &oid, &end))\n+\t\treturn error(_(\"unexpected non-hex object ID: %s\"), hash);\n+\n+\toidset_insert(commits, &oid);\n+\treturn 0;\n+}\n+\n static int graph_write(int argc, const char **argv)\n {\n-\tstruct string_list *pack_indexes = NULL;\n+\tstruct string_list pack_indexes = STRING_LIST_INIT_NODUP;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct oidset commits = OIDSET_INIT;\n \tstruct object_directory *odb = NULL;\n-\tstruct string_list lines;\n \tint result = 0;\n \tenum commit_graph_write_flags flags = 0;\n\n@@ -209,44 +221,32 @@ static int graph_write(int argc, const char **argv)\n \t\treturn 0;\n \t}\n\n-\tstring_list_init(&lines, 0);\n-\tif (opts.stdin_packs || opts.stdin_commits) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\n+\tif (opts.stdin_packs) {\n \t\twhile (strbuf_getline(&buf, stdin) != EOF)\n-\t\t\tstring_list_append(&lines, strbuf_detach(&buf, NULL));\n+\t\t\tstring_list_append(&pack_indexes,\n+\t\t\t\t\t strbuf_detach(&buf, NULL));\n+\t} else if (opts.stdin_commits) {\n+\t\toidset_init(&commits, 0);\n+\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n\n-\t\tif (opts.stdin_packs)\n-\t\t\tpack_indexes = &lines;\n-\t\tif (opts.stdin_commits) {\n-\t\t\tstruct string_list_item *item;\n-\t\t\toidset_init(&commits, lines.nr);\n-\t\t\tfor_each_string_list_item(item, &lines) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tconst char *end;\n-\n-\t\t\t\tif (parse_oid_hex(item->string, &oid, &end)) {\n-\t\t\t\t\terror(_(\"unexpected non-hex object ID: \"\n-\t\t\t\t\t\t\"%s\"), item->string);\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\toidset_insert(&commits, &oid);\n+\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\t\tif (read_one_commit(&commits, buf.buf)) {\n+\t\t\t\tresult = 1;\n+\t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\tflags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;\n \t\t}\n-\n-\t\tUNLEAK(buf);\n \t}\n\n \tif (write_commit_graph(odb,\n-\t\t\t pack_indexes,\n+\t\t\t opts.stdin_packs ? &pack_indexes : NULL,\n \t\t\t opts.stdin_commits ? &commits : NULL,\n \t\t\t flags,\n \t\t\t &split_opts))\n \t\tresult = 1;\n\n-\tUNLEAK(lines);\n+cleanup:\n+\tstring_list_clear(&pack_indexes, 0);\n+\tstrbuf_release(&buf);\n \treturn result;\n }\n\n--\n2.26.0.113.ge9739cdccc\n\n"},{"id":"401364","messageId":"20200710190238.16376-1-szeder.dev@gmail.com","threadId":"53395","inReplyTo":"56403dd377b996d21a4da1e01ffd6e691ac120bd.1589407014.git.me@ttaylorr.com","subject":"[PATCH] commit-graph: fix \"Collecting commits from input\" progress line","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-10T19:02:38Z","receivedAt":"2020-07-10T19:03:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To display a progress line while reading commits from standard input\nand looking them up, 5b6653e523 (builtin/commit-graph.c: dereference\ntags in builtin, 2020-05-13) should have added a pair of\nstart_delayed_progress() and stop_progress() calls around the loop\nreading stdin. Alas, the stop_progress() call ended up at the wrong\nplace, after write_commit_graph(), which does all the commit-graph\ncomputation and writing, and has several progress lines of its own.\nConsequintly, that new\n\n Collecting commits from input: 1234\n\nprogress line is overwritten by the first progress line shown by\nwrite_commit_graph(), and its final \"done\" line is shown last, after\neverything is finished:\n\n $ { sleep 3 ; git rev-list -3 HEAD ; sleep 1 ; } | ~/src/git/git commit-graph write --stdin-commits\n Expanding reachable commits in commit graph: 873402, done.\n Writing out commit graph in 4 passes: 100% (3493608/3493608), done.\n Collecting commits from input: 3, done.\n\nFurthermore, that stop_progress() call was added after the 'cleanup'\nlabel, where that loop reading stdin jumps in case of an error. In\ncase of invalid input this then results in the \"done\" line shown after\nthe error message:\n\n $ { sleep 3 ; git rev-list -3 HEAD ; echo junk ; } | ~/src/git/git commit-graph write --stdin-commits\n error: unexpected non-hex object ID: junk\n Collecting commits from input: 3, done.\n\nMove that stop_progress() call to the right place.\n\nWhile at it, drop the unnecessary 'if (progress)' condition protecting\nthe stop_progress() call, because that function is prepared to handle\na NULL progress struct.\n\nSigned-off-by: SZEDER Gábor \n---\n builtin/commit-graph.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 75455da138..796954da60 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -251,7 +251,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\t}\n \t\t}\n \n-\n+\t\tstop_progress(&progress);\n \t}\n \n \tif (write_commit_graph(odb,\n@@ -264,8 +264,6 @@ static int graph_write(int argc, const char **argv)\n cleanup:\n \tstring_list_clear(&pack_indexes, 0);\n \tstrbuf_release(&buf);\n-\tif (progress)\n-\t\tstop_progress(&progress);\n \treturn result;\n }\n \n-- \n2.27.0.547.g4ba2d26563\n\n"},{"id":"401365","messageId":"20200710191704.GA65960@syl.lan","threadId":"53395","inReplyTo":"20200710190238.16376-1-szeder.dev@gmail.com","subject":"Re: [PATCH] commit-graph: fix \"Collecting commits from input\" progress line","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-10T19:17:04Z","receivedAt":"2020-07-10T19:17:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Szeder,\n\nOn Fri, Jul 10, 2020 at 09:02:38PM +0200, SZEDER Gábor wrote:\n> To display a progress line while reading commits from standard input\n> and looking them up, 5b6653e523 (builtin/commit-graph.c: dereference\n> tags in builtin, 2020-05-13) should have added a pair of\n> start_delayed_progress() and stop_progress() calls around the loop\n> reading stdin. Alas, the stop_progress() call ended up at the wrong\n> place, after write_commit_graph(), which does all the commit-graph\n> computation and writing, and has several progress lines of its own.\n> Consequintly, that new\n\ns/Consequintly/Consequently\n\n>\n> Collecting commits from input: 1234\n>\n> progress line is overwritten by the first progress line shown by\n> write_commit_graph(), and its final \"done\" line is shown last, after\n> everything is finished:\n>\n> $ { sleep 3 ; git rev-list -3 HEAD ; sleep 1 ; } | ~/src/git/git commit-graph write --stdin-commits\n> Expanding reachable commits in commit graph: 873402, done.\n> Writing out commit graph in 4 passes: 100% (3493608/3493608), done.\n> Collecting commits from input: 3, done.\n>\n> Furthermore, that stop_progress() call was added after the 'cleanup'\n> label, where that loop reading stdin jumps in case of an error. In\n> case of invalid input this then results in the \"done\" line shown after\n> the error message:\n>\n> $ { sleep 3 ; git rev-list -3 HEAD ; echo junk ; } | ~/src/git/git commit-graph write --stdin-commits\n> error: unexpected non-hex object ID: junk\n> Collecting commits from input: 3, done.\n>\n> Move that stop_progress() call to the right place.\n>\n> While at it, drop the unnecessary 'if (progress)' condition protecting\n> the stop_progress() call, because that function is prepared to handle\n> a NULL progress struct.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> builtin/commit-graph.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 75455da138..796954da60 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -251,7 +251,7 @@ static int graph_write(int argc, const char **argv)\n> \t\t\t}\n> \t\t}\n>\n> -\n> +\t\tstop_progress(&progress);\n> \t}\n\nThanks for spotting this, and sorry for the couple of progress-related\nbugs that I seem to have generated. I appreciate you looking into it and\nfixing some of these.\n\n>\n> \tif (write_commit_graph(odb,\n> @@ -264,8 +264,6 @@ static int graph_write(int argc, const char **argv)\n> cleanup:\n> \tstring_list_clear(&pack_indexes, 0);\n> \tstrbuf_release(&buf);\n> -\tif (progress)\n> -\t\tstop_progress(&progress);\n> \treturn result;\n> }\n>\n> --\n> 2.27.0.547.g4ba2d26563\n\n\nAll looks good here. With the exception of the small typo that I\nnoticed, this is:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"401599","messageId":"20200715183347.GH11341@szeder.dev","threadId":"53395","inReplyTo":"20200710190238.16376-1-szeder.dev@gmail.com","subject":"Re: [PATCH] commit-graph: fix \"Collecting commits from input\" progress line","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-15T18:33:47Z","receivedAt":"2020-07-15T18:33:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Junio,\n\nit looks like this patch hasn't been picked up, but it fixes a minor\nbug introduced in this release cycle.\n\n\nOn Fri, Jul 10, 2020 at 09:02:38PM +0200, SZEDER Gábor wrote:\n> To display a progress line while reading commits from standard input\n> and looking them up, 5b6653e523 (builtin/commit-graph.c: dereference\n> tags in builtin, 2020-05-13) should have added a pair of\n> start_delayed_progress() and stop_progress() calls around the loop\n> reading stdin. Alas, the stop_progress() call ended up at the wrong\n> place, after write_commit_graph(), which does all the commit-graph\n> computation and writing, and has several progress lines of its own.\n> Consequintly, that new\n> \n> Collecting commits from input: 1234\n> \n> progress line is overwritten by the first progress line shown by\n> write_commit_graph(), and its final \"done\" line is shown last, after\n> everything is finished:\n> \n> $ { sleep 3 ; git rev-list -3 HEAD ; sleep 1 ; } | ~/src/git/git commit-graph write --stdin-commits\n> Expanding reachable commits in commit graph: 873402, done.\n> Writing out commit graph in 4 passes: 100% (3493608/3493608), done.\n> Collecting commits from input: 3, done.\n> \n> Furthermore, that stop_progress() call was added after the 'cleanup'\n> label, where that loop reading stdin jumps in case of an error. In\n> case of invalid input this then results in the \"done\" line shown after\n> the error message:\n> \n> $ { sleep 3 ; git rev-list -3 HEAD ; echo junk ; } | ~/src/git/git commit-graph write --stdin-commits\n> error: unexpected non-hex object ID: junk\n> Collecting commits from input: 3, done.\n> \n> Move that stop_progress() call to the right place.\n> \n> While at it, drop the unnecessary 'if (progress)' condition protecting\n> the stop_progress() call, because that function is prepared to handle\n> a NULL progress struct.\n> \n> Signed-off-by: SZEDER Gábor \n> ---\n> builtin/commit-graph.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n> \n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 75455da138..796954da60 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -251,7 +251,7 @@ static int graph_write(int argc, const char **argv)\n> \t\t\t}\n> \t\t}\n> \n> -\n> +\t\tstop_progress(&progress);\n> \t}\n> \n> \tif (write_commit_graph(odb,\n> @@ -264,8 +264,6 @@ static int graph_write(int argc, const char **argv)\n> cleanup:\n> \tstring_list_clear(&pack_indexes, 0);\n> \tstrbuf_release(&buf);\n> -\tif (progress)\n> -\t\tstop_progress(&progress);\n> \treturn result;\n> }\n> \n> -- \n> 2.27.0.547.g4ba2d26563\n> \n"},{"id":"401601","messageId":"d343c802-bb5a-5bad-5914-5cbcd50b80e5@gmail.com","threadId":"53395","inReplyTo":"20200715183347.GH11341@szeder.dev","subject":"Re: [PATCH] commit-graph: fix \"Collecting commits from input\" progress line","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-15T18:43:42Z","receivedAt":"2020-07-15T18:43:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/15/2020 2:33 PM, SZEDER Gábor wrote:\n> Junio,\n> \n> it looks like this patch hasn't been picked up, but it fixes a minor\n> bug introduced in this release cycle.\n\nThanks for the ping. I forgot to chime in with my :+1: on\nthis patch. Outside of the small typo in the commit message,\nthis is ready to ship.\n\nReviewed-by: Derrick Stolee \n\n> On Fri, Jul 10, 2020 at 09:02:38PM +0200, SZEDER Gábor wrote:\n>> To display a progress line while reading commits from standard input\n>> and looking them up, 5b6653e523 (builtin/commit-graph.c: dereference\n>> tags in builtin, 2020-05-13) should have added a pair of\n>> start_delayed_progress() and stop_progress() calls around the loop\n>> reading stdin. Alas, the stop_progress() call ended up at the wrong\n>> place, after write_commit_graph(), which does all the commit-graph\n>> computation and writing, and has several progress lines of its own.\n>> Consequintly, that new\n\ns/Consequintly/Consequently/\n\n>>\n>> Collecting commits from input: 1234\n>>\n>> progress line is overwritten by the first progress line shown by\n>> write_commit_graph(), and its final \"done\" line is shown last, after\n>> everything is finished:\n>>\n>> $ { sleep 3 ; git rev-list -3 HEAD ; sleep 1 ; } | ~/src/git/git commit-graph write --stdin-commits\n>> Expanding reachable commits in commit graph: 873402, done.\n>> Writing out commit graph in 4 passes: 100% (3493608/3493608), done.\n>> Collecting commits from input: 3, done.\n>>\n>> Furthermore, that stop_progress() call was added after the 'cleanup'\n>> label, where that loop reading stdin jumps in case of an error. In\n>> case of invalid input this then results in the \"done\" line shown after\n>> the error message:\n>>\n>> $ { sleep 3 ; git rev-list -3 HEAD ; echo junk ; } | ~/src/git/git commit-graph write --stdin-commits\n>> error: unexpected non-hex object ID: junk\n>> Collecting commits from input: 3, done.\n>>\n>> Move that stop_progress() call to the right place.\n>>\n>> While at it, drop the unnecessary 'if (progress)' condition protecting\n>> the stop_progress() call, because that function is prepared to handle\n>> a NULL progress struct.\n>>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> builtin/commit-graph.c | 4 +---\n>> 1 file changed, 1 insertion(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n>> index 75455da138..796954da60 100644\n>> --- a/builtin/commit-graph.c\n>> +++ b/builtin/commit-graph.c\n>> @@ -251,7 +251,7 @@ static int graph_write(int argc, const char **argv)\n>> \t\t\t}\n>> \t\t}\n>> \n>> -\n>> +\t\tstop_progress(&progress);\n>> \t}\n>> \n>> \tif (write_commit_graph(odb,\n>> @@ -264,8 +264,6 @@ static int graph_write(int argc, const char **argv)\n>> cleanup:\n>> \tstring_list_clear(&pack_indexes, 0);\n>> \tstrbuf_release(&buf);\n>> -\tif (progress)\n>> -\t\tstop_progress(&progress);\n>> \treturn result;\n>> }\n>> \n>> -- \n>> 2.27.0.547.g4ba2d26563\n>>\n\n"},{"id":"401603","messageId":"xmqq8sfkk3l1.fsf@gitster.c.googlers.com","threadId":"53395","inReplyTo":"d343c802-bb5a-5bad-5914-5cbcd50b80e5@gmail.com","subject":"Re: [PATCH] commit-graph: fix \"Collecting commits from input\" progress line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T18:58:34Z","receivedAt":"2020-07-15T18:58:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Thanks for the ping. I forgot to chime in with my :+1: on\n> this patch. Outside of the small typo in the commit message,\n> this is ready to ship.\n>\n> Reviewed-by: Derrick Stolee \n\nThanks, both. Will queue.\n"}]}