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

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 1, 2015, 2:40 p.m.
Message ID <20151101234028.4d5f47a2af9d3ec3e4c121e7@tcha.org>
Download mbox | patch
Permalink /patch/11255/
State Changes Requested
Headers show

Comments

Yuya Nishihara - Nov. 1, 2015, 2:40 p.m.
On Sat, 31 Oct 2015 21:53:32 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@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.

Instead, can't we try hard to detect empty set in pair expression?
Matt Harbison - Nov. 1, 2015, 4:11 p.m.
On Sun, 01 Nov 2015 09:40:28 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sat, 31 Oct 2015 21:53:32 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@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.

Patch

--- 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]):