Patchwork bookmarks: allow pushkey if new equals current

login
register
mail settings
Submitter Durham Goode
Date Aug. 26, 2014, 1:56 p.m.
Message ID <c27758084fdb17115211.1409061375@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/5600/
State Accepted
Headers show

Comments

Durham Goode - Aug. 26, 2014, 1:56 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1409054321 25200
#      Tue Aug 26 04:58:41 2014 -0700
# Node ID c27758084fdb17115211958666f3184282291143
# Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
bookmarks: allow pushkey if new equals current

Previously a bookmark pushkey would be rejected if the specified 'old' value
didn't match the servers 'current' value. This change allows this situation, as
long as the 'current' server value equals the 'new' pushkey value already.

We are trying to write a hook that forces a server bookmark to move forward,
using a changegroup hook. If the user also pushed the bookmark, they would get
an error, since they computed their pushkey (old,new) pair before the server
moved the bookmark. Long term, bundle2 will let us do this more smartly, but
this change seems reasonable for now.
Augie Fackler - Aug. 26, 2014, 7:05 p.m.
On Tue, Aug 26, 2014 at 06:56:15AM -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1409054321 25200
> #      Tue Aug 26 04:58:41 2014 -0700
> # Node ID c27758084fdb17115211958666f3184282291143
> # Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
> bookmarks: allow pushkey if new equals current

Seems reasonable. Queued.

>
> Previously a bookmark pushkey would be rejected if the specified 'old' value
> didn't match the servers 'current' value. This change allows this situation, as
> long as the 'current' server value equals the 'new' pushkey value already.
>
> We are trying to write a hook that forces a server bookmark to move forward,
> using a changegroup hook. If the user also pushed the bookmark, they would get
> an error, since they computed their pushkey (old,new) pair before the server
> moved the bookmark. Long term, bundle2 will let us do this more smartly, but
> this change seems reasonable for now.
>
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -228,7 +228,8 @@
>      w = repo.wlock()
>      try:
>          marks = repo._bookmarks
> -        if hex(marks.get(key, '')) != old:
> +        existing = hex(marks.get(key, ''))
> +        if existing != old and existing != new:
>              return False
>          if new == '':
>              del marks[key]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Aug. 26, 2014, 8:05 p.m.
On 08/26/2014 09:05 PM, Augie Fackler wrote:
> On Tue, Aug 26, 2014 at 06:56:15AM -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1409054321 25200
>> #      Tue Aug 26 04:58:41 2014 -0700
>> # Node ID c27758084fdb17115211958666f3184282291143
>> # Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
>> bookmarks: allow pushkey if new equals current
>
> Seems reasonable. Queued.

Does not seems that reasonable to me. Can we discuss it at the sprint?
Augie Fackler - Aug. 26, 2014, 8:06 p.m.
On Aug 26, 2014, at 4:05 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 
> 
> On 08/26/2014 09:05 PM, Augie Fackler wrote:
>> On Tue, Aug 26, 2014 at 06:56:15AM -0700, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1409054321 25200
>>> #      Tue Aug 26 04:58:41 2014 -0700
>>> # Node ID c27758084fdb17115211958666f3184282291143
>>> # Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
>>> bookmarks: allow pushkey if new equals current
>> 
>> Seems reasonable. Queued.
> 
> Does not seems that reasonable to me. Can we discuss it at the sprint?

Why not elaborate here? Basically, if you're making a request and the end state of your request is already the current state, I don't really see a reason to reject the push. Can you think of something?

> 
> -- 
> Pierre-Yves David
Pierre-Yves David - Aug. 26, 2014, 8:09 p.m.
On 08/26/2014 10:06 PM, Augie Fackler wrote:
>
> On Aug 26, 2014, at 4:05 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 08/26/2014 09:05 PM, Augie Fackler wrote:
>>> On Tue, Aug 26, 2014 at 06:56:15AM -0700, Durham Goode wrote:
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1409054321 25200
>>>> #      Tue Aug 26 04:58:41 2014 -0700
>>>> # Node ID c27758084fdb17115211958666f3184282291143
>>>> # Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
>>>> bookmarks: allow pushkey if new equals current
>>>
>>> Seems reasonable. Queued.
>>
>> Does not seems that reasonable to me. Can we discuss it at the sprint?
>
> Why not elaborate here?

Because its is 22h00 and I'm exausted ?

> Basically, if you're making a request and the end state of your request is already the current state, I don't really see a reason to reject the push. Can you think of something?

because it highlight the fact that the reasoning behind the pushkey was 
raced. And Mercurial abort all other case of race (whater the end state is).
Gregory Szorc - Aug. 26, 2014, 8:18 p.m.
On 8/26/14 1:09 PM, Pierre-Yves David wrote:
>
>
> On 08/26/2014 10:06 PM, Augie Fackler wrote:
>>
>> On Aug 26, 2014, at 4:05 PM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 08/26/2014 09:05 PM, Augie Fackler wrote:
>>>> On Tue, Aug 26, 2014 at 06:56:15AM -0700, Durham Goode wrote:
>>>>> # HG changeset patch
>>>>> # User Durham Goode <durham@fb.com>
>>>>> # Date 1409054321 25200
>>>>> #      Tue Aug 26 04:58:41 2014 -0700
>>>>> # Node ID c27758084fdb17115211958666f3184282291143
>>>>> # Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
>>>>> bookmarks: allow pushkey if new equals current
>>>>
>>>> Seems reasonable. Queued.
>>>
>>> Does not seems that reasonable to me. Can we discuss it at the sprint?
>>
>> Why not elaborate here?
>
> Because its is 22h00 and I'm exausted ?
>
>> Basically, if you're making a request and the end state of your
>> request is already the current state, I don't really see a reason to
>> reject the push. Can you think of something?
>
> because it highlight the fact that the reasoning behind the pushkey was
> raced. And Mercurial abort all other case of race (whater the end state
> is).

There is no race on the server, only a race on two clients who begin the 
push around the same time, but starting with different views of the 
server. As long as the result of the push is consistent and idempotent, 
we should be good.

This patch seems reasonable to me.
Pierre-Yves David - Aug. 26, 2014, 8:53 p.m.
On 08/26/2014 10:18 PM, Gregory Szorc wrote:
> On 8/26/14 1:09 PM, Pierre-Yves David wrote:
>>
>>
>> On 08/26/2014 10:06 PM, Augie Fackler wrote:
>>>
>>> On Aug 26, 2014, at 4:05 PM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>>
>>>>
>>>> On 08/26/2014 09:05 PM, Augie Fackler wrote:
>>>>> On Tue, Aug 26, 2014 at 06:56:15AM -0700, Durham Goode wrote:
>>>>>> # HG changeset patch
>>>>>> # User Durham Goode <durham@fb.com>
>>>>>> # Date 1409054321 25200
>>>>>> #      Tue Aug 26 04:58:41 2014 -0700
>>>>>> # Node ID c27758084fdb17115211958666f3184282291143
>>>>>> # Parent  7eef5a87ce3ff761711c69526bf422fdc2dc696d
>>>>>> bookmarks: allow pushkey if new equals current
>>>>>
>>>>> Seems reasonable. Queued.
>>>>
>>>> Does not seems that reasonable to me. Can we discuss it at the sprint?
>>>
>>> Why not elaborate here?
>>
>> Because its is 22h00 and I'm exausted ?
>>
>>> Basically, if you're making a request and the end state of your
>>> request is already the current state, I don't really see a reason to
>>> reject the push. Can you think of something?
>>
>> because it highlight the fact that the reasoning behind the pushkey was
>> raced. And Mercurial abort all other case of race (whater the end state
>> is).
>
> There is no race on the server, only a race on two clients who begin the
> push around the same time, but starting with different views of the
> server. As long as the result of the push is consistent and idempotent,
> we should be good.

The current strategy is for client to take all decisions. It  look at 
the server, decide for change to send and includes a way for the server 
to validate it is still in the state it was when the decision was made. 
This is how it used to work for bookmark and how it still work for 
changesets and phases. I do not believe the current special case is 
worth breaking the rules.

I still think we should delay discussion until the sprint.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -228,7 +228,8 @@ 
     w = repo.wlock()
     try:
         marks = repo._bookmarks
-        if hex(marks.get(key, '')) != old:
+        existing = hex(marks.get(key, ''))
+        if existing != old and existing != new:
             return False
         if new == '':
             del marks[key]