Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [subversive-dev] issue #353875 -- replace-with-revision on locked file fails even if file isn't being changed.

On 09/26/2011 07:26 PM, Alexander Gurov wrote:
> Hello,
> 
> I've read the report some time ago, there were some questions I did not have
> answers to and that is why I didn't started implementing this request:
> 1) currently "replace with" actions performs replacement only for the
> content of the files, but not for the properties (merge will cause a change
> in properties). Which way is better - I'm not sure.
> 2) Reverse merge is a nice thing but how it will go - I can't predict:
>      - it could produce conflicts
>      - it could skip some resources due to merge tracking information
>      - there could be some other issues I can't tell from the get-go

yes, I've got a few more pointers and facts. Indeed, the situation is overly
complex. Let me elaborate...


(1) Replace with Revision

The feature Replace-With-Revision is supposed to replace a given set of
nodes with a previous revision, so it will merge all consecutive revisions
from the current working revision backwards to the desired revision. This
can only produce conflicts when there are local changes in the working copy.
If the working copy is unchanged, it's a pretty trivial merge, as it is from
the same URL and has an uninterrupted revision range, basically identical to
'svn update'. So all Subversive has to do is revert local changes before the
merge. (And probably warn if there are local mods before doing so -- a
different feature request)

More things the current implementation does apparently not address is node
additions/removals against the desired revision. Example:

r1:
A  DIR
A  DIR/file
A  DIR/SUBDIR

r2:
A  DIR/new

r3:
D  DIR/file
D  DIR/SUBDIR

So if we're selecting DIR to go back to r1, DIR/new should vanish and
DIR/file and DIR/SUBDIR should reappear. An undo merge takes care of those, too.

This situation *can* reach an obstruction conflict, i.e. if there's an
unversioned file or dir in the way. Say after r3, we re-create a different
DIR/file but we don't add it to version control. Going back to r1 would then
not be able to bring the version controlled DIR/file back into the WC, and
it would mark an obstruction in 'status' output.

All of these situations should be handled by your merge code -- usually
though, a reverse merge Just Works. It should be identical to 'svn update
-rN', only the working copy stays at HEAD and remains committable.

A merge from the same branch does not affect mergeinfo, except of course if
a merged revision itself contained mergeinfo changes, which should be
correct in all cases. There should be no need to bother.

The only thing I would worry about is that the way this feature has been
implemented so far may have inadvertently created a new use case that some
people have started to rely on. I mean that possibly some users out there
are very well aware of the effects of an undo merge and have found out that
they sometimes prefer to bluntly overwrite their WC content with an export
of a previous revision, and have found that Subversive's
Replace-With-Revision does exactly that. But I think this is quite unlikely,
and on top of that, if users have this amount of knowledge they should
surely be able to get the same thing differently.

Another note: Replace-with-revision should actually take great care that the
supplied revision number is smaller than each WC node's current working
revision! (Merging forward is complete nonsense, should update instead.)


(2) Replace with URL

This is somewhat more complex. Indeed the most logical way to achieve this
would be to
- 'svn delete' the entire WC subtree to-be-replaced and then
- 'svn copy' from the other URL in its place.
This would of course cause replaces ('R' status) to be committed, which
might be undesirable ("evil twins", tree-conflicting against other users'
local changes during the next 'svn update', instead of the usual
update-text-merging). So instead one could 'svn export', replacing current
content, and then take care to 'svn delete' and 'svn add' nodes as needed
(is it doing that right now?). But the props are still lost that way, and it
adds open-ended WC logic outside of the upstream API: how to handle
server-excluded items? Items obstructed by unversioned nodes? externals?
etc. etc. -- the Subversion API is supposed to handle all those properly,
Subversive should not have to bother. But because this feature is added on
top of standard API, Subversive is forced to bother.

So IMO the replace-with-URL is a no-feature that should not have been
implemented in this way. Subversive should have provided a simple export
functionality and let the users figure out what they actually want to do.
However, since it's there now, it's not so trivial to make such a decision.
Is this feature used by users out there? What do they expect? I frankly
have no idea :)

If I as a Subversion literate were to use a "replace with revision / URL"
feature, I would definitely expect properties, externals etc. to be a part
of that -- but that is an entirely different feature request from this bug
report, and I, personally, am not interested in that at all. Maybe other
users would like that and speak out / propose a patch. As no-one has done so
yet, I guess no-one is aware of it nor has any problem with it.



That's why I decided to actually go for the fix that I posted: it's small
and it changes hardly anything (the Perfect here seems to be the Enemy of
the Good). All that the patch does is to avoid the annoying error message
and operation abort which, from the user's point of view, should not even
happen, as the file in question is not supposed to change at all. Compared
with a full 'svn export' and a file system copy, a file comparison hardly
weighs in as performance loss. Also, the comparison only takes place if the
particular exception has been thrown. If we were not comparing at that
point, the exception would simply abort the operation, and that would mean a
much larger performance loss, as the user would need to actually start
*thinking*, possibly redoing the operation multiple times and contacting
site admins until she figured out what is going on in the first place.


About that exception -- looking at my patch I do wonder why it is a
FileNotFoundException at all. It should be an IOException with "Permission
denied". Very confusing.

The Perfect: Subversive should probably lose the Replace-with-rev/URL
feature completely and tell users to use 'merge', 'copy' or 'export' instead
-- which is not for me to decide. I'm surprised though that this feature
doesn't cause major headaches on a regular basis, given all the details it
bluntly ignores, apparently... ;)


I hope you can draw some conclusions from my svn-dev insights, and that
you're not annoyed by my drastic suggestions. The proposed patch is the
least invasive fix, and IMHO its imperfection blends in well ;)

Thanks,
~Neels

> 
> Regarding your solution with catching exception and comparing files - it
> should work, but could run really slow if any big file where to happen being
> locked (and there are quite a chance for this with the binary files we're
> talking about).
> 
> That's why I haven't applied your patch and left it untouched until I've
> found some alternatives. But thinking back - it's not like there are a lot
> of ways to solve the problem, so your solution is probably the best at the
> moment and I should include it into the next build.
> 
> If there is some other points I should pay attention to - please tell me.
> 
> Best regards,
> Alexander Gurov.
> 
> 26.09.2011 16:57, Neels J Hofmeyr пишет:
>> Hi Subversive devs,
>>
>> I'd like to enquire about issue #353875, simply because we have a commitment
>> with a client to do something about this bug, which annoys them on a regular
>> basis. ( https://bugs.eclipse.org/bugs/show_bug.cgi?id=353875 )
>>
>> Is there anything else I can do to facilitate a fix being released in
>> upstream? Bundling a custom Subversive is (unfortunately) not an option for
>> our client.
>>
>> - If this needs more thinking to deeply fix the problem, is it an option to
>> have this or a similar "surface fix" in upstream in the meantime?
>> ( https://bugs.eclipse.org/bugs/attachment.cgi?id=200910&action=diff <https://bugs.eclipse.org/bugs/attachment.cgi?id=200910&action=diff> )
>>
>> - Do you have some hints for me, so I can improve my patch so that you will
>> agree to commit it?
>>
>> - Or hints for a different approach altogether, which I can try to implement
>> for/with you?
>>
>> We would be delighted to collaborate on this, to keep our customers happy.
>>
>> Thanks,
>> ~Neels
>>
>> On 08/04/2011 08:29 PM, Alexander Gurov wrote:
>>> I've read the task you've made and I think it contains some nice ideas. I
>>> can't say I will apply the patch as it is, there is a need to think about it
>>> a little more. And may be there is a reason to rethink the decision
>>> regarding the issue 294610. Anyway thank you for ideas and your overall
>>> analysis of the situation.
>> -----
>> Neels Hofmeyr | Senior Developer
>> elego Software Solutions GmbH | http://www.elego.de
>> Gustav-Meyer-Allee 25 | Building 12 - BIG | 13355 Berlin, Germany
>> fon +49 30 2345 8696 | fax +49 30 2345 8695
>> CEO Olaf Wagner | District Court Berlin-Charlottenburg HRB 77719
>>
>>
>>
>> _______________________________________________
>> subversive-dev mailing list
>> subversive-dev@xxxxxxxxxxx <mailto:subversive-dev@xxxxxxxxxxx>
>> https://dev.eclipse.org/mailman/listinfo/subversive-dev
> 
> 
> 
> _______________________________________________
> subversive-dev mailing list
> subversive-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/subversive-dev

Attachment: signature.asc
Description: OpenPGP digital signature


Back to the top