Implements lighttransaction(repo) which improves performance of hg undo and
prevents infinite loops caused by hooks that run true hg commands. Also adds
_getrevlog and _invertindex commands, factoring out code that will be reused.
Changes output for gaps when typing hg debugundohistory -l for slightly
better ui and clearer testing. Lastly, changes lock from repo lock to new
undolog lock and tests this. In the future, with some changes to undo.log,
we may be able to take the lock later, bypassing the need to take it for
read-only operations.
Details
- Reviewers
quark - Group Reviewers
Restricted Project - Commits
- rFBHGX63d071a85d9d: undo: improved performance and prep for hg undo
Diff Detail
- Repository
- rFBHGX Facebook Mercurial Extensions
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Using transaction.transaction is great! Requesting changes mainly because we shouldn't affect repo.currenttransaction().
hgext3rd/undo.py | ||
---|---|---|
35–39 | The old code is fine. Usually we avoid moving code unnecessarily. | |
51 | Might worth commenting the parameter type in safelog's docstring if it has to be a list. | |
71–84 | This could be simplified to one line later. | |
83 | This makes repo.currenttransaction() work but is not logically right. The transaction object constructed here is only for writing a couple of revlogs we care about and nothing else. Other users (currently none, but it's better to be a good citizen) using repo.transaction() will accidentally use our transaction object which does not work with hg recover when bad things happen. I think we can remove this line, and do a s/repo, /repo, tr, / substitution for log, _log* and writelog. Then pass the tr object to them. | |
86–111 | Worth a docstring saying when it returns True and when False. | |
237–238 | This didn't get mentioned in commit message. Might update commit message or make it a separate change. | |
264 | since you did a small cleanup here, might also add a space before #. | |
tests/test-undo.t | ||
153 | Is there a reason to use head -1? Usually we avoid that kind of filtering. If only certain fields needs to be tested, use -T to specify the template, like: hg log -r 'draft()' -T '{rev}:{node} {desc}\n' Otherwise the old code is fine, or use >/dev/null to discard the output. |
hgext3rd/undo.py | ||
---|---|---|
76–79 | Can you please explain how's lighttransaction different? It doesn't run hooks? Is it the only difference? |
hgext3rd/undo.py | ||
---|---|---|
76–79 | It avoids running anything that repo.transaction() runs except for transaction.transaction(). This is mostly hooks, but also includes some checks on locking, abandoned transactions and tag tracking. Calling lighttransaction, we should know that the repo is locked. Since lighttransaction is only used for recording information and doesn't actually perform any changes on the repo, ignoring tags and abandoned transactions should be fine. |
hgext3rd/undo.py | ||
---|---|---|
71–72 | Does this mean we take the lock for every command anyone ever runs? That will cause read operations to hang if something else is doing a pull/commit/etc. I don't think we can take the lock for every command. | |
98 | Since this doesn't check for existing repo transactions, we could end up with two transactions running at once. This seems pretty dangerous. Could we assert that there is no current transaction, and block any repo transaction from attempting to open? |
I think this is good enough for now. Ideally we can avoid taking locks or transactions if we don't actually write things. But that is a non-trivial change that can always be done later.
Commands to land this is a bit different for now:
hg update @ # makes sure "Differential Revision: " line appears in commit message hg import `hg phabread D50` # mark the imported commit as a successor of the old commit without "Differential Revision" hg prune --succ . 'tag(D50)' # push it hg push -r . --to @ # cleanups hg up tip # should have been done by pushrebase, but not yet hg tag --local --remove D50
I'll write a script to make that easier.
hgext3rd/undo.py | ||
---|---|---|
74–76 | Seems outdated? Without timeout we will be able to get the lock eventually. | |
78 | Usually desc is a single word without space, like undo. |
The old code is fine. Usually we avoid moving code unnecessarily.