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
