On Tue, Aug 10, 2010 at 14:41, Paul Burba <ptburba_at_gmail.com> wrote:
> On Tue, Aug 10, 2010 at 12:18 AM, Greg Stein <gstein_at_gmail.com> 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/path_at_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.
> 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.
Sure.
This is potentially a change in behavior, so if the file is also the
operation root of a copy/move, then it may want to note that only ONE
part of the revert operation has been performed. In 1.6, it would also
revert the copy/move operation.
Ah. Just thought of something. There is even one *more* potential
revert step to perform. Consider:
$ svn rm file2
$ svn cp file1 file2
$ edit file2
$ svn revert file2 # remove edits
$ svn revert file2 # undo the copy. now a local-delete
$ svn revert file2 # back to regular scheduling
Heh.
>...
>>> 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!).
Okay.
I do think there is a difference here, based on the concept of "don't
lose information that has not been retained elsewhere." Remove copied
files from the disk is no big deal, unless content edits have been
made. Removing added files is definitely bad.
>...
>> 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?
I would call it something like --remove-added-files to better clarify
what is happening. That name feels more like it is *actively* trashing
your data :-P ... guess we could also call it --trash-added-files :-P
I'm not sure about conflating the two options. The
--discard-local-edits simply allows a revert to proceed without (say)
issuing a warning such as "Local modifications have been made. Please
use --discard-local-edits to perform the revert, without seeing this
warning." Not sure, especially for the 3-revert case of "file2"
detailed above.
We could also have a flag along the lines of --all-changes that could
do both/three steps in one go.
To be honest, it's a bit beyond my interest scope at the moment :-P.
The data model provides for a lot more, refined behavior now. What and
how you work with that... go for it. I suspect we'll also need API
changes for this stuff... right now, we only have svn_wc_revert(), and
it hand-waves over all the possibilities that we've covered here.
Cheers,
-g
Received on 2010-08-10 21:43:37 CEST