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
Received on 2010-02-22 20:07:55 CET