Patchwork cmdutil: pass node instead of ctx to diffordiffstat

login
register
mail settings
Submitter Durham Goode
Date Jan. 5, 2016, 12:20 a.m.
Message ID <ab7ae1e47b6d84189188.1451953240@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12512/
State Accepted
Headers show

Comments

Durham Goode - Jan. 5, 2016, 12:20 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1451952844 28800
#      Mon Jan 04 16:14:04 2016 -0800
# Node ID ab7ae1e47b6d8418918874d98e0bf4cf11acf13f
# Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
cmdutil: pass node instead of ctx to diffordiffstat

93bcc73df8d5 changed showpatch to use ctx's more, but it accidentally passed
prev as a context and node as a binary string, when both should be passed as
binary strings (since diffordiffstat tries to resolve them via repo[X]).

This affected hggit since the existing ctx belongs to the git overlay, but the
resolved context (from the repo[X] resolution) should belong to the main repo.
This broke a test because it tried to look in the git repo for data that didn't
exist.

This feels like a deeper issue in hggit somewhere, but the fix is here trivial and
obviously more correct
Sean Farley - Jan. 5, 2016, 12:33 a.m.
Durham Goode <durham@fb.com> writes:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1451952844 28800
> #      Mon Jan 04 16:14:04 2016 -0800
> # Node ID ab7ae1e47b6d8418918874d98e0bf4cf11acf13f
> # Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
> cmdutil: pass node instead of ctx to diffordiffstat
>
> 93bcc73df8d5 changed showpatch to use ctx's more, but it accidentally passed
> prev as a context and node as a binary string, when both should be passed as
> binary strings (since diffordiffstat tries to resolve them via repo[X]).
>
> This affected hggit since the existing ctx belongs to the git overlay, but the
> resolved context (from the repo[X] resolution) should belong to the main repo.
> This broke a test because it tried to look in the git repo for data that didn't
> exist.

Ah, thanks for fixing that! I had a debugger window opened just now
looking into it. Much obliged.
Yuya Nishihara - Jan. 5, 2016, 2:33 p.m.
On Mon, 4 Jan 2016 16:20:40 -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1451952844 28800
> #      Mon Jan 04 16:14:04 2016 -0800
> # Node ID ab7ae1e47b6d8418918874d98e0bf4cf11acf13f
> # Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
> cmdutil: pass node instead of ctx to diffordiffstat
> 
> 93bcc73df8d5 changed showpatch to use ctx's more, but it accidentally passed
> prev as a context and node as a binary string, when both should be passed as
> binary strings (since diffordiffstat tries to resolve them via repo[X]).
> 
> This affected hggit since the existing ctx belongs to the git overlay, but the
> resolved context (from the repo[X] resolution) should belong to the main repo.
> This broke a test because it tried to look in the git repo for data that didn't
> exist.
> 
> This feels like a deeper issue in hggit somewhere, but the fix is here trivial and
> obviously more correct
> 
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1306,7 +1306,7 @@ class changeset_printer(object):
>              diff = self.diffopts.get('patch')
>              diffopts = patch.diffallopts(self.ui, self.diffopts)
>              node = ctx.node()
> -            prev = ctx.p1()
> +            prev = ctx.p1().node()

Makes sense. Pushed to the clowncopter as per Sean's review. Thank you both.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1306,7 +1306,7 @@  class changeset_printer(object):
             diff = self.diffopts.get('patch')
             diffopts = patch.diffallopts(self.ui, self.diffopts)
             node = ctx.node()
-            prev = ctx.p1()
+            prev = ctx.p1().node()
             if stat:
                 diffordiffstat(self.ui, self.repo, diffopts, prev, node,
                                match=matchfn, stat=True)