{"thread":{"id":"62851","subject":"[PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","startedAt":"2025-01-25T05:41:47Z","lastAt":"2025-03-15T23:17:26Z","messageCount":16,"participants":["Patrick Steinhardt","Johannes Sixt","Christian Reich","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511195","messageId":"20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im","threadId":"62851","inReplyTo":null,"subject":"[PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-25T05:41:34Z","receivedAt":"2025-01-25T05:41:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Unlinking a file may fail on Windows systems when the file is still held\nopen by another process. This is incompatible with POSIX semantics and\nby extension with Git's assumed semantics when unlinking files, which\nis that files can be unlinked regardless of whether they are still open\nor not. To counteract this incompatibility, we have some custom error\nhandling in the `mingw_unlink()` wrapper that first retries the deletion\nwith some delay, and then asks the user whether we should continue to\nretry.\n\nWhile this logic might be sensible in many callsites throughout Git, it\nis less when used in the reftable library. We only use unlink(3) there\nto delete tables which aren't referenced anymore, and the code is very\naware of the limitations on Windows. As such, all calls to unlink(3p)\ndon't perform any error checking at all and are fine with the call\nfailing.\n\nInstead, the library provides the `reftable_stack_clean()` function,\nwhich Git knows to execute in git-pack-refs(1) after compacting a stack.\nThe effect of this function is that all stale tables will eventually get\ndeleted once they aren't kept open anymore.\n\nSo while we're fine with unlink(3p) failing, the Windows-emulation of\nthat function will still perform several sleeps and ultimately end up\nasking the user:\n\n $ git pack-refs\n Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n\nIt even asks multiple times, which is doubly annoying and puzzling to\nthe user:\n\n 1. It asks when trying to delete the old file after having written the\n compacted stack.\n\n 2. It asks when reloading the stack, where it will try to unlink\n now-unreferenced tables.\n\n 3. It asks when calling `reftable_stack_clean()`, where it will try to\n unlink now-stale tables.\n\nFix the issue by making it possible to disable this behaviour with a\npreprocessor define. As \"git-compat-util.h\" is only included from\n\"system.h\", and given that \"system.h\" is only ever included by headers\nand code that are internal to the reftable library, we can set that\nmacro in this header without impacting anything else but the reftable\nlibrary.\n\nReported-by: Christian Reich \nSigned-off-by: Patrick Steinhardt \n---\nHi,\n\nThis patch fixes the issue reported in [1].\n\nThanks!\n\nPatrick\n\n[1]: \n---\n compat/mingw.c | 5 ++++-\n compat/mingw.h | 8 ++++++--\n reftable/system.h | 1 +\n 3 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 1d5b211b54..0e4b6a70a4 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)\n \treturn wbuf;\n }\n \n-int mingw_unlink(const char *pathname)\n+int mingw_unlink(const char *pathname, int handle_in_use_error)\n {\n \tint ret, tries = 0;\n \twchar_t wpathname[MAX_PATH];\n@@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)\n \twhile ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n \t\tif (!is_file_in_use_error(GetLastError()))\n \t\t\tbreak;\n+\t\tif (!handle_in_use_error)\n+\t\t\treturn ret;\n+\n \t\t/*\n \t\t * We assume that some other process had the source or\n \t\t * destination file open at the wrong moment and retry.\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex ebfb8ba423..a555af8d54 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -224,8 +224,12 @@ int uname(struct utsname *buf);\n * replacements of existing functions\n */\n \n-int mingw_unlink(const char *pathname);\n-#define unlink mingw_unlink\n+int mingw_unlink(const char *pathname, int handle_in_use_error);\n+#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n+# define unlink(path) mingw_unlink(path, 0)\n+#else\n+# define unlink(path) mingw_unlink(path, 1)\n+#endif\n \n int mingw_rmdir(const char *path);\n #define rmdir mingw_rmdir\ndiff --git a/reftable/system.h b/reftable/system.h\nindex 5274eca1d0..fe94bf205b 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -13,6 +13,7 @@ license that can be found in the LICENSE file or at\n \n #define DISABLE_SIGN_COMPARE_WARNINGS\n \n+#define MINGW_DONT_HANDLE_IN_USE_ERROR\n #include \"git-compat-util.h\"\n \n /*\n\n---\nbase-commit: 4e746b1a31f9f0036032b6f94279cf16fb363203\nchange-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7\n\n"},{"id":"511197","messageId":"6acb9e8a-7014-4605-b8af-59c4584e9fe6@kdbg.org","threadId":"62851","inReplyTo":"20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-01-25T08:19:57Z","receivedAt":"2025-01-25T08:20:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.01.25 um 06:41 schrieb Patrick Steinhardt:\n> While this logic might be sensible in many callsites throughout Git, it\n> is less when used in the reftable library. We only use unlink(3) there\n> to delete tables which aren't referenced anymore, and the code is very\n> aware of the limitations on Windows. As such, all calls to unlink(3p)\n> don't perform any error checking at all and are fine with the call\n> failing.\n> \n> Instead, the library provides the `reftable_stack_clean()` function,\n> which Git knows to execute in git-pack-refs(1) after compacting a stack.\n> The effect of this function is that all stale tables will eventually get\n> deleted once they aren't kept open anymore.\n> \n> So while we're fine with unlink(3p) failing, the Windows-emulation of\n> that function will still perform several sleeps and ultimately end up\n> asking the user:\n\nWhy don't the changes that your commits ending at 391bceae4350\n(\"compat/mingw: support POSIX semantics for atomic renames\", 2024-10-27)\nhelp in this case, too?\n\nSince the reftable layer is aware of the problem, why don't we just fix\nit there and instead sweep it under the rug in the compat layer?\n\n-- Hannes\n\n"},{"id":"511198","messageId":"Z5ShoLCWREAsWZVh@pks.im","threadId":"62851","inReplyTo":"6acb9e8a-7014-4605-b8af-59c4584e9fe6@kdbg.org","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-25T08:32:32Z","receivedAt":"2025-01-25T08:32:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jan 25, 2025 at 09:19:57AM +0100, Johannes Sixt wrote:\n> Am 25.01.25 um 06:41 schrieb Patrick Steinhardt:\n> > While this logic might be sensible in many callsites throughout Git, it\n> > is less when used in the reftable library. We only use unlink(3) there\n> > to delete tables which aren't referenced anymore, and the code is very\n> > aware of the limitations on Windows. As such, all calls to unlink(3p)\n> > don't perform any error checking at all and are fine with the call\n> > failing.\n> > \n> > Instead, the library provides the `reftable_stack_clean()` function,\n> > which Git knows to execute in git-pack-refs(1) after compacting a stack.\n> > The effect of this function is that all stale tables will eventually get\n> > deleted once they aren't kept open anymore.\n> > \n> > So while we're fine with unlink(3p) failing, the Windows-emulation of\n> > that function will still perform several sleeps and ultimately end up\n> > asking the user:\n> \n> Why don't the changes that your commits ending at 391bceae4350\n> (\"compat/mingw: support POSIX semantics for atomic renames\", 2024-10-27)\n> help in this case, too?\n\nThe user report was explicitly about compatibility with JGit, which\nstill had these files open. We don't have control over third-party\nclients and how exactly they open files, so it is expected that we may\nstill see failures with the deletion of in-use files.\n\n> Since the reftable layer is aware of the problem, why don't we just fix\n> it there and instead sweep it under the rug in the compat layer?\n\nI didn't really have a good idea for _how_ to do that. We automatically\npull in the compat version of `unlink()` via \"git-compat-util.h\", and we\ncannot easily change that. So the reftable library is basically unable\nto handle it there, because the assumed POSIX-behavior of `unlink()` is\ndifferent. I also don't want to reimplement the \"compat/\" layer for\nWindows, because it brings us a couple of features for free, like\nwide-character handling and handling the deletion of read-only files.\n\nAnother alternative would be to provide `reftable_unlink()` in the\nreftable system layer, implement it via `mingw_unlink()` with that\nsecond argument I'm introducing and then convert all callsites of unlink\nto use that function instead. But that felt unnecessarily complex to me.\n\nI'd be happy to hear about alternative ideas that didn't came to my\nmind.\n\nPatrick\n"},{"id":"511199","messageId":"08d7e79e-3a59-4163-be2b-97f0238142aa@t-online.de","threadId":"62851","inReplyTo":"20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Christian Reich","fromEmail":"zottelbart@t-online.de","sentAt":"2025-01-25T08:45:21Z","receivedAt":"2025-01-25T08:51:22Z","isPatch":true,"sender":{"key":"zottelbart@t-online.de","avatar":null},"body":"Greetings Patrick,\n\nthx you for the patch. I build an own version of git for windows with \nthis patch and it works as expected! *thumbsup*\n\nBut I see merge conflicts in git for windows main-branch:\n\n1. mingw_unlink looks different in windows-master in compare to the diff:\n\nhttps://github.com/git-for-windows/git/blob/main/compat/mingw.c#L551\n\n2. in mingw_rename is another call of mingw_unlink:\n\nhttps://github.com/git-for-windows/git/blob/main/compat/mingw.c#L2974\n\nSo I don't know how the patch would find it way to the windows version.\n\nThank you for your effort.\n\nChristian\n\nAm 25.01.2025 um 06:41 schrieb Patrick Steinhardt:\n> Unlinking a file may fail on Windows systems when the file is still held\n> open by another process. This is incompatible with POSIX semantics and\n> by extension with Git's assumed semantics when unlinking files, which\n> is that files can be unlinked regardless of whether they are still open\n> or not. To counteract this incompatibility, we have some custom error\n> handling in the `mingw_unlink()` wrapper that first retries the deletion\n> with some delay, and then asks the user whether we should continue to\n> retry.\n>\n> While this logic might be sensible in many callsites throughout Git, it\n> is less when used in the reftable library. We only use unlink(3) there\n> to delete tables which aren't referenced anymore, and the code is very\n> aware of the limitations on Windows. As such, all calls to unlink(3p)\n> don't perform any error checking at all and are fine with the call\n> failing.\n>\n> Instead, the library provides the `reftable_stack_clean()` function,\n> which Git knows to execute in git-pack-refs(1) after compacting a stack.\n> The effect of this function is that all stale tables will eventually get\n> deleted once they aren't kept open anymore.\n>\n> So while we're fine with unlink(3p) failing, the Windows-emulation of\n> that function will still perform several sleeps and ultimately end up\n> asking the user:\n>\n> $ git pack-refs\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n>\n> It even asks multiple times, which is doubly annoying and puzzling to\n> the user:\n>\n> 1. It asks when trying to delete the old file after having written the\n> compacted stack.\n>\n> 2. It asks when reloading the stack, where it will try to unlink\n> now-unreferenced tables.\n>\n> 3. It asks when calling `reftable_stack_clean()`, where it will try to\n> unlink now-stale tables.\n>\n> Fix the issue by making it possible to disable this behaviour with a\n> preprocessor define. As \"git-compat-util.h\" is only included from\n> \"system.h\", and given that \"system.h\" is only ever included by headers\n> and code that are internal to the reftable library, we can set that\n> macro in this header without impacting anything else but the reftable\n> library.\n>\n> Reported-by: Christian Reich \n> Signed-off-by: Patrick Steinhardt \n> ---\n> Hi,\n>\n> This patch fixes the issue reported in [1].\n>\n> Thanks!\n>\n> Patrick\n>\n> [1]: \n> ---\n> compat/mingw.c | 5 ++++-\n> compat/mingw.h | 8 ++++++--\n> reftable/system.h | 1 +\n> 3 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 1d5b211b54..0e4b6a70a4 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)\n> \treturn wbuf;\n> }\n> \n> -int mingw_unlink(const char *pathname)\n> +int mingw_unlink(const char *pathname, int handle_in_use_error)\n> {\n> \tint ret, tries = 0;\n> \twchar_t wpathname[MAX_PATH];\n> @@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)\n> \twhile ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n> \t\tif (!is_file_in_use_error(GetLastError()))\n> \t\t\tbreak;\n> +\t\tif (!handle_in_use_error)\n> +\t\t\treturn ret;\n> +\n> \t\t/*\n> \t\t * We assume that some other process had the source or\n> \t\t * destination file open at the wrong moment and retry.\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index ebfb8ba423..a555af8d54 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -224,8 +224,12 @@ int uname(struct utsname *buf);\n> * replacements of existing functions\n> */\n> \n> -int mingw_unlink(const char *pathname);\n> -#define unlink mingw_unlink\n> +int mingw_unlink(const char *pathname, int handle_in_use_error);\n> +#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n> +# define unlink(path) mingw_unlink(path, 0)\n> +#else\n> +# define unlink(path) mingw_unlink(path, 1)\n> +#endif\n> \n> int mingw_rmdir(const char *path);\n> #define rmdir mingw_rmdir\n> diff --git a/reftable/system.h b/reftable/system.h\n> index 5274eca1d0..fe94bf205b 100644\n> --- a/reftable/system.h\n> +++ b/reftable/system.h\n> @@ -13,6 +13,7 @@ license that can be found in the LICENSE file or at\n> \n> #define DISABLE_SIGN_COMPARE_WARNINGS\n> \n> +#define MINGW_DONT_HANDLE_IN_USE_ERROR\n> #include \"git-compat-util.h\"\n> \n> /*\n>\n> ---\n> base-commit: 4e746b1a31f9f0036032b6f94279cf16fb363203\n> change-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7\n>\n"},{"id":"511200","messageId":"267314c0-3aba-4f69-9f41-89392391290f@kdbg.org","threadId":"62851","inReplyTo":"Z5ShoLCWREAsWZVh@pks.im","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-01-25T14:28:28Z","receivedAt":"2025-01-25T14:28:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.01.25 um 09:32 schrieb Patrick Steinhardt:\n> The user report was explicitly about compatibility with JGit, which\n> still had these files open. We don't have control over third-party\n> clients and how exactly they open files, so it is expected that we may\n> still see failures with the deletion of in-use files.\n\nFair enough.\n\n> I'd be happy to hear about alternative ideas that didn't came to my\n> mind.\n\nInstead of calling _wunlink() in mingw_unlink, we could CreateFileW()\nwith access mode DELETE and flag FILE_FLAG_DELETE_ON_CLOSE, then close\nthe file right away. That would apply semantics that is similar, but not\nquite, POSIX at least among the files that we open ourselves.\n\nIt would be even better that we do not depend on the POSIX behavior in\nthe first place. As you said, the reftable library can live with failed\ndeletes. And I don't think we depend on the POSIX behavior anywhere else\nbecause we would see the \"try again?\" question much more frequently than\nwe do right now.\n\n-- Hannes\n\n"},{"id":"511205","messageId":"xmqqfrl6wdux.fsf@gitster.g","threadId":"62851","inReplyTo":"20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-26T01:41:42Z","receivedAt":"2025-01-26T01:41:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 1d5b211b54..0e4b6a70a4 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)\n> \treturn wbuf;\n> }\n> \n> -int mingw_unlink(const char *pathname)\n> +int mingw_unlink(const char *pathname, int handle_in_use_error)\n> {\n> \tint ret, tries = 0;\n> \twchar_t wpathname[MAX_PATH];\n> @@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)\n> \twhile ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n> \t\tif (!is_file_in_use_error(GetLastError()))\n> \t\t\tbreak;\n> +\t\tif (!handle_in_use_error)\n> +\t\t\treturn ret;\n> +\n> \t\t/*\n> \t\t * We assume that some other process had the source or\n> \t\t * destination file open at the wrong moment and retry.\n\nSo this is how we can avoid falling into the retry plus interaction.\nThis underlying function is prepared to offer both choices at\nruntime.\n\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index ebfb8ba423..a555af8d54 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -224,8 +224,12 @@ int uname(struct utsname *buf);\n> * replacements of existing functions\n> */\n> \n> -int mingw_unlink(const char *pathname);\n> -#define unlink mingw_unlink\n> +int mingw_unlink(const char *pathname, int handle_in_use_error);\n> +#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n> +# define unlink(path) mingw_unlink(path, 0)\n> +#else\n> +# define unlink(path) mingw_unlink(path, 1)\n> +#endif\n\nThis one is yucky. All calls to unlink() used in compilation units\nwith the CPP macro defined are going to fail on a path that is in\nuse, but in other code paths, there will be the retry loop.\n\nRegardless of the platform, the code must be prepared to see its\nunlink() fail and deal with the failure, but I wonder how much the\ninitial \"if file-in-use caused problem, retry with delay without\nbugging the end user\" loop is helping. \n\nAfter that retry loop expires, we go interactive, and I can\nunderstand why end-users may be annoyed by the code going\ninteractive at that point.\n\nBut wouldn't it be too drastic a change to break out of the retry\nloop immediately after the initial failure?\n\nUnless the case found in reftable is that the process that has the\nfile in use is ourselves but somebody else that is not under our\ncontrol, it could be that the current users are being helped by the\nretry loop because these other users would quickly close and exit\nwhile we are retrying before going interactive. What I am getting\nat is that it might be a less drastic move that helps users better\nif we moved the \"let's just accept the failure and return to the\ncaller\" after that non-interactive retry loop, instead of \"return\nafter even the first failure.\" That way, we'll still keep the\nautomatic and non-interactive recovery, and punt a bit earlier than\nbefore before we go into the other interactive retry loop.\n\nOf course, if we are depending on the ability to unlink what _we_\nourselves are using, we should stop doing that by reorganizing the\ncode. I recall we have done such a code shuffling to avoid removing\nopen files by flipping the order between unlink and close before\nonly to mollify Windows already in other code paths. But if we are\nfailing due to random other users having the file open at the same\ntime, at least the earlier non-interactive retry loop sounds like a\nreasonable workaround for quirks in the underlying filesystem to me.\n\nThanks.\n\n\n"},{"id":"511228","messageId":"Z5c6VlsdNXwYkUsv@pks.im","threadId":"62851","inReplyTo":"267314c0-3aba-4f69-9f41-89392391290f@kdbg.org","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-27T07:48:38Z","receivedAt":"2025-01-27T07:48:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jan 25, 2025 at 03:28:28PM +0100, Johannes Sixt wrote:\n> Am 25.01.25 um 09:32 schrieb Patrick Steinhardt:\n> > The user report was explicitly about compatibility with JGit, which\n> > still had these files open. We don't have control over third-party\n> > clients and how exactly they open files, so it is expected that we may\n> > still see failures with the deletion of in-use files.\n> \n> Fair enough.\n> \n> > I'd be happy to hear about alternative ideas that didn't came to my\n> > mind.\n> \n> Instead of calling _wunlink() in mingw_unlink, we could CreateFileW()\n> with access mode DELETE and flag FILE_FLAG_DELETE_ON_CLOSE, then close\n> the file right away. That would apply semantics that is similar, but not\n> quite, POSIX at least among the files that we open ourselves.\n\nHuh. And that works even when the file is still being held open by other\nprocesses? I don't know enough about Windows to be sure there and\nwouldn't quite feel comfortable with pushing a change like this into\n`unlink()` given that it is used in so many places by Git.\n\n> It would be even better that we do not depend on the POSIX behavior in\n> the first place. As you said, the reftable library can live with failed\n> deletes. And I don't think we depend on the POSIX behavior anywhere else\n> because we would see the \"try again?\" question much more frequently than\n> we do right now.\n\nI have a feeling that there's a misunderstanding here, either on my side\nor on yours. It's the rest of Git that wants to have POSIX behaviour for\n`unlink()`, not the reftable library. In the reftable library we don't\ncare at all whether or not the file got deleted -- we can live with it\nand know to retry at a later point. But because we use Git's `unlink()`\nimplementation we get the \"try again?\" questions for free, even though\nwe don't want to have it in the first place.\n\nSo the proposed fix is to _disable_ the POSIX emulation provided by Git\nin the reftable library. Which seems to be what you're proposing?\n\nLet me know in case I misunderstood, I'm a bit confused right now :)\n\nPatrick\n"},{"id":"511229","messageId":"Z5c8aEVP7NLpMN3N@pks.im","threadId":"62851","inReplyTo":"xmqqfrl6wdux.fsf@gitster.g","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-27T07:57:28Z","receivedAt":"2025-01-27T07:57:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jan 25, 2025 at 05:41:42PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > diff --git a/compat/mingw.h b/compat/mingw.h\n> > index ebfb8ba423..a555af8d54 100644\n> > --- a/compat/mingw.h\n> > +++ b/compat/mingw.h\n> > @@ -224,8 +224,12 @@ int uname(struct utsname *buf);\n> > * replacements of existing functions\n> > */\n> > \n> > -int mingw_unlink(const char *pathname);\n> > -#define unlink mingw_unlink\n> > +int mingw_unlink(const char *pathname, int handle_in_use_error);\n> > +#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n> > +# define unlink(path) mingw_unlink(path, 0)\n> > +#else\n> > +# define unlink(path) mingw_unlink(path, 1)\n> > +#endif\n> \n> This one is yucky. All calls to unlink() used in compilation units\n> with the CPP macro defined are going to fail on a path that is in\n> use, but in other code paths, there will be the retry loop.\n\nYeah, I don't like it either, but couldn't think of a better way to do\nthis.\n\n> Regardless of the platform, the code must be prepared to see its\n> unlink() fail and deal with the failure, but I wonder how much the\n> initial \"if file-in-use caused problem, retry with delay without\n> bugging the end user\" loop is helping. \n> \n> After that retry loop expires, we go interactive, and I can\n> understand why end-users may be annoyed by the code going\n> interactive at that point.\n> \n> But wouldn't it be too drastic a change to break out of the retry\n> loop immediately after the initial failure?\n\nIn general: yes. For the reftable library: no. The reftable backend\nalready knows to handle this situation just fine by simply retrying the\ndeletion on the next call to git-pack-refs(1). So the file will\neventually be removed, and it doesn't hurt to have it sitting around as\na stale file meanwhile.\n\n> Unless the case found in reftable is that the process that has the\n> file in use is ourselves but somebody else that is not under our\n> control, it could be that the current users are being helped by the\n> retry loop because these other users would quickly close and exit\n> while we are retrying before going interactive. What I am getting\n> at is that it might be a less drastic move that helps users better\n> if we moved the \"let's just accept the failure and return to the\n> caller\" after that non-interactive retry loop, instead of \"return\n> after even the first failure.\" That way, we'll still keep the\n> automatic and non-interactive recovery, and punt a bit earlier than\n> before before we go into the other interactive retry loop.\n\nIt would incur a delay though that is basically unnecessary. The file is\nnot being held open by the same process, but by a different one that is\nnot ourselves. And because Git opens with `FILE_SHARE_DELETE` on Windows\nwe even know that it wouldn't be a Git process that has the file open,\nbut something else (e.g. JGit, as reported by the user).\n\nChances are slim that such a third-party client would close the file in\nthe exact 71 milliseconds that we'd be sleeping for. And combined with\nthe fact that we know to clean up the file at a later point anyway I\nthink I'd rather not incur a user-facing delay only for a slight chance\nthat we might succeed unlinking the file.\n\n> Of course, if we are depending on the ability to unlink what _we_\n> ourselves are using, we should stop doing that by reorganizing the\n> code. I recall we have done such a code shuffling to avoid removing\n> open files by flipping the order between unlink and close before\n> only to mollify Windows already in other code paths. But if we are\n> failing due to random other users having the file open at the same\n> time, at least the earlier non-interactive retry loop sounds like a\n> reasonable workaround for quirks in the underlying filesystem to me.\n\nAs far as I'm aware this isn't an issue in the reftable library, we're\nbeing quite careful there. I'm quite sure that we'd otherwise have\ngotten reports about that before seeing reports about JGit keeping the\nfiles open.\n\nPatrick\n"},{"id":"511230","messageId":"Z5c8no4X_58AzdcY@pks.im","threadId":"62851","inReplyTo":"08d7e79e-3a59-4163-be2b-97f0238142aa@t-online.de","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-27T07:58:22Z","receivedAt":"2025-01-27T07:58:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jan 25, 2025 at 09:45:21AM +0100, Christian Reich wrote:\n> Greetings Patrick,\n> \n> thx you for the patch. I build an own version of git for windows with this\n> patch and it works as expected! *thumbsup*\n\nThanks for checking!\n\n> But I see merge conflicts in git for windows main-branch:\n\nThis will be dealt with by the respective maintainers eventually. I've\nalready Cc'd Johannes for input, who is the Git for Windows maintainer.\n\nPatrick\n"},{"id":"511313","messageId":"b9b548e0-e42e-4854-92f2-337a26f692ac@kdbg.org","threadId":"62851","inReplyTo":"Z5c6VlsdNXwYkUsv@pks.im","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-01-28T06:52:48Z","receivedAt":"2025-01-28T06:52:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.01.25 um 08:48 schrieb Patrick Steinhardt:\n> On Sat, Jan 25, 2025 at 03:28:28PM +0100, Johannes Sixt wrote:\n>> Am 25.01.25 um 09:32 schrieb Patrick Steinhardt:\n>> Instead of calling _wunlink() in mingw_unlink, we could CreateFileW()\n>> with access mode DELETE and flag FILE_FLAG_DELETE_ON_CLOSE, then close\n>> the file right away. That would apply semantics that is similar, but not\n>> quite, POSIX at least among the files that we open ourselves.\n> \n> Huh. And that works even when the file is still being held open by other\n> processes?\n\nOnly if the process cooperates and has opened the file with\nFILE_SHARE_DELETE. So, in general, no.\n\nI think, Cygwin implements unlink() in this manner.\n\n> I have a feeling that there's a misunderstanding here, either on my side\n> or on yours. It's the rest of Git that wants to have POSIX behaviour for\n> `unlink()`, not the reftable library.\n\nYes and no. Yes, we expect to be able to delete a file that was opened\nby some *other* Git process (e.g., packfiles during gc), but, no, we do\nnot delete files that have been opened in the current process and are\nstill open.\n\nFor this reason, I am arguing to remove the interactive part of\nmingw_unlink() and use the cooperative strategy I mentioned above. That\ngives us POSIX-like behavior for concurrent Git processes.\n\nThe interactive question is only useful when the user has control over\nan uncooperative process that keeps a file open for an extended time and\ncan find that processes, which is either obvious or extremely difficult.\nAs I said, I haven't seen the question since a long, long time now, but\nI am also the first to admit that my way of using Git is rather narrow.\n\n-- Hannes\n\n"},{"id":"511314","messageId":"Z5iEcjJpUI3spSrB@pks.im","threadId":"62851","inReplyTo":"b9b548e0-e42e-4854-92f2-337a26f692ac@kdbg.org","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-28T07:17:06Z","receivedAt":"2025-01-28T07:17:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 28, 2025 at 07:52:48AM +0100, Johannes Sixt wrote:\n> Am 27.01.25 um 08:48 schrieb Patrick Steinhardt:\n> > On Sat, Jan 25, 2025 at 03:28:28PM +0100, Johannes Sixt wrote:\n> >> Am 25.01.25 um 09:32 schrieb Patrick Steinhardt:\n> > I have a feeling that there's a misunderstanding here, either on my side\n> > or on yours. It's the rest of Git that wants to have POSIX behaviour for\n> > `unlink()`, not the reftable library.\n> \n> Yes and no. Yes, we expect to be able to delete a file that was opened\n> by some *other* Git process (e.g., packfiles during gc), but, no, we do\n> not delete files that have been opened in the current process and are\n> still open.\n> \n> For this reason, I am arguing to remove the interactive part of\n> mingw_unlink() and use the cooperative strategy I mentioned above. That\n> gives us POSIX-like behavior for concurrent Git processes.\n> \n> The interactive question is only useful when the user has control over\n> an uncooperative process that keeps a file open for an extended time and\n> can find that processes, which is either obvious or extremely difficult.\n> As I said, I haven't seen the question since a long, long time now, but\n> I am also the first to admit that my way of using Git is rather narrow.\n\nThat would be a much wider change compared to what I'm proposing though.\nI don't quite feel comfortable with pushing for such a change as I don't\nhave enough of a stake in Windows to be able to judge whether it would\nbe sensible or not.\n\nIf Dscho confirms your take I'm happy to do so. But otherwise I'd prefer\nto continue with the more limited scope, as I know that the behaviour is\nunexpected and unnecessary in the reftable library.\n\nPatrick\n"},{"id":"511400","messageId":"37c14379-68e4-4c52-a08d-b78c7797ef26@kdbg.org","threadId":"62851","inReplyTo":"Z5iEcjJpUI3spSrB@pks.im","subject":"Re: [PATCH] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-01-29T07:40:09Z","receivedAt":"2025-01-29T07:40:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 28.01.25 um 08:17 schrieb Patrick Steinhardt:\n> On Tue, Jan 28, 2025 at 07:52:48AM +0100, Johannes Sixt wrote:\n>> The interactive question is only useful when the user has control over\n>> an uncooperative process that keeps a file open for an extended time and\n>> can find that processes, which is either obvious or extremely difficult.\n>> As I said, I haven't seen the question since a long, long time now, but\n>> I am also the first to admit that my way of using Git is rather narrow.\n> \n> That would be a much wider change compared to what I'm proposing though.\n> I don't quite feel comfortable with pushing for such a change as I don't\n> have enough of a stake in Windows to be able to judge whether it would\n> be sensible or not.\n> \n> If Dscho confirms your take I'm happy to do so. But otherwise I'd prefer\n> to continue with the more limited scope, as I know that the behaviour is\n> unexpected and unnecessary in the reftable library.\n\nI can live with the narrow workaround that you proposed. A more thorough\nrewrite like I propose would have to cook for quite some time before it\ncan be released to the general public.\n\n-- Hannes\n\n"},{"id":"511965","messageId":"20250206-b4-pks-reftable-win32-in-use-errors-v2-1-56985a4f6186@pks.im","threadId":"62851","inReplyTo":"20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im","subject":"[PATCH v2] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-06T07:53:58Z","receivedAt":"2025-02-06T07:54:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Unlinking a file may fail on Windows systems when the file is still held\nopen by another process. This is incompatible with POSIX semantics and\nby extension with Git's assumed semantics when unlinking files, which\nis that files can be unlinked regardless of whether they are still open\nor not. To counteract this incompatibility, we have some custom error\nhandling in the `mingw_unlink()` wrapper that first retries the deletion\nwith some delay, and then asks the user whether we should continue to\nretry.\n\nWhile this logic might be sensible in many callsites throughout Git, it\nis less when used in the reftable library. We only use unlink(3) there\nto delete tables which aren't referenced anymore, and the code is very\naware of the limitations on Windows. As such, all calls to unlink(3p)\ndon't perform any error checking at all and are fine with the call\nfailing.\n\nInstead, the library provides the `reftable_stack_clean()` function,\nwhich Git knows to execute in git-pack-refs(1) after compacting a stack.\nThe effect of this function is that all stale tables will eventually get\ndeleted once they aren't kept open anymore.\n\nSo while we're fine with unlink(3p) failing, the Windows-emulation of\nthat function will still perform several sleeps and ultimately end up\nasking the user:\n\n $ git pack-refs\n Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n\nIt even asks multiple times, which is doubly annoying and puzzling to\nthe user:\n\n 1. It asks when trying to delete the old file after having written the\n compacted stack.\n\n 2. It asks when reloading the stack, where it will try to unlink\n now-unreferenced tables.\n\n 3. It asks when calling `reftable_stack_clean()`, where it will try to\n unlink now-stale tables.\n\nFix the issue by making it possible to disable this behaviour with a\npreprocessor define. As \"git-compat-util.h\" is only included from\n\"system.h\", and given that \"system.h\" is only ever included by headers\nand code that are internal to the reftable library, we can set that\nmacro in this header without impacting anything else but the reftable\nlibrary.\n\nReported-by: Christian Reich \nSigned-off-by: Patrick Steinhardt \n---\nHi,\n\nThis patch fixes the issue reported in [1].\n\nChanges in v2:\n - Rebased the patch on top of ps/reftable-sans-compat-util at\n 3f172f1391 (Makefile: skip reftable library for Coccinelle,\n 2025-02-03). This is done to fix a semantic merge conflict.\n - Link to v1: https://lore.kernel.org/r/20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im\n\nThanks!\n\nPatrick\n\n[1]: \n---\n compat/mingw/compat-util.c | 5 ++++-\n compat/mingw/posix.h | 8 ++++++--\n reftable/system.h | 1 +\n 3 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/mingw/compat-util.c b/compat/mingw/compat-util.c\nindex 1d5b211b54..0e4b6a70a4 100644\n--- a/compat/mingw/compat-util.c\n+++ b/compat/mingw/compat-util.c\n@@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)\n \treturn wbuf;\n }\n \n-int mingw_unlink(const char *pathname)\n+int mingw_unlink(const char *pathname, int handle_in_use_error)\n {\n \tint ret, tries = 0;\n \twchar_t wpathname[MAX_PATH];\n@@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)\n \twhile ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n \t\tif (!is_file_in_use_error(GetLastError()))\n \t\t\tbreak;\n+\t\tif (!handle_in_use_error)\n+\t\t\treturn ret;\n+\n \t\t/*\n \t\t * We assume that some other process had the source or\n \t\t * destination file open at the wrong moment and retry.\ndiff --git a/compat/mingw/posix.h b/compat/mingw/posix.h\nindex 8dddfa818d..88e0cf9292 100644\n--- a/compat/mingw/posix.h\n+++ b/compat/mingw/posix.h\n@@ -201,8 +201,12 @@ int uname(struct utsname *buf);\n * replacements of existing functions\n */\n \n-int mingw_unlink(const char *pathname);\n-#define unlink mingw_unlink\n+int mingw_unlink(const char *pathname, int handle_in_use_error);\n+#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n+# define unlink(path) mingw_unlink(path, 0)\n+#else\n+# define unlink(path) mingw_unlink(path, 1)\n+#endif\n \n int mingw_rmdir(const char *path);\n #define rmdir mingw_rmdir\ndiff --git a/reftable/system.h b/reftable/system.h\nindex dccdf11f76..1492bf6d70 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at\n \n /* This header glues the reftable library to the rest of Git */\n \n+#define MINGW_DONT_HANDLE_IN_USE_ERROR\n #include \"../compat/posix.h\"\n #include \n \n\n---\nbase-commit: 0fb5c2116049c665c6550d7e0419971a277af345\nchange-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7\n\n"},{"id":"514267","messageId":"de954c02-8711-43e9-8f4c-259718034497@t-online.de","threadId":"62851","inReplyTo":"20250206-b4-pks-reftable-win32-in-use-errors-v2-1-56985a4f6186@pks.im","subject":"Re: [PATCH v2] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Christian Reich","fromEmail":"zottelbart@t-online.de","sentAt":"2025-03-14T14:18:55Z","receivedAt":"2025-03-14T14:24:24Z","isPatch":true,"sender":{"key":"zottelbart@t-online.de","avatar":null},"body":"Greetings,\n\nthe patch didn't find the way in next git version.\n\nBut there is a change in git for windows, which prevents multiple asks \nthe same questions:\n\nhttps://github.com/git-for-windows/git/commit/dba1473162b4319a0a2c7f74ab48ed4a826f7ac7\n\nThe other option to prevent the asks, is to set GIT_ASK_YESNO but the \nsleeps slow down the process.\n\nBest Regards\n\nChristian\n\nAm 06.02.2025 um 08:53 schrieb Patrick Steinhardt:\n> Unlinking a file may fail on Windows systems when the file is still held\n> open by another process. This is incompatible with POSIX semantics and\n> by extension with Git's assumed semantics when unlinking files, which\n> is that files can be unlinked regardless of whether they are still open\n> or not. To counteract this incompatibility, we have some custom error\n> handling in the `mingw_unlink()` wrapper that first retries the deletion\n> with some delay, and then asks the user whether we should continue to\n> retry.\n>\n> While this logic might be sensible in many callsites throughout Git, it\n> is less when used in the reftable library. We only use unlink(3) there\n> to delete tables which aren't referenced anymore, and the code is very\n> aware of the limitations on Windows. As such, all calls to unlink(3p)\n> don't perform any error checking at all and are fine with the call\n> failing.\n>\n> Instead, the library provides the `reftable_stack_clean()` function,\n> which Git knows to execute in git-pack-refs(1) after compacting a stack.\n> The effect of this function is that all stale tables will eventually get\n> deleted once they aren't kept open anymore.\n>\n> So while we're fine with unlink(3p) failing, the Windows-emulation of\n> that function will still perform several sleeps and ultimately end up\n> asking the user:\n>\n> $ git pack-refs\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n>\n> It even asks multiple times, which is doubly annoying and puzzling to\n> the user:\n>\n> 1. It asks when trying to delete the old file after having written the\n> compacted stack.\n>\n> 2. It asks when reloading the stack, where it will try to unlink\n> now-unreferenced tables.\n>\n> 3. It asks when calling `reftable_stack_clean()`, where it will try to\n> unlink now-stale tables.\n>\n> Fix the issue by making it possible to disable this behaviour with a\n> preprocessor define. As \"git-compat-util.h\" is only included from\n> \"system.h\", and given that \"system.h\" is only ever included by headers\n> and code that are internal to the reftable library, we can set that\n> macro in this header without impacting anything else but the reftable\n> library.\n>\n> Reported-by: Christian Reich \n> Signed-off-by: Patrick Steinhardt \n> ---\n> Hi,\n>\n> This patch fixes the issue reported in [1].\n>\n> Changes in v2:\n> - Rebased the patch on top of ps/reftable-sans-compat-util at\n> 3f172f1391 (Makefile: skip reftable library for Coccinelle,\n> 2025-02-03). This is done to fix a semantic merge conflict.\n> - Link to v1: https://lore.kernel.org/r/20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im\n>\n> Thanks!\n>\n> Patrick\n>\n> [1]: \n> ---\n> compat/mingw/compat-util.c | 5 ++++-\n> compat/mingw/posix.h | 8 ++++++--\n> reftable/system.h | 1 +\n> 3 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/compat/mingw/compat-util.c b/compat/mingw/compat-util.c\n> index 1d5b211b54..0e4b6a70a4 100644\n> --- a/compat/mingw/compat-util.c\n> +++ b/compat/mingw/compat-util.c\n> @@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)\n> \treturn wbuf;\n> }\n> \n> -int mingw_unlink(const char *pathname)\n> +int mingw_unlink(const char *pathname, int handle_in_use_error)\n> {\n> \tint ret, tries = 0;\n> \twchar_t wpathname[MAX_PATH];\n> @@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)\n> \twhile ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n> \t\tif (!is_file_in_use_error(GetLastError()))\n> \t\t\tbreak;\n> +\t\tif (!handle_in_use_error)\n> +\t\t\treturn ret;\n> +\n> \t\t/*\n> \t\t * We assume that some other process had the source or\n> \t\t * destination file open at the wrong moment and retry.\n> diff --git a/compat/mingw/posix.h b/compat/mingw/posix.h\n> index 8dddfa818d..88e0cf9292 100644\n> --- a/compat/mingw/posix.h\n> +++ b/compat/mingw/posix.h\n> @@ -201,8 +201,12 @@ int uname(struct utsname *buf);\n> * replacements of existing functions\n> */\n> \n> -int mingw_unlink(const char *pathname);\n> -#define unlink mingw_unlink\n> +int mingw_unlink(const char *pathname, int handle_in_use_error);\n> +#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n> +# define unlink(path) mingw_unlink(path, 0)\n> +#else\n> +# define unlink(path) mingw_unlink(path, 1)\n> +#endif\n> \n> int mingw_rmdir(const char *path);\n> #define rmdir mingw_rmdir\n> diff --git a/reftable/system.h b/reftable/system.h\n> index dccdf11f76..1492bf6d70 100644\n> --- a/reftable/system.h\n> +++ b/reftable/system.h\n> @@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at\n> \n> /* This header glues the reftable library to the rest of Git */\n> \n> +#define MINGW_DONT_HANDLE_IN_USE_ERROR\n> #include \"../compat/posix.h\"\n> #include \n> \n>\n> ---\n> base-commit: 0fb5c2116049c665c6550d7e0419971a277af345\n> change-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7\n>\n"},{"id":"514268","messageId":"Z9REjHgrhzUzWoqJ@pks.im","threadId":"62851","inReplyTo":"de954c02-8711-43e9-8f4c-259718034497@t-online.de","subject":"Re: [PATCH v2] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-14T15:00:28Z","receivedAt":"2025-03-14T15:00:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 14, 2025 at 03:18:55PM +0100, Christian Reich wrote:\n> Greetings,\n> \n> the patch didn't find the way in next git version.\n> \n> But there is a change in git for windows, which prevents multiple asks the\n> same questions:\n> \n> https://github.com/git-for-windows/git/commit/dba1473162b4319a0a2c7f74ab48ed4a826f7ac7\n> \n> The other option to prevent the asks, is to set GIT_ASK_YESNO but the sleeps\n> slow down the process.\n\nThe patch is still blocked because [1] hasn't landed yet. Once it does I\nexpect the patch to be merged. But yes, this will not be part of Git\nv2.49, but of the next release.\n\nPatrick\n\n[1]: <20250127-pks-reftable-drop-git-compat-util-v1-0-6e280a564877@pks.im>\n"},{"id":"514364","messageId":"920228d0-5906-27f9-d42d-d92631909fd8@gmx.de","threadId":"62851","inReplyTo":"20250206-b4-pks-reftable-win32-in-use-errors-v2-1-56985a4f6186@pks.im","subject":"Re: [PATCH v2] reftable: ignore file-in-use errors when unlink(3p) fails on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-15T23:17:23Z","receivedAt":"2025-03-15T23:17:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 6 Feb 2025, Patrick Steinhardt wrote:\n\n> Unlinking a file may fail on Windows systems when the file is still held\n> open by another process. This is incompatible with POSIX semantics and\n> by extension with Git's assumed semantics when unlinking files, which\n> is that files can be unlinked regardless of whether they are still open\n> or not. To counteract this incompatibility, we have some custom error\n> handling in the `mingw_unlink()` wrapper that first retries the deletion\n> with some delay, and then asks the user whether we should continue to\n> retry.\n>\n> While this logic might be sensible in many callsites throughout Git, it\n> is less when used in the reftable library. We only use unlink(3) there\n> to delete tables which aren't referenced anymore, and the code is very\n> aware of the limitations on Windows. As such, all calls to unlink(3p)\n> don't perform any error checking at all and are fine with the call\n> failing.\n>\n> Instead, the library provides the `reftable_stack_clean()` function,\n> which Git knows to execute in git-pack-refs(1) after compacting a stack.\n> The effect of this function is that all stale tables will eventually get\n> deleted once they aren't kept open anymore.\n>\n> So while we're fine with unlink(3p) failing, the Windows-emulation of\n> that function will still perform several sleeps and ultimately end up\n> asking the user:\n>\n> $ git pack-refs\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n> Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n\n>\n> It even asks multiple times, which is doubly annoying and puzzling to\n> the user:\n>\n> 1. It asks when trying to delete the old file after having written the\n> compacted stack.\n>\n> 2. It asks when reloading the stack, where it will try to unlink\n> now-unreferenced tables.\n>\n> 3. It asks when calling `reftable_stack_clean()`, where it will try to\n> unlink now-stale tables.\n>\n> Fix the issue by making it possible to disable this behaviour with a\n> preprocessor define. As \"git-compat-util.h\" is only included from\n> \"system.h\", and given that \"system.h\" is only ever included by headers\n> and code that are internal to the reftable library, we can set that\n> macro in this header without impacting anything else but the reftable\n> library.\n>\n> Reported-by: Christian Reich \n> Signed-off-by: Patrick Steinhardt \n> ---\n> Hi,\n>\n> This patch fixes the issue reported in [1].\n>\n> Changes in v2:\n> - Rebased the patch on top of ps/reftable-sans-compat-util at\n> 3f172f1391 (Makefile: skip reftable library for Coccinelle,\n> 2025-02-03). This is done to fix a semantic merge conflict.\n> - Link to v1: https://lore.kernel.org/r/20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im\n>\n> Thanks!\n>\n> Patrick\n>\n> [1]: \n> ---\n> compat/mingw/compat-util.c | 5 ++++-\n> compat/mingw/posix.h | 8 ++++++--\n> reftable/system.h | 1 +\n> 3 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/compat/mingw/compat-util.c b/compat/mingw/compat-util.c\n> index 1d5b211b54..0e4b6a70a4 100644\n> --- a/compat/mingw/compat-util.c\n> +++ b/compat/mingw/compat-util.c\n> @@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)\n> \treturn wbuf;\n> }\n>\n> -int mingw_unlink(const char *pathname)\n> +int mingw_unlink(const char *pathname, int handle_in_use_error)\n> {\n> \tint ret, tries = 0;\n> \twchar_t wpathname[MAX_PATH];\n> @@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)\n> \twhile ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {\n> \t\tif (!is_file_in_use_error(GetLastError()))\n> \t\t\tbreak;\n> +\t\tif (!handle_in_use_error)\n> +\t\t\treturn ret;\n> +\n> \t\t/*\n> \t\t * We assume that some other process had the source or\n> \t\t * destination file open at the wrong moment and retry.\n> diff --git a/compat/mingw/posix.h b/compat/mingw/posix.h\n> index 8dddfa818d..88e0cf9292 100644\n> --- a/compat/mingw/posix.h\n> +++ b/compat/mingw/posix.h\n> @@ -201,8 +201,12 @@ int uname(struct utsname *buf);\n> * replacements of existing functions\n> */\n>\n> -int mingw_unlink(const char *pathname);\n> -#define unlink mingw_unlink\n> +int mingw_unlink(const char *pathname, int handle_in_use_error);\n> +#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR\n> +# define unlink(path) mingw_unlink(path, 0)\n> +#else\n> +# define unlink(path) mingw_unlink(path, 1)\n> +#endif\n>\n> int mingw_rmdir(const char *path);\n> #define rmdir mingw_rmdir\n> diff --git a/reftable/system.h b/reftable/system.h\n> index dccdf11f76..1492bf6d70 100644\n> --- a/reftable/system.h\n> +++ b/reftable/system.h\n> @@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at\n>\n> /* This header glues the reftable library to the rest of Git */\n>\n> +#define MINGW_DONT_HANDLE_IN_USE_ERROR\n> #include \"../compat/posix.h\"\n> #include \n\nIt's the pragmatic thing to do.\n\nA cleaner fix would be to introduce a proper function that says in its\nname that it tries to delete a file on a best-effort basis but won't\ninsist if that's not possible at the time.\n\nAs I have communicated with you on similar issues in the past, my\nperspective is that Git fails to abstract operating system-specific\nfunctionality correctly, and instead (ab-)uses POSIX/libc functions trying\nto do so. This is one of the things Subversion still does better. That's\nwhy we now have the unfortunate need to change the function signature of\n`mingw_unlink()` to something distinctly unlike `unlink()` (which\n`mingw_unlink()` originally clearly tried to emulate in an effort to\npretend to Git that it's okay to assume that every operating system\nbehaves like Linux).\n\nWhat strikes me as even less desirable is to _still_ keep the function\nsignature of `unlink()` the same, changing instead the behavior per-file\nvia that macro definition.\n\nUnfortunately, the cleaner fix would require to undo years of working\naround the lack of a proper platform abstraction layer, which would not\nonly be a huge amount of effort, but once again cause a lot of downstream\npain in Git for Windows, therefore I would actually oppose such an effort.\n\ntl;dr I will live with this patch.\n\nCiao,\nJohannes\n\n>\n>\n> ---\n> base-commit: 0fb5c2116049c665c6550d7e0419971a277af345\n> change-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7\n>\n>\n"}]}