This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11676

bookmarks: add support for `mirror` mode to `incoming`
ClosedPublic

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

Details

Reviewers
spectral
Alphare
Group Reviewers
hg-reviewers
Commits
rHG76c071bba40d: bookmarks: add support for `mirror` mode to `incoming`
Summary

This is more consistent.

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
spectral added a subscriber: spectral.Oct 15 2021, 9:18 PM
spectral added inline comments.
mercurial/bookmarks.py
816

I find the single line version of these functions much easier to read.

825

Nit: use set(localmarks) instead of set(repo._bookmarks)?

Also, perhaps I'm less familiar with idiomatic python and this isn't confusing for others, but would it make sense to explicitly use .keys() (so we have allmarks = set(remotemarks.keys()) | set(localmarks.keys())? It would have likely avoided a small amount of confusion I had a few lines below.

marmoute added inline comments.Oct 17 2021, 9:11 AM
mercurial/bookmarks.py
825

I think set(dict) is quite idiomatic, but using .keys is indeed clearer so lets do it.

marmoute updated this revision to Diff 30846.Oct 17 2021, 6:41 PM
spectral accepted this revision.Oct 17 2021, 7:34 PM
marmoute updated this revision to Diff 30851.Oct 18 2021, 3:21 AM
Alphare accepted this revision.Oct 18 2021, 6:05 AM
This revision is now accepted and ready to land.Oct 18 2021, 6:05 AM
marmoute added a commit: rHG76c071bba40d: bookmarks: add support for `mirror` mode to `incoming`.Oct 18 2021, 7:53 AM
Closed by commit rHG76c071bba40d: bookmarks: add support for `mirror` mode to `incoming` (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 30874

mercurial/bookmarks.py

changes = [] changes = []
key = lambda t: (t[0], t[1] or b'') key = lambda t: (t[0], t[1] or b'')
for b, node, writer, msg in sorted(changed, key=key): for b, node, writer, msg in sorted(changed, key=key):
changes.append((b, node)) changes.append((b, node))
writer(msg) writer(msg)
repo._bookmarks.applychanges(repo, tr, changes) repo._bookmarks.applychanges(repo, tr, changes)
def incoming(ui, repo, peer): def incoming(ui, repo, peer, mode=None):
"""Show bookmarks incoming from other to repo""" """Show bookmarks incoming from other to repo"""
ui.status(_(b"searching for changed bookmarks\n")) ui.status(_(b"searching for changed bookmarks\n"))
with peer.commandexecutor() as e: with peer.commandexecutor() as e:
remotemarks = unhexlifybookmarks( remotemarks = unhexlifybookmarks(
e.callcommand( e.callcommand(
b'listkeys', b'listkeys',
{ {
b'namespace': b'bookmarks', b'namespace': b'bookmarks',
}, },
).result() ).result()
) )
r = comparebookmarks(repo, remotemarks, repo._bookmarks)
addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
incomings = [] incomings = []
if ui.debugflag: if ui.debugflag:
getid = lambda id: id getid = lambda id: id
else: else:
getid = lambda id: id[:12] getid = lambda id: id[:12]
if ui.verbose: if ui.verbose:
def add(b, id, st): def add(b, id, st):
incomings.append(b" %-25s %s %s\n" % (b, getid(id), st)) incomings.append(b" %-25s %s %s\n" % (b, getid(id), st))
spectralUnsubmitted
Not Done

I find the single line version of these functions much easier to read.

spectral: I find the single line version of these functions much easier to read.
else: else:
def add(b, id, st): def add(b, id, st):
incomings.append(b" %-25s %s\n" % (b, getid(id))) incomings.append(b" %-25s %s\n" % (b, getid(id)))
if mode == b'mirror':
localmarks = repo._bookmarks
allmarks = set(remotemarks.keys()) | set(localmarks.keys())
spectralUnsubmitted
Not Done

Nit: use set(localmarks) instead of set(repo._bookmarks)?

Also, perhaps I'm less familiar with idiomatic python and this isn't confusing for others, but would it make sense to explicitly use .keys() (so we have allmarks = set(remotemarks.keys()) | set(localmarks.keys())? It would have likely avoided a small amount of confusion I had a few lines below.

spectral: Nit: use `set(localmarks)` instead of `set(repo._bookmarks)`? Also, perhaps I'm less familiar…
marmouteAuthorUnsubmitted
Done

I think set(dict) is quite idiomatic, but using .keys is indeed clearer so lets do it.

marmoute: I think set(dict) is quite idiomatic, but using .keys is indeed clearer so lets do it.
for b in sorted(allmarks):
loc = localmarks.get(b)
rem = remotemarks.get(b)
if loc == rem:
continue
elif loc is None:
add(b, hex(rem), _(b'added'))
elif rem is None:
add(b, hex(repo.nullid), _(b'removed'))
else:
add(b, hex(rem), _(b'changed'))
else:
r = comparebookmarks(repo, remotemarks, repo._bookmarks)
addsrc, adddst, advsrc, advdst, diverge, differ, invalid, same = r
for b, scid, dcid in addsrc: for b, scid, dcid in addsrc:
# i18n: "added" refers to a bookmark # i18n: "added" refers to a bookmark
add(b, hex(scid), _(b'added')) add(b, hex(scid), _(b'added'))
for b, scid, dcid in advsrc: for b, scid, dcid in advsrc:
# i18n: "advanced" refers to a bookmark # i18n: "advanced" refers to a bookmark
add(b, hex(scid), _(b'advanced')) add(b, hex(scid), _(b'advanced'))
for b, scid, dcid in diverge: for b, scid, dcid in diverge:
# i18n: "diverged" refers to a bookmark # i18n: "diverged" refers to a bookmark
add(b, hex(scid), _(b'diverged')) add(b, hex(scid), _(b'diverged'))
for b, scid, dcid in differ: for b, scid, dcid in differ:
# i18n: "changed" refers to a bookmark # i18n: "changed" refers to a bookmark
add(b, hex(scid), _(b'changed')) add(b, hex(scid), _(b'changed'))
if not incomings: if not incomings:
ui.status(_(b"no changed bookmarks found\n")) ui.status(_(b"no changed bookmarks found\n"))
return 1 return 1
for s in sorted(incomings): for s in sorted(incomings):
ui.write(s) ui.write(s)

mercurial/commands.py

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
try: try:
if b'bookmarks' not in other.listkeys(b'namespaces'): if b'bookmarks' not in other.listkeys(b'namespaces'):
ui.warn(_(b"remote doesn't support bookmarks\n")) ui.warn(_(b"remote doesn't support bookmarks\n"))
return 0 return 0
ui.pager(b'incoming') ui.pager(b'incoming')
ui.status( ui.status(
_(b'comparing with %s\n') % urlutil.hidepassword(source) _(b'comparing with %s\n') % urlutil.hidepassword(source)
) )
return bookmarks.incoming(ui, repo, other) return bookmarks.incoming(
ui, repo, other, mode=path.bookmarks_mode
)
finally: finally:
other.close() other.close()
return hg.incoming(ui, repo, source, opts) return hg.incoming(ui, repo, source, opts)
@command( @command(
b'init', b'init',

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 incoming --bookmark -v ../a
comparing with ../a
searching for changed bookmarks
@ 0d2164f0ce0d diverged
X 0d2164f0ce0d added
$ hg incoming --bookmark -v ../a --config 'paths.*:bookmarks.mode=babar'
(paths.*:bookmarks.mode has unknown value: "babar")
comparing with ../a
searching for changed bookmarks
@ 0d2164f0ce0d diverged
X 0d2164f0ce0d added
$ hg incoming --bookmark -v ../a --config 'paths.*:bookmarks.mode=mirror'
comparing with ../a
searching for changed bookmarks
@ 0d2164f0ce0d changed
@foo 000000000000 removed
X 0d2164f0ce0d added
X@foo 000000000000 removed
foo 000000000000 removed
foobar 000000000000 removed
$ hg pull ../a --config 'paths.*:bookmarks.mode=mirror' $ 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