| 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
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)
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
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()
# 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".