From 719bf471563e0af3e28e8f1fb6866a4b6a133fd2 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Fri, 25 May 2018 13:55:44 -0700 Subject: [PATCH] Merge `merge`s (#297) Summary: We had three graph merging functions: `merge`, `mergeConservative`, and `mergeManyConservative`. Of these, `merge` was never used outside of code directly testing its behavior, and `mergeConservative` is a strictly inferior version of `mergeManyConservative`. This commit removes `merge` and `mergeConservative`, and renames `mergeManyConservative` to `mergeConservative`. Paired with @decentralion. Test Plan: Existing unit tests suffice; some useless tests pruned. wchargin-branch: mmeerrggee --- src/cli/commands/combine.js | 2 +- src/core/graph.js | 88 +++---------------------- src/core/graph.test.js | 114 ++++++++------------------------- src/plugins/git/createGraph.js | 2 +- src/tools/loadCombinedGraph.js | 2 +- 5 files changed, 39 insertions(+), 169 deletions(-) diff --git a/src/cli/commands/combine.js b/src/cli/commands/combine.js index ea2b093..2ea3f81 100644 --- a/src/cli/commands/combine.js +++ b/src/cli/commands/combine.js @@ -43,6 +43,6 @@ async function combine(filenames: $ReadOnlyArray) { ) ) ); - const result = Graph.mergeManyConservative(graphs); + const result = Graph.mergeConservative(graphs); console.log(stringify(result, {space: 4})); } diff --git a/src/core/graph.js b/src/core/graph.js index 3fc1c38..9f60a18 100644 --- a/src/core/graph.js +++ b/src/core/graph.js @@ -2,7 +2,7 @@ import deepEqual from "lodash.isequal"; import stringify from "json-stable-stringify"; -import type {Address, Addressable, AddressMapJSON} from "./address"; +import type {Address, AddressMapJSON} from "./address"; import {AddressMap} from "./address"; import {toCompat, fromCompat} from "../util/compat"; import type {Compatible} from "../util/compat"; @@ -64,7 +64,7 @@ export class Graph { } copy(): Graph { - return Graph.mergeConservative(new Graph(), this); + return Graph.mergeConservative([this]); } equals(that: Graph): boolean { @@ -411,86 +411,16 @@ export class Graph { } /** - * Merge two graphs. When two nodes have the same address, a resolver - * function will be called with the two nodes; the resolver should - * return a new node with the same address, which will take the place - * of the two nodes in the new graph. Edges have similar behavior. + * Merge a collection of graphs. If multiple graphs have a node with a + * particular address, then the nodes must all have identical payload, + * and a single copy of this node will be included in the result + * graph; if not all nodes are identical, then an error is thrown. + * Likewise, all edges at a particular address must have identical + * source, destination, and payload. * * The existing graph objects are not modified. */ - static merge( - g1: Graph, - g2: Graph, - nodeResolver: (Node, Node) => Node, - edgeResolver: (Edge, Edge) => Edge - ): Graph { - const result: Graph = new Graph(); - g1.nodes().forEach((node) => { - if (g2.node(node.address) !== undefined) { - const resolved = nodeResolver(node, g2.node(node.address)); - result.addNode(resolved); - } else { - result.addNode(node); - } - }); - g2.nodes().forEach((node) => { - if (result.node(node.address) === undefined) { - result.addNode(node); - } - }); - g1.edges().forEach((edge) => { - if (g2.edge(edge.address) !== undefined) { - const resolved = edgeResolver(edge, g2.edge(edge.address)); - result.addEdge(resolved); - } else { - result.addEdge(edge); - } - }); - g2.edges().forEach((edge) => { - if (result.edge(edge.address) === undefined) { - result.addEdge(edge); - } - }); - return result; - } - - /** - * Merge two graphs, assuming that if `g1` and `g2` both have a node - * with a given address, then the nodes are deep-equal (and the same - * for edges). If this assumption does not hold, this function will - * raise an error. - */ - static mergeConservative(g1: Graph, g2: Graph): Graph { - function conservativeResolver( - kinds: string /* used for an error message on mismatch */, - a: T, - b: T - ): T { - if (deepEqual(a, b)) { - return a; - } else { - throw new Error( - `distinct ${kinds} with address ${JSON.stringify(a.address)}` - ); - } - } - const result: Graph = Graph.merge( - g1, - g2, - (u, v) => conservativeResolver("nodes", u, v), - (e, f) => conservativeResolver("edges", e, f) - ); - return result; - } - - /** - * Equivalent to - * - * graphs.reduce((g, h) => Graph.mergeConservative(g, h), new Graph()), - * - * but uses a mutable accumulator for improved performance. - */ - static mergeManyConservative(graphs: $ReadOnlyArray): Graph { + static mergeConservative(graphs: $ReadOnlyArray): Graph { const result = new Graph(); graphs.forEach((graph) => { graph.nodes().forEach((node) => { diff --git a/src/core/graph.test.js b/src/core/graph.test.js index 6e1267f..cfac4b5 100644 --- a/src/core/graph.test.js +++ b/src/core/graph.test.js @@ -885,7 +885,7 @@ describe("graph", () => { }); }); - describe("merging", () => { + describe("mergeConservative", () => { /** * Decompose the given graph into neighborhood graphs: for each * node `u`, create a graph with just that node, its neighbors, @@ -943,80 +943,30 @@ describe("graph", () => { return [].concat(edgeGraphs, nodeGraphs); } - it("conservatively recomposes a neighborhood decomposition", () => { - const result = neighborhoodDecomposition( - demoData.advancedMealGraph() - ).reduce((g1, g2) => Graph.mergeConservative(g1, g2), new Graph()); + it("recomposes a neighborhood decomposition", () => { + const result = Graph.mergeConservative( + neighborhoodDecomposition(demoData.advancedMealGraph()) + ); expect(result.equals(demoData.advancedMealGraph())).toBe(true); }); - it("conservatively recomposes an edge decomposition", () => { + it("recomposes an edge decomposition", () => { const result = edgeDecomposition(demoData.advancedMealGraph()).reduce( - (g1, g2) => Graph.mergeConservative(g1, g2), + (g1, g2) => Graph.mergeConservative([g1, g2]), new Graph() ); expect(result.equals(demoData.advancedMealGraph())).toBe(true); }); - it("conservatively merges a graph with itself", () => { - const result = Graph.mergeConservative( + it("merges a graph with itself", () => { + const result = Graph.mergeConservative([ demoData.advancedMealGraph(), - demoData.advancedMealGraph() - ); + demoData.advancedMealGraph(), + ]); expect(result.equals(demoData.advancedMealGraph())).toBe(true); }); - it("conservatively merges graphs of different payload types", () => { - const data = { - a: () => ({ - address: demoData.makeAddress("a", "EXPERIMENT"), - payload: "alpha", - }), - b: () => ({ - address: demoData.makeAddress("b", "EXPERIMENT"), - payload: "bravo", - }), - u: () => ({ - address: demoData.makeAddress("u", "EXPERIMENT"), - src: demoData.makeAddress("a", "EXPERIMENT"), - dst: demoData.makeAddress("b", "EXPERIMENT"), - payload: 21, - }), - c: () => ({ - address: demoData.makeAddress("c", "EXPERIMENT"), - payload: true, - }), - d: () => ({ - address: demoData.makeAddress("d", "EXPERIMENT"), - payload: false, - }), - v: () => ({ - address: demoData.makeAddress("v", "EXPERIMENT"), - src: demoData.makeAddress("c", "EXPERIMENT"), - dst: demoData.makeAddress("d", "EXPERIMENT"), - payload: null, - }), - }; - const g1: Graph = new Graph() - .addNode(data.a()) - .addNode(data.b()) - .addEdge(data.u()); - const g2: Graph = new Graph() - .addNode(data.c()) - .addNode(data.d()) - .addEdge(data.v()); - const result = Graph.mergeConservative(g1, g2); - const expected = new Graph() - .addNode(data.a()) - .addNode(data.b()) - .addEdge(data.u()) - .addNode(data.c()) - .addNode(data.d()) - .addEdge(data.v()); - expect(result.equals(expected)).toBe(true); - }); - - it("conservatively rejects a graph with conflicting nodes", () => { + it("rejects a graph with conflicting nodes", () => { const makeGraph: (nodePayload: string) => Graph = (nodePayload) => new Graph().addNode({ address: demoData.makeAddress("conflicting-node", "EXPERIMENT"), @@ -1025,11 +975,11 @@ describe("graph", () => { const g1 = makeGraph("one"); const g2 = makeGraph("two"); expect(() => { - Graph.mergeConservative(g1, g2); - }).toThrow(/distinct nodes with address/); + Graph.mergeConservative([g1, g2]); + }).toThrow(/node.*distinct contents/); }); - it("conservatively rejects a graph with conflicting edges", () => { + it("rejects a graph with conflicting edges", () => { const srcAddress = demoData.makeAddress("src", "EXPERIMENT"); const dstAddress = demoData.makeAddress("dst", "EXPERIMENT"); const makeGraph: (edgePayload: string) => Graph = (edgePayload) => @@ -1045,40 +995,30 @@ describe("graph", () => { const g1 = makeGraph("one"); const g2 = makeGraph("two"); expect(() => { - Graph.mergeConservative(g1, g2); - }).toThrow(/distinct edges with address/); + Graph.mergeConservative([g1, g2]); + }).toThrow(/edge.*distinct contents/); + }); + + it("is the identity on a singleton list", () => { + const merged = Graph.mergeConservative([demoData.advancedMealGraph()]); + expect(merged.equals(demoData.advancedMealGraph())).toBe(true); }); - function assertNotCalled(...args) { - throw new Error(`called with: ${args.join()}`); - } it("has the empty graph as a left identity", () => { - const merged = Graph.merge( + const merged = Graph.mergeConservative([ new Graph(), demoData.advancedMealGraph(), - assertNotCalled, - assertNotCalled - ); + ]); expect(merged.equals(demoData.advancedMealGraph())).toBe(true); }); + it("has the empty graph as a right identity", () => { - const merged = Graph.merge( + const merged = Graph.mergeConservative([ demoData.advancedMealGraph(), new Graph(), - assertNotCalled, - assertNotCalled - ); + ]); expect(merged.equals(demoData.advancedMealGraph())).toBe(true); }); - it("trivially merges the empty graph with itself", () => { - const merged = Graph.merge( - new Graph(), - new Graph(), - assertNotCalled, - assertNotCalled - ); - expect(merged.equals(new Graph())).toBe(true); - }); }); describe("JSON functions", () => { diff --git a/src/plugins/git/createGraph.js b/src/plugins/git/createGraph.js index 757c6c2..e16a745 100644 --- a/src/plugins/git/createGraph.js +++ b/src/plugins/git/createGraph.js @@ -40,7 +40,7 @@ class GitGraphCreator { const treeAndNameToSubmoduleUrls = this.treeAndNameToSubmoduleUrls( repository ); - return Graph.mergeManyConservative([ + return Graph.mergeConservative([ ...Object.keys(repository.commits).map((hash) => this.commitGraph(repository.commits[hash]) ), diff --git a/src/tools/loadCombinedGraph.js b/src/tools/loadCombinedGraph.js index d7253ad..cde4f66 100644 --- a/src/tools/loadCombinedGraph.js +++ b/src/tools/loadCombinedGraph.js @@ -31,5 +31,5 @@ export function loadCombinedGraph( ): Promise { const githubGraphPromise = fetchGithubGraph(repoOwner, repoName, token); const gitGraph = cloneGitGraph(repoOwner, repoName); - return githubGraphPromise.then((x) => Graph.mergeConservative(gitGraph, x)); + return githubGraphPromise.then((x) => Graph.mergeConservative([gitGraph, x])); }