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

Add new 'Sync Method' setting in place of mergeOnPull and new experimental obsidian-sync compatible sync method. #165

Merged
merged 4 commits into from Feb 2, 2022

Conversation

coddingtonbear
Copy link
Contributor

@coddingtonbear coddingtonbear commented Jan 17, 2022

I use both obsidian-sync and obsidian-git so I can have both live updates & mobile support (via obsidian-sync) while also having change-by-change history (via obsidian-git). While far from home over the holidays with my work laptop, I encountered more than a few merge conflicts due to a race condition in which my laptop at home dutifully received changes via obsidian-sync, committed them, and then while it attempted to push those changes, it discovered that my work laptop had already committed changes and was thus in a conflict.

What this does is add a third method for handling sync (in addition to the existing rebase and merge methods) that just lazily updates the current HEAD to match the sha of any fetched remote data without updating the local filesystem. That might seem surprising at first glance, but while using obsidian-sync, this works perfectly well due to obsidian-sync itself updating your local filesystem automatically as changes are discovered -- almost universally long before obsidian-git has even had a chance to discover that those changes exist on the remote. Additionally, if we keep obsidian-git out of that process, it can perform its normal commit/push processes for keeping an accurate historical record of changes to your notes without the possibility of getting trapped in a conflict.

This also refactors how we indicate to obsidian-git which sync method to use. Before this change, there was a setting named mergeOnPull that is either true (indicating that we should merge fetched changes) or false (indicating that we should rebase atop fetched changes). After this change, there is now a setting named syncMethod that can be set to one of three values: merge, rebase, or reset (this is the new obsidian-sync compatible option). Settings will be automatically migrated forward at plugin load-time.

I've been using this successfully for a week or so now, but it's important for me to say that although I've tested this pretty thoroughly with the new reset method and spot-checked merge and rebase, it would be best if somebody who uses the more-normal sync methods gave this a shot, too, before merging.

- Also adds support for new 'reset' method for compatibility with
obsidian sync.
- Obsoletes `mergeOnPull` setting to instead use `syncMethod = 'merge'`.

}
const pullResult = await this.git.pull([this.plugin.settings.mergeOnPull ? '--no-rebase' : '--rebase'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we were using git pull to fetch and merge changes. git pull is a shorthand for git fetch followed by either git rebase FETCH_HEAD or git merge FETCH_HEAD (https://git-scm.com/docs/git-pull/2.1.4#_description).

In the new changes on the right, you'll see that we're intentionally git fetch-ing separately from git merge or git rebase, but otherwise these are largely functionally the same.

Copy link
Collaborator

@Vinzent03 Vinzent03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late review. I like the idea to solve this race condition. I noticed that files edited on another device are always staged. Did you experience this too? What do you think about my changes?

src/main.ts Show resolved Hide resolved
src/settings.ts Outdated
if (plugin.settings.syncMethod) {
dropdown.setValue(plugin.settings.syncMethod)
} else {
dropdown.setValue('merge')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the default value of syncMethod it should always be non undefined.

src/simpleGit.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
@Vinzent03
Copy link
Collaborator

LGTM now. Thanks for the (I think) great idea of solving race conditions with other sync services.

@Vinzent03 Vinzent03 merged commit 0260f54 into denolehov:master Feb 2, 2022
@coddingtonbear
Copy link
Contributor Author

Awesome -- thanks for merging. Just btw, I'd been meaning to look into your comment "I noticed that files edited on another device are always staged", but haven't yet found the time and intend to have a closer look sometime over the next few days.

@coddingtonbear
Copy link
Contributor Author

Oh, nevermind, I see that you fixed that in f60f95c; thanks!

@Vinzent03
Copy link
Collaborator

Yeah I'm still unsure, but I think just unstaging all is fine.

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

2 participants