Patchwork [2,of,3,V3] exchange: add hooks before and after bookmark pull

login
register
mail settings
Submitter Ryan McElroy
Date Dec. 3, 2014, 5:09 a.m.
Message ID <e56cddc7a7f746881c40.1417583361@devbig105.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6974/
State Superseded
Headers show

Comments

Ryan McElroy - Dec. 3, 2014, 5:09 a.m.
# HG changeset patch
# User Ryan McElroy <rmcelroy@fb.com>
# Date 1417581231 28800
#      Tue Dec 02 20:33:51 2014 -0800
# Node ID e56cddc7a7f746881c40edf62eabfea6d9a829be
# Parent  8904023bad6c33d487253918437b31ac04d98c9a
exchange: add hooks before and after bookmark pull

The motivation behind these hooks is to enable proper testing of the share
extension's changes that enable sharing bookmarks. However, these could also
be used to ensure that certain bookmarks are never written locally, for example.
Matt Mackall - Dec. 5, 2014, 12:23 a.m.
On Tue, 2014-12-02 at 21:09 -0800, Ryan McElroy wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@fb.com>
> # Date 1417581231 28800
> #      Tue Dec 02 20:33:51 2014 -0800
> # Node ID e56cddc7a7f746881c40edf62eabfea6d9a829be
> # Parent  8904023bad6c33d487253918437b31ac04d98c9a
> exchange: add hooks before and after bookmark pull
> 
> The motivation behind these hooks is to enable proper testing of the share
> extension's changes that enable sharing bookmarks. However, these could also
> be used to ensure that certain bookmarks are never written locally, for example.

In general, adding a new user-visible feature Y just to support testing
the feature X you really want to add is not helping your cause. Because
it just pulls in a whole digression about whether Y is a good idea and
what precise form it should take and where are the docs, tests, etc. and
if I end up not liking Y for whatever reason, I can't just take X
because of the dependency.

Here, these hooks might be a good idea, but I'd rather have that
discussion in the context of "what is the total collection of hooks that
make sense for users with bundle2 and how can we make them nice and
organized and orthogonal" rather than "how can I test this corner-case
of shared bookmarks". And there's no reason to hold your work back on
that.

So for now, let's just defer that piece of the test. (Or do some sort of
trivial extension to trigger the transaction abort at the right place.)

What's the locking story here? Am I reading correctly that we'll try to
take the wlock on scrrepo?

I'm also not sold on sharing bookmarks by default. My concern is that
people might for instance want to do / be already doing something like:

/home/user/pub-repo <- has the public bookmarks, shared with hgweb
/home/user/prv-repo <- shared copy with private bookmarks and secret
csets

..and be sad when their private bookmarks get out. I don't think I've
seen anyone make a counterargument to that yet.
Ryan McElroy - Dec. 8, 2014, 5:50 p.m.
> -----Original Message-----

> From: Matt Mackall [mailto:mpm@selenic.com]

> Sent: Thursday, December 4, 2014 4:24 PM

> To: Ryan McElroy

> Cc: mercurial-devel@selenic.com

> Subject: Re: [PATCH 2 of 3 V3] exchange: add hooks before and after

> bookmark pull

> 

> On Tue, 2014-12-02 at 21:09 -0800, Ryan McElroy wrote:

> > # HG changeset patch

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

> > # Date 1417581231 28800

> > #      Tue Dec 02 20:33:51 2014 -0800

> > # Node ID e56cddc7a7f746881c40edf62eabfea6d9a829be

> > # Parent  8904023bad6c33d487253918437b31ac04d98c9a

> > exchange: add hooks before and after bookmark pull

> >

> > The motivation behind these hooks is to enable proper testing of the

> > share extension's changes that enable sharing bookmarks. However,

> > these could also be used to ensure that certain bookmarks are never

> written locally, for example.

> 

> In general, adding a new user-visible feature Y just to support testing the

> feature X you really want to add is not helping your cause. Because it just

> pulls in a whole digression about whether Y is a good idea and what precise

> form it should take and where are the docs, tests, etc. and if I end up not

> liking Y for whatever reason, I can't just take X because of the dependency.

> 

> Here, these hooks might be a good idea, but I'd rather have that discussion in

> the context of "what is the total collection of hooks that make sense for

> users with bundle2 and how can we make them nice and organized and

> orthogonal" rather than "how can I test this corner-case of shared

> bookmarks". And there's no reason to hold your work back on that.

> 

> So for now, let's just defer that piece of the test. (Or do some sort of trivial

> extension to trigger the transaction abort at the right place.)


I'll do the extension method of testing and remove the hooks requirement.

> 

> What's the locking story here? Am I reading correctly that we'll try to take the

> wlock on scrrepo?


It just uses the srcrepo object to do the read, and currently the bookmarks code takes the wlock, yes.

> 

> I'm also not sold on sharing bookmarks by default. My concern is that people

> might for instance want to do / be already doing something like:

> 

> /home/user/pub-repo <- has the public bookmarks, shared with hgweb

> /home/user/prv-repo <- shared copy with private bookmarks and secret

> csets

> 

> ..and be sad when their private bookmarks get out. I don't think I've seen

> anyone make a counterargument to that yet.


I'm happy to add this back in as an option. The next version will have it optional again.

> 

> --

> Mathematics is the supreme nostalgia of our time.

>
Pierre-Yves David - Dec. 8, 2014, 11:46 p.m.
On 12/04/2014 04:23 PM, Matt Mackall wrote:
> I'm also not sold on sharing bookmarks by default.

I see: `hg share xxx` as parallel with `hg clone xxx` and we changed `hg 
clone` and `hg pull` to take all bookmarks by default.

(side note: `hg share xxx` does bring bookmark along as hg clone do, I 
see this as bug that could be easily fixed)

> My concern is that people might for instance want to do / be already doing something like:
>
> /home/user/pub-repo <- has the public bookmarks, shared with hgweb
> /home/user/prv-repo <- shared copy with private bookmarks and secret
> csets
>
> ..and be sad when their private bookmarks get out. I don't think I've
> seen anyone make a counterargument to that yet.

Bookmark on secret changesets should not get out of the repo. So it 
should be fine.

I think it is very disturbing to see changesets created/moving around 
without the associated bookmarks. I think we have more current users of 
share that give up on bookmarks (or get bitten) for this reason than 
users that rely on the current behavior.

Bookmarks have been a core concept for years, but yet they struggle to 
get widespread confidence because they broke expectations in a lot of 
ways, feeling like a 1.5 class citizen (yes, I know, evolve is part of 
that). I feel like we need to push them more as a first class concept.

We already did some heavy backward compat breakage when we moved to 
exchange all bookmark by default on clone and pulls. I see this as a 
similar move (breaking less people) adding more consistency.

Maybe a middle ground is to have good "local" namespace in a remote 
bookmark worlds ?
Ryan McElroy - Dec. 12, 2014, 9:25 p.m.
I have an idea that might please everyone. Check it out!

First, go back to having sharing bookmarks be optional. Second enable it by default for new shared repositories, but don't turn it on for already-shared repositories. This way, a user upgrading won't suddenly be sharing their bookmarks when they weren't before, but new shares will get the expected behavior of sharing bookmarks. I think that makes the default in a way to satisfy Pierre-Yves, but makes sure we don't break the situation Matt outlined.

What do y'all think about this as a solution that solves the main concerns?

Thanks,

~Ryan

> -----Original Message-----

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

> Sent: Monday, December 8, 2014 3:47 PM

> To: Matt Mackall; Ryan McElroy

> Cc: mercurial-devel@selenic.com

> Subject: Re: [PATCH 2 of 3 V3] exchange: add hooks before and after

> bookmark pull

> 

> 

> 

> On 12/04/2014 04:23 PM, Matt Mackall wrote:

> > I'm also not sold on sharing bookmarks by default.

> 

> I see: `hg share xxx` as parallel with `hg clone xxx` and we changed `hg

> clone` and `hg pull` to take all bookmarks by default.

> 

> (side note: `hg share xxx` does bring bookmark along as hg clone do, I

> see this as bug that could be easily fixed)

> 

> > My concern is that people might for instance want to do / be already doing

> something like:

> >

> > /home/user/pub-repo <- has the public bookmarks, shared with hgweb

> > /home/user/prv-repo <- shared copy with private bookmarks and secret

> > csets

> >

> > ..and be sad when their private bookmarks get out. I don't think I've

> > seen anyone make a counterargument to that yet.

> 

> Bookmark on secret changesets should not get out of the repo. So it

> should be fine.

> 

> I think it is very disturbing to see changesets created/moving around

> without the associated bookmarks. I think we have more current users of

> share that give up on bookmarks (or get bitten) for this reason than

> users that rely on the current behavior.

> 

> Bookmarks have been a core concept for years, but yet they struggle to

> get widespread confidence because they broke expectations in a lot of

> ways, feeling like a 1.5 class citizen (yes, I know, evolve is part of

> that). I feel like we need to push them more as a first class concept.

> 

> We already did some heavy backward compat breakage when we moved to

> exchange all bookmark by default on clone and pulls. I see this as a

> similar move (breaking less people) adding more consistency.

> 

> Maybe a middle ground is to have good "local" namespace in a remote

> bookmark worlds ?

> 

> --

> Pierre-Yves David
Matt Mackall - Dec. 12, 2014, 9:37 p.m.
On Fri, 2014-12-12 at 21:25 +0000, Ryan McElroy wrote:
> I have an idea that might please everyone. Check it out!
> 
> First, go back to having sharing bookmarks be optional. Second enable it by default for new shared repositories, but don't turn it on for already-shared repositories. This way, a user upgrading won't suddenly be sharing their bookmarks when they weren't before, but new shares will get the expected behavior of sharing bookmarks. I think that makes the default in a way to satisfy Pierre-Yves, but makes sure we don't break the situation Matt outlined.
> 
> What do y'all think about this as a solution that solves the main concerns?

I think you should make forward progress by sending the patch that
enables sharing but doesn't turn it on by default. That's clearly
incrementally better than the current situation.

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1080,13 +1080,15 @@  def _pullbookmarks(pullop):
     """process the remote bookmark information to update the local one"""
     if 'bookmarks' in pullop.stepsdone:
         return
+    repo = pullop.repo
+    repo.hook('prepullbookmarks', throw=True)
     pullop.stepsdone.add('bookmarks')
-    repo = pullop.repo
     remotebookmarks = pullop.remotebookmarks
     bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
                              pullop.remote.url(),
                              pullop.gettransaction,
                              explicit=pullop.explicitbookmarks)
+    repo.hook('postpullbookmarks', throw=True)
 
 def _pullobsolete(pullop):
     """utility function to pull obsolete markers from a remote