[PATCH RFC] repo: add an ability to hide nodes in an appropriate way

[Augie Fackler]

On Mon, Mar 27, 2017 at 10:27:33AM +0200, Pierre-Yves David wrote:
>
>
> On 03/27/2017 12:32 AM, Kostia Balytskyi wrote:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikostia at fb.com>
> > # Date 1490567500 25200
> > #      Sun Mar 26 15:31:40 2017 -0700
> > # Node ID 43cd56f38c1ca2ad1920ed6804e1a90a5e263c0d
> > # Parent  c60091fa1426892552dd6c0dd4b9c49e3c3da045
> > repo: add an ability to hide nodes in an appropriate way
> >
> > Potentially frequent usecase is to hide nodes in the most appropriate
> > for current repo configuration way. Examples of things which use this
> > are rebase (strip or obsolete old nodes), shelve (strip of obsolete
> > temporary nodes) and probably others.
> > Jun Wu had an idea of having one place in core where this functionality
> > is implemented so that others can utilize it. This patch
> > implements this idea.
>
> I do not think this is a step in the right direction, creating obsolescence
> marker is not just about hiding changeset.

But we're using them for that today, all over the place. We're also
having pretty productive (I think!) discussions about non-obsmarker
based mechanisms for hiding things that are implementation details of
a sort (amend changesets that get folded immediately, shelve changes,
etc).

> It is about recording the
> evolution of time of changesets (predecessor ← [successors]). This data is
> used in multiple place for advance logic, exchanged across repository etc.
> Being too heavy handed on stripping will have problematic consequence for
> the user experiences. Having an easy interfaces to do such heavy handed
> pruning will encourage developer do to such mistake.

I'm struggling a bit to understand your argument here. I think your
argument is this: hiding changes can be done for many reasons, and
having a simple "just hide this I don't care how" API means people
won't think before they do the hiding. Is that an accurate
simplification and restating of your position here? (If no, please try
and restate with more nuance.)

> The fact this function only takes a list of nodes (instead of (pec, suc)
> mapping) is a good example of such simplification. Series from Jun about
> histedit is another. Obsolescence markers are not meant as a generic
> temporary hidden mechanism for local/temporary nodes and should not be used
> that way.

In broad strokes, I agree here. Have we made any conclusive progress
on a mechanism for archiving changes? The reason we're seeing all
these "abuses" of obsolete markers is that they're currently the only
append-only means by which things can be discarded, and there are (as
you know) ugly cache implications to running strip.

(I'm sort of +0 on this series, but I'd be more enthusiastic about
having a proper hiding mechanism in place that isn't obsmarkers before
we go too far down this road.)

>
> > 'insistonstripping' is necessary for cases when caller might need
> > to enforce or not enforce stripping depending on some configuration
> > (like shelve needs now).
> >
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -50,6 +50,7 @@ from . import (
> >      phases,
> >      pushkey,
> >      pycompat,
> > +    repair,
> >      repoview,
> >      revset,
> >      revsetlang,
> > @@ -1884,6 +1885,23 @@ class localrepository(object):
> >          # tag cache retrieval" case to work.
> >          self.invalidate()
> >
> > +    def hidenodes(self, nodes, insistonstripping=False):
> > +        '''Remove nodes from visible repoview in the most appropriate
> > +        supported way. When obsmarker creation is enabled, this
> > +        function will obsolete these nodes without successors
> > +        (unless insistonstripping is True). Otherwise, it will
> > +        strip nodes.
> > +
> > +        In future we can add other node-hiding capabilities to this
> > +        function.'''
> > +        with self.lock():
> > +            if obsolete.isenabled(self, obsolete.createmarkersopt)\
> > +               and not insistonstripping:
> > +                relations = [(self[n], ()) for n in nodes]
> > +                obsolete.createmarkers(self, relations)
> > +            else:
> > +                repair.strip(self.ui, self, nodes, backup=False)
> > +
> >      def walk(self, match, node=None):
> >          '''
> >          walk recursively through the directory tree or a given
> > diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> > --- a/tests/test-obsolete.t
> > +++ b/tests/test-obsolete.t
> > @@ -1259,3 +1259,43 @@ Test the --delete option of debugobsolet
> >    1ab51af8f9b41ef8c7f6f3312d4706d870b1fb74 29346082e4a9e27042b62d2da0e2de211c027621 0 \(.*\) {'user': 'test'} (re)
> >    $ cd ..
> >
> > +Test that repo.hidenodes respects obsolescense configs
> > +  $ hg init hidenodesrepo && cd hidenodesrepo
> > +  $ cat <<EOF > debughidenodes.py
> > +  > from __future__ import absolute_import
> > +  > from mercurial import (
> > +  >     cmdutil,
> > +  > )
> > +  > cmdtable = {}
> > +  > command = cmdutil.command(cmdtable)
> > +  > @command('debughidenodes',
> > +  >         [('', 'insiststrip', None, 'pass insistonstripping=True')])
> > +  > def debughidenodes(ui, repo, *args, **opts):
> > +  >     nodes = [repo[n].node() for n in args]
> > +  >     repo.hidenodes(nodes, insistonstripping=bool(opts.get('insiststrip')))
> > +  > EOF
> > +  $ cat <<EOF > .hg/hgrc
> > +  > [extensions]
> > +  > debughidenodes=debughidenodes.py
> > +  > EOF
> > +  $ echo a > a
> > +  $ hg add a && hg ci -m a && hg up null
> > +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> > +  $ hg --config experimental.evolution=! debughidenodes 0
> > +  $ hg log -qr "all()" --hidden | wc -l  # commit was actually stripped
> > +  \s*0 (re)
> > +  $ hg debugobsolete | wc -l  # no markers were created
> > +  \s*0 (re)
> > +  $ echo a > a && hg add a && hg ci -m a && hg up null
> > +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> > +  $ hg --config experimental.evolution=createmarkers debughidenodes 0
> > +  $ hg log -qr "all()" --hidden | wc -l  # commit was not stripped
> > +  \s*1 (re)
> > +  $ hg debugobsolete | wc -l  # a marker was created
> > +  \s*1 (re)
> > +  $ hg --config experimental.evolution=createmarkers --hidden debughidenodes 0 --insiststrip
> > +  $ hg log -qr "all()" --hidden | wc -l  # commit was actually stripped
> > +  \s*0 (re)
> > +  $ hg debugobsolete | wc -l  # no new markers were created
> > +  \s*1 (re)
> > +  $ cd ..
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel