Patchwork [1,of,2,V2] share: implement shared bookmark functionality

login
register
mail settings
Submitter Ryan McElroy
Date Dec. 2, 2014, 9:39 p.m.
Message ID <5fc6fc3b954321f2aed0.1417556384@devbig105.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6962/
State Superseded
Commit 141baca1605917f36178679c81a6e70a032de0ff
Delegated to: Ryan McElroy
Headers show

Comments

Ryan McElroy - Dec. 2, 2014, 9:39 p.m.
# HG changeset patch
# User Ryan McElroy <rmcelroy@fb.com>
# Date 1417078134 28800
#      Thu Nov 27 00:48:54 2014 -0800
# Node ID 5fc6fc3b954321f2aed07d7270f560e7c4c867be
# Parent  c4f7d3fbe855041951dfccb40b05031def426a2b
share: implement shared bookmark functionality

The hg.share function now creates a bookmarks.shared file in the destination
repository with contents pointing to the source repository with which the
destination should share bookmarks.

This enables bookmarks to be shared between shared repositories by extending
the bookmarks.bmstore methods that read and write bookmarks to take into account
a bookmarks.shared file that can point to another repository.
Didly - Dec. 2, 2014, 10:24 p.m.
On Tue, Dec 2, 2014 at 10:39 PM, Ryan McElroy <rm@fb.com> wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@fb.com>
> # Date 1417078134 28800
> #      Thu Nov 27 00:48:54 2014 -0800
> # Node ID 5fc6fc3b954321f2aed07d7270f560e7c4c867be
> # Parent  c4f7d3fbe855041951dfccb40b05031def426a2b
> share: implement shared bookmark functionality
>
> The hg.share function now creates a bookmarks.shared file in the destination
> repository with contents pointing to the source repository with which the
> destination should share bookmarks.
>
> This enables bookmarks to be shared between shared repositories by extending
> the bookmarks.bmstore methods that read and write bookmarks to take into account
> a bookmarks.shared file that can point to another repository.

Please see the comments that I just sent to V1 of your patch.

Cheers,

Angel
Pierre-Yves David - Dec. 2, 2014, 11:36 p.m.
On 12/02/2014 02:24 PM, Didly wrote:
> On Tue, Dec 2, 2014 at 10:39 PM, Ryan McElroy <rm@fb.com> wrote:
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy@fb.com>
>> # Date 1417078134 28800
>> #      Thu Nov 27 00:48:54 2014 -0800
>> # Node ID 5fc6fc3b954321f2aed07d7270f560e7c4c867be
>> # Parent  c4f7d3fbe855041951dfccb40b05031def426a2b
>> share: implement shared bookmark functionality
>>
>> The hg.share function now creates a bookmarks.shared file in the destination
>> repository with contents pointing to the source repository with which the
>> destination should share bookmarks.
>>
>> This enables bookmarks to be shared between shared repositories by extending
>> the bookmarks.bmstore methods that read and write bookmarks to take into account
>> a bookmarks.shared file that can point to another repository.
>
> Please see the comments that I just sent to V1 of your patch.

For the record, I do not seems to have received this comment on V1.
Pierre-Yves David - Dec. 2, 2014, 11:48 p.m.
On 12/02/2014 01:39 PM, Ryan McElroy wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@fb.com>
> # Date 1417078134 28800
> #      Thu Nov 27 00:48:54 2014 -0800
> # Node ID 5fc6fc3b954321f2aed07d7270f560e7c4c867be
> # Parent  c4f7d3fbe855041951dfccb40b05031def426a2b
> share: implement shared bookmark functionality
>
> The hg.share function now creates a bookmarks.shared file in the destination
> repository with contents pointing to the source repository with which the
> destination should share bookmarks.
>
> This enables bookmarks to be shared between shared repositories by extending
> the bookmarks.bmstore methods that read and write bookmarks to take into account
> a bookmarks.shared file that can point to another repository.
>
> diff --git a/hgext/share.py b/hgext/share.py
> --- a/hgext/share.py
> +++ b/hgext/share.py
> @@ -6,7 +6,9 @@
>   '''share a common history between several working directories'''
>
>   from mercurial.i18n import _
> -from mercurial import cmdutil, hg, util
> +from mercurial import cmdutil, hg, util, extensions, bookmarks
> +from mercurial.hg import repository, parseurl
> +import errno
>
>   cmdtable = {}
>   command = cmdutil.command(cmdtable)
> @@ -20,7 +22,7 @@ def share(ui, source, dest=None, noupdat
>       """create a new shared repository
>
>       Initialize a new repository and working directory that shares its
> -    history with another repository.
> +    history and bookmarks with another repository.

I think we can pretend bookmarks are part of history (Matt is going to 
object but they are). We we probably not need to update this.

>       .. note::
>
> @@ -67,3 +69,45 @@ def unshare(ui, repo):
>
>       # update store, spath, sopener and sjoin of repo
>       repo.unfiltered().__init__(repo.baseui, repo.root)
> +
> +def extsetup(ui):
> +    extensions.wrapfunction(bookmarks.bmstore, 'getbkfile', getbkfile)
> +    extensions.wrapfunction(bookmarks.bmstore, 'recordchange', recordchange)
> +    extensions.wrapfunction(bookmarks.bmstore, 'write', write)
> +
> +def getsrcrepo(repo):

This function is doing some smart stuff. It would be nice to document it.

> +    srcrepo = None
> +    try:
> +        source = repo.vfs.read('bookmarks.shared')
> +        srcurl, branches = parseurl(source)
> +        srcrepo = repository(repo.ui, srcurl)
> +    except IOError, inst:
> +        if inst.errno != errno.ENOENT:
> +            raise
> +    return srcrepo
> +
> +def getbkfile(orig, self, repo):
> +    repo = getsrcrepo(repo) or repo

Meh, we tend to avoid the `A or B` python magic because it is confusing 
to new comer.

> +    return orig(self, repo)
> +
> +def recordchange(orig, self, tr):
> +    tr.addfilegenerator('bookmarks', ('bookmarks',), self._write)
> +    orig(self, tr)
> +    _write(self)

So as pointer in a comment on V1 (sent after you sent a V2), your extra 
write is not going to be transactional here. You should hook yourself to 
the transaction finalization.

Also a bit confused about why the first function line is useful. Look 
absolutly redundant with the first one.

> +
> +def write(orig, self):
> +    # First write a bookmarks file in case we ever unshare
> +    orig(self)
> +    _write(self)
> +
> +def _write(self):

You probably what to document what is going on here.

> +    repo = getsrcrepo(self._repo) or self._repo
> +
> +    wlock = repo.wlock()
> +    try:
> +        f = repo.vfs('bookmarks', 'w', atomictemp=True)
> +        self._write(f)
> +        f.close()
> +
> +    finally:
> +        wlock.release()

You should rely on the existing mechanism when not in the shared case. 
Otherwise you'll most probably mess up transaction semantic.

> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -225,6 +225,18 @@ def share(ui, source, dest=None, update=
>                   continue
>           _update(r, uprev)
>
> +    bookpath = srcrepo.wvfs.base
> +    # follow shared repo to source
> +    try:
> +        bookpath = srcrepo.vfs.read('bookmarks.shared')
> +        ui.debug('bookmark path changed to %s' % bookpath)
> +    except IOError, inst:
> +        ui.debug('bookmkar path left as %s' % bookpath)
> +        if inst.errno != errno.ENOENT:
> +            raise
> +
> +    destvfs.write('bookmarks.shared', bookpath)
> +

I'm a bit confused about what is going one here. It seems that we 
already have a sharedpatch file. Why do we needs an extra one for bookmarks?

>   def copystore(ui, srcrepo, destpath):
>       '''copy files from store of srcrepo in destpath
>
> diff --git a/tests/test-share.t b/tests/test-share.t
> --- a/tests/test-share.t
> +++ b/tests/test-share.t
> @@ -128,6 +128,114 @@ check that a change does not propagate
>
>     $ cd ..
>
> +
> +test sharing bookmarks
> +
> +  $ hg share repo1 repo3
> +  updating working directory
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ cd repo1
> +  $ hg bookmark bm1
> +  $ hg bookmarks
> +   * bm1                       2:c2e0ac586386
> +  $ cd ../repo2
> +  $ hg book bm2
> +  $ hg bookmarks
> +     bm1                       2:c2e0ac586386
> +   * bm2                       3:0e6e70d1d5f1
> +  $ cd ../repo3
> +  $ hg bookmarks
> +     bm1                       2:c2e0ac586386
> +  $ hg book bm3
> +  $ hg bookmarks
> +     bm1                       2:c2e0ac586386
> +   * bm3                       2:c2e0ac586386
> +  $ cd ../repo1
> +  $ hg bookmarks
> +   * bm1                       2:c2e0ac586386
> +     bm3                       2:c2e0ac586386
> +
> +test that commits work
> +
> +  $ echo 'shared bookmarks' > a
> +  $ hg commit -m 'testing shared bookmarks'
> +  $ hg bookmarks
> +   * bm1                       3:b87954705719
> +     bm3                       2:c2e0ac586386
> +  $ cd ../repo3
> +  $ hg bookmarks
> +     bm1                       3:b87954705719
> +   * bm3                       2:c2e0ac586386
> +  $ echo 'more shared bookmarks' > a
> +  $ hg commit -m 'testing shared bookmarks'
> +  created new head
> +  $ hg bookmarks
> +     bm1                       3:b87954705719
> +   * bm3                       4:62f4ded848e4
> +  $ cd ../repo1
> +  $ hg bookmarks
> +   * bm1                       3:b87954705719
> +     bm3                       4:62f4ded848e4
> +  $ cd ..
> +
> +test pushing bookmarks works
> +
> +  $ hg clone repo3 repo4
> +  updating to branch default
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ cd repo4
> +  $ hg boo bm4
> +  $ echo foo > b
> +  $ hg commit -m 'foo in b'
> +  $ hg boo
> +     bm1                       3:b87954705719
> +     bm3                       4:62f4ded848e4
> +   * bm4                       5:92793bfc8cad
> +  $ hg push -B bm4
> +  pushing to $TESTTMP/repo3 (glob)
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files
> +  exporting bookmark bm4
> +  $ cd ../repo1
> +  $ hg bookmarks
> +   * bm1                       3:b87954705719
> +     bm3                       4:62f4ded848e4
> +     bm4                       5:92793bfc8cad
> +  $ cd ../repo3
> +  $ hg bookmarks
> +     bm1                       3:b87954705719
> +   * bm3                       4:62f4ded848e4
> +     bm4                       5:92793bfc8cad
> +  $ cd ..
> +
> +test behavior when sharing a shared repo
> +
> +  $ hg share repo3 repo5
> +  updating working directory
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ cd repo5
> +  $ hg book
> +     bm1                       3:b87954705719
> +     bm3                       4:62f4ded848e4
> +     bm4                       5:92793bfc8cad
> +  $ cd ..
> +
> +test what happens when an active bookmark is deleted
> +
> +  $ cd repo1
> +  $ hg boo -d bm3
> +  $ hg boo
> +   * bm1                       3:b87954705719
> +     bm4                       5:92793bfc8cad
> +  $ cd ../repo3
> +  $ hg boo
> +     bm1                       3:b87954705719
> +     bm4                       5:92793bfc8cad
> +
> +
>   Explicitly kill daemons to let the test exit on Windows
>
>     $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS

You probably what to add a test to check the bookmark are not updated if 
the transaction is aborted.
Ryan McElroy - Dec. 3, 2014, 12:46 a.m.
> -----Original Message-----

> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]

> Sent: Tuesday, December 2, 2014 3:48 PM

> To: Ryan McElroy; mercurial-devel@selenic.com

> Subject: Re: [PATCH 1 of 2 V2] share: implement shared bookmark

> functionality

> 

> 

> 

> On 12/02/2014 01:39 PM, Ryan McElroy wrote:

> > # HG changeset patch

> > # User Ryan McElroy <rmcelroy@fb.com>

> > # Date 1417078134 28800

> > #      Thu Nov 27 00:48:54 2014 -0800

> > # Node ID 5fc6fc3b954321f2aed07d7270f560e7c4c867be

> > # Parent  c4f7d3fbe855041951dfccb40b05031def426a2b

> > share: implement shared bookmark functionality

> >

> > The hg.share function now creates a bookmarks.shared file in the

> > destination repository with contents pointing to the source repository

> > with which the destination should share bookmarks.

> >

> > This enables bookmarks to be shared between shared repositories by

> > extending the bookmarks.bmstore methods that read and write

> bookmarks

> > to take into account a bookmarks.shared file that can point to another

> repository.

> >

> > diff --git a/hgext/share.py b/hgext/share.py

> > --- a/hgext/share.py

> > +++ b/hgext/share.py

> > @@ -6,7 +6,9 @@

> >   '''share a common history between several working directories'''

> >

> >   from mercurial.i18n import _

> > -from mercurial import cmdutil, hg, util

> > +from mercurial import cmdutil, hg, util, extensions, bookmarks from

> > +mercurial.hg import repository, parseurl import errno

> >

> >   cmdtable = {}

> >   command = cmdutil.command(cmdtable)

> > @@ -20,7 +22,7 @@ def share(ui, source, dest=None, noupdat

> >       """create a new shared repository

> >

> >       Initialize a new repository and working directory that shares its

> > -    history with another repository.

> > +    history and bookmarks with another repository.

> 

> I think we can pretend bookmarks are part of history (Matt is going to object

> but they are). We we probably not need to update this.


I prefer being explicit and correct in documentation; I'd like to leave this change.

> 

> >       .. note::

> >

> > @@ -67,3 +69,45 @@ def unshare(ui, repo):

> >

> >       # update store, spath, sopener and sjoin of repo

> >       repo.unfiltered().__init__(repo.baseui, repo.root)

> > +

> > +def extsetup(ui):

> > +    extensions.wrapfunction(bookmarks.bmstore, 'getbkfile', getbkfile)

> > +    extensions.wrapfunction(bookmarks.bmstore, 'recordchange',

> recordchange)

> > +    extensions.wrapfunction(bookmarks.bmstore, 'write', write)

> > +

> > +def getsrcrepo(repo):

> 

> This function is doing some smart stuff. It would be nice to document it.


Will do!

> 

> > +    srcrepo = None

> > +    try:

> > +        source = repo.vfs.read('bookmarks.shared')

> > +        srcurl, branches = parseurl(source)

> > +        srcrepo = repository(repo.ui, srcurl)

> > +    except IOError, inst:

> > +        if inst.errno != errno.ENOENT:

> > +            raise

> > +    return srcrepo

> > +

> > +def getbkfile(orig, self, repo):

> > +    repo = getsrcrepo(repo) or repo

> 

> Meh, we tend to avoid the `A or B` python magic because it is confusing to

> new comer.


Yeah, you're right, I'll do better here

> 

> > +    return orig(self, repo)

> > +

> > +def recordchange(orig, self, tr):

> > +    tr.addfilegenerator('bookmarks', ('bookmarks',), self._write)

> > +    orig(self, tr)

> > +    _write(self)

> 

> So as pointer in a comment on V1 (sent after you sent a V2), your extra write

> is not going to be transactional here. You should hook yourself to the

> transaction finalization.

> 

> Also a bit confused about why the first function line is useful. Look absolutly

> redundant with the first one.


Ah, yes, I think that's an artifact that I failed to clean up when I decided to call original. I'll make the suggested changes.

> 

> > +

> > +def write(orig, self):

> > +    # First write a bookmarks file in case we ever unshare

> > +    orig(self)

> > +    _write(self)

> > +

> > +def _write(self):

> 

> You probably what to document what is going on here.


Will do.

> 

> > +    repo = getsrcrepo(self._repo) or self._repo

> > +

> > +    wlock = repo.wlock()

> > +    try:

> > +        f = repo.vfs('bookmarks', 'w', atomictemp=True)

> > +        self._write(f)

> > +        f.close()

> > +

> > +    finally:

> > +        wlock.release()

> 

> You should rely on the existing mechanism when not in the shared case.

> Otherwise you'll most probably mess up transaction semantic.


I believe this is the same as the original code, but I can just call the original and factor out any pieces needed for both.

> 

> > diff --git a/mercurial/hg.py b/mercurial/hg.py

> > --- a/mercurial/hg.py

> > +++ b/mercurial/hg.py

> > @@ -225,6 +225,18 @@ def share(ui, source, dest=None, update=

> >                   continue

> >           _update(r, uprev)

> >

> > +    bookpath = srcrepo.wvfs.base

> > +    # follow shared repo to source

> > +    try:

> > +        bookpath = srcrepo.vfs.read('bookmarks.shared')

> > +        ui.debug('bookmark path changed to %s' % bookpath)

> > +    except IOError, inst:

> > +        ui.debug('bookmkar path left as %s' % bookpath)

> > +        if inst.errno != errno.ENOENT:

> > +            raise

> > +

> > +    destvfs.write('bookmarks.shared', bookpath)

> > +

> 

> I'm a bit confused about what is going one here. It seems that we already

> have a sharedpatch file. Why do we needs an extra one for bookmarks?


Originally it served as both a marker for sharing bookmarks and the path (which was not the same directory as the other path). Angel also pointed this out so I'll change it to reuse the same sharedpath file.

> 

> >   def copystore(ui, srcrepo, destpath):

> >       '''copy files from store of srcrepo in destpath

> >

> > diff --git a/tests/test-share.t b/tests/test-share.t

> > --- a/tests/test-share.t

> > +++ b/tests/test-share.t

> > @@ -128,6 +128,114 @@ check that a change does not propagate

> >

> >     $ cd ..

> >

> > +

> > +test sharing bookmarks

> > +

> > +  $ hg share repo1 repo3

> > +  updating working directory

> > +  2 files updated, 0 files merged, 0 files removed, 0 files

> > + unresolved  $ cd repo1  $ hg bookmark bm1  $ hg bookmarks

> > +   * bm1                       2:c2e0ac586386

> > +  $ cd ../repo2

> > +  $ hg book bm2

> > +  $ hg bookmarks

> > +     bm1                       2:c2e0ac586386

> > +   * bm2                       3:0e6e70d1d5f1

> > +  $ cd ../repo3

> > +  $ hg bookmarks

> > +     bm1                       2:c2e0ac586386

> > +  $ hg book bm3

> > +  $ hg bookmarks

> > +     bm1                       2:c2e0ac586386

> > +   * bm3                       2:c2e0ac586386

> > +  $ cd ../repo1

> > +  $ hg bookmarks

> > +   * bm1                       2:c2e0ac586386

> > +     bm3                       2:c2e0ac586386

> > +

> > +test that commits work

> > +

> > +  $ echo 'shared bookmarks' > a

> > +  $ hg commit -m 'testing shared bookmarks'

> > +  $ hg bookmarks

> > +   * bm1                       3:b87954705719

> > +     bm3                       2:c2e0ac586386

> > +  $ cd ../repo3

> > +  $ hg bookmarks

> > +     bm1                       3:b87954705719

> > +   * bm3                       2:c2e0ac586386

> > +  $ echo 'more shared bookmarks' > a

> > +  $ hg commit -m 'testing shared bookmarks'

> > +  created new head

> > +  $ hg bookmarks

> > +     bm1                       3:b87954705719

> > +   * bm3                       4:62f4ded848e4

> > +  $ cd ../repo1

> > +  $ hg bookmarks

> > +   * bm1                       3:b87954705719

> > +     bm3                       4:62f4ded848e4

> > +  $ cd ..

> > +

> > +test pushing bookmarks works

> > +

> > +  $ hg clone repo3 repo4

> > +  updating to branch default

> > +  2 files updated, 0 files merged, 0 files removed, 0 files

> > + unresolved  $ cd repo4  $ hg boo bm4  $ echo foo > b  $ hg commit -m

> > + 'foo in b'

> > +  $ hg boo

> > +     bm1                       3:b87954705719

> > +     bm3                       4:62f4ded848e4

> > +   * bm4                       5:92793bfc8cad

> > +  $ hg push -B bm4

> > +  pushing to $TESTTMP/repo3 (glob)

> > +  searching for changes

> > +  adding changesets

> > +  adding manifests

> > +  adding file changes

> > +  added 1 changesets with 1 changes to 1 files  exporting bookmark

> > + bm4  $ cd ../repo1  $ hg bookmarks

> > +   * bm1                       3:b87954705719

> > +     bm3                       4:62f4ded848e4

> > +     bm4                       5:92793bfc8cad

> > +  $ cd ../repo3

> > +  $ hg bookmarks

> > +     bm1                       3:b87954705719

> > +   * bm3                       4:62f4ded848e4

> > +     bm4                       5:92793bfc8cad

> > +  $ cd ..

> > +

> > +test behavior when sharing a shared repo

> > +

> > +  $ hg share repo3 repo5

> > +  updating working directory

> > +  2 files updated, 0 files merged, 0 files removed, 0 files

> > + unresolved  $ cd repo5  $ hg book

> > +     bm1                       3:b87954705719

> > +     bm3                       4:62f4ded848e4

> > +     bm4                       5:92793bfc8cad

> > +  $ cd ..

> > +

> > +test what happens when an active bookmark is deleted

> > +

> > +  $ cd repo1

> > +  $ hg boo -d bm3

> > +  $ hg boo

> > +   * bm1                       3:b87954705719

> > +     bm4                       5:92793bfc8cad

> > +  $ cd ../repo3

> > +  $ hg boo

> > +     bm1                       3:b87954705719

> > +     bm4                       5:92793bfc8cad

> > +

> > +

> >   Explicitly kill daemons to let the test exit on Windows

> >

> >     $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS

> 

> You probably what to add a test to check the bookmark are not updated if

> the transaction is aborted.


Sounds good; I'll ask you how to do that when I get to it.

> 

> --

> Pierre-Yves David
Matt Mackall - Dec. 4, 2014, 10:15 p.m.
On Tue, 2014-12-02 at 15:48 -0800, Pierre-Yves David wrote:

> > -    history with another repository.
> > +    history and bookmarks with another repository.
> 
> I think we can pretend bookmarks are part of history (Matt is going to 
> object but they are).

  history
      n 1: the aggregate of past events; "a critical time in the
           school's history"
      2: a record or narrative description of past events; "a history
         of France"; "he gave an inaccurate account of the plot to
         kill the president"; "the story of exposure to lead" [syn:
         {history}, {account}, {chronicle}, {story}]
      3: the discipline that records and interprets past events
         involving human beings; "he teaches Medieval history";
         "history takes the long view"
      4: the continuum of events occurring in succession leading from
         the past to the present and even into the future; "all of
         human history"
      5: all that is remembered of the past as preserved in writing; a
         body of knowledge; "the dawn of recorded history"; "from the
         beginning of history"

Commits are obviously "history". Bookmarks, since they intentionally
leave no record of any sort of when they're created, changed, or
deleted, are just as obviously not.

Patch

diff --git a/hgext/share.py b/hgext/share.py
--- a/hgext/share.py
+++ b/hgext/share.py
@@ -6,7 +6,9 @@ 
 '''share a common history between several working directories'''
 
 from mercurial.i18n import _
-from mercurial import cmdutil, hg, util
+from mercurial import cmdutil, hg, util, extensions, bookmarks
+from mercurial.hg import repository, parseurl
+import errno
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
@@ -20,7 +22,7 @@  def share(ui, source, dest=None, noupdat
     """create a new shared repository
 
     Initialize a new repository and working directory that shares its
-    history with another repository.
+    history and bookmarks with another repository.
 
     .. note::
 
@@ -67,3 +69,45 @@  def unshare(ui, repo):
 
     # update store, spath, sopener and sjoin of repo
     repo.unfiltered().__init__(repo.baseui, repo.root)
+
+def extsetup(ui):
+    extensions.wrapfunction(bookmarks.bmstore, 'getbkfile', getbkfile)
+    extensions.wrapfunction(bookmarks.bmstore, 'recordchange', recordchange)
+    extensions.wrapfunction(bookmarks.bmstore, 'write', write)
+
+def getsrcrepo(repo):
+    srcrepo = None
+    try:
+        source = repo.vfs.read('bookmarks.shared')
+        srcurl, branches = parseurl(source)
+        srcrepo = repository(repo.ui, srcurl)
+    except IOError, inst:
+        if inst.errno != errno.ENOENT:
+            raise
+    return srcrepo
+
+def getbkfile(orig, self, repo):
+    repo = getsrcrepo(repo) or repo
+    return orig(self, repo)
+
+def recordchange(orig, self, tr):
+    tr.addfilegenerator('bookmarks', ('bookmarks',), self._write)
+    orig(self, tr)
+    _write(self)
+
+def write(orig, self):
+    # First write a bookmarks file in case we ever unshare
+    orig(self)
+    _write(self)
+
+def _write(self):
+    repo = getsrcrepo(self._repo) or self._repo
+
+    wlock = repo.wlock()
+    try:
+        f = repo.vfs('bookmarks', 'w', atomictemp=True)
+        self._write(f)
+        f.close()
+
+    finally:
+        wlock.release()
diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -225,6 +225,18 @@  def share(ui, source, dest=None, update=
                 continue
         _update(r, uprev)
 
+    bookpath = srcrepo.wvfs.base
+    # follow shared repo to source
+    try:
+        bookpath = srcrepo.vfs.read('bookmarks.shared')
+        ui.debug('bookmark path changed to %s' % bookpath)
+    except IOError, inst:
+        ui.debug('bookmkar path left as %s' % bookpath)
+        if inst.errno != errno.ENOENT:
+            raise
+
+    destvfs.write('bookmarks.shared', bookpath)
+
 def copystore(ui, srcrepo, destpath):
     '''copy files from store of srcrepo in destpath
 
diff --git a/tests/test-share.t b/tests/test-share.t
--- a/tests/test-share.t
+++ b/tests/test-share.t
@@ -128,6 +128,114 @@  check that a change does not propagate
 
   $ cd ..
 
+
+test sharing bookmarks
+
+  $ hg share repo1 repo3
+  updating working directory
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ cd repo1
+  $ hg bookmark bm1
+  $ hg bookmarks
+   * bm1                       2:c2e0ac586386
+  $ cd ../repo2
+  $ hg book bm2
+  $ hg bookmarks
+     bm1                       2:c2e0ac586386
+   * bm2                       3:0e6e70d1d5f1
+  $ cd ../repo3
+  $ hg bookmarks
+     bm1                       2:c2e0ac586386
+  $ hg book bm3
+  $ hg bookmarks
+     bm1                       2:c2e0ac586386
+   * bm3                       2:c2e0ac586386
+  $ cd ../repo1
+  $ hg bookmarks
+   * bm1                       2:c2e0ac586386
+     bm3                       2:c2e0ac586386
+
+test that commits work
+
+  $ echo 'shared bookmarks' > a
+  $ hg commit -m 'testing shared bookmarks'
+  $ hg bookmarks
+   * bm1                       3:b87954705719
+     bm3                       2:c2e0ac586386
+  $ cd ../repo3
+  $ hg bookmarks
+     bm1                       3:b87954705719
+   * bm3                       2:c2e0ac586386
+  $ echo 'more shared bookmarks' > a
+  $ hg commit -m 'testing shared bookmarks'
+  created new head
+  $ hg bookmarks
+     bm1                       3:b87954705719
+   * bm3                       4:62f4ded848e4
+  $ cd ../repo1
+  $ hg bookmarks
+   * bm1                       3:b87954705719
+     bm3                       4:62f4ded848e4
+  $ cd ..
+
+test pushing bookmarks works
+
+  $ hg clone repo3 repo4
+  updating to branch default
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ cd repo4
+  $ hg boo bm4
+  $ echo foo > b
+  $ hg commit -m 'foo in b'
+  $ hg boo
+     bm1                       3:b87954705719
+     bm3                       4:62f4ded848e4
+   * bm4                       5:92793bfc8cad
+  $ hg push -B bm4
+  pushing to $TESTTMP/repo3 (glob)
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  exporting bookmark bm4
+  $ cd ../repo1
+  $ hg bookmarks
+   * bm1                       3:b87954705719
+     bm3                       4:62f4ded848e4
+     bm4                       5:92793bfc8cad
+  $ cd ../repo3
+  $ hg bookmarks
+     bm1                       3:b87954705719
+   * bm3                       4:62f4ded848e4
+     bm4                       5:92793bfc8cad
+  $ cd ..
+
+test behavior when sharing a shared repo
+
+  $ hg share repo3 repo5
+  updating working directory
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ cd repo5
+  $ hg book
+     bm1                       3:b87954705719
+     bm3                       4:62f4ded848e4
+     bm4                       5:92793bfc8cad
+  $ cd ..
+
+test what happens when an active bookmark is deleted
+
+  $ cd repo1
+  $ hg boo -d bm3
+  $ hg boo
+   * bm1                       3:b87954705719
+     bm4                       5:92793bfc8cad
+  $ cd ../repo3
+  $ hg boo
+     bm1                       3:b87954705719
+     bm4                       5:92793bfc8cad
+
+
 Explicitly kill daemons to let the test exit on Windows
 
   $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS