[PATCH 2 of 3] obsolete: append new markers to obsstore file instead of rewriting everything

[Adrian Buehlmann]

On 2012-07-04 02:26, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1341360168 -7200
> # Node ID 194c37e42b720e5ee9c2bba9f6d8d306cb3e1766
> # Parent  f7efc57ef2e61b94028726e5887ce97d159c2524
> obsolete: append new markers to obsstore file instead of rewriting everything
> 
> This is the second step toward incremental writing of marker inside a transaction.
> The obsstore file is now handled append only.
> 
> Header writing have been extracted from _writemarkers.
> 
> Because the _writemarkers method have been dropped, the push code directly reuse
> the serialised content of local repo `listkeys`. This is not very pretty, but this
> part of the protocol still need major improvement anyway.
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1808,9 +1808,8 @@
>                              self.ui.warn(_('updating %s to public failed!\n')
>                                              % newremotehead)
>                  if 'obsolete' in self.listkeys('namespaces') and self.obsstore:
> -                    data = self.obsstore._writemarkers()
> -                    r = remote.pushkey('obsolete', 'dump', '',
> -                                       base85.b85encode(data))
> +                    data = self.listkeys('obsolete')['dump']
> +                    r = remote.pushkey('obsolete', 'dump', '', data)
>                      if not r:
>                          self.ui.warn(_('failed to push obsolete markers!\n'))
>              finally:
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -209,12 +209,14 @@
>          """Write all markers on disk
>  
>          After this operation, "new" markers are considered "known"."""
> +        # XXX: transaction logic should be used
>          if self._new:
> -            # XXX: transaction logic should be used here. But for
> -            # now rewriting the whole file is good enough.
> -            f = self.sopener('obsstore', 'wb', atomictemp=True)
> +            f = self.sopener('obsstore', 'ab')
>              try:
> -                self._writemarkers(f)
> +                if f.tell() == 0:
> +                    # plain new obsstore
> +                    f.write(_pack('>B', _fmversion))
> +                _writemarkers(f.write, self._new)
>                  f.close()
>                  self._new[:] = []
>              except: # re-raises
> @@ -228,32 +230,25 @@
>          for suc in sucs:
>              self.successors.setdefault(suc, set()).add(marker)
>  
> -    def _writemarkers(self, stream=None):
> -        # Kept separate from flushmarkers(), it will be reused for
> -        # markers exchange.
> -        if stream is None:
> -            final = []
> -            w = final.append
> -        else:
> -            w = stream.write
> -        w(_pack('>B', _fmversion))
> -        for marker in self._all:
> -            pre, sucs, flags, metadata = marker
> -            nbsuc = len(sucs)
> -            format = _fmfixed + (_fmnode * nbsuc)
> -            data = [nbsuc, len(metadata), flags, pre]
> -            data.extend(sucs)
> -            w(_pack(format, *data))
> -            w(metadata)
> -        if stream is None:
> -            return ''.join(final)
> +def _writemarkers(write, markers):
> +    # Kept separate from flushmarkers(), it will be reused for
> +    # markers exchange.
> +    for marker in markers:
> +        pre, sucs, flags, metadata = marker
> +        nbsuc = len(sucs)
> +        format = _fmfixed + (_fmnode * nbsuc)
> +        data = [nbsuc, len(metadata), flags, pre]
> +        data.extend(sucs)
> +        write(_pack(format, *data))
> +        write(metadata)
>  
>  def listmarkers(repo):
>      """List markers over pushkey"""
>      if not repo.obsstore:
>          return {}
> -    data = repo.obsstore._writemarkers()
> -    return {'dump': base85.b85encode(data)}
> +    data = [_pack('>B', _fmversion)]
> +    _writemarkers(data.append, repo.obsstore)
> +    return {'dump': base85.b85encode(''.join(data))}
>  
>  def pushmarker(repo, key, old, new):
>      """Push markers over pushkey"""

This was pushed as http://selenic.com/repo/hg/rev/95d785ccb4e5

Bisecting reveals that it is the first changeset that causes test-obsolete.t to
fail on Windows with:

--- C:\Users\adi\hgrepos\hg-main\tests\test-obsolete.t
+++ C:\Users\adi\hgrepos\hg-main\tests\test-obsolete.t.err
@@ -43,8 +43,8 @@
   created new head
   $ hg debugobsolete -d '1337 0' `getid new_c` `getid new_2_c`
   $ hg debugobsolete
-  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
-  cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
+  abort: parsing obsolete marker: metadata is too short, 42 bytes expected, got 16777216
+  [255]

See also

http://hgbuildbot.kublai.com/builders/Windows%202008%20R2%20hg%20tests/builds/148/steps/run-tests.py%20%28python2.6%29/logs/stdio