On 20/04/2023 14:55, Manuel Jacob wrote:
# HG changeset patch
# User Manuel Jacob <m...@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

_______________________________________________
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Reply via email to