Patchwork [2,of,3,STABLE] context: ability to manipulate diff feature opt-ins

login
register
mail settings
Submitter Matt Mackall
Date July 20, 2015, 8:07 p.m.
Message ID <1437422876.19409.294.camel@selenic.com>
Download mbox | patch
Permalink /patch/10044/
State Not Applicable
Delegated to: Matt Mackall
Headers show

Comments

Matt Mackall - July 20, 2015, 8:07 p.m.
On Mon, 2015-07-20 at 20:18 +0900, Yuya Nishihara wrote:
> On Sat, 18 Jul 2015 14:24:30 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1437254506 25200
> > #      Sat Jul 18 14:21:46 2015 -0700
> > # Node ID b6ab3a470c0196159e8931c8c6dd4b5c4a52a4f6
> > # Parent  d88519184df5f9538b4f9294bee7a595d5dce6d2
> > context: ability to manipulate diff feature opt-ins
> > 
> > Before this patch, basectx.diff respected all diff options. Sometimes
> > consumers want to opt out of certain classes of diff features.
> > 
> > Change the function to call patch.difffeatureopts instead of
> > diffallopts and allow the various features to be opted out of via
> > arguments.
> > 
> > diff --git a/mercurial/context.py b/mercurial/context.py
> > --- a/mercurial/context.py
> > +++ b/mercurial/context.py
> > @@ -268,15 +268,23 @@ class basectx(object):
> >                                include, exclude, default,
> >                                auditor=r.auditor, ctx=self,
> >                                listsubrepos=listsubrepos, badfn=badfn)
> >  
> > -    def diff(self, ctx2=None, match=None, **opts):
> > -        """Returns a diff generator for the given contexts and matcher"""
> > +    def diff(self, ctx2=None, match=None, allowgit=True, allowwhitespace=True,
> > +             allowformatchanging=True, **opts):
> > +        """Returns a diff generator for the given contexts and matcher.
> > +
> > +        The various allow* arguments control whether the diff features under
> > +        that category are respected. See patch.difffeatureopts.
> > +        """
> >          if ctx2 is None:
> >              ctx2 = self.p1()
> >          if ctx2 is not None:
> >              ctx2 = self._repo[ctx2]
> > -        diffopts = patch.diffopts(self._repo.ui, opts)
> > +        diffopts = patch.difffeatureopts(self._repo.ui, opts=opts,
> > +                                         git=allowgit,
> > +                                         whitespace=allowwhitespace,
> > +                                         formatchanging=allowformatchanging)
> >          return patch.diff(self._repo, ctx2, self, match=match, opts=diffopts)
> 
> I'm noob about this API, but I think patch.diff*opts() exists to pack various
> diff-related options into one argument. It seems this change goes the opposite
> direction.

Yep. Also, it bears repeating: I really don't want _patch series_ on
stable. The explicit goal of stable is to maximize benefit/churn. If you
can't fix something in stable in one simple patch, it may be too much
churn. I don't see any reason why our diffstat code should be picky
about prefixes, so I think we should instead teach it that prefixes are
optional:


This patch may go a little too far since I just did a search and
replace, but we should probably also teach the patcher how to apply
prefixless patches.
Gregory Szorc - July 20, 2015, 9:13 p.m.
On Mon, Jul 20, 2015 at 1:07 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Mon, 2015-07-20 at 20:18 +0900, Yuya Nishihara wrote:
> > On Sat, 18 Jul 2015 14:24:30 -0700, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1437254506 25200
> > > #      Sat Jul 18 14:21:46 2015 -0700
> > > # Node ID b6ab3a470c0196159e8931c8c6dd4b5c4a52a4f6
> > > # Parent  d88519184df5f9538b4f9294bee7a595d5dce6d2
> > > context: ability to manipulate diff feature opt-ins
> > >
> > > Before this patch, basectx.diff respected all diff options. Sometimes
> > > consumers want to opt out of certain classes of diff features.
> > >
> > > Change the function to call patch.difffeatureopts instead of
> > > diffallopts and allow the various features to be opted out of via
> > > arguments.
> > >
> > > diff --git a/mercurial/context.py b/mercurial/context.py
> > > --- a/mercurial/context.py
> > > +++ b/mercurial/context.py
> > > @@ -268,15 +268,23 @@ class basectx(object):
> > >                                include, exclude, default,
> > >                                auditor=r.auditor, ctx=self,
> > >                                listsubrepos=listsubrepos, badfn=badfn)
> > >
> > > -    def diff(self, ctx2=None, match=None, **opts):
> > > -        """Returns a diff generator for the given contexts and
> matcher"""
> > > +    def diff(self, ctx2=None, match=None, allowgit=True,
> allowwhitespace=True,
> > > +             allowformatchanging=True, **opts):
> > > +        """Returns a diff generator for the given contexts and
> matcher.
> > > +
> > > +        The various allow* arguments control whether the diff
> features under
> > > +        that category are respected. See patch.difffeatureopts.
> > > +        """
> > >          if ctx2 is None:
> > >              ctx2 = self.p1()
> > >          if ctx2 is not None:
> > >              ctx2 = self._repo[ctx2]
> > > -        diffopts = patch.diffopts(self._repo.ui, opts)
> > > +        diffopts = patch.difffeatureopts(self._repo.ui, opts=opts,
> > > +                                         git=allowgit,
> > > +                                         whitespace=allowwhitespace,
> > > +
>  formatchanging=allowformatchanging)
> > >          return patch.diff(self._repo, ctx2, self, match=match,
> opts=diffopts)
> >
> > I'm noob about this API, but I think patch.diff*opts() exists to pack
> various
> > diff-related options into one argument. It seems this change goes the
> opposite
> > direction.
>
> Yep. Also, it bears repeating: I really don't want _patch series_ on
> stable. The explicit goal of stable is to maximize benefit/churn. If you
> can't fix something in stable in one simple patch, it may be too much
> churn. I don't see any reason why our diffstat code should be picky
> about prefixes, so I think we should instead teach it that prefixes are
> optional:
>

Sorry. I found this last week and I considered it stable worthy because it
prevents bustage of {diffstat}. I went with the more involved fix because,
well, it felt more proper than a hack that would only get refactored post
3.5 anyway.


>
> diff -r f8aead51aec0 mercurial/patch.py
> --- a/mercurial/patch.py        Sun Jul 19 18:11:18 2015 +0200
> +++ b/mercurial/patch.py        Mon Jul 20 15:02:19 2015 -0500
> @@ -15,7 +15,7 @@
>  import base85, mdiff, scmutil, util, diffhelpers, copies, encoding, error
>  import pathutil
>
> -gitre = re.compile('diff --git a/(.*) b/(.*)')
> +gitre = re.compile('diff --git (?:a/)(.*) (?:b/)(.*)')
>  tabsplitter = re.compile(r'(\t+|[^\t]+)')
>
>  class PatchError(Exception):
> @@ -324,7 +324,7 @@
>      gitpatches = []
>      for line in lr:
>          line = line.rstrip(' \r\n')
> -        if line.startswith('diff --git a/'):
> +        if line.startswith('diff --git '):
>              m = gitre.match(line)
>              if m:
>                  if gp:
> @@ -814,7 +814,7 @@
>  class header(object):
>      """patch header
>      """
> -    diffgit_re = re.compile('diff --git a/(.*) b/(.*)$')
> +    diffgit_re = re.compile('diff --git (?:a/)(.*) (?:b/)(.*)$')
>      diff_re = re.compile('diff -r .* (.*)$')
>      allhunks_re = re.compile('(?:index|deleted file) ')
>      pretty_re = re.compile('(?:new file|deleted file) ')
> @@ -1752,7 +1752,7 @@
>                  emitfile = False
>                  yield 'file', (afile, bfile, h, gp and gp.copy() or None)
>              yield 'hunk', h
> -        elif x.startswith('diff --git a/'):
> +        elif x.startswith('diff --git '):
>              m = gitre.match(x.rstrip(' \r\n'))
>              if not m:
>                  continue
> @@ -2476,7 +2476,7 @@
>              addresult()
>              # set numbers to 0 anyway when starting new file
>              adds, removes, isbinary = 0, 0, False
> -            if line.startswith('diff --git a/'):
> +            if line.startswith('diff --git '):
>                  filename = gitre.search(line).group(2)
>              elif line.startswith('diff -r'):
>                  # format: "diff -r ... -r ... filename"
>
> This patch may go a little too far since I just did a search and
> replace, but we should probably also teach the patcher how to apply
> prefixless patches.
>

Unfortunately, this patch breaks on filenames with spaces. (This is also a
deficiency of the Git patch format.)
Matt Mackall - July 21, 2015, 7:06 p.m.
On Mon, 2015-07-20 at 14:13 -0700, Gregory Szorc wrote:
> On Mon, Jul 20, 2015 at 1:07 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> > On Mon, 2015-07-20 at 20:18 +0900, Yuya Nishihara wrote:
> > > On Sat, 18 Jul 2015 14:24:30 -0700, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > > # Date 1437254506 25200
> > > > #      Sat Jul 18 14:21:46 2015 -0700
> > > > # Node ID b6ab3a470c0196159e8931c8c6dd4b5c4a52a4f6
> > > > # Parent  d88519184df5f9538b4f9294bee7a595d5dce6d2
> > > > context: ability to manipulate diff feature opt-ins
> > > >
> > > > Before this patch, basectx.diff respected all diff options. Sometimes
> > > > consumers want to opt out of certain classes of diff features.
> > > >
> > > > Change the function to call patch.difffeatureopts instead of
> > > > diffallopts and allow the various features to be opted out of via
> > > > arguments.
> > > >
> > > > diff --git a/mercurial/context.py b/mercurial/context.py
> > > > --- a/mercurial/context.py
> > > > +++ b/mercurial/context.py
> > > > @@ -268,15 +268,23 @@ class basectx(object):
> > > >                                include, exclude, default,
> > > >                                auditor=r.auditor, ctx=self,
> > > >                                listsubrepos=listsubrepos, badfn=badfn)
> > > >
> > > > -    def diff(self, ctx2=None, match=None, **opts):
> > > > -        """Returns a diff generator for the given contexts and
> > matcher"""
> > > > +    def diff(self, ctx2=None, match=None, allowgit=True,
> > allowwhitespace=True,
> > > > +             allowformatchanging=True, **opts):
> > > > +        """Returns a diff generator for the given contexts and
> > matcher.
> > > > +
> > > > +        The various allow* arguments control whether the diff
> > features under
> > > > +        that category are respected. See patch.difffeatureopts.
> > > > +        """
> > > >          if ctx2 is None:
> > > >              ctx2 = self.p1()
> > > >          if ctx2 is not None:
> > > >              ctx2 = self._repo[ctx2]
> > > > -        diffopts = patch.diffopts(self._repo.ui, opts)
> > > > +        diffopts = patch.difffeatureopts(self._repo.ui, opts=opts,
> > > > +                                         git=allowgit,
> > > > +                                         whitespace=allowwhitespace,
> > > > +
> >  formatchanging=allowformatchanging)
> > > >          return patch.diff(self._repo, ctx2, self, match=match,
> > opts=diffopts)
> > >
> > > I'm noob about this API, but I think patch.diff*opts() exists to pack
> > various
> > > diff-related options into one argument. It seems this change goes the
> > opposite
> > > direction.
> >
> > Yep. Also, it bears repeating: I really don't want _patch series_ on
> > stable. The explicit goal of stable is to maximize benefit/churn. If you
> > can't fix something in stable in one simple patch, it may be too much
> > churn. I don't see any reason why our diffstat code should be picky
> > about prefixes, so I think we should instead teach it that prefixes are
> > optional:
> >
> 
> Sorry. I found this last week and I considered it stable worthy because it
> prevents bustage of {diffstat}.

I'm not saying fixing this issue is not stable-worthy. I'm saying any
sort of significant moving of stuff around to make the fix pretty is not
how we do things in stable. We only care about user benefit/churn and
"pretty" is bad for that equation.

> Unfortunately, this patch breaks on filenames with spaces. (This is also a
> deficiency of the Git patch format.)

Do we not have any of those in the test suite? Because my change passes
tests. But also, if we're generating git diffs that aren't parseable by
git.. we have a bigger problem.
Pierre-Yves David - July 29, 2015, 8:14 p.m.
On 07/21/2015 12:06 PM, Matt Mackall wrote:
> On Mon, 2015-07-20 at 14:13 -0700, Gregory Szorc wrote:
>> On Mon, Jul 20, 2015 at 1:07 PM, Matt Mackall <mpm@selenic.com> wrote:
>>
>>> On Mon, 2015-07-20 at 20:18 +0900, Yuya Nishihara wrote:
>>>> On Sat, 18 Jul 2015 14:24:30 -0700, Gregory Szorc wrote:
>>>>> # HG changeset patch
>>>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>>>> # Date 1437254506 25200
>>>>> #      Sat Jul 18 14:21:46 2015 -0700
>>>>> # Node ID b6ab3a470c0196159e8931c8c6dd4b5c4a52a4f6
>>>>> # Parent  d88519184df5f9538b4f9294bee7a595d5dce6d2
>>>>> context: ability to manipulate diff feature opt-ins
>>>>>
>>>>> Before this patch, basectx.diff respected all diff options. Sometimes
>>>>> consumers want to opt out of certain classes of diff features.
>>>>>
>>>>> Change the function to call patch.difffeatureopts instead of
>>>>> diffallopts and allow the various features to be opted out of via
>>>>> arguments.
>>>>>
>>>>> diff --git a/mercurial/context.py b/mercurial/context.py
>>>>> --- a/mercurial/context.py
>>>>> +++ b/mercurial/context.py
>>>>> @@ -268,15 +268,23 @@ class basectx(object):
>>>>>                                 include, exclude, default,
>>>>>                                 auditor=r.auditor, ctx=self,
>>>>>                                 listsubrepos=listsubrepos, badfn=badfn)
>>>>>
>>>>> -    def diff(self, ctx2=None, match=None, **opts):
>>>>> -        """Returns a diff generator for the given contexts and
>>> matcher"""
>>>>> +    def diff(self, ctx2=None, match=None, allowgit=True,
>>> allowwhitespace=True,
>>>>> +             allowformatchanging=True, **opts):
>>>>> +        """Returns a diff generator for the given contexts and
>>> matcher.
>>>>> +
>>>>> +        The various allow* arguments control whether the diff
>>> features under
>>>>> +        that category are respected. See patch.difffeatureopts.
>>>>> +        """
>>>>>           if ctx2 is None:
>>>>>               ctx2 = self.p1()
>>>>>           if ctx2 is not None:
>>>>>               ctx2 = self._repo[ctx2]
>>>>> -        diffopts = patch.diffopts(self._repo.ui, opts)
>>>>> +        diffopts = patch.difffeatureopts(self._repo.ui, opts=opts,
>>>>> +                                         git=allowgit,
>>>>> +                                         whitespace=allowwhitespace,
>>>>> +
>>>   formatchanging=allowformatchanging)
>>>>>           return patch.diff(self._repo, ctx2, self, match=match,
>>> opts=diffopts)
>>>>
>>>> I'm noob about this API, but I think patch.diff*opts() exists to pack
>>> various
>>>> diff-related options into one argument. It seems this change goes the
>>> opposite
>>>> direction.
>>>
>>> Yep. Also, it bears repeating: I really don't want _patch series_ on
>>> stable. The explicit goal of stable is to maximize benefit/churn. If you
>>> can't fix something in stable in one simple patch, it may be too much
>>> churn. I don't see any reason why our diffstat code should be picky
>>> about prefixes, so I think we should instead teach it that prefixes are
>>> optional:
>>>
>>
>> Sorry. I found this last week and I considered it stable worthy because it
>> prevents bustage of {diffstat}.
>
> I'm not saying fixing this issue is not stable-worthy. I'm saying any
> sort of significant moving of stuff around to make the fix pretty is not
> how we do things in stable. We only care about user benefit/churn and
> "pretty" is bad for that equation.
>
>> Unfortunately, this patch breaks on filenames with spaces. (This is also a
>> deficiency of the Git patch format.)
>
> Do we not have any of those in the test suite? Because my change passes
> tests. But also, if we're generating git diffs that aren't parseable by
> git.. we have a bigger problem.

So, what is the general state of this ?

1) did you had a stable worthy issue?
2) if so was it fixed on stable or should we expect a fix in the next 
48h? (urg:-/)
3) What should I mark this series on patchwork?
Gregory Szorc - July 30, 2015, 3:57 a.m.
On Wed, Jul 29, 2015 at 1:14 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 07/21/2015 12:06 PM, Matt Mackall wrote:
>
>> On Mon, 2015-07-20 at 14:13 -0700, Gregory Szorc wrote:
>>
>>> On Mon, Jul 20, 2015 at 1:07 PM, Matt Mackall <mpm@selenic.com> wrote:
>>>
>>>  On Mon, 2015-07-20 at 20:18 +0900, Yuya Nishihara wrote:
>>>>
>>>>> On Sat, 18 Jul 2015 14:24:30 -0700, Gregory Szorc wrote:
>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>>>>>
>>>>>> # Date 1437254506 25200
>>>>>> #      Sat Jul 18 14:21:46 2015 -0700
>>>>>> # Node ID b6ab3a470c0196159e8931c8c6dd4b5c4a52a4f6
>>>>>> # Parent  d88519184df5f9538b4f9294bee7a595d5dce6d2
>>>>>> context: ability to manipulate diff feature opt-ins
>>>>>>
>>>>>> Before this patch, basectx.diff respected all diff options. Sometimes
>>>>>> consumers want to opt out of certain classes of diff features.
>>>>>>
>>>>>> Change the function to call patch.difffeatureopts instead of
>>>>>> diffallopts and allow the various features to be opted out of via
>>>>>> arguments.
>>>>>>
>>>>>> diff --git a/mercurial/context.py b/mercurial/context.py
>>>>>> --- a/mercurial/context.py
>>>>>> +++ b/mercurial/context.py
>>>>>> @@ -268,15 +268,23 @@ class basectx(object):
>>>>>>                                 include, exclude, default,
>>>>>>                                 auditor=r.auditor, ctx=self,
>>>>>>                                 listsubrepos=listsubrepos,
>>>>>> badfn=badfn)
>>>>>>
>>>>>> -    def diff(self, ctx2=None, match=None, **opts):
>>>>>> -        """Returns a diff generator for the given contexts and
>>>>>>
>>>>> matcher"""
>>>>
>>>>> +    def diff(self, ctx2=None, match=None, allowgit=True,
>>>>>>
>>>>> allowwhitespace=True,
>>>>
>>>>> +             allowformatchanging=True, **opts):
>>>>>> +        """Returns a diff generator for the given contexts and
>>>>>>
>>>>> matcher.
>>>>
>>>>> +
>>>>>> +        The various allow* arguments control whether the diff
>>>>>>
>>>>> features under
>>>>
>>>>> +        that category are respected. See patch.difffeatureopts.
>>>>>> +        """
>>>>>>           if ctx2 is None:
>>>>>>               ctx2 = self.p1()
>>>>>>           if ctx2 is not None:
>>>>>>               ctx2 = self._repo[ctx2]
>>>>>> -        diffopts = patch.diffopts(self._repo.ui, opts)
>>>>>> +        diffopts = patch.difffeatureopts(self._repo.ui, opts=opts,
>>>>>> +                                         git=allowgit,
>>>>>> +                                         whitespace=allowwhitespace,
>>>>>> +
>>>>>>
>>>>>   formatchanging=allowformatchanging)
>>>>
>>>>>           return patch.diff(self._repo, ctx2, self, match=match,
>>>>>>
>>>>> opts=diffopts)
>>>>
>>>>>
>>>>> I'm noob about this API, but I think patch.diff*opts() exists to pack
>>>>>
>>>> various
>>>>
>>>>> diff-related options into one argument. It seems this change goes the
>>>>>
>>>> opposite
>>>>
>>>>> direction.
>>>>>
>>>>
>>>> Yep. Also, it bears repeating: I really don't want _patch series_ on
>>>> stable. The explicit goal of stable is to maximize benefit/churn. If you
>>>> can't fix something in stable in one simple patch, it may be too much
>>>> churn. I don't see any reason why our diffstat code should be picky
>>>> about prefixes, so I think we should instead teach it that prefixes are
>>>> optional:
>>>>
>>>>
>>> Sorry. I found this last week and I considered it stable worthy because
>>> it
>>> prevents bustage of {diffstat}.
>>>
>>
>> I'm not saying fixing this issue is not stable-worthy. I'm saying any
>> sort of significant moving of stuff around to make the fix pretty is not
>> how we do things in stable. We only care about user benefit/churn and
>> "pretty" is bad for that equation.
>>
>>  Unfortunately, this patch breaks on filenames with spaces. (This is also
>>> a
>>> deficiency of the Git patch format.)
>>>
>>
>> Do we not have any of those in the test suite? Because my change passes
>> tests. But also, if we're generating git diffs that aren't parseable by
>> git.. we have a bigger problem.
>>
>
> So, what is the general state of this ?
>
> 1) did you had a stable worthy issue?
> 2) if so was it fixed on stable or should we expect a fix in the next 48h?
> (urg:-/)
> 3) What should I mark this series on patchwork?


While I think this bug fix is stable worthy, I have little to no
inclination to restructure this in the next 48 hours. I'm not exactly sure
what qualifies as a stable-appropriate patch and I'd rather not incur the
extra work to refactor this before and after 3.5. I'll just send again
after the freeze. Although, it sounds like there may be more discussion
about the general approach. So I dunno.
Matt Mackall - July 30, 2015, 9:56 p.m.
On Wed, 2015-07-29 at 20:57 -0700, Gregory Szorc wrote:
> On Wed, Jul 29, 2015 at 1:14 PM, Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
> 
> >
> >
> > On 07/21/2015 12:06 PM, Matt Mackall wrote:
> >
> >> On Mon, 2015-07-20 at 14:13 -0700, Gregory Szorc wrote:
> >>
> >>> On Mon, Jul 20, 2015 at 1:07 PM, Matt Mackall <mpm@selenic.com> wrote:
> >>>
> >>>  On Mon, 2015-07-20 at 20:18 +0900, Yuya Nishihara wrote:
> >>>>
> >>>>> On Sat, 18 Jul 2015 14:24:30 -0700, Gregory Szorc wrote:
> >>>>>
> >>>>>> # HG changeset patch
> >>>>>> # User Gregory Szorc <gregory.szorc@gmail.com>
> >>>>>>
> >>>>>> # Date 1437254506 25200
> >>>>>> #      Sat Jul 18 14:21:46 2015 -0700
> >>>>>> # Node ID b6ab3a470c0196159e8931c8c6dd4b5c4a52a4f6
> >>>>>> # Parent  d88519184df5f9538b4f9294bee7a595d5dce6d2
> >>>>>> context: ability to manipulate diff feature opt-ins
> >>>>>>
> >>>>>> Before this patch, basectx.diff respected all diff options. Sometimes
> >>>>>> consumers want to opt out of certain classes of diff features.
> >>>>>>
> >>>>>> Change the function to call patch.difffeatureopts instead of
> >>>>>> diffallopts and allow the various features to be opted out of via
> >>>>>> arguments.
> >>>>>>
> >>>>>> diff --git a/mercurial/context.py b/mercurial/context.py
> >>>>>> --- a/mercurial/context.py
> >>>>>> +++ b/mercurial/context.py
> >>>>>> @@ -268,15 +268,23 @@ class basectx(object):
> >>>>>>                                 include, exclude, default,
> >>>>>>                                 auditor=r.auditor, ctx=self,
> >>>>>>                                 listsubrepos=listsubrepos,
> >>>>>> badfn=badfn)
> >>>>>>
> >>>>>> -    def diff(self, ctx2=None, match=None, **opts):
> >>>>>> -        """Returns a diff generator for the given contexts and
> >>>>>>
> >>>>> matcher"""
> >>>>
> >>>>> +    def diff(self, ctx2=None, match=None, allowgit=True,
> >>>>>>
> >>>>> allowwhitespace=True,
> >>>>
> >>>>> +             allowformatchanging=True, **opts):
> >>>>>> +        """Returns a diff generator for the given contexts and
> >>>>>>
> >>>>> matcher.
> >>>>
> >>>>> +
> >>>>>> +        The various allow* arguments control whether the diff
> >>>>>>
> >>>>> features under
> >>>>
> >>>>> +        that category are respected. See patch.difffeatureopts.
> >>>>>> +        """
> >>>>>>           if ctx2 is None:
> >>>>>>               ctx2 = self.p1()
> >>>>>>           if ctx2 is not None:
> >>>>>>               ctx2 = self._repo[ctx2]
> >>>>>> -        diffopts = patch.diffopts(self._repo.ui, opts)
> >>>>>> +        diffopts = patch.difffeatureopts(self._repo.ui, opts=opts,
> >>>>>> +                                         git=allowgit,
> >>>>>> +                                         whitespace=allowwhitespace,
> >>>>>> +
> >>>>>>
> >>>>>   formatchanging=allowformatchanging)
> >>>>
> >>>>>           return patch.diff(self._repo, ctx2, self, match=match,
> >>>>>>
> >>>>> opts=diffopts)
> >>>>
> >>>>>
> >>>>> I'm noob about this API, but I think patch.diff*opts() exists to pack
> >>>>>
> >>>> various
> >>>>
> >>>>> diff-related options into one argument. It seems this change goes the
> >>>>>
> >>>> opposite
> >>>>
> >>>>> direction.
> >>>>>
> >>>>
> >>>> Yep. Also, it bears repeating: I really don't want _patch series_ on
> >>>> stable. The explicit goal of stable is to maximize benefit/churn. If you
> >>>> can't fix something in stable in one simple patch, it may be too much
> >>>> churn. I don't see any reason why our diffstat code should be picky
> >>>> about prefixes, so I think we should instead teach it that prefixes are
> >>>> optional:
> >>>>
> >>>>
> >>> Sorry. I found this last week and I considered it stable worthy because
> >>> it
> >>> prevents bustage of {diffstat}.
> >>>
> >>
> >> I'm not saying fixing this issue is not stable-worthy. I'm saying any
> >> sort of significant moving of stuff around to make the fix pretty is not
> >> how we do things in stable. We only care about user benefit/churn and
> >> "pretty" is bad for that equation.
> >>
> >>  Unfortunately, this patch breaks on filenames with spaces. (This is also
> >>> a
> >>> deficiency of the Git patch format.)
> >>>
> >>
> >> Do we not have any of those in the test suite? Because my change passes
> >> tests. But also, if we're generating git diffs that aren't parseable by
> >> git.. we have a bigger problem.
> >>
> >
> > So, what is the general state of this ?
> >
> > 1) did you had a stable worthy issue?
> > 2) if so was it fixed on stable or should we expect a fix in the next 48h?
> > (urg:-/)
> > 3) What should I mark this series on patchwork?
> 
> 
> While I think this bug fix is stable worthy, I have little to no
> inclination to restructure this in the next 48 hours. I'm not exactly sure
> what qualifies as a stable-appropriate patch and I'd rather not incur the
> extra work to refactor this before and after 3.5. I'll just send again
> after the freeze. Although, it sounds like there may be more discussion
> about the general approach. So I dunno.

(adding sid0)

We still have the important unanswered question:

"If the hack of allowing a/ to be optional in regexes isn't valid
because git diffs rely on them to handle filenames with spaces, then
don't we actually have to back out the change that allowed --no-prefix
with --git?"

Or put more directly:

"Do hg's --no-prefix --git diffs... actually work with git? If not, THAT
is the bug and the --stat issue is irrelevant."

Patch

diff -r f8aead51aec0 mercurial/patch.py
--- a/mercurial/patch.py	Sun Jul 19 18:11:18 2015 +0200
+++ b/mercurial/patch.py	Mon Jul 20 15:02:19 2015 -0500
@@ -15,7 +15,7 @@ 
 import base85, mdiff, scmutil, util, diffhelpers, copies, encoding, error
 import pathutil
 
-gitre = re.compile('diff --git a/(.*) b/(.*)')
+gitre = re.compile('diff --git (?:a/)(.*) (?:b/)(.*)')
 tabsplitter = re.compile(r'(\t+|[^\t]+)')
 
 class PatchError(Exception):
@@ -324,7 +324,7 @@ 
     gitpatches = []
     for line in lr:
         line = line.rstrip(' \r\n')
-        if line.startswith('diff --git a/'):
+        if line.startswith('diff --git '):
             m = gitre.match(line)
             if m:
                 if gp:
@@ -814,7 +814,7 @@ 
 class header(object):
     """patch header
     """
-    diffgit_re = re.compile('diff --git a/(.*) b/(.*)$')
+    diffgit_re = re.compile('diff --git (?:a/)(.*) (?:b/)(.*)$')
     diff_re = re.compile('diff -r .* (.*)$')
     allhunks_re = re.compile('(?:index|deleted file) ')
     pretty_re = re.compile('(?:new file|deleted file) ')
@@ -1752,7 +1752,7 @@ 
                 emitfile = False
                 yield 'file', (afile, bfile, h, gp and gp.copy() or None)
             yield 'hunk', h
-        elif x.startswith('diff --git a/'):
+        elif x.startswith('diff --git '):
             m = gitre.match(x.rstrip(' \r\n'))
             if not m:
                 continue
@@ -2476,7 +2476,7 @@ 
             addresult()
             # set numbers to 0 anyway when starting new file
             adds, removes, isbinary = 0, 0, False
-            if line.startswith('diff --git a/'):
+            if line.startswith('diff --git '):
                 filename = gitre.search(line).group(2)
             elif line.startswith('diff -r'):
                 # format: "diff -r ... -r ... filename"