Patchwork [2,of,3] share: accept optional bookmarks parameter

login
register
mail settings
Submitter Ryan McElroy
Date Dec. 1, 2014, 8:57 p.m.
Message ID <beb7a11c50c34e38adea.1417467443@devbig105.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6935/
State Changes Requested
Headers show

Comments

Ryan McElroy - Dec. 1, 2014, 8:57 p.m.
# HG changeset patch
# User Ryan McElroy <rmcelroy@fb.com>
# Date 1417078134 28800
#      Thu Nov 27 00:48:54 2014 -0800
# Node ID beb7a11c50c34e38adea9d8dd3650d42d16cf0ff
# Parent  c4f7d3fbe855041951dfccb40b05031def426a2b
share: accept optional bookmarks parameter

This modifies the share function in the hg module so it takes a boolean
bookmarks parameter and plumbs this through to the share command added by
the extension. If this parameter is true, the function will create a
bookmarks.shared file in the destination repository with contents pointing to
the source repository with which the destination should share bookmarks.

The actual bookmark sharing functionality will be added in a later patch.
Angel Ezquerra - Dec. 2, 2014, 10:23 p.m.
On Mon, Dec 1, 2014 at 9:57 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 beb7a11c50c34e38adea9d8dd3650d42d16cf0ff
> # Parent  c4f7d3fbe855041951dfccb40b05031def426a2b
> share: accept optional bookmarks parameter
>
> This modifies the share function in the hg module so it takes a boolean
> bookmarks parameter and plumbs this through to the share command added by
> the extension. If this parameter is true, the function will create a
> bookmarks.shared file in the destination repository with contents pointing to
> the source repository with which the destination should share bookmarks.
>
> The actual bookmark sharing functionality will be added in a later patch.

Why do you need to write the source path on the bookmarks.shared file
on the destination repository? Shouldn't it be the same path as the
one stored on the sharedpath file?

> diff --git a/hgext/share.py b/hgext/share.py
> --- a/hgext/share.py
> +++ b/hgext/share.py
> @@ -13,10 +13,12 @@
>  testedwith = 'internal'
>
>  @command('share',
> -    [('U', 'noupdate', None, _('do not create a working copy'))],
> -    _('[-U] SOURCE [DEST]'),
> +    [('U', 'noupdate', None, _('do not create a working copy')),
> +     ('B', 'bookmarks', None, _('share bookmarks with source repository')),
> +    ],
> +    _('[-U] [-B] SOURCE [DEST]'),
>      norepo=True)
> -def share(ui, source, dest=None, noupdate=False):
> +def share(ui, source, dest=None, noupdate=False, bookmarks=False):
>      """create a new shared repository
>
>      Initialize a new repository and working directory that shares its
> @@ -34,7 +36,7 @@
>         the broken clone to reset it to a changeset that still exists.
>      """
>
> -    return hg.share(ui, source, dest, not noupdate)
> +    return hg.share(ui, source, dest, not noupdate, bookmarks)
>
>  @command('unshare', [], '')
>  def unshare(ui, repo):
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -158,7 +158,7 @@
>          return ''
>      return os.path.basename(os.path.normpath(path))
>
> -def share(ui, source, dest=None, update=True):
> +def share(ui, source, dest=None, update=True, bookmarks=False):
>      '''create a shared repository'''
>
>      if not islocal(source):
> @@ -225,6 +225,19 @@
>                  continue
>          _update(r, uprev)
>
> +    if bookmarks:
> +        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

There is a typo on the ui.debug message.

> +        destvfs.write('bookmarks.shared', bookpath)
> +
>  def copystore(ui, srcrepo, destpath):
>      '''copy files from store of srcrepo in destpath


In addition to these comments I'd like to know if you plan to add
additional sharing options (e.g. mq patches). On the last sprint I
told mpm that I would look into doing something similar to this. The
idea is to make it possible to use shared repos to create a sort of
subrepository "cache" (although that is not really the right word).

The idea is to add a new "full share" mode that would create shares
that share everything with their source except the minimum amount of
things that are needed to maintain two separate working directories.
In my implementation these full shares would work by making the
destination repository use a vfs that would access the source
repository in all cases except a few exceptions. This seems quite
different to your approach. I'd like to make sure that what you do
does not clash with what I am trying to do.

I already have most of this implemented, but I have been quite busy
with RL these past few months so I am not ready to share it yet (no
pun intended :-) ). I hope to have some time in the next few weeks
though.

Cheers,

Angel
Ryan McElroy - Dec. 2, 2014, 11:50 p.m.
> -----Original Message-----

> From: ezquerra@gmail.com [mailto:ezquerra@gmail.com] On Behalf Of

> Angel Ezquerra

> Sent: Tuesday, December 2, 2014 2:24 PM

> To: Ryan McElroy

> Cc: mercurial-devel

> Subject: Re: [PATCH 2 of 3] share: accept optional bookmarks parameter

> 

> On Mon, Dec 1, 2014 at 9:57 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 beb7a11c50c34e38adea9d8dd3650d42d16cf0ff

> > # Parent  c4f7d3fbe855041951dfccb40b05031def426a2b

> > share: accept optional bookmarks parameter

> >

> > This modifies the share function in the hg module so it takes a

> > boolean bookmarks parameter and plumbs this through to the share

> > command added by the extension. If this parameter is true, the

> > function will create a bookmarks.shared file in the destination

> > repository with contents pointing to the source repository with which the

> destination should share bookmarks.

> >

> > The actual bookmark sharing functionality will be added in a later patch.

> 

> Why do you need to write the source path on the bookmarks.shared file on

> the destination repository? Shouldn't it be the same path as the one stored

> on the sharedpath file?


In the original version, sharing bookmarks was optional, and the path in the sharedpath file pointed to not-quite-the-right place so I considered it easier to store exactly what I wanted in this file. However, if it's going to be non-optional (looks like it), I agree that it makes sense to just reuse the sharedpath file and do the appropriate manipulations on the path stored there. I'll make that change in V3.

> 

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

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

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

> > @@ -13,10 +13,12 @@

> >  testedwith = 'internal'

> >

> >  @command('share',

> > -    [('U', 'noupdate', None, _('do not create a working copy'))],

> > -    _('[-U] SOURCE [DEST]'),

> > +    [('U', 'noupdate', None, _('do not create a working copy')),

> > +     ('B', 'bookmarks', None, _('share bookmarks with source repository')),

> > +    ],

> > +    _('[-U] [-B] SOURCE [DEST]'),

> >      norepo=True)

> > -def share(ui, source, dest=None, noupdate=False):

> > +def share(ui, source, dest=None, noupdate=False, bookmarks=False):

> >      """create a new shared repository

> >

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

> > @@ -34,7 +36,7 @@

> >         the broken clone to reset it to a changeset that still exists.

> >      """

> >

> > -    return hg.share(ui, source, dest, not noupdate)

> > +    return hg.share(ui, source, dest, not noupdate, bookmarks)

> >

> >  @command('unshare', [], '')

> >  def unshare(ui, repo):

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

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

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

> > @@ -158,7 +158,7 @@

> >          return ''

> >      return os.path.basename(os.path.normpath(path))

> >

> > -def share(ui, source, dest=None, update=True):

> > +def share(ui, source, dest=None, update=True, bookmarks=False):

> >      '''create a shared repository'''

> >

> >      if not islocal(source):

> > @@ -225,6 +225,19 @@

> >                  continue

> >          _update(r, uprev)

> >

> > +    if bookmarks:

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

> 

> There is a typo on the ui.debug message.


Thanks for noticing that, I will fix it.

> 

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

> > +

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

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

> 

> 

> In addition to these comments I'd like to know if you plan to add additional

> sharing options (e.g. mq patches). On the last sprint I told mpm that I would

> look into doing something similar to this. The idea is to make it possible to use

> shared repos to create a sort of subrepository "cache" (although that is not

> really the right word).

> 

> The idea is to add a new "full share" mode that would create shares that

> share everything with their source except the minimum amount of things

> that are needed to maintain two separate working directories.

> In my implementation these full shares would work by making the

> destination repository use a vfs that would access the source repository in all

> cases except a few exceptions. This seems quite different to your approach.

> I'd like to make sure that what you do does not clash with what I am trying to

> do.

> 

> I already have most of this implemented, but I have been quite busy with RL

> these past few months so I am not ready to share it yet (no pun intended :-)

> ). I hope to have some time in the next few weeks though.


I don't have plans beyond bookmarks. If you have something coming down the pipe that does much more complete sharing than this, that sounds pretty promising and I would be happy to see my approach here reverted when that comes in. Do you feel landing a revised version of this patch would harm what you're trying to do?

> 

> Cheers,

> 

> Angel
Pierre-Yves David - Dec. 2, 2014, 11:55 p.m.
On 12/02/2014 02:23 PM, Angel Ezquerra wrote:
> In addition to these comments I'd like to know if you plan to add
> additional sharing options (e.g. mq patches). On the last sprint I
> told mpm that I would look into doing something similar to this. The
> idea is to make it possible to use shared repos to create a sort of
> subrepository "cache" (although that is not really the right word).
>
> The idea is to add a new "full share" mode that would create shares
> that share everything with their source except the minimum amount of
> things that are needed to maintain two separate working directories.
> In my implementation these full shares would work by making the
> destination repository use a vfs that would access the source
> repository in all cases except a few exceptions. This seems quite
> different to your approach. I'd like to make sure that what you do
> does not clash with what I am trying to do.

I'm working on reworking the transaction logic and the repository 
format. My initial aims is to make the transaction atomic, but I will 
try to clean things up and help the shared case in the process. We 
should probably talk about your usecase and need.
Angel Ezquerra - Dec. 3, 2014, 7:29 a.m.
El 03/12/2014 00:56, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
escribió:
>
>
>
> On 12/02/2014 02:23 PM, Angel Ezquerra wrote:
>>
>> In addition to these comments I'd like to know if you plan to add
>> additional sharing options (e.g. mq patches). On the last sprint I
>> told mpm that I would look into doing something similar to this. The
>> idea is to make it possible to use shared repos to create a sort of
>> subrepository "cache" (although that is not really the right word).
>>
>> The idea is to add a new "full share" mode that would create shares
>> that share everything with their source except the minimum amount of
>> things that are needed to maintain two separate working directories.
>> In my implementation these full shares would work by making the
>> destination repository use a vfs that would access the source
>> repository in all cases except a few exceptions. This seems quite
>> different to your approach. I'd like to make sure that what you do
>> does not clash with what I am trying to do.
>
>
> I'm working on reworking the transaction logic and the repository format.
My initial aims is to make the transaction atomic, but I will try to clean
things up and help the shared case in the process. We should probably talk
about your usecase and need.

I'm literally going to the hospital right now, to have our second baby, so
I'll be busy for a couple of days! :-) After that we could schedule some
time on IRC to chat about this in some detail?

Angel

Patch

diff --git a/hgext/share.py b/hgext/share.py
--- a/hgext/share.py
+++ b/hgext/share.py
@@ -13,10 +13,12 @@ 
 testedwith = 'internal'
 
 @command('share',
-    [('U', 'noupdate', None, _('do not create a working copy'))],
-    _('[-U] SOURCE [DEST]'),
+    [('U', 'noupdate', None, _('do not create a working copy')),
+     ('B', 'bookmarks', None, _('share bookmarks with source repository')),
+    ],
+    _('[-U] [-B] SOURCE [DEST]'),
     norepo=True)
-def share(ui, source, dest=None, noupdate=False):
+def share(ui, source, dest=None, noupdate=False, bookmarks=False):
     """create a new shared repository
 
     Initialize a new repository and working directory that shares its
@@ -34,7 +36,7 @@ 
        the broken clone to reset it to a changeset that still exists.
     """
 
-    return hg.share(ui, source, dest, not noupdate)
+    return hg.share(ui, source, dest, not noupdate, bookmarks)
 
 @command('unshare', [], '')
 def unshare(ui, repo):
diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -158,7 +158,7 @@ 
         return ''
     return os.path.basename(os.path.normpath(path))
 
-def share(ui, source, dest=None, update=True):
+def share(ui, source, dest=None, update=True, bookmarks=False):
     '''create a shared repository'''
 
     if not islocal(source):
@@ -225,6 +225,19 @@ 
                 continue
         _update(r, uprev)
 
+    if bookmarks:
+        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