[PATCH] D144172: [Support] [Windows] Don't check file access time in equivalent(file_status, file_status)

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D144172#4131830 <https://reviews.llvm.org/D144172#4131830>, @hans wrote:

>>> From https://learn.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-by_handle_file_information it sounds like just comparing the LastWriteTime and VolumeSerialNumber should be enough to check file identities. Perhaps we should just do that?
>>
>> You mean `FileIndex`, not `LastWriteTime`? I guess that could be reasonable - but that also feels like a more risky change.
>
> Oops, yes I meant `FileIndex`.

When diving into this next step, I noticed two things.

1. We already have `file_status::getUniqueID()` which seems to do exactly what we want:

  UniqueID file_status::getUniqueID() const {
    // The file is uniquely identified by the volume serial number along
    // with the 64-bit file identifier.
    uint64_t FileID = (static_cast<uint64_t>(FileIndexHigh) << 32ULL) |
                      static_cast<uint64_t>(FileIndexLow);
  
    return UniqueID(VolumeSerialNumber, FileID);
  }

So we could simplify `equivalent()` (and other calls within the same file) to just call this neat method. That's nice.

2. The docs contain another rather annoying detail though. The next paragraph in the referenced docs say this:

> The ReFS file system, introduced with Windows Server 2012, includes 128-bit file identifiers. To retrieve the 128-bit file identifier use the GetFileInformationByHandleEx function with FileIdInfo to retrieve the FILE_ID_INFO structure. The 64-bit identifier in this structure is not guaranteed to be unique on ReFS.

So with that in mind, we'd need to extend the call to `getStatus()` to call `GetFileInformationByHandleEx(FileHandle, FileIdInfo, &IdInfo)` to get extended info about the file ID. We would need to extend the `file_status` struct, https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/include/llvm/Support/FileSystem.h#L225-L238, to include this. That's no problem. But we'd also need to extend `fs::UniqueID`, https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/include/llvm/Support/FileSystem/UniqueID.h#L26-L49, with that, and that seems to be much more annoying. We probably can't rely on 128 bit integers being available everywhere LLVM itself runs (since it can run on e.g. 32 bit hosts).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144172/new/

https://reviews.llvm.org/D144172