From patchwork Sun Nov  1 14:40:28 2015
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [STABLE] extdiff: abort if an empty revision is given with --patch
From: Yuya Nishihara <yuya@tcha.org>
X-Patchwork-Id: 11255
Message-Id: <20151101234028.4d5f47a2af9d3ec3e4c121e7@tcha.org>
To: Matt Harbison <mharbison72@gmail.com>
Cc: matt_harbison@yahoo.com, mercurial-devel@selenic.com
Date: Sun, 1 Nov 2015 23:40:28 +0900

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?

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