Change Comments

[ChrisPenner]

Overview

Implements a prototype of Change Comments, which will be Unison's simulacrum to Git's commit messages.
As part of this, also adds a config.toml file.

See this RFC for more in-depth discussion

Key differences from commit messages are:

Rather than baking commit messages directly into the commit, users can leave comments on changes, which are then synced between codebases.

Benefits:

• Rather than a single canonical commit message, multiple users can leave their own opinions on each commit.
• We can choose to allow scoping these comments to specific definitions, and allow things like “view all comments on this definition and their associated commits”; or “show me all commits which affect this definition and their comments”.
• We can roll up commit comments into Contribution Reviews, or the reverse (push contribution comments down into the commit comments when merged); thus keeping that valuable context present in the future.
• Comments are mutable, or at least we can allow adding new comment revisions without needing to rewrite code history and break collaborator branches.
• By separating messages from the causal itself we maintain sharing in storage between causals which are otherwise identical

Cons:

• Requires a new system or algorithm to sync comments

Implementation notes

This PR implements the UCM side of commit comments, I'll expand on this to add sync with Share as we go.

Notably, it only supports one active comment on each causal, re-annotating it will just edit that existing comment.
We don't support any form of syncing these across codebases or Share yet.

• Adds new change_comments and change_comment_revisions tables for storing causal comments
• Adds an annotate command which annotates a given causal, e.g. annotate /main annotate #abcdef etc.
• Adds a config file at $XDG_CONFIG/unisonlanguage/config.toml, which currently only includes author.name
• Adds config.edit which opens the editor on your config.
• Shows annotations in history:

Here's what it looks like if you have an error in your config:

scratch/main> annotate

  Error parsing configuration file at /Users/cpenner/.config/unisonlanguage/config.toml. Run `config.edit` to make changes.

    tomland errors number: 1
    tomland decode error:  Parse error during conversion from TOML to custom user type:
      2:1:
      |
    2 | <empty line>
      | ^
    unexpected end of input
    expecting '.' or '='

And if you don't have an author set:

scratch/main> annotate

  This workflow requires you to configure an author name.

  You can set your author name by running:
    `config.edit`

  then adding a line like:

  author-name = "Your Name"

Interesting/controversial decisions

• See the RFC for deeper discussion.

Test coverage

• See transcripts

Loose ends

• See RFC for next steps

[aryairani]

I'm still a little worried about adding and then outgrowing the config file and then needing to do a migration and uncertain cleanup. We already did that once and then eventually chose to absorb its contents into sqlite. What's the reasoning for going back to a file for this?

Otherwise seems great!

[ChrisPenner]

@aryairani
Gotcha; can you help me understand the viewpoint a bit? Can you expand on what you mean by "outgrowing" it? And what would be "uncertain" about the cleanup?

My understanding with the previous config was that it was configurator that we didn't particularly like, and also removed it because it was unused at the time: #5299

I think there's also a distinction to be made between codebase config and user config/preferences;
IMO things like formatting preferences, author name/email, UX/UI tweaks all seem like they should be global language settings rather than codebase specific, but since it's just the one setting for now I wouldn't choose to die on this hill.

[aryairani]

Yeah I definitely agree about codebase-specific stuff being separate from user config/preferences

• (and maybe also separate from some system-specific stuff although I can't think of any system-specific stuff)
• e.g. we support switching between codebases, but you're still the same person, with the same preferences regardless.

By "outgrowing" it, I meant I imagine wanting to implement user preferences that would be a headache to express and maintain as a .toml file; as opposed to in a user-preferences sqlite fully managed by ucm and/or ucm desktop.

By "cleanup" I meant if we upgrade to a database later; and by "uncertain", I meant we'd have to decide at runtime whether to delete the text file, or just leave it there, or ask the user.

For Configurator, I was thinking

• we didn't like the specific format
• we didn't like the mechanism for watching for external changes (alternative being to just restart ucm yourself to make changes take effect, but that's kind of a UX wart).
• we had let users specify remote branches for local namespaces to push/pull from, but now that's in sqlite. I forget what else was in there?

So I was wondering wdyt about a user preferences database? Does that become a pain for implementation reasons?

[ChrisPenner]

@aryairani Thanks that helps;

Yeah I think storing structured stuff is a good argument.
A global DB is an interesting idea, though I also think it seems a bit of extra work; e.g. managing two schemas, two sets of migrations etc.

Maybe adding it to the codebase is the best place to start and we can migrate it out later if we want?

[pchiusano]

My vote is also to not ever bring back random config files. They really bug me, honestly. And I would just use the existing database, no need to get fancy. I'd wager most users, myself included, only have one codebase that they use on their machine and anything else is a niche case for people who are doing development on Unison's implementation.

To the extent that people actually do have separate codebases, it's not clear that they will even want the same settings for both. (Imagine a "work" and a "personal" codebase, with different emails and/or author names) It's like, why introduce some inheritance system for a piece of information that people will maybe change... once or twice, ever? It just doesn't pay its way to try to introduce some reuse mechanism to avoid the user having to type config.author.set "Alice Smith" once if/when they ever create a new codebase.

[aryairani]

@ChrisPenner

Yeah I think storing structured stuff is a good argument. A global DB is an interesting idea, though I also think it seems a bit of extra work; e.g. managing two schemas, two sets of migrations etc.

Yeah, that makes sense. But I think we are still effectively managing two schemas and two sets of migrations even if we go ahead with a .toml/text file, we just don't normally think in those terms about it.

Maybe adding it to the codebase is the best place to start and we can migrate it out later if we want?

I like this (option three?) better from the cleanup angle, though we'd face some similar issues e.g. we migrate out from one codebase and then load another codebase that hasn't been migrated, we have to choose something. Or we just don't migrate anything, and they reconfigure their preferences from scratch, that's probably also okay for now.

A fourth option (related to the first): we go ahead with the .toml file but remain ready to switch to a user-level database and delete the file. I also kind of hate having the user have to manually edit this config file though. And I just saw the inevitable parse error message with fresh eyes. It's the best we can do if we've got a text file, but it's not great. What would it take to have ucm make the changes? Make it read-only, and flip the bits when needed.

A fifth option: read from ~/.gitconfig. We know they have a GitHub account, so they probably have a .gitconfig. This feels hard/weird/inappropriate to explain though (even though the concept is very simple), so I don't like that. It would be an ok place to grab an initial value from though.

I'm thinking:

• best is to introduce user/global preference database. We don't worry about migrations for now, just like with the .toml file.

  • bonus: It puts us on a low-friction track towards customizing folks' experiences for the better.

• second best: go ahead with .toml for now, but never ask the user to edit it manually, it remains a hidden implementation detail — we edit it with a ucm command just like we would if were part of a database, and we remain prepared to migrate it to rdbms before ever telling anyone it existed. In this case the parse error is fine as-is, because anyone who sees it is bad and should feel bad. Instead of "use edit.config" we tell them they know they've done a terrible thing. (joking; tell them whatever)

How much extra work do you think each of those options would be?

[aryairani]

@pchiusano wrote:

To the extent that people actually do have separate codebases, it's not clear that they will even want the same settings for both. (Imagine a "work" and a "personal" codebase, with different emails and/or author names) It's like, why introduce some inheritance system for a piece of information that people will maybe change... once or twice, ever? It just doesn't pay its way to try to introduce some reuse mechanism to avoid the user having to type config.author.set "Alice Smith" once if/when they ever create a new codebase.

Well, that's a good point re. having different publishing profiles for the same person. It doesn't really fit into any of the implementation proposals so far though, so *more thinking*.

They'll have (maybe?) a single key that corresponds to the specific machine, the same name in both cases, possibly a different email address; we weren't using email for identity though, but rather linking private keys to an account on Share (or Enterprise Share) that would be their identity/identities when publishing.

Let's still start with a user-level db or user-level text file (short term), assuming it's a manageable amount of work, and if we decide we need multiple identities or other codebase specific preferences, it'll be easy to override that in the codebase.