Incorrect divergent heads message on mercurial push
Closed, ResolvedPublic
Actions

Assigned To

None

Authored By

	fedegiova
	Mar 25 2016, 10:39 AM

Description

Reproducible steps:

Disallow dangerous changes
Using mercurial push a set of commits that updates multiple branches at the same time

Expected result:

The commits get pushed since the change-set doesn't try to create multiple heads on the same branch

Actual result:

The commits get rejected with

DANGEROUS CHANGE: The change you're attempting to push creates new, divergent heads for the branch 'ADC_float': 25ce5a013456. This is inadvisable and dangerous.

I've done the triage of the issue and I've found that is due to an uninitialized variable in src/applications/diffusion/engine/DiffusionCommitHookEngine.php.

At line 827 and 829 the variable head_map is augmented with a new key-value pair but the container has not been cleared; therefore the map accumulates items at every iteration of the loop, since at line 849 count($head_map) is used to detect the error condition that condition can be spuriously triggered if the pushed change-set involved multiple branches.

The fix consists in clearing the variable at every iteration like its already done in the other branches of the if .. else chain.

The proposed solution is attached as a patch.

phabricator_push.patch1 KBDownload

Version information:

phabricator fd72a2ff810144a80e396e8d68b41f07234e404a (Sat, Mar 12)
arcanist bb276740e7a87d7af52f9e9295cebf842c247425 (Sat, Mar 12)
phutil 89c7c2072813fd15bc66be8f7acbf2f8bc7e9bb6 (Sat, Mar 12)

Revisions and Commits

rP Phabricator
	D15522	rP4dc857e36d54 Fix an issue with incorrect split head detection in Mercurial after pushing a…

Related Objects

Mentioned Here: rPHU89c7c2072813: (stable) Promote 2016 Week 11
rARCbb276740e7a8: (stable) Promote 2016 Week 11
rPfd72a2ff8101: (stable) Promote 2016 Week 11

Event Timeline

fedegiova created this task.Mar 25 2016, 10:39 AM

Using mercurial push a set of commits that updates multiple branches at the same time

While it does look like the code is incorrect, I'm having trouble reproducing this part, specifically. I added some debugging code near line 832:

var_dump(
  array(
    'head_map' => $head_map,
  ));

Here's local working copy state before pushing: I have a new commit on two branches, branch-a and branch-b:

$ hg outgoing
comparing with ssh://local@localvault.phacility.com/diffusion/HGTEST/mercurial/
searching for changes
changeset:   24:a2ffdd988760
branch:      branch-a
parent:      22:17c9bca73cc2
user:        Evan Priestley <hg@yghe.net>
date:        Fri Mar 25 04:49:49 2016 -0700
summary:     x

changeset:   25:840d98510e02
branch:      branch-b
tag:         tip
parent:      23:01e656a1f89e
user:        Evan Priestley <hg@yghe.net>
date:        Fri Mar 25 04:51:54 2016 -0700
summary:     x

Now I push:

$ hg push -f
pushing to ssh://local@localvault.phacility.com/diffusion/HGTEST/mercurial/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 1 changes to 1 files
remote: array(1) {
remote:   ["head_map"]=>
remote:   array(1) {
remote:     ["01e656a1f89e39ec554c09eeb1c62266192f1998"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "840d98510e02c28a61e2ffa80faf5f8c21d63069"
remote:     }
remote:   }
remote: }
remote: array(1) {
remote:   ["head_map"]=>
remote:   array(2) {
remote:     ["01e656a1f89e39ec554c09eeb1c62266192f1998"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "840d98510e02c28a61e2ffa80faf5f8c21d63069"
remote:     }
remote:     ["17c9bca73cc2c4ec5f23824cd3ddbfc7e264aeeb"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "a2ffdd9887609aef383be238766dc43fbdff36c9"
remote:     }
remote:   }
remote: }
remote: [2016-03-25 04:53:47] EXCEPTION: (Exception) DEBUG: Stopping before accepting commit! at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:918]
remote: arcanist(head=master, ref.master=3d7ac867f538), corgi(head=87c4e69a9f9654390770045ac0e65271a3b542ab, ref.master=ad26f63a398b), instances(head=master, ref.master=794f8c37b870), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=master, ref.master=3493d9d5138e, custom=8), phutil(head=master, ref.master=b4f38af38480), services(head=master, ref.master=7feda77eb078)
remote:   #0 DiffusionCommitHookEngine::findMercurialChangegroupRefUpdates() called at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:688]
remote:   #1 DiffusionCommitHookEngine::findMercurialRefUpdates() called at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:214]
remote:   #2 DiffusionCommitHookEngine::findRefUpdates() called at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:116]
remote:   #3 DiffusionCommitHookEngine::execute() called at [<phabricator>/scripts/repository/commit_hook.php:134]
remote: transaction abort!
remote: rollback completed
abort: pretxnchangegroup.phabricator hook exited with status 255

While the state of head_map is not really correct the second time through, it gets the correct behavior (doesn't improperly detect split heads).

I likely built my local branches differently than you did -- how can I build a local state without split heads which reproduces this misfire of the detection code?

The only way I can see to get to an error state is if both branches had the same head before the push? I'm not sure how to build that state, although maybe I'm just missing some subtlety of hg branch or some sequence of merge/close operations which can force it.

I also tried pushing two brand new branches, but didn't hit issues on that, either.

I've rolled back my state to better identify the steps.

Here is the status of the repo before the push ( I've removed some commits which were not fundamental to the shape of the tree ) :

o  changeset:   36:9bbee24c0d4a
|  tag:         tip
|  user:        Francesco Bursi
|  date:        Fri Oct 30 08:17:35 2015 +0100
|  summary:     v1.00beta18
|
o  changeset:   35:a7cd41a9b58f
|  user:        Francesco Bursi
|  date:        Tue Sep 15 10:50:48 2015 +0200
|  summary:     v1.00beta17 emFile initialization bug fixed.
|
o  changeset:   27:fd9f23103726
|  user:        Francesco Bursi
|  date:        Wed Jan 28 13:52:09 2015 +0100
|  summary:     v1.00beta13
|
o    changeset:   26:311d66230c2b
|\   parent:      20:93c20e6696cd
| |  parent:      25:85418da42ded
| |  user:        Francesco Bursi
| |  date:        Tue Jan 27 14:48:24 2015 +0100
| |  summary:     Merge with emFile
| |
| o  changeset:   22:51f229e21b1c
| |  branch:      emFile
| |  user:        Francesco Bursi
| |  date:        Mon Jan 26 15:34:08 2015 +0100
| |  summary:     SPI 30MHz
| |
| o  changeset:   21:5f9c5b95106a
|/   branch:      emFile
|    user:        Francesco Bursi
|    date:        Mon Jan 26 15:10:54 2015 +0100
|    summary:     SPI 16MHz
|
o  changeset:   1:0f24efdbb52c
|  user:        Francesco Bursi
|  date:        Thu Apr 03 11:09:01 2014 +0200
|  summary:     Makefile modified
|
o  changeset:   0:178c8833be68
   user:        Francesco Bursi
   date:        Wed Apr 02 10:18:08 2014 +0200
   summary:     First commit

Here is the outgoing changeset

comparing with http://testuser:***@phabricator.local.com/diffusion/14/bsp/
searching for changes
@  changeset:   47:a2546ac21d8d
|  tag:         tip
|  parent:      43:40bbc01a3662
|  user:        Francesco Bursi
|  date:        Thu Mar 17 15:22:54 2016 +0100
|  summary:     file system tests
|
| o  changeset:   46:25ce5a013456
| |  branch:      ADC_float
| |  user:        Francesco Bursi
| |  date:        Mon Mar 21 14:53:22 2016 +0100
| |  summary:     efs patch tests
| |
| o  changeset:   45:8c3bf1169178
| |  branch:      ADC_float
| |  user:        Francesco Bursi
| |  date:        Mon Mar 21 08:39:44 2016 +0100
| |  summary:     HydraulicGetOpenChannelCurrent and HydraulicGetCloseChannelCurrent macro added
| |
| o  changeset:   44:13d5eecc31e0
|/   branch:      ADC_float
|    user:        Francesco Bursi
|    date:        Fri Mar 18 16:25:59 2016 +0100
|    summary:     Patched file system
|
o  changeset:   39:47583ac0e860
   user:        Francesco Bursi
   date:        Fri Oct 30 10:06:39 2015 +0100
   summary:     External packages fixed at a specific id (hash) code

o  changeset:   31:0adbbeec95be
|  branch:      tcl beta
|  user:        Francesco Bursi
|  date:        Thu Apr 30 16:51:30 2015 +0200
|  summary:     destroyer for private data added
|
o  changeset:   30:049da5931107
   branch:      tcl beta
   user:        Francesco Bursi
   date:        Thu Apr 30 14:32:56 2015 +0200
   summary:     wip picol used, finding a solution to destroy private function data

And finally the output of your debug code:

pushing to http://testuser:***@phabricator.local.com/diffusion/14/bsp/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 11 changesets with 154 changes to 140 files (+2 heads)
remote: array(1) {
remote:   ["head_map"]=>
remote:   array(1) {
remote:     ["9bbee24c0d4a88eeca83ae86b7333115f9eb1cb0"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "a2546ac21d8da5eb760c64fe6d2a20371aadb3d8"
remote:     }
remote:   }
remote: }
remote: array(1) {
remote:   ["head_map"]=>
remote:   array(2) {
remote:     ["9bbee24c0d4a88eeca83ae86b7333115f9eb1cb0"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "a2546ac21d8da5eb760c64fe6d2a20371aadb3d8"
remote:     }
remote:     ["0000000000000000000000000000000000000000"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "25ce5a013456fe7c0e34339ad4c659217c6ba143"
remote:     }
remote:   }
remote: }
remote: array(1) {
remote:   ["head_map"]=>
remote:   array(2) {
remote:     ["9bbee24c0d4a88eeca83ae86b7333115f9eb1cb0"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "a2546ac21d8da5eb760c64fe6d2a20371aadb3d8"
remote:     }
remote:     ["0000000000000000000000000000000000000000"]=>
remote:     array(1) {
remote:       [0]=>
remote:       string(40) "0adbbeec95be3e7705ede59747e925cab9055b26"
remote:     }
remote:   }
remote: }
remote: +---------------------------------------------------------------+
remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
remote: +---------------------------------------------------------------+
remote:             
remote: DANGEROUS CHANGE: The change you're attempting to push creates new, divergent heads for the branch 'ADC_float': 25ce5a013456. This is inadvisable and dangerous.
remote: Dangerous change protection is enabled for this repository.

So the push of new head of branch default: a2546ac21d8da5eb760c64fe6d2a20371aadb3d8 persist
into the head_map container and prevents the push of the ADC_float branch from being accepted

I've cloned the repo to a test machine, so I'm available to make more tests if you need

Ah, thanks! To hit this, I think we need to push a mixture of changes:

one or more commits which update an existing branch head; and
one or more commits which create a new branch head (of a different branch); and
the new branch needs to be processed after the update (not sure what determines this order).

I reproduced it locally, though, and confirmed your fix patch experimentally.

epriestley added a revision: D15522: Fix an issue with incorrect split head detection in Mercurial after pushing a medley of varied changes.Mar 25 2016, 2:26 PM

epriestley closed this task as Resolved by committing rP4dc857e36d54: Fix an issue with incorrect split head detection in Mercurial after pushing a….Mar 25 2016, 2:34 PM

epriestley added a commit: rP4dc857e36d54: Fix an issue with incorrect split head detection in Mercurial after pushing a….

I think this should now be resolved in HEAD of master. It will likely promote to stable in about 24 hours.

Please let us know if you're still seeing issues or run into anything else.

Thanks for the report! Your analysis and instructions were extremely helpful in understanding, reproducing, and resolving the issue.

Incorrect divergent heads message on mercurial pushClosed, ResolvedPublicActions