[PATCH] hg: add settings to make repositories non-publishing or use changeset evolution
Mads Kiilerich
mads at kiilerich.com
Sun Apr 23 14:41:14 UTC 2023
On 20/04/2023 14:55, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me at manueljacob.de>
> # Date 1681995256 -7200
> # Thu Apr 20 14:54:16 2023 +0200
> # Node ID 56aad162afcff89ec7e0b8cc7a60fff2bbde6ec1
> # Parent 294f5a6814edc267c87a77dad5131c08a50fae1c
> # EXP-Topic non_publishing_changeset_evolution
> hg: add settings to make repositories non-publishing or use changeset evolution
Thanks. I'm absolutely positive to the publishing part of this change.
I have more concerns about the evolution part.
For that reason I would like to have it split up so they can land
separately. While the code changes are very similar, they touch features
that have very different maturity.
> Setting repositories non-publishing and enabling changeset evolution makes it
> possible to refine changesets collaboratively. Doing both at the same time will
> therefore probably be the most common use case.
>
> There are some users that push to-be-reviewed changesets to Kallithea
> repositories, but don’t need or want obsolescence information to be shared.
> Those users can now enable use of phases by setting the repository
> non-publishing if they didn’t already do that by modifying the repository’s
> hgrc.
I don't know about obsolescence information, but I think it is relevant
to point out that this part of these changes is about controlling
(disabling) phases.publishing , and that phases can be used as
safetyguard independent of any use of evolve and obsolescence. Since
repositories (and thus Kallithea) defaults to publishing and puts on
safetyguards that can get in the way, it is nice to be able to disable
it in the UI (and not just in the repo .hg/hgrc).
> Having a publishing repository with changeset evolution can also be useful. For
> example, there could be a project that has the committed changesets in a
> publishing repository and uses email-based patch contributions. When a
> maintainer commits a patch, possibly after some modifications, Mercurial has
> the possibility to automatically add and share obsolescence information stating
> that the committed changeset is a successor of the contributor’s original local
> changeset that the patch was created from.
Regarding the evolution part of the change: I can see it (currently) is
all about setting [experimental] evolution=all . That seems to be an
undocumented Mercurial (or evolve?) flag, so it is really not clear what
it does - both in general and for Kallithea operations in Web Ui and
repo hosting. The relation to the unofficial evolve extension is not
clear to me. The option is in the experimental namespace, so it doesn't
seem like a thing to expose in the UI in a product that aim to be
stable. It will presumably only be supported in some hg versions. It
would be more relevant to support it in Kallithea if it was a proper
Mercurial feature.
Some of these concerns can perhaps be addressed in the Kallithea UI or
in commit message or code comments, but some of it would perhaps be
better solved in upstream code and documentation that this change could
reference.
> Previously, there were no settings in the web interface that modify
> per-repository Mercurial configs and no per-repository settings that apply to
> hg repositories but not git repositories. Therefore, when adding these
> settings, there was no pattern to follow and a lot of different design and
> implementation choices could have been taken.
It is true that Kallithea so far doesn't have UI for per repo settings.
It has always been possible to do this per repo by putting configuration
inside the server side repos .hg/hgrc , or globally with manual entries
in the Ui table.
One concern was how/where to store the info, in a way that scaled to all
the things users possibly could want to set for each repo. Also,
managing settings on each repo individually could be annoying. There
could be very relevant use cases for settings on groups that were
inherited to all repos inside the group.
Boolean fields in the Repository db records is probably a fine pragmatic
solution.
BUT it will need a db migration step, per docs/dev/dbmigrations.rst .
The new default repo settings also has to be considered - I'm not sure
they would need migration steps too. (And this need for a manual db step
do what we can't sneak this change in on the stable branch - it will
have to go to the default branch.)
> diff --git a/kallithea/controllers/api/api.py b/kallithea/controllers/api/api.py
> --- a/kallithea/controllers/api/api.py
> +++ b/kallithea/controllers/api/api.py
> @@ -928,6 +928,8 @@
> landing_rev='rev:tip',
> enable_statistics=None,
> enable_downloads=None,
> + publishing=None,
> + enable_changeset_evolution=None,
This should probably be documented in api.rst .
Here I also notice a design decision: variables (and database fields
etc) are named 'publishing', which seems a bit more generic than the
current semantics of meaning exactly the same as setting
phases.publishing for a Mercurial repo. That is probably fine and could
allow the flag to do more than that, even though backwards compatibility
would have to be considered.
(This might be even more true for enable_changeset_evolution. Also, the
consistent use of 'enable' all the way down to the database field might
be a bit redundant for a flag. But I don't have an opinion on that yet.)
> copy_permissions=False):
> """
> Creates a repository. The repository name contains the full path, but the
> @@ -982,6 +984,10 @@
> enable_statistics = defs.get('repo_enable_statistics')
> if enable_downloads is None:
> enable_downloads = defs.get('repo_enable_downloads')
> + if publishing is None:
> + publishing = defs.get('repo_publishing')
> + if enable_changeset_evolution is None:
> + enable_changeset_evolution = defs.get('repo_enable_changeset_evolution')
(Annoying that it has to be done here instead of in a consistent model
layer. But here and now we have to be consistent and play by the current
rules. A cleanup would be a different story ... and possibly not feasible.)
> try:
> data = dict(
> @@ -995,6 +1001,8 @@
> repo_landing_rev=landing_rev,
> repo_enable_statistics=enable_statistics,
> repo_enable_downloads=enable_downloads,
> + repo_publishing = publishing,
> + repo_enable_changeset_evolution=enable_changeset_evolution,
> repo_copy_permissions=copy_permissions,
> )
>
> @@ -1017,7 +1025,9 @@
> description=None, private=None,
> clone_uri=None, landing_rev=None,
> enable_statistics=None,
> - enable_downloads=None):
> + enable_downloads=None,
> + publishing=None,
> + enable_changeset_evolution=None):
> """
> Updates repo
> """
> @@ -1055,6 +1065,8 @@
> store_update(updates, landing_rev, 'repo_landing_rev')
> store_update(updates, enable_statistics, 'repo_enable_statistics')
> store_update(updates, enable_downloads, 'repo_enable_downloads')
> + store_update(updates, publishing, 'repo_publishing')
> + store_update(updates, enable_changeset_evolution, 'repo_enable_changeset_evolution')
>
> RepoModel().update(repo, **updates)
> meta.Session().commit()
> diff --git a/kallithea/lib/db_manage.py b/kallithea/lib/db_manage.py
> --- a/kallithea/lib/db_manage.py
> +++ b/kallithea/lib/db_manage.py
> @@ -178,6 +178,8 @@
> for k, v, t in [
> ('default_repo_enable_downloads', False, 'bool'),
> ('default_repo_enable_statistics', False, 'bool'),
> + ('default_repo_publishing', True, 'bool'),
> + ('default_repo_enable_changeset_evolution', False, 'bool'),
> ('default_repo_private', False, 'bool'),
> ('default_repo_type', 'hg', 'unicode')
> ]:
> diff --git a/kallithea/lib/utils.py b/kallithea/lib/utils.py
> --- a/kallithea/lib/utils.py
> +++ b/kallithea/lib/utils.py
> @@ -287,6 +287,12 @@
> baseui.setconfig(b'hooks', ascii_bytes(db.Ui.HOOK_UPDATE), b'python:kallithea.bin.vcs_hooks.update')
>
> if repo_path is not None:
> + db_repo = db.Repository.get_by_full_path(repo_path)
> + if db_repo is not None:
In what cases will we create a Ui for a repo that doesn't have a db
entry yet?
And in these cases, wouldn't it be better to use the configured defaults?
> + baseui.setconfig(b'phases', b'publish', db_repo.publishing)
> + if db_repo.enable_changeset_evolution:
> + baseui.setconfig(b'experimental', b'evolution', b'all')
> +
> # Note: MercurialRepository / mercurial.localrepo.instance will do this too, so it will always be possible to override db settings or what is hardcoded above
IIRC, there are some code paths in Mercurial where repo UIs are
created/copied and it invokes readconfig without using this UI factory.
Check carefully that the db settings are applied in all relevant cases.
> baseui.readconfig(safe_bytes(os.path.join(repo_path, '.hg', 'hgrc')))
>
> @@ -403,6 +409,8 @@
> defs = db.Setting.get_default_repo_settings(strip_prefix=True)
> enable_statistics = defs.get('repo_enable_statistics')
> enable_downloads = defs.get('repo_enable_downloads')
> + publishing = defs.get('repo_publishing')
> + enable_changeset_evolution = defs.get('repo_enable_changeset_evolution')
> private = defs.get('repo_private')
>
> for name, repo in sorted(initial_repo_dict.items()):
> @@ -425,6 +433,8 @@
> owner=user,
> enable_downloads=enable_downloads,
> enable_statistics=enable_statistics,
> + publishing=publishing,
> + enable_changeset_evolution=enable_changeset_evolution,
> private=private,
> state=db.Repository.STATE_CREATED
> )
> diff --git a/kallithea/model/db.py b/kallithea/model/db.py
> --- a/kallithea/model/db.py
> +++ b/kallithea/model/db.py
> @@ -922,6 +922,8 @@
> private = Column(Boolean(), nullable=False)
> enable_statistics = Column("statistics", Boolean(), nullable=False, default=True)
> enable_downloads = Column("downloads", Boolean(), nullable=False, default=True)
> + publishing = Column("publishing", Boolean(), nullable=False, default=True)
> + enable_changeset_evolution = Column("changeset_evolution", Boolean(), nullable=False, default=False)
> description = Column(Unicode(10000), nullable=False)
> created_on = Column(DateTime(timezone=False), nullable=False, default=datetime.datetime.now)
> updated_on = Column(DateTime(timezone=False), nullable=False, default=datetime.datetime.now)
> diff --git a/kallithea/model/forms.py b/kallithea/model/forms.py
> --- a/kallithea/model/forms.py
> +++ b/kallithea/model/forms.py
> @@ -264,6 +264,8 @@
>
> repo_enable_statistics = v.StringBoolean(if_missing=False)
> repo_enable_downloads = v.StringBoolean(if_missing=False)
> + repo_publishing = v.StringBoolean(if_missing=False)
I guess this (and below) should be True
> + repo_enable_changeset_evolution = v.StringBoolean(if_missing=False)
>
> if edit:
> owner = All(v.UnicodeString(not_empty=True), v.ValidRepoUser())
> @@ -440,6 +442,8 @@
> default_repo_private = v.StringBoolean(if_missing=False)
> default_repo_enable_statistics = v.StringBoolean(if_missing=False)
> default_repo_enable_downloads = v.StringBoolean(if_missing=False)
> + default_repo_publishing = v.StringBoolean(if_missing=False)
> + default_repo_enable_changeset_evolution = v.StringBoolean(if_missing=False)
>
> return _DefaultsForm
>
> diff --git a/kallithea/model/repo.py b/kallithea/model/repo.py
> --- a/kallithea/model/repo.py
> +++ b/kallithea/model/repo.py
> @@ -217,7 +217,9 @@
> for strip, k in [(0, 'repo_type'), (1, 'repo_enable_downloads'),
> (1, 'repo_description'),
> (1, 'repo_landing_rev'), (0, 'clone_uri'),
> - (1, 'repo_private'), (1, 'repo_enable_statistics')]:
> + (1, 'repo_private'), (1, 'repo_enable_statistics'),
> + (1, 'repo_publishing'),
> + (1, 'repo_enable_changeset_evolution')]:
> attr = k
> if strip:
> attr = remove_prefix(k, 'repo_')
> @@ -266,6 +268,8 @@
> 'repo_landing_rev',
> 'repo_private',
> 'repo_enable_statistics',
> + 'repo_publishing',
> + 'repo_enable_changeset_evolution',
> ]:
> if k in kwargs:
> setattr(cur_repo, remove_prefix(k, 'repo_'), kwargs[k])
> @@ -309,7 +313,8 @@
> private=False, clone_uri=None, repo_group=None,
> landing_rev='rev:tip', fork_of=None,
> copy_fork_permissions=False, enable_statistics=False,
> - enable_downloads=False,
> + enable_downloads=False, publishing=True,
> + enable_changeset_evolution=False,
> copy_group_permissions=False, state=db.Repository.STATE_PENDING):
> """
> Create repository inside database with PENDING state. This should only be
> @@ -345,6 +350,8 @@
>
> new_repo.enable_statistics = enable_statistics
> new_repo.enable_downloads = enable_downloads
> + new_repo.publishing = publishing
> + new_repo.enable_changeset_evolution = enable_changeset_evolution
>
> if fork_of:
> parent_repo = fork_of
> @@ -713,6 +720,8 @@
> fork_of = form_data.get('fork_parent_id')
> enable_statistics = form_data['repo_enable_statistics']
> enable_downloads = form_data['repo_enable_downloads']
> + publishing = form_data['repo_publishing']
> + enable_changeset_evolution = form_data['repo_enable_changeset_evolution']
> state = form_data.get('repo_state', db.Repository.STATE_PENDING)
>
> try:
> @@ -730,6 +739,8 @@
> copy_group_permissions=copy_group_permissions,
> enable_statistics=enable_statistics,
> enable_downloads=enable_downloads,
> + publishing=publishing,
> + enable_changeset_evolution=enable_changeset_evolution,
> state=state
> )
>
> diff --git a/kallithea/templates/admin/defaults/defaults.html b/kallithea/templates/admin/defaults/defaults.html
> --- a/kallithea/templates/admin/defaults/defaults.html
> +++ b/kallithea/templates/admin/defaults/defaults.html
> @@ -53,6 +53,20 @@
> <span class="help-block">${_('Enable download menu on summary page.')}</span>
> </div>
> </div>
> + <div class="form-group">
> + <label class="control-label" for="default_repo_publishing">${_('Publishing repository (Mercurial-specific)')}:</label>
> + <div>
> + ${h.checkbox('default_repo_publishing',value="True")}
> + <span class="help-block">${_('Set phase of changesets pushed to server to public.')}</span>
> + </div>
> + </div>
> + <div class="form-group">
> + <label class="control-label" for="default_repo_enable_changeset_evolution">${_('Enable changeset evolution (Mercurial-specific)')}:</label>
> + <div>
> + ${h.checkbox('default_repo_enable_changeset_evolution',value="True")}
> + <span class="help-block">${_('Enable all changeset evolution features.')}</span>
> + </div>
> + </div>
>
> <div class="form-group">
> <div class="buttons">
> diff --git a/kallithea/templates/admin/repos/repo_add_base.html b/kallithea/templates/admin/repos/repo_add_base.html
> --- a/kallithea/templates/admin/repos/repo_add_base.html
> +++ b/kallithea/templates/admin/repos/repo_add_base.html
> @@ -71,6 +71,22 @@
> <span class="help-block">${_('Enable download menu on summary page.')}</span>
> </div>
> </div>
> + <div id="hg_specific_settings">
> + <div class="form-group">
> + <label class="control-label" for="repo_publishing">${_('Publishing repository')}:</label>
> + <div>
> + ${h.checkbox('repo_publishing',value="True")}
> + <span class="help-block">${_('Set phase of changesets pushed to server to public.')}</span>
I wonder if it would be more clear and explicit to say something like
"Make local changesets public when they are pushed to the server" or
"Set phase of changesets to 'public' when they are pushed to the server".
> + </div>
> + </div>
> + <div class="form-group">
> + <label class="control-label" for="repo_enable_changeset_evolution">${_('Enable changeset evolution')}:</label>
> + <div>
> + ${h.checkbox('repo_enable_changeset_evolution',value="True")}
> + <span class="help-block">${_('Enable all changeset evolution features.')}</span>
> + </div>
> + </div>
> + </div>
> <div class="form-group">
> <div class="buttons">
> ${h.submit('add',_('Add'),class_="btn btn-default")}
> @@ -103,6 +119,20 @@
> setCopyPermsOption(e.val);
> });
>
> + function setVcsSpecificSettings(repo_type){
> + if(repo_type == "hg"){
> + $('#hg_specific_settings').show();
> + }
> + else{
> + $('#hg_specific_settings').hide();
> + }
> + }
Items that appear and disappear in the UI can be annoying. Would it
perhaps better to just disable these flags when creating git repos? But
ok, they could also be annoying to look at for those who only git repos...
> +
> + setVcsSpecificSettings($('#repo_type').val());
> + $('#repo_type').on("change", function(e) {
> + setVcsSpecificSettings(e.val);
> + });
Does this work as intended on page load, no matter what the default repo
type is?
> +
> $('#repo_landing_rev').select2({
> 'minimumResultsForSearch': -1
> });
> diff --git a/kallithea/templates/admin/repos/repo_edit_settings.html b/kallithea/templates/admin/repos/repo_edit_settings.html
> --- a/kallithea/templates/admin/repos/repo_edit_settings.html
> +++ b/kallithea/templates/admin/repos/repo_edit_settings.html
> @@ -76,6 +76,22 @@
> <span class="help-block">${_('Enable download menu on summary page.')}</span>
> </div>
> </div>
> + %if c.repo_info.repo_type == 'hg':
> + <div class="form-group">
> + <label class="control-label" for="repo_publishing">${_('Publishing repository')}:</label>
> + <div>
> + ${h.checkbox('repo_publishing',value="True")}
> + <span class="help-block">${_('Set phase of changesets pushed to server to public.')}</span>
> + </div>
> + </div>
> + <div class="form-group">
> + <label class="control-label" for="repo_enable_changeset_evolution">${_('Enable changeset evolution')}:</label>
> + <div>
> + ${h.checkbox('repo_enable_changeset_evolution',value="True")}
> + <span class="help-block">${_('Enable all changeset evolution features.')}</span>
> + </div>
> + </div>
> + %endif
>
> %if c.visual.repository_fields:
> ## EXTRA FIELDS
> diff --git a/kallithea/tests/conftest.py b/kallithea/tests/conftest.py
> --- a/kallithea/tests/conftest.py
> +++ b/kallithea/tests/conftest.py
> @@ -75,11 +75,12 @@
> if not int(os.environ.get('KALLITHEA_NO_TMP_PATH', 0)):
> create_test_env(TESTS_TMP_PATH, context.config(), reuse_database=bool(reuse_database))
>
> + kallithea.tests.base.testapp = context.create()
> +
> # set KALLITHEA_WHOOSH_TEST_DISABLE=1 to disable whoosh index during tests
> if not int(os.environ.get('KALLITHEA_WHOOSH_TEST_DISABLE', 0)):
> create_test_index(TESTS_TMP_PATH, context.config(), True)
>
> - kallithea.tests.base.testapp = context.create()
> # do initial repo scan
> repo2db_mapper(ScmModel().repo_scan(TESTS_TMP_PATH))
Why this change? It seems unrelated to the other changes. And it seems
like a good idea to create indices before creating/starting the app.
/Mads
More information about the kallithea-general
mailing list