On Tuesday 15 March 2011 08:34 PM, Julian Foad wrote:
On Wed, 2011-03-02, Arwin Arni wrote:
In my effort to understand the delta editor API, I took it upon myself
to try and implement the --dry-run flag for "svn update".
http://subversion.tigris.org/issues/show_bug.cgi?id=2491
Hi Arwin. Kamesh asked for technical feedback about this patch so
here's a review.
In my view, a "dry run" mode for update is useful but not very
important. The complexity of this implementation looks about what I'd
expect, not too bad, but I think right now is maybe not a good time to
add this because it does add significant complexity to code that we are
trying to work on.
Currently, externals are handled inside
subversion/libsvn_client/externals.c by running checkout/switch. For a
dry-run update to mimic a real update, the notifications have to be the
same. Since some of these notifications are generated by the above
mentioned checkout/switch runs, I have to implement dry-run for them
also. I'll take this up as a follow-up exercise. Now, the dry-run will
simply ignore any externals in the working copy.
It's fine to handle externals in a follow-up patch.
I see that '--parents' and '--set-depth' are not allowed in dry-run
mode. What is the reason behind that? Is it because they seem to be
difficult to implement, or you think they are not needed, or you intend
to implement them in a follow-up patch, or something else?
Well, the reason is that both --parents and --set-depth make permanent
changes to the WC which will not be reported in the output at all.. If
the user is passing these parameters, he has a fair idea of what these
"invisible" changes are, and so shouldn't be adding them to a dry-run
command. (Maybe we could just make them FALSE during a dry-run update so
it doesn't throw those errors that I coded. The user will be oblivious
to this, as the output isn't going to change in any way)
The reason I'm concerned about orthogonality is because if some parts of
the code don't support one feature, and other parts of the code don't
support another feature, that can make tasks such as refactoring the
code much more difficult.
Not quite sure I understand what you mean..
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 1075841)
+++ subversion/include/svn_wc.h (working copy)
@@ -4990,7 +4990,8 @@
* @a notify_baton and the path of the restored file. @a notify_func may
* be @c NULL if this notification is not required. If @a
* use_commit_times is TRUE, then set restored files' timestamps to
- * their last-commit-times.
+ * their last-commit-times. If @a dry_run is TRUE, only notifications are
+ * done. Using @a dry_run is meaningless if @a restore_files is FALSE.
The last sentence is not clear. What does it mean in terms of the
promises and requirements of this API?
This is in svn_wc_crawl_revisions5.
The value of dry_run is never referenced unless restore_files is TRUE.
restore_files dry_run outcome
----------------------------------------------------
TRUE TRUE dry-run restore
TRUE FALSE actually restore
FALSE TRUE/FALSE no restore
Index: subversion/svn/main.c
===================================================================
@@ -1733,6 +1733,13 @@
[...]
case opt_dry_run:
+ if (opt_state.parents)
+ {
+ err = svn_error_create
+ (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Can't specify --parents with --dry-run"));
+ return svn_cmdline_handle_exit_error(err, pool, "svn: ");
+ }
opt_state.dry_run = TRUE;
break;
It looks to me like this check will only work if "--parents" is
specified before "--dry-run" on the command line, and not if they are
specified the other way around.
Also, by putting the check here in "main.c", this restriction applies to
all commands, whereas I think this restriction should be specific to the
"update" command, even if there are currently no other commands that
accept both options.
To fix those two problems, please move the check into the "update"
command itself.
Hmm..
I guess my solution of allowing the user to pass these combination of
options, and making them a no-op would solve all these problems..
@@ -1764,6 +1771,14 @@
case opt_set_depth:
+ if (opt_state.dry_run)
+ {
[...]
Similarly here.
+ }
TEST SUITE
* subversion/tests/cmdline/svntest/actions.py
(_post_update_checks) : New function to do the checks after an update.
(run_and_verify_update): Accept a dry_run parameter. If TRUE, check that
the dry run hasn't affected the wc.
The phrase "the dry run" makes the reader ask, "What dry run?". I would
suggest: "If TRUE, first do a dry run and check that it hasn't affected
the WC."
Okay..
* subversion/tests/cmdline/changelist_tests.py
subversion/tests/cmdline/externals_tests.py
subversion/tests/cmdline/prop_tests.py
subversion/tests/cmdline/update_tests.py
subversion/tests/cmdline/resolved_tests.py
subversion/tests/cmdline/trans_tests.py
subversion/tests/cmdline/switch_tests.py
subversion/tests/cmdline/stat_tests.py
subversion/tests/cmdline/depth_tests.py
(Tests) : Fix callers of run_and_verify_update.
Rather than "Fix callers", say what the change is: "Pass dry_run=True to
some callers and dry_run=False to other callers ...".
Okay..
Why do some callers pass True and some pass False? I guess it's because
some callers use externals or --parents or --set-depth, and none of
those work properly with dry-run, so those are passing False.
That's right.. The calls with incompatible options (--parents and
--set-depth) pass FALSE to dry_run.
But we could take a different approach. Rather than the caller deciding
whether a dry-run check is going to work, we could let
run_and_verify_update() decide to perform the additional check
automatically when it can. If any of the args is "--parents" or
"--set-depth" then it must skip the dry-run check. Otherwise it should
perform the dry-run check and ignore any difference in the output caused
by externals not being reported in dry-run mode.
Yeah, I think the solution to the first problem kills these too.. except
externals... right now I need a way to skip the dry_run for any
externals related tests.. Also, I think always doing a dry_run + tests
is not the way to go.. I mean the caller should have an option of doing
dry-run checks. I think by solving the incompatible options problem, and
fixing externals too, the caller needn't have to worry about whether the
dry_run check is going to work. He just has to decide whether or not he
wants a dry_run check to be performed too. (This is in line with how
run_and_verify_merge has been implemented.)
In this way, we don't need to add the dry-run parameter to
run_and_verify_update() at all.
Also in the log message:
Fix Issue #2491 Add --dry-run flag to "svn update" client command
HEADERS AND DEPRECATIONS
Thanks for stating the summary of the issue and for dividing this rather
large log message into three main sections.
* subversion/include/svn_client.h
(svn_client_update4) : Modify signature and document it.
It would be better to say "Add a 'dry_run' parameter." That says
exactly what the change is. From that, the reader knows the signature
is modified and assumes it's documented.
(svn_client_update3) : Fix call of svn_client_update4 to pass
FALSE to dry_run.
We usually use the word "Fix" for mending something that was broken. I
think "Pass dry_run=FALSE to svn_client_update4()" would be a good way
to describe the change here.
(update_internal) : Accept dry_run.
Fix call of svn_wc_get_update_editor4,
svn_wc_crawl_revisions5.
For this kind of change you could write "Propagate the dry_run parameter
to svn_wc_crawl_revisions5()".
I agree.. my log-message-writing skills are still developing..
Thanks for this in-depth review.. Some of the things I learn here will
definitely be committed to memory now.
Regards,
Arwin Arni
