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

[Matt Mackall]

On Wed, 2014-03-19 at 13:25 -0500, Sean Farley wrote:
> Matt Mackall <mpm at selenic.com> writes:
> 
> > On Tue, 2014-03-18 at 19:49 -0500, Sean Farley wrote:
> >> Pierre-Yves David <pierre-yves.david at ens-lyon.org> writes:
> >> 
> >> > On 03/18/2014 05:23 PM, Sean Farley wrote:
> >> >> # HG changeset patch
> >> >> # User Sean Farley <sean.michael.farley at 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).

-- 
Mathematics is the supreme nostalgia of our time.