{"thread":{"id":"49227","subject":"[PATCH v3 0/7] use oidset for skiplist + docs + tests + comment support","startedAt":"2018-08-27T19:43:37Z","lastAt":"2018-09-03T14:50:10Z","messageCount":32,"participants":["Ævar Arnfjörð Bjarmason","René Scharfe"],"isPatch":true,"patchVersion":3,"patchTotal":7},"messages":[{"id":"356609","messageId":"20180827194323.17055-1-avarab@gmail.com","threadId":"49227","inReplyTo":null,"subject":"[PATCH v3 0/7] use oidset for skiplist + docs + tests + comment support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:16Z","receivedAt":"2018-08-27T19:43:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Aug 27 2018, René Scharfe wrote:\n\n> Am 27.08.2018 um 09:37 schrieb Ævar Arnfjörð Bjarmason:\n>> \n>> On Sat, Aug 25 2018, René Scharfe wrote:\n>> [...]\n>> Now, I like yours much better. I'm just saying that currently the\n>> patch/commit message combo is confusing about *what* it's\n>> doing. I.e. let's not mix up the correction of docs that were always\n>> wrong with a non-change in the user facing implementation, and some\n>> internal optimization all in one patch.\n>\n> Now you have me confused. Unsorted lists were always accepted, but the\n> documentation asked for a sorted one anyway, probably to avoid sorting\n> it with every use. Switching the underlying data structure makes that a\n> moot point -- sortedness is no longer a concern at all -- not in the\n> code, and not for users.\n>\n> Inviting users to run the array implementation with unsorted lists is\n> not incorrect, but it also doesn't help anyone. We could clarify that\n> sorted lists are preferred or recommended instead of required. I don't\n> see value in perfecting the documentation of a quirk just before\n> removing it, though.\n\nI think it's easier to clarify step-by-step with code, so here's an my\nversion of a v3 which implements what I was suggesting, but then of\ncourse while doing it I found other stuff to fix, changes since your\nv2:\n\nRené Scharfe (2):\n fsck: use strbuf_getline() to read skiplist file\n\nNone to the code.\n\nI changed some docs I add in earlier patches to now describe behavior\nin a past tense (and the s/sorted // change is earlier), and to change\nthe docs to say that sorting the list on-disk is now pointless for\noptimization purposes, but did something in the past.\n\n fsck: use oidset for skiplist\n\nThere is now a test for the bug you were fixing in your 1/2.\n\nÆvar Arnfjörð Bjarmason (5):\n fsck tests: setup of bogus commit object\n\nFixing some test redundancies while I'm at it.\n\n fsck tests: add a test for no skipList input\n\nWe didn't test the most basic skipList invocation, fixed while I was\nat it...\n\n fsck: document and test sorted skipList input\n\nTest that sorted & unsorted skipList input, and document that in the\npast we said this was required, but it never was, but what (ever so\nslight) optimization this gives us.\n\n fsck: document and test commented & empty line skipList input\n\nWe don't support comments or empty lines. Add tests for this not\nworking, and explicitly document that it doesn't work (I for one tried\nit).\n\n fsck: support comments & empty lines in skipList\n\nIgnoring comments and empty lines is very useful for a file format\nthat might be human-edited (I maintain one such file). Support that,\nand document & test for it.\n\n Documentation/config.txt | 22 ++++++++++----\n fsck.c | 48 ++++++++++-------------------\n fsck.h | 8 +++--\n t/t5504-fetch-receive-strict.sh | 53 ++++++++++++++++++++++++++++++---\n 4 files changed, 86 insertions(+), 45 deletions(-)\n\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356610","messageId":"20180827194323.17055-2-avarab@gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"[PATCH v3 1/7] fsck tests: setup of bogus commit object","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:17Z","receivedAt":"2018-08-27T19:43:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Several fsck tests used the exact same git-hash-object output, but had\ncopy/pasted that part of the setup code. Let's instead do that setup\nonce and use it in subsequent tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 62f3569891..6d268f3327 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -133,6 +133,10 @@ committer Bugs Bunny 1234567890 +0000\n This commit object intentionally broken\n EOF\n \n+test_expect_success 'setup bogus commit' '\n+\tcommit=\"$(git hash-object -t commit -w --stdin err &&\n@@ -142,7 +146,6 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n '\n \n test_expect_success 'push with receive.fsck.skipList' '\n-\tcommit=\"$(git hash-object -t commit -w --stdin dies' '\n '\n \n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n-\tcommit=\"$(git hash-object -t commit -w --stdin \n---\n t/t5504-fetch-receive-strict.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 6d268f3327..cbae31f330 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -137,6 +137,11 @@ test_expect_success 'setup bogus commit' '\n \tcommit=\"$(git hash-object -t commit -w --stdin err &&\n+\ttest_i18ngrep \"missingEmail\" err\n+'\n+\n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356612","messageId":"20180827194323.17055-4-avarab@gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"[PATCH v3 3/7] fsck: document and test sorted skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:19Z","receivedAt":"2018-08-27T19:43:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Ever since the skipList support was first added in cd94c6f91 (\"fsck:\ngit receive-pack: support excluding objects from fsck'ing\",\n2015-06-22) the documentation for the format has that the file is a\nsorted list of object names.\n\nThus, anyone using the feature would have thought the list needed to\nbe sorted. E.g. I recently in conjunction with my fetch.fsck.*\nimplementation in 1362df0d41 (\"fetch: implement fetch.fsck.*\",\n2018-07-27) wrote some code to ship a skipList, and went out of my way\nto sort it.\n\nDoing so seems intuitive, since it contains fixed-width records, and\nhas no support for comments, so one might expect it to be binary\nsearched in-place on-disk.\n\nHowever, as documented here this was never a requirement, so let's\nchange the documentation. Since this is a file format change let's\nalso document what was said about this in the past, so e.g. someone\nlike myself reading the new docs can see this never needed to be\nsorted (\"why do I have all this code to sort this thing...\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 10 +++++++++-\n t/t5504-fetch-receive-strict.sh | 19 +++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1c42364988..9359fb534e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1708,7 +1708,7 @@ doing the same for `receive.fsck.` and `fetch.fsck.`\n will only cause git to warn.\n \n fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n+\tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n \tbe ignored. This feature is useful when an established project\n \tshould be accepted despite early commits containing errors that\n@@ -1723,6 +1723,14 @@ Unlike variables like `color.ui` and `core.editor` the\n fall back on the `fsck.skipList` configuration if they aren't set. To\n uniformly configure the same fsck settings in different circumstances\n all three of them they must all set to the same values.\n++\n+Older versions of Git (before 2.20) documented that the object names\n+list should be sorted. This was never a requirement, the object names\n+can appear in any order, but when reading the list we track whether\n+the list is sorted for the purposes of an internal binary search\n+implementation, which can save itself some work with an already sorted\n+list. Unless you have a humongous list there's no reason to go out of\n+your way to pre-sort the list.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex cbae31f330..fa56052f0f 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -142,6 +142,25 @@ test_expect_success 'fsck with no skipList input' '\n \ttest_i18ngrep \"missingEmail\" err\n '\n \n+test_expect_success 'setup sorted and unsorted skipLists' '\n+\tcat >SKIP.unsorted <<-EOF &&\n+\t0000000000000000000000000000000000000004\n+\t0000000000000000000000000000000000000002\n+\t$commit\n+\t0000000000000000000000000000000000000001\n+\t0000000000000000000000000000000000000003\n+\tEOF\n+\tsort SKIP.unsorted >SKIP.sorted\n+'\n+\n+test_expect_success 'fsck with sorted skipList' '\n+\tgit -c fsck.skipList=SKIP.sorted fsck\n+'\n+\n+test_expect_success 'fsck with unsorted skipList' '\n+\tgit -c fsck.skipList=SKIP.unsorted fsck\n+'\n+\n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356613","messageId":"20180827194323.17055-5-avarab@gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"[PATCH v3 4/7] fsck: document and test commented & empty line skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:20Z","receivedAt":"2018-08-27T19:43:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There is currently no comment syntax for the fsck.skipList, this isn't\nreally by design, and it would be nice to have support for comments.\n\nDocument that this doesn't work, and test for how this errors\nout. These tests reveal a current bug, if there's invalid input the\noutput will emit some of the next line, and then go into uninitialized\nmemory. This is fixed in a subsequent change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 11 +++++++----\n t/t5504-fetch-receive-strict.sh | 21 +++++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9359fb534e..a8dfafa61d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1710,10 +1710,13 @@ will only cause git to warn.\n fsck.skipList::\n \tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tbe ignored. Comments ('#') and empty lines are not supported, and\n+\twill error out.\n++\n+This feature is useful when an established project should be accepted\n+despite early commits containing errors that can be safely ignored\n+such as invalid committer email addresses. Note: corrupt objects\n+cannot be skipped with this setting.\n +\n Like `fsck.` this variable has corresponding\n `receive.fsck.skipList` and `fetch.fsck.skipList` variants.\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex fa56052f0f..38aaf3b928 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -169,6 +169,27 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n '\n \n+test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n+\tcat >SKIP.with-comment <<-EOF &&\n+\t# Some bad commit\n+\t0000000000000000000000000000000000000001\n+\tEOF\n+\ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n+\tcat >SKIP.with-empty-line <<-EOF &&\n+\t0000000000000000000000000000000000000001\n+\n+\t0000000000000000000000000000000000000002\n+\tEOF\n+\ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n+'\n+\n+test_expect_failure 'fsck no garbage output from comments & empty lines errors' '\n+\ttest_line_count = 1 err-with-comment &&\n+\ttest_line_count = 1 err-with-empty-line\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tgit push . $commit:refs/heads/bogus &&\n \trm -rf dst &&\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356614","messageId":"20180827194323.17055-6-avarab@gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"[PATCH v3 5/7] fsck: use strbuf_getline() to read skiplist file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:21Z","receivedAt":"2018-08-27T19:43:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nbuffer is unlikely to contain a NUL character, so printing its contents\nusing %s in a die() format is unsafe (detected with ASan).\n\nUse an idiomatic strbuf_getline() loop instead, which ensures the buffer\nis always NUL-terminated, supports CRLF files as well, accepts files\nwithout a newline after the last line, supports any hash length\nautomatically, and is shorter.\n\nHelped-by: Jeff King \nSigned-off-by: Rene Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fsck.c | 25 ++++++++++++-------------\n t/t5504-fetch-receive-strict.sh | 2 +-\n 2 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex a0cee0be59..972a26b9ba 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -183,8 +183,9 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n static void init_skiplist(struct fsck_options *options, const char *path)\n {\n \tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n-\tint sorted, fd;\n-\tchar buffer[GIT_MAX_HEXSZ + 1];\n+\tint sorted;\n+\tFILE *fp;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n \tif (options->skiplist)\n@@ -194,25 +195,23 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\toptions->skiplist = &skiplist;\n \t}\n \n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n+\tfp = fopen(path, \"r\");\n+\tif (!fp)\n \t\tdie(\"Could not open skip list: %s\", path);\n-\tfor (;;) {\n+\twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n-\t\tint result = read_in_full(fd, buffer, sizeof(buffer));\n-\t\tif (result < 0)\n-\t\t\tdie_errno(\"Could not read '%s'\", path);\n-\t\tif (!result)\n-\t\t\tbreak;\n-\t\tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n')\n-\t\t\tdie(\"Invalid SHA-1: %s\", buffer);\n+\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n+\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n \t\toid_array_append(&skiplist, &oid);\n \t\tif (sorted && skiplist.nr > 1 &&\n \t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n \t\t\t\t &oid) > 0)\n \t\t\tsorted = 0;\n \t}\n-\tclose(fd);\n+\tif (ferror(fp))\n+\t\tdie_errno(\"Could not read '%s'\", path);\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\n \n \tif (sorted)\n \t\tskiplist.sorted = 1;\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 38aaf3b928..c7224db3bb 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -185,7 +185,7 @@ test_expect_success 'fsck with invalid or bogus skipList input (comments & empty\n \ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n '\n \n-test_expect_failure 'fsck no garbage output from comments & empty lines errors' '\n+test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n \ttest_line_count = 1 err-with-comment &&\n \ttest_line_count = 1 err-with-empty-line\n '\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356615","messageId":"20180827194323.17055-7-avarab@gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"[PATCH v3 6/7] fsck: use oidset for skiplist","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:22Z","receivedAt":"2018-08-27T19:43:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nObject IDs to skip are stored in a shared static oid_array. Lookups do\na binary search on the sorted array. The code checks if the object IDs\nare already in the correct order while loading and skips sorting in that\ncase. Lookups are done before reporting a (non-fatal) corruption and\nbefore checking .gitmodules files.\n\nSimplify the code by using an oidset instead. Memory usage is a bit\nhigher, but we don't need to worry about any sort order anymore. Embed\nthe oidset into struct fsck_options to make its ownership clear (no\nhidden sharing) and avoid unnecessary pointer indirection.\n\nPerformance on repositories with a low number of reported issues and\n.gitmodules files (i.e. the usual case) won't be affected much. The\noidset should be a bit quicker with higher numbers of bad objects in\nthe skipList.\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Rene Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 11 ++++++-----\n fsck.c | 23 ++---------------------\n fsck.h | 8 +++++---\n 3 files changed, 13 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex a8dfafa61d..3d0556e85d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1729,11 +1729,12 @@ all three of them they must all set to the same values.\n +\n Older versions of Git (before 2.20) documented that the object names\n list should be sorted. This was never a requirement, the object names\n-can appear in any order, but when reading the list we track whether\n-the list is sorted for the purposes of an internal binary search\n-implementation, which can save itself some work with an already sorted\n-list. Unless you have a humongous list there's no reason to go out of\n-your way to pre-sort the list.\n+could appear in any order, but when reading the list we tracked whether\n+the list was sorted for the purposes of an internal binary search\n+implementation, which could save itself some work with an already sorted\n+list. Unless you had a humongous list there was no reason to go out of\n+your way to pre-sort the list. After Git version 2.20 a hash implementation\n+is used instead, so there's now no reason to pre-sort the list.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/fsck.c b/fsck.c\nindex 972a26b9ba..4c643f1d40 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -10,7 +10,6 @@\n #include \"fsck.h\"\n #include \"refs.h\"\n #include \"utf8.h\"\n-#include \"sha1-array.h\"\n #include \"decorate.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n@@ -182,19 +181,10 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n \n static void init_skiplist(struct fsck_options *options, const char *path)\n {\n-\tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n-\tint sorted;\n \tFILE *fp;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n-\tif (options->skiplist)\n-\t\tsorted = options->skiplist->sorted;\n-\telse {\n-\t\tsorted = 1;\n-\t\toptions->skiplist = &skiplist;\n-\t}\n-\n \tfp = fopen(path, \"r\");\n \tif (!fp)\n \t\tdie(\"Could not open skip list: %s\", path);\n@@ -202,19 +192,12 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\tconst char *p;\n \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n-\t\toid_array_append(&skiplist, &oid);\n-\t\tif (sorted && skiplist.nr > 1 &&\n-\t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n-\t\t\t\t &oid) > 0)\n-\t\t\tsorted = 0;\n+\t\toidset_insert(&options->skiplist, &oid);\n \t}\n \tif (ferror(fp))\n \t\tdie_errno(\"Could not read '%s'\", path);\n \tfclose(fp);\n \tstrbuf_release(&sb);\n-\n-\tif (sorted)\n-\t\tskiplist.sorted = 1;\n }\n \n static int parse_msg_type(const char *str)\n@@ -319,9 +302,7 @@ static void append_msg_id(struct strbuf *sb, const char *msg_id)\n \n static int object_on_skiplist(struct fsck_options *opts, struct object *obj)\n {\n-\tif (opts && opts->skiplist && obj)\n-\t\treturn oid_array_lookup(opts->skiplist, &obj->oid) >= 0;\n-\treturn 0;\n+\treturn opts && obj && oidset_contains(&opts->skiplist, &obj->oid);\n }\n \n __attribute__((format (printf, 4, 5)))\ndiff --git a/fsck.h b/fsck.h\nindex 0c7e8c9428..b95595ae5f 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -1,6 +1,8 @@\n #ifndef GIT_FSCK_H\n #define GIT_FSCK_H\n \n+#include \"oidset.h\"\n+\n #define FSCK_ERROR 1\n #define FSCK_WARN 2\n #define FSCK_IGNORE 3\n@@ -35,12 +37,12 @@ struct fsck_options {\n \tfsck_error error_func;\n \tunsigned strict:1;\n \tint *msg_type;\n-\tstruct oid_array *skiplist;\n+\tstruct oidset skiplist;\n \tstruct decoration *object_names;\n };\n \n-#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }\n-#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }\n+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT }\n+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT }\n \n /* descend in all linked child objects\n * the return value is:\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356616","messageId":"20180827194323.17055-8-avarab@gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"[PATCH v3 7/7] fsck: support comments & empty lines in skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:43:23Z","receivedAt":"2018-08-27T19:43:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It's annoying not to be able to put comments and empty lines in the\nskipList, when e.g. keeping a big central list of commits to skip in\n/etc/gitconfig, which was my motivation for 1362df0d41 (\"fetch:\nimplement fetch.fsck.*\", 2018-07-27).\n\nImplement that, and document what version of Git this was changed in,\nsince this on-disk format can be expected to be used by multiple\nversions of git.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 4 ++--\n fsck.c | 2 ++\n t/t5504-fetch-receive-strict.sh | 6 +++---\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3d0556e85d..e6f95a7fb2 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1710,8 +1710,8 @@ will only cause git to warn.\n fsck.skipList::\n \tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. Comments ('#') and empty lines are not supported, and\n-\twill error out.\n+\tbe ignored. On versions of Git 2.20 and later comments ('#') and empty\n+\tlines are ignored, but will error out on older versions.\n +\n This feature is useful when an established project should be accepted\n despite early commits containing errors that can be safely ignored\ndiff --git a/fsck.c b/fsck.c\nindex 4c643f1d40..589548308a 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -190,6 +190,8 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\tdie(\"Could not open skip list: %s\", path);\n \twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n+\t\tif (!strcmp(sb.buf, \"\") || starts_with(sb.buf, \"#\"))\n+\t\t\tcontinue;\n \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n \t\toidset_insert(&options->skiplist, &oid);\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex c7224db3bb..a1bac164d1 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -169,20 +169,20 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n '\n \n-test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n+test_expect_success 'fsck with other accepted skipList input (comments & empty lines)' '\n \tcat >SKIP.with-comment <<-EOF &&\n \t# Some bad commit\n \t0000000000000000000000000000000000000001\n \tEOF\n \ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n-\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n+\ttest_i18ngrep \"missingEmail\" err-with-comment &&\n \tcat >SKIP.with-empty-line <<-EOF &&\n \t0000000000000000000000000000000000000001\n \n \t0000000000000000000000000000000000000002\n \tEOF\n \ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n-\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n+\ttest_i18ngrep \"missingEmail\" err-with-empty-line\n '\n \n test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356617","messageId":"87mut7egoj.fsf@evledraar.gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-1-avarab@gmail.com","subject":"Re: [PATCH v3 0/7] use oidset for skiplist + docs + tests + comment support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T19:46:20Z","receivedAt":"2018-08-27T19:46:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Aug 27 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Aug 27 2018, René Scharfe wrote:\n>\n>> Am 27.08.2018 um 09:37 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> On Sat, Aug 25 2018, René Scharfe wrote:\n>>> [...]\n>>> Now, I like yours much better. I'm just saying that currently the\n>>> patch/commit message combo is confusing about *what* it's\n>>> doing. I.e. let's not mix up the correction of docs that were always\n>>> wrong with a non-change in the user facing implementation, and some\n>>> internal optimization all in one patch.\n>>\n>> Now you have me confused. Unsorted lists were always accepted, but the\n>> documentation asked for a sorted one anyway, probably to avoid sorting\n>> it with every use. Switching the underlying data structure makes that a\n>> moot point -- sortedness is no longer a concern at all -- not in the\n>> code, and not for users.\n>>\n>> Inviting users to run the array implementation with unsorted lists is\n>> not incorrect, but it also doesn't help anyone. We could clarify that\n>> sorted lists are preferred or recommended instead of required. I don't\n>> see value in perfecting the documentation of a quirk just before\n>> removing it, though.\n>\n> I think it's easier to clarify step-by-step with code, so here's an my\n> version of a v3 which implements what I was suggesting, but then of\n> course while doing it I found other stuff to fix, changes since your\n> v2:\n\nSorry for breaking threading, forgot In-Reply-To, which should be\nhttps://public-inbox.org/git/c7cbc289-d86e-09dc-bdb3-b5496cf0b7ce@web.de/\n\n> René Scharfe (2):\n> fsck: use strbuf_getline() to read skiplist file\n>\n> None to the code.\n>\n> I changed some docs I add in earlier patches to now describe behavior\n> in a past tense (and the s/sorted // change is earlier), and to change\n> the docs to say that sorting the list on-disk is now pointless for\n> optimization purposes, but did something in the past.\n>\n> fsck: use oidset for skiplist\n>\n> There is now a test for the bug you were fixing in your 1/2.\n>\n> Ævar Arnfjörð Bjarmason (5):\n> fsck tests: setup of bogus commit object\n>\n> Fixing some test redundancies while I'm at it.\n>\n> fsck tests: add a test for no skipList input\n>\n> We didn't test the most basic skipList invocation, fixed while I was\n> at it...\n>\n> fsck: document and test sorted skipList input\n>\n> Test that sorted & unsorted skipList input, and document that in the\n> past we said this was required, but it never was, but what (ever so\n> slight) optimization this gives us.\n>\n> fsck: document and test commented & empty line skipList input\n>\n> We don't support comments or empty lines. Add tests for this not\n> working, and explicitly document that it doesn't work (I for one tried\n> it).\n>\n> fsck: support comments & empty lines in skipList\n>\n> Ignoring comments and empty lines is very useful for a file format\n> that might be human-edited (I maintain one such file). Support that,\n> and document & test for it.\n>\n> Documentation/config.txt | 22 ++++++++++----\n> fsck.c | 48 ++++++++++-------------------\n> fsck.h | 8 +++--\n> t/t5504-fetch-receive-strict.sh | 53 ++++++++++++++++++++++++++++++---\n> 4 files changed, 86 insertions(+), 45 deletions(-)\n"},{"id":"356619","messageId":"87lg8refcr.fsf@evledraar.gmail.com","threadId":"49227","inReplyTo":"20180827194323.17055-7-avarab@gmail.com","subject":"Re: [PATCH v3 6/7] fsck: use oidset for skiplist","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-27T20:15:00Z","receivedAt":"2018-08-27T20:15:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Aug 27 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> From: René Scharfe \n>\n> Object IDs to skip are stored in a shared static oid_array. Lookups do\n> a binary search on the sorted array. The code checks if the object IDs\n> are already in the correct order while loading and skips sorting in that\n> case. Lookups are done before reporting a (non-fatal) corruption and\n> before checking .gitmodules files.\n>\n> Simplify the code by using an oidset instead. Memory usage is a bit\n> higher, but we don't need to worry about any sort order anymore. Embed\n> the oidset into struct fsck_options to make its ownership clear (no\n> hidden sharing) and avoid unnecessary pointer indirection.\n>\n> Performance on repositories with a low number of reported issues and\n> .gitmodules files (i.e. the usual case) won't be affected much. The\n> oidset should be a bit quicker with higher numbers of bad objects in\n> the skipList.\n\nI didn't change this commit message at all, but FWIW this still has me\nsomewhat confused. What is the interaction with .gitmodules being\ndescribed here? You also mentioned it in\nhttps://public-inbox.org/git/113aa2d7-6f66-8d03-dda4-7337cda9b2df@web.de/\n(but I don't get that either)\n\nDoes that just mean that when cloning with --recursive with\ntransfer.fsckObjects=true we'll re-read the file for each \"clone\"\ninvocation, both for the main project and everything listed in\n.gitmodules?\n\nIf so, I think something like this commit message would be clearer:\n\n fsck: use oidset instead of oid_array for skipList\n\n Change the implementation of the skipList feature to use oidset\n instead of oid_array to store SHA-1s for later lookup.\n\n This list is parsed once on startup by fsck, fetch-pack or\n receive-pack depending on the *.skipList config in use, so for fetch\n it's currently re-parsed for each submodule fetch.\n\n Memory usage is a bit higher, but we don't need to keep track of the\n sort order anymore. Embed the oidset into struct fsck_options to\n make its ownership clear (no hidden sharing) and avoid unnecessary\n pointer indirection.\n\nThen let's just attach the test program you wrote in\nhttps://public-inbox.org/git/113aa2d7-6f66-8d03-dda4-7337cda9b2df@web.de/\nand note the results and let them speak for themselves.\n\nI can do all that if that seems fine to you. I also notice that the test\ncase only sets up a case where all the items on the skip list are bad\ncommits in the repo, it would be interesting to test with a few needles\nin a large haystack (I can modify it to do that...).\n\n> Helped-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Rene Scharfe \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config.txt | 11 ++++++-----\n> fsck.c | 23 ++---------------------\n> fsck.h | 8 +++++---\n> 3 files changed, 13 insertions(+), 29 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index a8dfafa61d..3d0556e85d 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1729,11 +1729,12 @@ all three of them they must all set to the same values.\n> +\n> Older versions of Git (before 2.20) documented that the object names\n> list should be sorted. This was never a requirement, the object names\n> -can appear in any order, but when reading the list we track whether\n> -the list is sorted for the purposes of an internal binary search\n> -implementation, which can save itself some work with an already sorted\n> -list. Unless you have a humongous list there's no reason to go out of\n> -your way to pre-sort the list.\n> +could appear in any order, but when reading the list we tracked whether\n> +the list was sorted for the purposes of an internal binary search\n> +implementation, which could save itself some work with an already sorted\n> +list. Unless you had a humongous list there was no reason to go out of\n> +your way to pre-sort the list. After Git version 2.20 a hash implementation\n> +is used instead, so there's now no reason to pre-sort the list.\n>\n> gc.aggressiveDepth::\n> \tThe depth parameter used in the delta compression\n> diff --git a/fsck.c b/fsck.c\n> index 972a26b9ba..4c643f1d40 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -10,7 +10,6 @@\n> #include \"fsck.h\"\n> #include \"refs.h\"\n> #include \"utf8.h\"\n> -#include \"sha1-array.h\"\n> #include \"decorate.h\"\n> #include \"oidset.h\"\n> #include \"packfile.h\"\n> @@ -182,19 +181,10 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n>\n> static void init_skiplist(struct fsck_options *options, const char *path)\n> {\n> -\tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n> -\tint sorted;\n> \tFILE *fp;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tstruct object_id oid;\n>\n> -\tif (options->skiplist)\n> -\t\tsorted = options->skiplist->sorted;\n> -\telse {\n> -\t\tsorted = 1;\n> -\t\toptions->skiplist = &skiplist;\n> -\t}\n> -\n> \tfp = fopen(path, \"r\");\n> \tif (!fp)\n> \t\tdie(\"Could not open skip list: %s\", path);\n> @@ -202,19 +192,12 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n> \t\tconst char *p;\n> \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n> \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n> -\t\toid_array_append(&skiplist, &oid);\n> -\t\tif (sorted && skiplist.nr > 1 &&\n> -\t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n> -\t\t\t\t &oid) > 0)\n> -\t\t\tsorted = 0;\n> +\t\toidset_insert(&options->skiplist, &oid);\n> \t}\n> \tif (ferror(fp))\n> \t\tdie_errno(\"Could not read '%s'\", path);\n> \tfclose(fp);\n> \tstrbuf_release(&sb);\n> -\n> -\tif (sorted)\n> -\t\tskiplist.sorted = 1;\n> }\n>\n> static int parse_msg_type(const char *str)\n> @@ -319,9 +302,7 @@ static void append_msg_id(struct strbuf *sb, const char *msg_id)\n>\n> static int object_on_skiplist(struct fsck_options *opts, struct object *obj)\n> {\n> -\tif (opts && opts->skiplist && obj)\n> -\t\treturn oid_array_lookup(opts->skiplist, &obj->oid) >= 0;\n> -\treturn 0;\n> +\treturn opts && obj && oidset_contains(&opts->skiplist, &obj->oid);\n> }\n>\n> __attribute__((format (printf, 4, 5)))\n> diff --git a/fsck.h b/fsck.h\n> index 0c7e8c9428..b95595ae5f 100644\n> --- a/fsck.h\n> +++ b/fsck.h\n> @@ -1,6 +1,8 @@\n> #ifndef GIT_FSCK_H\n> #define GIT_FSCK_H\n>\n> +#include \"oidset.h\"\n> +\n> #define FSCK_ERROR 1\n> #define FSCK_WARN 2\n> #define FSCK_IGNORE 3\n> @@ -35,12 +37,12 @@ struct fsck_options {\n> \tfsck_error error_func;\n> \tunsigned strict:1;\n> \tint *msg_type;\n> -\tstruct oid_array *skiplist;\n> +\tstruct oidset skiplist;\n> \tstruct decoration *object_names;\n> };\n>\n> -#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }\n> -#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }\n> +#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT }\n> +#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT }\n>\n> /* descend in all linked child objects\n> * the return value is:\n"},{"id":"356649","messageId":"20180828095219.23296-2-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 1/8] fsck tests: setup of bogus commit object","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:12Z","receivedAt":"2018-08-28T09:52:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Several fsck tests used the exact same git-hash-object output, but had\ncopy/pasted that part of the setup code. Let's instead do that setup\nonce and use it in subsequent tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 62f3569891..6d268f3327 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -133,6 +133,10 @@ committer Bugs Bunny 1234567890 +0000\n This commit object intentionally broken\n EOF\n \n+test_expect_success 'setup bogus commit' '\n+\tcommit=\"$(git hash-object -t commit -w --stdin err &&\n@@ -142,7 +146,6 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n '\n \n test_expect_success 'push with receive.fsck.skipList' '\n-\tcommit=\"$(git hash-object -t commit -w --stdin dies' '\n '\n \n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n-\tcommit=\"$(git hash-object -t commit -w --stdin On Mon, Aug 27 2018, Ævar Arnfjörð Bjarmason wrote:\n> Does that just mean that when cloning with --recursive with\n> transfer.fsckObjects=true we'll re-read the file for each \"clone\"\n> invocation, both for the main project and everything listed in\n> .gitmodules?\n>\n> If so, I think something like this commit message would be clearer:\n\nHere's a version that does that, I also integrated the perf test René\nwrote and used it in the commit messages. The range-diff with v3\nfollows:\n\n1: 09b52b0eae = 1: ba9ef73304 fsck tests: setup of bogus commit object\n2: 294a4dfd69 = 2: 1d20e37d3f fsck tests: add a test for no skipList input\n3: 15a6e16dc1 = 3: 953ad29f6d fsck: document and test sorted skipList input\n4: b93fe502c3 = 4: c8a0bd1555 fsck: document and test commented & empty line skipList input\n-: ---------- > 5: cbbb0326d3 fsck: add a performance test for skipList\n5: 3cf7d4e9eb ! 6: ca6aca2994 fsck: use strbuf_getline() to read skiplist file\n @@ -2,14 +2,33 @@\n \n fsck: use strbuf_getline() to read skiplist file\n \n - buffer is unlikely to contain a NUL character, so printing its contents\n - using %s in a die() format is unsafe (detected with ASan).\n + The buffer is unlikely to contain a NUL character, so printing its\n + contents using %s in a die() format is unsafe (detected with ASan).\n \n Use an idiomatic strbuf_getline() loop instead, which ensures the buffer\n is always NUL-terminated, supports CRLF files as well, accepts files\n without a newline after the last line, supports any hash length\n automatically, and is shorter.\n \n + This fixes a bug where emitting an error about an invalid line on say\n + line 1 would continue printing subsequent lines, and usually continue\n + into uninitialized memory.\n +\n + The performance impact of this, on a CentOS 7 box with RedHat GCC\n + 4.8.5-28:\n +\n + $ GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS='-j56 CFLAGS=\"-O3\"' ./run HEAD~ HEAD p1450-fsck-skip-list.sh\n + Test HEAD~ HEAD\n + ----------------------------------------------------------------------------------------\n + 1450.3: fsck with 0 skipped bad commits 7.75(7.39+0.35) 7.68(7.29+0.39) -0.9%\n + 1450.5: fsck with 1 skipped bad commits 7.70(7.30+0.40) 7.80(7.42+0.37) +1.3%\n + 1450.7: fsck with 10 skipped bad commits 7.77(7.37+0.40) 7.87(7.47+0.40) +1.3%\n + 1450.9: fsck with 100 skipped bad commits 7.82(7.41+0.40) 7.88(7.43+0.44) +0.8%\n + 1450.11: fsck with 1000 skipped bad commits 7.88(7.49+0.39) 7.84(7.43+0.40) -0.5%\n + 1450.13: fsck with 10000 skipped bad commits 8.02(7.63+0.39) 8.07(7.67+0.39) +0.6%\n + 1450.15: fsck with 100000 skipped bad commits 8.01(7.60+0.41) 8.08(7.70+0.38) +0.9%\n + 1450.17: fsck with 1000000 skipped bad commits 7.60(7.10+0.50) 7.37(7.18+0.19) -3.0%\n +\n Helped-by: Jeff King \n Signed-off-by: Rene Scharfe \n Signed-off-by: Ævar Arnfjörð Bjarmason \n6: bd51de7c14 ! 7: 58bfd04d96 fsck: use oidset for skiplist\n @@ -1,22 +1,33 @@\n Author: René Scharfe \n \n - fsck: use oidset for skiplist\n + fsck: use oidset instead of oid_array for skipList\n \n - Object IDs to skip are stored in a shared static oid_array. Lookups do\n - a binary search on the sorted array. The code checks if the object IDs\n - are already in the correct order while loading and skips sorting in that\n - case. Lookups are done before reporting a (non-fatal) corruption and\n - before checking .gitmodules files.\n + Change the implementation of the skipList feature to use oidset\n + instead of oid_array to store SHA-1s for later lookup.\n \n - Simplify the code by using an oidset instead. Memory usage is a bit\n - higher, but we don't need to worry about any sort order anymore. Embed\n - the oidset into struct fsck_options to make its ownership clear (no\n - hidden sharing) and avoid unnecessary pointer indirection.\n + This list is parsed once on startup by fsck, fetch-pack or\n + receive-pack depending on the *.skipList config in use. I.e. only once\n + per invocation, but note that for \"clone --recurse-submodules\" each\n + submodule will re-parse the list, in addition to the main project.\n \n - Performance on repositories with a low number of reported issues and\n - .gitmodules files (i.e. the usual case) won't be affected much. The\n - oidset should be a bit quicker with higher numbers of bad objects in\n - the skipList.\n + Memory usage is a bit higher, but we don't need to keep track of the\n + sort order anymore. Embed the oidset into struct fsck_options to make\n + its ownership clear (no hidden sharing) and avoid unnecessary pointer\n + indirection.\n +\n + The cumulative impact on performance of this & the preceding change,\n + using the test setup described in the previous commit:\n +\n + Test HEAD~2 HEAD~ HEAD\n + ----------------------------------------------------------------------------------------------------------------\n + 1450.3: fsck with 0 skipped bad commits 7.70(7.31+0.38) 7.72(7.33+0.38) +0.3% 7.70(7.30+0.40) +0.0%\n + 1450.5: fsck with 1 skipped bad commits 7.84(7.47+0.37) 7.69(7.32+0.36) -1.9% 7.71(7.29+0.41) -1.7%\n + 1450.7: fsck with 10 skipped bad commits 7.81(7.40+0.40) 7.94(7.57+0.36) +1.7% 7.92(7.55+0.37) +1.4%\n + 1450.9: fsck with 100 skipped bad commits 7.81(7.42+0.38) 7.95(7.53+0.41) +1.8% 7.83(7.42+0.41) +0.3%\n + 1450.11: fsck with 1000 skipped bad commits 7.99(7.62+0.36) 7.90(7.50+0.40) -1.1% 7.86(7.49+0.37) -1.6%\n + 1450.13: fsck with 10000 skipped bad commits 7.98(7.57+0.40) 7.94(7.53+0.40) -0.5% 7.90(7.45+0.44) -1.0%\n + 1450.15: fsck with 100000 skipped bad commits 7.97(7.57+0.39) 8.03(7.67+0.36) +0.8% 7.84(7.43+0.41) -1.6%\n + 1450.17: fsck with 1000000 skipped bad commits 7.72(7.22+0.50) 7.28(7.07+0.20) -5.7% 7.13(6.87+0.25) -7.6%\n \n Helped-by: Ævar Arnfjörð Bjarmason \n Signed-off-by: Rene Scharfe \n7: 17838cf68c ! 8: 0125461f13 fsck: support comments & empty lines in skipList\n @@ -11,6 +11,20 @@\n since this on-disk format can be expected to be used by multiple\n versions of git.\n \n + There is no notable performance impact from this change, using the\n + test setup described a couple of commist back:\n +\n + Test HEAD~ HEAD\n + ----------------------------------------------------------------------------------------\n + 1450.3: fsck with 0 skipped bad commits 7.81(7.42+0.39) 7.72(7.34+0.38) -1.2%\n + 1450.5: fsck with 1 skipped bad commits 7.75(7.36+0.38) 7.66(7.26+0.39) -1.2%\n + 1450.7: fsck with 10 skipped bad commits 7.81(7.43+0.38) 7.70(7.30+0.39) -1.4%\n + 1450.9: fsck with 100 skipped bad commits 7.85(7.42+0.42) 7.73(7.31+0.41) -1.5%\n + 1450.11: fsck with 1000 skipped bad commits 7.81(7.43+0.38) 7.84(7.46+0.38) +0.4%\n + 1450.13: fsck with 10000 skipped bad commits 7.87(7.47+0.40) 7.86(7.46+0.40) -0.1%\n + 1450.15: fsck with 100000 skipped bad commits 7.77(7.39+0.38) 7.83(7.48+0.34) +0.8%\n + 1450.17: fsck with 1000000 skipped bad commits 7.17(6.92+0.24) 7.11(6.85+0.26) -0.8%\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n diff --git a/Documentation/config.txt b/Documentation/config.txt\n\nRené Scharfe (3):\n fsck: add a performance test for skipList\n fsck: use strbuf_getline() to read skiplist file\n fsck: use oidset instead of oid_array for skipList\n\nÆvar Arnfjörð Bjarmason (5):\n fsck tests: setup of bogus commit object\n fsck tests: add a test for no skipList input\n fsck: document and test sorted skipList input\n fsck: document and test commented & empty line skipList input\n fsck: support comments & empty lines in skipList\n\n Documentation/config.txt | 22 ++++++++++----\n fsck.c | 48 ++++++++++-------------------\n fsck.h | 8 +++--\n t/perf/p1450-fsck-skip-list.sh | 40 +++++++++++++++++++++++++\n t/t5504-fetch-receive-strict.sh | 53 ++++++++++++++++++++++++++++++---\n 5 files changed, 126 insertions(+), 45 deletions(-)\n create mode 100755 t/perf/p1450-fsck-skip-list.sh\n\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356651","messageId":"20180828095219.23296-3-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 2/8] fsck tests: add a test for no skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:13Z","receivedAt":"2018-08-28T09:52:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The recent 65a836fa6b (\"fsck: add stress tests for fsck.skipList\",\n2018-07-27) added various stress tests for odd invocations of\nfsck.skipList, but didn't tests for some very simple ones, such as\nasserting that providing to skipList with a bad commit causes fsck to\nexit with a non-zero exit code. Add such a test.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 6d268f3327..cbae31f330 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -137,6 +137,11 @@ test_expect_success 'setup bogus commit' '\n \tcommit=\"$(git hash-object -t commit -w --stdin err &&\n+\ttest_i18ngrep \"missingEmail\" err\n+'\n+\n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356652","messageId":"20180828095219.23296-4-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 3/8] fsck: document and test sorted skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:14Z","receivedAt":"2018-08-28T09:52:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Ever since the skipList support was first added in cd94c6f91 (\"fsck:\ngit receive-pack: support excluding objects from fsck'ing\",\n2015-06-22) the documentation for the format has that the file is a\nsorted list of object names.\n\nThus, anyone using the feature would have thought the list needed to\nbe sorted. E.g. I recently in conjunction with my fetch.fsck.*\nimplementation in 1362df0d41 (\"fetch: implement fetch.fsck.*\",\n2018-07-27) wrote some code to ship a skipList, and went out of my way\nto sort it.\n\nDoing so seems intuitive, since it contains fixed-width records, and\nhas no support for comments, so one might expect it to be binary\nsearched in-place on-disk.\n\nHowever, as documented here this was never a requirement, so let's\nchange the documentation. Since this is a file format change let's\nalso document what was said about this in the past, so e.g. someone\nlike myself reading the new docs can see this never needed to be\nsorted (\"why do I have all this code to sort this thing...\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 10 +++++++++-\n t/t5504-fetch-receive-strict.sh | 19 +++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex eb66a11975..b2fdbc6764 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1710,7 +1710,7 @@ doing the same for `receive.fsck.` and `fetch.fsck.`\n will only cause git to warn.\n \n fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n+\tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n \tbe ignored. This feature is useful when an established project\n \tshould be accepted despite early commits containing errors that\n@@ -1725,6 +1725,14 @@ Unlike variables like `color.ui` and `core.editor` the\n fall back on the `fsck.skipList` configuration if they aren't set. To\n uniformly configure the same fsck settings in different circumstances\n all three of them they must all set to the same values.\n++\n+Older versions of Git (before 2.20) documented that the object names\n+list should be sorted. This was never a requirement, the object names\n+can appear in any order, but when reading the list we track whether\n+the list is sorted for the purposes of an internal binary search\n+implementation, which can save itself some work with an already sorted\n+list. Unless you have a humongous list there's no reason to go out of\n+your way to pre-sort the list.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex cbae31f330..fa56052f0f 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -142,6 +142,25 @@ test_expect_success 'fsck with no skipList input' '\n \ttest_i18ngrep \"missingEmail\" err\n '\n \n+test_expect_success 'setup sorted and unsorted skipLists' '\n+\tcat >SKIP.unsorted <<-EOF &&\n+\t0000000000000000000000000000000000000004\n+\t0000000000000000000000000000000000000002\n+\t$commit\n+\t0000000000000000000000000000000000000001\n+\t0000000000000000000000000000000000000003\n+\tEOF\n+\tsort SKIP.unsorted >SKIP.sorted\n+'\n+\n+test_expect_success 'fsck with sorted skipList' '\n+\tgit -c fsck.skipList=SKIP.sorted fsck\n+'\n+\n+test_expect_success 'fsck with unsorted skipList' '\n+\tgit -c fsck.skipList=SKIP.unsorted fsck\n+'\n+\n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356653","messageId":"20180828095219.23296-5-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 4/8] fsck: document and test commented & empty line skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:15Z","receivedAt":"2018-08-28T09:52:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There is currently no comment syntax for the fsck.skipList, this isn't\nreally by design, and it would be nice to have support for comments.\n\nDocument that this doesn't work, and test for how this errors\nout. These tests reveal a current bug, if there's invalid input the\noutput will emit some of the next line, and then go into uninitialized\nmemory. This is fixed in a subsequent change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 11 +++++++----\n t/t5504-fetch-receive-strict.sh | 21 +++++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b2fdbc6764..c7f033f036 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1712,10 +1712,13 @@ will only cause git to warn.\n fsck.skipList::\n \tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tbe ignored. Comments ('#') and empty lines are not supported, and\n+\twill error out.\n++\n+This feature is useful when an established project should be accepted\n+despite early commits containing errors that can be safely ignored\n+such as invalid committer email addresses. Note: corrupt objects\n+cannot be skipped with this setting.\n +\n Like `fsck.` this variable has corresponding\n `receive.fsck.skipList` and `fetch.fsck.skipList` variants.\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex fa56052f0f..38aaf3b928 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -169,6 +169,27 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n '\n \n+test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n+\tcat >SKIP.with-comment <<-EOF &&\n+\t# Some bad commit\n+\t0000000000000000000000000000000000000001\n+\tEOF\n+\ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n+\tcat >SKIP.with-empty-line <<-EOF &&\n+\t0000000000000000000000000000000000000001\n+\n+\t0000000000000000000000000000000000000002\n+\tEOF\n+\ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n+'\n+\n+test_expect_failure 'fsck no garbage output from comments & empty lines errors' '\n+\ttest_line_count = 1 err-with-comment &&\n+\ttest_line_count = 1 err-with-empty-line\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tgit push . $commit:refs/heads/bogus &&\n \trm -rf dst &&\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356654","messageId":"20180828095219.23296-6-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 5/8] fsck: add a performance test for skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:16Z","receivedAt":"2018-08-28T09:52:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nCreate a performance test to see how the skipList implementation\nperforms. First we setup N bad commits, then we see how progressively\nworking our way up to 0..N in increments of 10x does. I.e. the\nneedle(s) in the haystack get progressively more numerous.\n\nSigned-off-by: René Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/p1450-fsck-skip-list.sh | 40 ++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n create mode 100755 t/perf/p1450-fsck-skip-list.sh\n\ndiff --git a/t/perf/p1450-fsck-skip-list.sh b/t/perf/p1450-fsck-skip-list.sh\nnew file mode 100755\nindex 0000000000..c2b97d2487\n--- /dev/null\n+++ b/t/perf/p1450-fsck-skip-list.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+\n+test_description='Test fsck skipList performance'\n+\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+n=1000000\n+\n+test_expect_success \"setup $n bad commits\" '\n+\tfor i in $(test_seq 1 $n)\n+\tdo\n+\t\techo \"commit refs/heads/master\" &&\n+\t\techo \"committer C 1234567890 +0000\" &&\n+\t\techo \"data <skiplist\n+\t'\n+\n+\ttest_perf \"fsck with $skip skipped bad commits\" '\n+\t\tgit -c fsck.skipList=skiplist fsck\n+\t'\n+\n+\tcase $skip in\n+\t0) skip=1 ;;\n+\t*) skip=${skip}0 ;;\n+\tesac\n+done\n+\n+test_done\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356655","messageId":"20180828095219.23296-7-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 6/8] fsck: use strbuf_getline() to read skiplist file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:17Z","receivedAt":"2018-08-28T09:52:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nThe buffer is unlikely to contain a NUL character, so printing its\ncontents using %s in a die() format is unsafe (detected with ASan).\n\nUse an idiomatic strbuf_getline() loop instead, which ensures the buffer\nis always NUL-terminated, supports CRLF files as well, accepts files\nwithout a newline after the last line, supports any hash length\nautomatically, and is shorter.\n\nThis fixes a bug where emitting an error about an invalid line on say\nline 1 would continue printing subsequent lines, and usually continue\ninto uninitialized memory.\n\nThe performance impact of this, on a CentOS 7 box with RedHat GCC\n4.8.5-28:\n\n $ GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS='-j56 CFLAGS=\"-O3\"' ./run HEAD~ HEAD p1450-fsck-skip-list.sh\n Test HEAD~ HEAD\n ----------------------------------------------------------------------------------------\n 1450.3: fsck with 0 skipped bad commits 7.75(7.39+0.35) 7.68(7.29+0.39) -0.9%\n 1450.5: fsck with 1 skipped bad commits 7.70(7.30+0.40) 7.80(7.42+0.37) +1.3%\n 1450.7: fsck with 10 skipped bad commits 7.77(7.37+0.40) 7.87(7.47+0.40) +1.3%\n 1450.9: fsck with 100 skipped bad commits 7.82(7.41+0.40) 7.88(7.43+0.44) +0.8%\n 1450.11: fsck with 1000 skipped bad commits 7.88(7.49+0.39) 7.84(7.43+0.40) -0.5%\n 1450.13: fsck with 10000 skipped bad commits 8.02(7.63+0.39) 8.07(7.67+0.39) +0.6%\n 1450.15: fsck with 100000 skipped bad commits 8.01(7.60+0.41) 8.08(7.70+0.38) +0.9%\n 1450.17: fsck with 1000000 skipped bad commits 7.60(7.10+0.50) 7.37(7.18+0.19) -3.0%\n\nHelped-by: Jeff King \nSigned-off-by: Rene Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fsck.c | 25 ++++++++++++-------------\n t/t5504-fetch-receive-strict.sh | 2 +-\n 2 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex a0cee0be59..972a26b9ba 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -183,8 +183,9 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n static void init_skiplist(struct fsck_options *options, const char *path)\n {\n \tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n-\tint sorted, fd;\n-\tchar buffer[GIT_MAX_HEXSZ + 1];\n+\tint sorted;\n+\tFILE *fp;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n \tif (options->skiplist)\n@@ -194,25 +195,23 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\toptions->skiplist = &skiplist;\n \t}\n \n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n+\tfp = fopen(path, \"r\");\n+\tif (!fp)\n \t\tdie(\"Could not open skip list: %s\", path);\n-\tfor (;;) {\n+\twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n-\t\tint result = read_in_full(fd, buffer, sizeof(buffer));\n-\t\tif (result < 0)\n-\t\t\tdie_errno(\"Could not read '%s'\", path);\n-\t\tif (!result)\n-\t\t\tbreak;\n-\t\tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n')\n-\t\t\tdie(\"Invalid SHA-1: %s\", buffer);\n+\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n+\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n \t\toid_array_append(&skiplist, &oid);\n \t\tif (sorted && skiplist.nr > 1 &&\n \t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n \t\t\t\t &oid) > 0)\n \t\t\tsorted = 0;\n \t}\n-\tclose(fd);\n+\tif (ferror(fp))\n+\t\tdie_errno(\"Could not read '%s'\", path);\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\n \n \tif (sorted)\n \t\tskiplist.sorted = 1;\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 38aaf3b928..c7224db3bb 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -185,7 +185,7 @@ test_expect_success 'fsck with invalid or bogus skipList input (comments & empty\n \ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n '\n \n-test_expect_failure 'fsck no garbage output from comments & empty lines errors' '\n+test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n \ttest_line_count = 1 err-with-comment &&\n \ttest_line_count = 1 err-with-empty-line\n '\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356656","messageId":"20180828095219.23296-8-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 7/8] fsck: use oidset instead of oid_array for skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:18Z","receivedAt":"2018-08-28T09:52:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nChange the implementation of the skipList feature to use oidset\ninstead of oid_array to store SHA-1s for later lookup.\n\nThis list is parsed once on startup by fsck, fetch-pack or\nreceive-pack depending on the *.skipList config in use. I.e. only once\nper invocation, but note that for \"clone --recurse-submodules\" each\nsubmodule will re-parse the list, in addition to the main project.\n\nMemory usage is a bit higher, but we don't need to keep track of the\nsort order anymore. Embed the oidset into struct fsck_options to make\nits ownership clear (no hidden sharing) and avoid unnecessary pointer\nindirection.\n\nThe cumulative impact on performance of this & the preceding change,\nusing the test setup described in the previous commit:\n\n Test HEAD~2 HEAD~ HEAD\n ----------------------------------------------------------------------------------------------------------------\n 1450.3: fsck with 0 skipped bad commits 7.70(7.31+0.38) 7.72(7.33+0.38) +0.3% 7.70(7.30+0.40) +0.0%\n 1450.5: fsck with 1 skipped bad commits 7.84(7.47+0.37) 7.69(7.32+0.36) -1.9% 7.71(7.29+0.41) -1.7%\n 1450.7: fsck with 10 skipped bad commits 7.81(7.40+0.40) 7.94(7.57+0.36) +1.7% 7.92(7.55+0.37) +1.4%\n 1450.9: fsck with 100 skipped bad commits 7.81(7.42+0.38) 7.95(7.53+0.41) +1.8% 7.83(7.42+0.41) +0.3%\n 1450.11: fsck with 1000 skipped bad commits 7.99(7.62+0.36) 7.90(7.50+0.40) -1.1% 7.86(7.49+0.37) -1.6%\n 1450.13: fsck with 10000 skipped bad commits 7.98(7.57+0.40) 7.94(7.53+0.40) -0.5% 7.90(7.45+0.44) -1.0%\n 1450.15: fsck with 100000 skipped bad commits 7.97(7.57+0.39) 8.03(7.67+0.36) +0.8% 7.84(7.43+0.41) -1.6%\n 1450.17: fsck with 1000000 skipped bad commits 7.72(7.22+0.50) 7.28(7.07+0.20) -5.7% 7.13(6.87+0.25) -7.6%\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Rene Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 11 ++++++-----\n fsck.c | 23 ++---------------------\n fsck.h | 8 +++++---\n 3 files changed, 13 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c7f033f036..ebaa044689 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1731,11 +1731,12 @@ all three of them they must all set to the same values.\n +\n Older versions of Git (before 2.20) documented that the object names\n list should be sorted. This was never a requirement, the object names\n-can appear in any order, but when reading the list we track whether\n-the list is sorted for the purposes of an internal binary search\n-implementation, which can save itself some work with an already sorted\n-list. Unless you have a humongous list there's no reason to go out of\n-your way to pre-sort the list.\n+could appear in any order, but when reading the list we tracked whether\n+the list was sorted for the purposes of an internal binary search\n+implementation, which could save itself some work with an already sorted\n+list. Unless you had a humongous list there was no reason to go out of\n+your way to pre-sort the list. After Git version 2.20 a hash implementation\n+is used instead, so there's now no reason to pre-sort the list.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/fsck.c b/fsck.c\nindex 972a26b9ba..4c643f1d40 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -10,7 +10,6 @@\n #include \"fsck.h\"\n #include \"refs.h\"\n #include \"utf8.h\"\n-#include \"sha1-array.h\"\n #include \"decorate.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n@@ -182,19 +181,10 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n \n static void init_skiplist(struct fsck_options *options, const char *path)\n {\n-\tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n-\tint sorted;\n \tFILE *fp;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n-\tif (options->skiplist)\n-\t\tsorted = options->skiplist->sorted;\n-\telse {\n-\t\tsorted = 1;\n-\t\toptions->skiplist = &skiplist;\n-\t}\n-\n \tfp = fopen(path, \"r\");\n \tif (!fp)\n \t\tdie(\"Could not open skip list: %s\", path);\n@@ -202,19 +192,12 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\tconst char *p;\n \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n-\t\toid_array_append(&skiplist, &oid);\n-\t\tif (sorted && skiplist.nr > 1 &&\n-\t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n-\t\t\t\t &oid) > 0)\n-\t\t\tsorted = 0;\n+\t\toidset_insert(&options->skiplist, &oid);\n \t}\n \tif (ferror(fp))\n \t\tdie_errno(\"Could not read '%s'\", path);\n \tfclose(fp);\n \tstrbuf_release(&sb);\n-\n-\tif (sorted)\n-\t\tskiplist.sorted = 1;\n }\n \n static int parse_msg_type(const char *str)\n@@ -319,9 +302,7 @@ static void append_msg_id(struct strbuf *sb, const char *msg_id)\n \n static int object_on_skiplist(struct fsck_options *opts, struct object *obj)\n {\n-\tif (opts && opts->skiplist && obj)\n-\t\treturn oid_array_lookup(opts->skiplist, &obj->oid) >= 0;\n-\treturn 0;\n+\treturn opts && obj && oidset_contains(&opts->skiplist, &obj->oid);\n }\n \n __attribute__((format (printf, 4, 5)))\ndiff --git a/fsck.h b/fsck.h\nindex 0c7e8c9428..b95595ae5f 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -1,6 +1,8 @@\n #ifndef GIT_FSCK_H\n #define GIT_FSCK_H\n \n+#include \"oidset.h\"\n+\n #define FSCK_ERROR 1\n #define FSCK_WARN 2\n #define FSCK_IGNORE 3\n@@ -35,12 +37,12 @@ struct fsck_options {\n \tfsck_error error_func;\n \tunsigned strict:1;\n \tint *msg_type;\n-\tstruct oid_array *skiplist;\n+\tstruct oidset skiplist;\n \tstruct decoration *object_names;\n };\n \n-#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }\n-#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }\n+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT }\n+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT }\n \n /* descend in all linked child objects\n * the return value is:\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356657","messageId":"20180828095219.23296-9-avarab@gmail.com","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"[PATCH v4 8/8] fsck: support comments & empty lines in skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T09:52:19Z","receivedAt":"2018-08-28T09:52:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It's annoying not to be able to put comments and empty lines in the\nskipList, when e.g. keeping a big central list of commits to skip in\n/etc/gitconfig, which was my motivation for 1362df0d41 (\"fetch:\nimplement fetch.fsck.*\", 2018-07-27).\n\nImplement that, and document what version of Git this was changed in,\nsince this on-disk format can be expected to be used by multiple\nversions of git.\n\nThere is no notable performance impact from this change, using the\ntest setup described a couple of commist back:\n\n Test HEAD~ HEAD\n ----------------------------------------------------------------------------------------\n 1450.3: fsck with 0 skipped bad commits 7.81(7.42+0.39) 7.72(7.34+0.38) -1.2%\n 1450.5: fsck with 1 skipped bad commits 7.75(7.36+0.38) 7.66(7.26+0.39) -1.2%\n 1450.7: fsck with 10 skipped bad commits 7.81(7.43+0.38) 7.70(7.30+0.39) -1.4%\n 1450.9: fsck with 100 skipped bad commits 7.85(7.42+0.42) 7.73(7.31+0.41) -1.5%\n 1450.11: fsck with 1000 skipped bad commits 7.81(7.43+0.38) 7.84(7.46+0.38) +0.4%\n 1450.13: fsck with 10000 skipped bad commits 7.87(7.47+0.40) 7.86(7.46+0.40) -0.1%\n 1450.15: fsck with 100000 skipped bad commits 7.77(7.39+0.38) 7.83(7.48+0.34) +0.8%\n 1450.17: fsck with 1000000 skipped bad commits 7.17(6.92+0.24) 7.11(6.85+0.26) -0.8%\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 4 ++--\n fsck.c | 2 ++\n t/t5504-fetch-receive-strict.sh | 6 +++---\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ebaa044689..824634c412 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1712,8 +1712,8 @@ will only cause git to warn.\n fsck.skipList::\n \tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. Comments ('#') and empty lines are not supported, and\n-\twill error out.\n+\tbe ignored. On versions of Git 2.20 and later comments ('#') and empty\n+\tlines are ignored, but will error out on older versions.\n +\n This feature is useful when an established project should be accepted\n despite early commits containing errors that can be safely ignored\ndiff --git a/fsck.c b/fsck.c\nindex 4c643f1d40..589548308a 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -190,6 +190,8 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\tdie(\"Could not open skip list: %s\", path);\n \twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n+\t\tif (!strcmp(sb.buf, \"\") || starts_with(sb.buf, \"#\"))\n+\t\t\tcontinue;\n \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n \t\toidset_insert(&options->skiplist, &oid);\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex c7224db3bb..a1bac164d1 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -169,20 +169,20 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n '\n \n-test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n+test_expect_success 'fsck with other accepted skipList input (comments & empty lines)' '\n \tcat >SKIP.with-comment <<-EOF &&\n \t# Some bad commit\n \t0000000000000000000000000000000000000001\n \tEOF\n \ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n-\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n+\ttest_i18ngrep \"missingEmail\" err-with-comment &&\n \tcat >SKIP.with-empty-line <<-EOF &&\n \t0000000000000000000000000000000000000001\n \n \t0000000000000000000000000000000000000002\n \tEOF\n \ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n-\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n+\ttest_i18ngrep \"missingEmail\" err-with-empty-line\n '\n \n test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356697","messageId":"c356f239-f3f4-242b-fb75-95a4ccbe374b@web.de","threadId":"49227","inReplyTo":"87lg8refcr.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 6/7] fsck: use oidset for skiplist","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-28T14:29:09Z","receivedAt":"2018-08-28T14:29:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.08.2018 um 22:15 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Mon, Aug 27 2018, Ævar Arnfjörð Bjarmason wrote:\n> \n>> From: René Scharfe \n>>\n>> Object IDs to skip are stored in a shared static oid_array. Lookups do\n>> a binary search on the sorted array. The code checks if the object IDs\n>> are already in the correct order while loading and skips sorting in that\n>> case. Lookups are done before reporting a (non-fatal) corruption and\n>> before checking .gitmodules files.\n>>\n>> Simplify the code by using an oidset instead. Memory usage is a bit\n>> higher, but we don't need to worry about any sort order anymore. Embed\n>> the oidset into struct fsck_options to make its ownership clear (no\n>> hidden sharing) and avoid unnecessary pointer indirection.\n>>\n>> Performance on repositories with a low number of reported issues and\n>> .gitmodules files (i.e. the usual case) won't be affected much. The\n>> oidset should be a bit quicker with higher numbers of bad objects in\n>> the skipList.\n> \n> I didn't change this commit message at all, but FWIW this still has me\n> somewhat confused. What is the interaction with .gitmodules being\n> described here? You also mentioned it in\n> https://public-inbox.org/git/113aa2d7-6f66-8d03-dda4-7337cda9b2df@web.de/\n> (but I don't get that either)\n\nThe skipList is consulted before checking .gitmodules blobs, since\nfb16287719 (fsck: check skiplist for object in fsck_blob()).\n\n> Does that just mean that when cloning with --recursive with\n> transfer.fsckObjects=true we'll re-read the file for each \"clone\"\n> invocation, both for the main project and everything listed in\n> .gitmodules?\n\nThat is probably true, but I didn't mean that. I was only talking\nabout when an object is looked up in the skiplist (oid_array/oidset).\n\n> If so, I think something like this commit message would be clearer:\n> \n> fsck: use oidset instead of oid_array for skipList\n> \n> Change the implementation of the skipList feature to use oidset\n> instead of oid_array to store SHA-1s for later lookup.\n> \n> This list is parsed once on startup by fsck, fetch-pack or\n> receive-pack depending on the *.skipList config in use, so for fetch\n> it's currently re-parsed for each submodule fetch.\n\nOK; the patch doesn't change that, though. Mentioning it sound like\na good idea if the load takes a significant amount of time -- I\ndidn't measure this separately, so perhaps this needs to be explored\nfurther if people use big skipLists.\n\n> Memory usage is a bit higher, but we don't need to keep track of the\n> sort order anymore. Embed the oidset into struct fsck_options to\n> make its ownership clear (no hidden sharing) and avoid unnecessary\n> pointer indirection.\n> \n> Then let's just attach the test program you wrote in\n> https://public-inbox.org/git/113aa2d7-6f66-8d03-dda4-7337cda9b2df@web.de/\n> and note the results and let them speak for themselves.\n\nI was surprised that such a big repo can be mocked up relatively\nquickly, but the numbers are a bit underwhelming -- there is not a\nlot of change. The script is not too expensive, so I don't mind\nadding it.\n\n> I can do all that if that seems fine to you. I also notice that the test\n> case only sets up a case where all the items on the skip list are bad\n> commits in the repo, it would be interesting to test with a few needles\n> in a large haystack (I can modify it to do that...).\n\nI tried something like this first, and its results are even more\nboring. Since skipList lookups are done only for bad objects (and\n.gitmodules) you won't see any difference at all. Having only bad\nobjects is the edge case, this test being designed to highlight the\nperformance of the skipList implementation.\n\nRené\n"},{"id":"356700","messageId":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","threadId":"49227","inReplyTo":"20180828095219.23296-9-avarab@gmail.com","subject":"Re: [PATCH v4 8/8] fsck: support comments & empty lines in skipList","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-28T14:59:27Z","receivedAt":"2018-08-28T15:04:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.08.2018 um 11:52 schrieb Ævar Arnfjörð Bjarmason:\n> It's annoying not to be able to put comments and empty lines in the\n> skipList, when e.g. keeping a big central list of commits to skip in\n> /etc/gitconfig, which was my motivation for 1362df0d41 (\"fetch:\n> implement fetch.fsck.*\", 2018-07-27).\n> \n> Implement that, and document what version of Git this was changed in,\n> since this on-disk format can be expected to be used by multiple\n> versions of git.\n> \n> There is no notable performance impact from this change, using the\n> test setup described a couple of commist back:\n> \n> Test HEAD~ HEAD\n> ----------------------------------------------------------------------------------------\n> 1450.3: fsck with 0 skipped bad commits 7.81(7.42+0.39) 7.72(7.34+0.38) -1.2%\n> 1450.5: fsck with 1 skipped bad commits 7.75(7.36+0.38) 7.66(7.26+0.39) -1.2%\n> 1450.7: fsck with 10 skipped bad commits 7.81(7.43+0.38) 7.70(7.30+0.39) -1.4%\n> 1450.9: fsck with 100 skipped bad commits 7.85(7.42+0.42) 7.73(7.31+0.41) -1.5%\n> 1450.11: fsck with 1000 skipped bad commits 7.81(7.43+0.38) 7.84(7.46+0.38) +0.4%\n> 1450.13: fsck with 10000 skipped bad commits 7.87(7.47+0.40) 7.86(7.46+0.40) -0.1%\n> 1450.15: fsck with 100000 skipped bad commits 7.77(7.39+0.38) 7.83(7.48+0.34) +0.8%\n> 1450.17: fsck with 1000000 skipped bad commits 7.17(6.92+0.24) 7.11(6.85+0.26) -0.8%\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config.txt | 4 ++--\n> fsck.c | 2 ++\n> t/t5504-fetch-receive-strict.sh | 6 +++---\n> 3 files changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ebaa044689..824634c412 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1712,8 +1712,8 @@ will only cause git to warn.\n> fsck.skipList::\n> \tThe path to a list of object names (i.e. one SHA-1 per\n> \tline) that are known to be broken in a non-fatal way and should\n> -\tbe ignored. Comments ('#') and empty lines are not supported, and\n> -\twill error out.\n> +\tbe ignored. On versions of Git 2.20 and later comments ('#') and empty\n> +\tlines are ignored, but will error out on older versions.\n> +\n> This feature is useful when an established project should be accepted\n> despite early commits containing errors that can be safely ignored\n> diff --git a/fsck.c b/fsck.c\n> index 4c643f1d40..589548308a 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -190,6 +190,8 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n> \t\tdie(\"Could not open skip list: %s\", path);\n> \twhile (!strbuf_getline(&sb, fp)) {\n> \t\tconst char *p;\n> +\t\tif (!strcmp(sb.buf, \"\") || starts_with(sb.buf, \"#\"))\n> +\t\t\tcontinue;\n\nChecking sb.len == 0 is simpler and might be slightly quicker.\n\nBut what is an empty line? Shouldn't whitespace-only lines qualify?\nAnd why not allow trailing comments, while we're at it? I.e. what\nabout something like this instead?\n\n\t\tconst char *hash = strchr(sb.buf, '#);\n\t\tif (hash)\n\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n\t\tstrbuf_rtrim(&sb);\n\t\tif (!sb.len)\n\t\t\tcontinue;\n\nToo much?\n\n> \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n> \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n> \t\toidset_insert(&options->skiplist, &oid);\n> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> index c7224db3bb..a1bac164d1 100755\n> --- a/t/t5504-fetch-receive-strict.sh\n> +++ b/t/t5504-fetch-receive-strict.sh\n> @@ -169,20 +169,20 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n> \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n> '\n> \n> -test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n> +test_expect_success 'fsck with other accepted skipList input (comments & empty lines)' '\n> \tcat >SKIP.with-comment <<-EOF &&\n> \t# Some bad commit\n> \t0000000000000000000000000000000000000001\n> \tEOF\n> \ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n> -\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n> +\ttest_i18ngrep \"missingEmail\" err-with-comment &&\n> \tcat >SKIP.with-empty-line <<-EOF &&\n> \t0000000000000000000000000000000000000001\n> \n> \t0000000000000000000000000000000000000002\n> \tEOF\n> \ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n> -\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n> +\ttest_i18ngrep \"missingEmail\" err-with-empty-line\n> '\n> \n> test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n> \n"},{"id":"357203","messageId":"20180903144928.30691-1-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 00/10] use oidset for skiplist + docs + tests + comment support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:18Z","receivedAt":"2018-09-03T14:49:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Addresses feedback René had on v4. Notes below:\n\nRené Scharfe (3):\n fsck: add a performance test for skipList\n fsck: use strbuf_getline() to read skiplist file\n\nNo changes.\n\n fsck: use oidset instead of oid_array for skipList\n\nNow we note in the commit message that the skipList is re-consulted\nfor .gitmodules blobs, as René pointed out.\n\nÆvar Arnfjörð Bjarmason (7):\n fsck tests: setup of bogus commit object\n fsck tests: add a test for no skipList input\n\nNo changes.\n\n fsck: document and test sorted skipList input\n\nTrivial whitespace change in new docs paragraph.\n\n fsck: document and test commented & empty line skipList input\n\nNo changes.\n\n fsck: document that skipList input must be unabbreviated\n\nNEW: Now we document & test for the SHA-1s on the skip list being\nunabbreviated. It was always like this, but let's say so / test for\nit.\n\n fsck: add a performance test\n\nNEW: Add a generic new fsck perf test while I'm at it, in addition to\nthe more specific one René wrote.\n\n fsck: support comments & empty lines in skipList\n\nThe major change this time around, we now support trailing comments,\nleading whitespace before comments, whitespace-only lines etc. I went\none step further than what René suggested and decided to just call\nstrbuf_trim() so now we also support SHA-1s with leading spaces. I\ndon't see a good reason to go out of our way to ban those, and it's\nconsistent with other formats we use (and perhaps people will use this\nto align them in some way).\n\n Documentation/config.txt | 23 ++++++++---\n fsck.c | 60 +++++++++++++--------------\n fsck.h | 8 ++--\n t/perf/p1450-fsck.sh | 13 ++++++\n t/perf/p1451-fsck-skip-list.sh | 40 ++++++++++++++++++\n t/t5504-fetch-receive-strict.sh | 72 +++++++++++++++++++++++++++++++--\n 6 files changed, 171 insertions(+), 45 deletions(-)\n create mode 100755 t/perf/p1450-fsck.sh\n create mode 100755 t/perf/p1451-fsck-skip-list.sh\n\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357204","messageId":"20180903144928.30691-2-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 01/10] fsck tests: setup of bogus commit object","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:19Z","receivedAt":"2018-09-03T14:49:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Several fsck tests used the exact same git-hash-object output, but had\ncopy/pasted that part of the setup code. Let's instead do that setup\nonce and use it in subsequent tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 62f3569891..6d268f3327 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -133,6 +133,10 @@ committer Bugs Bunny 1234567890 +0000\n This commit object intentionally broken\n EOF\n \n+test_expect_success 'setup bogus commit' '\n+\tcommit=\"$(git hash-object -t commit -w --stdin err &&\n@@ -142,7 +146,6 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n '\n \n test_expect_success 'push with receive.fsck.skipList' '\n-\tcommit=\"$(git hash-object -t commit -w --stdin dies' '\n '\n \n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n-\tcommit=\"$(git hash-object -t commit -w --stdin \n---\n t/t5504-fetch-receive-strict.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 6d268f3327..cbae31f330 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -137,6 +137,11 @@ test_expect_success 'setup bogus commit' '\n \tcommit=\"$(git hash-object -t commit -w --stdin err &&\n+\ttest_i18ngrep \"missingEmail\" err\n+'\n+\n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357206","messageId":"20180903144928.30691-4-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 03/10] fsck: document and test sorted skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:21Z","receivedAt":"2018-09-03T14:49:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Ever since the skipList support was first added in cd94c6f91 (\"fsck:\ngit receive-pack: support excluding objects from fsck'ing\",\n2015-06-22) the documentation for the format has that the file is a\nsorted list of object names.\n\nThus, anyone using the feature would have thought the list needed to\nbe sorted. E.g. I recently in conjunction with my fetch.fsck.*\nimplementation in 1362df0d41 (\"fetch: implement fetch.fsck.*\",\n2018-07-27) wrote some code to ship a skipList, and went out of my way\nto sort it.\n\nDoing so seems intuitive, since it contains fixed-width records, and\nhas no support for comments, so one might expect it to be binary\nsearched in-place on-disk.\n\nHowever, as documented here this was never a requirement, so let's\nchange the documentation. Since this is a file format change let's\nalso document what was said about this in the past, so e.g. someone\nlike myself reading the new docs can see this never needed to be\nsorted (\"why do I have all this code to sort this thing...\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 10 +++++++++-\n t/t5504-fetch-receive-strict.sh | 19 +++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex eb66a11975..fd1b5837d0 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1710,7 +1710,7 @@ doing the same for `receive.fsck.` and `fetch.fsck.`\n will only cause git to warn.\n \n fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n+\tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n \tbe ignored. This feature is useful when an established project\n \tshould be accepted despite early commits containing errors that\n@@ -1725,6 +1725,14 @@ Unlike variables like `color.ui` and `core.editor` the\n fall back on the `fsck.skipList` configuration if they aren't set. To\n uniformly configure the same fsck settings in different circumstances\n all three of them they must all set to the same values.\n++\n+Older versions of Git (before 2.20) documented that the object names\n+list should be sorted. This was never a requirement, the object names\n+can appear in any order, but when reading the list we track whether\n+the list is sorted for the purposes of an internal binary search\n+implementation, which can save itself some work with an already sorted\n+list. Unless you have a humongous list there's no reason to go out of\n+your way to pre-sort the list.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex cbae31f330..fa56052f0f 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -142,6 +142,25 @@ test_expect_success 'fsck with no skipList input' '\n \ttest_i18ngrep \"missingEmail\" err\n '\n \n+test_expect_success 'setup sorted and unsorted skipLists' '\n+\tcat >SKIP.unsorted <<-EOF &&\n+\t0000000000000000000000000000000000000004\n+\t0000000000000000000000000000000000000002\n+\t$commit\n+\t0000000000000000000000000000000000000001\n+\t0000000000000000000000000000000000000003\n+\tEOF\n+\tsort SKIP.unsorted >SKIP.sorted\n+'\n+\n+test_expect_success 'fsck with sorted skipList' '\n+\tgit -c fsck.skipList=SKIP.sorted fsck\n+'\n+\n+test_expect_success 'fsck with unsorted skipList' '\n+\tgit -c fsck.skipList=SKIP.unsorted fsck\n+'\n+\n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357207","messageId":"20180903144928.30691-5-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 04/10] fsck: document and test commented & empty line skipList input","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:22Z","receivedAt":"2018-09-03T14:49:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There is currently no comment syntax for the fsck.skipList, this isn't\nreally by design, and it would be nice to have support for comments.\n\nDocument that this doesn't work, and test for how this errors\nout. These tests reveal a current bug, if there's invalid input the\noutput will emit some of the next line, and then go into uninitialized\nmemory. This is fixed in a subsequent change.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 11 +++++++----\n t/t5504-fetch-receive-strict.sh | 21 +++++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fd1b5837d0..0e1ce7de8b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1712,10 +1712,13 @@ will only cause git to warn.\n fsck.skipList::\n \tThe path to a list of object names (i.e. one SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tbe ignored. Comments ('#') and empty lines are not supported, and\n+\twill error out.\n++\n+This feature is useful when an established project should be accepted\n+despite early commits containing errors that can be safely ignored\n+such as invalid committer email addresses. Note: corrupt objects\n+cannot be skipped with this setting.\n +\n Like `fsck.` this variable has corresponding\n `receive.fsck.skipList` and `fetch.fsck.skipList` variants.\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex fa56052f0f..38aaf3b928 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -169,6 +169,27 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n '\n \n+test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n+\tcat >SKIP.with-comment <<-EOF &&\n+\t# Some bad commit\n+\t0000000000000000000000000000000000000001\n+\tEOF\n+\ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n+\tcat >SKIP.with-empty-line <<-EOF &&\n+\t0000000000000000000000000000000000000001\n+\n+\t0000000000000000000000000000000000000002\n+\tEOF\n+\ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n+'\n+\n+test_expect_failure 'fsck no garbage output from comments & empty lines errors' '\n+\ttest_line_count = 1 err-with-comment &&\n+\ttest_line_count = 1 err-with-empty-line\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tgit push . $commit:refs/heads/bogus &&\n \trm -rf dst &&\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357208","messageId":"20180903144928.30691-6-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 05/10] fsck: document that skipList input must be unabbreviated","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:23Z","receivedAt":"2018-09-03T14:49:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Abbreviating the SHA-1s in the skipList input has never worked, but\nthe documentation hasn't unambiguously stated that this is an error,\nand there was no test for it.\n\nLet's fix both since it would be easy for some later refactoring\ne.g. switch to accidentally switch to a looser OID parsing function,\ncausing the tests before this change to pass, but for older versions\nof git to be incompatible with the new skipList format.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 2 +-\n t/t5504-fetch-receive-strict.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0e1ce7de8b..3287c7ef8a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1710,7 +1710,7 @@ doing the same for `receive.fsck.` and `fetch.fsck.`\n will only cause git to warn.\n \n fsck.skipList::\n-\tThe path to a list of object names (i.e. one SHA-1 per\n+\tThe path to a list of object names (i.e. one unabbreviated SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n \tbe ignored. Comments ('#') and empty lines are not supported, and\n \twill error out.\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 38aaf3b928..96bf9facbd 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -190,6 +190,12 @@ test_expect_failure 'fsck no garbage output from comments & empty lines errors'\n \ttest_line_count = 1 err-with-empty-line\n '\n \n+test_expect_success 'fsck with invalid abbreviated skipList input' '\n+\techo $commit | test_copy_bytes 20 >SKIP.abbreviated &&\n+\ttest_must_fail git -c fsck.skipList=SKIP.abbreviated fsck 2>err-abbreviated &&\n+\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-abbreviated\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tgit push . $commit:refs/heads/bogus &&\n \trm -rf dst &&\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357209","messageId":"20180903144928.30691-7-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 06/10] fsck: add a performance test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:24Z","receivedAt":"2018-09-03T14:49:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a plain performance test for \"fsck\". This test will not be used to\n/ referred to in any upcoming commit of mine in this series, but\nhaving a simple test for fsck performance is valuable, so let's add it\nwhile we're at it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/p1450-fsck.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n create mode 100755 t/perf/p1450-fsck.sh\n\ndiff --git a/t/perf/p1450-fsck.sh b/t/perf/p1450-fsck.sh\nnew file mode 100755\nindex 0000000000..ae1b84198b\n--- /dev/null\n+++ b/t/perf/p1450-fsck.sh\n@@ -0,0 +1,13 @@\n+#!/bin/sh\n+\n+test_description='Test fsck performance'\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+\n+test_perf 'fsck' '\n+\tgit fsck\n+'\n+\n+test_done\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357210","messageId":"20180903144928.30691-9-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 08/10] fsck: use strbuf_getline() to read skiplist file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:26Z","receivedAt":"2018-09-03T14:49:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nThe buffer is unlikely to contain a NUL character, so printing its\ncontents using %s in a die() format is unsafe (detected with ASan).\n\nUse an idiomatic strbuf_getline() loop instead, which ensures the buffer\nis always NUL-terminated, supports CRLF files as well, accepts files\nwithout a newline after the last line, supports any hash length\nautomatically, and is shorter.\n\nThis fixes a bug where emitting an error about an invalid line on say\nline 1 would continue printing subsequent lines, and usually continue\ninto uninitialized memory.\n\nThe performance impact of this, on a CentOS 7 box with RedHat GCC\n4.8.5-28:\n\n $ GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS='-j56 CFLAGS=\"-O3\"' ./run HEAD~ HEAD p1451-fsck-skip-list.sh\n Test HEAD~ HEAD\n ----------------------------------------------------------------------------------------\n 1450.3: fsck with 0 skipped bad commits 7.75(7.39+0.35) 7.68(7.29+0.39) -0.9%\n 1450.5: fsck with 1 skipped bad commits 7.70(7.30+0.40) 7.80(7.42+0.37) +1.3%\n 1450.7: fsck with 10 skipped bad commits 7.77(7.37+0.40) 7.87(7.47+0.40) +1.3%\n 1450.9: fsck with 100 skipped bad commits 7.82(7.41+0.40) 7.88(7.43+0.44) +0.8%\n 1450.11: fsck with 1000 skipped bad commits 7.88(7.49+0.39) 7.84(7.43+0.40) -0.5%\n 1450.13: fsck with 10000 skipped bad commits 8.02(7.63+0.39) 8.07(7.67+0.39) +0.6%\n 1450.15: fsck with 100000 skipped bad commits 8.01(7.60+0.41) 8.08(7.70+0.38) +0.9%\n 1450.17: fsck with 1000000 skipped bad commits 7.60(7.10+0.50) 7.37(7.18+0.19) -3.0%\n\nHelped-by: Jeff King \nSigned-off-by: Rene Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fsck.c | 25 ++++++++++++-------------\n t/t5504-fetch-receive-strict.sh | 2 +-\n 2 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex a0cee0be59..972a26b9ba 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -183,8 +183,9 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n static void init_skiplist(struct fsck_options *options, const char *path)\n {\n \tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n-\tint sorted, fd;\n-\tchar buffer[GIT_MAX_HEXSZ + 1];\n+\tint sorted;\n+\tFILE *fp;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n \tif (options->skiplist)\n@@ -194,25 +195,23 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\toptions->skiplist = &skiplist;\n \t}\n \n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n+\tfp = fopen(path, \"r\");\n+\tif (!fp)\n \t\tdie(\"Could not open skip list: %s\", path);\n-\tfor (;;) {\n+\twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n-\t\tint result = read_in_full(fd, buffer, sizeof(buffer));\n-\t\tif (result < 0)\n-\t\t\tdie_errno(\"Could not read '%s'\", path);\n-\t\tif (!result)\n-\t\t\tbreak;\n-\t\tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n')\n-\t\t\tdie(\"Invalid SHA-1: %s\", buffer);\n+\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n+\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n \t\toid_array_append(&skiplist, &oid);\n \t\tif (sorted && skiplist.nr > 1 &&\n \t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n \t\t\t\t &oid) > 0)\n \t\t\tsorted = 0;\n \t}\n-\tclose(fd);\n+\tif (ferror(fp))\n+\t\tdie_errno(\"Could not read '%s'\", path);\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\n \n \tif (sorted)\n \t\tskiplist.sorted = 1;\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 96bf9facbd..d67ab37321 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -185,7 +185,7 @@ test_expect_success 'fsck with invalid or bogus skipList input (comments & empty\n \ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n '\n \n-test_expect_failure 'fsck no garbage output from comments & empty lines errors' '\n+test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n \ttest_line_count = 1 err-with-comment &&\n \ttest_line_count = 1 err-with-empty-line\n '\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357211","messageId":"20180903144928.30691-10-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 09/10] fsck: use oidset instead of oid_array for skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:27Z","receivedAt":"2018-09-03T14:49:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nChange the implementation of the skipList feature to use oidset\ninstead of oid_array to store SHA-1s for later lookup.\n\nThis list is parsed once on startup by fsck, fetch-pack or\nreceive-pack depending on the *.skipList config in use. I.e. only once\nper invocation, but note that for \"clone --recurse-submodules\" each\nsubmodule will re-parse the list, in addition to the main project, and\nit will be re-parsed when checking .gitmodules blobs, see\nfb16287719 (\"fsck: check skiplist for object in fsck_blob()\",\n2018-06-27).\n\nMemory usage is a bit higher, but we don't need to keep track of the\nsort order anymore. Embed the oidset into struct fsck_options to make\nits ownership clear (no hidden sharing) and avoid unnecessary pointer\nindirection.\n\nThe cumulative impact on performance of this & the preceding change,\nusing the test setup described in the previous commit:\n\n Test HEAD~2 HEAD~ HEAD\n ----------------------------------------------------------------------------------------------------------------\n 1450.3: fsck with 0 skipped bad commits 7.70(7.31+0.38) 7.72(7.33+0.38) +0.3% 7.70(7.30+0.40) +0.0%\n 1450.5: fsck with 1 skipped bad commits 7.84(7.47+0.37) 7.69(7.32+0.36) -1.9% 7.71(7.29+0.41) -1.7%\n 1450.7: fsck with 10 skipped bad commits 7.81(7.40+0.40) 7.94(7.57+0.36) +1.7% 7.92(7.55+0.37) +1.4%\n 1450.9: fsck with 100 skipped bad commits 7.81(7.42+0.38) 7.95(7.53+0.41) +1.8% 7.83(7.42+0.41) +0.3%\n 1450.11: fsck with 1000 skipped bad commits 7.99(7.62+0.36) 7.90(7.50+0.40) -1.1% 7.86(7.49+0.37) -1.6%\n 1450.13: fsck with 10000 skipped bad commits 7.98(7.57+0.40) 7.94(7.53+0.40) -0.5% 7.90(7.45+0.44) -1.0%\n 1450.15: fsck with 100000 skipped bad commits 7.97(7.57+0.39) 8.03(7.67+0.36) +0.8% 7.84(7.43+0.41) -1.6%\n 1450.17: fsck with 1000000 skipped bad commits 7.72(7.22+0.50) 7.28(7.07+0.20) -5.7% 7.13(6.87+0.25) -7.6%\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Rene Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 11 ++++++-----\n fsck.c | 23 ++---------------------\n fsck.h | 8 +++++---\n 3 files changed, 13 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3287c7ef8a..161ffe259e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1731,11 +1731,12 @@ all three of them they must all set to the same values.\n +\n Older versions of Git (before 2.20) documented that the object names\n list should be sorted. This was never a requirement, the object names\n-can appear in any order, but when reading the list we track whether\n-the list is sorted for the purposes of an internal binary search\n-implementation, which can save itself some work with an already sorted\n-list. Unless you have a humongous list there's no reason to go out of\n-your way to pre-sort the list.\n+could appear in any order, but when reading the list we tracked whether\n+the list was sorted for the purposes of an internal binary search\n+implementation, which could save itself some work with an already sorted\n+list. Unless you had a humongous list there was no reason to go out of\n+your way to pre-sort the list. After Git version 2.20 a hash implementation\n+is used instead, so there's now no reason to pre-sort the list.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/fsck.c b/fsck.c\nindex 972a26b9ba..4c643f1d40 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -10,7 +10,6 @@\n #include \"fsck.h\"\n #include \"refs.h\"\n #include \"utf8.h\"\n-#include \"sha1-array.h\"\n #include \"decorate.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n@@ -182,19 +181,10 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n \n static void init_skiplist(struct fsck_options *options, const char *path)\n {\n-\tstatic struct oid_array skiplist = OID_ARRAY_INIT;\n-\tint sorted;\n \tFILE *fp;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n-\tif (options->skiplist)\n-\t\tsorted = options->skiplist->sorted;\n-\telse {\n-\t\tsorted = 1;\n-\t\toptions->skiplist = &skiplist;\n-\t}\n-\n \tfp = fopen(path, \"r\");\n \tif (!fp)\n \t\tdie(\"Could not open skip list: %s\", path);\n@@ -202,19 +192,12 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\tconst char *p;\n \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n-\t\toid_array_append(&skiplist, &oid);\n-\t\tif (sorted && skiplist.nr > 1 &&\n-\t\t\t\toidcmp(&skiplist.oid[skiplist.nr - 2],\n-\t\t\t\t &oid) > 0)\n-\t\t\tsorted = 0;\n+\t\toidset_insert(&options->skiplist, &oid);\n \t}\n \tif (ferror(fp))\n \t\tdie_errno(\"Could not read '%s'\", path);\n \tfclose(fp);\n \tstrbuf_release(&sb);\n-\n-\tif (sorted)\n-\t\tskiplist.sorted = 1;\n }\n \n static int parse_msg_type(const char *str)\n@@ -319,9 +302,7 @@ static void append_msg_id(struct strbuf *sb, const char *msg_id)\n \n static int object_on_skiplist(struct fsck_options *opts, struct object *obj)\n {\n-\tif (opts && opts->skiplist && obj)\n-\t\treturn oid_array_lookup(opts->skiplist, &obj->oid) >= 0;\n-\treturn 0;\n+\treturn opts && obj && oidset_contains(&opts->skiplist, &obj->oid);\n }\n \n __attribute__((format (printf, 4, 5)))\ndiff --git a/fsck.h b/fsck.h\nindex 0c7e8c9428..b95595ae5f 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -1,6 +1,8 @@\n #ifndef GIT_FSCK_H\n #define GIT_FSCK_H\n \n+#include \"oidset.h\"\n+\n #define FSCK_ERROR 1\n #define FSCK_WARN 2\n #define FSCK_IGNORE 3\n@@ -35,12 +37,12 @@ struct fsck_options {\n \tfsck_error error_func;\n \tunsigned strict:1;\n \tint *msg_type;\n-\tstruct oid_array *skiplist;\n+\tstruct oidset skiplist;\n \tstruct decoration *object_names;\n };\n \n-#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }\n-#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }\n+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT }\n+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT }\n \n /* descend in all linked child objects\n * the return value is:\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357212","messageId":"20180903144928.30691-11-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 10/10] fsck: support comments & empty lines in skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:28Z","receivedAt":"2018-09-03T14:49:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It's annoying not to be able to put comments and empty lines in the\nskipList, when e.g. keeping a big central list of commits to skip in\n/etc/gitconfig, which was my motivation for 1362df0d41 (\"fetch:\nimplement fetch.fsck.*\", 2018-07-27).\n\nImplement that, and document what version of Git this was changed in,\nsince this on-disk format can be expected to be used by multiple\nversions of git.\n\nThere is no notable performance impact from this change, using the\ntest setup described a couple of commits back:\n\n Test HEAD~ HEAD\n ----------------------------------------------------------------------------------------\n 1450.3: fsck with 0 skipped bad commits 7.69(7.27+0.42) 7.86(7.48+0.37) +2.2%\n 1450.5: fsck with 1 skipped bad commits 7.69(7.30+0.38) 7.83(7.47+0.36) +1.8%\n 1450.7: fsck with 10 skipped bad commits 7.76(7.38+0.38) 7.79(7.38+0.41) +0.4%\n 1450.9: fsck with 100 skipped bad commits 7.76(7.38+0.38) 7.74(7.36+0.38) -0.3%\n 1450.11: fsck with 1000 skipped bad commits 7.71(7.30+0.41) 7.72(7.34+0.38) +0.1%\n 1450.13: fsck with 10000 skipped bad commits 7.74(7.34+0.40) 7.72(7.34+0.38) -0.3%\n 1450.15: fsck with 100000 skipped bad commits 7.75(7.40+0.35) 7.70(7.29+0.40) -0.6%\n 1450.17: fsck with 1000000 skipped bad commits 7.12(6.86+0.26) 7.13(6.87+0.26) +0.1%\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 5 +++--\n fsck.c | 14 ++++++++++++++\n t/t5504-fetch-receive-strict.sh | 19 ++++++++++++++++---\n 3 files changed, 33 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 161ffe259e..0906db3a99 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1712,8 +1712,9 @@ will only cause git to warn.\n fsck.skipList::\n \tThe path to a list of object names (i.e. one unabbreviated SHA-1 per\n \tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. Comments ('#') and empty lines are not supported, and\n-\twill error out.\n+\tbe ignored. On versions of Git 2.20 and later comments ('#'), empty\n+\tlines, and any leading and trailing whitespace is ignored. Everything\n+\tbut a SHA-1 per line will error out on older versions.\n +\n This feature is useful when an established project should be accepted\n despite early commits containing errors that can be safely ignored\ndiff --git a/fsck.c b/fsck.c\nindex 4c643f1d40..859b050b05 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -190,6 +190,20 @@ static void init_skiplist(struct fsck_options *options, const char *path)\n \t\tdie(\"Could not open skip list: %s\", path);\n \twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n+\t\tconst char *hash;\n+\n+\t\t/*\n+\t\t * Allow trailing comments, leading whitespace\n+\t\t * (including before commits), and empty or whitespace\n+\t\t * only lines.\n+\t\t */\n+\t\thash = strchr(sb.buf, '#');\n+\t\tif (hash)\n+\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n+\t\tstrbuf_trim(&sb);\n+\t\tif (!sb.len)\n+\t\t\tcontinue;\n+\n \t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n \t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n \t\toidset_insert(&options->skiplist, &oid);\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex d67ab37321..7bc706873c 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -169,20 +169,20 @@ test_expect_success 'fsck with invalid or bogus skipList input' '\n \ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n '\n \n-test_expect_success 'fsck with invalid or bogus skipList input (comments & empty lines)' '\n+test_expect_success 'fsck with other accepted skipList input (comments & empty lines)' '\n \tcat >SKIP.with-comment <<-EOF &&\n \t# Some bad commit\n \t0000000000000000000000000000000000000001\n \tEOF\n \ttest_must_fail git -c fsck.skipList=SKIP.with-comment fsck 2>err-with-comment &&\n-\ttest_i18ngrep \"^fatal: Invalid SHA-1: # Some bad commit$\" err-with-comment &&\n+\ttest_i18ngrep \"missingEmail\" err-with-comment &&\n \tcat >SKIP.with-empty-line <<-EOF &&\n \t0000000000000000000000000000000000000001\n \n \t0000000000000000000000000000000000000002\n \tEOF\n \ttest_must_fail git -c fsck.skipList=SKIP.with-empty-line fsck 2>err-with-empty-line &&\n-\ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-with-empty-line\n+\ttest_i18ngrep \"missingEmail\" err-with-empty-line\n '\n \n test_expect_success 'fsck no garbage output from comments & empty lines errors' '\n@@ -196,6 +196,19 @@ test_expect_success 'fsck with invalid abbreviated skipList input' '\n \ttest_i18ngrep \"^fatal: Invalid SHA-1: \" err-abbreviated\n '\n \n+test_expect_success 'fsck with exhaustive accepted skipList input (various types of comments etc.)' '\n+\t>SKIP.exhaustive &&\n+\techo \"# A commented line\" >>SKIP.exhaustive &&\n+\techo \"\" >>SKIP.exhaustive &&\n+\techo \" \" >>SKIP.exhaustive &&\n+\techo \" # Comment after whitespace\" >>SKIP.exhaustive &&\n+\techo \"$commit # Our bad commit (with leading whitespace and trailing comment)\" >>SKIP.exhaustive &&\n+\techo \"# Some bad commit (leading whitespace)\" >>SKIP.exhaustive &&\n+\techo \" 0000000000000000000000000000000000000001\" >>SKIP.exhaustive &&\n+\tgit -c fsck.skipList=SKIP.exhaustive fsck 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tgit push . $commit:refs/heads/bogus &&\n \trm -rf dst &&\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"},{"id":"357213","messageId":"20180903144928.30691-8-avarab@gmail.com","threadId":"49227","inReplyTo":"2b31e12e-20e9-3d08-58bd-977f8b83e0a7@web.de","subject":"[PATCH v5 07/10] fsck: add a performance test for skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-03T14:49:25Z","receivedAt":"2018-09-03T14:50:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: René Scharfe \n\nCreate a performance test to see how the skipList implementation\nperforms. First we setup N bad commits, then we see how progressively\nworking our way up to 0..N in increments of 10x does. I.e. the\nneedle(s) in the haystack get progressively more numerous.\n\nSigned-off-by: René Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/perf/p1451-fsck-skip-list.sh | 40 ++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n create mode 100755 t/perf/p1451-fsck-skip-list.sh\n\ndiff --git a/t/perf/p1451-fsck-skip-list.sh b/t/perf/p1451-fsck-skip-list.sh\nnew file mode 100755\nindex 0000000000..c2b97d2487\n--- /dev/null\n+++ b/t/perf/p1451-fsck-skip-list.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+\n+test_description='Test fsck skipList performance'\n+\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+n=1000000\n+\n+test_expect_success \"setup $n bad commits\" '\n+\tfor i in $(test_seq 1 $n)\n+\tdo\n+\t\techo \"commit refs/heads/master\" &&\n+\t\techo \"committer C 1234567890 +0000\" &&\n+\t\techo \"data <skiplist\n+\t'\n+\n+\ttest_perf \"fsck with $skip skipped bad commits\" '\n+\t\tgit -c fsck.skipList=skiplist fsck\n+\t'\n+\n+\tcase $skip in\n+\t0) skip=1 ;;\n+\t*) skip=${skip}0 ;;\n+\tesac\n+done\n+\n+test_done\n-- \n2.19.0.rc1.350.ge57e33dbd1\n\n"}]}