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

[Gregory Szorc]

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