diff mbox series

[v3,1/2] t/helper/test-chmtime: update mingw to support chmtime on directories

Message ID 052b3dd9bba8a79890863ace0992aaee41874402.1646201124.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 090a3085bc9f2101b69b3c8940278fb7c22f02c9
Headers show
Series Reduce explicit sleep calls in t7063 untracked cache tests | expand

Commit Message

Tao Klerks March 2, 2022, 6:05 a.m. UTC 
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
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
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 | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Johannes Schindelin March 9, 2022, 9:46 p.m. UTC | #1 
Hi Tao,

On Wed, 2 Mar 2022, Tao Klerks via GitGitGadget wrote:

> diff --git a/compat/mingw.c b/compat/mingw.c
> index 03af369b2b9..58f347d6ae5 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -961,9 +961,11 @@ 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;

I would prefer the short-and-sweet name `handle` here. _Especially_ since
we are no longer using `_get_osfhandle()` at all anymore, meaning that
the name is actually misleading.

> +
>  	if (xutftowcs_path(wfilename, file_name) < 0)
>  		return -1;
>
> @@ -975,7 +977,17 @@ 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) {
> +	osfilehandle = CreateFileW(wfilename,
> +				   FILE_WRITE_ATTRIBUTES,
> +				   0 /*FileShare.None*/,

Hmm. I wonder why you don't want to allow shared access? Like, why
disallow changing the mtime while a file is being read?

This is a change in behavior, and I think we should avoid that. Wine uses
`FILE_SHARE_READ | FILE_SHARE_WRITE` in its `_wopen()` implementation
(there are a couple of layers of indirection leading all the way down to
https://github.com/wine-mirror/wine/blob/1d178982ae5a73b18f367026c8689b56789c39fd/dlls/msvcrt/file.c#L2261).

The closest already-existing code that creates a file handle to a
directory is in `mingw_getcwd()`, and it even adds `FILE_SHARE_DELETE` to
the fray. That would probably be the best here, too.

The rest of the patch
> +				   NULL,
> +				   OPEN_EXISTING,
> +				   (attrs != INVALID_FILE_ATTRIBUTES &&
> +					(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;
>  	}
> @@ -987,12 +999,15 @@ 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 (osfilehandle != INVALID_HANDLE_VALUE)
> +		CloseHandle(osfilehandle);

It is quite obvious from the diff that it is quite impossible for
`osfilehandle` to equal `INVALID_HANDLE_VALUE`, because if that is the
case, we specifically `goto revert_attrs` which is two lines below:

>
>  revert_attrs:
>  	if (attrs != INVALID_FILE_ATTRIBUTES &&
> --
> gitgitgadget

Therefore, I would prefer the `CloseHandle()` to be as unconditional as
the now-replaced `close()` was.

Thank you,
Dscho
diff mbox series

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index 03af369b2b9..58f347d6ae5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -961,9 +961,11 @@  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;
+
 	if (xutftowcs_path(wfilename, file_name) < 0)
 		return -1;
 
@@ -975,7 +977,17 @@  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) {
+	osfilehandle = CreateFileW(wfilename,
+				   FILE_WRITE_ATTRIBUTES,
+				   0 /*FileShare.None*/,
+				   NULL,
+				   OPEN_EXISTING,
+				   (attrs != INVALID_FILE_ATTRIBUTES &&
+					(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;
 	}
@@ -987,12 +999,15 @@  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 (osfilehandle != INVALID_HANDLE_VALUE)
+		CloseHandle(osfilehandle);
 
 revert_attrs:
 	if (attrs != INVALID_FILE_ATTRIBUTES &&