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.
]]]
I seem to have grossly overlooked this.. I'll look into a way we can
implement this effectively..
(maybe we can intelligently fake this notification of the parents being
added.. I don't know)
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.
]]]
I can see a way to implement this easily, because --set-depth and
--depth differ only in the fact that the depth becomes sticky in
--set-depth.. So passing --set-depth can be treated as --depth for
dry-runs..
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?
Yeah, I was talking about this invisible change.. However, the delete
notification for the newly excluded paths IS indeed important.. So
treating --set-depth as --depth for dry-runs should do the trick.. In
code, depth_is_sticky is the part to ignore..
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.
I completely agree... I too believe in total consistency when it comes
to the user's experience. I hope I can make the code deliver this kind
of behavior soon..
(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.
I shall work toward this orthogonality.. I guess the only work items
left are:
1. --parents (I currently have no idea)
2. --set-depth (a fair idea - like I mentioned above)
3. externals (I know how.. and it involves more parts of our code like
switch and checkout so I'm just putting it on hold right now.. )
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.
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.
I saw this as a precedent and thought it's okay to do this..
Maybe there's some scope for improvement here too (?)
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
Regards,
Arwin Arni
