Patchwork [STABLE] bookmarks: use try/except for accessing a node

login
register
mail settings
Submitter Sean Farley
Date March 19, 2014, 12:23 a.m.
Message ID <8d3d12401c38524417f1.1395188636@laptop.local>
Download mbox | patch
Permalink /patch/3981/
State Rejected
Headers show

Comments

Sean Farley - March 19, 2014, 12:23 a.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1395182192 18000
#      Tue Mar 18 17:36:32 2014 -0500
# Branch stable
# Node ID 8d3d12401c38524417f1151b817fd210645a0602
# Parent  03774a2b6991b451bde7095238fde9ce98380d28
bookmarks: use try/except for accessing a node

Previously, in bookmarks.compare a changeset node would be tested via 'node in
repo' but this is tricky. A value of 'True' could be returned even if the node
is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
remote bookmark points to changeset that has been obsoleted locally. Therefore,
we wrap the block in a try/except which handles catching the error.

Writing a test is also tricky because I only observed this in extension land,
so I put a comment to explain why we use try/except.
Gregory Szorc - March 19, 2014, 12:33 a.m.
On 3/18/2014 5:23 PM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1395182192 18000
> #      Tue Mar 18 17:36:32 2014 -0500
> # Branch stable
> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
> bookmarks: use try/except for accessing a node
> 
> Previously, in bookmarks.compare a changeset node would be tested via 'node in
> repo' but this is tricky. A value of 'True' could be returned even if the node
> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
> remote bookmark points to changeset that has been obsoleted locally. Therefore,
> we wrap the block in a try/except which handles catching the error.
> 
> Writing a test is also tricky because I only observed this in extension land,
> so I put a comment to explain why we use try/except.

I /think/ I was poking at the same problem a week or two ago. I was able
to get my local repo to hit a RepoLookupError but was unable to write a
test case to reproduce.

I worked around the problem by throwing a repo = repo.unfiltered() in
bookmarks.compare(). I didn't submit a patch yet because I was hoping to
try a little harder getting a test to repro.
Pierre-Yves David - March 19, 2014, 12:35 a.m.
On 03/18/2014 05:23 PM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1395182192 18000
> #      Tue Mar 18 17:36:32 2014 -0500
> # Branch stable
> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
> bookmarks: use try/except for accessing a node
>
> Previously, in bookmarks.compare a changeset node would be tested via 'node in
> repo' but this is tricky. A value of 'True' could be returned even if the node
> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
> remote bookmark points to changeset that has been obsoleted locally. Therefore,
> we wrap the block in a try/except which handles catching the error.

1. `<node> in repo` should not return True if the revision is filtered.
    Consider fixing this instead.

2. During bookmark pulling, a bookmark should cleanly apply on a
    filtered changeset making it visible again.
    Consider fixing this instead.

3. There is multiple example of test using public changeset in
    test-obsolete.t You should be able to build a test case from that.
Sean Farley - March 19, 2014, 12:49 a.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 03/18/2014 05:23 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1395182192 18000
>> #      Tue Mar 18 17:36:32 2014 -0500
>> # Branch stable
>> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
>> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>> bookmarks: use try/except for accessing a node
>>
>> Previously, in bookmarks.compare a changeset node would be tested via 'node in
>> repo' but this is tricky. A value of 'True' could be returned even if the node
>> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
>> remote bookmark points to changeset that has been obsoleted locally. Therefore,
>> we wrap the block in a try/except which handles catching the error.
>
> 1. `<node> in repo` should not return True if the revision is filtered.
>     Consider fixing this instead.
>
> 2. During bookmark pulling, a bookmark should cleanly apply on a
>     filtered changeset making it visible again.
>     Consider fixing this instead.
>
> 3. There is multiple example of test using public changeset in
>     test-obsolete.t You should be able to build a test case from that.

This is a two-line fix for the craziness that is repoview. You are
welcome to fix the other things yourself but some of us don't have the
luxury of time as you might. This is a code change for correctness (and
a potential speed boost) that should go on stable.
Sean Farley - March 19, 2014, 12:51 a.m.
Gregory Szorc <gregory.szorc@gmail.com> writes:

> On 3/18/2014 5:23 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1395182192 18000
>> #      Tue Mar 18 17:36:32 2014 -0500
>> # Branch stable
>> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
>> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>> bookmarks: use try/except for accessing a node
>> 
>> Previously, in bookmarks.compare a changeset node would be tested via 'node in
>> repo' but this is tricky. A value of 'True' could be returned even if the node
>> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
>> remote bookmark points to changeset that has been obsoleted locally. Therefore,
>> we wrap the block in a try/except which handles catching the error.
>> 
>> Writing a test is also tricky because I only observed this in extension land,
>> so I put a comment to explain why we use try/except.
>
> I /think/ I was poking at the same problem a week or two ago. I was able
> to get my local repo to hit a RepoLookupError but was unable to write a
> test case to reproduce.
>
> I worked around the problem by throwing a repo = repo.unfiltered() in
> bookmarks.compare(). I didn't submit a patch yet because I was hoping to
> try a little harder getting a test to repro.

Unfortunately, I wasn't able to get a test case without the remote
bookmarks extension I was working on enabled. There were too many crazy
aspects of 'node in repo' returning True for a hidden node that made it
difficult to reproduce without the extension.
Gregory Szorc - March 19, 2014, 1:06 a.m.
On Mar 18, 2014, at 17:35, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
>> On 03/18/2014 05:23 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1395182192 18000
>> #      Tue Mar 18 17:36:32 2014 -0500
>> # Branch stable
>> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
>> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>> bookmarks: use try/except for accessing a node
>> 
>> Previously, in bookmarks.compare a changeset node would be tested via 'node in
>> repo' but this is tricky. A value of 'True' could be returned even if the node
>> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
>> remote bookmark points to changeset that has been obsoleted locally. Therefore,
>> we wrap the block in a try/except which handles catching the error.
> 
> 1. `<node> in repo` should not return True if the revision is filtered.
>   Consider fixing this instead.
> 
> 2. During bookmark pulling, a bookmark should cleanly apply on a
>   filtered changeset making it visible again.
>   Consider fixing this instead.
> 
> 3. There is multiple example of test using public changeset in
>   test-obsolete.t You should be able to build a test case from that.

I didn't have a problem writing the test: I had a problem getting the issue to reproduce!

It got me thinking that this might be an issue triggered by an extension.
Sean Farley - March 19, 2014, 1:20 a.m.
Gregory Szorc <gregory.szorc@gmail.com> writes:

> On Mar 18, 2014, at 17:35, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>> 
>> 
>> 
>>> On 03/18/2014 05:23 PM, Sean Farley wrote:
>>> # HG changeset patch
>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>> # Date 1395182192 18000
>>> #      Tue Mar 18 17:36:32 2014 -0500
>>> # Branch stable
>>> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
>>> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>>> bookmarks: use try/except for accessing a node
>>> 
>>> Previously, in bookmarks.compare a changeset node would be tested via 'node in
>>> repo' but this is tricky. A value of 'True' could be returned even if the node
>>> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
>>> remote bookmark points to changeset that has been obsoleted locally. Therefore,
>>> we wrap the block in a try/except which handles catching the error.
>> 
>> 1. `<node> in repo` should not return True if the revision is filtered.
>>   Consider fixing this instead.
>> 
>> 2. During bookmark pulling, a bookmark should cleanly apply on a
>>   filtered changeset making it visible again.
>>   Consider fixing this instead.
>> 
>> 3. There is multiple example of test using public changeset in
>>   test-obsolete.t You should be able to build a test case from that.
>
> I didn't have a problem writing the test: I had a problem getting the issue to reproduce!
>
> It got me thinking that this might be an issue triggered by an extension.

Yes, exactly, this was my issue as well.
Matt Mackall - March 19, 2014, 6:12 p.m.
On Tue, 2014-03-18 at 19:49 -0500, Sean Farley wrote:
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
> 
> > On 03/18/2014 05:23 PM, Sean Farley wrote:
> >> # HG changeset patch
> >> # User Sean Farley <sean.michael.farley@gmail.com>
> >> # Date 1395182192 18000
> >> #      Tue Mar 18 17:36:32 2014 -0500
> >> # Branch stable
> >> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
> >> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
> >> bookmarks: use try/except for accessing a node
> >>
> >> Previously, in bookmarks.compare a changeset node would be tested via 'node in
> >> repo' but this is tricky. A value of 'True' could be returned even if the node
> >> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
> >> remote bookmark points to changeset that has been obsoleted locally. Therefore,
> >> we wrap the block in a try/except which handles catching the error.
> >
> > 1. `<node> in repo` should not return True if the revision is filtered.
> >     Consider fixing this instead.
> >
> > 2. During bookmark pulling, a bookmark should cleanly apply on a
> >     filtered changeset making it visible again.
> >     Consider fixing this instead.
> >
> > 3. There is multiple example of test using public changeset in
> >     test-obsolete.t You should be able to build a test case from that.
> 
> This is a two-line fix for the craziness that is repoview.

Except it doesn't actually appear to be a fix. It just masks a symptom
of an underlying problem.

This is generally worse than doing nothing, because hiding the symptoms
makes it less likely that the underlying problem will be addressed.
Sean Farley - March 19, 2014, 6:25 p.m.
Matt Mackall <mpm@selenic.com> writes:

> On Tue, 2014-03-18 at 19:49 -0500, Sean Farley wrote:
>> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>> 
>> > On 03/18/2014 05:23 PM, Sean Farley wrote:
>> >> # HG changeset patch
>> >> # User Sean Farley <sean.michael.farley@gmail.com>
>> >> # Date 1395182192 18000
>> >> #      Tue Mar 18 17:36:32 2014 -0500
>> >> # Branch stable
>> >> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
>> >> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>> >> bookmarks: use try/except for accessing a node
>> >>
>> >> Previously, in bookmarks.compare a changeset node would be tested via 'node in
>> >> repo' but this is tricky. A value of 'True' could be returned even if the node
>> >> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
>> >> remote bookmark points to changeset that has been obsoleted locally. Therefore,
>> >> we wrap the block in a try/except which handles catching the error.
>> >
>> > 1. `<node> in repo` should not return True if the revision is filtered.
>> >     Consider fixing this instead.
>> >
>> > 2. During bookmark pulling, a bookmark should cleanly apply on a
>> >     filtered changeset making it visible again.
>> >     Consider fixing this instead.
>> >
>> > 3. There is multiple example of test using public changeset in
>> >     test-obsolete.t You should be able to build a test case from that.
>> 
>> This is a two-line fix for the craziness that is repoview.
>
> Except it doesn't actually appear to be a fix. It just masks a symptom
> of an underlying problem.

It might seem that way but this actually is a fix. The previous line was
testing for 'node in repo' and if that is false, then add the node to
the 'differ' list. Now, if we hit a RepoLookupError we add the node to
the 'differ' list again. The symptom isn't masked, it's correctly added
to the differ list instead.
Matt Mackall - March 19, 2014, 7:40 p.m.
On Wed, 2014-03-19 at 13:25 -0500, Sean Farley wrote:
> Matt Mackall <mpm@selenic.com> writes:
> 
> > On Tue, 2014-03-18 at 19:49 -0500, Sean Farley wrote:
> >> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
> >> 
> >> > On 03/18/2014 05:23 PM, Sean Farley wrote:
> >> >> # HG changeset patch
> >> >> # User Sean Farley <sean.michael.farley@gmail.com>
> >> >> # Date 1395182192 18000
> >> >> #      Tue Mar 18 17:36:32 2014 -0500
> >> >> # Branch stable
> >> >> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
> >> >> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
> >> >> bookmarks: use try/except for accessing a node
> >> >>
> >> >> Previously, in bookmarks.compare a changeset node would be tested via 'node in
> >> >> repo' but this is tricky. A value of 'True' could be returned even if the node
> >> >> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
> >> >> remote bookmark points to changeset that has been obsoleted locally. Therefore,
> >> >> we wrap the block in a try/except which handles catching the error.
> >> >
> >> > 1. `<node> in repo` should not return True if the revision is filtered.
> >> >     Consider fixing this instead.
> >> >
> >> > 2. During bookmark pulling, a bookmark should cleanly apply on a
> >> >     filtered changeset making it visible again.
> >> >     Consider fixing this instead.
> >> >
> >> > 3. There is multiple example of test using public changeset in
> >> >     test-obsolete.t You should be able to build a test case from that.
> >> 
> >> This is a two-line fix for the craziness that is repoview.
> >
> > Except it doesn't actually appear to be a fix. It just masks a symptom
> > of an underlying problem.
> 
> It might seem that way but this actually is a fix. The previous line was
> testing for 'node in repo' and if that is false, then add the node to
> the 'differ' list. Now, if we hit a RepoLookupError we add the node to
> the 'differ' list again. The symptom isn't masked, it's correctly added
> to the differ list instead.

Your assertion appears to be "we can do 'if x in repo: repo[x]' and get
a lookup error sometimes." 

Pierre-Yves assertion appears to be that this demonstrates a violation
of an invariant in repoview. It's hard to disagree: the behavior looks
obviously surprising/broken/nonsensical.

So this is the bug, not any lack of paranoid programming in
bookmarks.py.

If your fix is to add a try/except in one of the many places that our
invariants say should not be needed, you're just hiding the actual bug
(not to mention uglifying the code in the event the actual bug gets
fixed).
Sean Farley - March 19, 2014, 7:46 p.m.
Matt Mackall <mpm@selenic.com> writes:

> On Wed, 2014-03-19 at 13:25 -0500, Sean Farley wrote:
>> Matt Mackall <mpm@selenic.com> writes:
>> 
>> > On Tue, 2014-03-18 at 19:49 -0500, Sean Farley wrote:
>> >> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>> >> 
>> >> > On 03/18/2014 05:23 PM, Sean Farley wrote:
>> >> >> # HG changeset patch
>> >> >> # User Sean Farley <sean.michael.farley@gmail.com>
>> >> >> # Date 1395182192 18000
>> >> >> #      Tue Mar 18 17:36:32 2014 -0500
>> >> >> # Branch stable
>> >> >> # Node ID 8d3d12401c38524417f1151b817fd210645a0602
>> >> >> # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>> >> >> bookmarks: use try/except for accessing a node
>> >> >>
>> >> >> Previously, in bookmarks.compare a changeset node would be tested via 'node in
>> >> >> repo' but this is tricky. A value of 'True' could be returned even if the node
>> >> >> is hidden and accessing it will throw a 'RepoLookupError'. This happens when a
>> >> >> remote bookmark points to changeset that has been obsoleted locally. Therefore,
>> >> >> we wrap the block in a try/except which handles catching the error.
>> >> >
>> >> > 1. `<node> in repo` should not return True if the revision is filtered.
>> >> >     Consider fixing this instead.
>> >> >
>> >> > 2. During bookmark pulling, a bookmark should cleanly apply on a
>> >> >     filtered changeset making it visible again.
>> >> >     Consider fixing this instead.
>> >> >
>> >> > 3. There is multiple example of test using public changeset in
>> >> >     test-obsolete.t You should be able to build a test case from that.
>> >> 
>> >> This is a two-line fix for the craziness that is repoview.
>> >
>> > Except it doesn't actually appear to be a fix. It just masks a symptom
>> > of an underlying problem.
>> 
>> It might seem that way but this actually is a fix. The previous line was
>> testing for 'node in repo' and if that is false, then add the node to
>> the 'differ' list. Now, if we hit a RepoLookupError we add the node to
>> the 'differ' list again. The symptom isn't masked, it's correctly added
>> to the differ list instead.
>
> Your assertion appears to be "we can do 'if x in repo: repo[x]' and get
> a lookup error sometimes." 
>
> Pierre-Yves assertion appears to be that this demonstrates a violation
> of an invariant in repoview. It's hard to disagree: the behavior looks
> obviously surprising/broken/nonsensical.
>
> So this is the bug, not any lack of paranoid programming in
> bookmarks.py.
>
> If your fix is to add a try/except in one of the many places that our
> invariants say should not be needed, you're just hiding the actual bug
> (not to mention uglifying the code in the event the actual bug gets
> fixed).

Yes, of course. I don't know why I even bother. I'm clearly not good
enough to send patches.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -303,11 +303,13 @@  def compare(repo, srcmarks, dstmarks,
         elif b not in dstmarks:
             addsrc((b, srchex(srcmarks[b]), None))
         else:
             scid = srchex(srcmarks[b])
             dcid = dsthex(dstmarks[b])
-            if scid in repo and dcid in repo:
+            # we use try/except because testing for membership is tricky and
+            # sometimes gives True when a revision is hidden
+            try:
                 sctx = repo[scid]
                 dctx = repo[dcid]
                 if sctx.rev() < dctx.rev():
                     if validdest(repo, sctx, dctx):
                         advdst((b, scid, dcid))
@@ -316,11 +318,11 @@  def compare(repo, srcmarks, dstmarks,
                 else:
                     if validdest(repo, dctx, sctx):
                         advsrc((b, scid, dcid))
                     else:
                         diverge((b, scid, dcid))
-            else:
+            except error.RepoLookupError:
                 # it is too expensive to examine in detail, in this case
                 differ((b, scid, dcid))
 
     return results