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

login
register
mail settings
Submitter phabricator
Date Aug. 16, 2019, 8:26 p.m.
Message ID <differential-rev-PHID-DREV-yitt7pwxuaxaf4fsmtot-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41316/
State Superseded
Headers show

Comments

phabricator - Aug. 16, 2019, 8:26 p.m.
navaneeth.suresh created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

AFFECTED FILES
  mercurial/exchange.py
  tests/test-bookmarks-pushpull.t

CHANGE DETAILS




To: navaneeth.suresh, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 17, 2019, 7:26 p.m.
pulkit added a comment.


  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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 17, 2019, 9:30 p.m.
navaneeth.suresh added a comment.


  In D6731#98871 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 19, 2019, 3:23 p.m.
pulkit added a comment.


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

INLINE COMMENTS

> exchange.py:1043
> +    if ctx.phase() == phases.secret:
> +        raise error.Abort(_('bookmark %s points to a secret changeset') % b)
> +    return False

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

> exchange.py:1044
> +        raise error.Abort(_('bookmark %s points to a secret changeset') % b)
> +    return False
> +

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 ....

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 20, 2019, 12:27 p.m.
navaneeth.suresh added inline comments.
navaneeth.suresh marked 2 inline comments as done.

INLINE COMMENTS

> pulkit wrote in exchange.py: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 ....

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 20, 2019, 12:45 p.m.
pulkit added inline comments.

INLINE COMMENTS

> navaneeth.suresh wrote in exchange.py:1044
> updated to get rid of one return. if node is `None`, then hg will throw an error there.

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 20, 2019, 1:30 p.m.
navaneeth.suresh added inline comments.
navaneeth.suresh marked an inline comment as done.

INLINE COMMENTS

> pulkit wrote in exchange.py:1044
> If the node is None, then `if node and ....` will be false and the second condition won't be executed.

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]

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 20, 2019, 1:42 p.m.
pulkit added inline comments.

INLINE COMMENTS

> navaneeth.suresh wrote in 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]

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 20, 2019, 1:57 p.m.
navaneeth.suresh added inline comments.

INLINE COMMENTS

> pulkit wrote in 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.

`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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 22, 2019, 12:26 p.m.
navaneeth.suresh added inline comments.
navaneeth.suresh marked 2 inline comments as done.

INLINE COMMENTS

> navaneeth.suresh wrote in 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.

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 23, 2019, 11:09 p.m.
pulkit added a comment.


  Looks like you forgot to update tests.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 24, 2019, 2:23 a.m.
navaneeth.suresh added a comment.


  In D6731#99192 <https://phab.mercurial-scm.org/D6731#99192>, @pulkit wrote:
  
  > Looks like you forgot to update tests.
  
  My bad! Sorry about that. Updated it now. Kindly review.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 25, 2019, 2:57 p.m.
This revision is now accepted and ready to land.
pulkit added inline comments.
pulkit accepted this revision.

INLINE COMMENTS

> test-bookmarks-pushpull.t:1347
>  Pushing the bookmark "foo" now fails as it contains a secret changeset
>  #if b2-pushkey
>    $ hg push -r foo

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - Aug. 25, 2019, 3:02 p.m.
pulkit added a comment.


  Queued the patches for stable, many thanks!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6731/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6731

To: navaneeth.suresh, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel

Patch

diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -1322,3 +1322,42 @@ 
   abort: push failed on remote
   [255]
 #endif
+
+-- test for stop pushing bookmarks pointing to secret changesets
+
+Set up a "remote" repo
+  $ hg init issue6159remote
+  $ cd issue6159remote
+  $ echo a > a
+  $ hg add a
+  $ hg commit -m_
+  $ hg bookmark foo
+  $ cd ..
+
+Clone a local repo
+  $ hg clone -q issue6159remote issue6159local
+  $ cd issue6159local
+  $ hg up -qr foo
+  $ echo b > b
+
+Move the bookmark "foo" to point at a secret changeset
+  $ hg commit -qAm_
+  $ hg phase -s -f
+
+Pushing the bookmark "foo" now fails as it contains a secret changeset
+#if b2-pushkey
+  $ hg push -r foo
+  pushing to $TESTTMP/issue6159remote
+  searching for changes
+  no changes found (ignored 1 secret changesets)
+  abort: bookmark foo points to a secret changeset
+  [255]
+#endif
+#if b2-binary
+  $ hg push -r foo
+  pushing to $TESTTMP/issue6159remote
+  searching for changes
+  no changes found (ignored 1 secret changesets)
+  abort: bookmark foo points to a secret changeset
+  [255]
+#endif
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1034,6 +1034,15 @@ 
         return 'delete'
     return 'update'
 
+def _abortonsecretctx(pushop, node, b):
+    """abort if a given bookmark points to a secret changeset"""
+    if not node:
+        return False
+    ctx = pushop.repo[node]
+    if ctx.phase() == phases.secret:
+        raise error.Abort(_('bookmark %s points to a secret changeset') % b)
+    return False
+
 def _pushb2bookmarkspart(pushop, bundler):
     pushop.stepsdone.add('bookmarks')
     if not pushop.outbookmarks:
@@ -1042,6 +1051,7 @@ 
     allactions = []
     data = []
     for book, old, new in pushop.outbookmarks:
+        _abortonsecretctx(pushop, new, book)
         new = bin(new)
         data.append((book, new))
         allactions.append((book, _bmaction(old, new)))
@@ -1070,6 +1080,7 @@ 
         assert False
 
     for book, old, new in pushop.outbookmarks:
+        _abortonsecretctx(pushop, new, book)
         part = bundler.newpart('pushkey')
         part.addparam('namespace', enc('bookmarks'))
         part.addparam('key', enc(book))