Patchwork [5,of,5,relative-diff] patch.diff: add support for diffs relative to a subdirectory

login
register
mail settings
Submitter Siddharth Agarwal
Date March 17, 2015, 10:57 p.m.
Message ID <8229e988837bc3a89a26.1426633050@devbig136.prn2.facebook.com>
Download mbox | patch
Permalink /patch/8131/
State Superseded
Headers show

Comments

Siddharth Agarwal - March 17, 2015, 10:57 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1426624884 25200
#      Tue Mar 17 13:41:24 2015 -0700
# Node ID 8229e988837bc3a89a26b09378e8351e2cab0146
# Parent  5f0dfcf33d3c1c06856c9271f8006e9eb7898368
patch.diff: add support for diffs relative to a subdirectory

For now this implementation is pretty naive -- it filters out files right
before passing them into trydiff. In upcoming patches we'll add some more
smarts.
Martin von Zweigbergk - March 18, 2015, 3:46 a.m.
On Tue, Mar 17, 2015 at 4:00 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1426624884 25200
> #      Tue Mar 17 13:41:24 2015 -0700
> # Node ID 8229e988837bc3a89a26b09378e8351e2cab0146
> # Parent  5f0dfcf33d3c1c06856c9271f8006e9eb7898368
> patch.diff: add support for diffs relative to a subdirectory
>
> For now this implementation is pretty naive -- it filters out files right
> before passing them into trydiff. In upcoming patches we'll add some more
> smarts.
>
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -2034,7 +2034,7 @@
>      return mdiff.diffopts(**buildopts)
>
>  def diff(repo, node1=None, node2=None, match=None, changes=None,
> opts=None,
> -         losedatafn=None, prefix=''):
> +         losedatafn=None, prefix='', relroot=''):
>      '''yields diff of changes to files between two nodes, or node and
>      working directory.
>
> @@ -2051,7 +2051,9 @@
>
>      prefix is a filename prefix that is prepended to all filenames on
>      display (used for subrepos).
> -    '''
> +
> +    relroot, if not empty, must be normalized with a trailing /. Any match
> +    patterns that fall outside it will be ignored.'''
>
>      if opts is None:
>          opts = mdiff.defaultopts
> @@ -2095,9 +2097,21 @@
>      if opts.git or opts.upgrade:
>          copy = copies.pathcopies(ctx1, ctx2)
>
> +    if relroot is not None:
> +        # XXX this would ideally be done in the matcher rather than here
>

So why isn't it? I guess I'm missing an explanation for when relroot would
be used in the first place.


> +        def filterrel(l):
> +            return [f for f in l if f.startswith(relroot)]
> +        modified = filterrel(modified)
> +        added = filterrel(added)
> +        removed = filterrel(removed)
> +        # filter out copies where either side isn't inside the relative
> root
> +        copy = dict(((dst, src) for (dst, src) in copy.iteritems()
> +                     if dst.startswith(relroot)
> +                     and src.startswith(relroot)))
> +
>      def difffn(opts, losedata):
>          return trydiff(repo, revs, ctx1, ctx2, modified, added, removed,
> -                       copy, getfilectx, opts, losedata, prefix, '')
> +                       copy, getfilectx, opts, losedata, prefix, relroot)
>      if opts.upgrade and not opts.git:
>          try:
>              def losedata(fn):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - March 18, 2015, 4:10 a.m.
On 03/17/2015 08:46 PM, Martin von Zweigbergk wrote:
>
>     +    if relroot is not None:
>     +        # XXX this would ideally be done in the matcher rather
>     than here
>
>
> So why isn't it?

A patch I haven't sent out yet has more details, but supporting this in
the matcher in general is non-trivial. The matcher is generally designed
to support 'or'ing patterns, not 'and'ing them.

In a later patch I'll add an optimization for the match.always() case,
creating a new matcher where the only pattern is the relative root.

> I guess I'm missing an explanation for when relroot would be used in
> the first place.

relroot will be used to produce patches relative to a directory. e.g.

hg diff --relative mercurial

will produce a diff where the filenames are e.g. a/patch.py and b/patch.py.
Martin von Zweigbergk - March 18, 2015, 1:29 p.m.
On Tue, Mar 17, 2015 at 9:10 PM Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 03/17/2015 08:46 PM, Martin von Zweigbergk wrote:
> >
> >     +    if relroot is not None:
> >     +        # XXX this would ideally be done in the matcher rather
> >     than here
> >
> >
> > So why isn't it?
>
> A patch I haven't sent out yet has more details, but supporting this in
> the matcher in general is non-trivial. The matcher is generally designed
> to support 'or'ing patterns, not 'and'ing them.
>

Yep, I'll try to clean up some old patches that add support for that. They
only mutate an existing matcher. Is that good enough for your needs?

The first three patches look good to me. Can we wait with the rest until I
get those patches cleaned up? I think I should be able to do that today.


>
> In a later patch I'll add an optimization for the match.always() case,
> creating a new matcher where the only pattern is the relative root.
>

Actually, wouldn't that work with any pattern that doesn't already have an
"include"? I.e., can you just set "include=['$relroot']" if no includes
were passed on the command line?


>
> > I guess I'm missing an explanation for when relroot would be used in
> > the first place.
>
> relroot will be used to produce patches relative to a directory. e.g.
>
> hg diff --relative mercurial
>
> will produce a diff where the filenames are e.g. a/patch.py and b/patch.py.
>

And if there are paths outside "mercurial/", they are ignored? I found that
a little surprising. I see that git made the same decision. I would
probably have expected it to fail. What do people want this feature for?
Siddharth Agarwal - March 18, 2015, 4:58 p.m.
On 03/18/2015 06:29 AM, Martin von Zweigbergk wrote:
> Yep, I'll try to clean up some old patches that add support for that.
> They only mutate an existing matcher. Is that good enough for your needs?

In this case it would be more preferable to create a new matcher with
all the patterns of the existing matcher anded with some new ones.

> The first three patches look good to me. Can we wait with the rest
> until I get those patches cleaned up? I think I should be able to do
> that today.

OK.
Siddharth Agarwal - March 23, 2015, 2:39 a.m.
On 03/18/2015 06:29 AM, Martin von Zweigbergk wrote:
> And if there are paths outside "mercurial/", they are ignored?

Yeah. That's the same as saying hg diff mercurial/ so I don't think it's
surprising.

> I found that a little surprising. I see that git made the same
> decision. I would probably have expected it to fail. What do people
> want this feature for?

We're merging a bunch of repos into a single one (mirroring them as
subdirectories), and during the transition have patches that are
generated from the unified repo that need to be applied onto the
individual repos, and the other way round.

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -2034,7 +2034,7 @@ 
     return mdiff.diffopts(**buildopts)
 
 def diff(repo, node1=None, node2=None, match=None, changes=None, opts=None,
-         losedatafn=None, prefix=''):
+         losedatafn=None, prefix='', relroot=''):
     '''yields diff of changes to files between two nodes, or node and
     working directory.
 
@@ -2051,7 +2051,9 @@ 
 
     prefix is a filename prefix that is prepended to all filenames on
     display (used for subrepos).
-    '''
+
+    relroot, if not empty, must be normalized with a trailing /. Any match
+    patterns that fall outside it will be ignored.'''
 
     if opts is None:
         opts = mdiff.defaultopts
@@ -2095,9 +2097,21 @@ 
     if opts.git or opts.upgrade:
         copy = copies.pathcopies(ctx1, ctx2)
 
+    if relroot is not None:
+        # XXX this would ideally be done in the matcher rather than here
+        def filterrel(l):
+            return [f for f in l if f.startswith(relroot)]
+        modified = filterrel(modified)
+        added = filterrel(added)
+        removed = filterrel(removed)
+        # filter out copies where either side isn't inside the relative root
+        copy = dict(((dst, src) for (dst, src) in copy.iteritems()
+                     if dst.startswith(relroot)
+                     and src.startswith(relroot)))
+
     def difffn(opts, losedata):
         return trydiff(repo, revs, ctx1, ctx2, modified, added, removed,
-                       copy, getfilectx, opts, losedata, prefix, '')
+                       copy, getfilectx, opts, losedata, prefix, relroot)
     if opts.upgrade and not opts.git:
         try:
             def losedata(fn):