[PATCH 1 of 5] commit: use None value for missing files instead of overloading IOError
Augie Fackler
raf at durin42.com
Tue Aug 26 14:12:09 CDT 2014
On Mon, Aug 25, 2014 at 03:22:59AM +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski at 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 at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list