You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/07/03 19:19:52 UTC
RE: [PATCH] Single file merge tracking implementation. /merge-tracking/subversion/libsvn_client/diff.c
-----Original Message-----
From: Daniel L. Rall [mailto:dlr@collab.net]
Sent: Thu 6/29/2006 11:42 AM
To: Kamesh Jayachandran
Cc: SVN Dev
Subject: Re: [PATCH] Single file merge tracking implementation. /merge-tracking/subversion/libsvn_client/diff.c
On Wed, 28 Jun 2006, Kamesh Jayachandran wrote:
...
...
> @@ -1972,7 +1971,9 @@
> 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,
> + svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> apr_hash_t *props1, *props2;
A svn_client_ctx_t * is already available via merge_b->ctx. I'm not
sure why we pass this in separately for do_merge() -- it looks like an
oversight (WRT to the baton data type), which I've fixed on trunk in
r20286, and integrated into the merge-tracking branch (using
svnmerge.py).
Taken care.
...
> + /* Sanity check -- ensure that we have valid revisions to look at. */
> + if ((initial_revision1->kind == svn_opt_revision_unspecified)
> + || (initial_revision2->kind == svn_opt_revision_unspecified))
> + {
> + return svn_error_create
> + (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> + _("Not all required revisions are specified"));
> + }
This block is duplicated in do_merge() -- should we stick it into a
macro?
Taken care.
...
> @@ -2018,6 +2033,30 @@
> *revision2 = *initial_revision2;
> }
>
> + /* Establish first RA session to URL1. */
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session, URL1, NULL,
> + NULL, NULL, FALSE, TRUE,
> + ctx, pool));
In do_single_file_merge(), we call some other functions which take a
RA session, that we pass NULL into. Would it be possible to re-use
this RA session instead across those calls?
Taken care.
> + /* 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_session, revision1, path1, pool));
> + range.start += 1;
> + SVN_ERR(svn_client__get_revision_number
> + (&range.end, ra_session, revision2, path2, pool));
> + is_revert = (range.start > range.end);
is_revert will be wrong here because range.start was already modified
-- you copied the bad example I committed incrementally while trying
to handle the logic for reverts. See r20280 and r20285.
Taken care now the single-file merge implementation reflects that of directory merges at r20359.
> + /* 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_session, adm_access, pool));
> + SVN_ERR(calculate_merge_ranges(&remaining_ranges, rel_path, target_mergeinfo,
> + &range, is_revert, pool));
> +
...
You appear to be missing a loop around the merging of the revision
ranges and cleanup.
Taken care.
[[[
Patch by: Kamesh Jayachandran <ka...@collab.net>
Single file merge tracking implementation.
* 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.
* 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.
]]]
Re: [PATCH] Single file merge tracking implementation. /merge-tracking/subversion/libsvn_client/diff.c
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 07 Jul 2006, Kamesh Jayachandran wrote:
> Thanks Dan for the review and refined patch.
> >>+ 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?
>
> Originally tried to investigate why nodes under root are not
> canonicalized, did not have much success.
I'd like more information about this -- can you dig a little deeper?
Does this behavior also apply to children of the FS root when called
from do_merge()?
...
> >>+ 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.
>
> Could not understand it much would go through the same later and provide
> a fix for it.
...
Nailing this down in conjunction with the special casing for the
"svn:mergeinfo" housekeeping property makes sense.
I ripped out the UUID checking code (since this isn't a cross-repos
operation, despite what that comment looked like), made a few more
adjustments, and have committed this as r20471.
Thanks, Dan
Re: [PATCH] Single file merge tracking implementation. /merge-tracking/subversion/libsvn_client/diff.c
Posted by Kamesh Jayachandran <ka...@collab.net>.
Thanks Dan for the review and refined patch.
>> + 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?
>
>
Originally tried to investigate why nodes under root are not
canonicalized, did not have much success.
> Why are we retrieving the repository UUIDs for the files? To verify
> that both file URLs come from the same repository?
>
>
Yes. I recently found one more issue for which I don't have a clue to
solve(Not sure how to get the WC's uuid)
The issue is, the current patch records 'svn:mergeinfo' on a WC of repo1
when the merge was executed for difference between 2 revisions of
similarly looking file on repo2 and file also gets merged.
>> - /* 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?
>
>
Yes. Attached patch fixes it.
>> + /* 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.
>
>
Fixed in the attached patch.
>> + 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?
>
I could merge from two different repo to a wc of totally another
repo(Atleast for single file merges).
>
>> + 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?
>
>
Yes.
>> + 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.
>
>
Could not understand it much would go through the same later and provide
a fix for it.
>> }
>> + 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)
> ...
>
Could see in your new patch.
Find the attached patch.
Thanks.
With regards
Kamesh Jayachandran
[[[
Patch by: Kamesh Jayachandran <ka...@collab.net>
Patch by: Daniell L Rall <dl...@collab.net>
Single file merge tracking implementation.
* 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.
* subversion/libsvn_client/diff.c
(ENSURE_VALID_REVISION_KINDS): New macro to check whether merge revision
info are specified or not.
(grok_range_info_from_opt_revisions):
New function. converts opt_revision_t to svn_revnum_t, adjust the range
based on 'revert' or 'merge'.
(do_merge):
uses ENSURE_VALID_REVISION_KINDS.
uses grok_range_info_from_opt_revisions.
(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):
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.
]]]
Re: [PATCH] Single file merge tracking implementation. /merge-tracking/subversion/libsvn_client/diff.c
Posted by Daniel Rall <dl...@collab.net>.
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;
> }