Re: [PATCH][merge-tracking]Fix repeat merge problem while subtree of merge target having overlapping merge already

On Fri, 19 Jan 2007, Kamesh Jayachandran wrote:
...
> >+ SUBTREES_HAVING_MERGEINFO might have the subpaths which might have
> >
> >Avoid repeating words like "might" twice in the same sentence.
>
> Sure will correct it.

I already got this one during a rewrite of this doc string.

...
> >We don't only want to consider sub-trees here; we want to consider any
> >child of the merge target which has merge info. Files can have their
> >own merge info, which overrides any merge info inherited from their
> >(grand-)parent directory.
> >
> >Let's name this parameter CHILDREN_WITH_MERGEINFO instead. I've tried
> >to make the doc string more clear, too.
>
> Fine. I was under the thought that 'SUBTREE' includes both sub-sub
> directories and files(tree with no child at all). CHILDREN looks clear
> though.

Yeah, arguably. I too feel that children is more clear.

...
> >+ apr_ssize_t target_wcpath_len = strlen(target_wcpath);
> >
> >Why use apr_ssize_t instead of apr_size_t?
>
> Copy Paste mistake :).

Okay. Already got this one.

> >
> >+ for (hi = apr_hash_first(pool, subtrees_having_mergeinfo);
> >
> >The iterator over this hash doesn't need to be thread-safe, so we
> >could avoid passing in pool to apr_hash_first().
>
> What if in a hypothetical case where two simultaneous merges from the
> same application context on two different threads exercising the same
> portion of code?(A GUI built on top of JAVAHL?)

It won't matter, because they'll be using different apr_hash_t structs
(in memory). A pool can be provided to perform concurrent iterations
over the same hash struct (which we're not doing here). If a pool is
not provided, the hash's own iterator field is used, rather than
incurring an extra allocation from the pool parameter.

APR_DECLARE(apr_hash_index_t *) apr_hash_first(apr_pool_t *p, apr_hash_t *ht)
{
    apr_hash_index_t *hi;
    if (p)
        hi = apr_palloc(p, sizeof(*hi));
    else
        hi = &ht->iterator;

    ...
}

...
> >+ //set no-op set-path only on subset.
> >+ if ((klen < target_wcpath_len) ||
> >+ (strcmp(subtree_wc_target,
> >+ target_wcpath) == 0) ||
> >+ (strncmp(subtree_wc_target,
> >+ target_wcpath,
> >+ target_wcpath_len) != 0))
> >
> >Can't we use something from svn_path.h for this complex test?
> >svn_path_is_child() looks like it might work.
>
> It might work. But I don't favour it here. Because it does extra things
> which I am not interested in (Giving a tail portions since match.).
> Given that I know about my input here('subtree_wc_target',
> 'target_wcpath'), I don't want it to undergo the rigorous ':' checks for
> windows.

Okay. I cut it down to 3 lines, which I suppose is simple enough.

> You seemed to have removed this comment prior to this check '
>
> + //set no-op set-path only on subset.'
>
> May be it could be made as
>
> + //set no-op set-path only on child of target_wc_path not on
> itself and on non-childs.'
>
> >+ {
> >+ continue;
> >+ }
> >+
> >+ subtree_repospath = key + target_wcpath_len + 1;
> >+
> >+ SVN_ERR(reporter->set_path(report_baton, subtree_repospath,
> >+ is_revert ? r->end - 1 : r->end,
> >+ FALSE, NULL, pool));
> >+ }
> >+ }
> >
> >This entire block could use an inline comment indicating what it's
> >intended to do.
>
> Yes would do.

All of the above is handled by the comments I've added to the code
block. I find the phrasing of the "set no-op set-path" comment to be
somewhat nonsensical, but more importantly, it talks only about the
"what", but doesn't really get into the "why".

> >...
> >+/*
> >+ Fill SUBTREES_HAVING_MERGEINFO with subpaths which might have
> >+ intersecting merges.
> >+ SUBTREES_HAVING_MERGEINFO is assumed to be non-NULL.
> >+ For each such subtree calls do_merge or do_single_file_merge
> >+ based on the kind of subtree with the appropriate arguments.
> >
> >This API is slightly incohesive -- it serves two purposes:
> >
> >1) Gets the list of children with merge info.
> >
> >2) Performs the appropriate merges for each child.
> >
> >Typically, this isn't a good way to go. However, we can consider #1
> >to be a return value listing "what was changed", so I think we can let
> >this one slide. We might want to be a little more explicit about this
> >in the doc string, though, or name the API to reflect this
> >(e.g. discover_and_do_child_merges?).
>
> Sounds good.

Will you think about the best way to go, and send a doc or naming
patch for this one?

...
> >@@ -3547,6 +3692,25 @@
> > recursive diff-editor thing. */
> > else if (entry->kind == svn_node_dir)
> > {
> >+ if (strcmp(URL1, URL2) == 0)
> >
> >Inside of do_merge(), we perform some calculations on the two URLs
> >before using them to set the merge_baton.same_urls field. The
> >same_urls field is effectively what you're trying to use here to get
> >do_subtree_merges(). Why the difference? Is it important?
>
> It is just to do away with Three merge scenarios where these 'Subtree
> exclusion' does not make sense atleast currently.

Sure, the reasoning for the checks is the same in both places. But
the code used to implement the checks is different -- why? Is it
important that it is different?

...
> >@@ -3696,6 +3862,22 @@
> > recursive diff-editor thing. */
> > else if (entry->kind == svn_node_dir)
> > {
> >+ SVN_ERR(do_subtree_merges(subtrees_having_mergeinfo,
> >+ entry,
> >+ URL,
> >+ path,
> >+ revision1,
> >+ path,
> >+ revision2,
> >+ peg_revision,
> >+ recurse,
> >+ ignore_ancestry,
> >+ adm_access,
> >+ &merge_cmd_baton,
> >+ pool));
> >
> >And why does svn_client_merge_peg3() not gate do_subtree_merges() in
> >the same style of "if (same_urls)" conditional?
>
> We have only one URL in case of svn_client_merge_peg3() (Not sure why,
> but that is the way it is).

Okay. I guess we never run into the 3-URL case here.

...
> >In do_merge() and do_single_file_merge(), we also have:
> >
> > /* When only recording merge info, we don't perform an actual
> > merge for the specified range. */
> > if (merge_b->record_only)
> > {
> > /* ### TODO: Support sub-tree merge info. */
> > /* ### Handle WC-local reverts which have modified our merge
> > ### info. */
> > apr_hash_t *merges;
> > SVN_ERR(determine_merges_performed(&merges, target_wcpath,
> > &range,
> > &notify_b, pool));
> > return update_wc_merge_info(target_wcpath, entry, rel_path,
> > merges, is_revert, ra_session,
> > adm_access, ctx, pool);
> > }
> >
> >Is this TODO comment about sub-tree merge info still relevant? If so,
> >what needs to be changed?
>
> Yes it can be removed.

Okay, include that in the next patch. Can you also include another
test for sub-tree merge info, especially tests in which one of the
"internal" merges on a child path adds and/or substracts additional
merge info.

> I have a less clue about what that second comment line means.

It's a separate TODO comment. I believe it's about an 'svn merge -r N:N-1'

which reverted changes in the WC which had previous been merged in.
In such a case, we need to remove merge info from the WC. I'm not
clear on whether determine_merges_performed() and
update_wc_merge_info() handles that case. It rather looks like it
does, but this is another area that we could use a test case for.

- Dan