You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <ch...@gmail.com> on 2005/11/14 13:31:12 UTC
Re: svn commit: r17328 - branches/wc-propcaching/subversion/libsvn_wc
Peter,
I have some comments on your commit. Please check it inline. I hope it
helps you.
On 11/14/05, lundblad@tigris.org <lu...@tigris.org> wrote:
> Author: lundblad
> Date: Mon Nov 14 06:32:32 2005
> New Revision: 17328
>
> Modified:
> branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c
> branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c
> branches/wc-propcaching/subversion/libsvn_wc/copy.c
> branches/wc-propcaching/subversion/libsvn_wc/log.c
> branches/wc-propcaching/subversion/libsvn_wc/props.c
> branches/wc-propcaching/subversion/libsvn_wc/props.h
> branches/wc-propcaching/subversion/libsvn_wc/update_editor.c
>
> Log:
> On wc-propcaching branch: Stop storing a working props file unless there
> are property modifications and stop storing a base props file unless there are
> any properties. Use the prop-mods flag in the WC entry to determine
> which file to load when reading working properties.
>
> * subversion/libsvn_wc/props.c
> (svn_wc__load_props): New function.
> (svn_wc__install_props): Make sure prop files don't exist when they
> shouldn't.
> (svn_wc__merge_props, svn_wc_prop_list): Use svn_wc__load_props instead of
> loading props manually.
> (svn_wc__merge_prop_diffs): Add TODO to revisit this later.
> (svn_wc__has_props): Check baseprops if we're in a enough recent WC
> and there are no property modifications according to the entry.
> Add TODO for dberlin.
> (svn_wc_props_modified_p): Add TODO item.
> (svn_wc_get_prop_diffs): Use svn_wc__load_props to read properties.
>
> * subversion/libsvn_wc/props.h (svn_wc__load_props): Declare.
>
> * subversion/libsvn_wc/copy.c
> (copy_file_administratively): Use svn_wc__load_props().
>
> * subversion/libsvn_wc/adm_crawler.c
> (svn_wc_transmit_prop_deltas): Return early if there are no prop mods.
>
> * subversion/libsvn_wc/log.c (log_do_committed): Remove working props file.
>
> * subversion/libsvn_wc/adm_ops.c
> (revert_admin_things): Use svn_wc__install_props() to revert the props.
> This means we will always load the base props into memory, but we will
> have to do that anyway when we have flags for the presence of individual
> properties.
> Fix misspelled comment.
>
> * subversion/libsvn_wc/update_editor.c
> (install_added_props): Use svn_wc__install_props() to do most of the work
> for regular props.
>
> Modified: branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c
> Url: http://svn.collab.net/viewcvs/svn/branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c?rev=17328&p1=branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c&p2=branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c&r1=17327&r2=17328
> ==============================================================================
> --- branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c (original)
> +++ branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c Mon Nov 14 06:32:32 2005
> @@ -42,6 +42,7 @@
> #include "props.h"
> #include "translate.h"
> #include "entries.h"
> +#include "lock.h"
>
> #include "svn_private_config.h"
>
> @@ -896,6 +897,11 @@
>
> /* Get the right access baton for the job. */
> SVN_ERR (svn_wc_adm_probe_retrieve (&adm_access, adm_access, path, pool));
> +
> + /* For an enough recent WC, we can have a really easy out. */
> + if (svn_wc__adm_wc_format (adm_access) > SVN_WC__NO_PROPCACHING_VERSION
> + && ! entry->prop_mods)
> + return SVN_NO_ERROR;
You leave variable *tempfile uninitialized. But it only begin of
problems, if you fix this you have problems in log_do_commited when
commiting clearing of props, because tempfile doesn't exists and
log_do_commited thinks that there is no props changes!
>
> /* First, get the prop_path from the original path */
> SVN_ERR (svn_wc__prop_path (&props, path, entry->kind, FALSE, pool));
>
> Modified: branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c
> Url: http://svn.collab.net/viewcvs/svn/branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c?rev=17328&p1=branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c&p2=branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c&r1=17327&r2=17328
> ==============================================================================
> --- branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c (original)
> +++ branches/wc-propcaching/subversion/libsvn_wc/adm_ops.c Mon Nov 14 06:32:32 2005
> @@ -1349,6 +1349,7 @@
> apr_hash_t *modify_entry_atts = apr_hash_make (pool);
> svn_stringbuf_t *log_accum = svn_stringbuf_create ("", pool);
> const char *bprop, *rprop, *wprop; /* full paths */
> + apr_hash_t *baseprops;
> const char *adm_path = svn_wc_adm_access_path (adm_access);
>
> /* Build the full path of the thing we're reverting. */
> @@ -1377,27 +1378,13 @@
>
> /* Get the full list of property changes and see if any magic
> properties were changed. */
> - SVN_ERR (svn_wc_get_prop_diffs (&propchanges, NULL, fullpath,
> + SVN_ERR (svn_wc_get_prop_diffs (&propchanges, &baseprops, fullpath,
> adm_access, pool));
>
> /* Determine if any of the propchanges are the "magic" ones that
> might require changing the working file. */
> target_needs_retranslation = svn_wc__has_magic_property (propchanges);
>
> - /* There may be a base props file but no working props file, if
> - the mod was that the working file was `R'eplaced by a new
> - file with no props.
> -
> - If there is a pristing property file, copy it out as the
> - working property file, else just remove the working property
> - file. */
> - SVN_ERR (svn_wc__loggy_copy (&log_accum, NULL,
> - adm_access, svn_wc__copy_normal,
> - bprop, wprop, TRUE, pool));
> -
> - apr_hash_set (modify_entry_atts, SVN_WC__ENTRY_ATTR_PROP_MODS,
> - APR_HASH_KEY_STRING, "false");
> - /* ### TODO: Update properties flags when we have such flags. */
> }
> else if (entry->schedule == svn_wc_schedule_replace)
> {
> @@ -1413,14 +1400,14 @@
> svn_wc_props_modified_p is deliberately ignoring the
> base-props; it's "no" answer simply means that there are no
> working props. It's *still* possible that the base-props
> - exist, however, from the original replaced file. If they do,
> - then we need to restore them. */
> - SVN_ERR (svn_wc__loggy_copy (&log_accum, &tgt_modified,
> - adm_access, svn_wc__copy_normal,
> - bprop, wprop, FALSE, pool));
> + exist, however, from the original replaced file. */
> + baseprops = apr_hash_make (pool);
> + SVN_ERR (svn_wc__load_prop_file (rprop, baseprops, pool));
> }
>
> - /* ### TODO: Update props flags when we have them. */
> + if (modified_p || entry->schedule == svn_wc_schedule_replace)
> + SVN_ERR (svn_wc__install_props (&log_accum, adm_access, name, baseprops,
> + baseprops, FALSE, pool));
I like this change! :)
> Modified: branches/wc-propcaching/subversion/libsvn_wc/props.c
> Url: http://svn.collab.net/viewcvs/svn/branches/wc-propcaching/subversion/libsvn_wc/props.c?rev=17328&p1=branches/wc-propcaching/subversion/libsvn_wc/props.c&p2=branches/wc-propcaching/subversion/libsvn_wc/props.c&r1=17327&r2=17328
> ==============================================================================
> --- branches/wc-propcaching/subversion/libsvn_wc/props.c (original)
> +++ branches/wc-propcaching/subversion/libsvn_wc/props.c Mon Nov 14 06:32:32 2005
> @@ -1721,6 +1802,8 @@
> svn_boolean_t is_empty;
> const char *prop_path;
> const svn_wc_entry_t *entry;
> + svn_boolean_t has_propcaching =
> + svn_wc__adm_wc_format (adm_access) > SVN_WC__NO_PROPCACHING_VERSION;
>
> SVN_ERR (svn_wc_entry (&entry, path, adm_access, FALSE, pool));
>
> @@ -1732,7 +1815,13 @@
> return SVN_NO_ERROR;
> }
>
> - SVN_ERR (svn_wc__prop_path (&prop_path, path, entry->kind, FALSE, pool));
> + if (has_propcaching && ! entry->prop_mods)
> + SVN_ERR (svn_wc__prop_base_path (&prop_path, path, entry->kind, FALSE, pool));
> + else
> + SVN_ERR (svn_wc__prop_path (&prop_path, path, entry->kind, FALSE, pool));
> + /* ### TODO: (dberlin) When we have property flags in the entry, we can
> + know for sure that we *have* props if any flag is set.
> + This *might* not be enough. */
> SVN_ERR (empty_props_p (&is_empty, prop_path, pool));
>
> if (is_empty)
For present time svn_wc__has_props() used only in one place, in
assemble_status() mostyle for optimize call to svn_wc__is_special(). I
consider we should reconsider later necessity of this function.
--
Ivan Zhakov
Re: svn commit: r17328 - branches/wc-propcaching/subversion/libsvn_wc
Posted by Ivan Zhakov <ch...@gmail.com>.
On 11/14/05, Daniel Berlin <db...@dberlin.org> wrote:
> > >
> > > if (is_empty)
> > For present time svn_wc__has_props() used only in one place, in
> > assemble_status() mostyle for optimize call to svn_wc__is_special(). I
> > consider we should reconsider later necessity of this function.
>
> You'll notice i made is_special use the cached props in the
> has_properties patch, so when that goes in, it shouldn't be any speed
> gain to not use svn_wc__is_special.
Yes, I see. This is because I was noticed it. I am only said that we
need TODO comment in svn_wc__has_props() because possible we didn't
this function.
--
Ivan Zhakov
Re: svn commit: r17328 -
branches/wc-propcaching/subversion/libsvn_wc
Posted by Daniel Berlin <db...@dberlin.org>.
> >
> > if (is_empty)
> For present time svn_wc__has_props() used only in one place, in
> assemble_status() mostyle for optimize call to svn_wc__is_special(). I
> consider we should reconsider later necessity of this function.
You'll notice i made is_special use the cached props in the
has_properties patch, so when that goes in, it shouldn't be any speed
gain to not use svn_wc__is_special.
>
> --
> Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17328 - branches/wc-propcaching/subversion/libsvn_wc
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 14 Nov 2005, Ivan Zhakov wrote:
> Peter,
I have some comments on your commit. Please check it inline. I hope it
helps you.
On 11/14/05, lundblad@tigris.org <lu...@tigris.org> wrote:
> Author: lundblad
> Date: Mon Nov 14 06:32:32 2005
> Modified: branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c
> Url: http://svn.collab.net/viewcvs/svn/branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c?rev=17328&p1=branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c&p2=branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c&r1=17327&r2=17328
> ==============================================================================
> --- branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c (original)
> +++ branches/wc-propcaching/subversion/libsvn_wc/adm_crawler.c Mon Nov 14 06:32:32 2005
> @@ -42,6 +42,7 @@
> #include "props.h"
> #include "translate.h"
> #include "entries.h"
> +#include "lock.h"
>
> #include "svn_private_config.h"
>
> @@ -896,6 +897,11 @@
>
> /* Get the right access baton for the job. */
> SVN_ERR (svn_wc_adm_probe_retrieve (&adm_access, adm_access, path, pool));
> +
> + /* For an enough recent WC, we can have a really easy out. */
> + if (svn_wc__adm_wc_format (adm_access) > SVN_WC__NO_PROPCACHING_VERSION
> + && ! entry->prop_mods)
> + return SVN_NO_ERROR;
>You leave variable *tempfile uninitialized. But it only begin of
Good catch! Fixed in r17348.
>problems, if you fix this you have problems in log_do_commited when
>commiting clearing of props, because tempfile doesn't exists and
>log_do_commited thinks that there is no props changes!
But in this case, there were no property modifications, so is that a real
problem? The function now fullfills its API.
...
> - /* ### TODO: Update props flags when we have them. */
> + if (modified_p || entry->schedule == svn_wc_schedule_replace)
> + SVN_ERR (svn_wc__install_props (&log_accum, adm_access, name, baseprops,
> + baseprops, FALSE, pool));
>I like this change! :)
Thanks:-) It is in line with your envisioned property abstraction.
...
>For present time svn_wc__has_props() used only in one place, in
>assemble_status() mostyle for optimize call to svn_wc__is_special(). I
>consider we should reconsider later necessity of this function.
It actually has a semantic difference as well. The property status is set
to none if there are no properties at all, and normal if there or no
propmods. See TODO on the branch for how I thihnk we should handle this.
Thanks for the review!
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org