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

From: Tao Klerks <tao@klerks.biz>
Date: Mar 1, 2022, 21:14 UTC
Message-ID: <CAPMMpojoe8DgUteRAfuXCHQNFH6vnwj-Twze5i3A62JsvTUWVg@mail.gmail.com>
In-Reply-To: <37f66fae-911a-62f1-4711-c9f6c7794911@jeffhostetler.com>

On Tue, Mar 1, 2022 at 5:34 PM Jeff Hostetler <git@jeffhostetler.com> wrote:

Show 13 quoted lines

>
> On 3/1/22 4:45 AM, Tao Klerks via GitGitGadget wrote:
> >
> > -     if ((fh = _wopen(wfilename, O_RDWR | O_BINARY)) < 0) {
> > +     osfilehandle = CreateFileW(wfilename,
> > +                                0x100 /*FILE_WRITE_ATTRIBUTES*/,
>
> https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants
>
> indicates that FILE_WRITE_ATTRIBUTES is defined in <WinNT.h> which
> we get from <windows.h> which was included by "win32.h", so it should
> already be present.
>

Grr, should have asked the compiler instead of VSCode's autocomplete.

Thx, fixed.

Show 21 quoted lines

> > +                                0 /*FileShare.None*/,
> > +                                NULL,
> > +                                OPEN_EXISTING,
> > +                                attrs & FILE_ATTRIBUTE_DIRECTORY ?
> > +                                     FILE_FLAG_BACKUP_SEMANTICS : 0,
>
> There is a weird error case here.  If the GetFileAttributesW() call
> at the top fails, it returns INVALID_FILE_ATTRIBUTES (aka -1).  So
> the (attrs & ...) here expression is questionable.
>
> I'm not sure that there is a best way to handle the earlier failure
> (other than returning an error at the top), but we do try to limp
> along (for some reason).
>
> So maybe make this something like:
>
>      (attrs != INVALID_FILE_ATTRIBUTES &&
>       (attrs & FILE_ATTRIBUTE_DIRECTORY)) ?
>          FILE_FLAG_BACKUP_SEMANTICS : 0
>
>

Yikes, I didn't realize how negative numbers looked in binary (in C).

Thanks for this catch!