Now with a patch for discussion.
(More below...)
Julian Foad wrote:
>> Brane told me that while updating the JavaHL API he noticed that the new
>> svn_client_merge_automatic() is Yet Another Merge API, and would prefer make
>> the existing JavaHL merge API encompass that functionality rather than add
>> yet another variant. So I said if let's see if we can apply that idea to the
>> libsvn_client API.
>>
>> Idea: 'automatic' merge is (in a high level logical sense) most similar to
>> a subset of the 'pegged' merge. So make 'merge_peg' do the automatic merge
>> (like calling the automatic merge API) if the params are suitable. The
>> intention is that this should be easier for Subversion client developers to
>> understand and for them to DTRT.
>>
>> API differences between pegged and automatic merge:
>>
>> Pegged merge Automatic merge
>>
>> The 'find'API: Single merge_peg call does Separate'find' and 'do'..
>> the whole process. Result of 'find' ... used
>> used for 'svn mergeinfo' graph.
>>
>> Source: Revision ranges X:Y[,X:Y...] Revision range 0:Y
>> on branch URL@PEG on branch URL@PEG
>>
>> Target: optional non-WC tgt for 'find'
>>
>> If mergeinfo: skip already-merged revs (same)
>> record the merge
>>
>> No mergeinfo: don't skip revs error out
>> don't record
>>
>> Options differ: allow_mixed_rev allow_mixed_rev
>> allow_local_mods
>> allow_switched_subtrees
>> ignore_mergeinfo
>>
>> At that level, it looks close enough to be feasible to embed
>> 'automatic' merge inside 'pegged'. I'm looking closer now.
>>
>> Any thoughts?
Branko Čibej wrote:
> This is pretty much the same conclusion I came to earlier today. I looks
> like the easiest thing to do would be to make the _automatic_ APIs
> private within libsvn_client, move the selection logic to
> svn_client_merge_pegX, and simplify the client implementation.
>
> The only trouble with that is, as you say, that "svn mergeinfo" relies
> on having the automatic_find API available. I think this could be solved
> in several ways:
>
> 1. by splitting the merge-peg into two, as the current automatic-merge; or,
> 2. by making only the "do" phase of the automatic merge private, and
> wile leaving the "find" phase public -- but renaming it to something
> more in line with merginfo discovery; or,
> 3. somehow exposing the "find" phase through one of the existing (or
> revised) svn_client_mergeinfo_* APIs.
>
> I'm kind of leaning towards #3, but don't have a sense of how
> complicated that would be.
Mark Phippard wrote:
> There are obviously some benefits for having less API, especially when
> it comes time to rev it for something. That said, as a caller of the
> API, it seems nice that the separate "automatic merge" API can provide
> a more focused and simpler interface. For example, the interface does
> not need to expose things like a revision range, which should not
> apply when doing this kind of merge, but obviously is needed when
> doing a cherry-pick merge.
>
> Looking through a long list of API is hard sometime, but it is also
> hard when you want to do something like merge everything in BranchX
> and the interface requires passing a bunch of parameters that do not
> directly apply.
I like the focused API, but I also see how the automatic merge can be
considered to fill in a bit of missing functionality that merge_peg ought to
provide.
Perhaps we can have both. Teach merge_peg to DTRT in this case, and still have
the focused API available for when a client knows it wants an automatic merge.
Is there sufficient merit in that to outweigh the overhead of having to test
two similar but different entry points?
The attached patch moves the decision to call the 'automatic merge' API from
'svn' into 'svn_merge_peg5()'. I have run some merge tests and tree conflict
tests, but not the whole test suite yet. Here is the log msg.
[[[
Teach svn_client_merge_peg5() to do an automatic merge, and let 'svn merge'
rely on that instead of calling the dedicated automatic merge APIs.
TODO: Decide whether to keep or make private the 'automatic merge' APIs.
TODO: This reduces the verbosity of 'svn merge --verbose'. Consider
doing something about it, perhaps by adding some new notifications for the
notifier callback?
* subversion/include/svn_client.h,
subversion/libsvn_client/merge.c
(svn_client_merge_peg5): Do an automatic merge if no revision range
specified.
* subversion/svn/merge-cmd.c
(automatic_merge): Delete.
(run_merge): Don't take special action to handle an automatic merge, let
the pegged merge code path handle it.
]]]
Thoughts?
- Julian
Teach svn_client_merge_peg5() to do an automatic merge, and let 'svn merge'
rely on that instead of calling the dedicated automatic merge APIs.
TODO: Decide whether to keep or make private the 'automatic merge' APIs.
TODO: This reduces the verbosity of 'svn merge --verbose'. Consider
doing something about it, perhaps by adding some new notifications for the
notifier callback?
* subversion/include/svn_client.h,
subversion/libsvn_client/merge.c
(svn_client_merge_peg5): Do an automatic merge if no revision range
specified.
* subversion/svn/merge-cmd.c
(automatic_merge): Delete.
(run_merge): Don't take special action to handle an automatic merge, let
the pegged merge code path handle it.
--This line, and those below, will be ignored--
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1462264)
+++ subversion/include/svn_client.h (working copy)
@@ -3794,12 +3794,19 @@ svn_client_merge_reintegrate(const char
* subtractive merge ranges, they may overlap fully or partially,
* and/or they may partially or fully negate each other. This
* rangelist is not required to be sorted. If any revision in the
* list of provided ranges has an `unspecified' or unrecognized
* `kind', return #SVN_ERR_CLIENT_BAD_REVISION.
*
+ * If @a ranges_to_merge contains exactly one element, of which both the
+ * start and end revision numbers are SVN_INVALID_REVNUM, then perform an
+ * automatic merge as described in the documentation of
+ * svn_client_merge_find_automatic_merge() and
+ * svn_client_merge_do_automatic_merge(), with its attendant limitations
+ * on local modifications, mixed revisions and switched subtrees.
+ *
* All other options are handled identically to svn_client_merge5().
*
* @since New in 1.8.
*/
svn_error_t *
svn_client_merge_peg5(const char *source_path_or_url,
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1462264)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -11765,12 +11765,43 @@ svn_client_merge_peg5(const char *source
if (ranges_to_merge->nelts == 0)
return SVN_NO_ERROR;
SVN_ERR(get_target_and_lock_abspath(&target_abspath, &lock_abspath,
target_wcpath, ctx, pool));
+ /* Do an automatic merge if just one source and no revisions. */
+ if (! ignore_mergeinfo
+ && ranges_to_merge->nelts == 1
+ && APR_ARRAY_IDX(ranges_to_merge, 0, svn_merge_range_t *)->start
+ == SVN_INVALID_REVNUM
+ && APR_ARRAY_IDX(ranges_to_merge, 0, svn_merge_range_t *)->end
+ == SVN_INVALID_REVNUM)
+ {
+ svn_client_automatic_merge_t *merge;
+
+ /* Find the details of the merge needed. */
+ SVN_ERR(svn_client_find_automatic_merge(&merge,
+ source_path_or_url,
+ source_peg_revision,
+ target_wcpath,
+ allow_mixed_rev,
+ TRUE /*allow_local_mods*/,
+ TRUE /*allow_switched_subtrees*/,
+ ctx, pool, pool));
+
+ SVN_ERR(svn_client_do_automatic_merge(merge, target_wcpath, depth,
+ diff_ignore_ancestry,
+ force_delete, record_only,
+ dry_run, merge_options,
+ ctx, pool));
+
+ /* We already dealt with returning any conflict error, inside the
+ * above calls. */
+ conflict_report = NULL;
+ }
+ else
if (!dry_run)
SVN_WC__CALL_WITH_WRITE_LOCK(
merge_peg_locked(&conflict_report,
source_path_or_url, source_peg_revision,
ranges_to_merge,
target_abspath, depth, ignore_mergeinfo,
Index: subversion/svn/merge-cmd.c
===================================================================
--- subversion/svn/merge-cmd.c (revision 1462264)
+++ subversion/svn/merge-cmd.c (working copy)
@@ -60,89 +60,12 @@ ensure_wc_path_has_repo_revision(const c
_("Invalid merge source '%s'; a working copy path can only be "
"used with a repository revision (a number, a date, or head)"),
svn_dirent_local_style(path_or_url, scratch_pool));
return SVN_NO_ERROR;
}
-/* Automatic, merge-tracking merge, used for sync or reintegrate purposes. */
-static svn_error_t *
-automatic_merge(const char *source_path_or_url,
- const svn_opt_revision_t *source_revision,
- const char *target_wcpath,
- svn_depth_t depth,
- svn_boolean_t diff_ignore_ancestry,
- svn_boolean_t force_delete,
- svn_boolean_t record_only,
- svn_boolean_t dry_run,
- svn_boolean_t allow_mixed_rev,
- svn_boolean_t allow_local_mods,
- svn_boolean_t allow_switched_subtrees,
- svn_boolean_t verbose,
- const apr_array_header_t *merge_options,
- svn_client_ctx_t *ctx,
- apr_pool_t *scratch_pool)
-{
- svn_client_automatic_merge_t *merge;
-
- if (verbose)
- SVN_ERR(svn_cmdline_printf(scratch_pool, _("--- Checking branch relationship\n")));
- SVN_ERR_W(svn_cl__check_related_source_and_target(
- source_path_or_url, source_revision,
- target_wcpath, &unspecified_revision, ctx, scratch_pool),
- _("Source and target must be different but related branches"));
-
- if (verbose)
- SVN_ERR(svn_cmdline_printf(scratch_pool, _("--- Calculating automatic merge\n")));
-
- /* Find the 3-way merges needed (and check suitability of the WC). */
- SVN_ERR(svn_client_find_automatic_merge(&merge,
- source_path_or_url, source_revision,
- target_wcpath, allow_mixed_rev,
- allow_local_mods, allow_switched_subtrees,
- ctx, scratch_pool, scratch_pool));
-
- if (svn_client_automatic_merge_is_reintegrate_like(merge))
- {
- if (record_only)
- return svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
- _("The required merge is reintegrate-like, "
- "and the --record-only option "
- "cannot be used with this kind of merge"));
-
- if (depth != svn_depth_unknown)
- return svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
- _("The required merge is reintegrate-like, "
- "and the --depth option "
- "cannot be used with this kind of merge"));
-
- if (force_delete)
- return svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
- _("The required merge is reintegrate-like, "
- "and the --force option "
- "cannot be used with this kind of merge"));
-
- if (allow_mixed_rev)
- return svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
- _("The required merge is reintegrate-like, "
- "and the --allow-mixed-revisions option "
- "cannot be used with this kind of merge"));
- }
-
- if (verbose)
- SVN_ERR(svn_cmdline_printf(scratch_pool, _("--- Merging\n")));
-
- /* Perform the 3-way merges */
- SVN_ERR(svn_client_do_automatic_merge(merge, target_wcpath, depth,
- diff_ignore_ancestry,
- force_delete, record_only,
- dry_run, merge_options,
- ctx, scratch_pool));
-
- return SVN_NO_ERROR;
-}
-
/* Run a merge.
*
* (No docs yet, as this code was just hoisted out of svn_cl__merge().)
*
* Having FIRST_RANGE_START/_END params is ugly -- we should be able to use
* PEG_REVISION1/2 and/or RANGES_TO_MERGE instead, maybe adjusting the caller.
@@ -160,32 +83,13 @@ run_merge(svn_boolean_t two_sources_spec
apr_array_header_t *options,
svn_client_ctx_t *ctx,
apr_pool_t *scratch_pool)
{
svn_error_t *merge_err;
- /* Do an automatic merge if just one source and no revisions. */
- if ((! two_sources_specified)
- && (! opt_state->reintegrate)
- && (! opt_state->ignore_ancestry)
- && first_range_start.kind == svn_opt_revision_unspecified
- && first_range_end.kind == svn_opt_revision_unspecified)
- {
- merge_err = automatic_merge(sourcepath1, &peg_revision1, targetpath,
- opt_state->depth,
- FALSE /*diff_ignore_ancestry*/,
- opt_state->force, /* force_delete */
- opt_state->record_only,
- opt_state->dry_run,
- opt_state->allow_mixed_rev,
- TRUE /*allow_local_mods*/,
- TRUE /*allow_switched_subtrees*/,
- opt_state->verbose,
- options, ctx, scratch_pool);
- }
- else if (opt_state->reintegrate)
+ if (opt_state->reintegrate)
{
merge_err = svn_client_merge_reintegrate(
sourcepath1, &peg_revision1, targetpath,
opt_state->dry_run, options, ctx, scratch_pool);
}
else if (! two_sources_specified)
