Patchwork D2880: bundle: add the possibility to bundle bookmarks (issue5792)

login
register
mail settings
Submitter phabricator
Date March 16, 2018, 1:22 p.m.
Message ID <differential-rev-PHID-DREV-onowg4yymodqgzyrsuqd-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29562/
State New
Headers show

Comments

phabricator - March 16, 2018, 1:22 p.m.
lothiraldan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Also take the wlock when unbundling. It shouldn't have a big impact on
  performance.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundle2.py
  mercurial/commands.py
  mercurial/configitems.py
  mercurial/debugcommands.py
  tests/test-bundle-bookmarks.t

CHANGE DETAILS




To: lothiraldan, #hg-reviewers
Cc: mercurial-devel
phabricator - March 16, 2018, 1:27 p.m.
pulkit added inline comments.

INLINE COMMENTS

> configitems.py:422
>  )
> +coreconfigitem('experimental', 'bundle-bookmarks',
> +    default=False,

Any reason why we have all these config option as experimental? We talked at sprint about moving things out of experimental and we agreed on `experimental.bundle-phases`. This one sounds similar.

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 16, 2018, 1:49 p.m.
lothiraldan added a subscriber: indygreg.
lothiraldan added inline comments.

INLINE COMMENTS

> pulkit wrote in configitems.py:422
> Any reason why we have all these config option as experimental? We talked at sprint about moving things out of experimental and we agreed on `experimental.bundle-phases`. This one sounds similar.

I was afraid we might have to delete this option once we have the discussion about bundle spec format with @indygreg. If that would be the case, I would feel more comfortable deleting an experimental option.

If we agreed on moving bundle-phases out of experimental, I guess we can do the same for bundle-bookmarks.

@indygreg Do you think we would still have this option around once we have new-style bundle spec?

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: indygreg, pulkit, mercurial-devel
phabricator - March 16, 2018, 4:45 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> test-bundle-bookmarks.t:37-63
> +Bookmarks are restored when unbundling
> +  $ hg bundle --all bundle
> +  5 changesets found
> +  $ hg debugbundle bundle
> +  Stream params: {Compression: BZ}
> +  changegroup -- {nbchanges: 5, version: 02}
> +      426bada5c67598ca65036d57d9e4b64b0c1ce7a0

Can we have a similar test case where we create divergence? Create a fork in the graph in the debugdrawdag call above. Let's say you have commit F that branches off of B, then do something like this:

  $ hg bundle --all bundle
  $ hg strip --no-backup C
  $ hg bookmarks -f -r F D1
  $ hg unbundle -q bundle
  $ hg log -G -T '{desc} {bookmarks}\n'
    o  E
    |
    o  D D1 D2
    |
    o  C
    |
    |  o F D1@1
    |/
    o  B
    |
    o  A A1

I don't know if "@1" is the appropriate divergence marker, but I can't think of a better one. I haven't even looked at your code to try to guess what it would be in practice (if it would work at all).

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: martinvonz, indygreg, pulkit, mercurial-devel
phabricator - April 12, 2018, 4:16 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  What's the status of this patch? Is it still reviewable? If so, let's get a rebased version submitted, just in case things have changed.

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, pulkit, mercurial-devel
phabricator - April 12, 2018, 5:47 p.m.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D2880#52559, @indygreg wrote:
  
  > What's the status of this patch? Is it still reviewable? If so, let's get a rebased version submitted, just in case things have changed.
  
  
  I need to update the patch according to @martinvoz comment.
  
  I was also waiting on your comment on https://phab.mercurial-scm.org/D2880#46263, do you think we will keep the option once the bundlespec system is revamped? If yes, we can make the config option a non-experimental one. But if it will be dropped soon, I think keeping it experimental until it's dropped is preferable. What do you think?

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, pulkit, mercurial-devel
phabricator - April 14, 2018, 9:40 a.m.
lothiraldan added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-bundle-bookmarks.t:37-63
> Can we have a similar test case where we create divergence? Create a fork in the graph in the debugdrawdag call above. Let's say you have commit F that branches off of B, then do something like this:
> 
>   $ hg bundle --all bundle
>   $ hg strip --no-backup C
>   $ hg bookmarks -f -r F D1
>   $ hg unbundle -q bundle
>   $ hg log -G -T '{desc} {bookmarks}\n'
>     o  E
>     |
>     o  D D1 D2
>     |
>     o  C
>     |
>     |  o F D1@1
>     |/
>     o  B
>     |
>     o  A A1
> 
> I don't know if "@1" is the appropriate divergence marker, but I can't think of a better one. I haven't even looked at your code to try to guess what it would be in practice (if it would work at all).

I have added such test and indeed this case is not handled right now. I'm not aware of how Mercurial handle this case when exchanging bookmarks, I will need to take a look at the code to find out.

What is the correct behavior here? Change the existing bookmark or the bookmark from the bundle?

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, pulkit, mercurial-devel
phabricator - April 14, 2018, 4:41 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> lothiraldan wrote in test-bundle-bookmarks.t:37-63
> I have added such test and indeed this case is not handled right now. I'm not aware of how Mercurial handle this case when exchanging bookmarks, I will need to take a look at the code to find out.
> 
> What is the correct behavior here? Change the existing bookmark or the bookmark from the bundle?

I think it should be the same thing as pulling from another repo. Try putting the same changes in another repo and pulling from it without giving the path a name (nothing in [paths] config). I haven't checked what suffix will be used on the divergent bookmark, but it probably makes sense to use the same here (perhaps it's just going to be "https://phab.mercurial-scm.org/D1@1" as I guessed before).

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, pulkit, mercurial-devel
phabricator - April 17, 2018, 7:41 a.m.
lothiraldan added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-bundle-bookmarks.t:37-63
> I think it should be the same thing as pulling from another repo. Try putting the same changes in another repo and pulling from it without giving the path a name (nothing in [paths] config). I haven't checked what suffix will be used on the divergent bookmark, but it probably makes sense to use the same here (perhaps it's just going to be "https://phab.mercurial-scm.org/D1@1" as I guessed before).

I tried adding such scenarios but failed to produce a divergence scenario, no idea why.

I don't think I will have time to debug the issue before the freeze, so let's skip this changeset for 4.6

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, pulkit, mercurial-devel
phabricator - Nov. 29, 2018, 6:19 p.m.
martinvonz added a comment.


  Just a reminder that this has not been queued yet.

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, pulkit, mercurial-devel

Patch

diff --git a/tests/test-bundle-bookmarks.t b/tests/test-bundle-bookmarks.t
new file mode 100644
--- /dev/null
+++ b/tests/test-bundle-bookmarks.t
@@ -0,0 +1,80 @@ 
+  $ cat >> $HGRCPATH <<EOF
+  > [experimental]
+  > bundle-bookmarks=yes
+  > [extensions]
+  > strip=
+  > drawdag=$TESTDIR/drawdag.py
+  > EOF
+
+Set up repo with linear history
+  $ hg init linear
+  $ cd linear
+  $ hg debugdrawdag <<'EOF'
+  > E
+  > |
+  > D
+  > |
+  > C
+  > |
+  > B
+  > |
+  > A
+  > EOF
+  $ hg bookmarks -r A "A1"
+  $ hg bookmarks -r D "D1"
+  $ hg bookmarks -r D "D2"
+  $ hg log -G -T '{desc} {bookmarks}\n'
+  o  E
+  |
+  o  D D1 D2
+  |
+  o  C
+  |
+  o  B
+  |
+  o  A A1
+  
+Bookmarks are restored when unbundling
+  $ hg bundle --all bundle
+  5 changesets found
+  $ hg debugbundle bundle
+  Stream params: {Compression: BZ}
+  changegroup -- {nbchanges: 5, version: 02}
+      426bada5c67598ca65036d57d9e4b64b0c1ce7a0
+      112478962961147124edd43549aedd1a335e44bf
+      26805aba1e600a82e93661149f2313866a221a7b
+      f585351a92f85104bff7c284233c338b10eb1df7
+      9bc730a19041f9ec7cb33c626e811aa233efb18c
+  bookmarks -- {}
+      A1: Bk\xad\xa5\xc6u\x98\xcae\x03mW\xd9\xe4\xb6K\x0c\x1c\xe7\xa0 (esc)
+      D1: \xf5\x855\x1a\x92\xf8Q\x04\xbf\xf7\xc2\x84#<3\x8b\x10\xeb\x1d\xf7 (esc)
+      D2: \xf5\x855\x1a\x92\xf8Q\x04\xbf\xf7\xc2\x84#<3\x8b\x10\xeb\x1d\xf7 (esc)
+  $ hg strip --no-backup C
+  $ hg unbundle -q bundle
+  $ hg log -G -T '{desc} {bookmarks}\n'
+  o  E
+  |
+  o  D D1 D2
+  |
+  o  C
+  |
+  o  B
+  |
+  o  A A1
+  
+Bookmarks doesn't conflict with local bookmarks
+
+  $ hg bookmarks -d A1
+  $ hg bookmarks -r A "A2"
+  $ hg unbundle -q bundle
+  $ hg log -G -T '{desc} {bookmarks}\n'
+  o  E
+  |
+  o  D D1 D2
+  |
+  o  C
+  |
+  o  B
+  |
+  o  A A1 A2
+  
diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -33,6 +33,7 @@ 
     short,
 )
 from . import (
+    bookmarks,
     bundle2,
     changegroup,
     cmdutil,
@@ -326,6 +327,14 @@ 
             ui.write(indent_string)
             ui.write('%s %s\n' % (hex(head), phases.phasenames[phase]))
 
+def _debugbookmarks(ui, data, indent=0):
+    """display version and markers contained in 'data'"""
+    indent_string = ' ' * indent
+    bm = bookmarks.binarydecode(data)
+    for bookmark in sorted(bm):
+        ui.write(indent_string)
+        ui.write('%s: %s\n' % (bookmark[0], bookmark[1]))
+
 def _quasirepr(thing):
     if isinstance(thing, (dict, util.sortdict, collections.OrderedDict)):
         return '{%s}' % (
@@ -353,6 +362,9 @@ 
         if part.type == 'phase-heads':
             if not ui.quiet:
                 _debugphaseheads(ui, part, indent=4)
+        if part.type == 'bookmarks':
+            if not ui.quiet:
+                _debugbookmarks(ui, part, indent=4)
 
 @command('debugbundle',
         [('a', 'all', None, _('show all details')),
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -419,6 +419,9 @@ 
 coreconfigitem('experimental', 'archivemetatemplate',
     default=dynamicdefault,
 )
+coreconfigitem('experimental', 'bundle-bookmarks',
+    default=False,
+)
 coreconfigitem('experimental', 'bundle-phases',
     default=False,
 )
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1267,6 +1267,8 @@ 
         contentopts['obsolescence'] = True
     if repo.ui.configbool('experimental', 'bundle-phases'):
         contentopts['phases'] = True
+    if repo.ui.configbool('experimental', 'bundle-bookmarks'):
+        contentopts['bookmarks'] = True
     bundle2.writenewbundle(ui, repo, 'bundle', fname, bversion, outgoing,
                            contentopts, compression=bcompression,
                            compopts=compopts)
@@ -5406,7 +5408,7 @@ 
     """
     fnames = (fname1,) + fnames
 
-    with repo.lock():
+    with repo.wlock(), repo.lock():
         for fname in fnames:
             f = hg.openpath(ui, fname)
             gen = exchange.readbundle(ui, f, fname)
diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1599,6 +1599,14 @@ 
         phasedata = phases.binaryencode(headsbyphase)
         bundler.newpart('phase-heads', data=phasedata)
 
+    if opts.get('bookmarks', False):
+        # From exchange._pushb2bookmarkspart
+        data = []
+        for book, new in repo._bookmarks.items():
+            data.append((book, new))
+        checkdata = bookmarks.binaryencode(data)
+        bundler.newpart('bookmarks', data=checkdata)
+
 def addparttagsfnodescache(repo, bundler, outgoing):
     # we include the tags fnode cache for the bundle changeset
     # (as an optional parts)