Re: Some overlooked single-db-isms?
Thanks for catching this. I'll cook up a test and fix it. - Julian On Fri, 2010-09-03 at 01:22 -0400, C. Michael Pilato wrote: Tonight I ran into a codepath which triggers and assertion. $ svn up subversion/libsvn_wc/wc_db.c:383: (apr_err=235000) svn: In file 'subversion/libsvn_wc/wc_db.c' line 9823: assertion failed (!update_stub) Aborted The assertion is in svn_wc__db_temp_op_set_rev_and_repos_relpath(), asserting that, when in single-db mode, the caller didn't pass update_stub=TRUE. Well, tweak_entries() (in update_editor.c) ultimately does exactly that via call to tweak_node() with parent_stub=TRUE, and that's what caused my assertion. There's another call to tweak_node() in do_update_cleanup() that also passes parent_stub=TRUE. I can only assume that, too, can trigger the assertion. But I haven't run into it in practice myself yet.
Pascal bindings
On Thu, Sep 2, 2010 at 5:38 PM, Philip Martin philip.mar...@wandisco.com wrote: You are more likely to get some response if you send a patch against trunk with a log message, see http://subversion.apache.org/docs/community-guide/general.html#patches Thanks for the link. I'm not ready yet with the trunk version bindings. Even if you do that I don't know that there is any great demand for Pascal bindings. I'm not sure if there's any demand on them. It feels like I'm the only one who needs them. I've written a front-end for the subversion command-line client with FPC, and I'm not satisfied with it. The problem is parsing svn output. XML feature is good, but it cannot be used for all cases. And sometimes it hits the performance. Using the library seems to me the better solution. The new Delphi (which is ObjectPascal IDE) is proud of its subversion integration, but i'm sure it's written in C/C++, so they didn't have to use Pascal bindings. Are they generated or written by hand? They're 95% generated. Except for svn_error_code.h, since it uses macros heavily, and the parser is not yet good enough to handle them properly. Some macros are also hand translated: i.e macro (svn_pool.h) #define svn_pool_clear apr_pool_clear is translated as inline function by hand, since Pascal tends not to use macros. Do they have regression tests? How can this be applied? The Pascal bindings are not providing any kind of high-level wrappers over the library. All Subversion functions and structures are used directly. Are they tied to any particular environment? No. The bindings should be used with Free Pascal Compiler or Delphi and should work on any system these compiler support. I've tested them on Windows (SlikSVN dlls) and MacOSX (Xcode .dylibs) and the bindings did work fine. Is Dmitry volunteering to stick around and maintain these bindings? Yes, I am volunteering. if the bindings are accepted (acceptable). I'm also helping FPC project to maintain other C-library bindings (i.e. OpenGL, OpenCL) thanks, dmitry
Worried about single-db performance
Hi devs, From what I understand about the performance problems of WC-1 vs. WC-NG, and what I'm reading on this list, I expect(ed) a huge performance boost from WC-NG for certain client operations (especially on Windows, where the locking of WC-1 is quite problematic). Also, I knew I had to wait for single-db to see any real performance benifits. So after you guys switched on single-db, I eagerly gave trunk a spin... Now I'm a little worried, because I don't see any great speed increases (quite the contrary). Some details below. Maybe it's just too early to be looking at this (maybe it's a simple matter of optimizing the data model, adding indexes, optimizing some code paths, ...). So it's fine if you guys say: chill, just wait a couple more weeks. I just need to know whether I should be worried or not :-). Some details ... Setup: - Win XP 32-bit client machine, with antivirus switched off. - Single-db client: tr...@992042 (yesterday), release build with VCE2008 - 1.6 client: 1.6.5 binary from tigris.org that I still had lying around. - Medium size working copy (944 dirs, 10286 files), once checked out with the 1.6 client (WC-1), once checked out with the trunk-single-db client. - 1st run means after reboot, 2nd run means immediately after 1st run. Numbers: 1) Status (svn st) 1.6 client 1st run: real0m41.593s user0m0.015s sys 0m0.015s 1.6 client 2nd run: real0m1.203s user0m0.015s sys 0m0.031s Single-db client 1st run: real0m34.984s user0m0.015s sys 0m0.031s Single-db client 2nd run: real0m6.938s user0m0.015s sys 0m0.031s 2) Update (no changes, wc already up to date) (svn up) 1.6 client 1st run: real0m38.484s user0m0.015s sys 0m0.015s 1.6 client 2nd run: real0m1.141s user0m0.015s sys 0m0.015s Single-db client 1st run: real0m31.375s user0m0.015s sys 0m0.031s Single-db client 2nd run: real0m5.468s user0m0.031s sys 0m0.015s Anyone able to take away my worries :-) ? Cheers, -- Johan
Re: Do we want 'svn patch' to be able to add empty files?
On 02.09.2010 10:50, Branko Čibej wrote: On 02.09.2010 10:27, Daniel Shahaf wrote: Daniel Näslund wrote on Thu, Sep 02, 2010 at 07:13:00 +0200: On Wed, Sep 01, 2010 at 06:37:08PM +0100, Julian Foad wrote: This may be off topic, but I'm wondering whether Git has defined such operations on directories fully or at all, since it doesn't version them explicitly. I mean, can you tell the difference between add empty file A and add empty dir A? I could go and look, but haven't time today. If yes, great; if it doesn't, we'll have to invent syntax extensions to do it. (I'm recalling that the goal of this work is we want Subversion diffs to be able to support all valid Subversion changes, and we chose Git format as a basis for supporting that. We don't want to constrain ourselves to only the operations that Git supports.) Not supported at the moment: $ svn mkdir X A X $ svn status AX $ svn diff $ svn diff --git $ Suggestion: $ svn diff --git Index: empty === diff --git a/trunk/empty b/trunk/empty new directory mode 10644 IIRC trailing slashes on empty/ were suggested on IRC, what was the conclusion about that? E.g., just changing the 'new file mode 10644' line to mention directory instead. Haven't investigate what changes would be needed in the diff editor. By the way, are we just influenced by Git's format, or are we looking for some degree of interoperability? Consider adding a symlink: [[[ ### with git (in $wcroot/trunk/): diff --git a/trunk/bar b/trunk/bar new file mode 12 index 000..1910281 --- /dev/null +++ b/trunk/bar @@ -0,0 +1 @@ +foo \ No newline at end of file ### with svn (in $wcroot/trunk/): Index: bar === diff --git a/trunk/bar b/trunk/bar new file mode 10644 --- /dev/null (revision 0) +++ b/trunk/bar (working copy) @@ -0,0 +1 @@ +link foo \ No newline at end of file Property changes on: trunk/bar ___ Added: svn:special ## -0,0 +1 ## +* ]]] Hmm, this is interesting. :) Git faithfully (blindly?) interprets Unix permission bits, whiles SVN faithfully (blindly?) interprets the contents of special files ... I wonder if svn patch does the right thing here? Anyway, for the sake of interoperability, we'd have to emit and parse the git format for symlinks. Not that I'm too amused by the idea that git probably just does a chmod on the new file without thinking about it, but hey, All the World is Linux, right? :) Did some testing ... apparently git apply completely ignores the permission bits new file mode ... line, at least I haven't been able to force it to do anything with them. -- Brane
RE: Pascal bindings
-Original Message- From: C. Michael Pilato [mailto:cmpil...@collab.net] Sent: Thursday, September 02, 2010 7:47 PM To: Philip Martin Cc: dmitry boyarintsev; dev@subversion.apache.org Subject: Re: Pascal bindings On 09/02/2010 01:38 PM, Philip Martin wrote: dmitry boyarintsev skalogryz.li...@gmail.com writes: Hello Subversion-dev, I can see, that there's no much of interest in Pascal bindings. Well, that's quite understandable because of Pascal language not being popular. Anyway, I'll publish the headers on my site. Thank you for your great product! You are more likely to get some response if you send a patch against trunk with a log message, see http://subversion.apache.org/docs/community-guide/general.html#patches Even if you do that I don't know that there is any great demand for Pascal bindings. Are they generated or written by hand? Do they have regression tests? Are they tied to any particular environment? All great questions, Philip. Another one that's on my mind is, Is Dmitry volunteering to stick around and maintain these bindings? We really try to avoid drop-and-ignore contributions of this sort, where none of the active committership appears to be interested (or perhaps even qualified) to maintain the new code. While that's an ideal position to be in, isn't there room for a 'second class' addition to the main codebase where the code is present, hopefully working, yet comes with no warranty or is part of the automated test set. The alternative is that valuable code would end up being posted to some website and wouldn’t have the visibility in the future if someone did come along, decided that they wanted the feature, and then spent the effort to fix it up (assuming it was broken). As was mentioned, the python bindings were not maintained until recently when someone did send contributions in. If the python bindings were not present at all, this would not have happened. So I'd say a pragmatic approach would be to take the contribution, and place it off to the side slightly compared to the 'tested, maintained' codebase and have the tests available to run manually. Maybe if someone comes along to maintain the code, then it can be moved to being a 'full' member of the codebase. not that that is an issue in this case as Dmitry says he will actively maintain it, and other C bindings, although it appears he is already slightly alienated from contributing given his response.
Re: svn commit: r990385 - /subversion/trunk/subversion/libsvn_client/patch.c
s...@apache.org wrote on Sat, Aug 28, 2010 at 15:49:52 -: Author: stsp Date: Sat Aug 28 15:49:52 2010 New Revision: 990385 URL: http://svn.apache.org/viewvc?rev=990385view=rev Log: * subversion/libsvn_client/patch.c (try_stream_write): Remove a question I put into a comment, not realising that try_stream_write() is there to catch errors like disk is full. Modified: subversion/trunk/subversion/libsvn_client/patch.c Modified: subversion/trunk/subversion/libsvn_client/patch.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=990385r1=990384r2=990385view=diff == --- subversion/trunk/subversion/libsvn_client/patch.c (original) +++ subversion/trunk/subversion/libsvn_client/patch.c Sat Aug 28 15:49:52 2010 @@ -1236,11 +1236,7 @@ get_hunk_info(hunk_info_t **hi, patch_ta /* Attempt to write LEN bytes of DATA to STREAM, the underlying file * of which is at ABSPATH. Fail if not all bytes could be written to - * the stream. Do temporary allocations in POOL. - * ### stsp: Maybe we can remove this? It's only about checking wether - * ### all data was written, but we're usually doing buffered I/O - * ### anyway so if the disk or filesystem fails we likely won't - * ### see the failure right here. I've never seen this error out. */ + * the stream. Do temporary allocations in POOL. */ static svn_error_t * try_stream_write(svn_stream_t *stream, const char *abspath, const char *data, apr_size_t len, apr_pool_t *pool) I agree with your question, actually. Note the svn_stream_write() docs: 722 * The read and write handlers accept length arguments via pointer. 723 * On entry to the handler, the pointed-to value should be the amount 724 * of data which can be read or the amount of data to write. When the 725 * handler returns, the value is reset to the amount of data actually 726 * read or written. Handlers are obliged to complete a read or write 727 * to the maximum extent possible; thus, a short read with no 728 * associated error implies the end of the input stream, and a short ^^^ 729 * write should never occur without an associated error. ^
Re: svn commit: r990385 - /subversion/trunk/subversion/libsvn_client/patch.c
On Fri, Sep 03, 2010 at 01:47:06PM +0300, Daniel Shahaf wrote: s...@apache.org wrote on Sat, Aug 28, 2010 at 15:49:52 -: Author: stsp Date: Sat Aug 28 15:49:52 2010 New Revision: 990385 URL: http://svn.apache.org/viewvc?rev=990385view=rev Log: * subversion/libsvn_client/patch.c (try_stream_write): Remove a question I put into a comment, not realising that try_stream_write() is there to catch errors like disk is full. Modified: subversion/trunk/subversion/libsvn_client/patch.c Modified: subversion/trunk/subversion/libsvn_client/patch.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=990385r1=990384r2=990385view=diff == --- subversion/trunk/subversion/libsvn_client/patch.c (original) +++ subversion/trunk/subversion/libsvn_client/patch.c Sat Aug 28 15:49:52 2010 @@ -1236,11 +1236,7 @@ get_hunk_info(hunk_info_t **hi, patch_ta /* Attempt to write LEN bytes of DATA to STREAM, the underlying file * of which is at ABSPATH. Fail if not all bytes could be written to - * the stream. Do temporary allocations in POOL. - * ### stsp: Maybe we can remove this? It's only about checking wether - * ### all data was written, but we're usually doing buffered I/O - * ### anyway so if the disk or filesystem fails we likely won't - * ### see the failure right here. I've never seen this error out. */ + * the stream. Do temporary allocations in POOL. */ static svn_error_t * try_stream_write(svn_stream_t *stream, const char *abspath, const char *data, apr_size_t len, apr_pool_t *pool) I agree with your question, actually. Note the svn_stream_write() docs: 722 * The read and write handlers accept length arguments via pointer. 723 * On entry to the handler, the pointed-to value should be the amount 724 * of data which can be read or the amount of data to write. When the 725 * handler returns, the value is reset to the amount of data actually 726 * read or written. Handlers are obliged to complete a read or write 727 * to the maximum extent possible; thus, a short read with no 728 * associated error implies the end of the input stream, and a short ^^^ 729 * write should never occur without an associated error. ^ Oh, that's interesting. That's a weird API, with an output parameter that's only interesting when an error occured. But I guess this means we can really remove the silly try_stream_write() function. Thanks, Stefan
Re: svn commit: r992042 - in /subversion/trunk/subversion: libsvn_client/merge.c tests/cmdline/merge_authz_tests.py tests/cmdline/merge_tests.py tests/cmdline/merge_tree_conflict_tests.py tests/cmdlin
pbu...@apache.org writes:
Author: pburba
Date: Thu Sep 2 18:10:01 2010
New Revision: 992042
@@ -5718,6 +5849,37 @@ get_mergeinfo_paths(apr_array_header_t *
merge_cmd_baton-ctx-cancel_baton,
scratch_pool));
+ if (apr_hash_count(wb.missing_subtrees))
+{
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
../src/subversion/libsvn_client/merge.c: In function ‘get_mergeinfo_paths’:
../src/subversion/libsvn_client/merge.c:5855: warning: declaration of
‘iterpool’ shadows a previous local
../src/subversion/libsvn_client/merge.c:5822: warning: shadowed declaration is
here
Is the second iterpool deliberate?
--
Philip
Re: Do we want 'svn patch' to be able to add empty files?
On Fri, Sep 03, 2010 at 12:18:37PM +0200, Branko Čibej wrote: On 02.09.2010 10:50, Branko Čibej wrote: Hmm, this is interesting. :) Git faithfully (blindly?) interprets Unix permission bits, whiles SVN faithfully (blindly?) interprets the contents of special files ... I wonder if svn patch does the right thing here? Anyway, for the sake of interoperability, we'd have to emit and parse the git format for symlinks. Not that I'm too amused by the idea that git probably just does a chmod on the new file without thinking about it, but hey, All the World is Linux, right? :) Did some testing ... apparently git apply completely ignores the permission bits new file mode ... line, at least I haven't been able to force it to do anything with them. From builtin/apply::try_create_file() in the git source code: fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode 0100) ? 0777 : 0666); if (fd 0) return -1; Git only checks for the executable bit, AFAIK. Daniel
[PATCH] don't do autoprops on symbolic links
This patch is for the following file. https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/svn_apply_autoprops.py Log message: Do not apply autoprops on symbolic links in svn_apply_autoprops.py. Index: svn_apply_autoprops.py === --- svn_apply_autoprops.py (revision HEAD) +++ svn_apply_autoprops.py (working copy) @@ -124,6 +124,7 @@ prop_list = autoprops_line[1] matching_filenames = fnmatch.filter(filenames, fnmatch_str) +matching_filenames = [f for f in matching_filenames if not os.path.islink(f)] if not matching_filenames: continue
Re: Some overlooked single-db-isms?
I (Julian Foad) wrote: Thanks for catching this. I'll cook up a test and fix it. Fixed in r992276, with a new test. - Julian On Fri, 2010-09-03 at 01:22 -0400, C. Michael Pilato wrote: Tonight I ran into a codepath which triggers and assertion. $ svn up subversion/libsvn_wc/wc_db.c:383: (apr_err=235000) svn: In file 'subversion/libsvn_wc/wc_db.c' line 9823: assertion failed (!update_stub) Aborted The assertion is in svn_wc__db_temp_op_set_rev_and_repos_relpath(), asserting that, when in single-db mode, the caller didn't pass update_stub=TRUE. Well, tweak_entries() (in update_editor.c) ultimately does exactly that via call to tweak_node() with parent_stub=TRUE, and that's what caused my assertion. There's another call to tweak_node() in do_update_cleanup() that also passes parent_stub=TRUE. I can only assume that, too, can trigger the assertion. But I haven't run into it in practice myself yet.
Re: [PATCH] don't do autoprops on symbolic links
Sorry, that line should have been matching_filenames = [f for f in matching_filenames if not os.path.islink(dirname+'/'+f)] On Fri, Sep 3, 2010 at 8:15 PM, Wei-Yin Chen wyc...@video.ee.ntu.edu.twwrote: This patch is for the following file. https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/svn_apply_autoprops.py Log message: Do not apply autoprops on symbolic links in svn_apply_autoprops.py. Index: svn_apply_autoprops.py === --- svn_apply_autoprops.py +++ svn_apply_autoprops.py @@ -124,6 +124,7 @@ prop_list = autoprops_line[1] matching_filenames = fnmatch.filter(filenames, fnmatch_str) +matching_filenames = [f for f in matching_filenames if not os.path.islink(dirname+'/'+f)] if not matching_filenames: continue
Re: [PATCH] don't do autoprops on symbolic links
On Fri, Sep 03, 2010 at 08:58:09PM +0800, Wei-Yin Chen wrote: Sorry, that line should have been matching_filenames = [f for f in matching_filenames if not os.path.islink(dirname+'/'+f)] Hi, thanks for your patch! I think we should use os.sep instead of '/', because os.sep is more portable. Also, please put spaces around operators (a + b, instead of a+b), to keep the style of the script consistent. Thanks, Stefan On Fri, Sep 3, 2010 at 8:15 PM, Wei-Yin Chen wyc...@video.ee.ntu.edu.twwrote: This patch is for the following file. https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/svn_apply_autoprops.py Log message: Do not apply autoprops on symbolic links in svn_apply_autoprops.py. Index: svn_apply_autoprops.py === --- svn_apply_autoprops.py +++ svn_apply_autoprops.py @@ -124,6 +124,7 @@ prop_list = autoprops_line[1] matching_filenames = fnmatch.filter(filenames, fnmatch_str) +matching_filenames = [f for f in matching_filenames if not os.path.islink(dirname+'/'+f)] if not matching_filenames: continue
Re: svn commit: r992042 - in /subversion/trunk/subversion: libsvn_client/merge.c tests/cmdline/merge_authz_tests.py tests/cmdline/merge_tests.py tests/cmdline/merge_tree_conflict_tests.py tests/cmdlin
On Thu, Sep 2, 2010 at 5:52 PM, Julian Foad julian.f...@wandisco.com wrote:
pbu...@apache.org wrote:
Fix issue #2915 'Handle mergeinfo for subtrees missing due to removal by
non-svn command'.
With this change, if you attempt a merge-tracking aware merge to a WC
which is missing subtrees due to an OS-level deletion, then an error is
raised before any editor drives begin. The error message describes the
root of each missing path.
+1 to this solution.
Review comments below.
* subversion/libsvn_client/merge.c
(get_mergeinfo_walk_baton): Add some new members for tracking sub-
directories' dirents.
(record_missing_subtree_roots): New function.
(get_mergeinfo_walk_cb): Use new function to flag missing subtree
roots.
(get_mergeinfo_paths): Raise a SVN_ERR_CLIENT_NOT_READY_TO_MERGE error if
any unexpectedly missing subtrees are found.
* subversion/tests/cmdline/merge_authz_tests.py
(skipped paths get overriding mergeinfo): Don't test the file missing via
OS-delete scenario, this is now covered in a much clearer manner in the
new merge_tests.py.merge_with_os_deleted_subtrees test. Also remove not
about testing a missing directory, that is also covered in the new test.
* subversion/tests/cmdline/merge_tests.py
(merge_into_missing): Use --ignore-ancestry during this test's merge to
disregard mergeinfo and preserve the original intent of the test.
(skipped_files_get_correct_mergeinfo): Use a shallow WC to rather than an
OS-level delete to test issue #3440. Remove the second part of this test
which has no relevance now that merge tracking doesn't tolerate subtrees
missing via OS-deletion.
(merge_with_os_deleted_subtrees): New test.
(test_list): Add merge_with_os_deleted_subtrees.
* subversion/tests/cmdline/merge_tree_conflict_tests.py
(tree_conflicts_merge_edit_onto_missing,
tree_conflicts_merge_del_onto_missing): Use --ignore-ancestry during these
tests' merges to disregard mergeinfo and preserve the original intent of
the test. Don't bother with the post-merge commit either, since there is
nothing to commit as there are no mergeinfo changes.
* subversion/tests/cmdline/svntest/actions.py
(deep_trees_run_tests_scheme_for_merge): Add new args allowing caller to
use --ignore-ancestry during the merge and to skip the post merge commit
if desired.
[...]
@@ -5718,6 +5849,37 @@ get_mergeinfo_paths(apr_array_header_t *
merge_cmd_baton-ctx-cancel_baton,
scratch_pool));
+ if (apr_hash_count(wb.missing_subtrees))
+ {
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+ const char *missing_subtree_err_str = NULL;
+
+ for (hi = apr_hash_first(iterpool, wb.missing_subtrees);
That's what I get for writing some quick-and-dirty code for testing
purposes and letting it morph in my mind to complete status while I
work on the real problem. Fixed in
http://svn.apache.org/viewvc?view=revisionrevision=992314.
Iterpool isn't being used effectively here. Normally, we would allocate
the iterator in the outer pool (hi =
apr_hash_first(scratch_pool, ...)), and use 'iterpool' for any
temporary allocations within the loop and clear it each time round. In
this loop there aren't any, so it's redundant.
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *missing_abspath = svn__apr_hash_index_key(hi);
+
+ missing_subtree_err_str = apr_psprintf(
+ scratch_pool, %s%s\n,
+ missing_subtree_err_str ? missing_subtree_err_str : ,
+ svn_dirent_local_style(missing_abspath, scratch_pool));
In most realistic cases this way of constructing the string is fine, but
it looks like it's quadratic in time and memory space which might become
a problem when a merge is attempted into a pathological tree with
several thousand individual missing subtree roots.
Off the top of my head I would look at accumulating the string in an
svn_stringbuf, which will expand and occasionally re-alloc as you append
to it rather than re-allocating every time, and which remembers its
length so appending is quick.
Ditto.
+ }
+
+ if (missing_subtree_err_str)
Minor point: Either this if or the if (apr_hash_count ...) is
redundant.
Gone.
- Julian
On Fri, Sep 3, 2010 at 7:43 AM, Philip Martin
philip.mar...@wandisco.com wrote:
pbu...@apache.org writes:
Author: pburba
Date: Thu Sep 2 18:10:01 2010
New Revision: 992042
@@ -5718,6 +5849,37 @@ get_mergeinfo_paths(apr_array_header_t *
merge_cmd_baton-ctx-cancel_baton,
scratch_pool));
+ if (apr_hash_count(wb.missing_subtrees))
+{
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
../src/subversion/libsvn_client/merge.c: In function
Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES)
On Fri, Sep 3, 2010 at 04:46, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Greg Stein [mailto:gst...@gmail.com] Sent: vrijdag 3 september 2010 1:14 To: Julian Foad Cc: Erik Huelsmann; dev@subversion.apache.org Subject: Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES) On Thu, Sep 2, 2010 at 18:39, Julian Foad julian.f...@wandisco.com ... We need to describe how the layering works for copies, deletes, and adds. In particular I'm recalling something about how local adds aren't recursive, unlike copies, so an additional change within an added dir doesn't work the same way with regard to op_depth as it would inside a copied dir. Adds within other adds don't layer. You're simply adding more nodes at the parent's op_depth. Deleting an ancestor of a deleted node also does not create an additional layer. Instead, we establish the new op_depth and relabel all of the deleted children with that new op_depth so that it looks like one big happy delete operation. Copies/moves always introduce a new layer. I'm not sure if I follow your reasoning here. (layering vs op_depths) I think local additions should always be their own op (and have their own op_depth). That would simplify the revert and commit process: They would only have to do restructuring changes(add/delete) when there is a new op_depth. A local add is not a restructuring change. It is just adding new nodes into the tree. You can revert any of those nodes -- you are not required to revert the root of an add. You *do* have to go to the root of a copy/move to revert it, however. There is no way to revert the deletion of a child, so (again) you must revert a deletion at its op-root. So the commit harvester would be as simple as walking the operations (for restructuring operations) and checking present nodes for text and prop mods. And this leads me to think that you're confusing add with copy/move. Adds never have text/prop mods. The text and props are ALWAYS new. IOW, fix your terminology and then I can tell whether we agree or not. ... Cheers, -g
Re: Worried about single-db performance
On Fri, Sep 3, 2010 at 06:09, Johan Corveleyn jcor...@gmail.com wrote: Hi devs, From what I understand about the performance problems of WC-1 vs. WC-NG, and what I'm reading on this list, I expect(ed) a huge performance boost from WC-NG for certain client operations (especially on Windows, where the locking of WC-1 is quite problematic). Also, I knew I had to wait for single-db to see any real performance benifits. So after you guys switched on single-db, I eagerly gave trunk a spin... Now I'm a little worried, because I don't see any great speed increases (quite the contrary). Some details below. Maybe it's just too early to be looking at this (maybe it's a simple matter of optimizing the data model, adding indexes, optimizing some code paths, ...). So it's fine if you guys say: chill, just wait a couple more weeks. I just need to know whether I should be worried or not :-). It should already be faster. Obviously, that's not the case. My expectation is that it would be faster, and then we'd do some perf improvements to make it even faster. Sounds like we definitely have to do some of those other improvements. We have a schema change to make, and once that is done, then we can start looking at the performance. There could be lots of SQL queries that need to be optimized. I also *know* that we issue way too many queries. There should be ways to avoid a lot of those queries. I'd like to avoid any caching, and rely on SQLite to maintain in-memory caches. My gut says we just need to reduce the number of queries. Cheers, -g
Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES)
On Thu, Sep 2, 2010 at 22:51, Hyrum K. Wright hyrum_wri...@mail.utexas.edu wrote: ... My one concern (and perhaps this comes from not following the discussion closely enough) is how this impacts 1.7. This feels eerily like an eleventh-hour redesign, and our track record with these in the Nope. This has been on deck for months, but just hasn't happened in force until now. This schema change is required to support an add within a copy. (along with enabling other stuff like better reverts) ... Cheers, -g
Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES)
On Fri, Sep 3, 2010 at 10:41 AM, Greg Stein gst...@gmail.com wrote: On Thu, Sep 2, 2010 at 22:51, Hyrum K. Wright hyrum_wri...@mail.utexas.edu wrote: ... My one concern (and perhaps this comes from not following the discussion closely enough) is how this impacts 1.7. This feels eerily like an eleventh-hour redesign, and our track record with these in the Nope. This has been on deck for months, but just hasn't happened in force until now. This schema change is required to support an add within a copy. (along with enabling other stuff like better reverts) Sure, but is this bring us back to parity with 1.6 work, or is it new stuff we can do with wc-ng work? If the former, it's certainly a release blocker. If the latter, I'm not so sure -Hyrum
RE: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES)
-Original Message- From: Greg Stein [mailto:gst...@gmail.com] Sent: vrijdag 3 september 2010 17:23 To: Bert Huijben Cc: Julian Foad; Erik Huelsmann; dev@subversion.apache.org Subject: Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES) On Fri, Sep 3, 2010 at 04:46, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Greg Stein [mailto:gst...@gmail.com] Sent: vrijdag 3 september 2010 1:14 To: Julian Foad Cc: Erik Huelsmann; dev@subversion.apache.org Subject: Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES) On Thu, Sep 2, 2010 at 18:39, Julian Foad julian.f...@wandisco.com ... We need to describe how the layering works for copies, deletes, and adds. In particular I'm recalling something about how local adds aren't recursive, unlike copies, so an additional change within an added dir doesn't work the same way with regard to op_depth as it would inside a copied dir. Adds within other adds don't layer. You're simply adding more nodes at the parent's op_depth. Deleting an ancestor of a deleted node also does not create an additional layer. Instead, we establish the new op_depth and relabel all of the deleted children with that new op_depth so that it looks like one big happy delete operation. Copies/moves always introduce a new layer. I'm not sure if I follow your reasoning here. (layering vs op_depths) I think local additions should always be their own op (and have their own op_depth). That would simplify the revert and commit process: They would only have to do restructuring changes(add/delete) when there is a new op_depth. A local add is not a restructuring change. It is just adding new nodes into the tree. You can revert any of those nodes -- you are not required to revert the root of an add. You *do* have to go to the root of a copy/move to revert it, however. There is no way to revert the deletion of a child, so (again) you must revert a deletion at its op-root. So the commit harvester would be as simple as walking the operations (for restructuring operations) and checking present nodes for text and prop mods. And this leads me to think that you're confusing add with copy/move. Adds never have text/prop mods. The text and props are ALWAYS new. The commit harvester should be something like: Phase 1: Walk the tree (limited by depth) to find restructuring operations that must be committed. (Handle delete + copy + add (+ in 1.8 move)). Some of these operations will apply deeper then the limited depth (See issue #3699 and others), but only their operations are handled in these step: not local content changes. (This phase should see each local added directory/file as a separate operation, or a svn ci --depth empty DIR would commit all added children. Copies are one operation for the entire tree) Phase 2: Walk the tree (limited by depth) to find which nodes that have a pristine version with text or props that differs from the current version. (adds are ignored here; already handled in step 1). This would leave all operations outside the selected depth in the working copy for committing later; a much saner operation then the current behavior where an add or copy turns a depth limited operation in a recursive commit, including all the local modifications deeper in the tree. Bert
race condition
From IRC logs... 09:53 @danielsh wayita: tell artagnon http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-08-20#l83 --- has that been resolved? (can't find any record in mail/irc archives) So, do you remember what that race condition is? And whether it's been resolved? Daniel
Re: svn commit: r987513 - /subversion/trunk/subversion/svnrdump/svnrdump.c
Ping. Was the 'race condition' I mentioned in my other mail related to
'svnrdump load' setting revprop (which may fail if a hook hadn't been
set up)?
Bert Huijben wrote on Fri, Aug 20, 2010 at 10:55:44 -0700:
-Original Message-
From: artag...@apache.org [mailto:artag...@apache.org]
Sent: vrijdag 20 augustus 2010 7:09
To: comm...@subversion.apache.org
Subject: svn commit: r987513 -
/subversion/trunk/subversion/svnrdump/svnrdump.c
Author: artagnon
Date: Fri Aug 20 14:08:42 2010
New Revision: 987513
URL: http://svn.apache.org/viewvc?rev=987513view=rev
Log:
* subversion/svnrdump/svnrdump.c
(load_cmd): Check that we can set revprops before attempting to
drive the load editor.
Modified:
subversion/trunk/subversion/svnrdump/svnrdump.c
Modified: subversion/trunk/subversion/svnrdump/svnrdump.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/svnr
dump.c?rev=987513r1=987512r2=987513view=diff
==
--- subversion/trunk/subversion/svnrdump/svnrdump.c (original)
+++ subversion/trunk/subversion/svnrdump/svnrdump.c Fri Aug 20 14:08:42
2010
@@ -393,6 +393,17 @@ static svn_error_t *
load_cmd(apr_getopt_t *os, void *baton, apr_pool_t *pool)
{
opt_baton_t *opt_baton = baton;
+ svn_boolean_t can_set_revprops = FALSE;
+
+ SVN_ERR(svn_ra_has_capability(opt_baton-session,
can_set_revprops,
+commit-revprops, pool));
+ if(!can_set_revprops)
+return
+ svn_error_create
+ (SVN_ERR_REPOS_DISABLED_FEATURE, NULL,
+ _(Repository has not been enabled to accept revision
propchanges;\n
+ ask the administrator to create a pre-revprop-change hook));
+
The capability of committing revprops is unrelated to whether we can change
revision properties.
In 1.5 we added the option to add a list of revision properties to set
directly at commit, and this is what you check here. If you are committing to
older repositories you can just set the revision properties after the commit
(like you do for svn:author and svn:date).
And if you get a failure on doing that, then you should show this error.
Bert
Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES)
Hyrum K. Wright hyrum_wri...@mail.utexas.edu writes: Sure, but is this bring us back to parity with 1.6 work, or is it new stuff we can do with wc-ng work? If the former, it's certainly a release blocker. If the latter, I'm not so sure We currently have a regression from 1.6, wc-ng cannot record the revert-base when a child of a copied directory is replaced: svn cp wc/A wc/X svn rm wc/X/f svn cp wc/g wc/X/f # 1.7 has only one row to store text-base and revert-base -- Philip
Cherry picking changes from the performance branch
As I recall, Stefan recently declared the performance branch done. It's encouraging to see a few intrepid users and devs looking at the branch and providing feedback. Through IRC and other conversations, I've gotten the feeling that some of the changes made on the branch might be a bit too wide-spread to warrant whole-sale inclusion in 1.7, but several people have expressed interested in cherry picking at least some bits back to trunk. I've not yet done a comprehensive review of the changes on the branch, and would appreciate any suggestions folks may have of low-hanging, independent useful bits. For starters, I would consider: * the new svn_io_file_read_full2() API * the new svn_io_file_putc() API * the new svn_stringbuf_appendbyte() API Note that I don't include the caching work, since I think it might be much more far-reaching, and probably needs more review before going into trunk. The branch should also probably have a catch-up merge to prevent it from diverging too far. I'm happy to do so, as well as fix up any style nits which may exist on the branch. I'll do some digging to determine the various revision ranges to make the above suggestions merge to trunk, and post back. -Hyrum
Re: race condition
Hi Daniel, Daniel Shahaf writes: From IRC logs... 09:53 @danielsh wayita: tell artagnon http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-08-20#l83 --- has that been resolved? (can't find any record in mail/irc archives) So, do you remember what that race condition is? And whether it's been resolved? I've been a little preoccupied lately with some kernel and Git patches et al. I'll restart work on svnrdump this weekend. Also, I have some exams coming up next week, so I don't know how much time I'll be able to give after this weekend :| Thanks for the ping. -- Ram
Re: svn commit: r987513 - /subversion/trunk/subversion/svnrdump/svnrdump.c
Hi Daniel, Daniel Shahaf writes: Ping. Was the 'race condition' I mentioned in my other mail related to 'svnrdump load' setting revprop (which may fail if a hook hadn't been set up)? Yep. I have to try setting some dummy revprop and barf out quickly before getting caught up in between real work like svnsync does. -- Ram
Re: svn commit: r992390 - in /subversion/trunk/subversion: include/private/svn_sqlite.h libsvn_subr/sqlite.c libsvn_wc/wc_db.c libsvn_wc/wc_db.h
rhuij...@apache.org wrote on Fri, Sep 03, 2010 at 17:34:52 -:
+ for (i = 0; i db-nbr_statements; i++)
+if (db-prepared_stmts[i] db-prepared_stmts[i]-needs_reset)
+ err2 = svn_error_compose_create(
+ err2,
+ svn_sqlite__reset(db-prepared_stmts[i]));
+
+ err2 = svn_error_compose_create(
+ exec_sql(db, release_stmt),
+ err2);
Should the second err2 = ... line be inside the if, or not?
If yes, need { } block.
If not, indentation is wrong.
Re: Do we want 'svn patch' to be able to add empty files?
On Sep 3, 2010, at 7:10 AM, Daniel Näslund wrote: On Fri, Sep 03, 2010 at 12:18:37PM +0200, Branko Čibej wrote: On 02.09.2010 10:50, Branko Čibej wrote: Hmm, this is interesting. :) Git faithfully (blindly?) interprets Unix permission bits, whiles SVN faithfully (blindly?) interprets the contents of special files ... I wonder if svn patch does the right thing here? Anyway, for the sake of interoperability, we'd have to emit and parse the git format for symlinks. Not that I'm too amused by the idea that git probably just does a chmod on the new file without thinking about it, but hey, All the World is Linux, right? :) Did some testing ... apparently git apply completely ignores the permission bits new file mode ... line, at least I haven't been able to force it to do anything with them. From builtin/apply::try_create_file() in the git source code: fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode 0100) ? 0777 : 0666); if (fd 0) return -1; Git only checks for the executable bit, AFAIK. Correct, git and hg only store files as 0644 or 0755. Everything else gets handled by the umask on the user's machine. Daniel
Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES)
On Fri, Sep 3, 2010 at 11:46, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Greg Stein [mailto:gst...@gmail.com] Sent: vrijdag 3 september 2010 17:23 To: Bert Huijben Cc: Julian Foad; Erik Huelsmann; dev@subversion.apache.org Subject: Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES) On Fri, Sep 3, 2010 at 04:46, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Greg Stein [mailto:gst...@gmail.com] Sent: vrijdag 3 september 2010 1:14 To: Julian Foad Cc: Erik Huelsmann; dev@subversion.apache.org Subject: Re: [PROPOSAL] WC-NG: merge NODE_DATA, WORKING_NODE and BASE_NODE into a single table (NODES) On Thu, Sep 2, 2010 at 18:39, Julian Foad julian.f...@wandisco.com ... We need to describe how the layering works for copies, deletes, and adds. In particular I'm recalling something about how local adds aren't recursive, unlike copies, so an additional change within an added dir doesn't work the same way with regard to op_depth as it would inside a copied dir. Adds within other adds don't layer. You're simply adding more nodes at the parent's op_depth. Deleting an ancestor of a deleted node also does not create an additional layer. Instead, we establish the new op_depth and relabel all of the deleted children with that new op_depth so that it looks like one big happy delete operation. Copies/moves always introduce a new layer. I'm not sure if I follow your reasoning here. (layering vs op_depths) I think local additions should always be their own op (and have their own op_depth). That would simplify the revert and commit process: They would only have to do restructuring changes(add/delete) when there is a new op_depth. A local add is not a restructuring change. It is just adding new nodes into the tree. You can revert any of those nodes -- you are not required to revert the root of an add. You *do* have to go to the root of a copy/move to revert it, however. There is no way to revert the deletion of a child, so (again) you must revert a deletion at its op-root. So the commit harvester would be as simple as walking the operations (for restructuring operations) and checking present nodes for text and prop mods. And this leads me to think that you're confusing add with copy/move. Adds never have text/prop mods. The text and props are ALWAYS new. The commit harvester should be something like: Phase 1: Walk the tree (limited by depth) to find restructuring operations that must be committed. (Handle delete + copy + add (+ in 1.8 move)). Some of these operations will apply deeper then the limited depth (See issue #3699 and others), but only their operations are handled in these step: not local content changes. (This phase should see each local added directory/file as a separate operation, or a svn ci --depth empty DIR would commit all added children. Copies are one operation for the entire tree) Phase 2: Walk the tree (limited by depth) to find which nodes that have a pristine version with text or props that differs from the current version. (adds are ignored here; already handled in step 1). This would leave all operations outside the selected depth in the working copy for committing later; a much saner operation then the current behavior where an add or copy turns a depth limited operation in a recursive commit, including all the local modifications deeper in the tree. How confusing are you trying to make this? You're succeeding wonderfully. We are talking about *adds* and their op_depth value. What the fuck does that have to do with commits? All adds are very simple commit operations. They can all have the same op_depth value. There are no restrictions on adding more or removing them. They are free floating nodes within the various restructuring that copies/moves may have done. You can independently revert any single add. Get back to the original question: adds do not require variant op_depth values. Very simple. Any add under an added parent just uses that parent's op_depth. Cheers, -g
