| Message ID | pull.1166.v2.git.1646127910.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Reduce explicit sleep calls in t7063 untracked cache tests | expand |
> I do have a question to the list here: Do mingw.c changes need to be > upstreamed somewhere? I don't understand the exact relationship between this > file and the MinGW project. (sorry, failed to update the cover letter to remove the now-answered question :( ) On Tue, Mar 1, 2022 at 10:45 AM Tao Klerks via GitGitGadget <gitgitgadget@gmail.com> wrote: > > As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number > of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a > second, in order to avoid the untracked cache content being invalidated by > an mtime race condition. > > Even though it's only 9 seconds of sleeping that can be straightforwardly > replaced, it seems worth fixing if possible. > > Replace sleep calls with backdating of filesystem changes, but first fix the > test-tool chmtime functionality to work for directories in Windows. > > I do have a question to the list here: Do mingw.c changes need to be > upstreamed somewhere? I don't understand the exact relationship between this > file and the MinGW project. > > Tao Klerks (2): > t/helper/test-chmtime: update mingw to support chmtime on directories > t7063: mtime-mangling instead of delays in untracked cache testing > > compat/mingw.c | 21 +++++++++++++++++---- > t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------ > 2 files changed, 32 insertions(+), 16 deletions(-) > > > base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1166 > > Range-diff vs v1: > > 1: 76b6216281e ! 1: 7cdef0ad5fb t/helper/test-chmtime: update mingw to support chmtime on directories > @@ Commit message > The mingw_utime implementation in mingw.c does not support > directories. This means that "test-tool chmtime" fails on Windows when > targeting directories. This has previously been noted and sidestepped > - by Jeff Hostetler, in "t/helper/test-chmtime: skip directories > - on Windows" in the "Builtin FSMonitor Part 2" work. > + temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip > + directories on Windows" in the "Builtin FSMonitor Part 2" work, but > + not yet fixed. > > It would make sense to backdate file and folder changes in untracked > cache tests, to avoid needing to insert explicit delays/pauses in the > tests. > > - Add support for directory date manipulation in mingw_utime by calling > - CreateFileW() explicitly to create the directory handle, and > - CloseHandle() to close it. > + Add support for directory date manipulation in mingw_utime by > + replacing the file-oriented _wopen() call with the > + directory-supporting CreateFileW() windows API explicitly. > > Signed-off-by: Tao Klerks <tao@klerks.biz> > > ## compat/mingw.c ## > -@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times) > - int fh, rc; > +@@ compat/mingw.c: static inline void time_t_to_filetime(time_t t, FILETIME *ft) > + int mingw_utime (const char *file_name, const struct utimbuf *times) > + { > + FILETIME mft, aft; > +- int fh, rc; > ++ int rc; > DWORD attrs; > wchar_t wfilename[MAX_PATH]; > + HANDLE osfilehandle; > @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti > } > > - if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) { > -- rc = -1; > -- goto revert_attrs; > -+ if (attrs & FILE_ATTRIBUTE_DIRECTORY) { > -+ fh = 0; > -+ osfilehandle = CreateFileW(wfilename, > -+ 0x100 /*FILE_WRITE_ATTRIBUTES*/, > -+ 0 /*FileShare.None*/, > -+ NULL, > -+ OPEN_EXISTING, > -+ FILE_FLAG_BACKUP_SEMANTICS, > -+ NULL); > -+ if (osfilehandle == INVALID_HANDLE_VALUE) { > -+ errno = err_win_to_posix(GetLastError()); > -+ rc = -1; > -+ goto revert_attrs; > -+ } > -+ } else { > -+ if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) { > -+ rc = -1; > -+ goto revert_attrs; > -+ } > -+ osfilehandle = (HANDLE)_get_osfhandle(fh); > ++ osfilehandle = CreateFileW(wfilename, > ++ 0x100 /*FILE_WRITE_ATTRIBUTES*/, > ++ 0 /*FileShare.None*/, > ++ NULL, > ++ OPEN_EXISTING, > ++ attrs & FILE_ATTRIBUTE_DIRECTORY ? > ++ FILE_FLAG_BACKUP_SEMANTICS : 0, > ++ NULL); > ++ if (osfilehandle == INVALID_HANDLE_VALUE) { > ++ errno = err_win_to_posix(GetLastError()); > + rc = -1; > + goto revert_attrs; > } > - > - if (times) { > @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times) > GetSystemTimeAsFileTime(&mft); > aft = mft; > @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti > rc = 0; > - close(fh); > + > -+ if (fh) > -+ close(fh); > -+ else if (osfilehandle) > ++ if (osfilehandle != INVALID_HANDLE_VALUE) > + CloseHandle(osfilehandle); > > revert_attrs: > 2: a1806c56333 ! 2: 3e3c9c7faac t7063: mtime-mangling instead of delays in untracked cache testing > @@ t/t7063-status-untracked-cache.sh: sync_mtime () { > find . -type d -exec ls -ld {} + >/dev/null > } > > -+chmmtime_worktree_root () { > ++chmtime_worktree_root () { > + # chmtime doesnt handle relative paths on windows, so need > + # to "hardcode" a reference to the worktree folder name. > -+ cd .. && > -+ test-tool chmtime $1 worktree && > -+ cd worktree > ++ test-tool -C .. chmtime $1 worktree > +} > + > avoid_racy() { > @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' ' > touch one two three done/one dtwo/two dthree/three && > + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && > + test-tool chmtime =-300 done dtwo dthree && > -+ chmmtime_worktree_root =-300 && > ++ chmtime_worktree_root =-300 && > git add one two done/one && > : >.git/info/exclude && > git update-index --untracked-cache && > @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som > - rm base && > + test-tool chmtime =-180 done/two done/three done/four done/five done && > + # we need to ensure that the root dir is touched (in the past); > -+ chmmtime_worktree_root =-180 && > ++ chmtime_worktree_root =-180 && > sync_mtime > ' > > > -- > gitgitgadget
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number > of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a > second, in order to avoid the untracked cache content being invalidated by > an mtime race condition. > > Even though it's only 9 seconds of sleeping that can be straightforwardly > replaced, it seems worth fixing if possible. Absolutely. Thanks for an update.
As noted in a recent proposed patch to t/t7519-status-fsmonitor.sh, a number of test cases in t\t7063-status-untracked-cache.sh explicitly sleep a second, in order to avoid the untracked cache content being invalidated by an mtime race condition. Even though it's only 9 seconds of sleeping that can be straightforwardly replaced, it seems worth fixing if possible. Replace sleep calls with backdating of filesystem changes, but first fix the test-tool chmtime functionality to work for directories in Windows. I do have a question to the list here: Do mingw.c changes need to be upstreamed somewhere? I don't understand the exact relationship between this file and the MinGW project. Tao Klerks (2): t/helper/test-chmtime: update mingw to support chmtime on directories t7063: mtime-mangling instead of delays in untracked cache testing compat/mingw.c | 21 +++++++++++++++++---- t/t7063-status-untracked-cache.sh | 27 +++++++++++++++------------ 2 files changed, 32 insertions(+), 16 deletions(-) base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1166%2FTaoK%2Ftaok-untracked-cache-testing-remote-waits-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1166/TaoK/taok-untracked-cache-testing-remote-waits-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1166 Range-diff vs v1: 1: 76b6216281e ! 1: 7cdef0ad5fb t/helper/test-chmtime: update mingw to support chmtime on directories @@ Commit message The mingw_utime implementation in mingw.c does not support directories. This means that "test-tool chmtime" fails on Windows when targeting directories. This has previously been noted and sidestepped - by Jeff Hostetler, in "t/helper/test-chmtime: skip directories - on Windows" in the "Builtin FSMonitor Part 2" work. + temporarily by Jeff Hostetler, in "t/helper/test-chmtime: skip + directories on Windows" in the "Builtin FSMonitor Part 2" work, but + not yet fixed. It would make sense to backdate file and folder changes in untracked cache tests, to avoid needing to insert explicit delays/pauses in the tests. - Add support for directory date manipulation in mingw_utime by calling - CreateFileW() explicitly to create the directory handle, and - CloseHandle() to close it. + Add support for directory date manipulation in mingw_utime by + replacing the file-oriented _wopen() call with the + directory-supporting CreateFileW() windows API explicitly. Signed-off-by: Tao Klerks <tao@klerks.biz> ## compat/mingw.c ## -@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times) - int fh, rc; +@@ compat/mingw.c: static inline void time_t_to_filetime(time_t t, FILETIME *ft) + int mingw_utime (const char *file_name, const struct utimbuf *times) + { + FILETIME mft, aft; +- int fh, rc; ++ int rc; DWORD attrs; wchar_t wfilename[MAX_PATH]; + HANDLE osfilehandle; @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti } - if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) { -- rc = -1; -- goto revert_attrs; -+ if (attrs & FILE_ATTRIBUTE_DIRECTORY) { -+ fh = 0; -+ osfilehandle = CreateFileW(wfilename, -+ 0x100 /*FILE_WRITE_ATTRIBUTES*/, -+ 0 /*FileShare.None*/, -+ NULL, -+ OPEN_EXISTING, -+ FILE_FLAG_BACKUP_SEMANTICS, -+ NULL); -+ if (osfilehandle == INVALID_HANDLE_VALUE) { -+ errno = err_win_to_posix(GetLastError()); -+ rc = -1; -+ goto revert_attrs; -+ } -+ } else { -+ if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) { -+ rc = -1; -+ goto revert_attrs; -+ } -+ osfilehandle = (HANDLE)_get_osfhandle(fh); ++ osfilehandle = CreateFileW(wfilename, ++ 0x100 /*FILE_WRITE_ATTRIBUTES*/, ++ 0 /*FileShare.None*/, ++ NULL, ++ OPEN_EXISTING, ++ attrs & FILE_ATTRIBUTE_DIRECTORY ? ++ FILE_FLAG_BACKUP_SEMANTICS : 0, ++ NULL); ++ if (osfilehandle == INVALID_HANDLE_VALUE) { ++ errno = err_win_to_posix(GetLastError()); + rc = -1; + goto revert_attrs; } - - if (times) { @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *times) GetSystemTimeAsFileTime(&mft); aft = mft; @@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti rc = 0; - close(fh); + -+ if (fh) -+ close(fh); -+ else if (osfilehandle) ++ if (osfilehandle != INVALID_HANDLE_VALUE) + CloseHandle(osfilehandle); revert_attrs: 2: a1806c56333 ! 2: 3e3c9c7faac t7063: mtime-mangling instead of delays in untracked cache testing @@ t/t7063-status-untracked-cache.sh: sync_mtime () { find . -type d -exec ls -ld {} + >/dev/null } -+chmmtime_worktree_root () { ++chmtime_worktree_root () { + # chmtime doesnt handle relative paths on windows, so need + # to "hardcode" a reference to the worktree folder name. -+ cd .. && -+ test-tool chmtime $1 worktree && -+ cd worktree ++ test-tool -C .. chmtime $1 worktree +} + avoid_racy() { @@ t/t7063-status-untracked-cache.sh: test_expect_success 'setup' ' touch one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 one two three done/one dtwo/two dthree/three && + test-tool chmtime =-300 done dtwo dthree && -+ chmmtime_worktree_root =-300 && ++ chmtime_worktree_root =-300 && git add one two done/one && : >.git/info/exclude && git update-index --untracked-cache && @@ t/t7063-status-untracked-cache.sh: test_expect_success 'create/modify files, som - rm base && + test-tool chmtime =-180 done/two done/three done/four done/five done && + # we need to ensure that the root dir is touched (in the past); -+ chmmtime_worktree_root =-180 && ++ chmtime_worktree_root =-180 && sync_mtime '