Patchwork [1,of,9,STABLE] cmdutil: add the class to restore dirstate at unexpected failure easily

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Oct. 1, 2014, 4:18 p.m.
Message ID <91da9ccf1671d004fd47.1412180308@feefifofum>
Download mbox | patch
Permalink /patch/6066/
State Changes Requested
Headers show

Comments

Katsunori FUJIWARA - Oct. 1, 2014, 4:18 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1412179386 -32400
#      Thu Oct 02 01:03:06 2014 +0900
# Branch stable
# Node ID 91da9ccf1671d004fd4738ea99126acf103294a6
# Parent  a111e460318af49aeb6578cf142a63426c5e764d
cmdutil: add the class to restore dirstate at unexpected failure easily

Before this patch, after "dirstate.write()" execution, there is no way
to restore dirstate to the original status before "dirstate.write()".

In some code paths, "dirstate.invalidate()" is used to restore
dirstate to the original status, but it just discard changes in
memory. "dirstate.write()" prevents it from correctly working.

To fix the issue that recent (in memory) dirstate isn't visible to
external process (e.g. "precommit" hooks), "dirstate.write()" should
be invoked before invocation of external process. On the other hand,
dirstate should be restored to the original status before
"dirstate.write()" at failure according to the exit code of external
process.

Typical usecase is:

  1. build dirstate up
     (e.g. merging two revisions at rebase)
  2. make some additional changes on dirstate for commit
     (e.g. dropping the 2nd parent of merge at rebase)
  3. invoke "repo.commit()"
  4. execute "dirstate.write()" to make recent dirstate visible to
     external process
  5. invoke external process
  6. raise exception according to exit code of external process
  7. restore dirstate to one at step (1)

Keeping ".hg/dirstate" written at step (4) is not reasonable, because
changes at step (2) may decrease usability in some cases: for example
in "hg rebase" case, dropping the 2nd parent of merge prevents users
from:

  - re-merging by "hg resolve"
  - resuming rebase by "hg rebase --continue"
    (rebase requires "len(repo.parents()) == 2" for resuming)

This patch adds the class "dirstateguard" to restore dirstate at
unexpected failure easily.

This patch chooses adding new specific class instead of extending
"transaction" class, which also saves ".hg/dirstate" at the
construction time, because transaction nesting requires alternative
dirstate backup and increases complication.

This patch doesn't invoke "dirstate.write()" at the construction time,
because some client code paths (e.g. "duplicatecopies" invocation in
rebase) prevent it.

For readability, client code paths should be refactored in the future,
to invoke "dirstate.write()" at the construction time of "dirstateguard".
Matt Mackall - Oct. 1, 2014, 6:12 p.m.
On Thu, 2014-10-02 at 01:18 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1412179386 -32400
> #      Thu Oct 02 01:03:06 2014 +0900
> # Branch stable
> # Node ID 91da9ccf1671d004fd4738ea99126acf103294a6
> # Parent  a111e460318af49aeb6578cf142a63426c5e764d
> cmdutil: add the class to restore dirstate at unexpected failure easily

This is interesting, but way too complicated for stable. And way way way
too complicated for the last day of the release cycle.

At the sprint, we talked about build a more complex multi-file
transaction system to address the larger issues here. There are some
notes about that here:

https://titanpad.com/mercurial32-sprint (starting at line 123)
Katsunori FUJIWARA - Oct. 6, 2014, 4:30 p.m.
At Wed, 01 Oct 2014 13:12:14 -0500,
Matt Mackall wrote:
> 
> On Thu, 2014-10-02 at 01:18 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1412179386 -32400
> > #      Thu Oct 02 01:03:06 2014 +0900
> > # Branch stable
> > # Node ID 91da9ccf1671d004fd4738ea99126acf103294a6
> > # Parent  a111e460318af49aeb6578cf142a63426c5e764d
> > cmdutil: add the class to restore dirstate at unexpected failure easily
> 
> This is interesting, but way too complicated for stable. And way way way
> too complicated for the last day of the release cycle.

I was also worry about complication of the series and close-ness to
the day of the release cycle, but I posted this series as "STABLE"
because of "urgent" priority of this issue.

BTW, is this approach not good, even for "default" ?


> At the sprint, we talked about build a more complex multi-file
> transaction system to address the larger issues here. There are some
> notes about that here:
> 
> https://titanpad.com/mercurial32-sprint (starting at line 123)

This is interesting !

But IMHO, "extending transaction" approach seems not to be suitable to
resolve this problem, because:

  - transaction nesting occurs in some cases (e.g. "shelve => rebase"), and

  - "dirstate" may be already modified since the beginning of outer
    transaction, then

  - dirstate should be backed up into the file other than
    "dirstate.journal" at the beginning of inner transaction, but

  - such "dirstate" backup files are useless for transaction, and
    increases complication of its implementation


"transaction" and "dirstateguard" differ from each other in "what it
should do for dirstate" in cases other than "success".

                | success | fail    | "hg rollback"
  --------------+---------+---------+---------------
  transaction   | keep    | keep    | restore
  dirstateguard | keep    | restore | (not involved)


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Matt Mackall - Oct. 17, 2014, 9:18 p.m.
On Tue, 2014-10-07 at 01:30 +0900, FUJIWARA Katsunori wrote:
> At Wed, 01 Oct 2014 13:12:14 -0500,
> Matt Mackall wrote:
> > 
> > On Thu, 2014-10-02 at 01:18 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1412179386 -32400
> > > #      Thu Oct 02 01:03:06 2014 +0900
> > > # Branch stable
> > > # Node ID 91da9ccf1671d004fd4738ea99126acf103294a6
> > > # Parent  a111e460318af49aeb6578cf142a63426c5e764d
> > > cmdutil: add the class to restore dirstate at unexpected failure easily
> > 
> > This is interesting, but way too complicated for stable. And way way way
> > too complicated for the last day of the release cycle.
> 
> I was also worry about complication of the series and close-ness to
> the day of the release cycle, but I posted this series as "STABLE"
> because of "urgent" priority of this issue.
> 
> BTW, is this approach not good, even for "default" ?

Can you resend this after the freeze for discussion?

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2630,3 +2630,55 @@ 
     for f, clearable, allowcommit, msg, hint in unfinishedstates:
         if clearable and repo.vfs.exists(f):
             util.unlink(repo.join(f))
+
+class dirstateguard(object):
+    '''Restore dirstate at unexpected failure.
+
+    This saves ``.hg/dirstate`` into the backup file at the
+    construction time, and restores it if ``release`` is invoked
+    before ``close``.
+
+    This just removes the backup file at ``close`` before ``release``.
+    '''
+
+    def __init__(self, repo, name):
+        self.repo = repo
+        self.name = 'dirstate.backup.%s' % name
+        repo.vfs.write(self.name, repo.vfs.tryread('dirstate'))
+        self.active = True
+        self.closed = False
+
+    def __del__(self):
+        if self.active: # still active
+            # this may occur, even if this class is used correctly:
+            # for example, releasing other resources like transaction
+            # may raise exception before ``dirstateguard.release`` in
+            # ``release(tr, ....)``.
+            self._abort()
+
+    def close(self):
+        if not self.active: # already inactivated
+            msg = (_("can't close already inactivated backup: %s")
+                   % self.name)
+            raise util.Abort(msg)
+
+        self.repo.vfs.unlink(self.name)
+        self.active = False
+        self.closed = True
+
+    def _abort(self):
+        # this "invalidate" prevents "wlock.release" from writing
+        # changes of dirstate out after restoring to original status
+        self.repo.dirstate.invalidate()
+
+        self.repo.vfs.write('dirstate', self.repo.vfs.read(self.name))
+        self.repo.vfs.unlink(self.name)
+        self.active = False
+
+    def release(self):
+        if not self.closed:
+            if not self.active: # already inactivated
+                msg = (_("can't release already inactivated backup: %s")
+                       % self.name)
+                raise util.Abort(msg)
+            self._abort()