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

[Junio C Hamano <gitster@pobox.com>]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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