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

[Sean Farley]

Matt Mackall <mpm at selenic.com> writes:

> 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).

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