diff mbox series

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

Message ID 7cdef0ad5fb6fc1a16ee1cee27b9dec0300c8c1d.1646127910.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Reduce explicit sleep calls in t7063 untracked cache tests | expand

Commit Message

Tao Klerks March 1, 2022, 9:45 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 | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Jeff Hostetler March 1, 2022, 4:34 p.m. UTC | #1 
On 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:
> 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 | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 03af369b2b9..11c43ad2dfb 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,16 @@ 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,
> +				   0x100 /*FILE_WRITE_ATTRIBUTES*/,

https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants

indicates that FILE_WRITE_ATTRIBUTES is defined in <WinNT.h> which
we get from <windows.h> which was included by "win32.h", so it should
already be present.

> +				   0 /*FileShare.None*/,
> +				   NULL,
> +				   OPEN_EXISTING,
> +				   attrs & FILE_ATTRIBUTE_DIRECTORY ?
> +					FILE_FLAG_BACKUP_SEMANTICS : 0,

There is a weird error case here.  If the GetFileAttributesW() call
at the top fails, it returns INVALID_FILE_ATTRIBUTES (aka -1).  So
the (attrs & ...) here expression is questionable.

I'm not sure that there is a best way to handle the earlier failure
(other than returning an error at the top), but we do try to limp
along (for some reason).

So maybe make this something like:

     (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 +998,14 @@ 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 &&
>
Tao Klerks March 1, 2022, 9:14 p.m. UTC | #2 
On Tue, Mar 1, 2022 at 5:34 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
> On 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:
> >
> > -     if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
> > +     osfilehandle = CreateFileW(wfilename,
> > +                                0x100 /*FILE_WRITE_ATTRIBUTES*/,
>
> https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants
>
> indicates that FILE_WRITE_ATTRIBUTES is defined in <WinNT.h> which
> we get from <windows.h> which was included by "win32.h", so it should
> already be present.
>

Grr, should have asked the compiler instead of VSCode's autocomplete.

Thx, fixed.

> > +                                0 /*FileShare.None*/,
> > +                                NULL,
> > +                                OPEN_EXISTING,
> > +                                attrs & FILE_ATTRIBUTE_DIRECTORY ?
> > +                                     FILE_FLAG_BACKUP_SEMANTICS : 0,
>
> There is a weird error case here.  If the GetFileAttributesW() call
> at the top fails, it returns INVALID_FILE_ATTRIBUTES (aka -1).  So
> the (attrs & ...) here expression is questionable.
>
> I'm not sure that there is a best way to handle the earlier failure
> (other than returning an error at the top), but we do try to limp
> along (for some reason).
>
> So maybe make this something like:
>
>      (attrs != INVALID_FILE_ATTRIBUTES &&
>       (attrs & FILE_ATTRIBUTE_DIRECTORY)) ?
>          FILE_FLAG_BACKUP_SEMANTICS : 0
>
>

Yikes, I didn't realize how negative numbers looked in binary (in C).

Thanks for this catch!
diff mbox series

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index 03af369b2b9..11c43ad2dfb 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,16 @@  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,
+				   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;
 	}
@@ -987,12 +998,14 @@  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 &&