Patchwork [1,of,2,V2] debug: add debugexplainunstable that explains instabilities

login
register
mail settings
Submitter Anton Shestakov
Date Feb. 26, 2018, 2:05 p.m.
Message ID <0aa1728931cc2c2c7d6e.1519653926@neuro>
Download mbox | patch
Permalink /patch/28405/
State Superseded
Headers show

Comments

Anton Shestakov - Feb. 26, 2018, 2:05 p.m.
# HG changeset patch
# User Anton Shestakov <av6@dwimlabs.net>
# Date 1519649041 -28800
#      Mon Feb 26 20:44:01 2018 +0800
# Node ID 0aa1728931cc2c2c7d6ee0f18e0618fc17add42a
# Parent  aefb75730ea34f545f0756bf8441fc9ae07bf8dc
debug: add debugexplainunstable that explains instabilities

This is a port of evolve's feature of listing all unstable changesets in detail
(`hg evolve --list`).
Yuya Nishihara - Feb. 27, 2018, 11:45 a.m.
On Mon, 26 Feb 2018 22:05:26 +0800, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <av6@dwimlabs.net>
> # Date 1519649041 -28800
> #      Mon Feb 26 20:44:01 2018 +0800
> # Node ID 0aa1728931cc2c2c7d6ee0f18e0618fc17add42a
> # Parent  aefb75730ea34f545f0756bf8441fc9ae07bf8dc
> debug: add debugexplainunstable that explains instabilities

> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -1039,3 +1039,35 @@ def createmarkers(repo, relations, flag=
>                                   date=date, metadata=localmetadata,
>                                   ui=repo.ui)
>              repo.filteredrevcache.clear()
> +
> +def explainunstable(repo, ctx):

I think this is an obs"util" function.

> +    if ctx.contentdivergent():
> +        dsets = obsutil.divergentsets(repo, ctx)
> +        for dset in dsets:
> +            divnodes = [repo[n] for n in dset['divergentnodes']] # hmm

Hmm?
Anton Shestakov - Feb. 27, 2018, 1:11 p.m.
On Tue, 27 Feb 2018 20:45:21 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 26 Feb 2018 22:05:26 +0800, Anton Shestakov wrote:
> > # HG changeset patch
> > # User Anton Shestakov <av6@dwimlabs.net>
> > # Date 1519649041 -28800
> > #      Mon Feb 26 20:44:01 2018 +0800
> > # Node ID 0aa1728931cc2c2c7d6ee0f18e0618fc17add42a
> > # Parent  aefb75730ea34f545f0756bf8441fc9ae07bf8dc
> > debug: add debugexplainunstable that explains instabilities  
> 
> > diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> > --- a/mercurial/obsolete.py
> > +++ b/mercurial/obsolete.py
> > @@ -1039,3 +1039,35 @@ def createmarkers(repo, relations, flag=
> >                                   date=date, metadata=localmetadata,
> >                                   ui=repo.ui)
> >              repo.filteredrevcache.clear()
> > +
> > +def explainunstable(repo, ctx):  
> 
> I think this is an obs"util" function.

I thought so too, but then I noticed that explainunstable() uses
`bumpedfix`, which is a constant defined and documented with a pretty
long comment in obsolete.py, and the only other use of it is
incidentally also in obsolete.py, in a function that computes all
phase-divergent changesets (_computephasedivergentset). Its code is
very similar to what explainunstable function uses to explain the
corresponding instability. So I feel that it's fine to put this function
in the same file with the related code. Even if that file is
obsutil.py, but then quite a bit of code from obsolete.py would need to
be moved there too.

> > +    if ctx.contentdivergent():
> > +        dsets = obsutil.divergentsets(repo, ctx)
> > +        for dset in dsets:
> > +            divnodes = [repo[n] for n in dset['divergentnodes']] # hmm  
> 
> Hmm?

I tried to decide if we want divergentnodes be a list of ctx (easier to
work with) or node hashes (more consistent with "node"), but then forgot
about it.
Yuya Nishihara - Feb. 27, 2018, 3:44 p.m.
On Tue, 27 Feb 2018 21:11:17 +0800, Anton Shestakov wrote:
> On Tue, 27 Feb 2018 20:45:21 +0900
> Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Mon, 26 Feb 2018 22:05:26 +0800, Anton Shestakov wrote:
> > > # HG changeset patch
> > > # User Anton Shestakov <av6@dwimlabs.net>
> > > # Date 1519649041 -28800
> > > #      Mon Feb 26 20:44:01 2018 +0800
> > > # Node ID 0aa1728931cc2c2c7d6ee0f18e0618fc17add42a
> > > # Parent  aefb75730ea34f545f0756bf8441fc9ae07bf8dc
> > > debug: add debugexplainunstable that explains instabilities  
> > 
> > > diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> > > --- a/mercurial/obsolete.py
> > > +++ b/mercurial/obsolete.py
> > > @@ -1039,3 +1039,35 @@ def createmarkers(repo, relations, flag=
> > >                                   date=date, metadata=localmetadata,
> > >                                   ui=repo.ui)
> > >              repo.filteredrevcache.clear()
> > > +
> > > +def explainunstable(repo, ctx):  
> > 
> > I think this is an obs"util" function.
> 
> I thought so too, but then I noticed that explainunstable() uses
> `bumpedfix`, which is a constant defined and documented with a pretty
> long comment in obsolete.py, and the only other use of it is
> incidentally also in obsolete.py, in a function that computes all
> phase-divergent changesets (_computephasedivergentset). Its code is
> very similar to what explainunstable function uses to explain the
> corresponding instability. So I feel that it's fine to put this function
> in the same file with the related code. Even if that file is
> obsutil.py, but then quite a bit of code from obsolete.py would need to
> be moved there too.

Thanks for considering it deeply. I don't have strong opinion, but it seems
we're slowly moving non-core parts out of obsolete.py, and templating stuff
would be non-core.

Boris, do you have any preference?
Anton Shestakov - March 14, 2018, 11:07 a.m.
On Wed, 28 Feb 2018 00:44:49 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> On Tue, 27 Feb 2018 21:11:17 +0800, Anton Shestakov wrote:
> > On Tue, 27 Feb 2018 20:45:21 +0900
> > Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> > > On Mon, 26 Feb 2018 22:05:26 +0800, Anton Shestakov wrote:
> > > > # HG changeset patch
> > > > # User Anton Shestakov <av6@dwimlabs.net>
> > > > # Date 1519649041 -28800
> > > > #      Mon Feb 26 20:44:01 2018 +0800
> > > > # Node ID 0aa1728931cc2c2c7d6ee0f18e0618fc17add42a
> > > > # Parent  aefb75730ea34f545f0756bf8441fc9ae07bf8dc
> > > > debug: add debugexplainunstable that explains instabilities  
> > > 
> > > > diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> > > > --- a/mercurial/obsolete.py
> > > > +++ b/mercurial/obsolete.py
> > > > @@ -1039,3 +1039,35 @@ def createmarkers(repo, relations, flag=
> > > >                                   date=date, metadata=localmetadata,
> > > >                                   ui=repo.ui)
> > > >              repo.filteredrevcache.clear()
> > > > +
> > > > +def explainunstable(repo, ctx):  
> > > 
> > > I think this is an obs"util" function.
> > 
> > I thought so too, but then I noticed that explainunstable() uses
> > `bumpedfix`, which is a constant defined and documented with a pretty
> > long comment in obsolete.py, and the only other use of it is
> > incidentally also in obsolete.py, in a function that computes all
> > phase-divergent changesets (_computephasedivergentset). Its code is
> > very similar to what explainunstable function uses to explain the
> > corresponding instability. So I feel that it's fine to put this function
> > in the same file with the related code. Even if that file is
> > obsutil.py, but then quite a bit of code from obsolete.py would need to
> > be moved there too.
> 
> Thanks for considering it deeply. I don't have strong opinion, but it seems
> we're slowly moving non-core parts out of obsolete.py, and templating stuff
> would be non-core.
> 
> Boris, do you have any preference?

I actually tried to move this function to obsutil, and that required
moving bumpedfix to obsutil too, otherwise there would be a circular
import to resolve (because obsolete, naturally, imports obsutil). But
then I noticed that moving bumpedfix to obsutil breaks evolve extension,
because it expects to find obsolete.bumpedfix. Importing bumpedfix into
obsolete directly (from obsutil import ...) to save evolve from
breaking, in turn, raises a warning because there's only a handful of
modules that are allowed in direct imports (e.g. from node import hex
is allowed).

Not saying it's too difficult, but it just feels that there's sizable
refactoring to be had first if people want this in obsutil. For now
I've sent a V3 that still puts this function into obsolete.py.
Yuya Nishihara - March 14, 2018, 1:48 p.m.
On Wed, 14 Mar 2018 19:07:16 +0800, Anton Shestakov wrote:
> On Wed, 28 Feb 2018 00:44:49 +0900
> Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Tue, 27 Feb 2018 21:11:17 +0800, Anton Shestakov wrote:
> > > On Tue, 27 Feb 2018 20:45:21 +0900
> > > Yuya Nishihara <yuya@tcha.org> wrote:
> > > 
> > > > On Mon, 26 Feb 2018 22:05:26 +0800, Anton Shestakov wrote:
> > > > > # HG changeset patch
> > > > > # User Anton Shestakov <av6@dwimlabs.net>
> > > > > # Date 1519649041 -28800
> > > > > #      Mon Feb 26 20:44:01 2018 +0800
> > > > > # Node ID 0aa1728931cc2c2c7d6ee0f18e0618fc17add42a
> > > > > # Parent  aefb75730ea34f545f0756bf8441fc9ae07bf8dc
> > > > > debug: add debugexplainunstable that explains instabilities  
> > > > 
> > > > > diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> > > > > --- a/mercurial/obsolete.py
> > > > > +++ b/mercurial/obsolete.py
> > > > > @@ -1039,3 +1039,35 @@ def createmarkers(repo, relations, flag=
> > > > >                                   date=date, metadata=localmetadata,
> > > > >                                   ui=repo.ui)
> > > > >              repo.filteredrevcache.clear()
> > > > > +
> > > > > +def explainunstable(repo, ctx):  
> > > > 
> > > > I think this is an obs"util" function.
> > > 
> > > I thought so too, but then I noticed that explainunstable() uses
> > > `bumpedfix`, which is a constant defined and documented with a pretty
> > > long comment in obsolete.py, and the only other use of it is
> > > incidentally also in obsolete.py, in a function that computes all
> > > phase-divergent changesets (_computephasedivergentset). Its code is
> > > very similar to what explainunstable function uses to explain the
> > > corresponding instability. So I feel that it's fine to put this function
> > > in the same file with the related code. Even if that file is
> > > obsutil.py, but then quite a bit of code from obsolete.py would need to
> > > be moved there too.
> > 
> > Thanks for considering it deeply. I don't have strong opinion, but it seems
> > we're slowly moving non-core parts out of obsolete.py, and templating stuff
> > would be non-core.
> > 
> > Boris, do you have any preference?
> 
> I actually tried to move this function to obsutil, and that required
> moving bumpedfix to obsutil too, otherwise there would be a circular
> import to resolve (because obsolete, naturally, imports obsutil). But
> then I noticed that moving bumpedfix to obsutil breaks evolve extension,
> because it expects to find obsolete.bumpedfix.

Boris?

I think it's okay to move bumpedfix to obsutil.py and re-export by
obsolete.py.

> Importing bumpedfix into
> obsolete directly (from obsutil import ...) to save evolve from
> breaking, in turn, raises a warning because there's only a handful of
> modules that are allowed in direct imports (e.g. from node import hex
> is allowed).

'bumpedfix = obsutil.bumpedfix' should work.

Patch

diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -816,6 +816,17 @@  def debugdownload(ui, repo, url, output=
         if output:
             dest.close()
 
+@command('debugexplainunstable', [], _('REV'))
+def debugexplainunstable(ui, repo, rev):
+    """explain instabilities of a changeset"""
+    for entry in obsolete.explainunstable(repo, repo[rev]):
+        dnodes = ''
+        if entry.get('divergentnodes'):
+            dnodes = ' '.join('%s (%s)' % (ctx.hex(), ctx.phasestr())
+                              for ctx in entry['divergentnodes']) + ' '
+        ui.write('%s: %s%s %s\n' % (entry['instability'], dnodes,
+                                    entry['reason'], entry['node']))
+
 @command('debugextensions', cmdutil.formatteropts, [], norepo=True)
 def debugextensions(ui, **opts):
     '''show information about active extensions'''
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -1039,3 +1039,35 @@  def createmarkers(repo, relations, flag=
                                  date=date, metadata=localmetadata,
                                  ui=repo.ui)
             repo.filteredrevcache.clear()
+
+def explainunstable(repo, ctx):
+    result = []
+    if ctx.orphan():
+        for parent in ctx.parents():
+            kind = None
+            if parent.orphan():
+                kind = 'orphan'
+            elif parent.obsolete():
+                kind = 'obsolete'
+            if kind is not None:
+                result.append({'instability': 'orphan',
+                               'reason': '%s parent' % kind,
+                               'node': parent.hex()})
+    if ctx.phasedivergent():
+        predecessors = obsutil.allpredecessors(repo.obsstore, [ctx.node()],
+                                               ignoreflags=bumpedfix)
+        immutable = [repo[p] for p in predecessors
+                     if p in repo and not repo[p].mutable()]
+        for predecessor in immutable:
+            result.append({'instability': 'phase-divergent',
+                           'reason': 'immutable predecessor',
+                           'node': predecessor.hex()})
+    if ctx.contentdivergent():
+        dsets = obsutil.divergentsets(repo, ctx)
+        for dset in dsets:
+            divnodes = [repo[n] for n in dset['divergentnodes']] # hmm
+            result.append({'instability': 'content-divergent',
+                           'divergentnodes': divnodes,
+                           'reason': 'predecessor',
+                           'node': node.hex(dset['commonpredecessor'])})
+    return result
diff --git a/mercurial/obsutil.py b/mercurial/obsutil.py
--- a/mercurial/obsutil.py
+++ b/mercurial/obsutil.py
@@ -889,3 +889,23 @@  def _getfilteredreason(repo, changeid, c
 
             args = (changeid, firstsuccessors, remainingnumber)
             return filteredmsgtable['superseded_split_several'] % args
+
+def divergentsets(repo, ctx):
+    """Compute sets of commits divergent with a given one"""
+    cache = {}
+    base = {}
+    for n in allpredecessors(repo.obsstore, [ctx.node()]):
+        if n == ctx.node():
+            # a node can't be a base for divergence with itself
+            continue
+        nsuccsets = successorssets(repo, n, cache)
+        for nsuccset in nsuccsets:
+            if ctx.node() in nsuccset:
+                # we are only interested in *other* successor sets
+                continue
+            if tuple(nsuccset) in base:
+                # we already know the latest base for this divergency
+                continue
+            base[tuple(nsuccset)] = n
+    return [{'divergentnodes': divset, 'commonpredecessor': b}
+            for divset, b in base.iteritems()]
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -86,6 +86,7 @@  Show debug commands if there are no othe
   debugdirstate
   debugdiscovery
   debugdownload
+  debugexplainunstable
   debugextensions
   debugfileset
   debugformat
@@ -266,6 +267,7 @@  Show all commands + options
   debugdirstate: nodates, datesort
   debugdiscovery: old, nonheads, rev, ssh, remotecmd, insecure
   debugdownload: output
+  debugexplainunstable: 
   debugextensions: template
   debugfileset: rev
   debugformat: template
diff --git a/tests/test-help.t b/tests/test-help.t
--- a/tests/test-help.t
+++ b/tests/test-help.t
@@ -928,6 +928,8 @@  Test list of internal help commands
                  runs the changeset discovery protocol in isolation
    debugdownload
                  download a resource using Mercurial logic and config
+   debugexplainunstable
+                 explain instabilities of a changeset
    debugextensions
                  show information about active extensions
    debugfileset  parse and apply a fileset specification
diff --git a/tests/test-obsolete-divergent.t b/tests/test-obsolete-divergent.t
--- a/tests/test-obsolete-divergent.t
+++ b/tests/test-obsolete-divergent.t
@@ -717,3 +717,6 @@  Use scmutil.cleanupnodes API to create d
   a178212c3433c4e77b573f6011e29affb8aefa33 1a2a9b5b0030632400aa78e00388c20f99d3ec44 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '1', 'operation': 'amend', 'user': 'test'}
   a178212c3433c4e77b573f6011e29affb8aefa33 ad6478fb94ecec98b86daae98722865d494ac561 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '13', 'operation': 'test', 'user': 'test'}
   ad6478fb94ecec98b86daae98722865d494ac561 70d5a63ca112acb3764bc1d7320ca90ea688d671 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '9', 'operation': 'test', 'user': 'test'}
+
+  $ hg debugexplainunstable 1a2a9b5b0030
+  content-divergent: 70d5a63ca112acb3764bc1d7320ca90ea688d671 (draft) predecessor a178212c3433c4e77b573f6011e29affb8aefa33
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -1033,6 +1033,12 @@  test summary output
   orphan: 2 changesets
   phase-divergent: 1 changesets
 
+test debugexplainunstable output
+
+  $ hg debugexplainunstable 50c51b361e60
+  orphan: obsolete parent 3de5eca88c00aa039da7399a220f4a5221faa585
+  phase-divergent: immutable predecessor 245bde4270cd1072a27757984f9cda8ba26f08ca
+
 #if serve
 
   $ hg serve -n test -p $HGPORT -d --pid-file=hg.pid -A access.log -E errors.log