Page MenuHomePhabricator

bookmarks: move the `mirror` option to the `paths` section
ClosedPublic

Authored by marmoute on Oct 15 2021, 4:18 AM.

Details

Summary

A new bookmarks section with a mirror option have been added. That option
has never been released yet.

This new options is limited since it affect all paths without distinction. In
case where a repository is interacting with multiple peers, being able to
control behavior on a path basis can be quite valuable.

In addition, having more variant of behavior would be interesting, especially a
mode where no bookmark exchanged is tried at all. Such new mode (implemented
later) make a lot of sense for configuration on a path-basis.

Configuration of the default behavior is still possible through the usage of
generic path configuration. The "old" config, becomes:

[bookmarks]
mirror=True

becomes:

[path]
*:bookmarks.mode=mirror

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

marmoute created this revision.Oct 15 2021, 4:18 AM
pulkit added a subscriber: pulkit.Oct 15 2021, 8:56 AM

Can we add about this in relnotes?

cc @valentin.gatienbaron

I mentionned this very quickly to you a while back, here is an actual implementation. The new proposal provide the same feature you need while being more versatile that fit some of the real life usecase I have been seeing. Especially when combined with D11677.

Looks good to me aside from the nit and the pulkit's comment. Not sure on etiquette here (whether I should mark as accepted or not), defaulting to not marking as accepted until all outstanding comments are addressed.

mercurial/utils/urlutil.py
769

Nit: KNOWN instead of KNOW, and pluralize MODES. I'd also be fine with VALID or SUPPORTED instead of KNOWN.

marmoute added inline comments.Oct 17 2021, 9:07 AM
mercurial/utils/urlutil.py
769

Thanks for spotting this. updated version coming.

marmoute updated this revision to Diff 30845.Oct 17 2021, 6:41 PM
spectral accepted this revision.Oct 17 2021, 7:37 PM
spectral added inline comments.
mercurial/utils/urlutil.py
769

I still think this should be pluralized, since it's a set?

778

Sorry for not noticing this in the first review: should as be has?

Would it make sense to put the actual value %s in quotes in case it's empty, to avoid a message like:

(paths.foo:bookmarks.mode has unknown value )

(Having the message be enclosed in () makes this less necessary, so I'm fine either way)

marmoute updated this revision to Diff 30850.Oct 18 2021, 3:21 AM
marmoute added inline comments.Oct 18 2021, 3:21 AM
mercurial/utils/urlutil.py
778

It would make a lot of sense, I though I did.

I fixed that. Thanks!

marmoute updated this revision to Diff 30858.Oct 18 2021, 6:02 AM
Alphare accepted this revision.Oct 18 2021, 6:04 AM
This revision is now accepted and ready to land.Oct 18 2021, 6:04 AM
marmoute updated this revision to Diff 30864.Oct 18 2021, 6:21 AM