Patchwork [1,of,2] revset: add topo.revsonly argument for topo sort

login
register
mail settings
Submitter Xidorn Quan
Date Sept. 23, 2016, 1:26 p.m.
Message ID <9e8aeaddddf3bf6e61b3.1474637181@upsuper-mbp2.local>
Download mbox | patch
Permalink /patch/16770/
State Superseded
Headers show

Comments

Xidorn Quan - Sept. 23, 2016, 1:26 p.m.
# HG changeset patch
# User Xidorn Quan <me@upsuper.org>
# Date 1474636628 -36000
#      Fri Sep 23 23:17:08 2016 +1000
# Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
# Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
revset: add topo.revsonly argument for topo sort

This argument is used when we want an order which is not affected by
revisions other than given ones. It helps the next patch (rebase in
topo order) to keep sensible order when multiple separate revisions
are specified.
Pierre-Yves David - Sept. 23, 2016, 7:47 p.m.
On 09/23/2016 03:26 PM, Xidorn Quan wrote:
> # HG changeset patch
> # User Xidorn Quan <me@upsuper.org>
> # Date 1474636628 -36000
> #      Fri Sep 23 23:17:08 2016 +1000
> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
> revset: add topo.revsonly argument for topo sort
>
> This argument is used when we want an order which is not affected by
> revisions other than given ones. It helps the next patch (rebase in
> topo order) to keep sensible order when multiple separate revisions
> are specified.

I do not understand why this is necessary. How is the current code 
misbehaving in your case?
Xidorn Quan - Sept. 23, 2016, 10:01 p.m.
On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
> > # HG changeset patch
> > # User Xidorn Quan <me@upsuper.org>
> > # Date 1474636628 -36000
> > #      Fri Sep 23 23:17:08 2016 +1000
> > # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
> > # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
> > revset: add topo.revsonly argument for topo sort
> >
> > This argument is used when we want an order which is not affected by
> > revisions other than given ones. It helps the next patch (rebase in
> > topo order) to keep sensible order when multiple separate revisions
> > are specified.
> 
> I do not understand why this is necessary. How is the current code 
> misbehaving in your case?

I did this for fixing "Test multiple root handling" in
test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
probably should not do this, but instead, just update the result of that
test with my second patch.

- Xidorn
Pierre-Yves David - Sept. 23, 2016, 10:07 p.m.
On 09/24/2016 12:01 AM, Xidorn Quan wrote:
> On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
>> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
>>> # HG changeset patch
>>> # User Xidorn Quan <me@upsuper.org>
>>> # Date 1474636628 -36000
>>> #      Fri Sep 23 23:17:08 2016 +1000
>>> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
>>> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
>>> revset: add topo.revsonly argument for topo sort
>>>
>>> This argument is used when we want an order which is not affected by
>>> revisions other than given ones. It helps the next patch (rebase in
>>> topo order) to keep sensible order when multiple separate revisions
>>> are specified.
>>
>> I do not understand why this is necessary. How is the current code
>> misbehaving in your case?
>
> I did this for fixing "Test multiple root handling" in
> test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
> probably should not do this, but instead, just update the result of that
> test with my second patch.

What is the failure about?
Xidorn Quan - Sept. 23, 2016, 10:10 p.m.
On Sat, Sep 24, 2016, at 08:07 AM, Pierre-Yves David wrote:
> On 09/24/2016 12:01 AM, Xidorn Quan wrote:
> > On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
> >> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
> >>> # HG changeset patch
> >>> # User Xidorn Quan <me@upsuper.org>
> >>> # Date 1474636628 -36000
> >>> #      Fri Sep 23 23:17:08 2016 +1000
> >>> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
> >>> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
> >>> revset: add topo.revsonly argument for topo sort
> >>>
> >>> This argument is used when we want an order which is not affected by
> >>> revisions other than given ones. It helps the next patch (rebase in
> >>> topo order) to keep sensible order when multiple separate revisions
> >>> are specified.
> >>
> >> I do not understand why this is necessary. How is the current code
> >> misbehaving in your case?
> >
> > I did this for fixing "Test multiple root handling" in
> > test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
> > probably should not do this, but instead, just update the result of that
> > test with my second patch.
> 
> What is the failure about?

Please see the latest patch I sent to the list.

- Xidorn
Pierre-Yves David - Sept. 23, 2016, 10:16 p.m.
On 09/24/2016 12:10 AM, Xidorn Quan wrote:
> On Sat, Sep 24, 2016, at 08:07 AM, Pierre-Yves David wrote:
>> On 09/24/2016 12:01 AM, Xidorn Quan wrote:
>>> On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
>>>> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
>>>>> # HG changeset patch
>>>>> # User Xidorn Quan <me@upsuper.org>
>>>>> # Date 1474636628 -36000
>>>>> #      Fri Sep 23 23:17:08 2016 +1000
>>>>> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
>>>>> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
>>>>> revset: add topo.revsonly argument for topo sort
>>>>>
>>>>> This argument is used when we want an order which is not affected by
>>>>> revisions other than given ones. It helps the next patch (rebase in
>>>>> topo order) to keep sensible order when multiple separate revisions
>>>>> are specified.
>>>>
>>>> I do not understand why this is necessary. How is the current code
>>>> misbehaving in your case?
>>>
>>> I did this for fixing "Test multiple root handling" in
>>> test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
>>> probably should not do this, but instead, just update the result of that
>>> test with my second patch.
>>
>> What is the failure about?
>
> Please see the latest patch I sent to the list.

Can you explain that change to me ? (and then add this explanation in 
the commit description)

I appreciate your enthousiasm to get this improved (and this is 
definitely valuable to do so), but please avoid sending new version of 
the patch when the previous one is still discussed. This get confusing 
on the reviewers side.

Cheers,
Xidorn Quan - Sept. 23, 2016, 10:28 p.m.
On Sat, Sep 24, 2016, at 08:16 AM, Pierre-Yves David wrote:
> On 09/24/2016 12:10 AM, Xidorn Quan wrote:
> > On Sat, Sep 24, 2016, at 08:07 AM, Pierre-Yves David wrote:
> >> On 09/24/2016 12:01 AM, Xidorn Quan wrote:
> >>> On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
> >>>> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
> >>>>> # HG changeset patch
> >>>>> # User Xidorn Quan <me@upsuper.org>
> >>>>> # Date 1474636628 -36000
> >>>>> #      Fri Sep 23 23:17:08 2016 +1000
> >>>>> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
> >>>>> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
> >>>>> revset: add topo.revsonly argument for topo sort
> >>>>>
> >>>>> This argument is used when we want an order which is not affected by
> >>>>> revisions other than given ones. It helps the next patch (rebase in
> >>>>> topo order) to keep sensible order when multiple separate revisions
> >>>>> are specified.
> >>>>
> >>>> I do not understand why this is necessary. How is the current code
> >>>> misbehaving in your case?
> >>>
> >>> I did this for fixing "Test multiple root handling" in
> >>> test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
> >>> probably should not do this, but instead, just update the result of that
> >>> test with my second patch.
> >>
> >> What is the failure about?
> >
> > Please see the latest patch I sent to the list.
> 
> Can you explain that change to me ? (and then add this explanation in 
> the commit description)

That test is trying to rebase three different commits 7+11+9 on to 4.
None of them are directly connected, but 11 is a descendant of 7. With
the old behavior, the three commits would be rebased in 7,9,11 order,
and after my change, 7 and 11 would be grouped together, so the order
would be 9,7,11.

I thought that all three commits would become immediate children of 4
given they are not connected, and we should use revision order rather
than topo order for separate subgraph.

But it seems I was wrong. After the rebase, rebased 7 and 11 are still
in the same branch, which means they really should be grouped together.

> I appreciate your enthousiasm to get this improved (and this is 
> definitely valuable to do so), but please avoid sending new version of 
> the patch when the previous one is still discussed. This get confusing 
> on the reviewers side.

Oops, sorry about that.

- Xidorn
Pierre-Yves David - Sept. 23, 2016, 10:38 p.m.
On 09/24/2016 12:28 AM, Xidorn Quan wrote:
> On Sat, Sep 24, 2016, at 08:16 AM, Pierre-Yves David wrote:
>> On 09/24/2016 12:10 AM, Xidorn Quan wrote:
>>> On Sat, Sep 24, 2016, at 08:07 AM, Pierre-Yves David wrote:
>>>> On 09/24/2016 12:01 AM, Xidorn Quan wrote:
>>>>> On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
>>>>>> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Xidorn Quan <me@upsuper.org>
>>>>>>> # Date 1474636628 -36000
>>>>>>> #      Fri Sep 23 23:17:08 2016 +1000
>>>>>>> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
>>>>>>> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
>>>>>>> revset: add topo.revsonly argument for topo sort
>>>>>>>
>>>>>>> This argument is used when we want an order which is not affected by
>>>>>>> revisions other than given ones. It helps the next patch (rebase in
>>>>>>> topo order) to keep sensible order when multiple separate revisions
>>>>>>> are specified.
>>>>>>
>>>>>> I do not understand why this is necessary. How is the current code
>>>>>> misbehaving in your case?
>>>>>
>>>>> I did this for fixing "Test multiple root handling" in
>>>>> test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
>>>>> probably should not do this, but instead, just update the result of that
>>>>> test with my second patch.
>>>>
>>>> What is the failure about?
>>>
>>> Please see the latest patch I sent to the list.
>>
>> Can you explain that change to me ? (and then add this explanation in
>> the commit description)
>
> That test is trying to rebase three different commits 7+11+9 on to 4.
> None of them are directly connected, but 11 is a descendant of 7. With
> the old behavior, the three commits would be rebased in 7,9,11 order,
> and after my change, 7 and 11 would be grouped together, so the order
> would be 9,7,11.
>
> I thought that all three commits would become immediate children of 4
> given they are not connected, and we should use revision order rather
> than topo order for separate subgraph.
>
> But it seems I was wrong. After the rebase, rebased 7 and 11 are still
> in the same branch, which means they really should be grouped together.

Okay, thanks for the explanation, It makes a lot of sense and I'm happy 
that asking the question helped to clarify the situation

>> I appreciate your enthousiasm to get this improved (and this is
>> definitely valuable to do so), but please avoid sending new version of
>> the patch when the previous one is still discussed. This get confusing
>> on the reviewers side.
>
> Oops, sorry about that.

Nothing too terrible either. Can you send a V4 with a small explanation 
about this change to an existing tests in the changeset description ?

Please also update the first line of the description to contains "(BC)" 
(to flag the small behavior change) and "(issue5370)" (to automatically 
close your issue on the tracker.

Cheers,

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1861,17 +1861,17 @@  def roots(repo, subset, x):
     'desc': lambda c: c.description(),
     'user': lambda c: c.user(),
     'author': lambda c: c.user(),
     'date': lambda c: c.date()[0],
 }
 
 def _getsortargs(x):
     """Parse sort options into (set, [(key, reverse)], opts)"""
-    args = getargsdict(x, 'sort', 'set keys topo.firstbranch')
+    args = getargsdict(x, 'sort', 'set keys topo.firstbranch topo.revsonly')
     if 'set' not in args:
         # i18n: "sort" is a keyword
         raise error.ParseError(_('sort requires one or two arguments'))
     keys = "rev"
     if 'keys' in args:
         # i18n: "sort" is a keyword
         keys = getstring(args['keys'], _("sort spec must be a string"))
 
@@ -1880,29 +1880,28 @@  def _getsortargs(x):
         fk = k
         reverse = (k[0] == '-')
         if reverse:
             k = k[1:]
         if k not in _sortkeyfuncs and k != 'topo':
             raise error.ParseError(_("unknown sort key %r") % fk)
         keyflags.append((k, reverse))
 
-    if len(keyflags) > 1 and any(k == 'topo' for k, reverse in keyflags):
+    istopo = any(k == 'topo' for k, reverse in keyflags)
+    if istopo and len(keyflags) > 1:
         # i18n: "topo" is a keyword
         raise error.ParseError(_('topo sort order cannot be combined '
                                  'with other sort keys'))
 
     opts = {}
-    if 'topo.firstbranch' in args:
-        if any(k == 'topo' for k, reverse in keyflags):
-            opts['topo.firstbranch'] = args['topo.firstbranch']
-        else:
-            # i18n: "topo" and "topo.firstbranch" are keywords
-            raise error.ParseError(_('topo.firstbranch can only be used '
-                                     'when using the topo sort key'))
+    if not istopo and any(o.startswith('topo.') for o in args):
+        # i18n: "topo" and "topo.*" are keywords
+        raise error.ParseError(_('topo.* can only be used '
+                                 'when using the topo sort key'))
+    opts.update((k, v) for k, v in args.iteritems() if k.startswith('topo.'))
 
     return args['set'], keyflags, opts
 
 @predicate('sort(set[, [-]key... [, ...]])', safe=True, takeorder=True)
 def sort(repo, subset, x, order):
     """Sort set by keys. The default sort order is ascending, specify a key
     as ``-key`` to sort in descending order.
 
@@ -1911,45 +1910,51 @@  def sort(repo, subset, x, order):
     - ``rev`` for the revision number,
     - ``branch`` for the branch name,
     - ``desc`` for the commit message (description),
     - ``user`` for user name (``author`` can be used as an alias),
     - ``date`` for the commit date
     - ``topo`` for a reverse topographical sort
 
     The ``topo`` sort order cannot be combined with other sort keys. This sort
-    takes one optional argument, ``topo.firstbranch``, which takes a revset that
-    specifies what topographical branches to prioritize in the sort.
+    takes two optional arguments:
+    ```topo.firstbranch``` takes a revset that specifies what topographical
+    branches to prioritize in the sort.
+    ```topo.revsonly``` makes the sort not consider the relationship outside the
+    given revisions, and unconnected subgraphs would be sorted based on largest
+    revision number in it. Its value doesn't matter, and it takes effect as soon
+    as it presents in arguments.
 
     """
     s, keyflags, opts = _getsortargs(x)
     revs = getset(repo, subset, s)
 
     if not keyflags or order != defineorder:
         return revs
     if len(keyflags) == 1 and keyflags[0][0] == "rev":
         revs.sort(reverse=keyflags[0][1])
         return revs
     elif keyflags[0][0] == "topo":
         firstbranch = ()
         if 'topo.firstbranch' in opts:
             firstbranch = getset(repo, subset, opts['topo.firstbranch'])
-        revs = baseset(_toposort(revs, repo.changelog.parentrevs, firstbranch),
+        revs = baseset(_toposort(revs, repo.changelog.parentrevs,
+                                 firstbranch, 'topo.revsonly' in opts),
                        istopo=True)
         if keyflags[0][1]:
             revs.reverse()
         return revs
 
     # sort() is guaranteed to be stable
     ctxs = [repo[r] for r in revs]
     for k, reverse in reversed(keyflags):
         ctxs.sort(key=_sortkeyfuncs[k], reverse=reverse)
     return baseset([c.rev() for c in ctxs])
 
-def _toposort(revs, parentsfunc, firstbranch=()):
+def _toposort(revs, parentsfunc, firstbranch=(), revsonly=False):
     """Yield revisions from heads to roots one (topo) branch at a time.
 
     This function aims to be used by a graph generator that wishes to minimize
     the number of parallel branches and their interleaving.
 
     Example iteration order (numbers show the "true" order in a changelog):
 
       o  4
@@ -2093,17 +2098,18 @@  def _toposort(revs, parentsfunc, firstbr
             # after the subgroup merging because all elements from a subgroup
             # that relied on this rev must precede it.
             #
             # we also update the <parents> set to include the parents of the
             # new nodes.
             if rev == currentrev: # only display stuff in rev
                 gr[0].append(rev)
             gr[1].remove(rev)
-            parents = [p for p in parentsfunc(rev) if p > node.nullrev]
+            parents = [p for p in parentsfunc(rev) if p > node.nullrev and
+                       (not revsonly or p in revs)]
             gr[1].update(parents)
             for p in parents:
                 if p not in pendingset:
                     pendingset.add(p)
                     heappush(pendingheap, -p)
 
             # Look for a subgroup to display
             #
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -2087,25 +2087,34 @@  test sorting by multiple keys including 
   5 b111 t1   tu   130 0
   7 b111 t3   tu   130 0
   4 b111 m112 u111 110 14400
   3 b112 m111 u11  120 0
   2 b111 m11  u12  111 3600
   1 b11  m12  u111 112 7200
   0 b12  m111 u112 111 10800
 
+  $ hg log -r 'sort(3+6+7, topo, topo.revsonly=1)'
+  7 b111 t3   tu   130 0
+  6 b111 t2   tu   130 0
+  3 b112 m111 u11  120 0
+
 topographical sorting can't be combined with other sort keys, and you can't
-use the topo.firstbranch option when topo sort is not active:
+use the topo.* option when topo sort is not active:
 
   $ hg log -r 'sort(all(), "topo user")'
   hg: parse error: topo sort order cannot be combined with other sort keys
   [255]
 
   $ hg log -r 'sort(all(), user, topo.firstbranch=book1)'
-  hg: parse error: topo.firstbranch can only be used when using the topo sort key
+  hg: parse error: topo.* can only be used when using the topo sort key
+  [255]
+
+  $ hg log -r 'sort(all(), user, topo.revsonly=1)'
+  hg: parse error: topo.* can only be used when using the topo sort key
   [255]
 
 topo.firstbranch should accept any kind of expressions:
 
   $ hg log -r 'sort(0, topo, topo.firstbranch=(book1))'
   0 b12  m111 u112 111 10800
 
   $ cd ..