subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bert Huijben <b...@qqmail.nl>
Subject Re: svn commit: r1591301 -/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Date Thu, 01 May 2014 12:36:20 GMT
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@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. */
Mime
View raw message