This is an archive of the discontinued Mercurial Phabricator instance.

share: rework config options to be much clearer and easier
ClosedPublic

Authored by pulkit on Jan 15 2021, 1:47 AM.

Details

Summary

Recently I implemented various boolean configs which control how to behave when
there is a share-safe mismatch between source and share repository. Mismatch
means that source supports share-safe where as share does not or vice versa.

However, while discussion and documentation we realized that it's too
complicated and there are some combinations of values which makes no sense.

We decided to introduce a config option with 4 possible values which
makes controlling and understanding things easier.

The config option share.safe-mismatch.source-{not-}safe can have
following 4 values:

  • abort (default): error out if there is mismatch
  • allow: allow to work with respecting share source configuration
  • {up|down}grade-abort: try to {up|down}grade, if it fails, abort
  • {up|down}grade-allow: try to {up|down}grade, if it fails, continue in allow

mode

I am not sure if I can explain 3 config options which I deleted right now in
just 5 lines which is a sign of how complex they became.

No test changes demonstrate that functionality is same, only names have changed.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Jan 15 2021, 1:47 AM
marmoute added inline comments.
mercurial/localrepo.py
597–615

If you have a un-recognised value, you should display it.

629–635

This is a bit weird. I would expect something like:

upgraded = False
if upgrade:
    try:
        upgrade()
    else:
        upgraded = True
    except UpgradeError:
        if upgrade_abort:
            raise Abort(…)
if not upgraded:
    behavior_upgrade()

Am I missing something ?

pulkit added inline comments.Jan 15 2021, 7:09 AM
mercurial/localrepo.py
629–635

That logic lives in upgrade.upgrade_share_to_safe(). We only handle abort case here.

marmoute accepted this revision.Jan 15 2021, 2:45 PM

The code organization is a bit weird as I feel like we should only talk about upgrade for the process of actually touching the file on disk. Otherwise, we are only applying requirements logic.

I would prefer this got clarified a bit, but this is not a strong blocker.

marmoute requested changes to this revision.Jan 18 2021, 4:56 AM

We need a distinct option for upgrading and downgrading. The "risk" associated with each operation is quite different and should be dealt with differently.

This revision now requires changes to proceed.Jan 18 2021, 4:56 AM
pulkit updated this revision to Diff 25116.Jan 18 2021, 10:19 AM
pulkit updated this revision to Diff 25121.Jan 18 2021, 10:41 AM

The requested distinction happen as a follow up in D9824. It seems fine to me.

marmoute accepted this revision.Jan 18 2021, 10:59 AM
pulkit retitled this revision from share: collapse 3 different bool configs into one enum config to share: rework config options to be much clearer and easier.Jan 18 2021, 11:12 AM
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 25126.

The requested distinction happen as a follow up in D9824. It seems fine to me.

Folded into one.

marmoute accepted this revision.Jan 18 2021, 11:47 AM
mharbison72 added inline comments.
mercurial/localrepo.py
599

Unrelated to this patch, but maybe we should interpolate requirementsmod.SHARESAFE_REQUIREMENT so this doesn't get forgotten about when renaming the requirement?

mharbison72 accepted this revision.Jan 18 2021, 9:08 PM
This revision is now accepted and ready to land.Jan 18 2021, 9:08 PM