Bug 5165 - can't push long bookmark names with bundle2
Summary: can't push long bookmark names with bundle2
Status: RESOLVED FIXED
Alias: None
Product: Mercurial
Classification: Unclassified
Component: Mercurial (show other bugs)
Version: 3.7.3
Hardware: PC Mac OS
: urgent bug
Assignee: Bugzilla
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2016-03-30 08:19 UTC by Michael O'Connor
Modified: 2018-03-20 00:00 UTC (History)
5 users (show)

See Also:
Python Version: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael O'Connor 2016-03-30 08:19 UTC
I believe it's the case that there's generally no restriction on the length of bookmark names, however trying to push bookmarks of length >255 results in an exception being raised.

$ export LONG_BOOKMARK=a$(seq -s '' 300)
$ hg init repo1
$ hg clone repo1 repo2
$ cd repo2
$ touch a; hg commit -Am a
adding a
$ hg book ${LONG_BOOKMARK}
$ hg push -B ${LONG_BOOKMARK}
pushing to /Users/michaeloconnor/repo1
searching for changes
** unknown exception encountered, please report by visiting
** https://mercurial-scm.org/wiki/BugTracker
** Python 2.7.10 (default, Oct 23 2015, 19:19:21) [GCC 4.2.1 Compatible Apple LLVM 7.0.0 (clang-700.0.59.5)]
** Mercurial Distributed SCM (version 3.7.3)
** Extensions loaded:
Traceback (most recent call last):
...
  File "/usr/local/Cellar/mercurial/3.7.3/lib/python2.7/site-packages/mercurial/bundle2.py", line 936, in getchunks
    paramsizes = _pack(_makefpartparamsizes(len(parsizes) / 2), *parsizes)

The issue can at least currently be worked around by not using bundle2 to push bookmarks.

$ cat > /tmp/long_bookmarks.py <<EOF
def uisetup(ui):
    import mercurial.exchange as exchange
    exchange.b2partsgenorder.remove('bookmarks')
    del exchange.b2partsgenmapping['bookmarks']
EOF
$ hg --config extensions.long_bookmarks=/tmp/long_bookmarks.py push -B ${LONG_BOOKMARK}
pushing to /Users/michaeloconnor/repo1
searching for changes
adding changesets
adding manifests
adding file changes
added 1 changesets with 1 changes to 1 files
exporting bookmark a1234...
Comment 1 Pierre-Yves David 2016-03-30 15:10 UTC
Oops, I don't think there was a official limitation in pushkey (but some actual limitation of http protocol),

Should we add a sensible size limit to names from now?.

(I'll look at a work around for this this week).
Comment 2 Michael O'Connor 2016-03-30 22:25 UTC
Thanks!  

It would be convenient for us if it were possible to keep the length of bookmarks unrestricted: we have a code review system which puts some structure into bookmark names and this can cause our bookmark names to occasionally become quite long.  For example, if someone is working on a change called A which depends on a change called B which depends on a change called C, our code review system will set things up so this work goes in a bookmark called C/B/A.
Comment 3 Pierre-Yves David 2016-03-31 14:13 UTC
Nothing decided yet, but I think it would make sense to limit name length so it might happen. If it happen can you think of an alternative way to encode this dependency information?
Comment 4 Augie Fackler 2016-04-05 14:12 UTC
I'm not sure it really fits our BC guidelines to try and limit bookmark names now.

Why can't we fix bundle2?
Comment 5 Bugzilla 2016-04-16 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 6 Bugzilla 2016-04-27 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 7 Bugzilla 2016-05-08 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 8 Pierre-Yves David 2016-05-11 08:42 UTC
Bookmarks name already have a length limit because of pushkey implementation, they are just higher. Michael O'Connor workflow was going to hit it one way of another anyway. I would advise for encoding this information another way.

I think it make sense to enforce clear and sensible limit on our naming scheme, we already restricted some after the fact for special characted in bookmarks and branch I think enforcing a 255 bytes limit make sense here.
Comment 9 Augie Fackler 2016-05-11 08:47 UTC
Over ssh I'm pretty confident bookmarks have no length limits. Over http, it's certainly in the multiple kilobytes range. I think trying to retroactively declare bookmarks over 255 bytes in length a bug is a clear violation of our BC policy.
Comment 10 Bugzilla 2016-05-22 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 11 Bugzilla 2016-06-02 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 12 Bugzilla 2016-06-13 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 13 Bugzilla 2016-06-24 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 14 Bugzilla 2016-07-04 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 15 Bugzilla 2016-07-15 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 16 Bugzilla 2016-07-25 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 17 Bugzilla 2016-08-04 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 18 Bugzilla 2016-08-14 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 19 Bugzilla 2016-08-24 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 20 Bugzilla 2017-01-26 00:00 UTC
Bug marked urgent for 155 days, bumping
Comment 21 Bugzilla 2017-02-06 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 22 Bugzilla 2017-02-17 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 23 Bugzilla 2017-02-28 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 24 Bugzilla 2017-03-10 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 25 Bugzilla 2017-03-21 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 26 Bugzilla 2017-04-01 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 27 Bugzilla 2017-04-11 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 28 Bugzilla 2017-04-21 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 29 Bugzilla 2017-05-01 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 30 Bugzilla 2017-05-12 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 31 Bugzilla 2017-05-23 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 32 Bugzilla 2017-06-02 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 33 Bugzilla 2017-06-12 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 34 Bugzilla 2017-06-23 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 35 Bugzilla 2017-07-04 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 36 Bugzilla 2017-07-15 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 37 Bugzilla 2017-07-26 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 38 Bugzilla 2017-08-06 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 39 Bugzilla 2017-08-17 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 40 Bugzilla 2017-08-28 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 41 Bugzilla 2017-09-07 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 42 Bugzilla 2017-09-18 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 43 Bugzilla 2017-09-29 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 44 Bugzilla 2017-10-09 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 45 Bugzilla 2017-10-20 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 46 Bugzilla 2017-10-31 00:00 UTC
Bug marked urgent for 11 days, bumping
Comment 47 Bugzilla 2017-11-10 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 48 Bugzilla 2017-11-21 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 49 Bugzilla 2017-12-01 00:00 UTC
Bug marked urgent for 10 days, bumping
Comment 50 Boris Feld 2017-12-06 10:33 UTC
We have a patch ready that move bookmarks to a binary bundle2 part, moving the size limit from 255 to 64K.

@Michael would that be sufficient for your case?
Comment 51 vgatien-baron 2017-12-06 13:10 UTC
Not Michael, but same company. 64k is definitely more than enough. I would expect 64k-long names to break other tools built on top of hg, or run into filesystem limits anyway.
It would be great to see a fix for this land quickly in hg, as downgrading to bundle1 causes pain in the form of not getting the improvements made by bundle2, like better performance and behavior (atomicity, don't push revisions when the bookmarks raced, maybe others).
Comment 52 HG Bot 2017-12-07 19:15 UTC
Fixed by https://mercurial-scm.org/repo/hg/rev/3340d46a5c3f
Boris Feld <boris.feld@octobus.net>
bookmark: add methods to binary encode and decode bookmark values

Coming new bundle2 parts related to bookmark will use a binary encoding. It
encodes a series of '(bookmark, node)' pairs. Bookmark name has a high enough
size limit to not be affected by issue5165. (64K length, we are well covered)

(please test the fix)
Comment 53 HG Bot 2017-12-07 19:15 UTC
Fixed by https://mercurial-scm.org/repo/hg/rev/dbf868623daf
Boris Feld <boris.feld@octobus.net>
bookmark: add a 'check:bookmarks' bundle2 part

This part checks that bookmarks are still at the node they are expected to be.
This allows a pushing client to detect push race where the repository was
updated between the time it discovered the server state and the time it managed
to finish its push.

Such checking already exists when pushing bookmark through pushkey. This new
part can be inserted at the beginning of the bundle, triggering abort earlier.
In addition, we would like to move away from pushey to push bookmark. A step
useful to solve issue5165.

(please test the fix)
Comment 54 HG Bot 2017-12-08 13:50 UTC
Fixed by https://mercurial-scm.org/repo/hg/rev/a1e70c1dbec0
Boris Feld <boris.feld@octobus.net>
bookmark: use the 'bookmarks' bundle2 part to push bookmark update (issue5165)

We use the new binary parts we introduced earlier to exchange bookmark. The
payload is a bit more compact since we use binary and the length of bookmarks
is no longer constrained to 255.

.. fix:: Issue 5165

   Bookmark, whose name is longer than 255, can again be exchanged again
   between 4.4+ client and servers.

(please test the fix)
Comment 55 Bugzilla 2017-12-16 00:00 UTC
Bug was set to TESTING for 7 days, resolving
Comment 56 HG Bot 2018-03-12 18:40 UTC
Fixed by https://mercurial-scm.org/repo/hg/rev/09b58af83d44
Augie Fackler <augie@google.com>
bookmarks: test for exchanging long bookmark names (issue5165)

As far as I can tell a test for a long bookmark name never actually
got added when this was fixed. Let's document that a 300 byte bookmark
is something we're supporting (this patch started out life over a year
ago as a way for me to validate the problem, and I recently found it.)

I think the nonzero exits from the push operations are only because no
new changesets get exchanged. Please correct me if I'm wrong on that. :)

Differential Revision: https://phab.mercurial-scm.org/D2727

(please test the fix)
Comment 57 Bugzilla 2018-03-20 00:00 UTC
Bug was set to TESTING for 7 days, resolving