-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: main
Are you sure you want to change the base?
Conversation
*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). |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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. :)
1ba7ff6
to
be5b04f
Compare
@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). |
There was a problem hiding this 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
@@ -194,6 +214,9 @@ class MagicFile(object): | |||
_queue_remote = attr.ib(default=attr.Factory(list)) | |||
_is_working = attr.ib(default=None) | |||
|
|||
# XXX |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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).
Fixes #725
API, CLI and documentation for performing conflict resolution.