On 2006-2-16 Julian Foad <julianfoad@btopenworld.com> wrote:
> It seems likely that this will be accepted in principle, so I'm reviewing.
>
> It all looks absolutely fine, log message and code, except
> that it would be nice if the new test function were not an
> almost exact copy of an existing one but somehow shared much
> of its implementation. That's probably not strictly
> necessary, though.
>
> (If you try to re-apply this patch to the latest trunk code,
> watch out: we've just removed spaces after function names
> throughout the code, so you'll need to adjust the patch
> first. )
Thanks for the review Julian.
Here is a new patch to work with the new space changes and to
address your test duplication concerns, with which I agree.
In merge_tests.py I changed 'merge_one_file' and
'merge_with_implicit_target' to accept an 'arg_flav' option and
renamed them to *_helper. The helper is then called by two stub
routines with 'r' or with 'c'. So essentially those two tests
are run twice, but the bulk of the test is the same.
The rest of the diffs are the same aside from the paren/space changes.
-- bart
[[[
Extend -c/--change option in diff and merge to accept negative
revisions as meaning the reverse of a change, making it possible to
back out changes with -c rather than -r. E.g., -c -1234 is the same
as -r1234:1233.
* subversion/svn/main.c
(svn_cl__options): Update docstring for '--change'
(svn_cl__cmd_table): Update 'diff' and 'merge' docstrings.
(main): Handle parsing for negative revision number.
* subversion/tests/cmdline/diff_tests.py
(diff_only_property_change): Add checks for '-c' with negative and
positive args.
* subversion/tests/cmdline/merge_tests.py
(merge_one_file, merge_with_implicit_target): Renamed to '*_helper'.
Added 'arg_flav' arg to indicate whether to use '-r' or '-c'.
(merge_one_file_using_r): Added.
(merge_one_file_using_c): Added.
(merge_one_file_with_c): Replace with 'merge_one_file_using_c' stub.
(merge_with_implicit_target_using_r): Added.
(merge_with_implicit_target_using_c): Added.
(test_list): Add the new tests.
]]]
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c (revision 18522)
+++ subversion/svn/main.c (working copy)
@@ -66,7 +66,10 @@ const apr_getopt_option_t svn_cl__option
{"quiet", 'q', 0, N_("print as little as possible")},
{"recursive", 'R', 0, N_("descend recursively")},
{"non-recursive", 'N', 0, N_("operate on single directory only")},
- {"change", 'c', 1, N_("the change made by revision ARG (like -r ARG-1:ARG)")},
+ {"change", 'c', 1, N_
+ ("the change made by revision ARG (like -r ARG-1:ARG)\n"
+ " If ARG is negative this is like -r ARG:ARG-1")
+ },
{"revision", 'r', 1, N_
("ARG (some commands also take ARG1:ARG2 range)\n"
" A revision argument can be one of:\n"
@@ -284,6 +287,7 @@ const svn_opt_subcommand_desc2_t svn_cl_
" must be specified. M defaults to the current working version if any\n"
" TARGET is a working copy path, otherwise it defaults to HEAD.\n"
" The '-c M' option is equivalent to '-r N:M' where N = M-1.\n"
+ " Using '-c -M' does the reverse: '-r M:N' where N = M-1.\n"
"\n"
" 2. Display the differences between OLD-TGT as it was seen in OLDREV and\n"
" NEW-TGT as it was seen in NEWREV. PATHs, if given, are relative to\n"
@@ -292,6 +296,7 @@ const svn_opt_subcommand_desc2_t svn_cl_
" NEW-TGT defaults to OLD-TGT if not specified. -r N makes OLDREV default\n"
" to N, -r N:M makes OLDREV default to N and NEWREV default to M,\n"
" -c M makes OLDREV default to M-1 and NEWREV default to M.\n"
+ " -c -M makes OLDREV default to M and NEWREV default to M-1.\n"
"\n"
" 3. Shorthand for 'svn diff --old=OLD-URL[@OLDREV] --new=NEW-URL[@NEWREV]'\n"
"\n"
@@ -436,8 +441,9 @@ const svn_opt_subcommand_desc2_t svn_cl_
" 3. In the third form, SOURCE can be a URL, or working copy item\n"
" in which case the corresponding URL is used. This URL in\n"
" revision REV is compared as it existed between revisions N and \n"
- " M. If REV is not specified, HEAD is assumed. The '-c M'\n"
- " option is equivalent to '-r N:M' where N = M-1.\n"
+ " M. If REV is not specified, HEAD is assumed.\n"
+ " The '-c M' option is equivalent to '-r N:M' where N = M-1.\n"
+ " Using '-c -M' does the reverse: '-r M:N' where N = M-1\n"
"\n"
" WCPATH is the working copy path that will receive the changes.\n"
" If WCPATH is omitted, a default value of '.' is assumed, unless\n"
@@ -879,6 +885,7 @@ main(int argc, const char * const *argv)
case 'c':
{
char *end;
+ svn_revnum_t changeno;
if (opt_state.start_revision.kind != svn_opt_revision_unspecified)
{
err = svn_error_create
@@ -887,28 +894,35 @@ main(int argc, const char * const *argv)
"can't specify -c twice, or both -c and -r"));
return svn_cmdline_handle_exit_error(err, pool, "svn: ");
}
- opt_state.end_revision.value.number = strtol(opt_arg, &end, 10);
- opt_state.end_revision.kind = svn_opt_revision_number;
+ changeno = strtol(opt_arg, &end, 10);
if (end == opt_arg || *end != '\0')
{
err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("Non-numeric change argument given to -c"));
return svn_cmdline_handle_exit_error(err, pool, "svn: ");
}
- else if (opt_state.end_revision.value.number == 0)
+ if (changeno == 0)
{
err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("There is no change 0"));
return svn_cmdline_handle_exit_error(err, pool, "svn: ");
}
- else if (opt_state.end_revision.value.number < 0)
+ /* Figure out the range:
+ -c N -> -r N-1:N
+ -c -N -> -r N:N-1 */
+ if (changeno > 0)
{
- err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Argument to -c must be positive"));
- return svn_cmdline_handle_exit_error(err, pool, "svn: ");
+ opt_state.start_revision.value.number = changeno-1;
+ opt_state.end_revision.value.number = changeno;
}
- opt_state.start_revision.value.number = opt_state.end_revision.value.number - 1;
- opt_state.start_revision.kind = svn_opt_revision_number;
+ else
+ {
+ changeno = -changeno;
+ opt_state.start_revision.value.number = changeno;
+ opt_state.end_revision.value.number = changeno-1;
+ }
+ opt_state.start_revision.kind = svn_opt_revision_number;
+ opt_state.end_revision.kind = svn_opt_revision_number;
}
break;
case 'r':
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 18522)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -668,9 +668,15 @@ def diff_only_property_change(sbox):
svntest.actions.run_and_verify_svn(None, expected_output, [],
'diff', '-r', '1:2')
+ svntest.actions.run_and_verify_svn(None, expected_output, [],
+ 'diff', '-c', '2')
+
svntest.actions.run_and_verify_svn(None, expected_reverse_output, [],
'diff', '-r', '2:1')
+ svntest.actions.run_and_verify_svn(None, expected_reverse_output, [],
+ 'diff', '-c', '-2')
+
svntest.actions.run_and_verify_svn(None, expected_output, [],
'diff', '-r', '1')
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 18522)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -1066,9 +1066,7 @@ def merge_similar_unrelated_trees(sbox):
svntest.actions.run_and_verify_status(apply_path, expected_status)
#----------------------------------------------------------------------
-def merge_one_file(sbox):
- "merge one file (issue #1150)"
-
+def merge_one_file_helper(sbox, arg_flav):
sbox.build()
wc_dir = sbox.wc_dir
@@ -1099,10 +1097,19 @@ def merge_one_file(sbox):
# ### Yes, it would be nice to use run_and_verify_merge(), but it
# appears to be impossible to get the expected_foo trees working
# right. I think something is still assuming a directory target.
- svntest.actions.run_and_verify_svn(None,
- ['U ' + rho_path + '\n'], [],
- 'merge', '-r', '1:2',
- rho_url, rho_path)
+ if arg_flav == 'r':
+ svntest.actions.run_and_verify_svn(None ,
+ ['U ' + rho_path + '\n'], [],
+ 'merge', '-r', '1:2',
+ rho_url, rho_path)
+ elif arg_flav == 'c':
+ svntest.actions.run_and_verify_svn(None ,
+ ['U ' + rho_path + '\n'], [],
+ 'merge', '-c', '2',
+ rho_url, rho_path)
+ else:
+ raise svntest.Failure
+
expected_status.tweak(wc_rev=1)
expected_status.tweak('A/D/G/rho', status='M ')
svntest.actions.run_and_verify_status(wc_dir, expected_status)
@@ -1124,9 +1131,16 @@ def merge_one_file(sbox):
try:
os.chdir(G_path)
# Cannot use run_and_verify_merge with a file target
- svntest.actions.run_and_verify_svn(None,
- ['U rho\n'], [],
- 'merge', '-r', '1:2', rho_url)
+ if arg_flav == 'r':
+ svntest.actions.run_and_verify_svn(None,
+ ['U rho\n'], [],
+ 'merge', '-r', '1:2', rho_url)
+ elif arg_flav == 'c':
+ svntest.actions.run_and_verify_svn(None,
+ ['U rho\n'], [],
+ 'merge', '-c', '2', rho_url)
+ else:
+ raise svntest.Failure
# Inspect rho, make sure it's right.
rho_text = svntest.tree.get_text('rho')
@@ -1139,86 +1153,18 @@ def merge_one_file(sbox):
expected_status.tweak('A/D/G/rho', status='M ')
svntest.actions.run_and_verify_status(wc_dir, expected_status)
-#----------------------------------------------------------------------
-def merge_one_file_with_c(sbox):
- "merge one file using -c"
-
- sbox.build()
- wc_dir = sbox.wc_dir
-
- rho_rel_path = os.path.join('A', 'D', 'G', 'rho')
- rho_path = os.path.join(wc_dir, rho_rel_path)
- G_path = os.path.join(wc_dir, 'A', 'D', 'G')
- rho_url = svntest.main.current_repo_url + '/A/D/G/rho'
-
- # Change rho for revision 2
- svntest.main.file_append(rho_path, 'A new line in rho.\n')
-
- expected_output = wc.State(wc_dir, { rho_rel_path : Item(verb='Sending'), })
- expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
- expected_status.tweak(wc_rev=1)
- expected_status.tweak('A/D/G/rho', wc_rev=2)
- svntest.actions.run_and_verify_commit (wc_dir,
- expected_output,
- expected_status,
- None,
- None, None, None, None,
- wc_dir)
-
- # Backdate rho to revision 1, so we can merge in the rev 2 changes.
- svntest.actions.run_and_verify_svn(None, None, [],
- 'up', '-r', '1', rho_path)
-
- # Try one merge with an explicit target; it should succeed.
- # ### Yes, it would be nice to use run_and_verify_merge(), but it
- # appears to be impossible to get the expected_foo trees working
- # right. I think something is still assuming a directory target.
- svntest.actions.run_and_verify_svn(None,
- ['U ' + rho_path + '\n'], [],
- 'merge', '-c', '2',
- rho_url, rho_path)
- expected_status.tweak(wc_rev=1)
- expected_status.tweak('A/D/G/rho', status='M ')
- svntest.actions.run_and_verify_status(wc_dir, expected_status)
-
- # Inspect rho, make sure it's right.
- rho_text = svntest.tree.get_text(rho_path)
- if rho_text != "This is the file 'rho'.\nA new line in rho.\n":
- print "Unexpected text in merged '" + rho_path + "'"
- raise svntest.Failure
-
- # Restore rho to pristine revision 1, for another merge.
- svntest.actions.run_and_verify_svn(None, None, [], 'revert', rho_path)
- expected_status.tweak('A/D/G/rho', status=' ')
- svntest.actions.run_and_verify_status(wc_dir, expected_status)
-
- # Cd into the directory and run merge with no targets.
- # It should still merge into rho.
- saved_cwd = os.getcwd()
- try:
- os.chdir(G_path)
- # Cannot use run_and_verify_merge with a file target
- svntest.actions.run_and_verify_svn(None,
- ['U rho\n'], [],
- 'merge', '-c', '2', rho_url)
-
- # Inspect rho, make sure it's right.
- rho_text = svntest.tree.get_text('rho')
- if rho_text != "This is the file 'rho'.\nA new line in rho.\n":
- print "Unexpected text merging to 'rho' in '" + G_path + "'"
- raise svntest.Failure
- finally:
- os.chdir(saved_cwd)
+def merge_one_file_using_r(sbox):
+ "merge one file (issue #1150) using the -r option"
+ merge_one_file_helper(sbox, 'r')
- expected_status.tweak('A/D/G/rho', status='M ')
- svntest.actions.run_and_verify_status(wc_dir, expected_status)
+def merge_one_file_using_c(sbox):
+ "merge one file (issue #1150) using the -c option"
+ merge_one_file_helper(sbox, 'c')
#----------------------------------------------------------------------
# This is a regression for the enhancement added in issue #785.
-def merge_with_implicit_target (sbox):
- "merging a file with no explicit target path"
-
+def merge_with_implicit_target_helper(sbox, arg_flav):
sbox.build()
wc_dir = sbox.wc_dir
@@ -1261,8 +1207,14 @@ def merge_with_implicit_target (sbox):
os.chdir(os.path.join(other_wc, 'A'))
# merge using URL for sourcepath
- svntest.actions.run_and_verify_svn(None, ['U mu\n'], [],
- 'merge', '-r', '2:1', mu_url)
+ if arg_flav == 'r':
+ svntest.actions.run_and_verify_svn(None, ['U mu\n'], [],
+ 'merge', '-r', '2:1', mu_url)
+ elif arg_flav == 'c':
+ svntest.actions.run_and_verify_svn(None, ['U mu\n'], [],
+ 'merge', '-c', '-2', mu_url)
+ else:
+ raise svntest.Failure
# sanity-check resulting file
if (svntest.tree.get_text('mu') != orig_mu_text):
@@ -1270,8 +1222,14 @@ def merge_with_implicit_target (sbox):
# merge using filename for sourcepath
# Cannot use run_and_verify_merge with a file target
- svntest.actions.run_and_verify_svn(None, ['G mu\n'], [],
- 'merge', '-r', '1:2', 'mu')
+ if arg_flav == 'r':
+ svntest.actions.run_and_verify_svn(None, ['G mu\n'], [],
+ 'merge', '-r', '1:2', 'mu')
+ elif arg_flav == 'c':
+ svntest.actions.run_and_verify_svn(None, ['G mu\n'], [],
+ 'merge', '-c', '2', 'mu')
+ else:
+ raise svntest.Failure
# sanity-check resulting file
if (svntest.tree.get_text('mu') != orig_mu_text + added_mu_text):
@@ -1280,6 +1238,14 @@ def merge_with_implicit_target (sbox):
finally:
os.chdir(was_cwd)
+def merge_with_implicit_target_using_r(sbox):
+ "merging a file w/no explicit target path using -r"
+ merge_with_implicit_target_helper(sbox, 'r')
+
+def merge_with_implicit_target_using_c(sbox):
+ "merging a file w/no explicit target path using -c"
+ merge_with_implicit_target_helper(sbox, 'c')
+
#----------------------------------------------------------------------
def merge_with_prev (sbox):
@@ -3207,14 +3173,15 @@ test_list = [ None,
add_with_history,
delete_file_and_dir,
simple_property_merges,
- merge_with_implicit_target,
+ merge_with_implicit_target_using_r,
+ merge_with_implicit_target_using_c,
merge_catches_nonexistent_target,
merge_tree_deleted_in_target,
merge_similar_unrelated_trees,
merge_with_prev,
merge_binary_file,
- merge_one_file,
- merge_one_file_with_c,
+ merge_one_file_using_r,
+ merge_one_file_using_c,
merge_in_new_file_and_diff,
merge_skips_obstructions,
merge_into_missing,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 21 23:26:09 2006