Replace PathMap.normalisePath with the one from alhadis.utils

[hansonw]

PathMap currently has a different normalisePath implementation from
the one that the Resource class uses, leading to a mismatch between
PathMap keys and resource paths. This can lead to bugs where a
resource is marked as disposed but not removed from the PathMap;
attempting to fetch it again leads to exceptions from the null icon.

[Alhadis]

👋 Hey @hansonw, sorry for the delayed response (again).

The situation you're describing with the errors is somewhat puzzling, as the paths should already be normalised by the PathMap instance (specifically, here). Are you able to provide a specific case of this error?

[hansonw]

Oh! Thanks for the clarification there - didn't realize that :D

This particular problem has been reported by users of https://github.com/facebook/nuclide as we use non-standard URLs (e.g. nuclide://hostname/home/hansonw/test.txt) for remote files. I think I've narrowed down the problem to the fact that PathMap normalizes paths differently from
how Resource normalizes them internally (PathMap.normalisePath is different from the one in alhadis.utils. Would it be safe to consistently use the one from alhadis.utils instead? or is the PathMap version preferable?

[hansonw]

Updated the PR with an alternate fix.. let me know if that would work.
The unit tests in file-icons still pass and I've verified that the previous scenario doesn't break anymore. The exact scenario is:

1. Open a remote file, e.g. nuclide://hostname/home/hansonw/test.txt.
2. Delete it. We call this.paths.delete(resource.path) but due to the different normalisation implementation the entry isn't actually deleted.
3. Open the file again (e.g. with cmd-shift-T). We obtain the destroyed resource, which has a null icon, leading to an exception which blocks the file from opening.

[Alhadis]

as we use URLs (e.g. nuclide://hostname/home/hansonw/test.txt)

Ohhhh... damn, that explains everything. See, I didn't factor in URIs when I originally wrote the normalisePath function, because its chief purpose was to alleviate the headaches of dealing with Windows path separators. The PathMap class was also authored for the same purpose.

The reason for the breakage is, of course, that the leading protocol component is interpreted as a local directory. The fact that it's non-standard (nuclide://) has zero bearing on the input's interpretation: you'd get the same breakages and inconsistencies if if were, say, file://.

I'll be back in front of a laptop in about an hour or so, but it lacks a GUI (OpenBSD 6.2 and Intel hardware aren't getting along...), so I'll push a fix to your branch and get you to verify that it works for me. Have you made your branch editable by maintainers?

[hansonw]

Thank you, yes it is editable! Note that I think it's OK that the URIs are parsed correctly; as long as they're both normalized incorrectly *but in the same way* everything seems to work fine (all the code to read/check files wouldn't work with remote URIs anyway :p)

…

On Mon, Dec 18, 2017 at 12:09 AM John Gardner ***@***.***> wrote: as we use URLs (e.g. nuclide://hostname/home/hansonw/test.txt) Ohhhh... damn, that explains everything. See, I didn't factor in URIs when I originally wrote the normalisePath function, because its chief purpose was to alleviate the headaches of dealing with Windows path separators. The PathMap class was also authored for the same purpose. The reason for the breakage is, of course, that the leading protocol component is interpreted as a local directory. The fact that it's non-standard (nuclide://) has zero bearing on the input's interpretation: you'd get the same breakages and inconsistencies if if were, say, file://. I'll be back in front of a laptop in about an hour or so, but it lacks a GUI (OpenBSD 6.2 and Intel hardware aren't getting along...), so I'll push a fix to your branch and get you to verify that it works for me. Have you made your branch editable by maintainers? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjPYoTB_kM6A69TM-xpB_Xlh0nKKpRgks5tBh3HgaJpZM4REcIp> .

[Alhadis]

Honestly, I don't think well-formed URIs should even be normalised like ordinary paths. They're the exception rather than the norm, and it should be assumed that the author knows what they're doing.

I realised as I was writing this that URIs should be exempt from the case insensitivity check as well (because a remote resource might be hosted on a case-sensitive filesystem, while the local machine is running macOS or whatever).

I've pushed to your branch, give it a whirl. :D

[Alhadis]

Off-topic: My current laptop predicament in one picture:

Pictured left: MacBook with smashed screen
Pictured right: Bulky old Dell stuck with 80x25 terminal because of issues with inteldrm(4). 😢

Every time I've used Atom lately, it's been from a borrowed computer...

[hansonw]

Hahah! No spare monitors lying around? :(

Agreed that URIs should not be normalized at all! However the amended commit doesn't fix things, as Resource still uses normalisePath from alhadis.utils so there's still the mismatch between the PathMap keys and Resource.path.

It seems like the right fix would be to both use a consistent normalisation function everywhere in addition to fixing the function alhadis.utils to not normalise URLs.

[Alhadis]

(Nope, no spare screens lying around. 😢 I'm wayyyy too poor for that).

I've pushed an analogous modification to the alhadis.utils repo, which I'll hold off publishing until you've given me the green light that we're finally in the clear.

To test the change locally, you'll need to cd to wherever you cloned and linked alhadis.utils, then run make. Assuming you're using macOS or Linux, you shouldn't face any issues (BSD make complained to me before about an unrecognised option, which I guess is specifically a GNU thing).

[hansonw]

That seems to work! (I think there's a missing close paren ) at the end of the line there, though).
I'm still curious as to why we shouldn't unify the two normalisation functions - the URL checking logic seems like it should be in one place right?

[Alhadis]

(I think there's a missing close paren ) at the end of the line there, though).

Holy shit, great catch. 😥 Emacs doesn't balance bracket-pairs like Atom does, and this makes the third (or fourth) time I forgot to type a closing brace because I unconsciously expected it to be inserted as I typed. Thanks, muscle memory. 😀

I've force-pushed an amended commit. I should node -c lib/* or something as a smoke test to ensure shit like this doesn't get published by accident, because it probably would have had you not pointed it out. 😭

I'm still curious as to why we shouldn't unify the two normalisation functions - the URL checking logic seems like it should be in one place right?

Yes, that's true. Keep in mind the standalone function was written well-before PathMap was authored, and the latter performs some platform-specific normalisation which the original function doesn't. So the two functions aren't exactly identical in how they transform their input.

The latter was also churned out in a hurry, and I didn't want to introduce any last-minute complications by altering the logic of existing code. Do you foresee this being an issue...?

[hansonw]

So the two functions aren't exactly identical in how they transform their input.

Not having identical output is the cause of this issue in the first place ;) Otherwise we never would have noticed, despite the incorrect URL handling and all!

But it would certainly solve the problem for us if you pushed the change as-is. Thank you!

[Alhadis]

Done and done! Thanks for your patience!