[pacman-dev] [RFC] pactree rewrite in C

[Dave Reisner]

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