Patchwork [STABLE] repoview: do not crash when localtags refers to non existing or hidden revisions

login
register
mail settings
Submitter Angel Ezquerra
Date June 30, 2014, 5:19 p.m.
Message ID <5d8dbdb8b15c5f33f1ac.1404148743@ubuntu>
Download mbox | patch
Permalink /patch/5083/
State Changes Requested
Headers show

Comments

Angel Ezquerra - June 30, 2014, 5:19 p.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1404042755 -7200
#      Sun Jun 29 13:52:35 2014 +0200
# Branch stable
# Node ID 5d8dbdb8b15c5f33f1acd8281e695af003013637
# Parent  a4b67bf1f0a5051736c14d9c13fae50fd5f5e464
repoview: do not crash when localtags refers to non existing or hidden revisions

This fixes a crash that may happen when using mercurial 3.0.x.

The _gethiddenblockers function assumed that the output of readlocaltags was a
dict mapping tags to of valid nodes. However this is not necessarily the case.
When a local tag pointing to a non existing or no reachable hidden revision was
found, many mercurial commands would crash.

There are two possible solutions:

1. Make sure that tags.readlocaltags() always returns a dict mapping tags to
valid nodes
2. Check the output of readlocaltags() where it is used

This revision does #2 although #1 is probably more correct. I did not do #1
because I am unsure of the performance implications of #1.

tags.readlocaltags() is only used twice: where I made this fix and in
localrepo._findtags(). The latter is called whenever the localrepo.tags()
method is called with a filtered changelog.

If #1 is fine from a performance point of view I could send a new patch that
implemented it.

We may also want to add a warning when this happens (although it might be
annoying to get that warning for every command, possibly even more than once per
command).
Matt Mackall - June 30, 2014, 8:18 p.m.
On Mon, 2014-06-30 at 19:19 +0200, Angel Ezquerra wrote:
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1404042755 -7200
> #      Sun Jun 29 13:52:35 2014 +0200
> # Branch stable
> # Node ID 5d8dbdb8b15c5f33f1acd8281e695af003013637
> # Parent  a4b67bf1f0a5051736c14d9c13fae50fd5f5e464
> repoview: do not crash when localtags refers to non existing or hidden revisions
> 
> This fixes a crash that may happen when using mercurial 3.0.x.
> 
> The _gethiddenblockers function assumed that the output of readlocaltags was a
> dict mapping tags to of valid nodes. However this is not necessarily the case.
> When a local tag pointing to a non existing or no reachable hidden revision was
> found, many mercurial commands would crash.
> 
> There are two possible solutions:
> 
> 1. Make sure that tags.readlocaltags() always returns a dict mapping tags to
> valid nodes
> 2. Check the output of readlocaltags() where it is used
> 
> This revision does #2 although #1 is probably more correct. I did not do #1
> because I am unsure of the performance implications of #1.
> 
> tags.readlocaltags() is only used twice: where I made this fix and in
> localrepo._findtags(). The latter is called whenever the localrepo.tags()
> method is called with a filtered changelog.
> 
> If #1 is fine from a performance point of view I could send a new patch that
> implemented it.

I'd go with #1. Let me know if you've got time to do it before
tomorrow's release.
Angel Ezquerra - June 30, 2014, 10:32 p.m.
On Mon, Jun 30, 2014 at 10:18 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Mon, 2014-06-30 at 19:19 +0200, Angel Ezquerra wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>> # Date 1404042755 -7200
>> #      Sun Jun 29 13:52:35 2014 +0200
>> # Branch stable
>> # Node ID 5d8dbdb8b15c5f33f1acd8281e695af003013637
>> # Parent  a4b67bf1f0a5051736c14d9c13fae50fd5f5e464
>> repoview: do not crash when localtags refers to non existing or hidden revisions
>>
>> This fixes a crash that may happen when using mercurial 3.0.x.
>>
>> The _gethiddenblockers function assumed that the output of readlocaltags was a
>> dict mapping tags to of valid nodes. However this is not necessarily the case.
>> When a local tag pointing to a non existing or no reachable hidden revision was
>> found, many mercurial commands would crash.
>>
>> There are two possible solutions:
>>
>> 1. Make sure that tags.readlocaltags() always returns a dict mapping tags to
>> valid nodes
>> 2. Check the output of readlocaltags() where it is used
>>
>> This revision does #2 although #1 is probably more correct. I did not do #1
>> because I am unsure of the performance implications of #1.
>>
>> tags.readlocaltags() is only used twice: where I made this fix and in
>> localrepo._findtags(). The latter is called whenever the localrepo.tags()
>> method is called with a filtered changelog.
>>
>> If #1 is fine from a performance point of view I could send a new patch that
>> implemented it.
>
> I'd go with #1. Let me know if you've got time to do it before
> tomorrow's release.

I rewrote the patch as you suggested, I added a test and now I am
running the test suite. If everything goes fine I'll send V2 shortly.

Cheers,

Angel

Patch

diff -r a4b67bf1f0a5 -r 5d8dbdb8b15c mercurial/repoview.py
--- a/mercurial/repoview.py	Wed Jun 25 14:50:48 2014 -0700
+++ b/mercurial/repoview.py	Sun Jun 29 13:52:35 2014 +0200
@@ -41,7 +41,14 @@ 
         tags = {}
         tagsmod.readlocaltags(repo.ui, repo, tags, {})
         if tags:
-            blockers.extend(cl.rev(t[0]) for t in tags.values())
+            validtagrevs = []
+            for t in tags.values():
+                try:
+                    validtagrevs.append(cl.rev(t[0]))
+                except LookupError:
+                    # Ignore invalid tags
+                    pass
+            blockers.extend(validtagrevs)
     return blockers
 
 def computehidden(repo):