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

[Laurent Charignon]

I am sending a V5 taking into account your comments, thanks.
> On Jun 1, 2015, at 1:25 PM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
> 
> 
> 
> 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

By testing manually, I see that:
- some removed line
+ some added line
leads as expected to
+some removed line
- some added line

I will add this test case to the test and the doctest

> 
> 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.
OK, good catch
> 
> 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.
OK
> 
>> +    >>> 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.

We present it to the user and it is the default behavior.
Minimal example:
$ echo "a\nb\nc" > a ; hg add a ; hg commit -m "base"
$ echo "1\na\n2\nb\n3\nc\n4" > a ;  hg commit -im "change" 
Will show you four hunks with overlapping context.
My understanding is that one line of context in between changes defines a hunks. What do you think is wrong here, the number of hunks? The amount of context?


Laurent
> 
> -- 
> Pierre-Yves David