[PATCH 3 of 3] localrepo: use dirstate backup instead of handling dirstate file manually
FUJIWARA Katsunori
foozy at lares.dti.ne.jp
Wed May 11 06:52:47 EDT 2016
At Mon, 9 May 2016 21:32:52 -0700,
Durham Goode wrote:
>
> On 5/5/16 5:35 PM, Mateusz Kwapich wrote:
>
> > # HG changeset patch
> > # User Mateusz Kwapich <mitrandir at fb.com>
> > # Date 1462494839 25200
> > # Thu May 05 17:33:59 2016 -0700
> > # Node ID fce6ff5bc4dbf5529a271c1c8dc4b0e461e26251
> > # Parent 960056f578173d3da54cd0a3c1e05e06b39e64fe
> > localrepo: use dirstate backup instead of handling dirstate file manually
> >
> > This is one step towards having dirstate manage its own storage. It will
> > be useful for the implementation of sql dirstate [1].
> >
> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_SQLDirstatePlan&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=u7dnKhjl-J5CZT0KdlAGTPJmFvfgoYHAJWa0osnLlUM&s=aj3WvVtD8PW-GmdTjKx5W-usAjd0TePjfcnWJhaj8Bg&e=
> >
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -1013,9 +1013,6 @@ class localrepository(object):
> > _("abandoned transaction found"),
> > hint=_("run 'hg recover' to clean up transaction"))
> >
> > - # make journal.dirstate contain in-memory changes at this point
> > - self.dirstate.write(None)
> Why delete this line? The rest of the patch LGTM, but this line seems
> like a behavior change?
This dirstate.write() is expected to write in-memory changes out
before vfs.write("journal.dirstate", ...) in _writejournal(), because
this vfs.write() copies dirstate data directly from ".hg/dirstate"
file.
But dirstate.savebackup() invocation in the hunk of this patch below
implies internal dirstate.write() before creating "journal.dirstate",
and this dirstate.write(None) becomes meaningless.
> > @@ -1104,8 +1101,7 @@ class localrepository(object):
> > return [(vfs, undoname(x)) for vfs, x in self._journalfiles()]
> >
> > def _writejournal(self, desc):
> > - self.vfs.write("journal.dirstate",
> > - self.vfs.tryread("dirstate"))
> > + self.dirstate.savebackup(None, prefix='journal.')
> > self.vfs.write("journal.branch",
> > encoding.fromlocal(self.dirstate.branch()))
> > self.vfs.write("journal.desc",
> > -
> > idbase = "%.40f#%f" % (random.random(), time.time())
> > txnid = 'TXN:' + util.sha1(idbase).hexdigest()
> > self.hook('pretxnopen', throw=True, txnname=desc, txnid=txnid)
> > @@ -1049,7 +1046,7 @@ class localrepository(object):
> There's a dirstate.invalidate right above this line (just outside the
> context). Is it no longer necessary since the dirstate.restorebackup()
> below calls self.invalidate()
As Durham said, replacement of dirstate.write() with restorebackup()
makes corresponded dirstate.invalidate() meaningless.
Therefore, this patch should include 2 removal of
dirstate.invalidate(), because this patch includes 2 replacement of
dirstate.write() with restorebackup() below:
- at failure of transaction (restoring from 'journal.')
- at rollback of transaction (restoring from 'undo.')
IMHO, how about splitting (and adding more explanation) this patch
into one for replacement with savebackup() and another for replacement
with restorebackup() ?
> > # discard all changes (including ones already written
> > # out) in this transaction
> > - repo.vfs.rename('journal.dirstate', 'dirstate')
> > + repo.dirstate.restorebackup(None, prefix='journal.')
> >
> > repo.invalidate(clearfilecache=True)
> >
> > @@ -1104,8 +1101,7 @@ class localrepository(object):
> > return [(vfs, undoname(x)) for vfs, x in self._journalfiles()]
> >
> > def _writejournal(self, desc):
> > - self.vfs.write("journal.dirstate",
> > - self.vfs.tryread("dirstate"))
> > + self.dirstate.savebackup(None, prefix='journal.')
> > self.vfs.write("journal.branch",
> > encoding.fromlocal(self.dirstate.branch()))
> > self.vfs.write("journal.desc",
> > @@ -1191,7 +1187,7 @@ class localrepository(object):
> > # prevent dirstateguard from overwriting already restored one
> > dsguard.close()
> >
> > - self.vfs.rename('undo.dirstate', 'dirstate')
> > + self.dirstate.restorebackup(None, prefix='undo.')
> > try:
> > branch = self.vfs.read('undo.branch')
> > self.dirstate.setbranch(encoding.tolocal(branch))
> > @@ -1200,7 +1196,6 @@ class localrepository(object):
> > 'current branch is still \'%s\'\n')
> > % self.dirstate.branch())
> >
> > - self.dirstate.invalidate()
> > parents = tuple([p.rev() for p in self[None].parents()])
> > if len(parents) > 1:
> > ui.status(_('working directory now based on '
> > diff --git a/tests/test-inherit-mode.t b/tests/test-inherit-mode.t
> > --- a/tests/test-inherit-mode.t
> > +++ b/tests/test-inherit-mode.t
> > @@ -117,6 +117,7 @@ group can still write everything
> > 00660 ../push/.hg/cache/branch2-base
> > 00660 ../push/.hg/cache/rbc-names-v1
> > 00660 ../push/.hg/cache/rbc-revs-v1
> > + 00660 ../push/.hg/dirstate
> Why did this change?
> > 00660 ../push/.hg/requires
> > 00770 ../push/.hg/store/
> > 00660 ../push/.hg/store/00changelog.i
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=u7dnKhjl-J5CZT0KdlAGTPJmFvfgoYHAJWa0osnLlUM&s=Qc7-VkliMw7k1u0sdkDqUMYXhoqjSC55UWv_RUY8W3g&e=
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
----------------------------------------------------------------------
[FUJIWARA Katsunori] foozy at lares.dti.ne.jp
More information about the Mercurial-devel
mailing list