Re: RFC: How should revert handle copied/added items?
Greg Stein writes: > On Tue, Aug 10, 2010 at 14:41, Paul Burba wrote: >> On Tue, Aug 10, 2010 at 12:18 AM, Greg Stein wrote: >>... >>> For example, >>> >>> $ svn cp A/B C >>> $ svn revert C/D/file >>> >>> That should error. >>> >>> $ svn revert C >>> >>> Should succeed, and undo the copy that was made. >> >> Since we can't revert the root of an added subtree by itself, I assume >> that if the root of your revert target is a copy, that '-R' is >> implied? > > I generally don't think at the cmdline level :-P > > Reverting the root of a copy "should" imply -R, I think. But we also > defined revert as non-recursive by default (iirc). There is also great > potential for further edits under there (structural and content) which > the user might have forgotten. An implied -R could blast (say) some > local-deletes, adds, and content changes. When the user puts a -R on > the cmdline, then we assume they have checked the subtree and are > willing to revert it all. > > Having the cmdline tool say "To revert C (a copy of repos/p...@569), > please specify -R." The 1.6 client used to do that in some circumstances: $ svn mkdir wc/A wc/A/B $ svn revert wc/A svn: Try 'svn revert --depth infinity' instead? svn: Unable to lock 'wc/A/B' In 1.7 the behaviour has changed, probably inadvertently. The locks and the revert are now recurisive. Another behaviour change. If wc/A/B is in the repository: $ svn cp wc/A wc/C $ svn revert wc/C/B $ In 1.6 the revert does nothing, and produces no message. In 1.7 we get: $ svn revert wc/X/B Reverted 'wc/X/B' $ svn st wc A +wc/X ? wc/X/B which is an invalid wc state. -- Philip
Re: RFC: How should revert handle copied/added items?
On Tue, Aug 10, 2010 at 14:41, Paul Burba wrote: > On Tue, Aug 10, 2010 at 12:18 AM, Greg Stein wrote: >... >> For example, >> >> $ svn cp A/B C >> $ svn revert C/D/file >> >> That should error. >> >> $ svn revert C >> >> Should succeed, and undo the copy that was made. > > Since we can't revert the root of an added subtree by itself, I assume > that if the root of your revert target is a copy, that '-R' is > implied? I generally don't think at the cmdline level :-P Reverting the root of a copy "should" imply -R, I think. But we also defined revert as non-recursive by default (iirc). There is also great potential for further edits under there (structural and content) which the user might have forgotten. An implied -R could blast (say) some local-deletes, adds, and content changes. When the user puts a -R on the cmdline, then we assume they have checked the subtree and are willing to revert it all. Having the cmdline tool say "To revert C (a copy of repos/p...@569), please specify -R." It could even warn about edits to children. Dunno. >> $ edit C/D/file >> $ svn revert C/D/file >> >> Should succeed; reverting just local post-copy edits. > > What about this: > > $ svn revert -R C/D > > Should that revert C/D/file's post-copy edit or produce the same error > as the attempt to revert C/D/file above? You could go two ways on this, I think: 1) revert C/D/file, then error on C/D 2) error without doing anything > If the former then we have a situation where two consecutive reverts > are needed to fully revert (i.e. one to revert the edit, one to revert > the copy). Not that this is inherently bad, just a bit unusual. But > if we implement --discard-local-edits there is always the option to do > it in one fell swoop. I favor this option, assuming we implement > --discard-local-edits or something similar. > > If the latter, then it gets a bit cumbersome to revert multiple text > edits but keep the copy; you'd need to explicitly specify each path to > be reverted. Right. And --discard-local-edits was proffered as an example of content edits. Don't forget this scenario: $ svn cp A/B C $ svn rm C/D/file $ svn revert -R C This is just a different "layer" of operation over the copy. Another structural edit, instead of a content edit. I think it should succeed, and remove content from the disk. There is no potential data loss, since this was just a copy. If there was a local-add under there, then it gets more troublesome. Maybe remove all copied files, but leave behind the locally-added file and enough parent directories? >> There are UI issues with allowing the second to succeed if local mods >> are present, yet fail if not. I relegate this to "UI foo". >> Conceptually, the second revert should be able to succeed. >> >> Here is the more interesting scenario: >> >> $ svn cp file1 file2 >> $ edit file2 >> $ svn revert file2 > > If we implement the earlier behavior where 'svn cp A/B C, edit > C/D/file, svn revert C/D/file' reverts just local post-copy edits to > C/D/file, I think that is what we should do here too. *shrug* ... seems reasonable. >> This is where the two semantics come in. Do you want to revert the >> content edits? Or do you want to revert the copy? Questions and UI and >> API abound :-) >> >>> A) "Yes! svn revert only reverts the scheduled addition, but leaves >>> the item behind as unversioned". In this case the one exception [1] >>> becomes a bug which we can fix. >> >> For a schedule-add... yes, this is safest. This content may not exist >> anywhere else, so it should remain. >> >> If you're using the term "addition" to also mean "add with history", >> then I'll smack you and say you should use the term "copy". A revert >> of a copy has restrictions as noted above, and if it also has local >> changes, then leaving it behind may be important. > > Swing away, I was using "addition" to cover both copies and > schedule-adds. But I did so knowingly, because today svn revert > treats both the same way: The scheduled add or add with history is > reverted and the unversioned item is left on disk. The only exception Yeah. Well, we're trying to use better definitions because it is this exact "imprecision" which has caused us so many problems. Adds and copies are very different, and it helps us to reason much better about revert's behavior. >... >> A directory-copy with post-copy propchanges cannot be left behind. It >> becomes unversioned, meaning we have nowhere to leave the propchanges >> behind. It may be that users first revert the propchanges, *then* they >> revert the copy. Dunno. > > Again, I think the two revert approach is fine. When you make a copy > then edit that copy before it is committed I think of this as > "layering" changes atop one another. Yup. And further structural changes (add/delete, another copy, etc) are other types of layers. And that is also how we are going to store these things in wc.db: multiple layers of operations with one "current" view of the tree.
Re: RFC: How should revert handle copied/added items?
On Tue, Aug 10, 2010 at 12:18 AM, Greg Stein wrote: Greg and Peter, Thanks for your views on this topic. A few comments/questions below. > On Mon, Aug 9, 2010 at 12:54, Paul Burba wrote: >> We have several issues related to the question of how revert should >> handle locally added or copied items: >> >> 'svn revert of svn move' >> http://subversion.tigris.org/issues/show_bug.cgi?id=876 >> >> 'svn revert should provide a way to delete copied files' >> http://subversion.tigris.org/issues/show_bug.cgi?id=3101 >> >> 'Case only renames resulting from merges don't work or break the WC on >> case-insensitive file systems' >> http://subversion.tigris.org/issues/show_bug.cgi?id=3115 >> >> With only one exception that I know of[1], revert leaves local >> additions of every stripe as unversioned. My first question is >> simple: >> >> "Do we consider this the correct behavior?" > > I haven't read the above bugs, but have come to a "solid" view of how > 'svn revert' should behave. When stopping to think about the > *representation* of changes within the working copy, it also (imo) > leads to a better understanding of the operations performed and what a > "revert" of those operations can mean. > > I'll try to provide a brief description of my thoughts, then answer > your questions based on that, and then summarize any gaps. Please ask > for more detail, or to argue interpretation! > > There are *two* semantic concepts behind revert. The first is "revert > my structural changes" (e.g add/move/copy/delete). The second is > "revert my content changes" (e.g. text edits and prop changes). These > two concepts are conflated today; we make no distinction between them, > and provide no UI or API mechanism to distinguish *what* you would > like to revert. > > Content reverts are simple and straight-forward, and can be applied to > any node [which has content changes]. > > Structural changes *cannot* be applied to "any" node. If you copy a > (sub)tree to a new location, then you cannot revert a *child* of that > copy. Without stretching the brain along some inhuman path, it just > has no meaning. The child arrived as a result of a parent copy. You > can *delete* or *replace* the child using further operations, but an > implied-copy of a child can only be reverted by reverting the ancestor > which is the root of the copied-here operation. > > For example, > > $ svn cp A/B C > $ svn revert C/D/file > > That should error. > > $ svn revert C > > Should succeed, and undo the copy that was made. Since we can't revert the root of an added subtree by itself, I assume that if the root of your revert target is a copy, that '-R' is implied? > $ edit C/D/file > $ svn revert C/D/file > > Should succeed; reverting just local post-copy edits. What about this: $ svn revert -R C/D Should that revert C/D/file's post-copy edit or produce the same error as the attempt to revert C/D/file above? If the former then we have a situation where two consecutive reverts are needed to fully revert (i.e. one to revert the edit, one to revert the copy). Not that this is inherently bad, just a bit unusual. But if we implement --discard-local-edits there is always the option to do it in one fell swoop. I favor this option, assuming we implement --discard-local-edits or something similar. If the latter, then it gets a bit cumbersome to revert multiple text edits but keep the copy; you'd need to explicitly specify each path to be reverted. > There are UI issues with allowing the second to succeed if local mods > are present, yet fail if not. I relegate this to "UI foo". > Conceptually, the second revert should be able to succeed. > > Here is the more interesting scenario: > > $ svn cp file1 file2 > $ edit file2 > $ svn revert file2 If we implement the earlier behavior where 'svn cp A/B C, edit C/D/file, svn revert C/D/file' reverts just local post-copy edits to C/D/file, I think that is what we should do here too. > This is where the two semantics come in. Do you want to revert the > content edits? Or do you want to revert the copy? Questions and UI and > API abound :-) > >> A) "Yes! svn revert only reverts the scheduled addition, but leaves >> the item behind as unversioned". In this case the one exception [1] >> becomes a bug which we can fix. > > For a schedule-add... yes, this is safest. This content may not exist > anywhere else, so it should remain. > > If you're using the term "addition" to also mean "add with history", > then I'll smack you and say you should use the term "copy". A revert > of a copy has restrictions as noted above, and if it also has local > changes, then leaving it behind may be important. Swing away, I was using "addition" to cover both copies and schedule-adds. But I did so knowingly, because today svn revert treats both the same way: The scheduled add or add with history is reverted and the unversioned item is left on disk. The only exception I know of to this is what prompted me to write this thread in the firs
Re: RFC: How should revert handle copied/added items?
On Mon, Aug 9, 2010 at 12:54, Paul Burba wrote: > We have several issues related to the question of how revert should > handle locally added or copied items: > > 'svn revert of svn move' > http://subversion.tigris.org/issues/show_bug.cgi?id=876 > > 'svn revert should provide a way to delete copied files' > http://subversion.tigris.org/issues/show_bug.cgi?id=3101 > > 'Case only renames resulting from merges don't work or break the WC on > case-insensitive file systems' > http://subversion.tigris.org/issues/show_bug.cgi?id=3115 > > With only one exception that I know of[1], revert leaves local > additions of every stripe as unversioned. My first question is > simple: > > "Do we consider this the correct behavior?" I haven't read the above bugs, but have come to a "solid" view of how 'svn revert' should behave. When stopping to think about the *representation* of changes within the working copy, it also (imo) leads to a better understanding of the operations performed and what a "revert" of those operations can mean. I'll try to provide a brief description of my thoughts, then answer your questions based on that, and then summarize any gaps. Please ask for more detail, or to argue interpretation! There are *two* semantic concepts behind revert. The first is "revert my structural changes" (e.g add/move/copy/delete). The second is "revert my content changes" (e.g. text edits and prop changes). These two concepts are conflated today; we make no distinction between them, and provide no UI or API mechanism to distinguish *what* you would like to revert. Content reverts are simple and straight-forward, and can be applied to any node [which has content changes]. Structural changes *cannot* be applied to "any" node. If you copy a (sub)tree to a new location, then you cannot revert a *child* of that copy. Without stretching the brain along some inhuman path, it just has no meaning. The child arrived as a result of a parent copy. You can *delete* or *replace* the child using further operations, but an implied-copy of a child can only be reverted by reverting the ancestor which is the root of the copied-here operation. For example, $ svn cp A/B C $ svn revert C/D/file That should error. $ svn revert C Should succeed, and undo the copy that was made. $ edit C/D/file $ svn revert C/D/file Should succeed; reverting just local post-copy edits. There are UI issues with allowing the second to succeed if local mods are present, yet fail if not. I relegate this to "UI foo". Conceptually, the second revert should be able to succeed. Here is the more interesting scenario: $ svn cp file1 file2 $ edit file2 $ svn revert file2 This is where the two semantics come in. Do you want to revert the content edits? Or do you want to revert the copy? Questions and UI and API abound :-) > A) "Yes! svn revert only reverts the scheduled addition, but leaves > the item behind as unversioned". In this case the one exception [1] > becomes a bug which we can fix. For a schedule-add... yes, this is safest. This content may not exist anywhere else, so it should remain. If you're using the term "addition" to also mean "add with history", then I'll smack you and say you should use the term "copy". A revert of a copy has restrictions as noted above, and if it also has local changes, then leaving it behind may be important. A directory-copy with post-copy propchanges cannot be left behind. It becomes unversioned, meaning we have nowhere to leave the propchanges behind. It may be that users first revert the propchanges, *then* they revert the copy. Dunno. They said "revert", so "losing" information isn't that big of a deal. They told us to. > B) "No! svn revert should remove all additions from disk. I asked for > it after all!". In this case that one exception [1] is the correct > behavior and we need fix everything else. Disagree. These additions have no other source, so they shouldn't disappear. The user said to remove the *scheduled-add*. Not the file. "svn rm" could remove the file. But a simple revert should not remove anything from the disk. It should only remove what our metadata said to do with the file/dir. > C) Then of course, there is the more complex answer where we allow > revert to summarily delete additions in some cases, but not in others. > Perhaps local additions are left as unversioned, but copies are > deleted. Or perhaps in the latter case we only delete the addition if > the copy is identical to its source. No idea what you're saying. If you'are advocating some kind of non-deterministic behavior of leaving some stuff and rm'ing others... that's definitely crazy. > I'm wondering if there is any consensus on the 'A', ''B, or 'C' > approaches. If 'A' or 'B' then our task is straightforward. If it's > 'C', then we'll need some more discussion. > > I favor 'A'. I know that leaving behind unversioned items post-revert > can be a bit of a pain in some use cases (e.g. "Oh I merged the wrong > revs, let's r
Re: RFC: How should revert handle copied/added items?
[Paul Burba] > With only one exception that I know of[1], revert leaves local > additions of every stripe as unversioned. My first question is > simple: > > "Do we consider this the correct behavior?" > > A) "Yes! svn revert only reverts the scheduled addition, but leaves > the item behind as unversioned". In this case the one exception [1] > becomes a bug which we can fix. > > B) "No! svn revert should remove all additions from disk. I asked for > it after all!". In this case that one exception [1] is the correct > behavior and we need fix everything else. > > C) Then of course, there is the more complex answer where we allow > revert to summarily delete additions in some cases, but not in others. > Perhaps local additions are left as unversioned, but copies are > deleted. Or perhaps in the latter case we only delete the addition if > the copy is identical to its source. My instinct is C). Revert should return the wc to how it was before. If a file's presence and contents are due only to operations performed by svn (add-with-history (cp, mv, merge)), delete it. If the file was created separately from svn, keep it as an unversioned file. 'svn mkdir' is a corner case, since it is really just like 'mkdir' followed by 'svn add', and it isn't reasonable to have to keep track of which method was used. I don't have a strong opinion whether 'revert' should remove the directory. It hardly matters, really, because deleting an empty directory doesn't run the risk of losing very much of a user's hard work. > I favor 'A'. I know that leaving behind unversioned items > post-revert can be a bit of a pain in some use cases (e.g. "Oh I > merged the wrong revs, let's revert and redo the correct merge, hey, > what are all these skips!?!") but I think this approach is easy to > understand and explain to users. The workarounds if someone has a > problem with this behavior are also relatively simple. I'm a pretty heavy user of 'svn-clean' from contrib, so yeah, this doesn't bother me very much. Peter
RFC: How should revert handle copied/added items?
We have several issues related to the question of how revert should handle locally added or copied items: 'svn revert of svn move' http://subversion.tigris.org/issues/show_bug.cgi?id=876 'svn revert should provide a way to delete copied files' http://subversion.tigris.org/issues/show_bug.cgi?id=3101 'Case only renames resulting from merges don't work or break the WC on case-insensitive file systems' http://subversion.tigris.org/issues/show_bug.cgi?id=3115 With only one exception that I know of[1], revert leaves local additions of every stripe as unversioned. My first question is simple: "Do we consider this the correct behavior?" A) "Yes! svn revert only reverts the scheduled addition, but leaves the item behind as unversioned". In this case the one exception [1] becomes a bug which we can fix. B) "No! svn revert should remove all additions from disk. I asked for it after all!". In this case that one exception [1] is the correct behavior and we need fix everything else. C) Then of course, there is the more complex answer where we allow revert to summarily delete additions in some cases, but not in others. Perhaps local additions are left as unversioned, but copies are deleted. Or perhaps in the latter case we only delete the addition if the copy is identical to its source. I'm wondering if there is any consensus on the 'A', ''B, or 'C' approaches. If 'A' or 'B' then our task is straightforward. If it's 'C', then we'll need some more discussion. I favor 'A'. I know that leaving behind unversioned items post-revert can be a bit of a pain in some use cases (e.g. "Oh I merged the wrong revs, let's revert and redo the correct merge, hey, what are all these skips!?!") but I think this approach is easy to understand and explain to users. The workarounds if someone has a problem with this behavior are also relatively simple. While option 'C' is tempting, I think it adds needless complexity and probably results in behavior that is difficult to explain to users in a concise way [2]. Option 'B' is awful IMHO. We don't expect svn revert to remove unversioned items (http://subversion.tigris.org/issues/show_bug.cgi?id=1424#desc3) and I think this is quite a similar situation. Paul [1] As described in http://subversion.tigris.org/issues/show_bug.cgi?id=3115#desc8, the single-DB wcng working copy now allows us to successfully merge a case-only file rename into a WC on a case-insensitive file system. But when we revert such a change, the added file is deleted, rather than left as an unversioned obstruction to the original file. [2] My view is obviously colored by my experiences with merge tracking, where we tried to intelligently DTRT no matter what the user tried. This has left us with a feature that can be exceedingly challenging to explain..."oh, so are are doing a sync merge into a mixed revision working copy with switched subtrees and some subtrees at depth=empty, ok we'll attempt to record mergeinfo to describe this situation, here is what will happen..."
