Patchwork [3,of,4] diff: accept "dict" as diff option

login
register
mail settings
Submitter Boris Feld
Date May 24, 2018, 8:38 a.m.
Message ID <d76075d78a860ea2b3e0.1527151136@FB>
Download mbox | patch
Permalink /patch/31828/
State New
Headers show

Comments

Boris Feld - May 24, 2018, 8:38 a.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1526993961 -7200
#      Tue May 22 14:59:21 2018 +0200
# Node ID d76075d78a860ea2b3e01f3c05969cd0b8839f35
# Parent  2f2232e5f0b60a3dd591ea16dfffc3c0a050acdc
# EXP-Topic diff-cleanup
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d76075d78a86
diff: accept "dict" as diff option

Current callers of `context.diff` needs a simple way to pass diff options
(currently as a dictionary). To use it in more places we need to also support
the callers who already build a `diffopts` object. For convenience, we add
support for passing option either as a `diffopts` object or as a `dict`.
Yuya Nishihara - May 24, 2018, 1:43 p.m.
On Thu, 24 May 2018 10:38:56 +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1526993961 -7200
> #      Tue May 22 14:59:21 2018 +0200
> # Node ID d76075d78a860ea2b3e01f3c05969cd0b8839f35
> # Parent  2f2232e5f0b60a3dd591ea16dfffc3c0a050acdc
> # EXP-Topic diff-cleanup
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d76075d78a86
> diff: accept "dict" as diff option

> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -34,7 +34,6 @@ from . import (
>      patch,
>      pathutil,
>      phases,
> -    pycompat,
>      repoview,
>      revlog,
>      scmutil,
> @@ -302,9 +301,8 @@ class basectx(object):
>              ctx2 = self.p1()
>          if ctx2 is not None:
>              ctx2 = self._repo[ctx2]
> -        diffopts = patch.diffopts(self._repo.ui, pycompat.byteskwargs(opts))
>          return patch.diff(self._repo, ctx2, self, match=match, changes=changes,
> -                          opts=diffopts, losedatafn=losedatafn, prefix=prefix,
> +                          opts=opts, losedatafn=losedatafn, prefix=prefix,

I think this goes the opposite direction. We've introduced patch.diff*opts()
functions because initializing diffopts by patch.diffopts() tends to be buggy.

I'd rather force callers of ctx.diff() to build a diffopts object.
Boris Feld - June 15, 2018, 8:48 a.m.
On 24/05/2018 15:43, Yuya Nishihara wrote:
> On Thu, 24 May 2018 10:38:56 +0200, Boris Feld wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1526993961 -7200
>> #      Tue May 22 14:59:21 2018 +0200
>> # Node ID d76075d78a860ea2b3e01f3c05969cd0b8839f35
>> # Parent  2f2232e5f0b60a3dd591ea16dfffc3c0a050acdc
>> # EXP-Topic diff-cleanup
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r d76075d78a86
>> diff: accept "dict" as diff option
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -34,7 +34,6 @@ from . import (
>>       patch,
>>       pathutil,
>>       phases,
>> -    pycompat,
>>       repoview,
>>       revlog,
>>       scmutil,
>> @@ -302,9 +301,8 @@ class basectx(object):
>>               ctx2 = self.p1()
>>           if ctx2 is not None:
>>               ctx2 = self._repo[ctx2]
>> -        diffopts = patch.diffopts(self._repo.ui, pycompat.byteskwargs(opts))
>>           return patch.diff(self._repo, ctx2, self, match=match, changes=changes,
>> -                          opts=diffopts, losedatafn=losedatafn, prefix=prefix,
>> +                          opts=opts, losedatafn=losedatafn, prefix=prefix,
> I think this goes the opposite direction. We've introduced patch.diff*opts()
> functions because initializing diffopts by patch.diffopts() tends to be buggy.
Creating diffopts object requires the patch module. This creates a cycle 
in places where we would need it.

We have a slight preference on passing opts as dict as it would simplify 
callers code, but it's not a strong preference.
>
> I'd rather force callers of ctx.diff() to build a diffopts object.
Do you see a way to solves the import cycle?
Yuya Nishihara - June 15, 2018, 11:44 a.m.
On Fri, 15 Jun 2018 10:48:34 +0200, Boris FELD wrote:
> > I'd rather force callers of ctx.diff() to build a diffopts object.
> Do you see a way to solves the import cycle?

Which one?

Maybe we can move patch.diff*opts() functions to either mdiff or new module
if that's the case.
Boris FELD - June 17, 2018, 3:47 p.m.
On 15/06/2018 13:44, Yuya Nishihara wrote:
> On Fri, 15 Jun 2018 10:48:34 +0200, Boris FELD wrote:
>>> I'd rather force callers of ctx.diff() to build a diffopts object.
>> Do you see a way to solves the import cycle?
> Which one?

+  Import cycle: mercurial.obsutil -> mercurial.patch -> 
mercurial.scmutil -> mercurial.obsutil
+  Import cycle: mercurial.copies -> mercurial.scmutil -> 
mercurial.obsutil -> mercurial.patch -> mercurial.copies
+  Import cycle: mercurial.obsolete -> mercurial.obsutil -> 
mercurial.patch -> mercurial.scmutil -> mercurial.obsolete

>
> Maybe we can move patch.diff*opts() functions to either mdiff or new module
> if that's the case.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - June 18, 2018, 11:56 a.m.
On Sun, 17 Jun 2018 17:47:11 +0200, Boris FELD wrote:
> On 15/06/2018 13:44, Yuya Nishihara wrote:
> > On Fri, 15 Jun 2018 10:48:34 +0200, Boris FELD wrote:
> >>> I'd rather force callers of ctx.diff() to build a diffopts object.
> >> Do you see a way to solves the import cycle?
> > Which one?
> 
> +  Import cycle: mercurial.obsutil -> mercurial.patch -> 
> mercurial.scmutil -> mercurial.obsutil
> +  Import cycle: mercurial.copies -> mercurial.scmutil -> 
> mercurial.obsutil -> mercurial.patch -> mercurial.copies
> +  Import cycle: mercurial.obsolete -> mercurial.obsutil -> 
> mercurial.patch -> mercurial.scmutil -> mercurial.obsolete

IIUC, the cycle is caused by the following function call:

  obsolete.createmarkers() -> obsutil.geteffectflag() -> _cmpdiff()

and I think createmarkers() is more like a utility function than a storage
function. So, if we had a higher-level utility module for obsolete users,
which isn't imported from scmutil, maybe we can move createmarkers() and
geteffectflag() to that module. We'll also have to move scmutil.cleanupnodes()
to that module.

  <new utility module> -> scmutil / patch

Another option will be just delaying the import of the patch module, and add
TODO comment.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -34,7 +34,6 @@  from . import (
     patch,
     pathutil,
     phases,
-    pycompat,
     repoview,
     revlog,
     scmutil,
@@ -302,9 +301,8 @@  class basectx(object):
             ctx2 = self.p1()
         if ctx2 is not None:
             ctx2 = self._repo[ctx2]
-        diffopts = patch.diffopts(self._repo.ui, pycompat.byteskwargs(opts))
         return patch.diff(self._repo, ctx2, self, match=match, changes=changes,
-                          opts=diffopts, losedatafn=losedatafn, prefix=prefix,
+                          opts=opts, losedatafn=losedatafn, prefix=prefix,
                           relroot=relroot, copy=copy,
                           hunksfilterfn=hunksfilterfn)
 
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -2379,6 +2379,8 @@  def diffhunks(repo, node1=None, node2=No
 
     if opts is None:
         opts = mdiff.defaultopts
+    elif not isinstance(opts, mdiff.diffopts):
+        opts = diffopts(repo.ui, pycompat.byteskwargs(opts))
 
     if not node1 and not node2:
         node1 = repo.dirstate.p1()