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

[Martin von Zweigbergk]

On Wed, May 31, 2017 at 5:22 PM, Sean Farley <sean at farley.io> wrote:
> # HG changeset patch
> # User Sean Farley <sean at 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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel