Patchwork diff.noprefix no longer breaks {diffstat} (issue4755)

login
register
mail settings
Submitter via Mercurial-devel
Date Jan. 12, 2017, 12:10 p.m.
Message ID <M14D1YP5Br61saWo-xh-eCFvhq1OxO7vB00TCQyFu64qQZWQPrXHMpsgh654AbcPxjjPcko_3rulyal-r-CcQHuPrABGE4k5ewlD5fBDw9A=@protonmail.com>
Download mbox | patch
Permalink /patch/18211/
State Accepted
Headers show

Comments

via Mercurial-devel - Jan. 12, 2017, 12:10 p.m.
# HG changeset patch
# User Matthieu Laneuville <mlaneuville@protonmail.com>
# Date 1484222815 -32400
# Thu Jan 12 21:06:55 2017 +0900
# Node ID c0030823eee2d2f320fe4b7e9d8ec90d05485980
# Parent 493935e0327a2dca0987a702bf06fa89b62e6a5c
templatekw: force noprefix=False to insure diffstat consistency (issue4755)

The result of diffstatdata should not depend on having noprefix set or not, as
was reported in issue 4755. Forcing noprefix to false on call makes sure the
parser receives the diff in the correct format and returns the proper result.

Another way to fix this would have been to change the regular expressions in
path.diffstatdata(), but that would have introduced many unecessary special
cases.
Sean Farley - Jan. 13, 2017, 7:33 p.m.
Matthieu Laneuville via Mercurial-devel
<mercurial-devel@mercurial-scm.org> writes:

> # HG changeset patch
> # User Matthieu Laneuville <mlaneuville@protonmail.com>
> # Date 1484222815 -32400
> # Thu Jan 12 21:06:55 2017 +0900
> # Node ID c0030823eee2d2f320fe4b7e9d8ec90d05485980
> # Parent 493935e0327a2dca0987a702bf06fa89b62e6a5c
> templatekw: force noprefix=False to insure diffstat consistency (issue4755)
>
> The result of diffstatdata should not depend on having noprefix set or not, as
> was reported in issue 4755. Forcing noprefix to false on call makes sure the
> parser receives the diff in the correct format and returns the proper result.
>
> Another way to fix this would have been to change the regular expressions in
> path.diffstatdata(), but that would have introduced many unecessary special
> cases.

This reasoning and the test look good to me.
Gregory Szorc - Jan. 13, 2017, 9:05 p.m.
On Thu, Jan 12, 2017 at 4:10 AM, Matthieu Laneuville via Mercurial-devel <
mercurial-devel@mercurial-scm.org> wrote:

> # HG changeset patch
> # User Matthieu Laneuville <mlaneuville@protonmail.com>
> # Date 1484222815 -32400
> #      Thu Jan 12 21:06:55 2017 +0900
> # Node ID c0030823eee2d2f320fe4b7e9d8ec90d05485980
> # Parent  493935e0327a2dca0987a702bf06fa89b62e6a5c
> templatekw: force noprefix=False to insure diffstat consistency (issue4755)
>
> The result of diffstatdata should not depend on having noprefix set or
> not, as
> was reported in issue 4755. Forcing noprefix to false on call makes sure
> the
> parser receives the diff in the correct format and returns the proper
> result.
>
> Another way to fix this would have been to change the regular expressions
> in
> path.diffstatdata(), but that would have introduced many unecessary special
> cases.
>

For whoever reviews this, previous attempts at fixing this were at:

* https://www.mercurial-scm.org/pipermail/mercurial-devel/
2016-April/083448.html
* https://www.mercurial-scm.org/pipermail/mercurial-devel/2015
-December/077214.html

(The threads have interesting discussion.)

The approach in this patch seems right. I'm wondering why those of us that
tried before didn't see this solution. It seems so... obvious.


>
> diff -r 493935e0327a -r c0030823eee2 mercurial/templatekw.py
> --- a/mercurial/templatekw.py Tue Jan 10 06:59:31 2017 +0800
> +++ b/mercurial/templatekw.py Thu Jan 12 21:06:55 2017 +0900
> @@ -299,7 +299,7 @@
>      """String. Statistics of changes with the following format:
>      "modified files: +added/-removed lines"
>      """
> -    stats = patch.diffstatdata(util.iterlines(ctx.diff()))
> +    stats = patch.diffstatdata(util.iterlines(ctx.diff(noprefix=False)))
>      maxname, maxtotal, adds, removes, binary = patch.diffstatsum(stats)
>      return '%s: +%s/-%s' % (len(stats), adds, removes)
>
> diff -r 493935e0327a -r c0030823eee2 tests/test-import.t
> --- a/tests/test-import.t Tue Jan 10 06:59:31 2017 +0800
> +++ b/tests/test-import.t Thu Jan 12 21:06:55 2017 +0900
> @@ -1517,6 +1517,12 @@
>    |
>    o  initial [Babar] 2: +8/-0
>
> +Adding those config options should not change the output of diffstat.
> Bugfix #4755.
> +
> +  $ hg log -r . --template '{diffstat}\n'
> +  1: +1/-0
> +  $ hg log -r . --template '{diffstat}\n' --config diff.git=1 --config
> diff.noprefix=1
> +  1: +1/-0
>
> Importing with some success and some errors:
>
>
>
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
Yuya Nishihara - Jan. 14, 2017, 6:28 a.m.
On Fri, 13 Jan 2017 13:05:16 -0800, Gregory Szorc wrote:
> On Thu, Jan 12, 2017 at 4:10 AM, Matthieu Laneuville via Mercurial-devel <
> mercurial-devel@mercurial-scm.org> wrote:
> 
> > # HG changeset patch
> > # User Matthieu Laneuville <mlaneuville@protonmail.com>
> > # Date 1484222815 -32400
> > #      Thu Jan 12 21:06:55 2017 +0900
> > # Node ID c0030823eee2d2f320fe4b7e9d8ec90d05485980
> > # Parent  493935e0327a2dca0987a702bf06fa89b62e6a5c
> > templatekw: force noprefix=False to insure diffstat consistency (issue4755)
> >
> > The result of diffstatdata should not depend on having noprefix set or
> > not, as
> > was reported in issue 4755. Forcing noprefix to false on call makes sure
> > the
> > parser receives the diff in the correct format and returns the proper
> > result.
> >
> > Another way to fix this would have been to change the regular expressions
> > in
> > path.diffstatdata(), but that would have introduced many unecessary special
> > cases.
> >
> 
> For whoever reviews this, previous attempts at fixing this were at:
> 
> * https://www.mercurial-scm.org/pipermail/mercurial-devel/
> 2016-April/083448.html
> * https://www.mercurial-scm.org/pipermail/mercurial-devel/2015
> -December/077214.html
> 
> (The threads have interesting discussion.)
> 
> The approach in this patch seems right. I'm wondering why those of us that
> tried before didn't see this solution. It seems so... obvious.

We'd take the tri-state bool path for --no- flag option, e40343ce9c4c, per
cost/benefit ratio.

> > diff -r 493935e0327a -r c0030823eee2 mercurial/templatekw.py
> > --- a/mercurial/templatekw.py Tue Jan 10 06:59:31 2017 +0800
> > +++ b/mercurial/templatekw.py Thu Jan 12 21:06:55 2017 +0900
> > @@ -299,7 +299,7 @@
> >      """String. Statistics of changes with the following format:
> >      "modified files: +added/-removed lines"
> >      """
> > -    stats = patch.diffstatdata(util.iterlines(ctx.diff()))
> > +    stats = patch.diffstatdata(util.iterlines(ctx.diff(noprefix=False)))
> >      maxname, maxtotal, adds, removes, binary = patch.diffstatsum(stats)
> >      return '%s: +%s/-%s' % (len(stats), adds, removes)

Matthieu, the patch content appears to be totally corrupted because of your
mailer. Can you resend it by using patchbomb or pushgate?

https://www.mercurial-scm.org/wiki/ContributingChanges#Emailing_patches

Or maybe we can import the last attempt made by Piotr.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-April/083449.html
Augie Fackler - Jan. 14, 2017, 6:30 a.m.
> On Jan 12, 2017, at 7:10 AM, Matthieu Laneuville via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
> # HG changeset patch
> # User Matthieu Laneuville <mlaneuville@protonmail.com <mailto:mlaneuville@protonmail.com>>
> # Date 1484222815 -32400
> #      Thu Jan 12 21:06:55 2017 +0900
> # Node ID c0030823eee2d2f320fe4b7e9d8ec90d05485980
> # Parent  493935e0327a2dca0987a702bf06fa89b62e6a5c
> templatekw: force noprefix=False to insure diffstat consistency (issue4755)

Queued this, thanks.  Congratulations on your first Mercurial patch!

(It looks like your patch got some whitespace damage in-flight that required manual fixups, which suggests to me you might have pasted this into your mail client, which has an annoying tendency to break things. I recommend one of the methods listed on https://www.mercurial-scm.org/wiki/ContributingChanges for any future patches.)
Augie Fackler - Jan. 14, 2017, 6:33 a.m.
> On Jan 14, 2017, at 1:28 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>>> diff -r 493935e0327a -r c0030823eee2 mercurial/templatekw.py
>>> --- a/mercurial/templatekw.py Tue Jan 10 06:59:31 2017 +0800
>>> +++ b/mercurial/templatekw.py Thu Jan 12 21:06:55 2017 +0900
>>> @@ -299,7 +299,7 @@
>>>     """String. Statistics of changes with the following format:
>>>     "modified files: +added/-removed lines"
>>>     """
>>> -    stats = patch.diffstatdata(util.iterlines(ctx.diff()))
>>> +    stats = patch.diffstatdata(util.iterlines(ctx.diff(noprefix=False)))
>>>     maxname, maxtotal, adds, removes, binary = patch.diffstatsum(stats)
>>>     return '%s: +%s/-%s' % (len(stats), adds, removes)
> 
> Matthieu, the patch content appears to be totally corrupted because of your
> mailer. Can you resend it by using patchbomb or pushgate?

I managed to get it to apply with only minor surgery. No resend required.

Patch

diff -r 493935e0327a -r c0030823eee2 mercurial/templatekw.py
--- a/mercurial/templatekw.py Tue Jan 10 06:59:31 2017 +0800
+++ b/mercurial/templatekw.py Thu Jan 12 21:06:55 2017 +0900
@@ -299,7 +299,7 @@ 
"""String. Statistics of changes with the following format:
"modified files: +added/-removed lines"
"""
- stats = patch.diffstatdata(util.iterlines(ctx.diff()))
+ stats = patch.diffstatdata(util.iterlines(ctx.diff(noprefix=False)))
maxname, maxtotal, adds, removes, binary = patch.diffstatsum(stats)
return '%s: +%s/-%s' % (len(stats), adds, removes)

diff -r 493935e0327a -r c0030823eee2 tests/test-import.t
--- a/tests/test-import.t Tue Jan 10 06:59:31 2017 +0800
+++ b/tests/test-import.t Thu Jan 12 21:06:55 2017 +0900
@@ -1517,6 +1517,12 @@ 
|
o initial [Babar] 2: +8/-0

+Adding those config options should not change the output of diffstat. Bugfix #4755.
+
+ $ hg log -r . --template '{diffstat}\n'
+ 1: +1/-0
+ $ hg log -r . --template '{diffstat}\n' --config diff.git=1 --config diff.noprefix=1
+ 1: +1/-0

Importing with some success and some errors: