Arwin Arni wrote:
> 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.
The changes made by --parents are reported:
$ svn st -v
2 2 julianfoad .
2 2 julianfoad A
2 2 julianfoad A/B
$ svn up --parents A/B/C/D
A A/B/C
Updating 'A/B/C/D' ...
A A/B/C/D
Updated to revision 4.
]]]
The changes made by set-depth are reported:
[[[
$ svn st -v
2 2 julianfoad .
2 2 julianfoad A
2 2 julianfoad A/B
4 4 julianfoad A/B/C
4 4 julianfoad A/B/C/D
5 5 julianfoad A/B/C2
5 5 julianfoad A/B2
5 5 julianfoad A/B2/C
$ svn up --set-depth=empty A/B2
D A/B2/C
Updating 'A/B2' ...
Updated to revision 5.
]]]
But the depth of 'A/B2' as recorded in the WC has been changed from
'infinity' to 'empty', and that change isn't reported; is that what you
meant?
> 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.
I think the normal way to use a dry-run is to first think of the update
command that we wish to run, and then add the --dry-run option to it,
not to write a dry-run command first and then think about what options
to add to it.
> (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..
"Orthogonality occurs when two things can vary independently, they are
uncorrelated or are perpendicular." -- from
<http://en.wikipedia.org/wiki/Orthogonality>.
As example, if we want to make the test suite try a dry-run version of
every "update" command, if the --dry-run switch is "orthogonal" to the
other switches of the update command, then that means it will be
possible to add --dry-run to any update command without worrying about
it being incompatible with some of the other switches.
What I was trying to say is that the same principle applies at a lower
level as well.
> >> 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
OK, I see. We could write this as "If @a restore_files is FALSE, then
@a dry_run has no effect." or "... then @a dry_run is ignored."
The reason for this didn't make sense to me until I figured out that
svn_wc_crawl_revisions5() doesn't make any changes to the working copy
itself except for restoring files, and that therefore the dry-run
parameter only applies to restoring files.
> >> 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.)
The main business of this patch is to create the dry-run mode.
Automatically doing a dry-run and checking the result is a good way to
test that the dry-run option works properly, so that's something we
could include in the patch. (It might make the test suite run much more
slowly; we should measure that and see if it's a problem.)
However, we should not be changing lots of individual tests, some to
request a dry-run and some not to request it, without a good reason and
I don't think we have a good reason to do so.
I looked at run_and_verify_patch() and run_and_verify_merge(), both of
which take a dry_run parameter. The former has about 20 calls, all of
which pass True. The latter has about 200 calls, a quarter of which
pass False, but hardly any of them indicate why they are doing so.
> > 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
Thanks.
- Julian
