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
Received on 2011-03-16 07:45:29 CET