Hi Daniel,
let me comment as far as my knowledge goes. (probably more general remarks)
Daniel Näslund wrote:
> Hi Stefan!
>
> This patch is growing and growing and I'm worried that I'm heading off
> in the wrong direction. Perhaps you can skim through the patch and tell
> me if this is the right approach. You've said so earlier but all these
> lines of code makes me nervous. I like small fixes, and this is starting
> to look bloated.
>
> A preliminary log msg (it's a WIP)
> [[[
> Make 'svn patch' able to remove empty.
>
> * subversion/tests/cmdline/patch_tests.py
> (patch_remove_empty_dir): New.
>
> * subversion/libsvn_client/patch.c
> (is_dir_empty): Checks if a dir is empty, taking missing nodes into
> account.
If you create a new function, you can simply write
(function_name): New function.
The rationale is that you are supposed to write detailed function docs in
the function declaration's comment. (Like you did with the test, though I
personally would have written "New test." to be at least that clear). Of
course, you can add anything that is really special about the function. But
in general, that's not needed :)
Re is_dir_empty() comment: When I last created function comments starting
with "Helper for..." I was told that that's not really the desired way to
go. We should rather have descriptive names / concise docs, and we should
design functions to be re-used for a particular purpose, rather than
"hiding" code in specialised helper functions. I came to agree with that :)
Re is_dir_empty() impl: I remember us chatting about that. Is it not
possible to re-use some function that already exists?
svn_client__can_delete() maybe?
I vaguely expect is_dir_empty() to do some sort of crawl using an already
existing function. svn_client_status5(), maybe?
(disclaimer: I have no idea really, just poking to make sure.)
> (sort_compare_paths_by_depth): Compare func to use with
> svn_sort__hash(). Compares nr of '/' separators. Since the paths are
> canonicalized, it should work just fine.
Should this function maybe be called something more specific, like
"sort_compare_nr_of_path_elements()" or something?
> (collect_deleted_targets): If there is targets to be deleted, collect
> those targets and the targets no involving deletes in two arrays.
> TODO: We should condense the list of deleted dirs and be able to
> only delete the top-most parent.
> (apply_patches): TODO: We should remove the targets in
> delete_all_of_this. Or perhaps concatenate the two list into one?
This comment lacks a description of what is being done in the code :)
TODOs are sometimes more appropriate in the code as a comment. (Just poking.)
> ]]]
>
> Just a quick.
> [ ] Yes, using APR forces you to a lot of boilerplate.
You mean a lot of opaque APIs? With a (very) quick glance, I don't see any
unusual use of APR.
> [ ] Hey there, you don't need to do all that, just ...
...maybe use that svn_client_status5() function as said above, or find
another function. But I'm not entirely sure.
>
> Hope I'm not asking to much,
I know how stupid one can feel trying to write a patch in the complex lands
of Subversion... We're here to review. Thanks for asking for one!
I hope I'm actually poking in the right directions, though.
~Neels
Received on 2010-02-22 12:44:44 CET