I'm not following this thread, but here are some observations.
On Tue, 2008-09-16 at 15:12 +0400, Danil Shopyrin wrote:
> Here is the improved version of the original patch. Generally spoken,
> the song remains the same. There are only following changes:
> 1) things become more optimized:
> a) we don't event call svn_client__get_wc_mergeinfo() for dst if src has
> explicit mergeinfo set on it
> b) we don't call svn_client__get_wc_mergeinfo() second time for src if it's
> a 'mergeinfo affecting' copy
> 2) there is one more test
> 3) propagate_mergeinfo_within_wc()'s logic modified more carefully. We
> don't check for 'mergeinfo NON-affecting' copy for a locally
> added/replaced PAIR->src without history.
>
> The new log message is as follows:
> [[[
> Don't create any explicit (including empty) mergeinfo record during WC-to-WC
> copy if it's a 'mergeinfo NON-affecting' copy. WC-to-WC copy can be
> considered as 'megeinfo NON-affecting' if and only if the following
> conditions are satisfied:
> a) src and dst are located below the same URL
> b) mergeinfo (if any) for src and dst is inherited from this URL
> c) inherited mergeinfos (if any) for src and dst are equal.
I noticed that those conditions sound a bit redundant and not
crystal-clear. As I understand it, mergeinfo can only be inherited
as-is, not with modifications. Therefore doesn't (c) follow necessarily
from (b)? (And (a) is true for all nodes in a repository, and only
relevant as the referent of condition (b).)
Would the conditions be better said as:
- The destination node must inherit its mergeinfo from the same node
as the source node did.
- To know this, it is sufficient to determine that both chains of
inheritance pass through a common node.
> Index: subversion/libsvn_client/mergeinfo.h
> ===================================================================
> --- subversion/libsvn_client/mergeinfo.h (revision 33072)
> +++ subversion/libsvn_client/mergeinfo.h (working copy)
> @@ -83,7 +83,19 @@
> (ignored if NULL) or beyond any switched path.
>
> Set *WALKED_PATH to the path climbed from WCPATH to find inherited
> - mergeinfo, or "" if none was found. (ignored if NULL). */
> + mergeinfo, or "" if none was found. (ignored if NULL).
(I know you didn't change that sentence, but I'll note in passing that
it's not clear to me how a "path climbed" would be represented in a
string.)
> + Set *EFFECTIVE_MERGEINFO to the original (not adjusted) mergeinfo found on
> + WCPATH or one of its ancestors. Set *EFFECTIVE_MERGEINFO to NULL if there is
> + no mergeinfo found. (Ignore if EFFECTIVE_MERGEINFO is NULL).
The word "effective" does not convey the meaning of "original (not
adjusted)". "original" might be better.
> + Set *EFFECTIVE_URL to the last url scanned for mergeinfo. It can be the
> + url of WCPATH (in the case of explicit mergeinfo) or one of its ancestors
> + (in the case of inherited mergeinfo).
> +
> + Don't climb through mixed revision working copy if ALLOW_MIXED_REVISIONS
> + is FALSE.
If it does climb through a mixed-revision working copy, that contradicts
a paragraph higher up in the doc string (not visible in this diff). If
it doesn't climb through a mixed-revision working copy, what does it do?
> + */
> svn_error_t *
> svn_client__get_wc_mergeinfo(svn_mergeinfo_t *mergeinfo,
> svn_boolean_t *inherited,
> @@ -93,7 +105,10 @@
> const char *wcpath,
> const char *limit_path,
> const char **walked_path,
> + svn_mergeinfo_t *effective_mergeinfo,
> + const char **effective_url,
> svn_wc_adm_access_t *adm_access,
> + svn_boolean_t allow_mixed_revisions,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
I wonder if this API is being made more complex than is necessary. Might
it be better to split it, because the other callers don't want these new
facilities? (Do you really need to know the original mergeinfo? If not,
that would simplify the patch a bit too.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-16 15:47:19 CEST