{"thread":{"id":"56791","subject":"[PATCH 0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch","startedAt":"2021-10-26T22:35:36Z","lastAt":"2021-10-28T00:31:02Z","messageCount":7,"participants":["Neeraj K. Singh via GitGitGadget","Neeraj Singh via GitGitGadget","Johannes Schindelin","Junio C Hamano","Neeraj Singh"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"439695","messageId":"pull.1067.git.1635287730.gitgitgadget@gmail.com","threadId":"56791","inReplyTo":null,"subject":"[PATCH 0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch","fromName":"Neeraj K. Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-26T22:35:28Z","receivedAt":"2021-10-26T22:35:36Z","isPatch":true,"sender":{"key":"name:Neeraj K. Singh","avatar":null},"body":" * Fix prune code to be able to work against multiple cruft directories. I\n noticed this in self-review.\n\n * When dscho enabled core.fsyncobjectfiles=batch in git-for-windows we saw\n some test-failures in update-index tests. The root cause is that\n setup_work_tree does a chdir_notify, which erases the tmp-objdir state. I\n now unapply and reapply the tmp-objdir around setup_git_env.\n\nThis branch autosquashes cleanly and it needs to be merged with\nns/batched-fsync, where it currently merges cleanly.\n\nNeeraj Singh (2):\n fixup! tmp-objdir: new API for creating temporary writable databases\n fixup! tmp-objdir: new API for creating temporary writable databases\n\n builtin/prune.c | 1 +\n environment.c | 5 +++++\n tmp-objdir.c | 25 +++++++++++++++++++++++++\n tmp-objdir.h | 15 +++++++++++++++\n 4 files changed, 46 insertions(+)\n\n\nbase-commit: 50741b157f2f90df76a60418e2781b2c1e6e3c78\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1067%2Fneerajsi-msft%2Fns%2Ftmp-objdir-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1067/neerajsi-msft/ns/tmp-objdir-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1067\n-- \ngitgitgadget\n"},{"id":"439696","messageId":"d244cc4bb60959b8f3d8711a7aeb434efcc9d2a2.1635287730.git.gitgitgadget@gmail.com","threadId":"56791","inReplyTo":"pull.1067.git.1635287730.gitgitgadget@gmail.com","subject":"[PATCH 1/2] fixup! tmp-objdir: new API for creating temporary writable databases","fromName":"Neeraj Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-26T22:35:29Z","receivedAt":"2021-10-26T22:35:37Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"From: Neeraj Singh \n\nFix prune code to be able to delete multiple object directories. I\nwasn't properly resetting the strbuf with the path.\n\nSigned-off-by: Neeraj Singh \n---\n builtin/prune.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 9c72ecf5a58..6b6b0c7b011 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -31,6 +31,7 @@ static int prune_tmp_file(const char *fullpath)\n \t\tif (show_only || verbose)\n \t\t\tprintf(\"Removing stale temporary directory %s\\n\", fullpath);\n \t\tif (!show_only) {\n+\t\t\tstrbuf_reset(&remove_dir_buf);\n \t\t\tstrbuf_addstr(&remove_dir_buf, fullpath);\n \t\t\tremove_dir_recursively(&remove_dir_buf, 0);\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"439697","messageId":"ef5a087813b7dfd232a9366eee09774d197e2307.1635287730.git.gitgitgadget@gmail.com","threadId":"56791","inReplyTo":"pull.1067.git.1635287730.gitgitgadget@gmail.com","subject":"[PATCH 2/2] fixup! tmp-objdir: new API for creating temporary writable databases","fromName":"Neeraj Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-26T22:35:30Z","receivedAt":"2021-10-26T22:35:38Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"From: Neeraj Singh \n\nWhen setup_work_tree executes, it redoes setup of the object database\npath and various other aspects of the_repository. This destroys the\ntemporary object database state.\n\nThis commit removes the temporary object database and reapplies it\naround the operations in the chdir_notify callback.\n\nSigned-off-by: Neeraj Singh \n---\n environment.c | 5 +++++\n tmp-objdir.c | 25 +++++++++++++++++++++++++\n tmp-objdir.h | 15 +++++++++++++++\n 3 files changed, 45 insertions(+)\n\ndiff --git a/environment.c b/environment.c\nindex 46ec5072c05..7ba5ae06c71 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -17,6 +17,7 @@\n #include \"commit.h\"\n #include \"strvec.h\"\n #include \"object-store.h\"\n+#include \"tmp-objdir.h\"\n #include \"chdir-notify.h\"\n #include \"shallow.h\"\n \n@@ -344,10 +345,14 @@ static void update_relative_gitdir(const char *name,\n \t\t\t\t void *data)\n {\n \tchar *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());\n+\tstruct tmp_objdir *tmp_objdir = tmp_objdir_unapply_primary_odb();\n \ttrace_printf_key(&trace_setup_key,\n \t\t\t \"setup: move $GIT_DIR to '%s'\",\n \t\t\t path);\n+\n \tset_git_dir_1(path);\n+\tif (tmp_objdir)\n+\t\ttmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);\n \tfree(path);\n }\n \ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 45d42a7bcf0..3d38eeab66b 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"tmp-objdir.h\"\n+#include \"chdir-notify.h\"\n #include \"dir.h\"\n #include \"sigchain.h\"\n #include \"string-list.h\"\n@@ -12,6 +13,7 @@ struct tmp_objdir {\n \tstruct strbuf path;\n \tstruct strvec env;\n \tstruct object_directory *prev_odb;\n+\tint will_destroy;\n };\n \n /*\n@@ -315,4 +317,27 @@ void tmp_objdir_replace_primary_odb(struct tmp_objdir *t, int will_destroy)\n \tif (t->prev_odb)\n \t\tBUG(\"the primary object database is already replaced\");\n \tt->prev_odb = set_temporary_primary_odb(t->path.buf, will_destroy);\n+\tt->will_destroy = will_destroy;\n+}\n+\n+struct tmp_objdir *tmp_objdir_unapply_primary_odb(void)\n+{\n+\tif (!the_tmp_objdir || !the_tmp_objdir->prev_odb)\n+\t\treturn NULL;\n+\n+\trestore_primary_odb(the_tmp_objdir->prev_odb, the_tmp_objdir->path.buf);\n+\tthe_tmp_objdir->prev_odb = NULL;\n+\treturn the_tmp_objdir;\n+}\n+\n+void tmp_objdir_reapply_primary_odb(struct tmp_objdir *t, const char *old_cwd,\n+\t\tconst char *new_cwd)\n+{\n+\tchar *path;\n+\n+\tpath = reparent_relative_path(old_cwd, new_cwd, t->path.buf);\n+\tstrbuf_reset(&t->path);\n+\tstrbuf_addstr(&t->path, path);\n+\tfree(path);\n+\ttmp_objdir_replace_primary_odb(t, t->will_destroy);\n }\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex 75754cbfba6..a3145051f25 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -59,4 +59,19 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *);\n */\n void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy);\n \n+/*\n+ * If the primary object database was replaced by a temporary object directory,\n+ * restore it to its original value while keeping the directory contents around.\n+ * Returns NULL if the primary object database was not replaced.\n+ */\n+struct tmp_objdir *tmp_objdir_unapply_primary_odb(void);\n+\n+/*\n+ * Reapplies the former primary temporary object database, after protentially\n+ * changing its relative path.\n+ */\n+void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd,\n+\t\tconst char *new_cwd);\n+\n+\n #endif /* TMP_OBJDIR_H */\n-- \ngitgitgadget\n"},{"id":"439772","messageId":"nycvar.QRO.7.76.6.2110271439120.56@tvgsbejvaqbjf.bet","threadId":"56791","inReplyTo":"pull.1067.git.1635287730.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-10-27T12:44:00Z","receivedAt":"2021-10-27T12:44:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Neeraj,\n\nOn Tue, 26 Oct 2021, Neeraj K. Singh via GitGitGadget wrote:\n\n> * Fix prune code to be able to work against multiple cruft directories. I\n> noticed this in self-review.\n>\n> * When dscho enabled core.fsyncobjectfiles=batch in git-for-windows we saw\n> some test-failures in update-index tests. The root cause is that\n> setup_work_tree does a chdir_notify, which erases the tmp-objdir state. I\n> now unapply and reapply the tmp-objdir around setup_git_env.\n>\n> This branch autosquashes cleanly and it needs to be merged with\n> ns/batched-fsync, where it currently merges cleanly.\n>\n> Neeraj Singh (2):\n> fixup! tmp-objdir: new API for creating temporary writable databases\n> fixup! tmp-objdir: new API for creating temporary writable databases\n\nThank you for the fast work on the fixes!\n\nI applied both patches to the PR branch and pushed; Let's see how the CI\nover at https://github.com/git-for-windows/git/pull/3492 pans out.\n\nPlease note the original patch made it into `next` already (and is hence\nsubject to follow-up patches rather than being rewritten).\n\nTherefore, you may need to reword the commit messages so that they stand\non their own, as follow-up commits.\n\nAnd alternative would be to ask Junio to kick the topic out of `next` and\nback to `seen`, in which case you will probably be asked to submit a new\niteration of the original patch.\n\nThank you again!\nDscho\n\n>\n> builtin/prune.c | 1 +\n> environment.c | 5 +++++\n> tmp-objdir.c | 25 +++++++++++++++++++++++++\n> tmp-objdir.h | 15 +++++++++++++++\n> 4 files changed, 46 insertions(+)\n>\n>\n> base-commit: 50741b157f2f90df76a60418e2781b2c1e6e3c78\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1067%2Fneerajsi-msft%2Fns%2Ftmp-objdir-fixes-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1067/neerajsi-msft/ns/tmp-objdir-fixes-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1067\n> --\n> gitgitgadget\n>\n>\n"},{"id":"439832","messageId":"xmqqo87auqda.fsf@gitster.g","threadId":"56791","inReplyTo":"nycvar.QRO.7.76.6.2110271439120.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T21:09:21Z","receivedAt":"2021-10-27T21:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Neeraj Singh (2):\n>> fixup! tmp-objdir: new API for creating temporary writable databases\n>> fixup! tmp-objdir: new API for creating temporary writable databases\n>\n> Thank you for the fast work on the fixes!\n>\n> I applied both patches to the PR branch and pushed; Let's see how the CI\n> over at https://github.com/git-for-windows/git/pull/3492 pans out.\n>\n> Please note the original patch made it into `next` already (and is hence\n> subject to follow-up patches rather than being rewritten).\n>\n> Therefore, you may need to reword the commit messages so that they stand\n> on their own, as follow-up commits.\n>\n> And alternative would be to ask Junio to kick the topic out of `next` and\n> back to `seen`, in which case you will probably be asked to submit a new\n> iteration of the original patch.\n\nYeah, none of the above is attractive this late in the cycle X-<.\n\nIt probalby is best to queue the \"fixup!\" commits as they are on top\nof ns/tmp-objdir, merge the result to two topics that depend on\nns/tmp-objdir, and keep them without merging them down, until the\nrelease. When it is time to rewind 'next' after the release, it\nwould be a good chance to get rid of these \"oops, earlier we screwed\nup\" commits by redoing the tmp-objdir (and rebasing the other two\ntopics on top).\n\n"},{"id":"439844","messageId":"20211027225706.GA3984@neerajsi-x1.localdomain","threadId":"56791","inReplyTo":"xmqqo87auqda.fsf@gitster.g","subject":"Re: [PATCH 0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-10-27T22:57:06Z","receivedAt":"2021-10-27T22:57:13Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Wed, Oct 27, 2021 at 02:09:21PM -0700, Junio C Hamano wrote:\n> Yeah, none of the above is attractive this late in the cycle X-<.\n> \n> It probalby is best to queue the \"fixup!\" commits as they are on top\n> of ns/tmp-objdir, merge the result to two topics that depend on\n> ns/tmp-objdir, and keep them without merging them down, until the\n> release. When it is time to rewind 'next' after the release, it\n> would be a good chance to get rid of these \"oops, earlier we screwed\n> up\" commits by redoing the tmp-objdir (and rebasing the other two\n> topics on top).\n> \n\nHi Junio,\nApologies for the breakage! I just want to be 100% clear here: is there\nany action I should take with the patches, or will you handle the merge/rebase?\n\nFYI for anyone trying this on git-for-windows, there's one additional patch at:\nhttps://github.com/neerajsi-msft/git/commit/435e1d2e5e8fb422b0f08ff6a01a130584f7e249\n\nThat fixes a gfw-specific breakage that affects tmp_objdir_migrate and causes it to\ninfinitely create recursive directories until the disk fills up (surprisingly we don't\nhit stack overflow first).\n\nThanks,\nNeeraj\n"},{"id":"439852","messageId":"xmqqcznqt2gu.fsf@gitster.g","threadId":"56791","inReplyTo":"20211027225706.GA3984@neerajsi-x1.localdomain","subject":"Re: [PATCH 0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T00:30:57Z","receivedAt":"2021-10-28T00:31:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neeraj Singh writes:\n\n> On Wed, Oct 27, 2021 at 02:09:21PM -0700, Junio C Hamano wrote:\n>> Yeah, none of the above is attractive this late in the cycle X-<.\n>> \n>> It probalby is best to queue the \"fixup!\" commits as they are on top\n>> of ns/tmp-objdir, merge the result to two topics that depend on\n>> ns/tmp-objdir, and keep them without merging them down, until the\n>> release. When it is time to rewind 'next' after the release, it\n>> would be a good chance to get rid of these \"oops, earlier we screwed\n>> up\" commits by redoing the tmp-objdir (and rebasing the other two\n>> topics on top).\n>> \n>\n> Hi Junio,\n> Apologies for the breakage! I just want to be 100% clear here: is there\n> any action I should take with the patches, or will you handle the merge/rebase?\n\nIf we all agree on the above plan, then nothing for you for now, but\nwe'd ask you to send a cleaned-up patch after the upcoming release\nwhen the 'next' branch gets rewound and rebuilt, at which time we\ncan get rid of the \"oops, we screwed up\" fixup patches.\n\nThanks for finding and sending in the fix.\n\n"}]}