{"thread":{"id":"60331","subject":"[PATCH 0/3] rev-list: add support for commits in `--missing`","startedAt":"2023-10-09T10:55:43Z","lastAt":"2023-10-27T07:59:37Z","messageCount":61,"participants":["Karthik Nayak","Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"482856","messageId":"20231009105528.17777-1-karthik.188@gmail.com","threadId":"60331","inReplyTo":null,"subject":"[PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-09T10:55:25Z","receivedAt":"2023-10-09T10:55:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` option in git-rev-list(1) was introduced intitally\nto deal with missing blobs in the context of promissory notes.\nEventually the option was extended to also support tree objects in\n7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).\n\nThis patch series extends the `--missing` option to also add support for\ncommit objects. We do this by introducing a new flag `MISSING` which is\nadded whenever we encounter a missing commit during traversal. Then in\n`builtin/rev-list` we check for this flag and take the appropriate\naction based on the `--missing=*` option used.\n\nThis series is an alternate to the patch series I had posted earlier:\nhttps://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\nIn that patch, we introduced an option `--ignore-missing-links` which\nwas added to expose the `ignore_missing_links` bit to the user. The\nissue in that patch was that, the option `--ignore-missing-links` didn't\nplay well the pre-existing `--missing` option. This series avoids that\nroute and just extends the `--missing` option for commits to solve the\nsame problem.\n\nKarthik Nayak (3):\n  revision: rename bit to `do_not_die_on_missing_objects`\n  rev-list: move `show_commit()` to the bottom\n  rev-list: add commit object support in `--missing` option\n\n builtin/reflog.c            |  2 +-\n builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n list-objects.c              |  2 +-\n object.h                    |  2 +-\n revision.c                  |  9 +++-\n revision.h                  | 20 ++++----\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n 7 files changed, 145 insertions(+), 57 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\n-- \n2.41.0\n\n"},{"id":"482857","messageId":"20231009105528.17777-2-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231009105528.17777-1-karthik.188@gmail.com","subject":"[PATCH 1/3] revision: rename bit to `do_not_die_on_missing_objects`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-09T10:55:26Z","receivedAt":"2023-10-09T10:55:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the\nrevision walker does not die when encountering a missing tree. This is\ncurrently exclusively set within `builtin/rev-list.c` to ensure the\n`--missing` option works with missing trees.\n\nIn the upcoming commits, we will extend `--missing` to also support\nmissing commits. So let's rename the bit to\n`do_not_die_on_missing_objects`, which is object type agnostic and can\nbe used for both trees/commits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/reflog.c   |  2 +-\n builtin/rev-list.c |  2 +-\n list-objects.c     |  2 +-\n revision.h         | 17 +++++++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df63a5892e..9e369a5977 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tstruct rev_info revs;\n \n \t\trepo_init_revisions(the_repository, &revs, prefix);\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \t\trevs.ignore_missing = 1;\n \t\trevs.ignore_missing_links = 1;\n \t\tif (verbose)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..ea77489c38 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (arg_missing_action)\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex c25c72b32c..47296dff2f 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,\n \t\t    is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n-\t\tif (!revs->do_not_die_on_missing_tree)\n+\t\tif (!revs->do_not_die_on_missing_objects)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \ndiff --git a/revision.h b/revision.h\nindex 50091bbd13..c73c92ef40 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -212,18 +212,19 @@ struct rev_info {\n \n \t\t\t/*\n \t\t\t * Blobs are shown without regard for their existence.\n-\t\t\t * But not so for trees: unless exclude_promisor_objects\n+\t\t\t * But not so for trees/commits: unless exclude_promisor_objects\n \t\t\t * is set and the tree in question is a promisor object;\n \t\t\t * OR ignore_missing_links is set, the revision walker\n-\t\t\t * dies with a \"bad tree object HASH\" message when\n-\t\t\t * encountering a missing tree. For callers that can\n-\t\t\t * handle missing trees and want them to be filterable\n+\t\t\t * dies with a \"bad <type> object HASH\" message when\n+\t\t\t * encountering a missing object. For callers that can\n+\t\t\t * handle missing trees/commits and want them to be filterable\n \t\t\t * and showable, set this to true. The revision walker\n-\t\t\t * will filter and show such a missing tree as usual,\n-\t\t\t * but will not attempt to recurse into this tree\n-\t\t\t * object.\n+\t\t\t * will filter and show such a missing object as usual,\n+\t\t\t * but will not attempt to recurse into this tree/commit\n+\t\t\t * object. The revision walker will also set the MISSING\n+\t\t\t * flag for such objects.\n \t\t\t */\n-\t\t\tdo_not_die_on_missing_tree:1,\n+\t\t\tdo_not_die_on_missing_objects:1,\n \n \t\t\t/* for internal use only */\n \t\t\texclude_promisor_objects:1;\n-- \n2.41.0\n\n"},{"id":"482858","messageId":"20231009105528.17777-4-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231009105528.17777-1-karthik.188@gmail.com","subject":"[PATCH 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-09T10:55:28Z","receivedAt":"2023-10-09T10:55:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` object option in rev-list currently works only with\nmissing blobs/trees. For missing commits the revision walker fails with\na fatal error.\n\nLet's extend the functionality of `--missing` option to also support\ncommit objects. This is done by adding a new `MISSING` flag that the\nrevision walker sets whenever it encounters a missing tree/commit. The\nrevision walker will now continue the traversal and call `show_commit()`\neven for missing commits. In rev-list we can then check for this flag\nand call the existing code for parsing `--missing` objects.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, using the `--missing=print` option while disabling the\nalternate object directory allows us to find the boundary objects\nbetween the main and alternate object directory.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c          |  6 +++\n object.h                    |  2 +-\n revision.c                  |  9 ++++-\n revision.h                  |  3 ++\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 91 insertions(+), 3 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 98542e8b3c..604ae01d0c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n \n \tdisplay_progress(progress, ++progress_counter);\n \n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    commit->object.flags & MISSING) {\n+\t\tfinish_object__ma(&commit->object);\n+\t\treturn;\n+\t}\n+\n \tif (show_disk_usage)\n \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n \ndiff --git a/object.h b/object.h\nindex 114d45954d..12913e6116 100644\n--- a/object.h\n+++ b/object.h\n@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);\n \n /*\n  * object flag allocation:\n- * revision.h:               0---------10         15             23------27\n+ * revision.h:               0---------10         15             22------27\n  * fetch-pack.c:             01    67\n  * negotiator/default.c:       2--5\n  * walker.c:                 0-2\ndiff --git a/revision.c b/revision.c\nindex e789834dd1..615645d3d1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1168,7 +1168,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \t\tint gently = revs->ignore_missing_links ||\n-\t\t\t     revs->exclude_promisor_objects;\n+\t\t\t     revs->exclude_promisor_objects ||\n+\t\t\t     revs->do_not_die_on_missing_objects;\n \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&p->object.oid)) {\n@@ -1176,7 +1177,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\t\tbreak;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\treturn -1;\n+\n+\t\t\tif (!revs->do_not_die_on_missing_objects)\n+\t\t\t\treturn -1;\n+\t\t\telse\n+\t\t\t\tp->object.flags |= MISSING;\n \t\t}\n \t\tif (revs->sources) {\n \t\t\tchar **slot = revision_sources_at(revs->sources, p);\ndiff --git a/revision.h b/revision.h\nindex c73c92ef40..07906bc3bc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -41,6 +41,9 @@\n /* WARNING: This is also used as REACHABLE in commit-graph.c. */\n #define PULL_MERGE\t(1u<<15)\n \n+/* WARNING: This is also used as NEEDS_BITMAP in pack-bitmap.c. */\n+#define MISSING         (1u<<22)\n+\n #define TOPO_WALK_EXPLORED\t(1u<<23)\n #define TOPO_WALK_INDEGREE\t(1u<<24)\n \ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nnew file mode 100755\nindex 0000000000..bbff66e4fc\n--- /dev/null\n+++ b/t/t6022-rev-list-missing.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We setup the repository with two commits, this way HEAD is always\n+# available and we can hide commit 1.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit 1 &&\n+\ttest_commit 2\n+'\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n+\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\ttest_must_fail git rev-list --missing=error --objects \\\n+\t\t\t--no-object-names HEAD\n+\t'\n+done\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor action in \"allow-any\" \"print\"\n+\tdo\n+\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n+\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t# get the expected oids.\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\tHEAD ^$obj >expect.raw &&\n+\n+\t\t\t# Since both the commits have the `1.t` blob, rev-list\n+\t\t\t# will print it since its reachable from either commit. Unless\n+\t\t\t# the blob itself is missing.\n+\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw\n+\t\t\tfi &&\n+\n+\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n+\t\t\t\tHEAD >actual.raw &&\n+\n+\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t# oid to the expect list.\n+\t\t\tcase $action in\n+\t\t\tallow-any)\n+\t\t\t\t;;\n+\t\t\tprint)\n+\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\n+\t\t\tsort actual.raw >actual &&\n+\t\t\tsort expect.raw >expect &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n+\n+\n+test_done\n-- \n2.41.0\n\n"},{"id":"482859","messageId":"20231009105528.17777-3-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231009105528.17777-1-karthik.188@gmail.com","subject":"[PATCH 2/3] rev-list: move `show_commit()` to the bottom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-09T10:55:27Z","receivedAt":"2023-10-09T10:55:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `show_commit()` function already depends on `finish_commit()`, and\nin the upcoming commit, we'll also add a dependency on\n`finish_object__ma()`. Since in C symbols must be declared before\nthey're used, let's move `show_commit()` below both `finish_commit()`\nand `finish_object__ma()`, so the code is cleaner as a whole without the\nneed for declarations.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ea77489c38..98542e8b3c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)\n \treturn size;\n }\n \n-static void finish_commit(struct commit *commit);\n+static inline void finish_object__ma(struct object *obj)\n+{\n+\t/*\n+\t * Whether or not we try to dynamically fetch missing objects\n+\t * from the server, we currently DO NOT have the object.  We\n+\t * can either print, allow (ignore), or conditionally allow\n+\t * (ignore) them.\n+\t */\n+\tswitch (arg_missing_action) {\n+\tcase MA_ERROR:\n+\t\tdie(\"missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tcase MA_ALLOW_ANY:\n+\t\treturn;\n+\n+\tcase MA_PRINT:\n+\t\toidset_insert(&missing_objects, &obj->oid);\n+\t\treturn;\n+\n+\tcase MA_ALLOW_PROMISOR:\n+\t\tif (is_promisor_object(&obj->oid))\n+\t\t\treturn;\n+\t\tdie(\"unexpected missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tdefault:\n+\t\tBUG(\"unhandled missing_action\");\n+\t\treturn;\n+\t}\n+}\n+\n+static void finish_commit(struct commit *commit)\n+{\n+\tfree_commit_list(commit->parents);\n+\tcommit->parents = NULL;\n+\tfree_commit_buffer(the_repository->parsed_objects,\n+\t\t\t   commit);\n+}\n+\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)\n \tfinish_commit(commit);\n }\n \n-static void finish_commit(struct commit *commit)\n-{\n-\tfree_commit_list(commit->parents);\n-\tcommit->parents = NULL;\n-\tfree_commit_buffer(the_repository->parsed_objects,\n-\t\t\t   commit);\n-}\n-\n-static inline void finish_object__ma(struct object *obj)\n-{\n-\t/*\n-\t * Whether or not we try to dynamically fetch missing objects\n-\t * from the server, we currently DO NOT have the object.  We\n-\t * can either print, allow (ignore), or conditionally allow\n-\t * (ignore) them.\n-\t */\n-\tswitch (arg_missing_action) {\n-\tcase MA_ERROR:\n-\t\tdie(\"missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tcase MA_ALLOW_ANY:\n-\t\treturn;\n-\n-\tcase MA_PRINT:\n-\t\toidset_insert(&missing_objects, &obj->oid);\n-\t\treturn;\n-\n-\tcase MA_ALLOW_PROMISOR:\n-\t\tif (is_promisor_object(&obj->oid))\n-\t\t\treturn;\n-\t\tdie(\"unexpected missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tdefault:\n-\t\tBUG(\"unhandled missing_action\");\n-\t\treturn;\n-\t}\n-}\n-\n static int finish_object(struct object *obj, const char *name UNUSED,\n \t\t\t void *cb_data)\n {\n-- \n2.41.0\n\n"},{"id":"482933","messageId":"xmqqwmvvwj38.fsf@gitster.g","threadId":"60331","inReplyTo":"20231009105528.17777-1-karthik.188@gmail.com","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-09T22:02:03Z","receivedAt":"2023-10-09T22:02:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> This series is an alternate to the patch series I had posted earlier:\n> https://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\n> In that patch, we introduced an option `--ignore-missing-links` which\n> was added to expose the `ignore_missing_links` bit to the user. The\n> issue in that patch was that, the option `--ignore-missing-links` didn't\n> play well the pre-existing `--missing` option. This series avoids that\n> route and just extends the `--missing` option for commits to solve the\n> same problem.\n\nThanks for exploring the problem space in the other direction.\n\nI haven't really thought things through yet, but it looks to me that\nthe updated behaviour of \"--missing\" is more in line with what the\noption would have wanted to be from day one.\n\n> Karthik Nayak (3):\n>   revision: rename bit to `do_not_die_on_missing_objects`\n>   rev-list: move `show_commit()` to the bottom\n>   rev-list: add commit object support in `--missing` option\n>\n>  builtin/reflog.c            |  2 +-\n>  builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n>  list-objects.c              |  2 +-\n>  object.h                    |  2 +-\n>  revision.c                  |  9 +++-\n>  revision.h                  | 20 ++++----\n>  t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n>  7 files changed, 145 insertions(+), 57 deletions(-)\n>  create mode 100755 t/t6022-rev-list-missing.sh\n"},{"id":"482949","messageId":"ZSTs3BUVtaI9QIoA@tanuki","threadId":"60331","inReplyTo":"20231009105528.17777-1-karthik.188@gmail.com","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-10T06:19:08Z","receivedAt":"2023-10-10T06:19:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 09, 2023 at 12:55:25PM +0200, Karthik Nayak wrote:\n> The `--missing` option in git-rev-list(1) was introduced intitally\n> to deal with missing blobs in the context of promissory notes.\n> Eventually the option was extended to also support tree objects in\n> 7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).\n> \n> This patch series extends the `--missing` option to also add support for\n> commit objects. We do this by introducing a new flag `MISSING` which is\n> added whenever we encounter a missing commit during traversal. Then in\n> `builtin/rev-list` we check for this flag and take the appropriate\n> action based on the `--missing=*` option used.\n> \n> This series is an alternate to the patch series I had posted earlier:\n> https://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\n> In that patch, we introduced an option `--ignore-missing-links` which\n> was added to expose the `ignore_missing_links` bit to the user. The\n> issue in that patch was that, the option `--ignore-missing-links` didn't\n> play well the pre-existing `--missing` option. This series avoids that\n> route and just extends the `--missing` option for commits to solve the\n> same problem.\n\nI had already reviewed the patches internally at GitLab, so for what\nit's worth please feel free to add my Reviewed-by.\n\nPatrick\n\n> Karthik Nayak (3):\n>   revision: rename bit to `do_not_die_on_missing_objects`\n>   rev-list: move `show_commit()` to the bottom\n>   rev-list: add commit object support in `--missing` option\n> \n>  builtin/reflog.c            |  2 +-\n>  builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n>  list-objects.c              |  2 +-\n>  object.h                    |  2 +-\n>  revision.c                  |  9 +++-\n>  revision.h                  | 20 ++++----\n>  t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n>  7 files changed, 145 insertions(+), 57 deletions(-)\n>  create mode 100755 t/t6022-rev-list-missing.sh\n> \n> -- \n> 2.41.0\n> \n"},{"id":"482992","messageId":"xmqqil7etndo.fsf@gitster.g","threadId":"60331","inReplyTo":"ZSTs3BUVtaI9QIoA@tanuki","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-10T17:09:55Z","receivedAt":"2023-10-10T17:10:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I had already reviewed the patches internally at GitLab, so for what\n> it's worth please feel free to add my Reviewed-by.\n\nGreat.  It seems that 'seen' with this series fails to pass the\ntests, though.\n\nhttps://github.com/git/git/actions/runs/6462854176/job/17545104753\n\nThanks.\n\n>\n> Patrick\n>\n>> Karthik Nayak (3):\n>>   revision: rename bit to `do_not_die_on_missing_objects`\n>>   rev-list: move `show_commit()` to the bottom\n>>   rev-list: add commit object support in `--missing` option\n>> \n>>  builtin/reflog.c            |  2 +-\n>>  builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n>>  list-objects.c              |  2 +-\n>>  object.h                    |  2 +-\n>>  revision.c                  |  9 +++-\n>>  revision.h                  | 20 ++++----\n>>  t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n>>  7 files changed, 145 insertions(+), 57 deletions(-)\n>>  create mode 100755 t/t6022-rev-list-missing.sh\n>> \n>> -- \n>> 2.41.0\n>> \n"},{"id":"483042","messageId":"CAOLa=ZSbd_E+DAkhuGrUpfHkxaje3jrH9-fEDyctAPFExKnj9A@mail.gmail.com","threadId":"60331","inReplyTo":"xmqqil7etndo.fsf@gitster.g","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-11T10:37:03Z","receivedAt":"2023-10-11T10:37:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Oct 10, 2023 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> > I had already reviewed the patches internally at GitLab, so for what\n> > it's worth please feel free to add my Reviewed-by.\n>\n> Great.  It seems that 'seen' with this series fails to pass the\n> tests, though.\n>\n> https://github.com/git/git/actions/runs/6462854176/job/17545104753\n\nSeems like this is because of commit-graph being enabled, I think the\nbest thing to do here\nwould be to disable the commit graph of these tests.\n\nThis should do:\n\n    diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n    index bbff66e4fc..39a8402682 100755\n    --- a/t/t6022-rev-list-missing.sh\n    +++ b/t/t6022-rev-list-missing.sh\n    @@ -5,6 +5,11 @@ test_description='handling of missing objects in rev-list'\n     TEST_PASSES_SANITIZE_LEAK=true\n     . ./test-lib.sh\n\n    +# Disable writing the commit graph as the tests depend on making particular\n    +# commits hidden, the graph is created before that and rev-list\nwould default\n    +# to using the commit graph in such instances.\n    +GIT_TEST_COMMIT_GRAPH=0\n    +\n     # We setup the repository with two commits, this way HEAD is always\n     # available and we can hide commit 1.\n     test_expect_success 'create repository and alternate directory' '\n\nThanks for reporting. I'll wait for a day or two (for reviews) and\nwill add this to the second version\nof this series.\n"},{"id":"483058","messageId":"xmqqbkd5nlq0.fsf@gitster.g","threadId":"60331","inReplyTo":"CAOLa=ZSbd_E+DAkhuGrUpfHkxaje3jrH9-fEDyctAPFExKnj9A@mail.gmail.com","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T16:54:31Z","receivedAt":"2023-10-11T16:54:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Seems like this is because of commit-graph being enabled, I think\n> the best thing to do here would be to disable the commit graph of\n> these tests.\n\nIf the CI uncovered that the new code is broken and does not work\nwith commit-graph, wouldn't the above a totally wrong approach to\ncorrect it?  If the updated logic cannot work correctly when\ncommit-graph covers the history you intentionally break, shouldn't\nthe code, when the feature that is incompatible with commit-graph is\ntriggered, disable the commit-graph?  I am assuming that the new\nfeature is meant to be used to recover from a corrupt repository,\nand if it does not work well when commit-graph knows (now stale\nafter repository corruption) more about the objects that are corrupt\nin the object store, we do want to disable commit-graph.  After all,\ncommit-graph is a secondary information that is supposed to be\nrecoverable from the primary data that is what is in the object\nstore.  Disabling commit-graph in the test means you are telling the\nend-users \"do not use commit-graph if you want to use this feature\",\nwhich sounds like a wrong thing to do.\n"},{"id":"483142","messageId":"CAOLa=ZQxNX4oGtqrgLyKenC_D8M=9q0sFJVmo4fyjSPtgw315Q@mail.gmail.com","threadId":"60331","inReplyTo":"xmqqbkd5nlq0.fsf@gitster.g","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-12T10:44:27Z","receivedAt":"2023-10-12T10:44:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Oct 11, 2023 at 6:54 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n> > Seems like this is because of commit-graph being enabled, I think\n> > the best thing to do here would be to disable the commit graph of\n> > these tests.\n>\n> If the CI uncovered that the new code is broken and does not work\n> with commit-graph, wouldn't the above a totally wrong approach to\n> correct it?  If the updated logic cannot work correctly when\n> commit-graph covers the history you intentionally break, shouldn't\n> the code, when the feature that is incompatible with commit-graph is\n> triggered, disable the commit-graph?  I am assuming that the new\n> feature is meant to be used to recover from a corrupt repository,\n> and if it does not work well when commit-graph knows (now stale\n> after repository corruption) more about the objects that are corrupt\n> in the object store, we do want to disable commit-graph.  After all,\n> commit-graph is a secondary information that is supposed to be\n> recoverable from the primary data that is what is in the object\n> store.  Disabling commit-graph in the test means you are telling the\n> end-users \"do not use commit-graph if you want to use this feature\",\n> which sounds like a wrong thing to do.\n\nI agree with what you're saying. Disabling writing the commit-graph for\nonly the test doesn't serve the real usage. To ensure that the feature should\nwork with corrupt repositories with stale commit-graph, I'm thinking of\ndisabling the commit-graph whenever the `--missing` option is used. The\ncommit graph already provides an API for this, so it should be simple to do.\n\nThanks!\n"},{"id":"483144","messageId":"ZSfSt4tXx8sE68Bn@tanuki","threadId":"60331","inReplyTo":"CAOLa=ZQxNX4oGtqrgLyKenC_D8M=9q0sFJVmo4fyjSPtgw315Q@mail.gmail.com","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-12T11:04:23Z","receivedAt":"2023-10-12T11:04:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 12, 2023 at 12:44:27PM +0200, Karthik Nayak wrote:\n> On Wed, Oct 11, 2023 at 6:54 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Karthik Nayak <karthik.188@gmail.com> writes:\n> >\n> > > Seems like this is because of commit-graph being enabled, I think\n> > > the best thing to do here would be to disable the commit graph of\n> > > these tests.\n> >\n> > If the CI uncovered that the new code is broken and does not work\n> > with commit-graph, wouldn't the above a totally wrong approach to\n> > correct it?  If the updated logic cannot work correctly when\n> > commit-graph covers the history you intentionally break, shouldn't\n> > the code, when the feature that is incompatible with commit-graph is\n> > triggered, disable the commit-graph?  I am assuming that the new\n> > feature is meant to be used to recover from a corrupt repository,\n> > and if it does not work well when commit-graph knows (now stale\n> > after repository corruption) more about the objects that are corrupt\n> > in the object store, we do want to disable commit-graph.  After all,\n> > commit-graph is a secondary information that is supposed to be\n> > recoverable from the primary data that is what is in the object\n> > store.  Disabling commit-graph in the test means you are telling the\n> > end-users \"do not use commit-graph if you want to use this feature\",\n> > which sounds like a wrong thing to do.\n> \n> I agree with what you're saying. Disabling writing the commit-graph for\n> only the test doesn't serve the real usage. To ensure that the feature should\n> work with corrupt repositories with stale commit-graph, I'm thinking of\n> disabling the commit-graph whenever the `--missing` option is used. The\n> commit graph already provides an API for this, so it should be simple to do.\n> \n> Thanks!\n\nWouldn't this have the potential to significantly regress performance\nfor all those preexisting users of the `--missing` option? The commit\ngraph is quite an important optimization nowadays, and especially in\ncommands where we potentially walk a lot of commits (like we may do\nhere) it can result in queries that are orders of magnitudes faster.\n\nIf we were introducing a new option then I think this would be an\nacceptable tradeoff as we could still fix the performance issue in\nanother iteration. But we don't and instead aim to change the meaning of\n`--missing` which may already have users out there. Seen in that light\nit does not seem sensible to me to just disable the graph for them.\n\nDid you figure out what exactly the root cause of this functional\nregression is? If so, can we address that root cause instead?\n\nPatrick\n"},{"id":"483149","messageId":"CAOLa=ZTYqpzyynO=Ks0OkYxveoz9ouwjwj_jcGmr-gfi5_4fhA@mail.gmail.com","threadId":"60331","inReplyTo":"ZSfSt4tXx8sE68Bn@tanuki","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-12T13:23:27Z","receivedAt":"2023-10-12T13:24:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Oct 12, 2023 at 1:04 PM Patrick Steinhardt <ps@pks.im> wrote:\n> Wouldn't this have the potential to significantly regress performance\n> for all those preexisting users of the `--missing` option? The commit\n> graph is quite an important optimization nowadays, and especially in\n> commands where we potentially walk a lot of commits (like we may do\n> here) it can result in queries that are orders of magnitudes faster.\n>\n> If we were introducing a new option then I think this would be an\n> acceptable tradeoff as we could still fix the performance issue in\n> another iteration. But we don't and instead aim to change the meaning of\n> `--missing` which may already have users out there. Seen in that light\n> it does not seem sensible to me to just disable the graph for them.\n>\n\nAgreed, that there is a rather huge performance drop doing this. So either:\n1. Add commit support for `--missing` and disable commit-graph. There is huge\nperformance drop here for commits.\n2. Add commit support for `--missing` but disabling commit-graph is provided via\nan additional `--disable-commit-graph` option. This expects the user to manually\ndisable commit-graph for `--missing` to work correctly with commits.\n\nI was also thinking about this, but people who are using `--missing`\ntill date, also use\nit with `--objects`. Using `--objects` is already slow and doesn't\nbenefit from the\ncommit-graph. So I don't know if there is much of a performance hit.\n\nSome benchmarks on the Git repo:\n\nWith commit graphs:\n\n$ hyperfine --warmup 2 \"git rev-list --objects --missing=print HEAD > /dev/null\"\nBenchmark 1: git rev-list --objects --missing=print HEAD > /dev/null\n  Time (mean ± σ):      1.336 s ±  0.013 s    [User: 1.278 s, System: 0.054 s]\n  Range (min … max):    1.323 s …  1.365 s    10 runs\n\n$ hyperfine --warmup 2 \"git rev-list --missing=print HEAD > /dev/null\"\nBenchmark 1: git rev-list --missing=print HEAD > /dev/null\n  Time (mean ± σ):      29.6 ms ±   1.5 ms    [User: 20.2 ms, System: 9.1 ms]\n  Range (min … max):    27.5 ms …  35.1 ms    92 runs\n\nWithout commit graphs:\n\n$ hyperfine --warmup 2 \"git rev-list --objects --missing=print HEAD > /dev/null\"\nBenchmark 1: git rev-list --objects --missing=print HEAD > /dev/null\n  Time (mean ± σ):      1.735 s ±  0.022 s    [User: 1.672 s, System: 0.057 s]\n  Range (min … max):    1.703 s …  1.765 s    10 runs\n\n$ hyperfine --warmup 2 \"git rev-list --missing=print HEAD > /dev/null\"\nBenchmark 1: git rev-list --missing=print HEAD > /dev/null\n  Time (mean ± σ):     426.6 ms ±  10.2 ms    [User: 410.1 ms, System: 15.1 ms]\n  Range (min … max):   414.9 ms … 441.3 ms    10 runs\n\nThis shows that while there is definitely a performance loss when\ndisabling commit-graphs.\nThis loss is much lesser when used alongside the `--objects` flag.\n\nOverall, I lean towards the option \"1\", but okay with either.\n\n> Did you figure out what exactly the root cause of this functional\n> regression is? If so, can we address that root cause instead?\n\nAre you referring to `--missing` not working with commits? Looking at\nthe history\nof `--missing` shows:\n\n- caf3827e2f: This commits introduces the `--missing` option, it\nbasically states that\nfor the upcoming partial clone mechanism, we want to add support for identifying\nmissing objects.\n- 7c0fe330d5: This commit adds support for trees in `--missing`. It\nalso goes on to\nstate that `--missing` assumed only blobs could be missing.\n\nBoth of these show that `--missing` started off support for only blobs\nand eventually\nadded support to trees (similar to how we're adding support for\ncommits here). So\nI guess the intention was to never work with commits. But the documentation and\nthe option seem generic enough. Considering that trees was an extension, commits\nshould be treated the same way.\n"},{"id":"483153","messageId":"xmqqy1g7hl2y.fsf@gitster.g","threadId":"60331","inReplyTo":"ZSfSt4tXx8sE68Bn@tanuki","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-12T16:17:09Z","receivedAt":"2023-10-12T16:17:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Wouldn't this have the potential to significantly regress performance\n> for all those preexisting users of the `--missing` option? The commit\n> graph is quite an important optimization nowadays, and especially in\n> commands where we potentially walk a lot of commits (like we may do\n> here) it can result in queries that are orders of magnitudes faster.\n\nThe test fails only when GIT_TEST_COMMIT_GRAPH is on, which updates\nthe commit-graph every time a commit is made via \"git commit\" or\n\"git merge\".\n\nI'd suggest stepping back and think a bit.\n\nMy assumption has been that the failing test emulates this scenario\nthat can happen in real life:\n\n * The user creates a new commit.\n\n * A commit graph is written (not as part of GIT_TEST_COMMIT_GRAPH\n   that is not realistic, but as part of \"maintenance\").\n\n * The repository loses some objects due to corruption.\n\n * Now, \"--missing=print\" is invoked so that the user can view what\n   are missing.  Or \"--missing=allow-primisor\" to ensure that the\n   repository does not have missing objects other than the ones that\n   the promisor will give us if we asked again.\n\n * But because the connectivity of these objects appear in the\n   commit graph file, we fail to notice that these objects are\n   missing, producing wrong results.  If we disabled commit-graph\n   while traversal (an earlier writing of it was perfectly OK), then\n   \"rev-list --missing\" would have noticed and reported what the\n   user wanted to know.\n\nIn other words, the \"optimization\" you value is working to quickly\nproduce a wrong result.  Is it \"significantly regress\"ing if we\ndisabled it to obtain the correct result?\n\nMy assumption also has been that there is no point in running\n\"rev-list --missing\" if we know there is no repository corruption,\nand those who run \"rev-list --missing\" wants to know if the objects\nare really available, i.e. even if commit-graph that is out of sync\nwith reality says it exists, if it is not in the object store, they\nwould want to know that.\n\nIf you can show me that it is not the case, then I may be pursuaded\nwhy producing a result that is out of sync with reality _quickly_,\ninstead of taking time to produce a result that matches reality, is\na worthy \"optimization\" to keep.\n\nThanks.\n"},{"id":"483156","messageId":"xmqqa5snhkmt.fsf@gitster.g","threadId":"60331","inReplyTo":"CAOLa=ZQxNX4oGtqrgLyKenC_D8M=9q0sFJVmo4fyjSPtgw315Q@mail.gmail.com","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-12T16:26:50Z","receivedAt":"2023-10-12T16:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> ... To ensure that the feature should\n> work with corrupt repositories with stale commit-graph, I'm thinking of\n> disabling the commit-graph whenever the `--missing` option is used.\n\nJust to avoid giving anybody a wrong guideline, for most of the\nother features, we should assume that the commit-graph accurately\nrepresents the reality in the object database and take advantage of\nit, leaving it to \"git fsck\" to ensure that the commit-graph is\nhealthy.\n\nBut \"rev-list --missing\", especially when used with actions other\nthan allow-promisor or allow-any, is a feature that is only useful\nwhen one wants to really notice objects that are available in the\nobject store, and it would be a bug for it to pretend as if objects\nthe commit-graph has heard of exists without checking.\n\n"},{"id":"483190","messageId":"ZSjbYCXfSUtEIkAt@tanuki","threadId":"60331","inReplyTo":"xmqqy1g7hl2y.fsf@gitster.g","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-13T05:53:36Z","receivedAt":"2023-10-13T05:53:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 12, 2023 at 09:17:09AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Wouldn't this have the potential to significantly regress performance\n> > for all those preexisting users of the `--missing` option? The commit\n> > graph is quite an important optimization nowadays, and especially in\n> > commands where we potentially walk a lot of commits (like we may do\n> > here) it can result in queries that are orders of magnitudes faster.\n> \n> The test fails only when GIT_TEST_COMMIT_GRAPH is on, which updates\n> the commit-graph every time a commit is made via \"git commit\" or\n> \"git merge\".\n> \n> I'd suggest stepping back and think a bit.\n> \n> My assumption has been that the failing test emulates this scenario\n> that can happen in real life:\n> \n>  * The user creates a new commit.\n> \n>  * A commit graph is written (not as part of GIT_TEST_COMMIT_GRAPH\n>    that is not realistic, but as part of \"maintenance\").\n> \n>  * The repository loses some objects due to corruption.\n> \n>  * Now, \"--missing=print\" is invoked so that the user can view what\n>    are missing.  Or \"--missing=allow-primisor\" to ensure that the\n>    repository does not have missing objects other than the ones that\n>    the promisor will give us if we asked again.\n> \n>  * But because the connectivity of these objects appear in the\n>    commit graph file, we fail to notice that these objects are\n>    missing, producing wrong results.  If we disabled commit-graph\n>    while traversal (an earlier writing of it was perfectly OK), then\n>    \"rev-list --missing\" would have noticed and reported what the\n>    user wanted to know.\n> \n> In other words, the \"optimization\" you value is working to quickly\n> produce a wrong result.  Is it \"significantly regress\"ing if we\n> disabled it to obtain the correct result?\n\nIt depends, in my opinion. If:\n\n    - Wrong results caused by the commit graph are only introduced with\n      this patch series due to the changed behaviour of `--missing`.\n\n    - We disable commit graphs proactively only because of the changed\n      behaviour of `--missing`.\n\nThen yes, it does feel wrong to me to disable commit graphs and regress\nperformance for usecases that perviously worked both correct and fast.\n\n> My assumption also has been that there is no point in running\n> \"rev-list --missing\" if we know there is no repository corruption,\n> and those who run \"rev-list --missing\" wants to know if the objects\n> are really available, i.e. even if commit-graph that is out of sync\n> with reality says it exists, if it is not in the object store, they\n> would want to know that.\n> \n> If you can show me that it is not the case, then I may be pursuaded\n> why producing a result that is out of sync with reality _quickly_,\n> instead of taking time to produce a result that matches reality, is\n> a worthy \"optimization\" to keep.\n\nNote that I'm not saying that it's fine to return wrong results -- this\nis of course a bug that needs to be addressed somehow. After all, things\nworking correctly should always trump things working fast. But until now\nit felt more like we were going into the direction of disabling commit\ngraphs without checking whether there is an alternative solution that\nallows us to get the best of both worlds, correctness and performance.\n\nSo what I'm looking for in this thread is a reason why we _can't_ have\nthat, or at least can't have it without unreasonable amounts of work. We\nhave helpers like `lookup_commit_in_graph()` that are designed to detect\nstale commit graphs by double-checking whether a commit that has been\nlooked up via the commit graph actually exists in the repository. So I'm\nwondering whether this could help us address the issue.\n\nIf there is a good reason why all of that is not possible then I'm happy\nto carve in.\n\nPatrick\n"},{"id":"483192","messageId":"ZSkCGS3JPEQ71dOF@tanuki","threadId":"60331","inReplyTo":"ZSjbYCXfSUtEIkAt@tanuki","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-13T08:38:49Z","receivedAt":"2023-10-13T08:38:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 13, 2023 at 07:53:36AM +0200, Patrick Steinhardt wrote:\n> On Thu, Oct 12, 2023 at 09:17:09AM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > My assumption also has been that there is no point in running\n> > \"rev-list --missing\" if we know there is no repository corruption,\n> > and those who run \"rev-list --missing\" wants to know if the objects\n> > are really available, i.e. even if commit-graph that is out of sync\n> > with reality says it exists, if it is not in the object store, they\n> > would want to know that.\n> > \n> > If you can show me that it is not the case, then I may be pursuaded\n> > why producing a result that is out of sync with reality _quickly_,\n> > instead of taking time to produce a result that matches reality, is\n> > a worthy \"optimization\" to keep.\n> \n> Note that I'm not saying that it's fine to return wrong results -- this\n> is of course a bug that needs to be addressed somehow. After all, things\n> working correctly should always trump things working fast. But until now\n> it felt more like we were going into the direction of disabling commit\n> graphs without checking whether there is an alternative solution that\n> allows us to get the best of both worlds, correctness and performance.\n> \n> So what I'm looking for in this thread is a reason why we _can't_ have\n> that, or at least can't have it without unreasonable amounts of work. We\n> have helpers like `lookup_commit_in_graph()` that are designed to detect\n> stale commit graphs by double-checking whether a commit that has been\n> looked up via the commit graph actually exists in the repository. So I'm\n> wondering whether this could help us address the issue.\n> \n> If there is a good reason why all of that is not possible then I'm happy\n> to carve in.\n\nI've had a quick look at this problem so that I can solidify my own\ntrain of thought a bit. The issue is `repo_parse_commit_internal()`,\nwhich calls `parse_commit_in_graph()` without verifying that the object\nactually exists in the object database. It's the only callsite of that\nfunction outside of \"commit-graph.c\", as all other external callers\nwould call `lookup_commit_in_graph()` which _does_ perform the object\nexistence check.\n\nSo I think that the proper way to address the regression would be a\npatch similar to the following:\n\ndiff --git a/commit.c b/commit.c\nindex b3223478bc..109e9217e3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(r, item))\n+\tif (use_commit_graph && parse_commit_in_graph(r, item)) {\n+\t\tif (!has_object(r, &item->object.oid, 0))\n+\t\t\treturn quiet_on_missing ? -1 :\n+\t\t\t\terror(_(\"commit %s exists in commit-graph but not in the object database\"),\n+\t\t\t\t      oid_to_hex(&item->object.oid));\n \t\treturn 0;\n+\t}\n \n \tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n\nI wouldn't be surprised if there are other edge cases where this can\nlead to buggy behaviour.\n\nAlso, this issue may not necessarily stem from repository corruption. It\ncould for example happen that commits are getting garbage collected\nwithout the commit graph having been updated, whatever the reason may be\nfor this. In that case we would happily continue to return these commits\nfrom the commit graph even though the underlying object has since been\ndeleted. The repository itself is not corrupt though, we merely look at\nan out-of-date commit graph. And for what it's worth, I think that we\nshould always gracefully handle that case or otherwise the commit graph\nbecomes less useful overall.\n\nI didn't dig much deeper yet. And while the above patch fixes some of\nthe test failures, it doesn't fix them all. If we agree that this is the\nway to go then I'd be happy to turn this into a proper patch.\n\nPatrick\n"},{"id":"483196","messageId":"b0bf576c51a706367a758b8e30eca37edb9c2734.1697200576.git.ps@pks.im","threadId":"60331","inReplyTo":"ZSkCGS3JPEQ71dOF@tanuki","subject":"[PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-13T12:37:35Z","receivedAt":"2023-10-13T12:37:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Commit graphs can become stale and contain references to commits that do\nnot exist in the object database anymore. Theoretically, this can lead\nto a scenario where we are able to successfully look up any such commit\nvia the commit graph even though such a lookup would fail if done via\nthe object database directly.\n\nAs the commit graph is mostly intended as a sort of cache to speed up\nparsing of commits we do not want to have diverging behaviour in a\nrepository with and a repository without commit graphs, no matter\nwhether they are stale or not. As commits are otherwise immutable, the\nonly thing that we really need to care about is thus the presence or\nabsence of a commit.\n\nTo address potentially stale commit data that may exist in the graph,\nour `lookup_commit_in_graph()` function will check for the commit's\nexistence in both the commit graph, but also in the object database. So\neven if we were able to look up the commit's data in the graph, we would\nstill pretend as if the commit didn't exist if it is missing in the\nobject database.\n\nWe don't have the same safety net in `parse_commit_in_graph_one()`\nthough. This function is mostly used internally in \"commit-graph.c\"\nitself to validate the commit graph, and this usage is fine. We do\nexpose its functionality via `parse_commit_in_graph()` though, which\ngets called by `repo_parse_commit_internal()`, and that function is in\nturn used in many places in our codebase.\n\nFor all I can see this function is never used to directly turn an object\nID into a commit object without additional safety checks before or after\nthis lookup. What it is being used for though is to walk history via the\nparent chain of commits. So when commits in the parent chain of a graph\nwalk are missing it is possible that we wouldn't notice if that missing\ncommit was part of the commit graph. Thus, a query like `git rev-parse\nHEAD~2` can succeed even if the intermittent commit is missing.\n\nIt's unclear whether there are additional ways in which such stale\ncommit graphs can lead to problems. In any case, it feels like this is a\nbigger bug waiting to happen when we gain additional direct or indirect\ncallers of `repo_parse_commit_internal()`. So let's fix the inconsistent\nbehaviour by checking for object existence via the object database, as\nwell.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n commit.c                |  7 ++++++-\n t/t5318-commit-graph.sh | 23 +++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex b3223478bc..109e9217e3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(r, item))\n+\tif (use_commit_graph && parse_commit_in_graph(r, item)) {\n+\t\tif (!has_object(r, &item->object.oid, 0))\n+\t\t\treturn quiet_on_missing ? -1 :\n+\t\t\t\terror(_(\"commit %s exists in commit-graph but not in the object database\"),\n+\t\t\t\t      oid_to_hex(&item->object.oid));\n \t\treturn 0;\n+\t}\n \n \tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ba65f17dd9..25f8e9e2d3 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '\n \t)\n '\n \n+test_expect_success 'commit exists in commit-graph but not in object database' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\t\tgit commit-graph write --reachable &&\n+\n+\t\t# Corrupt the repository by deleting the intermittent commit\n+\t\t# object. Commands should notice that this object is absent and\n+\t\t# thus that the repository is corrupt even if the commit graph\n+\t\t# exists.\n+\t\toid=$(git rev-parse B) &&\n+\t\trm .git/objects/\"$(test_oid_to_path \"$oid\")\" &&\n+\n+\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n+\t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n+\t)\n+'\n+\n test_done\n-- \n2.42.0\n\n"},{"id":"483207","messageId":"xmqqpm1i1mem.fsf@gitster.g","threadId":"60331","inReplyTo":"ZSkCGS3JPEQ71dOF@tanuki","subject":"Re: [PATCH 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T17:07:29Z","receivedAt":"2023-10-13T17:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> actually exists in the object database. It's the only callsite of that\n> function outside of \"commit-graph.c\", as all other external callers\n> would call `lookup_commit_in_graph()` which _does_ perform the object\n> existence check.\n>\n> So I think that the proper way to address the regression would be a\n> patch similar to the following:\n>\n> diff --git a/commit.c b/commit.c\n> index b3223478bc..109e9217e3 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,\n>  \t\treturn -1;\n>  \tif (item->object.parsed)\n>  \t\treturn 0;\n> -\tif (use_commit_graph && parse_commit_in_graph(r, item))\n> +\tif (use_commit_graph && parse_commit_in_graph(r, item)) {\n> +\t\tif (!has_object(r, &item->object.oid, 0))\n> +\t\t\treturn quiet_on_missing ? -1 :\n> +\t\t\t\terror(_(\"commit %s exists in commit-graph but not in the object database\"),\n> +\t\t\t\t      oid_to_hex(&item->object.oid));\n>  \t\treturn 0;\n> +\t}\n\nIt is (overly) conservative, which I generally should find pleasing,\nbut as I said, for secondary information like commit-graph that is\nderived from the primary source only to precompute for optimization,\nour general attitude should be to trust it and let the optimization\nkick in, unless the operation being performed primarily cares about\nthe case where the result from using and not using the secondary\nsource differs.  An obvious example of such an operation is \"fsck\",\nwhere we do care and want to notice when the underlying object graph\nand what commit-graph records contradict with each other.  And my\nsuggestion to disable commit-graph while running the \"rev-list\"\ncommand with the \"--missing\" option is because that usage would fall\ninto the same category (please correct me if that is not the case) [*].\n\nSo for the purpose of \"rev-list --missing\", the above change does\ntake us closer to the answer we would get from the primary source,\nbut isn't it pessimising other users unnecessarily?  Do we have a\nrough idea (if we have a benchmark that would be even better) how\nmuch this lack of has_object() check is contributing to the\nperformance benefit of using commit-graph?  Majority of callers of\nthis code should not have to pay the additional overhead here, so\nunless it is small enough, this would be pessimising the generic\ncode for everybody to please \"rev-list --missing\", which is why I am\nworried about going in this direction.\n"},{"id":"483211","messageId":"xmqq1qdy1iyr.fsf@gitster.g","threadId":"60331","inReplyTo":"b0bf576c51a706367a758b8e30eca37edb9c2734.1697200576.git.ps@pks.im","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T18:21:48Z","receivedAt":"2023-10-13T18:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,\n>  \t\treturn -1;\n>  \tif (item->object.parsed)\n>  \t\treturn 0;\n> -\tif (use_commit_graph && parse_commit_in_graph(r, item))\n> +\tif (use_commit_graph && parse_commit_in_graph(r, item)) {\n> +\t\tif (!has_object(r, &item->object.oid, 0))\n> +\t\t\treturn quiet_on_missing ? -1 :\n> +\t\t\t\terror(_(\"commit %s exists in commit-graph but not in the object database\"),\n> +\t\t\t\t      oid_to_hex(&item->object.oid));\n>  \t\treturn 0;\n> +\t}\n\nEver since this codepath was introduced by 177722b3 (commit:\nintegrate commit graph with commit parsing, 2018-04-10), we blindly\ntrusted what commit-graph file says.  This change is a strict\nimprovement in the correctness department, but there are two things\nthat are a bit worrying.\n\nOne.  The additional check should certainly be cheaper than a full\nreading and parsing of an object, either from a loose object file or\nfrom a pack entry.  It may not hurt performance too much, but it\nstill would give us more confidence if we know by how much we are\npessimising good cases where the commit-graph does match reality.\nOur stance on these secondary files that store precomputed values\nfor optimization purposes is in general to use them blindly unless\nin exceptional cases where the operation values the correctness even\nwhen the validity of these secondary files is dubious (e.g., \"fsck\"),\nand doing this extra check regardless of the caller at this low level\nof the callchain is a bit worrying.\n\nAnother is that by the time parse_commit_in_graph() returns true and\nwe realize that the answer we got is bogus by asking has_object(),\nitem->object.parsed has already been toggled on, so the caller now\nhas a commit object that claimed it was already parsed and does not\nmatch reality.  Hopefully the caller takes an early exit upon seeing\na failure from parse_commit_gently() and the .parsed bit does not\nmatter, but maybe I am missing a case where it does.  I dunno.\n\nOther than that, sounds very sensible and the code change is clean.\n\nWill queue.  Thanks.\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index ba65f17dd9..25f8e9e2d3 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'commit exists in commit-graph but not in object database' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\ttest_commit A &&\n> +\t\ttest_commit B &&\n> +\t\ttest_commit C &&\n> +\t\tgit commit-graph write --reachable &&\n> +\n> +\t\t# Corrupt the repository by deleting the intermittent commit\n> +\t\t# object. Commands should notice that this object is absent and\n> +\t\t# thus that the repository is corrupt even if the commit graph\n> +\t\t# exists.\n> +\t\toid=$(git rev-parse B) &&\n> +\t\trm .git/objects/\"$(test_oid_to_path \"$oid\")\" &&\n> +\n> +\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n> +\t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"483302","messageId":"20231016103830.56486-1-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231009105528.17777-1-karthik.188@gmail.com","subject":"[PATCH v2 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-16T10:38:27Z","receivedAt":"2023-10-16T10:38:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` option in git-rev-list(1) was introduced intitally\nto deal with missing blobs in the context of promissory notes.\nEventually the option was extended to also support tree objects in\n7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).\n\nThis patch series extends the `--missing` option to also add support for\ncommit objects. We do this by introducing a new flag `MISSING` which is\nadded whenever we encounter a missing commit during traversal. Then in\n`builtin/rev-list` we check for this flag and take the appropriate\naction based on the `--missing=*` option used.\n\nThis series is an alternate to the patch series I had posted earlier:\nhttps://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\nIn that patch, we introduced an option `--ignore-missing-links` which\nwas added to expose the `ignore_missing_links` bit to the user. The\nissue in that patch was that, the option `--ignore-missing-links` didn't\nplay well the pre-existing `--missing` option. This series avoids that\nroute and just extends the `--missing` option for commits to solve the\nsame problem.\n\nChanges from v1:\n1. The patch series is now rebased on top of Patrick's work to make\ncommit-graphs more reliable. With this, we need to be more diligant\naround which commits are missing and only parse required commits. So\nwe add some extra checks here, especially because we don't want to\nallow missing commit's tree and parent to be parsed.\n2. The tests were preivously testing a commit with no parents, add\nan additional commit to ensure that the missing commit's parents aren't\nparsed.\n\nRange diff:\n\n1:  be2ac0a331 = 1:  c1c892aa86 revision: rename bit to `do_not_die_on_missing_objects`\n2:  b44983967f = 2:  67f6bfeaf7 rev-list: move `show_commit()` to the bottom\n3:  306d918aef ! 3:  6d8e3c721f rev-list: add commit object support in `--missing` option\n    @@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)\n      \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n      \n     \n    + ## list-objects.c ##\n    +@@ list-objects.c: static void do_traverse(struct traversal_context *ctx)\n    + \t\t * an uninteresting boundary commit may not have its tree\n    + \t\t * parsed yet, but we are not going to show them anyway\n    + \t\t */\n    +-\t\tif (!ctx->revs->tree_objects)\n    ++\t\tif (!ctx->revs->tree_objects || commit->object.flags & MISSING)\n    + \t\t\t; /* do not bother loading tree */\n    + \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n    + \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n    +\n      ## object.h ##\n     @@ object.h: void object_array_init(struct object_array *array);\n      \n    @@ object.h: void object_array_init(struct object_array *array);\n       * walker.c:                 0-2\n     \n      ## revision.c ##\n    +@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,\n    + \tstruct commit_list *parent = commit->parents;\n    + \tunsigned pass_flags;\n    + \n    +-\tif (commit->object.flags & ADDED)\n    ++\tif (commit->object.flags & ADDED || commit->object.flags & MISSING)\n    + \t\treturn 0;\n    + \tcommit->object.flags |= ADDED;\n    + \n     @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,\n      \tfor (parent = commit->parents; parent; parent = parent->next) {\n      \t\tstruct commit *p = parent->item;\n    @@ t/t6022-rev-list-missing.sh (new)\n     +# available and we can hide commit 1.\n     +test_expect_success 'create repository and alternate directory' '\n     +\ttest_commit 1 &&\n    -+\ttest_commit 2\n    ++\ttest_commit 2 &&\n    ++\ttest_commit 3\n     +'\n     +\n     +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n    @@ t/t6022-rev-list-missing.sh (new)\n     +\t\t\tgit rev-list --objects --no-object-names \\\n     +\t\t\t\tHEAD ^$obj >expect.raw &&\n     +\n    -+\t\t\t# Since both the commits have the `1.t` blob, rev-list\n    -+\t\t\t# will print it since its reachable from either commit. Unless\n    -+\t\t\t# the blob itself is missing.\n    ++\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n    ++\t\t\t# might be skipped, its blob must be accounted for.\n     +\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n    -+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw\n    ++\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n    ++\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n     +\t\t\tfi &&\n     +\n     +\t\t\tmv \"$path\" \"$path.hidden\" &&\n    @@ t/t6022-rev-list-missing.sh (new)\n     +\tdone\n     +done\n     +\n    -+\n     +test_done\n\n\nKarthik Nayak (3):\n  revision: rename bit to `do_not_die_on_missing_objects`\n  rev-list: move `show_commit()` to the bottom\n  rev-list: add commit object support in `--missing` option\n\n builtin/reflog.c            |  2 +-\n builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n list-objects.c              |  4 +-\n object.h                    |  2 +-\n revision.c                  | 11 +++--\n revision.h                  | 20 ++++----\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n 7 files changed, 147 insertions(+), 59 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\n-- \n2.41.0\n\n"},{"id":"483303","messageId":"20231016103830.56486-2-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231016103830.56486-1-karthik.188@gmail.com","subject":"[PATCH v2 1/3] revision: rename bit to `do_not_die_on_missing_objects`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-16T10:38:28Z","receivedAt":"2023-10-16T10:38:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the\nrevision walker does not die when encountering a missing tree. This is\ncurrently exclusively set within `builtin/rev-list.c` to ensure the\n`--missing` option works with missing trees.\n\nIn the upcoming commits, we will extend `--missing` to also support\nmissing commits. So let's rename the bit to\n`do_not_die_on_missing_objects`, which is object type agnostic and can\nbe used for both trees/commits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/reflog.c   |  2 +-\n builtin/rev-list.c |  2 +-\n list-objects.c     |  2 +-\n revision.h         | 17 +++++++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df63a5892e..9e369a5977 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tstruct rev_info revs;\n \n \t\trepo_init_revisions(the_repository, &revs, prefix);\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \t\trevs.ignore_missing = 1;\n \t\trevs.ignore_missing_links = 1;\n \t\tif (verbose)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..ea77489c38 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (arg_missing_action)\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex c25c72b32c..47296dff2f 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,\n \t\t    is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n-\t\tif (!revs->do_not_die_on_missing_tree)\n+\t\tif (!revs->do_not_die_on_missing_objects)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \ndiff --git a/revision.h b/revision.h\nindex 50091bbd13..c73c92ef40 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -212,18 +212,19 @@ struct rev_info {\n \n \t\t\t/*\n \t\t\t * Blobs are shown without regard for their existence.\n-\t\t\t * But not so for trees: unless exclude_promisor_objects\n+\t\t\t * But not so for trees/commits: unless exclude_promisor_objects\n \t\t\t * is set and the tree in question is a promisor object;\n \t\t\t * OR ignore_missing_links is set, the revision walker\n-\t\t\t * dies with a \"bad tree object HASH\" message when\n-\t\t\t * encountering a missing tree. For callers that can\n-\t\t\t * handle missing trees and want them to be filterable\n+\t\t\t * dies with a \"bad <type> object HASH\" message when\n+\t\t\t * encountering a missing object. For callers that can\n+\t\t\t * handle missing trees/commits and want them to be filterable\n \t\t\t * and showable, set this to true. The revision walker\n-\t\t\t * will filter and show such a missing tree as usual,\n-\t\t\t * but will not attempt to recurse into this tree\n-\t\t\t * object.\n+\t\t\t * will filter and show such a missing object as usual,\n+\t\t\t * but will not attempt to recurse into this tree/commit\n+\t\t\t * object. The revision walker will also set the MISSING\n+\t\t\t * flag for such objects.\n \t\t\t */\n-\t\t\tdo_not_die_on_missing_tree:1,\n+\t\t\tdo_not_die_on_missing_objects:1,\n \n \t\t\t/* for internal use only */\n \t\t\texclude_promisor_objects:1;\n-- \n2.41.0\n\n"},{"id":"483304","messageId":"20231016103830.56486-3-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231016103830.56486-1-karthik.188@gmail.com","subject":"[PATCH v2 2/3] rev-list: move `show_commit()` to the bottom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-16T10:38:29Z","receivedAt":"2023-10-16T10:38:46Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `show_commit()` function already depends on `finish_commit()`, and\nin the upcoming commit, we'll also add a dependency on\n`finish_object__ma()`. Since in C symbols must be declared before\nthey're used, let's move `show_commit()` below both `finish_commit()`\nand `finish_object__ma()`, so the code is cleaner as a whole without the\nneed for declarations.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ea77489c38..98542e8b3c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)\n \treturn size;\n }\n \n-static void finish_commit(struct commit *commit);\n+static inline void finish_object__ma(struct object *obj)\n+{\n+\t/*\n+\t * Whether or not we try to dynamically fetch missing objects\n+\t * from the server, we currently DO NOT have the object.  We\n+\t * can either print, allow (ignore), or conditionally allow\n+\t * (ignore) them.\n+\t */\n+\tswitch (arg_missing_action) {\n+\tcase MA_ERROR:\n+\t\tdie(\"missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tcase MA_ALLOW_ANY:\n+\t\treturn;\n+\n+\tcase MA_PRINT:\n+\t\toidset_insert(&missing_objects, &obj->oid);\n+\t\treturn;\n+\n+\tcase MA_ALLOW_PROMISOR:\n+\t\tif (is_promisor_object(&obj->oid))\n+\t\t\treturn;\n+\t\tdie(\"unexpected missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tdefault:\n+\t\tBUG(\"unhandled missing_action\");\n+\t\treturn;\n+\t}\n+}\n+\n+static void finish_commit(struct commit *commit)\n+{\n+\tfree_commit_list(commit->parents);\n+\tcommit->parents = NULL;\n+\tfree_commit_buffer(the_repository->parsed_objects,\n+\t\t\t   commit);\n+}\n+\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)\n \tfinish_commit(commit);\n }\n \n-static void finish_commit(struct commit *commit)\n-{\n-\tfree_commit_list(commit->parents);\n-\tcommit->parents = NULL;\n-\tfree_commit_buffer(the_repository->parsed_objects,\n-\t\t\t   commit);\n-}\n-\n-static inline void finish_object__ma(struct object *obj)\n-{\n-\t/*\n-\t * Whether or not we try to dynamically fetch missing objects\n-\t * from the server, we currently DO NOT have the object.  We\n-\t * can either print, allow (ignore), or conditionally allow\n-\t * (ignore) them.\n-\t */\n-\tswitch (arg_missing_action) {\n-\tcase MA_ERROR:\n-\t\tdie(\"missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tcase MA_ALLOW_ANY:\n-\t\treturn;\n-\n-\tcase MA_PRINT:\n-\t\toidset_insert(&missing_objects, &obj->oid);\n-\t\treturn;\n-\n-\tcase MA_ALLOW_PROMISOR:\n-\t\tif (is_promisor_object(&obj->oid))\n-\t\t\treturn;\n-\t\tdie(\"unexpected missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tdefault:\n-\t\tBUG(\"unhandled missing_action\");\n-\t\treturn;\n-\t}\n-}\n-\n static int finish_object(struct object *obj, const char *name UNUSED,\n \t\t\t void *cb_data)\n {\n-- \n2.41.0\n\n"},{"id":"483305","messageId":"20231016103830.56486-4-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231016103830.56486-1-karthik.188@gmail.com","subject":"[PATCH v2 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-16T10:38:30Z","receivedAt":"2023-10-16T10:38:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` object option in rev-list currently works only with\nmissing blobs/trees. For missing commits the revision walker fails with\na fatal error.\n\nLet's extend the functionality of `--missing` option to also support\ncommit objects. This is done by adding a new `MISSING` flag that the\nrevision walker sets whenever it encounters a missing tree/commit. The\nrevision walker will now continue the traversal and call `show_commit()`\neven for missing commits. In rev-list we can then check for this flag\nand call the existing code for parsing `--missing` objects.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, using the `--missing=print` option while disabling the\nalternate object directory allows us to find the boundary objects\nbetween the main and alternate object directory.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c          |  6 +++\n list-objects.c              |  2 +-\n object.h                    |  2 +-\n revision.c                  | 11 ++++--\n revision.h                  |  3 ++\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n 6 files changed, 93 insertions(+), 5 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 98542e8b3c..604ae01d0c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n \n \tdisplay_progress(progress, ++progress_counter);\n \n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    commit->object.flags & MISSING) {\n+\t\tfinish_object__ma(&commit->object);\n+\t\treturn;\n+\t}\n+\n \tif (show_disk_usage)\n \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 47296dff2f..60c5533721 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -387,7 +387,7 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t * an uninteresting boundary commit may not have its tree\n \t\t * parsed yet, but we are not going to show them anyway\n \t\t */\n-\t\tif (!ctx->revs->tree_objects)\n+\t\tif (!ctx->revs->tree_objects || commit->object.flags & MISSING)\n \t\t\t; /* do not bother loading tree */\n \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\ndiff --git a/object.h b/object.h\nindex 114d45954d..12913e6116 100644\n--- a/object.h\n+++ b/object.h\n@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);\n \n /*\n  * object flag allocation:\n- * revision.h:               0---------10         15             23------27\n+ * revision.h:               0---------10         15             22------27\n  * fetch-pack.c:             01    67\n  * negotiator/default.c:       2--5\n  * walker.c:                 0-2\ndiff --git a/revision.c b/revision.c\nindex e789834dd1..d147e0f611 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1110,7 +1110,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tstruct commit_list *parent = commit->parents;\n \tunsigned pass_flags;\n \n-\tif (commit->object.flags & ADDED)\n+\tif (commit->object.flags & ADDED || commit->object.flags & MISSING)\n \t\treturn 0;\n \tcommit->object.flags |= ADDED;\n \n@@ -1168,7 +1168,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \t\tint gently = revs->ignore_missing_links ||\n-\t\t\t     revs->exclude_promisor_objects;\n+\t\t\t     revs->exclude_promisor_objects ||\n+\t\t\t     revs->do_not_die_on_missing_objects;\n \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&p->object.oid)) {\n@@ -1176,7 +1177,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\t\tbreak;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\treturn -1;\n+\n+\t\t\tif (!revs->do_not_die_on_missing_objects)\n+\t\t\t\treturn -1;\n+\t\t\telse\n+\t\t\t\tp->object.flags |= MISSING;\n \t\t}\n \t\tif (revs->sources) {\n \t\t\tchar **slot = revision_sources_at(revs->sources, p);\ndiff --git a/revision.h b/revision.h\nindex c73c92ef40..07906bc3bc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -41,6 +41,9 @@\n /* WARNING: This is also used as REACHABLE in commit-graph.c. */\n #define PULL_MERGE\t(1u<<15)\n \n+/* WARNING: This is also used as NEEDS_BITMAP in pack-bitmap.c. */\n+#define MISSING         (1u<<22)\n+\n #define TOPO_WALK_EXPLORED\t(1u<<23)\n #define TOPO_WALK_INDEGREE\t(1u<<24)\n \ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nnew file mode 100755\nindex 0000000000..40265a4f66\n--- /dev/null\n+++ b/t/t6022-rev-list-missing.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We setup the repository with two commits, this way HEAD is always\n+# available and we can hide commit 1.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit 1 &&\n+\ttest_commit 2 &&\n+\ttest_commit 3\n+'\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n+\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\ttest_must_fail git rev-list --missing=error --objects \\\n+\t\t\t--no-object-names HEAD\n+\t'\n+done\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor action in \"allow-any\" \"print\"\n+\tdo\n+\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n+\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t# get the expected oids.\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\tHEAD ^$obj >expect.raw &&\n+\n+\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n+\t\t\t# might be skipped, its blob must be accounted for.\n+\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n+\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n+\t\t\tfi &&\n+\n+\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n+\t\t\t\tHEAD >actual.raw &&\n+\n+\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t# oid to the expect list.\n+\t\t\tcase $action in\n+\t\t\tallow-any)\n+\t\t\t\t;;\n+\t\t\tprint)\n+\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\n+\t\t\tsort actual.raw >actual &&\n+\t\t\tsort expect.raw >expect &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n+\n+test_done\n-- \n2.41.0\n\n"},{"id":"483311","messageId":"xmqqbkcyo7ro.fsf@gitster.g","threadId":"60331","inReplyTo":"20231016103830.56486-1-karthik.188@gmail.com","subject":"Re: [PATCH v2 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-16T16:24:11Z","receivedAt":"2023-10-16T16:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> ... some extra checks here, especially because we don't want to\n> allow missing commit's tree and parent to be parsed.\n\nGood changes relative to the previous round.\n\nA bad news is that with this series (especially [PATCH 3/3]) applied\non top of Patrick's \"always make sure the commit we found from the\ncommit-graph at least exists\" change, t5310 and t5320 seem to\nconsistently fail for me.  They pass with the first two steps\napplied without [3/3] but that is to be expected as they are both\nno-ops.\n\nThe change in 3/3 to list-objects.c::do_traverse() seems to be the\nculprit.  Reverting that single hunk makes t5310 and t5320 pass\nagain.  What I find alarming is that two tests that are touched by\nthis series, t5318 and t6022, do not seem to fail with the hunk\nreverted, which means the hunk has no meaningful test coverage for\nthe purpose of this series.\n\nI didn't even try to understand what is going on, so there may be a\nvery good reason that the change must be there for do_traverse() to\nwork correctly.  I dunno.\n\n>     +-\tif (commit->object.flags & ADDED)\n>     ++\tif (commit->object.flags & ADDED || commit->object.flags & MISSING)\n\nThis and others are syntactically correct C, but some folks may find\nit more readable to see each of the bitwise operations enclosed in a\npair of parentheses when combined by logical operations, i.e.,\n\n\tif ((commit->object.flags & ADDED) || (commit->object.flags & MISSING))\n\nIn this particular case, we can even say\n\n\t\tif (commit->object.flags & (ADDED | MISSING))\n\nmeaning, \"if we have either the ADDED or the MISSING bit set\", which\nmay make it even clearer.\n\nThanks.\n"},{"id":"483315","messageId":"CAOLa=ZT4EuoB8GHMe+a2nfq8Pfg5x7xzrEa_qV39U1HqUyS+Eg@mail.gmail.com","threadId":"60331","inReplyTo":"xmqqbkcyo7ro.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-16T19:01:23Z","receivedAt":"2023-10-16T19:01:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Oct 16, 2023 at 6:24 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Good changes relative to the previous round.\n>\n> A bad news is that with this series (especially [PATCH 3/3]) applied\n> on top of Patrick's \"always make sure the commit we found from the\n> commit-graph at least exists\" change, t5310 and t5320 seem to\n> consistently fail for me.  They pass with the first two steps\n> applied without [3/3] but that is to be expected as they are both\n> no-ops.\n>\n> The change in 3/3 to list-objects.c::do_traverse() seems to be the\n> culprit.  Reverting that single hunk makes t5310 and t5320 pass\n> again.\n\nIt seems that the new chunk introduced now causes collision because of\nthe bit field of the MISSING flag being the same as the pre-existing\nNEEDS_BITMAP flag. Earlier this wasn't a concern, but now, their paths\nhave converged at this change.\n\nSo changing the bit field for the MISSING flag from `22` to an unused `28`\nfixes the issue. I should have run the whole test suite again. Apologies.\n\n>  What I find alarming is that two tests that are touched by\n> this series, t5318 and t6022, do not seem to fail with the hunk\n> reverted, which means the hunk has no meaningful test coverage for\n> the purpose of this series.\n>\n\nWell, actually the newly introduced tests t6022 require this block,\nbut this is specific\nto when commit graph is enabled. So what you did see was correct, but\nthe test would\nalso fail with `GIT_TEST_COMMIT_GRAPH=1` if the block was removed. That's\nexactly why in this series I increased the number of commits in the\ntest block from 2->3.\n\n> >     +-        if (commit->object.flags & ADDED)\n> >     ++        if (commit->object.flags & ADDED || commit->object.flags & MISSING)\n>\n> This and others are syntactically correct C, but some folks may find\n> it more readable to see each of the bitwise operations enclosed in a\n> pair of parentheses when combined by logical operations, i.e.,\n>\n>         if ((commit->object.flags & ADDED) || (commit->object.flags & MISSING))\n>\n> In this particular case, we can even say\n>\n>                 if (commit->object.flags & (ADDED | MISSING))\n>\n> meaning, \"if we have either the ADDED or the MISSING bit set\", which\n> may make it even clearer.\n\nI agree with this, I'll add it in the next block.\n\nThanks for the quick review, I'll wait a day/two and send v3 with the\nchanges to fix tests\nand cleaner code.\n"},{"id":"483317","messageId":"xmqqwmvml32g.fsf@gitster.g","threadId":"60331","inReplyTo":"CAOLa=ZT4EuoB8GHMe+a2nfq8Pfg5x7xzrEa_qV39U1HqUyS+Eg@mail.gmail.com","subject":"Re: [PATCH v2 0/3] rev-list: add support for commits in `--missing`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-16T20:33:59Z","receivedAt":"2023-10-16T20:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Well, actually the newly introduced tests t6022 require this block,\n> but this is specific\n> to when commit graph is enabled.\n\nAh, of course.\n\n> Thanks for the quick review, I'll wait a day/two and send v3 with the\n> changes to fix tests\n> and cleaner code.\n\nThanks.\n"},{"id":"483331","messageId":"ZS4rmtBTYnp2RMiY@tanuki","threadId":"60331","inReplyTo":"xmqq1qdy1iyr.fsf@gitster.g","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-17T06:37:14Z","receivedAt":"2023-10-17T06:37:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 13, 2023 at 11:21:48AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,\n> >  \t\treturn -1;\n> >  \tif (item->object.parsed)\n> >  \t\treturn 0;\n> > -\tif (use_commit_graph && parse_commit_in_graph(r, item))\n> > +\tif (use_commit_graph && parse_commit_in_graph(r, item)) {\n> > +\t\tif (!has_object(r, &item->object.oid, 0))\n> > +\t\t\treturn quiet_on_missing ? -1 :\n> > +\t\t\t\terror(_(\"commit %s exists in commit-graph but not in the object database\"),\n> > +\t\t\t\t      oid_to_hex(&item->object.oid));\n> >  \t\treturn 0;\n> > +\t}\n> \n> Ever since this codepath was introduced by 177722b3 (commit:\n> integrate commit graph with commit parsing, 2018-04-10), we blindly\n> trusted what commit-graph file says.  This change is a strict\n> improvement in the correctness department, but there are two things\n> that are a bit worrying.\n> \n> One.  The additional check should certainly be cheaper than a full\n> reading and parsing of an object, either from a loose object file or\n> from a pack entry.  It may not hurt performance too much, but it\n> still would give us more confidence if we know by how much we are\n> pessimising good cases where the commit-graph does match reality.\n> Our stance on these secondary files that store precomputed values\n> for optimization purposes is in general to use them blindly unless\n> in exceptional cases where the operation values the correctness even\n> when the validity of these secondary files is dubious (e.g., \"fsck\"),\n> and doing this extra check regardless of the caller at this low level\n> of the callchain is a bit worrying.\n\nFair point indeed. The following is a worst-case scenario benchmark of\nof the change where we do a full topological walk of all reachable\ncommits in the graph, executed in linux.git. We parse commit parents via\n`repo_parse_commit_gently()`, so the new code path now basically has to\ncheck for object existence of every reachable commit:\n\nBenchmark 1: git -c core.commitGraph=true rev-list --topo-order --all (git = master)\n  Time (mean ± σ):      2.913 s ±  0.018 s    [User: 2.363 s, System: 0.548 s]\n  Range (min … max):    2.894 s …  2.950 s    10 runs\n\nBenchmark 2: git -c core.commitGraph=true rev-list --topo-order --all (git = pks-commit-graph-inconsistency)\n  Time (mean ± σ):      3.834 s ±  0.052 s    [User: 3.276 s, System: 0.556 s]\n  Range (min … max):    3.780 s …  3.961 s    10 runs\n\nBenchmark 3: git -c core.commitGraph=false rev-list --topo-order --all (git = master)\n  Time (mean ± σ):     13.841 s ±  0.084 s    [User: 13.152 s, System: 0.687 s]\n  Range (min … max):   13.714 s … 13.995 s    10 runs\n\nBenchmark 4: git -c core.commitGraph=false rev-list --topo-order --all (git = pks-commit-graph-inconsistency)\n  Time (mean ± σ):     13.762 s ±  0.116 s    [User: 13.094 s, System: 0.667 s]\n  Range (min … max):   13.645 s … 14.038 s    10 runs\n\nSummary\n  git -c core.commitGraph=true rev-list --topo-order --all (git = master) ran\n    1.32 ± 0.02 times faster than git -c core.commitGraph=true rev-list --topo-order --all (git = pks-commit-graph-inconsistency)\n    4.72 ± 0.05 times faster than git -c core.commitGraph=false rev-list --topo-order --all (git = pks-commit-graph-inconsistency)\n    4.75 ± 0.04 times faster than git -c core.commitGraph=false rev-list --topo-order --all (git = master)\n\nThe added check does lead to a performance regression indeed, which is\nnot all that unexpected. That being said, the commit-graph still results\nin a significant speedup compared to the case where we don't have it.\n\n> Another is that by the time parse_commit_in_graph() returns true and\n> we realize that the answer we got is bogus by asking has_object(),\n> item->object.parsed has already been toggled on, so the caller now\n> has a commit object that claimed it was already parsed and does not\n> match reality.  Hopefully the caller takes an early exit upon seeing\n> a failure from parse_commit_gently() and the .parsed bit does not\n> matter, but maybe I am missing a case where it does.  I dunno.\n\nWe could also call `unparse_commit()` when we notice the stale commit\ngraph item. This would be in the same spirit as the rest of this patch\nas it would lead to an overall safer end state.\n\nIn any case I'll wait for additional input before sending a v2, most\nimportantly to see whether we think that consistency trumps performance\nin this case. Personally I'm still of the mind that it should, which\nalso comes from the fact that we were fighting with stale commit graphs\nseveral times in production data.\n\nPatrick\n\n> Other than that, sounds very sensible and the code change is clean.\n> \n> Will queue.  Thanks.\n> \n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index ba65f17dd9..25f8e9e2d3 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '\n> >  \t)\n> >  '\n> >  \n> > +test_expect_success 'commit exists in commit-graph but not in object database' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\n> > +\t\ttest_commit A &&\n> > +\t\ttest_commit B &&\n> > +\t\ttest_commit C &&\n> > +\t\tgit commit-graph write --reachable &&\n> > +\n> > +\t\t# Corrupt the repository by deleting the intermittent commit\n> > +\t\t# object. Commands should notice that this object is absent and\n> > +\t\t# thus that the repository is corrupt even if the commit graph\n> > +\t\t# exists.\n> > +\t\toid=$(git rev-parse B) &&\n> > +\t\trm .git/objects/\"$(test_oid_to_path \"$oid\")\" &&\n> > +\n> > +\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n> > +\t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n> > +\t)\n> > +'\n> > +\n> >  test_done\n"},{"id":"483356","messageId":"xmqqjzrlhzci.fsf@gitster.g","threadId":"60331","inReplyTo":"ZS4rmtBTYnp2RMiY@tanuki","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T18:34:53Z","receivedAt":"2023-10-17T18:34:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Fair point indeed. The following is a worst-case scenario benchmark of\n> of the change where we do a full topological walk of all reachable\n> commits in the graph, executed in linux.git. We parse commit parents via\n> `repo_parse_commit_gently()`, so the new code path now basically has to\n> check for object existence of every reachable commit:\n> ...\n> The added check does lead to a performance regression indeed, which is\n> not all that unexpected. That being said, the commit-graph still results\n> in a significant speedup compared to the case where we don't have it.\n\nYeah, I agree that both points are expected.  An extra check that is\nmuch cheaper than the full parsing is paying a small price to be a\nbit more careful than before.  The question is if the price is small\nenough.  I am still not sure if the extra carefulness is warranted\nfor all normal cases to spend 30% extra cycles\n\n\nYeah, I agree that both points are expected.  An extra check that is\nmuch cheaper than the full parsing is paying a small price to be a\nbit more careful than before.  The question is if the price is small\nenough.  I am still not sure if the extra carefulness is warranted\nfor all normal cases to spend 30% extra cycles.\n\nThanks.\n"},{"id":"483470","messageId":"ZTDQjangLsQ1cSJl@tanuki","threadId":"60331","inReplyTo":"xmqqjzrlhzci.fsf@gitster.g","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-19T06:45:33Z","receivedAt":"2023-10-19T06:45:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 17, 2023 at 11:34:53AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Fair point indeed. The following is a worst-case scenario benchmark of\n> > of the change where we do a full topological walk of all reachable\n> > commits in the graph, executed in linux.git. We parse commit parents via\n> > `repo_parse_commit_gently()`, so the new code path now basically has to\n> > check for object existence of every reachable commit:\n> > ...\n> > The added check does lead to a performance regression indeed, which is\n> > not all that unexpected. That being said, the commit-graph still results\n> > in a significant speedup compared to the case where we don't have it.\n> \n> Yeah, I agree that both points are expected.  An extra check that is\n> much cheaper than the full parsing is paying a small price to be a\n> bit more careful than before.  The question is if the price is small\n> enough.  I am still not sure if the extra carefulness is warranted\n> for all normal cases to spend 30% extra cycles.\n> \n> Thanks.\n\nWell, seen in contexts like the thread that spawned this discussion I\nthink it's preferable to take a relatively smaller price compared to\ndisabling the commit graph entirely in some situations. With that in\nmind, personally I'd be happy with either of two outcomes here:\n\n    - We take the patch I proposed as a hardening measure, which allows\n      us to use the commit graph in basically any situation where we\n      could parse objects from the ODB without any downside except for a\n      performance hit.\n\n    - We say that corrupt repositories do not need to be accounted for\n      when using metadata caches like the commit-graph. That would mean\n      in consequence that for the `--missing` flag we would not have to\n      disable commit graphs.\n\nThe test failure that was exposed in Karthik's test only stems from the\nfact that we manually corrupt the repository and is not specific to the\n`--missing` flag. This is further stressed by the new test aded in my\nown patch, as we can trigger a similar bug when not using `--missing`.\n\nFor end users, object pruning would happen either via git-gc(1) or via\ngit-maintenance(1), and both of them do know to update commit graphs\nalready. This should significantly decrease the chance that such stale\ncommit graphs would be found out there in the wild, but it's still not\nimpossible of course. Especially in cases where you use lower-level\ncommands like git-repack(1) with cruft packs or git-prune(1), which is\noften the case for Git hosters, the risk is higher though.\n\nPatrick\n"},{"id":"483473","messageId":"ZTDn-Wd5xsFrBmqI@tanuki","threadId":"60331","inReplyTo":"ZTDQjangLsQ1cSJl@tanuki","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-19T08:25:29Z","receivedAt":"2023-10-19T08:25:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 19, 2023 at 08:45:34AM +0200, Patrick Steinhardt wrote:\n> On Tue, Oct 17, 2023 at 11:34:53AM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt <ps@pks.im> writes:\n> > \n> > > Fair point indeed. The following is a worst-case scenario benchmark of\n> > > of the change where we do a full topological walk of all reachable\n> > > commits in the graph, executed in linux.git. We parse commit parents via\n> > > `repo_parse_commit_gently()`, so the new code path now basically has to\n> > > check for object existence of every reachable commit:\n> > > ...\n> > > The added check does lead to a performance regression indeed, which is\n> > > not all that unexpected. That being said, the commit-graph still results\n> > > in a significant speedup compared to the case where we don't have it.\n> > \n> > Yeah, I agree that both points are expected.  An extra check that is\n> > much cheaper than the full parsing is paying a small price to be a\n> > bit more careful than before.  The question is if the price is small\n> > enough.  I am still not sure if the extra carefulness is warranted\n> > for all normal cases to spend 30% extra cycles.\n> > \n> > Thanks.\n> \n> Well, seen in contexts like the thread that spawned this discussion I\n> think it's preferable to take a relatively smaller price compared to\n> disabling the commit graph entirely in some situations. With that in\n> mind, personally I'd be happy with either of two outcomes here:\n> \n>     - We take the patch I proposed as a hardening measure, which allows\n>       us to use the commit graph in basically any situation where we\n>       could parse objects from the ODB without any downside except for a\n>       performance hit.\n> \n>     - We say that corrupt repositories do not need to be accounted for\n>       when using metadata caches like the commit-graph. That would mean\n>       in consequence that for the `--missing` flag we would not have to\n>       disable commit graphs.\n> \n> The test failure that was exposed in Karthik's test only stems from the\n> fact that we manually corrupt the repository and is not specific to the\n> `--missing` flag. This is further stressed by the new test aded in my\n> own patch, as we can trigger a similar bug when not using `--missing`.\n\nThere's another way to handle this, which is to conditionally enable the\nobject existence check. This would be less of a performance hit compared\nto disabling commit graphs altogether with `--missing`, but still ensure\nthat repository corruption was detected. Second, it would not regress\nperformance for all preexisting users of `repo_parse_commit_gently()`.\n\nPatrick\n"},{"id":"483480","messageId":"20231019121024.194317-1-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231016103830.56486-1-karthik.188@gmail.com","subject":"[PATCH v3 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-19T12:10:21Z","receivedAt":"2023-10-19T12:10:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` option in git-rev-list(1) was introduced intitally\nto deal with missing blobs in the context of promissory notes.\nEventually the option was extended to also support tree objects in\n7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).\n\nThis patch series extends the `--missing` option to also add support for\ncommit objects. We do this by introducing a new flag `MISSING` which is\nadded whenever we encounter a missing commit during traversal. Then in\n`builtin/rev-list` we check for this flag and take the appropriate\naction based on the `--missing=*` option used.\n\nThis series is an alternate to the patch series I had posted earlier:\nhttps://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\nIn that patch, we introduced an option `--ignore-missing-links` which\nwas added to expose the `ignore_missing_links` bit to the user. The\nissue in that patch was that, the option `--ignore-missing-links` didn't\nplay well the pre-existing `--missing` option. This series avoids that\nroute and just extends the `--missing` option for commits to solve the\nsame problem.\n\nV2 of the series can be found here: http://public-inbox.org/git/ZSkCGS3JPEQ71dOF@tanuki/T/#m10b562ec02834d0b222835a24a18e6ca725f99d1\n\nChanges from v2: \n\n* Change the flag MISSING's bit number to avoid collision with NEEDS_BITMAP\n  which was causing t5310 and t532 to fail.\n\nRange diff from v2:\n\n1:  a7018a319a = 1:  fa35cd21e6 revision: rename bit to `do_not_die_on_missing_objects`\n2:  b186a679d4 = 2:  31074df376 rev-list: move `show_commit()` to the bottom\n3:  0c8c140c27 ! 3:  d9b0d6da0c rev-list: add commit object support in `--missing` option\n    @@ object.h: void object_array_init(struct object_array *array);\n      /*\n       * object flag allocation:\n     - * revision.h:               0---------10         15             23------27\n    -+ * revision.h:               0---------10         15             22------27\n    ++ * revision.h:               0---------10         15             22------28\n       * fetch-pack.c:             01    67\n       * negotiator/default.c:       2--5\n       * walker.c:                 0-2\n    +@@ object.h: void object_array_init(struct object_array *array);\n    +  * builtin/show-branch.c:    0-------------------------------------------26\n    +  * builtin/unpack-objects.c:                                 2021\n    +  */\n    +-#define FLAG_BITS  28\n    ++#define FLAG_BITS  29\n    + \n    + #define TYPE_BITS 3\n    + \n     \n      ## revision.c ##\n     @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,\n    @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n      \tunsigned pass_flags;\n      \n     -\tif (commit->object.flags & ADDED)\n    -+\tif (commit->object.flags & ADDED || commit->object.flags & MISSING)\n    ++\tif (commit->object.flags & (ADDED | MISSING))\n      \t\treturn 0;\n      \tcommit->object.flags |= ADDED;\n      \n    @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n     \n      ## revision.h ##\n     @@\n    - /* WARNING: This is also used as REACHABLE in commit-graph.c. */\n    - #define PULL_MERGE\t(1u<<15)\n    + #define ANCESTRY_PATH\t(1u<<27)\n    + #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)\n      \n    -+/* WARNING: This is also used as NEEDS_BITMAP in pack-bitmap.c. */\n    -+#define MISSING         (1u<<22)\n    ++#define MISSING (1u<<28)\n     +\n    - #define TOPO_WALK_EXPLORED\t(1u<<23)\n    - #define TOPO_WALK_INDEGREE\t(1u<<24)\n    + #define DECORATE_SHORT_REFS\t1\n    + #define DECORATE_FULL_REFS\t2\n      \n     \n      ## t/t6022-rev-list-missing.sh (new) ##\n\n\nKarthik Nayak (3):\n  revision: rename bit to `do_not_die_on_missing_objects`\n  rev-list: move `show_commit()` to the bottom\n  rev-list: add commit object support in `--missing` option\n\n builtin/reflog.c            |  2 +-\n builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n list-objects.c              |  4 +-\n object.h                    |  4 +-\n revision.c                  | 11 +++--\n revision.h                  | 19 ++++----\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n 7 files changed, 147 insertions(+), 60 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\n-- \n2.42.0\n\n"},{"id":"483481","messageId":"20231019121024.194317-2-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231019121024.194317-1-karthik.188@gmail.com","subject":"[PATCH v3 1/3] revision: rename bit to `do_not_die_on_missing_objects`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-19T12:10:22Z","receivedAt":"2023-10-19T12:10:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the\nrevision walker does not die when encountering a missing tree. This is\ncurrently exclusively set within `builtin/rev-list.c` to ensure the\n`--missing` option works with missing trees.\n\nIn the upcoming commits, we will extend `--missing` to also support\nmissing commits. So let's rename the bit to\n`do_not_die_on_missing_objects`, which is object type agnostic and can\nbe used for both trees/commits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/reflog.c   |  2 +-\n builtin/rev-list.c |  2 +-\n list-objects.c     |  2 +-\n revision.h         | 17 +++++++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df63a5892e..9e369a5977 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tstruct rev_info revs;\n \n \t\trepo_init_revisions(the_repository, &revs, prefix);\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \t\trevs.ignore_missing = 1;\n \t\trevs.ignore_missing_links = 1;\n \t\tif (verbose)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..ea77489c38 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (arg_missing_action)\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex c25c72b32c..47296dff2f 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,\n \t\t    is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n-\t\tif (!revs->do_not_die_on_missing_tree)\n+\t\tif (!revs->do_not_die_on_missing_objects)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \ndiff --git a/revision.h b/revision.h\nindex 50091bbd13..c73c92ef40 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -212,18 +212,19 @@ struct rev_info {\n \n \t\t\t/*\n \t\t\t * Blobs are shown without regard for their existence.\n-\t\t\t * But not so for trees: unless exclude_promisor_objects\n+\t\t\t * But not so for trees/commits: unless exclude_promisor_objects\n \t\t\t * is set and the tree in question is a promisor object;\n \t\t\t * OR ignore_missing_links is set, the revision walker\n-\t\t\t * dies with a \"bad tree object HASH\" message when\n-\t\t\t * encountering a missing tree. For callers that can\n-\t\t\t * handle missing trees and want them to be filterable\n+\t\t\t * dies with a \"bad <type> object HASH\" message when\n+\t\t\t * encountering a missing object. For callers that can\n+\t\t\t * handle missing trees/commits and want them to be filterable\n \t\t\t * and showable, set this to true. The revision walker\n-\t\t\t * will filter and show such a missing tree as usual,\n-\t\t\t * but will not attempt to recurse into this tree\n-\t\t\t * object.\n+\t\t\t * will filter and show such a missing object as usual,\n+\t\t\t * but will not attempt to recurse into this tree/commit\n+\t\t\t * object. The revision walker will also set the MISSING\n+\t\t\t * flag for such objects.\n \t\t\t */\n-\t\t\tdo_not_die_on_missing_tree:1,\n+\t\t\tdo_not_die_on_missing_objects:1,\n \n \t\t\t/* for internal use only */\n \t\t\texclude_promisor_objects:1;\n-- \n2.42.0\n\n"},{"id":"483482","messageId":"20231019121024.194317-3-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231019121024.194317-1-karthik.188@gmail.com","subject":"[PATCH v3 2/3] rev-list: move `show_commit()` to the bottom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-19T12:10:23Z","receivedAt":"2023-10-19T12:10:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `show_commit()` function already depends on `finish_commit()`, and\nin the upcoming commit, we'll also add a dependency on\n`finish_object__ma()`. Since in C symbols must be declared before\nthey're used, let's move `show_commit()` below both `finish_commit()`\nand `finish_object__ma()`, so the code is cleaner as a whole without the\nneed for declarations.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ea77489c38..98542e8b3c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)\n \treturn size;\n }\n \n-static void finish_commit(struct commit *commit);\n+static inline void finish_object__ma(struct object *obj)\n+{\n+\t/*\n+\t * Whether or not we try to dynamically fetch missing objects\n+\t * from the server, we currently DO NOT have the object.  We\n+\t * can either print, allow (ignore), or conditionally allow\n+\t * (ignore) them.\n+\t */\n+\tswitch (arg_missing_action) {\n+\tcase MA_ERROR:\n+\t\tdie(\"missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tcase MA_ALLOW_ANY:\n+\t\treturn;\n+\n+\tcase MA_PRINT:\n+\t\toidset_insert(&missing_objects, &obj->oid);\n+\t\treturn;\n+\n+\tcase MA_ALLOW_PROMISOR:\n+\t\tif (is_promisor_object(&obj->oid))\n+\t\t\treturn;\n+\t\tdie(\"unexpected missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tdefault:\n+\t\tBUG(\"unhandled missing_action\");\n+\t\treturn;\n+\t}\n+}\n+\n+static void finish_commit(struct commit *commit)\n+{\n+\tfree_commit_list(commit->parents);\n+\tcommit->parents = NULL;\n+\tfree_commit_buffer(the_repository->parsed_objects,\n+\t\t\t   commit);\n+}\n+\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)\n \tfinish_commit(commit);\n }\n \n-static void finish_commit(struct commit *commit)\n-{\n-\tfree_commit_list(commit->parents);\n-\tcommit->parents = NULL;\n-\tfree_commit_buffer(the_repository->parsed_objects,\n-\t\t\t   commit);\n-}\n-\n-static inline void finish_object__ma(struct object *obj)\n-{\n-\t/*\n-\t * Whether or not we try to dynamically fetch missing objects\n-\t * from the server, we currently DO NOT have the object.  We\n-\t * can either print, allow (ignore), or conditionally allow\n-\t * (ignore) them.\n-\t */\n-\tswitch (arg_missing_action) {\n-\tcase MA_ERROR:\n-\t\tdie(\"missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tcase MA_ALLOW_ANY:\n-\t\treturn;\n-\n-\tcase MA_PRINT:\n-\t\toidset_insert(&missing_objects, &obj->oid);\n-\t\treturn;\n-\n-\tcase MA_ALLOW_PROMISOR:\n-\t\tif (is_promisor_object(&obj->oid))\n-\t\t\treturn;\n-\t\tdie(\"unexpected missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tdefault:\n-\t\tBUG(\"unhandled missing_action\");\n-\t\treturn;\n-\t}\n-}\n-\n static int finish_object(struct object *obj, const char *name UNUSED,\n \t\t\t void *cb_data)\n {\n-- \n2.42.0\n\n"},{"id":"483483","messageId":"20231019121024.194317-4-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231019121024.194317-1-karthik.188@gmail.com","subject":"[PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-19T12:10:24Z","receivedAt":"2023-10-19T12:10:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` object option in rev-list currently works only with\nmissing blobs/trees. For missing commits the revision walker fails with\na fatal error.\n\nLet's extend the functionality of `--missing` option to also support\ncommit objects. This is done by adding a new `MISSING` flag that the\nrevision walker sets whenever it encounters a missing tree/commit. The\nrevision walker will now continue the traversal and call `show_commit()`\neven for missing commits. In rev-list we can then check for this flag\nand call the existing code for parsing `--missing` objects.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, using the `--missing=print` option while disabling the\nalternate object directory allows us to find the boundary objects\nbetween the main and alternate object directory.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c          |  6 +++\n list-objects.c              |  2 +-\n object.h                    |  4 +-\n revision.c                  | 11 ++++--\n revision.h                  |  2 +\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n 6 files changed, 93 insertions(+), 6 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 98542e8b3c..604ae01d0c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n \n \tdisplay_progress(progress, ++progress_counter);\n \n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    commit->object.flags & MISSING) {\n+\t\tfinish_object__ma(&commit->object);\n+\t\treturn;\n+\t}\n+\n \tif (show_disk_usage)\n \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 47296dff2f..60c5533721 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -387,7 +387,7 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t * an uninteresting boundary commit may not have its tree\n \t\t * parsed yet, but we are not going to show them anyway\n \t\t */\n-\t\tif (!ctx->revs->tree_objects)\n+\t\tif (!ctx->revs->tree_objects || commit->object.flags & MISSING)\n \t\t\t; /* do not bother loading tree */\n \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\ndiff --git a/object.h b/object.h\nindex 114d45954d..b76830fce1 100644\n--- a/object.h\n+++ b/object.h\n@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);\n \n /*\n  * object flag allocation:\n- * revision.h:               0---------10         15             23------27\n+ * revision.h:               0---------10         15             22------28\n  * fetch-pack.c:             01    67\n  * negotiator/default.c:       2--5\n  * walker.c:                 0-2\n@@ -82,7 +82,7 @@ void object_array_init(struct object_array *array);\n  * builtin/show-branch.c:    0-------------------------------------------26\n  * builtin/unpack-objects.c:                                 2021\n  */\n-#define FLAG_BITS  28\n+#define FLAG_BITS  29\n \n #define TYPE_BITS 3\n \ndiff --git a/revision.c b/revision.c\nindex 219dc76716..8100806068 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1110,7 +1110,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tstruct commit_list *parent = commit->parents;\n \tunsigned pass_flags;\n \n-\tif (commit->object.flags & ADDED)\n+\tif (commit->object.flags & (ADDED | MISSING))\n \t\treturn 0;\n \tcommit->object.flags |= ADDED;\n \n@@ -1168,7 +1168,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \t\tint gently = revs->ignore_missing_links ||\n-\t\t\t     revs->exclude_promisor_objects;\n+\t\t\t     revs->exclude_promisor_objects ||\n+\t\t\t     revs->do_not_die_on_missing_objects;\n \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&p->object.oid)) {\n@@ -1176,7 +1177,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\t\tbreak;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\treturn -1;\n+\n+\t\t\tif (!revs->do_not_die_on_missing_objects)\n+\t\t\t\treturn -1;\n+\t\t\telse\n+\t\t\t\tp->object.flags |= MISSING;\n \t\t}\n \t\tif (revs->sources) {\n \t\t\tchar **slot = revision_sources_at(revs->sources, p);\ndiff --git a/revision.h b/revision.h\nindex c73c92ef40..d3c1ca0f4e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -53,6 +53,8 @@\n #define ANCESTRY_PATH\t(1u<<27)\n #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)\n \n+#define MISSING (1u<<28)\n+\n #define DECORATE_SHORT_REFS\t1\n #define DECORATE_FULL_REFS\t2\n \ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nnew file mode 100755\nindex 0000000000..40265a4f66\n--- /dev/null\n+++ b/t/t6022-rev-list-missing.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We setup the repository with two commits, this way HEAD is always\n+# available and we can hide commit 1.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit 1 &&\n+\ttest_commit 2 &&\n+\ttest_commit 3\n+'\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n+\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\ttest_must_fail git rev-list --missing=error --objects \\\n+\t\t\t--no-object-names HEAD\n+\t'\n+done\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor action in \"allow-any\" \"print\"\n+\tdo\n+\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n+\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t# get the expected oids.\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\tHEAD ^$obj >expect.raw &&\n+\n+\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n+\t\t\t# might be skipped, its blob must be accounted for.\n+\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n+\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n+\t\t\tfi &&\n+\n+\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n+\t\t\t\tHEAD >actual.raw &&\n+\n+\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t# oid to the expect list.\n+\t\t\tcase $action in\n+\t\t\tallow-any)\n+\t\t\t\t;;\n+\t\t\tprint)\n+\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\n+\t\t\tsort actual.raw >actual &&\n+\t\t\tsort expect.raw >expect &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n+\n+test_done\n-- \n2.42.0\n\n"},{"id":"483498","messageId":"xmqqzg0ey1kn.fsf@gitster.g","threadId":"60331","inReplyTo":"ZTDn-Wd5xsFrBmqI@tanuki","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-19T17:16:56Z","receivedAt":"2023-10-19T17:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> There's another way to handle this, which is to conditionally enable the\n> object existence check. This would be less of a performance hit compared\n> to disabling commit graphs altogether with `--missing`, but still ensure\n> that repository corruption was detected. Second, it would not regress\n> performance for all preexisting users of `repo_parse_commit_gently()`.\n\nThe above was what I meant to suggest when you demonstrated that the\ncode with additional check is still much more performant than\nrunning without the commit-graph optimization, yet has observable\nimpact on performance for normal codepaths that do not need the\nextra carefulness.\n\nBut I wasn't sure if it is easy to plumb the \"do we want to double\ncheck?  in other words, are we running something like --missing that\ncare the correctness a bit more than usual cases?\" bit down from the\ncaller, because this check is so deep in the callchain.\n\nThanks.\n\n"},{"id":"483532","messageId":"xmqq4jimuv26.fsf@gitster.g","threadId":"60331","inReplyTo":"20231019121024.194317-4-karthik.188@gmail.com","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-19T22:05:53Z","receivedAt":"2023-10-19T22:05:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> diff --git a/object.h b/object.h\n> index 114d45954d..b76830fce1 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);\n>  \n>  /*\n>   * object flag allocation:\n> - * revision.h:               0---------10         15             23------27\n> + * revision.h:               0---------10         15             22------28\n>   * fetch-pack.c:             01    67\n>   * negotiator/default.c:       2--5\n>   * walker.c:                 0-2\n> @@ -82,7 +82,7 @@ void object_array_init(struct object_array *array);\n>   * builtin/show-branch.c:    0-------------------------------------------26\n>   * builtin/unpack-objects.c:                                 2021\n>   */\n> -#define FLAG_BITS  28\n> +#define FLAG_BITS  29\n>  \n>  #define TYPE_BITS 3\n\nI am afraid that this is not a good direction to go, given that the\nway FLAG_BITS is used is like this:\n\n    /*\n     * The object type is stored in 3 bits.\n     */\n    struct object {\n            unsigned parsed : 1;\n            unsigned type : TYPE_BITS;\n            unsigned flags : FLAG_BITS;\n            struct object_id oid;\n    };\n\n28 was there, not as a random number of bits we happen to be using.\nIt was derived by (32 - 3 - 1), i.e. ensure the bitfields above are\nstored within a single word.\n\nsizeof(struct object) is 40 bytes on x86-64, with offsetof(oid)\nbeing 4 (i.e. the bitfields fit in a single 4-byte word).  If we\nmake FLAG_BITS 29, we will add 4 bytes to the structure and waste\n31-bit per each and every in-core objects.\n\nDo we really need to allocate a new bit in the object flags, which\nis already a scarce resource?\n"},{"id":"483533","messageId":"xmqqttqmtcc2.fsf@gitster.g","threadId":"60331","inReplyTo":"xmqq4jimuv26.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-19T23:35:41Z","receivedAt":"2023-10-19T23:35:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Do we really need to allocate a new bit in the object flags, which\n> is already a scarce resource?\n\nClarification.  I was *not* wondering if we can steal and (re|ab)use\na bit that is used for other purposes, in order to avoid allocating\na new bit.\n\nRather, I was wondering if we need to use object flags to mark these\nobjects, or can do what we want to do without using any object flags\nat all.  For the purpose of reporting \"missing\" objects, wouldn't it\nbe sufficient to walk the object graph and report our findings as we\ngo?  To avoid reporting the same object twice, as we reasonably can\nexpect that the missing objects are minority (compared to the total\nnumber of objects), perhaps the codepath that makes such a report\ncan use a hashmap of object_ids or something, for example.\n\nThanks.\n"},{"id":"483553","messageId":"20231020100024.GA2194074@coredump.intra.peff.net","threadId":"60331","inReplyTo":"xmqqzg0ey1kn.fsf@gitster.g","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:00:24Z","receivedAt":"2023-10-20T10:00:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2023 at 10:16:56AM -0700, Junio C Hamano wrote:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > There's another way to handle this, which is to conditionally enable the\n> > object existence check. This would be less of a performance hit compared\n> > to disabling commit graphs altogether with `--missing`, but still ensure\n> > that repository corruption was detected. Second, it would not regress\n> > performance for all preexisting users of `repo_parse_commit_gently()`.\n> \n> The above was what I meant to suggest when you demonstrated that the\n> code with additional check is still much more performant than\n> running without the commit-graph optimization, yet has observable\n> impact on performance for normal codepaths that do not need the\n> extra carefulness.\n> \n> But I wasn't sure if it is easy to plumb the \"do we want to double\n> check?  in other words, are we running something like --missing that\n> care the correctness a bit more than usual cases?\" bit down from the\n> caller, because this check is so deep in the callchain.\n\nI wonder if we would want a \"be extra careful\" flag that is read from\nthe environment? That is largely how GIT_REF_PARANOIA works, and then\nparticular operations set it (though actually it is the default these\ndays, so they no longer do so explicitly).\n\nI guess that is really like a global variable but even more gross. ;)\nBut it is nice that it can cross process boundaries, because \"how\ncareful do we want to be\" may be in the eye of the caller, especially\nfor plumbing commands. E.g., even without --missing, you may want\n\"rev-list\" to be extra careful about checking the existence of commits.\nThe caller in check_connected() could arguably turn that on by default\n(the way we used to turn on GIT_REF_PARANOIA for pruning repacks before\nit became the default).\n\n-Peff\n"},{"id":"483562","messageId":"CAOLa=ZR4Qd9x-zVv9OtL7KTU00W2uT-kxdhKOLFZbi5cnHdiSQ@mail.gmail.com","threadId":"60331","inReplyTo":"xmqqttqmtcc2.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-20T11:14:52Z","receivedAt":"2023-10-20T11:15:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Oct 20, 2023 at 1:35 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Do we really need to allocate a new bit in the object flags, which\n> > is already a scarce resource?\n>\n> Clarification.  I was *not* wondering if we can steal and (re|ab)use\n> a bit that is used for other purposes, in order to avoid allocating\n> a new bit.\n>\n> Rather, I was wondering if we need to use object flags to mark these\n> objects, or can do what we want to do without using any object flags\n> at all.  For the purpose of reporting \"missing\" objects, wouldn't it\n> be sufficient to walk the object graph and report our findings as we\n> go?  To avoid reporting the same object twice, as we reasonably can\n> expect that the missing objects are minority (compared to the total\n> number of objects), perhaps the codepath that makes such a report\n> can use a hashmap of object_ids or something, for example.\n>\n\nThis is kind of hard to do since the revision walking happens in revision.c\nand list-objects.c but the missing commits are needed in builtin/rev-list.c.\nSo even if we build a list of missing commits in list-objects.c, there is no\nnice way to send this back to rev-list.c.\n\nThe only way we communicate between these layers is through callbacks,\ni.e. the show_commit() function in rev-list.c is called for every commit that\nthe revision walk traverses over.\n\nI'm not entirely sure what the best path to take here to be honest. I will look\nand see if there are any bits where overlapping doesn't cause any issues\nin the meantime.\n"},{"id":"483565","messageId":"CAOLa=ZT6qEsVfNETYua=RjjepDsXFj8uSdphS2LR1gGK1_sGFg@mail.gmail.com","threadId":"60331","inReplyTo":"CAOLa=ZR4Qd9x-zVv9OtL7KTU00W2uT-kxdhKOLFZbi5cnHdiSQ@mail.gmail.com","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-20T14:47:11Z","receivedAt":"2023-10-20T14:47:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Oct 20, 2023 at 1:14 PM Karthik Nayak <karthik.188@gmail.com> wrote:\n> I'm not entirely sure what the best path to take here to be honest. I will look\n> and see if there are any bits where overlapping doesn't cause any issues\n> in the meantime.\n\nWas trying to use bit number 12, which coincides METAINFO_SHOWN in\nbuiltin/blame.c.\nFrom skimming over the code, METAINFO_SHOWN is used only within\nblame.c and there\nshould not be collisions here since blame.c doesn't set the\ndo_not_die_on_missing_objects bit\neither.\n\nThe tests seem to pass here too [1]. This could be the potential\nsolution here, Junio, what do you think?\nI could send another version if this approach looks good.\n\n[1] https://gitlab.com/gitlab-org/git/-/jobs/5339132966\n\nDiff against the tip of v3:\n\ndiff --git a/object.h b/object.h\nindex b76830fce1..0cb8c02b95 100644\n--- a/object.h\n+++ b/object.h\n@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);\n\n /*\n  * object flag allocation:\n- * revision.h:               0---------10         15             22------28\n+ * revision.h:               0---------10  12    15             22------27\n  * fetch-pack.c:             01    67\n  * negotiator/default.c:       2--5\n  * walker.c:                 0-2\n@@ -82,7 +82,7 @@ void object_array_init(struct object_array *array);\n  * builtin/show-branch.c:    0-------------------------------------------26\n  * builtin/unpack-objects.c:                                 2021\n  */\n-#define FLAG_BITS  29\n+#define FLAG_BITS  28\n\n #define TYPE_BITS 3\n\ndiff --git a/revision.h b/revision.h\nindex d3c1ca0f4e..d2de9d65e4 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -38,6 +38,9 @@\n #define PATCHSAME    (1u<<9)\n #define BOTTOM        (1u<<10)\n\n+/* WARNING: This is also used as METAINFO_SHOWN in builtin/blame.c. */\n+#define MISSING     (1u<<12)\n+\n /* WARNING: This is also used as REACHABLE in commit-graph.c. */\n #define PULL_MERGE    (1u<<15)\n\n@@ -53,7 +56,6 @@\n #define ANCESTRY_PATH    (1u<<27)\n #define ALL_REV_FLAGS    (((1u<<11)-1) | NOT_USER_GIVEN |\nTRACK_LINEAR | PULL_MERGE)\n\n-#define MISSING (1u<<28)\n\n #define DECORATE_SHORT_REFS    1\n #define DECORATE_FULL_REFS    2\n"},{"id":"483568","messageId":"xmqq1qdptffk.fsf@gitster.g","threadId":"60331","inReplyTo":"xmqqttqmtcc2.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T16:41:03Z","receivedAt":"2023-10-20T16:41:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Rather, I was wondering if we need to use object flags to mark these\n> objects, or can do what we want to do without using any object flags\n> at all.  For the purpose of reporting \"missing\" objects, wouldn't it\n> be sufficient to walk the object graph and report our findings as we\n> go?  To avoid reporting the same object twice, as we reasonably can\n> expect that the missing objects are minority (compared to the total\n> number of objects), perhaps the codepath that makes such a report\n> can use a hashmap of object_ids or something, for example.\n\nDigging from the bottom,\n\n * builtin/rev-list.c:show_commit() gets \"struct rev_list_info *\"\n   that has \"struct rev_info *\" [*].\n\n * list-objects.c:do_traverse() calls revision.c:get_revision() to\n   obtain commits, some of which may be missing ones, and things\n   behind get_revision() are responsible for marking the commit as\n   missing.  It has \"struct traversal_context *\", among whose\n   members is the \"revs\" member that is the \"struct rev_info *\".\n\n * revision.c:get_revision() and machinery behind it ultimately\n   discovers a missing commit in the revision.c:process_parents()\n   that loops over the parents commit_list.  It of course has access\n   to \"struct rev_info *\".\n\nSo, presumably, if we add a new member to \"struct rev_info\" that\noptionally [*] points at an oidset that records the object names of\nmissing objects we discovered so far (i.e., the set of missing\nobjects), the location we set the MISSING bit of a commit can\ninstead add the object name of the commit to the set.  And we can\nexport a function that takes \"struct rev_info *\" and \"struct object\n*\" (or \"struct object_id *\") to check for membership in the \"set of\nmissing objects\", which would be used where we checked the MISSING\nbit of a commit.\n\nI do not know the performance implications of going this route, but\nif we do not find a suitable vacant bit, we do not have to use any\nobject flags bit to do this, if we go this route, I would think.  I\nmay be missing some details that breaks the above outline, though.\n\n\n[Footnotes]\n\n * A potential #leftoverbits tangent.\n\n   Why is \"rev_list_info\" structure declared in <bisect.h>?  I\n   suspect that this is a fallout from recent header file shuffling,\n   but given who uses it (among which is rev-list:show_commit() that\n   has very little to do with bisection and uses the information in\n   rev_list_info when doing its normal non-bisect things), it does\n   not make much sense.\n\n * When .do_not_die_on_missing_objects is false, it can and should\n   be left NULL, but presumably we use the \"do not die\" bit even\n   when we are not necessarily collecting the missing objects?  So\n   the new member cannot replace the \"do not die\" bit completely.\n"},{"id":"483579","messageId":"xmqqpm19qjrs.fsf@gitster.g","threadId":"60331","inReplyTo":"20231020100024.GA2194074@coredump.intra.peff.net","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T17:35:35Z","receivedAt":"2023-10-20T17:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I guess that is really like a global variable but even more gross. ;)\n\nYeah, I tend to agree, but ...\n\n> But it is nice that it can cross process boundaries, because \"how\n> careful do we want to be\" may be in the eye of the caller, especially\n> for plumbing commands. E.g., even without --missing, you may want\n> \"rev-list\" to be extra careful about checking the existence of commits.\n\n... these are all very good points.\n\n> The caller in check_connected() could arguably turn that on by default\n> (the way we used to turn on GIT_REF_PARANOIA for pruning repacks before\n> it became the default).\n\nTrue.\n\nThanks.\n\n"},{"id":"483580","messageId":"xmqqh6mlqjb8.fsf@gitster.g","threadId":"60331","inReplyTo":"CAOLa=ZT6qEsVfNETYua=RjjepDsXFj8uSdphS2LR1gGK1_sGFg@mail.gmail.com","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T17:45:31Z","receivedAt":"2023-10-20T17:45:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Was trying to use bit number 12, which coincides METAINFO_SHOWN in\n> builtin/blame.c.\n> From skimming over the code, METAINFO_SHOWN is used only within\n> blame.c and there\n> should not be collisions here since blame.c doesn't set the\n> do_not_die_on_missing_objects bit\n> either.\n\nThanks for researching.  It sounds like it may be a better bit to\nsteal than the one used by the commit-graph, as long as there is no\nreason to expect that blame may want to work in a corrupt repository\nwith missing objects, but when it happens, we may regret the\ndecision we are making here.\n\nThanks.\n"},{"id":"483659","messageId":"ZTZHxh4v0_fKkZpg@tanuki","threadId":"60331","inReplyTo":"20231020100024.GA2194074@coredump.intra.peff.net","subject":"Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T10:15:34Z","receivedAt":"2023-10-23T10:15:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 20, 2023 at 06:00:24AM -0400, Jeff King wrote:\n> On Thu, Oct 19, 2023 at 10:16:56AM -0700, Junio C Hamano wrote:\n> \n> > Patrick Steinhardt <ps@pks.im> writes:\n> > \n> > > There's another way to handle this, which is to conditionally enable the\n> > > object existence check. This would be less of a performance hit compared\n> > > to disabling commit graphs altogether with `--missing`, but still ensure\n> > > that repository corruption was detected. Second, it would not regress\n> > > performance for all preexisting users of `repo_parse_commit_gently()`.\n> > \n> > The above was what I meant to suggest when you demonstrated that the\n> > code with additional check is still much more performant than\n> > running without the commit-graph optimization, yet has observable\n> > impact on performance for normal codepaths that do not need the\n> > extra carefulness.\n> > \n> > But I wasn't sure if it is easy to plumb the \"do we want to double\n> > check?  in other words, are we running something like --missing that\n> > care the correctness a bit more than usual cases?\" bit down from the\n> > caller, because this check is so deep in the callchain.\n> \n> I wonder if we would want a \"be extra careful\" flag that is read from\n> the environment? That is largely how GIT_REF_PARANOIA works, and then\n> particular operations set it (though actually it is the default these\n> days, so they no longer do so explicitly).\n> \n> I guess that is really like a global variable but even more gross. ;)\n> But it is nice that it can cross process boundaries, because \"how\n> careful do we want to be\" may be in the eye of the caller, especially\n> for plumbing commands. E.g., even without --missing, you may want\n> \"rev-list\" to be extra careful about checking the existence of commits.\n> The caller in check_connected() could arguably turn that on by default\n> (the way we used to turn on GIT_REF_PARANOIA for pruning repacks before\n> it became the default).\n\nGood point indeed, I also started to ask myself whether we'd want to\nhave a config option. An environment variable `GIT_OBJECT_PARANOIA`\nwould work equally well though and be less \"official\".\n\nWhat I like about this idea is that it would also allow us to easily\nunify the behaviour of `lookup_commit_in_graph()` and the new sanity\ncheck in `repo_parse_commit_gently()` that I'm introducing here. I'd\npersonally think that the default for any such toggle should be `true`,\nalso to keep the current behaviour of `lookup_commit_in_graph()`. But\nchanging it would be easy enough.\n\nI'll send a v2 of this series with such a toggle.\n\nPatrick\n"},{"id":"483764","messageId":"CAOLa=ZTUiYi1YqSuirmB9e_7B8ihC6rsFAR3VTDCL4SoLrEX_Q@mail.gmail.com","threadId":"60331","inReplyTo":"xmqq1qdptffk.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-24T11:34:01Z","receivedAt":"2023-10-24T11:34:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Oct 20, 2023 at 6:41 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Rather, I was wondering if we need to use object flags to mark these\n> > objects, or can do what we want to do without using any object flags\n> > at all.  For the purpose of reporting \"missing\" objects, wouldn't it\n> > be sufficient to walk the object graph and report our findings as we\n> > go?  To avoid reporting the same object twice, as we reasonably can\n> > expect that the missing objects are minority (compared to the total\n> > number of objects), perhaps the codepath that makes such a report\n> > can use a hashmap of object_ids or something, for example.\n>\n> Digging from the bottom,\n>\n>  * builtin/rev-list.c:show_commit() gets \"struct rev_list_info *\"\n>    that has \"struct rev_info *\" [*].\n>\n>  * list-objects.c:do_traverse() calls revision.c:get_revision() to\n>    obtain commits, some of which may be missing ones, and things\n>    behind get_revision() are responsible for marking the commit as\n>    missing.  It has \"struct traversal_context *\", among whose\n>    members is the \"revs\" member that is the \"struct rev_info *\".\n>\n>  * revision.c:get_revision() and machinery behind it ultimately\n>    discovers a missing commit in the revision.c:process_parents()\n>    that loops over the parents commit_list.  It of course has access\n>    to \"struct rev_info *\".\n>\n> So, presumably, if we add a new member to \"struct rev_info\" that\n> optionally [*] points at an oidset that records the object names of\n> missing objects we discovered so far (i.e., the set of missing\n> objects), the location we set the MISSING bit of a commit can\n> instead add the object name of the commit to the set.  And we can\n> export a function that takes \"struct rev_info *\" and \"struct object\n> *\" (or \"struct object_id *\") to check for membership in the \"set of\n> missing objects\", which would be used where we checked the MISSING\n> bit of a commit.\n>\n> I do not know the performance implications of going this route, but\n> if we do not find a suitable vacant bit, we do not have to use any\n> object flags bit to do this, if we go this route, I would think.  I\n> may be missing some details that breaks the above outline, though.\n>\n>\n> [Footnotes]\n>\n>  * A potential #leftoverbits tangent.\n>\n>    Why is \"rev_list_info\" structure declared in <bisect.h>?  I\n>    suspect that this is a fallout from recent header file shuffling,\n>    but given who uses it (among which is rev-list:show_commit() that\n>    has very little to do with bisection and uses the information in\n>    rev_list_info when doing its normal non-bisect things), it does\n>    not make much sense.\n>\n>  * When .do_not_die_on_missing_objects is false, it can and should\n>    be left NULL, but presumably we use the \"do not die\" bit even\n>    when we are not necessarily collecting the missing objects?  So\n>    the new member cannot replace the \"do not die\" bit completely.\n\nThanks for the suggestion, this does seem like a good way to go ahead without\nusing flags. The only performance issue being if there are too many commits\nwhich are missing, then oidset would be large.\n\nBut I think that's okay though.\n\n> Thanks for researching.  It sounds like it may be a better bit to\n> steal than the one used by the commit-graph, as long as there is no\n> reason to expect that blame may want to work in a corrupt repository\n> with missing objects, but when it happens, we may regret the\n> decision we are making here.\n>\n\nI don't see blame working with missing commits though, because it relies on\nparsing commits to get information to show to the user. So I think it's a safe\nbit to steal. Also, when the time comes we could always release the bit and\nmove to the solution you mentioned above.\n\nAnyways on the whole I think keeping it future compatible makes a lot\nmore sense.\nI'll send a patch series to implement an oidset instead of flags soon.\n\n- Karthik\n"},{"id":"483765","messageId":"20231024122631.158415-2-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231024122631.158415-1-karthik.188@gmail.com","subject":"[PATCH v4 1/3] revision: rename bit to `do_not_die_on_missing_objects`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-24T12:26:29Z","receivedAt":"2023-10-24T12:26:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the\nrevision walker does not die when encountering a missing tree. This is\ncurrently exclusively set within `builtin/rev-list.c` to ensure the\n`--missing` option works with missing trees.\n\nIn the upcoming commits, we will extend `--missing` to also support\nmissing commits. So let's rename the bit to\n`do_not_die_on_missing_objects`, which is object type agnostic and can\nbe used for both trees/commits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/reflog.c   |  2 +-\n builtin/rev-list.c |  2 +-\n list-objects.c     |  2 +-\n revision.h         | 17 +++++++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df63a5892e..9e369a5977 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tstruct rev_info revs;\n \n \t\trepo_init_revisions(the_repository, &revs, prefix);\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \t\trevs.ignore_missing = 1;\n \t\trevs.ignore_missing_links = 1;\n \t\tif (verbose)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..ea77489c38 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (arg_missing_action)\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex c25c72b32c..47296dff2f 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,\n \t\t    is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n-\t\tif (!revs->do_not_die_on_missing_tree)\n+\t\tif (!revs->do_not_die_on_missing_objects)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \ndiff --git a/revision.h b/revision.h\nindex 50091bbd13..c73c92ef40 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -212,18 +212,19 @@ struct rev_info {\n \n \t\t\t/*\n \t\t\t * Blobs are shown without regard for their existence.\n-\t\t\t * But not so for trees: unless exclude_promisor_objects\n+\t\t\t * But not so for trees/commits: unless exclude_promisor_objects\n \t\t\t * is set and the tree in question is a promisor object;\n \t\t\t * OR ignore_missing_links is set, the revision walker\n-\t\t\t * dies with a \"bad tree object HASH\" message when\n-\t\t\t * encountering a missing tree. For callers that can\n-\t\t\t * handle missing trees and want them to be filterable\n+\t\t\t * dies with a \"bad <type> object HASH\" message when\n+\t\t\t * encountering a missing object. For callers that can\n+\t\t\t * handle missing trees/commits and want them to be filterable\n \t\t\t * and showable, set this to true. The revision walker\n-\t\t\t * will filter and show such a missing tree as usual,\n-\t\t\t * but will not attempt to recurse into this tree\n-\t\t\t * object.\n+\t\t\t * will filter and show such a missing object as usual,\n+\t\t\t * but will not attempt to recurse into this tree/commit\n+\t\t\t * object. The revision walker will also set the MISSING\n+\t\t\t * flag for such objects.\n \t\t\t */\n-\t\t\tdo_not_die_on_missing_tree:1,\n+\t\t\tdo_not_die_on_missing_objects:1,\n \n \t\t\t/* for internal use only */\n \t\t\texclude_promisor_objects:1;\n-- \n2.42.0\n\n"},{"id":"483766","messageId":"20231024122631.158415-3-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231024122631.158415-1-karthik.188@gmail.com","subject":"[PATCH v4 2/3] rev-list: move `show_commit()` to the bottom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-24T12:26:30Z","receivedAt":"2023-10-24T12:26:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `show_commit()` function already depends on `finish_commit()`, and\nin the upcoming commit, we'll also add a dependency on\n`finish_object__ma()`. Since in C symbols must be declared before\nthey're used, let's move `show_commit()` below both `finish_commit()`\nand `finish_object__ma()`, so the code is cleaner as a whole without the\nneed for declarations.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ea77489c38..98542e8b3c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)\n \treturn size;\n }\n \n-static void finish_commit(struct commit *commit);\n+static inline void finish_object__ma(struct object *obj)\n+{\n+\t/*\n+\t * Whether or not we try to dynamically fetch missing objects\n+\t * from the server, we currently DO NOT have the object.  We\n+\t * can either print, allow (ignore), or conditionally allow\n+\t * (ignore) them.\n+\t */\n+\tswitch (arg_missing_action) {\n+\tcase MA_ERROR:\n+\t\tdie(\"missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tcase MA_ALLOW_ANY:\n+\t\treturn;\n+\n+\tcase MA_PRINT:\n+\t\toidset_insert(&missing_objects, &obj->oid);\n+\t\treturn;\n+\n+\tcase MA_ALLOW_PROMISOR:\n+\t\tif (is_promisor_object(&obj->oid))\n+\t\t\treturn;\n+\t\tdie(\"unexpected missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tdefault:\n+\t\tBUG(\"unhandled missing_action\");\n+\t\treturn;\n+\t}\n+}\n+\n+static void finish_commit(struct commit *commit)\n+{\n+\tfree_commit_list(commit->parents);\n+\tcommit->parents = NULL;\n+\tfree_commit_buffer(the_repository->parsed_objects,\n+\t\t\t   commit);\n+}\n+\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)\n \tfinish_commit(commit);\n }\n \n-static void finish_commit(struct commit *commit)\n-{\n-\tfree_commit_list(commit->parents);\n-\tcommit->parents = NULL;\n-\tfree_commit_buffer(the_repository->parsed_objects,\n-\t\t\t   commit);\n-}\n-\n-static inline void finish_object__ma(struct object *obj)\n-{\n-\t/*\n-\t * Whether or not we try to dynamically fetch missing objects\n-\t * from the server, we currently DO NOT have the object.  We\n-\t * can either print, allow (ignore), or conditionally allow\n-\t * (ignore) them.\n-\t */\n-\tswitch (arg_missing_action) {\n-\tcase MA_ERROR:\n-\t\tdie(\"missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tcase MA_ALLOW_ANY:\n-\t\treturn;\n-\n-\tcase MA_PRINT:\n-\t\toidset_insert(&missing_objects, &obj->oid);\n-\t\treturn;\n-\n-\tcase MA_ALLOW_PROMISOR:\n-\t\tif (is_promisor_object(&obj->oid))\n-\t\t\treturn;\n-\t\tdie(\"unexpected missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tdefault:\n-\t\tBUG(\"unhandled missing_action\");\n-\t\treturn;\n-\t}\n-}\n-\n static int finish_object(struct object *obj, const char *name UNUSED,\n \t\t\t void *cb_data)\n {\n-- \n2.42.0\n\n"},{"id":"483767","messageId":"20231024122631.158415-1-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231019121024.194317-1-karthik.188@gmail.com","subject":"[PATCH v4 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-24T12:26:28Z","receivedAt":"2023-10-24T12:26:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` option in git-rev-list(1) was introduced intitally\nto deal with missing blobs in the context of promissory notes.\nEventually the option was extended to also support tree objects in\n7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).\n\nThis patch series extends the `--missing` option to also add support for\ncommit objects. We do this by introducing a new flag `MISSING` which is\nadded whenever we encounter a missing commit during traversal. Then in\n`builtin/rev-list` we check for this flag and take the appropriate\naction based on the `--missing=*` option used.\n\nThis series is an alternate to the patch series I had posted earlier:\nhttps://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\nIn that patch, we introduced an option `--ignore-missing-links` which\nwas added to expose the `ignore_missing_links` bit to the user. The\nissue in that patch was that, the option `--ignore-missing-links` didn't\nplay well the pre-existing `--missing` option. This series avoids that\nroute and just extends the `--missing` option for commits to solve the\nsame problem.\n\nV3 of the series can be found here: https://lore.kernel.org/git/20231019121024.194317-1-karthik.188@gmail.com/T/#mf6a442e06f323a78a45af086ddd353998bab0052\n\nChanges from v3:\n- Instead of using flags on the object level as in the previous vesrions, we\nadd and use a missing_objects oidset in rev_info. This avoids flag bit-collision\nsince we were using an existing bit and doesn't require extending the flag size\nas done in v3. \n\nChangelog vs v3:\n\n1:  8c469cf479 = 1:  8c469cf479 revision: rename bit to `do_not_die_on_missing_objects`\n2:  76ce43d973 = 2:  76ce43d973 rev-list: move `show_commit()` to the bottom\n3:  4c640f9ab4 ! 3:  d892f0b82d rev-list: add commit object support in `--missing` option\n    @@ Commit message\n         a fatal error.\n     \n         Let's extend the functionality of `--missing` option to also support\n    -    commit objects. This is done by adding a new `MISSING` flag that the\n    -    revision walker sets whenever it encounters a missing tree/commit. The\n    -    revision walker will now continue the traversal and call `show_commit()`\n    -    even for missing commits. In rev-list we can then check for this flag\n    -    and call the existing code for parsing `--missing` objects.\n    +    commit objects. This is done by adding a `missing_objects` field to\n    +    `rev_info`. This field is an `oidset` to which we'll add the missing\n    +    commits as we encounter them. The revision walker will now continue the\n    +    traversal and call `show_commit()` even for missing commits. In rev-list\n    +    we can then check if the commit is a missing commit and call the\n    +    existing code for parsing `--missing` objects.\n     \n         A scenario where this option would be used is to find the boundary\n         objects between different object directories. Consider a repository with\n    @@ Commit message\n         alternate object directory allows us to find the boundary objects\n         between the main and alternate object directory.\n     \n    +    Helped-by: Patrick Steinhardt <ps@pks.im>\n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n     \n      ## builtin/rev-list.c ##\n    @@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)\n      \tdisplay_progress(progress, ++progress_counter);\n      \n     +\tif (revs->do_not_die_on_missing_objects &&\n    -+\t    commit->object.flags & MISSING) {\n    ++\t    oidset_contains(&revs->missing_objects, &commit->object.oid)) {\n     +\t\tfinish_object__ma(&commit->object);\n     +\t\treturn;\n     +\t}\n    @@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)\n     \n      ## list-objects.c ##\n     @@ list-objects.c: static void do_traverse(struct traversal_context *ctx)\n    - \t\t * an uninteresting boundary commit may not have its tree\n    - \t\t * parsed yet, but we are not going to show them anyway\n      \t\t */\n    --\t\tif (!ctx->revs->tree_objects)\n    -+\t\tif (!ctx->revs->tree_objects || commit->object.flags & MISSING)\n    + \t\tif (!ctx->revs->tree_objects)\n      \t\t\t; /* do not bother loading tree */\n    ++\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n    ++\t\t\t oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))\n    ++\t\t\t;\n      \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n      \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n    -\n    - ## object.h ##\n    -@@ object.h: void object_array_init(struct object_array *array);\n    - \n    - /*\n    -  * object flag allocation:\n    -- * revision.h:               0---------10         15             23------27\n    -+ * revision.h:               0---------10         15             22------28\n    -  * fetch-pack.c:             01    67\n    -  * negotiator/default.c:       2--5\n    -  * walker.c:                 0-2\n    -@@ object.h: void object_array_init(struct object_array *array);\n    -  * builtin/show-branch.c:    0-------------------------------------------26\n    -  * builtin/unpack-objects.c:                                 2021\n    -  */\n    --#define FLAG_BITS  28\n    -+#define FLAG_BITS  29\n    - \n    - #define TYPE_BITS 3\n    - \n    + \t\t\t\t\t\t\t\t commit);\n     \n      ## revision.c ##\n    +@@\n    + #include \"object-name.h\"\n    + #include \"object-file.h\"\n    + #include \"object-store-ll.h\"\n    ++#include \"oidset.h\"\n    + #include \"tag.h\"\n    + #include \"blob.h\"\n    + #include \"tree.h\"\n     @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,\n    - \tstruct commit_list *parent = commit->parents;\n    - \tunsigned pass_flags;\n      \n    --\tif (commit->object.flags & ADDED)\n    -+\tif (commit->object.flags & (ADDED | MISSING))\n    + \tif (commit->object.flags & ADDED)\n      \t\treturn 0;\n    ++\tif (revs->do_not_die_on_missing_objects &&\n    ++\t\toidset_contains(&revs->missing_objects, &commit->object.oid))\n    ++\t\treturn 0;\n      \tcommit->object.flags |= ADDED;\n      \n    + \tif (revs->include_check &&\n     @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,\n      \tfor (parent = commit->parents; parent; parent = parent->next) {\n      \t\tstruct commit *p = parent->item;\n    @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n     +\t\t\tif (!revs->do_not_die_on_missing_objects)\n     +\t\t\t\treturn -1;\n     +\t\t\telse\n    -+\t\t\t\tp->object.flags |= MISSING;\n    ++\t\t\t\toidset_insert(&revs->missing_objects, &p->object.oid);\n      \t\t}\n      \t\tif (revs->sources) {\n      \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n    +@@ revision.c: int prepare_revision_walk(struct rev_info *revs)\n    + \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n    + \t}\n    + \n    ++\tif (revs->do_not_die_on_missing_objects)\n    ++\t\toidset_init(&revs->missing_objects, 0);\n    ++\n    + \tif (!revs->reflog_info)\n    + \t\tprepare_to_use_bloom_filter(revs);\n    + \tif (!revs->unsorted_input)\n     \n      ## revision.h ##\n     @@\n    - #define ANCESTRY_PATH\t(1u<<27)\n    - #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)\n    + #include \"commit.h\"\n    + #include \"grep.h\"\n    + #include \"notes.h\"\n    ++#include \"oidset.h\"\n    + #include \"pretty.h\"\n    + #include \"diff.h\"\n    + #include \"commit-slab-decl.h\"\n    +@@ revision.h: struct rev_info {\n      \n    -+#define MISSING (1u<<28)\n    + \t/* Location where temporary objects for remerge-diff are written. */\n    + \tstruct tmp_objdir *remerge_objdir;\n     +\n    - #define DECORATE_SHORT_REFS\t1\n    - #define DECORATE_FULL_REFS\t2\n    ++\t/* Missing objects to be tracked without failing traversal. */\n    ++\tstruct oidset missing_objects;\n    + };\n      \n    + /**\n     \n      ## t/t6022-rev-list-missing.sh (new) ##\n     @@\n\n\nKarthik Nayak (3):\n  revision: rename bit to `do_not_die_on_missing_objects`\n  rev-list: move `show_commit()` to the bottom\n  rev-list: add commit object support in `--missing` option\n\n builtin/reflog.c            |  2 +-\n builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n list-objects.c              |  5 +-\n revision.c                  | 16 ++++++-\n revision.h                  | 21 +++++----\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n 6 files changed, 155 insertions(+), 56 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\n-- \n2.42.0\n\n"},{"id":"483768","messageId":"20231024122631.158415-4-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231024122631.158415-1-karthik.188@gmail.com","subject":"[PATCH v4 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-24T12:26:31Z","receivedAt":"2023-10-24T12:26:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` object option in rev-list currently works only with\nmissing blobs/trees. For missing commits the revision walker fails with\na fatal error.\n\nLet's extend the functionality of `--missing` option to also support\ncommit objects. This is done by adding a `missing_objects` field to\n`rev_info`. This field is an `oidset` to which we'll add the missing\ncommits as we encounter them. The revision walker will now continue the\ntraversal and call `show_commit()` even for missing commits. In rev-list\nwe can then check if the commit is a missing commit and call the\nexisting code for parsing `--missing` objects.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, using the `--missing=print` option while disabling the\nalternate object directory allows us to find the boundary objects\nbetween the main and alternate object directory.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c          |  6 +++\n list-objects.c              |  3 ++\n revision.c                  | 16 +++++++-\n revision.h                  |  4 ++\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 101 insertions(+), 2 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 98542e8b3c..37b52520b5 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n \n \tdisplay_progress(progress, ++progress_counter);\n \n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    oidset_contains(&revs->missing_objects, &commit->object.oid)) {\n+\t\tfinish_object__ma(&commit->object);\n+\t\treturn;\n+\t}\n+\n \tif (show_disk_usage)\n \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 47296dff2f..260089388c 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t */\n \t\tif (!ctx->revs->tree_objects)\n \t\t\t; /* do not bother loading tree */\n+\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n+\t\t\t oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))\n+\t\t\t;\n \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t\t\t commit);\ndiff --git a/revision.c b/revision.c\nindex 219dc76716..e60646c1a7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -6,6 +6,7 @@\n #include \"object-name.h\"\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n+#include \"oidset.h\"\n #include \"tag.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n@@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n+\tif (revs->do_not_die_on_missing_objects &&\n+\t\toidset_contains(&revs->missing_objects, &commit->object.oid))\n+\t\treturn 0;\n \tcommit->object.flags |= ADDED;\n \n \tif (revs->include_check &&\n@@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \t\tint gently = revs->ignore_missing_links ||\n-\t\t\t     revs->exclude_promisor_objects;\n+\t\t\t     revs->exclude_promisor_objects ||\n+\t\t\t     revs->do_not_die_on_missing_objects;\n \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&p->object.oid)) {\n@@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\t\tbreak;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\treturn -1;\n+\n+\t\t\tif (!revs->do_not_die_on_missing_objects)\n+\t\t\t\treturn -1;\n+\t\t\telse\n+\t\t\t\toidset_insert(&revs->missing_objects, &p->object.oid);\n \t\t}\n \t\tif (revs->sources) {\n \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n@@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n \t}\n \n+\tif (revs->do_not_die_on_missing_objects)\n+\t\toidset_init(&revs->missing_objects, 0);\n+\n \tif (!revs->reflog_info)\n \t\tprepare_to_use_bloom_filter(revs);\n \tif (!revs->unsorted_input)\ndiff --git a/revision.h b/revision.h\nindex c73c92ef40..f6bf422f0e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -4,6 +4,7 @@\n #include \"commit.h\"\n #include \"grep.h\"\n #include \"notes.h\"\n+#include \"oidset.h\"\n #include \"pretty.h\"\n #include \"diff.h\"\n #include \"commit-slab-decl.h\"\n@@ -373,6 +374,9 @@ struct rev_info {\n \n \t/* Location where temporary objects for remerge-diff are written. */\n \tstruct tmp_objdir *remerge_objdir;\n+\n+\t/* Missing objects to be tracked without failing traversal. */\n+\tstruct oidset missing_objects;\n };\n \n /**\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nnew file mode 100755\nindex 0000000000..40265a4f66\n--- /dev/null\n+++ b/t/t6022-rev-list-missing.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We setup the repository with two commits, this way HEAD is always\n+# available and we can hide commit 1.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit 1 &&\n+\ttest_commit 2 &&\n+\ttest_commit 3\n+'\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n+\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\ttest_must_fail git rev-list --missing=error --objects \\\n+\t\t\t--no-object-names HEAD\n+\t'\n+done\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor action in \"allow-any\" \"print\"\n+\tdo\n+\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n+\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t# get the expected oids.\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\tHEAD ^$obj >expect.raw &&\n+\n+\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n+\t\t\t# might be skipped, its blob must be accounted for.\n+\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n+\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n+\t\t\tfi &&\n+\n+\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n+\t\t\t\tHEAD >actual.raw &&\n+\n+\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t# oid to the expect list.\n+\t\t\tcase $action in\n+\t\t\tallow-any)\n+\t\t\t\t;;\n+\t\t\tprint)\n+\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\n+\t\t\tsort actual.raw >actual &&\n+\t\t\tsort expect.raw >expect &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n+\n+test_done\n-- \n2.42.0\n\n"},{"id":"483800","messageId":"xmqqbkcn52z4.fsf@gitster.g","threadId":"60331","inReplyTo":"20231024122631.158415-4-karthik.188@gmail.com","subject":"Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-24T17:45:19Z","receivedAt":"2023-10-24T17:45:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs,...\n>  \t\t\t\t\tbreak;\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\treturn -1;\n> +\n> +\t\t\tif (!revs->do_not_die_on_missing_objects)\n> +\t\t\t\treturn -1;\n> +\t\t\telse\n> +\t\t\t\toidset_insert(&revs->missing_objects, &p->object.oid);\n\nI would suspect that swapping if/else would make it easier to\nfollow.  Everybody else in the patch guards the use of the oidset\nwith \"were we told not to die on missing objects?\", i.e.,\n\n\tif (revs->do_not_die_on_missing_objects)\n\t\toidset_insert(&revs->missing_objects, &p->object.oid);\n\telse\n\t\treturn -1; /* corrupt repository */\n\n> @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)\n>  \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n>  \t}\n>  \n> +\tif (revs->do_not_die_on_missing_objects)\n> +\t\toidset_init(&revs->missing_objects, 0);\n\nI read the patch to make sure that .missing_objects oidset is used\nonly when .do_not_die_on_missing_objects is set and the oidset is\nuntouched unless it is initialized.  Well done.\n\nI know I floated \"perhaps oidset can replace the object bits,\nespecially because the number of objects that need marking is\nexpected to be small\", but I am curious what the performance\nimplication of this would be.  Is this something we can create\ncomparison easily?\n\nI noticed that nobody releases the resource held by this new oidset.\nShouldn't we do so in revision.c:release_revisions()?\n\nThanks.\n"},{"id":"483828","messageId":"xmqqfs1zh745.fsf@gitster.g","threadId":"60331","inReplyTo":"xmqqbkcn52z4.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-25T00:35:06Z","receivedAt":"2023-10-25T00:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I noticed that nobody releases the resource held by this new oidset.\n> Shouldn't we do so in revision.c:release_revisions()?\n\nIt seems that linux-leaks CI job noticed the same.\n\nhttps://github.com/git/git/actions/runs/6633599458/job/18021612518#step:5:2949\n\nI wonder if the following is sufficient?\n\n revision.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git c/revision.c w/revision.c\nindex 724a116401..7a67ff74dc 100644\n--- c/revision.c\n+++ w/revision.c\n@@ -3136,6 +3136,8 @@ void release_revisions(struct rev_info *revs)\n \tclear_decoration(&revs->merge_simplification, free);\n \tclear_decoration(&revs->treesame, free);\n \tline_log_free(revs);\n+\tif (revs->do_not_die_on_missing_objects)\n+\t\toidset_clear(&revs->missing_objects);\n }\n \n static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n\n"},{"id":"483831","messageId":"ZTi4Zd1by53q5gtM@tanuki","threadId":"60331","inReplyTo":"20231024122631.158415-4-karthik.188@gmail.com","subject":"Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T06:40:37Z","receivedAt":"2023-10-25T06:40:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 24, 2023 at 02:26:31PM +0200, Karthik Nayak wrote:\n> The `--missing` object option in rev-list currently works only with\n> missing blobs/trees. For missing commits the revision walker fails with\n> a fatal error.\n> \n> Let's extend the functionality of `--missing` option to also support\n> commit objects. This is done by adding a `missing_objects` field to\n> `rev_info`. This field is an `oidset` to which we'll add the missing\n> commits as we encounter them. The revision walker will now continue the\n> traversal and call `show_commit()` even for missing commits. In rev-list\n> we can then check if the commit is a missing commit and call the\n> existing code for parsing `--missing` objects.\n> \n> A scenario where this option would be used is to find the boundary\n> objects between different object directories. Consider a repository with\n> a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\n> object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\n> repository, using the `--missing=print` option while disabling the\n> alternate object directory allows us to find the boundary objects\n> between the main and alternate object directory.\n> \n> Helped-by: Patrick Steinhardt <ps@pks.im>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>  builtin/rev-list.c          |  6 +++\n>  list-objects.c              |  3 ++\n>  revision.c                  | 16 +++++++-\n>  revision.h                  |  4 ++\n>  t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n>  5 files changed, 101 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t6022-rev-list-missing.sh\n> \n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 98542e8b3c..37b52520b5 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n>  \n>  \tdisplay_progress(progress, ++progress_counter);\n>  \n> +\tif (revs->do_not_die_on_missing_objects &&\n> +\t    oidset_contains(&revs->missing_objects, &commit->object.oid)) {\n> +\t\tfinish_object__ma(&commit->object);\n> +\t\treturn;\n> +\t}\n> +\n>  \tif (show_disk_usage)\n>  \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n>  \n> diff --git a/list-objects.c b/list-objects.c\n> index 47296dff2f..260089388c 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)\n>  \t\t */\n>  \t\tif (!ctx->revs->tree_objects)\n>  \t\t\t; /* do not bother loading tree */\n> +\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n> +\t\t\t oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))\n> +\t\t\t;\n>  \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n>  \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n>  \t\t\t\t\t\t\t\t commit);\n> diff --git a/revision.c b/revision.c\n> index 219dc76716..e60646c1a7 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -6,6 +6,7 @@\n>  #include \"object-name.h\"\n>  #include \"object-file.h\"\n>  #include \"object-store-ll.h\"\n> +#include \"oidset.h\"\n>  #include \"tag.h\"\n>  #include \"blob.h\"\n>  #include \"tree.h\"\n> @@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n>  \n>  \tif (commit->object.flags & ADDED)\n>  \t\treturn 0;\n> +\tif (revs->do_not_die_on_missing_objects &&\n> +\t\toidset_contains(&revs->missing_objects, &commit->object.oid))\n\nNit: indentation is off here.\n\n> +\t\treturn 0;\n>  \tcommit->object.flags |= ADDED;\n>  \n>  \tif (revs->include_check &&\n> @@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n>  \tfor (parent = commit->parents; parent; parent = parent->next) {\n>  \t\tstruct commit *p = parent->item;\n>  \t\tint gently = revs->ignore_missing_links ||\n> -\t\t\t     revs->exclude_promisor_objects;\n> +\t\t\t     revs->exclude_promisor_objects ||\n> +\t\t\t     revs->do_not_die_on_missing_objects;\n>  \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n>  \t\t\tif (revs->exclude_promisor_objects &&\n>  \t\t\t    is_promisor_object(&p->object.oid)) {\n> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n>  \t\t\t\t\tbreak;\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\treturn -1;\n> +\n> +\t\t\tif (!revs->do_not_die_on_missing_objects)\n> +\t\t\t\treturn -1;\n> +\t\t\telse\n> +\t\t\t\toidset_insert(&revs->missing_objects, &p->object.oid);\n>  \t\t}\n>  \t\tif (revs->sources) {\n>  \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n> @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)\n>  \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n>  \t}\n>  \n> +\tif (revs->do_not_die_on_missing_objects)\n> +\t\toidset_init(&revs->missing_objects, 0);\n> +\n\nWhile we're initializing the new oidset, we never clear it. We should\nprobably call `oidset_clear()` in `release_revisions()`. And if we\nunconditionally initialized the oidset here then we can also call\n`oiadset_clear()` unconditionally there, which should be preferable\ngiven that `oidset_init()` does not allocate memory when no initial size\nwas given.\n\n>  \tif (!revs->reflog_info)\n>  \t\tprepare_to_use_bloom_filter(revs);\n>  \tif (!revs->unsorted_input)\n> diff --git a/revision.h b/revision.h\n> index c73c92ef40..f6bf422f0e 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -4,6 +4,7 @@\n>  #include \"commit.h\"\n>  #include \"grep.h\"\n>  #include \"notes.h\"\n> +#include \"oidset.h\"\n>  #include \"pretty.h\"\n>  #include \"diff.h\"\n>  #include \"commit-slab-decl.h\"\n> @@ -373,6 +374,9 @@ struct rev_info {\n>  \n>  \t/* Location where temporary objects for remerge-diff are written. */\n>  \tstruct tmp_objdir *remerge_objdir;\n> +\n> +\t/* Missing objects to be tracked without failing traversal. */\n> +\tstruct oidset missing_objects;\n\nAs far as I can see we only use this set to track missing commits, but\nnone of the other objects. The name thus feels a bit misleading to me,\nas a reader might rightfully assume that it contains _all_ missing\nobjects after the revwalk. Should we rename it to `missing_commits` to\nclarify?\n\nPatrick\n\n>  };\n>  \n>  /**\n> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> new file mode 100755\n> index 0000000000..40265a4f66\n> --- /dev/null\n> +++ b/t/t6022-rev-list-missing.sh\n> @@ -0,0 +1,74 @@\n> +#!/bin/sh\n> +\n> +test_description='handling of missing objects in rev-list'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +# We setup the repository with two commits, this way HEAD is always\n> +# available and we can hide commit 1.\n> +test_expect_success 'create repository and alternate directory' '\n> +\ttest_commit 1 &&\n> +\ttest_commit 2 &&\n> +\ttest_commit 3\n> +'\n> +\n> +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> +do\n> +\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n> +\t\toid=\"$(git rev-parse $obj)\" &&\n> +\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\ttest_when_finished \"mv $path.hidden $path\" &&\n> +\n> +\t\ttest_must_fail git rev-list --missing=error --objects \\\n> +\t\t\t--no-object-names HEAD\n> +\t'\n> +done\n> +\n> +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> +do\n> +\tfor action in \"allow-any\" \"print\"\n> +\tdo\n> +\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n> +\t\t\toid=\"$(git rev-parse $obj)\" &&\n> +\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\t\t# Before the object is made missing, we use rev-list to\n> +\t\t\t# get the expected oids.\n> +\t\t\tgit rev-list --objects --no-object-names \\\n> +\t\t\t\tHEAD ^$obj >expect.raw &&\n> +\n> +\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n> +\t\t\t# might be skipped, its blob must be accounted for.\n> +\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n> +\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n> +\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n> +\t\t\tfi &&\n> +\n> +\t\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n> +\n> +\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n> +\t\t\t\tHEAD >actual.raw &&\n> +\n> +\t\t\t# When the action is to print, we should also add the missing\n> +\t\t\t# oid to the expect list.\n> +\t\t\tcase $action in\n> +\t\t\tallow-any)\n> +\t\t\t\t;;\n> +\t\t\tprint)\n> +\t\t\t\tgrep ?$oid actual.raw &&\n> +\t\t\t\techo ?$oid >>expect.raw\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> +\n> +\t\t\tsort actual.raw >actual &&\n> +\t\t\tsort expect.raw >expect &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t'\n> +\tdone\n> +done\n> +\n> +test_done\n> -- \n> 2.42.0\n> \n"},{"id":"483848","messageId":"CAOLa=ZQ6K8g-r+Oq8Jq53QbB4qhKr6HLgpfT5OjtgzBW7DMzbg@mail.gmail.com","threadId":"60331","inReplyTo":"xmqqbkcn52z4.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-25T09:34:58Z","receivedAt":"2023-10-25T09:35:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Oct 24, 2023 at 7:45 PM Junio C Hamano <gitster@pobox.com> wrote:\n> I would suspect that swapping if/else would make it easier to\n> follow.  Everybody else in the patch guards the use of the oidset\n> with \"were we told not to die on missing objects?\", i.e.,\n>\n>         if (revs->do_not_die_on_missing_objects)\n>                 oidset_insert(&revs->missing_objects, &p->object.oid);\n>         else\n>                 return -1; /* corrupt repository */\n>\n\nMakes sense. Will change.\n\n> > @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)\n> >                                      FOR_EACH_OBJECT_PROMISOR_ONLY);\n> >       }\n> >\n> > +     if (revs->do_not_die_on_missing_objects)\n> > +             oidset_init(&revs->missing_objects, 0);\n>\n> I read the patch to make sure that .missing_objects oidset is used\n> only when .do_not_die_on_missing_objects is set and the oidset is\n> untouched unless it is initialized.  Well done.\n>\n> I know I floated \"perhaps oidset can replace the object bits,\n> especially because the number of objects that need marking is\n> expected to be small\", but I am curious what the performance\n> implication of this would be.  Is this something we can create\n> comparison easily?\n\nI did a simple comparision here, I randomly deleted commits which had\nchild commits with greater than 2 parents.\n\n$ git rev-list --missing=print HEAD | grep \"?\" | wc -l\n828\n\nUsing the flag bit:\n\n$ hyperfine -w 3 \"~/git/bin-wrappers/git rev-list --missing=allow-any HEAD\"\nBenchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD\n  Time (mean ± σ):     860.5 ms ±  15.2 ms    [User: 375.3 ms, System: 467.5 ms]\n  Range (min … max):   835.2 ms … 881.0 ms    10 runs\n\nUsing the oidset:\n\n$ hyperfine -w 3 \"~/git/bin-wrappers/git rev-list --missing=allow-any HEAD\"\nBenchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD\n  Time (mean ± σ):     901.3 ms ±   9.6 ms    [User: 394.3 ms, System: 488.3 ms]\n  Range (min … max):   885.0 ms … 913.2 ms    10 runs\n\nIts definitely slower, but not by much.\n\n>\n> > I noticed that nobody releases the resource held by this new oidset.\n> > Shouldn't we do so in revision.c:release_revisions()?\n>\n> It seems that linux-leaks CI job noticed the same.\n>\n> https://github.com/git/git/actions/runs/6633599458/job/18021612518#step:5:2949\n>\n> I wonder if the following is sufficient?\n>\n>  revision.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git c/revision.c w/revision.c\n> index 724a116401..7a67ff74dc 100644\n> --- c/revision.c\n> +++ w/revision.c\n> @@ -3136,6 +3136,8 @@ void release_revisions(struct rev_info *revs)\n>         clear_decoration(&revs->merge_simplification, free);\n>         clear_decoration(&revs->treesame, free);\n>         line_log_free(revs);\n> +       if (revs->do_not_die_on_missing_objects)\n> +               oidset_clear(&revs->missing_objects);\n>  }\n>\n>  static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n>\n\nYup, this seems right and was missed, will add.\n\n\nOn Wed, Oct 25, 2023 at 8:40 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> >\n> >       if (commit->object.flags & ADDED)\n> >               return 0;\n> > +     if (revs->do_not_die_on_missing_objects &&\n> > +             oidset_contains(&revs->missing_objects, &commit->object.oid))\n>\n> Nit: indentation is off here.\n>\n\nThanks, will fix.\n\n> > +\n> > +     /* Missing objects to be tracked without failing traversal. */\n> > +     struct oidset missing_objects;\n>\n> As far as I can see we only use this set to track missing commits, but\n> none of the other objects. The name thus feels a bit misleading to me,\n> as a reader might rightfully assume that it contains _all_ missing\n> objects after the revwalk. Should we rename it to `missing_commits` to\n> clarify?\n>\n\nFair enough, I was thinking of being future compatible. But probably best to\nbe specific now and refactor as needed in the future. Will change.\n\nThanks both for the review!\n"},{"id":"483909","messageId":"20231026101109.43110-1-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231024122631.158415-1-karthik.188@gmail.com","subject":"[PATCH v5 0/3] rev-list: add support for commits in `--missing`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-26T10:11:06Z","receivedAt":"2023-10-26T10:11:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` option in git-rev-list(1) was introduced intitally\nto deal with missing blobs in the context of promissory notes.\nEventually the option was extended to also support tree objects in\n7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).\n\nThis patch series extends the `--missing` option to also add support for\ncommit objects. We do this by introducing a new flag `MISSING` which is\nadded whenever we encounter a missing commit during traversal. Then in\n`builtin/rev-list` we check for this flag and take the appropriate\naction based on the `--missing=*` option used.\n\nThis series is an alternate to the patch series I had posted earlier:\nhttps://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.\nIn that patch, we introduced an option `--ignore-missing-links` which\nwas added to expose the `ignore_missing_links` bit to the user. The\nissue in that patch was that, the option `--ignore-missing-links` didn't\nplay well the pre-existing `--missing` option. This series avoids that\nroute and just extends the `--missing` option for commits to solve the\nsame problem.\n\nV4 of the series can be found here: https://lore.kernel.org/git/20231024122631.158415-1-karthik.188@gmail.com/T/#ma7f07bc637e20e2a9558b23e8081957af61f63ce\n\nChanges from v4:\n- Rename `missing_objects` to `missing_commits` since we only deal\nwith commits.\n- Fix incorrect indentation\n- clear oidset after traversal in `release_revisions()` \n\nRange diff:\n\n1:  8c469cf479 = 1:  8c469cf479 revision: rename bit to `do_not_die_on_missing_objects`\n2:  76ce43d973 = 2:  76ce43d973 rev-list: move `show_commit()` to the bottom\n3:  d892f0b82d ! 3:  6f0c74f888 rev-list: add commit object support in `--missing` option\n    @@ Commit message\n         between the main and alternate object directory.\n     \n         Helped-by: Patrick Steinhardt <ps@pks.im>\n    +    Helped-by: Junio C Hamano <gitster@pobox.com>\n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n     \n      ## builtin/rev-list.c ##\n    @@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)\n      \tdisplay_progress(progress, ++progress_counter);\n      \n     +\tif (revs->do_not_die_on_missing_objects &&\n    -+\t    oidset_contains(&revs->missing_objects, &commit->object.oid)) {\n    ++\t    oidset_contains(&revs->missing_commits, &commit->object.oid)) {\n     +\t\tfinish_object__ma(&commit->object);\n     +\t\treturn;\n     +\t}\n    @@ list-objects.c: static void do_traverse(struct traversal_context *ctx)\n      \t\tif (!ctx->revs->tree_objects)\n      \t\t\t; /* do not bother loading tree */\n     +\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n    -+\t\t\t oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))\n    ++\t\t\t oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))\n     +\t\t\t;\n      \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n      \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n    @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n      \tif (commit->object.flags & ADDED)\n      \t\treturn 0;\n     +\tif (revs->do_not_die_on_missing_objects &&\n    -+\t\toidset_contains(&revs->missing_objects, &commit->object.oid))\n    ++\t    oidset_contains(&revs->missing_commits, &commit->object.oid))\n     +\t\treturn 0;\n      \tcommit->object.flags |= ADDED;\n      \n    @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n      \t\t\t}\n     -\t\t\treturn -1;\n     +\n    -+\t\t\tif (!revs->do_not_die_on_missing_objects)\n    -+\t\t\t\treturn -1;\n    ++\t\t\tif (revs->do_not_die_on_missing_objects)\n    ++\t\t\t\toidset_insert(&revs->missing_commits, &p->object.oid);\n     +\t\t\telse\n    -+\t\t\t\toidset_insert(&revs->missing_objects, &p->object.oid);\n    ++\t\t\t\treturn -1; /* corrupt repository */\n      \t\t}\n      \t\tif (revs->sources) {\n      \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n    +@@ revision.c: void release_revisions(struct rev_info *revs)\n    + \tclear_decoration(&revs->merge_simplification, free);\n    + \tclear_decoration(&revs->treesame, free);\n    + \tline_log_free(revs);\n    ++\toidset_clear(&revs->missing_commits);\n    + }\n    + \n    + static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n     @@ revision.c: int prepare_revision_walk(struct rev_info *revs)\n      \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n      \t}\n      \n     +\tif (revs->do_not_die_on_missing_objects)\n    -+\t\toidset_init(&revs->missing_objects, 0);\n    ++\t\toidset_init(&revs->missing_commits, 0);\n     +\n      \tif (!revs->reflog_info)\n      \t\tprepare_to_use_bloom_filter(revs);\n    @@ revision.h: struct rev_info {\n      \t/* Location where temporary objects for remerge-diff are written. */\n      \tstruct tmp_objdir *remerge_objdir;\n     +\n    -+\t/* Missing objects to be tracked without failing traversal. */\n    -+\tstruct oidset missing_objects;\n    ++\t/* Missing commits to be tracked without failing traversal. */\n    ++\tstruct oidset missing_commits;\n      };\n      \n      /**\n\n\nKarthik Nayak (3):\n  revision: rename bit to `do_not_die_on_missing_objects`\n  rev-list: move `show_commit()` to the bottom\n  rev-list: add commit object support in `--missing` option\n\n builtin/reflog.c            |  2 +-\n builtin/rev-list.c          | 93 +++++++++++++++++++------------------\n list-objects.c              |  5 +-\n revision.c                  | 17 ++++++-\n revision.h                  | 21 +++++----\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++\n 6 files changed, 156 insertions(+), 56 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\n-- \n2.42.0\n\n"},{"id":"483910","messageId":"20231026101109.43110-2-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231026101109.43110-1-karthik.188@gmail.com","subject":"[PATCH v5 1/3] revision: rename bit to `do_not_die_on_missing_objects`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-26T10:11:07Z","receivedAt":"2023-10-26T10:11:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the\nrevision walker does not die when encountering a missing tree. This is\ncurrently exclusively set within `builtin/rev-list.c` to ensure the\n`--missing` option works with missing trees.\n\nIn the upcoming commits, we will extend `--missing` to also support\nmissing commits. So let's rename the bit to\n`do_not_die_on_missing_objects`, which is object type agnostic and can\nbe used for both trees/commits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/reflog.c   |  2 +-\n builtin/rev-list.c |  2 +-\n list-objects.c     |  2 +-\n revision.h         | 17 +++++++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex df63a5892e..9e369a5977 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tstruct rev_info revs;\n \n \t\trepo_init_revisions(the_repository, &revs, prefix);\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \t\trevs.ignore_missing = 1;\n \t\trevs.ignore_missing_links = 1;\n \t\tif (verbose)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..ea77489c38 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (arg_missing_action)\n-\t\trevs.do_not_die_on_missing_tree = 1;\n+\t\trevs.do_not_die_on_missing_objects = 1;\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex c25c72b32c..47296dff2f 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,\n \t\t    is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n-\t\tif (!revs->do_not_die_on_missing_tree)\n+\t\tif (!revs->do_not_die_on_missing_objects)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \ndiff --git a/revision.h b/revision.h\nindex 50091bbd13..c73c92ef40 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -212,18 +212,19 @@ struct rev_info {\n \n \t\t\t/*\n \t\t\t * Blobs are shown without regard for their existence.\n-\t\t\t * But not so for trees: unless exclude_promisor_objects\n+\t\t\t * But not so for trees/commits: unless exclude_promisor_objects\n \t\t\t * is set and the tree in question is a promisor object;\n \t\t\t * OR ignore_missing_links is set, the revision walker\n-\t\t\t * dies with a \"bad tree object HASH\" message when\n-\t\t\t * encountering a missing tree. For callers that can\n-\t\t\t * handle missing trees and want them to be filterable\n+\t\t\t * dies with a \"bad <type> object HASH\" message when\n+\t\t\t * encountering a missing object. For callers that can\n+\t\t\t * handle missing trees/commits and want them to be filterable\n \t\t\t * and showable, set this to true. The revision walker\n-\t\t\t * will filter and show such a missing tree as usual,\n-\t\t\t * but will not attempt to recurse into this tree\n-\t\t\t * object.\n+\t\t\t * will filter and show such a missing object as usual,\n+\t\t\t * but will not attempt to recurse into this tree/commit\n+\t\t\t * object. The revision walker will also set the MISSING\n+\t\t\t * flag for such objects.\n \t\t\t */\n-\t\t\tdo_not_die_on_missing_tree:1,\n+\t\t\tdo_not_die_on_missing_objects:1,\n \n \t\t\t/* for internal use only */\n \t\t\texclude_promisor_objects:1;\n-- \n2.42.0\n\n"},{"id":"483911","messageId":"20231026101109.43110-3-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231026101109.43110-1-karthik.188@gmail.com","subject":"[PATCH v5 2/3] rev-list: move `show_commit()` to the bottom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-26T10:11:08Z","receivedAt":"2023-10-26T10:11:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `show_commit()` function already depends on `finish_commit()`, and\nin the upcoming commit, we'll also add a dependency on\n`finish_object__ma()`. Since in C symbols must be declared before\nthey're used, let's move `show_commit()` below both `finish_commit()`\nand `finish_object__ma()`, so the code is cleaner as a whole without the\nneed for declarations.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ea77489c38..98542e8b3c 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)\n \treturn size;\n }\n \n-static void finish_commit(struct commit *commit);\n+static inline void finish_object__ma(struct object *obj)\n+{\n+\t/*\n+\t * Whether or not we try to dynamically fetch missing objects\n+\t * from the server, we currently DO NOT have the object.  We\n+\t * can either print, allow (ignore), or conditionally allow\n+\t * (ignore) them.\n+\t */\n+\tswitch (arg_missing_action) {\n+\tcase MA_ERROR:\n+\t\tdie(\"missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tcase MA_ALLOW_ANY:\n+\t\treturn;\n+\n+\tcase MA_PRINT:\n+\t\toidset_insert(&missing_objects, &obj->oid);\n+\t\treturn;\n+\n+\tcase MA_ALLOW_PROMISOR:\n+\t\tif (is_promisor_object(&obj->oid))\n+\t\t\treturn;\n+\t\tdie(\"unexpected missing %s object '%s'\",\n+\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n+\t\treturn;\n+\n+\tdefault:\n+\t\tBUG(\"unhandled missing_action\");\n+\t\treturn;\n+\t}\n+}\n+\n+static void finish_commit(struct commit *commit)\n+{\n+\tfree_commit_list(commit->parents);\n+\tcommit->parents = NULL;\n+\tfree_commit_buffer(the_repository->parsed_objects,\n+\t\t\t   commit);\n+}\n+\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)\n \tfinish_commit(commit);\n }\n \n-static void finish_commit(struct commit *commit)\n-{\n-\tfree_commit_list(commit->parents);\n-\tcommit->parents = NULL;\n-\tfree_commit_buffer(the_repository->parsed_objects,\n-\t\t\t   commit);\n-}\n-\n-static inline void finish_object__ma(struct object *obj)\n-{\n-\t/*\n-\t * Whether or not we try to dynamically fetch missing objects\n-\t * from the server, we currently DO NOT have the object.  We\n-\t * can either print, allow (ignore), or conditionally allow\n-\t * (ignore) them.\n-\t */\n-\tswitch (arg_missing_action) {\n-\tcase MA_ERROR:\n-\t\tdie(\"missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tcase MA_ALLOW_ANY:\n-\t\treturn;\n-\n-\tcase MA_PRINT:\n-\t\toidset_insert(&missing_objects, &obj->oid);\n-\t\treturn;\n-\n-\tcase MA_ALLOW_PROMISOR:\n-\t\tif (is_promisor_object(&obj->oid))\n-\t\t\treturn;\n-\t\tdie(\"unexpected missing %s object '%s'\",\n-\t\t    type_name(obj->type), oid_to_hex(&obj->oid));\n-\t\treturn;\n-\n-\tdefault:\n-\t\tBUG(\"unhandled missing_action\");\n-\t\treturn;\n-\t}\n-}\n-\n static int finish_object(struct object *obj, const char *name UNUSED,\n \t\t\t void *cb_data)\n {\n-- \n2.42.0\n\n"},{"id":"483912","messageId":"20231026101109.43110-4-karthik.188@gmail.com","threadId":"60331","inReplyTo":"20231026101109.43110-1-karthik.188@gmail.com","subject":"[PATCH v5 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-26T10:11:09Z","receivedAt":"2023-10-26T10:11:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` object option in rev-list currently works only with\nmissing blobs/trees. For missing commits the revision walker fails with\na fatal error.\n\nLet's extend the functionality of `--missing` option to also support\ncommit objects. This is done by adding a `missing_objects` field to\n`rev_info`. This field is an `oidset` to which we'll add the missing\ncommits as we encounter them. The revision walker will now continue the\ntraversal and call `show_commit()` even for missing commits. In rev-list\nwe can then check if the commit is a missing commit and call the\nexisting code for parsing `--missing` objects.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, using the `--missing=print` option while disabling the\nalternate object directory allows us to find the boundary objects\nbetween the main and alternate object directory.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c          |  6 +++\n list-objects.c              |  3 ++\n revision.c                  | 17 ++++++++-\n revision.h                  |  4 ++\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 102 insertions(+), 2 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 98542e8b3c..181353dcf5 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n \n \tdisplay_progress(progress, ++progress_counter);\n \n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    oidset_contains(&revs->missing_commits, &commit->object.oid)) {\n+\t\tfinish_object__ma(&commit->object);\n+\t\treturn;\n+\t}\n+\n \tif (show_disk_usage)\n \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 47296dff2f..f4e1104b56 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t */\n \t\tif (!ctx->revs->tree_objects)\n \t\t\t; /* do not bother loading tree */\n+\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n+\t\t\t oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))\n+\t\t\t;\n \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t\t\t commit);\ndiff --git a/revision.c b/revision.c\nindex 219dc76716..738bacad08 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -6,6 +6,7 @@\n #include \"object-name.h\"\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n+#include \"oidset.h\"\n #include \"tag.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n@@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    oidset_contains(&revs->missing_commits, &commit->object.oid))\n+\t\treturn 0;\n \tcommit->object.flags |= ADDED;\n \n \tif (revs->include_check &&\n@@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \t\tint gently = revs->ignore_missing_links ||\n-\t\t\t     revs->exclude_promisor_objects;\n+\t\t\t     revs->exclude_promisor_objects ||\n+\t\t\t     revs->do_not_die_on_missing_objects;\n \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&p->object.oid)) {\n@@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\t\tbreak;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\treturn -1;\n+\n+\t\t\tif (revs->do_not_die_on_missing_objects)\n+\t\t\t\toidset_insert(&revs->missing_commits, &p->object.oid);\n+\t\t\telse\n+\t\t\t\treturn -1; /* corrupt repository */\n \t\t}\n \t\tif (revs->sources) {\n \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n@@ -3109,6 +3118,7 @@ void release_revisions(struct rev_info *revs)\n \tclear_decoration(&revs->merge_simplification, free);\n \tclear_decoration(&revs->treesame, free);\n \tline_log_free(revs);\n+\toidset_clear(&revs->missing_commits);\n }\n \n static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n@@ -3800,6 +3810,9 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n \t}\n \n+\tif (revs->do_not_die_on_missing_objects)\n+\t\toidset_init(&revs->missing_commits, 0);\n+\n \tif (!revs->reflog_info)\n \t\tprepare_to_use_bloom_filter(revs);\n \tif (!revs->unsorted_input)\ndiff --git a/revision.h b/revision.h\nindex c73c92ef40..94c43138bc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -4,6 +4,7 @@\n #include \"commit.h\"\n #include \"grep.h\"\n #include \"notes.h\"\n+#include \"oidset.h\"\n #include \"pretty.h\"\n #include \"diff.h\"\n #include \"commit-slab-decl.h\"\n@@ -373,6 +374,9 @@ struct rev_info {\n \n \t/* Location where temporary objects for remerge-diff are written. */\n \tstruct tmp_objdir *remerge_objdir;\n+\n+\t/* Missing commits to be tracked without failing traversal. */\n+\tstruct oidset missing_commits;\n };\n \n /**\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nnew file mode 100755\nindex 0000000000..40265a4f66\n--- /dev/null\n+++ b/t/t6022-rev-list-missing.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We setup the repository with two commits, this way HEAD is always\n+# available and we can hide commit 1.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit 1 &&\n+\ttest_commit 2 &&\n+\ttest_commit 3\n+'\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n+\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\ttest_must_fail git rev-list --missing=error --objects \\\n+\t\t\t--no-object-names HEAD\n+\t'\n+done\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor action in \"allow-any\" \"print\"\n+\tdo\n+\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n+\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t# get the expected oids.\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\tHEAD ^$obj >expect.raw &&\n+\n+\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n+\t\t\t# might be skipped, its blob must be accounted for.\n+\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n+\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n+\t\t\tfi &&\n+\n+\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n+\t\t\t\tHEAD >actual.raw &&\n+\n+\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t# oid to the expect list.\n+\t\t\tcase $action in\n+\t\t\tallow-any)\n+\t\t\t\t;;\n+\t\t\tprint)\n+\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\n+\t\t\tsort actual.raw >actual &&\n+\t\t\tsort expect.raw >expect &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n+\n+test_done\n-- \n2.42.0\n\n"},{"id":"483916","messageId":"xmqqwmv9r24f.fsf@gitster.g","threadId":"60331","inReplyTo":"ZTi4Zd1by53q5gtM@tanuki","subject":"Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-26T12:37:20Z","receivedAt":"2023-10-26T12:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +\tif (revs->do_not_die_on_missing_objects)\n>> +\t\toidset_init(&revs->missing_objects, 0);\n>> +\n>\n> While we're initializing the new oidset, we never clear it. We should\n> probably call `oidset_clear()` in `release_revisions()`. And if we\n> unconditionally initialized the oidset here then we can also call\n> `oiadset_clear()` unconditionally there, which should be preferable\n> given that `oidset_init()` does not allocate memory when no initial size\n> was given.\n\nYup, I used the conditional one to match the above, but initializing\nunused oidset is cheap and frees us from having to worry about\nmistakes.  I like your idea much better.\n\n>> +\n>> +\t/* Missing objects to be tracked without failing traversal. */\n>> +\tstruct oidset missing_objects;\n>\n> As far as I can see we only use this set to track missing commits, but\n> none of the other objects. The name thus feels a bit misleading to me,\n> as a reader might rightfully assume that it contains _all_ missing\n> objects after the revwalk. Should we rename it to `missing_commits` to\n> clarify?\n\nAgain, very good suggestion.\n\nThanks.\n"},{"id":"483950","messageId":"ZTtXzg4NGJZzAqfS@tanuki","threadId":"60331","inReplyTo":"20231026101109.43110-4-karthik.188@gmail.com","subject":"Re: [PATCH v5 3/3] rev-list: add commit object support in `--missing` option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-27T06:25:30Z","receivedAt":"2023-10-27T06:25:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 26, 2023 at 12:11:09PM +0200, Karthik Nayak wrote:\n> The `--missing` object option in rev-list currently works only with\n> missing blobs/trees. For missing commits the revision walker fails with\n> a fatal error.\n> \n> Let's extend the functionality of `--missing` option to also support\n> commit objects. This is done by adding a `missing_objects` field to\n> `rev_info`. This field is an `oidset` to which we'll add the missing\n> commits as we encounter them. The revision walker will now continue the\n> traversal and call `show_commit()` even for missing commits. In rev-list\n> we can then check if the commit is a missing commit and call the\n> existing code for parsing `--missing` objects.\n> \n> A scenario where this option would be used is to find the boundary\n> objects between different object directories. Consider a repository with\n> a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\n> object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\n> repository, using the `--missing=print` option while disabling the\n> alternate object directory allows us to find the boundary objects\n> between the main and alternate object directory.\n> \n> Helped-by: Patrick Steinhardt <ps@pks.im>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>  builtin/rev-list.c          |  6 +++\n>  list-objects.c              |  3 ++\n>  revision.c                  | 17 ++++++++-\n>  revision.h                  |  4 ++\n>  t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n>  5 files changed, 102 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t6022-rev-list-missing.sh\n> \n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 98542e8b3c..181353dcf5 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n>  \n>  \tdisplay_progress(progress, ++progress_counter);\n>  \n> +\tif (revs->do_not_die_on_missing_objects &&\n> +\t    oidset_contains(&revs->missing_commits, &commit->object.oid)) {\n> +\t\tfinish_object__ma(&commit->object);\n> +\t\treturn;\n> +\t}\n> +\n>  \tif (show_disk_usage)\n>  \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n>  \n> diff --git a/list-objects.c b/list-objects.c\n> index 47296dff2f..f4e1104b56 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)\n>  \t\t */\n>  \t\tif (!ctx->revs->tree_objects)\n>  \t\t\t; /* do not bother loading tree */\n> +\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n> +\t\t\t oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))\n> +\t\t\t;\n>  \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n>  \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n>  \t\t\t\t\t\t\t\t commit);\n> diff --git a/revision.c b/revision.c\n> index 219dc76716..738bacad08 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -6,6 +6,7 @@\n>  #include \"object-name.h\"\n>  #include \"object-file.h\"\n>  #include \"object-store-ll.h\"\n> +#include \"oidset.h\"\n>  #include \"tag.h\"\n>  #include \"blob.h\"\n>  #include \"tree.h\"\n> @@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n>  \n>  \tif (commit->object.flags & ADDED)\n>  \t\treturn 0;\n> +\tif (revs->do_not_die_on_missing_objects &&\n> +\t    oidset_contains(&revs->missing_commits, &commit->object.oid))\n> +\t\treturn 0;\n>  \tcommit->object.flags |= ADDED;\n>  \n>  \tif (revs->include_check &&\n> @@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n>  \tfor (parent = commit->parents; parent; parent = parent->next) {\n>  \t\tstruct commit *p = parent->item;\n>  \t\tint gently = revs->ignore_missing_links ||\n> -\t\t\t     revs->exclude_promisor_objects;\n> +\t\t\t     revs->exclude_promisor_objects ||\n> +\t\t\t     revs->do_not_die_on_missing_objects;\n>  \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n>  \t\t\tif (revs->exclude_promisor_objects &&\n>  \t\t\t    is_promisor_object(&p->object.oid)) {\n> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n>  \t\t\t\t\tbreak;\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\treturn -1;\n> +\n> +\t\t\tif (revs->do_not_die_on_missing_objects)\n> +\t\t\t\toidset_insert(&revs->missing_commits, &p->object.oid);\n> +\t\t\telse\n> +\t\t\t\treturn -1; /* corrupt repository */\n>  \t\t}\n>  \t\tif (revs->sources) {\n>  \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n> @@ -3109,6 +3118,7 @@ void release_revisions(struct rev_info *revs)\n>  \tclear_decoration(&revs->merge_simplification, free);\n>  \tclear_decoration(&revs->treesame, free);\n>  \tline_log_free(revs);\n> +\toidset_clear(&revs->missing_commits);\n>  }\n>  \n>  static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n> @@ -3800,6 +3810,9 @@ int prepare_revision_walk(struct rev_info *revs)\n>  \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n>  \t}\n>  \n> +\tif (revs->do_not_die_on_missing_objects)\n> +\t\toidset_init(&revs->missing_commits, 0);\n> +\n\nWe unconditionally clear the oidset now, so shouldn't we also\nunconditionally initialize it?\n\nPatrick\n\n>  \tif (!revs->reflog_info)\n>  \t\tprepare_to_use_bloom_filter(revs);\n>  \tif (!revs->unsorted_input)\n> diff --git a/revision.h b/revision.h\n> index c73c92ef40..94c43138bc 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -4,6 +4,7 @@\n>  #include \"commit.h\"\n>  #include \"grep.h\"\n>  #include \"notes.h\"\n> +#include \"oidset.h\"\n>  #include \"pretty.h\"\n>  #include \"diff.h\"\n>  #include \"commit-slab-decl.h\"\n> @@ -373,6 +374,9 @@ struct rev_info {\n>  \n>  \t/* Location where temporary objects for remerge-diff are written. */\n>  \tstruct tmp_objdir *remerge_objdir;\n> +\n> +\t/* Missing commits to be tracked without failing traversal. */\n> +\tstruct oidset missing_commits;\n>  };\n>  \n>  /**\n> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> new file mode 100755\n> index 0000000000..40265a4f66\n> --- /dev/null\n> +++ b/t/t6022-rev-list-missing.sh\n> @@ -0,0 +1,74 @@\n> +#!/bin/sh\n> +\n> +test_description='handling of missing objects in rev-list'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +# We setup the repository with two commits, this way HEAD is always\n> +# available and we can hide commit 1.\n> +test_expect_success 'create repository and alternate directory' '\n> +\ttest_commit 1 &&\n> +\ttest_commit 2 &&\n> +\ttest_commit 3\n> +'\n> +\n> +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> +do\n> +\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n> +\t\toid=\"$(git rev-parse $obj)\" &&\n> +\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\ttest_when_finished \"mv $path.hidden $path\" &&\n> +\n> +\t\ttest_must_fail git rev-list --missing=error --objects \\\n> +\t\t\t--no-object-names HEAD\n> +\t'\n> +done\n> +\n> +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> +do\n> +\tfor action in \"allow-any\" \"print\"\n> +\tdo\n> +\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n> +\t\t\toid=\"$(git rev-parse $obj)\" &&\n> +\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\t\t# Before the object is made missing, we use rev-list to\n> +\t\t\t# get the expected oids.\n> +\t\t\tgit rev-list --objects --no-object-names \\\n> +\t\t\t\tHEAD ^$obj >expect.raw &&\n> +\n> +\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n> +\t\t\t# might be skipped, its blob must be accounted for.\n> +\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n> +\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n> +\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n> +\t\t\tfi &&\n> +\n> +\t\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n> +\n> +\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n> +\t\t\t\tHEAD >actual.raw &&\n> +\n> +\t\t\t# When the action is to print, we should also add the missing\n> +\t\t\t# oid to the expect list.\n> +\t\t\tcase $action in\n> +\t\t\tallow-any)\n> +\t\t\t\t;;\n> +\t\t\tprint)\n> +\t\t\t\tgrep ?$oid actual.raw &&\n> +\t\t\t\techo ?$oid >>expect.raw\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> +\n> +\t\t\tsort actual.raw >actual &&\n> +\t\t\tsort expect.raw >expect &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t'\n> +\tdone\n> +done\n> +\n> +test_done\n> -- \n> 2.42.0\n> \n"},{"id":"483953","messageId":"CAOLa=ZRdfA9tgFtcR9aW+W2DAmJKA0QrUzBYGncCBJdFJns+qQ@mail.gmail.com","threadId":"60331","inReplyTo":"ZTtXzg4NGJZzAqfS@tanuki","subject":"Re: [PATCH v5 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-27T07:54:26Z","receivedAt":"2023-10-27T07:54:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Oct 27, 2023 at 8:25 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> We unconditionally clear the oidset now, so shouldn't we also\n> unconditionally initialize it?\n>\n\nYou're right. I should have added that in.\n\nJunio, I'm replying to this email with the amended commit for v5. If\nyou want me to reroll\nv6, I could do that too. Thanks!\n"},{"id":"483954","messageId":"20231027075929.181817-1-karthik.188@gmail.com","threadId":"60331","inReplyTo":"ZTtXzg4NGJZzAqfS@tanuki","subject":"[PATCH v5 3/3] rev-list: add commit object support in `--missing` option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-10-27T07:59:29Z","receivedAt":"2023-10-27T07:59:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `--missing` object option in rev-list currently works only with\nmissing blobs/trees. For missing commits the revision walker fails with\na fatal error.\n\nLet's extend the functionality of `--missing` option to also support\ncommit objects. This is done by adding a `missing_objects` field to\n`rev_info`. This field is an `oidset` to which we'll add the missing\ncommits as we encounter them. The revision walker will now continue the\ntraversal and call `show_commit()` even for missing commits. In rev-list\nwe can then check if the commit is a missing commit and call the\nexisting code for parsing `--missing` objects.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, using the `--missing=print` option while disabling the\nalternate object directory allows us to find the boundary objects\nbetween the main and alternate object directory.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/rev-list.c          |  6 +++\n list-objects.c              |  3 ++\n revision.c                  | 16 +++++++-\n revision.h                  |  4 ++\n t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 101 insertions(+), 2 deletions(-)\n create mode 100755 t/t6022-rev-list-missing.sh\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 98542e8b3c..181353dcf5 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)\n \n \tdisplay_progress(progress, ++progress_counter);\n \n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    oidset_contains(&revs->missing_commits, &commit->object.oid)) {\n+\t\tfinish_object__ma(&commit->object);\n+\t\treturn;\n+\t}\n+\n \tif (show_disk_usage)\n \t\ttotal_disk_usage += get_object_disk_usage(&commit->object);\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 47296dff2f..f4e1104b56 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t */\n \t\tif (!ctx->revs->tree_objects)\n \t\t\t; /* do not bother loading tree */\n+\t\telse if (ctx->revs->do_not_die_on_missing_objects &&\n+\t\t\t oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))\n+\t\t\t;\n \t\telse if (repo_get_commit_tree(the_repository, commit)) {\n \t\t\tstruct tree *tree = repo_get_commit_tree(the_repository,\n \t\t\t\t\t\t\t\t commit);\ndiff --git a/revision.c b/revision.c\nindex 219dc76716..00d5c29bfc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -6,6 +6,7 @@\n #include \"object-name.h\"\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n+#include \"oidset.h\"\n #include \"tag.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n@@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n+\tif (revs->do_not_die_on_missing_objects &&\n+\t    oidset_contains(&revs->missing_commits, &commit->object.oid))\n+\t\treturn 0;\n \tcommit->object.flags |= ADDED;\n \n \tif (revs->include_check &&\n@@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \t\tint gently = revs->ignore_missing_links ||\n-\t\t\t     revs->exclude_promisor_objects;\n+\t\t\t     revs->exclude_promisor_objects ||\n+\t\t\t     revs->do_not_die_on_missing_objects;\n \t\tif (repo_parse_commit_gently(revs->repo, p, gently) < 0) {\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&p->object.oid)) {\n@@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t\t\tbreak;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\treturn -1;\n+\n+\t\t\tif (revs->do_not_die_on_missing_objects)\n+\t\t\t\toidset_insert(&revs->missing_commits, &p->object.oid);\n+\t\t\telse\n+\t\t\t\treturn -1; /* corrupt repository */\n \t\t}\n \t\tif (revs->sources) {\n \t\t\tchar **slot = revision_sources_at(revs->sources, p);\n@@ -3109,6 +3118,7 @@ void release_revisions(struct rev_info *revs)\n \tclear_decoration(&revs->merge_simplification, free);\n \tclear_decoration(&revs->treesame, free);\n \tline_log_free(revs);\n+\toidset_clear(&revs->missing_commits);\n }\n \n static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n@@ -3800,6 +3810,8 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n \t}\n \n+\toidset_init(&revs->missing_commits, 0);\n+\n \tif (!revs->reflog_info)\n \t\tprepare_to_use_bloom_filter(revs);\n \tif (!revs->unsorted_input)\ndiff --git a/revision.h b/revision.h\nindex c73c92ef40..94c43138bc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -4,6 +4,7 @@\n #include \"commit.h\"\n #include \"grep.h\"\n #include \"notes.h\"\n+#include \"oidset.h\"\n #include \"pretty.h\"\n #include \"diff.h\"\n #include \"commit-slab-decl.h\"\n@@ -373,6 +374,9 @@ struct rev_info {\n \n \t/* Location where temporary objects for remerge-diff are written. */\n \tstruct tmp_objdir *remerge_objdir;\n+\n+\t/* Missing commits to be tracked without failing traversal. */\n+\tstruct oidset missing_commits;\n };\n \n /**\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nnew file mode 100755\nindex 0000000000..40265a4f66\n--- /dev/null\n+++ b/t/t6022-rev-list-missing.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We setup the repository with two commits, this way HEAD is always\n+# available and we can hide commit 1.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit 1 &&\n+\ttest_commit 2 &&\n+\ttest_commit 3\n+'\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\n+\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\ttest_must_fail git rev-list --missing=error --objects \\\n+\t\t\t--no-object-names HEAD\n+\t'\n+done\n+\n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor action in \"allow-any\" \"print\"\n+\tdo\n+\t\ttest_expect_success \"rev-list --missing=$action with missing $obj\" '\n+\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t# get the expected oids.\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\tHEAD ^$obj >expect.raw &&\n+\n+\t\t\t# Blobs are shared by all commits, so evethough a commit/tree\n+\t\t\t# might be skipped, its blob must be accounted for.\n+\t\t\tif [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n+\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n+\t\t\tfi &&\n+\n+\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\tgit rev-list --missing=$action --objects --no-object-names \\\n+\t\t\t\tHEAD >actual.raw &&\n+\n+\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t# oid to the expect list.\n+\t\t\tcase $action in\n+\t\t\tallow-any)\n+\t\t\t\t;;\n+\t\t\tprint)\n+\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\n+\t\t\tsort actual.raw >actual &&\n+\t\t\tsort expect.raw >expect &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+done\n+\n+test_done\n-- \n2.42.0\n\n"}]}