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

[Idan Kamara]

On Mon, Sep 24, 2012 at 1:08 AM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:
>
>
> 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.

I haven't seen this happening when I was working on it,
but I trust you've been working on it long enough to think so.

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

What's the difference here?

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

So --hidden is needed on any command that accepts a
revision? Ouch.

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

The way I saw it is that if a filtered revision is requested indirectly,
e.g. getting all revisions in a repo, getting children/descendants of
a revision, getting the heads, etc. then those results shouldn't contain
filtered revisions.

But when a revision is requested explicitly in some way then it
should be returned even if it's filtered, under the assumption that
if something (or one) is asking for it, it knows it's filtered (because it's
hidden normally and he had to look for it specifically to find it in
the first place) and he wants it anyway.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120924/9f2e6d96/attachment.html>