Patchwork push should not allow to push diverged bookmark

login
register
mail settings
Submitter Marc Strapetz
Date Aug. 4, 2015, 12:26 p.m.
Message ID <55C0AF58.1080307@syntevo.com>
Download mbox | patch
Permalink /patch/10099/
State Superseded
Delegated to: Pierre-Yves David
Headers show

Comments

Marc Strapetz - Aug. 4, 2015, 12:26 p.m.
When having a bookmark for which there exists a new remote commit, it's 
still possible to push a diverged local commit without --force option. I 
think it's more reasonable to disallow such pushes, similar as when 
pushing a branch or a specific revision. The patch below should resolve 
the problem.

-Marc

# HG changeset patch
# User Marc Strapetz <marc.strapetz@syntevo.com>
# Date 1438690573 -7200
#      Tue Aug 04 14:16:13 2015 +0200
# Node ID 02624e707a527ded09612539b8e7fc26889679f7
# Parent  eabba9c75061254ff62827f92df0f32491c74b3d
push: disallow pushing diverged bookmark using "hg push <bookmark>" 
syntax without --force
Pierre-Yves David - Aug. 6, 2015, 10:24 p.m.
On 08/04/2015 05:26 AM, Marc Strapetz wrote:
> When having a bookmark for which there exists a new remote commit, it's
> still possible to push a diverged local commit without --force option. I
> think it's more reasonable to disallow such pushes, similar as when
> pushing a branch or a specific revision. The patch below should resolve
> the problem.

Can you give more details about the problematic cases ?
I'm not sure to know what case you are refering to (and we'll need to 
build a test case to prevent a regression).
Marc Strapetz - Aug. 7, 2015, 7 a.m.
On 07.08.2015 00:24, Pierre-Yves David wrote:
>
>
> On 08/04/2015 05:26 AM, Marc Strapetz wrote:
>> When having a bookmark for which there exists a new remote commit, it's
>> still possible to push a diverged local commit without --force option. I
>> think it's more reasonable to disallow such pushes, similar as when
>> pushing a branch or a specific revision. The patch below should resolve
>> the problem.
>
> Can you give more details about the problematic cases ?

Basically, every push of a bookmark is problematic. Consider following 
scenario:

Remote Repo ("default"):

r2 (ed20) o <bm>
           |
r1        o
           |
r0        o

Local Repo:

r2 (3b28) o <bm>
           |
r1        o
           |
r0        o

Now pushing the bookmark succeeds and creates a new head. This is 
usually not expected, but the push should fail instead.

$ push -B bm default
pushing to .../remote
searching for changes
remote has heads on branch 'default' that are not known locally: 
ed2016a85e51
adding changesets
adding manifests
adding file changes
added 1 changesets with 1 changes to 1 files (+1 heads)
updating bookmark bm

-Marc
Marc Strapetz - Sept. 3, 2015, 1:20 p.m.
On 07.08.2015 09:00, Marc Strapetz wrote:
> On 07.08.2015 00:24, Pierre-Yves David wrote:
>>
>>
>> On 08/04/2015 05:26 AM, Marc Strapetz wrote:
>>> When having a bookmark for which there exists a new remote commit, it's
>>> still possible to push a diverged local commit without --force option. I
>>> think it's more reasonable to disallow such pushes, similar as when
>>> pushing a branch or a specific revision. The patch below should resolve
>>> the problem.
>>
>> Can you give more details about the problematic cases ?
>
> Basically, every push of a bookmark is problematic. Consider following
> scenario:
>
> Remote Repo ("default"):
>
> r2 (ed20) o <bm>
>            |
> r1        o
>            |
> r0        o
>
> Local Repo:
>
> r2 (3b28) o <bm>
>            |
> r1        o
>            |
> r0        o
>
> Now pushing the bookmark succeeds and creates a new head. This is
> usually not expected, but the push should fail instead.
>
> $ push -B bm default
> pushing to .../remote
> searching for changes
> remote has heads on branch 'default' that are not known locally:
> ed2016a85e51
> adding changesets
> adding manifests
> adding file changes
> added 1 changesets with 1 changes to 1 files (+1 heads)
> updating bookmark bm

Any other opinions on that?

-Marc
Ryan McElroy - Sept. 9, 2015, 2:59 a.m.
On 9/3/2015 6:20 AM, Marc Strapetz wrote:
> On 07.08.2015 09:00, Marc Strapetz wrote:
>> On 07.08.2015 00:24, Pierre-Yves David wrote:
>>>
>>>
>>> On 08/04/2015 05:26 AM, Marc Strapetz wrote:
>>>> When having a bookmark for which there exists a new remote commit, 
>>>> it's
>>>> still possible to push a diverged local commit without --force 
>>>> option. I
>>>> think it's more reasonable to disallow such pushes, similar as when
>>>> pushing a branch or a specific revision. The patch below should 
>>>> resolve
>>>> the problem.
>>>
>>> Can you give more details about the problematic cases ?
>>
>> Basically, every push of a bookmark is problematic. Consider following
>> scenario:
>>
>> Remote Repo ("default"):
>>
>> r2 (ed20) o <bm>
>>            |
>> r1        o
>>            |
>> r0        o
>>
>> Local Repo:
>>
>> r2 (3b28) o <bm>
>>            |
>> r1        o
>>            |
>> r0        o
>>
>> Now pushing the bookmark succeeds and creates a new head. This is
>> usually not expected, but the push should fail instead.
>>
>> $ push -B bm default
>> pushing to .../remote
>> searching for changes
>> remote has heads on branch 'default' that are not known locally:
>> ed2016a85e51
>> adding changesets
>> adding manifests
>> adding file changes
>> added 1 changesets with 1 changes to 1 files (+1 heads)
>> updating bookmark bm
>
> Any other opinions on that?
>
> -Marc

I strongly agree with the direction of this patch. I wasn't even aware 
it was possible to push a new head on a single branch without --force, 
but I tested it out and sure enough, this happens!

That feels like a regression to me (normally new heads on a branch are 
not allowed without --force, right?) But when you send a new head with a 
bookmark, it's accepted? This feel broken to me at several levels; this 
patch is a good fix for one of those levels.

I agree with @pyd that we should have a test to ensure we don't regress 
this behavior.
Pierre-Yves David - Oct. 2, 2015, 11:01 p.m.
Okay, this is still a huge UI trap and we should probably fixes it


I'm not sure about what we should do here (to allow this to happen when 
really wanted), allowing this with --force but aborting otherwise?

On 09/08/2015 07:59 PM, Ryan McElroy wrote:
> On 9/3/2015 6:20 AM, Marc Strapetz wrote:
>> On 07.08.2015 09:00, Marc Strapetz wrote:
>>> On 07.08.2015 00:24, Pierre-Yves David wrote:
>>>>
>>>>
>>>> On 08/04/2015 05:26 AM, Marc Strapetz wrote:
>>>>> When having a bookmark for which there exists a new remote commit,
>>>>> it's
>>>>> still possible to push a diverged local commit without --force
>>>>> option. I
>>>>> think it's more reasonable to disallow such pushes, similar as when
>>>>> pushing a branch or a specific revision. The patch below should
>>>>> resolve
>>>>> the problem.
>>>>
>>>> Can you give more details about the problematic cases ?
>>>
>>> Basically, every push of a bookmark is problematic. Consider following
>>> scenario:
>>>
>>> Remote Repo ("default"):
>>>
>>> r2 (ed20) o <bm>
>>>            |
>>> r1        o
>>>            |
>>> r0        o
>>>
>>> Local Repo:
>>>
>>> r2 (3b28) o <bm>
>>>            |
>>> r1        o
>>>            |
>>> r0        o
>>>
>>> Now pushing the bookmark succeeds and creates a new head. This is
>>> usually not expected, but the push should fail instead.
>>>
>>> $ push -B bm default
>>> pushing to .../remote
>>> searching for changes
>>> remote has heads on branch 'default' that are not known locally:
>>> ed2016a85e51
>>> adding changesets
>>> adding manifests
>>> adding file changes
>>> added 1 changesets with 1 changes to 1 files (+1 heads)
>>> updating bookmark bm
>>
>> Any other opinions on that?
>>
>> -Marc
>
> I strongly agree with the direction of this patch. I wasn't even aware
> it was possible to push a new head on a single branch without --force,
> but I tested it out and sure enough, this happens!
>
> That feels like a regression to me (normally new heads on a branch are
> not allowed without --force, right?) But when you send a new head with a
> bookmark, it's accepted? This feel broken to me at several levels; this
> patch is a good fix for one of those levels.
>
> I agree with @pyd that we should have a test to ensure we don't regress
> this behavior.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff -r eabba9c75061 -r 02624e707a52 mercurial/discovery.py
--- a/mercurial/discovery.py	Fri Jun 26 18:45:29 2015 -0500
+++ b/mercurial/discovery.py	Tue Aug 04 14:16:13 2015 +0200
@@ -265,7 +265,7 @@ 
              if bookmarks.validdest(repo, rctx, lctx):
                  bookmarkedheads.add(lctx.node())
          else:
-            if bm in newbookmarks:
+            if bm in newbookmarks and not bm in remotebookmarks:
                  bookmarkedheads.add(repo[bm].node())

      # 3. Check for new heads.