Re: svn commit: r26507 - trunk/subversion/libsvn_client

On Tue, 11 Sep 2007, Kamesh Jayachandran wrote:
...
> Yes we are not making a deep copy here. Till r26505 we had a
> inconsistent code of duplicating curr->path and not doing so for
> curr->propval. After careful examination I leaned towards shallow copy.
>
> Currently shallow copy does not cause any problem and eventually won't,
> unless someone keeps reference to pointers of *last* 'merge_path_t'
> *across* code.
> I would lean towards a 'Doc string stating the shallow copy behaviour'
> rather than deep copying things.

This sounds fine in private routines like this, so long as it is
clearly documented.

> Daniel Rall wrote:
> >We're not making a deep copy of merge_path_t's fields here (e.g. for
> >foo->path). Does that currently result in a problem (e.g. due to
> >differing pool lifetime)? (It certainly could later when others
> >modify the code.)
> >
> >On Mon, 10 Sep 2007, kameshj@tigris.org wrote:
> >...
> >
> >>Code Cleanup.
> >>
> >>* subversion/libsvn_client/merge.c
> >> (insert_child_to_merge): Instead of 'member by member copy',
> >> follow *ptr1=*ptr2 style of copy, so that things look simple.
> >>
> >...
> >
> >>--- trunk/subversion/libsvn_client/merge.c (original)
> >>+++ trunk/subversion/libsvn_client/merge.c Mon Sep 10 04:57:25 2007
> >>@@ -3335,12 +3335,7 @@
> >> merge_path_t *);
> >> merge_path_t *curr_copy = apr_palloc(children_with_mergeinfo->pool,
> >> sizeof(*curr_copy));
> >>- curr_copy->path = curr->path;
> >>- curr_copy->missing_child = curr->missing_child;
> >>- curr_copy->switched = curr->switched;
> >>- curr_copy->has_noninheritable = curr->has_noninheritable;
> >>- curr_copy->absent = curr->absent;
> >>- curr_copy->propval = curr->propval;
> >>+ *curr_copy = *curr;
> >> APR_ARRAY_PUSH(children_with_mergeinfo, merge_path_t *) =
> >> curr_copy;
> >>
> >> /* Move all elements from INSERT_INDEX to the end of the array
> >> forward
> >>@@ -3350,24 +3345,12 @@
> >> merge_path_t *prev;
> >> curr = APR_ARRAY_IDX(children_with_mergeinfo, j, merge_path_t
> >> *);
> >> if (j == insert_index)
> >>- {
> >>- curr->path = insert_element->path;
> >>- curr->missing_child = insert_element->missing_child;
> >>- curr->switched = insert_element->switched;
> >>- curr->has_noninheritable =
> >>insert_element->has_noninheritable;
> >>- curr->absent = insert_element->absent;
> >>- curr->propval = insert_element->propval;
> >>- }
> >>+ *curr = *insert_element;
> >> else
> >> {
> >> prev = APR_ARRAY_IDX(children_with_mergeinfo, j - 1,
> >> merge_path_t *);
> >>- curr->path = prev->path;
> >>- curr->missing_child = prev->missing_child;
> >>- curr->switched = prev->switched;
> >>- curr->has_noninheritable = prev->has_noninheritable;
> >>- curr->absent = prev->absent;
> >>- curr->propval = prev->propval;
> >>+ *curr = *prev;
> >> }
> >> }
> >> }
> >>