[PATCH 8 of 8 clfilter-part1] clfilter: introduce `filteredrevs` attribute on changelog

[Pierre-Yves David]

On 20 sept. 2012, at 19:51, Idan Kamara wrote:

> On Thu, Sep 20, 2012 at 8:12 PM, <pierre-yves.david at logilab.fr> wrote:
> >
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david at logilab.fr>
> > # Date 1348160567 -7200
> > # Node ID 99ab3ca5b10c516485db9be9b8f5f6764ea7c7f7
> > # Parent  0f55ba7803997843a1e047f9b2fc112e53cabfb3
> > clfilter: introduce `filteredrevs` attribute on changelog
> 
> You've taken a slightly different approach on how to implement
> this thing than I have here: http://mercurial.markmail.org/thread/2fv3e2lgqohxle2g
> 
> What I liked about the wrapper classes is that it separates the logic
> of the filtering from the base implementations of revlog/changelog.
> As a result that means that if there aren't any filtered revisions then
> everything is the same as before (although the call 'x in set' where set
> is empty is probably negligible).
> 
> Any reason you think this is preferable?

Complexity.

Most of the revlog methods can't be efficiently filtered afterward. This means that most complexe methods need to be reimplemented with filtering in mind causing a massive duplication of codes. However with minor changes to the revlog code all those function can returns a properly filtered result if a few key lower level methods are overridden (mostly iteration). That's the meaning of all changesets that comes before this one.

This approach allows minimal code duplication and low complexity. But it requires to be implemented on changelog to redefines methods used in revlog code.


> Are you sure these errors you're raising below are correct? It seems
> to me that you shouldn't filter out revisions when they're asked for
> explicitly, but rather when one requests to know what is the set of
> all revisions in the repository.
> 
> For instance, if the user types `hg log -r 0` and 0 happens to be filtered,
> I think we should still display it rather than saying the revision doesn't
> exist.

Yes they are. For multiple reasons:

(1) The `hg log -r 0` case is tricky. Hidden revision must be filtered out of every commands by default (for consistency).

    This means filtered for revset too. You can't have "hg bundle -r '42::' bundles more revision than `hg log -r 42::` do.

    Then we have `hg log -r 0:0` that match nothing. Should `hg log -r 0` do ? I do not know.

    The current approach is to

    A) Have global --hidden flags:

        `hg log -r 0` --> no such revision
        `hg log -r 0 --hidden` --> Display 0

    B) Have a dedicated sub class on Exception to detect it's filtered

        `hg log -r 0` --> Revision 0 is hidden. do you want --hidden ?
        `hg log -r 0 --hidden` --> Display 0

    C) (maybe) make several command smarter

        `hg log -r 0` --> Display 0 because 0 is explicit enough

    This is far from perfect but it is the best I can come up with to get a consistent experience for user. Any suggestion are welcome.

(2) Raising those errors force to explicitly disabled filtering when you write code that need to run infiltered, preventing any filtering side effect. This make also incorrect code crash explicitly instead of silently ignores filtering. Those points are very important if we want to track down code that need updates. Such error raising helped me to catch a lot of error in this initial series.

I convinced Matt this has to be done during our phone call.

-- 
Pierre-Yves