From ef02077989d8f5ee9cab9199aacb00da6d119be1 Mon Sep 17 00:00:00 2001 From: Mitchell Rosen Date: Wed, 4 Sep 2024 11:26:21 -0400 Subject: [PATCH 1/2] add failing transcript --- unison-src/transcripts/fix-5326.md | 125 +++++++++++ unison-src/transcripts/fix-5326.output.md | 259 ++++++++++++++++++++++ 2 files changed, 384 insertions(+) create mode 100644 unison-src/transcripts/fix-5326.md create mode 100644 unison-src/transcripts/fix-5326.output.md diff --git a/unison-src/transcripts/fix-5326.md b/unison-src/transcripts/fix-5326.md new file mode 100644 index 0000000000..d891726487 --- /dev/null +++ b/unison-src/transcripts/fix-5326.md @@ -0,0 +1,125 @@ +```ucm +scratch/main> builtins.merge lib.builtin +``` + +```unison +x = 1 +``` + +```ucm +scratch/main> update +scratch/main> branch foo +scratch/main> +``` + +``` +main, foo +| +A +``` + +```unison +x = 2 +``` + +```ucm +scratch/main> update +scratch/main> branch bar +scratch/main> +``` + +``` +main, bar +| +| foo +| | +B - A +``` + +```unison +x = 3 +``` + +```ucm +scratch/main> update +``` + +``` +main +| +| bar foo +| | | +C - B - A +``` + +```unison +x = 4 +``` + +```ucm +scratch/main> update +scratch/foo> +``` + +``` +main +| +| bar foo +| | | +D - C - B - A +``` + +```unison +y = 5 +``` + +```ucm +scratch/foo> update +``` + +``` +main +| +| bar +| | +D - C - B - A + / + E + | + foo +``` + +```ucm +scratch/main> merge /foo +``` + +``` +main +| +| bar +| | +F - D - C - B - A + \ / + ----------- E + | + foo +``` + +```ucm:error +scratch/main> merge /bar +``` + +This should be a fast-forward, but we get this shape instead (which fails due to conflicts), because we incorrectly +compute `LCA(main, bar)` as `A`, not `B`. + +``` +main +| +| ------------ bar +| / \| +G - F - D - C - B - A + \ / + ----------- E + | + foo +``` diff --git a/unison-src/transcripts/fix-5326.output.md b/unison-src/transcripts/fix-5326.output.md new file mode 100644 index 0000000000..284beeec76 --- /dev/null +++ b/unison-src/transcripts/fix-5326.output.md @@ -0,0 +1,259 @@ +``` ucm +scratch/main> builtins.merge lib.builtin + + Done. + +``` +``` unison +x = 1 +``` + +``` ucm + + Loading changes detected in scratch.u. + + I found and typechecked these definitions in scratch.u. If you + do an `add` or `update`, here's how your codebase would + change: + + ⍟ These new definitions are ok to `add`: + + x : Nat + +``` +``` ucm +scratch/main> update + + Okay, I'm searching the branch for code that needs to be + updated... + + Done. + +scratch/main> branch foo + + Done. I've created the foo branch based off of main. + + Tip: To merge your work back into the main branch, first + `switch /main` then `merge /foo`. + +``` +``` +main, foo +| +A +``` + +``` unison +x = 2 +``` + +``` ucm + + Loading changes detected in scratch.u. + + I found and typechecked these definitions in scratch.u. If you + do an `add` or `update`, here's how your codebase would + change: + + ⍟ These names already exist. You can `update` them to your + new definition: + + x : Nat + +``` +``` ucm +scratch/main> update + + Okay, I'm searching the branch for code that needs to be + updated... + + Done. + +scratch/main> branch bar + + Done. I've created the bar branch based off of main. + + Tip: To merge your work back into the main branch, first + `switch /main` then `merge /bar`. + +``` +``` +main, bar +| +| foo +| | +B - A +``` + +``` unison +x = 3 +``` + +``` ucm + + Loading changes detected in scratch.u. + + I found and typechecked these definitions in scratch.u. If you + do an `add` or `update`, here's how your codebase would + change: + + ⍟ These names already exist. You can `update` them to your + new definition: + + x : Nat + +``` +``` ucm +scratch/main> update + + Okay, I'm searching the branch for code that needs to be + updated... + + Done. + +``` +``` +main +| +| bar foo +| | | +C - B - A +``` + +``` unison +x = 4 +``` + +``` ucm + + Loading changes detected in scratch.u. + + I found and typechecked these definitions in scratch.u. If you + do an `add` or `update`, here's how your codebase would + change: + + ⍟ These names already exist. You can `update` them to your + new definition: + + x : Nat + +``` +``` ucm +scratch/main> update + + Okay, I'm searching the branch for code that needs to be + updated... + + Done. + +``` +``` +main +| +| bar foo +| | | +D - C - B - A +``` + +``` unison +y = 5 +``` + +``` ucm + + Loading changes detected in scratch.u. + + I found and typechecked these definitions in scratch.u. If you + do an `add` or `update`, here's how your codebase would + change: + + ⍟ These new definitions are ok to `add`: + + y : Nat + +``` +``` ucm +scratch/foo> update + + Okay, I'm searching the branch for code that needs to be + updated... + + Done. + +``` +``` +main +| +| bar +| | +D - C - B - A + / + E + | + foo +``` + +``` ucm +scratch/main> merge /foo + + I merged scratch/foo into scratch/main. + +``` +``` +main +| +| bar +| | +F - D - C - B - A + \ / + ----------- E + | + foo +``` + +``` ucm +scratch/main> merge /bar + + I couldn't automatically merge scratch/bar into scratch/main. + However, I've added the definitions that need attention to the + top of scratch.u. + + When you're done, you can run + + merge.commit + + to merge your changes back into main and delete the temporary + branch. Or, if you decide to cancel the merge instead, you can + run + + delete.branch /merge-bar-into-main + + to delete the temporary branch and switch back to main. + +``` +``` unison:added-by-ucm scratch.u +-- scratch/main +x : Nat +x = 4 + +-- scratch/bar +x : Nat +x = 2 + +``` + +This should be a fast-forward, but we get this shape instead (which fails due to conflicts), because we incorrectly +compute `LCA(main, bar)` as `A`, not `B`. + +``` +main +| +| ------------ bar +| / \| +G - F - D - C - B - A + \ / + ----------- E + | + foo +``` + From 728f1b94b4f2287aa7d3680ca8b6edec0006429d Mon Sep 17 00:00:00 2001 From: Mitchell Rosen Date: Wed, 4 Sep 2024 11:28:59 -0400 Subject: [PATCH 2/2] fix LCA query --- .../U/Codebase/Sqlite/Queries.hs | 65 +++++++++++-------- unison-src/transcripts/fix-5326.md | 6 +- unison-src/transcripts/fix-5326.output.md | 31 ++------- 3 files changed, 46 insertions(+), 56 deletions(-) diff --git a/codebase2/codebase-sqlite/U/Codebase/Sqlite/Queries.hs b/codebase2/codebase-sqlite/U/Codebase/Sqlite/Queries.hs index d2ded0758e..033efb8655 100644 --- a/codebase2/codebase-sqlite/U/Codebase/Sqlite/Queries.hs +++ b/codebase2/codebase-sqlite/U/Codebase/Sqlite/Queries.hs @@ -2863,32 +2863,45 @@ before x y = selectAncestorsOfY = ancestorSql y lca :: CausalHashId -> CausalHashId -> Transaction (Maybe CausalHashId) -lca x y = - queryStreamCol (ancestorSql x) \nextX -> - queryStreamCol (ancestorSql y) \nextY -> do - let getNext = (,) <$> nextX <*> nextY - loop2 seenX seenY = - getNext >>= \case - (Just px, Just py) -> - let seenX' = Set.insert px seenX - seenY' = Set.insert py seenY - in if Set.member px seenY' - then pure (Just px) - else - if Set.member py seenX' - then pure (Just py) - else loop2 seenX' seenY' - (Nothing, Nothing) -> pure Nothing - (Just px, Nothing) -> loop1 nextX seenY px - (Nothing, Just py) -> loop1 nextY seenX py - loop1 getNext matches v = - if Set.member v matches - then pure (Just v) - else - getNext >>= \case - Just v -> loop1 getNext matches v - Nothing -> pure Nothing - loop2 (Set.singleton x) (Set.singleton y) +lca alice bob = + queryMaybeCol + [sql| + WITH RECURSIVE history_one (causal_id) AS ( + SELECT :alice + UNION + SELECT causal_parent.parent_id + FROM history_one + JOIN causal_parent ON history_one.causal_id = causal_parent.causal_id + ), + history_two (causal_id) AS ( + SELECT :bob + UNION + SELECT causal_parent.parent_id + FROM history_two + JOIN causal_parent ON history_two.causal_id = causal_parent.causal_id + ), + common_ancestors (causal_id) AS ( + SELECT causal_id + FROM history_one + INTERSECT + SELECT causal_id + FROM history_two + ORDER BY causal_id DESC + ) + SELECT causal_id + FROM common_ancestors + WHERE NOT EXISTS ( + SELECT 1 + FROM causal_parent + WHERE causal_parent.parent_id = common_ancestors.causal_id + AND EXISTS ( + SELECT 1 + FROM common_ancestors c + WHERE c.causal_id = causal_parent.causal_id + ) + ) + LIMIT 1 + |] ancestorSql :: CausalHashId -> Sql ancestorSql h = diff --git a/unison-src/transcripts/fix-5326.md b/unison-src/transcripts/fix-5326.md index d891726487..e09ab53419 100644 --- a/unison-src/transcripts/fix-5326.md +++ b/unison-src/transcripts/fix-5326.md @@ -105,12 +105,12 @@ F - D - C - B - A foo ``` -```ucm:error +```ucm scratch/main> merge /bar ``` -This should be a fast-forward, but we get this shape instead (which fails due to conflicts), because we incorrectly -compute `LCA(main, bar)` as `A`, not `B`. +This should be a fast-forward, but we used to get this shape instead (which fails due to conflicts), because we +incorrectly computed `LCA(main, bar)` as `A`, not `B`. ``` main diff --git a/unison-src/transcripts/fix-5326.output.md b/unison-src/transcripts/fix-5326.output.md index 284beeec76..bdddcbb6f0 100644 --- a/unison-src/transcripts/fix-5326.output.md +++ b/unison-src/transcripts/fix-5326.output.md @@ -214,36 +214,13 @@ F - D - C - B - A ``` ucm scratch/main> merge /bar - I couldn't automatically merge scratch/bar into scratch/main. - However, I've added the definitions that need attention to the - top of scratch.u. + 😶 - When you're done, you can run - - merge.commit - - to merge your changes back into main and delete the temporary - branch. Or, if you decide to cancel the merge instead, you can - run - - delete.branch /merge-bar-into-main - - to delete the temporary branch and switch back to main. - -``` -``` unison:added-by-ucm scratch.u --- scratch/main -x : Nat -x = 4 - --- scratch/bar -x : Nat -x = 2 + scratch/main was already up-to-date with scratch/bar. ``` - -This should be a fast-forward, but we get this shape instead (which fails due to conflicts), because we incorrectly -compute `LCA(main, bar)` as `A`, not `B`. +This should be a fast-forward, but we used to get this shape instead (which fails due to conflicts), because we +incorrectly computed `LCA(main, bar)` as `A`, not `B`. ``` main