On Mon, Oct 11, 2010 at 07:49:57PM -0500, Dan McGee wrote:
Comments on things:
1. The tertiary patch (b6a6f1ee8421) honestly just makes things unnecessarily compact. We aren't having a fewest line number contest, so I'd drop this completely. 2. For commit messages, I like to have signoffs (-s option to commit or a few other commands), as well as descriptive messages that use full sentences and proper capitalization and punctuation. For these, prefixing them with "pactree:" might be a nice idea. 3. With your permission, adding the standard copyright line we have in the rest of our scripts would be nice if you don't have a problem with it. You can just start the year with 2010. I'm also not sure why you changed the case of those "Authored" and "Original" lines in a random commit. 4. Please follow the include/header guidelines in HACKING. system headers, followed by libalpm headers (and probably a blank line in between). 5. I'm not a huge fan of the STREQ macro; it is only used 3 times and that isn't a really crazy construct anyway. 6. None of your function definitions fit our normal style. All on one line, opening brace next line. 7. Once again, HACKING- we use tabs and not spaces, so no et. The standard vim modeline there should be used. 8. The opening "/**" might confuse Doxygen, we should probably use "/*". 9. We are now reaching three versions of strtrim in the codebase- it would at least be good if they were all the same. 10. All functions (except main) can be marked static; this would allow inlining when compiling. 11. return x --> return(x), HACKING. 12. you are mixing return codes and pm_errno codes in alpm_local_init, and I don't like the "only error" assumption there. 14. I don't like the exit() call in cleanup- just return and let main return(0), or call exit() after a "bad" call to cleanup. I'm not sure why we do this in pacman...I don't like it there either. 15. I think it would be best to avoid parsing pacman.conf at all (too many variables there, since you gave no way of even specifying a different config file). I'd go the testdb route and permit a command line -b flag to specify a different DB location, and that is it. 16. I'd prefer Doxygen comments before the function declaration as opposed to inside, to match our precedent elsehwere. 17. For finding provides, you need to use alpm_depcmp() and not strcmp() as provides can have versions and such tacked on. See lib/libalpm/deps.c for some example usages. 18. char **argv --> char *argv[] for consistency. 19. print_provider and print_pkg look awfully similar. Can we put these together and do some smarts to decide which format string and such to pick? You can also use C string concatenation to your advantage perhaps to make this more understandable. "foo" CONSTANT "bar" CONSTANT "baz" will all be folded into one string. 20. For version, I'd just use the pacman version (see version(void) in pacman.c). 21. No-arg functions should really be written foo(void) according to the C spec, looks like we missed that in a few places in other util programs but might as well get it right here. 22. We try to wrap lines before they hit 80 characters. 23. Please always include {} in conditionals (e.g. walk_deps, walk_reverse_deps), even if the body is just one statement. HACKING 24. I wouldn't bail on a non-existant provider, I'd just print "provider <can't resolve>" or something sensical and move on (and not return). And again, returning a libalpm error code doesn't make a whole lot of sense here. 25. I know it isn't required by C99, but we tend to use forward variable declaration elsewhere in our codebase. 26. The magic argv[optind] would be nice to pull into a local variable as you reference it 5 times.
13 is unlucky so I skipped it. :P
-Dan
Awesome got my work cut out for me. Quick notes on your comments: 1-8, 10-12, 14-16, 18, 20-24, 26: i agree, i'll fix it. 9: nuts to that, i'm just cutting it out since i'm not parsing config. 17: this requires some reworking, but i think i like where this goes. 19: i may need some help with this one. pushing it towards the bottom of the list for now. 25: not sure i understand what a forward declaration means with regard to variables. do you just want all variables declared up front, at the start of each function? As I mentioned on IRC, I'll present a rewrite instead of trying to rebase this out. Trying to rebase this would be fairly insidious. d