Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
From: Jeff Hostetler <hidden>
Date: 2022-02-28 15:27:16
On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Tao Klerks <redacted> 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. 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. Signed-off-by: Tao Klerks <redacted> --- compat/mingw.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-)diff --git a/compat/mingw.c b/compat/mingw.c index 03af369b2b9..2284ea90511 100644 --- a/compat/mingw.c +++ b/compat/mingw.c@@ -964,6 +964,8 @@ int mingw_utime (const char *file_name, const struct utimbuf *times) int fh, rc; DWORD attrs; wchar_t wfilename[MAX_PATH]; + HANDLE osfilehandle;
I'd be more comfortable initializing this variable to INVALID_HANDLE_VALUE.
quoted hunk ↗ jump to hunk
+ if (xutftowcs_path(wfilename, file_name) < 0) return -1;@@ -975,9 +977,26 @@ int mingw_utime (const char *file_name, const struct utimbuf *times) SetFileAttributesW(wfilename, attrs & ~FILE_ATTRIBUTE_READONLY); } - if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) { - rc = -1; - goto revert_attrs; + if (attrs & FILE_ATTRIBUTE_DIRECTORY) { + fh = 0;
and here initializing fh = -1.
+ osfilehandle = CreateFileW(wfilename, + 0x100 /*FILE_WRITE_ATTRIBUTES*/, + 0 /*FileShare.None*/,
Is there a reason that you're not using the #define's here? I assume you ran into a header file problem or something, but there are other symbols used nearby, so I'm not sure.
quoted hunk ↗ jump to hunk
+ 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); } if (times) {@@ -987,12 +1006,16 @@ int mingw_utime (const char *file_name, const struct utimbuf *times) GetSystemTimeAsFileTime(&mft); aft = mft; } - if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) { + if (!SetFileTime(osfilehandle, NULL, &aft, &mft)) { errno = EINVAL; rc = -1; } else rc = 0; - close(fh); + + if (fh) + close(fh); + else if (osfilehandle) + CloseHandle(osfilehandle);
And then this becomes: if (fh != -1) close(fh) else if (osfilehandle != INVALID_HANDLE_VALUE) Closehandle(osfilehandle); Which I think makes it more clear that we'll properly close the handle.
revert_attrs: if (attrs != INVALID_FILE_ATTRIBUTES &&
An alternative solution would be to replace the _wopen() with a call to CreateFileW() without the backup flag for non-directories and just convert the whole routine to use HANDLE's rather fd's. Thanks Jeff