I don't suppose there's a way to easily test this? I'm guessing not...

Overall I like the feature. But the class initialization logic is wonky. That's not your fault: you happen to be wading into a mess. Since it looks like there will be a v2 on this series, I'd encourage you to add some inline comments about the __init__ mess at the least, or ideally refactor it so the code is cleaner.

Why wouldn't we want automated, non-interactive tools to have the benefits of watchman? There are plenty of use cases where complex Mercurial operations are scripted and would benefit from watchman.

This new config knob defaults to off and we're therefore changing the behavior to not warn when watchman is unavailable. This is a bit concerning to me because someone could have fsmonitor enabled and I don't think they'd ever get an error if watchman isn't working correctly.

In D5565#82224, @lothiraldan wrote:

Why have a flag for revlog v2 at all? Since v2 have not been "released"
yet, why not use sparse-revlog logic in all case when using revlog v2?

For the record, I enjoyed @martinvonz conversing with himself on this review!

In D5519#81903, @durin42 wrote:

In D5519#81899, @indygreg wrote:

The original purpose of the simple store repo was to flush out problems with storage abstraction by building an alternate storage backend. Now that we have storage interface unit tests and the SQLite storage backend, there is definitely less of a need for the simple store backend and we could probably consider deleting it.

I wonder if it's worth keeping the simpler simplestore code as an example. Probably not?

I agree that we should nuke the vendored CBOR package altogether.

The original purpose of the simple store repo was to flush out problems with storage abstraction by building an alternate storage backend. Now that we have storage interface unit tests and the SQLite storage backend, there is definitely less of a need for the simple store backend and we could probably consider deleting it.

I'd say `repository.py is the best place for docs, since localrepo.py` implements the interface and the interface is what matters.

It turns out we were hitting a bug in an ancient version of SQLite. Upgrading from 3.11 to 3.22 fixed it. I don't think this patch is needed.

Hold off on reviewing this. I'm still debugging some issues at Mozilla and want to be sure this patch is correct...

I'll be queuing this shortly.

This one had a merge conflict due to the introduction of a negative integer test. I fixed in flight by adding the --config to the new test.

This behavior is wonky and smells like a race condition in the kernel or something. But all changes are to places that are expected to fail. I don't think the actual error matters that much. And if this is beyond our control, there's nothing we can do. So ¯\_(ツ)_/¯

Where do we stand on the intent to mass reformat the code base?

I think this patch can be abandoned because of other work late in the 4.8 cycle?

Ideally we'd be using the vfs layer for I/O. But that is scope bloat and I'm not going to make you rewrite RFL to do things more correctly.

IIRC we #define PyInt and PyLong in the source for Python 3 compatibility. I remember doing something really hacky to unblock getting the C extensions to compile on Python 3 at the Paris sprint. It would definitely be a good idea to audit everything with PyInt and PyLong in it for sanity.

In D4750#73744, @mharbison72 wrote:

Apparently thg uses this to avoid loading the data into memory just to figure out the length like fctx.rawsize(). Any suggestions?

https://groups.google.com/d/msg/thg-dev/6ekYID7ho_o/fbdd6rVDAwAJ

db5501d93bcf6ada3426a45e901094fd877e370f changed a test. I'm surprised this patch doesn't include that revert as well. I'll verify the test as part of landing and amend as necessary.