This is an archive of the discontinued Mercurial Phabricator instance.

Differential D198

undo: prevent nested calls of log
ClosedPublic

Authored by felixmerk on Jul 31 2017, 2:00 PM.
Tags
None
Subscribers
stash

Details

Reviewers
stash
Group Reviewers
Restricted Project
Commits
rFBHGX135217f25bd7: undo: prevent nested calls of log
Summary

Certain extensions (specifically infinite push) call full fledge mercurial
commands. Since all proper mercurial commands might change the repo state,
these are tracked and changes are recorded, which at best causes confusing repo
states for the user and at worst prevents the user from undo-ing due to
precieved gaps in the log. By using an env flag we can avoid this.

This change was tested in a repo with infinite push enabled with unit tests.

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

felixmerk created this revision.Jul 31 2017, 2:00 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 31 2017, 2:00 PM
stash added a subscriber: stash.Aug 1 2017, 9:57 AM

Certain extensions (specifically infinite push) call full fledge mercurial commands.

I know only about hg update which calls hg pull and hg pullbackup which also calls hg pull. Anything else that I'm missing?

hgext3rd/undo.py
61–72

Can you use just one if statement?

if  ...:
        changes = safelog(repo, [""])
        if changes:
            _recordnewgap(repo)
        ...
        rootlog = True
else:
        rootlog = False
67

I don't think you need to use os.environ. It doesn't give any benefits because infinitepush doesn't create a separate process. A global variable is enough.

stash requested changes to this revision.Aug 1 2017, 9:57 AM
This revision now requires changes to proceed.Aug 1 2017, 9:57 AM
stash accepted this revision.Aug 1 2017, 10:25 AM
stash added inline comments.
hgext3rd/undo.py
67

Oh, I think I understand now. This is because of the hook the runs hg pushbackup command.

This revision is now accepted and ready to land.Aug 1 2017, 10:25 AM
felixmerk added inline comments.Aug 1 2017, 1:15 PM
hgext3rd/undo.py
67

Yes, pushbackup was the culprit when I ran into the issue

felixmerk added inline comments.Aug 1 2017, 3:22 PM
hgext3rd/undo.py
61–72

One if statement is doable, but increases likelihood of issues with simultaneous commands run. There are some edge cases (with two mercurial commands) that cause weird behavior in terms of what data is stored. Longer term, some fancy wrapping of the lock command will reduce this.

Closed by commit rFBHGX135217f25bd7: undo: prevent nested calls of log (authored by felixmerk). · Explain WhyAug 3 2017, 5:45 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/undo.py (20 lines)

Diff 519

hgext3rd/undo.py

# undo.py: records data in revlog for future undo functionality # undo.py: records data in revlog for future undo functionality
# #
# Copyright 2017 Facebook, Inc. # Copyright 2017 Facebook, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import os
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
cmdutil, cmdutil,
dispatch, dispatch,
error, error,
extensions, extensions,
hg, hg,
# This wrapper executes whenever a command is run. # This wrapper executes whenever a command is run.
# Some commands (eg hg sl) don't actually modify anything # Some commands (eg hg sl) don't actually modify anything
# ie can't be undone, but the command doesn't know this. # ie can't be undone, but the command doesn't know this.
command = fullargs command = fullargs
# Check wether undolog is consistent # Check wether undolog is consistent
# ie check wether the undo ext was # ie check wether the undo ext was
# off before this command # off before this command
if '_undologactive' not in os.environ:
changes = safelog(repo, [""]) changes = safelog(repo, [""])
if changes: if changes:
_recordnewgap(repo) _recordnewgap(repo)
# prevent nested calls
if '_undologactive' not in os.environ:
stashUnsubmitted
Not Done

I don't think you need to use os.environ. It doesn't give any benefits because infinitepush doesn't create a separate process. A global variable is enough.

stash: I don't think you need to use `os.environ`. It doesn't give any benefits because infinitepush…
stashUnsubmitted
Not Done

Oh, I think I understand now. This is because of the hook the runs hg pushbackup command.

stash: Oh, I think I understand now. This is because of the hook the runs `hg pushbackup` command.
felixmerkAuthorUnsubmitted
Not Done

Yes, pushbackup was the culprit when I ran into the issue

felixmerk: Yes, pushbackup was the culprit when I ran into the issue
os.environ['_undologactive'] = "active"
rootlog = True
else:
rootlog = False
stashUnsubmitted
Not Done

Can you use just one if statement?

if  ...:
        changes = safelog(repo, [""])
        if changes:
            _recordnewgap(repo)
        ...
        rootlog = True
else:
        rootlog = False
stash: Can you use just one if statement? ``` if ...: changes = safelog(repo, [""])…
felixmerkAuthorUnsubmitted
Not Done

One if statement is doable, but increases likelihood of issues with simultaneous commands run. There are some edge cases (with two mercurial commands) that cause weird behavior in terms of what data is stored. Longer term, some fancy wrapping of the lock command will reduce this.

felixmerk: One if statement is doable, but increases likelihood of issues with simultaneous commands run.
result = orig(lui, repo, cmd, fullargs, *args) result = orig(lui, repo, cmd, fullargs, *args)
# record changes to repo # record changes to repo
if rootlog:
safelog(repo, command) safelog(repo, command)
del os.environ['_undologactive']
return result return result
# Write: Log control # Write: Log control
def safelog(repo, command): def safelog(repo, command):
'''boilerplate for log command '''boilerplate for log command
input: input: