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

[Gregory Szorc]

On Wed, Jul 29, 2015 at 1:14 PM, Pierre-Yves David <
pierre-yves.david at 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 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?


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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150729/7cceafb0/attachment.html>