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

[Dan McGee]

On Mon, Oct 11, 2010 at 6:44 PM, Dave Reisner <d at falconindy.com> 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 <d at falconindy.com> 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 <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