Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Clone to SharedTree Revertible #23044
base: main
Are you sure you want to change the base?
Add Clone to SharedTree Revertible #23044
Changes from 10 commits
408ff9e
bdfbd3a
7f0777e
94b8b5a
73dfa36
1364971
f0a7c6e
0d593b4
09f10e5
896f22f
b967512
33e1fde
0f62865
698e1c1
5caf3c6
1784895
0d2bef6
022404a
2ab318c
aaff66b
115354b
f0c989c
dea35f7
7596fa7
0ec8140
e518bb7
13be6d4
b3ac268
855fe89
cec77a5
c4ed841
85319e7
80946ab
7d87eb7
bd83bd2
684943e
9969510
41acc0d
b7b2b2f
fa5e774
ec15843
233749d
e50bc39
fec248e
5603232
eddc5a3
04fdc50
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 was the reasoning behind allowing forkedBranch to be optional? why not just make it required and not allow clone to be called without a fork? the behavior for cloning a revertible onto the same checkout doesn't seem obvious to me and i'm not sure why we'd want to do so
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.
Cloning a revertible onto the same branch is useful for an application that wants to provide ownership of a revertible to multiple components. Suppose I have two components in my application that:
You'd want to clone the revertible, giving one to the first component and the clone to the second. Or, perhaps the parent component retains ownership of the original revertible and gives a new clone to each subcomponent. If you can't clone the revertible for the same branch, then you have to share the same revertible between the parent and both components, and now they do need to know about each other so that they know when to (or when not to) dispose the revertible.
Regarding the parameter being optional, it's just a convenience. For an application like the above, which operates solely on the main branch, it would be annoying at best and confusing at worst to be forced to pass in the main branch every time it wants to do a clone. With it being optional, the user doesn't need to understand the branching feature at all in order to clone revertibles.
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 agree that it would be useful, but I'm not entirely convinced that the changes here sufficiently support it. At the very least, I think it needs to be documented better.
some thoughts:
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.
For a set of clones of the same revertible associated with the same branch, the repair data will not be collected until all clones have called
dispose()
. This is the intended behavior, and I believe is the currently behavior, but if it's not covered by tests then we should add more tests.So, yes, if component A and component B each have a clone of a revertible, and are on the same branch, then when A does a revert it will do something, and when B does a revert it will (probably) no-op. This is what I would expect - the reason you cloned in the first place is so that A and B don't have to know what each other are doing, therefore, if B does an undo but it was already undone by A, nothing will happen (because it "already happened"). The other option which you alluded to in your first bullet is for B's undo stack to be changed along with A's undo stack. But if that's what the user wants, then A and B are not isolated and the user shouldn't have done a clone in the first place. Undo-stacks are probably 1:1 with branches most of the time, is my guess. Being able to clone a revertible on the same branch is more of a ref-counting thing. Like you have some code in your application that wants to ensure it can revert some specific data at some point in the future, so it holds on to a particular revertible - regardless of whether or not a clone of that revertible might get reverted anyway by some other means. I think it's likely to be a pretty uncommon scenario, but it might be useful to have as an option, it's easy for us to implement, and it's the natural behavior (IMO) of a parameterless call to
clone
.A review of potential scenarios (e.g. with Nick) to inform the documentation would probably not be a bad idea. If we find that it's just too hard to explain why you would do this, then that could be a sign that we should just leave it out.