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

[Ryan McElroy]

> -----Original Message-----
> From: Matt Mackall [mailto:mpm at selenic.com]
> Sent: Thursday, December 4, 2014 4:24 PM
> To: Ryan McElroy
> Cc: mercurial-devel at 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 at 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.
>