- User Since
- Jul 1 2017, 5:02 PM (81 w, 1 d)
Wed, Jan 16
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.
Sat, Jan 12
Fri, Jan 11
Thu, Jan 10
For the record, I enjoyed @martinvonz conversing with himself on this review!
Wed, Jan 9
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.
Thu, Dec 27
Dec 20 2018
I'd say `repository.py is the best place for docs, since localrepo.py` implements the interface and the interface is what matters.
Dec 11 2018
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...
Dec 10 2018
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 ¯\_(ツ)_/¯
Nov 29 2018
Nov 14 2018
Nov 13 2018
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.
Nov 9 2018
Nov 6 2018
Nov 5 2018
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.
Oct 31 2018
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.