Kamesh, this is a review of your patch at
http://subversion.tigris.org/issues/show_bug.cgi?id=2899#desc5
As part of learning this patch, I found myself writing a log message
anyway (for my own comprehension), so I offer it to you below. Please
feel free to correct it, or to not use it at all, when you commit.
Here's the log message:
[[[
Finish issue #2899: Set 'non-inheritable mergeinfo' for shallow merges.
* subversion/libsvn_client/merge.c
(struct notification_receiver_baton_t): New field merged_paths.
(notification_receiver): Add affected paths to notify_b->merged_paths.
(determine_merges_performed): Take two new parameters,
requested_depth and adm_access; all callers changed. Use the
parameters to set inheritability appropriately, depending on the
depth of the merge and the merge target's kind.
(update_wc_mergeinfo): Be slightly looser about inheritance when
merging rangelists: instead of never considering inheritance,
consider it iff the two ranges have equal inheritance.
(do_merge): Change depth parameter to requested_depth, and don't
make the range inheritable unless requested_depth is
svn_depth_infinity. When merging final merge ranges, explicitly
set mergeinfo for children that, due to non-infinite depth, can't
count on inheriting it. Clear the notify_b->merged_paths hash on
each iteration of the revision-range merge loop.
(do_single_file_merge): Clear the notify_b->merged_paths hash on
each iteration of the revision-range merge loop. ### See review
comment below about this change, though. ###
]]]
And here's the review (inline):
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 26491)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -1769,6 +1769,9 @@
> /* The number of operative notifications received. */
> apr_uint32_t nbr_operative_notifications;
>
> + /* The list of merged paths. */
> + apr_hash_t *merged_paths;
> +
> /* The list of any skipped paths, which should be examined and
> cleared after each invocation of the callback. */
> apr_hash_t *skipped_paths;
Okay, looks good. But since it's a hash table, you might want to
specify precisely what types the keys and values are. (I can see from
later code, e.g., the calls to apr_hash_this(), that the values are
ignored and only the keys matter.)
> @@ -1798,6 +1801,21 @@
> || notify->action == svn_wc_notify_update_add)
> notify_b->nbr_operative_notifications++;
>
> + if (notify->content_state == svn_wc_notify_state_merged
> + || notify->content_state == svn_wc_notify_state_changed
> + || notify->prop_state == svn_wc_notify_state_merged
> + || notify->prop_state == svn_wc_notify_state_changed
> + || notify->action == svn_wc_notify_update_add)
> + {
> + const char *merged_path = apr_pstrdup(notify_b->pool, notify->path);
> +
> + if (notify_b->merged_paths == NULL)
> + notify_b->merged_paths = apr_hash_make(notify_b->pool);
> +
> + apr_hash_set(notify_b->merged_paths, merged_path,
> + APR_HASH_KEY_STRING, merged_path);
> + }
> +
> if (notify->action == svn_wc_notify_skip)
> {
> const char *skipped_path = apr_pstrdup(notify_b->pool, notify->path);
I wasn't sure why we're testing for 'svn_wc_notify_state_changed' and
'svn_wc_notify_update_add' in order to decide whether or not to add
the path to notify_b->merged_paths. Possibly this is a typical idiom,
and I'm just too inexperienced with the merge code to recognize it.
If so, no need for a comment -- we just need a more educated reader.
But if it is not a typical idiom, then an explanatory comment would be
good, so future readers don't wonder why we're testing for conditions
that (at first glance) don't seem to be about merging.
> @@ -1824,6 +1842,8 @@
> determine_merges_performed(apr_hash_t **merges, const char *target_wcpath,
> svn_boolean_t target_missing_child,
> svn_merge_range_t *range,
> + svn_depth_t requested_depth,
> + svn_wc_adm_access_t *adm_access,
> notification_receiver_baton_t *notify_b,
> struct merge_cmd_baton *merge_b,
> apr_pool_t *pool)
Two new parameters added, but no change made to doc string. So, I'll
have to guess what the new parameters do... :-)
> @@ -1879,7 +1899,47 @@
> ;
> }
> }
> + if ((requested_depth != svn_depth_infinity) && notify_b->merged_paths)
> + {
> + apr_hash_index_t *hi;
> + const void *merged_path;
>
> + for (hi = apr_hash_first(NULL, notify_b->merged_paths); hi;
> + hi = apr_hash_next(hi))
> + {
> + const svn_wc_entry_t *child_entry;
> + svn_merge_range_t *child_merge_range;
> + apr_array_header_t *rangelist_of_child = NULL;
> + apr_hash_this(hi, &merged_path, NULL, NULL);
> + child_merge_range = svn_merge_range_dup(range, pool);
> + SVN_ERR(svn_wc__entry_versioned(&child_entry,
> + merged_path,
> + adm_access, FALSE,
> + pool));
> + if (((child_entry->kind == svn_node_dir) &&
> + (strcmp(merge_b->target, merged_path) == 0) &&
> + (requested_depth == svn_depth_immediates))
> + || ((child_entry->kind == svn_node_file) &&
> + (requested_depth == svn_depth_files)))
Usually the "&&" connectors go on the "next" line, not the "prev" line
(at least, that's how most of the Subversion code seems to me).
Either way, watch out for inaccurate indentation -- I think if you hit
TAB on some of the lines above, you'll find that some things are not
syntactically indented correctly. That makes the code harder to read.
Compare with this:
if (((child_entry->kind == svn_node_dir)
&& (strcmp(merge_b->target, merged_path) == 0)
&& (requested_depth == svn_depth_immediates))
|| ((child_entry->kind == svn_node_file)
&& (requested_depth == svn_depth_files)))
The structure of the conditional is now immediately apparent.
Now, about the semantics. Should this subexpression...
(requested_depth == svn_depth_files)
...be this instead:
((requested_depth == svn_depth_files)
|| (requested_depth == svn_depth_immediates))
? After all, svn_depth_immediates is a superset of svn_depth_files.
> + {
> + /* Set the explicit inheritable mergeinfo for,
> + 1. Merge target directory if depth is immediates.
> + 2. If merge is on a file and requested depth is 'files'.
> + */
> + child_merge_range->inheritable = TRUE;
> + rangelist_of_child = apr_array_make(pool, 1, sizeof(range));
> + }
Hmmm, why is the range inheritable in case (1) if this is a merge with
--depth=immediates? I would think it would be non-inheritable in that
case, because not everything underneath the merged_path (a directory)
received the merge in that case. (Or does inheritability only apply
to the files immediately in that directory, in which case yes, it
would be appropriate to set inheritable=TRUE ?)
> + if (rangelist_of_child)
> + {
> + APR_ARRAY_PUSH(rangelist_of_child, svn_merge_range_t *) =
> + child_merge_range;
> +
> + apr_hash_set(*merges, (const char *) merged_path,
> + APR_HASH_KEY_STRING, rangelist_of_child);
> + }
> + }
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -1959,7 +2019,7 @@
> else
> {
> SVN_ERR(svn_rangelist_merge(&rangelist, ranges,
> - svn_rangelist_ignore_inheritance,
> + svn_rangelist_equal_inheritance,
> subpool));
> }
Interesting. The new code seems correct to my untrained eye; what's
surprising is that it wasn't equal_inheritance before, actually.
> @@ -2204,7 +2264,8 @@
> ra_session,
> initial_revision2,
> pool));
> - range.inheritable = !target_missing_child;
> + range.inheritable = (!target_missing_child &&
> + (requested_depth == svn_depth_infinity));
> if (merge_type == merge_type_no_op)
> return SVN_NO_ERROR;
*nod* Makes sense to me :-).
> @@ -2409,7 +2472,55 @@
> target_mergeinfo,
> adm_access,
> subpool));
> + if ((requested_depth != svn_depth_infinity)
> + && notify_b.merged_paths)
> + {
> + svn_boolean_t indirect_child_mergeinfo = FALSE;
> + apr_hash_index_t *hi;
> + apr_hash_t *child_target_mergeinfo;
> + const void *merged_path;
>
> + for (hi = apr_hash_first(NULL, notify_b.merged_paths);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const svn_wc_entry_t *child_entry;
> + apr_hash_this(hi, &merged_path, NULL, NULL);
> + SVN_ERR(svn_wc__entry_versioned(&child_entry,
> + merged_path,
> + adm_access, FALSE,
> + pool));
> + if (((child_entry->kind == svn_node_dir) &&
> + (strcmp(merge_b->target, merged_path) == 0) &&
> + (requested_depth == svn_depth_immediates))
> + || ((child_entry->kind == svn_node_file) &&
> + (requested_depth == svn_depth_files)))
Similar comments about the condition here as for the earlier one.
(And about "&&" placement.)
> + {
> + /* Set the explicit inheritable mergeinfo for,
> + * 1. Merge target directory if depth is
> + * 'immediates'.
> + * 2. If merge is on a file and requested depth
> + * is 'files'.
> + */
> + SVN_ERR(get_wc_or_repos_mergeinfo(
> + &child_target_mergeinfo,
> + child_entry,
> + &indirect_child_mergeinfo,
> + FALSE,
> + svn_mergeinfo_inherited,
> + ra_session,
> + merged_path, adm_access,
> + ctx, subpool));
> + if (indirect_child_mergeinfo)
> + SVN_ERR(svn_client__record_wc_mergeinfo(
> + merged_path,
> + child_target_mergeinfo,
> + adm_access,
> + subpool));
> + }
> + }
> + }
> +
Please check my log message to make sure I have understood the entire
above block correctly.
> @@ -2862,6 +2976,8 @@
> notify_b.nbr_notifications = 0;
> if (notify_b.skipped_paths != NULL)
> svn_hash__clear(notify_b.skipped_paths);
> + if (notify_b.merged_paths != NULL)
> + svn_hash__clear(notify_b.merged_paths);
> }
This time we're in do_single_file_merge(). Do we need to clear the
notify_b->merged_paths hash in this case? Does this function ever use
that hash table?
> Index: subversion/tests/cmdline/merge_tests.py
> ===================================================================
> --- subversion/tests/cmdline/merge_tests.py (revision 26491)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
> @@ -8128,7 +8128,7 @@
> merge_loses_mergeinfo,
> single_file_replace_style_merge_capability,
> merge_to_out_of_date_target,
> - XFail(merge_with_depth_files),
> + merge_with_depth_files,
> merge_fails_if_subtree_is_deleted_on_src,
> no_mergeinfo_from_no_op_merge,
> merge_to_sparse_directories,
That's a satisfying diff hunk :-).
Okay, end of review. Note that I'm unfamiliar with much of the merge
code, so some of my comments may reflect that ignorance rather than
anything about your patch.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Sep 8 01:36:44 2007