Patchwork [1,of,3,ctx-cleanup] context: inline makememctx (API)

login
register
mail settings
Submitter Sean Farley
Date June 1, 2017, 12:22 a.m.
Message ID <9879720e90cf13e445d1.1496276526@1.0.0.127.in-addr.arpa>
Download mbox | patch
Permalink /patch/21108/
State Accepted
Headers show

Comments

Sean Farley - June 1, 2017, 12:22 a.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1494536055 25200
#      Thu May 11 13:54:15 2017 -0700
# Branch wctxds
# Node ID 9879720e90cf13e445d17fc22f53c071a22322d9
# Parent  60b3e8946da728c377a3a6aadb785ae308084614
context: inline makememctx (API)

I have always thought it weird that we have a helper method instead of
just using __init__. So, I ripped it out.
Augie Fackler - June 1, 2017, 2:23 p.m.
On Wed, May 31, 2017 at 05:22:06PM -0700, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1494536055 25200
> #      Thu May 11 13:54:15 2017 -0700
> # Branch wctxds
> # Node ID 9879720e90cf13e445d17fc22f53c071a22322d9
> # Parent  60b3e8946da728c377a3a6aadb785ae308084614
> context: inline makememctx (API)
>

[...]

> -    def __init__(self, repo, parents, text, files, filectxfn, user=None,
> -                 date=None, extra=None, editor=False):
> +    def __init__(self, repo, parents, text, files, filectxfn=None, user=None,
> +                 date=None, extra=None, branch=None, editor=False):
>          super(memctx, self).__init__(repo, text, user, date, extra)
>          self._rev = None
>          self._node = None
>          parents = [(p or nullid) for p in parents]
>          p1, p2 = parents
>          self._parents = [changectx(self._repo, p) for p in (p1, p2)]
>          files = sorted(set(files))
>          self._files = files
> +        if branch is not None:
> +            self._extra['branch'] = encoding.fromlocal(branch)
>          self.substate = {}
>
> -        # if store is not callable, wrap it in a function
> -        if not callable(filectxfn):
> +        if filectxfn is not None and isinstance(filectxfn, patch.filestore):
> +            def getfilectx(repo, memctx, path):

nit: I don't think this function need be nested. Could you send a
followup that moves it to be something like _defaultgetfilectx at
module level if I'm right?

> +                data, mode, copied = filectxfn.getfile(path)
> +                if data is None:
> +                    return None
> +                islink, isexec = mode
> +                return memfilectx(repo, path, data, islink=islink,
> +                                  isexec=isexec, copied=copied,
> +                                  memctx=memctx)
> +            self._filectxfn = getfilectx
> +        elif not callable(filectxfn):
> +            # if store is not callable, wrap it in a function
>              def getfilectx(repo, memctx, path):
>                  fctx = filectxfn[path]
>                  # this is weird but apparently we only keep track of one parent
>                  # (why not only store that instead of a tuple?)
>                  copied = fctx.renamed()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - June 1, 2017, 6:36 p.m.
Augie Fackler <raf@durin42.com> writes:

> On Wed, May 31, 2017 at 05:22:06PM -0700, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean@farley.io>
>> # Date 1494536055 25200
>> #      Thu May 11 13:54:15 2017 -0700
>> # Branch wctxds
>> # Node ID 9879720e90cf13e445d17fc22f53c071a22322d9
>> # Parent  60b3e8946da728c377a3a6aadb785ae308084614
>> context: inline makememctx (API)
>>
>
> [...]
>
>> -    def __init__(self, repo, parents, text, files, filectxfn, user=None,
>> -                 date=None, extra=None, editor=False):
>> +    def __init__(self, repo, parents, text, files, filectxfn=None, user=None,
>> +                 date=None, extra=None, branch=None, editor=False):
>>          super(memctx, self).__init__(repo, text, user, date, extra)
>>          self._rev = None
>>          self._node = None
>>          parents = [(p or nullid) for p in parents]
>>          p1, p2 = parents
>>          self._parents = [changectx(self._repo, p) for p in (p1, p2)]
>>          files = sorted(set(files))
>>          self._files = files
>> +        if branch is not None:
>> +            self._extra['branch'] = encoding.fromlocal(branch)
>>          self.substate = {}
>>
>> -        # if store is not callable, wrap it in a function
>> -        if not callable(filectxfn):
>> +        if filectxfn is not None and isinstance(filectxfn, patch.filestore):
>> +            def getfilectx(repo, memctx, path):
>
> nit: I don't think this function need be nested. Could you send a
> followup that moves it to be something like _defaultgetfilectx at
> module level if I'm right?

Sure, sounds good.
via Mercurial-devel - June 3, 2017, 5:05 a.m.
On Wed, May 31, 2017 at 5:22 PM, Sean Farley <sean@farley.io> wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1494536055 25200
> #      Thu May 11 13:54:15 2017 -0700
> # Branch wctxds
> # Node ID 9879720e90cf13e445d17fc22f53c071a22322d9
> # Parent  60b3e8946da728c377a3a6aadb785ae308084614
> context: inline makememctx (API)
>
> I have always thought it weird that we have a helper method instead of
> just using __init__. So, I ripped it out.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> index 59e548a..d3dc0c2 100644
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1107,15 +1107,17 @@ def tryimportone(ui, repo, hunk, parents
>                      raise error.Abort(str(e))
>                  if opts.get('exact'):
>                      editor = None
>                  else:
>                      editor = getcommiteditor(editform='import.bypass')
> -                memctx = context.makememctx(repo, (p1.node(), p2.node()),
> +                memctx = context.memctx(repo, (p1.node(), p2.node()),
>                                              message,
> -                                            user,
> -                                            date,
> -                                            branch, files, store,
> +                                            files=files,
> +                                            filectxfn=store,
> +                                            user=user,
> +                                            date=date,
> +                                            branch=branch,
>                                              editor=editor)

Fixing indentation in flight. Also adding line break after "repo, "
for consistency.

>                  n = memctx.commit()
>              finally:
>                  store.close()
>          if opts.get('exact') and nocommit:
> diff --git a/mercurial/context.py b/mercurial/context.py
> index e2994e7..af1a4ec 100644
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -387,28 +387,10 @@ class basectx(object):
>          for l in r:
>              l.sort()
>
>          return r
>
> -
> -def makememctx(repo, parents, text, user, date, branch, files, store,
> -               editor=None, extra=None):
> -    def getfilectx(repo, memctx, 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)
> -    if extra is None:
> -        extra = {}
> -    if branch:
> -        extra['branch'] = encoding.fromlocal(branch)
> -    ctx = memctx(repo, parents, text, files, getfilectx, user,
> -                 date, extra, editor)
> -    return ctx
> -
>  def _filterederror(repo, changeid):
>      """build an exception to be raised about a filtered changeid
>
>      This is extracted in a function to help extensions (eg: evolve) to
>      experiment with various message variants."""
> @@ -2048,24 +2030,36 @@ class memctx(committablectx):
>      # Mercurial <= 3.1 expects the filectxfn to raise IOError for missing files.
>      # Extensions that need to retain compatibility across Mercurial 3.1 can use
>      # this field to determine what to do in filectxfn.
>      _returnnoneformissingfiles = True
>
> -    def __init__(self, repo, parents, text, files, filectxfn, user=None,
> -                 date=None, extra=None, editor=False):
> +    def __init__(self, repo, parents, text, files, filectxfn=None, user=None,
> +                 date=None, extra=None, branch=None, editor=False):
>          super(memctx, self).__init__(repo, text, user, date, extra)
>          self._rev = None
>          self._node = None
>          parents = [(p or nullid) for p in parents]
>          p1, p2 = parents
>          self._parents = [changectx(self._repo, p) for p in (p1, p2)]
>          files = sorted(set(files))
>          self._files = files
> +        if branch is not None:
> +            self._extra['branch'] = encoding.fromlocal(branch)
>          self.substate = {}
>
> -        # if store is not callable, wrap it in a function
> -        if not callable(filectxfn):
> +        if filectxfn is not None and isinstance(filectxfn, patch.filestore):
> +            def getfilectx(repo, memctx, path):
> +                data, mode, copied = filectxfn.getfile(path)
> +                if data is None:
> +                    return None
> +                islink, isexec = mode
> +                return memfilectx(repo, path, data, islink=islink,
> +                                  isexec=isexec, copied=copied,
> +                                  memctx=memctx)
> +            self._filectxfn = getfilectx
> +        elif not callable(filectxfn):
> +            # if store is not callable, wrap it in a function
>              def getfilectx(repo, memctx, path):
>                  fctx = filectxfn[path]
>                  # this is weird but apparently we only keep track of one parent
>                  # (why not only store that instead of a tuple?)
>                  copied = fctx.renamed()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 3, 2017, 5:15 a.m.
On Wed, May 31, 2017 at 5:22 PM, Sean Farley <sean@farley.io> wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1494536055 25200
> #      Thu May 11 13:54:15 2017 -0700
> # Branch wctxds
> # Node ID 9879720e90cf13e445d17fc22f53c071a22322d9
> # Parent  60b3e8946da728c377a3a6aadb785ae308084614
> context: inline makememctx (API)
>
> I have always thought it weird that we have a helper method instead of
> just using __init__. So, I ripped it out.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> index 59e548a..d3dc0c2 100644
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1107,15 +1107,17 @@ def tryimportone(ui, repo, hunk, parents
>                      raise error.Abort(str(e))
>                  if opts.get('exact'):
>                      editor = None
>                  else:
>                      editor = getcommiteditor(editform='import.bypass')
> -                memctx = context.makememctx(repo, (p1.node(), p2.node()),
> +                memctx = context.memctx(repo, (p1.node(), p2.node()),
>                                              message,
> -                                            user,
> -                                            date,
> -                                            branch, files, store,
> +                                            files=files,
> +                                            filectxfn=store,
> +                                            user=user,
> +                                            date=date,
> +                                            branch=branch,
>                                              editor=editor)
>                  n = memctx.commit()
>              finally:
>                  store.close()
>          if opts.get('exact') and nocommit:
> diff --git a/mercurial/context.py b/mercurial/context.py
> index e2994e7..af1a4ec 100644
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -387,28 +387,10 @@ class basectx(object):
>          for l in r:
>              l.sort()
>
>          return r
>
> -
> -def makememctx(repo, parents, text, user, date, branch, files, store,
> -               editor=None, extra=None):
> -    def getfilectx(repo, memctx, 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)
> -    if extra is None:
> -        extra = {}
> -    if branch:
> -        extra['branch'] = encoding.fromlocal(branch)
> -    ctx = memctx(repo, parents, text, files, getfilectx, user,
> -                 date, extra, editor)
> -    return ctx
> -
>  def _filterederror(repo, changeid):
>      """build an exception to be raised about a filtered changeid
>
>      This is extracted in a function to help extensions (eg: evolve) to
>      experiment with various message variants."""
> @@ -2048,24 +2030,36 @@ class memctx(committablectx):
>      # Mercurial <= 3.1 expects the filectxfn to raise IOError for missing files.
>      # Extensions that need to retain compatibility across Mercurial 3.1 can use
>      # this field to determine what to do in filectxfn.
>      _returnnoneformissingfiles = True
>
> -    def __init__(self, repo, parents, text, files, filectxfn, user=None,
> -                 date=None, extra=None, editor=False):
> +    def __init__(self, repo, parents, text, files, filectxfn=None, user=None,
> +                 date=None, extra=None, branch=None, editor=False):
>          super(memctx, self).__init__(repo, text, user, date, extra)
>          self._rev = None
>          self._node = None
>          parents = [(p or nullid) for p in parents]
>          p1, p2 = parents
>          self._parents = [changectx(self._repo, p) for p in (p1, p2)]
>          files = sorted(set(files))
>          self._files = files
> +        if branch is not None:
> +            self._extra['branch'] = encoding.fromlocal(branch)
>          self.substate = {}
>
> -        # if store is not callable, wrap it in a function
> -        if not callable(filectxfn):
> +        if filectxfn is not None and isinstance(filectxfn, patch.filestore):
> +            def getfilectx(repo, memctx, path):
> +                data, mode, copied = filectxfn.getfile(path)
> +                if data is None:
> +                    return None
> +                islink, isexec = mode
> +                return memfilectx(repo, path, data, islink=islink,
> +                                  isexec=isexec, copied=copied,
> +                                  memctx=memctx)
> +            self._filectxfn = getfilectx
> +        elif not callable(filectxfn):
> +            # if store is not callable, wrap it in a function
>              def getfilectx(repo, memctx, path):
>                  fctx = filectxfn[path]

filectxfn can now be None. In that case, it looks like we end up here
(callable(None) is False). But None.__getitem__ is not going to make
sense, is it? Why did give filectxfn optional anyway?

Unless you're very quick to reply and tell me why I'm wrong, I'll drop
this patch for now so I can accept the others on top of it.

>                  # this is weird but apparently we only keep track of one parent
>                  # (why not only store that instead of a tuple?)
>                  copied = fctx.renamed()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - June 5, 2017, 5:48 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Wed, May 31, 2017 at 5:22 PM, Sean Farley <sean@farley.io> wrote:
>> # HG changeset patch
>> # User Sean Farley <sean@farley.io>
>> # Date 1494536055 25200
>> #      Thu May 11 13:54:15 2017 -0700
>> # Branch wctxds
>> # Node ID 9879720e90cf13e445d17fc22f53c071a22322d9
>> # Parent  60b3e8946da728c377a3a6aadb785ae308084614
>> context: inline makememctx (API)
>>
>> I have always thought it weird that we have a helper method instead of
>> just using __init__. So, I ripped it out.
>>
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> index 59e548a..d3dc0c2 100644
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -1107,15 +1107,17 @@ def tryimportone(ui, repo, hunk, parents
>>                      raise error.Abort(str(e))
>>                  if opts.get('exact'):
>>                      editor = None
>>                  else:
>>                      editor = getcommiteditor(editform='import.bypass')
>> -                memctx = context.makememctx(repo, (p1.node(), p2.node()),
>> +                memctx = context.memctx(repo, (p1.node(), p2.node()),
>>                                              message,
>> -                                            user,
>> -                                            date,
>> -                                            branch, files, store,
>> +                                            files=files,
>> +                                            filectxfn=store,
>> +                                            user=user,
>> +                                            date=date,
>> +                                            branch=branch,
>>                                              editor=editor)
>>                  n = memctx.commit()
>>              finally:
>>                  store.close()
>>          if opts.get('exact') and nocommit:
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> index e2994e7..af1a4ec 100644
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -387,28 +387,10 @@ class basectx(object):
>>          for l in r:
>>              l.sort()
>>
>>          return r
>>
>> -
>> -def makememctx(repo, parents, text, user, date, branch, files, store,
>> -               editor=None, extra=None):
>> -    def getfilectx(repo, memctx, 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)
>> -    if extra is None:
>> -        extra = {}
>> -    if branch:
>> -        extra['branch'] = encoding.fromlocal(branch)
>> -    ctx = memctx(repo, parents, text, files, getfilectx, user,
>> -                 date, extra, editor)
>> -    return ctx
>> -
>>  def _filterederror(repo, changeid):
>>      """build an exception to be raised about a filtered changeid
>>
>>      This is extracted in a function to help extensions (eg: evolve) to
>>      experiment with various message variants."""
>> @@ -2048,24 +2030,36 @@ class memctx(committablectx):
>>      # Mercurial <= 3.1 expects the filectxfn to raise IOError for missing files.
>>      # Extensions that need to retain compatibility across Mercurial 3.1 can use
>>      # this field to determine what to do in filectxfn.
>>      _returnnoneformissingfiles = True
>>
>> -    def __init__(self, repo, parents, text, files, filectxfn, user=None,
>> -                 date=None, extra=None, editor=False):
>> +    def __init__(self, repo, parents, text, files, filectxfn=None, user=None,
>> +                 date=None, extra=None, branch=None, editor=False):
>>          super(memctx, self).__init__(repo, text, user, date, extra)
>>          self._rev = None
>>          self._node = None
>>          parents = [(p or nullid) for p in parents]
>>          p1, p2 = parents
>>          self._parents = [changectx(self._repo, p) for p in (p1, p2)]
>>          files = sorted(set(files))
>>          self._files = files
>> +        if branch is not None:
>> +            self._extra['branch'] = encoding.fromlocal(branch)
>>          self.substate = {}
>>
>> -        # if store is not callable, wrap it in a function
>> -        if not callable(filectxfn):
>> +        if filectxfn is not None and isinstance(filectxfn, patch.filestore):
>> +            def getfilectx(repo, memctx, path):
>> +                data, mode, copied = filectxfn.getfile(path)
>> +                if data is None:
>> +                    return None
>> +                islink, isexec = mode
>> +                return memfilectx(repo, path, data, islink=islink,
>> +                                  isexec=isexec, copied=copied,
>> +                                  memctx=memctx)
>> +            self._filectxfn = getfilectx
>> +        elif not callable(filectxfn):
>> +            # if store is not callable, wrap it in a function
>>              def getfilectx(repo, memctx, path):
>>                  fctx = filectxfn[path]
>
> filectxfn can now be None. In that case, it looks like we end up here
> (callable(None) is False). But None.__getitem__ is not going to make
> sense, is it? Why did give filectxfn optional anyway?
>
> Unless you're very quick to reply and tell me why I'm wrong, I'll drop
> this patch for now so I can accept the others on top of it.

It's fine to drop; I'll fix it up this week (along with other feedback)
and resend.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
index 59e548a..d3dc0c2 100644
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1107,15 +1107,17 @@  def tryimportone(ui, repo, hunk, parents
                     raise error.Abort(str(e))
                 if opts.get('exact'):
                     editor = None
                 else:
                     editor = getcommiteditor(editform='import.bypass')
-                memctx = context.makememctx(repo, (p1.node(), p2.node()),
+                memctx = context.memctx(repo, (p1.node(), p2.node()),
                                             message,
-                                            user,
-                                            date,
-                                            branch, files, store,
+                                            files=files,
+                                            filectxfn=store,
+                                            user=user,
+                                            date=date,
+                                            branch=branch,
                                             editor=editor)
                 n = memctx.commit()
             finally:
                 store.close()
         if opts.get('exact') and nocommit:
diff --git a/mercurial/context.py b/mercurial/context.py
index e2994e7..af1a4ec 100644
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -387,28 +387,10 @@  class basectx(object):
         for l in r:
             l.sort()
 
         return r
 
-
-def makememctx(repo, parents, text, user, date, branch, files, store,
-               editor=None, extra=None):
-    def getfilectx(repo, memctx, 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)
-    if extra is None:
-        extra = {}
-    if branch:
-        extra['branch'] = encoding.fromlocal(branch)
-    ctx = memctx(repo, parents, text, files, getfilectx, user,
-                 date, extra, editor)
-    return ctx
-
 def _filterederror(repo, changeid):
     """build an exception to be raised about a filtered changeid
 
     This is extracted in a function to help extensions (eg: evolve) to
     experiment with various message variants."""
@@ -2048,24 +2030,36 @@  class memctx(committablectx):
     # Mercurial <= 3.1 expects the filectxfn to raise IOError for missing files.
     # Extensions that need to retain compatibility across Mercurial 3.1 can use
     # this field to determine what to do in filectxfn.
     _returnnoneformissingfiles = True
 
-    def __init__(self, repo, parents, text, files, filectxfn, user=None,
-                 date=None, extra=None, editor=False):
+    def __init__(self, repo, parents, text, files, filectxfn=None, user=None,
+                 date=None, extra=None, branch=None, editor=False):
         super(memctx, self).__init__(repo, text, user, date, extra)
         self._rev = None
         self._node = None
         parents = [(p or nullid) for p in parents]
         p1, p2 = parents
         self._parents = [changectx(self._repo, p) for p in (p1, p2)]
         files = sorted(set(files))
         self._files = files
+        if branch is not None:
+            self._extra['branch'] = encoding.fromlocal(branch)
         self.substate = {}
 
-        # if store is not callable, wrap it in a function
-        if not callable(filectxfn):
+        if filectxfn is not None and isinstance(filectxfn, patch.filestore):
+            def getfilectx(repo, memctx, path):
+                data, mode, copied = filectxfn.getfile(path)
+                if data is None:
+                    return None
+                islink, isexec = mode
+                return memfilectx(repo, path, data, islink=islink,
+                                  isexec=isexec, copied=copied,
+                                  memctx=memctx)
+            self._filectxfn = getfilectx
+        elif not callable(filectxfn):
+            # if store is not callable, wrap it in a function
             def getfilectx(repo, memctx, path):
                 fctx = filectxfn[path]
                 # this is weird but apparently we only keep track of one parent
                 # (why not only store that instead of a tuple?)
                 copied = fctx.renamed()