RE: svn commit: r1095130 - in /subversion/trunk/subversion: libsvn_client/client.h libsvn_client/externals.c libsvn_client/switch.c libsvn_client/update.c libsvn_wc/adm_crawler.c tests/cmdline/externals_tests.py

> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato_at_collab.net]
> Sent: woensdag 20 april 2011 19:45
> To: dev_at_subversion.apache.org
> Subject: Re: svn commit: r1095130 - in /subversion/trunk/subversion:
> libsvn_client/client.h libsvn_client/externals.c libsvn_client/switch.c
> libsvn_client/update.c libsvn_wc/adm_crawler.c
> tests/cmdline/externals_tests.py
>
> On 04/20/2011 01:32 PM, Stefan Sperling wrote:
> > For the record, Bert and I discussed this on IRC today.
> >
> > The conclusion is that this feature will be revived as it was implemented,
> > with the addition of a new flag to svn update that allows users to enable
> > external definitions on locally added directories. Without this flag,
> > external definitions on locally added directories will be ignored.
> >
> > There is a performance overhead involved in crawling the DB for external
> > definitions. Bert's opinion is that adding this overhead to every update
> > operation is not acceptable. I don't think that is a big problem.
> > But the command line switch will make both of us happy.
> >
> > This way, users can test their externals definitions locally before
> > committing them, which is the goal of issue #2267. But normal updates
> > will not have to endure additional overhead.
>
> This just sounds like a truly odd thing to hook to a command-line option.
> That said, I confess my ignorance of the details of the issue. We already
> have to crawl for external definitions as part of 'svn update', don't we?
> Isn't that what the external_func/external_baton pair passed to
> svn_wc_crawl_revisions5() is for?

For the crawling and the update process we look at the op_depth 0 (also known as BASE) tree. This extension (since 1.7.0) explicitly adds support for externals on nodes that are not yet committed to the repository. These newly added nodes are never touched by the update processor (unless they cause a tree conflict).
So there is not much to gain by building this added directory support in the update processing in libsvn_wc.

Another topic we discussed is that the update processor explicitly handles *the changes* in svn:externals properties.

The update processing of svn:externals doesn't just take the last set of properties. Instead it takes the old and the new definitions of the svn:externals and then performs a diff on them.

* Externals that are removed are then removed from the working copy. (After verifying that they were from the right location)
* Externals that are changed are switched or deleted and re-added if they are from a different repository (After verification)
* And new externals are added.

One problem with externals on added nodes is that we can only look at the last definition of the externals.
So instead we can only:
* Delete if some path looks like an old external (guessing) and is reused
* Add new externals
And old externals are left as is (see issue #3351 for a real problem).

For non-file externals its usually easy to fix these problems yourself (Something wrong: just delete it and it will get fixed on the next update). But for file externals you will have to hack the database or call libsvn_wc yourself.

So applying these uncommitted svn:externals by default opens new options to shoot yourself in the foot and break your wc. (When used with precaution and without file externals it is certainly useful. But we can't tell the user that when they are just prop-editing their property. )

I was busy writing another mail on possible solutions:
The best solution would be to store the currently applied externals somewhere outside the properties hash so we can still apply 'differences' instead of 'the last svn:externals' value.

But when we go that far, we should also look at more viewspec options.

        Bert
Received on 2011-04-20 21:25:18 CEST