Re: [PATCH] Single file merge tracking implementation. /merge-tracking/subversion/libsvn_client/diff.c

Thanks for the patch, Kamesh. Comments inline, and revised patch
attached.

On Mon, 03 Jul 2006, Kamesh Jayachandran wrote:
...
> * subversion/libsvn_fs_fs/tree.c
> (fs_change_node_prop):
> For file nodes under '/' we receive path as just the file names
> without a '/' at the begining. This happens for single file merges,
> so canonicalize the path to make sure 'mergeinfo.mergeto' is getting
> recorded as canonicalized path.

> Index: subversion/libsvn_fs_fs/tree.c
> ===================================================================
> --- subversion/libsvn_fs_fs/tree.c (revision 20359)
> +++ subversion/libsvn_fs_fs/tree.c (working copy)
> @@ -1458,9 +1458,13 @@
> directly. */
>
> svn_fs_txn_t *txn;
> + /* For file nodes under '/' we receive path as just the file names
> + * without a '/' at the begining. This happens for single file merges,
> + * so canonicalize the path to canon_path */
> + const char *canon_path = svn_fs_fs__canonicalize_abspath(path, pool);
> SVN_ERR(svn_fs_open_txn(&txn, root->fs, txn_id, pool));
>
> - SVN_ERR(svn_fs_fs__change_txn_mergeinfo(txn, path, value, pool));
> + SVN_ERR(svn_fs_fs__change_txn_mergeinfo(txn, canon_path, value, pool));
>
> SVN_ERR(svn_fs_fs__change_txn_prop(txn,
> SVN_FS_PROP_TXN_CONTAINS_MERGEINFO,

Seems like we'll be hitting this code path very often. Is this
canonicalization redundant in some instances? Can it be avoided, or
made specific to the "single-file merge" case?

> * subversion/libsvn_client/diff.c
> (ENSURE_VALID_REVISION_KINDS): New macro to check whether merge revision
> info are specified or not.
> (do_merge): uses ENSURE_VALID_REVISION_KINDS.
> (single_file_merge_get_file):
> Caller has to pass ra_session to reuse the ra_session.
> No need to translate 'svn_opt_revision_t' to 'svn_revnum_t', Caller
> has this info already, so no need of 'path' and 'revision' args.
> (do_single_file_merge):
> Added 'svn_boolean_t dry_run' as arg.
> Implemented merge tracking for single file merges the same way as
> directory merges.
> Remove the FIXME docstring for merge-tracking.
> Calls single_file_merge_get_file with the new signature.
> (svn_client_merge2):
> Calls do_single_file_merge with the new arg.
> (svn_client_merge_peg2):
> Calls do_single_file_merge with the new arg.

> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 20359)
> +++ subversion/libsvn_client/diff.c (working copy)
...
> @@ -1989,6 +1985,7 @@
> const svn_opt_revision_t *peg_revision,
> const char *target_wcpath,
> svn_wc_adm_access_t *adm_access,
> + svn_boolean_t dry_run,
> struct merge_cmd_baton *merge_b,
> apr_pool_t *pool)
> {

Addition of a DRY_RUN parameter is unnecessary -- it's already
available via the merge baton.

I noticed that do_merge() was also passing a redundant DRY_RUN
parameter, and have fixed that in trunk in r20430, and merged it into
the merge-tracking branch.

...
> + svn_merge_range_t range;

The addition of the RANGE variable obviates the need for rev1 and rev2.

...
> + const char *uuid1, *uuid2;

Do we really need these UUID variables? More on this below...

...
...
> - /* ### heh, funny. we could be fetching two fulltexts from two
> - *totally* different repositories here. :-) */
> - SVN_ERR(single_file_merge_get_file(&tmpfile1, &props1, &rev1,
> - URL1, path1, revision1,
> - merge_b, pool));
>
> - SVN_ERR(single_file_merge_get_file(&tmpfile2, &props2, &rev2,
> - URL2, path2, revision2,
> - merge_b, pool));
> + SVN_ERR(svn_client_uuid_from_url(&uuid1, URL1, ctx, pool));
> + SVN_ERR(svn_client_uuid_from_url(&uuid2, URL2, ctx, pool));

Why are we retrieving the repository UUIDs for the files? To verify
that both file URLs come from the same repository?

> - /* Discover any svn:mime-type values in the proplists */
> - pval = apr_hash_get(props1, SVN_PROP_MIME_TYPE, strlen(SVN_PROP_MIME_TYPE));
> - mimetype1 = pval ? pval->data : NULL;
> + /* Establish RA session to URL1. */
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session1, URL1, NULL,
> + NULL, NULL, FALSE, TRUE,
> + ctx, pool));
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session2, URL2, NULL,
> + NULL, NULL, FALSE, TRUE,
> + ctx, pool));

The comment is no longer in sync with the code, since we're now
opening two RA sessions. Do we actually need both sessions here?

> + /* Resolve the revision numbers, and store them as a merge range.
> + Note that the "start" of a merge range is inclusive. */
> + SVN_ERR(svn_client__get_revision_number
> + (&range.start, ra_session1, revision1, path1, pool));
> + SVN_ERR(svn_client__get_revision_number
> + (&range.end, ra_session2, revision2, path2, pool));
> + if (range.start == range.end)
> + /* No merge to perform. */
> + return SVN_NO_ERROR;
> + is_revert = (range.start > range.end);
> + if (is_revert)
> + range.end += 1;
> + else
> + range.start += 1;

This block of code duplicates what's in do_merge(). I wonder whether
it's worth consolidating, possibly into a route which produces a
struct containing a svn_merge_range_t and an is_revert flag.

> + if (strcmp(uuid1, uuid2) == 0)
> + {
> + /* Look at the merge info prop of the WC target to see what's
> + already been merged into it. */
> + SVN_ERR(parse_merge_info(&target_mergeinfo, target_wcpath, adm_access, ctx,
> + pool));
...
> + SVN_ERR(svn_client__path_relative_to_root(&rel_path, URL1, NULL,
> + ra_session1, adm_access, pool));
> + SVN_ERR(calculate_merge_ranges(&remaining_ranges, rel_path, target_mergeinfo,
> + &range, is_revert, pool));
> + }
> + else
> + {
> + remaining_ranges = apr_array_make(pool, 1, sizeof(&range));
> + APR_ARRAY_PUSH(remaining_ranges, svn_merge_range_t *) = ⦥
> + }

In the "else" case, I take it that we're ignoring any merge info
because the merge source is supposedly from a different repository?
AFAIK, this isn't (yet) a supported use case. However, while I was
expecting to find some validation code which already enforced this, I
didn't see any. Anyone spare a clue?

> + for (i = 0; i < remaining_ranges->nelts; i++)
> + {
> + /* When using this merge range, account for the exclusivity of
> + its low value (which is indicated by this operation being a
> + merge vs. revert). */
> + svn_merge_range_t *r = APR_ARRAY_IDX(remaining_ranges, i,
> + svn_merge_range_t *);
...
> + /* ### heh, funny. we could be fetching two fulltexts from two
> + *totally* different repositories here. :-) */

Is this comment what made you support the "merge source repos differs
from WC's repos" use case?

> + SVN_ERR(single_file_merge_get_file(&tmpfile1, ra_session1, &props1,
> + is_revert ? r->start : r->start - 1,
> + URL1, merge_b, pool));
>
> - SVN_ERR(merge_file_changed(adm_access,
> - &text_state, &prop_state,
> - merge_b->target,
> - tmpfile1,
> - tmpfile2,
> - rev1,
> - rev2,
> - mimetype1, mimetype2,
> - propchanges, props1,
> - merge_b));
> + SVN_ERR(single_file_merge_get_file(&tmpfile2, ra_session2, &props2,
> + is_revert ? r->end - 1 : r->end,
> + URL2, merge_b, pool));
>
> - /* Ignore if temporary file not found. It may have been renamed. */
> - err = svn_io_remove_file(tmpfile1, pool);
> - if (err && ! APR_STATUS_IS_ENOENT(err->apr_err))
> - return err;
> - svn_error_clear(err);
> - err = svn_io_remove_file(tmpfile2, pool);
> - if (err && ! APR_STATUS_IS_ENOENT(err->apr_err))
> - return err;
> - svn_error_clear(err);
> + /* Discover any svn:mime-type values in the proplists */
> + pval = apr_hash_get(props1, SVN_PROP_MIME_TYPE, strlen(SVN_PROP_MIME_TYPE));
> + mimetype1 = pval ? pval->data : NULL;
> +
> + pval = apr_hash_get(props2, SVN_PROP_MIME_TYPE, strlen(SVN_PROP_MIME_TYPE));
> + mimetype2 = pval ? pval->data : NULL;
> +
> + /* Deduce property diffs. */
> + SVN_ERR(svn_prop_diffs(&propchanges, props2, props1, pool));
> +
> + SVN_ERR(merge_file_changed(adm_access,
> + &text_state, &prop_state,
> + merge_b->target,
> + tmpfile1,
> + tmpfile2,
> + is_revert ? r->start : r->start - 1,
> + is_revert ? r->end - 1 : r->end,
> + mimetype1, mimetype2,
> + propchanges, props1,
> + merge_b));
> +
> + /* Ignore if temporary file not found. It may have been renamed. */
> + err = svn_io_remove_file(tmpfile1, pool);
> + if (err && ! APR_STATUS_IS_ENOENT(err->apr_err))
> + return err;
> + svn_error_clear(err);
> + err = svn_io_remove_file(tmpfile2, pool);
> + if (err && ! APR_STATUS_IS_ENOENT(err->apr_err))
> + return err;
> + svn_error_clear(err);
>
> - if (merge_b->ctx->notify_func2)
> - {
> - svn_wc_notify_t *notify
> - = svn_wc_create_notify(merge_b->target, svn_wc_notify_update_update,
> - pool);
> - notify->kind = svn_node_file;
> - notify->content_state = text_state;
> - notify->prop_state = prop_state;
> - (*merge_b->ctx->notify_func2)(merge_b->ctx->notify_baton2, notify,
> - pool);
> + if (ctx->notify_func2)
> + {
> + svn_wc_notify_t *notify
> + = svn_wc_create_notify(merge_b->target, svn_wc_notify_update_update,
> + pool);
> + notify->kind = svn_node_file;
> + notify->content_state = text_state;
> + notify->prop_state = prop_state;
> + (*ctx->notify_func2)(ctx->notify_baton2, notify, pool);
> + }

As with do_merge(), we'll be calling this notification function over
and over again for each revision range we merge into this file. In
both cases, not the right behavior. Instead, we need to aggregate the
notifications, and distill them into some reasonable form. When
notifications conflict, I think we want to give preference to latter
notifications.

> }
> + if ((!dry_run) && (strcmp(uuid1, uuid2) == 0) && (remaining_ranges->nelts > 0))

If we end up retaining usage of UUIDs, using a flag to represent this
condition (which you also use above) would be a lot eaiser to
understand:

     svn_boolean_t from_same_repos = (strcmp(uuid1, uuid2) == 0);
     if (from_same_repos)
       ...
       if (!merge_b->dry_run && from_same_repos && remaining_ranges->nelts > 0)
         ...

> + {
> + SVN_ERR(update_wc_merge_info(target_wcpath, target_mergeinfo, rel_path,
> + remaining_ranges, is_revert, adm_access,
> + pool));
> + }
>
> return SVN_NO_ERROR;
> }