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

725.conflict resolution #752

Open
wants to merge 67 commits into
base: main
Choose a base branch
from

Conversation

meejah
Copy link
Collaborator

@meejah meejah commented Mar 27, 2024

Fixes #725

API, CLI and documentation for performing conflict resolution.

docs/conflicts.rst Outdated Show resolved Hide resolved
docs/conflicts.rst Outdated Show resolved Hide resolved
docs/conflicts.rst Outdated Show resolved Hide resolved
*Restore old version*: Similarly, it is possible to produce a new Snapshot that effectively restores an older version of the same file.
We do not know of any UI that can do this.

*Completely new content*: As hinted above, it might be nice to be able to produce a resolution that is some combination of multiple versions (like one sometimes does with Git conflicts, for example).
Copy link
Member

Choose a reason for hiding this comment

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

In this context, I wonder also about pruning old snapshots (e.g., to save costs on metered/ZKAPs-enabled grids) and/or requesting deletion (e.g., in cases where a file might have been published unintentionally).

(These are somewhat of beyond just conflict detection though...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no deletion API in Tahoe-LAFS.

The closest there is (which would work for "pruning" too) is to simply stop refreshing leases on things you no longer care about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I think this is more directly related to "history" / "how the datamodel works" than anything about conflicts per se though)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is more directly related to "history" / "how the datamodel works" than anything about conflicts per se though

Yes, agreed (and hence my note/admission above that this is "beyond just conflict detection" -- so no need to really do anything here/now); this was mostly just an observation about something that I -- as a hypothetical/imaginary end user of this software -- might wonder about when encountering such a list of missing features. To that end, a note that there is no way to prune/delete snapshots might be nice here (depending on what level of knowledge the reader is assumed to have) but I also grant that a) reliable "deletion" isn't (ever?) possible, b) if there ever were deletion-like functionality, it would probably be a Tahoe-LAFS thing -- not a Magic-Folder thing, and c) this is more of a general concern (that isn't specific to conflicts). So feel free to ignore/disregard/resolve. :)

@meejah meejah force-pushed the 725.conflict-resolution--rebase branch from 1ba7ff6 to be5b04f Compare June 12, 2024 22:38
@meejah
Copy link
Collaborator Author

meejah commented Jun 13, 2024

@crwood can you write your desired "event" behavior as a ticket, please?

Other than that I think this is mergeable?

@crwood
Copy link
Member

crwood commented Jun 14, 2024

@crwood can you write your desired "event" behavior as a ticket, please?

Other than that I think this is mergeable?

Sorry for the late-ish reply here -- but yes, I've set aside some time today to finish my testing against this branch (and will file a ticket for the "event" suggestion alongside it).

Copy link
Member

@crwood crwood left a comment

Choose a reason for hiding this comment

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

LGTM.

I left a few additional inline comments below -- mostly minor or speculative. Feel free to address as you see fit (or not!) and merge.

src/magic_folder/magic_file.py Outdated Show resolved Hide resolved
@@ -194,6 +214,9 @@ class MagicFile(object):
_queue_remote = attr.ib(default=attr.Factory(list))
_is_working = attr.ib(default=None)

# XXX
Copy link
Member

Choose a reason for hiding this comment

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

What is this "XXX" supposed to signify? Should this be expanded into a comment? If not, remove.

#
# ...so I think we _do_ want to queue a local-update: an incoming
# resolution may happen, and if we have no local change: great,
# apply it. But if we _do_ have a local change, we'd want to
Copy link
Member

Choose a reason for hiding this comment

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

This sounds about right to me.

In such instances, though, I wonder if would it be worth perhaps uniquely identifying each individual "instance" of conflict -- perhaps associating each instance with a UUID or something -- so that we know or can track from which instance of conflict was ultimately resolved (and "conflict again" only if the other instance(s) were resolved)?

I was thinking about this originally in the context of #755, and wondering if, for example, it might make sense to have a "conflict-updated" event (in order to signal, say, that another change has happened on top of an already-conflicted file that did not resolve the conflict, or maybe to signal that a two-participant conflict has become a three-participant one -- e.g., going from "participants": ["Alice", "Bob"] to "participants": ["Alice", "Bob", "Charlie"]) versus, instead, just emitting another "conflict-detected" message (and considering all conflicts associated with a given relpath as being "resolved" if/when one of them is).

(I don't have particularly strong opinions here as I haven't thought very far past the simpler "two participants, one conflict" case...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we do already track multiple "instances" in the sense that we track which other participants are currently conflicted (which can be 0..n)

the only case i can think of might be where e.g. "alice" is conflicted, and then updates her copy further (so on "our" side we'd still be conflicted).

Is there anything that should behave differently in this case? (Clients could I think already figure out where the conflict "first" happened if they wanted, by examining the version tree).

Copy link
Member

Choose a reason for hiding this comment

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

the only case i can think of might be where e.g. "alice" is conflicted, and then updates her copy further (so on "our" side we'd still be conflicted).

It also still seems possible (I believe?) to have participants resolve a conflict in diverging ways that would require additional intervention. For example, Alice might accept/prefer Bob's copy, while Charlie ("at the same time") accepts/prefers Alice's original copy (i.e., prior to her accepting of Bob's).

I suppose the questions here are more UX-oriented ones -- about whether and how users need to be exposed (and/or make decisions about) each "intermediate" instance in a multi-conflict scenario. Presumably, whichever snapshots are "the latest" probably matter most here (especially since past snapshots are preserved in the event they are needed).

Anyway, I don't think there's anything that needs to be done about this right now; sorry for the digression!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, two resolutions "at the same time" are possible -- they should immediately produce another conflict (probably need lots more tests like this 🤔 ). However, this isn't logically "another instance" of an existing conflict, but a new conflict (between the Snapshot that Alice produced and the Snapshot that Charlie produced as resolutions).

src/magic_folder/magic_file.py Show resolved Hide resolved
src/magic_folder/test/test_magic_file.py Show resolved Hide resolved
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.

Conflict resolution
2 participants