D6554: cleanup: always `seek(0, io.SEEK_END)` after open in append mode before tell()

[durin42 (Augie Fackler)]

durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Consider the program:
  
    #include <stdio.h>
    
    int main() {
      FILE *f = fopen("narf", "w");
      fprintf(f, "narf\n");
      fclose(f);
    
      f = fopen("narf", "a");
      printf("%ld\n", ftell(f));
      fprintf(f, "troz\n");
      printf("%ld\n", ftell(f));
    
      return 0;
    }
  
  on macOS, FreeBSD, and Linux with glibc, this program prints
  
    5
    10
  
  but on musl libc (Alpine Linux and probably others) this prints
  
    0
    10
  
  By my reading of
  https://pubs.opengroup.org/onlinepubs/009695399/functions/fopen.html
  this is technically correct, specifically:
  
  > Opening a file with append mode (a as the first character in the
  > mode argument) shall cause all subsequent writes to the file to be
  > forced to the then current end-of-file, regardless of intervening
  > calls to fseek().
  
  in other words, the file position doesn't really matter in append-mode
  files, and we can't depend on it being at all meaningful unless we
  perform a seek() before tell() after open(..., 'a'). Experimentally
  after a .write() we can do a .tell() and it'll always be reasonable,
  but I'm unclear from reading the specification if that's a smart thing
  to rely on.
  
  These callsites were identified by applying the next changeset and
  then fixing new crashes.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6554

AFFECTED FILES
  mercurial/branchmap.py
  mercurial/obsolete.py
  mercurial/tags.py

CHANGE DETAILS

diff --git a/mercurial/tags.py b/mercurial/tags.py
--- a/mercurial/tags.py
+++ b/mercurial/tags.py
@@ -13,6 +13,7 @@
 from __future__ import absolute_import
 
 import errno
+import io
 
 from .node import (
     bin,
@@ -584,6 +585,7 @@
             fp = repo.vfs('localtags', 'r+')
         except IOError:
             fp = repo.vfs('localtags', 'a')
+            fp.seek(0, io.SEEK_END)
         else:
             prevtags = fp.read()
 
@@ -599,6 +601,7 @@
         if e.errno != errno.ENOENT:
             raise
         fp = repo.wvfs('.hgtags', 'ab')
+        fp.seek(0, io.SEEK_END)
     else:
         prevtags = fp.read()
 
@@ -793,6 +796,7 @@
 
         try:
             f = repo.cachevfs.open(_fnodescachefile, 'ab')
+            f.seek(0, io.SEEK_END)
             try:
                 # if the file has been truncated
                 actualoffset = f.tell()
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -71,6 +71,7 @@
 
 import errno
 import hashlib
+import io
 import struct
 
 from .i18n import _
@@ -625,6 +626,7 @@
                 new.append(m)
         if new:
             f = self.svfs('obsstore', 'ab')
+            f.seek(0, io.SEEK_END)
             try:
                 offset = f.tell()
                 transaction.add('obsstore', offset)
diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
--- a/mercurial/branchmap.py
+++ b/mercurial/branchmap.py
@@ -7,6 +7,7 @@
 
 from __future__ import absolute_import
 
+import io
 import struct
 
 from .node import (
@@ -640,6 +641,7 @@
         """ write the new branch names to revbranchcache """
         if self._rbcnamescount != 0:
             f = repo.cachevfs.open(_rbcnames, 'ab')
+            f.seek(0, io.SEEK_END)
             if f.tell() == self._rbcsnameslen:
                 f.write('\0')
             else:
@@ -661,6 +663,7 @@
         """ write the new revs to revbranchcache """
         revs = min(len(repo.changelog), len(self._rbcrevs) // _rbcrecsize)
         with repo.cachevfs.open(_rbcrevs, 'ab') as f:
+            f.seek(0, io.SEEK_END)
             if f.tell() != start:
                 repo.ui.debug("truncating cache/%s to %d\n" % (_rbcrevs, start))
                 f.seek(start)



To: durin42, #hg-reviewers
Cc: mercurial-devel