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

Commit Message

Tao Klerks Feb. 28, 2022, 9:40 a.m. UTC

From: Tao Klerks <[e-mail]>

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 <[e-mail]>
---
 compat/mingw.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

Comments

Jeff Hostetler Feb. 28, 2022, 3:27 p.m. UTC | #1

On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:
> From: Tao Klerks <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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.

> +
>   	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.

> +					   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

Junio C Hamano Feb. 28, 2022, 10 p.m. UTC | #2

"Tao Klerks via GitGitGadget" <[e-mail]> writes:

> From: Tao Klerks <[e-mail]>
>
> 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?

> 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.

OK.

> @@ -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.

>  	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.

> +		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).

> +	} 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.

> @@ -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.

>  		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 &&

Junio C Hamano Feb. 28, 2022, 10:01 p.m. UTC | #3

Jeff Hostetler <[e-mail]> writes:

>> +	HANDLE osfilehandle;
>
> I'd be more comfortable initializing this variable to
> INVALID_HANDLE_VALUE.

Both directories/files branches assign, so it is not needed.

>> +	if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
>> +		fh = 0;
>
> and here initializing fh = -1.

This does make it safer.

> And then this becomes:
>
> 	if (fh != -1)
> 		close(fh)
> 	else if (osfilehandle != INVALID_HANDLE_VALUE)
> 		Closehandle(osfilehandle);

Exactly.

Tao Klerks March 1, 2022, 8:16 a.m. UTC | #4

On Mon, Feb 28, 2022 at 4:27 PM Jeff Hostetler <[e-mail]> wrote:
>
>
>
> On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:

> > +     HANDLE osfilehandle;
>
> I'd be more comfortable initializing this variable to
> INVALID_HANDLE_VALUE.
>

Makes sense, thanks. (but less relevant after switch to CreateFileW)

> > +             fh = 0;
>
> and here initializing fh = -1.

Makes sense, thanks. (but overshadowed by switch to CreateFileW)

> > +             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.

I couldn't find these, and am a complete C APIs n00b - I have no idea
whether or how to add them. I figured commenting on their meaning
is the simplest safest thing to do locally?

> > +     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.

Perfect, thx.

>
> >
> >   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.
>

I was at first scared of making changes to things I don't fully
understand, but looking at the existing use of GetFileAttributesW
I don't think that makes much sense. This is cleaner/simpler
even if it is a bigger change, and consistent with other things.

Tao Klerks March 1, 2022, 8:21 a.m. UTC | #5

On Mon, Feb 28, 2022 at 11:00 PM Junio C Hamano <[e-mail]> wrote:
>
> "Tao Klerks via GitGitGadget" <[e-mail]> writes:
>
> > 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?
>

I've updated the text slightly in the next re-roll, but I didn't understand the
bit about dashes... What is "the comment after three dashes"?

> > +             fh = 0;
>
> This should be
>
>                 fh = -1;
>
> instead.  More on this later.
>

Makes sense, but obviated by full switch to CreateFileW().

> > +     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 ...
>

Makes sense, but obviated by full switch to CreateFileW().

Patch

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;
+
 	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;
+		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);
 	}
 
 	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);
 
 revert_attrs:
 	if (attrs != INVALID_FILE_ATTRIBUTES &&