subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@btopenworld.com>
Subject Re: svn commit: r1442598 - /subversion/trunk/subversion/libsvn_client/merge.c
Date Tue, 05 Feb 2013 16:28:23 GMT
Hi Bert.  Nice change again.


> URL: http://svn.apache.org/viewvc?rev=1442598&view=rev

> Log:
> Remove the difference in handling single-file and directory merges in the
> merge notification processing.
> 
> * subversion/libsvn_client/merge.c
>   (merge_cmd_baton_t): Remove separate variables and add struct specifically
>     for this task.
>   (notify_merge_begin): Handle single file merges as ordinary merges.
>   (do_file_merge): Create a fake children_with_mergeinfo array and keep it
>     up to date.
> 
>   (do_mergeinfo_aware_dir_merge,
>    do_directory_merge,
>    do_merge): Simplify drive reset code.
> 
> Modified:
>     subversion/trunk/subversion/libsvn_client/merge.c
> 
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> ==============================================================================
[...]
> +
> +  /* State for notify_merge_begin() */
> +  struct notify_begin_state_t
> +  {
> +    /* const char * indicating which abspath was last notified for the current
> +       operation. */

There's no point in the comment telling us this is a "const char *" :-)

How about "The path that was last notified..."?

> +    const char *last_abspath;
> +
> +    /* Reference to the on-and-only CHILDREN_WITH_MERGEINFO (see global comment

"one-and-only"

"comment)"

> +       or a similar list for single-file-merges */
> +    const apr_array_header_t *nodes_with_mergeinfo;
> +  } notify_begin;
> +
> } merge_cmd_baton_t;

[...]
> @@ -7143,34 +7135,60 @@ do_file_merge(svn_mergeinfo_catalog_t re
> 
>    if (!merge_b->record_only)
>      {
> -      svn_rangelist_t *ranges_to_merge = remaining_ranges;
> +      svn_rangelist_t *ranges_to_merge = apr_array_copy(scratch_pool,
> +                                                       
remaining_ranges);

Instead of copying the array unconditionally here ...

[...]
> +          /* If we have ancestrally related sources and more than one
> +             range to merge, eliminate no-op ranges before going through
> +             the effort of downloading the many copies of the file
> +             required to do these merges (two copies per range). */
> +          if (remaining_ranges->nelts > 1)
> +            {
> +              const char *old_sess_url;
> +              svn_error_t *err;
> +
> +              SVN_ERR(svn_client__ensure_ra_session_url(&old_sess_url,
> +                                                       
merge_b->ra_session1,
> +                                                       
primary_src->url,
> +                                                       
iterpool));
> +              err = remove_noop_merge_ranges(&ranges_to_merge,
> +                                             merge_b->ra_session1,
> +                                             remaining_ranges,
scratch_pool);

... and then overwriting it with something different here ...

> +              SVN_ERR(svn_error_compose_create(
> +                        err, svn_ra_reparent(merge_b->ra_session1,
> +                                             old_sess_url, iterpool)));
> +            }

... add an "else ranges_to_merge = apr_array_copy(...)" here?  I think that would make the
intent clearer as well as avoiding the copy.

> +
> +          /* To support notify_merge_begin() initialize our
> +             CHILD_WITH_MERGEINFO. See the comment
> +             'THE CHILDREN_WITH_MERGEINFO ARRAY' at the start of this file. */
> +
> +          child_with_mergeinfo = apr_array_make(scratch_pool, 1,
> +                                        sizeof(svn_client__merge_path_t
*));
> +
> +          /* ### Create a fake copy of merge_target as we don't keep
> +                 remaining_ranges in sync (yet). */
> +          target_info = apr_pcalloc(scratch_pool, sizeof(*target_info));
> +
> +          target_info->abspath = merge_target->abspath;
> +          target_info->remaining_ranges = ranges_to_merge;
> +
> +          APR_ARRAY_PUSH(child_with_mergeinfo, svn_client__merge_path_t *)
> +                                    = target_info;
> +
> +          /* And store in baton to allow using it from notify_merge_begin() */
> +          merge_b->notify_begin.nodes_with_mergeinfo = child_with_mergeinfo;
>          }
> 
> -      for (i = 0; i < ranges_to_merge->nelts; i++)
> +      while (ranges_to_merge->nelts > 0)
>          {
[...]
> +
> +          /* Poor mans delete first item */
> +          SVN_ERR(svn_rangelist_reverse(ranges_to_merge, iterpool));
> +          ranges_to_merge->nelts--;
> +          SVN_ERR(svn_rangelist_reverse(ranges_to_merge, iterpool));

Eww!  We have a better way:

    svn_sort__array_delete(ranges_to_merge, 0, 1);

(I suppose the 'svn_sort' prefix makes that function hard to find.)


- Julian

Mime
View raw message