[PATCH STABLE] extdiff: abort if an empty revision is given with --patch

Matt Harbison mharbison72 at gmail.com
Sun Nov 1 10:11:03 CST 2015


On Sun, 01 Nov 2015 09:40:28 -0500, Yuya Nishihara <yuya at tcha.org> wrote:

> On Sat, 31 Oct 2015 21:53:32 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison at yahoo.com>
>> # Date 1446342346 14400
>> #      Sat Oct 31 21:45:46 2015 -0400
>> # Branch stable
>> # Node ID c735abc328fefe3d0695a8d82b68ee0030f0e5bc
>> # Parent  6474b64045fb83733d6f7b774bbc8b2869bd1fed
>> extdiff: abort if an empty revision is given with --patch
>>
>> I ran into this when I forgot --hidden to make a precursor visible, but  
>> it will
>> occur any time one --rev evaluates to an empty set.  The error message  
>> is
>> probably slightly confusing since it can now be seen when "--rev" is  
>> actually
>> specified twice, but this seems better than the previous behavior of  
>> comparing
>> the first --rev argument against itself.  Since the issue isn't  
>> necessarily a
>> missing --hidden, I'm hesitant to abort this separately and mention the  
>> --hidden
>> flag.
>>
>> The reason forgetting --hidden causes the revision to be duplicated is  
>> that
>> scmutil.revpair() will calculate a revrange that looks like this:
>>
>>     $ hg extdiff --patch --rev 'precursors(29204)' --rev 29204
>>     <addset <baseset+ []>, <baseset [29204]>>
>>
>> ... which has 29204 as its first and last entry.  Since multiple --rev  
>> args are
>> provided, this causes None to _not_ be returned as the second item in  
>> the tuple.
>>
>> The extdiff functionality without --patch probably could use similar  
>> logic, but
>> I've only ever run into this with --patch when using it to compare with  
>> a
>> precursor.  Given that nobody has ever complained about it, I'm  
>> hesitant to
>> change that this close to a release.
>>
>> diff --git a/hgext/extdiff.py b/hgext/extdiff.py
>> --- a/hgext/extdiff.py
>> +++ b/hgext/extdiff.py
>> @@ -150,7 +150,7 @@
>>      if opts.get('patch'):
>>          if subrepos:
>>              raise error.Abort(_('--patch cannot be used with  
>> --subrepos'))
>> -        if node2 is None:
>> +        if node2 is None or node1a == node2:
>>              raise error.Abort(_('--patch requires two revisions'))
>>      else:
>>          mod_a, add_a, rem_a = map(set, repo.status(node1a, node2,  
>> matcher,
>> diff --git a/tests/test-extdiff.t b/tests/test-extdiff.t
>> --- a/tests/test-extdiff.t
>> +++ b/tests/test-extdiff.t
>> @@ -70,6 +70,12 @@
>>    [1]
>>  #endif
>>
>> +Specifying an empty revision to --patch should abort.
>> +
>> +  $ hg extdiff --patch --rev 'ancestor()' --rev 1
>> +  abort: --patch requires two revisions
>> +  [255]
>
> It seems a little strange for me that "hg extdiff --patch -r1 -r1"  
> aborts.

Agreed, but I was hesitant to change a method used for other things, in  
case something was dependent on the old behavior.  Now that I looked, the  
only other uses are commands.diff and commands.status, so it doesn't seem  
as risky.

> Instead, can't we try hard to detect empty set in pair expression?
>
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -717,6 +717,9 @@ def revpair(repo, revs):
>     if first is None:
>          raise error.Abort(_('empty revision range'))
> +    if (first == second and len(revs) >= 2
> +        and not all(revrange(repo, [r]) for r in revs)):
> +        raise error.Abort(_('empty revision on one side of range'))
>     # if top-level is range expression, the result must always be a pair
>      if first == second and len(revs) == 1 and not _pairspec(revs[0]):

The whole test suite passed on Linux with this, so I'll steal this for a  
v2 and let mpm decide the risk/reward.


More information about the Mercurial-devel mailing list