RE: [PATCH] kallithea pullrequests: allow limiting the number of additional changes shown
Hello Mads,
Thanks for the feedback (sorry for top-posting, outlook...)!
The overwhelming is on the server-side. If a large amount of revisions is
available, it takes a lot of time to build the graph showing the descendant
changesets.
Conditionally showing 'too long to be shown, click here to show' sounds like a
clean approach. I'll have a look if I can modify the code like that, thanks!
Greetings,
Mathias
-Original Message-
From: Mads Kiilerich
Sent: Monday, December 12, 2022 4:45 PM
To: Mathias De Mare (Nokia) ;
kallithea-general@sfconservancy.org
Subject: Re: [PATCH] kallithea pullrequests: allow limiting the number of
additional changes shown
Thank you.
Is the overwhelming on the server side or in the browser?
I don't like showing incorrect information. Lists must be reliable. It must be
made quite clear when lists have been truncated.
It would conceptually also be nice to be able to opt out of the truncation.
But that raise the questions: Why truncate from one end and not the other?
Could it be possible to give preference to truncate merged changesets before
truncating descendants? Doing further computation on a truncated list certainly
feels wrong. And why bother trying to show any list at all if it only shows a
random subset and thus can't be used for much?
Would it be a problem for your usecase to compute the full revision list, check
the amount of revisions and request.GET.get('show_additional_changes'), and
conditionally show "too long to be shown - click here to show" instead? (More
or less like how fulldiff is handled.)
Also: To minimize dependencies on tg, we prefer to consistently use
kallithea.CONFIG instead of tg.config (see kallithea/config/app_cfg.py ).
/Mads
On 05/12/2022 17:18, Mathias De Mare wrote:
> # HG changeset patch
> # User Mathias De Mare # Date 1669726298
> -3600
> # Tue Nov 29 13:51:38 2022 +0100
> # Node ID 2a5de196e40454dc9437e31e2224c183779a61d9
> # Parent 7037365a7bc351b81f05c790c6d3417d81d7bd5d
> kallithea pullrequests: allow limiting the number of additional
> changes shown
>
> We've noticed some scalability issues when many descendants exist for
> the changesets in a pull request.
> If we limit the number of proposed changesets to add to the review, we
> no longer overwhelm Kallithea in this case.
>
> (This occurred because we were merging a lot of heads in the
> repository.)
>
> diff --git a/kallithea/controllers/pullrequests.py
> b/kallithea/controllers/pullrequests.py
> --- a/kallithea/controllers/pullrequests.py
> +++ b/kallithea/controllers/pullrequests.py
> @@ -30,6 +30,7 @@ import traceback
>
> import formencode
> import mercurial.unionrepo
> +import tg
> from tg import request
> from tg import tmpl_context as c
> from tg.i18n import ugettext as _
> @@ -68,6 +69,14 @@ def _get_reviewer(user_id):
>
> return user
>
> +def _filter_additional_changes_revs(revs):
> +"""Allow hooking in to filter out some of the additional changes."""
> +additional_changes_rev_limit =
> tg.config.get('additional_changes_rev_limit')
> +if additional_changes_rev_limit and
> additional_changes_rev_limit.isnumeric():
> +new_revs = list(revs)
> +return new_revs[-int(additional_changes_rev_limit):]
> +else:
> +return revs
>
> class PullrequestsController(base.BaseRepoController):
>
> @@ -523,6 +532,7 @@ class PullrequestsController(base.BaseRe
> else: # look for descendants of PR head on source branch in
> org repo
> avail_revs = org_scm_instance._repo.revs('%s:: &
> branch(%s)',
>
> revs[0], c.cs_branch_name)
> +avail_revs =
> + _filter_additional_changes_revs(avail_revs)
> if len(avail_revs) > 1: # more than just revs[0]
> # also show changesets that not are descendants but
> would be merged in
> targethead =
> other_scm_instance.get_changeset(c.a_branch_name).raw_id
> @@ -537,6 +547,7 @@ class PullrequestsController(base.BaseRe
> hgrepo = org_scm_instance._repo
> show = set(hgrepo.revs('::%ld & !::parents(%s) &
> !::%s',
> avail_revs, revs[0],
> targethead))
> +show = _filter_additional_changes_revs(show)
> if show:
> c.update_msg = _('The following additional
> changes are available on %s:') % c.cs_branch_name
>
Re: [PATCH] kallithea pullrequests: allow limiting the number of additional changes shown
Thank you.
Is the overwhelming on the server side or in the browser?
I don't like showing incorrect information. Lists must be reliable. It
must be made quite clear when lists have been truncated.
It would conceptually also be nice to be able to opt out of the truncation.
But that raise the questions: Why truncate from one end and not the
other? Could it be possible to give preference to truncate merged
changesets before truncating descendants? Doing further computation on a
truncated list certainly feels wrong. And why bother trying to show any
list at all if it only shows a random subset and thus can't be used for
much?
Would it be a problem for your usecase to compute the full revision
list, check the amount of revisions and
request.GET.get('show_additional_changes'), and conditionally show "too
long to be shown - click here to show" instead? (More or less like how
fulldiff is handled.)
Also: To minimize dependencies on tg, we prefer to consistently use
kallithea.CONFIG instead of tg.config (see kallithea/config/app_cfg.py ).
/Mads
On 05/12/2022 17:18, Mathias De Mare wrote:
# HG changeset patch
# User Mathias De Mare
# Date 1669726298 -3600
# Tue Nov 29 13:51:38 2022 +0100
# Node ID 2a5de196e40454dc9437e31e2224c183779a61d9
# Parent 7037365a7bc351b81f05c790c6d3417d81d7bd5d
kallithea pullrequests: allow limiting the number of additional changes shown
We've noticed some scalability issues when many descendants exist
for the changesets in a pull request.
If we limit the number of proposed changesets to add to the review,
we no longer overwhelm Kallithea in this case.
(This occurred because we were merging a lot of heads in the repository.)
diff --git a/kallithea/controllers/pullrequests.py
b/kallithea/controllers/pullrequests.py
--- a/kallithea/controllers/pullrequests.py
+++ b/kallithea/controllers/pullrequests.py
@@ -30,6 +30,7 @@ import traceback
import formencode
import mercurial.unionrepo
+import tg
from tg import request
from tg import tmpl_context as c
from tg.i18n import ugettext as _
@@ -68,6 +69,14 @@ def _get_reviewer(user_id):
return user
+def _filter_additional_changes_revs(revs):
+"""Allow hooking in to filter out some of the additional changes."""
+additional_changes_rev_limit =
tg.config.get('additional_changes_rev_limit')
+if additional_changes_rev_limit and
additional_changes_rev_limit.isnumeric():
+new_revs = list(revs)
+return new_revs[-int(additional_changes_rev_limit):]
+else:
+return revs
class PullrequestsController(base.BaseRepoController):
@@ -523,6 +532,7 @@ class PullrequestsController(base.BaseRe
else: # look for descendants of PR head on source branch in
org repo
avail_revs = org_scm_instance._repo.revs('%s:: &
branch(%s)',
revs[0],
c.cs_branch_name)
+avail_revs = _filter_additional_changes_revs(avail_revs)
if len(avail_revs) > 1: # more than just revs[0]
# also show changesets that not are descendants but
would be merged in
targethead =
other_scm_instance.get_changeset(c.a_branch_name).raw_id
@@ -537,6 +547,7 @@ class PullrequestsController(base.BaseRe
hgrepo = org_scm_instance._repo
show = set(hgrepo.revs('::%ld & !::parents(%s) &
!::%s',
avail_revs, revs[0],
targethead))
+show = _filter_additional_changes_revs(show)
if show:
c.update_msg = _('The following additional
changes are available on %s:') % c.cs_branch_name
else:
diff --git a/kallithea/tests/functional/test_pullrequests.py
b/kallithea/tests/functional/test_pullrequests.py
--- a/kallithea/tests/functional/test_pullrequests.py
+++ b/kallithea/tests/functional/test_pullrequests.py
@@ -1,7 +1,9 @@
import re
+import mock
import pytest
+import kallithea.controllers
from kallithea.controllers.pullrequests import PullrequestsController
from kallithea.model import db, meta
from kallithea.tests import base
@@ -390,3 +392,18 @@ class TestPullrequestsGetRepoRefs(base.T
content='line1\n', message='commit1', vcs_type='hg',
parent=None, newfile=True)
# TODO
+
+class TestPullrequests(base.TestController):
+
+def test_filter_additional_changes_no_rev_limit(self):
+config_mock = {
+}
+with mock.patch('tg.config', config_mock):
+assert len(kallithea.controllers.pullrequests._filter_additional_changes_revs(["1",
"2", "3", "4"])) == 4
+
+def test_filter_additional_changes_rev_limit(self):
+config_mock = {
+'additional_changes_rev_limit': "2",
+}
+with mock.patch('tg.confi
[PATCH] kallithea pullrequests: allow limiting the number of additional changes shown
# HG changeset patch
# User Mathias De Mare
# Date 1669726298 -3600
# Tue Nov 29 13:51:38 2022 +0100
# Node ID 2a5de196e40454dc9437e31e2224c183779a61d9
# Parent 7037365a7bc351b81f05c790c6d3417d81d7bd5d
kallithea pullrequests: allow limiting the number of additional changes shown
We've noticed some scalability issues when many descendants exist
for the changesets in a pull request.
If we limit the number of proposed changesets to add to the review,
we no longer overwhelm Kallithea in this case.
(This occurred because we were merging a lot of heads in the repository.)
diff --git a/kallithea/controllers/pullrequests.py
b/kallithea/controllers/pullrequests.py
--- a/kallithea/controllers/pullrequests.py
+++ b/kallithea/controllers/pullrequests.py
@@ -30,6 +30,7 @@ import traceback
import formencode
import mercurial.unionrepo
+import tg
from tg import request
from tg import tmpl_context as c
from tg.i18n import ugettext as _
@@ -68,6 +69,14 @@ def _get_reviewer(user_id):
return user
+def _filter_additional_changes_revs(revs):
+"""Allow hooking in to filter out some of the additional changes."""
+additional_changes_rev_limit =
tg.config.get('additional_changes_rev_limit')
+if additional_changes_rev_limit and
additional_changes_rev_limit.isnumeric():
+new_revs = list(revs)
+return new_revs[-int(additional_changes_rev_limit):]
+else:
+return revs
class PullrequestsController(base.BaseRepoController):
@@ -523,6 +532,7 @@ class PullrequestsController(base.BaseRe
else: # look for descendants of PR head on source branch in
org repo
avail_revs = org_scm_instance._repo.revs('%s:: &
branch(%s)',
revs[0],
c.cs_branch_name)
+avail_revs = _filter_additional_changes_revs(avail_revs)
if len(avail_revs) > 1: # more than just revs[0]
# also show changesets that not are descendants but
would be merged in
targethead =
other_scm_instance.get_changeset(c.a_branch_name).raw_id
@@ -537,6 +547,7 @@ class PullrequestsController(base.BaseRe
hgrepo = org_scm_instance._repo
show = set(hgrepo.revs('::%ld & !::parents(%s) &
!::%s',
avail_revs, revs[0],
targethead))
+show = _filter_additional_changes_revs(show)
if show:
c.update_msg = _('The following additional changes
are available on %s:') % c.cs_branch_name
else:
diff --git a/kallithea/tests/functional/test_pullrequests.py
b/kallithea/tests/functional/test_pullrequests.py
--- a/kallithea/tests/functional/test_pullrequests.py
+++ b/kallithea/tests/functional/test_pullrequests.py
@@ -1,7 +1,9 @@
import re
+import mock
import pytest
+import kallithea.controllers
from kallithea.controllers.pullrequests import PullrequestsController
from kallithea.model import db, meta
from kallithea.tests import base
@@ -390,3 +392,18 @@ class TestPullrequestsGetRepoRefs(base.T
content='line1\n', message='commit1', vcs_type='hg',
parent=None, newfile=True)
# TODO
+
+class TestPullrequests(base.TestController):
+
+def test_filter_additional_changes_no_rev_limit(self):
+config_mock = {
+}
+with mock.patch('tg.config', config_mock):
+assert
len(kallithea.controllers.pullrequests._filter_additional_changes_revs(["1",
"2", "3", "4"])) == 4
+
+def test_filter_additional_changes_rev_limit(self):
+config_mock = {
+'additional_changes_rev_limit': "2",
+}
+with mock.patch('tg.config', config_mock):
+assert
len(kallithea.controllers.pullrequests._filter_additional_changes_revs(["1",
"2", "3", "4"])) == 2
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
