> 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
Received on 2011-03-17 07:44:06 CET