This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8117

bookmarks: prevent pushes of divergent bookmarks (foo@remote)
ClosedPublic

Authored by valentin.gatienbaron on Feb 13 2020, 11:32 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG8407031f195f: bookmarks: prevent pushes of divergent bookmarks (foo@remote)
Summary

Before this change, such bookmarks are write-only: a client can push
them but not pull/read them. And because these bookmark can't be read,
even pushes are limited (for instance trying to delete such a bookmark
fails with a vanilla client because the client thinks the bookmark is
neither on the local nor the remote).

This change makes the server refuses such bookmarks, and for earlier
errors, makes the client refuse to send them.

I think the change of behavior is acceptable because I think this is a
bug in push/pull, and I don't think we change the behavior of `hg
unbundle`, because it doesn't seem that hg bundle ever store
bookmarks (and even if it did, it would seem weird anyway to try to
send divergent bookmarks).

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit accepted this revision.Feb 21 2020, 4:14 AM
This revision is now accepted and ready to land.Feb 21 2020, 4:14 AM
durin42 added a subscriber: durin42.Feb 24 2020, 11:50 AM

I'm enthusiastic about this change, and in general a fan of declaring misfeatures in bookmarks exchange bugs. I think it's pretty clear the existing bookmarks exchange behavior isn't right to the point of being buggy.

marmoute added a subscriber: marmoute.Feb 24 2020, 12:06 PM

I am also +1 for this change. divergence bookmark are close to "remote" bookmark and should not be exchanged. Especially because it open the way to divergent-divergent-bookmark and other hells.

Revision Contents
Changeset List

Diff 20211

mercurial/bookmarks.py

def listbookmarks(repo): def listbookmarks(repo):
d = {} d = {}
for book, node in listbinbookmarks(repo): for book, node in listbinbookmarks(repo):
d[book] = hex(node) d[book] = hex(node)
return d return d
def pushbookmark(repo, key, old, new): def pushbookmark(repo, key, old, new):
if isdivergent(key):
return False
if bookmarksinstore(repo): if bookmarksinstore(repo):
wlock = util.nullcontextmanager() wlock = util.nullcontextmanager()
else: else:
wlock = repo.wlock() wlock = repo.wlock()
with wlock, repo.lock(), repo.transaction(b'bookmarks') as tr: with wlock, repo.lock(), repo.transaction(b'bookmarks') as tr:
marks = repo._bookmarks marks = repo._bookmarks
existing = hex(marks.get(key, b'')) existing = hex(marks.get(key, b''))
if existing != old and existing != new: if existing != old and existing != new:

mercurial/bundle2.py

) )
allhooks.append(hookargs) allhooks.append(hookargs)
for hookargs in allhooks: for hookargs in allhooks:
op.repo.hook( op.repo.hook(
b'prepushkey', throw=True, **pycompat.strkwargs(hookargs) b'prepushkey', throw=True, **pycompat.strkwargs(hookargs)
) )
for book, node in changes:
if bookmarks.isdivergent(book):
msg = _(b'cannot accept divergent bookmark %s!') % book
raise error.Abort(msg)
bookstore.applychanges(op.repo, op.gettransaction(), changes) bookstore.applychanges(op.repo, op.gettransaction(), changes)
if pushkeycompat: if pushkeycompat:
def runhook(unused_success): def runhook(unused_success):
for hookargs in allhooks: for hookargs in allhooks:
op.repo.hook(b'pushkey', **pycompat.strkwargs(hookargs)) op.repo.hook(b'pushkey', **pycompat.strkwargs(hookargs))

mercurial/exchange.py

if b in explicit: if b in explicit:
explicit.remove(b) explicit.remove(b)
if not pushed or repo[scid].rev() in pushed: if not pushed or repo[scid].rev() in pushed:
pushop.outbookmarks.append((b, dcid, scid)) pushop.outbookmarks.append((b, dcid, scid))
# search added bookmark # search added bookmark
for b, scid, dcid in addsrc: for b, scid, dcid in addsrc:
if b in explicit: if b in explicit:
explicit.remove(b) explicit.remove(b)
if bookmod.isdivergent(b):
pushop.ui.warn(_(b'cannot push divergent bookmark %s!\n') % b)
pushop.bkresult = 2
else:
pushop.outbookmarks.append((b, b'', scid)) pushop.outbookmarks.append((b, b'', scid))
# search for overwritten bookmark # search for overwritten bookmark
for b, scid, dcid in list(advdst) + list(diverge) + list(differ): for b, scid, dcid in list(advdst) + list(diverge) + list(differ):
if b in explicit: if b in explicit:
explicit.remove(b) explicit.remove(b)
pushop.outbookmarks.append((b, dcid, scid)) pushop.outbookmarks.append((b, dcid, scid))
# search for bookmark to delete # search for bookmark to delete
for b, scid, dcid in adddst: for b, scid, dcid in adddst:
if b in explicit: if b in explicit:

tests/test-bookmarks-pushpull.t

bundle2-input: end of bundle2 stream bundle2-input: end of bundle2 stream
bundle2-input-bundle: 0 parts total bundle2-input-bundle: 0 parts total
deleting remote bookmark W deleting remote bookmark W
listing keys for "phases" listing keys for "phases"
[1] [1]
#endif #endif
Divergent bookmark cannot be exported
$ hg book W@default
$ hg push -B W@default ../a
pushing to ../a
searching for changes
cannot push divergent bookmark W@default!
no changes found
[2]
$ hg book -d W@default
export the active bookmark export the active bookmark
$ hg bookmark V $ hg bookmark V
$ hg push -B . ../a $ hg push -B . ../a
pushing to ../a pushing to ../a
searching for changes searching for changes
no changes found no changes found
exporting bookmark V exporting bookmark V