( )⚙ D11675 bookmarks: move the `mirror` option to the `paths` section

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11675

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

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

Details

Reviewers
spectral
Alphare
Group Reviewers
hg-reviewers
Commits
rHG4d2ab365699e: bookmarks: move the `mirror` option to the `paths` section
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?

marmoute added a subscriber: valentin.gatienbaron.Oct 15 2021, 8:58 AM

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.

spectral added a subscriber: spectral.Oct 15 2021, 9:02 PM

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

Revision Contents
Changeset List

Diff 30873

mercurial/bookmarks.py

explicit.remove(b) explicit.remove(b)
ui.warn( ui.warn(
_(b"remote bookmark %s points to locally missing %s\n") _(b"remote bookmark %s points to locally missing %s\n")
% (b, hex(scid)[:12]) % (b, hex(scid)[:12])
) )
return changed return changed
def updatefromremote(ui, repo, remotemarks, path, trfunc, explicit=()): def updatefromremote(
ui, repo, remotemarks, path, trfunc, explicit=(), mode=None
):
ui.debug(b"checking for updated bookmarks\n") ui.debug(b"checking for updated bookmarks\n")
if ui.configbool(b'bookmarks', b'mirror'): if mode == b'mirror':
changed = mirroring_remote(ui, repo, remotemarks) changed = mirroring_remote(ui, repo, remotemarks)
else: else:
changed = merging_from_remote(ui, repo, remotemarks, path, explicit) changed = merging_from_remote(ui, repo, remotemarks, path, explicit)
if changed: if changed:
tr = trfunc() tr = trfunc()
changes = [] changes = []
key = lambda t: (t[0], t[1] or b'') key = lambda t: (t[0], t[1] or b'')

mercurial/configitems.py

) )
_registerdiffopts(section=b'annotate') _registerdiffopts(section=b'annotate')
# bookmarks.pushing: internal hack for discovery # bookmarks.pushing: internal hack for discovery
coreconfigitem( coreconfigitem(
b'bookmarks', b'bookmarks',
b'pushing', b'pushing',
default=list, default=list,
) )
coreconfigitem(
b'bookmarks',
b'mirror',
default=False,
)
# bundle.mainreporoot: internal hack for bundlerepo # bundle.mainreporoot: internal hack for bundlerepo
coreconfigitem( coreconfigitem(
b'bundle', b'bundle',
b'mainreporoot', b'mainreporoot',
default=b'', default=b'',
) )
coreconfigitem( coreconfigitem(
b'censor', b'censor',

mercurial/exchange.py

def _pullbookmarks(pullop): def _pullbookmarks(pullop):
"""process the remote bookmark information to update the local one""" """process the remote bookmark information to update the local one"""
if b'bookmarks' in pullop.stepsdone: if b'bookmarks' in pullop.stepsdone:
return return
pullop.stepsdone.add(b'bookmarks') pullop.stepsdone.add(b'bookmarks')
repo = pullop.repo repo = pullop.repo
remotebookmarks = pullop.remotebookmarks remotebookmarks = pullop.remotebookmarks
bookmarks_mode = None
if pullop.remote_path is not None:
bookmarks_mode = pullop.remote_path.bookmarks_mode
bookmod.updatefromremote( bookmod.updatefromremote(
repo.ui, repo.ui,
repo, repo,
remotebookmarks, remotebookmarks,
pullop.remote.url(), pullop.remote.url(),
pullop.gettransaction, pullop.gettransaction,
explicit=pullop.explicitbookmarks, explicit=pullop.explicitbookmarks,
mode=bookmarks_mode,
) )
def _pullobsolete(pullop): def _pullobsolete(pullop):
"""utility function to pull obsolete markers from a remote """utility function to pull obsolete markers from a remote
The `gettransaction` is function that return the pull transaction, creating The `gettransaction` is function that return the pull transaction, creating
one if necessary. We return the transaction to inform the calling code that one if necessary. We return the transaction to inform the calling code that

mercurial/helptext/config.txt

authentication entry with. Only used if the prefix doesn't include authentication entry with. Only used if the prefix doesn't include
a scheme. Supported schemes are http and https. They will match a scheme. Supported schemes are http and https. They will match
static-http and static-https respectively, as well. static-http and static-https respectively, as well.
(default: https) (default: https)
If no suitable authentication entry is found, the user is prompted If no suitable authentication entry is found, the user is prompted
for credentials as usual if required by the remote. for credentials as usual if required by the remote.
``bookmarks``
-------------
Controls some aspect of bookmarks.
``mirror``
When pulling, instead of merging local bookmarks and remote bookmarks,
replace local bookmarks by remote bookmarks. This is useful to replicate
a repository, or as an optimization. (default: False)
``cmdserver`` ``cmdserver``
------------- -------------
Controls command server settings. (ADVANCED) Controls command server settings. (ADVANCED)
``message-encodings`` ``message-encodings``
List of encodings for the ``m`` (message) channel. The first encoding List of encodings for the ``m`` (message) channel. The first encoding
supported by the server will be selected and advertised in the hello supported by the server will be selected and advertised in the hello
defined by this sub-option is evaluated to determine what to push. defined by this sub-option is evaluated to determine what to push.
For example, a value of ``.`` will push the working directory's For example, a value of ``.`` will push the working directory's
revision by default. revision by default.
Revsets specifying bookmarks will not result in the bookmark being Revsets specifying bookmarks will not result in the bookmark being
pushed. pushed.
``bookmarks.mode``
How bookmark will be dealt during the exchange. It support the following value
- ``default``: the default behavior, local and remote bookmarks are "merged"
on push/pull.
- ``mirror``: when pulling, replace local bookmarks by remote bookmarks. This
is useful to replicate a repository, or as an optimization.
The following special named paths exist: The following special named paths exist:
``default`` ``default``
The URL or directory to use when no source or remote is specified. The URL or directory to use when no source or remote is specified.
:hg:`clone` will automatically define this path to the location the :hg:`clone` will automatically define this path to the location the
repository was cloned from. repository was cloned from.

mercurial/utils/urlutil.py

return bytes(u) return bytes(u)
@pathsuboption(b'pushrev', b'pushrev') @pathsuboption(b'pushrev', b'pushrev')
def pushrevpathoption(ui, path, value): def pushrevpathoption(ui, path, value):
return value return value
SUPPORTED_BOOKMARKS_MODES = {
spectralUnsubmitted
Not Done

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

spectral: Nit: `KNOWN` instead of `KNOW`, and pluralize `MODES`. I'd also be fine with `VALID` or…
marmouteAuthorUnsubmitted
Done

Thanks for spotting this. updated version coming.

marmoute: Thanks for spotting this. updated version coming.
spectralUnsubmitted
Not Done

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

spectral: I still think this should be pluralized, since it's a set?
b'default',
b'mirror',
}
@pathsuboption(b'bookmarks.mode', b'bookmarks_mode')
def bookmarks_mode_option(ui, path, value):
if value not in SUPPORTED_BOOKMARKS_MODES:
path_name = path.name
spectralUnsubmitted
Not Done

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)

spectral: Sorry for not noticing this in the first review: should `as` be `has`? Would it make sense to…
marmouteAuthorUnsubmitted
Done

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

I fixed that. Thanks!

marmoute: It would make a lot of sense, I though I did. I fixed that. Thanks!
if path_name is None:
# this is an "anonymous" path, config comes from the global one
path_name = b'*'
msg = _(b'(paths.%s:bookmarks.mode has unknown value: "%s")\n')
msg %= (path_name, value)
ui.warn(msg)
if value == b'default':
value = None
return value
@pathsuboption(b'multi-urls', b'multi_urls') @pathsuboption(b'multi-urls', b'multi_urls')
def multiurls_pathoption(ui, path, value): def multiurls_pathoption(ui, path, value):
res = stringutil.parsebool(value) res = stringutil.parsebool(value)
if res is None: if res is None:
ui.warn( ui.warn(
_(b'(paths.%s:multi-urls not a boolean; ignoring)\n') % path.name _(b'(paths.%s:multi-urls not a boolean; ignoring)\n') % path.name
) )
res = False res = False

relnotes/next

== New Features == == New Features ==
* `debugrebuildfncache` now has an option to rebuild only the index files * `debugrebuildfncache` now has an option to rebuild only the index files
* a new `bookmarks.mode` path option have been introduced to control the
bookmark update strategy during exchange with a peer. See hg help paths for
details.
== Default Format Change == == Default Format Change ==
These changes affects newly created repositories (or new clone) done with These changes affects newly created repositories (or new clone) done with
Mercurial 6.0. Mercurial 6.0.
== New Experimental Features == == New Experimental Features ==
* `dirstate.remove` * `dirstate.remove`
* `dirstate.drop` * `dirstate.drop`
* `dirstate.__getitem__` * `dirstate.__getitem__`
Miscellaneous: Miscellaneous:
* `wireprotov1peer`'s `batchable` is now a simple function and not a generator * `wireprotov1peer`'s `batchable` is now a simple function and not a generator
anymore anymore
No newline at end of file No newline at end of file

tests/test-bookmarks-pushpull.t

X 1:9b140be10808 X 1:9b140be10808
X@foo 2:0d2164f0ce0d X@foo 2:0d2164f0ce0d
Y 0:4e3505fd9583 Y 0:4e3505fd9583
Z 2:0d2164f0ce0d Z 2:0d2164f0ce0d
foo -1:000000000000 foo -1:000000000000
* foobar 1:9b140be10808 * foobar 1:9b140be10808
$ cp .hg/bookmarks .hg/bookmarks.bak $ cp .hg/bookmarks .hg/bookmarks.bak
$ hg book -d X $ hg book -d X
$ hg pull ../a --config bookmarks.mirror=true $ hg pull ../a --config 'paths.*:bookmarks.mode=mirror'
pulling from ../a pulling from ../a
searching for changes searching for changes
no changes found no changes found
$ hg book $ hg book
@ 2:0d2164f0ce0d @ 2:0d2164f0ce0d
X 2:0d2164f0ce0d X 2:0d2164f0ce0d
Y 0:4e3505fd9583 Y 0:4e3505fd9583
Z 2:0d2164f0ce0d Z 2:0d2164f0ce0d

tests/test-help.t

by this sub-option is evaluated to determine what to push. by this sub-option is evaluated to determine what to push.
For example, a value of "." will push the working directory's revision For example, a value of "." will push the working directory's revision
by default. by default.
Revsets specifying bookmarks will not result in the bookmark being Revsets specifying bookmarks will not result in the bookmark being
pushed. pushed.
"bookmarks.mode"
How bookmark will be dealt during the exchange. It support the following
value
- "default": the default behavior, local and remote bookmarks are
"merged" on push/pull.
- "mirror": when pulling, replace local bookmarks by remote bookmarks.
This is useful to replicate a repository, or as an optimization.
The following special named paths exist: The following special named paths exist:
"default" "default"
The URL or directory to use when no source or remote is specified. The URL or directory to use when no source or remote is specified.
'hg clone' will automatically define this path to the location the 'hg clone' will automatically define this path to the location the
repository was cloned from. repository was cloned from.