On Mon, Feb 22, 2010 at 12:44:06PM +0100, Neels J Hofmeyr wrote: > 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 :)
Noted. Anything that eases the burden of writing doc comments are welcomed! :). > 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 :) Ok. But isn't this bordering to bike-shedding? :) > 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? I checked svn_client__can_delete() but decided to not use it threw some errors I didn't want. As for rolling my own status_func and using svn_client_status5(): Haven't thought that long. I was busy writing all the code to read dirs and populate hash tables :). If I used svn_client_status5() I could identify svn_wc_status_missing and svn_wc_status_unversioned withouth having to write all the dir and hash table code. I could save the paths in the status_baton. Doh, Why didn't I do it like that? > > (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? Good idea, will do! > > 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. Ok. But could someone atleast tell me where to find a linked list in 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. See above > > 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. And I appreciate you taking the time! Daniel
