[PATCH] transaction: reorder unlinking .hg/journal and .hg/journal.backupfiles

[FUJIWARA Katsunori]

At Sat, 17 Oct 2015 19:07:26 +0900,
Yuya Nishihara wrote:
> 
> On Fri, 16 Oct 2015 03:52:40 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> > # Date 1444933791 -32400
> > #      Fri Oct 16 03:29:51 2015 +0900
> > # Node ID 6fa58be5278941c147384bff82cda8de96cc2cd5
> > # Parent  07db7e95c464537aeb2dd7aba39de0813eaffd04
> > transaction: reorder unlinking .hg/journal and .hg/journal.backupfiles
> > 
> > After this reordering, absence of '.hg/journal' just before starting
> > new transaction means also absence of '.hg/journal.backupfiles'.
> > 
> > In this case, all temporary files for preceding transaction should be
> > completely unlinked, and HG_PENDING doesn't cause unintentional
> > reading stalled temporary files in.
> > 
> > Otherwise, 'repo.transaction()' raises exception with "run 'hg
> > recover' to clean up transaction" hint.
> > 
> > diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> > --- a/mercurial/transaction.py
> > +++ b/mercurial/transaction.py
> > @@ -75,10 +75,10 @@
> >              if not c:
> >                  raise
> >  
> > -    opener.unlink(journal)
> >      backuppath = "%s.backupfiles" % journal
> >      if opener.exists(backuppath):
> >          opener.unlink(backuppath)
> > +    opener.unlink(journal)
> >      try:
> >          for f in backupfiles:
> >              if opener.exists(f):
> > @@ -427,10 +427,11 @@
> >          self._writeundo()
> >          if self.after:
> >              self.after()
> > +        if self.opener.isfile(self._backupjournal):
> > +            self.opener.unlink(self._backupjournal)
> >          if self.opener.isfile(self.journal):
> >              self.opener.unlink(self.journal)
> > -        if self.opener.isfile(self._backupjournal):
> > -            self.opener.unlink(self._backupjournal)
> > +        if True:
> >              for l, _f, b, c in self._backupentries:
> >                  if l not in self._vfsmap and c:
> >                      self.report("couldn't remote %s: unknown cache location"
> 
> I'm afraid that this "if True" does more than the reordering. But I know little
> about the transaction, maybe I'm wrong.
> 

'self._backupjournal' (as '.hg/journal.backupfiles') is:

  - opened in write mode in 'transaction.__init__()'

    putting an entry into 'self._backupentries' should occur after
    transaction construction

    BTW, "version number of journal file" is written into
    'self._backupjournal' immediately, and this ensures that it isn't
    empty, too.

  - closed and unlinked only in 'transaction.close()' (= above) or
    'transaction._abort()', before clearing 'self._backupentries'

Therefore, existence of 'self._backupjournal' itself isn't related to
whether 'self._backupentries' has any entry or not, if transaction
object is constructed successfully.


> OT: "couldn't remote" is a typo of "couldn't remove" ?

Because multiple revisions added same messages into 'transaction.py',
I've thought that "remote something" may also mean "make something go
away" or so.

Although, "couldn't remove" is more clear for me, too :-)


> > @@ -497,10 +498,10 @@
> >  
> >          try:
> >              if not self.entries and not self._backupentries:
> > +                if self._backupjournal:
> > +                    self.opener.unlink(self._backupjournal)
> >                  if self.journal:
> >                      self.opener.unlink(self.journal)
> > -                if self._backupjournal:
> > -                    self.opener.unlink(self._backupjournal)
> >                  return
> >  
> >              self.report(_("transaction abort!\n"))
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at selenic.com
> > https://selenic.com/mailman/listinfo/mercurial-devel
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp