Patchwork [1,of,3] vfs: seek to the end of file when opening in append mode

login
register
mail settings
Submitter Matt Harbison
Date Feb. 4, 2015, 3:49 a.m.
Message ID <f78eca3cb69711a33cf7.1423021778@Envy>
Download mbox | patch
Permalink /patch/7647/
State Rejected
Headers show

Comments

Matt Harbison - Feb. 4, 2015, 3:49 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1422725984 18000
#      Sat Jan 31 12:39:44 2015 -0500
# Node ID f78eca3cb69711a33cf70b0a3f84f3fe8f730e43
# Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
vfs: seek to the end of file when opening in append mode

The position is implementation defined when opening in append mode, and it seems
like Linux sets it to EOF while Windows keeps it at zero.  This has caused
problems in the past when a file is opened and tell() is immediately called,
such as 48c232873a54 and 6bf93440a717.
Adrian Buehlmann - Feb. 4, 2015, 8:22 a.m.
On 2015-02-04 04:49, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1422725984 18000
> #      Sat Jan 31 12:39:44 2015 -0500
> # Node ID f78eca3cb69711a33cf70b0a3f84f3fe8f730e43
> # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
> vfs: seek to the end of file when opening in append mode
> 
> The position is implementation defined when opening in append mode, and it seems
> like Linux sets it to EOF while Windows keeps it at zero.  This has caused
> problems in the past when a file is opened and tell() is immediately called,
> such as 48c232873a54 and 6bf93440a717.
> 
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -20,6 +20,8 @@
>  systemrcpath = scmplatform.systemrcpath
>  userrcpath = scmplatform.userrcpath
>  
> +_SEEK_END = 2 # os.SEEK_END was introduced in Python 2.5
> +
>  class status(tuple):
>      '''Named tuple with a list of files per status. The 'deleted', 'unknown'
>         and 'ignored' properties are only relevant to the working copy.
> @@ -398,7 +400,13 @@
>              if basename:
>                  if atomictemp:
>                      util.ensuredirs(dirname, self.createmode, notindexed)
> -                    return util.atomictempfile(f, mode, self.createmode)
> +                    fp = util.atomictempfile(f, mode, self.createmode)
> +
> +                    # The position when opening in append mode is implementation
> +                    # defined, so make it consistent across all platforms.
> +                    if 'a' in mode:
> +                        fp.seek(0, _SEEK_END)
> +                    return fp
>                  try:
>                      if 'w' in mode:
>                          util.unlink(f)
> @@ -424,6 +432,12 @@
>          fp = util.posixfile(f, mode)
>          if nlink == 0:
>              self._fixfilemode(f)
> +
> +        # The position when opening in append mode is implementation defined, so
> +        # make it consistent across all platforms.
> +        if 'a' in mode:
> +            fp.seek(0, _SEEK_END)
> +
>          return fp
>  
>      def symlink(self, src, dst):

I'm not sure, but maybe this could be even done a few layers deeper
instead...

Both vfs and util.atomictempfile use posixfile.

And it's only the Windows-variant of posixfile that is currently known
to cause troubles.

Other OSes - by the spec, as we have found out - are free to be stupid
as well, but no one else but Windows has shown up so far.

So, perhaps, this could be done in windows.posixfile (where we happen to
already have a wrapper around osutil.posixfile), or possibly (hooray,
C-land) in the posixfile function in osutil.c

..and, in parallel, in pure.osutil for the posixfile class there
(applies to Windows case there only).
Matt Mackall - Feb. 4, 2015, 9:34 p.m.
On Wed, 2015-02-04 at 09:22 +0100, Adrian Buehlmann wrote:
> On 2015-02-04 04:49, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1422725984 18000
> > #      Sat Jan 31 12:39:44 2015 -0500
> > # Node ID f78eca3cb69711a33cf70b0a3f84f3fe8f730e43
> > # Parent  e1dbe0b215ae137eec53ceb12440536d570a83d2
> > vfs: seek to the end of file when opening in append mode
> > 
> > The position is implementation defined when opening in append mode, and it seems
> > like Linux sets it to EOF while Windows keeps it at zero.  This has caused
> > problems in the past when a file is opened and tell() is immediately called,
> > such as 48c232873a54 and 6bf93440a717.
> > 
> > diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> > --- a/mercurial/scmutil.py
> > +++ b/mercurial/scmutil.py
> > @@ -20,6 +20,8 @@
> >  systemrcpath = scmplatform.systemrcpath
> >  userrcpath = scmplatform.userrcpath
> >  
> > +_SEEK_END = 2 # os.SEEK_END was introduced in Python 2.5
> > +
> >  class status(tuple):
> >      '''Named tuple with a list of files per status. The 'deleted', 'unknown'
> >         and 'ignored' properties are only relevant to the working copy.
> > @@ -398,7 +400,13 @@
> >              if basename:
> >                  if atomictemp:
> >                      util.ensuredirs(dirname, self.createmode, notindexed)
> > -                    return util.atomictempfile(f, mode, self.createmode)
> > +                    fp = util.atomictempfile(f, mode, self.createmode)
> > +
> > +                    # The position when opening in append mode is implementation
> > +                    # defined, so make it consistent across all platforms.
> > +                    if 'a' in mode:
> > +                        fp.seek(0, _SEEK_END)
> > +                    return fp
> >                  try:
> >                      if 'w' in mode:
> >                          util.unlink(f)
> > @@ -424,6 +432,12 @@
> >          fp = util.posixfile(f, mode)
> >          if nlink == 0:
> >              self._fixfilemode(f)
> > +
> > +        # The position when opening in append mode is implementation defined, so
> > +        # make it consistent across all platforms.
> > +        if 'a' in mode:
> > +            fp.seek(0, _SEEK_END)
> > +
> >          return fp
> >  
> >      def symlink(self, src, dst):
> 
> I'm not sure, but maybe this could be even done a few layers deeper
> instead...
> 
> Both vfs and util.atomictempfile use posixfile.
> 
> And it's only the Windows-variant of posixfile that is currently known
> to cause troubles.
> 
> Other OSes - by the spec, as we have found out - are free to be stupid
> as well, but no one else but Windows has shown up so far.
> 
> So, perhaps, this could be done in windows.posixfile (where we happen to
> already have a wrapper around osutil.posixfile), or possibly (hooray,
> C-land) in the posixfile function in osutil.c
> 
> ..and, in parallel, in pure.osutil for the posixfile class there
> (applies to Windows case there only).

Yeah, I tend to agree. In general, we'd like to keep the number of
syscalls needed for generic file operations to a minimum, so putting
this in a Windows-only spot (posixfile) is worth the trouble.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -20,6 +20,8 @@ 
 systemrcpath = scmplatform.systemrcpath
 userrcpath = scmplatform.userrcpath
 
+_SEEK_END = 2 # os.SEEK_END was introduced in Python 2.5
+
 class status(tuple):
     '''Named tuple with a list of files per status. The 'deleted', 'unknown'
        and 'ignored' properties are only relevant to the working copy.
@@ -398,7 +400,13 @@ 
             if basename:
                 if atomictemp:
                     util.ensuredirs(dirname, self.createmode, notindexed)
-                    return util.atomictempfile(f, mode, self.createmode)
+                    fp = util.atomictempfile(f, mode, self.createmode)
+
+                    # The position when opening in append mode is implementation
+                    # defined, so make it consistent across all platforms.
+                    if 'a' in mode:
+                        fp.seek(0, _SEEK_END)
+                    return fp
                 try:
                     if 'w' in mode:
                         util.unlink(f)
@@ -424,6 +432,12 @@ 
         fp = util.posixfile(f, mode)
         if nlink == 0:
             self._fixfilemode(f)
+
+        # The position when opening in append mode is implementation defined, so
+        # make it consistent across all platforms.
+        if 'a' in mode:
+            fp.seek(0, _SEEK_END)
+
         return fp
 
     def symlink(self, src, dst):