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.
Details
Details
- Reviewers
pulkit - Group Reviewers
hg-reviewers - Commits
- rHG3332bde53714: exchange: abort on pushing bookmarks pointing to secret changesets (issue6159)
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
Comment Actions
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?
Comment Actions
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 .... | |
| mercurial/exchange.py | ||
|---|---|---|
| 1044 | updated to get rid of one return. if node is None, then hg will throw an error there. | |
| mercurial/exchange.py | ||
|---|---|---|
| 1044 | If the node is None, then if node and .... will be false and the second condition won't be executed. | |
| 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] | |
| 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. | |
| 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. | |
| tests/test-bookmarks-pushpull.t | ||
|---|---|---|
| 1347 ↗ | (On Diff #16305) | removing the casing in flight as it's no longer required. |
it will be nice to mention that the failure is about pushing that specific bookmark. something like: cannot push bookmark %s as it points ...