You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2010/09/14 16:25:52 UTC
svn commit: r996914 - in /subversion/trunk/subversion:
libsvn_wc/update_editor.c tests/cmdline/externals_tests.py
Author: rhuijben
Date: Tue Sep 14 14:25:52 2010
New Revision: 996914
URL: http://svn.apache.org/viewvc?rev=996914&view=rev
Log:
When mixing the two major hacks of the update editor (copy_from location
and file externals), we can get into an unexpected state. Update an
assertion to handle this state properly and slightly update the expected
test result.
* subversion/libsvn_wc/update_editor.c
(merge_file): Allow status normal for file externals, to allow
merging added file externals that are introduced as add with copyfrom
information.
* subversion/tests/cmdline/externals_tests.py
(merge_target_with_externals): Expect an 'U' notification as the update
is handled as part of updating the parent working copy. The second
update (which would have shown 'E') is a no-op, as the file is already
at the right revision.
(test_list): Remove XFail marking from update_modify_file_external.
Modified:
subversion/trunk/subversion/libsvn_wc/update_editor.c
subversion/trunk/subversion/tests/cmdline/externals_tests.py
Modified: subversion/trunk/subversion/libsvn_wc/update_editor.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/update_editor.c?rev=996914&r1=996913&r2=996914&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/update_editor.c (original)
+++ subversion/trunk/subversion/libsvn_wc/update_editor.c Tue Sep 14 14:25:52 2010
@@ -4408,10 +4408,17 @@ merge_file(svn_skel_t **work_items,
### base, but the rest of libsvn_wc appears to compensate
### for this fact (even tho it is schedule_normal!!).
### in any case, let's do the working copy file install
- ### from the revert base for file externals. */
+ ### from the revert base for file externals.
+
+ ### Sheesh^2: If the file external is based on a copy from
+ ### a different 'related' location we receive copyfrom info
+ ### on the add. And in that specific case (externals_tests
+ ### 25, "update that modifies a file external") we have a
+ ### status normal instead of added. */
if (file_external)
{
- SVN_ERR_ASSERT(status == svn_wc__db_status_added);
+ SVN_ERR_ASSERT(status == svn_wc__db_status_added
+ || status == svn_wc__db_status_normal);
/* The revert-base will be installed later in this function.
To tell the caller to install the new working text from
Modified: subversion/trunk/subversion/tests/cmdline/externals_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/externals_tests.py?rev=996914&r1=996913&r2=996914&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/externals_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/externals_tests.py Tue Sep 14 14:25:52 2010
@@ -1554,7 +1554,7 @@ def update_modify_file_external(sbox):
# Update to modify the file external, this asserts in update_editor.c
expected_output = svntest.wc.State(wc_dir, {
- 'A/external' : Item(status='E '),
+ 'A/external' : Item(status='U '),
})
expected_disk.tweak('A/mu', 'A/external',
contents=expected_disk.desc['A/mu'].contents
@@ -1600,7 +1600,7 @@ test_list = [ None,
relegate_external,
wc_repos_file_externals,
merge_target_with_externals,
- XFail(update_modify_file_external),
+ update_modify_file_external,
]
if __name__ == '__main__':
Re: Copies via update -- net win or net loss? (Was: svn commit: r996914
...)
Posted by "C. Michael Pilato" <cm...@collab.net>.
> At this point, I'm not even really asking for permission any more. If
> somebody doesn't beat me to it, I'll gut the feature myself after I wrap up
> some other things that hold my attention right now.
Tracking this work in issue #3711.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Copies via update -- net win or net loss? (Was: svn commit: r996914
...)
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 09/15/2010 12:01 PM, Daniel Shahaf wrote:
> C. Michael Pilato wrote on Tue, Sep 14, 2010 at 10:58:20 -0400:
>> On 09/14/2010 10:25 AM, rhuijben@apache.org wrote:
>>> Author: rhuijben
>>> Date: Tue Sep 14 14:25:52 2010
>>> New Revision: 996914
>>>
>>> URL: http://svn.apache.org/viewvc?rev=996914&view=rev
>>> Log:
>>> When mixing the two major hacks of the update editor (copy_from location
>>> and file externals), we can get into an unexpected state. Update an
>>> assertion to handle this state properly and slightly update the expected
>>> test result.
>>
>> In light of our vision for the future regarding the pristine cache, and the
>> seeming flakiness of the special-case code added to handle copies during
>> update, should we kill that feature?
>
> Could someone summarize the feature/hack/special-case being discussed
> here? Is it about using the copyfrom info to optimize some over-the-wire
> transmissions during an update, as opposed to always asking for the fulltext
> of a copied file? (e.g., I found locate_copyfrom() in update-editor.c)
That's the one.
The server does a little extra work to calculate a copyfrom path, a little
extra work to authorize that path. If it all checks out, the server sends
that copyfrom info plus any additional content/prop deltas against that
copyfrom source to the client instead of just transmitting the new contents
of the copied thing in full.
If the client sees copyfrom info, it then goes to try to find that same
thing in the working copy. If it manages to find something, it does a local
copy and then applies those extra deltas (if any) from the server. If it
doesn't find the copyfrom source locally, it then asks the server for the
pristine version of the copyfrom source so it has something to apply those
deltas to.
Yeah. locate_copyfrom(). Eek.
I don't doubt that in some cases, this code might save someone a bit of
network traffic. But our vision for this code in the future involves much
more reliable, less-heuristically-driven approaches. SHA1 checksums into a
database keyed on as much, etc. I seriously believe we'd be better off
losing this codepath today, if only so we can simplify what is already a
very complex operation (update).
At this point, I'm not even really asking for permission any more. If
somebody doesn't beat me to it, I'll gut the feature myself after I wrap up
some other things that hold my attention right now.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Copies via update -- net win or net loss? (Was: svn commit:
r996914 ...)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Tue, Sep 14, 2010 at 10:58:20 -0400:
> On 09/14/2010 10:25 AM, rhuijben@apache.org wrote:
> > Author: rhuijben
> > Date: Tue Sep 14 14:25:52 2010
> > New Revision: 996914
> >
> > URL: http://svn.apache.org/viewvc?rev=996914&view=rev
> > Log:
> > When mixing the two major hacks of the update editor (copy_from location
> > and file externals), we can get into an unexpected state. Update an
> > assertion to handle this state properly and slightly update the expected
> > test result.
>
> In light of our vision for the future regarding the pristine cache, and the
> seeming flakiness of the special-case code added to handle copies during
> update, should we kill that feature?
Could someone summarize the feature/hack/special-case being discussed
here? Is it about using the copyfrom info to optimize some over-the-wire
transmissions during an update, as opposed to always asking for the fulltext
of a copied file? (e.g., I found locate_copyfrom() in update-editor.c)
Thanks,
Daniel.
> I've never been convinced that it was
> truly beneficial as written anyway -- it seems to just sorta throws "what
> ifs" across the wire and then burdens the client with verification and
> handling before falling back to doing what it has always done for non-copied
> files.
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
Re: Copies via update -- net win or net loss? (Was: svn commit: r996914
...)
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 09/14/2010 11:34 AM, Julian Foad wrote:
> The success of Subversion 1.7 and beyond depends on consistent
> behaviour, reliability and our ability to understand and work with the
> code. It doesn't depend on keeping strict compatibility with
> undocumented half-baked quirks that are believed to sometimes do
> something nice for some people's purple files.
So... uh... how do you really feel? :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Copies via update -- net win or net loss? (Was: svn commit:
r996914 ...)
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-09-14 at 10:58 -0400, C. Michael Pilato wrote:
> On 09/14/2010 10:25 AM, rhuijben@apache.org wrote:
> > Author: rhuijben
> > Date: Tue Sep 14 14:25:52 2010
> > New Revision: 996914
> >
> > URL: http://svn.apache.org/viewvc?rev=996914&view=rev
> > Log:
> > When mixing the two major hacks of the update editor (copy_from location
> > and file externals), we can get into an unexpected state. Update an
> > assertion to handle this state properly and slightly update the expected
> > test result.
>
> In light of our vision for the future regarding the pristine cache, and the
> seeming flakiness of the special-case code added to handle copies during
> update, should we kill that feature? I've never been convinced that it was
> truly beneficial as written anyway -- it seems to just sorta throws "what
> ifs" across the wire and then burdens the client with verification and
> handling before falling back to doing what it has always done for non-copied
> files.
The success of Subversion 1.7 and beyond depends on consistent
behaviour, reliability and our ability to understand and work with the
code. It doesn't depend on keeping strict compatibility with
undocumented half-baked quirks that are believed to sometimes do
something nice for some people's purple files.
Big +1.
- Julian
Copies via update -- net win or net loss? (Was: svn commit: r996914
...)
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 09/14/2010 10:25 AM, rhuijben@apache.org wrote:
> Author: rhuijben
> Date: Tue Sep 14 14:25:52 2010
> New Revision: 996914
>
> URL: http://svn.apache.org/viewvc?rev=996914&view=rev
> Log:
> When mixing the two major hacks of the update editor (copy_from location
> and file externals), we can get into an unexpected state. Update an
> assertion to handle this state properly and slightly update the expected
> test result.
In light of our vision for the future regarding the pristine cache, and the
seeming flakiness of the special-case code added to handle copies during
update, should we kill that feature? I've never been convinced that it was
truly beneficial as written anyway -- it seems to just sorta throws "what
ifs" across the wire and then burdens the client with verification and
handling before falling back to doing what it has always done for non-copied
files.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand