| 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
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.
> -----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. >
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 ?
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
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