Are you sure that it doesn't return the mergeinfo as it applies to the target, even if it has found that information by looking at the parent? (That is what I tried to say)
I'm not familiar enough with this code to really review it from just the code, but this eliding code is quite sensitive and isn't that well tested (as in general it doesn't affect merge results for common merge patterns.. and our test mostly just check for changed nodes, not the actual property changes).
In general we should try not to add svn:mergeinfo in more places than necessary to record the actual merges and I was(/am?) afraid that this might happen as a side effect of this merge. The result would be that all new merges to the same tree would be slower, while the eliding process was added to make it faster if a catch-up merge made mergeinfo match the inherited mergeinfo.
(Perhaps this last part makes your patch safe… I just find it hard to verify. And hard to tell where the performance costs are)
Bert
Sent from Windows Mail
From: Julian Foad
Sent: Thursday, May 1, 2014 1:12 PM
To: Bert Huijben
Cc: dev_at_subversion.apache.org
Bert Huijben wrote:
> This might make us add svn:mergeinfo on nodes that didn't have this property
> before eliding, while the old code tried to avoid that by checking to see if
> the value was inherited from an ancestor.
Hi Bert. I can't quite parse your sentence unambiguously, but I believe this change has no functional effect.
In the case where this node (at target_abspath) has no explicit mergeinfo, this code said before my change:
set 'target_mergeinfo' to <inherited mergeinfo, if any found, else null>
set 'inherited' to <true, if inherited mergeinfo found, else false>
if (inherited || target_mergeinfo == NULL) return
Therefore it would always return at this point.
Now, in the same case, after my change, the code says:
set 'target_mergeinfo' to <null>
if (target_mergeinfo == NULL) return
So, again, it will always return at this point.
The only way this 'target_mergeinfo' value can propagate past that 'if' statement is if it is explicit mergeinfo. That's the case both before and after my change.
- Julian
>URL: http://svn.apache.org/r1591301
>Log:
>* subversion/libsvn_client/mergeinfo.c
> (svn_client__elide_mergeinfo): A tiny simplification: when we want only
> explicit mergeinfo, ask for only explicit mergeinfo.
>
>Modified:
> subversion/trunk/subversion/libsvn_client/mergeinfo.c
>
>Modified: subversion/trunk/subversion/libsvn_client/mergeinfo.c
>==============================================================================
>--- subversion/trunk/subversion/libsvn_client/mergeinfo.c (original)
>+++ subversion/trunk/subversion/libsvn_client/mergeinfo.c Wed Apr 30 14:12:08 2014
>@@ -922,13 +922,12 @@ svn_client__elide_mergeinfo(const char *
> {
> svn_mergeinfo_t target_mergeinfo;
> svn_mergeinfo_t mergeinfo = NULL;
>- svn_boolean_t inherited;
> const char *walk_path;
> svn_error_t *err;
>
> /* Get the TARGET_WCPATH's explicit mergeinfo. */
>- err = svn_client__get_wc_mergeinfo(&target_mergeinfo, &inherited,
>- svn_mergeinfo_inherited,
>+ err = svn_client__get_wc_mergeinfo(&target_mergeinfo, NULL,
>+ svn_mergeinfo_explicit,
> target_abspath,
> limit_abspath,
> &walk_path, FALSE,
>@@ -951,7 +950,7 @@ svn_client__elide_mergeinfo(const char *
>
> /* If TARGET_WCPATH has no explicit mergeinfo, there's nothing to
> elide, we're done. */
>- if (inherited || target_mergeinfo == NULL)
>+ if (target_mergeinfo == NULL)
> return SVN_NO_ERROR;
>
> /* Get TARGET_WCPATH's inherited mergeinfo from the WC. */
Received on 2014-05-01 14:45:16 CEST