internal_phase_migration.py

"""internal-phase migration support."""

# This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version.

from google_hgext import util
from mercurial import exthelper
from mercurial import localrepo
from mercurial import scmutil


eh = exthelper.exthelper()
old_name = b'internal-phase'

# disabled; TODO: enable in a subsequent release
# from mercurial import phases
# from mercurial import requirements
# @eh.reposetup
# def enable_internal_phase(ui, repo):
#   """Enable the internal-phase."""
#   del ui  # unused
#   if not is_fig(repo):
#     return
#   if phases.supportinternal(repo):
#     return

#   repo.requirements.add(requirements.INTERNAL_PHASE_REQUIREMENT)
#   scmutil.writereporequirements(repo)
#   assert phases.supportinternal(repo)


@eh.wrapfunction(localrepo, 'ensurerequirementsrecognized')
def check_reqs(orig, requirements_, supported):
  """Override localrepo.ensurerequirementsrecognized for compatibility.

  Make it appear as if 'internal-phase' is supported
  on later versions of Mercurial where it's been removed.

  Avoids a crash when starting up on a repo with the
  deprecated 'internal-phase' so that
  ``remove_internal_phase_requirement`` can effect
  its change.

  Args:
    orig: original function
    requirements_: same as orig
    supported: same as orig

  Returns:
    same as orig
  """
  return orig(requirements_, supported | {old_name})


@eh.reposetup
def remove_internal_phase_requirement(ui, repo):
  """Remove any internal-phase requirement.

  In 74fb1842f8b9, Mercurial renamed the requirement for enabling the internal
  phase. This change happened during Google's adoption of the feature,
  so the current codebase also expects to support the original name.
  Remove the old requirement.

  Remove this code after the internal phase work has stabilized.

  Args:
    ui: the ui
    repo: the repo

  Returns:
    nothing
  """
  if not util.is_fig(repo):
    return
  try:
    repo.requirements.remove(old_name)
  except KeyError:
    return
  try:
    scmutil.writereporequirements(repo)
  except PermissionError:
    ui.log(b'shelve', b'no permission to update repository requirements\n')
  except Exception as exc:  # pylint: disable=broad-except
    msg = f'phase migration failed to remove internal-phase ({exc!r})\n'
    bmsg = msg.encode()
    ui.warn(bmsg)
    ui.log(b'shelve', bmsg)

If I understand your code correctly in remove_internal_phase_requirement seems to make sure the old requirements is dropped, but does not inject the newer requirements, so you would move from a internal-phase compatible repo (with the new requirement) to a not internal-phase compatible repository (without any requirements).

In addition, the except Exception behavior seems a bit wide if you can encounter permission error. Code along this line should be better.

Something along this line should work.

if old_names & repo.requirements:
    repo.requirements.difference_update(old_names)
    repo.requirements.update(new_names)
    try:
        scmutil.writereporequirements(repo)
    except PermissionError:
        ui.debug(b'no permission to update repository requirements')
    except Exception:  # pylint: disable=broad-except
        ui.warn(b'phase migration failed to remove internal-phase')

Regarding migration windows, You could have a first version of this that ensure the old internal-phase is written an disk, but the version understand it anyway. Then when 6.2 is out and newer version are sufficiently deployed, you switch it to writing the internal-phase-2 requirements on disk.
Since you are doing things in reposetup, you have access to the ui and could easily control the direction of the behavior through the config.

I think we've decided that any repos with internal-phase will simply lose that feature (and have to fall back to local disk shelves). From there, we're focused entirely now on getting rollback-safe support for internal-phase-2 (first cut a release that recognizes the requirement from unstable HEAD), then in a subsequent release, re-enable that feature.

This approach does mean that users who were experimenting with internal-phase will have the feature disabled for this transition Windows and we're okay with that tradeoff (confirming your first paragraph above).

As for writing requirements, we're aiming to write requirements as little as possible (because of the potential for things to go wrong), so we're focusing on mainly adding the requirement just once after support for it has been rolled out for at least one release.

Good tips on PermissionError vs. a broad Exception.

The latest revision reflects the implementation we've adopted.