3-way diff

[mitchellwrosen]

Overview

The PR amends the namespace diff infrastructure to use the 3-way diff algorithm, and adds a new "propagated" diff entry type for propagated updates. It also adds a libdeps diff to the payload.

Test coverage

I've rerun the existing transcripts, which should cover all of the changes.

[ChrisPenner]

Not sure where this top-level module came from but I'm guessing it wasn't intentional given that it's a copy of the actual module in the right place?

[mitchellwrosen]

Huh, this must have come from a git merge due to bad rename detection. Like, the file moved and also changed substantially so git didn't register it as a rename. I'll delete this.

[mitchellwrosen]

Wait, no, I probably accidentally created this without realizing.

[ChrisPenner]

Probably worth testing one of the common queries on prod with and without suffixification to see if the real-world cost changes :)

[mitchellwrosen]

Yeah, how might we go about doing this? (Btw, this was one of the functions you authored before I took over)

[ChrisPenner]

It's a little leg-work to do it, sorry >_>

The queries it's running are here: https://github.com/unisoncomputing/share-api/blob/main/src/Share/Postgres/NameLookups/Queries.hs#L44-L122

You'll need to copy-paste that, then manually fill in the parameters into the query which is a little annoying tbh; you can probably query a release or something from the debug_project_releases view, then cross reference that with scoped_term_name_lookup to get the params you need;

From there you can EXPLAIN ... and/or EXPLAIN ANALYSE on the queries with and without the suffixification.

If you need DB credentials you can get a read-only login for psql via vault read secret/production/enlil/userdb-readonly; For some reason our main DB is still called users 🤷🏼

Or my preferred method:

pgprod_ro () {
        pg_secret=$(vault read /secret/production/enlil/userdb-readonly -format=json | jq .data)
        PGPASSWORD="$(echo "$pg_secret" | jq -r .password)" psql --username "$(echo "$pg_secret"  | jq -r .username)" --host "$(echo "$pg_secret"  | jq -r .hostname)" --port "$(echo "$pg_secret"  | jq -r .port)" -d users "$@"
}

[ChrisPenner]

If you need a hand knowing where to poke around don't hesitate to ask, it's probably good to get more folks used to digging around :)

[ChrisPenner]

Not positive, and not really a big deal, but you may be able to downgrade the Monad constraint to Applicative if you use Compose rather than MaybeT here?

[ChrisPenner]

Would probably be worth defining an ADT for this :)

[mitchellwrosen]

I started on this, but the Nothing is useful because we do a lot of MaybeT and whenNothingM with the result. So that leaves "left means constructor, right means term" for putting into an ADT – I think we could probably use this as a generic thing in the Unison codebase, like an even more generic version of Referent':

data ConstructorOrTerm a = Constructor a | Term b

But I'm not sure what a good name might be. We could leave this alone for now and refactor into a better type later?

[ChrisPenner]

Realistically never never comes :P But I'm fine leaving it as-is if it doesn't feel right 👍

[ChrisPenner]

Are the changes here (and in the other transcript changes) expected? Looks like a conflict was introduced?

[ChrisPenner]

Looks great, good job on all of this!

I think I'd have expected to see some new transcripts, do the existing ones cover all the new cases w/r to propagated vs manual changes?

Regardless, if the transcript outputs look good we can let @hojberg know about the changes and once he's got a UI ready we can try it out locally and on staging.

Consider it approved ✅, but we should wait till the frontend is ready to handle this version before merging since otherwise it'll block Share deploys until that's done

Also, I see changes in the unison submodule, is there a related PR there that needs to be merged first?