This is wrong, because now remotefilelog will do ensurestart=True (the default) which is a significant performance hit for remotefilelog users.

Wait, I just saw the chain. Thanks for weird emails phabricator!

This doesn't apply to stable. I also checked default, and it doesn't apply there either.

It said revision before, so at a minimum we should add an s...

In D7295#110254, @mharbison72 wrote:

In D7295#107482, @indygreg wrote:

Since type checking is slow (but there are state files we can reuse to speed things up), we'll need to figure out how to make this work in CI. But I have no doubt we can figure something out. Out of curiosity, how long does pytype take to run in a clean source directory, without any state files?

It takes 15-20 minutes on a 2018 Mac mini. In addition to the disabled files here, I've disabled:

mercurial/httppeer.py
mercurial/repoview.py
mercurial/localrepo.py
mercurial/revlog.py
mercurial/merge.py
mercurial/testing/storage.py

But I've also added back these, and sprinkled various disable comments around:

mercurial/bundlerepo.py
mercurial/error.py
mercurial/exchange.py
mercurial/policy.py
mercurial/pycompat.py
mercurial/scmwindows.py
mercurial/windows.py
mercurial/wireprotoframing.py

Even with the state files, it seems to take 10-15 minutes or so.

In D7296#109672, @dlax wrote:

Sorry, still not ok afaict :/

In D7252#109656, @durin42 wrote:

In D7252#109627, @Alphare wrote:

Sorry to necropost, but since this broke the Rust implementation, I was wondering what the best approach would be to replicate this behavior, and I am starting to think that this should be reverted.

Oh, you mean the Rust version doesn't do the same rejection?
Given that you're about to do a hash lookup, I'm a little skeptical that a endswith('/') check would show up meaningfully in a profiler, but I'm willing to be proven wrong?

IIUC, currently any new path passes through the pathauditor first for validation, so any further check would be completely redundant.

Plausible, but I'd like some sort of test coverage demonstrating that.

My intuition is that adding this check is purely here to satisfy the fuzzer, but would never happen in real life. Adding checks to this (very) internal data structure comes at a cost, both in performance and in code ergonomics.

Yes, this was largely added to make the fuzzer not get stuck on OOM conditions. That said, if the pathauditor can't catch this, we need to defend against this DoS vector at this layer, and it's such a small check at this layer I'm inclined to keep it unless it is measurably slowing down real uses...

In D7252#109627, @Alphare wrote:

Sorry to necropost, but since this broke the Rust implementation, I was wondering what the best approach would be to replicate this behavior, and I am starting to think that this should be reverted.

I still want to keep the annotations I added. :)