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

[Tao Klerks <tao@klerks.biz>]

On Mon, Feb 28, 2022 at 11:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> 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().