Dan,
Made your recommended changes (both here and from IRC). Committed
r23998.
Some comments below:
> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Monday, March 19, 2007 12:59 PM
> To: Paul Burba
> Cc: SVN Dev; Kamesh Jayachandran
> Subject: Re: [merge-tracking]: Don't combine repos mergeinfo
> with local mergeinfo if the later is modified.
>
> On Mon, 19 Mar 2007, Paul Burba wrote:
> ...
> > Attached is a patch addressing our recent IRC conversation
> regarding
> > the need to not combine repos mergeinfo with local
> mergeinfo when the
> > latter is modified. Per your suggestion I created a new secondary
> > header file in include/private/.
> ...
>
> Nice Paul! Comments in-line below.
>
> > [[[
> > On the merge-tracking branch: Don't combine repos with local merge
> > info when the latter is modified.
> >
> > * subversion/include/private/svn_wc_private.h
> > New file.
> > (svn_wc__props_modified): Similar to svn_wc_props_modified_p()
> > but also indicates the specific props that are modified and their
> > new values.
> >
> > * build.conf
> > (libsvn_wc): Add private\svn_wc_private.h to msvc-export list.
> >
> > * subversion/libsvn_client/diff.c
> > Include private/svn_wc_private.h
> > (get_wc_and_repos_merge_info): Use
> svn_wc__props_modified() to detect
> > modified local merge info.
> > local changes to merge info. Remove FIXME portion of comment.
>
> I assume the the last line of the log msg for
> get_wc_and_repos_merge_info() is what we intend to use here.
Yes, fixed that in the commit.
> > * subversion/libsvn_wc/props.c
> > (svn_wc__props_modified): New.
> > ]]]
>
> Content-Description: local.mergeinfo.mods.diff.txt
> > Index: build.conf
> > ===================================================================
> > --- build.conf (revision 23893)
> > +++ build.conf (working copy)
> > @@ -328,7 +328,7 @@
> > path = subversion/libsvn_wc
> > libs = libsvn_delta libsvn_diff libsvn_subr aprutil apriconv apr
> > install = lib -msvc-export = svn_wc.h
> > +msvc-export = svn_wc.h private\svn_wc_private.h
> >
> > # Subversion plugin for Apache's mod_dav [mod_dav_svn]
> > Index: subversion/include/private/svn_wc_private.h
> > ===================================================================
> > --- subversion/include/private/svn_wc_private.h (revision 0)
> > +++ subversion/include/private/svn_wc_private.h (revision 0)
> > @@ -0,0 +1,45 @@
> > +/*
> > + * svn_wc_private.h: Declarations for libsvn_wc APIs which are
> > + * only used internally to Subversion.
> > + *
> > + *
> >
> +====================================================================
> > + * Copyright (c) 2000-2007 CollabNet. All rights reserved.
>
> Just 2007 is sufficient.
Noted for future reference. I merged up with trunk before committing
this and so picked up eh's svn_wc_private.h (which of course has the
correct copyright date).
> > + * This software is licensed as described in the file
> COPYING, which
> > + * you should have received as part of this distribution.
> The terms
> > + * are also available at
> http://subversion.tigris.org/license-1.html.
> > + * If newer versions of this license are posted there, you
> may use a
> > + * newer version instead, at your option.
> > + *
> > + * This software consists of voluntary contributions made by many
> > + * individuals. For exact contribution history, see the revision
> > + * history and logs, available at http://subversion.tigris.org/.
> > + *
> > +
> ====================================================================
> > + */
> > +
> > +#ifndef SVN_WC_PRIVATE_H
> > +#define SVN_WC_PRIVATE_H
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif /* __cplusplus */
> > +
> > +/** Set @a *modified_p to non-zero if @a path's properties are
> > +modified
> > + * with regard to the base revision, else set @a
> modified_p to zero.
> > + * If any properties are modified set @a *which_props to hashtable
> > + * (<tt>const char *name</tt> -> <tt>const svn_string_t
> *value</tt>)
> > + * that contains only the modified properties, else @a
> *which_props
> > +is
> > + * set to an empty hash. @a adm_access must be an access baton for
> > + * @a path. @a *which_props is allocated in @a pool.
> > + */
> > +svn_error_t *svn_wc__props_modified(svn_boolean_t *modified_p,
> > + const char *path,
> > + apr_hash_t **which_props,
> > + svn_wc_adm_access_t
> *adm_access,
> > + apr_pool_t *pool);
> > +
>
> We could drop the modified_p flag here, instead using the doc
> string to indicate that *WHICH_PROPS will be set to NULL (or
> an empty hash) when no props are modified.
I dropped modified_p and used WHICH_PROPS as you suggest, though I used
an empty hash rather than NULL if there are no prop changes.
> > +#ifdef __cplusplus
> > +}
> > +#endif /* __cplusplus */
> > +
> > +#endif /* SVN_WC_PRIVATE_H */
> >
> > Property changes on: subversion\include\private\svn_wc_private.h
> > ___________________________________________________________________
> > Name: svn:eol-style
> > + native
> >
> > Index: subversion/libsvn_client/diff.c
> > ===================================================================
> > --- subversion/libsvn_client/diff.c (revision 23893)
> > +++ subversion/libsvn_client/diff.c (working copy)
> > @@ -45,6 +45,7 @@
> > #include "client.h"
> > #include <assert.h>
> >
> > +#include "private/svn_wc_private.h"
> > #include "svn_private_config.h"
> >
> > /* Sanity check -- ensure that we have valid revisions to
> look at. */
> > @@ -1947,6 +1948,9 @@
> >
> > if (repos_mergeinfo != NULL)
> > {
> > + svn_boolean_t modified_props;
> > + apr_hash_t *which_props;
>
> That would do away with this "which_props" local variable.
>
> > +
> > /* If pre-existing local changes reverted some of
> the merge info
> > on TARGET_WCPATH, this should be recorded in the
> WC's merge
> > info as TARGET_WCPATH's complete set of merge
> info minus @@
> > -1958,13 +1962,18 @@
> > using a separate, negative set of merge source -> revision
> > range mappings. */
> >
> > - /* ### FIXME: Avoid combining local merge info with
> repos merge
> > - ### info for TARGET_WCPATH when its merge info has been
> > - ### locally modified. We need a variation of
> > - ### libsvn_wc/props.c:svn_wc_props_modified_p()
> which reports
> > - ### on a single property value. */
> > - SVN_ERR(svn_mergeinfo_merge(target_mergeinfo,
> repos_mergeinfo,
> > - pool));
> > + /* Avoid combining local merge info with repos merge info for
> > + TARGET_WCPATH when its merge info has been
> locally modified. */
> > + SVN_ERR(svn_wc__props_modified(&modified_props,
> target_wcpath,
> > + &which_props, adm_access,
> > + pool));
> > +
> > + if(!modified_props
> > + || !apr_hash_get(which_props, SVN_PROP_MERGE_INFO,
> > + APR_HASH_KEY_STRING))
> > + {
> > + SVN_ERR(svn_mergeinfo_merge(target_mergeinfo,
> repos_mergeinfo,
> > + pool));
> > + }
>
> ...and make sense here for the caller.
>
> > }
> > return SVN_NO_ERROR;
> > }
> > Index: subversion/libsvn_wc/props.c
> > ===================================================================
> > --- subversion/libsvn_wc/props.c (revision 23893)
> > +++ subversion/libsvn_wc/props.c (working copy)
> > @@ -2549,3 +2549,53 @@
> > }
> > return FALSE;
> > }
> > +
> > +svn_error_t *
> > +svn_wc__props_modified(svn_boolean_t *modified_p,
> > + const char *path,
> > + apr_hash_t **which_props,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool) {
> > + SVN_ERR(svn_wc_props_modified_p(modified_p, path, adm_access,
> > +pool));
>
> I'd rather expect svn_wc_props_modified_p() to be calling
> this new routine, rather than vice-versa. If WHICH_PROPS is
> NULL, we could bail out of the old svn_wc_props_modified_p()
> logic earlier.
I created one private helper function in props.c, modified_props()
/* Common implementation for svn_wc_props_modified_p()
and svn_wc__props_modified().
Set *MODIFIED_P to true if PATH's properties are modified
with regard to the base revision, else set MODIFIED_P to false.
If WHICH_PROPS is non-null and there are prop mods then set
*WHICH_PROPS to a (const char *propname) ->
(const svn_string_t *propvalue) key:value mapping of only
the modified properties. */
svn_error_t *
modified_props(svn_boolean_t *modified_p,
const char *path,
apr_hash_t **which_props,
svn_wc_adm_access_t *adm_access,
apr_pool_t *pool)
This function implements both svn_wc_props_modified_p() and
svn_wc__props_modified() but, as you suggested in IRC, behaves
differently depending if which_props is NULL or not.
svn_wc_props_modified_p() and svn_wc__props_modified() are now just thin
wrappers around modified_props().
> > + *which_props = apr_hash_make(pool);
> > +
> > + if (*modified_p)
> > + {
> > + apr_pool_t *subpool = svn_pool_create(pool);
> > + const svn_wc_entry_t *entry;
> > + const char *prop_path, *prop_base_path;
> > + apr_array_header_t *local_propchanges;
> > + apr_hash_t *localprops, *baseprops;
> > + int i;
> > +
> > + SVN_ERR(svn_wc_entry(&entry, path, adm_access, TRUE,
> subpool));
> > +
> > + /* Get the paths of the working and base prop files. */
> > + SVN_ERR(svn_wc__prop_path(&prop_path, path,
> entry->kind, FALSE,
> > + subpool));
> > + SVN_ERR(svn_wc__prop_base_path(&prop_base_path,
> path, entry->kind,
> > + FALSE, subpool));
> > +
> > + /* Load the working and base props into temporary
> hashses. */
> > + localprops = apr_hash_make(subpool);
> > + baseprops = apr_hash_make(subpool);
> > + SVN_ERR(svn_wc__load_prop_file(prop_path,
> localprops, subpool));
> > + SVN_ERR(svn_wc__load_prop_file(prop_base_path, baseprops,
> > + subpool));
> > +
> > + /* Don't use subpool here. */
> > + SVN_ERR(svn_prop_diffs(&local_propchanges, localprops,
> > + baseprops, pool));
> > +
> > + /* Put any modified props in *WHICH_PROPS. */
> > + for (i = 0; i < local_propchanges->nelts; i++)
> > + {
> > + svn_prop_t *propt = &APR_ARRAY_IDX(local_propchanges, i,
> > + svn_prop_t);
> > + apr_hash_set(*which_props, propt->name,
> > + APR_HASH_KEY_STRING, propt->value);
> > + }
> > + svn_pool_destroy(subpool);
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > \ No newline at end of file
>
> Also, is there any overlap with svn_client__get_prop_from_wc()?
To the extent we are getting props yes, but
svn_client__get_prop_from_wc() only gets one prop and doesn't tell us if
it was modified.
> We call svn_client__get_prop_from_wc() from
> svn_client__parse_merge_info(), which is in turn called from
> a loop inside get_wc_merge_info() (meaning we're probably
> doing some redudant work here).
Hmmm, maybe bulk up get_wc_merge_info() so it knows not only if
mergeinfo was inherited, but also if it is locally modfied?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 22 16:37:30 2007