{"thread":{"id":"59929","subject":"[PATCH 0/3] commit -a -m: allow the top-level tree to become empty again","startedAt":"2023-06-29T13:23:18Z","lastAt":"2023-06-29T19:20:44Z","messageCount":6,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"479010","messageId":"pull.1554.git.1688044991.gitgitgadget@gmail.com","threadId":"59929","inReplyTo":null,"subject":"[PATCH 0/3] commit -a -m: allow the top-level tree to become empty again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-29T13:23:07Z","receivedAt":"2023-06-29T13:23:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series is in response to the bug report in\nhttps://github.com/git-for-windows/git/issues/4462 that demonstrates that\ngit commit -a -m would no longer always stage all updates to tracked\nfiles. The bug has been introduced in Git v2.40.0.\n\nJohannes Schindelin (3):\n do_read_index(): always mark index as initialized unless erroring out\n split-index: accept that a base index can be empty\n commit -a -m: allow the top-level tree to become empty again\n\n builtin/commit.c | 7 ++-----\n read-cache.c | 15 +++++++++------\n t/t2200-add-update.sh | 11 +++++++++++\n 3 files changed, 22 insertions(+), 11 deletions(-)\n\n\nbase-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1554%2Fdscho%2Ffix-git-commit-a-m-when-tree-becomes-empty-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1554/dscho/fix-git-commit-a-m-when-tree-becomes-empty-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1554\n-- \ngitgitgadget\n"},{"id":"479011","messageId":"7ecfae7d6f63d235e6efe85b1b1d7c8f3fc08c29.1688044991.git.gitgitgadget@gmail.com","threadId":"59929","inReplyTo":"pull.1554.git.1688044991.gitgitgadget@gmail.com","subject":"[PATCH 1/3] do_read_index(): always mark index as initialized unless erroring out","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-29T13:23:08Z","receivedAt":"2023-06-29T13:23:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index\nfrom read_cache(), 2008-08-23) a flag was introduced into the\n`index_state` structure to indicate whether it had been initialized (or\nmore correctly: read and parsed).\n\nThere was one code path that was not handled, though: when the index\nfile does not yet exist (but the `must_exist` parameter is set to 0 to\nindicate that that's okay). In this instance, Git wants to go forward\nwith a new, pristine Git index, almost as if the file had existed and\ncontained no index entries or extensions.\n\nSince Git wants to handle this situation the same as if an \"empty\" Git\nindex file existed, let's set the `initialized` flag also in that case.\n\nThis is necessary to prepare for fixing the bug where the condition\n`cache_nr == 0` is incorrectly used as an indicator that the index was\nalready read, and the condition `initialized != 0` needs to be used\ninstead.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex f4c31a68c85..b10caa9831c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2285,6 +2285,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tif (fd < 0) {\n \t\tif (!must_exist && errno == ENOENT) {\n \t\t\tset_new_index_sparsity(istate);\n+\t\t\tistate->initialized = 1;\n \t\t\treturn 0;\n \t\t}\n \t\tdie_errno(_(\"%s: index file open failed\"), path);\n-- \ngitgitgadget\n\n"},{"id":"479012","messageId":"81118ce106222993ef17586fb0f249d8319f3b90.1688044991.git.gitgitgadget@gmail.com","threadId":"59929","inReplyTo":"pull.1554.git.1688044991.gitgitgadget@gmail.com","subject":"[PATCH 2/3] split-index: accept that a base index can be empty","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-29T13:23:09Z","receivedAt":"2023-06-29T13:23:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe are about to fix an ancient bug where `do_read_index()` pretended\nthat the index was not initialized when there are no index entries.\n\nBefore the `index_state` structure gained the `initialized` flag in\n913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from\nread_cache(), 2008-08-23), that was the best we could do (even if it was\nincorrect: it is totally possible to read a Git index file that contains\nno index entries).\n\nThis pattern was repeated also in 998330ac2e7 (read-cache: look for\nshared index files next to the index, too, 2021-08-26), which we fix\nhere by _not_ mistaking an empty base index for a missing\n`sharedindex.*` file.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b10caa9831c..e15a472f54f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2455,12 +2455,14 @@ int read_index_from(struct index_state *istate, const char *path,\n \n \tbase_oid_hex = oid_to_hex(&split_index->base_oid);\n \tbase_path = xstrfmt(\"%s/sharedindex.%s\", gitdir, base_oid_hex);\n-\ttrace2_region_enter_printf(\"index\", \"shared/do_read_index\",\n-\t\t\t\t the_repository, \"%s\", base_path);\n-\tret = do_read_index(split_index->base, base_path, 0);\n-\ttrace2_region_leave_printf(\"index\", \"shared/do_read_index\",\n-\t\t\t\t the_repository, \"%s\", base_path);\n-\tif (!ret) {\n+\tif (file_exists(base_path)) {\n+\t\ttrace2_region_enter_printf(\"index\", \"shared/do_read_index\",\n+\t\t\t\t\tthe_repository, \"%s\", base_path);\n+\n+\t\tret = do_read_index(split_index->base, base_path, 0);\n+\t\ttrace2_region_leave_printf(\"index\", \"shared/do_read_index\",\n+\t\t\t\t\tthe_repository, \"%s\", base_path);\n+\t} else {\n \t\tchar *path_copy = xstrdup(path);\n \t\tchar *base_path2 = xstrfmt(\"%s/sharedindex.%s\",\n \t\t\t\t\t dirname(path_copy), base_oid_hex);\n-- \ngitgitgadget\n\n"},{"id":"479013","messageId":"08c50b64e2a93300eed196505936e58ce8bb639b.1688044991.git.gitgitgadget@gmail.com","threadId":"59929","inReplyTo":"pull.1554.git.1688044991.gitgitgadget@gmail.com","subject":"[PATCH 3/3] commit -a -m: allow the top-level tree to become empty again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-29T13:23:10Z","receivedAt":"2023-06-29T13:23:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 03267e8656c (commit: discard partial cache before (re-)reading it,\n2022-11-08), a memory leak was plugged by discarding any partial index\nbefore re-reading it.\n\nThe problem with this memory leak fix is that it was based on an\nincomplete understanding of the logic introduced in 7168624c353 (Do not\ngenerate full commit log message if it is not going to be used,\n2007-11-28).\n\nThat logic was introduced to add a shortcut when committing without\nediting the commit message interactively. A part of that logic was to\nensure that the index was read into memory:\n\n\tif (!active_nr && read_cache() < 0)\n\t\tdie(...)\n\nTranslation to English: If the index has not yet been read, read it, and\nif that fails, error out.\n\nThat logic was incorrect, though: It used `!active_nr` as an indicator\nthat the index was not yet read. Usually this is not a problem because\nin the vast majority of instances, the index contains at least one\nentry.\n\nAnd it was natural to do it this way because at the time that condition\nwas introduced, the `index_state` structure had no explicit flag to\nindicate that it was initialized: This flag was only introduced in\n913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from\nread_cache(), 2008-08-23), but that commit did not adjust the code path\nwhere no index file was found and a new, pristine index was initialized.\n\nNow, when the index does not contain any entry (which is quite\ncommon in Git's test suite because it starts quite a many repositories\nfrom scratch), subsequent calls to `do_read_index()` will mistake the\nindex not to be initialized, and read it again unnecessarily.\n\nThis is a problem because after initializing the empty index e.g. the\n`cache_tree` in that index could have been initialized before a\nsubsequent call to `do_read_index()` wants to ensure an initialized\nindex. And if that subsequent call mistakes the index not to have been\ninitialized, it would lead to leaked memory.\n\nThe correct fix for that memory leak is to adjust the condition so that\nit does not mistake `active_nr == 0` to mean that the index has not yet\nbeen read.\n\nUsing the `initialized` flag instead, we avoid that mistake, and as a\nbonus we can fix a bug at the same time that was introduced by the\nmemory leak fix: When deleting all tracked files and then asking `git\ncommit -a -m ...` to commit the result, Git would internally update the\nindex, then discard and re-read the index undoing the update, and fail\nto commit anything.\n\nThis fixes https://github.com/git-for-windows/git/issues/4462\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/commit.c | 7 ++-----\n t/t2200-add-update.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 65a5c0e29d5..4cf2baaf943 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -998,11 +998,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstruct object_id oid;\n \t\tconst char *parent = \"HEAD\";\n \n-\t\tif (!the_index.cache_nr) {\n-\t\t\tdiscard_index(&the_index);\n-\t\t\tif (repo_read_index(the_repository) < 0)\n-\t\t\t\tdie(_(\"Cannot read index\"));\n-\t\t}\n+\t\tif (!the_index.initialized && repo_read_index(the_repository) < 0)\n+\t\t\tdie(_(\"Cannot read index\"));\n \n \t\tif (amend)\n \t\t\tparent = \"HEAD^1\";\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex be394f1131a..c01492f33f8 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -197,4 +197,15 @@ test_expect_success '\"add -u non-existent\" should fail' '\n \t! grep \"non-existent\" actual\n '\n \n+test_expect_success '\"commit -a\" implies \"add -u\" if index becomes empty' '\n+\tgit rm -rf \\* &&\n+\tgit commit -m clean-slate &&\n+\ttest_commit file1 &&\n+\trm file1.t &&\n+\ttest_tick &&\n+\tgit commit -a -m remove &&\n+\tgit ls-tree HEAD: >out &&\n+\ttest_must_be_empty out\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"479023","messageId":"xmqqcz1e9jny.fsf@gitster.g","threadId":"59929","inReplyTo":"81118ce106222993ef17586fb0f249d8319f3b90.1688044991.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] split-index: accept that a base index can be empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-29T19:02:09Z","receivedAt":"2023-06-29T19:06:11Z","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> We are about to fix an ancient bug where `do_read_index()` pretended\n> that the index was not initialized when there are no index entries.\n>\n> Before the `index_state` structure gained the `initialized` flag in\n> 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from\n> read_cache(), 2008-08-23), that was the best we could do (even if it was\n> incorrect: it is totally possible to read a Git index file that contains\n> no index entries).\n\nYeah, I very much remember how that single bit made our live much\neasier.\n\n> This pattern was repeated also in 998330ac2e7 (read-cache: look for\n> shared index files next to the index, too, 2021-08-26), which we fix\n> here by _not_ mistaking an empty base index for a missing\n> `sharedindex.*` file.\n\nAhh, this is in the codepath to deal with a separate worktree. We\nallow sharing of the \"sharedindex.*\" file across worktrees and\nentries read from the \"index\" files from individual worktrees to\noverlay it. But we also do allow worktrees to have their own\n\"sharedindex.*\" file, which is what the commit in question wanted to\ndo, and the way it (wanted to) implement was\n\n - check the \"gitdir\" version first, as before\n - if that did not exist, then look at the one next to \"index\"\n\nbut \"if that did not exist\" was implemented incorrectly and did not\naccount for the case where that \"gitdir\" version was an empty index.\n\nSo, instead, updated code checks and reads the \"gitdir\" version *if*\nthe file exists, regardless of how many entries there are in it.\n\nMakes sense.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> read-cache.c | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index b10caa9831c..e15a472f54f 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2455,12 +2455,14 @@ int read_index_from(struct index_state *istate, const char *path,\n> \n> \tbase_oid_hex = oid_to_hex(&split_index->base_oid);\n> \tbase_path = xstrfmt(\"%s/sharedindex.%s\", gitdir, base_oid_hex);\n> -\ttrace2_region_enter_printf(\"index\", \"shared/do_read_index\",\n> -\t\t\t\t the_repository, \"%s\", base_path);\n> -\tret = do_read_index(split_index->base, base_path, 0);\n> -\ttrace2_region_leave_printf(\"index\", \"shared/do_read_index\",\n> -\t\t\t\t the_repository, \"%s\", base_path);\n> -\tif (!ret) {\n> +\tif (file_exists(base_path)) {\n> +\t\ttrace2_region_enter_printf(\"index\", \"shared/do_read_index\",\n> +\t\t\t\t\tthe_repository, \"%s\", base_path);\n> +\n> +\t\tret = do_read_index(split_index->base, base_path, 0);\n> +\t\ttrace2_region_leave_printf(\"index\", \"shared/do_read_index\",\n> +\t\t\t\t\tthe_repository, \"%s\", base_path);\n> +\t} else {\n> \t\tchar *path_copy = xstrdup(path);\n> \t\tchar *base_path2 = xstrfmt(\"%s/sharedindex.%s\",\n> \t\t\t\t\t dirname(path_copy), base_oid_hex);\n"},{"id":"479025","messageId":"xmqq5y769iyl.fsf@gitster.g","threadId":"59929","inReplyTo":"08c50b64e2a93300eed196505936e58ce8bb639b.1688044991.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] commit -a -m: allow the top-level tree to become empty again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-29T19:17:22Z","receivedAt":"2023-06-29T19:20:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> That logic was introduced to add a shortcut when committing without\n> editing the commit message interactively. A part of that logic was to\n> ensure that the index was read into memory:\n>\n> \tif (!active_nr && read_cache() < 0)\n> \t\tdie(...)\n>\n> Translation to English: If the index has not yet been read, read it, and\n> if that fails, error out.\n\nWell described. It does make sense to turn !active_nr used here\ninto a check on the .initialized member.\n\n> And it was natural to do it this way because at the time that condition\n> was introduced, the `index_state` structure had no explicit flag to\n> indicate that it was initialized: This flag was only introduced in\n> 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from\n> read_cache(), 2008-08-23), but that commit did not adjust the code path\n> where no index file was found and a new, pristine index was initialized.\n\nMy mistake, but after 15 years it probably is beyond statute of\nlimitations ;-)\n\n> Using the `initialized` flag instead, we avoid that mistake, and as a\n> bonus we can fix a bug at the same time that was introduced by the\n> memory leak fix: When deleting all tracked files and then asking `git\n> commit -a -m ...` to commit the result, Git would internally update the\n> index, then discard and re-read the index undoing the update, and fail\n> to commit anything.\n\nThat does sound like the primary bug fixed with this change, not a\nbonus, but anyway, the change is very sensible and clearly described\nwith a good test. Will queue.\n\nThanks.\n"}]}