[PATCH 1 of 2 RFC] bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B

[Stephen Lee]

On Sat, Mar 16, 2013 at 9:20 AM, Kevin Bullock
<kbullock+mercurial at ringworld.org> wrote:
> On 13 Mar 2013, at 6:13 AM, Stephen Lee wrote:
>
>> # HG changeset patch
>> # User Stephen Lee <sphen.lee at gmail.com>
>> # Date 1363171419 -39600
>> # Node ID 8dc95dea42930a4fa251f3bd67a3eb759b51e304
>> # Parent  a07be895373394be66ba38b1ff111e26aca03ac8
>> bookmarks: allow push to create a new remote head if it is about to be bookmarked via -B
>>
>> Push is currently allowed to create a new head if there is a remote bookmark that will
>> be updated to point to the new head.  If the bookmark is not known remotely then
>> push aborts, even if a -B argument is about to push the bookmark. This change allows
>> push to continue in this case.  This does not require a wireproto force.
>>
>> This is an RFC change.  The list of new bookmarks is stored in config to avoid changing
>> the signature of localrepo.push which is wrapped by many extensions (both bundled and external).
>> A real solution would need to find a cleaner way to do this.
>
> After thinking about it a bit more, the problem with this is that we don't (and can't) know whether the "new" bookmark exists remotely before we push the changesets. This will lead to people clobbering remote bookmarks when they just meant to create a new remote bookmark, and blaming us for it. This is currently prevented by the "abort: push creates new remote head deadbeefd00d!".
>
> (Note that you _can_ still clobber a remote bookmark by --force'ing the push, and then pushing the bookmark explicitly with -B.)
>
> So we need to prevent clobbering remote bookmarks when you didn't mean to. But in order to do that, we'd need to know ahead of time whether the bookmark already exists locally -- and we haven't done the pushkey exchange yet.
>
> It would probably be possible in the current wire protocol to send a listkeys('bookmarks') before the push, but this would be a) racy, and b) a (probably) much more invasive change to the client side. Or we could wait for bundle2. :/

The checkheads function in discovery.py actually does this already to
see if a new head is a valid destination for a remote bookmark.
It is racy, except the pushkey protocol has a built-in race detection
mechanism - the "old" parameter.  Pushing a new key requires the
previous value of the key to be known, we do this by listing all keys
(atomically), deciding what needs to be pushed, then using the old
parameter to ensure that the key did not change since we listed.

The problem is that the bookmark keys are listed twice, once in
discovery.py:checkheads, and again in commands.py:push.
This means, even though we know the bookmark does not exist remotely
in checkheads, it might be created before we get back to push - and in
this case we clobber it...  This problem exists even without my patch
(for the case where we check for valid destination).  For large
changegroups, the window where a race can occur can be large, however
in practice I suspect it never happens (does the unbundle wireproto
command grab the wlock?).

The correct way to do this is to listkeys at the beginning of push,
and somehow pass them into checkheads.
Perhaps a propertycache on the remote peer?  This certainly is a more
invasive change to the client (but so is bundle2 :P ).

Thanks for the review!
I hope I'm not being too pushy - bundle2 is an important step forward,
but I don't think I want to wait for it to be ready/stable.

>
> I remain open to correction on any of my above analysis, but I think we have to wait on this one.
>
> pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
> Kevin R. Bullock
>