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