{"thread":{"id":"60828","subject":"[PATCH 0/3] rev-list: allow missing tips with --missing","startedAt":"2024-02-01T11:58:26Z","lastAt":"2024-02-08T20:42:46Z","messageCount":20,"participants":["Christian Couder","Eric Sunshine","Junio C Hamano","Linus Arver"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"487734","messageId":"20240201115809.1177064-1-christian.couder@gmail.com","threadId":"60828","inReplyTo":null,"subject":"[PATCH 0/3] rev-list: allow missing tips with --missing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-01T11:58:06Z","receivedAt":"2024-02-01T11:58:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"A recent patch series, kn/rev-list-missing-fix [1] extended the\n`--missing` option in git-rev-list(1) to support commit objects.\n\nUnfortunately, git-rev-list(1) with `--missing` set to something other\nthan 'error' still fails, usually with a \"fatal: bad object \"\nerror message, when a missing object is passed as an argument.\n\nThis patch series removes this limitation and when using\n`--missing=print` allows all missing objects to be printed including\nthose that are passed as arguments.\n\nPatches 1/3 (revision: clarify a 'return NULL' in get_reference()) and\n2/3 (t6022: fix 'even though' typo in comment) are very small\npreparatory cleanups.\n\nPatch 3/3 (rev-list: add --allow-missing-tips to be used with\n--missing=...) introduces the new `--allow-missing-tips` option that\nallows git-rev-list(1) with `--missing` to not fail when one of the\ntips it is passed is missing.\n\n[1] https://lore.kernel.org/git/20231026101109.43110-1-karthik.188@gmail.com/\n\nChristian Couder (3):\n revision: clarify a 'return NULL' in get_reference()\n t6022: fix 'even though' typo in comment\n rev-list: add --allow-missing-tips to be used with --missing=...\n\n builtin/rev-list.c | 24 ++++++++++++++++-\n revision.c | 11 +++++---\n revision.h | 8 ++++++\n t/t6022-rev-list-missing.sh | 53 ++++++++++++++++++++++++++++++++++++-\n 4 files changed, 90 insertions(+), 6 deletions(-)\n\n-- \n2.43.0.496.gd667eb0d7d.dirty\n\n"},{"id":"487735","messageId":"20240201115809.1177064-2-christian.couder@gmail.com","threadId":"60828","inReplyTo":"20240201115809.1177064-1-christian.couder@gmail.com","subject":"[PATCH 1/3] revision: clarify a 'return NULL' in get_reference()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-01T11:58:07Z","receivedAt":"2024-02-01T11:58:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In general when we know a pointer variable is NULL, it's clearer to\nexplicitely return NULL than to return that variable.\n\nIn get_reference() when 'object' is NULL, we already return NULL\nwhen 'revs->exclude_promisor_objects && is_promisor_object(oid)' is\ntrue, but we return 'object' when 'revs->ignore_missing' is true.\n\nLet's make the code clearer and more uniform by also explicitely\nreturning NULL when 'revs->ignore_missing' is true.\n\nSigned-off-by: Christian Couder \n---\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..4c5cd7c3ce 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -385,7 +385,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \n \tif (!object) {\n \t\tif (revs->ignore_missing)\n-\t\t\treturn object;\n+\t\t\treturn NULL;\n \t\tif (revs->exclude_promisor_objects && is_promisor_object(oid))\n \t\t\treturn NULL;\n \t\tdie(\"bad object %s\", name);\n-- \n2.43.0.496.gd667eb0d7d.dirty\n\n"},{"id":"487736","messageId":"20240201115809.1177064-3-christian.couder@gmail.com","threadId":"60828","inReplyTo":"20240201115809.1177064-1-christian.couder@gmail.com","subject":"[PATCH 2/3] t6022: fix 'even though' typo in comment","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-01T11:58:08Z","receivedAt":"2024-02-01T11:58:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t6022-rev-list-missing.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nindex 211672759a..527aa94f07 100755\n--- a/t/t6022-rev-list-missing.sh\n+++ b/t/t6022-rev-list-missing.sh\n@@ -46,7 +46,7 @@ do\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# Blobs are shared by all commits, so even though 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-- \n2.43.0.496.gd667eb0d7d.dirty\n\n"},{"id":"487737","messageId":"20240201115809.1177064-4-christian.couder@gmail.com","threadId":"60828","inReplyTo":"20240201115809.1177064-1-christian.couder@gmail.com","subject":"[PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-01T11:58:09Z","receivedAt":"2024-02-01T11:58:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In 9830926c7d (rev-list: add commit object support in `--missing`\noption, 2023-10-27) we fixed the `--missing` option in `git rev-list`\nso that it now works with commits too.\n\nUnfortunately, such a command would still fail with a \"fatal: bad\nobject \" if it is passed a missing commit, blob or tree as an\nargument.\n\nWhen such a command is used to find the dependencies of some objects,\nfor example the dependencies of quarantined objects, it would be\nbetter if the command would instead consider such missing objects,\nespecially commits, in the same way as other missing objects.\n\nIf, for example `--missing=print` is used, it would be nice for some\nuse cases if the missing tips passed as arguments were reported in\nthe same way as other missing objects instead of the command just\nfailing.\n\nLet's introduce a new `--allow-missing-tips` option to make it work\nlike this.\n\nSigned-off-by: Christian Couder \n---\n builtin/rev-list.c | 24 ++++++++++++++++-\n revision.c | 9 ++++---\n revision.h | 8 ++++++\n t/t6022-rev-list-missing.sh | 51 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 88 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex b3f4783858..ae7bb15478 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -562,6 +562,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\t\tbreak;\n \t\t}\n \t}\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tif (!strcmp(arg, \"--allow-missing-tips\")) {\n+\t\t\tif (arg_missing_action == MA_ERROR)\n+\t\t\t\tdie(_(\"option '%s' only makes sense with '%s' set to '%s' or '%s'\"),\n+\t\t\t\t \"--allow-missing-tips\", \"--missing=\", \"allow-*\", \"print\");\n+\t\t\trevs.do_not_die_on_missing_tips = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n \n \tif (arg_missing_action)\n \t\trevs.do_not_die_on_missing_objects = 1;\n@@ -627,6 +637,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue; /* already handled above */\n \t\tif (skip_prefix(arg, \"--missing=\", &arg))\n \t\t\tcontinue; /* already handled above */\n+\t\tif (!strcmp(arg, \"--allow-missing-tips\"))\n+\t\t\tcontinue; /* already handled above */\n \n \t\tif (!strcmp(arg, (\"--no-object-names\"))) {\n \t\t\targ_show_object_names = 0;\n@@ -753,9 +765,19 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \tif (arg_print_omitted)\n \t\toidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);\n-\tif (arg_missing_action == MA_PRINT)\n+\tif (arg_missing_action == MA_PRINT) {\n+\t\tstruct oidset_iter iter;\n+\t\tstruct object_id *oid;\n+\n \t\toidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);\n \n+\t\t/* Already add missing commits */\n+\t\toidset_iter_init(&revs.missing_commits, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toidset_insert(&missing_objects, oid);\n+\t\toidset_clear(&revs.missing_commits);\n+\t}\n+\n \ttraverse_commit_list_filtered(\n \t\t&revs, show_commit, show_object, &info,\n \t\t(arg_print_omitted ? &omitted_objects : NULL));\ndiff --git a/revision.c b/revision.c\nindex 4c5cd7c3ce..9f25faa249 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -388,6 +388,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\t\treturn NULL;\n \t\tif (revs->exclude_promisor_objects && is_promisor_object(oid))\n \t\t\treturn NULL;\n+\t\tif (revs->do_not_die_on_missing_tips) {\n+\t\t\toidset_insert(&revs->missing_commits, oid);\n+\t\t\treturn NULL;\n+\t\t}\n \t\tdie(\"bad object %s\", name);\n \t}\n \tobject->flags |= flags;\n@@ -1947,6 +1951,7 @@ void repo_init_revisions(struct repository *r,\n \tinit_display_notes(&revs->notes_opt);\n \tlist_objects_filter_init(&revs->filter);\n \tinit_ref_exclusions(&revs->ref_excludes);\n+\toidset_init(&revs->missing_commits, 0);\n }\n \n static void add_pending_commit_list(struct rev_info *revs,\n@@ -2184,7 +2189,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n \tif (!object)\n-\t\treturn revs->ignore_missing ? 0 : -1;\n+\t\treturn (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n \tfree(oc.path);\n@@ -3830,8 +3835,6 @@ 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 94c43138bc..67435a5d8a 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -227,6 +227,14 @@ struct rev_info {\n \t\t\t */\n \t\t\tdo_not_die_on_missing_objects:1,\n \n+\t\t\t/*\n+\t\t\t * When the do_not_die_on_missing_objects flag above is set,\n+\t\t\t * a rev walk could still die with \"fatal: bad object \"\n+\t\t\t * if one of the tips it is passed is missing. With this flag\n+\t\t\t * such a tip will be reported as missing too.\n+\t\t\t */\n+\t\t\t do_not_die_on_missing_tips:1,\n+\n \t\t\t/* for internal use only */\n \t\t\texclude_promisor_objects:1;\n \ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nindex 527aa94f07..283e8fc2c2 100755\n--- a/t/t6022-rev-list-missing.sh\n+++ b/t/t6022-rev-list-missing.sh\n@@ -77,4 +77,55 @@ do\n \tdone\n done\n \n+for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n+do\n+\tfor tip in \"\" \"HEAD\"\n+\tdo\n+\t\tfor action in \"allow-any\" \"print\"\n+\t\tdo\n+\t\t\ttest_expect_success \"--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'\" '\n+\t\t\t\toid=\"$(git rev-parse $obj)\" &&\n+\t\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\t\t\t# Before the object is made missing, we use rev-list to\n+\t\t\t\t# get the expected oids.\n+\t\t\t\tif [ \"$tip\" = \"HEAD\" ]; then\n+\t\t\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\t\t\tHEAD ^$obj >expect.raw\n+\t\t\t\telse\n+\t\t\t\t\t>expect.raw\n+\t\t\t\tfi &&\n+\n+\t\t\t\t# Blobs are shared by all commits, so even though a commit/tree\n+\t\t\t\t# might be skipped, its blob must be accounted for.\n+\t\t\t\tif [ \"$tip\" = \"HEAD\" ] && [ $obj != \"HEAD:1.t\" ]; then\n+\t\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n+\t\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n+\t\t\t\tfi &&\n+\n+\t\t\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\t\t\tgit rev-list --missing=$action --allow-missing-tips \\\n+\t\t\t\t --objects --no-object-names $oid $tip >actual.raw &&\n+\n+\t\t\t\t# When the action is to print, we should also add the missing\n+\t\t\t\t# oid to the expect list.\n+\t\t\t\tcase $action in\n+\t\t\t\tallow-any)\n+\t\t\t\t\t;;\n+\t\t\t\tprint)\n+\t\t\t\t\tgrep ?$oid actual.raw &&\n+\t\t\t\t\techo ?$oid >>expect.raw\n+\t\t\t\t\t;;\n+\t\t\t\tesac &&\n+\n+\t\t\t\tsort actual.raw >actual &&\n+\t\t\t\tsort expect.raw >expect &&\n+\t\t\t\ttest_cmp expect actual\n+\t\t\t'\n+\t\tdone\n+\tdone\n+done\n+\n test_done\n-- \n2.43.0.496.gd667eb0d7d.dirty\n\n"},{"id":"487744","messageId":"CAPig+cSNM0VJZ5SpvazY5T6rFvXuoTdfgD5J5f36PW4iW7xLVA@mail.gmail.com","threadId":"60828","inReplyTo":"20240201115809.1177064-2-christian.couder@gmail.com","subject":"Re: [PATCH 1/3] revision: clarify a 'return NULL' in get_reference()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-01T14:53:45Z","receivedAt":"2024-02-01T14:53:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 1, 2024 at 6:58 AM Christian Couder\n wrote:\n> In general when we know a pointer variable is NULL, it's clearer to\n> explicitely return NULL than to return that variable.\n\ns/explicitely/explicitly/\n\n> In get_reference() when 'object' is NULL, we already return NULL\n> when 'revs->exclude_promisor_objects && is_promisor_object(oid)' is\n> true, but we return 'object' when 'revs->ignore_missing' is true.\n>\n> Let's make the code clearer and more uniform by also explicitely\n> returning NULL when 'revs->ignore_missing' is true.\n\ns/explicitely/explicitly/\n\n> Signed-off-by: Christian Couder \n"},{"id":"487752","messageId":"CAP8UFD3ka-HyUAdJ8RTfMt1HPmp5nQ3EKK2soE1Vjn+7C5OmMA@mail.gmail.com","threadId":"60828","inReplyTo":"CAPig+cSNM0VJZ5SpvazY5T6rFvXuoTdfgD5J5f36PW4iW7xLVA@mail.gmail.com","subject":"Re: [PATCH 1/3] revision: clarify a 'return NULL' in get_reference()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-01T16:49:26Z","receivedAt":"2024-02-01T16:49:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 1, 2024 at 3:53 PM Eric Sunshine wrote:\n>\n> On Thu, Feb 1, 2024 at 6:58 AM Christian Couder\n> wrote:\n> > In general when we know a pointer variable is NULL, it's clearer to\n> > explicitely return NULL than to return that variable.\n>\n> s/explicitely/explicitly/\n\n[...]\n\n> > Let's make the code clearer and more uniform by also explicitely\n> > returning NULL when 'revs->ignore_missing' is true.\n>\n> s/explicitely/explicitly/\n\nThanks, it's fixed in my current version. Not sure I have to resend\njust to fix this though.\n"},{"id":"487775","messageId":"xmqqo7d0x7fm.fsf@gitster.g","threadId":"60828","inReplyTo":"20240201115809.1177064-4-christian.couder@gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-01T20:20:45Z","receivedAt":"2024-02-01T20:20:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> In 9830926c7d (rev-list: add commit object support in `--missing`\n> option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n> so that it now works with commits too.\n>\n> Unfortunately, such a command would still fail with a \"fatal: bad\n> object \" if it is passed a missing commit, blob or tree as an\n> argument.\n>\n> When such a command is used to find the dependencies of some objects,\n> for example the dependencies of quarantined objects, it would be\n> better if the command would instead consider such missing objects,\n> especially commits, in the same way as other missing objects.\n>\n> If, for example `--missing=print` is used, it would be nice for some\n> use cases if the missing tips passed as arguments were reported in\n> the same way as other missing objects instead of the command just\n> failing.\n>\n> Let's introduce a new `--allow-missing-tips` option to make it work\n> like this.\n\nOK. Unlike a missing object referenced by a tree, a commit, or a\ntag, where the expected type of the missing object is known to Git,\nI would expect that nobody knowsn what type these missing objects at\nthe tip have. So do we now require \"object X missing\" instead of\n\"commit X missing\" in the output? If we are not giving any type\ninformation even when we know what the type we expect is, then we do\nnot have to worry about this change introducing a new output logic,\nI guess.\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index b3f4783858..ae7bb15478 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -562,6 +562,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> \t\t\t\tbreak;\n> \t\t}\n> \t}\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tconst char *arg = argv[i];\n> +\t\tif (!strcmp(arg, \"--allow-missing-tips\")) {\n> +\t\t\tif (arg_missing_action == MA_ERROR)\n> +\t\t\t\tdie(_(\"option '%s' only makes sense with '%s' set to '%s' or '%s'\"),\n> +\t\t\t\t \"--allow-missing-tips\", \"--missing=\", \"allow-*\", \"print\");\n> +\t\t\trevs.do_not_die_on_missing_tips = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nIt is unfortunate that we need to add yet another dumb loop that\ndoes not even try to understand there may be an option whose value\nhappens to be the string strcmp() looks for (we already have two\nsuch loops above this hunk). I have to wonder if we can do better.\n\nAn idle piece of idea. Perhaps we can instruct setup_revisions()\nnot to die immediately upon seeing a problematic argument, marking\nthe revs as \"broken\" instead, and keep going and interpreting as\nmuch as it could, so that it may record the presence of \"--missing\",\n\"--exclude-promisor-objects\", and \"--allow-missing-tips\". Then upon\nseeing setup_revisions() return with such an error, we can redo the\ncall with these bits already on.\n\nAnyway, I digress.\n\n> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> index 527aa94f07..283e8fc2c2 100755\n> --- a/t/t6022-rev-list-missing.sh\n> +++ b/t/t6022-rev-list-missing.sh\n> @@ -77,4 +77,55 @@ do\n> \tdone\n> done\n> \n> +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> +do\n> +\tfor tip in \"\" \"HEAD\"\n> +\tdo\n> +\t\tfor action in \"allow-any\" \"print\"\n> +\t\tdo\n> +\t\t\ttest_expect_success \"--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'\" '\n> +\t\t\t\toid=\"$(git rev-parse $obj)\" &&\n> +\t\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\t\t\t# Before the object is made missing, we use rev-list to\n> +\t\t\t\t# get the expected oids.\n> +\t\t\t\tif [ \"$tip\" = \"HEAD\" ]; then\n\nStyle:\n\n if test \"$tip\" = \"HEAD\"\n then\n\n> +\t\t\t\t\tgit rev-list --objects --no-object-names \\\n> +\t\t\t\t\t\tHEAD ^$obj >expect.raw\n> +\t\t\t\telse\n> +\t\t\t\t\t>expect.raw\n> +\t\t\t\tfi &&\n> +\n> +\t\t\t\t# Blobs are shared by all commits, so even though a commit/tree\n> +\t\t\t\t# might be skipped, its blob must be accounted for.\n> +\t\t\t\tif [ \"$tip\" = \"HEAD\" ] && [ $obj != \"HEAD:1.t\" ]; then\n\nDitto.\n\n> +\t\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n> +\t\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n> +\t\t\t\tfi &&\n> +\n> +\t\t\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n> +\n> +\t\t\t\tgit rev-list --missing=$action --allow-missing-tips \\\n> +\t\t\t\t --objects --no-object-names $oid $tip >actual.raw &&\n> +\n> +\t\t\t\t# When the action is to print, we should also add the missing\n> +\t\t\t\t# oid to the expect list.\n> +\t\t\t\tcase $action in\n> +\t\t\t\tallow-any)\n> +\t\t\t\t\t;;\n> +\t\t\t\tprint)\n> +\t\t\t\t\tgrep ?$oid actual.raw &&\n> +\t\t\t\t\techo ?$oid >>expect.raw\n> +\t\t\t\t\t;;\n\nOK. We do not say anything more than the object name (and the fact\nthat it is missing with a single byte '?'), so my earlier worry was\nunfounded. Good.\n\n> +\t\t\t\tesac &&\n> +\n> +\t\t\t\tsort actual.raw >actual &&\n> +\t\t\t\tsort expect.raw >expect &&\n> +\t\t\t\ttest_cmp expect actual\n> +\t\t\t'\n> +\t\tdone\n> +\tdone\n> +done\n> +\n> test_done\n\nTHanks.\n"},{"id":"487777","messageId":"xmqqil37yix1.fsf@gitster.g","threadId":"60828","inReplyTo":"20240201115809.1177064-4-christian.couder@gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-01T21:27:22Z","receivedAt":"2024-02-01T21:27:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> When such a command is used to find the dependencies of some objects,\n> for example the dependencies of quarantined objects, it would be\n> better if the command would instead consider such missing objects,\n> especially commits, in the same way as other missing objects.\n>\n> If, for example `--missing=print` is used, it would be nice for some\n> use cases if the missing tips passed as arguments were reported in\n> the same way as other missing objects instead of the command just\n> failing.\n>\n> Let's introduce a new `--allow-missing-tips` option to make it work\n> like this.\n\nAn obvious question is if this even needs to be a new option. What\nare expected use cases where --missing=print without this option is\nbeneficial? If there is no plausible use case, perhaps we can treat\nit as a mere bugfix to the existing --missing mechanism, especially\ngiven that support of commits in \"--missing\" itself is relatively\na new thing.\n\nIf we can do this as a bugfix that is always on when --missing is\nused, then we do not have to worry about adding another tasteless\nloop outside the main command line parser, which is a huge upside\n;-).\n"},{"id":"487809","messageId":"CAP8UFD3Hfgud19y_K1bZOudkBE-ss1_SgWTRVkJ7gUbYJ400yQ@mail.gmail.com","threadId":"60828","inReplyTo":"xmqqil37yix1.fsf@gitster.g","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-02T11:29:22Z","receivedAt":"2024-02-02T11:29:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 1, 2024 at 10:27 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > When such a command is used to find the dependencies of some objects,\n> > for example the dependencies of quarantined objects, it would be\n> > better if the command would instead consider such missing objects,\n> > especially commits, in the same way as other missing objects.\n> >\n> > If, for example `--missing=print` is used, it would be nice for some\n> > use cases if the missing tips passed as arguments were reported in\n> > the same way as other missing objects instead of the command just\n> > failing.\n> >\n> > Let's introduce a new `--allow-missing-tips` option to make it work\n> > like this.\n>\n> An obvious question is if this even needs to be a new option. What\n> are expected use cases where --missing=print without this option is\n> beneficial?\n\nI am not sure if such a case is really beneficial but some\npeople/script/forges might rely on an error from `git rev-list\n--missing=print` to propagate back an error to some user interface.\n\n> If there is no plausible use case, perhaps we can treat\n> it as a mere bugfix to the existing --missing mechanism, especially\n> given that support of commits in \"--missing\" itself is relatively\n> a new thing.\n\n`--missing=...` detecting missing commits is new, but it might have\nbeen used to find missing blobs or trees for some time as it exists\nsince:\n\ncaf3827e2f (rev-list: add list-objects filtering support, 2017-11-21)\n\n> If we can do this as a bugfix that is always on when --missing is\n> used, then we do not have to worry about adding another tasteless\n> loop outside the main command line parser, which is a huge upside\n> ;-).\n\nEven if I very much dislike the tasteless loops, I'd rather not risk\nuser regressions by changing how `git rev-list --missing=...` handles\nerrors in the commits it is passed as arguments. See my other message\nresponding to your previous comments about how I'd prefer we handle\nthis.\n"},{"id":"487810","messageId":"CAP8UFD0HwQr+qj=FrDQXJeY8PhVH1e7oErbVopgtv2X8ig7a7Q@mail.gmail.com","threadId":"60828","inReplyTo":"xmqqo7d0x7fm.fsf@gitster.g","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-02T11:29:25Z","receivedAt":"2024-02-02T11:29:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 1, 2024 at 9:20 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > In 9830926c7d (rev-list: add commit object support in `--missing`\n> > option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n> > so that it now works with commits too.\n> >\n> > Unfortunately, such a command would still fail with a \"fatal: bad\n> > object \" if it is passed a missing commit, blob or tree as an\n> > argument.\n> >\n> > When such a command is used to find the dependencies of some objects,\n> > for example the dependencies of quarantined objects, it would be\n> > better if the command would instead consider such missing objects,\n> > especially commits, in the same way as other missing objects.\n> >\n> > If, for example `--missing=print` is used, it would be nice for some\n> > use cases if the missing tips passed as arguments were reported in\n> > the same way as other missing objects instead of the command just\n> > failing.\n> >\n> > Let's introduce a new `--allow-missing-tips` option to make it work\n> > like this.\n>\n> OK. Unlike a missing object referenced by a tree, a commit, or a\n> tag, where the expected type of the missing object is known to Git,\n> I would expect that nobody knowsn what type these missing objects at\n> the tip have. So do we now require \"object X missing\" instead of\n> \"commit X missing\" in the output? If we are not giving any type\n> information even when we know what the type we expect is, then we do\n> not have to worry about this change introducing a new output logic,\n> I guess.\n\nYeah, we are not giving type information in the output, only a `?`\nbefore the oid.\n\n> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > index b3f4783858..ae7bb15478 100644\n> > --- a/builtin/rev-list.c\n> > +++ b/builtin/rev-list.c\n> > @@ -562,6 +562,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> > break;\n> > }\n> > }\n> > + for (i = 1; i < argc; i++) {\n> > + const char *arg = argv[i];\n> > + if (!strcmp(arg, \"--allow-missing-tips\")) {\n> > + if (arg_missing_action == MA_ERROR)\n> > + die(_(\"option '%s' only makes sense with '%s' set to '%s' or '%s'\"),\n> > + \"--allow-missing-tips\", \"--missing=\", \"allow-*\", \"print\");\n> > + revs.do_not_die_on_missing_tips = 1;\n> > + break;\n> > + }\n> > + }\n>\n> It is unfortunate that we need to add yet another dumb loop that\n> does not even try to understand there may be an option whose value\n> happens to be the string strcmp() looks for (we already have two\n> such loops above this hunk). I have to wonder if we can do better.\n\nI am also not happy with adding yet another dump loop like this. I did\nit because I couldn't think of a simple solution to avoid that.\n\n> An idle piece of idea. Perhaps we can instruct setup_revisions()\n> not to die immediately upon seeing a problematic argument, marking\n> the revs as \"broken\" instead, and keep going and interpreting as\n> much as it could, so that it may record the presence of \"--missing\",\n> \"--exclude-promisor-objects\", and \"--allow-missing-tips\". Then upon\n> seeing setup_revisions() return with such an error, we can redo the\n> call with these bits already on.\n\nWhen we discussed rejecting some rev walking options before calling\nsetup_revisions() in the context of `git replay`, one thing people\nseemed to agree on was that there should be a mechanism in\nsetup_revisions() that allows us to tell setup_revisions() which rev\nwalking options it should allow or not. I think that such a mechanism\nmight need an early parsing of options which could be reused for\noptions like `--exclude-promisor-objects`, `--missing=...` and\n`--allow-missing-tips`. But I think adding such a mechanism does not\nbelong to this patch series. It's some cleanup and refactoring that we\nmight have needed for a long time already.\n\nIn my current version, I have added the following NEEDSWORK comment\nbefore the three dump loops to make sure we remember about this:\n\n /*\n * NEEDSWORK: These dump loops to look for some options early\n * are ugly. We really need setup_revisions() to have a\n * mechanism to allow and disallow some sets of options for\n * different commands (like rev-list, replay, etc). Such\n * mechanism should do an early parsing of option and be able\n * to manage the `--exclude-promisor-objects`, `--missing=...`\n * and `--allow-missing-tips` options below.\n */\n\nI have also added the following to the commit message:\n\n\"We introduce another dump loop to look for that options early, but\naddressing the dump loops should likely be part of adding a new\nmechanism to setup_revisions() which is a different topic. Anyway\nlet's add a big NEEDSWORK comment to remember that we really need to\ndo this.\"\n\n> Anyway, I digress.\n>\n> > diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> > index 527aa94f07..283e8fc2c2 100755\n> > --- a/t/t6022-rev-list-missing.sh\n> > +++ b/t/t6022-rev-list-missing.sh\n> > @@ -77,4 +77,55 @@ do\n> > done\n> > done\n> >\n> > +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> > +do\n> > + for tip in \"\" \"HEAD\"\n> > + do\n> > + for action in \"allow-any\" \"print\"\n> > + do\n> > + test_expect_success \"--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'\" '\n> > + oid=\"$(git rev-parse $obj)\" &&\n> > + path=\".git/objects/$(test_oid_to_path $oid)\" &&\n> > +\n> > + # Before the object is made missing, we use rev-list to\n> > + # get the expected oids.\n> > + if [ \"$tip\" = \"HEAD\" ]; then\n>\n> Style:\n>\n> if test \"$tip\" = \"HEAD\"\n> then\n\nFixed in my current version. In it, I also fixed in patch 2/3 a\nsimilar style typo in the tests before these ones.\n\n> > + git rev-list --objects --no-object-names \\\n> > + HEAD ^$obj >expect.raw\n> > + else\n> > + >expect.raw\n> > + fi &&\n> > +\n> > + # Blobs are shared by all commits, so even though a commit/tree\n> > + # might be skipped, its blob must be accounted for.\n> > + if [ \"$tip\" = \"HEAD\" ] && [ $obj != \"HEAD:1.t\" ]; then\n>\n> Ditto.\n\nFixed too in my current version.\n\n> > + echo $(git rev-parse HEAD:1.t) >>expect.raw &&\n> > + echo $(git rev-parse HEAD:2.t) >>expect.raw\n> > + fi &&\n> > +\n> > + mv \"$path\" \"$path.hidden\" &&\n> > + test_when_finished \"mv $path.hidden $path\" &&\n> > +\n> > + git rev-list --missing=$action --allow-missing-tips \\\n> > + --objects --no-object-names $oid $tip >actual.raw &&\n> > +\n> > + # When the action is to print, we should also add the missing\n> > + # oid to the expect list.\n> > + case $action in\n> > + allow-any)\n> > + ;;\n> > + print)\n> > + grep ?$oid actual.raw &&\n> > + echo ?$oid >>expect.raw\n> > + ;;\n>\n> OK. We do not say anything more than the object name (and the fact\n> that it is missing with a single byte '?'), so my earlier worry was\n> unfounded. Good.\n>\n> > + esac &&\n> > +\n> > + sort actual.raw >actual &&\n> > + sort expect.raw >expect &&\n> > + test_cmp expect actual\n> > + '\n> > + done\n> > + done\n> > +done\n> > +\n> > test_done\n>\n> THanks.\n\nThanks for the review!\n"},{"id":"487825","messageId":"xmqq5xz6vmnk.fsf@gitster.g","threadId":"60828","inReplyTo":"CAP8UFD0HwQr+qj=FrDQXJeY8PhVH1e7oErbVopgtv2X8ig7a7Q@mail.gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T16:47:11Z","receivedAt":"2024-02-02T16:47:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> I am also not happy with adding yet another dump loop like this. I did\n> it because I couldn't think of a simple solution to avoid that.\n\nFWIW, that word is \"dumB\" loop, if you are including the word in\nyour updated version.\n\nBut I am still wondering if this can be unconditionally turned on\nany time --missing=... is used. Have you considered it? If we can\ndo that, it would eliminate the need for that extra broken loop that\nwill fail with \"git rev-list --grep --allow-missing-tips -20 HEAD\".\n\n"},{"id":"487826","messageId":"xmqqv876u7ql.fsf@gitster.g","threadId":"60828","inReplyTo":"CAP8UFD3Hfgud19y_K1bZOudkBE-ss1_SgWTRVkJ7gUbYJ400yQ@mail.gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T16:54:42Z","receivedAt":"2024-02-02T16:54:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> An obvious question is if this even needs to be a new option. What\n>> are expected use cases where --missing=print without this option is\n>> beneficial?\n>\n> I am not sure if such a case is really beneficial but some\n> people/script/forges might rely on an error from `git rev-list\n> --missing=print` to propagate back an error to some user interface.\n\nYes, and they are missing errors for starting point objects right\nnow, which is a bug, just like they were missing output for commit\nobjects before we fixed it recently?\n\n>> If there is no plausible use case, perhaps we can treat\n>> it as a mere bugfix to the existing --missing mechanism, especially\n>> given that support of commits in \"--missing\" itself is relatively\n>> a new thing.\n>\n> `--missing=...` detecting missing commits is new, but it might have\n> been used to find missing blobs or trees for some time as it exists\n> since:\n>\n> caf3827e2f (rev-list: add list-objects filtering support, 2017-11-21)\n\nSo it fixes a long-standing bug. So what?\n\nEspecially given that the change in behaviour is to error out only\nwhen the user gave an object name for a missing object, and we know\nthe user wants to be notified on all missing objects \"found\" during\nthe traversal, it smells like (1) a rare enough case that may not\nmatter in the real world and (2) the existing scripts that may feed\npotentially missing objects to the command would be doing an extra\ncheck to ensure what they feed the command are not missing, in order\nto avoid triggering that \"starting point objects induce an error\ninstead of --missing=print report\" bug, and the bugfix will make\nsuch a workaround unnecessary.\n\n\n\n\n"},{"id":"488128","messageId":"owlywmrgmx2j.fsf@fine.c.googlers.com","threadId":"60828","inReplyTo":"20240201115809.1177064-4-christian.couder@gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-02-07T09:40:36Z","receivedAt":"2024-02-07T09:40:39Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"\nChristian Couder writes:\n\n> In 9830926c7d (rev-list: add commit object support in `--missing`\n> option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n> so that it now works with commits too.\n>\n> Unfortunately, such a command would still fail with a \"fatal: bad\n> object \" if it is passed a missing commit, blob or tree as an\n> argument.\n\nHmm, but according to the manpage for rev-list, it only accepts commits\nas arguments. Conflict...?\n\nAlso it took me a second to realize that you are talking about\nmissing objects passed in at the command line to `git rev-list` and not\nthe objects that this command encounters during the normal rev walking\nprocess. It would have been a touch nicer to say something more\nexplicit, like\n\n In 9830926c7d (rev-list: add commit object support in `--missing`\n option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n so that it works with missing commits, not just blobs/trees.\n \n Unfortunately, it would still fail with a \"fatal: bad\n object \" if it is passed a missing commit as an\n argument (before the rev walking even begins).\n\n> When such a command is used to find the dependencies of some objects,\n> for example the dependencies of quarantined objects, it would be\n> better if the command would instead consider such missing objects,\n> especially commits, in the same way as other missing objects.\n\nCould you define what quarantined objects are (it's not in the manpage\nfor rev-list)? But also, I'm not sure how much additional value this\nparagraph is adding on top of what you already said in the first two\nparagraphs.\n\n> If, for example `--missing=print` is used, it would be nice for some\n> use cases if the missing tips passed as arguments were reported in\n> the same way as other missing objects instead of the command just\n> failing.\n>\n> Let's introduce a new `--allow-missing-tips` option to make it work\n> like this.\n\nI assume \"tips\" means \"the starting commits which are passed into this\ncommand from which it begins the rev walk\". Might be worth including in\nthe commit message.\n\nBut also, it's curious that you chose the word \"allow\" when we already\nhave \"--ignore-missing\". I assume this is because you still want the\nmissing tips to still be displayed in the output, and not ignored\n(omitted from processing altogether). Perhaps \"--report-missing-tips\"\nis more explicit?\n\n> @@ -753,9 +765,19 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> \n> \tif (arg_print_omitted)\n> \t\toidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);\n> -\tif (arg_missing_action == MA_PRINT)\n> +\tif (arg_missing_action == MA_PRINT) {\n> +\t\tstruct oidset_iter iter;\n> +\t\tstruct object_id *oid;\n> +\n> \t\toidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);\n> \n> +\t\t/* Already add missing commits */\n\nDid you mean \"Add already-missing commits\"? Perhaps even more explicit\nwould be \"Add missing tips\"?\n\n> +\t\toidset_iter_init(&revs.missing_commits, &iter);\n> +\t\twhile ((oid = oidset_iter_next(&iter)))\n> +\t\t\toidset_insert(&missing_objects, oid);\n> +\t\toidset_clear(&revs.missing_commits);\n> +\t}\n\nAside: I am surprised that there is no helper function already that\nsimply copies things in one oidset into another oidset.\n\n> \ttraverse_commit_list_filtered(\n> \t\t&revs, show_commit, show_object, &info,\n> \t\t(arg_print_omitted ? &omitted_objects : NULL));\n> diff --git a/revision.c b/revision.c\n> index 4c5cd7c3ce..9f25faa249 100644\n> --- a/revision.c\n> +++ b/revision.c\n> \n> [...]\n> \n> @@ -2184,7 +2189,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n> \t\tverify_non_filename(revs->prefix, arg);\n> \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n> \tif (!object)\n> -\t\treturn revs->ignore_missing ? 0 : -1;\n> +\t\treturn (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n> \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n> \tfree(oc.path);\n\nWith a few more context lines, the diff looks like\n\n--8<---------------cut here---------------start------------->8---\n if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))\n return revs->ignore_missing ? 0 : -1;\n if (!cant_be_filename)\n verify_non_filename(revs->prefix, arg);\n object = get_reference(revs, arg, &oid, flags ^ local_flags);\n if (!object)\n- return revs->ignore_missing ? 0 : -1;\n+ return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n free(oc.path);\n return 0;\n--8<---------------cut here---------------end--------------->8---\n\nand it's not obvious to me why you only touched the \"if (!object)\" case\nbut not the \"if (get_oid_with_context(...))\" case. Are there any\nsubtleties here that would not be obvious to reviewers?\n\n> @@ -3830,8 +3835,6 @@ 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)\n> diff --git a/revision.h b/revision.h\n> index 94c43138bc..67435a5d8a 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -227,6 +227,14 @@ struct rev_info {\n> \t\t\t */\n> \t\t\tdo_not_die_on_missing_objects:1,\n> \n> +\t\t\t/*\n> +\t\t\t * When the do_not_die_on_missing_objects flag above is set,\n> +\t\t\t * a rev walk could still die with \"fatal: bad object \"\n> +\t\t\t * if one of the tips it is passed is missing. With this flag\n> +\t\t\t * such a tip will be reported as missing too.\n> +\t\t\t */\n> +\t\t\t do_not_die_on_missing_tips:1,\n> +\n> \t\t\t/* for internal use only */\n> \t\t\texclude_promisor_objects:1;\n\nIIUC, we won't get to even do the rev walk though if all tips are\nmissing. So perhaps a more precise comment would be\n\n /*\n * When the do_not_die_on_missing_objects flag above is set,\n * we could prematurely die with \"fatal: bad object \"\n * if one of the tips it is passed is missing before even getting to\n * the rev walk. With this flag such a tip will be reported as\n * missing in the output after the rev walk completes.\n */\n \n> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> index 527aa94f07..283e8fc2c2 100755\n> --- a/t/t6022-rev-list-missing.sh\n> +++ b/t/t6022-rev-list-missing.sh\n> @@ -77,4 +77,55 @@ do\n> \tdone\n> done\n> \n> +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> +do\n> +\tfor tip in \"\" \"HEAD\"\n\nSo far from the patch diff it was not obvious that you wanted to check\nfor the empty string as a \"tip\".\n\n> +\tdo\n> +\t\tfor action in \"allow-any\" \"print\"\n> +\t\tdo\n> +\t\t\ttest_expect_success \"--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'\" '\n\nIf I run this test without the new --allow-missing-tips flag, I get \n\n fatal: bad object 82335b23aa7234320d6f8055243c852e4b6a5ca3\n not ok 11 - --missing=allow-any --allow-missing-tips with tip 'HEAD~1' missing and tip ''\n\nand I think the last \"tip ''\" part is misleading because IIUC it's not\nactually passed in as a tip (see my comment below about shell quoting).\n\n> +\t\t\t\toid=\"$(git rev-parse $obj)\" &&\n> +\t\t\t\tpath=\".git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\t\t\t# Before the object is made missing, we use rev-list to\n> +\t\t\t\t# get the expected oids.\n> +\t\t\t\tif [ \"$tip\" = \"HEAD\" ]; then\n> +\t\t\t\t\tgit rev-list --objects --no-object-names \\\n> +\t\t\t\t\t\tHEAD ^$obj >expect.raw\n> +\t\t\t\telse\n> +\t\t\t\t\t>expect.raw\n> +\t\t\t\tfi &&\n\nOK so you are using this empty string to clear the expect.raw file. If\nthat's true then what stops us from doing this at the start of every\niteration?\n\n> +\t\t\t\t# Blobs are shared by all commits, so even though a commit/tree\n> +\t\t\t\t# might be skipped, its blob must be accounted for.\n> +\t\t\t\tif [ \"$tip\" = \"HEAD\" ] && [ $obj != \"HEAD:1.t\" ]; then\n> +\t\t\t\t\techo $(git rev-parse HEAD:1.t) >>expect.raw &&\n> +\t\t\t\t\techo $(git rev-parse HEAD:2.t) >>expect.raw\n> +\t\t\t\tfi &&\n> +\n> +\t\t\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\t\t\ttest_when_finished \"mv $path.hidden $path\" &&\n> +\n> +\t\t\t\tgit rev-list --missing=$action --allow-missing-tips \\\n> +\t\t\t\t --objects --no-object-names $oid $tip >actual.raw &&\n\nThe fact that $tip is not quoted here means that this is equivalent to\n\n --objects --no-object-names $oid >actual.raw &&\n\nand not\n\n --objects --no-object-names $oid \"\" >actual.raw &&\n\nfor the case where tip is the empty string. I wonder if we could avoid\nbeing nuanced here with subtle shell quoting rules, especially because\nthese are tests (where making everything as explicit as possible is\ndesirable).\n\n> +\t\t\t\t# When the action is to print, we should also add the missing\n> +\t\t\t\t# oid to the expect list.\n> +\t\t\t\tcase $action in\n> +\t\t\t\tallow-any)\n> +\t\t\t\t\t;;\n> +\t\t\t\tprint)\n> +\t\t\t\t\tgrep ?$oid actual.raw &&\n> +\t\t\t\t\techo ?$oid >>expect.raw\n> +\t\t\t\t\t;;\n> +\t\t\t\tesac &&\n> +\n> +\t\t\t\tsort actual.raw >actual &&\n> +\t\t\t\tsort expect.raw >expect &&\n> +\t\t\t\ttest_cmp expect actual\n> +\t\t\t'\n> +\t\tdone\n> +\tdone\n> +done\n> +\n\nWait, but where are we actually making the $tip commit's object\n_missing_? Hmm...\n\nAh, actually the tips are just the $obj variables. So it seems like you\ncould have done\n\n for tip in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n\nin the beginning to make it more obvious.\n\nAlso, given how most of this is identical from the loop already in t6022\njust above it, it would help to add a comment at the top of this one\nexplaining how it's different than the one above it.\n\nThanks.\n\n> test_done\n> -- \n> 2.43.0.496.gd667eb0d7d.dirty\n"},{"id":"488129","messageId":"owlyttmkmwaf.fsf@fine.c.googlers.com","threadId":"60828","inReplyTo":"CAP8UFD3Hfgud19y_K1bZOudkBE-ss1_SgWTRVkJ7gUbYJ400yQ@mail.gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-02-07T09:57:28Z","receivedAt":"2024-02-07T09:57:30Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Christian Couder writes:\n\n> On Thu, Feb 1, 2024 at 10:27 PM Junio C Hamano wrote:\n>>\n>> Christian Couder writes:\n>>\n>> > When such a command is used to find the dependencies of some objects,\n>> > for example the dependencies of quarantined objects, it would be\n>> > better if the command would instead consider such missing objects,\n>> > especially commits, in the same way as other missing objects.\n>> >\n>> > If, for example `--missing=print` is used, it would be nice for some\n>> > use cases if the missing tips passed as arguments were reported in\n>> > the same way as other missing objects instead of the command just\n>> > failing.\n>> >\n>> > Let's introduce a new `--allow-missing-tips` option to make it work\n>> > like this.\n>>\n>> An obvious question is if this even needs to be a new option. What\n>> are expected use cases where --missing=print without this option is\n>> beneficial?\n>\n> I am not sure if such a case is really beneficial but some\n> people/script/forges might rely on an error from `git rev-list\n> --missing=print` to propagate back an error to some user interface.\n\nI currently learn toward just making the new flag's behavior be absorved\ninto the existing \"--missing=...\" flag. Nevertheless, you raise an\ninteresting concern.\n\nPerhaps a compromise would be to make \"--missing=...\" learn the new\nbehavior of this patch as Junio suggested, but to introduce a new flag,\nsomething like \"--fail-on-missing-tips\" to fail early if any of the tip\ncommits' objects are missing? That way we could keep the current\n\"strict\" behavior of complaining if we feed rev-list any tips whose\nobjects are missing. And for the vast majority of cases the\n\"--missing=...\" flag could (intuitively) gracefully handle tips with\nmissing objects and you wouldn't have to pass in the additional flag.\n\nIOW, make the minority (certainly not majority, I think?) of users who\nreally need the error propagation use the (new) extra flag, while the\nrest of us (including the version of you who was surprised by the\nlimited behavior of \"--missing=...\", enough to write this series) don't\nhave to.\n\nThanks.\n"},{"id":"488142","messageId":"CAP8UFD3D=a-3cVNpjobOdq=F5dC+aW4qYu3fXCxTnDg=GrnSwA@mail.gmail.com","threadId":"60828","inReplyTo":"owlywmrgmx2j.fsf@fine.c.googlers.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-07T16:11:39Z","receivedAt":"2024-02-07T16:11:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Feb 7, 2024 at 10:40 AM Linus Arver wrote:\n>\n>\n> Christian Couder writes:\n>\n> > In 9830926c7d (rev-list: add commit object support in `--missing`\n> > option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n> > so that it now works with commits too.\n> >\n> > Unfortunately, such a command would still fail with a \"fatal: bad\n> > object \" if it is passed a missing commit, blob or tree as an\n> > argument.\n>\n> Hmm, but according to the manpage for rev-list, it only accepts commits\n> as arguments. Conflict...?\n\nIt actually kind of \"works\" when you pass blobs or trees. It looks\nlike the command just doesn't use them (except for checking that they\nactually exist) unless options like --objects, --missing=print and\nperhaps others are used. So yeah, the doc might need an update, but I\nthink it could be a separate issue, as it's not new and doesn't depend\non this small series.\n\n> Also it took me a second to realize that you are talking about\n> missing objects passed in at the command line to `git rev-list` and not\n> the objects that this command encounters during the normal rev walking\n> process. It would have been a touch nicer to say something more\n> explicit, like\n>\n> In 9830926c7d (rev-list: add commit object support in `--missing`\n> option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n> so that it works with missing commits, not just blobs/trees.\n>\n> Unfortunately, it would still fail with a \"fatal: bad\n> object \" if it is passed a missing commit as an\n> argument (before the rev walking even begins).\n\nThanks, I have used your suggestions in my current version, except\nthat I still use \"passed a missing commit, blob or tree\" instead of\n\"passed a missing commit\" as the command also accepts blobs and trees.\n\n> > When such a command is used to find the dependencies of some objects,\n> > for example the dependencies of quarantined objects, it would be\n> > better if the command would instead consider such missing objects,\n> > especially commits, in the same way as other missing objects.\n>\n> Could you define what quarantined objects are (it's not in the manpage\n> for rev-list)?\n\n\"quarantined objects\" refers to the following doc:\n\nhttps://www.git-scm.com/docs/git-receive-pack#_quarantine_environment\n\nSo to clarify things, the above paragraph looks like the following now:\n\n\"When such a command is used to find the dependencies of some objects,\nfor example the dependencies of quarantined objects (see the\n\"QUARANTINE ENVIRONMENT\" section in the git-receive-pack(1)\ndocumentation), it would be better if the command would instead\nconsider such missing objects, especially commits, in the same way as\nother missing objects.\"\n\n> But also, I'm not sure how much additional value this\n> paragraph is adding on top of what you already said in the first two\n> paragraphs.\n\nIt's a more specific example, and it's how we detected the issue we\nwould like to address, so I think it has some value. I am Ok with\nremoving it, if others don't see some value in it though.\n\n> > If, for example `--missing=print` is used, it would be nice for some\n> > use cases if the missing tips passed as arguments were reported in\n> > the same way as other missing objects instead of the command just\n> > failing.\n> >\n> > Let's introduce a new `--allow-missing-tips` option to make it work\n> > like this.\n>\n> I assume \"tips\" means \"the starting commits which are passed into this\n> command from which it begins the rev walk\". Might be worth including in\n> the commit message.\n\nI think \"tips\" is better than \"commits\" because blobs and trees are\nallowed (see above).\n\n> But also, it's curious that you chose the word \"allow\" when we already\n> have \"--ignore-missing\". I assume this is because you still want the\n> missing tips to still be displayed in the output, and not ignored\n> (omitted from processing altogether). Perhaps \"--report-missing-tips\"\n> is more explicit?\n\nI think \"allow\" is better as it makes it clearer that the command will\nnot fail while it should fail without the option.\n\nAnyway I think I will implement Junio's suggestion to just avoid\nadding a new option and just change the behavior of the command when\n--missing=[print|allow*] is used.\n\n> > @@ -753,9 +765,19 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> >\n> > if (arg_print_omitted)\n> > oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);\n> > - if (arg_missing_action == MA_PRINT)\n> > + if (arg_missing_action == MA_PRINT) {\n> > + struct oidset_iter iter;\n> > + struct object_id *oid;\n> > +\n> > oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);\n> >\n> > + /* Already add missing commits */\n>\n> Did you mean \"Add already-missing commits\"? Perhaps even more explicit\n> would be \"Add missing tips\"?\n\nYeah, right. I have changed it in my current version.\n\n> > + oidset_iter_init(&revs.missing_commits, &iter);\n> > + while ((oid = oidset_iter_next(&iter)))\n> > + oidset_insert(&missing_objects, oid);\n> > + oidset_clear(&revs.missing_commits);\n> > + }\n>\n> Aside: I am surprised that there is no helper function already that\n> simply copies things in one oidset into another oidset.\n\nYeah, I was surprised too. I only found the following similar code in\nlist-objects-filter.c:\n\n oidset_iter_init(src, &iter);\n while ((src_oid = oidset_iter_next(&iter)) != NULL)\n oidset_insert(dest, src_oid);\n\nSo yeah perhaps we could create such a helper function.\n\n> > traverse_commit_list_filtered(\n> > &revs, show_commit, show_object, &info,\n> > (arg_print_omitted ? &omitted_objects : NULL));\n> > diff --git a/revision.c b/revision.c\n> > index 4c5cd7c3ce..9f25faa249 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> >\n> > [...]\n> >\n> > @@ -2184,7 +2189,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n> > verify_non_filename(revs->prefix, arg);\n> > object = get_reference(revs, arg, &oid, flags ^ local_flags);\n> > if (!object)\n> > - return revs->ignore_missing ? 0 : -1;\n> > + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n> > add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> > add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n> > free(oc.path);\n>\n> With a few more context lines, the diff looks like\n>\n> --8<---------------cut here---------------start------------->8---\n> if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))\n> return revs->ignore_missing ? 0 : -1;\n> if (!cant_be_filename)\n> verify_non_filename(revs->prefix, arg);\n> object = get_reference(revs, arg, &oid, flags ^ local_flags);\n> if (!object)\n> - return revs->ignore_missing ? 0 : -1;\n> + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n> add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n> free(oc.path);\n> return 0;\n> --8<---------------cut here---------------end--------------->8---\n>\n> and it's not obvious to me why you only touched the \"if (!object)\" case\n> but not the \"if (get_oid_with_context(...))\" case. Are there any\n> subtleties here that would not be obvious to reviewers?\n\nI thought that if we can't get an oid, we cannot put anything in the\n'missing_commit' oidset anyway, so it might be better to error out.\n\n> > @@ -3830,8 +3835,6 @@ int prepare_revision_walk(struct rev_info *revs)\n> > FOR_EACH_OBJECT_PROMISOR_ONLY);\n> > }\n> >\n> > - oidset_init(&revs->missing_commits, 0);\n> > -\n> > if (!revs->reflog_info)\n> > prepare_to_use_bloom_filter(revs);\n> > if (!revs->unsorted_input)\n> > diff --git a/revision.h b/revision.h\n> > index 94c43138bc..67435a5d8a 100644\n> > --- a/revision.h\n> > +++ b/revision.h\n> > @@ -227,6 +227,14 @@ struct rev_info {\n> > */\n> > do_not_die_on_missing_objects:1,\n> >\n> > + /*\n> > + * When the do_not_die_on_missing_objects flag above is set,\n> > + * a rev walk could still die with \"fatal: bad object \"\n> > + * if one of the tips it is passed is missing. With this flag\n> > + * such a tip will be reported as missing too.\n> > + */\n> > + do_not_die_on_missing_tips:1,\n> > +\n> > /* for internal use only */\n> > exclude_promisor_objects:1;\n>\n> IIUC, we won't get to even do the rev walk though if all tips are\n> missing. So perhaps a more precise comment would be\n>\n> /*\n> * When the do_not_die_on_missing_objects flag above is set,\n> * we could prematurely die with \"fatal: bad object \"\n> * if one of the tips it is passed is missing before even getting to\n> * the rev walk. With this flag such a tip will be reported as\n> * missing in the output after the rev walk completes.\n> */\n\nThanks for the suggestion, I have now:\n\n /*\n * When the do_not_die_on_missing_objects flag above is set,\n * we could prematurely die with \"fatal: bad object \"\n * if one of the tips passed to a rev walk is missing, before\n * the rev walk even starts. With this flag such a tip will\n * be reported as missing in the output after the rev walk\n * completes.\n */\n do_not_die_on_missing_tips:1,\n\n\n> > diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> > index 527aa94f07..283e8fc2c2 100755\n> > --- a/t/t6022-rev-list-missing.sh\n> > +++ b/t/t6022-rev-list-missing.sh\n> > @@ -77,4 +77,55 @@ do\n> > done\n> > done\n> >\n> > +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n> > +do\n> > + for tip in \"\" \"HEAD\"\n>\n> So far from the patch diff it was not obvious that you wanted to check\n> for the empty string as a \"tip\".\n\nWe want to check that things work as expected both:\n\n - when we pass only one missing tip, and:\n - when we pass one missing tip and a tip that is not missing.\n\nWe are setting 'tip' to \"\" in the former case so that when we will use\n\"$oid $tip\" in the `git rev-list --missing=$action\n--allow-missing-tips ...` command below, it will be like we passed\nonly \"$oid\" to the command.\n\n> > + do\n> > + for action in \"allow-any\" \"print\"\n> > + do\n> > + test_expect_success \"--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'\" '\n>\n> If I run this test without the new --allow-missing-tips flag, I get\n>\n> fatal: bad object 82335b23aa7234320d6f8055243c852e4b6a5ca3\n> not ok 11 - --missing=allow-any --allow-missing-tips with tip 'HEAD~1' missing and tip ''\n>\n> and I think the last \"tip ''\" part is misleading because IIUC it's not\n> actually passed in as a tip (see my comment below about shell quoting).\n>\n> > + oid=\"$(git rev-parse $obj)\" &&\n> > + path=\".git/objects/$(test_oid_to_path $oid)\" &&\n> > +\n> > + # Before the object is made missing, we use rev-list to\n> > + # get the expected oids.\n> > + if [ \"$tip\" = \"HEAD\" ]; then\n> > + git rev-list --objects --no-object-names \\\n> > + HEAD ^$obj >expect.raw\n> > + else\n> > + >expect.raw\n> > + fi &&\n>\n> OK so you are using this empty string to clear the expect.raw file. If\n> that's true then what stops us from doing this at the start of every\n> iteration?\n\nI am not sure what you mean. Both:\n\n git rev-list --objects --no-object-names HEAD ^$obj >expect.raw\n\nand:\n\n >expect.raw #2\n\nare clearing \"expect.raw\" as \">expect.raw\" is used in both cases.\n\nIf we did the latter at the start of every iteration, then when we do\nthe former afterwards, we would clear \"expect.raw\" raw again, while\nthere is no point in clearing it twice.\n\n> > + # Blobs are shared by all commits, so even though a commit/tree\n> > + # might be skipped, its blob must be accounted for.\n> > + if [ \"$tip\" = \"HEAD\" ] && [ $obj != \"HEAD:1.t\" ]; then\n> > + echo $(git rev-parse HEAD:1.t) >>expect.raw &&\n> > + echo $(git rev-parse HEAD:2.t) >>expect.raw\n> > + fi &&\n> > +\n> > + mv \"$path\" \"$path.hidden\" &&\n> > + test_when_finished \"mv $path.hidden $path\" &&\n> > +\n> > + git rev-list --missing=$action --allow-missing-tips \\\n> > + --objects --no-object-names $oid $tip >actual.raw &&\n>\n> The fact that $tip is not quoted here means that this is equivalent to\n>\n> --objects --no-object-names $oid >actual.raw &&\n>\n> and not\n>\n> --objects --no-object-names $oid \"\" >actual.raw &&\n\nYeah, right, and that's what we want.\n\n> for the case where tip is the empty string. I wonder if we could avoid\n> being nuanced here with subtle shell quoting rules, especially because\n> these are tests (where making everything as explicit as possible is\n> desirable).\n\nI think it's not very subtle, but perhaps a comment would help. So I\nadded the following to my current version before the loop:\n\n # We want to check that things work when both\n # - all the tips passed are missing (case tip = \"\"), and\n # - there is one missing tip and one existing tip (case tip = \"HEAD\")\n\n> > + # When the action is to print, we should also add the missing\n> > + # oid to the expect list.\n> > + case $action in\n> > + allow-any)\n> > + ;;\n> > + print)\n> > + grep ?$oid actual.raw &&\n> > + echo ?$oid >>expect.raw\n> > + ;;\n> > + esac &&\n> > +\n> > + sort actual.raw >actual &&\n> > + sort expect.raw >expect &&\n> > + test_cmp expect actual\n> > + '\n> > + done\n> > + done\n> > +done\n> > +\n>\n> Wait, but where are we actually making the $tip commit's object\n> _missing_? Hmm...\n>\n> Ah, actually the tips are just the $obj variables. So it seems like you\n> could have done\n>\n> for tip in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n>\n> in the beginning to make it more obvious.\n\nThe previous tests in this test script used \"oid\" as the variable name\nfor possibly missing objects, so I did the same in the tests I added.\nI could have used \"tip\" and \"extra_tip\" instead of \"oid\" and \"tip\" or\nperhaps \"oid\" and \"extra_oid\", but I don't think it makes a big\ndifference in understanding these tests.\n\n> Also, given how most of this is identical from the loop already in t6022\n> just above it, it would help to add a comment at the top of this one\n> explaining how it's different than the one above it.\n\nThe one I added has an extra `for tip in \"\" \"HEAD\"` loop. Anyway I\nthink I will just modify the existing test in the next version I will\nsend, as I plan to implement Junio's suggestion and there will be no\nnew option.\n\nThanks for the review!\n"},{"id":"488144","messageId":"xmqqfry4cjxt.fsf@gitster.g","threadId":"60828","inReplyTo":"owlyttmkmwaf.fsf@fine.c.googlers.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T16:34:22Z","receivedAt":"2024-02-07T16:34:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Arver writes:\n\n> IOW, make the minority (certainly not majority, I think?) of users who\n> really need the error propagation use the (new) extra flag, while the\n> rest of us (including the version of you who was surprised by the\n> limited behavior of \"--missing=...\", enough to write this series) don't\n> have to.\n\nI am skeptical that we even want that. Those who want to use the\n\"--missing\" and care about special casing missing starting points\ncan easily check with things like \"cat-file -e\" before even running\nthe \"rev-list --missing\".\n"},{"id":"488145","messageId":"CAP8UFD34a3Njx52ja05ZpSuRrORGsFu2zN7uBYprx5ABpn3r3w@mail.gmail.com","threadId":"60828","inReplyTo":"owlyttmkmwaf.fsf@fine.c.googlers.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-07T16:38:23Z","receivedAt":"2024-02-07T16:38:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Feb 7, 2024 at 10:57 AM Linus Arver wrote:\n>\n> Christian Couder writes:\n>\n> > On Thu, Feb 1, 2024 at 10:27 PM Junio C Hamano wrote:\n> >>\n> >> Christian Couder writes:\n> >>\n> >> > When such a command is used to find the dependencies of some objects,\n> >> > for example the dependencies of quarantined objects, it would be\n> >> > better if the command would instead consider such missing objects,\n> >> > especially commits, in the same way as other missing objects.\n> >> >\n> >> > If, for example `--missing=print` is used, it would be nice for some\n> >> > use cases if the missing tips passed as arguments were reported in\n> >> > the same way as other missing objects instead of the command just\n> >> > failing.\n> >> >\n> >> > Let's introduce a new `--allow-missing-tips` option to make it work\n> >> > like this.\n> >>\n> >> An obvious question is if this even needs to be a new option. What\n> >> are expected use cases where --missing=print without this option is\n> >> beneficial?\n> >\n> > I am not sure if such a case is really beneficial but some\n> > people/script/forges might rely on an error from `git rev-list\n> > --missing=print` to propagate back an error to some user interface.\n>\n> I currently learn toward just making the new flag's behavior be absorved\n> into the existing \"--missing=...\" flag.\n\nOk, then I am going to implement that in the next version I will send.\n\n> Nevertheless, you raise an\n> interesting concern.\n>\n> Perhaps a compromise would be to make \"--missing=...\" learn the new\n> behavior of this patch as Junio suggested, but to introduce a new flag,\n> something like \"--fail-on-missing-tips\" to fail early if any of the tip\n> commits' objects are missing? That way we could keep the current\n> \"strict\" behavior of complaining if we feed rev-list any tips whose\n> objects are missing. And for the vast majority of cases the\n> \"--missing=...\" flag could (intuitively) gracefully handle tips with\n> missing objects and you wouldn't have to pass in the additional flag.\n>\n> IOW, make the minority (certainly not majority, I think?) of users who\n> really need the error propagation use the (new) extra flag, while the\n> rest of us (including the version of you who was surprised by the\n> limited behavior of \"--missing=...\", enough to write this series) don't\n> have to.\n\nI agree that the majority of users would prefer `git rev-list` with\n\"--missing=\" (when is not \"error\") not to error out when\none of the tips is missing. And yeah, indeed at GitLab, we are among\nthis majority of users. I was worried about a small minority relying\non the fact that it would error out in such case. But maybe we don't\nneed to care unless it appears that this minority actually exists.\n"},{"id":"488171","messageId":"owlyr0hom257.fsf@fine.c.googlers.com","threadId":"60828","inReplyTo":"CAP8UFD3D=a-3cVNpjobOdq=F5dC+aW4qYu3fXCxTnDg=GrnSwA@mail.gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-02-07T20:48:36Z","receivedAt":"2024-02-07T20:48:38Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Christian Couder writes:\n\n> On Wed, Feb 7, 2024 at 10:40 AM Linus Arver wrote:\n>>\n>>\n>> Christian Couder writes:\n>>\n>> > In 9830926c7d (rev-list: add commit object support in `--missing`\n>> > option, 2023-10-27) we fixed the `--missing` option in `git rev-list`\n>> > so that it now works with commits too.\n>> >\n>> > Unfortunately, such a command would still fail with a \"fatal: bad\n>> > object \" if it is passed a missing commit, blob or tree as an\n>> > argument.\n>>\n>> Hmm, but according to the manpage for rev-list, it only accepts commits\n>> as arguments. Conflict...?\n>\n> It actually kind of \"works\" when you pass blobs or trees. It looks\n> like the command just doesn't use them (except for checking that they\n> actually exist) unless options like --objects, --missing=print and\n> perhaps others are used. So yeah, the doc might need an update, but I\n> think it could be a separate issue, as it's not new and doesn't depend\n> on this small series.\n\nSG. But also, if there's a way to put invisible (developer-facing, not\nuser facing) comments inside the relevant doc file it might be easy\nenough to add a to this series.\n\n>> > When such a command is used to find the dependencies of some objects,\n>> > for example the dependencies of quarantined objects, it would be\n>> > better if the command would instead consider such missing objects,\n>> > especially commits, in the same way as other missing objects.\n>>\n>> Could you define what quarantined objects are (it's not in the manpage\n>> for rev-list)?\n>\n> \"quarantined objects\" refers to the following doc:\n>\n> https://www.git-scm.com/docs/git-receive-pack#_quarantine_environment\n>\n> So to clarify things, the above paragraph looks like the following now:\n>\n> \"When such a command is used to find the dependencies of some objects,\n> for example the dependencies of quarantined objects (see the\n> \"QUARANTINE ENVIRONMENT\" section in the git-receive-pack(1)\n> documentation), it would be better if the command would instead\n> consider such missing objects, especially commits, in the same way as\n> other missing objects.\"\n\nThis reads much better, and is a good motivation to keep in the log\nmessage.\n\n>> But also, I'm not sure how much additional value this\n>> paragraph is adding on top of what you already said in the first two\n>> paragraphs.\n>\n> It's a more specific example, and it's how we detected the issue we\n> would like to address, so I think it has some value.\n\nAgreed.\n\n>> > If, for example `--missing=print` is used, it would be nice for some\n>> > use cases if the missing tips passed as arguments were reported in\n>> > the same way as other missing objects instead of the command just\n>> > failing.\n>> >\n>> > Let's introduce a new `--allow-missing-tips` option to make it work\n>> > like this.\n>>\n>> I assume \"tips\" means \"the starting commits which are passed into this\n>> command from which it begins the rev walk\". Might be worth including in\n>> the commit message.\n>\n> I think \"tips\" is better than \"commits\" because blobs and trees are\n> allowed (see above).\n\nMakes sense.\n\n>> > + oidset_iter_init(&revs.missing_commits, &iter);\n>> > + while ((oid = oidset_iter_next(&iter)))\n>> > + oidset_insert(&missing_objects, oid);\n>> > + oidset_clear(&revs.missing_commits);\n>> > + }\n>>\n>> Aside: I am surprised that there is no helper function already that\n>> simply copies things in one oidset into another oidset.\n>\n> Yeah, I was surprised too. I only found the following similar code in\n> list-objects-filter.c:\n>\n> oidset_iter_init(src, &iter);\n> while ((src_oid = oidset_iter_next(&iter)) != NULL)\n> oidset_insert(dest, src_oid);\n>\n> So yeah perhaps we could create such a helper function.\n\nPerhaps a NEEDSWORK comment is appropriate?\n\n>> > traverse_commit_list_filtered(\n>> > &revs, show_commit, show_object, &info,\n>> > (arg_print_omitted ? &omitted_objects : NULL));\n>> > diff --git a/revision.c b/revision.c\n>> > index 4c5cd7c3ce..9f25faa249 100644\n>> > --- a/revision.c\n>> > +++ b/revision.c\n>> >\n>> > [...]\n>> >\n>> > @@ -2184,7 +2189,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n>> > verify_non_filename(revs->prefix, arg);\n>> > object = get_reference(revs, arg, &oid, flags ^ local_flags);\n>> > if (!object)\n>> > - return revs->ignore_missing ? 0 : -1;\n>> > + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n>> > add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n>> > add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n>> > free(oc.path);\n>>\n>> With a few more context lines, the diff looks like\n>>\n>> --8<---------------cut here---------------start------------->8---\n>> if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))\n>> return revs->ignore_missing ? 0 : -1;\n>> if (!cant_be_filename)\n>> verify_non_filename(revs->prefix, arg);\n>> object = get_reference(revs, arg, &oid, flags ^ local_flags);\n>> if (!object)\n>> - return revs->ignore_missing ? 0 : -1;\n>> + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n>> add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n>> add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n>> free(oc.path);\n>> return 0;\n>> --8<---------------cut here---------------end--------------->8---\n>>\n>> and it's not obvious to me why you only touched the \"if (!object)\" case\n>> but not the \"if (get_oid_with_context(...))\" case. Are there any\n>> subtleties here that would not be obvious to reviewers?\n>\n> I thought that if we can't get an oid, we cannot put anything in the\n> 'missing_commit' oidset anyway, so it might be better to error out.\n\nAh, makes sense. This is a subtle detail, and perhaps worth keeping\neither as a comment (just above the \"if (get_oid_with_context(...))\"\ncase) or in the log message.\n\n>> > diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n>> > index 527aa94f07..283e8fc2c2 100755\n>> > --- a/t/t6022-rev-list-missing.sh\n>> > +++ b/t/t6022-rev-list-missing.sh\n>> > @@ -77,4 +77,55 @@ do\n>> > done\n>> > done\n>> >\n>> > +for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n>> > +do\n>> > + for tip in \"\" \"HEAD\"\n>>\n>> So far from the patch diff it was not obvious that you wanted to check\n>> for the empty string as a \"tip\".\n>\n> We want to check that things work as expected both:\n>\n> - when we pass only one missing tip, and:\n> - when we pass one missing tip and a tip that is not missing.\n\nAh, I see. I think you could add a comment like\n\n We want to check that things work as expected both:\n \n - when we pass only one missing tip (when $tip is \"\"), and:\n - when we pass one missing tip and a tip that is not missing (when\n $tip is \"HEAD\").\n\nat the top of the test case, and probably rename $obj to $missing_tip,\nand rename $tip to $existing_tip.\n\nAside: it's a bit unfortunate that the meaning of \"missing\" becomes\noverloaded slightly here because one could say '$tip == \"\"' is a\n\"missing\" tip becauuse the name is not provided. Subtle. Plus there's\nsome interplay with the \"if (get_oid_with_context(...))\" case above\nbecause we will (?) hit that path if we do pass in \"\" (empty string arg)\nas a tip to rev-list. It might be worth saying in the comments in the\nimplementation, something like\n\n The term \"missing\" here means the case where we could not find the object\n given the object_id. For example, given HEAD~1, its object id (oid)\n could be 82335b23aa7234320d6f8055243c852e4b6a5ca3. If no real object\n with this oid exists, it is considered missing. Providing an empty\n string to rev-list does not touch the \"missing tip\" codepath.\n\nI wrote the above hastily so it may need further edits to make it\nsuccinct. But the point is that this definition isn't spelled out in the\ntest case, which makes the \"\" argument for $tip that much more subtle.\n\n>> > + do\n>> > + for action in \"allow-any\" \"print\"\n>> > + do\n>> > + test_expect_success \"--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'\" '\n>>\n>> If I run this test without the new --allow-missing-tips flag, I get\n>>\n>> fatal: bad object 82335b23aa7234320d6f8055243c852e4b6a5ca3\n>> not ok 11 - --missing=allow-any --allow-missing-tips with tip 'HEAD~1' missing and tip ''\n>>\n>> and I think the last \"tip ''\" part is misleading because IIUC it's not\n>> actually passed in as a tip (see my comment below about shell quoting).\n>>\n>> > + oid=\"$(git rev-parse $obj)\" &&\n>> > + path=\".git/objects/$(test_oid_to_path $oid)\" &&\n>> > +\n>> > + # Before the object is made missing, we use rev-list to\n>> > + # get the expected oids.\n>> > + if [ \"$tip\" = \"HEAD\" ]; then\n>> > + git rev-list --objects --no-object-names \\\n>> > + HEAD ^$obj >expect.raw\n>> > + else\n>> > + >expect.raw\n>> > + fi &&\n>>\n>> OK so you are using this empty string to clear the expect.raw file. If\n>> that's true then what stops us from doing this at the start of every\n>> iteration?\n>\n> I am not sure what you mean. Both:\n>\n> git rev-list --objects --no-object-names HEAD ^$obj >expect.raw\n>\n> and:\n>\n> >expect.raw #2\n>\n> are clearing \"expect.raw\" as \">expect.raw\" is used in both cases.\n>\n> If we did the latter at the start of every iteration, then when we do\n> the former afterwards, we would clear \"expect.raw\" raw again, while\n> there is no point in clearing it twice.\n\nYes but doing it that way would separate \"set up a clean environment for\nthis test case\" from \"find the oid of the non-missing tip\" from each\nother in the same if/else stanza, which I believe helps readability.\n\n>> for the case where tip is the empty string. I wonder if we could avoid\n>> being nuanced here with subtle shell quoting rules, especially because\n>> these are tests (where making everything as explicit as possible is\n>> desirable).\n>\n> I think it's not very subtle, but perhaps a comment would help. So I\n> added the following to my current version before the loop:\n>\n> # We want to check that things work when both\n> # - all the tips passed are missing (case tip = \"\"), and\n> # - there is one missing tip and one existing tip (case tip = \"HEAD\")\n\nGreat, thanks.\n\n>> > + # When the action is to print, we should also add the missing\n>> > + # oid to the expect list.\n>> > + case $action in\n>> > + allow-any)\n>> > + ;;\n>> > + print)\n>> > + grep ?$oid actual.raw &&\n>> > + echo ?$oid >>expect.raw\n>> > + ;;\n>> > + esac &&\n>> > +\n>> > + sort actual.raw >actual &&\n>> > + sort expect.raw >expect &&\n>> > + test_cmp expect actual\n>> > + '\n>> > + done\n>> > + done\n>> > +done\n>> > +\n>>\n>> Wait, but where are we actually making the $tip commit's object\n>> _missing_? Hmm...\n>>\n>> Ah, actually the tips are just the $obj variables. So it seems like you\n>> could have done\n>>\n>> for tip in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n>>\n>> in the beginning to make it more obvious.\n>\n> The previous tests in this test script used \"oid\" as the variable name\n> for possibly missing objects, so I did the same in the tests I added.\n\nDidn't the previous tests _always_ make those objects missing?\n\n> I could have used \"tip\" and \"extra_tip\" instead of \"oid\" and \"tip\" or\n> perhaps \"oid\" and \"extra_oid\", but I don't think it makes a big\n> difference in understanding these tests.\n\nI agree that it doesn't make a big difference now (to me at least), but\nI worry for the other future Git developers who'll need to do that small\namount of mental effort to gain the same understanding as us in this\nreview process. I would still prefer the names \"missing_tip\" (or\n\"possibly_missing_tip\" if my immediate comment above is not correct) and\n\"existing_tip\" as suggested in my other comment.\n\n>> Also, given how most of this is identical from the loop already in t6022\n>> just above it, it would help to add a comment at the top of this one\n>> explaining how it's different than the one above it.\n>\n> The one I added has an extra `for tip in \"\" \"HEAD\"` loop. Anyway I\n> think I will just modify the existing test in the next version I will\n> send, as I plan to implement Junio's suggestion and there will be no\n> new option.\n\nSG.\n\n> Thanks for the review!\n\nYou're welcome.\n\nNow that I have your attention (was this my plan all along? ;) ), I will\ntake this opportunity to ping you directly for a review of my own patch\nseries for the trailers subsystem:\nhttps://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/\nwhich is in its 4th iteration after many helpful comments from Junio.\nPlease don't let the patch count (28) raise any alarms --- they used to\nbe 10 but I broke them down into smaller steps to ease the review\nprocess.\n\nThanks.\n"},{"id":"488231","messageId":"CAP8UFD148VDVto3_NwQcCkRGFZ1qHHDtYTXVxV_=VsOjgqJcbg@mail.gmail.com","threadId":"60828","inReplyTo":"owlyr0hom257.fsf@fine.c.googlers.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-08T15:03:59Z","receivedAt":"2024-02-08T15:04:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Feb 7, 2024 at 9:48 PM Linus Arver wrote:\n> Christian Couder writes:\n\n> > It actually kind of \"works\" when you pass blobs or trees. It looks\n> > like the command just doesn't use them (except for checking that they\n> > actually exist) unless options like --objects, --missing=print and\n> > perhaps others are used. So yeah, the doc might need an update, but I\n> > think it could be a separate issue, as it's not new and doesn't depend\n> > on this small series.\n>\n> SG. But also, if there's a way to put invisible (developer-facing, not\n> user facing) comments inside the relevant doc file it might be easy\n> enough to add a to this series.\n\nIt might seem easy to add/improve some doc, but there is sometimes\nbikeshedding. So I don't think making this series dependent on such a\ndco update is a good thing for both that doc update and this series. I\nwill try to work on such a doc update soon though.\n\n> > \"quarantined objects\" refers to the following doc:\n> >\n> > https://www.git-scm.com/docs/git-receive-pack#_quarantine_environment\n> >\n> > So to clarify things, the above paragraph looks like the following now:\n> >\n> > \"When such a command is used to find the dependencies of some objects,\n> > for example the dependencies of quarantined objects (see the\n> > \"QUARANTINE ENVIRONMENT\" section in the git-receive-pack(1)\n> > documentation), it would be better if the command would instead\n> > consider such missing objects, especially commits, in the same way as\n> > other missing objects.\"\n>\n> This reads much better, and is a good motivation to keep in the log\n> message.\n\nOk, I have kept it in the V2 I just sent:\n\nhttps://lore.kernel.org/git/20240208135055.2705260-1-christian.couder@gmail.com/\n\nBy the way, sorry for forgetting to use the --in-reply-to=... option\nwhen I sent it.\n\n> > Yeah, I was surprised too. I only found the following similar code in\n> > list-objects-filter.c:\n> >\n> > oidset_iter_init(src, &iter);\n> > while ((src_oid = oidset_iter_next(&iter)) != NULL)\n> > oidset_insert(dest, src_oid);\n> >\n> > So yeah perhaps we could create such a helper function.\n>\n> Perhaps a NEEDSWORK comment is appropriate?\n\nI actually added another small patch in the V2 that refactors the\nexisting code into a function in oidset.{c,h}.\n\n\n> >> With a few more context lines, the diff looks like\n> >>\n> >> --8<---------------cut here---------------start------------->8---\n> >> if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))\n> >> return revs->ignore_missing ? 0 : -1;\n> >> if (!cant_be_filename)\n> >> verify_non_filename(revs->prefix, arg);\n> >> object = get_reference(revs, arg, &oid, flags ^ local_flags);\n> >> if (!object)\n> >> - return revs->ignore_missing ? 0 : -1;\n> >> + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;\n> >> add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> >> add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n> >> free(oc.path);\n> >> return 0;\n> >> --8<---------------cut here---------------end--------------->8---\n> >>\n> >> and it's not obvious to me why you only touched the \"if (!object)\" case\n> >> but not the \"if (get_oid_with_context(...))\" case. Are there any\n> >> subtleties here that would not be obvious to reviewers?\n> >\n> > I thought that if we can't get an oid, we cannot put anything in the\n> > 'missing_commit' oidset anyway, so it might be better to error out.\n>\n> Ah, makes sense. This is a subtle detail, and perhaps worth keeping\n> either as a comment (just above the \"if (get_oid_with_context(...))\"\n> case) or in the log message.\n\nI added a code comment just above the \"if (get_oid_with_context(...))\"\ncase in the V2.\n\n\n> Ah, I see. I think you could add a comment like\n>\n> We want to check that things work as expected both:\n>\n> - when we pass only one missing tip (when $tip is \"\"), and:\n> - when we pass one missing tip and a tip that is not missing (when\n> $tip is \"HEAD\").\n>\n> at the top of the test case, and probably rename $obj to $missing_tip,\n> and rename $tip to $existing_tip.\n\nI have renamed the variables like you suggest and added a code comment.\n\n> Aside: it's a bit unfortunate that the meaning of \"missing\" becomes\n> overloaded slightly here because one could say '$tip == \"\"' is a\n> \"missing\" tip becauuse the name is not provided. Subtle. Plus there's\n> some interplay with the \"if (get_oid_with_context(...))\" case above\n> because we will (?) hit that path if we do pass in \"\" (empty string arg)\n> as a tip to rev-list. It might be worth saying in the comments in the\n> implementation, something like\n>\n> The term \"missing\" here means the case where we could not find the object\n> given the object_id. For example, given HEAD~1, its object id (oid)\n> could be 82335b23aa7234320d6f8055243c852e4b6a5ca3. If no real object\n> with this oid exists, it is considered missing. Providing an empty\n> string to rev-list does not touch the \"missing tip\" codepath.\n>\n> I wrote the above hastily so it may need further edits to make it\n> succinct. But the point is that this definition isn't spelled out in the\n> test case, which makes the \"\" argument for $tip that much more subtle.\n\nI think this is related to the --missing option in general (which has\nbeen with us for around 6 years already), not specifically to this\nseries, so I think it can be done separately.\n\n> >> OK so you are using this empty string to clear the expect.raw file. If\n> >> that's true then what stops us from doing this at the start of every\n> >> iteration?\n> >\n> > I am not sure what you mean. Both:\n> >\n> > git rev-list --objects --no-object-names HEAD ^$obj >expect.raw\n> >\n> > and:\n> >\n> > >expect.raw #2\n> >\n> > are clearing \"expect.raw\" as \">expect.raw\" is used in both cases.\n> >\n> > If we did the latter at the start of every iteration, then when we do\n> > the former afterwards, we would clear \"expect.raw\" raw again, while\n> > there is no point in clearing it twice.\n>\n> Yes but doing it that way would separate \"set up a clean environment for\n> this test case\" from \"find the oid of the non-missing tip\" from each\n> other in the same if/else stanza, which I believe helps readability.\n\nOn one hand it can help readability, but on other hand some people\ninterested in test performance might see it as some waste. So I prefer\nnot to do it for now.\n\n> >> Also, given how most of this is identical from the loop already in t6022\n> >> just above it, it would help to add a comment at the top of this one\n> >> explaining how it's different than the one above it.\n> >\n> > The one I added has an extra `for tip in \"\" \"HEAD\"` loop. Anyway I\n> > think I will just modify the existing test in the next version I will\n> > send, as I plan to implement Junio's suggestion and there will be no\n> > new option.\n\nActually I changed my mind and didn't modify the existing test, but I\nrenamed the variables as you suggested.\n\n> Now that I have your attention (was this my plan all along? ;) ), I will\n> take this opportunity to ping you directly for a review of my own patch\n> series for the trailers subsystem:\n> https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/\n> which is in its 4th iteration after many helpful comments from Junio.\n> Please don't let the patch count (28) raise any alarms --- they used to\n> be 10 but I broke them down into smaller steps to ease the review\n> process.\n\nI will try to take a look soon. Thanks for working on improving the\ntrailers subsystem!\n"},{"id":"488255","messageId":"owlycyt6d6ws.fsf@fine.c.googlers.com","threadId":"60828","inReplyTo":"CAP8UFD148VDVto3_NwQcCkRGFZ1qHHDtYTXVxV_=VsOjgqJcbg@mail.gmail.com","subject":"Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-02-08T20:42:43Z","receivedAt":"2024-02-08T20:42:46Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Christian Couder writes:\n\n> On Wed, Feb 7, 2024 at 9:48 PM Linus Arver wrote:\n>> Christian Couder writes:\n>\n>> > It actually kind of \"works\" when you pass blobs or trees. It looks\n>> > like the command just doesn't use them (except for checking that they\n>> > actually exist) unless options like --objects, --missing=print and\n>> > perhaps others are used. So yeah, the doc might need an update, but I\n>> > think it could be a separate issue, as it's not new and doesn't depend\n>> > on this small series.\n>>\n>> SG. But also, if there's a way to put invisible (developer-facing, not\n>> user facing) comments inside the relevant doc file it might be easy\n>> enough to add a to this series.\n>\n> It might seem easy to add/improve some doc, but there is sometimes\n> bikeshedding. So I don't think making this series dependent on such a\n> dco update is a good thing for both that doc update and this series. I\n> will try to work on such a doc update soon though.\n\nOh, I did not mean to say you should add doc updates in this series at\nall. I was just asking if you could add a comment with \"NEEDSWORK\" or\nsimilar that only developers will see. Looks like asciidoc does support\nsuch comments that are left out of the published text:\nhttps://docs.asciidoctor.org/asciidoc/latest/comments/. I just tested\nwith \"//\" style comments (asciidoc has other styles also, not sure which\none is preferred in our codebase) and running \"make\" to generate the\ndocs confirm that we can indeed put in such developer-facing comments.\n\nBack to this topic, you could add something like \"// NEEDSWORK: rev-list\ncan take blob and tree objects also as arguments, not just commits\".\nThis is why I thought it would be easy.\n\n>> > \"quarantined objects\" refers to the following doc:\n>> >\n>> > https://www.git-scm.com/docs/git-receive-pack#_quarantine_environment\n>> >\n>> > So to clarify things, the above paragraph looks like the following now:\n>> >\n>> > \"When such a command is used to find the dependencies of some objects,\n>> > for example the dependencies of quarantined objects (see the\n>> > \"QUARANTINE ENVIRONMENT\" section in the git-receive-pack(1)\n>> > documentation), it would be better if the command would instead\n>> > consider such missing objects, especially commits, in the same way as\n>> > other missing objects.\"\n>>\n>> This reads much better, and is a good motivation to keep in the log\n>> message.\n>\n> Ok, I have kept it in the V2 I just sent:\n>\n> https://lore.kernel.org/git/20240208135055.2705260-1-christian.couder@gmail.com/\n>\n> By the way, sorry for forgetting to use the --in-reply-to=... option\n> when I sent it.\n\nNP, thanks for the heads up. See you in the new thread.\n\n>> Aside: it's a bit unfortunate that the meaning of \"missing\" becomes\n>> overloaded slightly here because one could say '$tip == \"\"' is a\n>> \"missing\" tip becauuse the name is not provided. Subtle. Plus there's\n>> some interplay with the \"if (get_oid_with_context(...))\" case above\n>> because we will (?) hit that path if we do pass in \"\" (empty string arg)\n>> as a tip to rev-list. It might be worth saying in the comments in the\n>> implementation, something like\n>>\n>> The term \"missing\" here means the case where we could not find the object\n>> given the object_id. For example, given HEAD~1, its object id (oid)\n>> could be 82335b23aa7234320d6f8055243c852e4b6a5ca3. If no real object\n>> with this oid exists, it is considered missing. Providing an empty\n>> string to rev-list does not touch the \"missing tip\" codepath.\n>>\n>> I wrote the above hastily so it may need further edits to make it\n>> succinct. But the point is that this definition isn't spelled out in the\n>> test case, which makes the \"\" argument for $tip that much more subtle.\n>\n> I think this is related to the --missing option in general (which has\n> been with us for around 6 years already), not specifically to this\n> series, so I think it can be done separately.\n\nAh, thanks for the added context. I agree.\n\n>> >> OK so you are using this empty string to clear the expect.raw file. If\n>> >> that's true then what stops us from doing this at the start of every\n>> >> iteration?\n>> >\n>> > I am not sure what you mean. Both:\n>> >\n>> > git rev-list --objects --no-object-names HEAD ^$obj >expect.raw\n>> >\n>> > and:\n>> >\n>> > >expect.raw #2\n>> >\n>> > are clearing \"expect.raw\" as \">expect.raw\" is used in both cases.\n>> >\n>> > If we did the latter at the start of every iteration, then when we do\n>> > the former afterwards, we would clear \"expect.raw\" raw again, while\n>> > there is no point in clearing it twice.\n>>\n>> Yes but doing it that way would separate \"set up a clean environment for\n>> this test case\" from \"find the oid of the non-missing tip\" from each\n>> other in the same if/else stanza, which I believe helps readability.\n>\n> On one hand it can help readability, but on other hand some people\n> interested in test performance might see it as some waste. So I prefer\n> not to do it for now.\n\nIn most situations I would agree with \"let's hurt test performance by\n1%, in exchange for better readability\". I think this is one such\nsituation. And also, it's not even clear if we can measure the\nperformance hit from an extra \">expect.raw\" call.\n\nBut as this is a small nit, I won't pursue this suggestion further.\n\n>> Now that I have your attention (was this my plan all along? ;) ), I will\n>> take this opportunity to ping you directly for a review of my own patch\n>> series for the trailers subsystem:\n>> https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/\n>> which is in its 4th iteration after many helpful comments from Junio.\n>> Please don't let the patch count (28) raise any alarms --- they used to\n>> be 10 but I broke them down into smaller steps to ease the review\n>> process.\n>\n> I will try to take a look soon. Thanks for working on improving the\n> trailers subsystem!\n\nThanks!\n"}]}