On Mon, Dec 12, 2011 at 5:51 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Responses in-line, but first a reminder of why I'm doing this.
>
> It's because I've been looking at making 'svn merge' give better diagnostics and confirmations.
+1 to the spirit of that!
> Things like issuing an error or warning if the source and target are in fact the same 'branch', or if the user is trying to reintegrate in the wrong direction (the same direction as some sync merges have been done). To do this, the client needs to ask questions like 'what branches and revisions would this merge affect?' before executing the merge. Similar issues apply to all kinds of merges, but I happen to have taken the reintegrate first, since it's known to be a wrapper for a two-URL merge.
>
> I expect kind of control this would be especially valuable for GUIs, and I would appreciate feedback on this matter.
>
> I don't suggest that every client should be required to execute the steps separately. I think we should keep the 'svn_client_merge_reintegrate' API as a shortcut, and not deprecate it.
+1
> Opinions welcome.
>
> Daniel Shahaf wrote:
>
>> OK. I can't find reintegrate_merge_locked(),
>
>
> Oops, I meant 'merge_reintegrate_locked'.
>
> And:
>> Julian Foad wrote:
>>> >
>>> > svn_client_find_reintegrate_merge(&url1, &rev1, &url2, &rev2, ...);
>>> > svn_client_merge4(url1, rev1, url2, rev2, ...);
>>>
>>> One issue is that there is some duplication of work in this
>>> formulation. Both of these functions check that the target WC is
>>> suitable for merging into. Both open up RA sessions. Both determine
>>> the youngest common ancestor.
>>
>> I don't think "opening an RA session" is a problem for a function
>> that's about to do a merge operation :)
>
> Yes, even for a small merge the cost of that can't be of much importance.
>
>> Checking the WC's sanity, presumably that's necessary since the WC may
>> be modified after svn_client_find_reintegrate_merge() but before
>> svn_client_merge4()? Especially if the user is prompted to +1 the
>> output of svn_client_find_reintegrate_merge() before the actual merge
>> is done. (and then goes to lunch, comes back, makes some commits, finds
>> a loose TSVN dialog and OKs it)
>
> Good thoughts. The current situation in this patch (v2) is that step 1 (_find_reintegrate_merge) checks the WC is:
>
> * single-revision
> * unmodified
> * unswitched
>
> and step 2 (_merge4) checks the WC is:
>
> * single-revision
>
> because that's all that a normal (non-reintegrate) invocation of svn_client_merge4() needs.
>
> If step 2 could repeat the same check as step 1, that would be useful for your scenario, but otherwise there's little point in step 2 doing a partial check. So for now I'll take the simple solution which is to call svn_client_merge4(allow_mixed_rev=TRUE) to bypass the check.
>
> If and when we want to perform the full check in step 2, then we'll want to alter svn_client_merge4() in some way to provide more control over those checks, or else expose the check function separately. The best way might become clearer with further work on the APIs.
Agreed, this can be dealt with later (if it needs to be dealt with at all).
Overall I think this patch looks good. The only problem I see is:
> Index: subversion/svn/merge-cmd.c
> ===================================================================
> --- subversion/svn/merge-cmd.c (revision 1213025)
> +++ subversion/svn/merge-cmd.c (working copy)
> @@ -39,6 +39,59 @@
>
> /*** Code. ***/
>
> +/* Do a reintegrate merge from SOURCE_PATH_OR_URL_at_SOURCE_PEG_REVISION into
> + * TARGET_WCPATH. Do it with a WC write lock unless DRY_RUN is true. */
> +static svn_error_t *
> +merge_reintegrate(const char *source_path_or_url,
> + const svn_opt_revision_t *source_peg_revision,
> + const char *target_wcpath,
> + svn_boolean_t dry_run,
> + svn_boolean_t quiet,
> + const apr_array_header_t *merge_options,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool)
> +{
> + const char *url1, *url2;
> + svn_revnum_t rev1, rev2;
> +
> + SVN_ERR(svn_client_find_reintegrate_merge(
> + &url1, &rev1, &url2, &rev2,
> + source_path_or_url, source_peg_revision, target_wcpath,
> + ctx, scratch_pool, scratch_pool));
svn_client_find_reintegrate_merge() takes an absolute WC path, but we
might be passing it a relative path here. That will ultimately cause
an assertion in wc_db.c:
svn: E235000: In file '..\..\..\subversion\libsvn_wc\wc_db.c' line
7173: assertion failed (svn_dirent_is_absolute(local_abspath))
Paul
Received on 2011-12-12 17:48:01 CET