On Sun, Mar 26, 2017 at 04:44:11PM -0700, Jun Wu wrote:
> I think this is a good direction. See inline comments.
>
> Not related directly to this patch, but I was thinking introducing some
> "post-transaction" mechanism for transaction. So the caller won't need to
> put the "strip" outside a transaction. Compare:
>
> Currently:
>
> if obsmarker-enabled:
> with repo.transaction():
> ....
> create commits
> create markers for strip # ideally inside the transaction
> else:
> with repo.transaction():
> create commits
> strip # must be outside a transaction
>
> If we have "post-transaction" for repair.strip. The code could be written in
> a cleaner form without the "if", and there is only one transaction as
> expected.
This seems like a good thing to have in a world where we expect strip
to continue to be a thing we have to use, but I'd rather the
engineering energy that could go into this API instead went into a
safer append-only hiding mechanism.
>
> Excerpts from Kostia Balytskyi's message of 2017-03-26 15:32:55 -0700:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikos...@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.
> >
> > 'insistonstripping' is necessary for cases when caller might need
> > to enforce or not enforce stripping depending on some configuration
> > (like shelve needs now).
>
> That sounds against the motivation of introducing the method. Could the
> caller just call repair.strip, or disable createmarkers (using
> configoverride) if it insists to?
>
> >
> > 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):
>
> Since it may call "strip", "hide" is not accurate. Maybe just call it
> "strip".
>
> > + '''Remove nodes from visible repoview in the most appropriate
>
> Since strip won't support pruning a commit in the middle, and root-based
> hidden won't support that either. Maybe make it clear nodes' descendants
> will also be affected.
>
> > + 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():
>
> Then select "nodes::" here to get their descendants hidden.
>
> > + 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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
