{"thread":{"id":"58917","subject":"[PATCH 0/4] Optionally skip hashing index on write","startedAt":"2022-12-07T17:26:20Z","lastAt":"2023-01-17T14:49:34Z","messageCount":59,"participants":["Derrick Stolee via GitGitGadget","Eric Sunshine","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Derrick Stolee","SZEDER Gábor","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"468728","messageId":"pull.1439.git.1670433958.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":null,"subject":"[PATCH 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-07T17:25:54Z","receivedAt":"2022-12-07T17:26:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Writing the index is a critical action that takes place in multiple Git\ncommands. The recent performance improvements available with the sparse\nindex show how often the I/O costs around the index can affect different Git\ncommands, although reading the index takes place more often than a write.\n\nThe index is written through the hashfile API, both as a buffered write and\nas a computation of its trailing hash. This trailing hash is an expectation\nof the file format: several optional index extensions are provided using\nindicators immediately preceding the trailing hash. 'git fsck' will complain\nif the trailing hash does not match the contents of the file up to that\npoint.\n\nHowever, computing the hash is expensive. This is even more expensive now\nthat we've moved to sha1dc instead of hardware-accelerated SHA1\ncomputations.\n\nThis series provides a new config option, index.skipHash, that allows users\nto disable computing the hash as Git writes the index. In this case, the\ntrailing hash is stored as the null hash (all bytes are zero).\n\nThe implementation is rather simple, but the patches organize different\naspects in a way that we could split things out:\n\n * Patch 1 adds a 'skip_hash' option to 'struct hashfile' that creates this\n as a general option to the hashwrite API.\n * The motivation in Patch 1 avoids the talk about the chunk-format API and\n instead focuses on the index as the first example that could make use of\n this.\n * Patch 2 creates the index.skipHash config option and updates 'git fsck'\n to ignore a null hash on the index. This is demonstrated by a very simple\n test.\n * Patch 3 creates a test helper to get the trailing hash of a file, and\n adds a concrete check on the trailing hash when index.skipHash=true.\n * Patch 4 (which could be deferred until later) adds index.skipHash=true as\n an implication of feature.manyFiles and as a setting in Scalar.\n\nThe end result is that index writes speed up significantly, enough that 'git\nupdate-index --force-write' improves by 1.75x.\n\nSimilar patches appeared in my earlier refs RFC [1].\n\n[1]\nhttps://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\n\nUpdates since RFC\n=================\n\n * The config name is now index.skipHash to make it more clear that the\n default is to not skip the hash, and choosing this option requires a true\n value.\n * Use oidread() instead of an ad-hoc loop.\n * Patches 3 and 4 are new. 3 adds some extra test verification that wasn't\n in the RFC.\n * I think that patch 4 is helpful to see now, but I could understand if we\n wanted to defer that patch until after index.skipHash has had more time\n to bake.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: allow skipping the hash function\n read-cache: add index.skipHash config option\n test-lib-functions: add helper for trailing hash\n features: feature.manyFiles implies fast index writes\n\n Documentation/config/feature.txt | 3 +++\n Documentation/config/index.txt | 8 ++++++++\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n read-cache.c | 15 ++++++++++++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 22 ++++++++++++++++++++++\n t/test-lib-functions.sh | 8 ++++++++\n 10 files changed, 77 insertions(+), 4 deletions(-)\n\n\nbase-commit: 805265fcf7a737664a8321aaf4a0587b78435184\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1439%2Fderrickstolee%2Fhashfile-skip-hash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1439/derrickstolee/hashfile-skip-hash-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1439\n-- \ngitgitgadget\n"},{"id":"468729","messageId":"5fb4b5a36ac806f3ee07a614bcb93df2c430507c.1670433958.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.git.1670433958.gitgitgadget@gmail.com","subject":"[PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-07T17:25:56Z","receivedAt":"2022-12-07T17:26:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change allowed skipping the hashing portion of the\nhashwrite API, using it instead as a buffered write API. Disabling the\nhashwrite can be particularly helpful when the write operation is in a\ncritical path.\n\nOne such critical path is the writing of the index. This operation is so\ncritical that the sparse index was created specifically to reduce the\nsize of the index to make these writes (and reads) faster.\n\nFollowing a similar approach to one used in the microsoft/git fork [1],\nadd a new config option (index.skipHash) that allows disabling this\nhashing during the index write. The cost is that we can no longer\nvalidate the contents for corruption-at-rest using the trailing hash.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nWhile older Git versions will not recognize the null hash as a special\ncase, the file format itself is still being met in terms of its\nstructure. Using this null hash will still allow Git operations to\nfunction across older versions.\n\nThe one exception is 'git fsck' which checks the hash of the index file.\nThis used to be a check on every index read, but was split out to just\nthe index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n2017-04-14).\n\nHere, we disable this check if the trailing hash is all zeroes. We add a\nwarning to the config option that this may cause undesirable behavior\nwith older Git versions.\n\nAs a quick comparison, I tested 'git update-index --force-write' with\nand without index.computeHash=false on a copy of the Linux kernel\nrepository.\n\nBenchmark 1: with hash\n Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n Range (min … max): 34.3 ms … 79.1 ms 82 runs\n\nBenchmark 2: without hash\n Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n Range (min … max): 16.3 ms … 42.0 ms 69 runs\n\nSummary\n 'without hash' ran\n 1.78 ± 0.76 times faster than 'with hash'\n\nThese performance benefits are substantial enough to allow users the\nability to opt-in to this feature, even with the potential confusion\nwith older 'git fsck' versions.\n\nIt is critical that this test is placed before the test_index_version\ntests, since those tests obliterate the .git/config file and hence lose\nthe setting from GIT_TEST_DEFAULT_HASH, if set.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/index.txt | 8 ++++++++\n read-cache.c | 14 +++++++++++++-\n t/t1600-index.sh | 8 ++++++++\n 3 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/index.txt b/Documentation/config/index.txt\nindex 75f3a2d1054..3ea0962631d 100644\n--- a/Documentation/config/index.txt\n+++ b/Documentation/config/index.txt\n@@ -30,3 +30,11 @@ index.version::\n \tSpecify the version with which new index files should be\n \tinitialized. This does not affect existing repositories.\n \tIf `feature.manyFiles` is enabled, then the default is 4.\n+\n+index.skipHash::\n+\tWhen enabled, do not compute the trailing hash for the index file.\n+\tInstead, write a trailing set of bytes with value zero, indicating\n+\tthat the computation was skipped.\n++\n+If you enable `index.skipHash`, then older Git clients may report that\n+your index is corrupt during `git fsck`.\ndiff --git a/read-cache.c b/read-cache.c\nindex 46f5e497b14..fb4d6fb6387 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tgit_hash_ctx c;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tint hdr_version;\n+\tunsigned char *start, *end;\n+\tstruct object_id oid;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(_(\"bad signature 0x%08x\"), hdr->hdr_signature);\n@@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tif (!verify_index_checksum)\n \t\treturn 0;\n \n+\tend = (unsigned char *)hdr + size;\n+\tstart = end - the_hash_algo->rawsz;\n+\toidread(&oid, start);\n+\tif (oideq(&oid, null_oid()))\n+\t\treturn 0;\n+\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n \tthe_hash_algo->final_fn(hash, &c);\n-\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n+\tif (!hasheq(hash, end - the_hash_algo->rawsz))\n \t\treturn error(_(\"bad index file sha1 signature\"));\n \treturn 0;\n }\n@@ -2915,9 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tint ieot_entries = 1;\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n+\tint skip_hash;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n+\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n+\t\tf->skip_hash = skip_hash;\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 010989f90e6..df07c587e0e 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -65,6 +65,14 @@ test_expect_success 'out of bounds index.version issues warning' '\n \t)\n '\n \n+test_expect_success 'index.skipHash config option' '\n+\t(\n+\t\trm -f .git/index &&\n+\t\tgit -c index.skipHash=true add a &&\n+\t\tgit fsck\n+\t)\n+'\n+\n test_index_version () {\n \tINDEX_VERSION_CONFIG=$1 &&\n \tFEATURE_MANY_FILES=$2 &&\n-- \ngitgitgadget\n\n"},{"id":"468730","messageId":"40ee8dbaef06f8f4265d12436455279499d7ac01.1670433958.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.git.1670433958.gitgitgadget@gmail.com","subject":"[PATCH 1/4] hashfile: allow skipping the hash function","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-07T17:25:55Z","receivedAt":"2022-12-07T17:26:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API is useful for generating files that include a trailing\nhash of the file's contents up to that point. Using such a hash is\nhelpful for verifying the file for corruption-at-rest, such as a faulty\ndrive causing flipped bits.\n\nGit's index file includes this trailing hash, so it uses a 'struct\nhashfile' to handle the I/O to the file. This was very convenient to\nallow using the hashfile methods during these operations.\n\nHowever, hashing the file contents during write comes at a performance\npenalty. It's slower to hash the bytes on their way to the disk than\nwithout that step. This problem is made worse by the replacement of\nhardware-accelerated SHA1 computations with the software-based sha1dc\ncomputation.\n\nThis write cost is significant, and the checksum capability is likely\nnot worth that cost for such a short-lived file. The index is rewritten\nfrequently and the only time the checksum is checked is during 'git\nfsck'. Thus, it would be helpful to allow a user to opt-out of the hash\ncomputation.\n\nWe first need to allow Git to opt-out of the hash computation in the\nhashfile API. The buffered writes of the API are still helpful, so it\nmakes sense to make the change here.\n\nIntroduce a new 'skip_hash' option to 'struct hashfile'. When set, the\nupdate_fn and final_fn members of the_hash_algo are skipped. When\nfinalizing the hashfile, the trailing hash is replaced with the null\nhash.\n\nThis use of a trailing null hash would be desireable in either case,\nsince we do not want to special case a file format to have a different\nlength depending on whether it was hashed or not. When the final bytes\nof a file are all zero, we can infer that it was written without\nhashing, and thus that verification is not available as a check for file\nconsistency. This also means that we could easily toggle hashing for any\nfile format we desire.\n\nA version of this patch has existed in the microsoft/git fork since\n2017 [1] (the linked commit was rebased in 2018, but the original dates\nback to January 2017). Here, the change to make the index use this fast\npath is delayed until a later change.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nCo-authored-by: Kevin Willford \nSigned-off-by: Kevin Willford \nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 59ef3398ca2..3243473c3d7 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -45,7 +45,8 @@ void hashflush(struct hashfile *f)\n \tunsigned offset = f->offset;\n \n \tif (offset) {\n-\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n+\t\tif (!f->skip_hash)\n+\t\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n \t\tflush(f, f->buffer, offset);\n \t\tf->offset = 0;\n \t}\n@@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \tint fd;\n \n \thashflush(f);\n-\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n+\tif (f->skip_hash)\n+\t\tmemset(f->buffer, 0, the_hash_algo->rawsz);\n+\telse\n+\t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & CSUM_HASH_IN_STREAM)\n@@ -108,7 +114,8 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\t * the hashfile's buffer. In this block,\n \t\t\t * f->offset is necessarily zero.\n \t\t\t */\n-\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n+\t\t\tif (!f->skip_hash)\n+\t\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n \t\t\tflush(f, buf, nr);\n \t\t} else {\n \t\t\t/*\n@@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name,\n \tf->tp = tp;\n \tf->name = name;\n \tf->do_crc = 0;\n+\tf->skip_hash = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n \n \tf->buffer_len = buffer_len;\ndiff --git a/csum-file.h b/csum-file.h\nindex 0d29f528fbc..29468067f81 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -20,6 +20,13 @@ struct hashfile {\n \tsize_t buffer_len;\n \tunsigned char *buffer;\n \tunsigned char *check_buffer;\n+\n+\t/**\n+\t * If set to 1, skip_hash indicates that we should\n+\t * not actually compute the hash for this hashfile and\n+\t * instead only use it as a buffered write.\n+\t */\n+\tunsigned int skip_hash;\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"468731","messageId":"a20bf8de8645c19a02838d70a7c2c3b00378a50d.1670433958.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.git.1670433958.gitgitgadget@gmail.com","subject":"[PATCH 3/4] test-lib-functions: add helper for trailing hash","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-07T17:25:57Z","receivedAt":"2022-12-07T17:26:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt can be helpful to check that a file format with a trailing hash has a\nspecific hash in the final bytes of a written file. This is made more\napparent by recent changes that allow skipping the hash algorithm and\nwriting a null hash at the end of the file instead.\n\nAdd a new test_trailing_hash helper and use it in t1600 to verify that\nindex.skipHash=true really does skip the hash computation, since\n'git fsck' does not actually verify the hash.\n\nKeep the 'git fsck' call to ensure that any potential future change to\ncheck the index hash does not cause an error in this case.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1600-index.sh | 3 +++\n t/test-lib-functions.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex df07c587e0e..55816756607 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -69,6 +69,9 @@ test_expect_success 'index.skipHash config option' '\n \t(\n \t\trm -f .git/index &&\n \t\tgit -c index.skipHash=true add a &&\n+\t\ttest_trailing_hash .git/index >hash &&\n+\t\techo $(test_oid zero) >expect &&\n+\t\ttest_cmp expect hash &&\n \t\tgit fsck\n \t)\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b32..e88acfdb68a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1875,3 +1875,11 @@ test_cmp_config_output () {\n \tsort config-actual >sorted-actual &&\n \ttest_cmp sorted-expect sorted-actual\n }\n+\n+# Given a filename, extract its trailing hash as a hex string\n+test_trailing_hash () {\n+\tlocal file=\"$1\" &&\n+\ttail -c $(test_oid rawsz) \"$file\" | \\\n+\t\ttest-tool hexdump | \\\n+\t\tsed \"s/ //g\"\n+}\n-- \ngitgitgadget\n\n"},{"id":"468732","messageId":"77bf5d5ff27729a39ac00d52af3c09610d733b14.1670433958.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.git.1670433958.gitgitgadget@gmail.com","subject":"[PATCH 4/4] features: feature.manyFiles implies fast index writes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-07T17:25:58Z","receivedAt":"2022-12-07T17:26:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe recent addition of the index.skipHash config option allows index\nwrites to speed up by skipping the hash computation for the trailing\nchecksum. This is particularly critical for repositories with many files\nat HEAD, so add this config option to two cases where users in that\nscenario may opt-in to such behavior:\n\n 1. The feature.manyFiles config option enables some options that are\n helpful for repositories with many files at HEAD.\n\n 2. 'scalar register' and 'scalar reconfigure' set config options that\n optimize for large repositories.\n\nIn both of these cases, set index.skipHash=true to gain this\nspeedup. Add tests that demonstrate the proper way that\nindex.skipHash=true can override feature.manyFiles=true.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/feature.txt | 3 +++\n read-cache.c | 7 ++++---\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 13 ++++++++++++-\n 6 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 95975e50912..f0e1d4cb2be 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -23,6 +23,9 @@ feature.manyFiles::\n \tworking directory. With many files, commands such as `git status` and\n \t`git checkout` may be slow and these new defaults improve performance:\n +\n+* `index.skipHash=true` speeds up index writes by not computing a trailing\n+ checksum.\n++\n * `index.version=4` enables path-prefix compression in the index.\n +\n * `core.untrackedCache=true` enables the untracked cache. This setting assumes\ndiff --git a/read-cache.c b/read-cache.c\nindex fb4d6fb6387..1844953fba7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2923,12 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tint ieot_entries = 1;\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n-\tint skip_hash;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n-\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n-\t\tf->skip_hash = skip_hash;\n+\tif (istate->repo) {\n+\t\tprepare_repo_settings(istate->repo);\n+\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n+\t}\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 3021921c53d..3dbd3f0e2ec 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -47,6 +47,7 @@ void prepare_repo_settings(struct repository *r)\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\n+\t\tr->settings.index_skip_hash = 1;\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n@@ -61,6 +62,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n+\trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 6c461c5b9de..e8c67ffe165 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -42,6 +42,7 @@ struct repo_settings {\n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \n \tint index_version;\n+\tint index_skip_hash;\n \tenum untracked_cache_setting core_untracked_cache;\n \n \tint pack_use_sparse;\ndiff --git a/scalar.c b/scalar.c\nindex 6c52243cdf1..b49bb8c24ec 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -143,6 +143,7 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"credential.validate\", \"false\", 1 }, /* GCM4W-only */\n \t\t{ \"gc.auto\", \"0\", 1 },\n \t\t{ \"gui.GCWarning\", \"false\", 1 },\n+\t\t{ \"index.skipHash\", \"false\", 1 },\n \t\t{ \"index.threads\", \"true\", 1 },\n \t\t{ \"index.version\", \"4\", 1 },\n \t\t{ \"merge.stat\", \"false\", 1 },\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 55816756607..be0a0a8a008 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -72,7 +72,18 @@ test_expect_success 'index.skipHash config option' '\n \t\ttest_trailing_hash .git/index >hash &&\n \t\techo $(test_oid zero) >expect &&\n \t\ttest_cmp expect hash &&\n-\t\tgit fsck\n+\t\tgit fsck &&\n+\n+\t\trm -f .git/index &&\n+\t\tgit -c feature.manyFiles=true add a &&\n+\t\ttest_trailing_hash .git/index >hash &&\n+\t\ttest_cmp expect hash &&\n+\n+\t\trm -f .git/index &&\n+\t\tgit -c feature.manyFiles=true \\\n+\t\t -c index.skipHash=false add a &&\n+\t\ttest_trailing_hash .git/index >hash &&\n+\t\t! test_cmp expect hash\n \t)\n '\n \n-- \ngitgitgadget\n"},{"id":"468734","messageId":"CAPig+cRSY+c-fOQBeC5ff0-s3+_kzFEjcBOSHP6C8ca9t7zr+w@mail.gmail.com","threadId":"58917","inReplyTo":"5fb4b5a36ac806f3ee07a614bcb93df2c430507c.1670433958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-07T18:59:33Z","receivedAt":"2022-12-07T18:59:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 7, 2022 at 12:27 PM Derrick Stolee via GitGitGadget\n wrote:\n> The previous change allowed skipping the hashing portion of the\n> hashwrite API, using it instead as a buffered write API. Disabling the\n> hashwrite can be particularly helpful when the write operation is in a\n> critical path.\n>\n> One such critical path is the writing of the index. This operation is so\n> critical that the sparse index was created specifically to reduce the\n> size of the index to make these writes (and reads) faster.\n>\n> Following a similar approach to one used in the microsoft/git fork [1],\n> add a new config option (index.skipHash) that allows disabling this\n> hashing during the index write. The cost is that we can no longer\n> validate the contents for corruption-at-rest using the trailing hash.\n> [...]\n> Signed-off-by: Derrick Stolee \n> ---\n> diff --git a/Documentation/config/index.txt b/Documentation/config/index.txt\n> @@ -30,3 +30,11 @@ index.version::\n> +index.skipHash::\n> + When enabled, do not compute the trailing hash for the index file.\n> + Instead, write a trailing set of bytes with value zero, indicating\n> + that the computation was skipped.\n> ++\n> +If you enable `index.skipHash`, then older Git clients may report that\n> +your index is corrupt during `git fsck`.\n\nThis documentation is rather minimal. Given this description, are\nreaders going to understand the purpose of the option, when they\nshould use it, what the impact will be, when and why they should avoid\nit, etc.?\n\n> diff --git a/t/t1600-index.sh b/t/t1600-index.sh\n> @@ -65,6 +65,14 @@ test_expect_success 'out of bounds index.version issues warning' '\n> +test_expect_success 'index.skipHash config option' '\n> + (\n> + rm -f .git/index &&\n> + git -c index.skipHash=true add a &&\n> + git fsck\n> + )\n> +'\n\nWhat is the purpose of the subshell here?\n"},{"id":"468738","messageId":"221207.868rjiam86.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"40ee8dbaef06f8f4265d12436455279499d7ac01.1670433958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] hashfile: allow skipping the hash function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T22:13:15Z","receivedAt":"2022-12-07T22:21:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> [...]\n> However, hashing the file contents during write comes at a performance\n> penalty. It's slower to hash the bytes on their way to the disk than\n> without that step. This problem is made worse by the replacement of\n> hardware-accelerated SHA1 computations with the software-based sha1dc\n> computation.\n\nMore on that lack of HW accel later...\n\n> This write cost is significant\n\nDon't you mean hashing cost, or do we also do additional writes if we do\nthe hashing?\n\n> , and the checksum capability is likely\n> not worth that cost for such a short-lived file. The index is rewritten\n> frequently and the only time the checksum is checked is during 'git\n> fsck'. Thus, it would be helpful to allow a user to opt-out of the hash\n> computation.\n\nI didn't know that, and had assumed that we at least checked it on the\nfull read (and I found this bit of the commit message after writing the\nlast paragraphs here at the end, so maybe skipping this is fine...).\n\n> [...]\n> @@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n> \tint fd;\n> \n> \thashflush(f);\n> -\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n> +\n> +\tif (f->skip_hash)\n> +\t\tmemset(f->buffer, 0, the_hash_algo->rawsz);\n\nHere you're hardcoding a new version of null_oid(), but we can use it\ninstead. Perhaps:\n\t\n\tdiff --git a/csum-file.c b/csum-file.c\n\tindex 3243473c3d7..b54c4f66cbb 100644\n\t--- a/csum-file.c\n\t+++ b/csum-file.c\n\t@@ -63,11 +63,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n\t \t\t enum fsync_component component, unsigned int flags)\n\t {\n\t \tint fd;\n\t+\tconst struct object_id *const noid = null_oid();\n\t \n\t \thashflush(f);\n\t \n\t \tif (f->skip_hash)\n\t-\t\tmemset(f->buffer, 0, the_hash_algo->rawsz);\n\t+\t\tmemcpy(f->buffer, noid, sizeof(*noid));\n\t \telse\n\t \t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n\t \n> @@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name,\n> \tf->tp = tp;\n> \tf->name = name;\n> \tf->do_crc = 0;\n> +\tf->skip_hash = 0;\n> \tthe_hash_algo->init_fn(&f->ctx);\n> \n> \tf->buffer_len = buffer_len;\n\nI think I pointed out in the RFC that we'd be much faster with\nnon-sha1collisiondetection, and that maybe this would get us partway to\nthe performance you desired (or maybe we'd think that was a more\nacceptable trade-off, as it didn't make the format\nbackwards-incompatible).\n\nBut just from seeing \"do_crc\" here in the context, did you benchmark it\nagainst that? How does it perform?\n\nThere's no place to put that in the index, but we *could* encode it in\nthe hash, just with a lot of leading zeros.\n\nMaybe that would give us some/most of the performance benefits, with the\nbenefit of a checksum?\n\nOr maybe not, but I think it's worth exploring & supporting a different\n& faster SHA-1 implementation before making (even opt-in) backwards\nincompatible format changes for performance reasons, and if even that's\ntoo slow maybe crc32 would be sufficient (but not compatible), but still\nsafer?\n"},{"id":"468741","messageId":"221207.864ju6alyg.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"5fb4b5a36ac806f3ee07a614bcb93df2c430507c.1670433958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T22:25:06Z","receivedAt":"2022-12-07T22:27:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> [...]\n> diff --git a/read-cache.c b/read-cache.c\n> index 46f5e497b14..fb4d6fb6387 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n> \tgit_hash_ctx c;\n> \tunsigned char hash[GIT_MAX_RAWSZ];\n> \tint hdr_version;\n> +\tunsigned char *start, *end;\n> +\tstruct object_id oid;\n> \n> \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n> \t\treturn error(_(\"bad signature 0x%08x\"), hdr->hdr_signature);\n> @@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n> \tif (!verify_index_checksum)\n> \t\treturn 0;\n> \n> +\tend = (unsigned char *)hdr + size;\n> +\tstart = end - the_hash_algo->rawsz;\n> +\toidread(&oid, start);\n> +\tif (oideq(&oid, null_oid()))\n> +\t\treturn 0;\n\nIt's good to see this use the existing hashing support, as I suggested\nin the RFC comments. Glad it helped.\n\n> \tint ieot_entries = 1;\n> \tstruct index_entry_offset_table *ieot = NULL;\n> \tint nr, nr_threads;\n> +\tint skip_hash;\n\nYou don't need this variable.\n> \n> \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n> \n> +\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n> +\t\tf->skip_hash = skip_hash;\n\nBecause this can just be:\n\n\tgit_config_get_maybe_bool(\"index.skiphash\", &f->skip_hash);\n\nI.e. the config API guarantees that it won't touch the variable if the\nkey doesn't exist, so no need for the intermediate variable.\n\nIn a later commit you convert this to that very API use when moving this\nto the repo-settings.\n\nCan we maybe skip straight to that step?\n\n> +test_expect_success 'index.skipHash config option' '\n> +\t(\n> +\t\trm -f .git/index &&\n> +\t\tgit -c index.skipHash=true add a &&\n> +\t\tgit fsck\n> +\t)\n\nWhy the subshell?\n"},{"id":"468742","messageId":"221207.86zgby97bk.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"a20bf8de8645c19a02838d70a7c2c3b00378a50d.1670433958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] test-lib-functions: add helper for trailing hash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T22:27:45Z","receivedAt":"2022-12-07T22:29:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> [...]\n> +# Given a filename, extract its trailing hash as a hex string\n> +test_trailing_hash () {\n> +\tlocal file=\"$1\" &&\n> +\ttail -c $(test_oid rawsz) \"$file\" | \\\n> +\t\ttest-tool hexdump | \\\n\nYou don't need the \"\\\"'s here.\n\n> +\t\tsed \"s/ //g\"\n> +}\n\nAt the end of this series we have one test file using this\ntest_trailing_hash, can't we just add it to t1600-index.sh?\n"},{"id":"468743","messageId":"221207.86v8mm972z.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"77bf5d5ff27729a39ac00d52af3c09610d733b14.1670433958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T22:30:04Z","receivedAt":"2022-12-07T22:34:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> [...]\n> diff --git a/read-cache.c b/read-cache.c\n> index fb4d6fb6387..1844953fba7 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2923,12 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> \tint ieot_entries = 1;\n> \tstruct index_entry_offset_table *ieot = NULL;\n> \tint nr, nr_threads;\n> -\tint skip_hash;\n> \n> \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n> \n> -\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n> -\t\tf->skip_hash = skip_hash;\n> +\tif (istate->repo) {\n> +\t\tprepare_repo_settings(istate->repo);\n> +\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n> +\t}\n\nUrm, are we ever going to find ourselves in a situation where:\n\n * We have read the settings for the_repository\n * We have an index we're about to write out as our \"main index\", but\n the istate->repo *isn't* the_repository.\n * Even then, wouldn't the two copies of the repos have read the same\n repo settings?\n\nBut maybe there's a really obvious submodule / worktree / whatever edge\ncase I'm missing.\n\nBut if not, shouldn't we just always read/write this from\nthe_repository?\n\n> +\t\trm -f .git/index &&\n> +\t\tgit -c feature.manyFiles=true \\\n> +\t\t -c index.skipHash=false add a &&\n> +\t\ttest_trailing_hash .git/index >hash &&\n> +\t\t! test_cmp expect hash\n\nWe had a parallel thread where we discussed \"! test_cmp\" being an\nanti-pattern, i.e. you want them not to be the same, but you want it to\nstill show a diff, Maybe just \"! cmp\" ?\n\nI.e. either the diff will be meaningless, or we really should be\nasserting the actual value we want, not what it shouldn't be.\n\nso in this case, shouldn't we assert that it's the 0000... value, or the\nactual hash (depending on which way around we're testing this)?\n"},{"id":"468749","messageId":"xmqqilim6bhh.fsf@gitster.g","threadId":"58917","inReplyTo":"pull.1439.git.1670433958.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] Optionally skip hashing index on write","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T23:27:22Z","receivedAt":"2022-12-07T23:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Writing the index is a critical action that takes place in multiple Git\n> commands. The recent performance improvements available with the sparse\n> index show how often the I/O costs around the index can affect different Git\n> commands, although reading the index takes place more often than a write.\n\nThe sparse-index work is great in that it offers correctness while\ntaking advantage of the knowledge of which part of the tree is\nquiescent and unused to boost performance. I am not sure a change\nto reduce file safety can be compared with it, in that one is pure\nimprovement, while the other is trade-off.\n\nAs long as we will keep the \"create into a new file, write it fully\nand fsync + rename to the final\" pattern, we do not need the trailing\nchecksum to protect us from a truncated output due to index-writing\nprocess dying in the middle, so I do not mind that trade-off, though.\n\nProtecting files from bit flipping filesystem corruption is a\ndifferent matter. Folks at hosting sites like GitHub would know how\noften they detect object corruption (I presume they do not have to\ndeal with the index file on the server end that often, but loose and\npack object files have the trailing checksums the same way) thanks\nto the trailing checksum, and what the consequences are if we lost\nthat safety (I am guessing it would be minimum, though).\n\nThanks.\n"},{"id":"468750","messageId":"221208.86mt7y946t.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"5fb4b5a36ac806f3ee07a614bcb93df2c430507c.1670433958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T23:06:49Z","receivedAt":"2022-12-07T23:37:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> [...]\n> While older Git versions will not recognize the null hash as a special\n> case, the file format itself is still being met in terms of its\n> structure. Using this null hash will still allow Git operations to\n> function across older versions.\n\nThat's good news, but...\n\n> The one exception is 'git fsck' which checks the hash of the index file.\n> This used to be a check on every index read, but was split out to just\n> the index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n> 2017-04-14).\n\n...uh, what?\n\nIs there an implied claim here that versions before v2.13.0 don't count\nas \"older versions\"?\n\nI.e. doesn't v2.12.0 hard fail the verification for all index writing?\nIt's only after v2.13.0 that we do it only for the fsck.\n\nThat seems like a rather significant caveat that we should be noting\nprominently in the docs added in 4/4.\n\n> As a quick comparison, I tested 'git update-index --force-write' with\n> and without index.computeHash=false on a copy of the Linux kernel\n> repository.\n\nIt took me a bit to see why I was failing to reproduce this, before\nfinding that it's because you mention index.computeHash here, but it's\nindex.skipHash now.\n>\n> Benchmark 1: with hash\n> Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n> Range (min … max): 34.3 ms … 79.1 ms 82 runs\n>\n> Benchmark 2: without hash\n> Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n> Range (min … max): 16.3 ms … 42.0 ms 69 runs\n>\n> Summary\n> 'without hash' ran\n> 1.78 ± 0.76 times faster than 'with hash'\n\nI suggested in\nhttps://lore.kernel.org/git/221207.868rjiam86.gmgdl@evledraar.gmail.com/\nearlier to benchmark this against not-sha1collisiondetection.\n\nI don't think I get HW-accelerated SHA-1 on that box with OPENSSL (how\ndo I check...?). The results on my main development box are:\n\t\n\t$ hyperfine -L g sha1dc,openssl -L v false,true -n '{g} with index.skipHash={v}' './git.{g} -c index.skipHash={v} -C /dev/shm/linux-mem update-index --force-write' -w 5 -r 10\n\tBenchmark 1: sha1dc with index.skipHash=false\n\t Time (mean ± σ): 37.0 ms ± 2.3 ms [User: 30.8 ms, System: 6.0 ms]\n\t Range (min … max): 35.1 ms … 41.4 ms 10 runs\n\t \n\tBenchmark 2: openssl with index.skipHash=false\n\t Time (mean ± σ): 21.5 ms ± 0.4 ms [User: 15.0 ms, System: 6.3 ms]\n\t Range (min … max): 20.7 ms … 22.0 ms 10 runs\n\t \n\tBenchmark 3: sha1dc with index.skipHash=true\n\t Time (mean ± σ): 13.5 ms ± 0.4 ms [User: 7.9 ms, System: 5.4 ms]\n\t Range (min … max): 13.0 ms … 14.2 ms 10 runs\n\t \n\tBenchmark 4: openssl with index.skipHash=true\n\t Time (mean ± σ): 14.2 ms ± 0.3 ms [User: 9.5 ms, System: 4.6 ms]\n\t Range (min … max): 13.6 ms … 14.6 ms 10 runs\n\t \n\tSummary\n\t 'sha1dc with index.skipHash=true' ran\n\t 1.05 ± 0.04 times faster than 'openssl with index.skipHash=true'\n\t 1.60 ± 0.05 times faster than 'openssl with index.skipHash=false'\n\t 2.75 ± 0.19 times faster than 'sha1dc with index.skipHash=false'\n\nSo, curiously it's proportionally much slower for me with the hash\nchecking, and skipping it with openssl is on the order of the results\nyou see.\n"},{"id":"468751","messageId":"221208.86ilim93ll.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"xmqqilim6bhh.fsf@gitster.g","subject":"Re: [PATCH 0/4] Optionally skip hashing index on write","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T23:42:30Z","receivedAt":"2022-12-07T23:49:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 08 2022, Junio C Hamano wrote:\n\n> Protecting files from bit flipping filesystem corruption is a\n> different matter. Folks at hosting sites like GitHub would know how\n> often they detect object corruption (I presume they do not have to\n> deal with the index file on the server end that often, but loose and\n> pack object files have the trailing checksums the same way) thanks\n> to the trailing checksum, and what the consequences are if we lost\n> that safety (I am guessing it would be minimum, though).\n\nI don't think this checksum does much for us in practice, but just on\nthis point in general: Extrapolating results at when it\ncomes to making general decisions about git's data safety isn't a good\nidea.\n\nI don't know about GitHub's hardware, but servers almost universally use\nECC ram, and tend to use things like error-correcting filesystem RAID\netc.\n\nData in that area is really interesting when it comes to running git in\nthat sort of setup, but it really shouldn't be extrapolated to git's\nuserbase in general.\n\nA lot of those users will be using cheap memory and/or storage devices\nwithout any error correction.\n\nThey're also likely to stress our reliability guarantees in other ways,\ne.g. yanking their power cord (or equivalent), which a server typically\nwon't need to deal with.\n"},{"id":"468757","messageId":"xmqqr0xa4v65.fsf@gitster.g","threadId":"58917","inReplyTo":"221208.86mt7y946t.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-08T00:05:06Z","receivedAt":"2022-12-08T00:05:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Is there an implied claim here that versions before v2.13.0 don't count\n> as \"older versions\"?\n>\n> I.e. doesn't v2.12.0 hard fail the verification for all index writing?\n> It's only after v2.13.0 that we do it only for the fsck.\n>\n> That seems like a rather significant caveat that we should be noting\n> prominently in the docs added in 4/4.\n\nTrue enough.\n\nIt seems that we only did security releases from 2.30.x track and\nupwards for the past year or so, and anything older may not matter\nanymore. Documenting it should be sufficient.\n\nI actually was wondering what impact it would have if we made this\nchange unconditionally.\n\nThanks.\n"},{"id":"468772","messageId":"Y5GTJuM3jNnStvsQ@coredump.intra.peff.net","threadId":"58917","inReplyTo":"221207.868rjiam86.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] hashfile: allow skipping the hash function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-08T07:32:54Z","receivedAt":"2022-12-08T07:33:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 07, 2022 at 11:13:15PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > +\tif (f->skip_hash)\n> > +\t\tmemset(f->buffer, 0, the_hash_algo->rawsz);\n> \n> Here you're hardcoding a new version of null_oid(), but we can use it\n> instead. Perhaps:\n> \t\n> \tdiff --git a/csum-file.c b/csum-file.c\n> \tindex 3243473c3d7..b54c4f66cbb 100644\n> \t--- a/csum-file.c\n> \t+++ b/csum-file.c\n> \t@@ -63,11 +63,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n> \t \t\t enum fsync_component component, unsigned int flags)\n> \t {\n> \t \tint fd;\n> \t+\tconst struct object_id *const noid = null_oid();\n> \t \n> \t \thashflush(f);\n> \t \n> \t \tif (f->skip_hash)\n> \t-\t\tmemset(f->buffer, 0, the_hash_algo->rawsz);\n> \t+\t\tmemcpy(f->buffer, noid, sizeof(*noid));\n> \t \telse\n> \t \t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n\nI don't think that's quite right; the object_id struct has other stuff\nin it. You'd probably want:\n\n hashcpy(f->buffer, null_oid());\n\nBut I think we already have a helper to just do it directly:\n\n hashclr(f->buffer);\n\n-Peff\n"},{"id":"468794","messageId":"3a47b7ac-25dc-5f70-9ab4-08c781df4cda@github.com","threadId":"58917","inReplyTo":"xmqqilim6bhh.fsf@gitster.g","subject":"Re: [PATCH 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-08T16:38:39Z","receivedAt":"2022-12-08T16:39:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/7/2022 6:27 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> Writing the index is a critical action that takes place in multiple Git\n>> commands. The recent performance improvements available with the sparse\n>> index show how often the I/O costs around the index can affect different Git\n>> commands, although reading the index takes place more often than a write.\n> \n> The sparse-index work is great in that it offers correctness while\n> taking advantage of the knowledge of which part of the tree is\n> quiescent and unused to boost performance. I am not sure a change\n> to reduce file safety can be compared with it, in that one is pure\n> improvement, while the other is trade-off.\n\nI agree that this is a trade-off, and we should both be careful about\nwhether or not we even make this a possibility for certain file\nformats. The index is an interesting case for a couple reasons:\n\n1. Writes block users. Writing the index takes place in many user-\n blocking foreground operations. The speed improvement directly\n impacts their use. Other file formats are typically written in\n the background (commit-graph, multi-pack-index) or are super-\n critical to correctness (pack-files).\n\n2. Index files are short lived. It is rare that a user leaves an\n index for a long time with many staged changes. That's the condition\n that's required for losing an index file to cause a loss of work\n (or maybe I'm missing something). Outside of staged changes, the\n index can be completely destroyed and rewritten with minimal impact\n to the user.\n\n> As long as we will keep the \"create into a new file, write it fully\n> and fsync + rename to the final\" pattern, we do not need the trailing\n> checksum to protect us from a truncated output due to index-writing\n> process dying in the middle, so I do not mind that trade-off, though.\n> \n> Protecting files from bit flipping filesystem corruption is a\n> different matter. Folks at hosting sites like GitHub would know how\n> often they detect object corruption (I presume they do not have to\n> deal with the index file on the server end that often, but loose and\n> pack object files have the trailing checksums the same way) thanks\n> to the trailing checksum, and what the consequences are if we lost\n> that safety (I am guessing it would be minimum, though).\n\nI agree that we need to be careful about which files get this\ntreatement.\n\nBut I also want to point out that I'm not using hosting servers as\nevidence that this has worked in practice, but instead many developer\nmachines in large monorepos who have had this enabled (via the\nmicrosoft/git fork) for years. We've not come across an instance where\nthis loss of a trailing hash has been an issue.\n\nThanks,\n-Stolee\n"},{"id":"468891","messageId":"37efcfcd-fea1-c1d7-65f7-ae5f2d2a12e6@github.com","threadId":"58917","inReplyTo":"CAPig+cRSY+c-fOQBeC5ff0-s3+_kzFEjcBOSHP6C8ca9t7zr+w@mail.gmail.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-12T13:59:41Z","receivedAt":"2022-12-12T13:59:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/7/2022 1:59 PM, Eric Sunshine wrote:\n> On Wed, Dec 7, 2022 at 12:27 PM Derrick Stolee via GitGitGadget\n> wrote:\n>> +If you enable `index.skipHash`, then older Git clients may report that\n>> +your index is corrupt during `git fsck`.\n> \n> This documentation is rather minimal. Given this description, are\n> readers going to understand the purpose of the option, when they\n> should use it, what the impact will be, when and why they should avoid\n> it, etc.?\n\nI will expand this with explicit version numbers for older Git versions.\n\n>> diff --git a/t/t1600-index.sh b/t/t1600-index.sh\n>> @@ -65,6 +65,14 @@ test_expect_success 'out of bounds index.version issues warning' '\n>> +test_expect_success 'index.skipHash config option' '\n>> + (\n>> + rm -f .git/index &&\n>> + git -c index.skipHash=true add a &&\n>> + git fsck\n>> + )\n>> +'\n> \n> What is the purpose of the subshell here?\n\nI was matching the style of the nearby tests, but they are all\nmodifying GIT_INDEX_VERSION, which isn't necessary here.\n\nThanks,\n-Stolee\n"},{"id":"468892","messageId":"5301dfa1-71c9-1ff4-6ba0-e37f25e8ef62@github.com","threadId":"58917","inReplyTo":"221208.86mt7y946t.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-12T14:05:11Z","receivedAt":"2022-12-12T14:05:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/7/2022 6:06 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>> [...]\n>> While older Git versions will not recognize the null hash as a special\n>> case, the file format itself is still being met in terms of its\n>> structure. Using this null hash will still allow Git operations to\n>> function across older versions.\n> \n> That's good news, but...\n> \n>> The one exception is 'git fsck' which checks the hash of the index file.\n>> This used to be a check on every index read, but was split out to just\n>> the index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n>> 2017-04-14).\n> \n> ...uh, what?\n> \n> Is there an implied claim here that versions before v2.13.0 don't count\n> as \"older versions\"?\n> \n> I.e. doesn't v2.12.0 hard fail the verification for all index writing?\n> It's only after v2.13.0 that we do it only for the fsck.\n> \n> That seems like a rather significant caveat that we should be noting\n> prominently in the docs added in 4/4.\n\nI can add those details.\n \n>> As a quick comparison, I tested 'git update-index --force-write' with\n>> and without index.computeHash=false on a copy of the Linux kernel\n>> repository.\n> \n> It took me a bit to see why I was failing to reproduce this, before\n> finding that it's because you mention index.computeHash here, but it's\n> index.skipHash now.\n>>\n>> Benchmark 1: with hash\n>> Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n>> Range (min … max): 34.3 ms … 79.1 ms 82 runs\n>>\n>> Benchmark 2: without hash\n>> Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n>> Range (min … max): 16.3 ms … 42.0 ms 69 runs\n>>\n>> Summary\n>> 'without hash' ran\n>> 1.78 ± 0.76 times faster than 'with hash'\n> \n> I suggested in\n> https://lore.kernel.org/git/221207.868rjiam86.gmgdl@evledraar.gmail.com/\n> earlier to benchmark this against not-sha1collisiondetection.\n\nGenerally, I'm avoiding that benchmark because sha1dc is here to stay.\n\nIf users want to go through the trouble of compiling to use the non-dc\nversion, then I would expect the difference to be less noticeable, but\nstill significant. However, I would strongly avoid considering compiling\nboth into the client by default, letting certain paths use sha1dc and\nothers using non-dc. Certain secure environments currently only use Git\nunder exceptions that allow SHA1 for \"non-cryptographic\" reasons, but\nalso with the understanding that sha1dc is used as a safety measure.\nAdding the non-dc version back in would put that understanding at risk.\n\nThanks,\n-Stolee\n"},{"id":"468893","messageId":"95b8bca3-51fc-7cbc-aa06-5de8b01e7021@github.com","threadId":"58917","inReplyTo":"221207.86zgby97bk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/4] test-lib-functions: add helper for trailing hash","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-12T14:10:12Z","receivedAt":"2022-12-12T14:10:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/7/2022 5:27 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>> [...]\n>> +# Given a filename, extract its trailing hash as a hex string\n>> +test_trailing_hash () {\n>> +\tlocal file=\"$1\" &&\n>> +\ttail -c $(test_oid rawsz) \"$file\" | \\\n>> +\t\ttest-tool hexdump | \\\n> \n> You don't need the \"\\\"'s here.\n\nThanks.\n \n>> +\t\tsed \"s/ //g\"\n>> +}\n> \n> At the end of this series we have one test file using this\n> test_trailing_hash, can't we just add it to t1600-index.sh?\n\nI imagine that it would be helpful to access the trailing hash\nfor testing other file formats in the future. It certainly\ncould have helped the incremental commit-graph work, so we\ncould check that the filenames do match the trailing hashes.\n\nThanks,\n-Stolee\n"},{"id":"468894","messageId":"1b2aa60c-633f-0a7f-e8e3-5d0f01ff94b6@github.com","threadId":"58917","inReplyTo":"221207.86v8mm972z.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-12T14:18:40Z","receivedAt":"2022-12-12T14:18:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/7/2022 5:30 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>> [...]\n>> diff --git a/read-cache.c b/read-cache.c\n>> index fb4d6fb6387..1844953fba7 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -2923,12 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>> \tint ieot_entries = 1;\n>> \tstruct index_entry_offset_table *ieot = NULL;\n>> \tint nr, nr_threads;\n>> -\tint skip_hash;\n>> \n>> \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n>> \n>> -\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n>> -\t\tf->skip_hash = skip_hash;\n>> +\tif (istate->repo) {\n>> +\t\tprepare_repo_settings(istate->repo);\n>> +\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n>> +\t}\n> \n> Urm, are we ever going to find ourselves in a situation where:\n> \n> * We have read the settings for the_repository\n> * We have an index we're about to write out as our \"main index\", but\n> the istate->repo *isn't* the_repository.\n> * Even then, wouldn't the two copies of the repos have read the same\n> repo settings?\n> \n> But maybe there's a really obvious submodule / worktree / whatever edge\n> case I'm missing.\n> \n> But if not, shouldn't we just always read/write this from\n> the_repository?\n\nI don't understand your concern. We call prepare_repo_settings(istate->repo)\njust before using these settings, so we are using whatever repository-local\nconfig we have available to us.\n\nIf you're thinking that we could be writing an index but istate->repo is\nsomehow the \"wrong\" repo, then that is a larger problem. This patch is\ndoing the best thing it can with the information it is given.\n\n>> +\t\trm -f .git/index &&\n>> +\t\tgit -c feature.manyFiles=true \\\n>> +\t\t -c index.skipHash=false add a &&\n>> +\t\ttest_trailing_hash .git/index >hash &&\n>> +\t\t! test_cmp expect hash\n> \n> We had a parallel thread where we discussed \"! test_cmp\" being an\n> anti-pattern, i.e. you want them not to be the same, but you want it to\n> still show a diff, Maybe just \"! cmp\" ?\n\nI couldn't tell from this sentence whether test_cmp or cmp would show\nthe diff, but from testing I see that test_cmp shows the diff (for\ndebugging purposes, I'm sure) while cmp shows the position of the first\ndifference.\n\n\"! cmp\" would work here, since we don't care about what the real hash is.\n \n> I.e. either the diff will be meaningless, or we really should be\n> asserting the actual value we want, not what it shouldn't be.\n> \n> so in this case, shouldn't we assert that it's the 0000... value, or the\n> actual hash (depending on which way around we're testing this)?\n\nWhen it should be the null hash, we assert that it is that value.\n\nWhen it isn't, we do not assert the exact hash because we do not want\nother modifications to the index (or surrounding tests) to cause that\nhash to change, causing toil for future contributors. \"! cmp\" suffices\nfor this case to show that the config inheritance is working correctly.\n\nThanks,\n-Stolee\n"},{"id":"468899","messageId":"b582d681581a356534ef018c3a12004b2d9779a7.1670862677.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v2.git.1670862677.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] hashfile: allow skipping the hash function","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T16:31:14Z","receivedAt":"2022-12-12T16:31:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API is useful for generating files that include a trailing\nhash of the file's contents up to that point. Using such a hash is\nhelpful for verifying the file for corruption-at-rest, such as a faulty\ndrive causing flipped bits.\n\nGit's index file includes this trailing hash, so it uses a 'struct\nhashfile' to handle the I/O to the file. This was very convenient to\nallow using the hashfile methods during these operations.\n\nHowever, hashing the file contents during write comes at a performance\npenalty. It's slower to hash the bytes on their way to the disk than\nwithout that step. This problem is made worse by the replacement of\nhardware-accelerated SHA1 computations with the software-based sha1dc\ncomputation.\n\nThis write cost is significant, and the checksum capability is likely\nnot worth that cost for such a short-lived file. The index is rewritten\nfrequently and the only time the checksum is checked is during 'git\nfsck'. Thus, it would be helpful to allow a user to opt-out of the hash\ncomputation.\n\nWe first need to allow Git to opt-out of the hash computation in the\nhashfile API. The buffered writes of the API are still helpful, so it\nmakes sense to make the change here.\n\nIntroduce a new 'skip_hash' option to 'struct hashfile'. When set, the\nupdate_fn and final_fn members of the_hash_algo are skipped. When\nfinalizing the hashfile, the trailing hash is replaced with the null\nhash.\n\nThis use of a trailing null hash would be desireable in either case,\nsince we do not want to special case a file format to have a different\nlength depending on whether it was hashed or not. When the final bytes\nof a file are all zero, we can infer that it was written without\nhashing, and thus that verification is not available as a check for file\nconsistency. This also means that we could easily toggle hashing for any\nfile format we desire.\n\nA version of this patch has existed in the microsoft/git fork since\n2017 [1] (the linked commit was rebased in 2018, but the original dates\nback to January 2017). Here, the change to make the index use this fast\npath is delayed until a later change.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nCo-authored-by: Kevin Willford \nSigned-off-by: Kevin Willford \nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 59ef3398ca2..cce13c0f047 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -45,7 +45,8 @@ void hashflush(struct hashfile *f)\n \tunsigned offset = f->offset;\n \n \tif (offset) {\n-\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n+\t\tif (!f->skip_hash)\n+\t\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n \t\tflush(f, f->buffer, offset);\n \t\tf->offset = 0;\n \t}\n@@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \tint fd;\n \n \thashflush(f);\n-\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n+\tif (f->skip_hash)\n+\t\thashclr(f->buffer);\n+\telse\n+\t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & CSUM_HASH_IN_STREAM)\n@@ -108,7 +114,8 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\t * the hashfile's buffer. In this block,\n \t\t\t * f->offset is necessarily zero.\n \t\t\t */\n-\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n+\t\t\tif (!f->skip_hash)\n+\t\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n \t\t\tflush(f, buf, nr);\n \t\t} else {\n \t\t\t/*\n@@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name,\n \tf->tp = tp;\n \tf->name = name;\n \tf->do_crc = 0;\n+\tf->skip_hash = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n \n \tf->buffer_len = buffer_len;\ndiff --git a/csum-file.h b/csum-file.h\nindex 0d29f528fbc..29468067f81 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -20,6 +20,13 @@ struct hashfile {\n \tsize_t buffer_len;\n \tunsigned char *buffer;\n \tunsigned char *check_buffer;\n+\n+\t/**\n+\t * If set to 1, skip_hash indicates that we should\n+\t * not actually compute the hash for this hashfile and\n+\t * instead only use it as a buffered write.\n+\t */\n+\tunsigned int skip_hash;\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"468900","messageId":"e640dff53dd3f0aa48bd40fcf0e261292bbded66.1670862677.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v2.git.1670862677.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] features: feature.manyFiles implies fast index writes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T16:31:17Z","receivedAt":"2022-12-12T16:31:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe recent addition of the index.skipHash config option allows index\nwrites to speed up by skipping the hash computation for the trailing\nchecksum. This is particularly critical for repositories with many files\nat HEAD, so add this config option to two cases where users in that\nscenario may opt-in to such behavior:\n\n 1. The feature.manyFiles config option enables some options that are\n helpful for repositories with many files at HEAD.\n\n 2. 'scalar register' and 'scalar reconfigure' set config options that\n optimize for large repositories.\n\nIn both of these cases, set index.skipHash=true to gain this\nspeedup. Add tests that demonstrate the proper way that\nindex.skipHash=true can override feature.manyFiles=true.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/feature.txt | 5 +++++\n read-cache.c | 5 ++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 13 ++++++++++++-\n 6 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 95975e50912..e52bc6b8584 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -23,6 +23,11 @@ feature.manyFiles::\n \tworking directory. With many files, commands such as `git status` and\n \t`git checkout` may be slow and these new defaults improve performance:\n +\n+* `index.skipHash=true` speeds up index writes by not computing a trailing\n+ checksum. Note that this will cause Git versions earlier than 2.13.0 to\n+ refuse to parse the index and Git versions earlier than 2.40.0 will report\n+ a corrupted index during `git fsck`.\n++\n * `index.version=4` enables path-prefix compression in the index.\n +\n * `core.untrackedCache=true` enables the untracked cache. This setting assumes\ndiff --git a/read-cache.c b/read-cache.c\nindex 3f7de8b2e20..1844953fba7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2926,7 +2926,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n-\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n+\tif (istate->repo) {\n+\t\tprepare_repo_settings(istate->repo);\n+\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n+\t}\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 3021921c53d..3dbd3f0e2ec 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -47,6 +47,7 @@ void prepare_repo_settings(struct repository *r)\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\n+\t\tr->settings.index_skip_hash = 1;\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n@@ -61,6 +62,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n+\trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 6c461c5b9de..e8c67ffe165 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -42,6 +42,7 @@ struct repo_settings {\n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \n \tint index_version;\n+\tint index_skip_hash;\n \tenum untracked_cache_setting core_untracked_cache;\n \n \tint pack_use_sparse;\ndiff --git a/scalar.c b/scalar.c\nindex 6c52243cdf1..b49bb8c24ec 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -143,6 +143,7 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"credential.validate\", \"false\", 1 }, /* GCM4W-only */\n \t\t{ \"gc.auto\", \"0\", 1 },\n \t\t{ \"gui.GCWarning\", \"false\", 1 },\n+\t\t{ \"index.skipHash\", \"false\", 1 },\n \t\t{ \"index.threads\", \"true\", 1 },\n \t\t{ \"index.version\", \"4\", 1 },\n \t\t{ \"merge.stat\", \"false\", 1 },\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 55914bc3506..103743a1c7d 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -71,7 +71,18 @@ test_expect_success 'index.skipHash config option' '\n \ttest_trailing_hash .git/index >hash &&\n \techo $(test_oid zero) >expect &&\n \ttest_cmp expect hash &&\n-\tgit fsck\n+\tgit fsck &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\ttest_cmp expect hash &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true \\\n+\t\t-c index.skipHash=false add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\t! cmp expect hash\n '\n \n test_index_version () {\n-- \ngitgitgadget\n"},{"id":"468901","messageId":"pull.1439.v2.git.1670862677.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.git.1670433958.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T16:31:13Z","receivedAt":"2022-12-12T16:31:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Writing the index is a critical action that takes place in multiple Git\ncommands. The recent performance improvements available with the sparse\nindex show how often the I/O costs around the index can affect different Git\ncommands, although reading the index takes place more often than a write.\n\nThe index is written through the hashfile API, both as a buffered write and\nas a computation of its trailing hash. This trailing hash is an expectation\nof the file format: several optional index extensions are provided using\nindicators immediately preceding the trailing hash. 'git fsck' will complain\nif the trailing hash does not match the contents of the file up to that\npoint.\n\nHowever, computing the hash is expensive. This is even more expensive now\nthat we've moved to sha1dc instead of hardware-accelerated SHA1\ncomputations.\n\nThis series provides a new config option, index.skipHash, that allows users\nto disable computing the hash as Git writes the index. In this case, the\ntrailing hash is stored as the null hash (all bytes are zero).\n\nThe implementation is rather simple, but the patches organize different\naspects in a way that we could split things out:\n\n * Patch 1 adds a 'skip_hash' option to 'struct hashfile' that creates this\n as a general option to the hashwrite API.\n * The motivation in Patch 1 avoids the talk about the chunk-format API and\n instead focuses on the index as the first example that could make use of\n this.\n * Patch 2 creates the index.skipHash config option and updates 'git fsck'\n to ignore a null hash on the index. This is demonstrated by a very simple\n test.\n * Patch 3 creates a test helper to get the trailing hash of a file, and\n adds a concrete check on the trailing hash when index.skipHash=true.\n * Patch 4 (which could be deferred until later) adds index.skipHash=true as\n an implication of feature.manyFiles and as a setting in Scalar.\n\nThe end result is that index writes speed up significantly, enough that 'git\nupdate-index --force-write' improves by 1.75x.\n\nSimilar patches appeared in my earlier refs RFC [1].\n\n[1]\nhttps://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\n\nUpdates since v1\n================\n\n * In Patch 1, use hashcler() instead of memset().\n * In Patches 2 and 4, be explicit about which Git versions will exhibit\n different behavior when reading an index with a null trailing hash.\n * In Patch 2, used a more compact way of loading from config.\n * In Patch 2 (and following) dropped the sub-shell in t1600-index.sh.\n * In Patch 3, dropped back-ticks when using a multi-line pipe.\n * In Patch 4, use \"! cmp\" instead of \"! test_cmp\"\n\n\nUpdates since RFC\n=================\n\n * The config name is now index.skipHash to make it more clear that the\n default is to not skip the hash, and choosing this option requires a true\n value.\n * Use oidread() instead of an ad-hoc loop.\n * Patches 3 and 4 are new. 3 adds some extra test verification that wasn't\n in the RFC.\n * I think that patch 4 is helpful to see now, but I could understand if we\n wanted to defer that patch until after index.skipHash has had more time\n to bake.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: allow skipping the hash function\n read-cache: add index.skipHash config option\n test-lib-functions: add helper for trailing hash\n features: feature.manyFiles implies fast index writes\n\n Documentation/config/feature.txt | 5 +++++\n Documentation/config/index.txt | 9 +++++++++\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n read-cache.c | 15 ++++++++++++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 20 ++++++++++++++++++++\n t/test-lib-functions.sh | 8 ++++++++\n 10 files changed, 78 insertions(+), 4 deletions(-)\n\n\nbase-commit: 805265fcf7a737664a8321aaf4a0587b78435184\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1439%2Fderrickstolee%2Fhashfile-skip-hash-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1439/derrickstolee/hashfile-skip-hash-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1439\n\nRange-diff vs v1:\n\n 1: 40ee8dbaef0 ! 1: b582d681581 hashfile: allow skipping the hash function\n @@ csum-file.c: int finalize_hashfile(struct hashfile *f, unsigned char *result,\n -\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n +\n +\tif (f->skip_hash)\n -+\t\tmemset(f->buffer, 0, the_hash_algo->rawsz);\n ++\t\thashclr(f->buffer);\n +\telse\n +\t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n +\n 2: 5fb4b5a36ac ! 2: c8a2fd3a470 read-cache: add index.skipHash config option\n @@ Commit message\n The one exception is 'git fsck' which checks the hash of the index file.\n This used to be a check on every index read, but was split out to just\n the index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n - 2017-04-14).\n + 2017-04-14) and released first in Git 2.13.0. Document the versions that\n + relaxed these restrictions, with the optimistic expectation that this\n + change will be included in Git 2.40.0.\n \n Here, we disable this check if the trailing hash is all zeroes. We add a\n warning to the config option that this may cause undesirable behavior\n with older Git versions.\n \n As a quick comparison, I tested 'git update-index --force-write' with\n - and without index.computeHash=false on a copy of the Linux kernel\n + and without index.skipHash=true on a copy of the Linux kernel\n repository.\n \n Benchmark 1: with hash\n @@ Documentation/config/index.txt: index.version::\n +\tInstead, write a trailing set of bytes with value zero, indicating\n +\tthat the computation was skipped.\n ++\n -+If you enable `index.skipHash`, then older Git clients may report that\n -+your index is corrupt during `git fsck`.\n ++If you enable `index.skipHash`, then Git clients older than 2.13.0 will\n ++refuse to parse the index and Git clients older than 2.40.0 will report an\n ++error during `git fsck`.\n \n ## read-cache.c ##\n @@ read-cache.c: static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n @@ read-cache.c: static int verify_hdr(const struct cache_header *hdr, unsigned lon\n \treturn 0;\n }\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n - \tint ieot_entries = 1;\n - \tstruct index_entry_offset_table *ieot = NULL;\n - \tint nr, nr_threads;\n -+\tint skip_hash;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n -+\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n -+\t\tf->skip_hash = skip_hash;\n ++\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n +\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n @@ t/t1600-index.sh: test_expect_success 'out of bounds index.version issues warnin\n '\n \n +test_expect_success 'index.skipHash config option' '\n -+\t(\n -+\t\trm -f .git/index &&\n -+\t\tgit -c index.skipHash=true add a &&\n -+\t\tgit fsck\n -+\t)\n ++\trm -f .git/index &&\n ++\tgit -c index.skipHash=true add a &&\n ++\tgit fsck\n +'\n +\n test_index_version () {\n 3: a20bf8de864 ! 3: 813e81a0582 test-lib-functions: add helper for trailing hash\n @@ Commit message\n Signed-off-by: Derrick Stolee \n \n ## t/t1600-index.sh ##\n -@@ t/t1600-index.sh: test_expect_success 'index.skipHash config option' '\n - \t(\n - \t\trm -f .git/index &&\n - \t\tgit -c index.skipHash=true add a &&\n -+\t\ttest_trailing_hash .git/index >hash &&\n -+\t\techo $(test_oid zero) >expect &&\n -+\t\ttest_cmp expect hash &&\n - \t\tgit fsck\n - \t)\n +@@ t/t1600-index.sh: test_expect_success 'out of bounds index.version issues warning' '\n + test_expect_success 'index.skipHash config option' '\n + \trm -f .git/index &&\n + \tgit -c index.skipHash=true add a &&\n ++\ttest_trailing_hash .git/index >hash &&\n ++\techo $(test_oid zero) >expect &&\n ++\ttest_cmp expect hash &&\n + \tgit fsck\n '\n + \n \n ## t/test-lib-functions.sh ##\n @@ t/test-lib-functions.sh: test_cmp_config_output () {\n @@ t/test-lib-functions.sh: test_cmp_config_output () {\n +# Given a filename, extract its trailing hash as a hex string\n +test_trailing_hash () {\n +\tlocal file=\"$1\" &&\n -+\ttail -c $(test_oid rawsz) \"$file\" | \\\n -+\t\ttest-tool hexdump | \\\n ++\ttail -c $(test_oid rawsz) \"$file\" |\n ++\t\ttest-tool hexdump |\n +\t\tsed \"s/ //g\"\n +}\n 4: 77bf5d5ff27 ! 4: e640dff53dd features: feature.manyFiles implies fast index writes\n @@ Documentation/config/feature.txt: feature.manyFiles::\n \t`git checkout` may be slow and these new defaults improve performance:\n +\n +* `index.skipHash=true` speeds up index writes by not computing a trailing\n -+ checksum.\n ++ checksum. Note that this will cause Git versions earlier than 2.13.0 to\n ++ refuse to parse the index and Git versions earlier than 2.40.0 will report\n ++ a corrupted index during `git fsck`.\n ++\n * `index.version=4` enables path-prefix compression in the index.\n +\n @@ Documentation/config/feature.txt: feature.manyFiles::\n \n ## read-cache.c ##\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n - \tint ieot_entries = 1;\n - \tstruct index_entry_offset_table *ieot = NULL;\n - \tint nr, nr_threads;\n --\tint skip_hash;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n --\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n --\t\tf->skip_hash = skip_hash;\n +-\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n +\tif (istate->repo) {\n +\t\tprepare_repo_settings(istate->repo);\n +\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n @@ scalar.c: static int set_recommended_config(int reconfigure)\n \n ## t/t1600-index.sh ##\n @@ t/t1600-index.sh: test_expect_success 'index.skipHash config option' '\n - \t\ttest_trailing_hash .git/index >hash &&\n - \t\techo $(test_oid zero) >expect &&\n - \t\ttest_cmp expect hash &&\n --\t\tgit fsck\n -+\t\tgit fsck &&\n + \ttest_trailing_hash .git/index >hash &&\n + \techo $(test_oid zero) >expect &&\n + \ttest_cmp expect hash &&\n +-\tgit fsck\n ++\tgit fsck &&\n +\n -+\t\trm -f .git/index &&\n -+\t\tgit -c feature.manyFiles=true add a &&\n -+\t\ttest_trailing_hash .git/index >hash &&\n -+\t\ttest_cmp expect hash &&\n ++\trm -f .git/index &&\n ++\tgit -c feature.manyFiles=true add a &&\n ++\ttest_trailing_hash .git/index >hash &&\n ++\ttest_cmp expect hash &&\n +\n -+\t\trm -f .git/index &&\n -+\t\tgit -c feature.manyFiles=true \\\n -+\t\t -c index.skipHash=false add a &&\n -+\t\ttest_trailing_hash .git/index >hash &&\n -+\t\t! test_cmp expect hash\n - \t)\n ++\trm -f .git/index &&\n ++\tgit -c feature.manyFiles=true \\\n ++\t\t-c index.skipHash=false add a &&\n ++\ttest_trailing_hash .git/index >hash &&\n ++\t! cmp expect hash\n '\n \n + test_index_version () {\n\n-- \ngitgitgadget\n"},{"id":"468902","messageId":"c8a2fd3a470e1c12a7b182854874f63ed25b9992.1670862677.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v2.git.1670862677.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T16:31:15Z","receivedAt":"2022-12-12T16:31:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change allowed skipping the hashing portion of the\nhashwrite API, using it instead as a buffered write API. Disabling the\nhashwrite can be particularly helpful when the write operation is in a\ncritical path.\n\nOne such critical path is the writing of the index. This operation is so\ncritical that the sparse index was created specifically to reduce the\nsize of the index to make these writes (and reads) faster.\n\nFollowing a similar approach to one used in the microsoft/git fork [1],\nadd a new config option (index.skipHash) that allows disabling this\nhashing during the index write. The cost is that we can no longer\nvalidate the contents for corruption-at-rest using the trailing hash.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nWhile older Git versions will not recognize the null hash as a special\ncase, the file format itself is still being met in terms of its\nstructure. Using this null hash will still allow Git operations to\nfunction across older versions.\n\nThe one exception is 'git fsck' which checks the hash of the index file.\nThis used to be a check on every index read, but was split out to just\nthe index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n2017-04-14) and released first in Git 2.13.0. Document the versions that\nrelaxed these restrictions, with the optimistic expectation that this\nchange will be included in Git 2.40.0.\n\nHere, we disable this check if the trailing hash is all zeroes. We add a\nwarning to the config option that this may cause undesirable behavior\nwith older Git versions.\n\nAs a quick comparison, I tested 'git update-index --force-write' with\nand without index.skipHash=true on a copy of the Linux kernel\nrepository.\n\nBenchmark 1: with hash\n Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n Range (min … max): 34.3 ms … 79.1 ms 82 runs\n\nBenchmark 2: without hash\n Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n Range (min … max): 16.3 ms … 42.0 ms 69 runs\n\nSummary\n 'without hash' ran\n 1.78 ± 0.76 times faster than 'with hash'\n\nThese performance benefits are substantial enough to allow users the\nability to opt-in to this feature, even with the potential confusion\nwith older 'git fsck' versions.\n\nIt is critical that this test is placed before the test_index_version\ntests, since those tests obliterate the .git/config file and hence lose\nthe setting from GIT_TEST_DEFAULT_HASH, if set.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/index.txt | 9 +++++++++\n read-cache.c | 12 +++++++++++-\n t/t1600-index.sh | 6 ++++++\n 3 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/index.txt b/Documentation/config/index.txt\nindex 75f3a2d1054..5d62489c302 100644\n--- a/Documentation/config/index.txt\n+++ b/Documentation/config/index.txt\n@@ -30,3 +30,12 @@ index.version::\n \tSpecify the version with which new index files should be\n \tinitialized. This does not affect existing repositories.\n \tIf `feature.manyFiles` is enabled, then the default is 4.\n+\n+index.skipHash::\n+\tWhen enabled, do not compute the trailing hash for the index file.\n+\tInstead, write a trailing set of bytes with value zero, indicating\n+\tthat the computation was skipped.\n++\n+If you enable `index.skipHash`, then Git clients older than 2.13.0 will\n+refuse to parse the index and Git clients older than 2.40.0 will report an\n+error during `git fsck`.\ndiff --git a/read-cache.c b/read-cache.c\nindex 46f5e497b14..3f7de8b2e20 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tgit_hash_ctx c;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tint hdr_version;\n+\tunsigned char *start, *end;\n+\tstruct object_id oid;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(_(\"bad signature 0x%08x\"), hdr->hdr_signature);\n@@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tif (!verify_index_checksum)\n \t\treturn 0;\n \n+\tend = (unsigned char *)hdr + size;\n+\tstart = end - the_hash_algo->rawsz;\n+\toidread(&oid, start);\n+\tif (oideq(&oid, null_oid()))\n+\t\treturn 0;\n+\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n \tthe_hash_algo->final_fn(hash, &c);\n-\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n+\tif (!hasheq(hash, end - the_hash_algo->rawsz))\n \t\treturn error(_(\"bad index file sha1 signature\"));\n \treturn 0;\n }\n@@ -2918,6 +2926,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n+\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 010989f90e6..45feb0fc5d8 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -65,6 +65,12 @@ test_expect_success 'out of bounds index.version issues warning' '\n \t)\n '\n \n+test_expect_success 'index.skipHash config option' '\n+\trm -f .git/index &&\n+\tgit -c index.skipHash=true add a &&\n+\tgit fsck\n+'\n+\n test_index_version () {\n \tINDEX_VERSION_CONFIG=$1 &&\n \tFEATURE_MANY_FILES=$2 &&\n-- \ngitgitgadget\n\n"},{"id":"468903","messageId":"813e81a058227bd373cec802e443fcd677042fb4.1670862677.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v2.git.1670862677.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] test-lib-functions: add helper for trailing hash","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T16:31:16Z","receivedAt":"2022-12-12T16:31:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt can be helpful to check that a file format with a trailing hash has a\nspecific hash in the final bytes of a written file. This is made more\napparent by recent changes that allow skipping the hash algorithm and\nwriting a null hash at the end of the file instead.\n\nAdd a new test_trailing_hash helper and use it in t1600 to verify that\nindex.skipHash=true really does skip the hash computation, since\n'git fsck' does not actually verify the hash.\n\nKeep the 'git fsck' call to ensure that any potential future change to\ncheck the index hash does not cause an error in this case.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1600-index.sh | 3 +++\n t/test-lib-functions.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 45feb0fc5d8..55914bc3506 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -68,6 +68,9 @@ test_expect_success 'out of bounds index.version issues warning' '\n test_expect_success 'index.skipHash config option' '\n \trm -f .git/index &&\n \tgit -c index.skipHash=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\techo $(test_oid zero) >expect &&\n+\ttest_cmp expect hash &&\n \tgit fsck\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b32..60308843f8f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1875,3 +1875,11 @@ test_cmp_config_output () {\n \tsort config-actual >sorted-actual &&\n \ttest_cmp sorted-expect sorted-actual\n }\n+\n+# Given a filename, extract its trailing hash as a hex string\n+test_trailing_hash () {\n+\tlocal file=\"$1\" &&\n+\ttail -c $(test_oid rawsz) \"$file\" |\n+\t\ttest-tool hexdump |\n+\t\tsed \"s/ //g\"\n+}\n-- \ngitgitgadget\n\n"},{"id":"468913","messageId":"221212.868rjc5w8t.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"5301dfa1-71c9-1ff4-6ba0-e37f25e8ef62@github.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-12T18:01:51Z","receivedAt":"2022-12-12T18:14:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 12 2022, Derrick Stolee wrote:\n\n> On 12/7/2022 6:06 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> From: Derrick Stolee \n>>> [...]\n>>> While older Git versions will not recognize the null hash as a special\n>>> case, the file format itself is still being met in terms of its\n>>> structure. Using this null hash will still allow Git operations to\n>>> function across older versions.\n>> \n>> That's good news, but...\n>> \n>>> The one exception is 'git fsck' which checks the hash of the index file.\n>>> This used to be a check on every index read, but was split out to just\n>>> the index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n>>> 2017-04-14).\n>> \n>> ...uh, what?\n>> \n>> Is there an implied claim here that versions before v2.13.0 don't count\n>> as \"older versions\"?\n>> \n>> I.e. doesn't v2.12.0 hard fail the verification for all index writing?\n>> It's only after v2.13.0 that we do it only for the fsck.\n>> \n>> That seems like a rather significant caveat that we should be noting\n>> prominently in the docs added in 4/4.\n>\n> I can add those details.\n> \n>>> As a quick comparison, I tested 'git update-index --force-write' with\n>>> and without index.computeHash=false on a copy of the Linux kernel\n>>> repository.\n>> \n>> It took me a bit to see why I was failing to reproduce this, before\n>> finding that it's because you mention index.computeHash here, but it's\n>> index.skipHash now.\n>>>\n>>> Benchmark 1: with hash\n>>> Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n>>> Range (min … max): 34.3 ms … 79.1 ms 82 runs\n>>>\n>>> Benchmark 2: without hash\n>>> Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n>>> Range (min … max): 16.3 ms … 42.0 ms 69 runs\n>>>\n>>> Summary\n>>> 'without hash' ran\n>>> 1.78 ± 0.76 times faster than 'with hash'\n>> \n>> I suggested in\n>> https://lore.kernel.org/git/221207.868rjiam86.gmgdl@evledraar.gmail.com/\n>> earlier to benchmark this against not-sha1collisiondetection.\n>\n> Generally, I'm avoiding that benchmark because sha1dc is here to stay.\n>\n> If users want to go through the trouble of compiling to use the non-dc\n> version, then I would expect the difference to be less noticeable, but\n> still significant. However, I would strongly avoid considering compiling\n> both into the client by default, letting certain paths use sha1dc and\n> others using non-dc. Certain secure environments currently only use Git\n> under exceptions that allow SHA1 for \"non-cryptographic\" reasons, but\n> also with the understanding that sha1dc is used as a safety measure.\n> Adding the non-dc version back in would put that understanding at risk.\n\nDoesn't using a checksum for our own index count as a \"non-cryptographic\nreason\"? I.e. we control the .git/index file, and the context is that\nwe're checking if bytes we wrote to disk are corrupt since we last saw\nthem.\n\nEven if hypothetically an attacker could craft files to go into the\nindex (knowing our envelope) in such a way as to craft a collision\nbetween that index file and some other index file I don't see how that\nwould give the attacker anything. We'd still have a valid index, and\nwe'd probably be replacing that crafted index with a new one anyway.\n\nI understand that some organizations have SHA-1 on some naughty list,\nand using it again in non-SHA1DC contexts might trigger some audit.\n\nSo it wouldn't be something for everyone, and it's orthagonal to the\nbenefits of a new ref format or index format.\n\nBut if we're considering new formats, I think it's worth considering a\nnon-format change which doesn't get us all of the way of no\nchecksumming, but more than halfway there.\n\nMaybe we'll still want the \"don't do any checksumming\", but maybe some\nwould find that enough (particularly if SHA-1 HW acceleration is\navailable).\n\n\n\n"},{"id":"468915","messageId":"20221212181438.GA3411@szeder.dev","threadId":"58917","inReplyTo":"813e81a058227bd373cec802e443fcd677042fb4.1670862677.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] test-lib-functions: add helper for trailing hash","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-12-12T18:14:38Z","receivedAt":"2022-12-12T18:17:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 12, 2022 at 04:31:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/t/t1600-index.sh b/t/t1600-index.sh\n> index 45feb0fc5d8..55914bc3506 100755\n> --- a/t/t1600-index.sh\n> +++ b/t/t1600-index.sh\n> @@ -68,6 +68,9 @@ test_expect_success 'out of bounds index.version issues warning' '\n> test_expect_success 'index.skipHash config option' '\n> \trm -f .git/index &&\n> \tgit -c index.skipHash=true add a &&\n> +\ttest_trailing_hash .git/index >hash &&\n> +\techo $(test_oid zero) >expect &&\n\nNit: test_oid zero >expect\n\n> +\ttest_cmp expect hash &&\n> \tgit fsck\n> '\n"},{"id":"468918","messageId":"221212.86v8mg4gr2.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"1b2aa60c-633f-0a7f-e8e3-5d0f01ff94b6@github.com","subject":"Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-12T18:27:01Z","receivedAt":"2022-12-12T18:33:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 12 2022, Derrick Stolee wrote:\n\n> On 12/7/2022 5:30 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> From: Derrick Stolee \n>>> [...]\n>>> diff --git a/read-cache.c b/read-cache.c\n>>> index fb4d6fb6387..1844953fba7 100644\n>>> --- a/read-cache.c\n>>> +++ b/read-cache.c\n>>> @@ -2923,12 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>>> \tint ieot_entries = 1;\n>>> \tstruct index_entry_offset_table *ieot = NULL;\n>>> \tint nr, nr_threads;\n>>> -\tint skip_hash;\n>>> \n>>> \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n>>> \n>>> -\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n>>> -\t\tf->skip_hash = skip_hash;\n>>> +\tif (istate->repo) {\n>>> +\t\tprepare_repo_settings(istate->repo);\n>>> +\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n>>> +\t}\n>> \n>> Urm, are we ever going to find ourselves in a situation where:\n>> \n>> * We have read the settings for the_repository\n>> * We have an index we're about to write out as our \"main index\", but\n>> the istate->repo *isn't* the_repository.\n>> * Even then, wouldn't the two copies of the repos have read the same\n>> repo settings?\n>> \n>> But maybe there's a really obvious submodule / worktree / whatever edge\n>> case I'm missing.\n>> \n>> But if not, shouldn't we just always read/write this from\n>> the_repository?\n>\n> I don't understand your concern. We call prepare_repo_settings(istate->repo)\n> just before using these settings, so we are using whatever repository-local\n> config we have available to us.\n>\n> If you're thinking that we could be writing an index but istate->repo is\n> somehow the \"wrong\" repo, then that is a larger problem. This patch is\n> doing the best thing it can with the information it is given.\n\nIt's not a concern, just confusion :)\n\nIn the preceding step (and this is still the case in your v2) we used\ngit_config_get_maybe_bool(), if we meant to use istate->repo shouldn't\nwe have used repo_config_get_maybe_bool() to begin with?\n\nAnd will we ever get !istate->repo? If not should we BUG() here?\nOtherwise the 4/4 changes this to a state where we'll no longer read the\nindex.skipHash setting if that \"repo\" is NULL, but our previous\nthe_repository was non-NULL...\n"},{"id":"468920","messageId":"CAPig+cTA=4DZEhM23tVfoRjxPuBAgs=+UGc5+-6rg4DYQxxKdg@mail.gmail.com","threadId":"58917","inReplyTo":"37efcfcd-fea1-c1d7-65f7-ae5f2d2a12e6@github.com","subject":"Re: [PATCH 2/4] read-cache: add index.skipHash config option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-12T18:55:11Z","receivedAt":"2022-12-12T18:57:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 12, 2022 at 8:59 AM Derrick Stolee wrote:\n> On 12/7/2022 1:59 PM, Eric Sunshine wrote:\n> > On Wed, Dec 7, 2022 at 12:27 PM Derrick Stolee via GitGitGadget\n> > wrote:\n> >> +index.skipHash::\n> >> + When enabled, do not compute the trailing hash for the index file.\n> >> + Instead, write a trailing set of bytes with value zero, indicating\n> >> + that the computation was skipped.\n> >> ++\n> >> +If you enable `index.skipHash`, then older Git clients may report that\n> >> +your index is corrupt during `git fsck`.\n> >\n> > This documentation is rather minimal. Given this description, are\n> > readers going to understand the purpose of the option, when they\n> > should use it, what the impact will be, when and why they should avoid\n> > it, etc.?\n>\n> I will expand this with explicit version numbers for older Git versions.\n\nOkay, but that doesn't address the larger questions I asked. The\ndocumentation, as written, gives no explanation of the purpose of this\noption. Since you conceived of the option and implemented it, you\nimplicitly understand its use-case and repercussions which might arise\nfrom using it, but is the typical reader going to understand all that?\nNamely, is the reader going to understand:\n\n* why this option exists\n* what problem it is trying to solve\n* when to use it\n* when not to use it\n* what the repercussions are of not computing a hash for the index\n* etc.\n\nAre the answers to those questions documented somewhere? If so, then\nthe documentation for this option should link to that discussion (and\nvice-versa). If not, then those questions should be answered by this\ndocumentation.\n"},{"id":"468954","messageId":"CA+P7+xoaQFsYcwpaDz3Hg61cckfvK960z_rCTC3XTHeLzxwkvA@mail.gmail.com","threadId":"58917","inReplyTo":"3a47b7ac-25dc-5f70-9ab4-08c781df4cda@github.com","subject":"Re: [PATCH 0/4] Optionally skip hashing index on write","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-12-12T22:22:12Z","receivedAt":"2022-12-12T22:22:30Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Dec 8, 2022 at 8:50 AM Derrick Stolee wrote:\n>\n> On 12/7/2022 6:27 PM, Junio C Hamano wrote:\n> > \"Derrick Stolee via GitGitGadget\" writes:\n> >\n> >> Writing the index is a critical action that takes place in multiple Git\n> >> commands. The recent performance improvements available with the sparse\n> >> index show how often the I/O costs around the index can affect different Git\n> >> commands, although reading the index takes place more often than a write.\n> >\n> > The sparse-index work is great in that it offers correctness while\n> > taking advantage of the knowledge of which part of the tree is\n> > quiescent and unused to boost performance. I am not sure a change\n> > to reduce file safety can be compared with it, in that one is pure\n> > improvement, while the other is trade-off.\n>\n> I agree that this is a trade-off, and we should both be careful about\n> whether or not we even make this a possibility for certain file\n> formats. The index is an interesting case for a couple reasons:\n>\n> 1. Writes block users. Writing the index takes place in many user-\n> blocking foreground operations. The speed improvement directly\n> impacts their use. Other file formats are typically written in\n> the background (commit-graph, multi-pack-index) or are super-\n> critical to correctness (pack-files).\n>\n> 2. Index files are short lived. It is rare that a user leaves an\n> index for a long time with many staged changes. That's the condition\n> that's required for losing an index file to cause a loss of work\n> (or maybe I'm missing something). Outside of staged changes, the\n> index can be completely destroyed and rewritten with minimal impact\n> to the user.\n>\n\nIs this information in the commit messages somewhere? I didn't see it\nin the cover letter. Nor did I see any other explanation in the cover\nletter besides \"this makes it faster\". I would expect such trade off\nor analysis of \"what do we lose\" to be in the cover letter, as it may\nnot be clear otherwise.\n\nI do agree these reasons are good, but it can be confusing to later\nreviewers when looking back at the code for an option like this and\nwondering why it exists.\n\nThanks,\nJake\n"},{"id":"468967","messageId":"xmqqh6y0qfzg.fsf@gitster.g","threadId":"58917","inReplyTo":"20221212181438.GA3411@szeder.dev","subject":"Re: [PATCH v2 3/4] test-lib-functions: add helper for trailing hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-13T00:55:47Z","receivedAt":"2022-12-13T00:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> +\ttest_trailing_hash .git/index >hash &&\n>> +\techo $(test_oid zero) >expect &&\n>\n> Nit: test_oid zero >expect\n>\n>> +\ttest_cmp expect hash &&\n\nUnfortunately they are not equivalent.\n\nUsually we write these helpers to terminate their output with LF,\nrelying on the fact that terminating LF will be dropped when used in\na command substition, e.g. VAR=$(HELPER), but test_oid deviates from\nthe pattern and does not give the terminating LF to its output.\n\n\n"},{"id":"469123","messageId":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v2.git.1670862677.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-15T15:06:56Z","receivedAt":"2022-12-15T15:07:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Writing the index is a critical action that takes place in multiple Git\ncommands. The recent performance improvements available with the sparse\nindex show how often the I/O costs around the index can affect different Git\ncommands, although reading the index takes place more often than a write.\n\nThe index is written through the hashfile API, both as a buffered write and\nas a computation of its trailing hash. This trailing hash is an expectation\nof the file format: several optional index extensions are provided using\nindicators immediately preceding the trailing hash. 'git fsck' will complain\nif the trailing hash does not match the contents of the file up to that\npoint.\n\nHowever, computing the hash is expensive. This is even more expensive now\nthat we've moved to sha1dc instead of hardware-accelerated SHA1\ncomputations.\n\nThis series provides a new config option, index.skipHash, that allows users\nto disable computing the hash as Git writes the index. In this case, the\ntrailing hash is stored as the null hash (all bytes are zero).\n\nThe implementation is rather simple, but the patches organize different\naspects in a way that we could split things out:\n\n * Patch 1 adds a 'skip_hash' option to 'struct hashfile' that creates this\n as a general option to the hashwrite API.\n * The motivation in Patch 1 avoids the talk about the chunk-format API and\n instead focuses on the index as the first example that could make use of\n this.\n * Patch 2 creates the index.skipHash config option and updates 'git fsck'\n to ignore a null hash on the index. This is demonstrated by a very simple\n test.\n * Patch 3 creates a test helper to get the trailing hash of a file, and\n adds a concrete check on the trailing hash when index.skipHash=true.\n * Patch 4 (which could be deferred until later) adds index.skipHash=true as\n an implication of feature.manyFiles and as a setting in Scalar.\n\nThe end result is that index writes speed up significantly, enough that 'git\nupdate-index --force-write' improves by 1.75x.\n\nSimilar patches appeared in my earlier refs RFC [1].\n\n[1]\nhttps://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\n\nUpdates since v2\n================\n\n * Patch 2 now has additional details about why to use the config option in\n its message. The discussion about why the index is special is included in\n the commit message.\n\n\nUpdates since v1\n================\n\n * In Patch 1, use hashcler() instead of memset().\n * In Patches 2 and 4, be explicit about which Git versions will exhibit\n different behavior when reading an index with a null trailing hash.\n * In Patch 2, used a more compact way of loading from config.\n * In Patch 2 (and following) dropped the sub-shell in t1600-index.sh.\n * In Patch 3, dropped back-ticks when using a multi-line pipe.\n * In Patch 4, use \"! cmp\" instead of \"! test_cmp\"\n\n\nUpdates since RFC\n=================\n\n * The config name is now index.skipHash to make it more clear that the\n default is to not skip the hash, and choosing this option requires a true\n value.\n * Use oidread() instead of an ad-hoc loop.\n * Patches 3 and 4 are new. 3 adds some extra test verification that wasn't\n in the RFC.\n * I think that patch 4 is helpful to see now, but I could understand if we\n wanted to defer that patch until after index.skipHash has had more time\n to bake.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: allow skipping the hash function\n read-cache: add index.skipHash config option\n test-lib-functions: add helper for trailing hash\n features: feature.manyFiles implies fast index writes\n\n Documentation/config/feature.txt | 5 +++++\n Documentation/config/index.txt | 11 +++++++++++\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n read-cache.c | 15 ++++++++++++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 20 ++++++++++++++++++++\n t/test-lib-functions.sh | 8 ++++++++\n 10 files changed, 80 insertions(+), 4 deletions(-)\n\n\nbase-commit: 805265fcf7a737664a8321aaf4a0587b78435184\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1439%2Fderrickstolee%2Fhashfile-skip-hash-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1439/derrickstolee/hashfile-skip-hash-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1439\n\nRange-diff vs v2:\n\n 1: b582d681581 = 1: b582d681581 hashfile: allow skipping the hash function\n 2: c8a2fd3a470 ! 2: 00738c81a12 read-cache: add index.skipHash config option\n @@ Commit message\n critical that the sparse index was created specifically to reduce the\n size of the index to make these writes (and reads) faster.\n \n + This trade-off between file stability at rest and write-time performance\n + is not easy to balance. The index is an interesting case for a couple\n + reasons:\n +\n + 1. Writes block users. Writing the index takes place in many user-\n + blocking foreground operations. The speed improvement directly\n + impacts their use. Other file formats are typically written in the\n + background (commit-graph, multi-pack-index) or are super-critical to\n + correctness (pack-files).\n +\n + 2. Index files are short lived. It is rare that a user leaves an index\n + for a long time with many staged changes. Outside of staged changes,\n + the index can be completely destroyed and rewritten with minimal\n + impact to the user.\n +\n Following a similar approach to one used in the microsoft/git fork [1],\n add a new config option (index.skipHash) that allows disabling this\n hashing during the index write. The cost is that we can no longer\n @@ Documentation/config/index.txt: index.version::\n +\n +index.skipHash::\n +\tWhen enabled, do not compute the trailing hash for the index file.\n -+\tInstead, write a trailing set of bytes with value zero, indicating\n ++\tThis accelerates Git commands that manipulate the index, such as\n ++\t`git add`, `git commit`, or `git status`. Instead of storing the\n ++\tchecksum, write a trailing set of bytes with value zero, indicating\n +\tthat the computation was skipped.\n ++\n +If you enable `index.skipHash`, then Git clients older than 2.13.0 will\n 3: 813e81a0582 = 3: 86370af1351 test-lib-functions: add helper for trailing hash\n 4: e640dff53dd = 4: 6490bd445eb features: feature.manyFiles implies fast index writes\n\n-- \ngitgitgadget\n"},{"id":"469124","messageId":"b582d681581a356534ef018c3a12004b2d9779a7.1671116820.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] hashfile: allow skipping the hash function","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-15T15:06:57Z","receivedAt":"2022-12-15T15:07:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API is useful for generating files that include a trailing\nhash of the file's contents up to that point. Using such a hash is\nhelpful for verifying the file for corruption-at-rest, such as a faulty\ndrive causing flipped bits.\n\nGit's index file includes this trailing hash, so it uses a 'struct\nhashfile' to handle the I/O to the file. This was very convenient to\nallow using the hashfile methods during these operations.\n\nHowever, hashing the file contents during write comes at a performance\npenalty. It's slower to hash the bytes on their way to the disk than\nwithout that step. This problem is made worse by the replacement of\nhardware-accelerated SHA1 computations with the software-based sha1dc\ncomputation.\n\nThis write cost is significant, and the checksum capability is likely\nnot worth that cost for such a short-lived file. The index is rewritten\nfrequently and the only time the checksum is checked is during 'git\nfsck'. Thus, it would be helpful to allow a user to opt-out of the hash\ncomputation.\n\nWe first need to allow Git to opt-out of the hash computation in the\nhashfile API. The buffered writes of the API are still helpful, so it\nmakes sense to make the change here.\n\nIntroduce a new 'skip_hash' option to 'struct hashfile'. When set, the\nupdate_fn and final_fn members of the_hash_algo are skipped. When\nfinalizing the hashfile, the trailing hash is replaced with the null\nhash.\n\nThis use of a trailing null hash would be desireable in either case,\nsince we do not want to special case a file format to have a different\nlength depending on whether it was hashed or not. When the final bytes\nof a file are all zero, we can infer that it was written without\nhashing, and thus that verification is not available as a check for file\nconsistency. This also means that we could easily toggle hashing for any\nfile format we desire.\n\nA version of this patch has existed in the microsoft/git fork since\n2017 [1] (the linked commit was rebased in 2018, but the original dates\nback to January 2017). Here, the change to make the index use this fast\npath is delayed until a later change.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nCo-authored-by: Kevin Willford \nSigned-off-by: Kevin Willford \nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 59ef3398ca2..cce13c0f047 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -45,7 +45,8 @@ void hashflush(struct hashfile *f)\n \tunsigned offset = f->offset;\n \n \tif (offset) {\n-\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n+\t\tif (!f->skip_hash)\n+\t\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n \t\tflush(f, f->buffer, offset);\n \t\tf->offset = 0;\n \t}\n@@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \tint fd;\n \n \thashflush(f);\n-\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n+\tif (f->skip_hash)\n+\t\thashclr(f->buffer);\n+\telse\n+\t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & CSUM_HASH_IN_STREAM)\n@@ -108,7 +114,8 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\t * the hashfile's buffer. In this block,\n \t\t\t * f->offset is necessarily zero.\n \t\t\t */\n-\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n+\t\t\tif (!f->skip_hash)\n+\t\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n \t\t\tflush(f, buf, nr);\n \t\t} else {\n \t\t\t/*\n@@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name,\n \tf->tp = tp;\n \tf->name = name;\n \tf->do_crc = 0;\n+\tf->skip_hash = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n \n \tf->buffer_len = buffer_len;\ndiff --git a/csum-file.h b/csum-file.h\nindex 0d29f528fbc..29468067f81 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -20,6 +20,13 @@ struct hashfile {\n \tsize_t buffer_len;\n \tunsigned char *buffer;\n \tunsigned char *check_buffer;\n+\n+\t/**\n+\t * If set to 1, skip_hash indicates that we should\n+\t * not actually compute the hash for this hashfile and\n+\t * instead only use it as a buffered write.\n+\t */\n+\tunsigned int skip_hash;\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"469125","messageId":"00738c81a1212970910da6f29fe3ecef87c2ec3a.1671116820.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-15T15:06:58Z","receivedAt":"2022-12-15T15:07:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change allowed skipping the hashing portion of the\nhashwrite API, using it instead as a buffered write API. Disabling the\nhashwrite can be particularly helpful when the write operation is in a\ncritical path.\n\nOne such critical path is the writing of the index. This operation is so\ncritical that the sparse index was created specifically to reduce the\nsize of the index to make these writes (and reads) faster.\n\nThis trade-off between file stability at rest and write-time performance\nis not easy to balance. The index is an interesting case for a couple\nreasons:\n\n1. Writes block users. Writing the index takes place in many user-\n blocking foreground operations. The speed improvement directly\n impacts their use. Other file formats are typically written in the\n background (commit-graph, multi-pack-index) or are super-critical to\n correctness (pack-files).\n\n2. Index files are short lived. It is rare that a user leaves an index\n for a long time with many staged changes. Outside of staged changes,\n the index can be completely destroyed and rewritten with minimal\n impact to the user.\n\nFollowing a similar approach to one used in the microsoft/git fork [1],\nadd a new config option (index.skipHash) that allows disabling this\nhashing during the index write. The cost is that we can no longer\nvalidate the contents for corruption-at-rest using the trailing hash.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nWhile older Git versions will not recognize the null hash as a special\ncase, the file format itself is still being met in terms of its\nstructure. Using this null hash will still allow Git operations to\nfunction across older versions.\n\nThe one exception is 'git fsck' which checks the hash of the index file.\nThis used to be a check on every index read, but was split out to just\nthe index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n2017-04-14) and released first in Git 2.13.0. Document the versions that\nrelaxed these restrictions, with the optimistic expectation that this\nchange will be included in Git 2.40.0.\n\nHere, we disable this check if the trailing hash is all zeroes. We add a\nwarning to the config option that this may cause undesirable behavior\nwith older Git versions.\n\nAs a quick comparison, I tested 'git update-index --force-write' with\nand without index.skipHash=true on a copy of the Linux kernel\nrepository.\n\nBenchmark 1: with hash\n Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n Range (min … max): 34.3 ms … 79.1 ms 82 runs\n\nBenchmark 2: without hash\n Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n Range (min … max): 16.3 ms … 42.0 ms 69 runs\n\nSummary\n 'without hash' ran\n 1.78 ± 0.76 times faster than 'with hash'\n\nThese performance benefits are substantial enough to allow users the\nability to opt-in to this feature, even with the potential confusion\nwith older 'git fsck' versions.\n\nIt is critical that this test is placed before the test_index_version\ntests, since those tests obliterate the .git/config file and hence lose\nthe setting from GIT_TEST_DEFAULT_HASH, if set.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/index.txt | 11 +++++++++++\n read-cache.c | 12 +++++++++++-\n t/t1600-index.sh | 6 ++++++\n 3 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/index.txt b/Documentation/config/index.txt\nindex 75f3a2d1054..23c7985eb40 100644\n--- a/Documentation/config/index.txt\n+++ b/Documentation/config/index.txt\n@@ -30,3 +30,14 @@ index.version::\n \tSpecify the version with which new index files should be\n \tinitialized. This does not affect existing repositories.\n \tIf `feature.manyFiles` is enabled, then the default is 4.\n+\n+index.skipHash::\n+\tWhen enabled, do not compute the trailing hash for the index file.\n+\tThis accelerates Git commands that manipulate the index, such as\n+\t`git add`, `git commit`, or `git status`. Instead of storing the\n+\tchecksum, write a trailing set of bytes with value zero, indicating\n+\tthat the computation was skipped.\n++\n+If you enable `index.skipHash`, then Git clients older than 2.13.0 will\n+refuse to parse the index and Git clients older than 2.40.0 will report an\n+error during `git fsck`.\ndiff --git a/read-cache.c b/read-cache.c\nindex 46f5e497b14..3f7de8b2e20 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tgit_hash_ctx c;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tint hdr_version;\n+\tunsigned char *start, *end;\n+\tstruct object_id oid;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(_(\"bad signature 0x%08x\"), hdr->hdr_signature);\n@@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tif (!verify_index_checksum)\n \t\treturn 0;\n \n+\tend = (unsigned char *)hdr + size;\n+\tstart = end - the_hash_algo->rawsz;\n+\toidread(&oid, start);\n+\tif (oideq(&oid, null_oid()))\n+\t\treturn 0;\n+\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n \tthe_hash_algo->final_fn(hash, &c);\n-\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n+\tif (!hasheq(hash, end - the_hash_algo->rawsz))\n \t\treturn error(_(\"bad index file sha1 signature\"));\n \treturn 0;\n }\n@@ -2918,6 +2926,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n+\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 010989f90e6..45feb0fc5d8 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -65,6 +65,12 @@ test_expect_success 'out of bounds index.version issues warning' '\n \t)\n '\n \n+test_expect_success 'index.skipHash config option' '\n+\trm -f .git/index &&\n+\tgit -c index.skipHash=true add a &&\n+\tgit fsck\n+'\n+\n test_index_version () {\n \tINDEX_VERSION_CONFIG=$1 &&\n \tFEATURE_MANY_FILES=$2 &&\n-- \ngitgitgadget\n\n"},{"id":"469126","messageId":"86370af1351a275d5583c76c07b536754e4b6afc.1671116820.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] test-lib-functions: add helper for trailing hash","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-15T15:06:59Z","receivedAt":"2022-12-15T15:07:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt can be helpful to check that a file format with a trailing hash has a\nspecific hash in the final bytes of a written file. This is made more\napparent by recent changes that allow skipping the hash algorithm and\nwriting a null hash at the end of the file instead.\n\nAdd a new test_trailing_hash helper and use it in t1600 to verify that\nindex.skipHash=true really does skip the hash computation, since\n'git fsck' does not actually verify the hash.\n\nKeep the 'git fsck' call to ensure that any potential future change to\ncheck the index hash does not cause an error in this case.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1600-index.sh | 3 +++\n t/test-lib-functions.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 45feb0fc5d8..55914bc3506 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -68,6 +68,9 @@ test_expect_success 'out of bounds index.version issues warning' '\n test_expect_success 'index.skipHash config option' '\n \trm -f .git/index &&\n \tgit -c index.skipHash=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\techo $(test_oid zero) >expect &&\n+\ttest_cmp expect hash &&\n \tgit fsck\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b32..60308843f8f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1875,3 +1875,11 @@ test_cmp_config_output () {\n \tsort config-actual >sorted-actual &&\n \ttest_cmp sorted-expect sorted-actual\n }\n+\n+# Given a filename, extract its trailing hash as a hex string\n+test_trailing_hash () {\n+\tlocal file=\"$1\" &&\n+\ttail -c $(test_oid rawsz) \"$file\" |\n+\t\ttest-tool hexdump |\n+\t\tsed \"s/ //g\"\n+}\n-- \ngitgitgadget\n\n"},{"id":"469127","messageId":"6490bd445ebea41223eec38784426ad17a0711b6.1671116820.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] features: feature.manyFiles implies fast index writes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-15T15:07:00Z","receivedAt":"2022-12-15T15:07:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe recent addition of the index.skipHash config option allows index\nwrites to speed up by skipping the hash computation for the trailing\nchecksum. This is particularly critical for repositories with many files\nat HEAD, so add this config option to two cases where users in that\nscenario may opt-in to such behavior:\n\n 1. The feature.manyFiles config option enables some options that are\n helpful for repositories with many files at HEAD.\n\n 2. 'scalar register' and 'scalar reconfigure' set config options that\n optimize for large repositories.\n\nIn both of these cases, set index.skipHash=true to gain this\nspeedup. Add tests that demonstrate the proper way that\nindex.skipHash=true can override feature.manyFiles=true.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/feature.txt | 5 +++++\n read-cache.c | 5 ++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 13 ++++++++++++-\n 6 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 95975e50912..e52bc6b8584 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -23,6 +23,11 @@ feature.manyFiles::\n \tworking directory. With many files, commands such as `git status` and\n \t`git checkout` may be slow and these new defaults improve performance:\n +\n+* `index.skipHash=true` speeds up index writes by not computing a trailing\n+ checksum. Note that this will cause Git versions earlier than 2.13.0 to\n+ refuse to parse the index and Git versions earlier than 2.40.0 will report\n+ a corrupted index during `git fsck`.\n++\n * `index.version=4` enables path-prefix compression in the index.\n +\n * `core.untrackedCache=true` enables the untracked cache. This setting assumes\ndiff --git a/read-cache.c b/read-cache.c\nindex 3f7de8b2e20..1844953fba7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2926,7 +2926,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n-\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n+\tif (istate->repo) {\n+\t\tprepare_repo_settings(istate->repo);\n+\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n+\t}\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 3021921c53d..3dbd3f0e2ec 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -47,6 +47,7 @@ void prepare_repo_settings(struct repository *r)\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\n+\t\tr->settings.index_skip_hash = 1;\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n@@ -61,6 +62,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n+\trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 6c461c5b9de..e8c67ffe165 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -42,6 +42,7 @@ struct repo_settings {\n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \n \tint index_version;\n+\tint index_skip_hash;\n \tenum untracked_cache_setting core_untracked_cache;\n \n \tint pack_use_sparse;\ndiff --git a/scalar.c b/scalar.c\nindex 6c52243cdf1..b49bb8c24ec 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -143,6 +143,7 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"credential.validate\", \"false\", 1 }, /* GCM4W-only */\n \t\t{ \"gc.auto\", \"0\", 1 },\n \t\t{ \"gui.GCWarning\", \"false\", 1 },\n+\t\t{ \"index.skipHash\", \"false\", 1 },\n \t\t{ \"index.threads\", \"true\", 1 },\n \t\t{ \"index.version\", \"4\", 1 },\n \t\t{ \"merge.stat\", \"false\", 1 },\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 55914bc3506..103743a1c7d 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -71,7 +71,18 @@ test_expect_success 'index.skipHash config option' '\n \ttest_trailing_hash .git/index >hash &&\n \techo $(test_oid zero) >expect &&\n \ttest_cmp expect hash &&\n-\tgit fsck\n+\tgit fsck &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\ttest_cmp expect hash &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true \\\n+\t\t-c index.skipHash=false add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\t! cmp expect hash\n '\n \n test_index_version () {\n-- \ngitgitgadget\n"},{"id":"469133","messageId":"221215.865yec3b1j.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/4] Optionally skip hashing index on write","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T15:56:11Z","receivedAt":"2022-12-15T16:08:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 15 2022, Derrick Stolee via GitGitGadget wrote:\n\n> Updates since v2\n> ================\n>\n> * Patch 2 now has additional details about why to use the config option in\n> its message. The discussion about why the index is special is included in\n> the commit message.\n\nRe the comments I had on an earlier version[1] I tried this series with\nthese changes squashed in:\n\t\n\t1: b582d681581 = 1: 53d289cf82c hashfile: allow skipping the hash function\n\t2: 00738c81a12 ! 2: db487f3e76d read-cache: add index.skipHash config option\n\t @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n\t \n\t \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n\t \n\t -+\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n\t ++\trepo_config_get_maybe_bool(istate->repo, \"index.skiphash\", (int *)&f->skip_hash);\n\t +\n\t \tfor (i = removed = extended = 0; i < entries; i++) {\n\t \t\tif (cache[i]->ce_flags & CE_REMOVE)\n\t3: 86370af1351 = 3: 35188bbcfb5 test-lib-functions: add helper for trailing hash\n\t4: 6490bd445eb ! 4: 4354328e8fc features: feature.manyFiles implies fast index writes\n\t @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n\t \n\t \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n\t \n\t --\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n\t +-\trepo_config_get_maybe_bool(istate->repo, \"index.skiphash\", (int *)&f->skip_hash);\n\t +\tif (istate->repo) {\n\t ++\t\tint ours, theirs;\n\t ++\n\t +\t\tprepare_repo_settings(istate->repo);\n\t -+\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n\t ++\n\t ++\t\tours = istate->repo->settings.index_skip_hash;\n\t ++\t\ttheirs = the_repository->settings.index_skip_hash;\n\t ++\n\t ++\t\tif (ours != theirs)\n\t ++\t\t\tBUG(\"ours %d theirs %d\", ours, theirs);\n\t ++\t\tf->skip_hash = ours;\n\t ++\t} else {\n\t ++\t\tf->skip_hash = the_repository->settings.index_skip_hash;\n\t ++\t\t/*BUG(\"no repo???\");*/\n\t +\t}\n\t \n\t \tfor (i = removed = extended = 0; i < entries; i++) {\n\nWith those all of your tests still pass. Which leads to these outstanding questions:\n\na) If we're doing to use \"istate->repo\" in 4/4 why not do so in 2/4,\n neither patch discusses *that* part of the change.\n\n On an unrelated topic there's this[2] fix-up of yours, 2/2 still\n seems like it needs the same treatment.\n\nb) In 2/4 we'd get the config if \"istate->repo\" was NULL, if you\n uncomment that BUG() in the squashed 4/4 we'll get test failures all\n over the place. Aren't these places where we'd get the config before,\n but istate->repo isn't set up properly, so with 4/4 we won't get it?\n\n Maybe that's desired, but again, neither commit discusses this\n change.\n\nNow, I *think* it's a good idea to defer to the already set-up\n'istate->repo', but I also know (and have some local patches to fix...)\nabout the cases where we don't set it up (but should), so it can't be\nfully relied on.\n\nSo even if we can't produce a behavior difference, just doing e.g.:\n\n\tstruct repository *r = istate->repo ? istate->repo : the_repository;\n\nAnd then using:\n\n\tprepare_repo_settings(r);\n f->skip_hash = r->->settings.index_skip_hash;\n\nSeems sensible. I just don't get why 4/4 has that seeming fix-up of 2/4\nafter-the-fact. Isn't it better to carve that bit out of 4/4, just do\nthe config setup in repo-settings.c to begin with, and have 4/4 do what\nits $subject says, i.e. to have \"feature.manyFiles\" imply this new\nconfig?\n\nThe rest of this all looks sensible to me, sans some isolated things\nI'll comment on on individual patches.\n\n1. https://lore.kernel.org/git/221212.86v8mg4gr2.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/857d1abec4cf124e011c7f84276ce105cb5b3a96.1670866407.git.gitgitgadget@gmail.com/\n"},{"id":"469134","messageId":"221215.861qp03agm.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"00738c81a1212970910da6f29fe3ecef87c2ec3a.1671116820.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/4] read-cache: add index.skipHash config option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T16:12:01Z","receivedAt":"2022-12-15T16:20:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 15 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n\n> +\tend = (unsigned char *)hdr + size;\n\nCommentary: The \"unsigned char *\" cast has moved up here from the below,\nthat's needed (we're casting from the struct), and we should get rid of\nit from below, good.\n\n> +\tstart = end - the_hash_algo->rawsz;\n\nOkey, so here we mark the start, which is the end minus the rawsz,\nbut...\n\n> +\toidread(&oid, start);\n> +\tif (oideq(&oid, null_oid()))\n> +\t\treturn 0;\n> +\n> \tthe_hash_algo->init_fn(&c);\n> \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n> \tthe_hash_algo->final_fn(hash, &c);\n> -\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n> +\tif (!hasheq(hash, end - the_hash_algo->rawsz))\n\n...here we got rid of the cast, which is good, but let's not use \"end -\nthe_hash_algo->rawsz\" here, let's use \"start\", which you already\ncomputed as \"end - the_hash_algo->rawsz\". This is just repeating it.\n\nI wondered if I just missed it being modified in the interim before\ncarefully re-reading this, but we pass your tests with:\n\n\t- if (!hasheq(hash, end - the_hash_algo->rawsz))\n\t+ assert((end - the_hash_algo->rawsz) == start);\n\t+ if (!hasheq(hash, start))\n\nSo, we can indeed juse the simpler \"start\" here, and it makes it easier\nto read, as we're assured that it didn't move in the interim.\n\n> +\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n\nAside from the question of whether we use the repo_*() variant here,\nwhich I noted in my reply to the CL. The cast is suspicious.\n\nSo, in the 1/4 we added this as *unsigned*:\n\t\n\t+\t * If set to 1, skip_hash indicates that we should\n\t+\t * not actually compute the hash for this hashfile and\n\t+\t * instead only use it as a buffered write.\n\t+\t */\n\t+\tunsigned int skip_hash;\n\nBut you need the cast here since the config API can and will set the\n\"dest\" to -1. See the \"*dest == -1\" test in git_configset_get_value().\n\nSo, here we're relying on a \"unsigned int\" cast'd to \"int\" correctly\ndoing the right thing on a \"-1\" assignment.\n\nI'm not sure if that's portable or leads to undefined behavior, but in\nany case, won't such a -1 value be read back as ~0 from that \"unsigned\nint\" variable on most modern platforms?\n\nJust bypassing that entirely and making it \"int\" seems better here, or\nhaving an intermediate variable.\n\nI also wondered if this was all my fault, in your original version you\nwere doing:\n\n\tint skip_hash;\n\t[...]\n\tif (!git_config_get_maybe_bool(\"index.skiphash\", &skip_hash))\n\t\tf->skip_hash = skip_hash;\n\nAnd I suggested that this was redundant, and that you could just write\nto \"f->skip_hash\" directly.\n\nBut I didn't notice it was \"unsigned\", and in any case your original\nversion had the same issue of assigning a -1 to the unsigned variable...\n"},{"id":"469174","messageId":"9e754af8-ecd3-6aed-60e8-2fc09a6a8759@github.com","threadId":"58917","inReplyTo":"221215.865yec3b1j.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-16T13:41:03Z","receivedAt":"2022-12-16T13:41:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/15/22 10:56 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Dec 15 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> Updates since v2\n>> ================\n>>\n>> * Patch 2 now has additional details about why to use the config option in\n>> its message. The discussion about why the index is special is included in\n>> the commit message.\n\n\n> So even if we can't produce a behavior difference, just doing e.g.:\n> \n> \tstruct repository *r = istate->repo ? istate->repo : the_repository;\n> \n> And then using:\n> \n> \tprepare_repo_settings(r);\n> f->skip_hash = r->->settings.index_skip_hash;\n\nThis, and other comments are sensible and will be reflected in v4.\n \n> Seems sensible. I just don't get why 4/4 has that seeming fix-up of 2/4\n> after-the-fact. Isn't it better to carve that bit out of 4/4, just do\n> the config setup in repo-settings.c to begin with, and have 4/4 do what\n> its $subject says, i.e. to have \"feature.manyFiles\" imply this new\n> config?\n\nThe point is to make patch 4 completely independent, and be able to\npull it out if necessary. There's no reason to load the config inside\nrepo-settings.c unless it's part of something like feature.manyFiles.\n\nThanks,\n-Stolee\n"},{"id":"469175","messageId":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v3.git.1671116820.gitgitgadget@gmail.com","subject":"[PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-16T15:31:14Z","receivedAt":"2022-12-16T15:31:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Writing the index is a critical action that takes place in multiple Git\ncommands. The recent performance improvements available with the sparse\nindex show how often the I/O costs around the index can affect different Git\ncommands, although reading the index takes place more often than a write.\n\nThe index is written through the hashfile API, both as a buffered write and\nas a computation of its trailing hash. This trailing hash is an expectation\nof the file format: several optional index extensions are provided using\nindicators immediately preceding the trailing hash. 'git fsck' will complain\nif the trailing hash does not match the contents of the file up to that\npoint.\n\nHowever, computing the hash is expensive. This is even more expensive now\nthat we've moved to sha1dc instead of hardware-accelerated SHA1\ncomputations.\n\nThis series provides a new config option, index.skipHash, that allows users\nto disable computing the hash as Git writes the index. In this case, the\ntrailing hash is stored as the null hash (all bytes are zero).\n\nThe implementation is rather simple, but the patches organize different\naspects in a way that we could split things out:\n\n * Patch 1 adds a 'skip_hash' option to 'struct hashfile' that creates this\n as a general option to the hashwrite API.\n * The motivation in Patch 1 avoids the talk about the chunk-format API and\n instead focuses on the index as the first example that could make use of\n this.\n * Patch 2 creates the index.skipHash config option and updates 'git fsck'\n to ignore a null hash on the index. This is demonstrated by a very simple\n test.\n * Patch 3 creates a test helper to get the trailing hash of a file, and\n adds a concrete check on the trailing hash when index.skipHash=true.\n * Patch 4 (which could be deferred until later) adds index.skipHash=true as\n an implication of feature.manyFiles and as a setting in Scalar.\n\nThe end result is that index writes speed up significantly, enough that 'git\nupdate-index --force-write' improves by 1.75x.\n\nSimilar patches appeared in my earlier refs RFC [1].\n\n[1]\nhttps://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\n\nUpdates since v3\n================\n\n * Patch 1 uses an \"int\" instead of \"unsigned int\". This was a carry-over\n from Kevin's patch in microsoft/git, but our use of it in patch 2 is\n different and thus is better with a signed int.\n * Patch 2 uses a fallback to the_repository if istate->repo is NULL. This\n allows the setting to be applied to more cases where istate->repo is not\n set.\n * Patch 2 also uses 'start' in more places, since it already computed the\n beginning of the hash.\n * Patch 4 slightly modifies its use of prepare_repo_settings() due to Patch\n 2's fallback to the_repository.\n\n\nUpdates since v2\n================\n\n * Patch 2 now has additional details about why to use the config option in\n its message. The discussion about why the index is special is included in\n the commit message.\n\n\nUpdates since v1\n================\n\n * In Patch 1, use hashcler() instead of memset().\n * In Patches 2 and 4, be explicit about which Git versions will exhibit\n different behavior when reading an index with a null trailing hash.\n * In Patch 2, used a more compact way of loading from config.\n * In Patch 2 (and following) dropped the sub-shell in t1600-index.sh.\n * In Patch 3, dropped back-ticks when using a multi-line pipe.\n * In Patch 4, use \"! cmp\" instead of \"! test_cmp\"\n\n\nUpdates since RFC\n=================\n\n * The config name is now index.skipHash to make it more clear that the\n default is to not skip the hash, and choosing this option requires a true\n value.\n * Use oidread() instead of an ad-hoc loop.\n * Patches 3 and 4 are new. 3 adds some extra test verification that wasn't\n in the RFC.\n * I think that patch 4 is helpful to see now, but I could understand if we\n wanted to defer that patch until after index.skipHash has had more time\n to bake.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: allow skipping the hash function\n read-cache: add index.skipHash config option\n test-lib-functions: add helper for trailing hash\n features: feature.manyFiles implies fast index writes\n\n Documentation/config/feature.txt | 5 +++++\n Documentation/config/index.txt | 11 +++++++++++\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n read-cache.c | 14 +++++++++++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 20 ++++++++++++++++++++\n t/test-lib-functions.sh | 8 ++++++++\n 10 files changed, 79 insertions(+), 4 deletions(-)\n\n\nbase-commit: 805265fcf7a737664a8321aaf4a0587b78435184\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1439%2Fderrickstolee%2Fhashfile-skip-hash-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1439/derrickstolee/hashfile-skip-hash-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1439\n\nRange-diff vs v3:\n\n 1: b582d681581 ! 1: c99470d4676 hashfile: allow skipping the hash function\n @@ csum-file.h: struct hashfile {\n \tunsigned char *check_buffer;\n +\n +\t/**\n -+\t * If set to 1, skip_hash indicates that we should\n ++\t * If non-zero, skip_hash indicates that we should\n +\t * not actually compute the hash for this hashfile and\n +\t * instead only use it as a buffered write.\n +\t */\n -+\tunsigned int skip_hash;\n ++\tint skip_hash;\n };\n \n /* Checkpoint */\n 2: 00738c81a12 ! 2: aae047cbc9f read-cache: add index.skipHash config option\n @@ Commit message\n \n [1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n \n + We load this config from the repository config given by istate->repo,\n + with a fallback to the_repository if it is not set.\n +\n While older Git versions will not recognize the null hash as a special\n case, the file format itself is still being met in terms of its\n structure. Using this null hash will still allow Git operations to\n @@ read-cache.c: static int verify_hdr(const struct cache_header *hdr, unsigned lon\n \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n \tthe_hash_algo->final_fn(hash, &c);\n -\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n -+\tif (!hasheq(hash, end - the_hash_algo->rawsz))\n ++\tif (!hasheq(hash, start))\n \t\treturn error(_(\"bad index file sha1 signature\"));\n \treturn 0;\n }\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n + \tint ieot_entries = 1;\n + \tstruct index_entry_offset_table *ieot = NULL;\n + \tint nr, nr_threads;\n ++\tstruct repository *r = istate->repo ? istate->repo : the_repository;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n -+\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n ++\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n +\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n 3: 86370af1351 = 3: 27fbe52e748 test-lib-functions: add helper for trailing hash\n 4: 6490bd445eb ! 4: 075921514f2 features: feature.manyFiles implies fast index writes\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n --\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n -+\tif (istate->repo) {\n -+\t\tprepare_repo_settings(istate->repo);\n -+\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n -+\t}\n +-\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n ++\tprepare_repo_settings(r);\n ++\tf->skip_hash = r->settings.index_skip_hash;\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n\n-- \ngitgitgadget\n"},{"id":"469176","messageId":"c99470d46763cdfbde01629888c379c448b3579d.1671204678.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","subject":"[PATCH v4 1/4] hashfile: allow skipping the hash function","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-16T15:31:15Z","receivedAt":"2022-12-16T15:31:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API is useful for generating files that include a trailing\nhash of the file's contents up to that point. Using such a hash is\nhelpful for verifying the file for corruption-at-rest, such as a faulty\ndrive causing flipped bits.\n\nGit's index file includes this trailing hash, so it uses a 'struct\nhashfile' to handle the I/O to the file. This was very convenient to\nallow using the hashfile methods during these operations.\n\nHowever, hashing the file contents during write comes at a performance\npenalty. It's slower to hash the bytes on their way to the disk than\nwithout that step. This problem is made worse by the replacement of\nhardware-accelerated SHA1 computations with the software-based sha1dc\ncomputation.\n\nThis write cost is significant, and the checksum capability is likely\nnot worth that cost for such a short-lived file. The index is rewritten\nfrequently and the only time the checksum is checked is during 'git\nfsck'. Thus, it would be helpful to allow a user to opt-out of the hash\ncomputation.\n\nWe first need to allow Git to opt-out of the hash computation in the\nhashfile API. The buffered writes of the API are still helpful, so it\nmakes sense to make the change here.\n\nIntroduce a new 'skip_hash' option to 'struct hashfile'. When set, the\nupdate_fn and final_fn members of the_hash_algo are skipped. When\nfinalizing the hashfile, the trailing hash is replaced with the null\nhash.\n\nThis use of a trailing null hash would be desireable in either case,\nsince we do not want to special case a file format to have a different\nlength depending on whether it was hashed or not. When the final bytes\nof a file are all zero, we can infer that it was written without\nhashing, and thus that verification is not available as a check for file\nconsistency. This also means that we could easily toggle hashing for any\nfile format we desire.\n\nA version of this patch has existed in the microsoft/git fork since\n2017 [1] (the linked commit was rebased in 2018, but the original dates\nback to January 2017). Here, the change to make the index use this fast\npath is delayed until a later change.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nCo-authored-by: Kevin Willford \nSigned-off-by: Kevin Willford \nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 59ef3398ca2..cce13c0f047 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -45,7 +45,8 @@ void hashflush(struct hashfile *f)\n \tunsigned offset = f->offset;\n \n \tif (offset) {\n-\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n+\t\tif (!f->skip_hash)\n+\t\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n \t\tflush(f, f->buffer, offset);\n \t\tf->offset = 0;\n \t}\n@@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \tint fd;\n \n \thashflush(f);\n-\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n+\tif (f->skip_hash)\n+\t\thashclr(f->buffer);\n+\telse\n+\t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & CSUM_HASH_IN_STREAM)\n@@ -108,7 +114,8 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\t * the hashfile's buffer. In this block,\n \t\t\t * f->offset is necessarily zero.\n \t\t\t */\n-\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n+\t\t\tif (!f->skip_hash)\n+\t\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n \t\t\tflush(f, buf, nr);\n \t\t} else {\n \t\t\t/*\n@@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name,\n \tf->tp = tp;\n \tf->name = name;\n \tf->do_crc = 0;\n+\tf->skip_hash = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n \n \tf->buffer_len = buffer_len;\ndiff --git a/csum-file.h b/csum-file.h\nindex 0d29f528fbc..793a59da12b 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -20,6 +20,13 @@ struct hashfile {\n \tsize_t buffer_len;\n \tunsigned char *buffer;\n \tunsigned char *check_buffer;\n+\n+\t/**\n+\t * If non-zero, skip_hash indicates that we should\n+\t * not actually compute the hash for this hashfile and\n+\t * instead only use it as a buffered write.\n+\t */\n+\tint skip_hash;\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"469177","messageId":"aae047cbc9f62654c7bd84fbc7ba6c8dfdc3b234.1671204678.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","subject":"[PATCH v4 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-16T15:31:16Z","receivedAt":"2022-12-16T15:31:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change allowed skipping the hashing portion of the\nhashwrite API, using it instead as a buffered write API. Disabling the\nhashwrite can be particularly helpful when the write operation is in a\ncritical path.\n\nOne such critical path is the writing of the index. This operation is so\ncritical that the sparse index was created specifically to reduce the\nsize of the index to make these writes (and reads) faster.\n\nThis trade-off between file stability at rest and write-time performance\nis not easy to balance. The index is an interesting case for a couple\nreasons:\n\n1. Writes block users. Writing the index takes place in many user-\n blocking foreground operations. The speed improvement directly\n impacts their use. Other file formats are typically written in the\n background (commit-graph, multi-pack-index) or are super-critical to\n correctness (pack-files).\n\n2. Index files are short lived. It is rare that a user leaves an index\n for a long time with many staged changes. Outside of staged changes,\n the index can be completely destroyed and rewritten with minimal\n impact to the user.\n\nFollowing a similar approach to one used in the microsoft/git fork [1],\nadd a new config option (index.skipHash) that allows disabling this\nhashing during the index write. The cost is that we can no longer\nvalidate the contents for corruption-at-rest using the trailing hash.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nWe load this config from the repository config given by istate->repo,\nwith a fallback to the_repository if it is not set.\n\nWhile older Git versions will not recognize the null hash as a special\ncase, the file format itself is still being met in terms of its\nstructure. Using this null hash will still allow Git operations to\nfunction across older versions.\n\nThe one exception is 'git fsck' which checks the hash of the index file.\nThis used to be a check on every index read, but was split out to just\nthe index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n2017-04-14) and released first in Git 2.13.0. Document the versions that\nrelaxed these restrictions, with the optimistic expectation that this\nchange will be included in Git 2.40.0.\n\nHere, we disable this check if the trailing hash is all zeroes. We add a\nwarning to the config option that this may cause undesirable behavior\nwith older Git versions.\n\nAs a quick comparison, I tested 'git update-index --force-write' with\nand without index.skipHash=true on a copy of the Linux kernel\nrepository.\n\nBenchmark 1: with hash\n Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n Range (min … max): 34.3 ms … 79.1 ms 82 runs\n\nBenchmark 2: without hash\n Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n Range (min … max): 16.3 ms … 42.0 ms 69 runs\n\nSummary\n 'without hash' ran\n 1.78 ± 0.76 times faster than 'with hash'\n\nThese performance benefits are substantial enough to allow users the\nability to opt-in to this feature, even with the potential confusion\nwith older 'git fsck' versions.\n\nIt is critical that this test is placed before the test_index_version\ntests, since those tests obliterate the .git/config file and hence lose\nthe setting from GIT_TEST_DEFAULT_HASH, if set.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/index.txt | 11 +++++++++++\n read-cache.c | 13 ++++++++++++-\n t/t1600-index.sh | 6 ++++++\n 3 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/index.txt b/Documentation/config/index.txt\nindex 75f3a2d1054..23c7985eb40 100644\n--- a/Documentation/config/index.txt\n+++ b/Documentation/config/index.txt\n@@ -30,3 +30,14 @@ index.version::\n \tSpecify the version with which new index files should be\n \tinitialized. This does not affect existing repositories.\n \tIf `feature.manyFiles` is enabled, then the default is 4.\n+\n+index.skipHash::\n+\tWhen enabled, do not compute the trailing hash for the index file.\n+\tThis accelerates Git commands that manipulate the index, such as\n+\t`git add`, `git commit`, or `git status`. Instead of storing the\n+\tchecksum, write a trailing set of bytes with value zero, indicating\n+\tthat the computation was skipped.\n++\n+If you enable `index.skipHash`, then Git clients older than 2.13.0 will\n+refuse to parse the index and Git clients older than 2.40.0 will report an\n+error during `git fsck`.\ndiff --git a/read-cache.c b/read-cache.c\nindex 46f5e497b14..fde0697873c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tgit_hash_ctx c;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tint hdr_version;\n+\tunsigned char *start, *end;\n+\tstruct object_id oid;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(_(\"bad signature 0x%08x\"), hdr->hdr_signature);\n@@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tif (!verify_index_checksum)\n \t\treturn 0;\n \n+\tend = (unsigned char *)hdr + size;\n+\tstart = end - the_hash_algo->rawsz;\n+\toidread(&oid, start);\n+\tif (oideq(&oid, null_oid()))\n+\t\treturn 0;\n+\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n \tthe_hash_algo->final_fn(hash, &c);\n-\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n+\tif (!hasheq(hash, start))\n \t\treturn error(_(\"bad index file sha1 signature\"));\n \treturn 0;\n }\n@@ -2915,9 +2923,12 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tint ieot_entries = 1;\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n+\tstruct repository *r = istate->repo ? istate->repo : the_repository;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n+\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 010989f90e6..45feb0fc5d8 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -65,6 +65,12 @@ test_expect_success 'out of bounds index.version issues warning' '\n \t)\n '\n \n+test_expect_success 'index.skipHash config option' '\n+\trm -f .git/index &&\n+\tgit -c index.skipHash=true add a &&\n+\tgit fsck\n+'\n+\n test_index_version () {\n \tINDEX_VERSION_CONFIG=$1 &&\n \tFEATURE_MANY_FILES=$2 &&\n-- \ngitgitgadget\n\n"},{"id":"469178","messageId":"27fbe52e748e3a1d637b514d467cc4ec433fbda3.1671204678.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","subject":"[PATCH v4 3/4] test-lib-functions: add helper for trailing hash","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-16T15:31:17Z","receivedAt":"2022-12-16T15:31:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt can be helpful to check that a file format with a trailing hash has a\nspecific hash in the final bytes of a written file. This is made more\napparent by recent changes that allow skipping the hash algorithm and\nwriting a null hash at the end of the file instead.\n\nAdd a new test_trailing_hash helper and use it in t1600 to verify that\nindex.skipHash=true really does skip the hash computation, since\n'git fsck' does not actually verify the hash.\n\nKeep the 'git fsck' call to ensure that any potential future change to\ncheck the index hash does not cause an error in this case.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1600-index.sh | 3 +++\n t/test-lib-functions.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 45feb0fc5d8..55914bc3506 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -68,6 +68,9 @@ test_expect_success 'out of bounds index.version issues warning' '\n test_expect_success 'index.skipHash config option' '\n \trm -f .git/index &&\n \tgit -c index.skipHash=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\techo $(test_oid zero) >expect &&\n+\ttest_cmp expect hash &&\n \tgit fsck\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b32..60308843f8f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1875,3 +1875,11 @@ test_cmp_config_output () {\n \tsort config-actual >sorted-actual &&\n \ttest_cmp sorted-expect sorted-actual\n }\n+\n+# Given a filename, extract its trailing hash as a hex string\n+test_trailing_hash () {\n+\tlocal file=\"$1\" &&\n+\ttail -c $(test_oid rawsz) \"$file\" |\n+\t\ttest-tool hexdump |\n+\t\tsed \"s/ //g\"\n+}\n-- \ngitgitgadget\n\n"},{"id":"469179","messageId":"075921514f23163ce95839ec981d7ad5e070facd.1671204678.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","subject":"[PATCH v4 4/4] features: feature.manyFiles implies fast index writes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-16T15:31:18Z","receivedAt":"2022-12-16T15:31:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe recent addition of the index.skipHash config option allows index\nwrites to speed up by skipping the hash computation for the trailing\nchecksum. This is particularly critical for repositories with many files\nat HEAD, so add this config option to two cases where users in that\nscenario may opt-in to such behavior:\n\n 1. The feature.manyFiles config option enables some options that are\n helpful for repositories with many files at HEAD.\n\n 2. 'scalar register' and 'scalar reconfigure' set config options that\n optimize for large repositories.\n\nIn both of these cases, set index.skipHash=true to gain this\nspeedup. Add tests that demonstrate the proper way that\nindex.skipHash=true can override feature.manyFiles=true.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/feature.txt | 5 +++++\n read-cache.c | 3 ++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 13 ++++++++++++-\n 6 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 95975e50912..e52bc6b8584 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -23,6 +23,11 @@ feature.manyFiles::\n \tworking directory. With many files, commands such as `git status` and\n \t`git checkout` may be slow and these new defaults improve performance:\n +\n+* `index.skipHash=true` speeds up index writes by not computing a trailing\n+ checksum. Note that this will cause Git versions earlier than 2.13.0 to\n+ refuse to parse the index and Git versions earlier than 2.40.0 will report\n+ a corrupted index during `git fsck`.\n++\n * `index.version=4` enables path-prefix compression in the index.\n +\n * `core.untrackedCache=true` enables the untracked cache. This setting assumes\ndiff --git a/read-cache.c b/read-cache.c\nindex fde0697873c..feefa0f68ba 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2927,7 +2927,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n-\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n+\tprepare_repo_settings(r);\n+\tf->skip_hash = r->settings.index_skip_hash;\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 3021921c53d..3dbd3f0e2ec 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -47,6 +47,7 @@ void prepare_repo_settings(struct repository *r)\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\n+\t\tr->settings.index_skip_hash = 1;\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n@@ -61,6 +62,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n+\trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 6c461c5b9de..e8c67ffe165 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -42,6 +42,7 @@ struct repo_settings {\n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \n \tint index_version;\n+\tint index_skip_hash;\n \tenum untracked_cache_setting core_untracked_cache;\n \n \tint pack_use_sparse;\ndiff --git a/scalar.c b/scalar.c\nindex 6c52243cdf1..b49bb8c24ec 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -143,6 +143,7 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"credential.validate\", \"false\", 1 }, /* GCM4W-only */\n \t\t{ \"gc.auto\", \"0\", 1 },\n \t\t{ \"gui.GCWarning\", \"false\", 1 },\n+\t\t{ \"index.skipHash\", \"false\", 1 },\n \t\t{ \"index.threads\", \"true\", 1 },\n \t\t{ \"index.version\", \"4\", 1 },\n \t\t{ \"merge.stat\", \"false\", 1 },\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 55914bc3506..103743a1c7d 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -71,7 +71,18 @@ test_expect_success 'index.skipHash config option' '\n \ttest_trailing_hash .git/index >hash &&\n \techo $(test_oid zero) >expect &&\n \ttest_cmp expect hash &&\n-\tgit fsck\n+\tgit fsck &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\ttest_cmp expect hash &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true \\\n+\t\t-c index.skipHash=false add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\t! cmp expect hash\n '\n \n test_index_version () {\n-- \ngitgitgadget\n"},{"id":"469181","messageId":"221216.86sfhf1gbc.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-16T15:43:41Z","receivedAt":"2022-12-16T16:10:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 16 2022, Derrick Stolee via GitGitGadget wrote:\n\nThanks for the update!\n\n> Updates since v3\n> ================\n>\n> * Patch 1 uses an \"int\" instead of \"unsigned int\". This was a carry-over\n> from Kevin's patch in microsoft/git, but our use of it in patch 2 is\n> different and thus is better with a signed int.\n> * Patch 2 uses a fallback to the_repository if istate->repo is NULL. This\n> allows the setting to be applied to more cases where istate->repo is not\n> set.\n> * Patch 2 also uses 'start' in more places, since it already computed the\n> beginning of the hash.\n> * Patch 4 slightly modifies its use of prepare_repo_settings() due to Patch\n> 2's fallback to the_repository.\n\nIt's good that it's \"int\" now instead of \"unsigned int\", but just doing\nthat search-replacement I think misses the implied (which I really\nshould have made more explicit) question in my v3 feedback\n(https://lore.kernel.org/git/221215.861qp03agm.gmgdl@evledraar.gmail.com/):\nWhat should we do about the -1 state then?\n\nWith your 2/4 here we'll accept e.g.:\n\n\t./git -c index.skipHash=12345 status\n\nAs well as:\n\n\t./git -c index.skipHash=blahblah status\n\nBut with the migration to repo-settings.c we start refusing the latter\nof those, as you inherit a stricture in repo-settings.c.\n\nAside: I think this series would be much more readable if it were just\nsquashed into one patch. 1/4 introduces code, but no way to reach it,\nwith 2/4 we have config, 3/4 adds a test 4/4 tweaks how to\nread/parse/interpret/combine the config.\n\nBut as it is split up the individual steps should make sense. The 2/4\nhere should really just use \"bool\", not \"maybe_bool\" to begin with, no?\n\nAnd part of this in 4/4 is inheriting a non-stricture in\nrepo-settings.c, but for this new config variable that we're introducing\nas only a boolean from day one can't we just die() on anything that's\nnot a boolean?\n\nOn other implied feedback, in\nhttps://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/\nI noted the switching between istate->repo & the_repository, and that\nyou could hit a BUG() (when uncommenting a line in my testing patch on\ntop) if we didn't have istate->repo:\n\nThere was a commit message update for that:\n\n> 2: 00738c81a12 ! 2: aae047cbc9f read-cache: add index.skipHash config option\n> @@ Commit message\n> \n> [1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n> \n> + We load this config from the repository config given by istate->repo,\n> + with a fallback to the_repository if it is not set.\n\nBut I think that really sweeps a potential issue under the rug. What are\nthese cases where we don't get istate->repo, are those expected? Is\npreferring istate->repo always known to be redundant now, and just done\nfor good measure, or are there subtle cases (e.g. when reading\nsubmodules) where we pick the wrong repo's config?\n\nIn that context I'd really like to see some testing of submodules in the\nadded tests, i.e. does this act as we'd like it to when you set the\nconfig as \"true\" in the parent, but \"false\" in the submodule, & the\nother way around? That's a case that should stress this \"the_repository\"\nv.s. \"istate->repo\".\n\n> While older Git versions will not recognize the null hash as a special\n> case, the file format itself is still being met in terms of its\n> structure. Using this null hash will still allow Git operations to\n> @@ read-cache.c: static int verify_hdr(const struct cache_header *hdr, unsigned lon\n> \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n> \tthe_hash_algo->final_fn(hash, &c);\n> -\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n> -+\tif (!hasheq(hash, end - the_hash_algo->rawsz))\n> ++\tif (!hasheq(hash, start))\n> \t\treturn error(_(\"bad index file sha1 signature\"));\n> \treturn 0;\n\nThanks, good to have this suggested simplification.\n\n> @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> + \tint ieot_entries = 1;\n> + \tstruct index_entry_offset_table *ieot = NULL;\n> + \tint nr, nr_threads;\n> ++\tstruct repository *r = istate->repo ? istate->repo : the_repository;\n> \n> \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n> \n> -+\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n> ++\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n> +\n> \tfor (i = removed = extended = 0; i < entries; i++) {\n\nRe the above this looks good, but did we introduce an untested behavior\nchange here or not?\n\n> \t\tif (cache[i]->ce_flags & CE_REMOVE)\n> 3: 86370af1351 = 3: 27fbe52e748 test-lib-functions: add helper for trailing hash\n> 4: 6490bd445eb ! 4: 075921514f2 features: feature.manyFiles implies fast index writes\n> @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n> \n> \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n> \n> --\tgit_config_get_maybe_bool(\"index.skiphash\", (int *)&f->skip_hash);\n> -+\tif (istate->repo) {\n> -+\t\tprepare_repo_settings(istate->repo);\n> -+\t\tf->skip_hash = istate->repo->settings.index_skip_hash;\n> -+\t}\n> +-\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n> ++\tprepare_repo_settings(r);\n> ++\tf->skip_hash = r->settings.index_skip_hash;\n> \n> \tfor (i = removed = extended = 0; i < entries; i++) {\n> \t\tif (cache[i]->ce_flags & CE_REMOVE)\n\nI really don't care per-se where we read the config, as long as it's\ndoing what we expect from the UX point of view.\n\nBut in your\nhttps://lore.kernel.org/git/9e754af8-ecd3-6aed-60e8-2fc09a6a8759@github.com/\nyou noted \"There's no reason to load the config inside repo-settings.c\nunless it's part of something like feature.manyFiles.\".\n\nI think that's true, but on the flip side of that: Is there a reason to\nmove the reading of such localized config (only needed in read-cache.c,\nas 2/4 shows) to repo-settings.c, just because it's now relying on the\n\"manyfiles\"?\n\nI think the unstated reason for that is that while we read the\n\"manyfiles\" config we didn't stick it in the \"struct repo_settings\",\ntherefore the reading of it is conveniently done in repo-settings.c,\nwhich the 4/4 here does.\n\nBut I think it makes more sense to just stick the \"manyfiles\" in that\nstruct, not stick \"skip_hash\" there, and then just check the \"manyfiles\"\nin 4/4, but read \"index.skiphash\" locally.\n\nMaybe not worth the churn at this point, but I do wonder if we're going\nin the wrong direction here, i.e. if all config that happens to rely on\nfeatures must be added to repo-settings.c, or whether we should reserve\nthat for truly \"global\" (or mostly global) config.\n\n\n"},{"id":"469232","messageId":"20221217173758.GB3411@szeder.dev","threadId":"58917","inReplyTo":"xmqqh6y0qfzg.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] test-lib-functions: add helper for trailing hash","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-12-17T17:37:58Z","receivedAt":"2022-12-17T17:38:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Dec 13, 2022 at 09:55:47AM +0900, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> >> +\ttest_trailing_hash .git/index >hash &&\n> >> +\techo $(test_oid zero) >expect &&\n> >\n> > Nit: test_oid zero >expect\n> >\n> >> +\ttest_cmp expect hash &&\n> \n> Unfortunately they are not equivalent.\n> \n> Usually we write these helpers to terminate their output with LF,\n> relying on the fact that terminating LF will be dropped when used in\n> a command substition, e.g. VAR=$(HELPER), but test_oid deviates from\n> the pattern and does not give the terminating LF to its output.\n\nOh, indeed. But why does it omit the trailing LF?! Alas, 2c02b110da\n(t: add test functions to translate hash-related values, 2018-09-13)\ndoesn't seem to mention anything about it. However, skimming through\nthe output of\n\n git grep 'test_oid ' -- ':/t/*.sh'\n\nit appears that all but three uses of 'test_oid' are in command\nsubstitutions, and those three exceptions are in t0000 checking that\n'test_oid' actually works. So I don't see any benefit of omitting\nthat trailing LF, but this and similar cases show its drawbacks.\n\n $ git grep 'echo $(test_oid ' -- ':/t/*.sh'\n t/t1302-repo-version.sh: echo $(test_oid version) >expect &&\n t/t5313-pack-bounds-checks.sh: echo $(test_oid oidfff) >file &&\n\n\n"},{"id":"469837","messageId":"b2164be2-72e9-cee5-26db-3e4fbfec3051@github.com","threadId":"58917","inReplyTo":"221216.86sfhf1gbc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-06T15:33:09Z","receivedAt":"2023-01-06T15:33:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/16/2022 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n\n> But as it is split up the individual steps should make sense. The 2/4\n> here should really just use \"bool\", not \"maybe_bool\" to begin with, no?\n \n> And part of this in 4/4 is inheriting a non-stricture in\n> repo-settings.c, but for this new config variable that we're introducing\n> as only a boolean from day one can't we just die() on anything that's\n> not a boolean?\n> \n> On other implied feedback, in\n> https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/\n> I noted the switching between istate->repo & the_repository, and that\n> you could hit a BUG() (when uncommenting a line in my testing patch on\n> top) if we didn't have istate->repo:\n> \n> There was a commit message update for that:\n> \n>> 2: 00738c81a12 ! 2: aae047cbc9f read-cache: add index.skipHash config option\n>> @@ Commit message\n>> \n>> [1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n>> \n>> + We load this config from the repository config given by istate->repo,\n>> + with a fallback to the_repository if it is not set.\n> \n> But I think that really sweeps a potential issue under the rug. What are\n> these cases where we don't get istate->repo, are those expected? Is\n> preferring istate->repo always known to be redundant now, and just done\n> for good measure, or are there subtle cases (e.g. when reading\n> submodules) where we pick the wrong repo's config?\n\nAfter investigating some of the failures from creating a BUG() statement\nwhen istate->repo is NULL I see several problems, and they are not related\nto submodules for the most part.\n\nThe first issues I've found are due to code paths that operate on the_index\nwithout actually initializing it with a do_read_index(), or otherwise\ninitialize an index using a memset() instead of a common initializer. This\nlooks to be a frequent enough problem that solving it would require a\nsignificant effort. It's not a quick fix.\n\n> In that context I'd really like to see some testing of submodules in the\n> added tests, i.e. does this act as we'd like it to when you set the\n> config as \"true\" in the parent, but \"false\" in the submodule, & the\n> other way around? That's a case that should stress this \"the_repository\"\n> v.s. \"istate->repo\".\n\nI can add a test for this, though we do not do that for every new config\noption. Further, we can only rely on commands like 'git add' that correctly\ninitialize the index from a do_read_index(). It should be sufficient to\nuse the index-local repository (when available) and trust that the\nrepo_config_...() method is doing the right thing.\n\n> I really don't care per-se where we read the config, as long as it's\n> doing what we expect from the UX point of view.\n> \n> But in your\n> https://lore.kernel.org/git/9e754af8-ecd3-6aed-60e8-2fc09a6a8759@github.com/\n> you noted \"There's no reason to load the config inside repo-settings.c\n> unless it's part of something like feature.manyFiles.\".\n> \n> I think that's true, but on the flip side of that: Is there a reason to\n> move the reading of such localized config (only needed in read-cache.c,\n> as 2/4 shows) to repo-settings.c, just because it's now relying on the\n> \"manyfiles\"?\n\nYes, because it makes it clear what options are part of these meta-config\nkeys. It is better to centralize the parsing instead of having several\ndifferent ad-hoc parsing strategies.\n\nThanks,\n-Stolee\n"},{"id":"469838","messageId":"pull.1439.v5.git.1673022717.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v4.git.1671204678.gitgitgadget@gmail.com","subject":"[PATCH v5 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-06T16:31:52Z","receivedAt":"2023-01-06T16:32:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Writing the index is a critical action that takes place in multiple Git\ncommands. The recent performance improvements available with the sparse\nindex show how often the I/O costs around the index can affect different Git\ncommands, although reading the index takes place more often than a write.\n\nThe index is written through the hashfile API, both as a buffered write and\nas a computation of its trailing hash. This trailing hash is an expectation\nof the file format: several optional index extensions are provided using\nindicators immediately preceding the trailing hash. 'git fsck' will complain\nif the trailing hash does not match the contents of the file up to that\npoint.\n\nHowever, computing the hash is expensive. This is even more expensive now\nthat we've moved to sha1dc instead of hardware-accelerated SHA1\ncomputations.\n\nThis series provides a new config option, index.skipHash, that allows users\nto disable computing the hash as Git writes the index. In this case, the\ntrailing hash is stored as the null hash (all bytes are zero).\n\nThe implementation is rather simple, but the patches organize different\naspects in a way that we could split things out:\n\n * Patch 1 adds a 'skip_hash' option to 'struct hashfile' that creates this\n as a general option to the hashwrite API.\n * The motivation in Patch 1 avoids the talk about the chunk-format API and\n instead focuses on the index as the first example that could make use of\n this.\n * Patch 2 creates the index.skipHash config option and updates 'git fsck'\n to ignore a null hash on the index. This is demonstrated by a very simple\n test.\n * Patch 3 creates a test helper to get the trailing hash of a file, and\n adds a concrete check on the trailing hash when index.skipHash=true.\n * Patch 4 (which could be deferred until later) adds index.skipHash=true as\n an implication of feature.manyFiles and as a setting in Scalar.\n\nThe end result is that index writes speed up significantly, enough that 'git\nupdate-index --force-write' improves by 1.75x.\n\nSimilar patches appeared in my earlier refs RFC [1].\n\n[1]\nhttps://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\n\nUpdates since v4\n================\n\n * Patch 2 now uses repo_config_get_bool() instead of\n repo_config_get_maybe_bool().\n * A test is added for submodule-level config.\n\n\nUpdates since v3\n================\n\n * Patch 1 uses an \"int\" instead of \"unsigned int\". This was a carry-over\n from Kevin's patch in microsoft/git, but our use of it in patch 2 is\n different and thus is better with a signed int.\n * Patch 2 uses a fallback to the_repository if istate->repo is NULL. This\n allows the setting to be applied to more cases where istate->repo is not\n set.\n * Patch 2 also uses 'start' in more places, since it already computed the\n beginning of the hash.\n * Patch 4 slightly modifies its use of prepare_repo_settings() due to Patch\n 2's fallback to the_repository.\n\n\nUpdates since v2\n================\n\n * Patch 2 now has additional details about why to use the config option in\n its message. The discussion about why the index is special is included in\n the commit message.\n\n\nUpdates since v1\n================\n\n * In Patch 1, use hashcler() instead of memset().\n * In Patches 2 and 4, be explicit about which Git versions will exhibit\n different behavior when reading an index with a null trailing hash.\n * In Patch 2, used a more compact way of loading from config.\n * In Patch 2 (and following) dropped the sub-shell in t1600-index.sh.\n * In Patch 3, dropped back-ticks when using a multi-line pipe.\n * In Patch 4, use \"! cmp\" instead of \"! test_cmp\"\n\n\nUpdates since RFC\n=================\n\n * The config name is now index.skipHash to make it more clear that the\n default is to not skip the hash, and choosing this option requires a true\n value.\n * Use oidread() instead of an ad-hoc loop.\n * Patches 3 and 4 are new. 3 adds some extra test verification that wasn't\n in the RFC.\n * I think that patch 4 is helpful to see now, but I could understand if we\n wanted to defer that patch until after index.skipHash has had more time\n to bake.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: allow skipping the hash function\n read-cache: add index.skipHash config option\n test-lib-functions: add helper for trailing hash\n features: feature.manyFiles implies fast index writes\n\n Documentation/config/feature.txt | 5 +++++\n Documentation/config/index.txt | 11 +++++++++++\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n read-cache.c | 14 +++++++++++++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 30 ++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 8 ++++++++\n 10 files changed, 89 insertions(+), 4 deletions(-)\n\n\nbase-commit: 805265fcf7a737664a8321aaf4a0587b78435184\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1439%2Fderrickstolee%2Fhashfile-skip-hash-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1439/derrickstolee/hashfile-skip-hash-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1439\n\nRange-diff vs v4:\n\n 1: c99470d4676 = 1: c99470d4676 hashfile: allow skipping the hash function\n 2: aae047cbc9f ! 2: a3c79308171 read-cache: add index.skipHash config option\n @@ Commit message\n ability to opt-in to this feature, even with the potential confusion\n with older 'git fsck' versions.\n \n + Test this new config option, both at a command-line level and within a\n + submodule. The confirmation is currently limited to confirm that 'git\n + fsck' does not complain about the index. Future updates will make this\n + test more robust.\n +\n It is critical that this test is placed before the test_index_version\n tests, since those tests obliterate the .git/config file and hence lose\n the setting from GIT_TEST_DEFAULT_HASH, if set.\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n -+\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n ++\trepo_config_get_bool(r, \"index.skiphash\", &f->skip_hash);\n +\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n @@ t/t1600-index.sh: test_expect_success 'out of bounds index.version issues warnin\n +test_expect_success 'index.skipHash config option' '\n +\trm -f .git/index &&\n +\tgit -c index.skipHash=true add a &&\n -+\tgit fsck\n ++\tgit fsck &&\n ++\n ++\ttest_commit start &&\n ++\tgit -c protocol.file.allow=always submodule add ./ sub &&\n ++\tgit config index.skipHash false &&\n ++\tgit -C sub config index.skipHash true &&\n ++\t>sub/file &&\n ++\tgit -C sub add a &&\n ++\tgit -C sub fsck\n +'\n +\n test_index_version () {\n 3: 27fbe52e748 ! 3: dab9b15de47 test-lib-functions: add helper for trailing hash\n @@ Commit message\n \n Add a new test_trailing_hash helper and use it in t1600 to verify that\n index.skipHash=true really does skip the hash computation, since\n - 'git fsck' does not actually verify the hash.\n + 'git fsck' does not actually verify the hash. This confirms that when\n + the config is disabled explicitly in a super project but enabled in a\n + submodule, then the use of repo_config_get_bool() loads config from the\n + correct repository in the case of 'git add'. There are other cases where\n + istate->repo is NULL and thus this config is loaded instead from\n + the_repository, but that's due to many different code paths initializing\n + index_state structs in their own way.\n \n Keep the 'git fsck' call to ensure that any potential future change to\n check the index hash does not cause an error in this case.\n @@ t/t1600-index.sh: test_expect_success 'out of bounds index.version issues warnin\n +\ttest_trailing_hash .git/index >hash &&\n +\techo $(test_oid zero) >expect &&\n +\ttest_cmp expect hash &&\n - \tgit fsck\n + \tgit fsck &&\n + \n + \ttest_commit start &&\n +@@ t/t1600-index.sh: test_expect_success 'index.skipHash config option' '\n + \tgit -C sub config index.skipHash true &&\n + \t>sub/file &&\n + \tgit -C sub add a &&\n ++\ttest_trailing_hash .git/modules/sub/index >hash &&\n ++\ttest_cmp expect hash &&\n + \tgit -C sub fsck\n '\n \n \n 4: 075921514f2 ! 4: 1beedcb5ba1 features: feature.manyFiles implies fast index writes\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n --\trepo_config_get_maybe_bool(r, \"index.skiphash\", &f->skip_hash);\n +-\trepo_config_get_bool(r, \"index.skiphash\", &f->skip_hash);\n +\tprepare_repo_settings(r);\n +\tf->skip_hash = r->settings.index_skip_hash;\n \n @@ scalar.c: static int set_recommended_config(int reconfigure)\n \n ## t/t1600-index.sh ##\n @@ t/t1600-index.sh: test_expect_success 'index.skipHash config option' '\n - \ttest_trailing_hash .git/index >hash &&\n - \techo $(test_oid zero) >expect &&\n \ttest_cmp expect hash &&\n --\tgit fsck\n -+\tgit fsck &&\n -+\n + \tgit fsck &&\n + \n +\trm -f .git/index &&\n +\tgit -c feature.manyFiles=true add a &&\n +\ttest_trailing_hash .git/index >hash &&\n -+\ttest_cmp expect hash &&\n ++\tcmp expect hash &&\n +\n +\trm -f .git/index &&\n +\tgit -c feature.manyFiles=true \\\n -+\t\t-c index.skipHash=false add a &&\n ++\t -c index.skipHash=false add a &&\n +\ttest_trailing_hash .git/index >hash &&\n -+\t! cmp expect hash\n - '\n - \n - test_index_version () {\n ++\t! cmp expect hash &&\n ++\n + \ttest_commit start &&\n + \tgit -c protocol.file.allow=always submodule add ./ sub &&\n + \tgit config index.skipHash false &&\n\n-- \ngitgitgadget\n"},{"id":"469839","messageId":"c99470d46763cdfbde01629888c379c448b3579d.1673022717.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v5.git.1673022717.gitgitgadget@gmail.com","subject":"[PATCH v5 1/4] hashfile: allow skipping the hash function","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-06T16:31:53Z","receivedAt":"2023-01-06T16:32:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API is useful for generating files that include a trailing\nhash of the file's contents up to that point. Using such a hash is\nhelpful for verifying the file for corruption-at-rest, such as a faulty\ndrive causing flipped bits.\n\nGit's index file includes this trailing hash, so it uses a 'struct\nhashfile' to handle the I/O to the file. This was very convenient to\nallow using the hashfile methods during these operations.\n\nHowever, hashing the file contents during write comes at a performance\npenalty. It's slower to hash the bytes on their way to the disk than\nwithout that step. This problem is made worse by the replacement of\nhardware-accelerated SHA1 computations with the software-based sha1dc\ncomputation.\n\nThis write cost is significant, and the checksum capability is likely\nnot worth that cost for such a short-lived file. The index is rewritten\nfrequently and the only time the checksum is checked is during 'git\nfsck'. Thus, it would be helpful to allow a user to opt-out of the hash\ncomputation.\n\nWe first need to allow Git to opt-out of the hash computation in the\nhashfile API. The buffered writes of the API are still helpful, so it\nmakes sense to make the change here.\n\nIntroduce a new 'skip_hash' option to 'struct hashfile'. When set, the\nupdate_fn and final_fn members of the_hash_algo are skipped. When\nfinalizing the hashfile, the trailing hash is replaced with the null\nhash.\n\nThis use of a trailing null hash would be desireable in either case,\nsince we do not want to special case a file format to have a different\nlength depending on whether it was hashed or not. When the final bytes\nof a file are all zero, we can infer that it was written without\nhashing, and thus that verification is not available as a check for file\nconsistency. This also means that we could easily toggle hashing for any\nfile format we desire.\n\nA version of this patch has existed in the microsoft/git fork since\n2017 [1] (the linked commit was rebased in 2018, but the original dates\nback to January 2017). Here, the change to make the index use this fast\npath is delayed until a later change.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nCo-authored-by: Kevin Willford \nSigned-off-by: Kevin Willford \nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 14 +++++++++++---\n csum-file.h | 7 +++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 59ef3398ca2..cce13c0f047 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -45,7 +45,8 @@ void hashflush(struct hashfile *f)\n \tunsigned offset = f->offset;\n \n \tif (offset) {\n-\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n+\t\tif (!f->skip_hash)\n+\t\t\tthe_hash_algo->update_fn(&f->ctx, f->buffer, offset);\n \t\tflush(f, f->buffer, offset);\n \t\tf->offset = 0;\n \t}\n@@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \tint fd;\n \n \thashflush(f);\n-\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n+\tif (f->skip_hash)\n+\t\thashclr(f->buffer);\n+\telse\n+\t\tthe_hash_algo->final_fn(f->buffer, &f->ctx);\n+\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & CSUM_HASH_IN_STREAM)\n@@ -108,7 +114,8 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\t * the hashfile's buffer. In this block,\n \t\t\t * f->offset is necessarily zero.\n \t\t\t */\n-\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n+\t\t\tif (!f->skip_hash)\n+\t\t\t\tthe_hash_algo->update_fn(&f->ctx, buf, nr);\n \t\t\tflush(f, buf, nr);\n \t\t} else {\n \t\t\t/*\n@@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name,\n \tf->tp = tp;\n \tf->name = name;\n \tf->do_crc = 0;\n+\tf->skip_hash = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n \n \tf->buffer_len = buffer_len;\ndiff --git a/csum-file.h b/csum-file.h\nindex 0d29f528fbc..793a59da12b 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -20,6 +20,13 @@ struct hashfile {\n \tsize_t buffer_len;\n \tunsigned char *buffer;\n \tunsigned char *check_buffer;\n+\n+\t/**\n+\t * If non-zero, skip_hash indicates that we should\n+\t * not actually compute the hash for this hashfile and\n+\t * instead only use it as a buffered write.\n+\t */\n+\tint skip_hash;\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"469840","messageId":"a3c793081714dd39731590f04dcd9189a6f30a38.1673022717.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v5.git.1673022717.gitgitgadget@gmail.com","subject":"[PATCH v5 2/4] read-cache: add index.skipHash config option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-06T16:31:54Z","receivedAt":"2023-01-06T16:32:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change allowed skipping the hashing portion of the\nhashwrite API, using it instead as a buffered write API. Disabling the\nhashwrite can be particularly helpful when the write operation is in a\ncritical path.\n\nOne such critical path is the writing of the index. This operation is so\ncritical that the sparse index was created specifically to reduce the\nsize of the index to make these writes (and reads) faster.\n\nThis trade-off between file stability at rest and write-time performance\nis not easy to balance. The index is an interesting case for a couple\nreasons:\n\n1. Writes block users. Writing the index takes place in many user-\n blocking foreground operations. The speed improvement directly\n impacts their use. Other file formats are typically written in the\n background (commit-graph, multi-pack-index) or are super-critical to\n correctness (pack-files).\n\n2. Index files are short lived. It is rare that a user leaves an index\n for a long time with many staged changes. Outside of staged changes,\n the index can be completely destroyed and rewritten with minimal\n impact to the user.\n\nFollowing a similar approach to one used in the microsoft/git fork [1],\nadd a new config option (index.skipHash) that allows disabling this\nhashing during the index write. The cost is that we can no longer\nvalidate the contents for corruption-at-rest using the trailing hash.\n\n[1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201\n\nWe load this config from the repository config given by istate->repo,\nwith a fallback to the_repository if it is not set.\n\nWhile older Git versions will not recognize the null hash as a special\ncase, the file format itself is still being met in terms of its\nstructure. Using this null hash will still allow Git operations to\nfunction across older versions.\n\nThe one exception is 'git fsck' which checks the hash of the index file.\nThis used to be a check on every index read, but was split out to just\nthe index in a33fc72fe91 (read-cache: force_verify_index_checksum,\n2017-04-14) and released first in Git 2.13.0. Document the versions that\nrelaxed these restrictions, with the optimistic expectation that this\nchange will be included in Git 2.40.0.\n\nHere, we disable this check if the trailing hash is all zeroes. We add a\nwarning to the config option that this may cause undesirable behavior\nwith older Git versions.\n\nAs a quick comparison, I tested 'git update-index --force-write' with\nand without index.skipHash=true on a copy of the Linux kernel\nrepository.\n\nBenchmark 1: with hash\n Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms]\n Range (min … max): 34.3 ms … 79.1 ms 82 runs\n\nBenchmark 2: without hash\n Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms]\n Range (min … max): 16.3 ms … 42.0 ms 69 runs\n\nSummary\n 'without hash' ran\n 1.78 ± 0.76 times faster than 'with hash'\n\nThese performance benefits are substantial enough to allow users the\nability to opt-in to this feature, even with the potential confusion\nwith older 'git fsck' versions.\n\nTest this new config option, both at a command-line level and within a\nsubmodule. The confirmation is currently limited to confirm that 'git\nfsck' does not complain about the index. Future updates will make this\ntest more robust.\n\nIt is critical that this test is placed before the test_index_version\ntests, since those tests obliterate the .git/config file and hence lose\nthe setting from GIT_TEST_DEFAULT_HASH, if set.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/index.txt | 11 +++++++++++\n read-cache.c | 13 ++++++++++++-\n t/t1600-index.sh | 14 ++++++++++++++\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/index.txt b/Documentation/config/index.txt\nindex 75f3a2d1054..23c7985eb40 100644\n--- a/Documentation/config/index.txt\n+++ b/Documentation/config/index.txt\n@@ -30,3 +30,14 @@ index.version::\n \tSpecify the version with which new index files should be\n \tinitialized. This does not affect existing repositories.\n \tIf `feature.manyFiles` is enabled, then the default is 4.\n+\n+index.skipHash::\n+\tWhen enabled, do not compute the trailing hash for the index file.\n+\tThis accelerates Git commands that manipulate the index, such as\n+\t`git add`, `git commit`, or `git status`. Instead of storing the\n+\tchecksum, write a trailing set of bytes with value zero, indicating\n+\tthat the computation was skipped.\n++\n+If you enable `index.skipHash`, then Git clients older than 2.13.0 will\n+refuse to parse the index and Git clients older than 2.40.0 will report an\n+error during `git fsck`.\ndiff --git a/read-cache.c b/read-cache.c\nindex 46f5e497b14..d73a81e41ae 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tgit_hash_ctx c;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tint hdr_version;\n+\tunsigned char *start, *end;\n+\tstruct object_id oid;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(_(\"bad signature 0x%08x\"), hdr->hdr_signature);\n@@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)\n \tif (!verify_index_checksum)\n \t\treturn 0;\n \n+\tend = (unsigned char *)hdr + size;\n+\tstart = end - the_hash_algo->rawsz;\n+\toidread(&oid, start);\n+\tif (oideq(&oid, null_oid()))\n+\t\treturn 0;\n+\n \tthe_hash_algo->init_fn(&c);\n \tthe_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);\n \tthe_hash_algo->final_fn(hash, &c);\n-\tif (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))\n+\tif (!hasheq(hash, start))\n \t\treturn error(_(\"bad index file sha1 signature\"));\n \treturn 0;\n }\n@@ -2915,9 +2923,12 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tint ieot_entries = 1;\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n+\tstruct repository *r = istate->repo ? istate->repo : the_repository;\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n+\trepo_config_get_bool(r, \"index.skiphash\", &f->skip_hash);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 010989f90e6..98c5a83db73 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -65,6 +65,20 @@ test_expect_success 'out of bounds index.version issues warning' '\n \t)\n '\n \n+test_expect_success 'index.skipHash config option' '\n+\trm -f .git/index &&\n+\tgit -c index.skipHash=true add a &&\n+\tgit fsck &&\n+\n+\ttest_commit start &&\n+\tgit -c protocol.file.allow=always submodule add ./ sub &&\n+\tgit config index.skipHash false &&\n+\tgit -C sub config index.skipHash true &&\n+\t>sub/file &&\n+\tgit -C sub add a &&\n+\tgit -C sub fsck\n+'\n+\n test_index_version () {\n \tINDEX_VERSION_CONFIG=$1 &&\n \tFEATURE_MANY_FILES=$2 &&\n-- \ngitgitgadget\n\n"},{"id":"469841","messageId":"dab9b15de4794fd8ed45edff86ba89f4f63a335c.1673022717.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v5.git.1673022717.gitgitgadget@gmail.com","subject":"[PATCH v5 3/4] test-lib-functions: add helper for trailing hash","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-06T16:31:55Z","receivedAt":"2023-01-06T16:32:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt can be helpful to check that a file format with a trailing hash has a\nspecific hash in the final bytes of a written file. This is made more\napparent by recent changes that allow skipping the hash algorithm and\nwriting a null hash at the end of the file instead.\n\nAdd a new test_trailing_hash helper and use it in t1600 to verify that\nindex.skipHash=true really does skip the hash computation, since\n'git fsck' does not actually verify the hash. This confirms that when\nthe config is disabled explicitly in a super project but enabled in a\nsubmodule, then the use of repo_config_get_bool() loads config from the\ncorrect repository in the case of 'git add'. There are other cases where\nistate->repo is NULL and thus this config is loaded instead from\nthe_repository, but that's due to many different code paths initializing\nindex_state structs in their own way.\n\nKeep the 'git fsck' call to ensure that any potential future change to\ncheck the index hash does not cause an error in this case.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1600-index.sh | 5 +++++\n t/test-lib-functions.sh | 8 ++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 98c5a83db73..2f792bb8ffa 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -68,6 +68,9 @@ test_expect_success 'out of bounds index.version issues warning' '\n test_expect_success 'index.skipHash config option' '\n \trm -f .git/index &&\n \tgit -c index.skipHash=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\techo $(test_oid zero) >expect &&\n+\ttest_cmp expect hash &&\n \tgit fsck &&\n \n \ttest_commit start &&\n@@ -76,6 +79,8 @@ test_expect_success 'index.skipHash config option' '\n \tgit -C sub config index.skipHash true &&\n \t>sub/file &&\n \tgit -C sub add a &&\n+\ttest_trailing_hash .git/modules/sub/index >hash &&\n+\ttest_cmp expect hash &&\n \tgit -C sub fsck\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 796093a7b32..60308843f8f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1875,3 +1875,11 @@ test_cmp_config_output () {\n \tsort config-actual >sorted-actual &&\n \ttest_cmp sorted-expect sorted-actual\n }\n+\n+# Given a filename, extract its trailing hash as a hex string\n+test_trailing_hash () {\n+\tlocal file=\"$1\" &&\n+\ttail -c $(test_oid rawsz) \"$file\" |\n+\t\ttest-tool hexdump |\n+\t\tsed \"s/ //g\"\n+}\n-- \ngitgitgadget\n\n"},{"id":"469842","messageId":"1beedcb5ba112b05f6ef9cd306378b140ca73f8c.1673022717.git.gitgitgadget@gmail.com","threadId":"58917","inReplyTo":"pull.1439.v5.git.1673022717.gitgitgadget@gmail.com","subject":"[PATCH v5 4/4] features: feature.manyFiles implies fast index writes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-06T16:31:56Z","receivedAt":"2023-01-06T16:32:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe recent addition of the index.skipHash config option allows index\nwrites to speed up by skipping the hash computation for the trailing\nchecksum. This is particularly critical for repositories with many files\nat HEAD, so add this config option to two cases where users in that\nscenario may opt-in to such behavior:\n\n 1. The feature.manyFiles config option enables some options that are\n helpful for repositories with many files at HEAD.\n\n 2. 'scalar register' and 'scalar reconfigure' set config options that\n optimize for large repositories.\n\nIn both of these cases, set index.skipHash=true to gain this\nspeedup. Add tests that demonstrate the proper way that\nindex.skipHash=true can override feature.manyFiles=true.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/feature.txt | 5 +++++\n read-cache.c | 3 ++-\n repo-settings.c | 2 ++\n repository.h | 1 +\n scalar.c | 1 +\n t/t1600-index.sh | 11 +++++++++++\n 6 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 95975e50912..e52bc6b8584 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -23,6 +23,11 @@ feature.manyFiles::\n \tworking directory. With many files, commands such as `git status` and\n \t`git checkout` may be slow and these new defaults improve performance:\n +\n+* `index.skipHash=true` speeds up index writes by not computing a trailing\n+ checksum. Note that this will cause Git versions earlier than 2.13.0 to\n+ refuse to parse the index and Git versions earlier than 2.40.0 will report\n+ a corrupted index during `git fsck`.\n++\n * `index.version=4` enables path-prefix compression in the index.\n +\n * `core.untrackedCache=true` enables the untracked cache. This setting assumes\ndiff --git a/read-cache.c b/read-cache.c\nindex d73a81e41ae..feefa0f68ba 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2927,7 +2927,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tf = hashfd(tempfile->fd, tempfile->filename.buf);\n \n-\trepo_config_get_bool(r, \"index.skiphash\", &f->skip_hash);\n+\tprepare_repo_settings(r);\n+\tf->skip_hash = r->settings.index_skip_hash;\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 3021921c53d..3dbd3f0e2ec 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -47,6 +47,7 @@ void prepare_repo_settings(struct repository *r)\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\n+\t\tr->settings.index_skip_hash = 1;\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n@@ -61,6 +62,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n+\trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 6c461c5b9de..e8c67ffe165 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -42,6 +42,7 @@ struct repo_settings {\n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \n \tint index_version;\n+\tint index_skip_hash;\n \tenum untracked_cache_setting core_untracked_cache;\n \n \tint pack_use_sparse;\ndiff --git a/scalar.c b/scalar.c\nindex 6c52243cdf1..b49bb8c24ec 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -143,6 +143,7 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"credential.validate\", \"false\", 1 }, /* GCM4W-only */\n \t\t{ \"gc.auto\", \"0\", 1 },\n \t\t{ \"gui.GCWarning\", \"false\", 1 },\n+\t\t{ \"index.skipHash\", \"false\", 1 },\n \t\t{ \"index.threads\", \"true\", 1 },\n \t\t{ \"index.version\", \"4\", 1 },\n \t\t{ \"merge.stat\", \"false\", 1 },\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 2f792bb8ffa..0ebbae13058 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -73,6 +73,17 @@ test_expect_success 'index.skipHash config option' '\n \ttest_cmp expect hash &&\n \tgit fsck &&\n \n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\tcmp expect hash &&\n+\n+\trm -f .git/index &&\n+\tgit -c feature.manyFiles=true \\\n+\t -c index.skipHash=false add a &&\n+\ttest_trailing_hash .git/index >hash &&\n+\t! cmp expect hash &&\n+\n \ttest_commit start &&\n \tgit -c protocol.file.allow=always submodule add ./ sub &&\n \tgit config index.skipHash false &&\n-- \ngitgitgadget\n"},{"id":"469857","messageId":"xmqqmt6vqo2w.fsf@gitster.g","threadId":"58917","inReplyTo":"b2164be2-72e9-cee5-26db-3e4fbfec3051@github.com","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-06T22:45:43Z","receivedAt":"2023-01-06T22:45:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> After investigating some of the failures from creating a BUG() statement\n> when istate->repo is NULL I see several problems, and they are not related\n> to submodules for the most part.\n>\n> The first issues I've found are due to code paths that operate on the_index\n> without actually initializing it with a do_read_index(), or otherwise\n> initialize an index using a memset() instead of a common initializer. This\n> looks to be a frequent enough problem that solving it would require a\n> significant effort. It's not a quick fix.\n\nThanks. It is not entirely unexpected X-<, considering how the\nconnection from the in-core repository and the in-core index has\nbeen developed and evolved. So in short, istate->repo is not yet\nready to be used, until the problems you identified are resolved?\n\nWe probably should start paying down such technical debts. We've\npunted on them too many times, saying \"yes this is klunky but what\nwe have is good enough for adding this feature\", I suspect?\n\nThanks.\n"},{"id":"469859","messageId":"bb48b1e2-819f-8bf3-ef4a-b9e4d23080b2@github.com","threadId":"58917","inReplyTo":"xmqqmt6vqo2w.fsf@gitster.g","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-06T23:40:52Z","receivedAt":"2023-01-06T23:41:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/6/2023 5:45 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> After investigating some of the failures from creating a BUG() statement\n>> when istate->repo is NULL I see several problems, and they are not related\n>> to submodules for the most part.\n>>\n>> The first issues I've found are due to code paths that operate on the_index\n>> without actually initializing it with a do_read_index(), or otherwise\n>> initialize an index using a memset() instead of a common initializer. This\n>> looks to be a frequent enough problem that solving it would require a\n>> significant effort. It's not a quick fix.\n> \n> Thanks. It is not entirely unexpected X-<, considering how the\n> connection from the in-core repository and the in-core index has\n> been developed and evolved. So in short, istate->repo is not yet\n> ready to be used, until the problems you identified are resolved?\n> \n> We probably should start paying down such technical debts. We've\n> punted on them too many times, saying \"yes this is klunky but what\n> we have is good enough for adding this feature\", I suspect?\n\nYes, I'll make note to prioritize this soon.\n\nThanks,\n-Stolee\n"},{"id":"469984","messageId":"230109.86mt6r1v6d.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"bb48b1e2-819f-8bf3-ef4a-b9e4d23080b2@github.com","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-09T17:15:12Z","receivedAt":"2023-01-09T17:20:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 06 2023, Derrick Stolee wrote:\n\n> On 1/6/2023 5:45 PM, Junio C Hamano wrote:\n>> Derrick Stolee writes:\n>> \n>>> After investigating some of the failures from creating a BUG() statement\n>>> when istate->repo is NULL I see several problems, and they are not related\n>>> to submodules for the most part.\n>>>\n>>> The first issues I've found are due to code paths that operate on the_index\n>>> without actually initializing it with a do_read_index(), or otherwise\n>>> initialize an index using a memset() instead of a common initializer. This\n>>> looks to be a frequent enough problem that solving it would require a\n>>> significant effort. It's not a quick fix.\n>> \n>> Thanks. It is not entirely unexpected X-<, considering how the\n>> connection from the in-core repository and the in-core index has\n>> been developed and evolved. So in short, istate->repo is not yet\n>> ready to be used, until the problems you identified are resolved?\n>> \n>> We probably should start paying down such technical debts. We've\n>> punted on them too many times, saying \"yes this is klunky but what\n>> we have is good enough for adding this feature\", I suspect?\n>\n> Yes, I'll make note to prioritize this soon.\n\nI noted in passing in [1] that I had those patches locally, if I'm\nunderstanding you correctly and you're planning to work on changes\nthat'll make \"istate->repo\" always non-NULL.\n\nI've rebased those on top of your \"ds/omit-trailing-hash-in-index\". I'm\nCI-ing those now & hoping to submit them soon (I've had it working for a\nwhile, but there was some interaction with your patches). Preview at\n[2].\n\n1. https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/\n2. https://github.com/avar/git/compare/avar-ds/omit-trailing-hash-in-index...avar/do-not-lazy-populate-istate-repo\n"},{"id":"469993","messageId":"8bb310ff-5dd1-1470-18ac-68b81d2f488c@github.com","threadId":"58917","inReplyTo":"230109.86mt6r1v6d.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-09T18:00:22Z","receivedAt":"2023-01-09T18:02:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/9/2023 12:15 PM, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jan 06 2023, Derrick Stolee wrote:\n>> On 1/6/2023 5:45 PM, Junio C Hamano wrote:\n\n>>> We probably should start paying down such technical debts. We've\n>>> punted on them too many times, saying \"yes this is klunky but what\n>>> we have is good enough for adding this feature\", I suspect?\n>>\n>> Yes, I'll make note to prioritize this soon.\n> \n> I noted in passing in [1] that I had those patches locally, if I'm\n> understanding you correctly and you're planning to work on changes\n> that'll make \"istate->repo\" always non-NULL.\n> \n> I've rebased those on top of your \"ds/omit-trailing-hash-in-index\". I'm\n> CI-ing those now & hoping to submit them soon (I've had it working for a\n> while, but there was some interaction with your patches). Preview at\n> [2].\n> \n> 1. https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/\n> 2. https://github.com/avar/git/compare/avar-ds/omit-trailing-hash-in-index...avar/do-not-lazy-populate-istate-repo\n\nThanks for doing this so I don't need to.\n\nSome quick pre-submission feedback: your \"treewide\" patch [1] is far\ntoo big and doing too much all at once. It's difficult to know why\nyou're doing things when you're doing them, especially the choices\nfor the validate_cache_repo() calls.\n\nIn particular, I noticed that you used \"the_repository\" in some cases\nwhere a local \"struct repository *\" would be better (even if it is\ncurrently pointing to the_repository as in builtin/sparse-checkout.c\nin update_working_directory()). These would be easier to catch in\nsmaller diffs.\n\n[1] https://github.com/avar/git/commit/7732a41800dfc8f5dbf909560615d6048d583ed9\n\nLooking forward to your series.\n\n-Stolee\n"},{"id":"470002","messageId":"230109.86a62r1p38.gmgdl@evledraar.gmail.com","threadId":"58917","inReplyTo":"8bb310ff-5dd1-1470-18ac-68b81d2f488c@github.com","subject":"Re: [PATCH v4 0/4] Optionally skip hashing index on write","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-09T19:22:11Z","receivedAt":"2023-01-09T19:32:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 09 2023, Derrick Stolee wrote:\n\n> On 1/9/2023 12:15 PM, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Jan 06 2023, Derrick Stolee wrote:\n>>> On 1/6/2023 5:45 PM, Junio C Hamano wrote:\n>\n>>>> We probably should start paying down such technical debts. We've\n>>>> punted on them too many times, saying \"yes this is klunky but what\n>>>> we have is good enough for adding this feature\", I suspect?\n>>>\n>>> Yes, I'll make note to prioritize this soon.\n>> \n>> I noted in passing in [1] that I had those patches locally, if I'm\n>> understanding you correctly and you're planning to work on changes\n>> that'll make \"istate->repo\" always non-NULL.\n>> \n>> I've rebased those on top of your \"ds/omit-trailing-hash-in-index\". I'm\n>> CI-ing those now & hoping to submit them soon (I've had it working for a\n>> while, but there was some interaction with your patches). Preview at\n>> [2].\n>> \n>> 1. https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/\n>> 2. https://github.com/avar/git/compare/avar-ds/omit-trailing-hash-in-index...avar/do-not-lazy-populate-istate-repo\n>\n> Thanks for doing this so I don't need to.\n>\n> Some quick pre-submission feedback: your \"treewide\" patch [1] is far\n> too big and doing too much all at once. It's difficult to know why\n> you're doing things when you're doing them, especially the choices\n> for the validate_cache_repo() calls.\n\nYeah, I don't like it either :)\n\nI'm mulling over whether to keep it at all. I.e. the fix to pass the\ndata is relatively small, but most of that patch is validation paranoia.\n\nIt was helpful to write that to validate it for my own sanity, but maybe\nit's not worth keeping it.\n\nUltimately it's just making things BUG(...) during testing that would\notherwise be a NULL-pointer dereference, so I'm currently leaning\ntowards (especially with this early feedback) just skipping it.\n\nNow that we have a SANITIZE=address CI job the error feedback if/when\nthat would happen is arguably worse than the BUG(...) output.\n\n> In particular, I noticed that you used \"the_repository\" in some cases\n> where a local \"struct repository *\" would be better (even if it is\n> currently pointing to the_repository as in builtin/sparse-checkout.c\n> in update_working_directory()). These would be easier to catch in\n> smaller diffs.\n\nYes, definitely. I was careful to use an existing \"struct repository *\"\nin favor of \"the_repository\", but clearly not careful enough!\n\nI'll fix that, and look carefully over the rest before submission in\ncase I've missed other similar cases.\n\n> [1] https://github.com/avar/git/commit/7732a41800dfc8f5dbf909560615d6048d583ed9\n>\n> Looking forward to your series.\n\nThanks, since I sent the above CI passed, so it'll be sooner than later,\nbut I'll massage it a bit per the above before submission.\n"},{"id":"470381","messageId":"xmqq358cgn3s.fsf@gitster.g","threadId":"58917","inReplyTo":"pull.1439.v5.git.1673022717.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/4] Optionally skip hashing index on write","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T09:31:51Z","receivedAt":"2023-01-15T09:31:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Writing the index is a critical action that takes place in multiple Git\n> commands. The recent performance improvements available with the sparse\n> index show how often the I/O costs around the index can affect different Git\n> commands, although reading the index takes place more often than a write.\n\nhttps://github.com/git/git/actions/runs/3922482355/jobs/6705454550#step:4:2006\n\nshows that all other platforms passed but osx-gcc failed one of the\ntests this series added. Could it be that there is something racy\nabout the test (1006.6 if I am not mistaken)?\n\n"},{"id":"470496","messageId":"76204710-356a-2a85-9057-302e6619b9df@github.com","threadId":"58917","inReplyTo":"xmqq358cgn3s.fsf@gitster.g","subject":"Re: [PATCH v5 0/4] Optionally skip hashing index on write","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-17T14:49:27Z","receivedAt":"2023-01-17T14:49:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/15/2023 4:31 AM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> Writing the index is a critical action that takes place in multiple Git\n>> commands. The recent performance improvements available with the sparse\n>> index show how often the I/O costs around the index can affect different Git\n>> commands, although reading the index takes place more often than a write.\n> \n> https://github.com/git/git/actions/runs/3922482355/jobs/6705454550#step:4:2006\n> \n> shows that all other platforms passed but osx-gcc failed one of the\n> tests this series added. Could it be that there is something racy\n> about the test (1006.6 if I am not mistaken)?\n\nYes, you are right. The patch below fixes the problem. Sorry about that!\n\n-Stolee\n \n-- >8 --\n\nFrom 6827205f979ecfa66f4f9edabfb247cff05f0605 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Tue, 17 Jan 2023 09:46:04 -0500\nSubject: [PATCH] t1600: fix racy index.skipHash test\n\nThe test 1600.6 can fail under --stress due to mtime collisions. Most of\nthe tests include a removal of the index file to guarantee that the\nindex is updated. However, the submodule test addded in ee1f0c242ef\n(read-cache: add index.skipHash config option, 2023-01-06) did not\ninclude this removal. Thus, on rare occasions, the test can fail because\nthe index still has a non-null trailing hash, as detected by the helper\nadded in da9acde14ed (test-lib-functions: add helper for trailing hash,\n2023-01-06).\n\nBy removing the submodule's index before the 'git -C sub add a' command,\nwe guarantee that the index is rewritten with the new index.skipHash\nconfig option.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1600-index.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1600-index.sh b/t/t1600-index.sh\nindex 0ebbae13058..9368d82f7d7 100755\n--- a/t/t1600-index.sh\n+++ b/t/t1600-index.sh\n@@ -88,6 +88,7 @@ test_expect_success 'index.skipHash config option' '\n \tgit -c protocol.file.allow=always submodule add ./ sub &&\n \tgit config index.skipHash false &&\n \tgit -C sub config index.skipHash true &&\n+\trm -f .git/modules/sub/index &&\n \t>sub/file &&\n \tgit -C sub add a &&\n \ttest_trailing_hash .git/modules/sub/index >hash &&\n-- \n2.38.0.vfs.0.0\n\n"}]}