Patchwork [2,of,9,mergedriver] filemerge: turn filemerge.filemerge into a class

login
register
mail settings
Submitter Siddharth Agarwal
Date Oct. 7, 2015, 7:33 a.m.
Message ID <3225fccf647a4c45e86b.1444203217@dev6666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/10846/
State Deferred
Headers show

Comments

Siddharth Agarwal - Oct. 7, 2015, 7:33 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1444181292 25200
#      Tue Oct 06 18:28:12 2015 -0700
# Node ID 3225fccf647a4c45e86b84746a1768ef408e96c7
# Parent  c9e8d74f140234d28d5af30927731223d7496431
filemerge: turn filemerge.filemerge into a class

We're going to store a bunch of state in between function calls on objects of
this class.
Augie Fackler - Oct. 7, 2015, 6:04 p.m.
On Wed, Oct 07, 2015 at 12:33:37AM -0700, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1444181292 25200
> #      Tue Oct 06 18:28:12 2015 -0700
> # Node ID 3225fccf647a4c45e86b84746a1768ef408e96c7
> # Parent  c9e8d74f140234d28d5af30927731223d7496431
> filemerge: turn filemerge.filemerge into a class
>
> We're going to store a bunch of state in between function calls on objects of
> this class.
>
> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -536,9 +536,15 @@ def mergerecordupdates(orig, repo, actio
>
>  # Override filemerge to prompt the user about how they wish to merge
>  # largefiles. This will handle identical edits without prompting the user.
> -def overridefilemerge(origfn, repo, mynode, orig, fcd, fco, fca, labels=None):
> +def overridefilemerge(origfn, mergectx):
> +    orig = mergectx._orig
>      if not lfutil.isstandin(orig):
> -        return origfn(repo, mynode, orig, fcd, fco, fca, labels=labels)
> +        return origfn(mergectx)
> +
> +    fca = mergectx._fca
> +    fcd = mergectx._fcd
> +    fco = mergectx._fco
> +    repo = mergectx._repo
>
>      ahash = fca.data().strip().lower()
>      dhash = fcd.data().strip().lower()
> diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
> --- a/hgext/largefiles/uisetup.py
> +++ b/hgext/largefiles/uisetup.py
> @@ -102,7 +102,7 @@ def uisetup(ui):
>                                      overrides.mergerecordupdates)
>      entry = extensions.wrapfunction(merge, 'update',
>                                      overrides.mergeupdate)
> -    entry = extensions.wrapfunction(filemerge, 'filemerge',
> +    entry = extensions.wrapfunction(filemerge.mergectx, 'merge',
>                                      overrides.overridefilemerge)
>      entry = extensions.wrapfunction(cmdutil, 'copy',
>                                      overrides.overridecopy)
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -431,17 +431,25 @@ def _formatlabels(repo, fcd, fco, fca, l
>          newlabels.append(_formatconflictmarker(repo, ca, tmpl, labels[2], pad))
>      return newlabels
>
> -def filemerge(repo, mynode, orig, fcd, fco, fca, labels=None):
> -    """perform a 3-way merge in the working directory
> +class mergectx(object):
> +    """perform a 3-way merge in the working directory"""

it looks like this docstring lost its way and wants to live on
mergectx.merge()? and then this type wants to be documented as a state
container for merges or something like that

>
> -    mynode = parent node before merge
> -    orig = original local filename before merge
> -    fco = other file context
> -    fca = ancestor file context
> -    fcd = local file context for current/destination file
> -    """
> +    def __init__(self, repo, mynode, orig, fcd, fco, fca, labels=None):
> +        """
> +        mynode = parent node before merge
> +        orig = original local filename before merge
> +        fco = other file context
> +        fca = ancestor file context
> +        fcd = local file context for current/destination file"""
> +        self._repo = repo
> +        self._mynode = mynode
> +        self._orig = orig
> +        self._fcd = fcd
> +        self._fco = fco
> +        self._fca = fca
> +        self._labels = labels
>
> -    if True:
> +    def merge(self):
>          def temp(prefix, ctx):
>              pre = "%s~%s." % (os.path.basename(ctx.path()), prefix)
>              (fd, name) = tempfile.mkstemp(prefix=pre)
> @@ -451,6 +459,14 @@ def filemerge(repo, mynode, orig, fcd, f
>              f.close()
>              return name
>
> +        repo = self._repo
> +        mynode = self._mynode
> +        orig = self._orig
> +        fcd = self._fcd
> +        fco = self._fco
> +        fca = self._fca
> +        labels = self._labels
> +
>          if not fco.cmp(fcd): # files identical?
>              return None
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -308,8 +308,9 @@ class mergestate(object):
>          f = self._repo.vfs('merge/' + hash)
>          self._repo.wwrite(dfile, f.read(), flags)
>          f.close()
> -        r = filemerge.filemerge(self._repo, self._local, lfile, fcd, fco, fca,
> -                                labels=labels)
> +        ctx = filemerge.mergectx(self._repo, self._local, lfile, fcd, fco, fca,
> +                                 labels=labels)
> +        r = ctx.merge()
>          if r is None:
>              # no real conflict
>              del self._state[dfile]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Siddharth Agarwal - Oct. 7, 2015, 6:31 p.m.
On 10/7/15 11:04 AM, Augie Fackler wrote:
>> >-def filemerge(repo, mynode, orig, fcd, fco, fca, labels=None):
>> >-    """perform a 3-way merge in the working directory
>> >+class mergectx(object):
>> >+    """perform a 3-way merge in the working directory"""
> it looks like this docstring lost its way and wants to live on
> mergectx.merge()? and then this type wants to be documented as a state
> container for merges or something like that
>

Yeah, I think you're right. Any chance you could you fix it in flight, 
Pierre-Yves?
Augie Fackler - Oct. 7, 2015, 6:55 p.m.
> On Oct 7, 2015, at 14:31, Siddharth Agarwal <sid@less-broken.com> wrote:
> 
> On 10/7/15 11:04 AM, Augie Fackler wrote:
>>> >-def filemerge(repo, mynode, orig, fcd, fco, fca, labels=None):
>>> >-    """perform a 3-way merge in the working directory
>>> >+class mergectx(object):
>>> >+    """perform a 3-way merge in the working directory"""
>> it looks like this docstring lost its way and wants to live on
>> mergectx.merge()? and then this type wants to be documented as a state
>> container for merges or something like that
>> 
> 
> Yeah, I think you're right. Any chance you could you fix it in flight, Pierre-Yves?

I've poked a followup midstream (which we can fold if people care), and have queued the whole series.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Siddharth Agarwal - Oct. 8, 2015, 3:41 a.m.
On 10/7/15 11:55 AM, Augie Fackler wrote:
>> On Oct 7, 2015, at 14:31, Siddharth Agarwal <sid@less-broken.com> wrote:
>>
>> On 10/7/15 11:04 AM, Augie Fackler wrote:
>>>>> -def filemerge(repo, mynode, orig, fcd, fco, fca, labels=None):
>>>>> -    """perform a 3-way merge in the working directory
>>>>> +class mergectx(object):
>>>>> +    """perform a 3-way merge in the working directory"""
>>> it looks like this docstring lost its way and wants to live on
>>> mergectx.merge()? and then this type wants to be documented as a state
>>> container for merges or something like that
>>>
>> Yeah, I think you're right. Any chance you could you fix it in flight, Pierre-Yves?
> I've poked a followup midstream (which we can fold if people care), and have queued the whole series.

I'm trying out a cleaner alternative implementation using coroutines, so 
could you please drop patch 2 and your followup for now? (Patch 1 still 
needs to be there because I need the extra level of indentation to catch 
GeneratorExit). Patches 3 onwards apply cleanly even without patch 2.

I think I used to have clowncopter access, but apparently not any longer.

- Siddharth


>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 8, 2015, 5:22 a.m.
On 10/07/2015 08:41 PM, Siddharth Agarwal wrote:
> On 10/7/15 11:55 AM, Augie Fackler wrote:
>>> On Oct 7, 2015, at 14:31, Siddharth Agarwal <sid@less-broken.com> wrote:
>>>
>>> On 10/7/15 11:04 AM, Augie Fackler wrote:
>>>>>> -def filemerge(repo, mynode, orig, fcd, fco, fca, labels=None):
>>>>>> -    """perform a 3-way merge in the working directory
>>>>>> +class mergectx(object):
>>>>>> +    """perform a 3-way merge in the working directory"""
>>>> it looks like this docstring lost its way and wants to live on
>>>> mergectx.merge()? and then this type wants to be documented as a state
>>>> container for merges or something like that
>>>>
>>> Yeah, I think you're right. Any chance you could you fix it in
>>> flight, Pierre-Yves?
>> I've poked a followup midstream (which we can fold if people care),
>> and have queued the whole series.
>
> I'm trying out a cleaner alternative implementation using coroutines, so
> could you please drop patch 2 and your followup for now? (Patch 1 still
> needs to be there because I need the extra level of indentation to catch
> GeneratorExit). Patches 3 onwards apply cleanly even without patch 2.

I've pruned them. But be aware that last time I tried to yield the 
coroutine hammer, mpm stopped me because they were "too obscure python 
feature that will confuse contributor"
Siddharth Agarwal - Oct. 8, 2015, 6:48 a.m.
On 10/7/15 10:22 PM, Pierre-Yves David wrote:
> I've pruned them. But be aware that last time I tried to yield the 
> coroutine hammer, mpm stopped me because they were "too obscure python 
> feature that will confuse contributor"
>

In this case a coroutine turns out to be *much* simpler than an explicit 
state machine. I actually have a state machine implementation [1] but it 
turns out to be twice the length of a coroutine that does the exact same 
thing.

[1] 
http://42.netv6.net/sid0-wip/hg/file/c307a3387a95/mercurial/filemerge.py#l440

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -536,9 +536,15 @@  def mergerecordupdates(orig, repo, actio
 
 # Override filemerge to prompt the user about how they wish to merge
 # largefiles. This will handle identical edits without prompting the user.
-def overridefilemerge(origfn, repo, mynode, orig, fcd, fco, fca, labels=None):
+def overridefilemerge(origfn, mergectx):
+    orig = mergectx._orig
     if not lfutil.isstandin(orig):
-        return origfn(repo, mynode, orig, fcd, fco, fca, labels=labels)
+        return origfn(mergectx)
+
+    fca = mergectx._fca
+    fcd = mergectx._fcd
+    fco = mergectx._fco
+    repo = mergectx._repo
 
     ahash = fca.data().strip().lower()
     dhash = fcd.data().strip().lower()
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -102,7 +102,7 @@  def uisetup(ui):
                                     overrides.mergerecordupdates)
     entry = extensions.wrapfunction(merge, 'update',
                                     overrides.mergeupdate)
-    entry = extensions.wrapfunction(filemerge, 'filemerge',
+    entry = extensions.wrapfunction(filemerge.mergectx, 'merge',
                                     overrides.overridefilemerge)
     entry = extensions.wrapfunction(cmdutil, 'copy',
                                     overrides.overridecopy)
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -431,17 +431,25 @@  def _formatlabels(repo, fcd, fco, fca, l
         newlabels.append(_formatconflictmarker(repo, ca, tmpl, labels[2], pad))
     return newlabels
 
-def filemerge(repo, mynode, orig, fcd, fco, fca, labels=None):
-    """perform a 3-way merge in the working directory
+class mergectx(object):
+    """perform a 3-way merge in the working directory"""
 
-    mynode = parent node before merge
-    orig = original local filename before merge
-    fco = other file context
-    fca = ancestor file context
-    fcd = local file context for current/destination file
-    """
+    def __init__(self, repo, mynode, orig, fcd, fco, fca, labels=None):
+        """
+        mynode = parent node before merge
+        orig = original local filename before merge
+        fco = other file context
+        fca = ancestor file context
+        fcd = local file context for current/destination file"""
+        self._repo = repo
+        self._mynode = mynode
+        self._orig = orig
+        self._fcd = fcd
+        self._fco = fco
+        self._fca = fca
+        self._labels = labels
 
-    if True:
+    def merge(self):
         def temp(prefix, ctx):
             pre = "%s~%s." % (os.path.basename(ctx.path()), prefix)
             (fd, name) = tempfile.mkstemp(prefix=pre)
@@ -451,6 +459,14 @@  def filemerge(repo, mynode, orig, fcd, f
             f.close()
             return name
 
+        repo = self._repo
+        mynode = self._mynode
+        orig = self._orig
+        fcd = self._fcd
+        fco = self._fco
+        fca = self._fca
+        labels = self._labels
+
         if not fco.cmp(fcd): # files identical?
             return None
 
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -308,8 +308,9 @@  class mergestate(object):
         f = self._repo.vfs('merge/' + hash)
         self._repo.wwrite(dfile, f.read(), flags)
         f.close()
-        r = filemerge.filemerge(self._repo, self._local, lfile, fcd, fco, fca,
-                                labels=labels)
+        ctx = filemerge.mergectx(self._repo, self._local, lfile, fcd, fco, fca,
+                                 labels=labels)
+        r = ctx.merge()
         if r is None:
             # no real conflict
             del self._state[dfile]