[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