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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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