Re: [PATCH 1/2] t/helper/test-chmtime: update mingw to support chmtime on directories
- From
Tao Klerks <tao@klerks.biz>
- Date
- Mar 1, 2022, 08:16 UTC
- Message-ID
- <CAPMMpoiQw0UhnH0WP_opB0iQn8cRx8qT_Cy_W8PnHpxbwATw2A@mail.gmail.com>
- In-Reply-To
- <b5fb21f5-4aa6-fa36-a8f6-abf82b543c1d@jeffhostetler.com>
On Mon, Feb 28, 2022 at 4:27 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
> > > > On 2/28/22 4:40 AM, Tao Klerks via GitGitGadget wrote:
Show 5 quoted lines
> > + 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)
Show 7 quoted lines
> > + 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?
Show 13 quoted lines
> > + 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.
Show 10 quoted lines
> > > > > 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.