[PATCH V4] revert: add an experimental config to use inverted selection

[Pierre-Yves David]

On 06/01/2015 11:17 AM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon at fb.com>
> # Date 1432930312 25200
> #      Fri May 29 13:11:52 2015 -0700
> # Node ID c929901771de740fc7ffc8c8822dc288c119d9bb
> # Parent  6084926366b979e81e5dcc84fa595965d4c07883
> revert: add an experimental config to use inverted selection
>
> We had a discussion on the list about the interactive ui for revert. This patch
> adds a flag to allow people to test the second alternative (referred to as
> proposition 2 on the mailing list). It effectively inverts the signs in the
> diff displayed to match the output of hg diff. A test is added to show an
> example.
>
> By setting the following flag in the config, one can try proposition 2:
> [experimental]
>   revertalternateinteractivemode=True
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -3139,10 +3139,21 @@
>           diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
>           diffopts.nodates = True
>           diffopts.git = True
> -        diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts)
> +        reversehunks =  repo.ui.configbool('experimental',
> +                                           'revertalternateinteractivemode',
> +                                           False)
> +        if reversehunks:
> +            diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts)
> +        else:
> +            diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts)
>           originalchunks = patch.parsepatch(diff)
> +
>           try:
> +
>               chunks = recordfilter(repo.ui, originalchunks)
> +            if reversehunks:
> +                chunks = patch.reversehunks(chunks)
> +
>           except patch.PatchError, err:
>               raise util.Abort(_('error parsing patch: %s') % err)
>
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -1385,6 +1385,84 @@
>               return s
>       return s[:i]
>
> +def reversehunks(hunks):
> +    '''reverse the signs in the hunks given as argument
> +
> +    This function operates on hunks coming out of patch.filterpatch, that is
> +    a list of the form: [header1, hunk1, hunk2, header2...]. Example usage:
> +
> +    >>> rawpatch = """diff --git a/folder1/g b/folder1/g
> +    ... --- a/folder1/g
> +    ... +++ b/folder1/g
> +    ... @@ -1,7 +1,7 @@
> +    ... +firstline
> +    ...  c
> +    ...  1
> +    ...  2
> +    ... -3
> +    ... -4
> +    ...  5
> +    ...  d
> +    ... +lastline"""

You are still lacking tests for the most common for on patch:

- some removed line
+ some added line

(Testing it manually show that the inversion changes the sign in place 
leading to the unusual:

+ some removed line
- some added line

This is okay as long as we do not display this to the user. but we 
should probably document that.)


> +    After parsepatch hunks look like: [header1, header2, ...]:

This comment is seen as output by doctest and makes it fails.

As pointed in a previous review, I think you should drop most of them. 
If any comment is dimmed necessary you can use '>>> # blah' for it.

> +    >>> hunks = parsepatch(rawpatch)
> +
> +    After filterpatch hunks look like: [header1, hunk1, hunk2, header2...]
> +    We expand the list of headers by appending their respective hunks:
> +    >>> hunkscomingfromfilterpatch = []
> +    >>> for h in hunks:
> +    ...     hunkscomingfromfilterpatch.append(h)
> +    ...     hunkscomingfromfilterpatch.extend(h.hunks)
> +
> +    Actual hunk inversion:
> +    >>> reversedhunks = reversehunks(hunkscomingfromfilterpatch)
> +
> +    Printing the hunk and checking the result:
> +    >>> fp = cStringIO.StringIO()
> +    >>> for c in reversedhunks:
> +    ...      c.write(fp)
> +    >>> fp.seek(0)
> +    >>> reversedpatch = fp.read()
> +    >>> print reversedpatch
> +    diff --git a/folder1/g b/folder1/g
> +    --- a/folder1/g
> +    +++ b/folder1/g
> +    @@ -1,4 +1,3 @@
> +    -firstline
> +     c
> +     1
> +     2
> +    @@ -1,5 +2,7 @@
> +     c
> +     1
> +     2
> +    +3
> +    +4
> +     5
> +     d
> +    @@ -6,3 +5,2 @@
> +     5
> +     d
> +    -lastline
> +
> +    '''

I'm not sure why this single hunk get splitted into 3 with over lapping 
context. This is probably okay as long as we do not present that to the 
user, but we should document it.

-- 
Pierre-Yves David