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

On Mon, Oct 11, 2010 at 6:44 PM, Dave Reisner wrote:

On Mon, Oct 11, 2010 at 11:12:47PM +0200, Xavier Chantry wrote:

...

On Mon, Oct 11, 2010 at 5:10 PM, Dave Reisner wrote:

...

Oooh, thanks for the reminder -- I had meant to resolve command line args as providers and never got around to it. I'll need to read up on dot's graph specification language. My next project needs to revolve around increasing the number of hours in a given weekend.

Don't worry, take as many weekends as you want (well not too much :D), as this will be planned for the next major release 3.5.0 (minor releases == bugfixes). Let's hope this will happen in a few months, but it often gets delayed. In any cases, it would be awesome if you could complete this.

Not a problem. I merged it into a branch off of my pacman repo today (hurray for learning about git-filter-branch) and weaved it into Makefile.am. This should be feature complete in a few weeks at most.

One issue -- I'm not very familiar with autotools. When I build, pactree in src/util is "compiled" into a shell script. The real binary seems to be left behind src/util/.libs, and this doesn't seem to be the case for the other binaries in the directory. If you get a chance, I've got a pacman branch at:

 http://github.com/falconindy/pacman

with my work in the pactree-c branch. Could you take a glance at it (no rush) and let me know if I've done something wrong with Makefile.am or neglected to mention the binary elsewhere for libtool?

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 " 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