{"thread":{"id":"57482","subject":"[PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","startedAt":"2022-02-28T09:40:44Z","lastAt":"2022-03-09T21:53:32Z","messageCount":30,"participants":["Tao Klerks via GitGitGadget","Ævar Arnfjörð Bjarmason","Jeff Hostetler","Junio C Hamano","Tao Klerks","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"449733","messageId":"pull.1166.git.1646041236.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":null,"subject":"[PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-28T09:40:34Z","receivedAt":"2022-02-28T09:40:44Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number\nof test cases in t\\t7063-status-untracked-cache.sh explicitly sleep a\nsecond, in order to avoid the untracked cache content being invalidated by\nan mtime race condition.\n\nEven though it's only 9 seconds of sleeping that can be straightforwardly\nreplaced, it seems worth fixing if possible.\n\nReplace sleep calls with backdating of filesystem changes, but first fix the\ntest-tool chmtime functionality to work for directories in Windows.\n\nI do have a question to the list here: Do mingw.c changes need to be\nupstreamed somewhere? I don't understand the exact relationship between this\nfile and the MinGW project.\n\nTao Klerks (2):\n t/helper/test-chmtime: update mingw to support chmtime on directories\n t7063: mtime-mangling instead of delays in untracked cache testing\n\n compat/mingw.c | 33 ++++++++++++++++++++++++++-----\n t/t7063-status-untracked-cache.sh | 29 ++++++++++++++++-----------\n 2 files changed, 45 insertions(+), 17 deletions(-)\n\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1166\n-- \ngitgitgadget\n"},{"id":"449734","messageId":"76b6216281e3463821e650495f3090c677905f73.1646041236.git.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.git.1646041236.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-28T09:40:35Z","receivedAt":"2022-02-28T09:40:46Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nThe mingw_utime implementation in mingw.c does not support\ndirectories. This means that \"test-tool chmtime\" fails on Windows when\ntargeting directories. This has previously been noted and sidestepped\nby Jeff Hostetler, in \"t/helper/test-chmtime: skip directories\non Windows\" in the \"Builtin FSMonitor Part 2\" work.\n\nIt would make sense to backdate file and folder changes in untracked\ncache tests, to avoid needing to insert explicit delays/pauses in the\ntests.\n\nAdd support for directory date manipulation in mingw_utime by calling\nCreateFileW() explicitly to create the directory handle, and\nCloseHandle() to close it.\n\nSigned-off-by: Tao Klerks \n---\n compat/mingw.c | 33 ++++++++++++++++++++++++++++-----\n 1 file changed, 28 insertions(+), 5 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 03af369b2b9..2284ea90511 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \tint fh, rc;\n \tDWORD attrs;\n \twchar_t wfilename[MAX_PATH];\n+\tHANDLE osfilehandle;\n+\n \tif (xutftowcs_path(wfilename, file_name) < 0)\n \t\treturn -1;\n \n@@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n \t}\n \n-\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n-\t\trc = -1;\n-\t\tgoto revert_attrs;\n+\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n+\t\tfh = 0;\n+\t\tosfilehandle = CreateFileW(wfilename,\n+\t\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n+\t\t\t\t\t 0 /*FileShare.None*/,\n+\t\t\t\t\t NULL,\n+\t\t\t\t\t OPEN_EXISTING,\n+\t\t\t\t\t FILE_FLAG_BACKUP_SEMANTICS,\n+\t\t\t\t\t NULL);\n+\t\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n+\t\t\terrno = err_win_to_posix(GetLastError());\n+\t\t\trc = -1;\n+\t\t\tgoto revert_attrs;\n+\t\t}\n+\t} else {\n+\t\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n+\t\t\trc = -1;\n+\t\t\tgoto revert_attrs;\n+\t\t}\n+\t\tosfilehandle = (HANDLE)_get_osfhandle(fh);\n \t}\n \n \tif (times) {\n@@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tGetSystemTimeAsFileTime(&mft);\n \t\taft = mft;\n \t}\n-\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n+\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n \t} else\n \t\trc = 0;\n-\tclose(fh);\n+\n+\tif (fh)\n+\t\tclose(fh);\n+\telse if (osfilehandle)\n+\t\tCloseHandle(osfilehandle);\n \n revert_attrs:\n \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n-- \ngitgitgadget\n\n"},{"id":"449735","messageId":"a1806c56333ee96f9cda8c77ae56517181b691c0.1646041237.git.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.git.1646041236.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-28T09:40:36Z","receivedAt":"2022-02-28T09:40:48Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nThe untracked cache test uses an avoid_racy function to deal with\nan mtime-resolution challenge in testing: If an untracked cache\nentry's mtime falls in the same second as the mtime of the index\nthe untracked cache was stored in, then it cannot be trusted.\n\nExplicitly delaying tests is a simple effective strategy to\navoid these issues, but should be avoided where possible.\n\nSwitch from a delay-based strategy to instead backdating\nall file changes using test-tool chmtime, where that is an\noption, to shave 9 seconds off the test run time.\n\nDon't update test cases that delay for other reasons, for now at\nleast (4 seconds).\n\nSigned-off-by: Tao Klerks \n---\n t/t7063-status-untracked-cache.sh | 29 +++++++++++++++++------------\n 1 file changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex a0c123b0a77..039b4de8d25 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -24,6 +24,14 @@ sync_mtime () {\n \tfind . -type d -exec ls -ld {} + >/dev/null\n }\n \n+chmmtime_worktree_root () {\n+\t# chmtime doesnt handle relative paths on windows, so need\n+\t# to \"hardcode\" a reference to the worktree folder name.\n+\tcd .. &&\n+\ttest-tool chmtime $1 worktree &&\n+\tcd worktree\n+}\n+\n avoid_racy() {\n \tsleep 1\n }\n@@ -90,6 +98,9 @@ test_expect_success 'setup' '\n \tcd worktree &&\n \tmkdir done dtwo dthree &&\n \ttouch one two three done/one dtwo/two dthree/three &&\n+\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n+\ttest-tool chmtime =-300 done dtwo dthree &&\n+\tchmmtime_worktree_root =-300 &&\n \tgit add one two done/one &&\n \t: >.git/info/exclude &&\n \tgit update-index --untracked-cache &&\n@@ -142,7 +153,6 @@ two\n EOF\n \n test_expect_success 'status first time (empty cache)' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -166,7 +176,6 @@ test_expect_success 'untracked cache after first status' '\n '\n \n test_expect_success 'status second time (fully populated cache)' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -190,8 +199,8 @@ test_expect_success 'untracked cache after second status' '\n '\n \n test_expect_success 'modify in root directory, one dir invalidation' '\n-\tavoid_racy &&\n \t: >four &&\n+\ttest-tool chmtime =-240 four &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -241,7 +250,6 @@ EOF\n '\n \n test_expect_success 'new .gitignore invalidates recursively' '\n-\tavoid_racy &&\n \techo four >.gitignore &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n@@ -292,7 +300,6 @@ EOF\n '\n \n test_expect_success 'new info/exclude invalidates everything' '\n-\tavoid_racy &&\n \techo three >>.git/info/exclude &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n@@ -520,14 +527,14 @@ test_expect_success 'create/modify files, some of which are gitignored' '\n \techo three >done/three && # three is gitignored\n \techo four >done/four && # four is gitignored at a higher level\n \techo five >done/five && # five is not gitignored\n-\techo test >base && #we need to ensure that the root dir is touched\n-\trm base &&\n+\ttest-tool chmtime =-180 done/two done/three done/four done/five done &&\n+\t# we need to ensure that the root dir is touched (in the past);\n+\tchmmtime_worktree_root =-180 &&\n \tsync_mtime\n '\n \n test_expect_success 'test sparse status with untracked cache' '\n \t: >../trace.output &&\n-\tavoid_racy &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n \tiuc status --porcelain >../status.iuc &&\n@@ -570,7 +577,6 @@ EOF\n '\n \n test_expect_success 'test sparse status again with untracked cache' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n@@ -597,11 +603,11 @@ EOF\n test_expect_success 'set up for test of subdir and sparse checkouts' '\n \tmkdir done/sub &&\n \tmkdir done/sub/sub &&\n-\techo \"sub\" > done/sub/sub/file\n+\techo \"sub\" > done/sub/sub/file &&\n+\ttest-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done\n '\n \n test_expect_success 'test sparse status with untracked cache and subdir' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n@@ -651,7 +657,6 @@ EOF\n '\n \n test_expect_success 'test sparse status again with untracked cache and subdir' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n-- \ngitgitgadget\n"},{"id":"449744","messageId":"220228.86bkyr5k1n.gmgdl@evledraar.gmail.com","threadId":"57482","inReplyTo":"pull.1166.git.1646041236.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-28T11:03:04Z","receivedAt":"2022-02-28T11:04:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:\n\n> As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number\n> of test cases in t\\t7063-status-untracked-cache.sh explicitly sleep a\n> second, in order to avoid the untracked cache content being invalidated by\n> an mtime race condition.\n>\n> Even though it's only 9 seconds of sleeping that can be straightforwardly\n> replaced, it seems worth fixing if possible.\n>\n> Replace sleep calls with backdating of filesystem changes, but first fix the\n> test-tool chmtime functionality to work for directories in Windows.\n\nThanks, it will be nice to have that test run a bit faster in wallclock\nterms.\n\n> I do have a question to the list here: Do mingw.c changes need to be\n> upstreamed somewhere? I don't understand the exact relationship between this\n> file and the MinGW project.\n\nNo, it's 100% ours. We just put (somewhat inconsistently in some cases)\nour own code that is only needed for \"compat\" with some \"platforms\" into\ncompat/*.\n\nWell, somewhat/mostly, then \"contrib/\" is supposed to be third-party\nimports, but some code in both is 100% ours, some is out-of-tree, some\nis effectively perma-forked and has no \"upstream\" anymore etc.\n"},{"id":"449766","messageId":"b5fb21f5-4aa6-fa36-a8f6-abf82b543c1d@jeffhostetler.com","threadId":"57482","inReplyTo":"76b6216281e3463821e650495f3090c677905f73.1646041236.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-02-28T15:27:08Z","receivedAt":"2022-02-28T15:27:16Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:\n> From: Tao Klerks \n> \n> The mingw_utime implementation in mingw.c does not support\n> directories. This means that \"test-tool chmtime\" fails on Windows when\n> targeting directories. This has previously been noted and sidestepped\n> by Jeff Hostetler, in \"t/helper/test-chmtime: skip directories\n> on Windows\" in the \"Builtin FSMonitor Part 2\" work.\n> \n> It would make sense to backdate file and folder changes in untracked\n> cache tests, to avoid needing to insert explicit delays/pauses in the\n> tests.\n> \n> Add support for directory date manipulation in mingw_utime by calling\n> CreateFileW() explicitly to create the directory handle, and\n> CloseHandle() to close it.\n> \n> Signed-off-by: Tao Klerks \n> ---\n> compat/mingw.c | 33 ++++++++++++++++++++++++++++-----\n> 1 file changed, 28 insertions(+), 5 deletions(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 03af369b2b9..2284ea90511 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \tint fh, rc;\n> \tDWORD attrs;\n> \twchar_t wfilename[MAX_PATH];\n> +\tHANDLE osfilehandle;\n\nI'd be more comfortable initializing this variable to\nINVALID_HANDLE_VALUE.\n\n> +\n> \tif (xutftowcs_path(wfilename, file_name) < 0)\n> \t\treturn -1;\n> \n> @@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n> \t}\n> \n> -\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> -\t\trc = -1;\n> -\t\tgoto revert_attrs;\n> +\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n> +\t\tfh = 0;\n\nand here initializing fh = -1.\n\n> +\t\tosfilehandle = CreateFileW(wfilename,\n> +\t\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n> +\t\t\t\t\t 0 /*FileShare.None*/,\n\nIs there a reason that you're not using the #define's here?\nI assume you ran into a header file problem or something, but\nthere are other symbols used nearby, so I'm not sure.\n\n> +\t\t\t\t\t NULL,\n> +\t\t\t\t\t OPEN_EXISTING,\n> +\t\t\t\t\t FILE_FLAG_BACKUP_SEMANTICS,\n> +\t\t\t\t\t NULL);\n> +\t\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n> +\t\t\terrno = err_win_to_posix(GetLastError());\n> +\t\t\trc = -1;\n> +\t\t\tgoto revert_attrs;\n> +\t\t}\n> +\t} else {\n> +\t\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> +\t\t\trc = -1;\n> +\t\t\tgoto revert_attrs;\n> +\t\t}\n> +\t\tosfilehandle = (HANDLE)_get_osfhandle(fh);\n> \t}\n> \n> \tif (times) {\n> @@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tGetSystemTimeAsFileTime(&mft);\n> \t\taft = mft;\n> \t}\n> -\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n> +\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n> \t\terrno = EINVAL;\n> \t\trc = -1;\n> \t} else\n> \t\trc = 0;\n> -\tclose(fh);\n> +\n> +\tif (fh)\n> +\t\tclose(fh);\n> +\telse if (osfilehandle)\n> +\t\tCloseHandle(osfilehandle);\n\nAnd then this becomes:\n\n\tif (fh != -1)\n\t\tclose(fh)\n\telse if (osfilehandle != INVALID_HANDLE_VALUE)\n\t\tClosehandle(osfilehandle);\n\nWhich I think makes it more clear that we'll properly close the handle.\n\n> \n> revert_attrs:\n> \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n> \n\nAn alternative solution would be to replace the _wopen() with\na call to CreateFileW() without the backup flag for non-directories\nand just convert the whole routine to use HANDLE's rather fd's.\n\nThanks\nJeff\n"},{"id":"449767","messageId":"06511c93-3e40-a5d4-142c-cab81199e75e@jeffhostetler.com","threadId":"57482","inReplyTo":"pull.1166.git.1646041236.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-02-28T15:29:17Z","receivedAt":"2022-02-28T15:29:22Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:\n> As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number\n> of test cases in t\\t7063-status-untracked-cache.sh explicitly sleep a\n> second, in order to avoid the untracked cache content being invalidated by\n> an mtime race condition.\n> \n> Even though it's only 9 seconds of sleeping that can be straightforwardly\n> replaced, it seems worth fixing if possible.\n> \n> Replace sleep calls with backdating of filesystem changes, but first fix the\n> test-tool chmtime functionality to work for directories in Windows.\n> \n> I do have a question to the list here: Do mingw.c changes need to be\n> upstreamed somewhere? I don't understand the exact relationship between this\n> file and the MinGW project.\n> \n> Tao Klerks (2):\n> t/helper/test-chmtime: update mingw to support chmtime on directories\n> t7063: mtime-mangling instead of delays in untracked cache testing\n> \n> compat/mingw.c | 33 ++++++++++++++++++++++++++-----\n> t/t7063-status-untracked-cache.sh | 29 ++++++++++++++++-----------\n> 2 files changed, 45 insertions(+), 17 deletions(-)\n> \n> \n> base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1166\n> \n\nThanks for working on this. It would be nice to be able\nto drop that commit (read: hack) from my fsmonitor series.\n\nJeff\n"},{"id":"449810","messageId":"xmqqfso2zm6l.fsf@gitster.g","threadId":"57482","inReplyTo":"76b6216281e3463821e650495f3090c677905f73.1646041236.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T22:00:18Z","receivedAt":"2022-02-28T22:00:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> From: Tao Klerks \n>\n> The mingw_utime implementation in mingw.c does not support\n> directories. This means that \"test-tool chmtime\" fails on Windows when\n> targeting directories. This has previously been noted and sidestepped\n> by Jeff Hostetler, in \"t/helper/test-chmtime: skip directories\n> on Windows\" in the \"Builtin FSMonitor Part 2\" work.\n\nI was expecting that this will be applicable _before_ FSMonitor Part\n2 and later. This mention would probably belong to the comment\nafter three-dashes?\n\n> It would make sense to backdate file and folder changes in untracked\n> cache tests, to avoid needing to insert explicit delays/pauses in the\n> tests.\n>\n> Add support for directory date manipulation in mingw_utime by calling\n> CreateFileW() explicitly to create the directory handle, and\n> CloseHandle() to close it.\n\nOK.\n\n> @@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \tint fh, rc;\n> \tDWORD attrs;\n> \twchar_t wfilename[MAX_PATH];\n> +\tHANDLE osfilehandle;\n\nThe variable is not initialized here, but that is perfectly OK.\nWe'll set it in both branches for directories and files.\n\n> \tif (xutftowcs_path(wfilename, file_name) < 0)\n> \t\treturn -1;\n> \n> @@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n> \t}\n> \n> -\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> -\t\trc = -1;\n> -\t\tgoto revert_attrs;\n> +\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n\nExcellent. We can reuse existing attrs without any extra calls to\nintroduce the new codepath to deal with directories.\n\n> +\t\tfh = 0;\n\nThis should be\n\n\t\tfh = -1;\n\ninstead. More on this later.\n\n> +\t\tosfilehandle = CreateFileW(wfilename,\n> +\t\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n> +\t\t\t\t\t 0 /*FileShare.None*/,\n> +\t\t\t\t\t NULL,\n> +\t\t\t\t\t OPEN_EXISTING,\n> +\t\t\t\t\t FILE_FLAG_BACKUP_SEMANTICS,\n> +\t\t\t\t\t NULL);\n> +\t\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n> +\t\t\terrno = err_win_to_posix(GetLastError());\n> +\t\t\trc = -1;\n> +\t\t\tgoto revert_attrs;\n> +\t\t}\n\nUpon an error, we'll jump to revert_attrs but otherwise, we will\nhave a valid osfilehandle (which presumably is not NULL).\n\n> +\t} else {\n> +\t\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> +\t\t\trc = -1;\n> +\t\t\tgoto revert_attrs;\n> +\t\t}\n> +\t\tosfilehandle = (HANDLE)_get_osfhandle(fh);\n\nThis side is the same as before. This code assumes that, as long as\n_wopen() gives us a usable fh, _get_osfhandle(fh) would always give\nus a valid handle. This assumption should be safe, as the original\ncode has been relying on it anyway.\n\n> @@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tGetSystemTimeAsFileTime(&mft);\n> \t\taft = mft;\n> \t}\n> -\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n> +\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n\nAnd we use the osfilehandle we obtained, either from the code that\nis identical to the original for files or from the new code for\ndirectories. Good.\n\n> \t\terrno = EINVAL;\n> \t\trc = -1;\n> \t} else\n> \t\trc = 0;\n> -\tclose(fh);\n> +\n> +\tif (fh)\n> +\t\tclose(fh);\n> +\telse if (osfilehandle)\n> +\t\tCloseHandle(osfilehandle);\n\nIn the context of \"git\" process, I do not think we would ever close\nFD#0, so it may be safe to assume that _wopen() above will never\nyield FD#0, so this is not quite wrong per-se, but to be\nfuture-proof, it would be even safer to instead do:\n\n\tif (0 <= fh)\n\t\tclose(fh);\n\telse if (osfilehandle)\n\t\tCloseHandle(osfilehandle);\n\nhere. That is consistent with the error checking done where\n_wopen() was called to obtain it above, i.e.\n\n\tif ((fh = _wopen(...)) < 0)\n\t\t... error ...\n\n> revert_attrs:\n> \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n"},{"id":"449811","messageId":"xmqqa6eazm45.fsf@gitster.g","threadId":"57482","inReplyTo":"b5fb21f5-4aa6-fa36-a8f6-abf82b543c1d@jeffhostetler.com","subject":"Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T22:01:46Z","receivedAt":"2022-02-28T22:01:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n>> +\tHANDLE osfilehandle;\n>\n> I'd be more comfortable initializing this variable to\n> INVALID_HANDLE_VALUE.\n\nBoth directories/files branches assign, so it is not needed.\n\n>> +\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n>> +\t\tfh = 0;\n>\n> and here initializing fh = -1.\n\nThis does make it safer.\n\n> And then this becomes:\n>\n> \tif (fh != -1)\n> \t\tclose(fh)\n> \telse if (osfilehandle != INVALID_HANDLE_VALUE)\n> \t\tClosehandle(osfilehandle);\n\nExactly.\n"},{"id":"449813","messageId":"xmqqr17my6pq.fsf@gitster.g","threadId":"57482","inReplyTo":"a1806c56333ee96f9cda8c77ae56517181b691c0.1646041237.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T22:19:45Z","receivedAt":"2022-02-28T22:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> From: Tao Klerks \n>\n> The untracked cache test uses an avoid_racy function to deal with\n> an mtime-resolution challenge in testing: If an untracked cache\n> entry's mtime falls in the same second as the mtime of the index\n> the untracked cache was stored in, then it cannot be trusted.\n>\n> Explicitly delaying tests is a simple effective strategy to\n> avoid these issues, but should be avoided where possible.\n>\n> Switch from a delay-based strategy to instead backdating\n> all file changes using test-tool chmtime, where that is an\n> option, to shave 9 seconds off the test run time.\n\nOK.\n\n> Don't update test cases that delay for other reasons, for now at\n> least (4 seconds).\n\nSad but one step at a time.\n\n> +chmmtime_worktree_root () {\n\nNot \"chmtime_worktree_root\"? Is doubled-m intended?\n\n> +\t# chmtime doesnt handle relative paths on windows, so need\n> +\t# to \"hardcode\" a reference to the worktree folder name.\n> +\tcd .. &&\n> +\ttest-tool chmtime $1 worktree &&\n> +\tcd worktree\n\nAn unsuspecting caller will be left in a directory it didn't expect\nto when \"test-tool chmtime\" fails, which is not nice.\n\n\t(\n\t\tcd .. &&\n\t\ttest-tool chmtime \"$1\" worktree\n\t)\n\nat least until the tool learns to do\n\n\ttest-tool chmtime \"$1\" ../worktree\n\nor\n\n\ttest-tool -C.. chmtime \"$1\" worktree\n\nOh, isn't the last one already available?\n\n> +}\n> +\n> avoid_racy() {\n> \tsleep 1\n> }\n> @@ -90,6 +98,9 @@ test_expect_success 'setup' '\n> \tcd worktree &&\n> \tmkdir done dtwo dthree &&\n> \ttouch one two three done/one dtwo/two dthree/three &&\n> +\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n> +\ttest-tool chmtime =-300 done dtwo dthree &&\n\nNot a huge deal, but it would have been very nice if \"test-tool chmtime\"\nhad a \"-R\" option to recurse into directories.\n"},{"id":"449883","messageId":"CAPMMpoiQw0UhnH0WP_opB0iQn8cRx8qT_Cy_W8PnHpxbwATw2A@mail.gmail.com","threadId":"57482","inReplyTo":"b5fb21f5-4aa6-fa36-a8f6-abf82b543c1d@jeffhostetler.com","subject":"Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-01T08:16:52Z","receivedAt":"2022-03-01T08:17:36Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Feb 28, 2022 at 4:27 PM Jeff Hostetler wrote:\n>\n>\n>\n> On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:\n\n> > + HANDLE osfilehandle;\n>\n> I'd be more comfortable initializing this variable to\n> INVALID_HANDLE_VALUE.\n>\n\nMakes sense, thanks. (but less relevant after switch to CreateFileW)\n\n> > + fh = 0;\n>\n> and here initializing fh = -1.\n\nMakes sense, thanks. (but overshadowed by switch to CreateFileW)\n\n> > + osfilehandle = CreateFileW(wfilename,\n> > + 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n> > + 0 /*FileShare.None*/,\n>\n> Is there a reason that you're not using the #define's here?\n> I assume you ran into a header file problem or something, but\n> there are other symbols used nearby, so I'm not sure.\n\nI couldn't find these, and am a complete C APIs n00b - I have no idea\nwhether or how to add them. I figured commenting on their meaning\nis the simplest safest thing to do locally?\n\n> > + if (fh)\n> > + close(fh);\n> > + else if (osfilehandle)\n> > + CloseHandle(osfilehandle);\n>\n> And then this becomes:\n>\n> if (fh != -1)\n> close(fh)\n> else if (osfilehandle != INVALID_HANDLE_VALUE)\n> Closehandle(osfilehandle);\n>\n> Which I think makes it more clear that we'll properly close the handle.\n\nPerfect, thx.\n\n>\n> >\n> > revert_attrs:\n> > if (attrs != INVALID_FILE_ATTRIBUTES &&\n> >\n>\n> An alternative solution would be to replace the _wopen() with\n> a call to CreateFileW() without the backup flag for non-directories\n> and just convert the whole routine to use HANDLE's rather fd's.\n>\n\nI was at first scared of making changes to things I don't fully\nunderstand, but looking at the existing use of GetFileAttributesW\nI don't think that makes much sense. This is cleaner/simpler\neven if it is a bigger change, and consistent with other things.\n"},{"id":"449884","messageId":"CAPMMpoi_2iczL94jfVktH0Whnr8Gn-54cLgpH6Q0iM0uteQsyg@mail.gmail.com","threadId":"57482","inReplyTo":"xmqqfso2zm6l.fsf@gitster.g","subject":"Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-01T08:21:45Z","receivedAt":"2022-03-01T08:22:03Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Feb 28, 2022 at 11:00 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > The mingw_utime implementation in mingw.c does not support\n> > directories. This means that \"test-tool chmtime\" fails on Windows when\n> > targeting directories. This has previously been noted and sidestepped\n> > by Jeff Hostetler, in \"t/helper/test-chmtime: skip directories\n> > on Windows\" in the \"Builtin FSMonitor Part 2\" work.\n>\n> I was expecting that this will be applicable _before_ FSMonitor Part\n> 2 and later. This mention would probably belong to the comment\n> after three-dashes?\n>\n\nI've updated the text slightly in the next re-roll, but I didn't understand the\nbit about dashes... What is \"the comment after three dashes\"?\n\n> > + fh = 0;\n>\n> This should be\n>\n> fh = -1;\n>\n> instead. More on this later.\n>\n\nMakes sense, but obviated by full switch to CreateFileW().\n\n> > + if (fh)\n> > + close(fh);\n> > + else if (osfilehandle)\n> > + CloseHandle(osfilehandle);\n>\n> In the context of \"git\" process, I do not think we would ever close\n> FD#0, so it may be safe to assume that _wopen() above will never\n> yield FD#0, so this is not quite wrong per-se, but to be\n> future-proof, it would be even safer to instead do:\n>\n> if (0 <= fh)\n> close(fh);\n> else if (osfilehandle)\n> CloseHandle(osfilehandle);\n>\n> here. That is consistent with the error checking done where\n> _wopen() was called to obtain it above, i.e.\n>\n> if ((fh = _wopen(...)) < 0)\n> ... error ...\n>\n\nMakes sense, but obviated by full switch to CreateFileW().\n"},{"id":"449897","messageId":"CAPMMpojHUds8dxKNQcMe6nLj_W6ANazpTH38jBzWix4oLHshPg@mail.gmail.com","threadId":"57482","inReplyTo":"xmqqr17my6pq.fsf@gitster.g","subject":"Re: [PATCH 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-01T09:44:53Z","receivedAt":"2022-03-01T09:45:14Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Feb 28, 2022 at 11:19 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > +chmmtime_worktree_root () {\n>\n> Not \"chmtime_worktree_root\"? Is doubled-m intended?\n>\n\nOops, typo, fixing.\n\n> > + # chmtime doesnt handle relative paths on windows, so need\n> > + # to \"hardcode\" a reference to the worktree folder name.\n> > + cd .. &&\n> > + test-tool chmtime $1 worktree &&\n> > + cd worktree\n>\n> An unsuspecting caller will be left in a directory it didn't expect\n> to when \"test-tool chmtime\" fails, which is not nice.\n>\n> (\n> cd .. &&\n> test-tool chmtime \"$1\" worktree\n> )\n>\n> at least until the tool learns to do\n>\n> test-tool chmtime \"$1\" ../worktree\n>\n> or\n>\n> test-tool -C.. chmtime \"$1\" worktree\n>\n> Oh, isn't the last one already available?\n\nAh, lovely, thank you!\n"},{"id":"449898","messageId":"pull.1166.v2.git.1646127910.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.git.1646041236.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-01T09:45:08Z","receivedAt":"2022-03-01T09:45:19Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number\nof test cases in t\\t7063-status-untracked-cache.sh explicitly sleep a\nsecond, in order to avoid the untracked cache content being invalidated by\nan mtime race condition.\n\nEven though it's only 9 seconds of sleeping that can be straightforwardly\nreplaced, it seems worth fixing if possible.\n\nReplace sleep calls with backdating of filesystem changes, but first fix the\ntest-tool chmtime functionality to work for directories in Windows.\n\nI do have a question to the list here: Do mingw.c changes need to be\nupstreamed somewhere? I don't understand the exact relationship between this\nfile and the MinGW project.\n\nTao Klerks (2):\n t/helper/test-chmtime: update mingw to support chmtime on directories\n t7063: mtime-mangling instead of delays in untracked cache testing\n\n compat/mingw.c | 21 +++++++++++++++++----\n t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------\n 2 files changed, 32 insertions(+), 16 deletions(-)\n\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1166\n\nRange-diff vs v1:\n\n 1: 76b6216281e ! 1: 7cdef0ad5fb t/helper/test-chmtime: update mingw to support chmtime on directories\n @@ Commit message\n The mingw_utime implementation in mingw.c does not support\n directories. This means that \"test-tool chmtime\" fails on Windows when\n targeting directories. This has previously been noted and sidestepped\n - by Jeff Hostetler, in \"t/helper/test-chmtime: skip directories\n - on Windows\" in the \"Builtin FSMonitor Part 2\" work.\n + temporarily by Jeff Hostetler, in \"t/helper/test-chmtime: skip\n + directories on Windows\" in the \"Builtin FSMonitor Part 2\" work, but\n + not yet fixed.\n \n It would make sense to backdate file and folder changes in untracked\n cache tests, to avoid needing to insert explicit delays/pauses in the\n tests.\n \n - Add support for directory date manipulation in mingw_utime by calling\n - CreateFileW() explicitly to create the directory handle, and\n - CloseHandle() to close it.\n + Add support for directory date manipulation in mingw_utime by\n + replacing the file-oriented _wopen() call with the\n + directory-supporting CreateFileW() windows API explicitly.\n \n Signed-off-by: Tao Klerks \n \n ## compat/mingw.c ##\n -@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times)\n - \tint fh, rc;\n +@@ compat/mingw.c: static inline void time_t_to_filetime(time_t t, FILETIME *ft)\n + int mingw_utime (const char *file_name, const struct utimbuf *times)\n + {\n + \tFILETIME mft, aft;\n +-\tint fh, rc;\n ++\tint rc;\n \tDWORD attrs;\n \twchar_t wfilename[MAX_PATH];\n +\tHANDLE osfilehandle;\n @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n \t}\n \n -\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n --\t\trc = -1;\n --\t\tgoto revert_attrs;\n -+\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n -+\t\tfh = 0;\n -+\t\tosfilehandle = CreateFileW(wfilename,\n -+\t\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n -+\t\t\t\t\t 0 /*FileShare.None*/,\n -+\t\t\t\t\t NULL,\n -+\t\t\t\t\t OPEN_EXISTING,\n -+\t\t\t\t\t FILE_FLAG_BACKUP_SEMANTICS,\n -+\t\t\t\t\t NULL);\n -+\t\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n -+\t\t\terrno = err_win_to_posix(GetLastError());\n -+\t\t\trc = -1;\n -+\t\t\tgoto revert_attrs;\n -+\t\t}\n -+\t} else {\n -+\t\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n -+\t\t\trc = -1;\n -+\t\t\tgoto revert_attrs;\n -+\t\t}\n -+\t\tosfilehandle = (HANDLE)_get_osfhandle(fh);\n ++\tosfilehandle = CreateFileW(wfilename,\n ++\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n ++\t\t\t\t 0 /*FileShare.None*/,\n ++\t\t\t\t NULL,\n ++\t\t\t\t OPEN_EXISTING,\n ++\t\t\t\t attrs & FILE_ATTRIBUTE_DIRECTORY ?\n ++\t\t\t\t\tFILE_FLAG_BACKUP_SEMANTICS : 0,\n ++\t\t\t\t NULL);\n ++\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n ++\t\terrno = err_win_to_posix(GetLastError());\n + \t\trc = -1;\n + \t\tgoto revert_attrs;\n \t}\n - \n - \tif (times) {\n @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tGetSystemTimeAsFileTime(&mft);\n \t\taft = mft;\n @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n \t\trc = 0;\n -\tclose(fh);\n +\n -+\tif (fh)\n -+\t\tclose(fh);\n -+\telse if (osfilehandle)\n ++\tif (osfilehandle != INVALID_HANDLE_VALUE)\n +\t\tCloseHandle(osfilehandle);\n \n revert_attrs:\n 2: a1806c56333 ! 2: 3e3c9c7faac t7063: mtime-mangling instead of delays in untracked cache testing\n @@ t/t7063-status-untracked-cache.sh: sync_mtime () {\n \tfind . -type d -exec ls -ld {} + >/dev/null\n }\n \n -+chmmtime_worktree_root () {\n ++chmtime_worktree_root () {\n +\t# chmtime doesnt handle relative paths on windows, so need\n +\t# to \"hardcode\" a reference to the worktree folder name.\n -+\tcd .. &&\n -+\ttest-tool chmtime $1 worktree &&\n -+\tcd worktree\n ++\ttest-tool -C .. chmtime $1 worktree\n +}\n +\n avoid_racy() {\n @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' '\n \ttouch one two three done/one dtwo/two dthree/three &&\n +\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n +\ttest-tool chmtime =-300 done dtwo dthree &&\n -+\tchmmtime_worktree_root =-300 &&\n ++\tchmtime_worktree_root =-300 &&\n \tgit add one two done/one &&\n \t: >.git/info/exclude &&\n \tgit update-index --untracked-cache &&\n @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som\n -\trm base &&\n +\ttest-tool chmtime =-180 done/two done/three done/four done/five done &&\n +\t# we need to ensure that the root dir is touched (in the past);\n -+\tchmmtime_worktree_root =-180 &&\n ++\tchmtime_worktree_root =-180 &&\n \tsync_mtime\n '\n \n\n-- \ngitgitgadget\n"},{"id":"449899","messageId":"7cdef0ad5fb6fc1a16ee1cee27b9dec0300c8c1d.1646127910.git.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.v2.git.1646127910.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-01T09:45:09Z","receivedAt":"2022-03-01T09:45:21Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nThe mingw_utime implementation in mingw.c does not support\ndirectories. This means that \"test-tool chmtime\" fails on Windows when\ntargeting directories. This has previously been noted and sidestepped\ntemporarily by Jeff Hostetler, in \"t/helper/test-chmtime: skip\ndirectories on Windows\" in the \"Builtin FSMonitor Part 2\" work, but\nnot yet fixed.\n\nIt would make sense to backdate file and folder changes in untracked\ncache tests, to avoid needing to insert explicit delays/pauses in the\ntests.\n\nAdd support for directory date manipulation in mingw_utime by\nreplacing the file-oriented _wopen() call with the\ndirectory-supporting CreateFileW() windows API explicitly.\n\nSigned-off-by: Tao Klerks \n---\n compat/mingw.c | 21 +++++++++++++++++----\n 1 file changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 03af369b2b9..11c43ad2dfb 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)\n int mingw_utime (const char *file_name, const struct utimbuf *times)\n {\n \tFILETIME mft, aft;\n-\tint fh, rc;\n+\tint rc;\n \tDWORD attrs;\n \twchar_t wfilename[MAX_PATH];\n+\tHANDLE osfilehandle;\n+\n \tif (xutftowcs_path(wfilename, file_name) < 0)\n \t\treturn -1;\n \n@@ -975,7 +977,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n \t}\n \n-\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n+\tosfilehandle = CreateFileW(wfilename,\n+\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n+\t\t\t\t 0 /*FileShare.None*/,\n+\t\t\t\t NULL,\n+\t\t\t\t OPEN_EXISTING,\n+\t\t\t\t attrs & FILE_ATTRIBUTE_DIRECTORY ?\n+\t\t\t\t\tFILE_FLAG_BACKUP_SEMANTICS : 0,\n+\t\t\t\t NULL);\n+\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n+\t\terrno = err_win_to_posix(GetLastError());\n \t\trc = -1;\n \t\tgoto revert_attrs;\n \t}\n@@ -987,12 +998,14 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tGetSystemTimeAsFileTime(&mft);\n \t\taft = mft;\n \t}\n-\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n+\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n \t} else\n \t\trc = 0;\n-\tclose(fh);\n+\n+\tif (osfilehandle != INVALID_HANDLE_VALUE)\n+\t\tCloseHandle(osfilehandle);\n \n revert_attrs:\n \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n-- \ngitgitgadget\n\n"},{"id":"449900","messageId":"3e3c9c7faace505958aa01ff82bef5fad3204c67.1646127910.git.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.v2.git.1646127910.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-01T09:45:10Z","receivedAt":"2022-03-01T09:45:23Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nThe untracked cache test uses an avoid_racy function to deal with\nan mtime-resolution challenge in testing: If an untracked cache\nentry's mtime falls in the same second as the mtime of the index\nthe untracked cache was stored in, then it cannot be trusted.\n\nExplicitly delaying tests is a simple effective strategy to\navoid these issues, but should be avoided where possible.\n\nSwitch from a delay-based strategy to instead backdating\nall file changes using test-tool chmtime, where that is an\noption, to shave 9 seconds off the test run time.\n\nDon't update test cases that delay for other reasons, for now at\nleast (4 seconds).\n\nSigned-off-by: Tao Klerks \n---\n t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex a0c123b0a77..eae57dc78a6 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -24,6 +24,12 @@ sync_mtime () {\n \tfind . -type d -exec ls -ld {} + >/dev/null\n }\n \n+chmtime_worktree_root () {\n+\t# chmtime doesnt handle relative paths on windows, so need\n+\t# to \"hardcode\" a reference to the worktree folder name.\n+\ttest-tool -C .. chmtime $1 worktree\n+}\n+\n avoid_racy() {\n \tsleep 1\n }\n@@ -90,6 +96,9 @@ test_expect_success 'setup' '\n \tcd worktree &&\n \tmkdir done dtwo dthree &&\n \ttouch one two three done/one dtwo/two dthree/three &&\n+\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n+\ttest-tool chmtime =-300 done dtwo dthree &&\n+\tchmtime_worktree_root =-300 &&\n \tgit add one two done/one &&\n \t: >.git/info/exclude &&\n \tgit update-index --untracked-cache &&\n@@ -142,7 +151,6 @@ two\n EOF\n \n test_expect_success 'status first time (empty cache)' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -166,7 +174,6 @@ test_expect_success 'untracked cache after first status' '\n '\n \n test_expect_success 'status second time (fully populated cache)' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -190,8 +197,8 @@ test_expect_success 'untracked cache after second status' '\n '\n \n test_expect_success 'modify in root directory, one dir invalidation' '\n-\tavoid_racy &&\n \t: >four &&\n+\ttest-tool chmtime =-240 four &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -241,7 +248,6 @@ EOF\n '\n \n test_expect_success 'new .gitignore invalidates recursively' '\n-\tavoid_racy &&\n \techo four >.gitignore &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n@@ -292,7 +298,6 @@ EOF\n '\n \n test_expect_success 'new info/exclude invalidates everything' '\n-\tavoid_racy &&\n \techo three >>.git/info/exclude &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n@@ -520,14 +525,14 @@ test_expect_success 'create/modify files, some of which are gitignored' '\n \techo three >done/three && # three is gitignored\n \techo four >done/four && # four is gitignored at a higher level\n \techo five >done/five && # five is not gitignored\n-\techo test >base && #we need to ensure that the root dir is touched\n-\trm base &&\n+\ttest-tool chmtime =-180 done/two done/three done/four done/five done &&\n+\t# we need to ensure that the root dir is touched (in the past);\n+\tchmtime_worktree_root =-180 &&\n \tsync_mtime\n '\n \n test_expect_success 'test sparse status with untracked cache' '\n \t: >../trace.output &&\n-\tavoid_racy &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n \tiuc status --porcelain >../status.iuc &&\n@@ -570,7 +575,6 @@ EOF\n '\n \n test_expect_success 'test sparse status again with untracked cache' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n@@ -597,11 +601,11 @@ EOF\n test_expect_success 'set up for test of subdir and sparse checkouts' '\n \tmkdir done/sub &&\n \tmkdir done/sub/sub &&\n-\techo \"sub\" > done/sub/sub/file\n+\techo \"sub\" > done/sub/sub/file &&\n+\ttest-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done\n '\n \n test_expect_success 'test sparse status with untracked cache and subdir' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n@@ -651,7 +655,6 @@ EOF\n '\n \n test_expect_success 'test sparse status again with untracked cache and subdir' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n-- \ngitgitgadget\n"},{"id":"449902","messageId":"CAPMMpog2r6ydon0rCfGSGbiBFYrQNuCTS0u0HJ5yf2mGzhHxAg@mail.gmail.com","threadId":"57482","inReplyTo":"pull.1166.v2.git.1646127910.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-01T09:49:54Z","receivedAt":"2022-03-01T09:50:11Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"> I do have a question to the list here: Do mingw.c changes need to be\n> upstreamed somewhere? I don't understand the exact relationship between this\n> file and the MinGW project.\n\n(sorry, failed to update the cover letter to remove the now-answered\nquestion :( )\n\nOn Tue, Mar 1, 2022 at 10:45 AM Tao Klerks via GitGitGadget\n wrote:\n>\n> As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number\n> of test cases in t\\t7063-status-untracked-cache.sh explicitly sleep a\n> second, in order to avoid the untracked cache content being invalidated by\n> an mtime race condition.\n>\n> Even though it's only 9 seconds of sleeping that can be straightforwardly\n> replaced, it seems worth fixing if possible.\n>\n> Replace sleep calls with backdating of filesystem changes, but first fix the\n> test-tool chmtime functionality to work for directories in Windows.\n>\n> I do have a question to the list here: Do mingw.c changes need to be\n> upstreamed somewhere? I don't understand the exact relationship between this\n> file and the MinGW project.\n>\n> Tao Klerks (2):\n> t/helper/test-chmtime: update mingw to support chmtime on directories\n> t7063: mtime-mangling instead of delays in untracked cache testing\n>\n> compat/mingw.c | 21 +++++++++++++++++----\n> t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------\n> 2 files changed, 32 insertions(+), 16 deletions(-)\n>\n>\n> base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1166\n>\n> Range-diff vs v1:\n>\n> 1: 76b6216281e ! 1: 7cdef0ad5fb t/helper/test-chmtime: update mingw to support chmtime on directories\n> @@ Commit message\n> The mingw_utime implementation in mingw.c does not support\n> directories. This means that \"test-tool chmtime\" fails on Windows when\n> targeting directories. This has previously been noted and sidestepped\n> - by Jeff Hostetler, in \"t/helper/test-chmtime: skip directories\n> - on Windows\" in the \"Builtin FSMonitor Part 2\" work.\n> + temporarily by Jeff Hostetler, in \"t/helper/test-chmtime: skip\n> + directories on Windows\" in the \"Builtin FSMonitor Part 2\" work, but\n> + not yet fixed.\n>\n> It would make sense to backdate file and folder changes in untracked\n> cache tests, to avoid needing to insert explicit delays/pauses in the\n> tests.\n>\n> - Add support for directory date manipulation in mingw_utime by calling\n> - CreateFileW() explicitly to create the directory handle, and\n> - CloseHandle() to close it.\n> + Add support for directory date manipulation in mingw_utime by\n> + replacing the file-oriented _wopen() call with the\n> + directory-supporting CreateFileW() windows API explicitly.\n>\n> Signed-off-by: Tao Klerks \n>\n> ## compat/mingw.c ##\n> -@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times)\n> - int fh, rc;\n> +@@ compat/mingw.c: static inline void time_t_to_filetime(time_t t, FILETIME *ft)\n> + int mingw_utime (const char *file_name, const struct utimbuf *times)\n> + {\n> + FILETIME mft, aft;\n> +- int fh, rc;\n> ++ int rc;\n> DWORD attrs;\n> wchar_t wfilename[MAX_PATH];\n> + HANDLE osfilehandle;\n> @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n> }\n>\n> - if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> -- rc = -1;\n> -- goto revert_attrs;\n> -+ if (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n> -+ fh = 0;\n> -+ osfilehandle = CreateFileW(wfilename,\n> -+ 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n> -+ 0 /*FileShare.None*/,\n> -+ NULL,\n> -+ OPEN_EXISTING,\n> -+ FILE_FLAG_BACKUP_SEMANTICS,\n> -+ NULL);\n> -+ if (osfilehandle == INVALID_HANDLE_VALUE) {\n> -+ errno = err_win_to_posix(GetLastError());\n> -+ rc = -1;\n> -+ goto revert_attrs;\n> -+ }\n> -+ } else {\n> -+ if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> -+ rc = -1;\n> -+ goto revert_attrs;\n> -+ }\n> -+ osfilehandle = (HANDLE)_get_osfhandle(fh);\n> ++ osfilehandle = CreateFileW(wfilename,\n> ++ 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n> ++ 0 /*FileShare.None*/,\n> ++ NULL,\n> ++ OPEN_EXISTING,\n> ++ attrs & FILE_ATTRIBUTE_DIRECTORY ?\n> ++ FILE_FLAG_BACKUP_SEMANTICS : 0,\n> ++ NULL);\n> ++ if (osfilehandle == INVALID_HANDLE_VALUE) {\n> ++ errno = err_win_to_posix(GetLastError());\n> + rc = -1;\n> + goto revert_attrs;\n> }\n> -\n> - if (times) {\n> @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times)\n> GetSystemTimeAsFileTime(&mft);\n> aft = mft;\n> @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n> rc = 0;\n> - close(fh);\n> +\n> -+ if (fh)\n> -+ close(fh);\n> -+ else if (osfilehandle)\n> ++ if (osfilehandle != INVALID_HANDLE_VALUE)\n> + CloseHandle(osfilehandle);\n>\n> revert_attrs:\n> 2: a1806c56333 ! 2: 3e3c9c7faac t7063: mtime-mangling instead of delays in untracked cache testing\n> @@ t/t7063-status-untracked-cache.sh: sync_mtime () {\n> find . -type d -exec ls -ld {} + >/dev/null\n> }\n>\n> -+chmmtime_worktree_root () {\n> ++chmtime_worktree_root () {\n> + # chmtime doesnt handle relative paths on windows, so need\n> + # to \"hardcode\" a reference to the worktree folder name.\n> -+ cd .. &&\n> -+ test-tool chmtime $1 worktree &&\n> -+ cd worktree\n> ++ test-tool -C .. chmtime $1 worktree\n> +}\n> +\n> avoid_racy() {\n> @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' '\n> touch one two three done/one dtwo/two dthree/three &&\n> + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n> + test-tool chmtime =-300 done dtwo dthree &&\n> -+ chmmtime_worktree_root =-300 &&\n> ++ chmtime_worktree_root =-300 &&\n> git add one two done/one &&\n> : >.git/info/exclude &&\n> git update-index --untracked-cache &&\n> @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som\n> - rm base &&\n> + test-tool chmtime =-180 done/two done/three done/four done/five done &&\n> + # we need to ensure that the root dir is touched (in the past);\n> -+ chmmtime_worktree_root =-180 &&\n> ++ chmtime_worktree_root =-180 &&\n> sync_mtime\n> '\n>\n>\n> --\n> gitgitgadget\n"},{"id":"449935","messageId":"37f66fae-911a-62f1-4711-c9f6c7794911@jeffhostetler.com","threadId":"57482","inReplyTo":"7cdef0ad5fb6fc1a16ee1cee27b9dec0300c8c1d.1646127910.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-03-01T16:34:13Z","receivedAt":"2022-03-01T16:34:17Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:\n> From: Tao Klerks \n> \n> The mingw_utime implementation in mingw.c does not support\n> directories. This means that \"test-tool chmtime\" fails on Windows when\n> targeting directories. This has previously been noted and sidestepped\n> temporarily by Jeff Hostetler, in \"t/helper/test-chmtime: skip\n> directories on Windows\" in the \"Builtin FSMonitor Part 2\" work, but\n> not yet fixed.\n> \n> It would make sense to backdate file and folder changes in untracked\n> cache tests, to avoid needing to insert explicit delays/pauses in the\n> tests.\n> \n> Add support for directory date manipulation in mingw_utime by\n> replacing the file-oriented _wopen() call with the\n> directory-supporting CreateFileW() windows API explicitly.\n> \n> Signed-off-by: Tao Klerks \n> ---\n> compat/mingw.c | 21 +++++++++++++++++----\n> 1 file changed, 17 insertions(+), 4 deletions(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 03af369b2b9..11c43ad2dfb 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)\n> int mingw_utime (const char *file_name, const struct utimbuf *times)\n> {\n> \tFILETIME mft, aft;\n> -\tint fh, rc;\n> +\tint rc;\n> \tDWORD attrs;\n> \twchar_t wfilename[MAX_PATH];\n> +\tHANDLE osfilehandle;\n> +\n> \tif (xutftowcs_path(wfilename, file_name) < 0)\n> \t\treturn -1;\n> \n> @@ -975,7 +977,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n> \t}\n> \n> -\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> +\tosfilehandle = CreateFileW(wfilename,\n> +\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n\nhttps://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants\n\nindicates that FILE_WRITE_ATTRIBUTES is defined in which\nwe get from which was included by \"win32.h\", so it should\nalready be present.\n\n> +\t\t\t\t 0 /*FileShare.None*/,\n> +\t\t\t\t NULL,\n> +\t\t\t\t OPEN_EXISTING,\n> +\t\t\t\t attrs & FILE_ATTRIBUTE_DIRECTORY ?\n> +\t\t\t\t\tFILE_FLAG_BACKUP_SEMANTICS : 0,\n\nThere is a weird error case here. If the GetFileAttributesW() call\nat the top fails, it returns INVALID_FILE_ATTRIBUTES (aka -1). So\nthe (attrs & ...) here expression is questionable.\n\nI'm not sure that there is a best way to handle the earlier failure\n(other than returning an error at the top), but we do try to limp\nalong (for some reason).\n\nSo maybe make this something like:\n\n (attrs != INVALID_FILE_ATTRIBUTES &&\n (attrs & FILE_ATTRIBUTE_DIRECTORY)) ?\n FILE_FLAG_BACKUP_SEMANTICS : 0\n\n\n> +\t\t\t\t NULL);\n> +\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n> +\t\terrno = err_win_to_posix(GetLastError());\n> \t\trc = -1;\n> \t\tgoto revert_attrs;\n> \t}\n> @@ -987,12 +998,14 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tGetSystemTimeAsFileTime(&mft);\n> \t\taft = mft;\n> \t}\n> -\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n> +\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n> \t\terrno = EINVAL;\n> \t\trc = -1;\n> \t} else\n> \t\trc = 0;\n> -\tclose(fh);\n> +\n> +\tif (osfilehandle != INVALID_HANDLE_VALUE)\n> +\t\tCloseHandle(osfilehandle);\n> \n> revert_attrs:\n> \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n> \n"},{"id":"449944","messageId":"xmqqsfs1r29t.fsf@gitster.g","threadId":"57482","inReplyTo":"pull.1166.v2.git.1646127910.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-01T17:49:50Z","receivedAt":"2022-03-01T17:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number\n> of test cases in t\\t7063-status-untracked-cache.sh explicitly sleep a\n> second, in order to avoid the untracked cache content being invalidated by\n> an mtime race condition.\n>\n> Even though it's only 9 seconds of sleeping that can be straightforwardly\n> replaced, it seems worth fixing if possible.\n\nAbsolutely. Thanks for an update.\n\n\n"},{"id":"449947","messageId":"xmqqk0ddr1n1.fsf@gitster.g","threadId":"57482","inReplyTo":"3e3c9c7faace505958aa01ff82bef5fad3204c67.1646127910.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-01T18:03:30Z","receivedAt":"2022-03-01T18:03:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> +chmtime_worktree_root () {\n> +\t# chmtime doesnt handle relative paths on windows, so need\n> +\t# to \"hardcode\" a reference to the worktree folder name.\n> +\ttest-tool -C .. chmtime $1 worktree\n> +}\n> +\n\nEnclose $1 in a pair of double-quotes to help readers. They do not\nhave to wonder if the caller is interested in (or has to worry\nabout) triggering word splitting at $IFS if you did so.\n\n> avoid_racy() {\n> \tsleep 1\n> }\n> @@ -90,6 +96,9 @@ test_expect_success 'setup' '\n> \tcd worktree &&\n> \tmkdir done dtwo dthree &&\n> \ttouch one two three done/one dtwo/two dthree/three &&\n> +\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n> +\ttest-tool chmtime =-300 done dtwo dthree &&\n> +\tchmtime_worktree_root =-300 &&\n\nI am wondering if it is better to spelling it out like this:\n\n\ttest-tool -C.. chmtime =-300 worktree &&\n\ninstead of hiding the fact that \"../worktree\" is being touched\nbehind a one-line helper. Being able to explicitly write \"worktree\"\nin the context that this particular code path uses the \"worktree\"\ndirectory is a big plus, but at the same time, bypassing the helper\nmakes it unclear why we just don't chmtime \"../worktree\", and will\nstrongly tempt future developers into breaking it, so, I dunno.\n\nWhat's the reason why utime() works only on a path in the current\ndirectory and cannot take \"../worktree\" again? If we cannot solve\nthat, I guess an extra helper with a big comment, like we see in\nthis patch, would be the least bad solution.\n\nThanks.\n"},{"id":"450020","messageId":"CAPMMpojoe8DgUteRAfuXCHQNFH6vnwj-Twze5i3A62JsvTUWVg@mail.gmail.com","threadId":"57482","inReplyTo":"37f66fae-911a-62f1-4711-c9f6c7794911@jeffhostetler.com","subject":"Re: [PATCH v2 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-01T21:14:20Z","receivedAt":"2022-03-01T21:14:38Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Mar 1, 2022 at 5:34 PM Jeff Hostetler wrote:\n>\n> On 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:\n> >\n> > - if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> > + osfilehandle = CreateFileW(wfilename,\n> > + 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n>\n> https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants\n>\n> indicates that FILE_WRITE_ATTRIBUTES is defined in which\n> we get from which was included by \"win32.h\", so it should\n> already be present.\n>\n\nGrr, should have asked the compiler instead of VSCode's autocomplete.\n\nThx, fixed.\n\n> > + 0 /*FileShare.None*/,\n> > + NULL,\n> > + OPEN_EXISTING,\n> > + attrs & FILE_ATTRIBUTE_DIRECTORY ?\n> > + FILE_FLAG_BACKUP_SEMANTICS : 0,\n>\n> There is a weird error case here. If the GetFileAttributesW() call\n> at the top fails, it returns INVALID_FILE_ATTRIBUTES (aka -1). So\n> the (attrs & ...) here expression is questionable.\n>\n> I'm not sure that there is a best way to handle the earlier failure\n> (other than returning an error at the top), but we do try to limp\n> along (for some reason).\n>\n> So maybe make this something like:\n>\n> (attrs != INVALID_FILE_ATTRIBUTES &&\n> (attrs & FILE_ATTRIBUTE_DIRECTORY)) ?\n> FILE_FLAG_BACKUP_SEMANTICS : 0\n>\n>\n\nYikes, I didn't realize how negative numbers looked in binary (in C).\n\nThanks for this catch!\n"},{"id":"450024","messageId":"CAPMMpojJhw40DK7mEpZ-4ht6FHohAms=G9LsPQQx6+LCMmWtwA@mail.gmail.com","threadId":"57482","inReplyTo":"xmqqk0ddr1n1.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-01T22:13:38Z","receivedAt":"2022-03-01T22:13:54Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Mar 1, 2022 at 7:03 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > +chmtime_worktree_root () {\n> > + # chmtime doesnt handle relative paths on windows, so need\n> > + # to \"hardcode\" a reference to the worktree folder name.\n> > + test-tool -C .. chmtime $1 worktree\n> > +}\n> > +\n>\n> Enclose $1 in a pair of double-quotes to help readers. They do not\n> have to wonder if the caller is interested in (or has to worry\n> about) triggering word splitting at $IFS if you did so.\n\nAbsolutely, thx.\n\n>\n> > avoid_racy() {\n> > sleep 1\n> > }\n> > @@ -90,6 +96,9 @@ test_expect_success 'setup' '\n> > cd worktree &&\n> > mkdir done dtwo dthree &&\n> > touch one two three done/one dtwo/two dthree/three &&\n> > + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n> > + test-tool chmtime =-300 done dtwo dthree &&\n> > + chmtime_worktree_root =-300 &&\n>\n> I am wondering if it is better to spelling it out like this:\n>\n> test-tool -C.. chmtime =-300 worktree &&\n>\n> instead of hiding the fact that \"../worktree\" is being touched\n> behind a one-line helper. Being able to explicitly write \"worktree\"\n> in the context that this particular code path uses the \"worktree\"\n> directory is a big plus, but at the same time, bypassing the helper\n> makes it unclear why we just don't chmtime \"../worktree\", and will\n> strongly tempt future developers into breaking it, so, I dunno.\n>\n> What's the reason why utime() works only on a path in the current\n> directory and cannot take \"../worktree\" again? If we cannot solve\n> that, I guess an extra helper with a big comment, like we see in\n> this patch, would be the least bad solution.\n>\n\nHeh. It didn't work, in my initial tests. Now it does. It turns out I was\ninitially getting the directory handle with\n\"FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES\", and\neverything worked except modifying the current folder via a relative\npath expression, which yielded a \"Permission denied\" error. I\nworked around it by explicitly changing the current directory...\n\nThen I realized FILE_WRITE_DATA wasn't necessary (but didn't\nconnect the dots). Then you noted the \"-C ..\" arg to test-tool\n(and I still didn't connect the dots).\n\nThe problem was never relative paths, but rather trying to get a\nwritable handle to the current directory. The only reason \"-C ..\"\nworked was that I already stopped trying to get a writable handle.\n\nI have no idea what it means to get a writable handle to a\ndirectory, but apparently you can't do it for your current\ndirectory. Now I know.\n\nThanks for the nudge, this is all clean now.\n"},{"id":"450054","messageId":"pull.1166.v3.git.1646201124.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.v2.git.1646127910.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-02T06:05:22Z","receivedAt":"2022-03-02T06:05:32Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Replace sleep calls with backdating of filesystem changes, but first fix the\ntest-tool chmtime functionality to work for directories in Windows.\n\nIn this third version, a bug with inaccessible-path-handling that Jeff found\nis fixed, and a relative-path-avoiding strategy that had been added in t7073\nis simplified out after Junio's comments revealed it was misguided.\n\nTao Klerks (2):\n t/helper/test-chmtime: update mingw to support chmtime on directories\n t7063: mtime-mangling instead of delays in untracked cache testing\n\n compat/mingw.c | 23 +++++++++++++++++++----\n t/t7063-status-untracked-cache.sh | 21 +++++++++------------\n 2 files changed, 28 insertions(+), 16 deletions(-)\n\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1166\n\nRange-diff vs v2:\n\n 1: 7cdef0ad5fb ! 1: 052b3dd9bba t/helper/test-chmtime: update mingw to support chmtime on directories\n @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n \n -\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n +\tosfilehandle = CreateFileW(wfilename,\n -+\t\t\t\t 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n ++\t\t\t\t FILE_WRITE_ATTRIBUTES,\n +\t\t\t\t 0 /*FileShare.None*/,\n +\t\t\t\t NULL,\n +\t\t\t\t OPEN_EXISTING,\n -+\t\t\t\t attrs & FILE_ATTRIBUTE_DIRECTORY ?\n ++\t\t\t\t (attrs != INVALID_FILE_ATTRIBUTES &&\n ++\t\t\t\t\t(attrs & FILE_ATTRIBUTE_DIRECTORY)) ?\n +\t\t\t\t\tFILE_FLAG_BACKUP_SEMANTICS : 0,\n +\t\t\t\t NULL);\n +\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n \t\taft = mft;\n \t}\n -\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n ++\n +\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n 2: 3e3c9c7faac ! 2: dceb2857609 t7063: mtime-mangling instead of delays in untracked cache testing\n @@ Commit message\n Signed-off-by: Tao Klerks \n \n ## t/t7063-status-untracked-cache.sh ##\n -@@ t/t7063-status-untracked-cache.sh: sync_mtime () {\n - \tfind . -type d -exec ls -ld {} + >/dev/null\n - }\n - \n -+chmtime_worktree_root () {\n -+\t# chmtime doesnt handle relative paths on windows, so need\n -+\t# to \"hardcode\" a reference to the worktree folder name.\n -+\ttest-tool -C .. chmtime $1 worktree\n -+}\n -+\n - avoid_racy() {\n - \tsleep 1\n - }\n @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' '\n \tcd worktree &&\n \tmkdir done dtwo dthree &&\n \ttouch one two three done/one dtwo/two dthree/three &&\n +\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n +\ttest-tool chmtime =-300 done dtwo dthree &&\n -+\tchmtime_worktree_root =-300 &&\n ++\ttest-tool chmtime =-300 . &&\n \tgit add one two done/one &&\n \t: >.git/info/exclude &&\n \tgit update-index --untracked-cache &&\n @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som\n -\trm base &&\n +\ttest-tool chmtime =-180 done/two done/three done/four done/five done &&\n +\t# we need to ensure that the root dir is touched (in the past);\n -+\tchmtime_worktree_root =-180 &&\n ++\ttest-tool chmtime =-180 . &&\n \tsync_mtime\n '\n \n\n-- \ngitgitgadget\n"},{"id":"450055","messageId":"052b3dd9bba8a79890863ace0992aaee41874402.1646201124.git.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.v3.git.1646201124.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-02T06:05:23Z","receivedAt":"2022-03-02T06:05:35Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nThe mingw_utime implementation in mingw.c does not support\ndirectories. This means that \"test-tool chmtime\" fails on Windows when\ntargeting directories. This has previously been noted and sidestepped\ntemporarily by Jeff Hostetler, in \"t/helper/test-chmtime: skip\ndirectories on Windows\" in the \"Builtin FSMonitor Part 2\" work, but\nnot yet fixed.\n\nIt would make sense to backdate file and folder changes in untracked\ncache tests, to avoid needing to insert explicit delays/pauses in the\ntests.\n\nAdd support for directory date manipulation in mingw_utime by\nreplacing the file-oriented _wopen() call with the\ndirectory-supporting CreateFileW() windows API explicitly.\n\nSigned-off-by: Tao Klerks \n---\n compat/mingw.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 03af369b2b9..58f347d6ae5 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)\n int mingw_utime (const char *file_name, const struct utimbuf *times)\n {\n \tFILETIME mft, aft;\n-\tint fh, rc;\n+\tint rc;\n \tDWORD attrs;\n \twchar_t wfilename[MAX_PATH];\n+\tHANDLE osfilehandle;\n+\n \tif (xutftowcs_path(wfilename, file_name) < 0)\n \t\treturn -1;\n \n@@ -975,7 +977,17 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n \t}\n \n-\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n+\tosfilehandle = CreateFileW(wfilename,\n+\t\t\t\t FILE_WRITE_ATTRIBUTES,\n+\t\t\t\t 0 /*FileShare.None*/,\n+\t\t\t\t NULL,\n+\t\t\t\t OPEN_EXISTING,\n+\t\t\t\t (attrs != INVALID_FILE_ATTRIBUTES &&\n+\t\t\t\t\t(attrs & FILE_ATTRIBUTE_DIRECTORY)) ?\n+\t\t\t\t\tFILE_FLAG_BACKUP_SEMANTICS : 0,\n+\t\t\t\t NULL);\n+\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n+\t\terrno = err_win_to_posix(GetLastError());\n \t\trc = -1;\n \t\tgoto revert_attrs;\n \t}\n@@ -987,12 +999,15 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \t\tGetSystemTimeAsFileTime(&mft);\n \t\taft = mft;\n \t}\n-\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n+\n+\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n \t\terrno = EINVAL;\n \t\trc = -1;\n \t} else\n \t\trc = 0;\n-\tclose(fh);\n+\n+\tif (osfilehandle != INVALID_HANDLE_VALUE)\n+\t\tCloseHandle(osfilehandle);\n \n revert_attrs:\n \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n-- \ngitgitgadget\n\n"},{"id":"450056","messageId":"dceb2857609fad8a8f93e489adc478ab0eb71c51.1646201124.git.gitgitgadget@gmail.com","threadId":"57482","inReplyTo":"pull.1166.v3.git.1646201124.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-02T06:05:24Z","receivedAt":"2022-03-02T06:05:38Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nThe untracked cache test uses an avoid_racy function to deal with\nan mtime-resolution challenge in testing: If an untracked cache\nentry's mtime falls in the same second as the mtime of the index\nthe untracked cache was stored in, then it cannot be trusted.\n\nExplicitly delaying tests is a simple effective strategy to\navoid these issues, but should be avoided where possible.\n\nSwitch from a delay-based strategy to instead backdating\nall file changes using test-tool chmtime, where that is an\noption, to shave 9 seconds off the test run time.\n\nDon't update test cases that delay for other reasons, for now at\nleast (4 seconds).\n\nSigned-off-by: Tao Klerks \n---\n t/t7063-status-untracked-cache.sh | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex a0c123b0a77..ca90ee805e7 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -90,6 +90,9 @@ test_expect_success 'setup' '\n \tcd worktree &&\n \tmkdir done dtwo dthree &&\n \ttouch one two three done/one dtwo/two dthree/three &&\n+\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n+\ttest-tool chmtime =-300 done dtwo dthree &&\n+\ttest-tool chmtime =-300 . &&\n \tgit add one two done/one &&\n \t: >.git/info/exclude &&\n \tgit update-index --untracked-cache &&\n@@ -142,7 +145,6 @@ two\n EOF\n \n test_expect_success 'status first time (empty cache)' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -166,7 +168,6 @@ test_expect_success 'untracked cache after first status' '\n '\n \n test_expect_success 'status second time (fully populated cache)' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -190,8 +191,8 @@ test_expect_success 'untracked cache after second status' '\n '\n \n test_expect_success 'modify in root directory, one dir invalidation' '\n-\tavoid_racy &&\n \t: >four &&\n+\ttest-tool chmtime =-240 four &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../actual &&\n@@ -241,7 +242,6 @@ EOF\n '\n \n test_expect_success 'new .gitignore invalidates recursively' '\n-\tavoid_racy &&\n \techo four >.gitignore &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n@@ -292,7 +292,6 @@ EOF\n '\n \n test_expect_success 'new info/exclude invalidates everything' '\n-\tavoid_racy &&\n \techo three >>.git/info/exclude &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n@@ -520,14 +519,14 @@ test_expect_success 'create/modify files, some of which are gitignored' '\n \techo three >done/three && # three is gitignored\n \techo four >done/four && # four is gitignored at a higher level\n \techo five >done/five && # five is not gitignored\n-\techo test >base && #we need to ensure that the root dir is touched\n-\trm base &&\n+\ttest-tool chmtime =-180 done/two done/three done/four done/five done &&\n+\t# we need to ensure that the root dir is touched (in the past);\n+\ttest-tool chmtime =-180 . &&\n \tsync_mtime\n '\n \n test_expect_success 'test sparse status with untracked cache' '\n \t: >../trace.output &&\n-\tavoid_racy &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n \tiuc status --porcelain >../status.iuc &&\n@@ -570,7 +569,6 @@ EOF\n '\n \n test_expect_success 'test sparse status again with untracked cache' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n@@ -597,11 +595,11 @@ EOF\n test_expect_success 'set up for test of subdir and sparse checkouts' '\n \tmkdir done/sub &&\n \tmkdir done/sub/sub &&\n-\techo \"sub\" > done/sub/sub/file\n+\techo \"sub\" > done/sub/sub/file &&\n+\ttest-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done\n '\n \n test_expect_success 'test sparse status with untracked cache and subdir' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n@@ -651,7 +649,6 @@ EOF\n '\n \n test_expect_success 'test sparse status again with untracked cache and subdir' '\n-\tavoid_racy &&\n \t: >../trace.output &&\n \tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n \tgit status --porcelain >../status.actual &&\n-- \ngitgitgadget\n"},{"id":"450485","messageId":"CAPMMpogoVzHBh8j4GZG-tRpAxap4wY+hAray5V_QUcaNpf7dEQ@mail.gmail.com","threadId":"57482","inReplyTo":"pull.1166.v3.git.1646201124.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-05T04:24:01Z","receivedAt":"2022-03-05T04:24:16Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Hi Junio,\n\nI see that in the latest \"what's coming\" update, this patch series is\nlisted as expecting reroll, but as far as I can see there have been no\ncomments since I sent this V3 out.\n\nWhat should be my next step, if I think this is \"as ready as I know to\nget it\" without any comments on the latest submission?\n\nThanks,\nTao\n\n(Apologies to recipients of duplicate email in cc, I accidentally sent\nhtml email that the list rejected)\n\nOn Wed, Mar 2, 2022, 07:05 Tao Klerks via GitGitGadget\n wrote:\n>\n> Replace sleep calls with backdating of filesystem changes, but first fix the\n> test-tool chmtime functionality to work for directories in Windows.\n>\n> In this third version, a bug with inaccessible-path-handling that Jeff found\n> is fixed, and a relative-path-avoiding strategy that had been added in t7073\n> is simplified out after Junio's comments revealed it was misguided.\n>\n> Tao Klerks (2):\n> t/helper/test-chmtime: update mingw to support chmtime on directories\n> t7063: mtime-mangling instead of delays in untracked cache testing\n>\n> compat/mingw.c | 23 +++++++++++++++++++----\n> t/t7063-status-untracked-cache.sh | 21 +++++++++------------\n> 2 files changed, 28 insertions(+), 16 deletions(-)\n>\n>\n> base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1166\n>\n> Range-diff vs v2:\n>\n> 1: 7cdef0ad5fb ! 1: 052b3dd9bba t/helper/test-chmtime: update mingw to support chmtime on directories\n> @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n>\n> - if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> + osfilehandle = CreateFileW(wfilename,\n> -+ 0x100 /*FILE_WRITE_ATTRIBUTES*/,\n> ++ FILE_WRITE_ATTRIBUTES,\n> + 0 /*FileShare.None*/,\n> + NULL,\n> + OPEN_EXISTING,\n> -+ attrs & FILE_ATTRIBUTE_DIRECTORY ?\n> ++ (attrs != INVALID_FILE_ATTRIBUTES &&\n> ++ (attrs & FILE_ATTRIBUTE_DIRECTORY)) ?\n> + FILE_FLAG_BACKUP_SEMANTICS : 0,\n> + NULL);\n> + if (osfilehandle == INVALID_HANDLE_VALUE) {\n> @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti\n> aft = mft;\n> }\n> - if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n> ++\n> + if (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n> errno = EINVAL;\n> rc = -1;\n> 2: 3e3c9c7faac ! 2: dceb2857609 t7063: mtime-mangling instead of delays in untracked cache testing\n> @@ Commit message\n> Signed-off-by: Tao Klerks \n>\n> ## t/t7063-status-untracked-cache.sh ##\n> -@@ t/t7063-status-untracked-cache.sh: sync_mtime () {\n> - find . -type d -exec ls -ld {} + >/dev/null\n> - }\n> -\n> -+chmtime_worktree_root () {\n> -+ # chmtime doesnt handle relative paths on windows, so need\n> -+ # to \"hardcode\" a reference to the worktree folder name.\n> -+ test-tool -C .. chmtime $1 worktree\n> -+}\n> -+\n> - avoid_racy() {\n> - sleep 1\n> - }\n> @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' '\n> cd worktree &&\n> mkdir done dtwo dthree &&\n> touch one two three done/one dtwo/two dthree/three &&\n> + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n> + test-tool chmtime =-300 done dtwo dthree &&\n> -+ chmtime_worktree_root =-300 &&\n> ++ test-tool chmtime =-300 . &&\n> git add one two done/one &&\n> : >.git/info/exclude &&\n> git update-index --untracked-cache &&\n> @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som\n> - rm base &&\n> + test-tool chmtime =-180 done/two done/three done/four done/five done &&\n> + # we need to ensure that the root dir is touched (in the past);\n> -+ chmtime_worktree_root =-180 &&\n> ++ test-tool chmtime =-180 . &&\n> sync_mtime\n> '\n>\n>\n> --\n> gitgitgadget\n"},{"id":"450505","messageId":"xmqqv8wqvj54.fsf@gitster.g","threadId":"57482","inReplyTo":"CAPMMpogoVzHBh8j4GZG-tRpAxap4wY+hAray5V_QUcaNpf7dEQ@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-06T21:57:43Z","receivedAt":"2022-03-06T21:57:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n> I see that in the latest \"what's coming\" update, this patch series is\n> listed as expecting reroll, but as far as I can see there have been no\n> comments since I sent this V3 out.\n\nI see reference on the source: line for that topic in the\n\"What's cooking\" report you are referring to, so I suspect I have\nbeen too busy and haven't got around to replacing it with ,\nperhaps?\n\n"},{"id":"450509","messageId":"CAPMMpojOrnSRQgA51+O7wfc3tGFO8w9mtejw8i8h1MoA5WvS5Q@mail.gmail.com","threadId":"57482","inReplyTo":"xmqqv8wqvj54.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-07T05:37:02Z","receivedAt":"2022-03-07T05:37:18Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Eek, my apologies. I will remember to check that!\n\nOn Sun, Mar 6, 2022 at 10:57 PM Junio C Hamano wrote:\n>\n> Tao Klerks writes:\n>\n> > I see that in the latest \"what's coming\" update, this patch series is\n> > listed as expecting reroll, but as far as I can see there have been no\n> > comments since I sent this V3 out.\n>\n> I see reference on the source: line for that topic in the\n> \"What's cooking\" report you are referring to, so I suspect I have\n> been too busy and haven't got around to replacing it with ,\n> perhaps?\n>\n"},{"id":"450629","messageId":"xmqqlexlsk77.fsf@gitster.g","threadId":"57482","inReplyTo":"CAPMMpojOrnSRQgA51+O7wfc3tGFO8w9mtejw8i8h1MoA5WvS5Q@mail.gmail.com","subject":"Re: [PATCH v3 0/2] Reduce explicit sleep calls in t7063 untracked cache tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-07T18:15:24Z","receivedAt":"2022-03-07T18:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n> On Sun, Mar 6, 2022 at 10:57 PM Junio C Hamano wrote:\n>>\n>> Tao Klerks writes:\n>>\n>> > I see that in the latest \"what's coming\" update, this patch series is\n>> > listed as expecting reroll, but as far as I can see there have been no\n>> > comments since I sent this V3 out.\n>>\n>> I see reference on the source: line for that topic in the\n>> \"What's cooking\" report you are referring to, so I suspect I have\n>> been too busy and haven't got around to replacing it with ,\n>> perhaps?\n>>\n> Eek, my apologies. I will remember to check that!\n\nNo need to apologize.\n\nIt often happens that I forget to update the status of a series\nafter a new iteration replaces the old one when sending out a new\nissue of the \"What's cooking\" report. It is very much appreciated\nto be watchful and report such mistakes when you notice it.\n\nThis time, it turns out that what happened was that I updated the\n\"What's cooking\" report and sent it out, and then continued working\nfor the rest of the day to queue new topics and replace existing\nones. And when you pinged me, I already had queued.\n\nTo me, judging from the exchanges between you and Jeff Hostetler,\nthe topic at v3 seems to be ready for 'next', so let me mark it as\nsuch in the draft of the next issue of \"What's cooking\" I keep\nlocally.\n\nThanks.\n\n\n\n\n"},{"id":"450933","messageId":"nycvar.QRO.7.76.6.2203092234440.357@tvgsbejvaqbjf.bet","threadId":"57482","inReplyTo":"052b3dd9bba8a79890863ace0992aaee41874402.1646201124.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-09T21:46:35Z","receivedAt":"2022-03-09T21:46:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tao,\n\nOn Wed, 2 Mar 2022, Tao Klerks via GitGitGadget wrote:\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 03af369b2b9..58f347d6ae5 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -961,9 +961,11 @@ static inline void time_t_to_filetime(time_t t, FILETIME *ft)\n> int mingw_utime (const char *file_name, const struct utimbuf *times)\n> {\n> \tFILETIME mft, aft;\n> -\tint fh, rc;\n> +\tint rc;\n> \tDWORD attrs;\n> \twchar_t wfilename[MAX_PATH];\n> +\tHANDLE osfilehandle;\n\nI would prefer the short-and-sweet name `handle` here. _Especially_ since\nwe are no longer using `_get_osfhandle()` at all anymore, meaning that\nthe name is actually misleading.\n\n> +\n> \tif (xutftowcs_path(wfilename, file_name) < 0)\n> \t\treturn -1;\n>\n> @@ -975,7 +977,17 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tSetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY);\n> \t}\n>\n> -\tif ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {\n> +\tosfilehandle = CreateFileW(wfilename,\n> +\t\t\t\t FILE_WRITE_ATTRIBUTES,\n> +\t\t\t\t 0 /*FileShare.None*/,\n\nHmm. I wonder why you don't want to allow shared access? Like, why\ndisallow changing the mtime while a file is being read?\n\nThis is a change in behavior, and I think we should avoid that. Wine uses\n`FILE_SHARE_READ | FILE_SHARE_WRITE` in its `_wopen()` implementation\n(there are a couple of layers of indirection leading all the way down to\nhttps://github.com/wine-mirror/wine/blob/1d178982ae5a73b18f367026c8689b56789c39fd/dlls/msvcrt/file.c#L2261).\n\nThe closest already-existing code that creates a file handle to a\ndirectory is in `mingw_getcwd()`, and it even adds `FILE_SHARE_DELETE` to\nthe fray. That would probably be the best here, too.\n\nThe rest of the patch\n> +\t\t\t\t NULL,\n> +\t\t\t\t OPEN_EXISTING,\n> +\t\t\t\t (attrs != INVALID_FILE_ATTRIBUTES &&\n> +\t\t\t\t\t(attrs & FILE_ATTRIBUTE_DIRECTORY)) ?\n> +\t\t\t\t\tFILE_FLAG_BACKUP_SEMANTICS : 0,\n> +\t\t\t\t NULL);\n> +\tif (osfilehandle == INVALID_HANDLE_VALUE) {\n> +\t\terrno = err_win_to_posix(GetLastError());\n> \t\trc = -1;\n> \t\tgoto revert_attrs;\n> \t}\n> @@ -987,12 +999,15 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n> \t\tGetSystemTimeAsFileTime(&mft);\n> \t\taft = mft;\n> \t}\n> -\tif (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {\n> +\n> +\tif (!SetFileTime(osfilehandle, NULL, &aft, &mft)) {\n> \t\terrno = EINVAL;\n> \t\trc = -1;\n> \t} else\n> \t\trc = 0;\n> -\tclose(fh);\n> +\n> +\tif (osfilehandle != INVALID_HANDLE_VALUE)\n> +\t\tCloseHandle(osfilehandle);\n\nIt is quite obvious from the diff that it is quite impossible for\n`osfilehandle` to equal `INVALID_HANDLE_VALUE`, because if that is the\ncase, we specifically `goto revert_attrs` which is two lines below:\n\n>\n> revert_attrs:\n> \tif (attrs != INVALID_FILE_ATTRIBUTES &&\n> --\n> gitgitgadget\n\nTherefore, I would prefer the `CloseHandle()` to be as unconditional as\nthe now-replaced `close()` was.\n\nThank you,\nDscho\n"},{"id":"450936","messageId":"nycvar.QRO.7.76.6.2203092250080.357@tvgsbejvaqbjf.bet","threadId":"57482","inReplyTo":"dceb2857609fad8a8f93e489adc478ab0eb71c51.1646201124.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] t7063: mtime-mangling instead of delays in untracked cache testing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-09T21:53:22Z","receivedAt":"2022-03-09T21:53:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tao,\n\nOn Wed, 2 Mar 2022, Tao Klerks via GitGitGadget wrote:\n\n> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n> index a0c123b0a77..ca90ee805e7 100755\n> --- a/t/t7063-status-untracked-cache.sh\n> +++ b/t/t7063-status-untracked-cache.sh\n> @@ -90,6 +90,9 @@ test_expect_success 'setup' '\n> \tcd worktree &&\n> \tmkdir done dtwo dthree &&\n> \ttouch one two three done/one dtwo/two dthree/three &&\n> +\ttest-tool chmtime =-300 one two three done/one dtwo/two dthree/three &&\n> +\ttest-tool chmtime =-300 done dtwo dthree &&\n> +\ttest-tool chmtime =-300 . &&\n\nAt first I puzzled why we need `done`, `dtwo`, dthree` and `.`, too, but\nit makes sense: the invariant is that all of the files/directories whose\nmtime is adjusted in these three new lines have the same mtime.\n\nA similar issue is...\n\n> @@ -520,14 +519,14 @@ test_expect_success 'create/modify files, some of which are gitignored' '\n> \techo three >done/three && # three is gitignored\n> \techo four >done/four && # four is gitignored at a higher level\n> \techo five >done/five && # five is not gitignored\n> -\techo test >base && #we need to ensure that the root dir is touched\n> -\trm base &&\n> +\ttest-tool chmtime =-180 done/two done/three done/four done/five done &&\n> +\t# we need to ensure that the root dir is touched (in the past);\n> +\ttest-tool chmtime =-180 . &&\n\nHere, where I needed a moment to understand the invariant (and hence the\nneed to adjust more than just the root directory's mtime).\n\n\n> @@ -597,11 +595,11 @@ EOF\n> test_expect_success 'set up for test of subdir and sparse checkouts' '\n> \tmkdir done/sub &&\n> \tmkdir done/sub/sub &&\n> -\techo \"sub\" > done/sub/sub/file\n> +\techo \"sub\" > done/sub/sub/file &&\n> +\ttest-tool chmtime =-120 done/sub/sub/file done/sub/sub done/sub done\n\nSimilar situation here, too.\n\nAll this is to say that this patch looks good to me.\n\nThanks,\nDscho\n"}]}