On Tue, Aug 10, 2010 at 12:18 AM, Greg Stein <gst...@gmail.com> 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 <ptbu...@gmail.com> 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 first place: merge of case-only remanes -- http://subversion.tigris.org/issues/show_bug.cgi?id=3115#desc10 > 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. svn revert will take the cautious approach and revert the "top" layer of changes first (i.e. the prop/text edits) and leave you with the copy. You can then revert that layer with another svn revert. A --discard-local-edits will allow you to do it all in one step if you wish. > 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. I 100% agree. I only threw 'B' out there as the inverse to 'A'. As I said, "Option 'B' is awful IMHO". Though I suppose someone could make the argument that if you use 'svn revert' you expect information to be lost. Not me, but the argument could be made with a straight face. >> 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. Nope, this is the option where we *don't* take the blanket approach of A or B for both copies and additions, but rather treat them differently. Again I was using "additions" in the general (and incorrect) sense of both scheduled adds and copies (and no, I won't do that again!). >> 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. > > The user said "revert my svn add". They didn't say "remove the file". > >>... > > The rest of the message seemed to be trying to argue what to do around > certain types of situations. Whatever. Revert is about an *operation*. > It does not mean "remove" in any sense. > > Reverting a *copy* or a *move* can conceivably remove content from the > filesystem. This is where the *two* semantics come in to help inform > the situation. If there are local edits (post-copy/move), then tossing > the content is very troublesome. If there are no local edits, then > rm'ing is easy. The content that was sitting (uncommitted) on disk is > located elsewhere. When local edits are present, the revert could > conceivably require a switch 'svn revert B --discard-local-edits'. I like this new option, it gives us the flexibility to cover just about every use case. The only thing that might be even better: A second option (--discard-local-adds) to remove scheduled additions from disk as well. Or combine both behaviors into one option, --discard-local, whose behavior is dependent on whether we are reverting a local add or a copy. Thoughts? Paul > Or > the user could individually revert local edits, then revert the > entirety of B. > > Inventing a new revert switch to discard propchanges on B, but NOT > revert its underlying copy/move-here is an exercise for the reader. > > Cheers, > -g