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

[Adrian Buehlmann]

On 2012-07-15 14:38, Pierre-Yves David wrote:
> On Sun, Jul 15, 2012 at 01:40:58PM +0200, Adrian Buehlmann wrote:
>> On 2012-07-04 02:26, Pierre-Yves David wrote:
>>> 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
>>
>> 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
>>
> 
> Looking at the error message, it seems that the version headers where not
> written. Is there any specific behavion windows regarding open(xxx, 'ab') or
> f.tell() ?

Yeah. Windows fun again.

It turns out that file objects on Windows [1] seem to behave strangely,
compared to Linux: after f = open(..., 'ab'), f.tell() returns 0L on
Windows for nonzero (!) files, whereas on Linux, it returns the size of
the file.

[1] as seen here with Python 2.7.3 (default, Apr 10 2012, 23:24:47) [MSC
v.1500 64 bit (AMD64)] on win32)

This hack lets test-obsolete.t pass for me on Windows:

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -47,15 +47,15 @@
 - N*20 bytes: new changesets identifiers.

 - M bytes: metadata as a sequence of nul-terminated strings. Each
   string contains a key and a value, separated by a color ':', without
   additional encoding. Keys cannot contain '\0' or ':' and values
   cannot contain '\0'.
 """
-import struct
+import struct, os
 from mercurial import util, base85
 from i18n import _

 _pack = struct.pack
 _unpack = struct.unpack


@@ -190,14 +190,15 @@
         marker = (str(prec), tuple(succs), int(flag), encodemeta(metadata))
         self.add(transaction, marker)

     def add(self, transaction, marker):
         """Add a new marker to the store"""
         if marker not in self._all:
             f = self.sopener('obsstore', 'ab')
+            f.seek(0, os.SEEK_END)
             try:
                 offset = f.tell()
                 transaction.add('obsstore', offset)
                 if offset == 0:
                     # new file add version header
                     f.write(_pack('>B', _fmversion))
                 _writemarkers(f.write, [marker])