[PATCH] convert: svn-sink: test if execute bit was actually stored

[Patrick Mézard]

Hello,

Maxim Dounin a écrit :
> I've spent some more time on it, but unfortunately failed to provide
> details before your pushed patches. I'm still not ready with any
> patches, but here are some notes (in no particular order):
> 
> 1. Filectx.parents() uses filectx.renamed() and looks like it should use
> _filelog.renamed(). I haven't find any filectx.parents() users though.

That's right, I spotted it and forgot afterwards. This is fixed by 3ef279074c77, thanks.

> 2. Filectx.annotate() uses filectx.renamed() and looks like it should
> use _filelog.renamed() too. It won't degrade due to this commits though,
> since it has safeguard for filectx.linkrev() != filectx.rev() case
> committed by Brendan Cully (changeset 1a437b0f4902).

Agreed.

> 3. Patch.py :: diff :: renamed() uses filectx.renamed(), and my first
> impression that it would benefit from new logic committed. I'm not
> really investigated this though, it will need additional checking.

I think both versions are correct but the former one may be faster.
 
> 4. There are some cases where filelog.renamed() are used, which probably
> will benefit from new code too. I haven't checked them yet.

I did not checked those.
 
> 5. Changectx.filectxs() will produce filectx objects without changeset's
> in them (I haven't find any users of this though). There are also other
> cases where filectx may be erroneously created without changeset.

That's one of the reason I avoid filectx when I can. The other is workingfilectx inheriting from filectx makes any changes really complicated especially with the lazy initialization behaviours (this remark also applies to (working)changectx). The second reason is why I left the renamed() call unchanged in annotate().
 
> 2. We should use logic when calculating renamed() when we are traveling
> though changesets, but shouldn't when we are following filelog. Current
> implementation in filectx introduce ambiguity, since we never know (or
> should keep it in mind, or should test if rev == linkrev) if filectx
> object was created from changectx and thus holds changeset, or it
> wasn't. Example of this - Brendan's guard in filectx.annotate().
> 
> Probably we should consider moving this into changectx class instead,
> and maintaining rev == linkrev invariant in filectx.

This is a breaking change and a complicated one but it sounds good to me, more logical. Let's start with this renamed thing, here is an attempt to rollback filectx.renamed() behaviour (good for third-parties code) and move the new code into changectx. Requiring a filectx to be passed to changectx.renamed() may look a little verbose but I don't want this operation to look as a cheap one (you need a filelog here) and it allows filelog/filectx to be reused.

What do you think ?


diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -240,7 +240,7 @@ class mercurial_source(converter_source)
         copies = {}
         for name in files:
             try:
-                copies[name] = ctx.filectx(name).renamed()[0]
+                copies[name] = ctx.renamed(ctx.filectx(name))[0]
             except TypeError:
                 pass
         return copies
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -881,8 +881,7 @@ def debugrename(ui, repo, file1, *pats, 
     ctx = repo.changectx(opts.get('rev', 'tip'))
     for src, abs, rel, exact in cmdutil.walk(repo, (file1,) + pats, opts,
                                              ctx.node()):
-        fctx = ctx.filectx(abs)
-        m = fctx.filelog().renamed(fctx.filenode())
+        m = ctx.filectx(abs).renamed()
         if m:
             ui.write(_("%s renamed from %s:%s\n") % (rel, m[0], hex(m[1])))
         else:
@@ -1727,7 +1726,8 @@ def log(ui, repo, *pats, **opts):
         # filectx logic.
 
         try:
-            return repo.changectx(rev).filectx(fn).renamed()
+            ctx = repo.changectx(rev)
+            return ctx.renamed(ctx.filectx(fn))
         except revlog.LookupError:
             pass
         return None
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -142,6 +142,36 @@ class changectx(object):
         n = self._repo.changelog.ancestor(self._node, c2._node)
         return changectx(self._repo, n)
 
+    def renamed(self, fctx):
+        """Check the file revision was created by a rename in this
+        changeset.
+        
+        Return False or a pair (copyname, copynode) where copyname is
+        the origin filename and copynode the origin file node.
+        """
+        renamed = fctx.renamed()
+        if not renamed:
+            return renamed
+
+        if self.rev() == fctx.linkrev():
+            return renamed
+
+        fname = fctx.path()
+        try:
+            fnode = self.filenode(fname)
+        except revlog.LookupError:
+            return False
+        if fnode != fctx.filenode():
+            return False
+
+        for p in self.parents():
+            try:
+                if fnode == p.filenode(fname):
+                    return None
+            except revlog.LookupError:
+                pass
+        return renamed
+
 class filectx(object):
     """A filecontext object makes access to data related to a particular
        filerevision convenient."""
@@ -252,36 +282,21 @@ class filectx(object):
     def cmp(self, text): return self._filelog.cmp(self._filenode, text)
 
     def renamed(self):
-        """check if file was actually renamed in this changeset revision
+        """Check current file revision was created by a rename.
 
-        If rename logged in file revision, we report copy for changeset only
-        if file revisions linkrev points back to the changeset in question
-        or both changeset parents contain different file revisions.
+        Return False or a pair (copyname, copynode) where copyname is the
+        origin file name and copynode the origin file node.
+        Use changectx.renamed() to know if this file revision was renamed
+        in a specific changeset.
         """
-
-        renamed = self._filelog.renamed(self._filenode)
-        if not renamed:
-            return renamed
-
-        if self.rev() == self.linkrev():
-            return renamed
-
-        name = self.path()
-        fnode = self._filenode
-        for p in self._changectx.parents():
-            try:
-                if fnode == p.filenode(name):
-                    return None
-            except revlog.LookupError:
-                pass
-        return renamed
+        return self._filelog.renamed(self._filenode)
 
     def parents(self):
         p = self._path
         fl = self._filelog
         pl = [(p, n, fl) for n in self._filelog.parents(self._filenode)]
 
-        r = self._filelog.renamed(self._filenode)
+        r = self.renamed()
         if r:
             pl[0] = (r[0], r[1], None)