Re: Bug: Changing folder case with `git mv` crashes on case-insensitive file system

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 4 May 2021, brian m. carlson wrote:

> On 2021-05-04 at 03:46:12, Junio C Hamano wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> > > Yeah, this is because your operating system returns EINVAL in this case.
> > > POSIX specifies EINVAL when you're trying to make a directory a
> > > subdirectory of itself.  Which, I mean, I guess is a valid
> > > interpretation here, but it of course makes renaming the path needlessly
> > > difficult.
> > > ...
> > > I suspect part of the problem here is two fold: on macOS we can't
> > > distinguish an attempt to rename the path due to it folding or
> > > canonicalizing to the same thing from a real attempt to move an actual
> > > directory into itself.  The latter would be a problem we'd want to
> > > report, and the former is not.  Unfortunately, detecting this is
> > > difficult because that means we'd have to implement the macOS
> > > canonicalization algorithm in Git and we don't want to do that.
> >
> > I agree we'd probably need to resort to macOS specific hack (like we
> > have NFS or Coda specific hacks), but it may not be too bad.
> >
> > After seeing EINVAL, we can lstat src 'foo' and dst 'FOO', and
> > realize that both are directories and have the same st_dev/st_ino,
> > which should be fairly straightforward, no?
> >
> > For that, we do not exactly have to depend on any part of macOS-ism;
> > we do depend on the traditional "within the same device, inum is a
> > good way to tell if two filesystem entities are the same".
>
> Yes, although that won't work on Windows, which I don't believe has the
> concept of inodes and almost certainly has the same problem.  CCing
> Dscho in case he has some ideas on how we can make this more resilient
> there.

Windows does not really have inodes. But it has what it calls "file IDs".
This concept is pretty much what you expect on inodes, except on that
still-used file system called FAT (for full details, see
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-by_handle_file_information):

	In the FAT file system, the file ID is generated from the first
	cluster of the containing directory and the byte offset within the
	directory of the entry for the file. Some defragmentation products
	change this byte offset. (Windows in-box defragmentation does
	not.) Thus, a FAT file ID can change over time. Renaming a file in
	the FAT file system can also change the file ID, but only if the
	new file name is longer than the old one.

In this instance, because we're not actually renaming (yet), the file ID
would probably be okay. But in general, we should assume that we do not
have inodes on Windows.

There is another complication: it is not actually cheap to get to that
file ID. For performance reasons, we introduced that (optional) FSCache
feature in Git for Windows where cache the `lstat()` results because the
assumption that `lstat()` is fast really only holds true on Linux.

In fact, what we do is to use `FindFirstFile()`/`FindNextFile()` to
enumerate an entire directory's worth of `lstat()` data, because funnily
enough, it is a lot faster when we need an entire directory's worth of
`lstat()` data anyway (calling `GetFileAttributes()` for individual files
is of course faster, but not for a dozen files or so).

But even `GetFileAttributes()` won't get you that "file ID". You have to
create a file handle (via `CreateFile()`, which is *expensive*) and then
call `GetFileInformationByHandle()`.

Now, way too much of Git's source code still pretends that `lstat()` is
just this fast operation and we can do it left and right and not say what
we _actually_ want to know. That function is called when we need only
parts of the `lstat()` data. Sometimes it is even used to determine
whether a file or directory is present. But since Git does not have proper
abstraction, `mingw_lstat()` _still_ has to fill in all the information.

So _if_ we need that file ID information, I would be very much in favor of
introducing a proper abstraction, where differentiate between the
intention (think `get_inode(const char *path)`) from the
platform-dependent implementation detail (think `lstat()`, `CreateFile()`
and `GetFileInformationByHandle()`).

Ciao,
Dscho

> In any event, I'm not planning on writing a patch for this since I have
> no way to test it, but I'm sure someone who uses macOS could probably
> write one reasonably easily.
> --
> brian m. carlson (he/him or they/them)
> Houston, Texas, US
>