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

[Matt Mackall]

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.

-- 
Mathematics is the supreme nostalgia of our time.