Patchwork patch: accept non-node arguments to diff()

login
register
mail settings
Submitter Gregory Szorc
Date June 23, 2014, 5:26 a.m.
Message ID <f420697523a0ae7140e8.1403501180@77.1.168.192.in-addr.arpa>
Download mbox | patch
Permalink /patch/5043/
State Rejected
Headers show

Comments

Gregory Szorc - June 23, 2014, 5:26 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1403501129 25200
#      Sun Jun 22 22:25:29 2014 -0700
# Node ID f420697523a0ae7140e86ccc013e31682bcf4ee3
# Parent  cd3c79392056a0d965236e4986a7a4b5d580f3e5
patch: accept non-node arguments to diff()

patch.diff() appears to work with non-bin argument values to node1
and node2 because it feeds them into repo[]. However, this breaks
the actual generated patch because the arguments are always
hex-encoded. This may result in double hex encoding.

I discovered this as part of implementing an extension. I have no
clue if anything in the core is impacted. I doubt it. One less
footgun for extension authors.
Sean Farley - June 23, 2014, 3:37 p.m.
Gregory Szorc writes:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1403501129 25200
> #      Sun Jun 22 22:25:29 2014 -0700
> # Node ID f420697523a0ae7140e86ccc013e31682bcf4ee3
> # Parent  cd3c79392056a0d965236e4986a7a4b5d580f3e5
> patch: accept non-node arguments to diff()
>
> patch.diff() appears to work with non-bin argument values to node1
> and node2 because it feeds them into repo[]. However, this breaks
> the actual generated patch because the arguments are always
> hex-encoded. This may result in double hex encoding.
>
> I discovered this as part of implementing an extension. I have no
> clue if anything in the core is impacted. I doubt it. One less
> footgun for extension authors.
>
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -1620,9 +1620,9 @@ def diff(repo, node1=None, node2=None, m
>          return []
>  
>      revs = None
>      hexfunc = repo.ui.debugflag and hex or short
> -    revs = [hexfunc(node) for node in [node1, node2] if node]
> +    revs = [hexfunc(ctx.node()) for ctx in [ctx1, ctx2] if ctx.node()]

This will conflict with my patch series here:
http://www.selenic.com/pipermail/mercurial-devel/2014-June/059689.html
Matt Mackall - June 23, 2014, 6:45 p.m.
On Mon, 2014-06-23 at 10:37 -0500, Sean Farley wrote:
> Gregory Szorc writes:
> 
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1403501129 25200
> > #      Sun Jun 22 22:25:29 2014 -0700
> > # Node ID f420697523a0ae7140e86ccc013e31682bcf4ee3
> > # Parent  cd3c79392056a0d965236e4986a7a4b5d580f3e5
> > patch: accept non-node arguments to diff()
> >
> > patch.diff() appears to work with non-bin argument values to node1
> > and node2 because it feeds them into repo[]. However, this breaks
> > the actual generated patch because the arguments are always
> > hex-encoded. This may result in double hex encoding.
> >
> > I discovered this as part of implementing an extension. I have no
> > clue if anything in the core is impacted. I doubt it. One less
> > footgun for extension authors.
> >
> > diff --git a/mercurial/patch.py b/mercurial/patch.py
> > --- a/mercurial/patch.py
> > +++ b/mercurial/patch.py
> > @@ -1620,9 +1620,9 @@ def diff(repo, node1=None, node2=None, m
> >          return []
> >  
> >      revs = None
> >      hexfunc = repo.ui.debugflag and hex or short
> > -    revs = [hexfunc(node) for node in [node1, node2] if node]
> > +    revs = [hexfunc(ctx.node()) for ctx in [ctx1, ctx2] if ctx.node()]
> 
> This will conflict with my patch series here:
> http://www.selenic.com/pipermail/mercurial-devel/2014-June/059689.html

More importantly, your conflicting patch apparently also fixes the
problem.

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -1620,9 +1620,9 @@  def diff(repo, node1=None, node2=None, m
         return []
 
     revs = None
     hexfunc = repo.ui.debugflag and hex or short
-    revs = [hexfunc(node) for node in [node1, node2] if node]
+    revs = [hexfunc(ctx.node()) for ctx in [ctx1, ctx2] if ctx.node()]
 
     copy = {}
     if opts.git or opts.upgrade:
         copy = copies.pathcopies(ctx1, ctx2)