On Tue, Aug 10, 2010 at 12:18 AM, Greg Stein <gstein_at_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 <ptburba_at_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
Received on 2010-08-10 20:42:12 CEST