Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> From: Tao Klerks <tao@klerks.biz>
>
> 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.
I was expecting that this will be applicable _before_ FSMonitor Part 2 and later. This mention would probably belong to the comment after three-dashes?
Show 7 quoted lines
> 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.
Show 5 quoted lines
> @@ -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;
The variable is not initialized here, but that is perfectly OK. We'll set it in both branches for directories and files.
Show 11 quoted lines
> 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) {Excellent. We can reuse existing attrs without any extra calls to introduce the new codepath to deal with directories.
> + fh = 0;
This should be
fh = -1;
instead. More on this later.
Show 12 quoted lines
> + 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;
> + }Upon an error, we'll jump to revert_attrs but otherwise, we will have a valid osfilehandle (which presumably is not NULL).
Show 6 quoted lines
> + } else {
> + if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
> + rc = -1;
> + goto revert_attrs;
> + }
> + osfilehandle = (HANDLE)_get_osfhandle(fh);This side is the same as before. This code assumes that, as long as _wopen() gives us a usable fh, _get_osfhandle(fh) would always give us a valid handle. This assumption should be safe, as the original code has been relying on it anyway.
Show 6 quoted lines
> @@ -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)) {And we use the osfilehandle we obtained, either from the code that is identical to the original for files or from the new code for directories. Good.
Show 10 quoted lines
> errno = EINVAL;
> rc = -1;
> } else
> rc = 0;
> - close(fh);
> +
> + if (fh)
> + close(fh);
> + else if (osfilehandle)
> + CloseHandle(osfilehandle);
In the context of "git" process, I do not think we would ever close FD#0, so it may be safe to assume that _wopen() above will never yield FD#0, so this is not quite wrong per-se, but to be future-proof, it would be even safer to instead do:
if (0 <= fh)
close(fh);
else if (osfilehandle)
CloseHandle(osfilehandle);
here. That is consistent with the error checking done where _wopen() was called to obtain it above, i.e.
if ((fh = _wopen(...)) < 0)
... error ...
> revert_attrs:
> if (attrs != INVALID_FILE_ATTRIBUTES &&
Tao Klerks via GitGitGadget
