From 22df464e0d78d4cfb98aafa942f74e4ecfb1fe0b Mon Sep 17 00:00:00 2001
From: Quincy Morgan <quincy.morgan@icloud.com>
Date: Mon, 29 Jun 2020 10:51:12 -0400
Subject: [PATCH] Fix issue where valid merge operations could be blocked
 Disable merge when it'd create a redundant multipolygon or add a redundant
 membership

---
 modules/actions/merge_polygon.js | 31 ++++++++++++++++++++++++++++---
 modules/operations/merge.js      | 23 ++++++++++++++---------
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/modules/actions/merge_polygon.js b/modules/actions/merge_polygon.js
index 5c0aee36f..8d5829401 100644
--- a/modules/actions/merge_polygon.js
+++ b/modules/actions/merge_polygon.js
@@ -1,6 +1,6 @@
 import { geoPolygonContainsPolygon } from '../geo';
 import { osmJoinWays, osmRelation } from '../osm';
-import { utilArrayGroupBy, utilObjectOmit } from '../util';
+import { utilArrayGroupBy, utilArrayIntersection, utilObjectOmit } from '../util';
 
 
 export function actionMergePolygon(ids, newRelationId) {
@@ -114,10 +114,35 @@ export function actionMergePolygon(ids, newRelationId) {
     action.disabled = function(graph) {
         var entities = groupEntities(graph);
         if (entities.other.length > 0 ||
-            entities.closedWay.length + entities.multipolygon.length < 2)
+            entities.closedWay.length + entities.multipolygon.length < 2) {
             return 'not_eligible';
-        if (!entities.multipolygon.every(function(r) { return r.isComplete(graph); }))
+        }
+        if (!entities.multipolygon.every(function(r) { return r.isComplete(graph); })) {
             return 'incomplete_relation';
+        }
+
+        if (!entities.multipolygon.length) {
+            var sharedMultipolygons = [];
+            entities.closedWay.forEach(function(way, i) {
+                if (i === 0) {
+                    sharedMultipolygons = graph.parentMultipolygons(way);
+                } else {
+                    sharedMultipolygons = utilArrayIntersection(sharedMultipolygons, graph.parentMultipolygons(way));
+                }
+            });
+            sharedMultipolygons = sharedMultipolygons.filter(function(relation) {
+                return relation.members.length === entities.closedWay.length;
+            });
+            if (sharedMultipolygons.length) {
+                // don't create a new multipolygon if it'd be redundant
+                return 'not_eligible';
+            }
+        } else if (entities.closedWay.some(function(way) {
+                return utilArrayIntersection(graph.parentMultipolygons(way), entities.multipolygon).length;
+            })) {
+            // don't add a way to a multipolygon again if it's already a member
+            return 'not_eligible';
+        }
     };
 
 
diff --git a/modules/operations/merge.js b/modules/operations/merge.js
index ea7a2bbc1..fc0d0d50d 100644
--- a/modules/operations/merge.js
+++ b/modules/operations/merge.js
@@ -14,19 +14,24 @@ export function operationMerge(context, selectedIDs) {
     var _action = getAction();
 
     function getAction() {
+        // prefer a non-disabled action first
         var join = actionJoin(selectedIDs);
-        if (join.disabled(context.graph()) !== 'not_eligible') {
-            return join;
-        }
+        if (!join.disabled(context.graph())) return join;
+
         var merge = actionMerge(selectedIDs);
-        if (merge.disabled(context.graph()) !== 'not_eligible') {
-            return merge;
-        }
+        if (!merge.disabled(context.graph())) return merge;
+
         var mergePolygon = actionMergePolygon(selectedIDs);
-        if (mergePolygon.disabled(context.graph()) !== 'not_eligible') {
-            return mergePolygon;
-        }
+        if (!mergePolygon.disabled(context.graph())) return mergePolygon;
+
         var mergeNodes = actionMergeNodes(selectedIDs);
+        if (!mergeNodes.disabled(context.graph())) return mergeNodes;
+
+        // otherwise prefer an action with an interesting disabled reason
+        if (join.disabled(context.graph()) !== 'not_eligible') return join;
+        if (merge.disabled(context.graph()) !== 'not_eligible') return merge;
+        if (mergePolygon.disabled(context.graph()) !== 'not_eligible') return mergePolygon;
+
         return mergeNodes;
     }