Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove NoSync versions of root branch primitives #5057

Draft
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

ChrisPenner
Copy link
Contributor

Overview

There are several stepAtNoSync variations exposed, these seem to be intended to be used to:

  • Batch up several unrelated changes into a single step in history
  • Allow writing code which uses Cli getCurrentBranch combinators against a branch which hasn't been "committed" yet.

Which seems reasonable, but in reality it's resulted in places in code where the in-memory branch has been altered, but it hasn't been saved to SQLite yet, which is pretty dangerous because:

  • If you close UCM you THINK you've saved some work, but it'll disappear
  • UCM's checks all operate on the in-memory branch, so you may be able to corrupt your codebase, e.g. save things you don't have names for, delete things that are still depended on, etc.

This change removes these combinators.

We still have an atomicity problem in syncing UCM and SQLite, but moving the sqlite and memory updates right next to each other vastly decreases the likelihood something will go wrong here.

The motivation for this is that we'll be performing edits to branches in multiple roots soon, so relying on a single in-memory root is no longer feasible, so NoSync doesn't make sense.

Implementation notes

  • Replaces uses of NoSync combinators with synchronous versions. In update.old behaviour is changed slightly so patch propagation and update are now in different causals, but since that command is deprecated I figure it's fine. If we'd like to maintain the same old behaviour I can do the extra work for it.
  • Removes the NoSync combinators so we won't be tempted to cause this problem for ourselves again. If you ever need to do this, you can load the branch into memory and pass it around as a variable explicitly.

Test coverage

Existing transcripts should cover it.

Loose ends

We should investigate better approaches to guaranteeing atomicity between SQLite and UCM, e.g. masking ctrl-c, checking that they're synced at each prompt render, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant