Patchwork [1,of,5] commit: use None value for missing files instead of overloading IOError

login
register
mail settings
Submitter Mads Kiilerich
Date Aug. 25, 2014, 1:22 a.m.
Message ID <705fb28816fda33dc8be.1408929779@localhost.localdomain>
Download mbox | patch
Permalink /patch/5586/
State Changes Requested
Headers show

Comments

Mads Kiilerich - Aug. 25, 2014, 1:22 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1408927601 -7200
#      Mon Aug 25 02:46:41 2014 +0200
# Node ID 705fb28816fda33dc8be24aa5f658ad5459fdbaf
# Parent  90cf454edd709c616d1e5ea4f30fb4d02f0c01a4
commit: use None value for missing files instead of overloading IOError

The internal API used IOError to indicate that a file should be marked as
removed.

There is some correlation between IOError (especially with ENOENT) and files
that should be removed, but using IOErrors to represent file removal internally
required some hacks.

Instead, use the value None to indicate that the file not is present.

Before, spurious IO errors could cause commits that silently removed files.
They will now be reported like all other IO errors so the root cause can be
fixed.

It is hard to introspect this internal API so for now we stay backward
compatible but will issue a warning when it is used.
Augie Fackler - Aug. 26, 2014, 7:12 p.m.
On Mon, Aug 25, 2014 at 03:22:59AM +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1408927601 -7200
> #      Mon Aug 25 02:46:41 2014 +0200
> # Node ID 705fb28816fda33dc8be24aa5f658ad5459fdbaf
> # Parent  90cf454edd709c616d1e5ea4f30fb4d02f0c01a4
> commit: use None value for missing files instead of overloading IOError
>
> The internal API used IOError to indicate that a file should be marked as
> removed.
>
> There is some correlation between IOError (especially with ENOENT) and files
> that should be removed, but using IOErrors to represent file removal internally
> required some hacks.
>
> Instead, use the value None to indicate that the file not is present.
>
> Before, spurious IO errors could cause commits that silently removed files.
> They will now be reported like all other IO errors so the root cause can be
> fixed.
>
> It is hard to introspect this internal API so for now we stay backward
> compatible but will issue a warning when it is used.

I think I'd rather you just break things hard now, and extension
maintainers will notice.

Otherwise we'll never get out of the mess.

>
> diff --git a/hgext/convert/bzr.py b/hgext/convert/bzr.py
> --- a/hgext/convert/bzr.py
> +++ b/hgext/convert/bzr.py
> @@ -122,8 +122,7 @@ class bzr_source(converter_source):
>              kind = revtree.kind(fileid)
>          if kind not in supportedkinds:
>              # the file is not available anymore - was deleted
> -            raise IOError(_('%s is not available in %s anymore') %
> -                    (name, rev))
> +            return None, None
>          mode = self._modecache[(name, rev)]
>          if kind == 'symlink':
>              target = revtree.get_symlink_target(fileid)
> diff --git a/hgext/convert/common.py b/hgext/convert/common.py
> --- a/hgext/convert/common.py
> +++ b/hgext/convert/common.py
> @@ -88,8 +88,8 @@ class converter_source(object):
>      def getfile(self, name, rev):
>          """Return a pair (data, mode) where data is the file content
>          as a string and mode one of '', 'x' or 'l'. rev is the
> -        identifier returned by a previous call to getchanges(). Raise
> -        IOError to indicate that name was deleted in rev.
> +        identifier returned by a previous call to getchanges().
> +        Data is None if file is missing/deleted in rev.
>          """
>          raise NotImplementedError
>
> diff --git a/hgext/convert/cvs.py b/hgext/convert/cvs.py
> --- a/hgext/convert/cvs.py
> +++ b/hgext/convert/cvs.py
> @@ -220,7 +220,7 @@ class convert_cvs(converter_source):
>
>          self._parse()
>          if rev.endswith("(DEAD)"):
> -            raise IOError
> +            return None, None
>
>          args = ("-N -P -kk -r %s --" % rev).split()
>          args.append(self.cvsrepo + '/' + name)
> diff --git a/hgext/convert/darcs.py b/hgext/convert/darcs.py
> --- a/hgext/convert/darcs.py
> +++ b/hgext/convert/darcs.py
> @@ -8,7 +8,7 @@
>  from common import NoRepo, checktool, commandline, commit, converter_source
>  from mercurial.i18n import _
>  from mercurial import util
> -import os, shutil, tempfile, re
> +import os, shutil, tempfile, re, errno
>
>  # The naming drift of ElementTree is fun!
>
> @@ -192,8 +192,13 @@ class darcs_source(converter_source, com
>          if rev != self.lastrev:
>              raise util.Abort(_('internal calling inconsistency'))
>          path = os.path.join(self.tmppath, name)
> -        data = util.readfile(path)
> -        mode = os.lstat(path).st_mode
> +        try:
> +            data = util.readfile(path)
> +            mode = os.lstat(path).st_mode
> +        except IOError, inst:
> +            if inst.errno == errno.ENOENT:
> +                return None, None
> +            raise
>          mode = (mode & 0111) and 'x' or ''
>          return data, mode
>
> diff --git a/hgext/convert/git.py b/hgext/convert/git.py
> --- a/hgext/convert/git.py
> +++ b/hgext/convert/git.py
> @@ -135,7 +135,7 @@ class convert_git(converter_source):
>
>      def getfile(self, name, rev):
>          if rev == hex(nullid):
> -            raise IOError
> +            return None, None
>          if name == '.hgsub':
>              data = '\n'.join([m.hgsub() for m in self.submoditer()])
>              mode = ''
> diff --git a/hgext/convert/gnuarch.py b/hgext/convert/gnuarch.py
> --- a/hgext/convert/gnuarch.py
> +++ b/hgext/convert/gnuarch.py
> @@ -137,9 +137,8 @@ class gnuarch_source(converter_source, c
>          if rev != self.lastrev:
>              raise util.Abort(_('internal calling inconsistency'))
>
> -        # Raise IOError if necessary (i.e. deleted files).
>          if not os.path.lexists(os.path.join(self.tmppath, name)):
> -            raise IOError
> +            return None, None
>
>          return self._getfile(name, rev)
>
> diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
> --- a/hgext/convert/hg.py
> +++ b/hgext/convert/hg.py
> @@ -134,6 +134,8 @@ class mercurial_sink(converter_sink):
>          def getfilectx(repo, memctx, f):
>              v = files[f]
>              data, mode = source.getfile(f, v)
> +            if data is None:
> +                return None
>              if f == '.hgtags':
>                  data = self._rewritetags(source, revmap, data)
>              return context.memfilectx(self.repo, f, data, 'l' in mode,
> @@ -351,8 +353,8 @@ class mercurial_source(converter_source)
>          try:
>              fctx = self.changectx(rev)[name]
>              return fctx.data(), fctx.flags()
> -        except error.LookupError, err:
> -            raise IOError(err)
> +        except error.LookupError:
> +            return None, None
>
>      def getchanges(self, rev):
>          ctx = self.changectx(rev)
> diff --git a/hgext/convert/monotone.py b/hgext/convert/monotone.py
> --- a/hgext/convert/monotone.py
> +++ b/hgext/convert/monotone.py
> @@ -282,11 +282,11 @@ class monotone_source(converter_source,
>
>      def getfile(self, name, rev):
>          if not self.mtnisfile(name, rev):
> -            raise IOError # file was deleted or renamed
> +            return None, None
>          try:
>              data = self.mtnrun("get_file_of", name, r=rev)
>          except Exception:
> -            raise IOError # file was deleted or renamed
> +            return None, None
>          self.mtnloadmanifest(rev)
>          node, attr = self.files.get(name, (None, ""))
>          return data, attr
> diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
> --- a/hgext/convert/p4.py
> +++ b/hgext/convert/p4.py
> @@ -181,7 +181,7 @@ class p4_source(converter_source):
>                  contents += data
>
>          if mode is None:
> -            raise IOError(0, "bad stat")
> +            return None, None
>
>          if keywords:
>              contents = keywords.sub("$\\1$", contents)
> diff --git a/hgext/convert/subversion.py b/hgext/convert/subversion.py
> --- a/hgext/convert/subversion.py
> +++ b/hgext/convert/subversion.py
> @@ -933,7 +933,7 @@ class svn_source(converter_source):
>      def getfile(self, file, rev):
>          # TODO: ra.get_file transmits the whole file instead of diffs.
>          if file in self.removed:
> -            raise IOError
> +            return None, None
>          mode = ''
>          try:
>              new_module, revnum = revsplit(rev)[1:]
> @@ -954,7 +954,7 @@ class svn_source(converter_source):
>              notfound = (svn.core.SVN_ERR_FS_NOT_FOUND,
>                  svn.core.SVN_ERR_RA_DAV_PATH_NOT_FOUND)
>              if e.apr_err in notfound: # File not found
> -                raise IOError
> +                return None, None
>              raise
>          if mode == 'l':
>              link_prefix = "link "
> @@ -1236,9 +1236,8 @@ class svn_sink(converter_sink, commandli
>
>          # Apply changes to working copy
>          for f, v in files:
> -            try:
> -                data, mode = source.getfile(f, v)
> -            except IOError:
> +            data, mode = source.getfile(f, v)
> +            if data is None:
>                  self.delete.append(f)
>              else:
>                  self.putfile(f, mode, data)
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -283,7 +283,7 @@ def collapse(repo, first, last, commitop
>                                        isexec='x' in flags,
>                                        copied=copied.get(path))
>              return mctx
> -        raise IOError()
> +        return None
>
>      if commitopts.get('message'):
>          message = commitopts['message']
> diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
> --- a/hgext/largefiles/lfcommands.py
> +++ b/hgext/largefiles/lfcommands.py
> @@ -146,7 +146,7 @@ def _addchangeset(ui, rsrc, rdst, ctx, r
>              try:
>                  fctx = ctx.filectx(lfutil.standin(f))
>              except error.LookupError:
> -                raise IOError
> +                return None
>              renamed = fctx.renamed()
>              if renamed:
>                  renamed = lfutil.splitstandin(renamed[0])
> @@ -248,7 +248,7 @@ def _lfconvert_addchangeset(rsrc, rdst,
>              try:
>                  fctx = ctx.filectx(srcfname)
>              except error.LookupError:
> -                raise IOError
> +                return None
>              renamed = fctx.renamed()
>              if renamed:
>                  # standin is always a largefile because largefile-ness
> @@ -298,7 +298,7 @@ def _getnormalcontext(repo, ctx, f, revm
>      try:
>          fctx = ctx.filectx(f)
>      except error.LookupError:
> -        raise IOError
> +        return None
>      renamed = fctx.renamed()
>      if renamed:
>          renamed = renamed[0]
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2124,7 +2124,7 @@ def amend(ui, repo, commitfunc, old, ext
>                                                    copied=copied.get(path))
>                          return mctx
>                      except KeyError:
> -                        raise IOError
> +                        return None
>              else:
>                  ui.note(_('copying changeset %s to %s\n') % (old, base))
>
> @@ -2133,7 +2133,7 @@ def amend(ui, repo, commitfunc, old, ext
>                      try:
>                          return old.filectx(path)
>                      except KeyError:
> -                        raise IOError
> +                        return None
>
>                  user = opts.get('user') or old.user()
>                  date = opts.get('date') or old.date()
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -347,7 +347,10 @@ class basectx(object):
>  def makememctx(repo, parents, text, user, date, branch, files, store,
>                 editor=None):
>      def getfilectx(repo, memctx, path):
> -        data, (islink, isexec), copied = store.getfile(path)
> +        data, mode, copied = store.getfile(path)
> +        if data is None:
> +            return None
> +        islink, isexec = mode
>          return memfilectx(repo, path, data, islink=islink, isexec=isexec,
>                                    copied=copied, memctx=memctx)
>      extra = {}
> @@ -1626,7 +1629,9 @@ class memctx(committablectx):
>              self._repo.savecommitmessage(self._text)
>
>      def filectx(self, path, filelog=None):
> -        """get a file context from the working directory"""
> +        """get a file context from the working directory
> +
> +        Returns None if file doesn't exist and should be removed."""
>          return self._filectxfn(self._repo, self, path)
>
>      def commit(self):
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1394,9 +1394,12 @@ class localrepository(object):
>                      self.ui.note(f + "\n")
>                      try:
>                          fctx = ctx[f]
> -                        new[f] = self._filecommit(fctx, m1, m2, linkrev, trp,
> -                                                  changed)
> -                        m1.set(f, fctx.flags())
> +                        if fctx is None:
> +                            removed.append(f)
> +                        else:
> +                            new[f] = self._filecommit(fctx, m1, m2, linkrev,
> +                                                      trp, changed)
> +                            m1.set(f, fctx.flags())
>                      except OSError, inst:
>                          self.ui.warn(_("trouble committing %s!\n") % f)
>                          raise
> @@ -1407,6 +1410,7 @@ class localrepository(object):
>                              raise
>                          else:
>                              removed.append(f)
> +                            self.ui.warn(("internal error: IOError removes\n"))
>
>                  # update manifest
>                  m1.update(new)
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -382,7 +382,7 @@ class abstractbackend(object):
>
>      def getfile(self, fname):
>          """Return target file data and flags as a (data, (islink,
> -        isexec)) tuple.
> +        isexec)) tuple. Data is None if file is missing/deleted.
>          """
>          raise NotImplementedError
>
> @@ -426,7 +426,12 @@ class fsbackend(abstractbackend):
>          except OSError, e:
>              if e.errno != errno.ENOENT:
>                  raise
> -        return (self.opener.read(fname), (False, isexec))
> +        try:
> +            return (self.opener.read(fname), (False, isexec))
> +        except IOError, e:
> +            if e.errno != errno.ENOENT:
> +                raise
> +            return None, None
>
>      def setfile(self, fname, data, mode, copysource):
>          islink, isexec = mode
> @@ -528,7 +533,7 @@ class filestore(object):
>          if fname in self.data:
>              return self.data[fname]
>          if not self.opener or fname not in self.files:
> -            raise IOError
> +            return None, None, None
>          fn, mode, copied = self.files[fname]
>          return self.opener.read(fn), mode, copied
>
> @@ -554,7 +559,7 @@ class repobackend(abstractbackend):
>          try:
>              fctx = self.ctx[fname]
>          except error.LookupError:
> -            raise IOError
> +            return None, None
>          flags = fctx.flags()
>          return fctx.data(), ('l' in flags, 'x' in flags)
>
> @@ -597,13 +602,12 @@ class patchfile(object):
>          self.copysource = gp.oldpath
>          self.create = gp.op in ('ADD', 'COPY', 'RENAME')
>          self.remove = gp.op == 'DELETE'
> -        try:
> -            if self.copysource is None:
> -                data, mode = backend.getfile(self.fname)
> -                self.exists = True
> -            else:
> -                data, mode = store.getfile(self.copysource)[:2]
> -                self.exists = backend.exists(self.fname)
> +        if self.copysource is None:
> +            data, mode = backend.getfile(self.fname)
> +        else:
> +            data, mode = store.getfile(self.copysource)[:2]
> +        if data is not None:
> +            self.exists = self.copysource is None or backend.exists(self.fname)
>              self.missing = False
>              if data:
>                  self.lines = mdiff.splitnewlines(data)
> @@ -622,7 +626,7 @@ class patchfile(object):
>                              l = l[:-2] + '\n'
>                          nlines.append(l)
>                      self.lines = nlines
> -        except IOError:
> +        else:
>              if self.create:
>                  self.missing = False
>              if self.mode is None:
> @@ -1380,6 +1384,8 @@ def _applydiff(ui, fp, patcher, backend,
>                  data, mode = None, None
>                  if gp.op in ('RENAME', 'COPY'):
>                      data, mode = store.getfile(gp.oldpath)[:2]
> +                    # FIXME: failing getfile has never been handled here
> +                    assert data is not None
>                  if gp.mode:
>                      mode = gp.mode
>                      if gp.op == 'ADD':
> @@ -1404,15 +1410,13 @@ def _applydiff(ui, fp, patcher, backend,
>          elif state == 'git':
>              for gp in values:
>                  path = pstrip(gp.oldpath)
> -                try:
> -                    data, mode = backend.getfile(path)
> -                except IOError, e:
> -                    if e.errno != errno.ENOENT:
> -                        raise
> +                data, mode = backend.getfile(path)
> +                if data is None:
>                      # The error ignored here will trigger a getfile()
>                      # error in a place more appropriate for error
>                      # handling, and will not interrupt the patching
>                      # process.
> +                    pass
>                  else:
>                      store.setfile(path, data, mode)
>          else:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Aug. 26, 2014, 8:55 p.m.
On 08/26/2014 09:12 PM, Augie Fackler wrote:
> On Mon, Aug 25, 2014 at 03:22:59AM +0200, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1408927601 -7200
>> #      Mon Aug 25 02:46:41 2014 +0200
>> # Node ID 705fb28816fda33dc8be24aa5f658ad5459fdbaf
>> # Parent  90cf454edd709c616d1e5ea4f30fb4d02f0c01a4
>> commit: use None value for missing files instead of overloading IOError
>>
>> The internal API used IOError to indicate that a file should be marked as
>> removed.
>>
>> There is some correlation between IOError (especially with ENOENT) and files
>> that should be removed, but using IOErrors to represent file removal internally
>> required some hacks.
>>
>> Instead, use the value None to indicate that the file not is present.
>>
>> Before, spurious IO errors could cause commits that silently removed files.
>> They will now be reported like all other IO errors so the root cause can be
>> fixed.
>>
>> It is hard to introspect this internal API so for now we stay backward
>> compatible but will issue a warning when it is used.
>
> I think I'd rather you just break things hard now, and extension
> maintainers will notice.
>
> Otherwise we'll never get out of the mess.

Meh, I believe a grace period would be nice to give some room for 
extension people to update. A noisy enough warning should be enough for 
them to notice than an upgrade is needed.

(from someone maintaining extension with massive usage of memctx and 
trying to keep stable+default compat to please mpm)

Patch

diff --git a/hgext/convert/bzr.py b/hgext/convert/bzr.py
--- a/hgext/convert/bzr.py
+++ b/hgext/convert/bzr.py
@@ -122,8 +122,7 @@  class bzr_source(converter_source):
             kind = revtree.kind(fileid)
         if kind not in supportedkinds:
             # the file is not available anymore - was deleted
-            raise IOError(_('%s is not available in %s anymore') %
-                    (name, rev))
+            return None, None
         mode = self._modecache[(name, rev)]
         if kind == 'symlink':
             target = revtree.get_symlink_target(fileid)
diff --git a/hgext/convert/common.py b/hgext/convert/common.py
--- a/hgext/convert/common.py
+++ b/hgext/convert/common.py
@@ -88,8 +88,8 @@  class converter_source(object):
     def getfile(self, name, rev):
         """Return a pair (data, mode) where data is the file content
         as a string and mode one of '', 'x' or 'l'. rev is the
-        identifier returned by a previous call to getchanges(). Raise
-        IOError to indicate that name was deleted in rev.
+        identifier returned by a previous call to getchanges().
+        Data is None if file is missing/deleted in rev.
         """
         raise NotImplementedError
 
diff --git a/hgext/convert/cvs.py b/hgext/convert/cvs.py
--- a/hgext/convert/cvs.py
+++ b/hgext/convert/cvs.py
@@ -220,7 +220,7 @@  class convert_cvs(converter_source):
 
         self._parse()
         if rev.endswith("(DEAD)"):
-            raise IOError
+            return None, None
 
         args = ("-N -P -kk -r %s --" % rev).split()
         args.append(self.cvsrepo + '/' + name)
diff --git a/hgext/convert/darcs.py b/hgext/convert/darcs.py
--- a/hgext/convert/darcs.py
+++ b/hgext/convert/darcs.py
@@ -8,7 +8,7 @@ 
 from common import NoRepo, checktool, commandline, commit, converter_source
 from mercurial.i18n import _
 from mercurial import util
-import os, shutil, tempfile, re
+import os, shutil, tempfile, re, errno
 
 # The naming drift of ElementTree is fun!
 
@@ -192,8 +192,13 @@  class darcs_source(converter_source, com
         if rev != self.lastrev:
             raise util.Abort(_('internal calling inconsistency'))
         path = os.path.join(self.tmppath, name)
-        data = util.readfile(path)
-        mode = os.lstat(path).st_mode
+        try:
+            data = util.readfile(path)
+            mode = os.lstat(path).st_mode
+        except IOError, inst:
+            if inst.errno == errno.ENOENT:
+                return None, None
+            raise
         mode = (mode & 0111) and 'x' or ''
         return data, mode
 
diff --git a/hgext/convert/git.py b/hgext/convert/git.py
--- a/hgext/convert/git.py
+++ b/hgext/convert/git.py
@@ -135,7 +135,7 @@  class convert_git(converter_source):
 
     def getfile(self, name, rev):
         if rev == hex(nullid):
-            raise IOError
+            return None, None
         if name == '.hgsub':
             data = '\n'.join([m.hgsub() for m in self.submoditer()])
             mode = ''
diff --git a/hgext/convert/gnuarch.py b/hgext/convert/gnuarch.py
--- a/hgext/convert/gnuarch.py
+++ b/hgext/convert/gnuarch.py
@@ -137,9 +137,8 @@  class gnuarch_source(converter_source, c
         if rev != self.lastrev:
             raise util.Abort(_('internal calling inconsistency'))
 
-        # Raise IOError if necessary (i.e. deleted files).
         if not os.path.lexists(os.path.join(self.tmppath, name)):
-            raise IOError
+            return None, None
 
         return self._getfile(name, rev)
 
diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -134,6 +134,8 @@  class mercurial_sink(converter_sink):
         def getfilectx(repo, memctx, f):
             v = files[f]
             data, mode = source.getfile(f, v)
+            if data is None:
+                return None
             if f == '.hgtags':
                 data = self._rewritetags(source, revmap, data)
             return context.memfilectx(self.repo, f, data, 'l' in mode,
@@ -351,8 +353,8 @@  class mercurial_source(converter_source)
         try:
             fctx = self.changectx(rev)[name]
             return fctx.data(), fctx.flags()
-        except error.LookupError, err:
-            raise IOError(err)
+        except error.LookupError:
+            return None, None
 
     def getchanges(self, rev):
         ctx = self.changectx(rev)
diff --git a/hgext/convert/monotone.py b/hgext/convert/monotone.py
--- a/hgext/convert/monotone.py
+++ b/hgext/convert/monotone.py
@@ -282,11 +282,11 @@  class monotone_source(converter_source, 
 
     def getfile(self, name, rev):
         if not self.mtnisfile(name, rev):
-            raise IOError # file was deleted or renamed
+            return None, None
         try:
             data = self.mtnrun("get_file_of", name, r=rev)
         except Exception:
-            raise IOError # file was deleted or renamed
+            return None, None
         self.mtnloadmanifest(rev)
         node, attr = self.files.get(name, (None, ""))
         return data, attr
diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
--- a/hgext/convert/p4.py
+++ b/hgext/convert/p4.py
@@ -181,7 +181,7 @@  class p4_source(converter_source):
                 contents += data
 
         if mode is None:
-            raise IOError(0, "bad stat")
+            return None, None
 
         if keywords:
             contents = keywords.sub("$\\1$", contents)
diff --git a/hgext/convert/subversion.py b/hgext/convert/subversion.py
--- a/hgext/convert/subversion.py
+++ b/hgext/convert/subversion.py
@@ -933,7 +933,7 @@  class svn_source(converter_source):
     def getfile(self, file, rev):
         # TODO: ra.get_file transmits the whole file instead of diffs.
         if file in self.removed:
-            raise IOError
+            return None, None
         mode = ''
         try:
             new_module, revnum = revsplit(rev)[1:]
@@ -954,7 +954,7 @@  class svn_source(converter_source):
             notfound = (svn.core.SVN_ERR_FS_NOT_FOUND,
                 svn.core.SVN_ERR_RA_DAV_PATH_NOT_FOUND)
             if e.apr_err in notfound: # File not found
-                raise IOError
+                return None, None
             raise
         if mode == 'l':
             link_prefix = "link "
@@ -1236,9 +1236,8 @@  class svn_sink(converter_sink, commandli
 
         # Apply changes to working copy
         for f, v in files:
-            try:
-                data, mode = source.getfile(f, v)
-            except IOError:
+            data, mode = source.getfile(f, v)
+            if data is None:
                 self.delete.append(f)
             else:
                 self.putfile(f, mode, data)
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -283,7 +283,7 @@  def collapse(repo, first, last, commitop
                                       isexec='x' in flags,
                                       copied=copied.get(path))
             return mctx
-        raise IOError()
+        return None
 
     if commitopts.get('message'):
         message = commitopts['message']
diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
--- a/hgext/largefiles/lfcommands.py
+++ b/hgext/largefiles/lfcommands.py
@@ -146,7 +146,7 @@  def _addchangeset(ui, rsrc, rdst, ctx, r
             try:
                 fctx = ctx.filectx(lfutil.standin(f))
             except error.LookupError:
-                raise IOError
+                return None
             renamed = fctx.renamed()
             if renamed:
                 renamed = lfutil.splitstandin(renamed[0])
@@ -248,7 +248,7 @@  def _lfconvert_addchangeset(rsrc, rdst, 
             try:
                 fctx = ctx.filectx(srcfname)
             except error.LookupError:
-                raise IOError
+                return None
             renamed = fctx.renamed()
             if renamed:
                 # standin is always a largefile because largefile-ness
@@ -298,7 +298,7 @@  def _getnormalcontext(repo, ctx, f, revm
     try:
         fctx = ctx.filectx(f)
     except error.LookupError:
-        raise IOError
+        return None
     renamed = fctx.renamed()
     if renamed:
         renamed = renamed[0]
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2124,7 +2124,7 @@  def amend(ui, repo, commitfunc, old, ext
                                                   copied=copied.get(path))
                         return mctx
                     except KeyError:
-                        raise IOError
+                        return None
             else:
                 ui.note(_('copying changeset %s to %s\n') % (old, base))
 
@@ -2133,7 +2133,7 @@  def amend(ui, repo, commitfunc, old, ext
                     try:
                         return old.filectx(path)
                     except KeyError:
-                        raise IOError
+                        return None
 
                 user = opts.get('user') or old.user()
                 date = opts.get('date') or old.date()
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -347,7 +347,10 @@  class basectx(object):
 def makememctx(repo, parents, text, user, date, branch, files, store,
                editor=None):
     def getfilectx(repo, memctx, path):
-        data, (islink, isexec), copied = store.getfile(path)
+        data, mode, copied = store.getfile(path)
+        if data is None:
+            return None
+        islink, isexec = mode
         return memfilectx(repo, path, data, islink=islink, isexec=isexec,
                                   copied=copied, memctx=memctx)
     extra = {}
@@ -1626,7 +1629,9 @@  class memctx(committablectx):
             self._repo.savecommitmessage(self._text)
 
     def filectx(self, path, filelog=None):
-        """get a file context from the working directory"""
+        """get a file context from the working directory
+
+        Returns None if file doesn't exist and should be removed."""
         return self._filectxfn(self._repo, self, path)
 
     def commit(self):
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1394,9 +1394,12 @@  class localrepository(object):
                     self.ui.note(f + "\n")
                     try:
                         fctx = ctx[f]
-                        new[f] = self._filecommit(fctx, m1, m2, linkrev, trp,
-                                                  changed)
-                        m1.set(f, fctx.flags())
+                        if fctx is None:
+                            removed.append(f)
+                        else:
+                            new[f] = self._filecommit(fctx, m1, m2, linkrev,
+                                                      trp, changed)
+                            m1.set(f, fctx.flags())
                     except OSError, inst:
                         self.ui.warn(_("trouble committing %s!\n") % f)
                         raise
@@ -1407,6 +1410,7 @@  class localrepository(object):
                             raise
                         else:
                             removed.append(f)
+                            self.ui.warn(("internal error: IOError removes\n"))
 
                 # update manifest
                 m1.update(new)
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -382,7 +382,7 @@  class abstractbackend(object):
 
     def getfile(self, fname):
         """Return target file data and flags as a (data, (islink,
-        isexec)) tuple.
+        isexec)) tuple. Data is None if file is missing/deleted.
         """
         raise NotImplementedError
 
@@ -426,7 +426,12 @@  class fsbackend(abstractbackend):
         except OSError, e:
             if e.errno != errno.ENOENT:
                 raise
-        return (self.opener.read(fname), (False, isexec))
+        try:
+            return (self.opener.read(fname), (False, isexec))
+        except IOError, e:
+            if e.errno != errno.ENOENT:
+                raise
+            return None, None
 
     def setfile(self, fname, data, mode, copysource):
         islink, isexec = mode
@@ -528,7 +533,7 @@  class filestore(object):
         if fname in self.data:
             return self.data[fname]
         if not self.opener or fname not in self.files:
-            raise IOError
+            return None, None, None
         fn, mode, copied = self.files[fname]
         return self.opener.read(fn), mode, copied
 
@@ -554,7 +559,7 @@  class repobackend(abstractbackend):
         try:
             fctx = self.ctx[fname]
         except error.LookupError:
-            raise IOError
+            return None, None
         flags = fctx.flags()
         return fctx.data(), ('l' in flags, 'x' in flags)
 
@@ -597,13 +602,12 @@  class patchfile(object):
         self.copysource = gp.oldpath
         self.create = gp.op in ('ADD', 'COPY', 'RENAME')
         self.remove = gp.op == 'DELETE'
-        try:
-            if self.copysource is None:
-                data, mode = backend.getfile(self.fname)
-                self.exists = True
-            else:
-                data, mode = store.getfile(self.copysource)[:2]
-                self.exists = backend.exists(self.fname)
+        if self.copysource is None:
+            data, mode = backend.getfile(self.fname)
+        else:
+            data, mode = store.getfile(self.copysource)[:2]
+        if data is not None:
+            self.exists = self.copysource is None or backend.exists(self.fname)
             self.missing = False
             if data:
                 self.lines = mdiff.splitnewlines(data)
@@ -622,7 +626,7 @@  class patchfile(object):
                             l = l[:-2] + '\n'
                         nlines.append(l)
                     self.lines = nlines
-        except IOError:
+        else:
             if self.create:
                 self.missing = False
             if self.mode is None:
@@ -1380,6 +1384,8 @@  def _applydiff(ui, fp, patcher, backend,
                 data, mode = None, None
                 if gp.op in ('RENAME', 'COPY'):
                     data, mode = store.getfile(gp.oldpath)[:2]
+                    # FIXME: failing getfile has never been handled here
+                    assert data is not None
                 if gp.mode:
                     mode = gp.mode
                     if gp.op == 'ADD':
@@ -1404,15 +1410,13 @@  def _applydiff(ui, fp, patcher, backend,
         elif state == 'git':
             for gp in values:
                 path = pstrip(gp.oldpath)
-                try:
-                    data, mode = backend.getfile(path)
-                except IOError, e:
-                    if e.errno != errno.ENOENT:
-                        raise
+                data, mode = backend.getfile(path)
+                if data is None:
                     # The error ignored here will trigger a getfile()
                     # error in a place more appropriate for error
                     # handling, and will not interrupt the patching
                     # process.
+                    pass
                 else:
                     store.setfile(path, data, mode)
         else: