While trying to improve input validation in svn merge, I stumbled
across an apparently unsupported use case.
It does not seem possible right now to merge into locally added
files, because the Subversion assumes that the merge target will
always have a corresponding URL in the repository, and errors out.
With a bit of special-casing during error handling in a few places,
I succeeded in making this use case work:
$ svn diff ^/trunk/alpha_at_3 ^/trunk/alpha_at_4
Index: alpha
===================================================================
--- alpha (revision 3)
+++ alpha (revision 4)
@@ -1 +1,2 @@
alpha
+aaa
$ echo foo > foo
$ svn add foo
A foo
$ svn merge ^/trunk/alpha_at_3 ^/trunk/alpha_at_4 foo
Conflict discovered in '/tmp/svn-sandbox/branch/foo'.
Select: (p) postpone, (df) diff-full, (e) edit,
(mc) mine-conflict, (tc) theirs-conflict,
(s) show all options: p
--- Merging r4 into 'foo':
C foo
--- Recording mergeinfo for merge of r4 into 'foo':
U foo
Summary of conflicts:
Text conflicts: 1
$ svn diff
Index: foo
===================================================================
--- foo (revision 0)
+++ foo (working copy)
@@ -0,0 +1,6 @@
+<<<<<<< .working
+foo
+=======
+alpha
+aaa
+>>>>>>> .merge-right.r4
Property changes on: foo
___________________________________________________________________
Added: svn:mergeinfo
Merged /trunk/alpha:r4
$
A no-op change also works fine, it creates mergeinfo on the added file:
$ svn add foo
A foo
$ svn merge ^/trunk/alpha_at_2 ^/trunk/alpha_at_3 foo
--- Recording mergeinfo for merge of r3 into 'foo':
U foo
$ svn st
A foo
$ svn diff
Index: foo
===================================================================
--- foo (revision 0)
+++ foo (working copy)
@@ -0,0 +1 @@
+foo
Property changes on: foo
___________________________________________________________________
Added: svn:mergeinfo
Merged /trunk/alpha:r3
Is this new behaviour something we want, or is it undesirable for
some reason? I cannot think of a good reason to forbid such merges.
A related error is where svn tries to get the repository URL for
every target during the first phase of command line processing,
if one of the targets is a repository-relative URL.
That's clearly a bogus error, and is fixed by the patch, too.
The patch also sprinkles some additional input validation checks,
and adds a new test, which passes.
The of the new errors being thrown should enhance user experience.
For instance, in one case, the error message shown to the user was
the dreaded "Cannot replace directory from within", while all that
happened was that input arguments didn't confirm to the syntax
desribed in the help text. Which is... unfriendly.
Before writing a log message and committing this I'd like to know
whether the behaviour change for added files is sound. And maybe
I should split the patch up to isolate the behaviour change?
I haven't run make check yet either, in fear that lots of merge
tests will just start to fail on me when I try.
Comments welcome.
Thanks,
Stefan
Index: subversion/tests/cmdline/input_validation_tests.py
===================================================================
--- subversion/tests/cmdline/input_validation_tests.py (revision 982221)
+++ subversion/tests/cmdline/input_validation_tests.py (working copy)
@@ -149,6 +149,26 @@ def invalid_log_targets(sbox):
"specified after a URL for 'svn log', but.*is " +
"not a relative path", 'log', target1, target2)
+def invalid_merge_args(sbox):
+ "invalid arguments for 'merge'"
+ sbox.build(read_only=True)
+ run_and_verify_svn_in_wc(sbox, "svn: A working copy merge source needs "
+ "an explicit revision", 'merge', 'iota', '^/')
+ for (src, target) in [('iota_at_HEAD', '^/'), ('iota_at_BASE', 'file://')]:
+ run_and_verify_svn_in_wc(sbox, "svn: Merge sources must both be either "
+ "paths or URLs", 'merge', src, target)
+ run_and_verify_svn_in_wc(sbox, "svn: Merge target.*does not exist in " +
+ "the working copy",
+ 'merge', 'iota_at_BASE', 'iota_at_HEAD', 'nonexistent')
+ run_and_verify_svn_in_wc(sbox, "svn: Too many arguments given",
+ 'merge', '-c42', '^/A/B', '^/A/C', 'iota')
+ run_and_verify_svn_in_wc(sbox, "svn: Cannot specify a revision range with" +
+ " two URLs", 'merge', '-c42', '^/mu', '^/')
+ run_and_verify_svn_in_wc(sbox, "svn: Merge target.*does not exist in " +
+ "the working copy",
+ 'merge', '-c42', '^/mu', 'nonexistent')
+
+
########################################################################
# Run the tests
@@ -165,6 +185,7 @@ test_list = [ None,
invalid_export_targets,
invalid_import_args,
invalid_log_targets,
+ invalid_merge_args,
]
if __name__ == '__main__':
Index: subversion/svn/merge-cmd.c
===================================================================
--- subversion/svn/merge-cmd.c (revision 982221)
+++ subversion/svn/merge-cmd.c (working copy)
@@ -337,6 +337,11 @@ svn_cl__merge(apr_getopt_t *os,
}
else
{
+ if (svn_path_is_url(sourcepath1) != svn_path_is_url(sourcepath2))
+ return svn_error_return(svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR,
+ NULL,
+ _("Merge sources must both be "
+ "either paths or URLs")));
err = svn_client_merge3(sourcepath1,
&first_range_start,
sourcepath2,
Index: subversion/libsvn_client/cmdline.c
===================================================================
--- subversion/libsvn_client/cmdline.c (revision 982221)
+++ subversion/libsvn_client/cmdline.c (working copy)
@@ -125,11 +125,14 @@ check_root_url_of_target(const char **root_url,
* If the target itself is a URL to a repository that does not exist,
* that's fine, too. The callers will deal with this argument in an
* appropriate manter if it does not make any sense.
+ *
+ * Also tolerate locally added targets ("bad revision" error).
*/
if ((err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
|| (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
|| (err->apr_err == SVN_ERR_WC_NOT_WORKING_COPY)
- || (err->apr_err == SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED))
+ || (err->apr_err == SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED)
+ || (err->apr_err == SVN_ERR_CLIENT_BAD_REVISION))
{
svn_error_clear(err);
return SVN_NO_ERROR;
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 982221)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -3343,6 +3343,7 @@ get_full_mergeinfo(svn_mergeinfo_t *recorded_merge
svn_revnum_t target_rev;
svn_opt_revision_t peg_revision;
apr_pool_t *sesspool = NULL;
+ svn_error_t *err;
/* Assert that we have sane input. */
SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(start)
@@ -3350,10 +3351,27 @@ get_full_mergeinfo(svn_mergeinfo_t *recorded_merge
&& (start > end));
peg_revision.kind = svn_opt_revision_working;
- SVN_ERR(svn_client__derive_location(&url, &target_rev, target_abspath,
- &peg_revision, ra_session,
- ctx, result_pool, scratch_pool));
+ err = svn_client__derive_location(&url, &target_rev, target_abspath,
+ &peg_revision, ra_session,
+ ctx, result_pool, scratch_pool);
+ if (err)
+ {
+ if (err->apr_err == SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED)
+ {
+ /* We've been asked to operate on a target which has no location
+ * in the repository. Either it's unversioned (but attempts to
+ * merge into unversioned targets should not get as far as here),
+ * or it is locally added, in which case the target's implicit
+ * mergeinfo is empty. */
+ svn_error_clear(err);
+ *implicit_mergeinfo = apr_hash_make(result_pool);
+ return SVN_NO_ERROR;
+ }
+ else
+ return svn_error_return(err);
+ }
+
if (target_rev <= end)
{
/* We're asking about a range outside our natural history
@@ -8293,6 +8311,11 @@ do_merge(apr_hash_t **modified_subtrees,
SVN_ERR(svn_wc_read_kind(&target_kind, ctx->wc_ctx, target_abspath, FALSE,
pool));
+ if (target_kind != svn_node_dir && target_kind != svn_node_file)
+ return svn_error_return(svn_error_createf(
+ SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Merge target '%s' does not exist in the "
+ "working copy"), target_abspath));
/* Ensure a known depth. */
if (depth == svn_depth_unknown)
@@ -8670,12 +8693,18 @@ merge_locked(const char *source1,
apr_pool_t *sesspool;
svn_boolean_t same_repos;
const char *source_repos_uuid1, *source_repos_uuid2;
+ svn_node_kind_t target_kind;
- /* Sanity check our input -- we require specified revisions. */
+ /* Sanity check our input -- we require specified revisions,
+ * and either 2 paths or 2 URLs. */
if ((revision1->kind == svn_opt_revision_unspecified)
|| (revision2->kind == svn_opt_revision_unspecified))
return svn_error_create(SVN_ERR_CLIENT_BAD_REVISION, NULL,
_("Not all required revisions are specified"));
+ if (svn_path_is_url(source1) != svn_path_is_url(source2))
+ return svn_error_return(svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Merge sources must both be "
+ "either paths or URLs")));
/* ### FIXME: This function really ought to do a history check on
the left and right sides of the merge source, and -- if one is an
@@ -8703,6 +8732,14 @@ merge_locked(const char *source1,
_("'%s' has no URL"),
svn_dirent_local_style(source2, scratch_pool));
+ SVN_ERR(svn_wc_read_kind(&target_kind, ctx->wc_ctx, target_abspath, FALSE,
+ scratch_pool));
+ if (target_kind != svn_node_dir && target_kind != svn_node_file)
+ return svn_error_return(svn_error_createf(
+ SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Merge target '%s' does not exist in the "
+ "working copy"), target_abspath));
+
/* 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,
@@ -10193,6 +10230,7 @@ merge_peg_locked(const char *source,
svn_boolean_t use_sleep = FALSE;
svn_error_t *err;
svn_boolean_t same_repos;
+ svn_node_kind_t target_kind;
SVN_ERR_ASSERT(svn_dirent_is_absolute(target_abspath));
@@ -10204,6 +10242,14 @@ merge_peg_locked(const char *source,
_("'%s' has no URL"),
svn_dirent_local_style(source, scratch_pool));
+ SVN_ERR(svn_wc_read_kind(&target_kind, ctx->wc_ctx, target_abspath, FALSE,
+ scratch_pool));
+ if (target_kind != svn_node_dir && target_kind != svn_node_file)
+ return svn_error_return(svn_error_createf(
+ SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Merge target '%s' does not exist in the "
+ "working copy"), target_abspath));
+
/* 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,
Received on 2010-08-04 16:30:35 CEST