This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6731

exchange: abort on pushing bookmarks pointing to secret changesets (issue6159)
ClosedPublic

Authored by navaneeth.suresh on Aug 16 2019, 4:26 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG3332bde53714: exchange: abort on pushing bookmarks pointing to secret changesets (issue6159)
Summary

Until now, if there is a bookmark points to a changeset which is in secret
phase, hg will push the bookmark, but not the changeset referenced by that
bookmark. This leaves the server bookmarks in a bad state, because that
bookmark now points to a revision that does not exist on the server. This
patch makes hg to abort on such cases.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit added a subscriber: pulkit.Aug 17 2019, 3:26 PM

In such cases, I like the idea of having fix as two patches, first which demonstrates the bug and the second which fixes the bug. What do you think?

navaneeth.suresh added a parent revision: D6740: tests: add test to demonstrate issue6159.Aug 17 2019, 5:29 PM
In D6731#98871, @pulkit wrote:

In such cases, I like the idea of having fix as two patches, first which demonstrates the bug and the second which fixes the bug. What do you think?

Yeah, sounds good to me. The stack has been updated.

pulkit added a comment.Aug 19 2019, 11:23 AM

Can you also add a test where we are pushing multiple heads/bookmarks and one of the bookmark is problematic?

mercurial/exchange.py
1043

it will be nice to mention that the failure is about pushing that specific bookmark. something like: cannot push bookmark %s as it points ...

1044

What does the return value mean here? I think we can get rid of this.

Something like this:

if node and pushop.repo[node].phase() == phases.secret:
       raise ....
navaneeth.suresh marked 2 inline comments as done.Aug 20 2019, 8:27 AM
navaneeth.suresh added inline comments.
mercurial/exchange.py
1044

updated to get rid of one return. if node is None, then hg will throw an error there.

pulkit added inline comments.Aug 20 2019, 8:45 AM
mercurial/exchange.py
1044

If the node is None, then if node and .... will be false and the second condition won't be executed.

navaneeth.suresh marked an inline comment as done.Aug 20 2019, 9:30 AM
navaneeth.suresh added inline comments.
mercurial/exchange.py
1044

I'm getting the following error with the code snippet that you've suggested:

+  Traceback (most recent call last):
+    File "/tmp/hgtests.nS3TJv/install/bin/hg", line 43, in <module>
+      dispatch.run()
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 99, in run
+      status = dispatch(req)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 225, in dispatch
+      ret = _runcatch(req) or 0
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 376, in _runcatch
+      return _callcatch(ui, _runcatchfunc)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 384, in _callcatch
+      return scmutil.callcatch(ui, func)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/scmutil.py", line 167, in callcatch
+      return func()
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 367, in _runcatchfunc
+      return _dispatch(req)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 1021, in _dispatch
+      cmdpats, cmdoptions)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 756, in runcommand
+      ret = _runcommand(ui, options, cmd, d)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 1030, in _runcommand
+      return cmdfunc()
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 1018, in <lambda>
+      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/util.py", line 1682, in check
+      return func(*args, **kwargs)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/commands.py", line 4666, in push
+      opargs=opargs)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 568, in push
+      _pushbundle2(pushop)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1149, in _pushbundle2
+      ret = partgen(pushop, bundler)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1025, in _pushb2bookmarks
+      return _pushb2bookmarkspart(pushop, bundler)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1052, in _pushb2bookmarkspart
+      _abortonsecretctx(pushop, new, book)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1039, in _abortonsecretctx
+      ctx = pushop.repo[node]
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/localrepo.py", line 1430, in __getitem__
+      (changeid, type(changeid)))
+  mercurial.error.ProgrammingError: unsupported changeid '' of type <type 'str'>
   [1]
pulkit added inline comments.Aug 20 2019, 9:42 AM
mercurial/exchange.py
1044

You won't need ctx = pushop.repo[node] anymore then.

In general, I suggest understanding the error as why are they are happening and you will find a fix.

navaneeth.suresh added inline comments.Aug 20 2019, 9:57 AM
mercurial/exchange.py
1044

ctx = pushop.repo[node] is needed to check the phase of the ctx. also, iiuc, if i write if a and b: pass in python, both a and b are evaluated before performing the comparison. iiuc, when ctx is None, ctx.phase() won't exist as ctx = repo[None] is not actually a changeset but, the uncommitted changes in the working directory instead. if we need to skip the return, we need to have the ctx = pushop.repo[node] inside the if statement. but, for me, skipping the further evaluation earlier using a return without the nested if with more alignment sounds good.

navaneeth.suresh marked 2 inline comments as done.Aug 22 2019, 8:26 AM
navaneeth.suresh added inline comments.
mercurial/exchange.py
1044

@pulkit i have updated as you suggested to get rid of all returns. sorry for being vague, thanks for the suggestion.

pulkit added a comment.Aug 23 2019, 7:09 PM

Looks like you forgot to update tests.

navaneeth.suresh added a comment.Aug 23 2019, 10:23 PM
In D6731#99192, @pulkit wrote:

Looks like you forgot to update tests.

My bad! Sorry about that. Updated it now. Kindly review.

pulkit accepted this revision.Aug 25 2019, 10:56 AM
pulkit added inline comments.
tests/test-bookmarks-pushpull.t
1347

removing the casing in flight as it's no longer required.

This revision is now accepted and ready to land.Aug 25 2019, 10:56 AM
pulkit added a comment.Aug 25 2019, 11:02 AM

Queued the patches for stable, many thanks!

Revision Contents
Changeset List

PathPackages
Mmercurial/exchange.py (8 lines)
Mtests/test-bookmarks-pushpull.t (23 lines)

Diff 16305

mercurial/exchange.py

def _bmaction(old, new): def _bmaction(old, new):
"""small utility for bookmark pushing""" """small utility for bookmark pushing"""
if not old: if not old:
return 'export' return 'export'
elif not new: elif not new:
return 'delete' return 'delete'
return 'update' return 'update'
def _abortonsecretctx(pushop, node, b):
"""abort if a given bookmark points to a secret changeset"""
if node and pushop.repo[node].phase() == phases.secret:
raise error.Abort(_('cannot push bookmark %s as it points to a secret'
' changeset') % b)
def _pushb2bookmarkspart(pushop, bundler): def _pushb2bookmarkspart(pushop, bundler):
pulkitUnsubmitted
Done

it will be nice to mention that the failure is about pushing that specific bookmark. something like: cannot push bookmark %s as it points ...

pulkit: it will be nice to mention that the failure is about pushing that specific bookmark. something…
pushop.stepsdone.add('bookmarks') pushop.stepsdone.add('bookmarks')
pulkitUnsubmitted
Done

What does the return value mean here? I think we can get rid of this.

Something like this:

if node and pushop.repo[node].phase() == phases.secret:
       raise ....
pulkit: What does the return value mean here? I think we can get rid of this. Something like this…
navaneeth.sureshAuthorUnsubmitted
Done

updated to get rid of one return. if node is None, then hg will throw an error there.

navaneeth.suresh: updated to get rid of one return. if node is `None`, then hg will throw an error there.
pulkitUnsubmitted
Done

If the node is None, then if node and .... will be false and the second condition won't be executed.

pulkit: If the node is None, then `if node and ....` will be false and the second condition won't be…
navaneeth.sureshAuthorUnsubmitted
Done

I'm getting the following error with the code snippet that you've suggested:

+  Traceback (most recent call last):
+    File "/tmp/hgtests.nS3TJv/install/bin/hg", line 43, in <module>
+      dispatch.run()
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 99, in run
+      status = dispatch(req)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 225, in dispatch
+      ret = _runcatch(req) or 0
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 376, in _runcatch
+      return _callcatch(ui, _runcatchfunc)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 384, in _callcatch
+      return scmutil.callcatch(ui, func)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/scmutil.py", line 167, in callcatch
+      return func()
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 367, in _runcatchfunc
+      return _dispatch(req)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 1021, in _dispatch
+      cmdpats, cmdoptions)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 756, in runcommand
+      ret = _runcommand(ui, options, cmd, d)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 1030, in _runcommand
+      return cmdfunc()
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/dispatch.py", line 1018, in <lambda>
+      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/util.py", line 1682, in check
+      return func(*args, **kwargs)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/commands.py", line 4666, in push
+      opargs=opargs)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 568, in push
+      _pushbundle2(pushop)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1149, in _pushbundle2
+      ret = partgen(pushop, bundler)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1025, in _pushb2bookmarks
+      return _pushb2bookmarkspart(pushop, bundler)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1052, in _pushb2bookmarkspart
+      _abortonsecretctx(pushop, new, book)
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/exchange.py", line 1039, in _abortonsecretctx
+      ctx = pushop.repo[node]
+    File "/tmp/hgtests.nS3TJv/install/lib/python/mercurial/localrepo.py", line 1430, in __getitem__
+      (changeid, type(changeid)))
+  mercurial.error.ProgrammingError: unsupported changeid '' of type <type 'str'>
   [1]
navaneeth.suresh: I'm getting the following error with the code snippet that you've suggested: ``` + Traceback…
pulkitUnsubmitted
Done

You won't need ctx = pushop.repo[node] anymore then.

In general, I suggest understanding the error as why are they are happening and you will find a fix.

pulkit: You won't need `ctx = pushop.repo[node]` anymore then. In general, I suggest understanding the…
navaneeth.sureshAuthorUnsubmitted
Done

ctx = pushop.repo[node] is needed to check the phase of the ctx. also, iiuc, if i write if a and b: pass in python, both a and b are evaluated before performing the comparison. iiuc, when ctx is None, ctx.phase() won't exist as ctx = repo[None] is not actually a changeset but, the uncommitted changes in the working directory instead. if we need to skip the return, we need to have the ctx = pushop.repo[node] inside the if statement. but, for me, skipping the further evaluation earlier using a return without the nested if with more alignment sounds good.

navaneeth.suresh: `ctx = pushop.repo[node]` is needed to check the phase of the ctx. also, iiuc, if i write `if a…
navaneeth.sureshAuthorUnsubmitted
Done

@pulkit i have updated as you suggested to get rid of all returns. sorry for being vague, thanks for the suggestion.

navaneeth.suresh: @pulkit i have updated as you suggested to get rid of all returns. sorry for being vague…
if not pushop.outbookmarks: if not pushop.outbookmarks:
return return
allactions = [] allactions = []
data = [] data = []
for book, old, new in pushop.outbookmarks: for book, old, new in pushop.outbookmarks:
_abortonsecretctx(pushop, new, book)
new = bin(new) new = bin(new)
data.append((book, new)) data.append((book, new))
allactions.append((book, _bmaction(old, new))) allactions.append((book, _bmaction(old, new)))
checkdata = bookmod.binaryencode(data) checkdata = bookmod.binaryencode(data)
bundler.newpart('bookmarks', data=checkdata) bundler.newpart('bookmarks', data=checkdata)
def handlereply(op): def handlereply(op):
ui = pushop.ui ui = pushop.ui
targetid = int(exc.partid) targetid = int(exc.partid)
for partid, book, action in part2book: for partid, book, action in part2book:
if partid == targetid: if partid == targetid:
raise error.Abort(bookmsgmap[action][1].rstrip() % book) raise error.Abort(bookmsgmap[action][1].rstrip() % book)
# we should not be called for part we did not generated # we should not be called for part we did not generated
assert False assert False
for book, old, new in pushop.outbookmarks: for book, old, new in pushop.outbookmarks:
_abortonsecretctx(pushop, new, book)
part = bundler.newpart('pushkey') part = bundler.newpart('pushkey')
part.addparam('namespace', enc('bookmarks')) part.addparam('namespace', enc('bookmarks'))
part.addparam('key', enc(book)) part.addparam('key', enc(book))
part.addparam('old', enc(old)) part.addparam('old', enc(old))
part.addparam('new', enc(new)) part.addparam('new', enc(new))
action = 'update' action = 'update'
if not old: if not old:
action = 'export' action = 'export'

tests/test-bookmarks-pushpull.t

$ cd issue6159local $ cd issue6159local
$ hg up -qr foo $ hg up -qr foo
$ echo b > b $ echo b > b
Move the bookmark "foo" to point at a secret changeset Move the bookmark "foo" to point at a secret changeset
$ hg commit -qAm_ --config phases.new-commit=secret $ hg commit -qAm_ --config phases.new-commit=secret
Pushing the bookmark "foo" now fails as it contains a secret changeset Pushing the bookmark "foo" now fails as it contains a secret changeset
#if b2-pushkey #if b2-pushkey
pulkitUnsubmitted
Not Done

removing the casing in flight as it's no longer required.

pulkit: removing the casing in flight as it's no longer required.
$ hg push -r foo $ hg push -r foo
pushing to $TESTTMP/issue6159remote pushing to $TESTTMP/issue6159remote
searching for changes searching for changes
no changes found (ignored 1 secret changesets) no changes found (ignored 1 secret changesets)
abort: updating bookmark foo failed! abort: cannot push bookmark foo as it points to a secret changeset
[255] [255]
#endif #endif
#if b2-binary #if b2-binary
$ hg push -r foo $ hg push -r foo
pushing to $TESTTMP/issue6159remote pushing to $TESTTMP/issue6159remote
searching for changes searching for changes
no changes found (ignored 1 secret changesets) no changes found (ignored 1 secret changesets)
updating bookmark foo abort: cannot push bookmark foo as it points to a secret changeset
[1]
#endif
Now the "remote" repo contains a bookmark pointing to a nonexistent revision
$ cd ../issue6159remote
#if b2-pushkey
$ hg bookmark
* foo 0:1599bc8b897a
$ hg log -r 1599bc8b897a
0:1599bc8b897a _ (no-eol)
#endif
#if b2-binary
$ hg bookmark
no bookmarks set
$ cat .hg/bookmarks
cf489fd8a374cab73c2dc19e899bde6fe3a43f8f foo
$ hg log -r cf489fd8a374
abort: unknown revision 'cf489fd8a374'!
[255] [255]
#endif #endif