On Fri, Oct 01, 2010 at 03:58:53PM +0100, Julian Foad wrote:
> On Fri, 2010-10-01 at 09:16 -0400, Paul Burba wrote:
> > On Fri, Oct 1, 2010 at 7:25 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> > > Stefan Sperling wrote:
> > >> I'd like to propose that we disallow merges into mixed-revision working
> > >> copies by default for 1.7, and only allow such merges if a special command
> > >> line option has been passed.
> >
> > What will the behavior be during --dry-run merges?
> >
> > A) No change
> >
> > B) Warn that your target is mixed-rev, but proceed as we always did
> >
> > C) Fail unless the new option is used.
>
> It should be (C): a dry-run should be as close as possible to the "real"
> behaviour. I can't see any benefit in making a dry-run merge behave
> differently from a non-dry-run merge in this respect.
Yes.
>
> > What about --record-only merges? I think here it makes sense to
> > mirror whatever behavior we decide upon for a "real" merge-tracking
> > aware merge.
>
> Agreed.
I agree, too.
> > >> The reason for this is that merges into mixed-rev WCs can easily create
> > >> conflicts which are tedious to resolve. Even if you know about the problem
> > >> and try to run "svn update" before every merge, it's quite easy to forget
> > >> about running "svn update".
> > >>
> > >> Also, we've been advertising this as best practice for some time.
> > >> Quoting the book on the matter:
> > >> Never merge into working copies with a mixture of working revision
> > >> numbers, or with “switched” subdirectories (as described next in the
> > >> section called “Traversing Branches”). A merge target should be a
> > >> working copy which represents a single location in the repository at a
> > >> single point in time.
> > >>
> > >> Note that while the book recommends not to merge into working copies
> > >> with switched subtrees, I don't intend to enforce this by default.
> > >>
> > >> I've talked about this with Paul on IRC and he had no objections.
> > >> See http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-09-30#l468
> > >> for the full discussion.
> > >>
> > >> Below is a work-in-progress diff that starts making this change.
> > >> Tests fail with it. I've only checked the merge tests for now.
> > >>
> > >> Before continuing with this, fixing up tests, bumping APIs, and eventually
> > >> adding the command line option that allows merges into mixed-rev WCs,
> > >> I'd like to ask whether anyone has concerns about this idea.
> > >
> > > The proposal sounds good to me.
> > >
> > > To be more specific about what the proposal is:
> > >
> > > * It does not apply to re-integrate merges. (They always enforce this
> > > anyway?)
> >
> > Right, there should be *no* override on reintegrate's requirement for
> > a uniform rev target.
> >
> > > * It applies to merge-tracking merges.
> > >
> > > * It [applies/does not appy] to non-merge-tracking merges?
> >
> > Stefan and I didn't discuss these last two. I'm inclined to think that
> > this warning should apply only to merge-tracking aware merges.
>
> I have no opinion on that.
I think we should check for mixed-revisions in non-merge-tracking-aware
merges also.
> > > As for the tests: I think it would be best to keep some or all of the
> > > tests doing merges into a mixed-rev WC as they do now, partly so as not
> > > to lose test coverage of this condition, but more importantly to
> > > preserve the purpose of any regression tests that happen to be testing
> > > for bugs which only showed up in mixed-rev mode.
> > >
> > > In other words, fix most of them by adding the bypass flag rather than
> > > by adding an "svn update" command.
> >
> > Good point, that makes more sense.
> >
> > Paul
>
>
> - Julian
>
Below is an updated work-in-progress patch (no log message yet, sorry).
It adds a new --allow-mixed-revisions option to the svn merge command.
This option is purpusefully not mentioned in the error message, because
the point is that we want people to run update unless they really have
a good reason not to do so.
Please review and/or help fix failing tests if you like :)
Stefan
Index: subversion/svn/merge-cmd.c
===================================================================
--- subversion/svn/merge-cmd.c (revision 1026107)
+++ subversion/svn/merge-cmd.c (working copy)
@@ -293,6 +293,10 @@ svn_cl__merge(apr_getopt_t *os,
return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("--reintegrate can only be used with "
"a single merge source"));
+ if (opt_state->allow_mixed_rev)
+ return svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
+ _("--allow-mixed-revisions cannot be used "
+ "with --reintegrate"));
}
if (! two_sources_specified) /* TODO: Switch order of if */
@@ -322,7 +326,7 @@ svn_cl__merge(apr_getopt_t *os,
opt_state->dry_run,
options, ctx, pool);
else
- err = svn_client_merge_peg3(sourcepath1,
+ err = svn_client_merge_peg4(sourcepath1,
ranges_to_merge,
&peg_revision1,
targetpath,
@@ -331,6 +335,7 @@ svn_cl__merge(apr_getopt_t *os,
opt_state->force,
opt_state->record_only,
opt_state->dry_run,
+ opt_state->allow_mixed_rev,
options,
ctx,
pool);
@@ -342,7 +347,7 @@ svn_cl__merge(apr_getopt_t *os,
NULL,
_("Merge sources must both be "
"either paths or URLs")));
- err = svn_client_merge3(sourcepath1,
+ err = svn_client_merge4(sourcepath1,
&first_range_start,
sourcepath2,
&first_range_end,
@@ -352,6 +357,7 @@ svn_cl__merge(apr_getopt_t *os,
opt_state->force,
opt_state->record_only,
opt_state->dry_run,
+ opt_state->allow_mixed_rev,
options,
ctx,
pool);
Index: subversion/svn/cl.h
===================================================================
--- subversion/svn/cl.h (revision 1026107)
+++ subversion/svn/cl.h (working copy)
@@ -230,6 +230,7 @@ typedef struct svn_cl__opt_state_t
svn_boolean_t internal_diff; /* override diff_cmd in config file */
svn_boolean_t use_git_diff_format; /* Use git's extended diff format */
svn_boolean_t old_patch_target_names; /* Use target names from old side */
+ svn_boolean_t allow_mixed_rev; /* Allow operation on mixed-revision WC */
} svn_cl__opt_state_t;
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c (revision 1026107)
+++ subversion/svn/main.c (working copy)
@@ -123,6 +123,7 @@ typedef enum {
opt_internal_diff,
opt_use_git_diff_format,
opt_old_patch_target_names,
+ opt_allow_mixed_revisions,
} svn_cl__longopt_t;
#define SVN_CL__OPTION_CONTINUATION_INDENT " "
@@ -396,6 +397,10 @@ const apr_getopt_option_t svn_cl__options[] =
"this option will cause the name \"foo.c\" to be used\n"
SVN_CL__OPTION_CONTINUATION_INDENT
"[alias: --optn]")},
+ {"allow-mixed-revisions", opt_allow_mixed_revisions, 0,
+ N_("Allow merge into mixed-revision working copy\n"
+ SVN_CL__OPTION_CONTINUATION_INDENT
+ "[alias: --amix]")},
/* Long-opt Aliases
*
@@ -428,6 +433,7 @@ const apr_getopt_option_t svn_cl__options[] =
{"nul", opt_no_unlock, 0, NULL},
{"keep-lock", opt_no_unlock, 0, NULL},
{"optn", opt_old_patch_target_names, 0, NULL},
+ {"amix", opt_allow_mixed_revisions, 0, NULL},
{0, 0, 0, 0},
};
@@ -786,7 +792,8 @@ const svn_opt_subcommand_desc2_t svn_cl__cmd_table
" The --ignore-ancestry option overrides this, forcing Subversion to\n"
" regard the sources as unrelated and not to track the merge.\n"),
{'r', 'c', 'N', opt_depth, 'q', opt_force, opt_dry_run, opt_merge_cmd,
- opt_record_only, 'x', opt_ignore_ancestry, opt_accept, opt_reintegrate} },
+ opt_record_only, 'x', opt_ignore_ancestry, opt_accept, opt_reintegrate,
+ opt_allow_mixed_revisions} },
{ "mergeinfo", svn_cl__mergeinfo, {0}, N_
("Display merge-related information.\n"
@@ -1841,6 +1848,9 @@ main(int argc, const char *argv[])
case opt_old_patch_target_names:
opt_state.old_patch_target_names = TRUE;
break;
+ case opt_allow_mixed_revisions:
+ opt_state.allow_mixed_rev = TRUE;
+ break;
default:
/* Hmmm. Perhaps this would be a good place to squirrel away
opts that commands like svn diff might need. Hmmm indeed. */
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1026107)
+++ subversion/include/svn_client.h (working copy)
@@ -3068,9 +3068,39 @@ svn_client_diff_summarize_peg(const char *path,
* If @a dry_run is TRUE, the merge is carried out, and full notification
* feedback is provided, but the working copy is not modified.
*
+ * If allow_mixed_rev is @c FALSE, and @a merge_target is a mixed-revision
+ * working copy, raise @c SVN_ERR_CLIENT_NOT_READY_TO_MERGE.
+ * Because users rarely intend to merge into mixed-revision working copies,
+ * it is recommended to set this parameter to FALSE by default unless the
+ * user has explicitly requested a merge into a mixed-revision working copy.
+ *
* The authentication baton cached in @a ctx is used to communicate with the
* repository.
*
+ * @since New in 1.7.
+ */
+svn_error_t *
+svn_client_merge4(const char *source1,
+ const svn_opt_revision_t *revision1,
+ const char *source2,
+ const svn_opt_revision_t *revision2,
+ const char *target_wcpath,
+ svn_depth_t depth,
+ svn_boolean_t ignore_ancestry,
+ svn_boolean_t force,
+ svn_boolean_t record_only,
+ svn_boolean_t dry_run,
+ svn_boolean_t allow_mixed_rev,
+ const apr_array_header_t *merge_options,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_client_merge4(), but with @a allow_mixed_rev set to
+ * @c TRUE.
+ *
+ * @deprecated Provided for backward compatibility with the 1.6 API.
+ *
* @since New in 1.5.
*/
svn_error_t *
@@ -3172,8 +3202,31 @@ svn_client_merge_reintegrate(const char *source,
* list of provided ranges has an `unspecified' or unrecognized
* `kind', return #SVN_ERR_CLIENT_BAD_REVISION.
*
- * All other options are handled identically to svn_client_merge3().
+ * All other options are handled identically to svn_client_merge4().
*
+ * @since New in 1.7.
+ */
+svn_error_t *
+svn_client_merge_peg4(const char *source,
+ const apr_array_header_t *ranges_to_merge,
+ const svn_opt_revision_t *peg_revision,
+ const char *target_wcpath,
+ svn_depth_t depth,
+ svn_boolean_t ignore_ancestry,
+ svn_boolean_t force,
+ svn_boolean_t record_only,
+ svn_boolean_t dry_run,
+ svn_boolean_t allow_mixed_rev,
+ const apr_array_header_t *merge_options,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_client_merge_peg4(), but with @a allow_mixed_rev set to
+ * @c TRUE.
+ *
+ * @deprecated Provided for backward compatibility with the 1.6 API.
+ *
* @since New in 1.5.
*/
svn_error_t *
Index: subversion/libsvn_client/deprecated.c
===================================================================
--- subversion/libsvn_client/deprecated.c (revision 1026107)
+++ subversion/libsvn_client/deprecated.c (working copy)
@@ -1338,6 +1338,27 @@ svn_client_log(const apr_array_header_t *targets,
/*** From merge.c ***/
svn_error_t *
+svn_client_merge3(const char *source1,
+ const svn_opt_revision_t *revision1,
+ const char *source2,
+ const svn_opt_revision_t *revision2,
+ const char *target_wcpath,
+ svn_depth_t depth,
+ svn_boolean_t ignore_ancestry,
+ svn_boolean_t force,
+ svn_boolean_t record_only,
+ svn_boolean_t dry_run,
+ const apr_array_header_t *merge_options,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ return svn_client_merge4(source1, revision1, source2, revision2,
+ target_wcpath, depth, ignore_ancestry, force,
+ record_only, dry_run, TRUE, merge_options,
+ ctx, pool);
+}
+
+svn_error_t *
svn_client_merge2(const char *source1,
const svn_opt_revision_t *revision1,
const char *source2,
@@ -1376,8 +1397,26 @@ svn_client_merge(const char *source1,
dry_run, NULL, ctx, pool);
}
+svn_error_t *
+svn_client_merge_peg3(const char *source,
+ const apr_array_header_t *ranges_to_merge,
+ const svn_opt_revision_t *peg_revision,
+ const char *target_wcpath,
+ svn_depth_t depth,
+ svn_boolean_t ignore_ancestry,
+ svn_boolean_t force,
+ svn_boolean_t record_only,
+ svn_boolean_t dry_run,
+ const apr_array_header_t *merge_options,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ return svn_client_merge_peg4(source, ranges_to_merge, peg_revision,
+ target_wcpath, depth, ignore_ancestry, force,
+ record_only, dry_run, TRUE, merge_options,
+ ctx, pool);
+}
-
svn_error_t *
svn_client_merge_peg2(const char *source,
const svn_opt_revision_t *revision1,
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1026107)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -8862,6 +8862,61 @@ merge_cousins_and_supplement_mergeinfo(const char
return SVN_NO_ERROR;
}
+/* Perform checks to determine whether of the working copy at TARGET_ABSPATH
+ * can safely be used as a merge target. Checks are performed according to
+ * the ALLOW_MIXED_REV, ALLOW_LOCAL_MODS, and ALLOW_SWITCHED_SUBTREES
+ * parameters. If any checks fail, raise SVN_ERR_CLIENT_NOT_READY_TO_MERGE.
+ *
+ * E.g. if all the ALLOW_* parameters are FALSE, TARGET_ABSPATH must
+ * be a single-revision, pristine, unswitched working copy.
+ * In other words, it must reflect a subtree of the repostiory as found
+ * at single revision -- although sparse checkouts are permitted. */
+static svn_error_t *
+ensure_wc_is_suitable_merge_target(const char *target_abspath,
+ svn_client_ctx_t *ctx,
+ svn_boolean_t allow_mixed_rev,
+ svn_boolean_t allow_local_mods,
+ svn_boolean_t allow_switched_subtrees,
+ apr_pool_t *scratch_pool)
+{
+ svn_wc_revision_status_t *wc_stat;
+
+ /* Avoid the following status crawl if we don't need it. */
+ if (allow_mixed_rev && allow_local_mods && allow_switched_subtrees)
+ return SVN_NO_ERROR;
+
+ /* Get a WC summary with min/max revisions set to the BASE revision. */
+ SVN_ERR(svn_wc_revision_status2(&wc_stat, ctx->wc_ctx, target_abspath, NULL,
+ FALSE, ctx->cancel_func, ctx->cancel_baton,
+ scratch_pool, scratch_pool));
+
+ if (! allow_switched_subtrees && wc_stat->switched)
+ return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
+ _("Cannot merge into a working copy "
+ "with a switched subtree"));
+
+ if (! allow_local_mods && wc_stat->modified)
+ return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
+ _("Cannot merge into a working copy "
+ "that has local modifications"));
+
+ if (! allow_mixed_rev)
+ {
+ if (! (SVN_IS_VALID_REVNUM(wc_stat->min_rev)
+ && SVN_IS_VALID_REVNUM(wc_stat->max_rev)))
+ return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
+ _("Cannot determine revision of working copy"));
+
+ if (wc_stat->min_rev != wc_stat->max_rev)
+ return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
+ _("Cannot merge into mixed-revision "
+ "working copy; try updating first"));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
/*-----------------------------------------------------------------------*/
/*** Public APIs ***/
@@ -8877,6 +8932,7 @@ merge_locked(const char *source1,
svn_boolean_t force,
svn_boolean_t record_only,
svn_boolean_t dry_run,
+ svn_boolean_t allow_mixed_rev,
const apr_array_header_t *merge_options,
svn_client_ctx_t *ctx,
apr_pool_t *scratch_pool)
@@ -8952,6 +9008,12 @@ merge_locked(const char *source1,
_("Merge target '%s' does not exist in the "
"working copy"), target_abspath));
+
+ /* Do not allow merges into mixed-revision working copies. */
+ SVN_ERR(ensure_wc_is_suitable_merge_target(target_abspath, ctx,
+ allow_mixed_rev, TRUE, TRUE,
+ scratch_pool));
+
/* Determine the working copy target's repository root URL. */
working_rev.kind = svn_opt_revision_working;
SVN_ERR(svn_client__get_repos_root(&wc_repos_root, target_abspath,
@@ -9150,6 +9212,7 @@ struct merge_baton {
svn_boolean_t force;
svn_boolean_t record_only;
svn_boolean_t dry_run;
+ svn_boolean_t allow_mixed_rev;
const apr_array_header_t *merge_options;
svn_client_ctx_t *ctx;
};
@@ -9163,7 +9226,8 @@ merge_cb(void *baton, apr_pool_t *result_pool, apr
SVN_ERR(merge_locked(b->source1, b->revision1, b->source2, b->revision2,
b->target_abspath, b->depth, b->ignore_ancestry,
b->force, b->record_only, b->dry_run,
- b->merge_options, b->ctx, scratch_pool));
+ b->allow_mixed_rev, b->merge_options, b->ctx,
+ scratch_pool));
return SVN_NO_ERROR;
}
@@ -9191,7 +9255,7 @@ get_target_and_lock_abspath(const char **target_ab
}
svn_error_t *
-svn_client_merge3(const char *source1,
+svn_client_merge4(const char *source1,
const svn_opt_revision_t *revision1,
const char *source2,
const svn_opt_revision_t *revision2,
@@ -9201,6 +9265,7 @@ svn_error_t *
svn_boolean_t force,
svn_boolean_t record_only,
svn_boolean_t dry_run,
+ svn_boolean_t allow_mixed_rev,
const apr_array_header_t *merge_options,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
@@ -9221,6 +9286,7 @@ svn_error_t *
baton.force = force;
baton.record_only = record_only;
baton.dry_run = dry_run;
+ baton.allow_mixed_rev = allow_mixed_rev;
baton.merge_options = merge_options;
baton.ctx = ctx;
@@ -9235,45 +9301,6 @@ svn_error_t *
}
-/* If TARGET_WCPATH does not reflect a single-revision, pristine,
- unswitched working copy -- in other words, a subtree found in a
- single revision (although sparse checkouts are permitted) -- raise
- SVN_ERR_CLIENT_NOT_READY_TO_MERGE. */
-static svn_error_t *
-ensure_wc_reflects_repository_subtree(const char *target_abspath,
- svn_client_ctx_t *ctx,
- apr_pool_t *scratch_pool)
-{
- svn_wc_revision_status_t *wc_stat;
-
- /* Get a WC summary with min/max revisions set to the BASE revision. */
- SVN_ERR(svn_wc_revision_status2(&wc_stat, ctx->wc_ctx, target_abspath, NULL,
- FALSE, ctx->cancel_func, ctx->cancel_baton,
- scratch_pool, scratch_pool));
-
- if (wc_stat->switched)
- return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
- _("Cannot reintegrate into a working copy "
- "with a switched subtree"));
-
- if (wc_stat->modified)
- return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
- _("Cannot reintegrate into a working copy "
- "that has local modifications"));
-
- if (! (SVN_IS_VALID_REVNUM(wc_stat->min_rev)
- && SVN_IS_VALID_REVNUM(wc_stat->max_rev)))
- return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
- _("Cannot determine revision of working copy"));
-
- if (wc_stat->min_rev != wc_stat->max_rev)
- return svn_error_create(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
- _("Cannot reintegrate into mixed-revision "
- "working copy; try updating first"));
-
- return SVN_NO_ERROR;
-}
-
/* Check if mergeinfo for a given path is described explicitly or via
inheritance in a mergeinfo catalog.
@@ -10221,8 +10248,11 @@ merge_reintegrate_locked(const char *source,
svn_dirent_local_style(target_abspath,
scratch_pool));
- SVN_ERR(ensure_wc_reflects_repository_subtree(target_abspath, ctx,
- scratch_pool));
+ /* A reintegrate merge requires the merge target to reflect a subtree
+ * of the repository as found at a single revision. */
+ SVN_ERR(ensure_wc_is_suitable_merge_target(target_abspath, ctx,
+ FALSE, FALSE, FALSE,
+ scratch_pool));
SVN_ERR(svn_wc__node_get_base_rev(&target_base_rev, ctx->wc_ctx,
target_abspath, scratch_pool));
@@ -10438,6 +10468,7 @@ merge_peg_locked(const char *source,
svn_boolean_t force,
svn_boolean_t record_only,
svn_boolean_t dry_run,
+ svn_boolean_t allow_mixed_rev,
const apr_array_header_t *merge_options,
svn_client_ctx_t *ctx,
apr_pool_t *scratch_pool)
@@ -10479,6 +10510,10 @@ merge_peg_locked(const char *source,
_("Merge target '%s' does not exist in the "
"working copy"), target_abspath));
+ SVN_ERR(ensure_wc_is_suitable_merge_target(target_abspath, ctx,
+ allow_mixed_rev, TRUE, TRUE,
+ scratch_pool));
+
/* Determine the working copy target's repository root URL. */
working_rev.kind = svn_opt_revision_working;
SVN_ERR(svn_client__get_repos_root(&wc_repos_root, target_abspath,
@@ -10541,6 +10576,7 @@ struct merge_peg_baton {
svn_boolean_t force;
svn_boolean_t record_only;
svn_boolean_t dry_run;
+ svn_boolean_t allow_mixed_rev;
const apr_array_header_t *merge_options;
svn_client_ctx_t *ctx;
};
@@ -10554,13 +10590,14 @@ merge_peg_cb(void *baton, apr_pool_t *result_pool,
SVN_ERR(merge_peg_locked(b->source, b->ranges_to_merge, b->peg_revision,
b->target_abspath, b->depth, b->ignore_ancestry,
b->force, b->record_only, b->dry_run,
- b->merge_options, b->ctx, scratch_pool));
+ b->allow_mixed_rev, b->merge_options, b->ctx,
+ scratch_pool));
return SVN_NO_ERROR;
}
svn_error_t *
-svn_client_merge_peg3(const char *source,
+svn_client_merge_peg4(const char *source,
const apr_array_header_t *ranges_to_merge,
const svn_opt_revision_t *peg_revision,
const char *target_wcpath,
@@ -10569,6 +10606,7 @@ svn_error_t *
svn_boolean_t force,
svn_boolean_t record_only,
svn_boolean_t dry_run,
+ svn_boolean_t allow_mixed_rev,
const apr_array_header_t *merge_options,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
@@ -10592,6 +10630,7 @@ svn_error_t *
baton.force = force;
baton.record_only = record_only;
baton.dry_run = dry_run;
+ baton.allow_mixed_rev = allow_mixed_rev;
baton.merge_options = merge_options;
baton.ctx = ctx;
Received on 2010-10-21 22:37:45 CEST