{"thread":{"id":"60211","subject":"[PATCH] revision: add `--ignore-missing-links` user option","startedAt":"2023-09-08T17:42:26Z","lastAt":"2023-09-27T16:27:25Z","messageCount":20,"participants":["Karthik Nayak","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481566","messageId":"20230908174208.249184-1-karthik.188@gmail.com","threadId":"60211","inReplyTo":null,"subject":"[PATCH] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-08T17:42:07Z","receivedAt":"2023-09-08T17:42:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The revision backend is used by multiple porcelain commands such as\ngit-rev-list(1) and git-log(1). The backend currently supports ignoring\nmissing links by setting the `ignore_missing_links` bit. This allows the\nrevision walk to skip any objects links which are missing.\n\nCurrently there is no way to use git-rev-list(1) to traverse the objects\nof the main object directory (GIT_OBJECT_DIRECTORY) and print the\nboundary objects when moving from the main object directory to the\nalternate object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES).\n\nBy exposing this new flag `--ignore-missing-links`, users can set the\nrequired env variables (GIT_OBJECT_DIRECTORY and\nGIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to\nfind the boundary objects between object directories.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/rev-list-options.txt | 5 ++++\n revision.c | 2 ++\n t/t6022-rev-list-alternates.sh | 43 ++++++++++++++++++++++++++++++\n 3 files changed, 50 insertions(+)\n create mode 100755 t/t6022-rev-list-alternates.sh\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a4a0cb93b2..a0b48db8a8 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -227,6 +227,11 @@ explicitly.\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\n \n+--ignore-missing-links::\n+\tWhen an object points to another object that is missing, pretend as if the\n+\tlink did not exist. These missing links are not written to stdout unless\n+\tthe --boundary flag is passed.\n+\n ifndef::git-rev-list[]\n --bisect::\n \tPretend as if the bad bisection ref `refs/bisect/bad`\ndiff --git a/revision.c b/revision.c\nindex 2f4c53ea20..cbfcbf6e28 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n+\t} else if (!strcmp(arg, \"--ignore-missing-links\")) {\n+\t\trevs->ignore_missing_links = 1;\n \t} else if (opt && opt->allow_exclude_promisor_objects &&\n \t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\ndiff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh\nnew file mode 100755\nindex 0000000000..626ebb2dce\n--- /dev/null\n+++ b/t/t6022-rev-list-alternates.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='handling of alternates in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We create 5 commits and move them to the alt directory and\n+# create 5 more commits which will stay in the main odb.\n+test_expect_success 'create repository and alternate directory' '\n+\tgit init main &&\n+\ttest_commit_bulk -C main 5 &&\n+\tmkdir alt &&\n+\tmv main/.git/objects/* alt &&\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5\n+'\n+\n+# When the alternate odb is provided, all commits are listed.\n+test_expect_success 'rev-list passes with alternate object directory' '\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_stdout_line_count = 10 git -C main rev-list HEAD\n+'\n+\n+# When the alternate odb is not provided, rev-list fails since the 5th commit's\n+# parent is not present in the main odb.\n+test_expect_success 'rev-list fails without alternate object directory' '\n+\ttest_must_fail git -C main rev-list HEAD\n+'\n+\n+# With `--ignore-missing-links`, we stop the traversal when we encounter a\n+# missing link.\n+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n+\ttest_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD\n+'\n+\n+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n+# commits.\n+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n+\tgit -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&\n+\ttest_stdout_line_count = 6 cat list-output &&\n+\ttest_stdout_line_count = 1 cat list-output | grep \"^-\"\n+'\n+\n+test_done\n-- \n2.41.0\n\n"},{"id":"481573","messageId":"xmqqwmx0sca3.fsf@gitster.g","threadId":"60211","inReplyTo":"20230908174208.249184-1-karthik.188@gmail.com","subject":"Re: [PATCH] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-08T19:19:48Z","receivedAt":"2023-09-08T19:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The revision backend is used by multiple porcelain commands such as\n> git-rev-list(1) and git-log(1). The backend currently supports ignoring\n> missing links by setting the `ignore_missing_links` bit. This allows the\n> revision walk to skip any objects links which are missing.\n\n> Currently there is no way to use git-rev-list(1) to traverse the objects\n> of the main object directory (GIT_OBJECT_DIRECTORY) and print the\n> boundary objects when moving from the main object directory to the\n> alternate object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES).\n\nThe above description needs tightened up a bit, I think.\n\nWhat is left unsaid is that you arranged a repository to borrow from\nan alternate object directory (or two), and plan to walk objects\nwith this bit on in the repository, while leaving the alternates\ndisabled. Without stating that you plan to disable the alternates\nwhile this mode of operation happens, nothing would happen when the\ntraversal goes from the main to the alternate because no links are\nbroken, no?\n\n> By exposing this new flag `--ignore-missing-links`, users can set the\n> required env variables (GIT_OBJECT_DIRECTORY and\n> GIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to\n> find the boundary objects between object directories.\n\nThis command being a plumbing, there is not much reason to object to\nsurfacing features that already internally exist to the command line\noption. Having said that, \n\n * Suppose your traversal with --ignore-missing-links from the tip\n of a branch reaches a tree object A, and the tree object A has a\n link to a blob B and a blob C. But B is in a separate object\n store that you usually access via the alternate mechanism.\n Instead of barfing \"The repository is corrupt---object A points\n at object B that does not exist\", we pretend that A does not have\n the link to B and keep traversing, discovering C and other\n objects.\n\n That much we can read from the above and also the documentation\n part of the patch. The interaction with --boundary needs to be\n clarified in this description and the documentation, though. It\n is unclear if you show 'A' or 'B' in this scenario.\n\n * Some traversals use the ignore-missing-links bit implicitly and\n currently there is no way to turn it off. Is it plausible that\n user may want to explicitly toggle it off, with the option\n negated, i.e. --no-ignore-missing-links? I do not immediately\n see the utility of such an option, but that is only due to my\n lack of imagination. For now, I think it makes sense not to\n allow negating this option, until somebody comes up with a useful\n use case.\n\n> +--ignore-missing-links::\n> +\tWhen an object points to another object that is missing, pretend as if the\n> +\tlink did not exist. These missing links are not written to stdout unless\n> +\tthe --boundary flag is passed.\n\nDoes \"git rev-list\" ever writes \"links\"? I thought not. \n\n\"These missing objects are not written\" would be more sensible, but\nwe never write missing objects with or without the option, so it\nis not even worth saying.\n\nWhen \"--boundary\" is passed, do they appear as if they are\navailable? If not, then the above description is very misleading.\n\n During traversal, if an object that is referenced does not\n exist, pretend as if the reference itself does not exist,\n instead of dying of a repository corruption. Running the\n command with the \"--boundary\" option makes these missing\n objects, together with the objects on the edge of revision\n ranges (i.e. true boundary objects), appear on the output,\n prefixed with '-'.\n\nor something like that, perhaps?\n\n> +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n> +# missing link.\n> +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n> +\ttest_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD\n> +'\n> +\n> +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n> +# commits.\n> +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n> +\tgit -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&\n> +\ttest_stdout_line_count = 6 cat list-output &&\n> +\ttest_stdout_line_count = 1 cat list-output | grep \"^-\"\n> +'\n\nThese tests are way too loose. Not only you want to see certain\nnumber of boundary objects, you _know_ exactly which object should\nbe on the boundary, and you should check that instead. That will\nallow you to find a mistake to write commit 'A' that refers to a\nmissing commit 'B', when they wanted to write the missing comit 'B',\nas a boundary object, for example.\n\nThanks.\n\n"},{"id":"481766","messageId":"CAOLa=ZT25MB4c4ag3wRruipsxxBdX-+ovr+Eq3uMDuQJvJyVeg@mail.gmail.com","threadId":"60211","inReplyTo":"xmqqwmx0sca3.fsf@gitster.g","subject":"Re: [PATCH] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-12T14:42:55Z","receivedAt":"2023-09-12T14:43:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Sep 8, 2023 at 9:19 PM Junio C Hamano wrote:\n> The above description needs tightened up a bit, I think.\n>\n> What is left unsaid is that you arranged a repository to borrow from\n> an alternate object directory (or two), and plan to walk objects\n> with this bit on in the repository, while leaving the alternates\n> disabled. Without stating that you plan to disable the alternates\n> while this mode of operation happens, nothing would happen when the\n> traversal goes from the main to the alternate because no links are\n> broken, no?\n>\n\nFair enough, I agree with your points. I'll amend the message to highlight this\nscenario.\n\n> > By exposing this new flag `--ignore-missing-links`, users can set the\n> > required env variables (GIT_OBJECT_DIRECTORY and\n> > GIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to\n> > find the boundary objects between object directories.\n>\n> This command being a plumbing, there is not much reason to object to\n> surfacing features that already internally exist to the command line\n> option. Having said that,\n>\n> * Suppose your traversal with --ignore-missing-links from the tip\n> of a branch reaches a tree object A, and the tree object A has a\n> link to a blob B and a blob C. But B is in a separate object\n> store that you usually access via the alternate mechanism.\n> Instead of barfing \"The repository is corrupt---object A points\n> at object B that does not exist\", we pretend that A does not have\n> the link to B and keep traversing, discovering C and other\n> objects.\n>\n> That much we can read from the above and also the documentation\n> part of the patch. The interaction with --boundary needs to be\n> clarified in this description and the documentation, though. It\n> is unclear if you show 'A' or 'B' in this scenario.\n\nDo note that the `--boundary` option only works with commits. Keeping this in\nmind `--ignore-missing-links` when used with `--boundary` doesn't even traverse\nnon-commit objects. Which means trees/blobs being corrupted shouldn't matter.\n\nBut I did realize that `--ignore-missing-links` as this patch stands\nis broken when\nused alongside the `--objects` flag (`--boundary` doesn't work with\n`--objects` at the\nmoment, this is something I plan to tackle soon after with a\n`--boundary-objects` flag).\nThe second version of this patch will have a fix to ensure that even\nnon-commit objects\nare ignored during traversal if `--objects` option is used.\n\n>\n> * Some traversals use the ignore-missing-links bit implicitly and\n> currently there is no way to turn it off. Is it plausible that\n> user may want to explicitly toggle it off, with the option\n> negated, i.e. --no-ignore-missing-links? I do not immediately\n> see the utility of such an option, but that is only due to my\n> lack of imagination. For now, I think it makes sense not to\n> allow negating this option, until somebody comes up with a useful\n> use case.\n>\n\nAgreed!\n\n> > +--ignore-missing-links::\n> > + When an object points to another object that is missing, pretend as if the\n> > + link did not exist. These missing links are not written to stdout unless\n> > + the --boundary flag is passed.\n>\n> Does \"git rev-list\" ever writes \"links\"? I thought not.\n>\n> \"These missing objects are not written\" would be more sensible, but\n> we never write missing objects with or without the option, so it\n> is not even worth saying.\n>\n> When \"--boundary\" is passed, do they appear as if they are\n> available? If not, then the above description is very misleading.\n>\n> During traversal, if an object that is referenced does not\n> exist, pretend as if the reference itself does not exist,\n> instead of dying of a repository corruption. Running the\n> command with the \"--boundary\" option makes these missing\n> objects, together with the objects on the edge of revision\n> ranges (i.e. true boundary objects), appear on the output,\n> prefixed with '-'.\n>\n> or something like that, perhaps?\n>\n\nThis indeed is better, I've copied and modified it as needed.\n\n> > +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n> > +# missing link.\n> > +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n> > + test_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD\n> > +'\n> > +\n> > +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n> > +# commits.\n> > +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n> > + git -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&\n> > + test_stdout_line_count = 6 cat list-output &&\n> > + test_stdout_line_count = 1 cat list-output | grep \"^-\"\n> > +'\n>\n> These tests are way too loose. Not only you want to see certain\n> number of boundary objects, you _know_ exactly which object should\n> be on the boundary, and you should check that instead. That will\n> allow you to find a mistake to write commit 'A' that refers to a\n> missing commit 'B', when they wanted to write the missing comit 'B',\n> as a boundary object, for example.\n>\n\nFair enough, I will make them more specific and add some tests for\nmissing trees/blobs.\n\n> Thanks.\n\nThank you for the review. Will send the next version of the patch soon :)\n"},{"id":"481769","messageId":"20230912155820.136111-1-karthik.188@gmail.com","threadId":"60211","inReplyTo":"20230908174208.249184-1-karthik.188@gmail.com","subject":"[PATCH v2] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-12T15:58:20Z","receivedAt":"2023-09-12T15:58:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The revision backend is used by multiple porcelain commands such as\ngit-rev-list(1) and git-log(1). The backend currently supports ignoring\nmissing links by setting the `ignore_missing_links` bit. This allows the\nrevision walk to skip any objects links which are missing. Expose this\nbit via an `--ignore-missing-links` user option.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, enabling this option along with the `--boundary` option for\nwhile disabling the alternate object directory allows us to find the\nboundary objects between the main and alternate object directory.\n\nSigned-off-by: Karthik Nayak \n---\n\nChanges from v1:\n1. Changes in the commit message and option description to be more specific\nand list why and what the changes are.\n2. Ensure the new option also works with the existing `--objects` options.\n3. More specific testing for boundary commit.\n\nRange diff against v1:\n\n1: c0a4dca9b0 ! 1: e3f4d85732 revision: add `--ignore-missing-links` user option\n @@ Commit message\n The revision backend is used by multiple porcelain commands such as\n git-rev-list(1) and git-log(1). The backend currently supports ignoring\n missing links by setting the `ignore_missing_links` bit. This allows the\n - revision walk to skip any objects links which are missing.\n + revision walk to skip any objects links which are missing. Expose this\n + bit via an `--ignore-missing-links` user option.\n \n - Currently there is no way to use git-rev-list(1) to traverse the objects\n - of the main object directory (GIT_OBJECT_DIRECTORY) and print the\n - boundary objects when moving from the main object directory to the\n - alternate object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES).\n -\n - By exposing this new flag `--ignore-missing-links`, users can set the\n - required env variables (GIT_OBJECT_DIRECTORY and\n - GIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to\n - find the boundary objects between object directories.\n + A scenario where this option would be used is to find the boundary\n + objects between different object directories. Consider a repository with\n + a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\n + object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\n + repository, enabling this option along with the `--boundary` option for\n + while disabling the alternate object directory allows us to find the\n + boundary objects between the main and alternate object directory.\n \n Signed-off-by: Karthik Nayak \n \n @@ Documentation/rev-list-options.txt: explicitly.\n \tthe bad input was not given.\n \n +--ignore-missing-links::\n -+\tWhen an object points to another object that is missing, pretend as if the\n -+\tlink did not exist. These missing links are not written to stdout unless\n -+\tthe --boundary flag is passed.\n ++\tDuring traversal, if an object that is referenced does not\n ++\texist, instead of dying of a repository corruption, pretend as\n ++\tif the reference itself does not exist. Running the command\n ++\twith the `--boundary` option makes these missing commits,\n ++\ttogether with the commits on the edge of revision ranges\n ++\t(i.e. true boundary objects), appear on the output, prefixed\n ++\twith '-'.\n +\n ifndef::git-rev-list[]\n --bisect::\n \tPretend as if the bad bisection ref `refs/bisect/bad`\n \n + ## builtin/rev-list.c ##\n +@@ builtin/rev-list.c: static int finish_object(struct object *obj, const char *name UNUSED,\n + {\n + \tstruct rev_list_info *info = cb_data;\n + \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n +-\t\tfinish_object__ma(obj);\n ++\t\tif (!info->revs->ignore_missing_links)\n ++\t\t\tfinish_object__ma(obj);\n + \t\treturn 1;\n + \t}\n + \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n +\n ## revision.c ##\n @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n @@ t/t6022-rev-list-alternates.sh (new)\n +test_expect_success 'create repository and alternate directory' '\n +\tgit init main &&\n +\ttest_commit_bulk -C main 5 &&\n ++\tBOUNDARY_COMMIT=$(git -C main rev-parse HEAD) &&\n +\tmkdir alt &&\n +\tmv main/.git/objects/* alt &&\n +\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5\n +'\n +\n -+# When the alternate odb is provided, all commits are listed.\n ++# when the alternate odb is provided, all commits are listed along with the boundary\n ++# commit.\n +test_expect_success 'rev-list passes with alternate object directory' '\n -+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_stdout_line_count = 10 git -C main rev-list HEAD\n ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n ++\ttest_stdout_line_count = 10 cat actual &&\n ++\tgrep $BOUNDARY_COMMIT actual\n +'\n +\n +# When the alternate odb is not provided, rev-list fails since the 5th commit's\n @@ t/t6022-rev-list-alternates.sh (new)\n +'\n +\n +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n -+# missing link.\n ++# missing link. The boundary commit is not listed as we haven't used the\n ++# `--boundary` options.\n +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n -+\ttest_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD\n ++\tgit -C main rev-list --ignore-missing-links HEAD >actual &&\n ++\ttest_stdout_line_count = 5 cat actual &&\n ++\t! grep -$BOUNDARY_COMMIT actual\n +'\n +\n +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n +# commits.\n +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n -+\tgit -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&\n -+\ttest_stdout_line_count = 6 cat list-output &&\n -+\ttest_stdout_line_count = 1 cat list-output | grep \"^-\"\n ++\tgit -C main rev-list --ignore-missing-links --boundary HEAD >actual &&\n ++\ttest_stdout_line_count = 6 cat actual &&\n ++\tgrep -$BOUNDARY_COMMIT actual\n ++'\n ++\n ++# The `--ignore-missing-links` option should ensure that git-rev-list(1) doesn't\n ++# fail when used alongside `--objects` when a tree is missing.\n ++test_expect_success 'rev-list --ignore-missing-links works with missing tree' '\n ++\techo \"foo\" >main/file &&\n ++\tgit -C main add file &&\n ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 11\" &&\n ++\tTREE_OID=$(git -C main rev-parse HEAD^{tree}) &&\n ++\tmkdir alt/${TREE_OID:0:2} &&\n ++\tmv main/.git/objects/${TREE_OID:0:2}/${TREE_OID:2} alt/${TREE_OID:0:2}/ &&\n ++\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n ++\t! grep $TREE_OID actual\n ++'\n ++\n ++# Similar to above, it should also work when a blob is missing.\n ++test_expect_success 'rev-list --ignore-missing-links works with missing blob' '\n ++\techo \"bar\" >main/file &&\n ++\tgit -C main add file &&\n ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 12\" &&\n ++\tBLOB_OID=$(git -C main rev-parse HEAD:file) &&\n ++\tmkdir alt/${BLOB_OID:0:2} &&\n ++\tmv main/.git/objects/${BLOB_OID:0:2}/${BLOB_OID:2} alt/${BLOB_OID:0:2}/ &&\n ++\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n ++\t! grep $BLOB_OID actual\n +'\n +\n +test_done\n\n\n Documentation/rev-list-options.txt | 9 ++++\n builtin/rev-list.c | 3 +-\n revision.c | 2 +\n t/t6022-rev-list-alternates.sh | 75 ++++++++++++++++++++++++++++++\n 4 files changed, 88 insertions(+), 1 deletion(-)\n create mode 100755 t/t6022-rev-list-alternates.sh\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a4a0cb93b2..8ee713db3d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -227,6 +227,15 @@ explicitly.\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\n \n+--ignore-missing-links::\n+\tDuring traversal, if an object that is referenced does not\n+\texist, instead of dying of a repository corruption, pretend as\n+\tif the reference itself does not exist. Running the command\n+\twith the `--boundary` option makes these missing commits,\n+\ttogether with the commits on the edge of revision ranges\n+\t(i.e. true boundary objects), appear on the output, prefixed\n+\twith '-'.\n+\n ifndef::git-rev-list[]\n --bisect::\n \tPretend as if the bad bisection ref `refs/bisect/bad`\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..5239d83c76 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,\n {\n \tstruct rev_list_info *info = cb_data;\n \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n-\t\tfinish_object__ma(obj);\n+\t\tif (!info->revs->ignore_missing_links)\n+\t\t\tfinish_object__ma(obj);\n \t\treturn 1;\n \t}\n \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\ndiff --git a/revision.c b/revision.c\nindex 2f4c53ea20..cbfcbf6e28 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n+\t} else if (!strcmp(arg, \"--ignore-missing-links\")) {\n+\t\trevs->ignore_missing_links = 1;\n \t} else if (opt && opt->allow_exclude_promisor_objects &&\n \t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\ndiff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh\nnew file mode 100755\nindex 0000000000..08d9ffde5f\n--- /dev/null\n+++ b/t/t6022-rev-list-alternates.sh\n@@ -0,0 +1,75 @@\n+#!/bin/sh\n+\n+test_description='handling of alternates in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We create 5 commits and move them to the alt directory and\n+# create 5 more commits which will stay in the main odb.\n+test_expect_success 'create repository and alternate directory' '\n+\tgit init main &&\n+\ttest_commit_bulk -C main 5 &&\n+\tBOUNDARY_COMMIT=$(git -C main rev-parse HEAD) &&\n+\tmkdir alt &&\n+\tmv main/.git/objects/* alt &&\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5\n+'\n+\n+# when the alternate odb is provided, all commits are listed along with the boundary\n+# commit.\n+test_expect_success 'rev-list passes with alternate object directory' '\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n+\ttest_stdout_line_count = 10 cat actual &&\n+\tgrep $BOUNDARY_COMMIT actual\n+'\n+\n+# When the alternate odb is not provided, rev-list fails since the 5th commit's\n+# parent is not present in the main odb.\n+test_expect_success 'rev-list fails without alternate object directory' '\n+\ttest_must_fail git -C main rev-list HEAD\n+'\n+\n+# With `--ignore-missing-links`, we stop the traversal when we encounter a\n+# missing link. The boundary commit is not listed as we haven't used the\n+# `--boundary` options.\n+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n+\tgit -C main rev-list --ignore-missing-links HEAD >actual &&\n+\ttest_stdout_line_count = 5 cat actual &&\n+\t! grep -$BOUNDARY_COMMIT actual\n+'\n+\n+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n+# commits.\n+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n+\tgit -C main rev-list --ignore-missing-links --boundary HEAD >actual &&\n+\ttest_stdout_line_count = 6 cat actual &&\n+\tgrep -$BOUNDARY_COMMIT actual\n+'\n+\n+# The `--ignore-missing-links` option should ensure that git-rev-list(1) doesn't\n+# fail when used alongside `--objects` when a tree is missing.\n+test_expect_success 'rev-list --ignore-missing-links works with missing tree' '\n+\techo \"foo\" >main/file &&\n+\tgit -C main add file &&\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 11\" &&\n+\tTREE_OID=$(git -C main rev-parse HEAD^{tree}) &&\n+\tmkdir alt/${TREE_OID:0:2} &&\n+\tmv main/.git/objects/${TREE_OID:0:2}/${TREE_OID:2} alt/${TREE_OID:0:2}/ &&\n+\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n+\t! grep $TREE_OID actual\n+'\n+\n+# Similar to above, it should also work when a blob is missing.\n+test_expect_success 'rev-list --ignore-missing-links works with missing blob' '\n+\techo \"bar\" >main/file &&\n+\tgit -C main add file &&\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 12\" &&\n+\tBLOB_OID=$(git -C main rev-parse HEAD:file) &&\n+\tmkdir alt/${BLOB_OID:0:2} &&\n+\tmv main/.git/objects/${BLOB_OID:0:2}/${BLOB_OID:2} alt/${BLOB_OID:0:2}/ &&\n+\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n+\t! grep $BLOB_OID actual\n+'\n+\n+test_done\n-- \n2.41.0\n\n"},{"id":"481775","messageId":"ZQCa3GHT3D7aibJ1@nand.local","threadId":"60211","inReplyTo":"20230912155820.136111-1-karthik.188@gmail.com","subject":"Re: [PATCH v2] revision: add `--ignore-missing-links` user option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-12T17:07:40Z","receivedAt":"2023-09-12T17:07:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 12, 2023 at 05:58:20PM +0200, Karthik Nayak wrote:\n> The revision backend is used by multiple porcelain commands such as\n> git-rev-list(1) and git-log(1). The backend currently supports ignoring\n> missing links by setting the `ignore_missing_links` bit. This allows the\n> revision walk to skip any objects links which are missing. Expose this\n> bit via an `--ignore-missing-links` user option.\n>\n> A scenario where this option would be used is to find the boundary\n> objects between different object directories. Consider a repository with\n> a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\n> object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\n> repository, enabling this option along with the `--boundary` option for\n> while disabling the alternate object directory allows us to find the\n> boundary objects between the main and alternate object directory.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n>\n> Changes from v1:\n> 1. Changes in the commit message and option description to be more specific\n> and list why and what the changes are.\n> 2. Ensure the new option also works with the existing `--objects` options.\n> 3. More specific testing for boundary commit.\n>\n> Range diff against v1:\n>\n> 1: c0a4dca9b0 ! 1: e3f4d85732 revision: add `--ignore-missing-links` user option\n> @@ Commit message\n> The revision backend is used by multiple porcelain commands such as\n> git-rev-list(1) and git-log(1). The backend currently supports ignoring\n> missing links by setting the `ignore_missing_links` bit. This allows the\n> - revision walk to skip any objects links which are missing.\n> + revision walk to skip any objects links which are missing. Expose this\n> + bit via an `--ignore-missing-links` user option.\n>\n> - Currently there is no way to use git-rev-list(1) to traverse the objects\n> - of the main object directory (GIT_OBJECT_DIRECTORY) and print the\n> - boundary objects when moving from the main object directory to the\n> - alternate object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES).\n> -\n> - By exposing this new flag `--ignore-missing-links`, users can set the\n> - required env variables (GIT_OBJECT_DIRECTORY and\n> - GIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to\n> - find the boundary objects between object directories.\n> + A scenario where this option would be used is to find the boundary\n> + objects between different object directories. Consider a repository with\n> + a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\n> + object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\n> + repository, enabling this option along with the `--boundary` option for\n> + while disabling the alternate object directory allows us to find the\n> + boundary objects between the main and alternate object directory.\n>\n> Signed-off-by: Karthik Nayak \n>\n> @@ Documentation/rev-list-options.txt: explicitly.\n> \tthe bad input was not given.\n>\n> +--ignore-missing-links::\n> -+\tWhen an object points to another object that is missing, pretend as if the\n> -+\tlink did not exist. These missing links are not written to stdout unless\n> -+\tthe --boundary flag is passed.\n> ++\tDuring traversal, if an object that is referenced does not\n> ++\texist, instead of dying of a repository corruption, pretend as\n> ++\tif the reference itself does not exist. Running the command\n> ++\twith the `--boundary` option makes these missing commits,\n> ++\ttogether with the commits on the edge of revision ranges\n> ++\t(i.e. true boundary objects), appear on the output, prefixed\n> ++\twith '-'.\n> +\n> ifndef::git-rev-list[]\n> --bisect::\n> \tPretend as if the bad bisection ref `refs/bisect/bad`\n>\n> + ## builtin/rev-list.c ##\n> +@@ builtin/rev-list.c: static int finish_object(struct object *obj, const char *name UNUSED,\n> + {\n> + \tstruct rev_list_info *info = cb_data;\n> + \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n> +-\t\tfinish_object__ma(obj);\n> ++\t\tif (!info->revs->ignore_missing_links)\n> ++\t\t\tfinish_object__ma(obj);\n> + \t\treturn 1;\n> + \t}\n> + \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n> +\n> ## revision.c ##\n> @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t\trevs->limited = 1;\n> @@ t/t6022-rev-list-alternates.sh (new)\n> +test_expect_success 'create repository and alternate directory' '\n> +\tgit init main &&\n> +\ttest_commit_bulk -C main 5 &&\n> ++\tBOUNDARY_COMMIT=$(git -C main rev-parse HEAD) &&\n> +\tmkdir alt &&\n> +\tmv main/.git/objects/* alt &&\n> +\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5\n> +'\n> +\n> -+# When the alternate odb is provided, all commits are listed.\n> ++# when the alternate odb is provided, all commits are listed along with the boundary\n> ++# commit.\n> +test_expect_success 'rev-list passes with alternate object directory' '\n> -+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_stdout_line_count = 10 git -C main rev-list HEAD\n> ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n> ++\ttest_stdout_line_count = 10 cat actual &&\n> ++\tgrep $BOUNDARY_COMMIT actual\n> +'\n> +\n> +# When the alternate odb is not provided, rev-list fails since the 5th commit's\n> @@ t/t6022-rev-list-alternates.sh (new)\n> +'\n> +\n> +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n> -+# missing link.\n> ++# missing link. The boundary commit is not listed as we haven't used the\n> ++# `--boundary` options.\n> +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n> -+\ttest_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD\n> ++\tgit -C main rev-list --ignore-missing-links HEAD >actual &&\n> ++\ttest_stdout_line_count = 5 cat actual &&\n> ++\t! grep -$BOUNDARY_COMMIT actual\n> +'\n> +\n> +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n> +# commits.\n> +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n> -+\tgit -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&\n> -+\ttest_stdout_line_count = 6 cat list-output &&\n> -+\ttest_stdout_line_count = 1 cat list-output | grep \"^-\"\n> ++\tgit -C main rev-list --ignore-missing-links --boundary HEAD >actual &&\n> ++\ttest_stdout_line_count = 6 cat actual &&\n> ++\tgrep -$BOUNDARY_COMMIT actual\n> ++'\n> ++\n> ++# The `--ignore-missing-links` option should ensure that git-rev-list(1) doesn't\n> ++# fail when used alongside `--objects` when a tree is missing.\n> ++test_expect_success 'rev-list --ignore-missing-links works with missing tree' '\n> ++\techo \"foo\" >main/file &&\n> ++\tgit -C main add file &&\n> ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 11\" &&\n> ++\tTREE_OID=$(git -C main rev-parse HEAD^{tree}) &&\n> ++\tmkdir alt/${TREE_OID:0:2} &&\n> ++\tmv main/.git/objects/${TREE_OID:0:2}/${TREE_OID:2} alt/${TREE_OID:0:2}/ &&\n> ++\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n> ++\t! grep $TREE_OID actual\n> ++'\n> ++\n> ++# Similar to above, it should also work when a blob is missing.\n> ++test_expect_success 'rev-list --ignore-missing-links works with missing blob' '\n> ++\techo \"bar\" >main/file &&\n> ++\tgit -C main add file &&\n> ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 12\" &&\n> ++\tBLOB_OID=$(git -C main rev-parse HEAD:file) &&\n> ++\tmkdir alt/${BLOB_OID:0:2} &&\n> ++\tmv main/.git/objects/${BLOB_OID:0:2}/${BLOB_OID:2} alt/${BLOB_OID:0:2}/ &&\n> ++\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n> ++\t! grep $BLOB_OID actual\n> +'\n> +\n> +test_done\n>\n>\n> Documentation/rev-list-options.txt | 9 ++++\n> builtin/rev-list.c | 3 +-\n> revision.c | 2 +\n> t/t6022-rev-list-alternates.sh | 75 ++++++++++++++++++++++++++++++\n> 4 files changed, 88 insertions(+), 1 deletion(-)\n> create mode 100755 t/t6022-rev-list-alternates.sh\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index a4a0cb93b2..8ee713db3d 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -227,6 +227,15 @@ explicitly.\n> \tUpon seeing an invalid object name in the input, pretend as if\n> \tthe bad input was not given.\n>\n> +--ignore-missing-links::\n> +\tDuring traversal, if an object that is referenced does not\n> +\texist, instead of dying of a repository corruption, pretend as\n> +\tif the reference itself does not exist. Running the command\n> +\twith the `--boundary` option makes these missing commits,\n> +\ttogether with the commits on the edge of revision ranges\n> +\t(i.e. true boundary objects), appear on the output, prefixed\n> +\twith '-'.\n> +\n> ifndef::git-rev-list[]\n> --bisect::\n> \tPretend as if the bad bisection ref `refs/bisect/bad`\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index ff715d6918..5239d83c76 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,\n> {\n> \tstruct rev_list_info *info = cb_data;\n> \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n> -\t\tfinish_object__ma(obj);\n> +\t\tif (!info->revs->ignore_missing_links)\n> +\t\t\tfinish_object__ma(obj);\n> \t\treturn 1;\n> \t}\n> \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n\n\n> diff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh\n> new file mode 100755\n> index 0000000000..08d9ffde5f\n> --- /dev/null\n> +++ b/t/t6022-rev-list-alternates.sh\n> @@ -0,0 +1,75 @@\n> +#!/bin/sh\n> +\n> +test_description='handling of alternates in rev-list'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +# We create 5 commits and move them to the alt directory and\n> +# create 5 more commits which will stay in the main odb.\n> +test_expect_success 'create repository and alternate directory' '\n> +\tgit init main &&\n\nWe don't necessarily have to initialize a repository, as the test suite\nalready does so for us. So we may want to write this instead as:\n\n test_commit_bulk 5 &&\n git clone --reference=. --shared . alt &&\n test_commit_bulk -C alt --start=6 5\n\n> +# when the alternate odb is provided, all commits are listed along with the boundary\n> +# commit.\n> +test_expect_success 'rev-list passes with alternate object directory' '\n> +\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n> +\ttest_stdout_line_count = 10 cat actual &&\n> +\tgrep $BOUNDARY_COMMIT actual\n> +'\n\nHere, I think we'd want to make sure that we have not just 10 lines of\noutput, but that they are the 10 that we expect, like so:\n\n git -C alt rev-list --all --objects --no-object-names >actual.raw &&\n {\n git rev-list --all --objects --no-object-names &&\n git -C alt rev-list --all --objects --no-object-names --not \\\n --alternate-refs\n } >expect.raw &&\n sort actual.raw >actual &&\n sort expect.raw >expect &&\n test_cmp expect actual\n\nWhen reviewing this and tweaking some of the tests locally, I found it\nuseful to have some convenience functions like \"hide_alternates\" and\n\"show_alternates\" to control whether or not \"alt\" could see its\nalternate or not.\n\nFrom my review locally, the resulting changes (which can be applied\ndirectly on top of your patch here look like):\n\n--- 8< ---\ndiff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh\nindex 08d9ffde5f..ef4231b2de 100755\n--- a/t/t6022-rev-list-alternates.sh\n+++ b/t/t6022-rev-list-alternates.sh\n@@ -8,68 +8,86 @@ TEST_PASSES_SANITIZE_LEAK=true\n # We create 5 commits and move them to the alt directory and\n # create 5 more commits which will stay in the main odb.\n test_expect_success 'create repository and alternate directory' '\n-\tgit init main &&\n-\ttest_commit_bulk -C main 5 &&\n-\tBOUNDARY_COMMIT=$(git -C main rev-parse HEAD) &&\n-\tmkdir alt &&\n-\tmv main/.git/objects/* alt &&\n-\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5\n+\ttest_commit_bulk 5 &&\n+\tgit clone --reference=. --shared . alt &&\n+\ttest_commit_bulk --start=6 -C alt 5\n '\n\n # when the alternate odb is provided, all commits are listed along with the boundary\n # commit.\n test_expect_success 'rev-list passes with alternate object directory' '\n-\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n-\ttest_stdout_line_count = 10 cat actual &&\n-\tgrep $BOUNDARY_COMMIT actual\n+\tgit -C alt rev-list --all --objects --no-object-names >actual.raw &&\n+\t{\n+\t\tgit rev-list --all --objects --no-object-names &&\n+\t\tgit -C alt rev-list --all --objects --no-object-names --not \\\n+\t\t\t--alternate-refs\n+\t} >expect.raw &&\n+\tsort actual.raw >actual &&\n+\tsort expect.raw >expect &&\n+\ttest_cmp expect actual\n '\n\n+alt=alt/.git/objects/info/alternates\n+\n+hide_alternates () {\n+\ttest -f \"$alt.bak\" || mv \"$alt\" \"$alt.bak\"\n+}\n+\n+show_alternates () {\n+\ttest -f \"$alt\" || mv \"$alt.bak\" \"$alt\"\n+}\n+\n # When the alternate odb is not provided, rev-list fails since the 5th commit's\n # parent is not present in the main odb.\n test_expect_success 'rev-list fails without alternate object directory' '\n-\ttest_must_fail git -C main rev-list HEAD\n+\thide_alternates &&\n+\ttest_must_fail git -C alt rev-list HEAD\n '\n\n # With `--ignore-missing-links`, we stop the traversal when we encounter a\n # missing link. The boundary commit is not listed as we haven't used the\n # `--boundary` options.\n test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n-\tgit -C main rev-list --ignore-missing-links HEAD >actual &&\n-\ttest_stdout_line_count = 5 cat actual &&\n-\t! grep -$BOUNDARY_COMMIT actual\n+\thide_alternates &&\n+\n+\tgit -C alt rev-list --objects --no-object-names \\\n+\t\t--ignore-missing-links HEAD >actual.raw &&\n+\tgit -C alt cat-file --batch-check=\"%(objectname)\" \\\n+\t\t--batch-all-objects >expect.raw &&\n+\n+\tsort actual.raw >actual &&\n+\tsort expect.raw >expect &&\n+\ttest_cmp expect actual\n '\n\n # With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n # commits.\n test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n-\tgit -C main rev-list --ignore-missing-links --boundary HEAD >actual &&\n-\ttest_stdout_line_count = 6 cat actual &&\n-\tgrep -$BOUNDARY_COMMIT actual\n+\tgit -C alt rev-list --ignore-missing-links --boundary HEAD >got &&\n+\tgrep \"^-$(git rev-parse HEAD)\" got\n '\n\n-# The `--ignore-missing-links` option should ensure that git-rev-list(1) doesn't\n-# fail when used alongside `--objects` when a tree is missing.\n-test_expect_success 'rev-list --ignore-missing-links works with missing tree' '\n-\techo \"foo\" >main/file &&\n-\tgit -C main add file &&\n-\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 11\" &&\n-\tTREE_OID=$(git -C main rev-parse HEAD^{tree}) &&\n-\tmkdir alt/${TREE_OID:0:2} &&\n-\tmv main/.git/objects/${TREE_OID:0:2}/${TREE_OID:2} alt/${TREE_OID:0:2}/ &&\n-\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n-\t! grep $TREE_OID actual\n+test_expect_success \"setup for rev-list --ignore-missing-links with missing objects\" '\n+\tshow_alternates &&\n+\ttest_commit -C alt 11\n '\n\n-# Similar to above, it should also work when a blob is missing.\n-test_expect_success 'rev-list --ignore-missing-links works with missing blob' '\n-\techo \"bar\" >main/file &&\n-\tgit -C main add file &&\n-\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 12\" &&\n-\tBLOB_OID=$(git -C main rev-parse HEAD:file) &&\n-\tmkdir alt/${BLOB_OID:0:2} &&\n-\tmv main/.git/objects/${BLOB_OID:0:2}/${BLOB_OID:2} alt/${BLOB_OID:0:2}/ &&\n-\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n-\t! grep $BLOB_OID actual\n-'\n+for obj in \"HEAD^{tree}\" \"HEAD:11.t\"\n+do\n+\t# The `--ignore-missing-links` option should ensure that git-rev-list(1)\n+\t# doesn't fail when used alongside `--objects` when a tree/blob is\n+\t# missing.\n+\ttest_expect_success \"rev-list --ignore-missing-links with missing $type\" '\n+\t\toid=\"$(git -C alt rev-parse $obj)\" &&\n+\t\tpath=\"alt/.git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\tgit -C alt rev-list --ignore-missing-links --objects HEAD \\\n+\t\t\t>actual &&\n+\t\t! grep $oid actual\n+\t'\n+done\n\n test_done\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"481801","messageId":"CAOLa=ZSVobP9Be9W3f1BWoZjLUYwOqOpCQMCXVH34=iGKfXnmw@mail.gmail.com","threadId":"60211","inReplyTo":"ZQCa3GHT3D7aibJ1@nand.local","subject":"Re: [PATCH v2] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-13T09:32:13Z","receivedAt":"2023-09-13T09:32:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Sep 12, 2023 at 7:07 PM Taylor Blau wrote:\n> > +# We create 5 commits and move them to the alt directory and\n> > +# create 5 more commits which will stay in the main odb.\n> > +test_expect_success 'create repository and alternate directory' '\n> > + git init main &&\n>\n> We don't necessarily have to initialize a repository, as the test suite\n> already does so for us. So we may want to write this instead as:\n>\n> test_commit_bulk 5 &&\n> git clone --reference=. --shared . alt &&\n> test_commit_bulk -C alt --start=6 5\n>\n\nI was trying to use the env variable `GIT_ALTERNATE_OBJECT_DIRECTORIES` and\nget hence ended up creating a new repository. But I really like the\nconvenience functions\nthat you've suggested below. With that, this seems like the way to go.\n\n> > +# when the alternate odb is provided, all commits are listed along with the boundary\n> > +# commit.\n> > +test_expect_success 'rev-list passes with alternate object directory' '\n> > + GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n> > + test_stdout_line_count = 10 cat actual &&\n> > + grep $BOUNDARY_COMMIT actual\n> > +'\n>\n> Here, I think we'd want to make sure that we have not just 10 lines of\n> output, but that they are the 10 that we expect, like so:\n>\n> git -C alt rev-list --all --objects --no-object-names >actual.raw &&\n> {\n> git rev-list --all --objects --no-object-names &&\n> git -C alt rev-list --all --objects --no-object-names --not \\\n> --alternate-refs\n> } >expect.raw &&\n> sort actual.raw >actual &&\n> sort expect.raw >expect &&\n> test_cmp expect actual\n>\n> When reviewing this and tweaking some of the tests locally, I found it\n> useful to have some convenience functions like \"hide_alternates\" and\n> \"show_alternates\" to control whether or not \"alt\" could see its\n> alternate or not.\n>\n> From my review locally, the resulting changes (which can be applied\n> directly on top of your patch here look like):\n>\n\nThis is much better. I didn't know about `test_oid_to_path` and\n`test_when_finished`, and overall\nyour patch looks much nicer and is more thorough in the testing. I'll\nadd it to the next version.\nWill wait a day or two for more feedback before I submit v3.\n\nThanks again for your review and the patch :)\n"},{"id":"481807","messageId":"ZQHuo6oKoivXwdrC@nand.local","threadId":"60211","inReplyTo":"CAOLa=ZSVobP9Be9W3f1BWoZjLUYwOqOpCQMCXVH34=iGKfXnmw@mail.gmail.com","subject":"Re: [PATCH v2] revision: add `--ignore-missing-links` user option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T17:17:23Z","receivedAt":"2023-09-13T17:18:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 13, 2023 at 11:32:13AM +0200, Karthik Nayak wrote:\n> > From my review locally, the resulting changes (which can be applied\n> > directly on top of your patch here look like):\n>\n> This is much better. I didn't know about `test_oid_to_path` and\n> `test_when_finished`, and overall your patch looks much nicer and is\n> more thorough in the testing. I'll add it to the next version.\n\nThanks for folding it in! I felt bad that I might have stepped on your\ntoes by saying \"here's how I would do it\", but by the time I had applied\nyour patch locally to review it, I had already generated the\naforementioned diff.\n\n> Will wait a day or two for more feedback before I submit v3.\n\nSounds like a plan :-).\n\nThanks,\nTaylor\n"},{"id":"481867","messageId":"20230915083415.263187-1-knayak@gitlab.com","threadId":"60211","inReplyTo":"20230912155820.136111-1-karthik.188@gmail.com","subject":"[PATCH v3] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-15T08:34:15Z","receivedAt":"2023-09-15T08:34:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nThe revision backend is used by multiple porcelain commands such as\ngit-rev-list(1) and git-log(1). The backend currently supports ignoring\nmissing links by setting the `ignore_missing_links` bit. This allows the\nrevision walk to skip any objects links which are missing. Expose this\nbit via an `--ignore-missing-links` user option.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, enabling this option along with the `--boundary` option\nwhile disabling the alternate object directory allows us to find the\nboundary objects between the main and alternate object directory.\n\nHelped-by: Taylor Blau \nSigned-off-by: Karthik Nayak \n---\n\nChanges since v2:\n- Refactored the tests thanks to Taylor! \n\nRange diff against version 2:\n\n 1: e3f4d85732 ! 1: a08f3637a0 revision: add `--ignore-missing-links` user option\n @@ Commit message\n while disabling the alternate object directory allows us to find the\n boundary objects between the main and alternate object directory.\n \n + Helped-by: Taylor Blau \n Signed-off-by: Karthik Nayak \n \n ## Documentation/rev-list-options.txt ##\n @@ t/t6022-rev-list-alternates.sh (new)\n +# We create 5 commits and move them to the alt directory and\n +# create 5 more commits which will stay in the main odb.\n +test_expect_success 'create repository and alternate directory' '\n -+\tgit init main &&\n -+\ttest_commit_bulk -C main 5 &&\n -+\tBOUNDARY_COMMIT=$(git -C main rev-parse HEAD) &&\n -+\tmkdir alt &&\n -+\tmv main/.git/objects/* alt &&\n -+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5\n ++\ttest_commit_bulk 5 &&\n ++\tgit clone --reference=. --shared . alt &&\n ++\ttest_commit_bulk --start=6 -C alt 5\n +'\n +\n +# when the alternate odb is provided, all commits are listed along with the boundary\n +# commit.\n +test_expect_success 'rev-list passes with alternate object directory' '\n -+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&\n -+\ttest_stdout_line_count = 10 cat actual &&\n -+\tgrep $BOUNDARY_COMMIT actual\n ++\tgit -C alt rev-list --all --objects --no-object-names >actual.raw &&\n ++\t{\n ++\t\tgit rev-list --all --objects --no-object-names &&\n ++\t\tgit -C alt rev-list --all --objects --no-object-names --not \\\n ++\t\t\t--alternate-refs\n ++\t} >expect.raw &&\n ++\tsort actual.raw >actual &&\n ++\tsort expect.raw >expect &&\n ++\ttest_cmp expect actual\n +'\n +\n ++alt=alt/.git/objects/info/alternates\n ++\n ++hide_alternates () {\n ++\ttest -f \"$alt.bak\" || mv \"$alt\" \"$alt.bak\"\n ++}\n ++\n ++show_alternates () {\n ++\ttest -f \"$alt\" || mv \"$alt.bak\" \"$alt\"\n ++}\n ++\n +# When the alternate odb is not provided, rev-list fails since the 5th commit's\n +# parent is not present in the main odb.\n +test_expect_success 'rev-list fails without alternate object directory' '\n -+\ttest_must_fail git -C main rev-list HEAD\n ++\thide_alternates &&\n ++\ttest_must_fail git -C alt rev-list HEAD\n +'\n +\n +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n +# missing link. The boundary commit is not listed as we haven't used the\n +# `--boundary` options.\n +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n -+\tgit -C main rev-list --ignore-missing-links HEAD >actual &&\n -+\ttest_stdout_line_count = 5 cat actual &&\n -+\t! grep -$BOUNDARY_COMMIT actual\n ++\thide_alternates &&\n ++\n ++\tgit -C alt rev-list --objects --no-object-names \\\n ++\t\t--ignore-missing-links HEAD >actual.raw &&\n ++\tgit -C alt cat-file --batch-check=\"%(objectname)\" \\\n ++\t\t--batch-all-objects >expect.raw &&\n ++\n ++\tsort actual.raw >actual &&\n ++\tsort expect.raw >expect &&\n ++\ttest_must_fail git -C alt rev-list HEAD\n +'\n +\n +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n +# commits.\n +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n -+\tgit -C main rev-list --ignore-missing-links --boundary HEAD >actual &&\n -+\ttest_stdout_line_count = 6 cat actual &&\n -+\tgrep -$BOUNDARY_COMMIT actual\n ++\tgit -C alt rev-list --ignore-missing-links --boundary HEAD >got &&\n ++\tgrep \"^-$(git rev-parse HEAD)\" got\n +'\n +\n -+# The `--ignore-missing-links` option should ensure that git-rev-list(1) doesn't\n -+# fail when used alongside `--objects` when a tree is missing.\n -+test_expect_success 'rev-list --ignore-missing-links works with missing tree' '\n -+\techo \"foo\" >main/file &&\n -+\tgit -C main add file &&\n -+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 11\" &&\n -+\tTREE_OID=$(git -C main rev-parse HEAD^{tree}) &&\n -+\tmkdir alt/${TREE_OID:0:2} &&\n -+\tmv main/.git/objects/${TREE_OID:0:2}/${TREE_OID:2} alt/${TREE_OID:0:2}/ &&\n -+\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n -+\t! grep $TREE_OID actual\n ++test_expect_success \"setup for rev-list --ignore-missing-links with missing objects\" '\n ++\tshow_alternates &&\n ++\ttest_commit -C alt 11\n +'\n +\n -+# Similar to above, it should also work when a blob is missing.\n -+test_expect_success 'rev-list --ignore-missing-links works with missing blob' '\n -+\techo \"bar\" >main/file &&\n -+\tgit -C main add file &&\n -+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m\"commit 12\" &&\n -+\tBLOB_OID=$(git -C main rev-parse HEAD:file) &&\n -+\tmkdir alt/${BLOB_OID:0:2} &&\n -+\tmv main/.git/objects/${BLOB_OID:0:2}/${BLOB_OID:2} alt/${BLOB_OID:0:2}/ &&\n -+\tgit -C main rev-list --ignore-missing-links --objects HEAD >actual &&\n -+\t! grep $BLOB_OID actual\n -+'\n ++for obj in \"HEAD^{tree}\" \"HEAD:11.t\"\n ++do\n ++\t# The `--ignore-missing-links` option should ensure that git-rev-list(1)\n ++\t# doesn't fail when used alongside `--objects` when a tree/blob is\n ++\t# missing.\n ++\ttest_expect_success \"rev-list --ignore-missing-links with missing $type\" '\n ++\t\toid=\"$(git -C alt rev-parse $obj)\" &&\n ++\t\tpath=\"alt/.git/objects/$(test_oid_to_path $oid)\" &&\n ++\n ++\t\tmv \"$path\" \"$path.hidden\" &&\n ++\t\ttest_when_finished \"mv $path.hidden $path\" &&\n ++\n ++\t\tgit -C alt rev-list --ignore-missing-links --objects HEAD \\\n ++\t\t\t>actual &&\n ++\t\t! grep $oid actual\n ++ '\n ++done\n +\n +test_done\n\n\n Documentation/rev-list-options.txt | 9 +++\n builtin/rev-list.c | 3 +-\n revision.c | 2 +\n t/t6022-rev-list-alternates.sh | 93 ++++++++++++++++++++++++++++++\n 4 files changed, 106 insertions(+), 1 deletion(-)\n create mode 100755 t/t6022-rev-list-alternates.sh\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a4a0cb93b2..8ee713db3d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -227,6 +227,15 @@ explicitly.\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\n \n+--ignore-missing-links::\n+\tDuring traversal, if an object that is referenced does not\n+\texist, instead of dying of a repository corruption, pretend as\n+\tif the reference itself does not exist. Running the command\n+\twith the `--boundary` option makes these missing commits,\n+\ttogether with the commits on the edge of revision ranges\n+\t(i.e. true boundary objects), appear on the output, prefixed\n+\twith '-'.\n+\n ifndef::git-rev-list[]\n --bisect::\n \tPretend as if the bad bisection ref `refs/bisect/bad`\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff715d6918..5239d83c76 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,\n {\n \tstruct rev_list_info *info = cb_data;\n \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n-\t\tfinish_object__ma(obj);\n+\t\tif (!info->revs->ignore_missing_links)\n+\t\t\tfinish_object__ma(obj);\n \t\treturn 1;\n \t}\n \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\ndiff --git a/revision.c b/revision.c\nindex 2f4c53ea20..cbfcbf6e28 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n+\t} else if (!strcmp(arg, \"--ignore-missing-links\")) {\n+\t\trevs->ignore_missing_links = 1;\n \t} else if (opt && opt->allow_exclude_promisor_objects &&\n \t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\ndiff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh\nnew file mode 100755\nindex 0000000000..567dd21876\n--- /dev/null\n+++ b/t/t6022-rev-list-alternates.sh\n@@ -0,0 +1,93 @@\n+#!/bin/sh\n+\n+test_description='handling of alternates in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We create 5 commits and move them to the alt directory and\n+# create 5 more commits which will stay in the main odb.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit_bulk 5 &&\n+\tgit clone --reference=. --shared . alt &&\n+\ttest_commit_bulk --start=6 -C alt 5\n+'\n+\n+# when the alternate odb is provided, all commits are listed along with the boundary\n+# commit.\n+test_expect_success 'rev-list passes with alternate object directory' '\n+\tgit -C alt rev-list --all --objects --no-object-names >actual.raw &&\n+\t{\n+\t\tgit rev-list --all --objects --no-object-names &&\n+\t\tgit -C alt rev-list --all --objects --no-object-names --not \\\n+\t\t\t--alternate-refs\n+\t} >expect.raw &&\n+\tsort actual.raw >actual &&\n+\tsort expect.raw >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+alt=alt/.git/objects/info/alternates\n+\n+hide_alternates () {\n+\ttest -f \"$alt.bak\" || mv \"$alt\" \"$alt.bak\"\n+}\n+\n+show_alternates () {\n+\ttest -f \"$alt\" || mv \"$alt.bak\" \"$alt\"\n+}\n+\n+# When the alternate odb is not provided, rev-list fails since the 5th commit's\n+# parent is not present in the main odb.\n+test_expect_success 'rev-list fails without alternate object directory' '\n+\thide_alternates &&\n+\ttest_must_fail git -C alt rev-list HEAD\n+'\n+\n+# With `--ignore-missing-links`, we stop the traversal when we encounter a\n+# missing link. The boundary commit is not listed as we haven't used the\n+# `--boundary` options.\n+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n+\thide_alternates &&\n+\n+\tgit -C alt rev-list --objects --no-object-names \\\n+\t\t--ignore-missing-links HEAD >actual.raw &&\n+\tgit -C alt cat-file --batch-check=\"%(objectname)\" \\\n+\t\t--batch-all-objects >expect.raw &&\n+\n+\tsort actual.raw >actual &&\n+\tsort expect.raw >expect &&\n+\ttest_must_fail git -C alt rev-list HEAD\n+'\n+\n+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n+# commits.\n+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n+\tgit -C alt rev-list --ignore-missing-links --boundary HEAD >got &&\n+\tgrep \"^-$(git rev-parse HEAD)\" got\n+'\n+\n+test_expect_success \"setup for rev-list --ignore-missing-links with missing objects\" '\n+\tshow_alternates &&\n+\ttest_commit -C alt 11\n+'\n+\n+for obj in \"HEAD^{tree}\" \"HEAD:11.t\"\n+do\n+\t# The `--ignore-missing-links` option should ensure that git-rev-list(1)\n+\t# doesn't fail when used alongside `--objects` when a tree/blob is\n+\t# missing.\n+\ttest_expect_success \"rev-list --ignore-missing-links with missing $type\" '\n+\t\toid=\"$(git -C alt rev-parse $obj)\" &&\n+\t\tpath=\"alt/.git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\tgit -C alt rev-list --ignore-missing-links --objects HEAD \\\n+\t\t\t>actual &&\n+\t\t! grep $oid actual\n+ '\n+done\n+\n+test_done\n-- \n2.41.0\n\n"},{"id":"481892","messageId":"xmqqfs3fe08e.fsf@gitster.g","threadId":"60211","inReplyTo":"20230915083415.263187-1-knayak@gitlab.com","subject":"Re: [PATCH v3] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-15T18:54:09Z","receivedAt":"2023-09-15T18:56:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> From: Karthik Nayak \n>\n> The revision backend is used by multiple porcelain commands such as\n> git-rev-list(1) and git-log(1). The backend currently supports ignoring\n> missing links by setting the `ignore_missing_links` bit. This allows the\n> revision walk to skip any objects links which are missing. Expose this\n> bit via an `--ignore-missing-links` user option.\n\nGiven the above \"we merely surface a feature that already exists and\nsupported to be used by the end users from the command line\" claim ...\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index ff715d6918..5239d83c76 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,\n> {\n> \tstruct rev_list_info *info = cb_data;\n> \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n> -\t\tfinish_object__ma(obj);\n> +\t\tif (!info->revs->ignore_missing_links)\n> +\t\t\tfinish_object__ma(obj);\n> \t\treturn 1;\n> \t}\n\n... this hunk is a bit unexpected. As a low-level plumbing command,\nshouldn't it be left to the user who gives --ignore-missing-links\nfrom their command line to specify how the missing \"obj\" here should\nbe dealt with by giving the \"--missing=\" option? While giving\n\"allow-promisor\" may not make much sense, \"--missing=allow-any\" may\nof course make sense (it is the same as hardcoding the decision not\nto call finish_object__ma() at all), and so may \"--missing=print\".\n\nStepping back a bit, with \"--missing=print\", is this change still\nneeded? The missing objects discovered will be shown at the end,\nwith the setting, no?\n\nThanks.\n\n"},{"id":"481948","messageId":"CAOLa=ZQ9ZRmYe5b6R9ZTTpDCzb2L0UmxkeBujb2kOMSeFuwJGA@mail.gmail.com","threadId":"60211","inReplyTo":"xmqqfs3fe08e.fsf@gitster.g","subject":"Re: [PATCH v3] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-18T10:12:28Z","receivedAt":"2023-09-18T10:13:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Sep 15, 2023 at 8:54 PM Junio C Hamano wrote:\n>\n> Karthik Nayak writes:\n>\n> > From: Karthik Nayak \n> >\n> > The revision backend is used by multiple porcelain commands such as\n> > git-rev-list(1) and git-log(1). The backend currently supports ignoring\n> > missing links by setting the `ignore_missing_links` bit. This allows the\n> > revision walk to skip any objects links which are missing. Expose this\n> > bit via an `--ignore-missing-links` user option.\n>\n> Given the above \"we merely surface a feature that already exists and\n> supported to be used by the end users from the command line\" claim ...\n>\n> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > index ff715d6918..5239d83c76 100644\n> > --- a/builtin/rev-list.c\n> > +++ b/builtin/rev-list.c\n> > @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,\n> > {\n> > struct rev_list_info *info = cb_data;\n> > if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n> > - finish_object__ma(obj);\n> > + if (!info->revs->ignore_missing_links)\n> > + finish_object__ma(obj);\n> > return 1;\n> > }\n>\n> ... this hunk is a bit unexpected. As a low-level plumbing command,\n> shouldn't it be left to the user who gives --ignore-missing-links\n> from their command line to specify how the missing \"obj\" here should\n> be dealt with by giving the \"--missing=\" option? While giving\n> \"allow-promisor\" may not make much sense, \"--missing=allow-any\" may\n> of course make sense (it is the same as hardcoding the decision not\n> to call finish_object__ma() at all), and so may \"--missing=print\".\n>\n\nThis is to be expected, in my opinion. In terms of revision.c and setting the\n`revs->ignore_missing_links` bit, the traversal will go throw all\nobjects (commits\nand otherwise) and call `show_commit` or `show_object` on them.\n\nHere there is a difference for commits and non-commit objects.\n1. Commit objects: commits are parsed in revision.c and after that the\n`show_commit`\nfunction is called only when the object is available.\n2. Non-commit objects: while trees are parsed in revision.c, blobs are\nnever parsed and\nhence, ` show_object` can be called on missing blobs. This is left to\nthe user to handle. In\nour case, we error out in `rev-list.c`, which is not what we want when using the\n`--ignore-missing-links` option. Hence, this addition.\n\nThere is an argument to be made around compatibility between the\n`--missing` option\nand `--ignore-missing-links` option, but since the former only works\nwith non-commit objects\nI think the latter should be independent, and also the latter is about\nignoring all missing links.\nI also don't think the user should again specify what to do with\nmissing links by adding\n`--missing=allow-any` as `--ignore-missing-links` is a superset of it.\n\n> Stepping back a bit, with \"--missing=print\", is this change still\n> needed? The missing objects discovered will be shown at the end,\n> with the setting, no?\n>\n\nThe main difference is that the `--missing` options works entirely\nwith non-commit\nobjects (I'm assuming this was built with promisor notes in mind). So\nif a commit is\nmissing, git-rev-list(1) will still barf an error, but this error\nhandling is not in\n`builtin/rev-list.c` rather is in a layer above in `revision.c`. So it\nwouldn't be trivial for\nthe `--missing` option to support missing commit links. So that's why we expose\n`--ignore-missing-links` which ensures any kind of object (commits\nincluded) if missing,\nis ignored.\n\nThanks for the review!\n"},{"id":"481958","messageId":"xmqqv8c7mq4o.fsf@gitster.g","threadId":"60211","inReplyTo":"CAOLa=ZQ9ZRmYe5b6R9ZTTpDCzb2L0UmxkeBujb2kOMSeFuwJGA@mail.gmail.com","subject":"Re: [PATCH v3] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T15:56:39Z","receivedAt":"2023-09-18T16:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> Given the above \"we merely surface a feature that already exists and\n>> supported to be used by the end users from the command line\" claim ...\n>>\n>> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n>> > index ff715d6918..5239d83c76 100644\n>> > --- a/builtin/rev-list.c\n>> > +++ b/builtin/rev-list.c\n>> > @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,\n>> > {\n>> > struct rev_list_info *info = cb_data;\n>> > if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n>> > - finish_object__ma(obj);\n>> > + if (!info->revs->ignore_missing_links)\n>> > + finish_object__ma(obj);\n>> > return 1;\n>> > }\n>>\n>> ... this hunk is a bit unexpected. As a low-level plumbing command,\n>> shouldn't it be left to the user who gives --ignore-missing-links\n>> from their command line to specify how the missing \"obj\" here should\n>> be dealt with by giving the \"--missing=\" option? While giving\n>> \"allow-promisor\" may not make much sense, \"--missing=allow-any\" may\n>> of course make sense (it is the same as hardcoding the decision not\n>> to call finish_object__ma() at all), and so may \"--missing=print\".\n>>\n>\n> This is to be expected, in my opinion. In terms of revision.c and\n> setting the `revs->ignore_missing_links` bit, the traversal will\n> go throw all objects (commits and otherwise) and call\n> `show_commit` or `show_object` on them.\n\nYes. And the user can choose how to handle such an object here by\ntelling finish_object__ma() with the --missing= option, so\nletting them do so, instead of robbing the choice from them, would\nbe a more flexible design here, right?\n\n> if a commit is\n> missing, git-rev-list(1) will still barf an error, but this error\n\nOK, yeah, I do see the need for setting the ignore-missing-links bit\nfor what you are doing, and --missing and --ignore-missing-links are\northogonal options. Getting rid of the hardcoded skipping of\nfinish_object__ma() would make sense from this angle, too.\n\nThanks.\n"},{"id":"482008","messageId":"CAOLa=ZQdtdpu3KMMpvgr16A19xtjtOXG=HAtNrLKv97-D=Cd+g@mail.gmail.com","threadId":"60211","inReplyTo":"xmqqv8c7mq4o.fsf@gitster.g","subject":"Re: [PATCH v3] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-19T08:45:37Z","receivedAt":"2023-09-19T08:46:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Sep 18, 2023 at 5:56 PM Junio C Hamano wrote:\n> Karthik Nayak writes:\n> > This is to be expected, in my opinion. In terms of revision.c and\n> > setting the `revs->ignore_missing_links` bit, the traversal will\n> > go throw all objects (commits and otherwise) and call\n> > `show_commit` or `show_object` on them.\n>\n> Yes. And the user can choose how to handle such an object here by\n> telling finish_object__ma() with the --missing= option, so\n> letting them do so, instead of robbing the choice from them, would\n> be a more flexible design here, right?\n>\n> > if a commit is\n> > missing, git-rev-list(1) will still barf an error, but this error\n>\n> OK, yeah, I do see the need for setting the ignore-missing-links bit\n> for what you are doing, and --missing and --ignore-missing-links are\n> orthogonal options. Getting rid of the hardcoded skipping of\n> finish_object__ma() would make sense from this angle, too.\n\nWell. The only problem is that setting `ignore_missing_links` bit never calls\n`show_commit` for missing commits (since commits are pre-parsed in revision.c).\nSo to keep that behavior consistent for non-commit objects, I hardcoded the\nskipping of `finish_object__ma()` in `show_object`.\n\nIf I remove the hardcoding, it would mean that `--ignore-missing-links` would\nskip missing commits but for non-commits objects, the user would have to pass\n`--missing=allow-any` else rev-list would still error out with a\nmissing object error.\n\nDon't you think this would be confusing for the user?\nI'm happy to send a revised version removing this hardcoding if you still think\notherwise :)\n"},{"id":"482015","messageId":"xmqqfs3ai4bg.fsf@gitster.g","threadId":"60211","inReplyTo":"CAOLa=ZQdtdpu3KMMpvgr16A19xtjtOXG=HAtNrLKv97-D=Cd+g@mail.gmail.com","subject":"Re: [PATCH v3] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-19T15:13:39Z","receivedAt":"2023-09-19T15:13:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> If I remove the hardcoding, it would mean that\n> `--ignore-missing-links` would skip missing commits but for\n> non-commits objects, the user would have to pass\n> `--missing=allow-any` else rev-list would still error out with a\n> missing object error.\n>\n> Don't you think this would be confusing for the user? I'm happy\n> to send a revised version removing this hardcoding if you still\n> think otherwise :)\n\nYes. This is an example of flexibility and ergonomics at odds, and\nfor a low-level plumbing like rev-list, I would prefer not to limit\nthe flexibility unnecessarily.\n\nI do not care about the ability to pass allow-any here. But when\nyou traverse a range A..B with the --ignore-missing-links option,\nthe reporting mechanism based on the --boundary cannot tell which\nones are at the usual \"traversal boundaries\" and which ones are ones\nbeyond the broken links, can it? If you allowed the users to pass\n'print', then those reported with '?' prefix would be the missing\nones. The ones that are reported with '-' prefix may still be\nmixture of the two kinds, but you can now subtract one set from the\nother set to see which ones are true boundaries and which ones are\nmissing. The hardcoded \"we do not let __ma() logic to kick in\"\nmakes it impossible, which is what I find disturbing.\n\nThanks.\n\n\n\n\n"},{"id":"482051","messageId":"20230920104507.21664-1-karthik.188@gmail.com","threadId":"60211","inReplyTo":"20230915083415.263187-1-knayak@gitlab.com","subject":"[PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-20T10:45:07Z","receivedAt":"2023-09-20T10:46:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The revision backend is used by multiple porcelain commands such as\ngit-rev-list(1) and git-log(1). The backend currently supports ignoring\nmissing links by setting the `ignore_missing_links` bit. This allows the\nrevision walk to skip any objects links which are missing. Expose this\nbit via an `--ignore-missing-links` user option.\n\nA scenario where this option would be used is to find the boundary\nobjects between different object directories. Consider a repository with\na main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate\nobject directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a\nrepository, enabling this option along with the `--boundary` option for\nwhile disabling the alternate object directory allows us to find the\nboundary objects between the main and alternate object directory.\n\nHelped-by: Taylor Blau \nSigned-off-by: Karthik Nayak \n---\n\nChanges from v3:\n1. Remove hard-coded skipping of finish_object__ma(...). This means that\n`--ignore-missing-links` needs to be used with `--missing=...` for missing\nnon-commit objects, but also now provides the flexibility to the user instead.\nFixes to the tests around this.\n2. Fix an incorrect test.\n3. Capitalize first character in test's comment.\n\nRange diff from v4\n\n1: a08f3637a0 ! 1: 639a8cc385 revision: add `--ignore-missing-links` user option\n @@ Documentation/rev-list-options.txt: explicitly.\n --bisect::\n \tPretend as if the bad bisection ref `refs/bisect/bad`\n \n - ## builtin/rev-list.c ##\n -@@ builtin/rev-list.c: static int finish_object(struct object *obj, const char *name UNUSED,\n - {\n - \tstruct rev_list_info *info = cb_data;\n - \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n --\t\tfinish_object__ma(obj);\n -+\t\tif (!info->revs->ignore_missing_links)\n -+\t\t\tfinish_object__ma(obj);\n - \t\treturn 1;\n - \t}\n - \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n -\n ## revision.c ##\n @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n @@ t/t6022-rev-list-alternates.sh (new)\n +\ttest_commit_bulk --start=6 -C alt 5\n +'\n +\n -+# when the alternate odb is provided, all commits are listed along with the boundary\n ++# When the alternate odb is provided, all commits are listed along with the boundary\n +# commit.\n +test_expect_success 'rev-list passes with alternate object directory' '\n +\tgit -C alt rev-list --all --objects --no-object-names >actual.raw &&\n @@ t/t6022-rev-list-alternates.sh (new)\n +\thide_alternates &&\n +\n +\tgit -C alt rev-list --objects --no-object-names \\\n -+\t\t--ignore-missing-links HEAD >actual.raw &&\n ++\t\t--ignore-missing-links --missing=allow-any HEAD >actual.raw &&\n +\tgit -C alt cat-file --batch-check=\"%(objectname)\" \\\n +\t\t--batch-all-objects >expect.raw &&\n +\n +\tsort actual.raw >actual &&\n +\tsort expect.raw >expect &&\n -+\ttest_must_fail git -C alt rev-list HEAD\n ++\ttest_cmp expect actual\n +'\n +\n +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n @@ t/t6022-rev-list-alternates.sh (new)\n +\t\tmv \"$path\" \"$path.hidden\" &&\n +\t\ttest_when_finished \"mv $path.hidden $path\" &&\n +\n -+\t\tgit -C alt rev-list --ignore-missing-links --objects HEAD \\\n ++\t\tgit -C alt rev-list --ignore-missing-links --missing=allow-any --objects HEAD \\\n +\t\t\t>actual &&\n +\t\t! grep $oid actual\n + '\n\n Documentation/rev-list-options.txt | 9 +++\n revision.c | 2 +\n t/t6022-rev-list-alternates.sh | 93 ++++++++++++++++++++++++++++++\n 3 files changed, 104 insertions(+)\n create mode 100755 t/t6022-rev-list-alternates.sh\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a4a0cb93b2..8ee713db3d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -227,6 +227,15 @@ explicitly.\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\n \n+--ignore-missing-links::\n+\tDuring traversal, if an object that is referenced does not\n+\texist, instead of dying of a repository corruption, pretend as\n+\tif the reference itself does not exist. Running the command\n+\twith the `--boundary` option makes these missing commits,\n+\ttogether with the commits on the edge of revision ranges\n+\t(i.e. true boundary objects), appear on the output, prefixed\n+\twith '-'.\n+\n ifndef::git-rev-list[]\n --bisect::\n \tPretend as if the bad bisection ref `refs/bisect/bad`\ndiff --git a/revision.c b/revision.c\nindex 2f4c53ea20..cbfcbf6e28 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n+\t} else if (!strcmp(arg, \"--ignore-missing-links\")) {\n+\t\trevs->ignore_missing_links = 1;\n \t} else if (opt && opt->allow_exclude_promisor_objects &&\n \t\t !strcmp(arg, \"--exclude-promisor-objects\")) {\n \t\tif (fetch_if_missing)\ndiff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh\nnew file mode 100755\nindex 0000000000..9ba739c830\n--- /dev/null\n+++ b/t/t6022-rev-list-alternates.sh\n@@ -0,0 +1,93 @@\n+#!/bin/sh\n+\n+test_description='handling of alternates in rev-list'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# We create 5 commits and move them to the alt directory and\n+# create 5 more commits which will stay in the main odb.\n+test_expect_success 'create repository and alternate directory' '\n+\ttest_commit_bulk 5 &&\n+\tgit clone --reference=. --shared . alt &&\n+\ttest_commit_bulk --start=6 -C alt 5\n+'\n+\n+# When the alternate odb is provided, all commits are listed along with the boundary\n+# commit.\n+test_expect_success 'rev-list passes with alternate object directory' '\n+\tgit -C alt rev-list --all --objects --no-object-names >actual.raw &&\n+\t{\n+\t\tgit rev-list --all --objects --no-object-names &&\n+\t\tgit -C alt rev-list --all --objects --no-object-names --not \\\n+\t\t\t--alternate-refs\n+\t} >expect.raw &&\n+\tsort actual.raw >actual &&\n+\tsort expect.raw >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+alt=alt/.git/objects/info/alternates\n+\n+hide_alternates () {\n+\ttest -f \"$alt.bak\" || mv \"$alt\" \"$alt.bak\"\n+}\n+\n+show_alternates () {\n+\ttest -f \"$alt\" || mv \"$alt.bak\" \"$alt\"\n+}\n+\n+# When the alternate odb is not provided, rev-list fails since the 5th commit's\n+# parent is not present in the main odb.\n+test_expect_success 'rev-list fails without alternate object directory' '\n+\thide_alternates &&\n+\ttest_must_fail git -C alt rev-list HEAD\n+'\n+\n+# With `--ignore-missing-links`, we stop the traversal when we encounter a\n+# missing link. The boundary commit is not listed as we haven't used the\n+# `--boundary` options.\n+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n+\thide_alternates &&\n+\n+\tgit -C alt rev-list --objects --no-object-names \\\n+\t\t--ignore-missing-links --missing=allow-any HEAD >actual.raw &&\n+\tgit -C alt cat-file --batch-check=\"%(objectname)\" \\\n+\t\t--batch-all-objects >expect.raw &&\n+\n+\tsort actual.raw >actual &&\n+\tsort expect.raw >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n+# commits.\n+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n+\tgit -C alt rev-list --ignore-missing-links --boundary HEAD >got &&\n+\tgrep \"^-$(git rev-parse HEAD)\" got\n+'\n+\n+test_expect_success \"setup for rev-list --ignore-missing-links with missing objects\" '\n+\tshow_alternates &&\n+\ttest_commit -C alt 11\n+'\n+\n+for obj in \"HEAD^{tree}\" \"HEAD:11.t\"\n+do\n+\t# The `--ignore-missing-links` option should ensure that git-rev-list(1)\n+\t# doesn't fail when used alongside `--objects` when a tree/blob is\n+\t# missing.\n+\ttest_expect_success \"rev-list --ignore-missing-links with missing $type\" '\n+\t\toid=\"$(git -C alt rev-parse $obj)\" &&\n+\t\tpath=\"alt/.git/objects/$(test_oid_to_path $oid)\" &&\n+\n+\t\tmv \"$path\" \"$path.hidden\" &&\n+\t\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\t\tgit -C alt rev-list --ignore-missing-links --missing=allow-any --objects HEAD \\\n+\t\t\t>actual &&\n+\t\t! grep $oid actual\n+ '\n+done\n+\n+test_done\n-- \n2.41.0\n\n"},{"id":"482073","messageId":"xmqqh6noc13c.fsf@gitster.g","threadId":"60211","inReplyTo":"20230920104507.21664-1-karthik.188@gmail.com","subject":"Re: [PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-20T15:32:07Z","receivedAt":"2023-09-20T15:32:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index a4a0cb93b2..8ee713db3d 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -227,6 +227,15 @@ explicitly.\n> \tUpon seeing an invalid object name in the input, pretend as if\n> \tthe bad input was not given.\n> \n> +--ignore-missing-links::\n> +\tDuring traversal, if an object that is referenced does not\n> +\texist, instead of dying of a repository corruption, pretend as\n> +\tif the reference itself does not exist. Running the command\n> +\twith the `--boundary` option makes these missing commits,\n> +\ttogether with the commits on the edge of revision ranges\n> +\t(i.e. true boundary objects), appear on the output, prefixed\n> +\twith '-'.\n\nThere needs an explanation of interaction with --missing=\noption here, no? \"--missing=allow-any\" and \"--missing=print\" are\nsensible choices, I presume. The former allows the traversal to\nproceed, as you described in one of your responses. Also with\n\"--missing=print\", the user can more directly find out which are the\nmissing objects, even without using the \"--boundary\" that requires\nthem to sift between missing objects and the objects that are truly\non boundary.\n\nHere is my attempt:\n\n --ignore-missing-links::\n During traversal, if an object that is referenced does not\n exist, instead of dying of a repository corruption, allow\n `--missing=` to decide what to do.\n +\n `--missing=print` will make the command print a list of missing\n objects, prefixed with a \"?\" character.\n +\n `--missing=allow-any` will make the command proceed without doing\n anything special. Used with `--boundary`, output these missing\n objects mixed with the commits on the edge of revision ranges,\n prefixed with a \"-\" character.\n\nIt might make sense to add\n\n +\n Use of this option with other 'missing-action' may probably not\n give useful behaviour.\n\nat the end, but it may not be useful to the readers to say \"we allow\neven more extra flexibility but haven't thought through what good\nthey would do\".\n\n> +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n> +# missing link. The boundary commit is not listed as we haven't used the\n> +# `--boundary` options.\n> +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n> +\thide_alternates &&\n> +\n> +\tgit -C alt rev-list --objects --no-object-names \\\n> +\t\t--ignore-missing-links --missing=allow-any HEAD >actual.raw &&\n> +\tgit -C alt cat-file --batch-check=\"%(objectname)\" \\\n> +\t\t--batch-all-objects >expect.raw &&\n> +\n> +\tsort actual.raw >actual &&\n> +\tsort expect.raw >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nThis gives a good baseline. \"--missing=print\" without \"--boundary\"\nmay have more obvious use cases, but is there a practical use case\nfor the output from an invocation with \"--missing=allow-any\" without\n\"--boundary\"? Just being curious if I am missing something obvious.\n\nPerhaps add another test that uses \"--missing=print\" instead, and\ncheck that the \"? missing\" output matches what we expect to be\nmissing? The same comment applies to the other test that uses\n\"--missing=allow-any\" without \"--boundary\" we see later.\n\n> +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n> +# commits.\n> +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n> +\tgit -C alt rev-list --ignore-missing-links --boundary HEAD >got &&\n> +\tgrep \"^-$(git rev-parse HEAD)\" got\n> +'\n\nThis makes sure what we expect to appear in 'got' actually is in\n'got', but we should also make sure 'got' does not have anything\nunexpected. \n\n> +test_expect_success \"setup for rev-list --ignore-missing-links with missing objects\" '\n> +\tshow_alternates &&\n> +\ttest_commit -C alt 11\n> +'\n> +\n> +for obj in \"HEAD^{tree}\" \"HEAD:11.t\"\n> +do\n> +\t# The `--ignore-missing-links` option should ensure that git-rev-list(1)\n> +\t# doesn't fail when used alongside `--objects` when a tree/blob is\n> +\t# missing.\n> +\ttest_expect_success \"rev-list --ignore-missing-links with missing $type\" '\n> +\t\toid=\"$(git -C alt rev-parse $obj)\" &&\n> +\t\tpath=\"alt/.git/objects/$(test_oid_to_path $oid)\" &&\n> +\n> +\t\tmv \"$path\" \"$path.hidden\" &&\n> +\t\ttest_when_finished \"mv $path.hidden $path\" &&\n\nIn the first iteration, we check without the tree object and we only\nensure that removed tree does not appear in the output---but we know\nthe blob that is referenced by that removed tree will not appear in\nthe output, either, don't we? Don't we want to check that, too?\n\nIn the second iteration, we have resurrected the tree but removed\nthe blob that is referenced by the tree, so we would not see that\nblob in the output, which makes sense.\n\n> +\t\tgit -C alt rev-list --ignore-missing-links --missing=allow-any --objects HEAD \\\n> +\t\t\t>actual &&\n> +\t\t! grep $oid actual\n> + '\n> +done\n> +\n> +test_done\n\nThanks.\n"},{"id":"482097","messageId":"CAOLa=ZT_DVw4Na-rj4mk2ULkrqDyVP2_cOpgdLozFjn3RJbTvg@mail.gmail.com","threadId":"60211","inReplyTo":"xmqqh6noc13c.fsf@gitster.g","subject":"Re: [PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-21T10:53:11Z","receivedAt":"2023-09-21T18:37:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Sep 20, 2023 at 5:32 PM Junio C Hamano wrote:\n>\n> There needs an explanation of interaction with --missing=\n> option here, no? \"--missing=allow-any\" and \"--missing=print\" are\n> sensible choices, I presume. The former allows the traversal to\n> proceed, as you described in one of your responses. Also with\n> \"--missing=print\", the user can more directly find out which are the\n> missing objects, even without using the \"--boundary\" that requires\n> them to sift between missing objects and the objects that are truly\n> on boundary.\n>\n> Here is my attempt:\n>\n> --ignore-missing-links::\n> During traversal, if an object that is referenced does not\n> exist, instead of dying of a repository corruption, allow\n> `--missing=` to decide what to do.\n> +\n> `--missing=print` will make the command print a list of missing\n> objects, prefixed with a \"?\" character.\n> +\n> `--missing=allow-any` will make the command proceed without doing\n> anything special. Used with `--boundary`, output these missing\n> objects mixed with the commits on the edge of revision ranges,\n> prefixed with a \"-\" character.\n>\n> It might make sense to add\n>\n> +\n> Use of this option with other 'missing-action' may probably not\n> give useful behaviour.\n>\n> at the end, but it may not be useful to the readers to say \"we allow\n> even more extra flexibility but haven't thought through what good\n> they would do\".\n>\n\nI was thinking about this, but mostly didn't do this, because the\ninteraction with `--missing`\nis only for non-commit objects. Because for missing commits,\n`--ignore-missing-links` skips\nthe commit and the value of `--missing` doesn't make any difference.\n\nIt's only for non-commit objects that `--missing` comes into play. So\nperhaps change the current\nexplanation to:\n\n--ignore-missing-links::\n During traversal, if a commit that is referenced does not\n exist, instead of dying of a repository corruption, pretend as\n if the commit itself does not exist. Running the command\n with the `--boundary` option makes these missing commits,\n together with the commits on the edge of revision ranges\n (i.e. true boundary objects), appear on the output, prefixed\n with '-'.\n\nThis way `--ignore-missing-links` is specific to commits, combining\nthis with `--missing=...` for\nnon-commit objects is left to the user. What do you think?\n\n> > +# With `--ignore-missing-links`, we stop the traversal when we encounter a\n> > +# missing link. The boundary commit is not listed as we haven't used the\n> > +# `--boundary` options.\n> > +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '\n> > + hide_alternates &&\n> > +\n> > + git -C alt rev-list --objects --no-object-names \\\n> > + --ignore-missing-links --missing=allow-any HEAD >actual.raw &&\n> > + git -C alt cat-file --batch-check=\"%(objectname)\" \\\n> > + --batch-all-objects >expect.raw &&\n> > +\n> > + sort actual.raw >actual &&\n> > + sort expect.raw >expect &&\n> > + test_cmp expect actual\n> > +'\n>\n> This gives a good baseline. \"--missing=print\" without \"--boundary\"\n> may have more obvious use cases, but is there a practical use case\n> for the output from an invocation with \"--missing=allow-any\" without\n> \"--boundary\"? Just being curious if I am missing something obvious.\n>\n\nNot really, but it's easier to build up the testing, here without\nboundary we can\nuse cat-file to test all objects (commits and others) that are output\nby rev-list.\n\nThen we can build on top of this in the next test, where we can also ensure that\nboundary commits are printed. This however is very simplistic, as\nyou've mentioned.\nThere could be other objects and we don't really check.\n\n> Perhaps add another test that uses \"--missing=print\" instead, and\n> check that the \"? missing\" output matches what we expect to be\n> missing? The same comment applies to the other test that uses\n> \"--missing=allow-any\" without \"--boundary\" we see later.\n>\n\nSure, we can add that too!\n\n> > +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary\n> > +# commits.\n> > +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '\n> > + git -C alt rev-list --ignore-missing-links --boundary HEAD >got &&\n> > + grep \"^-$(git rev-parse HEAD)\" got\n> > +'\n>\n> This makes sure what we expect to appear in 'got' actually is in\n> 'got', but we should also make sure 'got' does not have anything\n> unexpected.\n>\n\nYeah, I can add that in too.\n\n> > +test_expect_success \"setup for rev-list --ignore-missing-links with missing objects\" '\n> > + show_alternates &&\n> > + test_commit -C alt 11\n> > +'\n> > +\n> > +for obj in \"HEAD^{tree}\" \"HEAD:11.t\"\n> > +do\n> > + # The `--ignore-missing-links` option should ensure that git-rev-list(1)\n> > + # doesn't fail when used alongside `--objects` when a tree/blob is\n> > + # missing.\n> > + test_expect_success \"rev-list --ignore-missing-links with missing $type\" '\n> > + oid=\"$(git -C alt rev-parse $obj)\" &&\n> > + path=\"alt/.git/objects/$(test_oid_to_path $oid)\" &&\n> > +\n> > + mv \"$path\" \"$path.hidden\" &&\n> > + test_when_finished \"mv $path.hidden $path\" &&\n>\n> In the first iteration, we check without the tree object and we only\n> ensure that removed tree does not appear in the output---but we know\n> the blob that is referenced by that removed tree will not appear in\n> the output, either, don't we? Don't we want to check that, too?\n>\n> In the second iteration, we have resurrected the tree but removed\n> the blob that is referenced by the tree, so we would not see that\n> blob in the output, which makes sense.\n>\n\nI was implementing this change and just realized that for missing\ntrees, show_object() is\nnever called (that is --missing=print has no effect).\n\nThat means we only call show_object() when there is a missing blob.\n\nSo this effectively means:\n1. missing commits: --ignore-missing-links works, --missing=... has no effect\n2. missing trees: --ignore-missing-links works, --missing=... has no effect\n3. missing blobs: --ignore-missing-links works in conjunction\nwith --missing=...\n\nI now think it does make even more sense to hardcode the skipping of\n`finish_object__ma` this way\nwe can state that `--ignore-missing-links` and `--missing` are\nincompatible, wherein `--ignore-missing-links`\nignores any missing object (irrelevant of type) and `--missing` is\nused to specifically handle missing blobs\nand provides options.\n\nThis is also how currently `--boundary` and `--missing=print` is\nspecific to commits and blobs respectively.\nWhat do you think?\n"},{"id":"482104","messageId":"xmqq5y435obx.fsf@gitster.g","threadId":"60211","inReplyTo":"CAOLa=ZT_DVw4Na-rj4mk2ULkrqDyVP2_cOpgdLozFjn3RJbTvg@mail.gmail.com","subject":"Re: [PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-21T19:16:34Z","receivedAt":"2023-09-21T20:12:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> I was thinking about this, but mostly didn't do this, because the\n> interaction with `--missing` is only for non-commit\n> objects. Because for missing commits, `--ignore-missing-links`\n> skips the commit and the value of `--missing` doesn't make any\n> difference.\n\nHmph, somehow that smells like an existing bug. So does the \"trees\nare not shown by --missing=print, and show_object() is never called\nfor missing objects unless they are blobs\" you mention. When the\nuser asks \"instead of dying, list them so that I can ask around and\nfetch them to repair this repository\", shouldn't we do just that?\n\nI wonder if these bugs are something people may be taking advatage\nof and cannot be fixed retroactively? If we can fix these and nobody\ncomplains, that would give us the ideal outcome, I would think.\n\nThanks.\n\n\n"},{"id":"482230","messageId":"CAOLa=ZThB0DrRKg98tr7JLu8yPRyDXe_ngkpS3ZfesHQ-f1DLg@mail.gmail.com","threadId":"60211","inReplyTo":"xmqq5y435obx.fsf@gitster.g","subject":"Re: [PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-24T16:14:30Z","receivedAt":"2023-09-24T16:15:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Sep 21, 2023 at 9:16 PM Junio C Hamano wrote:\n>\n> Karthik Nayak writes:\n>\n> > I was thinking about this, but mostly didn't do this, because the\n> > interaction with `--missing` is only for non-commit\n> > objects. Because for missing commits, `--ignore-missing-links`\n> > skips the commit and the value of `--missing` doesn't make any\n> > difference.\n>\n> Hmph, somehow that smells like an existing bug. So does the \"trees\n> are not shown by --missing=print, and show_object() is never called\n> for missing objects unless they are blobs\" you mention. When the\n> user asks \"instead of dying, list them so that I can ask around and\n> fetch them to repair this repository\", shouldn't we do just that?\n>\n> I wonder if these bugs are something people may be taking advatage\n> of and cannot be fixed retroactively? If we can fix these and nobody\n> complains, that would give us the ideal outcome, I would think.\n>\n\nLet me prefix with saying that I was partly wrong. `--missing` does work for\ntrees, only that it's ineffective when used along with the\n`ignore_missing_links` bit.\n\nBut for commits, `--missing` was never configured to work with. I did a quick\nlook at the code, we can do something like this for commits too, i.e.\nadd support\nfor the `--missing` option. We'll have to add a new flag (maybe\nMISSING) so it can\nbe set during within `repo_parse_commit_gently` so we can parse this\nas a missing\nobject in rev-list.c and act accordingly.\n\nIt would invalidate this patch series in some sense. But I'm okay with\nthat. Does that\nsound good to you?\n"},{"id":"482287","messageId":"xmqq5y3yw5qz.fsf@gitster.g","threadId":"60211","inReplyTo":"CAOLa=ZThB0DrRKg98tr7JLu8yPRyDXe_ngkpS3ZfesHQ-f1DLg@mail.gmail.com","subject":"Re: [PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-25T16:57:08Z","receivedAt":"2023-09-25T16:57:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Let me prefix with saying that I was partly wrong. `--missing` does work for\n> trees, only that it's ineffective when used along with the\n> `ignore_missing_links` bit.\n>\n> But for commits, `--missing` was never configured to work with. I\n> did a quick look at the code, we can do something like this for\n> commits too, i.e. add support for the `--missing` option. We'll\n> have to add a new flag (maybe MISSING) so it can be set during\n> within `repo_parse_commit_gently` so we can parse this as a\n> missing object in rev-list.c and act accordingly.\n\nDo you mean that process_parents() would now throw such a commit to\nthe resulting list successfully instead of omitting when \"--missing\"\nis requested? That sounds like a right thing to do but at the same\ntime is a fix with major impact. I do not offhand know what the\nramifications are, for example, when bitmap traversal is in use (I\nassume such a missing commit would not be catalogued in the bitmap?).\n\nTaylor, what do you think?\n"},{"id":"482379","messageId":"CAOLa=ZT0OhGo-9zhHq4NFQ7mBiMpwnUPPf3ou5nYMSTQP-VV4A@mail.gmail.com","threadId":"60211","inReplyTo":"xmqq5y3yw5qz.fsf@gitster.g","subject":"Re: [PATCH v4] revision: add `--ignore-missing-links` user option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-09-27T16:26:51Z","receivedAt":"2023-09-27T16:27:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Sep 25, 2023 at 6:57 PM Junio C Hamano wrote:\n>\n> Karthik Nayak writes:\n>\n> > Let me prefix with saying that I was partly wrong. `--missing` does work for\n> > trees, only that it's ineffective when used along with the\n> > `ignore_missing_links` bit.\n> >\n> > But for commits, `--missing` was never configured to work with. I\n> > did a quick look at the code, we can do something like this for\n> > commits too, i.e. add support for the `--missing` option. We'll\n> > have to add a new flag (maybe MISSING) so it can be set during\n> > within `repo_parse_commit_gently` so we can parse this as a\n> > missing object in rev-list.c and act accordingly.\n>\n> Do you mean that process_parents() would now throw such a commit to\n> the resulting list successfully instead of omitting when \"--missing\"\n> is requested? That sounds like a right thing to do but at the same\n> time is a fix with major impact.\n\nYes, but with appropriate flag added. Which will be a new flag.\n\n> I do not offhand know what the\n> ramifications are, for example, when bitmap traversal is in use (I\n> assume such a missing commit would not be catalogued in the bitmap?).\n>\n\nIf there is a missing commit or object, will there even be a bitmap?\nI can think of the two scenarios:\n1. Object is missing before bitmap creation: In such a scenario, the bitmap\ndoesn't get created, since an object is missing. Could be any type of object.\n2. Object is missing after bitmap creation: In this case, the bitmap\nalready exists\nand rev-list won't even know that the commit is missing and simply output the\nobjects as if the objects exist.\n\nOverall, this makes sense, but curious to hear what Taylor has to say.\nI also might\npost a patch series in this direction to consolidate our thoughts and\nget a feedback\nfrom the list.\n"}]}