{"thread":{"id":"59439","subject":"[PATCH 0/4] Fix a few split-index bugs","startedAt":"2023-03-22T16:01:06Z","lastAt":"2023-03-27T16:43:32Z","messageCount":19,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"473919","messageId":"pull.1497.git.1679500859.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":null,"subject":"[PATCH 0/4] Fix a few split-index bugs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T16:00:55Z","receivedAt":"2023-03-22T16:01:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I received an internal bug report that after upgrading from v2.39.2 to\nv2.40.0, some users ran into the following error message:\n\nBUG: fsmonitor.c:21: fsmonitor_dirty has more entries than the index (57 > 0)\n\n\nIt sounds very much like the report we received in\nhttps://lore.kernel.org/git/CAC7ZvybvykKQyMWcZoKXxFDu_amnkxZCDq2C6KHoyhmHN2tcKw@mail.gmail.com/,\nbut sadly that thread petered out when the reporter stopped being able to\nreproduce the problem.\n\nAfter a few days of investigating, I am convinced that this is due to some\nold bugs, and not actually a regression in v2.40.0 (although I can believe\nthat some improvements in v2.40.0 might make it easier to run into these\nbugs).\n\nThis patch series addresses those bugs.\n\nNote: While the Git maintainer has stated a strong preference to introduce\nregression tests in the same patch that fixes the corresponding regression,\nthis patch series starts with a stand-alone patch that demonstrates a\nproblematic scenario via a new test_expect_failure test case. The reason why\nI specifically split out the test into its own commit is that there is a lot\nof information to unpack in the commit message that is larger than any of\nthe subsequent bug fixes. Besides, it motivates not only the second patch\n(which marks the test case as test_expect_success) but paints the larger\npicture necessary to understand also the need for the remaining two patches.\n\nThis patch series is based on maint-2.37, the oldest maintenance branch it\napplies without merge conflicts. When merging with next, there are only\ntrivial conflicts in unpack-trees.c due to en/dir-api-cleanup where\no->result is now o->internal.result.\n\nJohannes Schindelin (4):\n split-index & fsmonitor: demonstrate a bug\n split-index; stop abusing the `base_oid` to strip the \"link\" extension\n fsmonitor: avoid overriding `cache_changed` bits\n unpack-trees: take care to propagate the split-index flag\n\n fsmonitor.h | 2 +-\n read-cache.c | 37 ++++++++++++++++++++++--------------\n t/t7527-builtin-fsmonitor.sh | 37 ++++++++++++++++++++++++++++++++++++\n unpack-trees.c | 2 ++\n 4 files changed, 63 insertions(+), 15 deletions(-)\n\n\nbase-commit: eb88fe1ff5ceb34845f0919b8bdc60d8a1703cf6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1497%2Fdscho%2Ffix-split-index-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1497/dscho/fix-split-index-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1497\n-- \ngitgitgadget\n"},{"id":"473920","messageId":"c025fccbdde1a4df11ba36552f86369ed74d37d2.1679500859.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.git.1679500859.gitgitgadget@gmail.com","subject":"[PATCH 1/4] split-index & fsmonitor: demonstrate a bug","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T16:00:56Z","receivedAt":"2023-03-22T16:01:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis commit adds a new test case that demonstrates a bug in the\nsplit-index code that is triggered under certain circumstances when the\nFSMonitor is enabled, and its symptom manifests in the form of one of\nthe following error messages:\n\n BUG: fsmonitor.c:20: fsmonitor_dirty has more entries than the index (2 > 1)\n\n BUG: unpack-trees.c:776: pos doesn't point to the first entry of / in index\n\n error: invalid path ''\n error: The following untracked working tree files would be overwritten by reset:\n initial.t\n\nWhich of these error messages appears depends on timing-dependent\nconditions.\n\nTechnically the root cause lies with a bug in the split-index code that\nhas nothing to do with FSMonitor, but for the sake of this new test case\nit was the easiest way to trigger the bug.\n\nThe bug is this: Under specific conditions, Git needs to skip writing\nthe \"link\" extension (which is the index extension containing the\ninformation pertaining to the split-index). To do that, the `base_oid`\nattribute of the `split_index` structure in the in-memory index is\nzeroed out, and `do_write_index()` specifically checks for a \"null\"\n`base_oid` to understand that the \"link\" extension should not be\nwritten. However, this violates the consistency of the in-memory index\nstructure, but that does not cause problems in most cases because the\nprocess exits without using the in-memory index structure anymore,\nanyway.\n\nBut: _When_ the in-memory index is still used (which is the case e.g. in\n`git rebase`), subsequent writes of `the_index` are at risk of writing\nout a bogus index file, one that _should_ have a \"link\" extension but\ndoes not. In many cases, the `SPLIT_INDEX_ORDERED` flag _happens_ to be\nset for subsequent writes, forcing the shared index to be written, which\nre-initializes `base_oid` to a non-bogus state, and all is good.\n\nWhen it is _not_ set, however, all kinds of mayhem ensue, resulting in\nabove-mentioned error messages, and often enough putting worktrees in a\ntotally broken state where the only recourse is to manually delete the\n`index` and the `index.lock` files and then call `git reset` manually.\nNot something to ask users to do.\n\nThe reason why it is comparatively easy to trigger the bug with\nFSMonitor is that there is _another_ bug in the FSMonitor code:\n`mark_fsmonitor_valid()` sets `cache_changed` to 1, i.e. treating that\nvariable as a Boolean. But it is a bit field, and 1 happens to be the\n`SOMETHING_CHANGED` bit that forces the \"link\" extension to be skipped\nwhen writing the index, among other things.\n\n\"Comparatively easy\" is a relative term in this context, for sure. The\nessence of how the new test case triggers the bug is as following:\n\n1. The `git rebase` invocation will first reset the worktree to\n a commit that contains only the `one.t` file, and then execute a\n rebase script that starts with the following commands (commit hashes\n skipped):\n\n label onto\n\n reset initial\n pick two\n label two\n\n reset two\n pick three\n [...]\n\n2. Before executing the `label` command, a split index is written, as\n well as the shared index.\n\n3. The `reset initial` command in the rebase script writes out a new\n split index but skips writing the shared index, as intended.\n\n4. The `pick two` command updates the worktree and refreshes the index,\n marking the `two.t` entry as valid via the FSMonitor, which sets the\n `SOMETHING_CHANGED` bit in `cache_changed`, which in turn causes the\n `base_oid` attribute to be zeroed out and a full (non-split) index\n to be written (making sure _not_ to write the \"link\" extension).\n\n5. Now, the `reset two` command will leave the worktree alone, but\n still write out a new split index, not writing the shared index\n (because `base_oid` is still zeroed out, and there is no index entry\n update requiring it to be written, either).\n\n6. When it is turn to run `pick three`, the index is read, but it is\n too short: It only contains a single entry when there should be two,\n because the \"link\" extension is missing from the written-out index\n file.\n\nThere are three bugs at play, actually, which will be fixed over the\ncourse of the next commits:\n\n- The `base_oid` attribute should not be zeroed out to indicate when\n the \"link\" extension should not be written, as it puts the in-memory\n index structure into an inconsistent state.\n\n- The FSMonitor should not overwrite bits in `cache_changed`.\n\n- The `unpack_trees()` function tries to reuse the `split_index`\n structure from the source index, if any, but does not propagate the\n `SPLIT_INDEX_ORDERED` flag.\n\nWhile a fix for the second bug would let this test case pass, there are\nother conditions where the `SOMETHING_CHANGED` bit is set. Therefore,\nthe bug that most crucially needs to be fixed is the first one.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7527-builtin-fsmonitor.sh | 37 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex d419085379c..cbafdd69602 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -1003,4 +1003,41 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '\n \tegrep \"^event: nfd/d_${utf8_nfc}/?$\" ./unicode.trace\n '\n \n+test_expect_failure 'split-index and FSMonitor work well together' '\n+\tgit init split-index &&\n+\ttest_when_finished \"git -C \\\"$PWD/split-index\\\" \\\n+\t\tfsmonitor--daemon stop\" &&\n+\t(\n+\t\tcd split-index &&\n+\t\tgit config core.splitIndex true &&\n+\t\t# force split-index in most cases\n+\t\tgit config splitIndex.maxPercentChange 99 &&\n+\t\tgit config core.fsmonitor true &&\n+\n+\t\t# Create the following commit topology:\n+\t\t#\n+\t\t# * merge three\n+\t\t# |\\\n+\t\t# | * three\n+\t\t# * | merge two\n+\t\t# |\\|\n+\t\t# | * two\n+\t\t# * | one\n+\t\t# |/\n+\t\t# * 5a5efd7 initial\n+\n+\t\ttest_commit initial &&\n+\t\ttest_commit two &&\n+\t\ttest_commit three &&\n+\t\tgit reset --hard initial &&\n+\t\ttest_commit one &&\n+\t\ttest_tick &&\n+\t\tgit merge two &&\n+\t\ttest_tick &&\n+\t\tgit merge three &&\n+\n+\t\tgit rebase --force-rebase -r one\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"473921","messageId":"f1897b880729b649ab24da14cbc3432d44b7c731.1679500859.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.git.1679500859.gitgitgadget@gmail.com","subject":"[PATCH 2/4] split-index; stop abusing the `base_oid` to strip the \"link\" extension","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T16:00:57Z","receivedAt":"2023-03-22T16:01:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen a split-index is in effect, the `$GIT_DIR/index` file needs to\ncontain a \"link\" extension that contains all the information about the\nsplit-index, including the information about the shared index.\n\nHowever, in some cases Git needs to suppress writing that \"link\"\nextension (i.e. to fall back to writing a full index) even if the\nin-memory index structure _has_ a `split_index` configured. This is the\ncase e.g. when \"too many not shared\" index entries exist.\n\nIn such instances, the current code sets the `base_oid` field of said\n`split_index` structure to all-zero to indicate that `do_write_index()`\nshould skip writing the \"link\" extension.\n\nThis can lead to problems later on, when the in-memory index is still\nused to perform other operations and eventually wants to write a\nsplit-index, detects the presence of the `split_index` and reuses that,\ntoo (under the assumption that it has been initialized correctly and\nstill has a non-null `base_oid`).\n\nLet's stop zeroing out the `base_oid` to indicate that the \"link\"\nextension should not be written.\n\nOne might be tempted to simply call `discard_split_index()` instead,\nunder the assumption that Git decided to write a non-split index and\ntherefore the the `split_index` structure might no longer be wanted.\nHowever, that is not possible because that would release index entries\nin `split_index->base` that are likely to still be in use. Therefore we\ncannot do that.\n\nThe next best thing we _can_ do is to introduce a flag, specifically\nindicating when the \"link\" extension should be skipped. So that's what\nwe do here.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 37 ++++++++++++++++++++++--------------\n t/t7527-builtin-fsmonitor.sh | 2 +-\n 2 files changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b09128b1884..8fcb2d54c05 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2868,6 +2868,12 @@ static int record_ieot(void)\n \treturn !git_config_get_index_threads(&val) && val != 1;\n }\n \n+enum strip_extensions {\n+\tWRITE_ALL_EXTENSIONS = 0,\n+\tSTRIP_ALL_EXTENSIONS = 1,\n+\tSTRIP_LINK_EXTENSION_ONLY = 2\n+};\n+\n /*\n * On success, `tempfile` is closed. If it is the temporary file\n * of a `struct lock_file`, we will therefore effectively perform\n@@ -2876,7 +2882,7 @@ static int record_ieot(void)\n * rely on it.\n */\n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n-\t\t\t int strip_extensions, unsigned flags)\n+\t\t\t enum strip_extensions strip_extensions, unsigned flags)\n {\n \tuint64_t start = getnanotime();\n \tstruct hashfile *f;\n@@ -3045,7 +3051,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\treturn -1;\n \t}\n \n-\tif (!strip_extensions && istate->split_index &&\n+\tif (strip_extensions == WRITE_ALL_EXTENSIONS && istate->split_index &&\n \t !is_null_oid(&istate->split_index->base_oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n@@ -3060,7 +3066,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && !drop_cache_tree && istate->cache_tree) {\n+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && !drop_cache_tree && istate->cache_tree) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\n@@ -3070,7 +3076,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->resolve_undo) {\n+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && istate->resolve_undo) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tresolve_undo_write(&sb, istate->resolve_undo);\n@@ -3081,7 +3087,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->untracked) {\n+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && istate->untracked) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_untracked_extension(&sb, istate->untracked);\n@@ -3092,7 +3098,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->fsmonitor_last_update) {\n+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && istate->fsmonitor_last_update) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_fsmonitor_extension(&sb, istate);\n@@ -3166,8 +3172,14 @@ static int commit_locked_index(struct lock_file *lk)\n \t\treturn commit_lock_file(lk);\n }\n \n+/*\n+ * Write the Git index into a `.lock` file\n+ *\n+ * If `strip_link_extension` is non-zero, avoid writing any \"link\" extension\n+ * (used by the split-index feature).\n+ */\n static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n-\t\t\t\t unsigned flags)\n+\t\t\t\t unsigned flags, int strip_link_extension)\n {\n \tint ret;\n \tint was_full = istate->sparse_index == INDEX_EXPANDED;\n@@ -3185,7 +3197,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \t */\n \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n \t\t\t\t \"%s\", get_lock_file_path(lock));\n-\tret = do_write_index(istate, lock->tempfile, 0, flags);\n+\tret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);\n \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n \t\t\t\t \"%s\", get_lock_file_path(lock));\n \n@@ -3214,7 +3226,7 @@ static int write_split_index(struct index_state *istate,\n {\n \tint ret;\n \tprepare_to_write_split_index(istate);\n-\tret = do_write_locked_index(istate, lock, flags);\n+\tret = do_write_locked_index(istate, lock, flags, 0);\n \tfinish_writing_split_index(istate);\n \treturn ret;\n }\n@@ -3366,9 +3378,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif ((!si && !test_split_index_env) ||\n \t alternate_index_output ||\n \t (istate->cache_changed & ~EXTMASK)) {\n-\t\tif (si)\n-\t\t\toidclr(&si->base_oid);\n-\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\tret = do_write_locked_index(istate, lock, flags, 1);\n \t\tgoto out;\n \t}\n \n@@ -3394,8 +3404,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\t/* Same initial permissions as the main .git/index file */\n \t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n \t\tif (!temp) {\n-\t\t\toidclr(&si->base_oid);\n-\t\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\t\tret = do_write_locked_index(istate, lock, flags, 1);\n \t\t\tgoto out;\n \t\t}\n \t\tret = write_shared_index(istate, &temp, flags);\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex cbafdd69602..9fab9a2ab38 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -1003,7 +1003,7 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '\n \tegrep \"^event: nfd/d_${utf8_nfc}/?$\" ./unicode.trace\n '\n \n-test_expect_failure 'split-index and FSMonitor work well together' '\n+test_expect_success 'split-index and FSMonitor work well together' '\n \tgit init split-index &&\n \ttest_when_finished \"git -C \\\"$PWD/split-index\\\" \\\n \t\tfsmonitor--daemon stop\" &&\n-- \ngitgitgadget\n\n"},{"id":"473922","messageId":"c1c35f0f026993c493eaae3cf643b25c339205c4.1679500859.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.git.1679500859.gitgitgadget@gmail.com","subject":"[PATCH 3/4] fsmonitor: avoid overriding `cache_changed` bits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T16:00:58Z","receivedAt":"2023-03-22T16:01:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs of e636a7b4d030 (read-cache: be specific what part of the index has\nchanged, 2014-06-13), the paradigm `cache_changed = 1` fell out of\nfashion and it became a bit field instead.\n\nThis is important because some bits have specific meaning and should not\nbe unset without care, e.g. `SPLIT_INDEX_ORDERED`.\n\nHowever, b5a816975206 (mark_fsmonitor_valid(): mark the index as changed\nif needed, 2019-05-24) did use the `cache_changed` attribute as if it\nwere a Boolean instead of a bit field.\n\nThat not only would override the `SPLIT_INDEX_ORDERED` bit when marking\nindex entries as valid via the FSMonitor, but worse: it would set the\n`SOMETHING_OTHER` bit (whose value is 1). This means that Git would\nunnecessarily force a full index to be written out when a split index\nwas asked for.\n\nLet's instead use the bit that is specifically intended to indicate\nFSMonitor-triggered changes, allowing the split-index feature to work as\ndesigned.\n\nNoticed-by: Jeff Hostetler \nSigned-off-by: Johannes Schindelin \n---\n fsmonitor.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex edf7ce5203b..778707b131b 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -86,7 +86,7 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache\n \t !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n \t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\treturn;\n-\t\tistate->cache_changed = 1;\n+\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n \t\tce->ce_flags |= CE_FSMONITOR_VALID;\n \t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"473923","messageId":"3963d3e542896b9cdf871dc7ea13330ddac87795.1679500859.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.git.1679500859.gitgitgadget@gmail.com","subject":"[PATCH 4/4] unpack-trees: take care to propagate the split-index flag","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T16:00:59Z","receivedAt":"2023-03-22T16:01:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen copying the `split_index` structure from one index structure to\nanother, we need to propagate the `SPLIT_INDEX_ORDERED` flag, too, if it\nis set, otherwise Git might forget to write the shared index when that\nis actually needed.\n\nIt just so _happens_ that in many instances when `unpack_trees()` is\ncalled, the result causes the shared index to be written anyway, but\nthere are edge cases when that is not so.\n\nSigned-off-by: Johannes Schindelin \n---\n unpack-trees.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 90b92114be8..ca5e47c77c0 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1916,6 +1916,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t * create a new one.\n \t\t */\n \t\to->result.split_index = o->src_index->split_index;\n+\t\tif (o->src_index->cache_changed & SPLIT_INDEX_ORDERED)\n+\t\t\to->result.cache_changed |= SPLIT_INDEX_ORDERED;\n \t\to->result.split_index->refcount++;\n \t} else {\n \t\to->result.split_index = init_split_index(&o->result);\n-- \ngitgitgadget\n"},{"id":"473927","messageId":"xmqqfs9wg3ho.fsf@gitster.g","threadId":"59439","inReplyTo":"pull.1497.git.1679500859.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] Fix a few split-index bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T16:22:43Z","receivedAt":"2023-03-22T16:23:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Note: While the Git maintainer has stated a strong preference to introduce\n> regression tests in the same patch that fixes the corresponding regression,\n> this patch series starts with a stand-alone patch that demonstrates a\n> problematic scenario via a new test_expect_failure test case.\n\nIt is fine, especially to show existing/old bugs that need extensive\nexplanation.\n\n> This patch series is based on maint-2.37, the oldest maintenance branch it\n> applies without merge conflicts. When merging with next, there are only\n> trivial conflicts in unpack-trees.c due to en/dir-api-cleanup where\n> o->result is now o->internal.result.\n\nThanks for digging into old and important case. Maintenance of the\nindex data structure is a crucial part of the health of the system.\n"},{"id":"473963","messageId":"xmqqjzz8cwdp.fsf@gitster.g","threadId":"59439","inReplyTo":"f1897b880729b649ab24da14cbc3432d44b7c731.1679500859.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] split-index; stop abusing the `base_oid` to strip the \"link\" extension","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T21:24:34Z","receivedAt":"2023-03-22T21:24:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When a split-index is in effect, the `$GIT_DIR/index` file needs to\n> contain a \"link\" extension that contains all the information about the\n> split-index, including the information about the shared index.\n> ...\n> Let's stop zeroing out the `base_oid` to indicate that the \"link\"\n> extension should not be written.\n\nNicely explained.\n\n> One might be tempted to simply call `discard_split_index()` instead,\n> under the assumption that Git decided to write a non-split index and\n> therefore the the `split_index` structure might no longer be wanted.\n\n\"the the\".\n\n> +enum strip_extensions {\n> +\tWRITE_ALL_EXTENSIONS = 0,\n> +\tSTRIP_ALL_EXTENSIONS = 1,\n> +\tSTRIP_LINK_EXTENSION_ONLY = 2\n> +};\n\nWe do not need to spell out the specific values for this enum; the\nusers' (i.e. the callers of do_write_index()) sole requirement is\nfor these symbols to have different values.\n\nAlso do we envision that (1) we would need to keep STRIP_LINK_ONLY\nto be with the largest value among the enum values, or (2) we would\nnever add new value to the set? Otherwise let's end the last one\nwith a trailing comma.\n\nLooking at the way strip_extensions variable is used in\ndo_write_index(), an alternative design might be to make it a set of\nbits (e.g. unsigned write_extension) and give one bit to each\nextension. But such a clean-up is better left outside the topic, I\nwould imagine, as we do not have any need to skip an arbitrary set\nof extensions right now.\n\n> +/*\n> + * Write the Git index into a `.lock` file\n> + *\n> + * If `strip_link_extension` is non-zero, avoid writing any \"link\" extension\n> + * (used by the split-index feature).\n> + */\n\nNot exposing \"enum strip_extensions\" to the caller of this function,\nlike this patch does, is probably a very safe and sensible thing to\ndo. We do not have a reason to allow its callers to (perhaps\nmistakenly) pass STRIP_ALL_EXTENSIONS to it.\n\n> static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n> -\t\t\t\t unsigned flags)\n> +\t\t\t\t unsigned flags, int strip_link_extension)\n> {\n> \tint ret;\n> \tint was_full = istate->sparse_index == INDEX_EXPANDED;\n> @@ -3185,7 +3197,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n> \t */\n> \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n> \t\t\t\t \"%s\", get_lock_file_path(lock));\n> -\tret = do_write_index(istate, lock->tempfile, 0, flags);\n> +\tret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);\n> \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n> \t\t\t\t \"%s\", get_lock_file_path(lock));\n> \n\nOK.\n\nVery nicely done.\n"},{"id":"473985","messageId":"80be2220-ae7b-be65-2ab0-787e52169250@jeffhostetler.com","threadId":"59439","inReplyTo":"c025fccbdde1a4df11ba36552f86369ed74d37d2.1679500859.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] split-index & fsmonitor: demonstrate a bug","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-03-23T13:39:44Z","receivedAt":"2023-03-23T13:45:19Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/22/23 12:00 PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> This commit adds a new test case that demonstrates a bug in the\n> split-index code that is triggered under certain circumstances when the\n> FSMonitor is enabled, and its symptom manifests in the form of one of\n> the following error messages:\n> \n> BUG: fsmonitor.c:20: fsmonitor_dirty has more entries than the index (2 > 1)\n> \n> BUG: unpack-trees.c:776: pos doesn't point to the first entry of / in index\n> \n> error: invalid path ''\n> error: The following untracked working tree files would be overwritten by reset:\n> initial.t\n> \n> Which of these error messages appears depends on timing-dependent\n> conditions.\n> \n> Technically the root cause lies with a bug in the split-index code that\n> has nothing to do with FSMonitor, but for the sake of this new test case\n> it was the easiest way to trigger the bug.\n> \n> The bug is this: Under specific conditions, Git needs to skip writing\n> the \"link\" extension (which is the index extension containing the\n> information pertaining to the split-index). To do that, the `base_oid`\n> attribute of the `split_index` structure in the in-memory index is\n> zeroed out, and `do_write_index()` specifically checks for a \"null\"\n> `base_oid` to understand that the \"link\" extension should not be\n> written. However, this violates the consistency of the in-memory index\n> structure, but that does not cause problems in most cases because the\n> process exits without using the in-memory index structure anymore,\n> anyway.\n> \n> But: _When_ the in-memory index is still used (which is the case e.g. in\n> `git rebase`), subsequent writes of `the_index` are at risk of writing\n> out a bogus index file, one that _should_ have a \"link\" extension but\n> does not. In many cases, the `SPLIT_INDEX_ORDERED` flag _happens_ to be\n> set for subsequent writes, forcing the shared index to be written, which\n> re-initializes `base_oid` to a non-bogus state, and all is good.\n> \n> When it is _not_ set, however, all kinds of mayhem ensue, resulting in\n> above-mentioned error messages, and often enough putting worktrees in a\n> totally broken state where the only recourse is to manually delete the\n> `index` and the `index.lock` files and then call `git reset` manually.\n> Not something to ask users to do.\n> \n> The reason why it is comparatively easy to trigger the bug with\n> FSMonitor is that there is _another_ bug in the FSMonitor code:\n> `mark_fsmonitor_valid()` sets `cache_changed` to 1, i.e. treating that\n> variable as a Boolean. But it is a bit field, and 1 happens to be the\n> `SOMETHING_CHANGED` bit that forces the \"link\" extension to be skipped\n> when writing the index, among other things.\n> \n> \"Comparatively easy\" is a relative term in this context, for sure. The\n> essence of how the new test case triggers the bug is as following:\n> \n> 1. The `git rebase` invocation will first reset the worktree to\n> a commit that contains only the `one.t` file, and then execute a\n> rebase script that starts with the following commands (commit hashes\n> skipped):\n> \n> label onto\n> \n> reset initial\n> pick two\n> label two\n> \n> reset two\n> pick three\n> [...]\n> \n> 2. Before executing the `label` command, a split index is written, as\n> well as the shared index.\n> \n> 3. The `reset initial` command in the rebase script writes out a new\n> split index but skips writing the shared index, as intended.\n> \n> 4. The `pick two` command updates the worktree and refreshes the index,\n> marking the `two.t` entry as valid via the FSMonitor, which sets the\n> `SOMETHING_CHANGED` bit in `cache_changed`, which in turn causes the\n> `base_oid` attribute to be zeroed out and a full (non-split) index\n> to be written (making sure _not_ to write the \"link\" extension).\n> \n> 5. Now, the `reset two` command will leave the worktree alone, but\n> still write out a new split index, not writing the shared index\n> (because `base_oid` is still zeroed out, and there is no index entry\n> update requiring it to be written, either).\n> \n> 6. When it is turn to run `pick three`, the index is read, but it is\n> too short: It only contains a single entry when there should be two,\n> because the \"link\" extension is missing from the written-out index\n> file.\n> \n> There are three bugs at play, actually, which will be fixed over the\n> course of the next commits:\n> \n> - The `base_oid` attribute should not be zeroed out to indicate when\n> the \"link\" extension should not be written, as it puts the in-memory\n> index structure into an inconsistent state.\n> \n> - The FSMonitor should not overwrite bits in `cache_changed`.\n> \n> - The `unpack_trees()` function tries to reuse the `split_index`\n> structure from the source index, if any, but does not propagate the\n> `SPLIT_INDEX_ORDERED` flag.\n> \n> While a fix for the second bug would let this test case pass, there are\n> other conditions where the `SOMETHING_CHANGED` bit is set. Therefore,\n> the bug that most crucially needs to be fixed is the first one.\n> \n> Signed-off-by: Johannes Schindelin \n\nVery well said. Thank you!!!\n\n> ---\n> t/t7527-builtin-fsmonitor.sh | 37 ++++++++++++++++++++++++++++++++++++\n> 1 file changed, 37 insertions(+)\n> \n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> index d419085379c..cbafdd69602 100755\n> --- a/t/t7527-builtin-fsmonitor.sh\n> +++ b/t/t7527-builtin-fsmonitor.sh\n> @@ -1003,4 +1003,41 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '\n> \tegrep \"^event: nfd/d_${utf8_nfc}/?$\" ./unicode.trace\n> '\n> \n> +test_expect_failure 'split-index and FSMonitor work well together' '\n> +\tgit init split-index &&\n> +\ttest_when_finished \"git -C \\\"$PWD/split-index\\\" \\\n> +\t\tfsmonitor--daemon stop\" &&\n> +\t(\n> +\t\tcd split-index &&\n> +\t\tgit config core.splitIndex true &&\n> +\t\t# force split-index in most cases\n> +\t\tgit config splitIndex.maxPercentChange 99 &&\n> +\t\tgit config core.fsmonitor true &&\n> +\n> +\t\t# Create the following commit topology:\n> +\t\t#\n> +\t\t# * merge three\n> +\t\t# |\\\n> +\t\t# | * three\n> +\t\t# * | merge two\n> +\t\t# |\\|\n> +\t\t# | * two\n> +\t\t# * | one\n> +\t\t# |/\n> +\t\t# * 5a5efd7 initial\n> +\n> +\t\ttest_commit initial &&\n> +\t\ttest_commit two &&\n> +\t\ttest_commit three &&\n> +\t\tgit reset --hard initial &&\n> +\t\ttest_commit one &&\n> +\t\ttest_tick &&\n> +\t\tgit merge two &&\n> +\t\ttest_tick &&\n> +\t\tgit merge three &&\n> +\n> +\t\tgit rebase --force-rebase -r one\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"473986","messageId":"ba23850b-0fc5-4a82-49df-88550ae8b4ed@jeffhostetler.com","threadId":"59439","inReplyTo":"xmqqjzz8cwdp.fsf@gitster.g","subject":"Re: [PATCH 2/4] split-index; stop abusing the `base_oid` to strip the \"link\" extension","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-03-23T13:59:15Z","receivedAt":"2023-03-23T14:00:07Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/22/23 5:24 PM, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n>> From: Johannes Schindelin \n>>\n>> When a split-index is in effect, the `$GIT_DIR/index` file needs to\n>> contain a \"link\" extension that contains all the information about the\n>> split-index, including the information about the shared index.\n>> ...\n>> Let's stop zeroing out the `base_oid` to indicate that the \"link\"\n>> extension should not be written.\n> \n> Nicely explained.\n> \n>> One might be tempted to simply call `discard_split_index()` instead,\n>> under the assumption that Git decided to write a non-split index and\n>> therefore the the `split_index` structure might no longer be wanted.\n> \n> \"the the\".\n> \n>> +enum strip_extensions {\n>> +\tWRITE_ALL_EXTENSIONS = 0,\n>> +\tSTRIP_ALL_EXTENSIONS = 1,\n>> +\tSTRIP_LINK_EXTENSION_ONLY = 2\n>> +};\n> \n> We do not need to spell out the specific values for this enum; the\n> users' (i.e. the callers of do_write_index()) sole requirement is\n> for these symbols to have different values.\n\nThere are several calls to do_write_locked_index() that pass 0 or 1\nas the new final arg. If we update them to use these enum values,\nthen we don't need integer values here.\n\n> \n> Also do we envision that (1) we would need to keep STRIP_LINK_ONLY\n> to be with the largest value among the enum values, or (2) we would\n> never add new value to the set? Otherwise let's end the last one\n> with a trailing comma.\n> \n> Looking at the way strip_extensions variable is used in\n> do_write_index(), an alternative design might be to make it a set of\n> bits (e.g. unsigned write_extension) and give one bit to each\n> extension. But such a clean-up is better left outside the topic, I\n> would imagine, as we do not have any need to skip an arbitrary set\n> of extensions right now.\n\nAgreed, I thought about suggesting a set of bits too, but right now\nwe only need to strip all of them or just this one.\n\n> \n>> +/*\n>> + * Write the Git index into a `.lock` file\n>> + *\n>> + * If `strip_link_extension` is non-zero, avoid writing any \"link\" extension\n>> + * (used by the split-index feature).\n>> + */\n> \n> Not exposing \"enum strip_extensions\" to the caller of this function,\n> like this patch does, is probably a very safe and sensible thing to\n> do. We do not have a reason to allow its callers to (perhaps\n> mistakenly) pass STRIP_ALL_EXTENSIONS to it.\n> \n>> static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n>> -\t\t\t\t unsigned flags)\n>> +\t\t\t\t unsigned flags, int strip_link_extension)\n>> {\n>> \tint ret;\n>> \tint was_full = istate->sparse_index == INDEX_EXPANDED;\n>> @@ -3185,7 +3197,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n>> \t */\n>> \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n>> \t\t\t\t \"%s\", get_lock_file_path(lock));\n>> -\tret = do_write_index(istate, lock->tempfile, 0, flags);\n>> +\tret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);\n\nIn the else of the ?: operator, could we use the WRITE_ALL_EXTENSIONS\ninstead of 0?\n\n>> \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n>> \t\t\t\t \"%s\", get_lock_file_path(lock));\n>> \n> \n> OK.\n> \n> Very nicely done.\n"},{"id":"473988","messageId":"5a6e83e6-c14b-f2bb-5ec1-669ee7d246c8@jeffhostetler.com","threadId":"59439","inReplyTo":"3963d3e542896b9cdf871dc7ea13330ddac87795.1679500859.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] unpack-trees: take care to propagate the split-index flag","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-03-23T14:32:00Z","receivedAt":"2023-03-23T14:32:09Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/22/23 12:00 PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When copying the `split_index` structure from one index structure to\n> another, we need to propagate the `SPLIT_INDEX_ORDERED` flag, too, if it\n> is set, otherwise Git might forget to write the shared index when that\n> is actually needed.\n> \n> It just so _happens_ that in many instances when `unpack_trees()` is\n> called, the result causes the shared index to be written anyway, but\n> there are edge cases when that is not so.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> unpack-trees.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 90b92114be8..ca5e47c77c0 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1916,6 +1916,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \t\t * create a new one.\n> \t\t */\n> \t\to->result.split_index = o->src_index->split_index;\n> +\t\tif (o->src_index->cache_changed & SPLIT_INDEX_ORDERED)\n> +\t\t\to->result.cache_changed |= SPLIT_INDEX_ORDERED;\n\nNice find!\n\n> \t\to->result.split_index->refcount++;\n> \t} else {\n> \t\to->result.split_index = init_split_index(&o->result);\n"},{"id":"473991","messageId":"aaad07e3-11c9-9bb1-89ec-f9418cb2ef77@jeffhostetler.com","threadId":"59439","inReplyTo":"f1897b880729b649ab24da14cbc3432d44b7c731.1679500859.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] split-index; stop abusing the `base_oid` to strip the \"link\" extension","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-03-23T15:22:00Z","receivedAt":"2023-03-23T15:22:07Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/22/23 12:00 PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When a split-index is in effect, the `$GIT_DIR/index` file needs to\n> contain a \"link\" extension that contains all the information about the\n> split-index, including the information about the shared index.\n> \n> However, in some cases Git needs to suppress writing that \"link\"\n> extension (i.e. to fall back to writing a full index) even if the\n> in-memory index structure _has_ a `split_index` configured. This is the\n> case e.g. when \"too many not shared\" index entries exist.\n> \n> In such instances, the current code sets the `base_oid` field of said\n> `split_index` structure to all-zero to indicate that `do_write_index()`\n> should skip writing the \"link\" extension.\n> \n> This can lead to problems later on, when the in-memory index is still\n> used to perform other operations and eventually wants to write a\n> split-index, detects the presence of the `split_index` and reuses that,\n> too (under the assumption that it has been initialized correctly and\n> still has a non-null `base_oid`).\n> \n> Let's stop zeroing out the `base_oid` to indicate that the \"link\"\n> extension should not be written.\n> \n> One might be tempted to simply call `discard_split_index()` instead,\n> under the assumption that Git decided to write a non-split index and\n> therefore the the `split_index` structure might no longer be wanted.\n> However, that is not possible because that would release index entries\n> in `split_index->base` that are likely to still be in use. Therefore we\n> cannot do that.\n> \n> The next best thing we _can_ do is to introduce a flag, specifically\n> indicating when the \"link\" extension should be skipped. So that's what\n> we do here.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> read-cache.c | 37 ++++++++++++++++++++++--------------\n> t/t7527-builtin-fsmonitor.sh | 2 +-\n> 2 files changed, 24 insertions(+), 15 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index b09128b1884..8fcb2d54c05 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2868,6 +2868,12 @@ static int record_ieot(void)\n> \treturn !git_config_get_index_threads(&val) && val != 1;\n> }\n> \n> +enum strip_extensions {\n> +\tWRITE_ALL_EXTENSIONS = 0,\n> +\tSTRIP_ALL_EXTENSIONS = 1,\n> +\tSTRIP_LINK_EXTENSION_ONLY = 2\n> +};\n\nEarlier (in a response to Junio's response on this commit) I said\nthat I didn't think we needed to make a bit set here, but I want\nto re-think that or at least walk thru the change and talk out loud.\n\nI'll explain in-line below.\n\n> +\n> /*\n> * On success, `tempfile` is closed. If it is the temporary file\n> * of a `struct lock_file`, we will therefore effectively perform\n> @@ -2876,7 +2882,7 @@ static int record_ieot(void)\n> * rely on it.\n> */\n> static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> -\t\t\t int strip_extensions, unsigned flags)\n> +\t\t\t enum strip_extensions strip_extensions, unsigned flags)\n> {\n> \tuint64_t start = getnanotime();\n> \tstruct hashfile *f;\n> @@ -3045,7 +3051,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> \t\t\treturn -1;\n> \t}\n> \n> -\tif (!strip_extensions && istate->split_index &&\n> +\tif (strip_extensions == WRITE_ALL_EXTENSIONS && istate->split_index &&\n> \t !is_null_oid(&istate->split_index->base_oid)) {\n\n(I hate all of this double negative logic...)\n\nHere we only want the extension if we have WRITE_ALL, so that is\nNOT STRIP_ALL and NOT STRIP_LINK_ONLY, so that is OK.\n\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \n> @@ -3060,7 +3066,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> \t\tif (err)\n> \t\t\treturn -1;\n> \t}\n> -\tif (!strip_extensions && !drop_cache_tree && istate->cache_tree) {\n> +\tif (strip_extensions != STRIP_ALL_EXTENSIONS && !drop_cache_tree && istate->cache_tree) {\n\nHere we only want the extension when NOT STRIP_ALL, so this is\neither WRITE_ALL or STRIP_LINK_ONLY, so this is OK. The rest are\nthe same, so I'll omit them.\n\n[...]\n\nAll of this looks correct, but I stumbled over things on my first\nor second reading. I wonder if it would it simplify things to define\nthis as:\n\nenum strip_extensions {\n\tWRITE_ALL_EXTENSIONS = 0,\n\tSTRIP_LINK_EXTENSION = (1<0),\n\tSTRIP_OTHER_EXTENSIONS = (1<1),\n\tSTRIP_ALL_EXTENSIONS = (STRIP_LINK_EXTENSION\n\t\t\t\t | STRIP_OTHER_EXTENSIONS),\n};\n\nThen the link test becomes:\n\tif ( ! (strip_extensions & STRIP_LINK_EXTENSION) &&\n\t istate->split_index &&\n\t ...) {\n\nand the others become:\n\n\tif ( ! (strip_extensions & STRIP_OTHER_EXTENSIONS) &&\n\t ...) {\n\nIf we need to add the ability later to strip an individual,\nwe can easily add a bit to the enum and update the _ALL_ mask\nand the corresponding `if` test.\n\nIn a later commit (probably in another series), we can invert\nthese double negatives to improve readability.\n\n> +/*\n> + * Write the Git index into a `.lock` file\n> + *\n> + * If `strip_link_extension` is non-zero, avoid writing any \"link\" extension\n> + * (used by the split-index feature).\n> + */\n> static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n> -\t\t\t\t unsigned flags)\n> +\t\t\t\t unsigned flags, int strip_link_extension)\n> {\n> \tint ret;\n> \tint was_full = istate->sparse_index == INDEX_EXPANDED;\n> @@ -3185,7 +3197,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n> \t */\n> \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n> \t\t\t\t \"%s\", get_lock_file_path(lock));\n> -\tret = do_write_index(istate, lock->tempfile, 0, flags);\n> +\tret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);\n> \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n> \t\t\t\t \"%s\", get_lock_file_path(lock));\n> \n> @@ -3214,7 +3226,7 @@ static int write_split_index(struct index_state *istate,\n> {\n> \tint ret;\n> \tprepare_to_write_split_index(istate);\n> -\tret = do_write_locked_index(istate, lock, flags);\n> +\tret = do_write_locked_index(istate, lock, flags, 0);\n\ncould we use the enum values here instead of 0 ?\n\n> \tfinish_writing_split_index(istate);\n> \treturn ret;\n> }\n> @@ -3366,9 +3378,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n> \tif ((!si && !test_split_index_env) ||\n> \t alternate_index_output ||\n> \t (istate->cache_changed & ~EXTMASK)) {\n> -\t\tif (si)\n> -\t\t\toidclr(&si->base_oid);\n> -\t\tret = do_write_locked_index(istate, lock, flags);\n> +\t\tret = do_write_locked_index(istate, lock, flags, 1);\n\nand here\n\n> \t\tgoto out;\n> \t}\n> \n> @@ -3394,8 +3404,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n> \t\t/* Same initial permissions as the main .git/index file */\n> \t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n> \t\tif (!temp) {\n> -\t\t\toidclr(&si->base_oid);\n> -\t\t\tret = do_write_locked_index(istate, lock, flags);\n> +\t\t\tret = do_write_locked_index(istate, lock, flags, 1);\n\nand here\n\n> \t\t\tgoto out;\n> \t\t}\n> \t\tret = write_shared_index(istate, &temp, flags);\n> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\n> index cbafdd69602..9fab9a2ab38 100755\n> --- a/t/t7527-builtin-fsmonitor.sh\n> +++ b/t/t7527-builtin-fsmonitor.sh\n> @@ -1003,7 +1003,7 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '\n> \tegrep \"^event: nfd/d_${utf8_nfc}/?$\" ./unicode.trace\n> '\n> \n> -test_expect_failure 'split-index and FSMonitor work well together' '\n> +test_expect_success 'split-index and FSMonitor work well together' '\n> \tgit init split-index &&\n> \ttest_when_finished \"git -C \\\"$PWD/split-index\\\" \\\n> \t\tfsmonitor--daemon stop\" &&\n\nThanks\nJeff\n"},{"id":"473994","messageId":"xmqqh6uba1vs.fsf@gitster.g","threadId":"59439","inReplyTo":"ba23850b-0fc5-4a82-49df-88550ae8b4ed@jeffhostetler.com","subject":"Re: [PATCH 2/4] split-index; stop abusing the `base_oid` to strip the \"link\" extension","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T16:06:15Z","receivedAt":"2023-03-23T16:06:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n>>> +enum strip_extensions {\n>>> +\tWRITE_ALL_EXTENSIONS = 0,\n>>> +\tSTRIP_ALL_EXTENSIONS = 1,\n>>> +\tSTRIP_LINK_EXTENSION_ONLY = 2\n>>> +};\n>> We do not need to spell out the specific values for this enum; the\n>> users' (i.e. the callers of do_write_index()) sole requirement is\n>> for these symbols to have different values.\n>\n> There are several calls to do_write_locked_index() that pass 0 or 1\n> as the new final arg. If we update them to use these enum values,\n> then we don't need integer values here.\n\nGood eyes. Yes, the new caller that selectively passes\nSTRIP_LINK_EXTENSION_ONLY should pass WRITE_ALL_EXTENSIONS, not 0,\non the other side of ?: as you pointed out.\n\nThanks.\n"},{"id":"474206","messageId":"89b3cd9a66823829ce6e425e827b70bc2816d3cd.1679870743.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.v2.git.1679870743.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] fsmonitor: avoid overriding `cache_changed` bits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-26T22:45:42Z","receivedAt":"2023-03-26T22:46:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs of e636a7b4d030 (read-cache: be specific what part of the index has\nchanged, 2014-06-13), the paradigm `cache_changed = 1` fell out of\nfashion and it became a bit field instead.\n\nThis is important because some bits have specific meaning and should not\nbe unset without care, e.g. `SPLIT_INDEX_ORDERED`.\n\nHowever, b5a816975206 (mark_fsmonitor_valid(): mark the index as changed\nif needed, 2019-05-24) did use the `cache_changed` attribute as if it\nwere a Boolean instead of a bit field.\n\nThat not only would override the `SPLIT_INDEX_ORDERED` bit when marking\nindex entries as valid via the FSMonitor, but worse: it would set the\n`SOMETHING_OTHER` bit (whose value is 1). This means that Git would\nunnecessarily force a full index to be written out when a split index\nwas asked for.\n\nLet's instead use the bit that is specifically intended to indicate\nFSMonitor-triggered changes, allowing the split-index feature to work as\ndesigned.\n\nNoticed-by: Jeff Hostetler \nSigned-off-by: Johannes Schindelin \n---\n fsmonitor.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex edf7ce5203b..778707b131b 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -86,7 +86,7 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache\n \t !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n \t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\treturn;\n-\t\tistate->cache_changed = 1;\n+\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n \t\tce->ce_flags |= CE_FSMONITOR_VALID;\n \t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"474207","messageId":"8cc075f63252206972880d0167b694ed86ec0311.1679870743.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.v2.git.1679870743.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] split-index; stop abusing the `base_oid` to strip the \"link\" extension","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-26T22:45:41Z","receivedAt":"2023-03-26T22:46:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen a split-index is in effect, the `$GIT_DIR/index` file needs to\ncontain a \"link\" extension that contains all the information about the\nsplit-index, including the information about the shared index.\n\nHowever, in some cases Git needs to suppress writing that \"link\"\nextension (i.e. to fall back to writing a full index) even if the\nin-memory index structure _has_ a `split_index` configured. This is the\ncase e.g. when \"too many not shared\" index entries exist.\n\nIn such instances, the current code sets the `base_oid` field of said\n`split_index` structure to all-zero to indicate that `do_write_index()`\nshould skip writing the \"link\" extension.\n\nThis can lead to problems later on, when the in-memory index is still\nused to perform other operations and eventually wants to write a\nsplit-index, detects the presence of the `split_index` and reuses that,\ntoo (under the assumption that it has been initialized correctly and\nstill has a non-null `base_oid`).\n\nLet's stop zeroing out the `base_oid` to indicate that the \"link\"\nextension should not be written.\n\nOne might be tempted to simply call `discard_split_index()` instead,\nunder the assumption that Git decided to write a non-split index and\ntherefore the `split_index` structure might no longer be wanted.\nHowever, that is not possible because that would release index entries\nin `split_index->base` that are likely to still be in use. Therefore we\ncannot do that.\n\nThe next best thing we _can_ do is to introduce a bit field to indicate\nspecifically which index extensions (not) to write. So that's what we do\nhere.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 49 +++++++++++++++++++++++-------------\n t/t7527-builtin-fsmonitor.sh | 2 +-\n 2 files changed, 33 insertions(+), 18 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b09128b1884..7c9a0eeeac8 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2868,6 +2868,16 @@ static int record_ieot(void)\n \treturn !git_config_get_index_threads(&val) && val != 1;\n }\n \n+enum write_extensions {\n+\tWRITE_NO_EXTENSION = 0,\n+\tWRITE_SPLIT_INDEX_EXTENSION = 1<<0,\n+\tWRITE_CACHE_TREE_EXTENSION = 1<<1,\n+\tWRITE_RESOLVE_UNDO_EXTENSION = 1<<2,\n+\tWRITE_UNTRACKED_CACHE_EXTENSION = 1<<3,\n+\tWRITE_FSMONITOR_EXTENSION = 1<<4,\n+};\n+#define WRITE_ALL_EXTENSIONS ((enum write_extensions)-1)\n+\n /*\n * On success, `tempfile` is closed. If it is the temporary file\n * of a `struct lock_file`, we will therefore effectively perform\n@@ -2876,7 +2886,7 @@ static int record_ieot(void)\n * rely on it.\n */\n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n-\t\t\t int strip_extensions, unsigned flags)\n+\t\t\t enum write_extensions write_extensions, unsigned flags)\n {\n \tuint64_t start = getnanotime();\n \tstruct hashfile *f;\n@@ -3045,8 +3055,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\treturn -1;\n \t}\n \n-\tif (!strip_extensions && istate->split_index &&\n-\t !is_null_oid(&istate->split_index->base_oid)) {\n+\tif (write_extensions & WRITE_SPLIT_INDEX_EXTENSION &&\n+\t istate->split_index) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tif (istate->sparse_index)\n@@ -3060,7 +3070,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && !drop_cache_tree && istate->cache_tree) {\n+\tif (write_extensions & WRITE_CACHE_TREE_EXTENSION &&\n+\t !drop_cache_tree && istate->cache_tree) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\n@@ -3070,7 +3081,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->resolve_undo) {\n+\tif (write_extensions & WRITE_RESOLVE_UNDO_EXTENSION &&\n+\t istate->resolve_undo) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tresolve_undo_write(&sb, istate->resolve_undo);\n@@ -3081,7 +3093,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->untracked) {\n+\tif (write_extensions & WRITE_UNTRACKED_CACHE_EXTENSION &&\n+\t istate->untracked) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_untracked_extension(&sb, istate->untracked);\n@@ -3092,7 +3105,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->fsmonitor_last_update) {\n+\tif (write_extensions & WRITE_FSMONITOR_EXTENSION &&\n+\t istate->fsmonitor_last_update) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_fsmonitor_extension(&sb, istate);\n@@ -3166,8 +3180,10 @@ static int commit_locked_index(struct lock_file *lk)\n \t\treturn commit_lock_file(lk);\n }\n \n-static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n-\t\t\t\t unsigned flags)\n+static int do_write_locked_index(struct index_state *istate,\n+\t\t\t\t struct lock_file *lock,\n+\t\t\t\t unsigned flags,\n+\t\t\t\t enum write_extensions write_extensions)\n {\n \tint ret;\n \tint was_full = istate->sparse_index == INDEX_EXPANDED;\n@@ -3185,7 +3201,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \t */\n \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n \t\t\t\t \"%s\", get_lock_file_path(lock));\n-\tret = do_write_index(istate, lock->tempfile, 0, flags);\n+\tret = do_write_index(istate, lock->tempfile, write_extensions, flags);\n \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n \t\t\t\t \"%s\", get_lock_file_path(lock));\n \n@@ -3214,7 +3230,7 @@ static int write_split_index(struct index_state *istate,\n {\n \tint ret;\n \tprepare_to_write_split_index(istate);\n-\tret = do_write_locked_index(istate, lock, flags);\n+\tret = do_write_locked_index(istate, lock, flags, WRITE_ALL_EXTENSIONS);\n \tfinish_writing_split_index(istate);\n \treturn ret;\n }\n@@ -3289,7 +3305,7 @@ static int write_shared_index(struct index_state *istate,\n \n \ttrace2_region_enter_printf(\"index\", \"shared/do_write_index\",\n \t\t\t\t the_repository, \"%s\", get_tempfile_path(*temp));\n-\tret = do_write_index(si->base, *temp, 1, flags);\n+\tret = do_write_index(si->base, *temp, WRITE_NO_EXTENSION, flags);\n \ttrace2_region_leave_printf(\"index\", \"shared/do_write_index\",\n \t\t\t\t the_repository, \"%s\", get_tempfile_path(*temp));\n \n@@ -3366,9 +3382,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif ((!si && !test_split_index_env) ||\n \t alternate_index_output ||\n \t (istate->cache_changed & ~EXTMASK)) {\n-\t\tif (si)\n-\t\t\toidclr(&si->base_oid);\n-\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\tret = do_write_locked_index(istate, lock, flags,\n+\t\t\t\t\t ~WRITE_SPLIT_INDEX_EXTENSION);\n \t\tgoto out;\n \t}\n \n@@ -3394,8 +3409,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\t/* Same initial permissions as the main .git/index file */\n \t\ttemp = mks_tempfile_sm(git_path(\"sharedindex_XXXXXX\"), 0, 0666);\n \t\tif (!temp) {\n-\t\t\toidclr(&si->base_oid);\n-\t\t\tret = do_write_locked_index(istate, lock, flags);\n+\t\t\tret = do_write_locked_index(istate, lock, flags,\n+\t\t\t\t\t\t ~WRITE_SPLIT_INDEX_EXTENSION);\n \t\t\tgoto out;\n \t\t}\n \t\tret = write_shared_index(istate, &temp, flags);\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex cbafdd69602..9fab9a2ab38 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -1003,7 +1003,7 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '\n \tegrep \"^event: nfd/d_${utf8_nfc}/?$\" ./unicode.trace\n '\n \n-test_expect_failure 'split-index and FSMonitor work well together' '\n+test_expect_success 'split-index and FSMonitor work well together' '\n \tgit init split-index &&\n \ttest_when_finished \"git -C \\\"$PWD/split-index\\\" \\\n \t\tfsmonitor--daemon stop\" &&\n-- \ngitgitgadget\n\n"},{"id":"474208","messageId":"pull.1497.v2.git.1679870743.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.git.1679500859.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Fix a few split-index bugs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-26T22:45:39Z","receivedAt":"2023-03-26T22:46:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I received an internal bug report that after upgrading from v2.39.2 to\nv2.40.0, some users ran into the following error message:\n\nBUG: fsmonitor.c:21: fsmonitor_dirty has more entries than the index (57 > 0)\n\n\nIt sounds very much like the report we received in\nhttps://lore.kernel.org/git/CAC7ZvybvykKQyMWcZoKXxFDu_amnkxZCDq2C6KHoyhmHN2tcKw@mail.gmail.com/,\nbut sadly that thread petered out when the reporter stopped being able to\nreproduce the problem.\n\nAfter a few days of investigating, I am convinced that this is due to some\nold bugs, and not actually a regression in v2.40.0 (although I can believe\nthat some improvements in v2.40.0 might make it easier to run into these\nbugs).\n\nThis patch series addresses those bugs.\n\nNote: While the Git maintainer has stated a strong preference to introduce\nregression tests in the same patch that fixes the corresponding regression,\nthis patch series starts with a stand-alone patch that demonstrates a\nproblematic scenario via a new test_expect_failure test case. The reason why\nI specifically split out the test into its own commit is that there is a lot\nof information to unpack in the commit message that is larger than any of\nthe subsequent bug fixes. Besides, it motivates not only the second patch\n(which marks the test case as test_expect_success) but paints the larger\npicture necessary to understand also the need for the remaining two patches.\n\nThis patch series is based on maint-2.37, the oldest maintenance branch it\napplies without merge conflicts. When merging with next, there are only\ntrivial conflicts in unpack-trees.c due to en/dir-api-cleanup where\no->result is now o->internal.result.\n\nChanges since v1:\n\n * Fix a double \"the\" in a commit message\n * Replace enum strip_extensions by the bit field enum write_extensions,\n inverting the meaning of the values to avoid double negatives\n * Leave a trailing comma at the definition of the enum values\n\nJohannes Schindelin (4):\n split-index & fsmonitor: demonstrate a bug\n split-index; stop abusing the `base_oid` to strip the \"link\" extension\n fsmonitor: avoid overriding `cache_changed` bits\n unpack-trees: take care to propagate the split-index flag\n\n fsmonitor.h | 2 +-\n read-cache.c | 49 +++++++++++++++++++++++-------------\n t/t7527-builtin-fsmonitor.sh | 37 +++++++++++++++++++++++++++\n unpack-trees.c | 2 ++\n 4 files changed, 72 insertions(+), 18 deletions(-)\n\n\nbase-commit: eb88fe1ff5ceb34845f0919b8bdc60d8a1703cf6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1497%2Fdscho%2Ffix-split-index-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1497/dscho/fix-split-index-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1497\n\nRange-diff vs v1:\n\n 1: c025fccbdde = 1: c025fccbdde split-index & fsmonitor: demonstrate a bug\n 2: f1897b88072 ! 2: 8cc075f6325 split-index; stop abusing the `base_oid` to strip the \"link\" extension\n @@ Commit message\n \n One might be tempted to simply call `discard_split_index()` instead,\n under the assumption that Git decided to write a non-split index and\n - therefore the the `split_index` structure might no longer be wanted.\n + therefore the `split_index` structure might no longer be wanted.\n However, that is not possible because that would release index entries\n in `split_index->base` that are likely to still be in use. Therefore we\n cannot do that.\n \n - The next best thing we _can_ do is to introduce a flag, specifically\n - indicating when the \"link\" extension should be skipped. So that's what\n - we do here.\n + The next best thing we _can_ do is to introduce a bit field to indicate\n + specifically which index extensions (not) to write. So that's what we do\n + here.\n \n Signed-off-by: Johannes Schindelin \n \n @@ read-cache.c: static int record_ieot(void)\n \treturn !git_config_get_index_threads(&val) && val != 1;\n }\n \n -+enum strip_extensions {\n -+\tWRITE_ALL_EXTENSIONS = 0,\n -+\tSTRIP_ALL_EXTENSIONS = 1,\n -+\tSTRIP_LINK_EXTENSION_ONLY = 2\n ++enum write_extensions {\n ++\tWRITE_NO_EXTENSION = 0,\n ++\tWRITE_SPLIT_INDEX_EXTENSION = 1<<0,\n ++\tWRITE_CACHE_TREE_EXTENSION = 1<<1,\n ++\tWRITE_RESOLVE_UNDO_EXTENSION = 1<<2,\n ++\tWRITE_UNTRACKED_CACHE_EXTENSION = 1<<3,\n ++\tWRITE_FSMONITOR_EXTENSION = 1<<4,\n +};\n ++#define WRITE_ALL_EXTENSIONS ((enum write_extensions)-1)\n +\n /*\n * On success, `tempfile` is closed. If it is the temporary file\n @@ read-cache.c: static int record_ieot(void)\n */\n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n -\t\t\t int strip_extensions, unsigned flags)\n -+\t\t\t enum strip_extensions strip_extensions, unsigned flags)\n ++\t\t\t enum write_extensions write_extensions, unsigned flags)\n {\n \tuint64_t start = getnanotime();\n \tstruct hashfile *f;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t}\n \n -\tif (!strip_extensions && istate->split_index &&\n -+\tif (strip_extensions == WRITE_ALL_EXTENSIONS && istate->split_index &&\n - \t !is_null_oid(&istate->split_index->base_oid)) {\n +-\t !is_null_oid(&istate->split_index->base_oid)) {\n ++\tif (write_extensions & WRITE_SPLIT_INDEX_EXTENSION &&\n ++\t istate->split_index) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n + \t\tif (istate->sparse_index)\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n -\tif (!strip_extensions && !drop_cache_tree && istate->cache_tree) {\n -+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && !drop_cache_tree && istate->cache_tree) {\n ++\tif (write_extensions & WRITE_CACHE_TREE_EXTENSION &&\n ++\t !drop_cache_tree && istate->cache_tree) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\t\treturn -1;\n \t}\n -\tif (!strip_extensions && istate->resolve_undo) {\n -+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && istate->resolve_undo) {\n ++\tif (write_extensions & WRITE_RESOLVE_UNDO_EXTENSION &&\n ++\t istate->resolve_undo) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tresolve_undo_write(&sb, istate->resolve_undo);\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\t\treturn -1;\n \t}\n -\tif (!strip_extensions && istate->untracked) {\n -+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && istate->untracked) {\n ++\tif (write_extensions & WRITE_UNTRACKED_CACHE_EXTENSION &&\n ++\t istate->untracked) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_untracked_extension(&sb, istate->untracked);\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\t\treturn -1;\n \t}\n -\tif (!strip_extensions && istate->fsmonitor_last_update) {\n -+\tif (strip_extensions != STRIP_ALL_EXTENSIONS && istate->fsmonitor_last_update) {\n ++\tif (write_extensions & WRITE_FSMONITOR_EXTENSION &&\n ++\t istate->fsmonitor_last_update) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_fsmonitor_extension(&sb, istate);\n @@ read-cache.c: static int commit_locked_index(struct lock_file *lk)\n \t\treturn commit_lock_file(lk);\n }\n \n -+/*\n -+ * Write the Git index into a `.lock` file\n -+ *\n -+ * If `strip_link_extension` is non-zero, avoid writing any \"link\" extension\n -+ * (used by the split-index feature).\n -+ */\n - static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n +-static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n -\t\t\t\t unsigned flags)\n -+\t\t\t\t unsigned flags, int strip_link_extension)\n ++static int do_write_locked_index(struct index_state *istate,\n ++\t\t\t\t struct lock_file *lock,\n ++\t\t\t\t unsigned flags,\n ++\t\t\t\t enum write_extensions write_extensions)\n {\n \tint ret;\n \tint was_full = istate->sparse_index == INDEX_EXPANDED;\n @@ read-cache.c: static int do_write_locked_index(struct index_state *istate, struc\n \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n \t\t\t\t \"%s\", get_lock_file_path(lock));\n -\tret = do_write_index(istate, lock->tempfile, 0, flags);\n -+\tret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);\n ++\tret = do_write_index(istate, lock->tempfile, write_extensions, flags);\n \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n \t\t\t\t \"%s\", get_lock_file_path(lock));\n \n @@ read-cache.c: static int write_split_index(struct index_state *istate,\n \tint ret;\n \tprepare_to_write_split_index(istate);\n -\tret = do_write_locked_index(istate, lock, flags);\n -+\tret = do_write_locked_index(istate, lock, flags, 0);\n ++\tret = do_write_locked_index(istate, lock, flags, WRITE_ALL_EXTENSIONS);\n \tfinish_writing_split_index(istate);\n \treturn ret;\n }\n +@@ read-cache.c: static int write_shared_index(struct index_state *istate,\n + \n + \ttrace2_region_enter_printf(\"index\", \"shared/do_write_index\",\n + \t\t\t\t the_repository, \"%s\", get_tempfile_path(*temp));\n +-\tret = do_write_index(si->base, *temp, 1, flags);\n ++\tret = do_write_index(si->base, *temp, WRITE_NO_EXTENSION, flags);\n + \ttrace2_region_leave_printf(\"index\", \"shared/do_write_index\",\n + \t\t\t\t the_repository, \"%s\", get_tempfile_path(*temp));\n + \n @@ read-cache.c: int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tif ((!si && !test_split_index_env) ||\n \t alternate_index_output ||\n @@ read-cache.c: int write_locked_index(struct index_state *istate, struct lock_fil\n -\t\tif (si)\n -\t\t\toidclr(&si->base_oid);\n -\t\tret = do_write_locked_index(istate, lock, flags);\n -+\t\tret = do_write_locked_index(istate, lock, flags, 1);\n ++\t\tret = do_write_locked_index(istate, lock, flags,\n ++\t\t\t\t\t ~WRITE_SPLIT_INDEX_EXTENSION);\n \t\tgoto out;\n \t}\n \n @@ read-cache.c: int write_locked_index(struct index_state *istate, struct lock_fil\n \t\tif (!temp) {\n -\t\t\toidclr(&si->base_oid);\n -\t\t\tret = do_write_locked_index(istate, lock, flags);\n -+\t\t\tret = do_write_locked_index(istate, lock, flags, 1);\n ++\t\t\tret = do_write_locked_index(istate, lock, flags,\n ++\t\t\t\t\t\t ~WRITE_SPLIT_INDEX_EXTENSION);\n \t\t\tgoto out;\n \t\t}\n \t\tret = write_shared_index(istate, &temp, flags);\n 3: c1c35f0f026 = 3: 89b3cd9a668 fsmonitor: avoid overriding `cache_changed` bits\n 4: 3963d3e5428 = 4: df61146eaf5 unpack-trees: take care to propagate the split-index flag\n\n-- \ngitgitgadget\n"},{"id":"474209","messageId":"c025fccbdde1a4df11ba36552f86369ed74d37d2.1679870743.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.v2.git.1679870743.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] split-index & fsmonitor: demonstrate a bug","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-26T22:45:40Z","receivedAt":"2023-03-26T22:46:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis commit adds a new test case that demonstrates a bug in the\nsplit-index code that is triggered under certain circumstances when the\nFSMonitor is enabled, and its symptom manifests in the form of one of\nthe following error messages:\n\n BUG: fsmonitor.c:20: fsmonitor_dirty has more entries than the index (2 > 1)\n\n BUG: unpack-trees.c:776: pos doesn't point to the first entry of / in index\n\n error: invalid path ''\n error: The following untracked working tree files would be overwritten by reset:\n initial.t\n\nWhich of these error messages appears depends on timing-dependent\nconditions.\n\nTechnically the root cause lies with a bug in the split-index code that\nhas nothing to do with FSMonitor, but for the sake of this new test case\nit was the easiest way to trigger the bug.\n\nThe bug is this: Under specific conditions, Git needs to skip writing\nthe \"link\" extension (which is the index extension containing the\ninformation pertaining to the split-index). To do that, the `base_oid`\nattribute of the `split_index` structure in the in-memory index is\nzeroed out, and `do_write_index()` specifically checks for a \"null\"\n`base_oid` to understand that the \"link\" extension should not be\nwritten. However, this violates the consistency of the in-memory index\nstructure, but that does not cause problems in most cases because the\nprocess exits without using the in-memory index structure anymore,\nanyway.\n\nBut: _When_ the in-memory index is still used (which is the case e.g. in\n`git rebase`), subsequent writes of `the_index` are at risk of writing\nout a bogus index file, one that _should_ have a \"link\" extension but\ndoes not. In many cases, the `SPLIT_INDEX_ORDERED` flag _happens_ to be\nset for subsequent writes, forcing the shared index to be written, which\nre-initializes `base_oid` to a non-bogus state, and all is good.\n\nWhen it is _not_ set, however, all kinds of mayhem ensue, resulting in\nabove-mentioned error messages, and often enough putting worktrees in a\ntotally broken state where the only recourse is to manually delete the\n`index` and the `index.lock` files and then call `git reset` manually.\nNot something to ask users to do.\n\nThe reason why it is comparatively easy to trigger the bug with\nFSMonitor is that there is _another_ bug in the FSMonitor code:\n`mark_fsmonitor_valid()` sets `cache_changed` to 1, i.e. treating that\nvariable as a Boolean. But it is a bit field, and 1 happens to be the\n`SOMETHING_CHANGED` bit that forces the \"link\" extension to be skipped\nwhen writing the index, among other things.\n\n\"Comparatively easy\" is a relative term in this context, for sure. The\nessence of how the new test case triggers the bug is as following:\n\n1. The `git rebase` invocation will first reset the worktree to\n a commit that contains only the `one.t` file, and then execute a\n rebase script that starts with the following commands (commit hashes\n skipped):\n\n label onto\n\n reset initial\n pick two\n label two\n\n reset two\n pick three\n [...]\n\n2. Before executing the `label` command, a split index is written, as\n well as the shared index.\n\n3. The `reset initial` command in the rebase script writes out a new\n split index but skips writing the shared index, as intended.\n\n4. The `pick two` command updates the worktree and refreshes the index,\n marking the `two.t` entry as valid via the FSMonitor, which sets the\n `SOMETHING_CHANGED` bit in `cache_changed`, which in turn causes the\n `base_oid` attribute to be zeroed out and a full (non-split) index\n to be written (making sure _not_ to write the \"link\" extension).\n\n5. Now, the `reset two` command will leave the worktree alone, but\n still write out a new split index, not writing the shared index\n (because `base_oid` is still zeroed out, and there is no index entry\n update requiring it to be written, either).\n\n6. When it is turn to run `pick three`, the index is read, but it is\n too short: It only contains a single entry when there should be two,\n because the \"link\" extension is missing from the written-out index\n file.\n\nThere are three bugs at play, actually, which will be fixed over the\ncourse of the next commits:\n\n- The `base_oid` attribute should not be zeroed out to indicate when\n the \"link\" extension should not be written, as it puts the in-memory\n index structure into an inconsistent state.\n\n- The FSMonitor should not overwrite bits in `cache_changed`.\n\n- The `unpack_trees()` function tries to reuse the `split_index`\n structure from the source index, if any, but does not propagate the\n `SPLIT_INDEX_ORDERED` flag.\n\nWhile a fix for the second bug would let this test case pass, there are\nother conditions where the `SOMETHING_CHANGED` bit is set. Therefore,\nthe bug that most crucially needs to be fixed is the first one.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7527-builtin-fsmonitor.sh | 37 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh\nindex d419085379c..cbafdd69602 100755\n--- a/t/t7527-builtin-fsmonitor.sh\n+++ b/t/t7527-builtin-fsmonitor.sh\n@@ -1003,4 +1003,41 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '\n \tegrep \"^event: nfd/d_${utf8_nfc}/?$\" ./unicode.trace\n '\n \n+test_expect_failure 'split-index and FSMonitor work well together' '\n+\tgit init split-index &&\n+\ttest_when_finished \"git -C \\\"$PWD/split-index\\\" \\\n+\t\tfsmonitor--daemon stop\" &&\n+\t(\n+\t\tcd split-index &&\n+\t\tgit config core.splitIndex true &&\n+\t\t# force split-index in most cases\n+\t\tgit config splitIndex.maxPercentChange 99 &&\n+\t\tgit config core.fsmonitor true &&\n+\n+\t\t# Create the following commit topology:\n+\t\t#\n+\t\t# * merge three\n+\t\t# |\\\n+\t\t# | * three\n+\t\t# * | merge two\n+\t\t# |\\|\n+\t\t# | * two\n+\t\t# * | one\n+\t\t# |/\n+\t\t# * 5a5efd7 initial\n+\n+\t\ttest_commit initial &&\n+\t\ttest_commit two &&\n+\t\ttest_commit three &&\n+\t\tgit reset --hard initial &&\n+\t\ttest_commit one &&\n+\t\ttest_tick &&\n+\t\tgit merge two &&\n+\t\ttest_tick &&\n+\t\tgit merge three &&\n+\n+\t\tgit rebase --force-rebase -r one\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"474210","messageId":"df61146eaf5bb6245f8607bacfb91ab5613e322a.1679870743.git.gitgitgadget@gmail.com","threadId":"59439","inReplyTo":"pull.1497.v2.git.1679870743.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] unpack-trees: take care to propagate the split-index flag","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-26T22:45:43Z","receivedAt":"2023-03-26T22:46:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen copying the `split_index` structure from one index structure to\nanother, we need to propagate the `SPLIT_INDEX_ORDERED` flag, too, if it\nis set, otherwise Git might forget to write the shared index when that\nis actually needed.\n\nIt just so _happens_ that in many instances when `unpack_trees()` is\ncalled, the result causes the shared index to be written anyway, but\nthere are edge cases when that is not so.\n\nSigned-off-by: Johannes Schindelin \n---\n unpack-trees.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 90b92114be8..ca5e47c77c0 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1916,6 +1916,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t * create a new one.\n \t\t */\n \t\to->result.split_index = o->src_index->split_index;\n+\t\tif (o->src_index->cache_changed & SPLIT_INDEX_ORDERED)\n+\t\t\to->result.cache_changed |= SPLIT_INDEX_ORDERED;\n \t\to->result.split_index->refcount++;\n \t} else {\n \t\to->result.split_index = init_split_index(&o->result);\n-- \ngitgitgadget\n"},{"id":"474236","messageId":"42b9b692-88a2-5ff7-e857-20bd1f9bb0f2@jeffhostetler.com","threadId":"59439","inReplyTo":"pull.1497.v2.git.1679870743.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] Fix a few split-index bugs","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-03-27T14:48:32Z","receivedAt":"2023-03-27T14:48:39Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/26/23 6:45 PM, Johannes Schindelin via GitGitGadget wrote:\n> I received an internal bug report that after upgrading from v2.39.2 to\n> v2.40.0, some users ran into the following error message:\n> \n...\n\nVery nice. I like the new bit mask and getting rid of the double\nnegative strip/write logic.\n\n\nSigned-off-by: Jeff Hostetler \n\nThanks!\nJeff\n\n"},{"id":"474244","messageId":"xmqq4jq6cfib.fsf@gitster.g","threadId":"59439","inReplyTo":"42b9b692-88a2-5ff7-e857-20bd1f9bb0f2@jeffhostetler.com","subject":"Re: [PATCH v2 0/4] Fix a few split-index bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-27T16:42:36Z","receivedAt":"2023-03-27T16:43:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> On 3/26/23 6:45 PM, Johannes Schindelin via GitGitGadget wrote:\n>> I received an internal bug report that after upgrading from v2.39.2 to\n>> v2.40.0, some users ran into the following error message:\n>> \n> ...\n>\n> Very nice. I like the new bit mask and getting rid of the double\n> negative strip/write logic.\n\nYup, it indeed made the logic easier to see to split them into\nindividual bits per extensions even though an initial \"gut\" reaction\nmay have been \"you ain't gonna need such a flexibility\" ;-).\n\nThanks, both. Will replace.\n"}]}