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

[Matt Mackall]

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 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.

(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."

-- 
Mathematics is the supreme nostalgia of our time.