Re: svn commit: r33112 - review for back-port

[Julian Foad <ju...@btopenworld.com>]

On Tue, 2008-09-16 at 17:21 -0700, pburba@tigris.org wrote:
> Author: pburba
> Date: Tue Sep 16 17:21:53 2008
> New Revision: 33112
> 
> Log:
> Fix a bug and greatly improve merge performance in a common use case.
> 
> Quite a while back glasser pointed out that the code to filter
> self-referential mergeinfo was a performance hog - see
> http://svn.haxx.se/dev/archive-2008-02/0206.shtml.  More recently arfrever
> found a merge bug where this filtering code removed valid mergeinfo - see
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&&msgNo=142849.
> This change implements glasser's suggestion, which also fixes the problem
> arfrever noted.
> 
> Suggested by: glasser
> 
> * subversion/libsvn_client/merge.c
>   (split_mergeinfo_on_revision): New.
>   (filter_self_referential_mergeinfo): Where possible use
>   svn_client__get_history_as_mergeinfo() one time per incoming mergeinfo
>   merge source rather than svn_client__repos_locations() for each discrete
>   range in each merge source's rangelist.
> 
> * subversion/tests/cmdline/merge_tests.py
>   (natural_history_filtering): Remove comment about XFail.
>   (test_list): Remove XFail from natural_history_filtering.

[...]
> +              /* So far we've only been manipulating rangelists, now we
> +                 actually create *YOUNGER_MERGEINFO and then remove the older
> +                 ranges from *MERGEINFO */
> +              if (!(*younger_mergeinfo))
> +                *younger_mergeinfo = apr_hash_make(pool);
> +              apr_hash_set(*younger_mergeinfo,
> +                           (const char *)merge_source_path,

No functional harm, but that's a redundant cast.

> +                           APR_HASH_KEY_STRING, younger_rangelist);
> +              SVN_ERR(svn_mergeinfo_remove(mergeinfo, *younger_mergeinfo,
> +                                           *mergeinfo, pool));
> +              break; /* ...out of for (i = 0; i < rangelist->nelts; i++) */
> +            }

[...]
> +                      /* Check if PATH@TARGET_ENTRY->REVISION exists at
> +                         RANGE->START on the same line of history. */
> +                      err = svn_client__repos_locations(&start_url,
> +                                                        &start_revision,
> +                                                        NULL,
> +                                                        NULL,
> +                                                        merge_b->ra_session2,
> +                                                        target_url,
> +                                                        &peg_rev,
> +                                                        &rev1_opt,
> +                                                        &rev2_opt,
> +                                                        merge_b->ctx,
> +                                                        pool);
> +                      if (err)
>                          {
> -                          /* PATH@TARGET_ENTRY->REVISION didn't exist at
> -                             RANGE->START or is unrelated to the resource
> -                             PATH@RANGE->START.  Either way we don't
> -                             filter. */
> -                          svn_error_clear(err);
> -                          err = NULL;
> -                          APR_ARRAY_PUSH(adjusted_rangelist,
> -                                         svn_merge_range_t *) = range;
> -                        }
> +                          if (err->apr_err == SVN_ERR_CLIENT_UNRELATED_RESOURCES
> +                              || err->apr_err == SVN_ERR_FS_NOT_FOUND
> +                              || err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)

The addition of SVN_ERR_FS_NO_SUCH_REVISION here should be supported by
documenting that svn_client__repos_locations() does in fact return this
error code. (I assume it was found to do so in practice, but I cannot
see where it comes from.)

Note also that in r33296, cmpilato removed a bit of redundant code that
was introduced in this function, that looked like:
[[[
    const char *merge_source_root_url;

    SVN_ERR(svn_ra_get_repos_root2(merge_b->ra_session2,
                                   &merge_source_root_url, pool));
]]]
This caused GCC to issue a warning because the same variable is declared
and initialised again at an inner scope, but is otherwise harmless.
r33296 is not proposed for back-port.


Everything else looks fine.

I have voted +1 on back-porting to 1.5.x. (Haven't time tonight to
review the test as well.)

- Julian



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org