On Mon, Dec 12, 2011 at 5:51 AM, Julian Foad <[email protected]> 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@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
