On Tue, Jan 13, 2009 at 3:14 PM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Tue, Jan 13, 2009 at 05:53:43PM +0000, Julian Foad wrote:
>> Stefan and I just had a short debugging session. I think in merge_file()
>> it's getting confused between the file that had modifications ('alpha')
>> and the target where it's trying to put them ('alpha.moved') and making
>> wrong if/else decisions, ending up calling
>> svn_wc__merge_internal(target="alpha.moved") when it shouldn't.
>>
>> I've added "###" comments about this into that function. That's all I
>> can do today. Anyone else?
>>
>> Putting the issue number in the Subject line so I can find the thread
>> again.
>
> Here's a diff that "fixes" the issue, or rather, illustrates that
> the root of the problem is some incompatibility between copy
> improvements and tree conflicts:
>
> Index: subversion/libsvn_repos/reporter.c
> ===================================================================
> --- subversion/libsvn_repos/reporter.c (revision 35215)
> +++ subversion/libsvn_repos/reporter.c (working copy)
> @@ -661,6 +661,7 @@
> *copyfrom_path = NULL;
> *copyfrom_rev = SVN_INVALID_REVNUM;
>
> +#if 0 /* copyfrom functionality has been disabled, see issue #3354 */
> if (b->send_copyfrom_args)
> {
> /* Find the destination of the nearest 'copy event' which may have
> @@ -700,6 +701,7 @@
> }
> }
> }
> +#endif
>
> return b->editor->add_file(path, parent_baton,
> *copyfrom_path, *copyfrom_rev,
>
>
> While this diff would technically enable us to remove this blocking
> issue from TODO-1.6, I'd rather like to find a proper fix, rather
> than committing this diff.
>
> I guess a proper fix would either:
>
> 1) Reconcile the two features in a way that prevents them from
> stepping on each others' toes.
>
> This might actually be a fairly simple fix once it is clear
> what needs to be done. I don't understand yet what needs to
> be done. It does not look like anyone else does either, because
> the issue is still open :)
>
> From #svn-dev:
> <stsp> my brain is honestly overloaded with the task of imagining all the
> stuff that happens during the update
> <stsp> it's complex because tree conflicts do their thing, copy improvements
> do their thing, and normal update does its thing
> <stsp> and I think somewhere they are stepping on each others' toes, but I
> don't know yet who is stepping on whom
> <julianf> yeah, we really need to separate it better.
>
> Anyone willing to help?
>
> 2) Back out copy improvements completely.
>
> First off, here's what Ben Collins-Sussman said about this in #svn-dev:
> <sussman> I say, feel free to rewrite or scratch the copy-impromevments
> <sussman> I agree that we need a more holistic approach
> <sussman> and TC seems like it's much more holistic
>
> But this might be easier said than done, especially so close
> before the 1.6 release.
>
> Some food for thought:
>
> The copy improvements made in 1.5 were designed to mitigate the problem
> of losing local changes made to a file which is moved during an update.
>
> Copy improvements...
>
> a) ... are known to only work when an update carries out operations
> in a certain order. More precisely, if the update happens to carry
> out the deletion of the file before the corresponding add,
> local changes are *not* moved over to the new location.
> Users have reported that they see no effect from the improvements made,
> see comment by Yann FOUBERT in issue #503
> http://subversion.tigris.org/issues/show_bug.cgi?id=503
> Effectively the feature is non-deterministic from the user's
> point of view.
When I am testing tree conflict scenarios, I seem to see this feature
in action a lot more than I thought I used to. Is it possible that
since the old behavior would "unversion" the file and we now preserve
it as a tree conflict that this actually is working more often now?
> b) ... are a bit redundant now that we flag this situation as
> a tree conflict anyway. Then again, they save users from
> manually moving local changes over to a moved file.
While I am not against removing the feature for 1.6, I am not sure I'd
say it is redundant. If it was, we would not have this on the
wishlist for a "ultimate" tree conflicts solution.
> c) ... are planned to be re-worked in a reliably way eventually
> as part of the tree-conflicts effort. But note that this is a
> plan, or rather, and idea. There are no concrete design proposals
> yet. We may want to consider this vapour-ware for now, rather
> than a serious alternative to the current copy improvements
> implementation.
>
> Copy improvements involved many public API modifications, and
> RA protocol modifications (mostly adding "copyfrom args").
> These would need to stay until 2.0 at least, but operations
> involving this API could probably be turned into no-ops.
> Depending on how 2c) will eventually look like, we might end
> up reusing this API or parts of it.
Was there actually any new API created specifically for this? I am
not sure adding copyfrom args really qualifies. I see that as more
"enabling" the feature than defining it. There is no reason that
information could have not always existed in the API before this
change. In fact, ISTR there was some API where it was already present
and others where it was not. I guess my point is that if we decide to
stop using this information for a release, I am not sure we need to
really think about deprecating anything right away because of it.
If we cannot figure out this specific problem, I'd say we should
consider disabling the feature for 1.6 but otherwise leaving the code
and API in place. I do think that the bulk of our users will not
really consider tree conflicts "solved" until our code is doing this
part reliably. Perhaps that will ultimately require first-class
rename support to get there, but I think people really want this
feature to work.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1022751
Received on 2009-01-13 21:25:03 CET