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

[Sean Farley]

Martin von Zweigbergk <martinvonz at google.com> writes:

> 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.

It's fine to drop; I'll fix it up this week (along with other feedback)
and resend.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170605/1e303b0d/attachment.sig>