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?
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.
> 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?
> 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.
> @@ -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."
> * 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 ...".
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.
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.
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()".
Regards,
- Julian
Received on 2011-03-15 16:04:41 CET