{"thread":{"id":"60793","subject":"[PATCH 0/2] index-pack: fsck honor checks","startedAt":"2024-01-25T20:51:27Z","lastAt":"2024-03-09T01:55:23Z","messageCount":29,"participants":["John Cai via GitGitGadget","Junio C Hamano","John Cai","Jonathan Tan","Patrick Steinhardt","Christian Couder","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487384","messageId":"pull.1658.git.git.1706215884.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":null,"subject":"[PATCH 0/2] index-pack: fsck honor checks","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-25T20:51:22Z","receivedAt":"2024-01-25T20:51:27Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-index-pack has a --strict mode that can take an optional argument to\nprovide a list of fsck issues to change their severity. --fsck-objects does\nnot have such a utility, which would be useful if one would like to be more\nlenient or strict on data integrity in a repository.\n\nLike --strict, Allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nJohn Cai (2):\n index-pack: test and document --strict=\n index-pack: --fsck-objects to take an optional argument for fsck msgs\n\n Documentation/git-index-pack.txt | 19 +++++++++++----\n builtin/index-pack.c | 5 ++--\n t/t5300-pack-object.sh | 41 ++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+), 6 deletions(-)\n\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1658%2Fjohn-cai%2Fjc%2Findex-pack-fsck-honor-checks-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1658/john-cai/jc/index-pack-fsck-honor-checks-v1\nPull-Request: https://github.com/git/git/pull/1658\n-- \ngitgitgadget\n"},{"id":"487385","messageId":"9b353aff73d6351b86cc7b55982f1565e76d08e9.1706215884.git.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.git.git.1706215884.gitgitgadget@gmail.com","subject":"[PATCH 1/2] index-pack: test and document --strict=","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-25T20:51:23Z","receivedAt":"2024-01-25T20:53:03Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n--strict. However this is a hidden argument that was not documented nor\ntested. Though true that most users would not call this option\ndirection, (nor use index-pack for that matter) it is still useful to\ndocument and test this feature.\n\nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 9 +++++++--\n builtin/index-pack.c | 2 +-\n t/t5300-pack-object.sh | 22 ++++++++++++++++++++++\n 3 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 6486620c3d8..14f806d07d1 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -79,8 +79,13 @@ OPTIONS\n \tto force the version for the generated pack index, and to force\n \t64-bit index entries on objects located above the given offset.\n \n---strict::\n-\tDie, if the pack contains broken objects or links.\n+--strict[=]::\n+\tDie, if the pack contains broken objects or links. If `` is passed,\n+\tit should be a comma-separated list of `=` elements where\n+\t`` and `` are used to change the severity of some possible\n+\tissues, eg: `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n+\tfor the `fsck.` configuration options in `linkgit:git-fsck[1] for\n+\tmore information on the possible values of `` and ``.\n \n --progress-title::\n \tFor internal use only.\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1ea87e01f29..1e53ca23775 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d402ec18b79..9563372ae27 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_when_finished rm -rf strict &&\n+\tgit init strict &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_commit first hello &&\n+\t\tcat >commit <<-EOF &&\n+\t\ttree $(git rev-parse HEAD^{tree})\n+\t\tparent $(git rev-parse HEAD)\n+\t\tauthor A U Thor\n+\t\tcommitter A U Thor\n+\n+\t\tcommit: this is a commit wit bad emails\n+\n+\t\tEOF\n+\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n+\t\tPACK=$(git pack-objects test \n\ngit-index-pack has a --strict mode that can take an optional argument to\nprovide a list of fsck issues to change their severity. --fsck-objects\ndoes not have such a utility, which would be useful if one would like to\nbe more lenient or strict on data integrity in a repository.\n\nLike --strict, Allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nThis commit also removes the \"For internal use only\" note for\n--fsck-objects, and documents the option. This won't often be used by\nthe normal end user, but it turns out it is useful for Git forges like\nGitLab.\n\nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 10 ++++++++--\n builtin/index-pack.c | 5 +++--\n t/t5300-pack-object.sh | 29 ++++++++++++++++++++++++-----\n 3 files changed, 35 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 14f806d07d1..37709b13c88 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -96,8 +96,14 @@ default and \"Indexing objects\" when `--stdin` is specified.\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \n---fsck-objects::\n-\tFor internal use only.\n+--fsck-objects[=]::\n+\tInstructs index-pack to check for broken objects instead of broken\n+\tlinks. If `` is passed, it should be a comma-separated list of\n+\t`=` where `` and `` are used to\n+\tchange the severity of `fsck` errors, eg: `--strict=\"missingEmail=ignore,badTagName=ignore\"`.\n+\tSee the entry for the `fsck.` configuration options in\n+\t`linkgit:git-fsck[1] for more information on the possible values of\n+\t`` and ``.\n +\n Die if the pack contains broken objects. If the pack contains a tree\n pointing to a .gitmodules blob that does not exist, prints the hash of\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1e53ca23775..519162f5b91 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] [--fsck-objects[=]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n@@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t} else if (!strcmp(arg, \"--check-self-contained-and-connected\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcheck_self_contained_and_connected = 1;\n-\t\t\t} else if (!strcmp(arg, \"--fsck-objects\")) {\n+\t\t\t} else if (skip_to_optional_arg(arg, \"--fsck-objects\", &arg)) {\n \t\t\t\tdo_fsck_object = 1;\n+\t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n \t\t\t} else if (!strcmp(arg, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 9563372ae27..916cf939beb 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n-test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n-\ttest_when_finished rm -rf strict &&\n+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '\n \tgit init strict &&\n \t(\n \t\tcd strict &&\n@@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n \n \t\tEOF\n \t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n-\t\tPACK=$(git pack-objects test pack-name\n \t)\n '\n \n+test_with_bad_commit () {\n+\tmust_fail_arg=\"$1\" &&\n+\tmust_pass_arg=\"$2\" &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_expect_fail git index-pack \"$must_fail_arg\" \"test-$(cat pack-name).pack\"\n+\t\tgit index-pack \"$must_pass_arg\" \"test-$(cat pack-name).pack\"\n+\t)\n+}\n+\n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_with_bad_commit --strict --strict=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '\n+\ttest_with_bad_commit --fsck-objects --fsck-objects=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '\n+\trm -rf strict\n+'\n+\n test_expect_success 'honor pack.packSizeLimit' '\n \tgit config pack.packSizeLimit 3m &&\n \tpackname_10=$(git pack-objects test-10 ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-25T22:46:02Z","receivedAt":"2024-01-25T22:46:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n> --strict. However this is a hidden argument that was not documented nor\n> tested. Though true that most users would not call this option\n> direction, (nor use index-pack for that matter) it is still useful to\n\nThough it is true that ... call this option directly (nor use\nindex-pack, for that matter), it is still ...\n\nor something like that, probably.\n\n> document and test this feature.\n\nAnd I agree with that. Thanks for adding the necessary doc.\n\n> +--strict[=]::\n\n in the context of \"git fsck --help\" seems to refer to the\nleft hand side of =. sounds as if it is\njust the list of without saying anything about their severity,\nwhich is not what we want to imply.\n\nEither use a made-up word that is clearly different and can not be\nmistaken as a list of , or spell it out a bit more\nexplicitly, may make it easier to follow?\n\n\t--strict[=]\n\t--strict[==...]\n\nI dunno.\n\nUse of and below looks good in the body of the\nparagraph here.\n\n> +\tDie, if the pack contains broken objects or links. If `` is passed,\n> +\tit should be a comma-separated list of `=` elements where\n> +\t`` and `` are used to change the severity of some possible\n> +\tissues, eg: `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n> +\tfor the `fsck.` configuration options in `linkgit:git-fsck[1] for\n> +\tmore information on the possible values of `` and ``.\n\n\"eg:\" -> \"e.g.,\" probably.\n\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index d402ec18b79..9563372ae27 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '\n> \t)\n> '\n> \n> +test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n> +\ttest_when_finished rm -rf strict &&\n> +\tgit init strict &&\n> +\t(\n> +\t\tcd strict &&\n> +\t\ttest_commit first hello &&\n> +\t\tcat >commit <<-EOF &&\n> +\t\ttree $(git rev-parse HEAD^{tree})\n> +\t\tparent $(git rev-parse HEAD)\n> +\t\tauthor A U Thor\n> +\t\tcommitter A U Thor\n> +\n> +\t\tcommit: this is a commit wit bad emails\n\n\"wit\" -> \"with\"; as this typo does not contribute anything to\nthe badness we expect index-pack to notice, it would pay to\nmake sure we do not have it, to avoid distracting readers.\n\n> +\t\tEOF\n> +\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n> +\t\tPACK=$(git pack-objects test +\t\ttest_must_fail git index-pack --strict \"test-$PACK.pack\" &&\n> +\t\tgit index-pack --strict=\"missingEmail=ignore\" \"test-$PACK.pack\"\n> +\t)\n> +'\n> +\n> test_expect_success 'honor pack.packSizeLimit' '\n> \tgit config pack.packSizeLimit 3m &&\n> \tpackname_10=$(git pack-objects test-10 writes:\n\n> From: John Cai \n>\n> git-index-pack has a --strict mode that can take an optional argument to\n\n\"mode\" -> \"option\", probably.\n\n> provide a list of fsck issues to change their severity. --fsck-objects\n> does not have such a utility, which would be useful if one would like to\n> be more lenient or strict on data integrity in a repository.\n>\n> Like --strict, Allow --fsck-objects to also take a list of fsck msgs to\n> change the severity.\n\n\"Allow\" -> \"allow\".\n\n> This commit also removes the \"For internal use only\" note for\n> --fsck-objects, and documents the option. This won't often be used by\n> the normal end user, but it turns out it is useful for Git forges like\n> GitLab.\n\n\"This commit also removes\", \"documents\" -> \"Remove\", \"document\".\n\n> ---fsck-objects::\n> -\tFor internal use only.\n> +--fsck-objects[=]::\n> +\tInstructs index-pack to check for broken objects instead of broken\n> +\tlinks. If `` is passed, it should be a comma-separated list of\n\nVery much pleased to see an additional description that is written\nto clarify the difference between this option and the other --strict\noption. The original was totally unclear on this point, and it is\nvery much appreciated.\n\nThe other option notices and dies upon seeing either a broken object\nor a dangling link. This one only diagnoses broken objects and does\nnot care if the objects are connected. However, saying \"instead of\"\nhere tempts readers to mistakenly think that the other one only\nchecks links and this one only checks contents, which is not what we\nwant to say. Perhaps \"to check for broken objects, but unlike\n`--strict`, do not choke on broken links\" or something?\n\nSame comment on as the previous step.\n\n> +\t`=` where `` and `` are used to\n> +\tchange the severity of `fsck` errors, eg: `--strict=\"missingEmail=ignore,badTagName=ignore\"`.\n\nSame comment for \"eg:\" as before.\n\n> @@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n> \t\t\t} else if (!strcmp(arg, \"--check-self-contained-and-connected\")) {\n> \t\t\t\tstrict = 1;\n> \t\t\t\tcheck_self_contained_and_connected = 1;\n> -\t\t\t} else if (!strcmp(arg, \"--fsck-objects\")) {\n> +\t\t\t} else if (skip_to_optional_arg(arg, \"--fsck-objects\", &arg)) {\n> \t\t\t\tdo_fsck_object = 1;\n> +\t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n> \t\t\t} else if (!strcmp(arg, \"--verify\")) {\n> \t\t\t\tverify = 1;\n> \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\n\nThe implementation of this part looks quite obvious, once you see\nhow \"--strict[==]\" is implemented.\n\nLooking good.\n\nThanks.\n"},{"id":"487414","messageId":"pull.1658.v2.git.git.1706289180.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.git.git.1706215884.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] index-pack: fsck honor checks","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-26T17:12:58Z","receivedAt":"2024-01-26T17:13:03Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-index-pack has a --strict mode that can take an optional argument to\nprovide a list of fsck issues to change their severity. --fsck-objects does\nnot have such a utility, which would be useful if one would like to be more\nlenient or strict on data integrity in a repository.\n\nLike --strict, Allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nChange since V1:\n\n * edited commit messages\n * clarified formatting in documentation for --strict= and --fsck-objects=\n\nJohn Cai (2):\n index-pack: test and document --strict=\n index-pack: --fsck-objects to take an optional argument for fsck msgs\n\n Documentation/git-index-pack.txt | 19 +++++++++++----\n builtin/index-pack.c | 5 ++--\n t/t5300-pack-object.sh | 41 ++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+), 6 deletions(-)\n\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1658%2Fjohn-cai%2Fjc%2Findex-pack-fsck-honor-checks-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1658/john-cai/jc/index-pack-fsck-honor-checks-v2\nPull-Request: https://github.com/git/git/pull/1658\n\nRange-diff vs v1:\n\n 1: 9b353aff73d ! 1: b3b3e8bd0bf index-pack: test and document --strict=\n @@ Commit message\n 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n --strict. However this is a hidden argument that was not documented nor\n - tested. Though true that most users would not call this option\n - direction, (nor use index-pack for that matter) it is still useful to\n + tested. Though it is true that most users would not call this option\n + directly, (nor use index-pack for that matter) it is still useful to\n document and test this feature.\n \n Signed-off-by: John Cai \n @@ Documentation/git-index-pack.txt: OPTIONS\n \n ---strict::\n -\tDie, if the pack contains broken objects or links.\n -+--strict[=]::\n ++--strict[==...]::\n +\tDie, if the pack contains broken objects or links. If `` is passed,\n +\tit should be a comma-separated list of `=` elements where\n +\t`` and `` are used to change the severity of some possible\n -+\tissues, eg: `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n ++\tissues, e.g., `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n +\tfor the `fsck.` configuration options in `linkgit:git-fsck[1] for\n +\tmore information on the possible values of `` and ``.\n \n @@ t/t5300-pack-object.sh: test_expect_success 'index-pack with --strict' '\n +\t\tauthor A U Thor\n +\t\tcommitter A U Thor\n +\n -+\t\tcommit: this is a commit wit bad emails\n ++\t\tcommit: this is a commit with bad emails\n +\n +\t\tEOF\n +\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n 2: 074e0c7ab92 ! 2: cce63c6465f index-pack: --fsck-objects to take an optional argument for fsck msgs\n @@ Metadata\n ## Commit message ##\n index-pack: --fsck-objects to take an optional argument for fsck msgs\n \n - git-index-pack has a --strict mode that can take an optional argument to\n - provide a list of fsck issues to change their severity. --fsck-objects\n - does not have such a utility, which would be useful if one would like to\n - be more lenient or strict on data integrity in a repository.\n + git-index-pack has a --strict option that can take an optional argument\n + to provide a list of fsck issues to change their severity.\n + --fsck-objects does not have such a utility, which would be useful if\n + one would like to be more lenient or strict on data integrity in a\n + repository.\n \n - Like --strict, Allow --fsck-objects to also take a list of fsck msgs to\n + Like --strict, allow --fsck-objects to also take a list of fsck msgs to\n change the severity.\n \n - This commit also removes the \"For internal use only\" note for\n - --fsck-objects, and documents the option. This won't often be used by\n - the normal end user, but it turns out it is useful for Git forges like\n - GitLab.\n + Remove the \"For internal use only\" note for --fsck-objects, and document\n + the option. This won't often be used by the normal end user, but it\n + turns out it is useful for Git forges like GitLab.\n \n Signed-off-by: John Cai \n \n @@ Documentation/git-index-pack.txt: default and \"Indexing objects\" when `--stdin`\n \n ---fsck-objects::\n -\tFor internal use only.\n -+--fsck-objects[=]::\n -+\tInstructs index-pack to check for broken objects instead of broken\n -+\tlinks. If `` is passed, it should be a comma-separated list of\n -+\t`=` where `` and `` are used to\n -+\tchange the severity of `fsck` errors, eg: `--strict=\"missingEmail=ignore,badTagName=ignore\"`.\n -+\tSee the entry for the `fsck.` configuration options in\n -+\t`linkgit:git-fsck[1] for more information on the possible values of\n -+\t`` and ``.\n ++--fsck-objects[==...]::\n ++\tInstructs index-pack to check for broken objects, but unlike `--strict`,\n ++\tdoes not choke on broken links. If `` is passed, it should be\n ++\ta comma-separated list of `=` where `` and\n ++\t`` are used to change the severity of `fsck` errors e.g.,\n ++\t`--strict=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for\n ++\tthe `fsck.` configuration options in `linkgit:git-fsck[1] for\n ++\tmore information on the possible values of `` and ``.\n +\n Die if the pack contains broken objects. If the pack contains a tree\n pointing to a .gitmodules blob that does not exist, prints the hash of\n\n-- \ngitgitgadget\n"},{"id":"487415","messageId":"b3b3e8bd0bf2c83b57debef81edc39970beaf05b.1706289180.git.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.v2.git.git.1706289180.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] index-pack: test and document --strict=","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-26T17:12:59Z","receivedAt":"2024-01-26T17:13:05Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n--strict. However this is a hidden argument that was not documented nor\ntested. Though it is true that most users would not call this option\ndirectly, (nor use index-pack for that matter) it is still useful to\ndocument and test this feature.\n\nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 9 +++++++--\n builtin/index-pack.c | 2 +-\n t/t5300-pack-object.sh | 22 ++++++++++++++++++++++\n 3 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 6486620c3d8..f7a98bbf9c8 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -79,8 +79,13 @@ OPTIONS\n \tto force the version for the generated pack index, and to force\n \t64-bit index entries on objects located above the given offset.\n \n---strict::\n-\tDie, if the pack contains broken objects or links.\n+--strict[==...]::\n+\tDie, if the pack contains broken objects or links. If `` is passed,\n+\tit should be a comma-separated list of `=` elements where\n+\t`` and `` are used to change the severity of some possible\n+\tissues, e.g., `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n+\tfor the `fsck.` configuration options in `linkgit:git-fsck[1] for\n+\tmore information on the possible values of `` and ``.\n \n --progress-title::\n \tFor internal use only.\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1ea87e01f29..1e53ca23775 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d402ec18b79..496fffa0f8a 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_when_finished rm -rf strict &&\n+\tgit init strict &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_commit first hello &&\n+\t\tcat >commit <<-EOF &&\n+\t\ttree $(git rev-parse HEAD^{tree})\n+\t\tparent $(git rev-parse HEAD)\n+\t\tauthor A U Thor\n+\t\tcommitter A U Thor\n+\n+\t\tcommit: this is a commit with bad emails\n+\n+\t\tEOF\n+\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n+\t\tPACK=$(git pack-objects test \n\ngit-index-pack has a --strict option that can take an optional argument\nto provide a list of fsck issues to change their severity.\n--fsck-objects does not have such a utility, which would be useful if\none would like to be more lenient or strict on data integrity in a\nrepository.\n\nLike --strict, allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nRemove the \"For internal use only\" note for --fsck-objects, and document\nthe option. This won't often be used by the normal end user, but it\nturns out it is useful for Git forges like GitLab.\n\nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 10 ++++++++--\n builtin/index-pack.c | 5 +++--\n t/t5300-pack-object.sh | 29 ++++++++++++++++++++++++-----\n 3 files changed, 35 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex f7a98bbf9c8..916652d3b1b 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -96,8 +96,14 @@ default and \"Indexing objects\" when `--stdin` is specified.\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \n---fsck-objects::\n-\tFor internal use only.\n+--fsck-objects[==...]::\n+\tInstructs index-pack to check for broken objects, but unlike `--strict`,\n+\tdoes not choke on broken links. If `` is passed, it should be\n+\ta comma-separated list of `=` where `` and\n+\t`` are used to change the severity of `fsck` errors e.g.,\n+\t`--strict=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for\n+\tthe `fsck.` configuration options in `linkgit:git-fsck[1] for\n+\tmore information on the possible values of `` and ``.\n +\n Die if the pack contains broken objects. If the pack contains a tree\n pointing to a .gitmodules blob that does not exist, prints the hash of\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1e53ca23775..519162f5b91 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] [--fsck-objects[=]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n@@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t} else if (!strcmp(arg, \"--check-self-contained-and-connected\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcheck_self_contained_and_connected = 1;\n-\t\t\t} else if (!strcmp(arg, \"--fsck-objects\")) {\n+\t\t\t} else if (skip_to_optional_arg(arg, \"--fsck-objects\", &arg)) {\n \t\t\t\tdo_fsck_object = 1;\n+\t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n \t\t\t} else if (!strcmp(arg, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 496fffa0f8a..a58f91035d1 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n-test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n-\ttest_when_finished rm -rf strict &&\n+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '\n \tgit init strict &&\n \t(\n \t\tcd strict &&\n@@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n \n \t\tEOF\n \t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n-\t\tPACK=$(git pack-objects test pack-name\n \t)\n '\n \n+test_with_bad_commit () {\n+\tmust_fail_arg=\"$1\" &&\n+\tmust_pass_arg=\"$2\" &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_expect_fail git index-pack \"$must_fail_arg\" \"test-$(cat pack-name).pack\"\n+\t\tgit index-pack \"$must_pass_arg\" \"test-$(cat pack-name).pack\"\n+\t)\n+}\n+\n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_with_bad_commit --strict --strict=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '\n+\ttest_with_bad_commit --fsck-objects --fsck-objects=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '\n+\trm -rf strict\n+'\n+\n test_expect_success 'honor pack.packSizeLimit' '\n \tgit config pack.packSizeLimit 3m &&\n \tpackname_10=$(git pack-objects test-10 ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-26T18:03:37Z","receivedAt":"2024-01-26T18:03:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n> --strict. However this is a hidden argument that was not documented nor\n> tested. Though it is true that most users would not call this option\n> directly, (nor use index-pack for that matter) it is still useful to\n> document and test this feature.\n>\n> Signed-off-by: John Cai \n> ---\n> Documentation/git-index-pack.txt | 9 +++++++--\n> builtin/index-pack.c | 2 +-\n> t/t5300-pack-object.sh | 22 ++++++++++++++++++++++\n> 3 files changed, 30 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\n> index 6486620c3d8..f7a98bbf9c8 100644\n> --- a/Documentation/git-index-pack.txt\n> +++ b/Documentation/git-index-pack.txt\n> @@ -79,8 +79,13 @@ OPTIONS\n> \tto force the version for the generated pack index, and to force\n> \t64-bit index entries on objects located above the given offset.\n> \n> ---strict::\n> -\tDie, if the pack contains broken objects or links.\n> +--strict[==...]::\n> +\tDie, if the pack contains broken objects or links. If `` is passed,\n> +\tit should be a comma-separated list of `=` elements where\n> +\t`` and `` are used to change the severity of some possible\n> +\tissues, e.g., `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n\nThere no longer is , so I'll tweak the text perhaps like so:\n\n\tAn optional value that is a comma-separated list of '='\n\tcan be passed to change the severity of some possible issues, ...\n\nwhile queueing. Will probably do the same for the --fsck-objects\nside in the next patch.\n\nOther than that, thanks for a pleasant read.\n\n> +\tfor the `fsck.` configuration options in `linkgit:git-fsck[1] for\n> +\tmore information on the possible values of `` and ``.\n> \n> --progress-title::\n> \tFor internal use only.\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 1ea87e01f29..1e53ca23775 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -24,7 +24,7 @@\n> #include \"setup.h\"\n> \n> static const char index_pack_usage[] =\n> -\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n> +\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n> \n> struct object_entry {\n> \tstruct pack_idx_entry idx;\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index d402ec18b79..496fffa0f8a 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '\n> \t)\n> '\n> \n> +test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n> +\ttest_when_finished rm -rf strict &&\n> +\tgit init strict &&\n> +\t(\n> +\t\tcd strict &&\n> +\t\ttest_commit first hello &&\n> +\t\tcat >commit <<-EOF &&\n> +\t\ttree $(git rev-parse HEAD^{tree})\n> +\t\tparent $(git rev-parse HEAD)\n> +\t\tauthor A U Thor\n> +\t\tcommitter A U Thor\n> +\n> +\t\tcommit: this is a commit with bad emails\n> +\n> +\t\tEOF\n> +\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n> +\t\tPACK=$(git pack-objects test +\t\ttest_must_fail git index-pack --strict \"test-$PACK.pack\" &&\n> +\t\tgit index-pack --strict=\"missingEmail=ignore\" \"test-$PACK.pack\"\n> +\t)\n> +'\n> +\n> test_expect_success 'honor pack.packSizeLimit' '\n> \tgit config pack.packSizeLimit 3m &&\n> \tpackname_10=$(git pack-objects test-10 writes:\n\n> +--fsck-objects[==...]::\n> +\tInstructs index-pack to check for broken objects, but unlike `--strict`,\n> +\tdoes not choke on broken links. If `` is passed, it should be\n> +\ta comma-separated list of `=` where `` and\n> +\t`` are used to change the severity of `fsck` errors e.g.,\n> +\t`--strict=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for\n\nIn addition to the comment I made in my reply to 1/2, this should\nbe `--fsck-objects=\"missingEmail=ignore,badTagName=ignore\"`, I\nthink. Will treak locally.\n\nThanks.\n"},{"id":"487424","messageId":"47AE747C-69E0-467F-9EC6-860E7791C346@gmail.com","threadId":"60793","inReplyTo":"xmqqwmrwm08t.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-01-26T20:18:42Z","receivedAt":"2024-01-26T20:18:44Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 26 Jan 2024, at 13:13, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> +--fsck-objects[==...]::\n>> +\tInstructs index-pack to check for broken objects, but unlike `--strict`,\n>> +\tdoes not choke on broken links. If `` is passed, it should be\n>> +\ta comma-separated list of `=` where `` and\n>> +\t`` are used to change the severity of `fsck` errors e.g.,\n>> +\t`--strict=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for\n>\n> In addition to the comment I made in my reply to 1/2, this should\n> be `--fsck-objects=\"missingEmail=ignore,badTagName=ignore\"`, I\n> think. Will treak locally.\n\nGood catch. I might as well re-roll this since I forgot to add a Reviewed-by:\nChristian Couder trailer to the commits.\n\n>\n> Thanks.\n"},{"id":"487427","messageId":"pull.1658.v3.git.git.1706302749.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.v2.git.git.1706289180.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] index-pack: fsck honor checks","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-26T20:59:07Z","receivedAt":"2024-01-26T20:59:13Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-index-pack has a --strict mode that can take an optional argument to\nprovide a list of fsck issues to change their severity. --fsck-objects does\nnot have such a utility, which would be useful if one would like to be more\nlenient or strict on data integrity in a repository.\n\nLike --strict, Allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nChanges since V2:\n\n * fixed some typos in the documentation\n * added commit trailers\n\nChange since V1:\n\n * edited commit messages\n * clarified formatting in documentation for --strict= and --fsck-objects=\n\nJohn Cai (2):\n index-pack: test and document --strict==...\n index-pack: --fsck-objects to take an optional argument for fsck msgs\n\n Documentation/git-index-pack.txt | 26 +++++++++++++-------\n builtin/index-pack.c | 5 ++--\n t/t5300-pack-object.sh | 41 ++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 10 deletions(-)\n\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1658%2Fjohn-cai%2Fjc%2Findex-pack-fsck-honor-checks-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1658/john-cai/jc/index-pack-fsck-honor-checks-v3\nPull-Request: https://github.com/git/git/pull/1658\n\nRange-diff vs v2:\n\n 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=\n @@ Metadata\n Author: John Cai \n \n ## Commit message ##\n - index-pack: test and document --strict=\n + index-pack: test and document --strict==...\n \n 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n @@ Commit message\n directly, (nor use index-pack for that matter) it is still useful to\n document and test this feature.\n \n + Reviewed-by: Christian Couder \n Signed-off-by: John Cai \n \n ## Documentation/git-index-pack.txt ##\n @@ Documentation/git-index-pack.txt: OPTIONS\n ---strict::\n -\tDie, if the pack contains broken objects or links.\n +--strict[==...]::\n -+\tDie, if the pack contains broken objects or links. If `` is passed,\n -+\tit should be a comma-separated list of `=` elements where\n -+\t`` and `` are used to change the severity of some possible\n -+\tissues, e.g., `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry\n -+\tfor the `fsck.` configuration options in `linkgit:git-fsck[1] for\n -+\tmore information on the possible values of `` and ``.\n ++\tDie, if the pack contains broken objects or links. An optional\n ++\tcomma-separated list of `=` can be passed to change\n ++\tthe severity of some possible issues, e.g.,\n ++\t `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry for the\n ++\t`fsck.` configuration options in linkgit:git-fsck[1] for more\n ++\tinformation on the possible values of `` and ``.\n \n --progress-title::\n \tFor internal use only.\n @@ builtin/index-pack.c\n \n static const char index_pack_usage[] =\n -\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n -+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n ++\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n 2: cce63c6465f ! 2: a2b9adb93d8 index-pack: --fsck-objects to take an optional argument for fsck msgs\n @@ Commit message\n the option. This won't often be used by the normal end user, but it\n turns out it is useful for Git forges like GitLab.\n \n + Reviewed-by: Christian Couder \n Signed-off-by: John Cai \n \n ## Documentation/git-index-pack.txt ##\n @@ Documentation/git-index-pack.txt: default and \"Indexing objects\" when `--stdin`\n \n ---fsck-objects::\n -\tFor internal use only.\n -+--fsck-objects[==...]::\n -+\tInstructs index-pack to check for broken objects, but unlike `--strict`,\n -+\tdoes not choke on broken links. If `` is passed, it should be\n -+\ta comma-separated list of `=` where `` and\n -+\t`` are used to change the severity of `fsck` errors e.g.,\n -+\t`--strict=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for\n -+\tthe `fsck.` configuration options in `linkgit:git-fsck[1] for\n -+\tmore information on the possible values of `` and ``.\n ++--fsck-objects[==...]::\n ++\tDie if the pack contains broken objects. If the pack contains a tree\n ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n ++\tthat blob (for the caller to check) after the hash that goes into the\n ++\tname of the pack/idx file (see \"Notes\").\n +\n - Die if the pack contains broken objects. If the pack contains a tree\n - pointing to a .gitmodules blob that does not exist, prints the hash of\n +-Die if the pack contains broken objects. If the pack contains a tree\n +-pointing to a .gitmodules blob that does not exist, prints the hash of\n +-that blob (for the caller to check) after the hash that goes into the\n +-name of the pack/idx file (see \"Notes\").\n ++Unlike `--strict` however, don't choke on broken links. An optional\n ++comma-separated list of `=` can be passed to change the\n ++severity of some possible issues, e.g.,\n ++`--fsck-objects=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for the\n ++`fsck.` configuration options in linkgit:git-fsck[1] for more\n ++information on the possible values of `` and ``.\n + \n + --threads=::\n + \tSpecifies the number of threads to spawn when resolving\n \n ## builtin/index-pack.c ##\n @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n --\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] ( | --stdin [--fix-thin] [])\";\n -+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[=]] [--fsck-objects[=]] ( | --stdin [--fix-thin] [])\";\n +-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] ( | --stdin [--fix-thin] [])\";\n ++\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n\n-- \ngitgitgadget\n"},{"id":"487428","messageId":"cdf7fc7fe8a9d5664c9fdfb85f8c9293aa446c13.1706302749.git.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.v3.git.git.1706302749.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] index-pack: test and document --strict==...","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-26T20:59:08Z","receivedAt":"2024-01-26T20:59:13Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n--strict. However this is a hidden argument that was not documented nor\ntested. Though it is true that most users would not call this option\ndirectly, (nor use index-pack for that matter) it is still useful to\ndocument and test this feature.\n\nReviewed-by: Christian Couder \nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 9 +++++++--\n builtin/index-pack.c | 2 +-\n t/t5300-pack-object.sh | 22 ++++++++++++++++++++++\n 3 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 6486620c3d8..694bb9409bf 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -79,8 +79,13 @@ OPTIONS\n \tto force the version for the generated pack index, and to force\n \t64-bit index entries on objects located above the given offset.\n \n---strict::\n-\tDie, if the pack contains broken objects or links.\n+--strict[==...]::\n+\tDie, if the pack contains broken objects or links. An optional\n+\tcomma-separated list of `=` can be passed to change\n+\tthe severity of some possible issues, e.g.,\n+\t `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry for the\n+\t`fsck.` configuration options in linkgit:git-fsck[1] for more\n+\tinformation on the possible values of `` and ``.\n \n --progress-title::\n \tFor internal use only.\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1ea87e01f29..240c7021168 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d402ec18b79..496fffa0f8a 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_when_finished rm -rf strict &&\n+\tgit init strict &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_commit first hello &&\n+\t\tcat >commit <<-EOF &&\n+\t\ttree $(git rev-parse HEAD^{tree})\n+\t\tparent $(git rev-parse HEAD)\n+\t\tauthor A U Thor\n+\t\tcommitter A U Thor\n+\n+\t\tcommit: this is a commit with bad emails\n+\n+\t\tEOF\n+\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n+\t\tPACK=$(git pack-objects test \n\ngit-index-pack has a --strict option that can take an optional argument\nto provide a list of fsck issues to change their severity.\n--fsck-objects does not have such a utility, which would be useful if\none would like to be more lenient or strict on data integrity in a\nrepository.\n\nLike --strict, allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nRemove the \"For internal use only\" note for --fsck-objects, and document\nthe option. This won't often be used by the normal end user, but it\nturns out it is useful for Git forges like GitLab.\n\nReviewed-by: Christian Couder \nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 17 +++++++++++------\n builtin/index-pack.c | 5 +++--\n t/t5300-pack-object.sh | 29 ++++++++++++++++++++++++-----\n 3 files changed, 38 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 694bb9409bf..3db1062d1e4 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -96,13 +96,18 @@ default and \"Indexing objects\" when `--stdin` is specified.\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \n---fsck-objects::\n-\tFor internal use only.\n+--fsck-objects[==...]::\n+\tDie if the pack contains broken objects. If the pack contains a tree\n+\tpointing to a .gitmodules blob that does not exist, prints the hash of\n+\tthat blob (for the caller to check) after the hash that goes into the\n+\tname of the pack/idx file (see \"Notes\").\n +\n-Die if the pack contains broken objects. If the pack contains a tree\n-pointing to a .gitmodules blob that does not exist, prints the hash of\n-that blob (for the caller to check) after the hash that goes into the\n-name of the pack/idx file (see \"Notes\").\n+Unlike `--strict` however, don't choke on broken links. An optional\n+comma-separated list of `=` can be passed to change the\n+severity of some possible issues, e.g.,\n+`--fsck-objects=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for the\n+`fsck.` configuration options in linkgit:git-fsck[1] for more\n+information on the possible values of `` and ``.\n \n --threads=::\n \tSpecifies the number of threads to spawn when resolving\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 240c7021168..a3a37bd215d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n@@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t} else if (!strcmp(arg, \"--check-self-contained-and-connected\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcheck_self_contained_and_connected = 1;\n-\t\t\t} else if (!strcmp(arg, \"--fsck-objects\")) {\n+\t\t\t} else if (skip_to_optional_arg(arg, \"--fsck-objects\", &arg)) {\n \t\t\t\tdo_fsck_object = 1;\n+\t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n \t\t\t} else if (!strcmp(arg, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 496fffa0f8a..a58f91035d1 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n-test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n-\ttest_when_finished rm -rf strict &&\n+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '\n \tgit init strict &&\n \t(\n \t\tcd strict &&\n@@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n \n \t\tEOF\n \t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n-\t\tPACK=$(git pack-objects test pack-name\n \t)\n '\n \n+test_with_bad_commit () {\n+\tmust_fail_arg=\"$1\" &&\n+\tmust_pass_arg=\"$2\" &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_expect_fail git index-pack \"$must_fail_arg\" \"test-$(cat pack-name).pack\"\n+\t\tgit index-pack \"$must_pass_arg\" \"test-$(cat pack-name).pack\"\n+\t)\n+}\n+\n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_with_bad_commit --strict --strict=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '\n+\ttest_with_bad_commit --fsck-objects --fsck-objects=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '\n+\trm -rf strict\n+'\n+\n test_expect_success 'honor pack.packSizeLimit' '\n \tgit config pack.packSizeLimit 3m &&\n \tpackname_10=$(git pack-objects test-10 writes:\n\n> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=\n> @@ Metadata\n> Author: John Cai \n> \n> ## Commit message ##\n> - index-pack: test and document --strict=\n> + index-pack: test and document --strict==...\n\nAh, I missed this one. Nice spotting.\n\n> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n> @@ Commit message\n> directly, (nor use index-pack for that matter) it is still useful to\n> document and test this feature.\n> \n> + Reviewed-by: Christian Couder \n> Signed-off-by: John Cai \n\nI haven't seen Christian involved (by getting Cc'ed these patches,\nsending out review comments, or giving his Reviewed-by:) during\nthese three rounds of this topic. I'll wait until I hear from him\nbefore queuing this, just to be safe.\n\n> ++\tDie, if the pack contains broken objects or links. An optional\n> ++\tcomma-separated list of `=` can be passed to change\n> ++\tthe severity of some possible issues, e.g.,\n> ++\t `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry for the\n> ++\t`fsck.` configuration options in linkgit:git-fsck[1] for more\n> ++\tinformation on the possible values of `` and ``.\n\nThis is much better than the tentative text I tweaked. Nice.\n\n> ++--fsck-objects[==...]::\n> ++\tDie if the pack contains broken objects. If the pack contains a tree\n> ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n> ++\tthat blob (for the caller to check) after the hash that goes into the\n> ++\tname of the pack/idx file (see \"Notes\").\n\nNot a new problem bit I have to wonder what happens if the pack\ncontains many trees that point at different blobs for \".gitmodules\"\npath and many of these blobs are not included in the packfile? Will\nthe caller receive all of these blob object names so that they can\nbe verified? The reference to the \"Notes\" only refer to the fact\nthat usually a single hash value that is used in constructing the\nname of the packfile \"pack-.pack\" is emitted to the\nstandard output, which is not wrong per se, but does not help\nreaders very much wrt to understanding this.\n\n[jc: dragging JTan into the thread, as this comes from his 5476e1ef\n(fetch-pack: print and use dangling .gitmodules, 2021-02-22)].\n\n> +\n> ++Unlike `--strict` however, don't choke on broken links. An optional\n\nYou'd need a comma on both sides of \"however\" used like this, I\nthink. \n\nIn any case, I thought your original construction to have this\n\"unlike\" immediately after \"die on broken objects\" was far easier to\nfollow.\n\nThanks.\n"},{"id":"487431","messageId":"BF772E83-2BFE-4652-A742-67FADF3D8FE2@gmail.com","threadId":"60793","inReplyTo":"xmqqfryjn686.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-01-26T22:11:14Z","receivedAt":"2024-01-26T22:11:16Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 26 Jan 2024, at 16:18, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=\n>> @@ Metadata\n>> Author: John Cai \n>>\n>> ## Commit message ##\n>> - index-pack: test and document --strict=\n>> + index-pack: test and document --strict==...\n>\n> Ah, I missed this one. Nice spotting.\n>\n>> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n>> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n>> @@ Commit message\n>> directly, (nor use index-pack for that matter) it is still useful to\n>> document and test this feature.\n>>\n>> + Reviewed-by: Christian Couder \n>> Signed-off-by: John Cai \n>\n> I haven't seen Christian involved (by getting Cc'ed these patches,\n> sending out review comments, or giving his Reviewed-by:) during\n> these three rounds of this topic. I'll wait until I hear from him\n> before queuing this, just to be safe.\n\nChristian was involved on an off-list review of this patch series. You can see\nit in [1].\n\n\n1. https://gitlab.com/gitlab-org/git/-/merge_requests/88\n>\n>> ++\tDie, if the pack contains broken objects or links. An optional\n>> ++\tcomma-separated list of `=` can be passed to change\n>> ++\tthe severity of some possible issues, e.g.,\n>> ++\t `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry for the\n>> ++\t`fsck.` configuration options in linkgit:git-fsck[1] for more\n>> ++\tinformation on the possible values of `` and ``.\n>\n> This is much better than the tentative text I tweaked. Nice.\n>\n>> ++--fsck-objects[==...]::\n>> ++\tDie if the pack contains broken objects. If the pack contains a tree\n>> ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n>> ++\tthat blob (for the caller to check) after the hash that goes into the\n>> ++\tname of the pack/idx file (see \"Notes\").\n>\n> Not a new problem bit I have to wonder what happens if the pack\n> contains many trees that point at different blobs for \".gitmodules\"\n> path and many of these blobs are not included in the packfile? Will\n> the caller receive all of these blob object names so that they can\n> be verified? The reference to the \"Notes\" only refer to the fact\n> that usually a single hash value that is used in constructing the\n> name of the packfile \"pack-.pack\" is emitted to the\n> standard output, which is not wrong per se, but does not help\n> readers very much wrt to understanding this.\n>\n> [jc: dragging JTan into the thread, as this comes from his 5476e1ef\n> (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].\n\nsounds good, will wait for some clarification here\n>\n>> +\n>> ++Unlike `--strict` however, don't choke on broken links. An optional\n>\n> You'd need a comma on both sides of \"however\" used like this, I\n> think.\n\ngood catch\n\n>\n> In any case, I thought your original construction to have this\n> \"unlike\" immediately after \"die on broken objects\" was far easier to\n> follow.\n\nI'll reformulate this to be clearer. From the previous version I realized I\ndidn't take into account the pre-existing \"Die if the pack contains broken\nobjects\" block so I put it at the beginning. But now I think you're right in\nthat the \"Unlike...\" comes too late.\n\n>\n> Thanks.\n"},{"id":"487432","messageId":"20240126221357.2940676-1-jonathantanmy@google.com","threadId":"60793","inReplyTo":"xmqqfryjn686.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-01-26T22:13:57Z","receivedAt":"2024-01-26T22:14:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> > ++--fsck-objects[==...]::\n> > ++\tDie if the pack contains broken objects. If the pack contains a tree\n> > ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n> > ++\tthat blob (for the caller to check) after the hash that goes into the\n> > ++\tname of the pack/idx file (see \"Notes\").\n> \n> Not a new problem bit I have to wonder what happens if the pack\n> contains many trees that point at different blobs for \".gitmodules\"\n> path and many of these blobs are not included in the packfile? Will\n> the caller receive all of these blob object names so that they can\n> be verified? The reference to the \"Notes\" only refer to the fact\n> that usually a single hash value that is used in constructing the\n> name of the packfile \"pack-.pack\" is emitted to the\n> standard output, which is not wrong per se, but does not help\n> readers very much wrt to understanding this.\n> \n> [jc: dragging JTan into the thread, as this comes from his 5476e1ef\n> (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].\n\nAh...I can see how that documentation isn't clear. The intention of that\ncommit is to check every link to a .gitmodules blob. The tests perhaps\nshould have been written with 2 .gitmodules blobs (in separate commits),\nbut I think the production code works: I tried changing the test to have\n2 commits each with their own .gitmodules blob, and error messages were\nprinted for both blobs.\n\n(If someone changes that test, e.g. to have 2 blobs, the \">h\" in the\n\"configure_exclusion\" invocations look superfluous and is perhaps a\ncopy-and-paste error from other tests that needed the hash later.)\n"},{"id":"487446","messageId":"BE30DB47-1488-40A3-BD0C-804F97DE0C88@gmail.com","threadId":"60793","inReplyTo":"20240126221357.2940676-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-01-27T02:31:37Z","receivedAt":"2024-01-27T02:31:40Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Jonathan,\n\nOn 26 Jan 2024, at 17:13, Jonathan Tan wrote:\n\n> Junio C Hamano writes:\n>>> ++--fsck-objects[==...]::\n>>> ++\tDie if the pack contains broken objects. If the pack contains a tree\n>>> ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n>>> ++\tthat blob (for the caller to check) after the hash that goes into the\n>>> ++\tname of the pack/idx file (see \"Notes\").\n>>\n>> Not a new problem bit I have to wonder what happens if the pack\n>> contains many trees that point at different blobs for \".gitmodules\"\n>> path and many of these blobs are not included in the packfile? Will\n>> the caller receive all of these blob object names so that they can\n>> be verified? The reference to the \"Notes\" only refer to the fact\n>> that usually a single hash value that is used in constructing the\n>> name of the packfile \"pack-.pack\" is emitted to the\n>> standard output, which is not wrong per se, but does not help\n>> readers very much wrt to understanding this.\n>>\n>> [jc: dragging JTan into the thread, as this comes from his 5476e1ef\n>> (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].\n>\n> Ah...I can see how that documentation isn't clear. The intention of that\n> commit is to check every link to a .gitmodules blob. The tests perhaps\n> should have been written with 2 .gitmodules blobs (in separate commits),\n> but I think the production code works: I tried changing the test to have\n> 2 commits each with their own .gitmodules blob, and error messages were\n> printed for both blobs.\n\nThanks for clarifying! Would you mind providing a patch to revise the wording\nhere to make it clearer? I would try but I feel like I might get the wording\nwrong.\n>\n> (If someone changes that test, e.g. to have 2 blobs, the \">h\" in the\n> \"configure_exclusion\" invocations look superfluous and is perhaps a\n> copy-and-paste error from other tests that needed the hash later.)\n\nthanks\nJohn\n"},{"id":"487506","messageId":"ZbeI0ksoUQEkbt90@tanuki","threadId":"60793","inReplyTo":"BF772E83-2BFE-4652-A742-67FADF3D8FE2@gmail.com","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:15:30Z","receivedAt":"2024-01-29T11:15:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 26, 2024 at 05:11:14PM -0500, John Cai wrote:\n> Hi Junio,\n> \n> On 26 Jan 2024, at 16:18, Junio C Hamano wrote:\n> \n> > \"John Cai via GitGitGadget\" writes:\n> >\n> >> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=\n> >> @@ Metadata\n> >> Author: John Cai \n> >>\n> >> ## Commit message ##\n> >> - index-pack: test and document --strict=\n> >> + index-pack: test and document --strict==...\n> >\n> > Ah, I missed this one. Nice spotting.\n> >\n> >> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n> >> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n> >> @@ Commit message\n> >> directly, (nor use index-pack for that matter) it is still useful to\n> >> document and test this feature.\n> >>\n> >> + Reviewed-by: Christian Couder \n> >> Signed-off-by: John Cai \n> >\n> > I haven't seen Christian involved (by getting Cc'ed these patches,\n> > sending out review comments, or giving his Reviewed-by:) during\n> > these three rounds of this topic. I'll wait until I hear from him\n> > before queuing this, just to be safe.\n> \n> Christian was involved on an off-list review of this patch series. You can see\n> it in [1].\n> \n> 1. https://gitlab.com/gitlab-org/git/-/merge_requests/88\n\nI'm always a bit hesitant to add trailers referring to off-list reviews\nto commits. It's impossible for a future reader to discover how that\ntrailer came to be by just using the mailing list archive, and expecting\nthem to use third-party services to verify them feels wrong to me.\n\nIt's part of the reason why I'm pushing more into the direction of\non-list reviews at GitLab. It makes it a lot more obvious how such a\nReviewed-by came to be and keeps things self-contained on the mailing\nlist. It also grows new contributors who are becoming more familiar with\nhow the Git mailing list works. If such a review already happened\ninternally due to whatever reason then I think it ought to be fine for\nthat reviewer to chime in saying that they have already reviewed the\npatch series and that things look good to them.\n\nPatrick\n"},{"id":"487528","messageId":"xmqqplxkjbwf.fsf@gitster.g","threadId":"60793","inReplyTo":"ZbeI0ksoUQEkbt90@tanuki","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T17:18:56Z","receivedAt":"2024-01-29T17:19:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I'm always a bit hesitant to add trailers referring to off-list reviews\n> to commits. It's impossible for a future reader to discover how that\n> trailer came to be by just using the mailing list archive, and expecting\n> them to use third-party services to verify them feels wrong to me.\n>\n> It's part of the reason why I'm pushing more into the direction of\n> on-list reviews at GitLab. It makes it a lot more obvious how such a\n> Reviewed-by came to be and keeps things self-contained on the mailing\n> list. It also grows new contributors who are becoming more familiar with\n> how the Git mailing list works. If such a review already happened\n> internally due to whatever reason then I think it ought to be fine for\n> that reviewer to chime in saying that they have already reviewed the\n> patch series and that things look good to them.\n\nThanks. That would improve clarifying a situation like this one\n(eh, actually, once it is done this particular situation wouldn't\nneed any clarification).\n\n"},{"id":"487667","messageId":"20240131223032.4065897-1-jonathantanmy@google.com","threadId":"60793","inReplyTo":"BE30DB47-1488-40A3-BD0C-804F97DE0C88@gmail.com","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-01-31T22:30:32Z","receivedAt":"2024-01-31T22:30:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"John Cai writes:\n> Hi Jonathan,\n> \n> On 26 Jan 2024, at 17:13, Jonathan Tan wrote:\n> \n> > Junio C Hamano writes:\n> >>> ++--fsck-objects[==...]::\n> >>> ++\tDie if the pack contains broken objects. If the pack contains a tree\n> >>> ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n> >>> ++\tthat blob (for the caller to check) after the hash that goes into the\n> >>> ++\tname of the pack/idx file (see \"Notes\").\n\n> Thanks for clarifying! Would you mind providing a patch to revise the wording\n> here to make it clearer? I would try but I feel like I might get the wording\n> wrong.\n\nI think the wording there is already mostly correct, except maybe make\neverything plural (a tree -> trees, a .gitmodules blob -> .gitmodules\nblobs, hash of that blob -> hashes of those blobs). We might also need\nto modify a test to show that the current code indeed handles the plural\nsituation correctly. I don't have time right now to get to this, so\nhopefully someone could pick this up.\n"},{"id":"487694","messageId":"222CEC85-73B0-49CC-BB81-D6E6F36018B3@gmail.com","threadId":"60793","inReplyTo":"20240131223032.4065897-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 0/2] index-pack: fsck honor checks","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-02-01T01:34:54Z","receivedAt":"2024-02-01T01:34:56Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Jonathan,\n\nOn 31 Jan 2024, at 17:30, Jonathan Tan wrote:\n\n> John Cai writes:\n>> Hi Jonathan,\n>>\n>> On 26 Jan 2024, at 17:13, Jonathan Tan wrote:\n>>\n>>> Junio C Hamano writes:\n>>>>> ++--fsck-objects[==...]::\n>>>>> ++\tDie if the pack contains broken objects. If the pack contains a tree\n>>>>> ++\tpointing to a .gitmodules blob that does not exist, prints the hash of\n>>>>> ++\tthat blob (for the caller to check) after the hash that goes into the\n>>>>> ++\tname of the pack/idx file (see \"Notes\").\n>\n>> Thanks for clarifying! Would you mind providing a patch to revise the wording\n>> here to make it clearer? I would try but I feel like I might get the wording\n>> wrong.\n>\n> I think the wording there is already mostly correct, except maybe make\n> everything plural (a tree -> trees, a .gitmodules blob -> .gitmodules\n> blobs, hash of that blob -> hashes of those blobs). We might also need\n> to modify a test to show that the current code indeed handles the plural\n> situation correctly. I don't have time right now to get to this, so\n> hopefully someone could pick this up.\n\nThanks! It sounds like we may want to tackle this as part of another patch.\n\nJohn\n"},{"id":"487695","messageId":"pull.1658.v4.git.git.1706751483.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.v3.git.git.1706302749.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] index-pack: fsck honor checks","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-01T01:38:00Z","receivedAt":"2024-02-01T01:38:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-index-pack has a --strict mode that can take an optional argument to\nprovide a list of fsck issues to change their severity. --fsck-objects does\nnot have such a utility, which would be useful if one would like to be more\nlenient or strict on data integrity in a repository.\n\nLike --strict, Allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nChanges since V3:\n\n * clarification of --fsck-objects documentation wording\n\nChanges since V2:\n\n * fixed some typos in the documentation\n * added commit trailers\n\nChange since V1:\n\n * edited commit messages\n * clarified formatting in documentation for --strict= and --fsck-objects=\n\nJohn Cai (2):\n index-pack: test and document --strict==...\n index-pack: --fsck-objects to take an optional argument for fsck msgs\n\n Documentation/git-index-pack.txt | 26 +++++++++++++-------\n builtin/index-pack.c | 5 ++--\n t/t5300-pack-object.sh | 41 ++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 10 deletions(-)\n\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1658%2Fjohn-cai%2Fjc%2Findex-pack-fsck-honor-checks-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1658/john-cai/jc/index-pack-fsck-honor-checks-v4\nPull-Request: https://github.com/git/git/pull/1658\n\nRange-diff vs v3:\n\n 1: cdf7fc7fe8a = 1: cdf7fc7fe8a index-pack: test and document --strict==...\n 2: a2b9adb93d8 ! 2: f29ab9136fb index-pack: --fsck-objects to take an optional argument for fsck msgs\n @@ Documentation/git-index-pack.txt: default and \"Indexing objects\" when `--stdin`\n ---fsck-objects::\n -\tFor internal use only.\n +--fsck-objects[==...]::\n -+\tDie if the pack contains broken objects. If the pack contains a tree\n -+\tpointing to a .gitmodules blob that does not exist, prints the hash of\n -+\tthat blob (for the caller to check) after the hash that goes into the\n -+\tname of the pack/idx file (see \"Notes\").\n ++\tDie if the pack contains broken objects, but unlike `--strict`, don't\n ++\tchoke on broken links. If the pack contains a tree pointing to a\n ++\t.gitmodules blob that does not exist, prints the hash of that blob\n ++\t(for the caller to check) after the hash that goes into the name of the\n ++\tpack/idx file (see \"Notes\").\n +\n -Die if the pack contains broken objects. If the pack contains a tree\n -pointing to a .gitmodules blob that does not exist, prints the hash of\n -that blob (for the caller to check) after the hash that goes into the\n -name of the pack/idx file (see \"Notes\").\n -+Unlike `--strict` however, don't choke on broken links. An optional\n -+comma-separated list of `=` can be passed to change the\n -+severity of some possible issues, e.g.,\n ++An optional comma-separated list of `=` can be passed to\n ++change the severity of some possible issues, e.g.,\n +`--fsck-objects=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for the\n +`fsck.` configuration options in linkgit:git-fsck[1] for more\n +information on the possible values of `` and ``.\n\n-- \ngitgitgadget\n"},{"id":"487696","messageId":"cdf7fc7fe8a9d5664c9fdfb85f8c9293aa446c13.1706751483.git.gitgitgadget@gmail.com","threadId":"60793","inReplyTo":"pull.1658.v4.git.git.1706751483.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] index-pack: test and document --strict==...","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-01T01:38:01Z","receivedAt":"2024-02-01T01:38:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n5d477a334a (fsck (receive-pack): allow demoting errors to warnings,\n2015-06-22) allowed a list of fsck msg to downgrade to be passed to\n--strict. However this is a hidden argument that was not documented nor\ntested. Though it is true that most users would not call this option\ndirectly, (nor use index-pack for that matter) it is still useful to\ndocument and test this feature.\n\nReviewed-by: Christian Couder \nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 9 +++++++--\n builtin/index-pack.c | 2 +-\n t/t5300-pack-object.sh | 22 ++++++++++++++++++++++\n 3 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 6486620c3d8..694bb9409bf 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -79,8 +79,13 @@ OPTIONS\n \tto force the version for the generated pack index, and to force\n \t64-bit index entries on objects located above the given offset.\n \n---strict::\n-\tDie, if the pack contains broken objects or links.\n+--strict[==...]::\n+\tDie, if the pack contains broken objects or links. An optional\n+\tcomma-separated list of `=` can be passed to change\n+\tthe severity of some possible issues, e.g.,\n+\t `--strict=\"missingEmail=ignore,badTagName=error\"`. See the entry for the\n+\t`fsck.` configuration options in linkgit:git-fsck[1] for more\n+\tinformation on the possible values of `` and ``.\n \n --progress-title::\n \tFor internal use only.\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1ea87e01f29..240c7021168 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d402ec18b79..496fffa0f8a 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_when_finished rm -rf strict &&\n+\tgit init strict &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_commit first hello &&\n+\t\tcat >commit <<-EOF &&\n+\t\ttree $(git rev-parse HEAD^{tree})\n+\t\tparent $(git rev-parse HEAD)\n+\t\tauthor A U Thor\n+\t\tcommitter A U Thor\n+\n+\t\tcommit: this is a commit with bad emails\n+\n+\t\tEOF\n+\t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n+\t\tPACK=$(git pack-objects test \n\ngit-index-pack has a --strict option that can take an optional argument\nto provide a list of fsck issues to change their severity.\n--fsck-objects does not have such a utility, which would be useful if\none would like to be more lenient or strict on data integrity in a\nrepository.\n\nLike --strict, allow --fsck-objects to also take a list of fsck msgs to\nchange the severity.\n\nRemove the \"For internal use only\" note for --fsck-objects, and document\nthe option. This won't often be used by the normal end user, but it\nturns out it is useful for Git forges like GitLab.\n\nReviewed-by: Christian Couder \nSigned-off-by: John Cai \n---\n Documentation/git-index-pack.txt | 17 +++++++++++------\n builtin/index-pack.c | 5 +++--\n t/t5300-pack-object.sh | 29 ++++++++++++++++++++++++-----\n 3 files changed, 38 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 694bb9409bf..5a20deefd5f 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -96,13 +96,18 @@ default and \"Indexing objects\" when `--stdin` is specified.\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \n---fsck-objects::\n-\tFor internal use only.\n+--fsck-objects[==...]::\n+\tDie if the pack contains broken objects, but unlike `--strict`, don't\n+\tchoke on broken links. If the pack contains a tree pointing to a\n+\t.gitmodules blob that does not exist, prints the hash of that blob\n+\t(for the caller to check) after the hash that goes into the name of the\n+\tpack/idx file (see \"Notes\").\n +\n-Die if the pack contains broken objects. If the pack contains a tree\n-pointing to a .gitmodules blob that does not exist, prints the hash of\n-that blob (for the caller to check) after the hash that goes into the\n-name of the pack/idx file (see \"Notes\").\n+An optional comma-separated list of `=` can be passed to\n+change the severity of some possible issues, e.g.,\n+`--fsck-objects=\"missingEmail=ignore,badTagName=ignore\"`. See the entry for the\n+`fsck.` configuration options in linkgit:git-fsck[1] for more\n+information on the possible values of `` and ``.\n \n --threads=::\n \tSpecifies the number of threads to spawn when resolving\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 240c7021168..a3a37bd215d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -24,7 +24,7 @@\n #include \"setup.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n@@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t} else if (!strcmp(arg, \"--check-self-contained-and-connected\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcheck_self_contained_and_connected = 1;\n-\t\t\t} else if (!strcmp(arg, \"--fsck-objects\")) {\n+\t\t\t} else if (skip_to_optional_arg(arg, \"--fsck-objects\", &arg)) {\n \t\t\t\tdo_fsck_object = 1;\n+\t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n \t\t\t} else if (!strcmp(arg, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 496fffa0f8a..a58f91035d1 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '\n \t)\n '\n \n-test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n-\ttest_when_finished rm -rf strict &&\n+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '\n \tgit init strict &&\n \t(\n \t\tcd strict &&\n@@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n \n \t\tEOF\n \t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n-\t\tPACK=$(git pack-objects test pack-name\n \t)\n '\n \n+test_with_bad_commit () {\n+\tmust_fail_arg=\"$1\" &&\n+\tmust_pass_arg=\"$2\" &&\n+\t(\n+\t\tcd strict &&\n+\t\ttest_expect_fail git index-pack \"$must_fail_arg\" \"test-$(cat pack-name).pack\"\n+\t\tgit index-pack \"$must_pass_arg\" \"test-$(cat pack-name).pack\"\n+\t)\n+}\n+\n+test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n+\ttest_with_bad_commit --strict --strict=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '\n+\ttest_with_bad_commit --fsck-objects --fsck-objects=\"missingEmail=ignore\"\n+'\n+\n+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '\n+\trm -rf strict\n+'\n+\n test_expect_success 'honor pack.packSizeLimit' '\n \tgit config pack.packSizeLimit 3m &&\n \tpackname_10=$(git pack-objects test-10 writes:\n\n>>> Thanks for clarifying! Would you mind providing a patch to revise the wording\n>>> here to make it clearer? I would try but I feel like I might get the wording\n>>> wrong.\n>>\n>> I think the wording there is already mostly correct, except maybe make\n>> everything plural (a tree -> trees, a .gitmodules blob -> .gitmodules\n>> blobs, hash of that blob -> hashes of those blobs). We might also need\n>> to modify a test to show that the current code indeed handles the plural\n>> situation correctly. I don't have time right now to get to this, so\n>> hopefully someone could pick this up.\n>\n> Thanks! It sounds like we may want to tackle this as part of another patch.\n\nYeah, the existing documentation has been with our users for some\ntime, and it is not ultra urgent to fix it in that sense. I'd say\nthat it can even wait until JTan gets bored with what he's doing and\nneeds some distraction himself ;-) \n\nAs long as our collective mind remembers it as #leftoverbits it\nwould be sufficient.\n\nThanks, both.\n"},{"id":"487822","messageId":"CAP8UFD3zNgY2qXQo3eU5fAMU6T-uWPMcTR473rgyrtF3pK_1-w@mail.gmail.com","threadId":"60793","inReplyTo":"pull.1658.v4.git.git.1706751483.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/2] index-pack: fsck honor checks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-02T15:48:22Z","receivedAt":"2024-02-02T15:48:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"(John, sorry for having already sent this only to you.)\n\nOn Thu, Feb 1, 2024 at 2:47 AM John Cai via GitGitGadget\n wrote:\n>\n> git-index-pack has a --strict mode that can take an optional argument to\n> provide a list of fsck issues to change their severity. --fsck-objects does\n> not have such a utility, which would be useful if one would like to be more\n> lenient or strict on data integrity in a repository.\n>\n> Like --strict, Allow --fsck-objects to also take a list of fsck msgs to\n> change the severity.\n>\n> Changes since V3:\n>\n> * clarification of --fsck-objects documentation wording\n>\n> Changes since V2:\n>\n> * fixed some typos in the documentation\n> * added commit trailers\n>\n> Change since V1:\n>\n> * edited commit messages\n> * clarified formatting in documentation for --strict= and --fsck-objects=\n>\n> John Cai (2):\n> index-pack: test and document --strict==...\n> index-pack: --fsck-objects to take an optional argument for fsck msgs\n\nI reviewed internally on GitLab the initial versions of this series\nand I reviewed this version 4. It looks great to me, so I am happy to\ngive my \"Reviewed-by:\".\n\nThanks!\n"},{"id":"490275","messageId":"20240308222439.GB1908@szeder.dev","threadId":"60793","inReplyTo":"f29ab9136fb4c23c5700a73731a5e220f92b7c30.1706751483.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-03-08T22:24:39Z","receivedAt":"2024-03-08T22:24:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Feb 01, 2024 at 01:38:02AM +0000, John Cai via GitGitGadget wrote:\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index 496fffa0f8a..a58f91035d1 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '\n> \t)\n> '\n> \n> -test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n> -\ttest_when_finished rm -rf strict &&\n> +test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '\n> \tgit init strict &&\n> \t(\n> \t\tcd strict &&\n> @@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n> \n> \t\tEOF\n> \t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n> -\t\tPACK=$(git pack-objects test -\t\ttest_must_fail git index-pack --strict \"test-$PACK.pack\" &&\n> -\t\tgit index-pack --strict=\"missingEmail=ignore\" \"test-$PACK.pack\"\n> +\t\tgit pack-objects test pack-name\n> \t)\n> '\n> \n> +test_with_bad_commit () {\n> +\tmust_fail_arg=\"$1\" &&\n> +\tmust_pass_arg=\"$2\" &&\n> +\t(\n> +\t\tcd strict &&\n> +\t\ttest_expect_fail git index-pack \"$must_fail_arg\" \"test-$(cat pack-name).pack\"\n\nThere is no such command as 'test_expect_fail', resulting in:\n\n expecting success of 5300.34 'index-pack with --strict downgrading fsck msgs':\n test_with_bad_commit --strict --strict=\"missingEmail=ignore\"\n\n + test_with_bad_commit --strict --strict=missingEmail=ignore\n + must_fail_arg=--strict\n + must_pass_arg=--strict=missingEmail=ignore\n + cd strict\n + cat pack-name\n + test_expect_fail git index-pack --strict test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack\n ./t5300-pack-object.sh: 468: eval: test_expect_fail: not found\n + cat pack-name\n + git index-pack --strict=missingEmail=ignore test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack\n e4e1649155bf444fbd9cd85e376628d6eaf3d3bd\n\n ok 34 - index-pack with --strict downgrading fsck msgs\n\nThe missing command should fail the test, but it doesn't, because the\n&&-chain is broken on this line as well.\n\n"},{"id":"490282","messageId":"67B9F50F-4499-4059-A49E-B70CBD36B9B2@gmail.com","threadId":"60793","inReplyTo":"20240308222439.GB1908@szeder.dev","subject":"Re: [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-03-09T01:55:21Z","receivedAt":"2024-03-09T01:55:23Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Szeder,\n\nOn 8 Mar 2024, at 17:24, SZEDER Gábor wrote:\n\n> On Thu, Feb 01, 2024 at 01:38:02AM +0000, John Cai via GitGitGadget wrote:\n>> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n>> index 496fffa0f8a..a58f91035d1 100755\n>> --- a/t/t5300-pack-object.sh\n>> +++ b/t/t5300-pack-object.sh\n>> @@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '\n>> \t)\n>> '\n>>\n>> -test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n>> -\ttest_when_finished rm -rf strict &&\n>> +test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '\n>> \tgit init strict &&\n>> \t(\n>> \t\tcd strict &&\n>> @@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '\n>>\n>> \t\tEOF\n>> \t\tgit hash-object --literally -t commit -w --stdin commit_list &&\n>> -\t\tPACK=$(git pack-objects test > -\t\ttest_must_fail git index-pack --strict \"test-$PACK.pack\" &&\n>> -\t\tgit index-pack --strict=\"missingEmail=ignore\" \"test-$PACK.pack\"\n>> +\t\tgit pack-objects test pack-name\n>> \t)\n>> '\n>>\n>> +test_with_bad_commit () {\n>> +\tmust_fail_arg=\"$1\" &&\n>> +\tmust_pass_arg=\"$2\" &&\n>> +\t(\n>> +\t\tcd strict &&\n>> +\t\ttest_expect_fail git index-pack \"$must_fail_arg\" \"test-$(cat pack-name).pack\"\n>\n> There is no such command as 'test_expect_fail', resulting in:\n\nIndeed, thanks for catching this.\n\n>\n> expecting success of 5300.34 'index-pack with --strict downgrading fsck msgs':\n> test_with_bad_commit --strict --strict=\"missingEmail=ignore\"\n>\n> + test_with_bad_commit --strict --strict=missingEmail=ignore\n> + must_fail_arg=--strict\n> + must_pass_arg=--strict=missingEmail=ignore\n> + cd strict\n> + cat pack-name\n> + test_expect_fail git index-pack --strict test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack\n> ./t5300-pack-object.sh: 468: eval: test_expect_fail: not found\n> + cat pack-name\n> + git index-pack --strict=missingEmail=ignore test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack\n> e4e1649155bf444fbd9cd85e376628d6eaf3d3bd\n>\n> ok 34 - index-pack with --strict downgrading fsck msgs\n>\n> The missing command should fail the test, but it doesn't, because the\n> &&-chain is broken on this line as well.\n\nyes will need to fix this as well\n\nthanks\nJohn\n"}]}