You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by makl <ma...@tigris.org> on 2004/03/14 13:53:24 UTC
[PATCH] [Issue 1751] file not switched if it is the same in source
and destination
[[[
Fix file not switched if it is the same in source and destination
Patch from <ma...@tigris.org>
* subversion/tests/clients/cmdline/switch_tests.py
(main): Remove XFAIL from nonrecursive_switching
(nonrecursive_switching): Extend the test to ensure that
subdirectories are not switched.
* subversion/libsvn_wc/adm_ops.c
(non_recursively_tweak_entries): New function similar to
recursively_tweak_entries but non recursive.
(svn_wc__do_update_cleanup): Call non_recursively_tweak_entries
]]]
Re: [PATCH] [Issue 1751] file not switched if it is the same in source
and destination
Posted by makl <ma...@tigris.org>.
Philip Martin wrote:
> Subversion's standard pool pattern is described in HACKING.
I know. I have only interpreted the code to get the intended behaviour.
> As it happens svn_wc_entries_read is special, the entries are
> allocated from the pool used to open the access baton and not from the
> pool parameter.
Seems I missed this part in the documentation of svn_wc_entries_read.
With this detail in mind my arguments do not hold.
I will post a modified patch in the next days.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 1751] file not switched if it is the same in
source and destination
Posted by Philip Martin <ph...@codematters.co.uk>.
makl <ma...@tigris.org> writes:
>>>+ /* Read DIRPATH's entries. */
>>>+ SVN_ERR (svn_wc_entries_read (&entries, dirpath, TRUE, subpool));
>>
>> I know you copied this code, but the pool handling isn't correct.
>> It should be using pool here, not subpool.
>
> Potentially we read a big bunch of entries and i think the memory should
> be freed as soon as possible. And for the recursive case, after
> completing the operation we have all entries of the complete working
> copy in memory. This sounds not correct for me.
Subversion's standard pool pattern is described in HACKING. The point
of the subpool is to handle the iteration, so it *must* be cleared
within the loop. This means that any memory that must persist from
one iteration to the next cannot be allocated from the subpool. You
don't need to worry about the amount of memory allocated by one-shot,
non-iterative, allocations as it is the callers responsibility to
provide a suitable pool, and clear it if required.
As it happens svn_wc_entries_read is special, the entries are
allocated from the pool used to open the access baton and not from the
pool parameter. In most cases calling svn_wc_entries_read doesn't
allocate any additional memory.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 1751] file not switched if it is the same in source
and destination
Posted by makl <ma...@tigris.org>.
Philip Martin wrote:
> makl <ma...@tigris.org> writes:
> I feel there is too much code duplication, would it be possible to use
> a single tweak_entries function with a recurse flag?
No problem.
>>+ /* Read DIRPATH's entries. */
>>+ SVN_ERR (svn_wc_entries_read (&entries, dirpath, TRUE, subpool));
>
> I know you copied this code, but the pool handling isn't correct.
> It should be using pool here, not subpool.
Potentially we read a big bunch of entries and i think the memory should
be freed as soon as possible. And for the recursive case, after
completing the operation we have all entries of the complete working
copy in memory. This sounds not correct for me.
>> # Check out the trunk as "wc2"
>>- svntest.main.run_svn(None, 'co', '-N', trunk_url, wc2_dir)
>>+ svntest.main.run_svn(None, 'co', trunk_url, wc2_dir)
>
>
> That's quite a change, is this test still testing whatever it was
> supposed to test originally?
Remember the recipe. For this two cases it's irrelevant whether the WC
is checked out recursively or not.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 1751] file not switched if it is the same in source
and destination
Posted by Julian Foad <ju...@btopenworld.com>.
makl wrote:
> Version 2 of the patch.
>
> [[[
> Fix remaining part of issue 1751. File not switched if it is the same
> in source and destination.
>
> Patch from <ma...@tigris.org>
>
> * subversion/tests/clients/cmdline/switch_tests.py
> (main): Remove XFAIL from nonrecursive_switching.
It is called 'test_list', not 'main'.
> (nonrecursive_switching): Extend the test to ensure that
> subdirectories are not switched.
>
> * subversion/libsvn_wc/adm_ops.c
> (tweak_entries): Change of non_recursively_tweak_entries to be
> non recursiv if it is necessary.
It looks like it is 'recursively_tweak_entries' that was changed and renamed. The pool usage was also changed. Perhaps write it like this:
* subversion/libsvn_wc/adm_ops.c
(tweak_entries): Rename from 'recursively_tweak_entries'. Add a
'recurse' flag so that recursion is optional. Fix pool usage.
> (svn_wc__do_update_cleanup): Call tweak_entries.
> ]]]
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 9193)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -98,19 +101,18 @@
> if (base_url)
> child_url = svn_path_url_add_component (base_url, name, subpool);
>
> - /* If a file, or deleted or absent dir, then tweak the entry but
> - don't recurse. */
> + /* If a file, or deleted or absent dir in recursive mode, then tweak the
> + entry but don't recurse. */
> if ((current_entry->kind == svn_node_file)
> - || current_entry->deleted
> - || current_entry->absent)
> + || (recurse && (current_entry->deleted || current_entry->absent)))
> {
> SVN_ERR (svn_wc__tweak_entry (entries, name,
> child_url, new_rev, &write_required,
> svn_wc_adm_access_pool (dirpath)));
> }
>
> - /* If a directory... */
> - else if (current_entry->kind == svn_node_dir)
> + /* If a directory and recursiv... */
"recursive"
The rest of the patch looks OK.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 1751] file not switched if it is the same in source
and destination
Posted by makl <ma...@tigris.org>.
Philip Martin wrote:
> makl <ma...@tigris.org> writes:
>
>
>>Index: subversion/libsvn_wc/adm_ops.c
>>===================================================================
>>--- subversion/libsvn_wc/adm_ops.c (revision 9193)
>>+++ subversion/libsvn_wc/adm_ops.c (working copy)
>
>
>>@@ -200,20 +202,13 @@
>> SVN_ERR (svn_wc_adm_retrieve (&dir_access, adm_access, path, pool));
>>
>> if (! recursive)
>>- {
>>- svn_boolean_t write_required = FALSE;
>>- SVN_ERR (svn_wc_entries_read (&entries, dir_access, TRUE, pool));
>>- SVN_ERR (svn_wc__tweak_entry (entries, SVN_WC_ENTRY_THIS_DIR,
>>- base_url, new_revision, &write_required,
>>- svn_wc_adm_access_pool (dir_access)));
>>- if (write_required)
>>- SVN_ERR (svn_wc__entries_write (entries, dir_access, pool));
>>- }
>>+ SVN_ERR (tweak_entries (dir_access, base_url, new_revision,
>>+ NULL, NULL, FALSE,
>>+ FALSE, pool));
>
>
> Why is this passing FALSE for remove_missing_dirs (and thus NULL for
> the notify stuff)? I'm not saying it's wrong, I just don't understand
> it.
remove_missing_dirs and the notify stuff are only used in the recursive
case. Passing them as FALSE and null, everyone can see at the first
sight that they are irrelevant for the non-recursive case.
I have no problem if you prefer one function call and loosing the
"if(!recursive)".
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 1751] file not switched if it is the same in
source and destination
Posted by Philip Martin <ph...@codematters.co.uk>.
makl <ma...@tigris.org> writes:
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 9193)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -200,20 +202,13 @@
> SVN_ERR (svn_wc_adm_retrieve (&dir_access, adm_access, path, pool));
>
> if (! recursive)
> - {
> - svn_boolean_t write_required = FALSE;
> - SVN_ERR (svn_wc_entries_read (&entries, dir_access, TRUE, pool));
> - SVN_ERR (svn_wc__tweak_entry (entries, SVN_WC_ENTRY_THIS_DIR,
> - base_url, new_revision, &write_required,
> - svn_wc_adm_access_pool (dir_access)));
> - if (write_required)
> - SVN_ERR (svn_wc__entries_write (entries, dir_access, pool));
> - }
> + SVN_ERR (tweak_entries (dir_access, base_url, new_revision,
> + NULL, NULL, FALSE,
> + FALSE, pool));
Why is this passing FALSE for remove_missing_dirs (and thus NULL for
the notify stuff)? I'm not saying it's wrong, I just don't understand
it.
> else
> - SVN_ERR (recursively_tweak_entries (dir_access, base_url,
> - new_revision, notify_func,
> - notify_baton, remove_missing_dirs,
> - pool));
> + SVN_ERR (tweak_entries (dir_access, base_url, new_revision,
> + notify_func, notify_baton, remove_missing_dirs,
> + TRUE, pool));
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 1751] file not switched if it is the same in source
and destination
Posted by makl <ma...@tigris.org>.
Version 2 of the patch.
[[[
Fix remaining part of issue 1751. File not switched if it is the same
in source and destination.
Patch from <ma...@tigris.org>
* subversion/tests/clients/cmdline/switch_tests.py
(main): Remove XFAIL from nonrecursive_switching.
(nonrecursive_switching): Extend the test to ensure that
subdirectories are not switched.
* subversion/libsvn_wc/adm_ops.c
(tweak_entries): Change of non_recursively_tweak_entries to be
non recursiv if it is necessary.
(svn_wc__do_update_cleanup): Call tweak_entries.
]]]
Re: [PATCH] [Issue 1751] file not switched if it is the same in
source and destination
Posted by Philip Martin <ph...@codematters.co.uk>.
makl <ma...@tigris.org> writes:
> [[[
> Fix file not switched if it is the same in source and destination
>
> Patch from <ma...@tigris.org>
>
> * subversion/tests/clients/cmdline/switch_tests.py
> (main): Remove XFAIL from nonrecursive_switching
> (nonrecursive_switching): Extend the test to ensure that
> subdirectories are not switched.
>
> * subversion/libsvn_wc/adm_ops.c
> (non_recursively_tweak_entries): New function similar to
> recursively_tweak_entries but non recursive.
I feel there is too much code duplication, would it be possible to use
a single tweak_entries function with a recurse flag?
> (svn_wc__do_update_cleanup): Call non_recursively_tweak_entries
> ]]]
>
>
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 9041)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -161,6 +161,69 @@
>
>
> +/* The main non-recursive body of svn_wc__do_update_cleanup. */
> +static svn_error_t *
> +non_recursively_tweak_entries (svn_wc_adm_access_t *dirpath,
> + const char *base_url,
> + svn_revnum_t new_rev,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *entries;
> + apr_hash_index_t *hi;
> + apr_pool_t *subpool = svn_pool_create (pool);
> + svn_boolean_t write_required = FALSE;
> +
> + /* Read DIRPATH's entries. */
> + SVN_ERR (svn_wc_entries_read (&entries, dirpath, TRUE, subpool));
I know you copied this code, but the pool handling isn't correct.
It should be using pool here, not subpool.
> +
> + /* Tweak "this_dir" */
> + SVN_ERR (svn_wc__tweak_entry (entries, SVN_WC_ENTRY_THIS_DIR,
> + base_url, new_rev, &write_required,
> + svn_wc_adm_access_pool (dirpath)));
> +
> + /* Recursively loop over all children. */
> + for (hi = apr_hash_first (subpool, entries); hi; hi = apr_hash_next (hi))
Use pool here.
> + {
> + const void *key;
> + void *val;
> + const char *name;
> + svn_wc_entry_t *current_entry;
> + const char *child_url = NULL;
> +
Add svn_pool_clear here.
> + apr_hash_this (hi, &key, NULL, &val);
> + name = key;
> + current_entry = val;
> +
> + /* Ignore the "this dir" entry. */
> + if (! strcmp (name, SVN_WC_ENTRY_THIS_DIR))
> + continue;
> +
> + /* Derive the new URL for the current (child) entry */
> + if (base_url)
> + child_url = svn_path_url_add_component (base_url, name, subpool);
> +
> + /* If a file, or deleted or absent dir, then tweak the entry but
> + don't recurse. */
> + if (current_entry->kind == svn_node_file)
> + {
> + SVN_ERR (svn_wc__tweak_entry (entries, name,
> + child_url, new_rev, &write_required,
> + svn_wc_adm_access_pool (dirpath)));
> + }
> + }
> +
> + /* Write a shiny new entries file to disk. */
> + if (write_required)
> + SVN_ERR (svn_wc__entries_write (entries, dirpath, subpool));
> +
> Index: subversion/tests/clients/cmdline/switch_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/switch_tests.py (revision 9041)
> +++ subversion/tests/clients/cmdline/switch_tests.py (working copy)
> @@ -636,9 +636,12 @@
> wc1_new_file = os.path.join(wc1_dir, 'branch', 'version1', 'newfile')
> wc2_new_file = os.path.join(wc2_dir, 'newfile')
> wc2_mu_file = os.path.join(wc2_dir, 'mu')
> + wc2_B_dir = os.path.join(wc2_dir, 'B')
> + wc2_C_dir = os.path.join(wc2_dir, 'C')
> + wc2_D_dir = os.path.join(wc2_dir, 'D')
>
> # Check out the trunk as "wc2"
> - svntest.main.run_svn(None, 'co', '-N', trunk_url, wc2_dir)
> + svntest.main.run_svn(None, 'co', trunk_url, wc2_dir)
That's quite a change, is this test still testing whatever it was
supposed to test originally?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org