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

[Pierre-Yves David]

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 at 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 at 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?

-- 
Pierre-Yves David