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