Dan McGee wrote :
1. Please enable the pre-commit hook in git: mv .git/hooks/pre-commit.sample .git/hooks/pre-commit This will ensure you don't have the following that I caught when applying the patch locally:
dmcgee@galway ~/projects/pacman (master) $ git am -3 < /tmp/pacman-verify.patch Applying: New feature: files verification /home/dmcgee/projects/pacman/.git/rebase-apply/patch:160: trailing whitespace. warning: 1 line adds whitespace errors. Done.
I think -k would be a better option, after seeing Aaron's comments. Why don't we change it to that instead. Done. I replaced every "verify" by "check".
You missed adding it to the query directions, somewhere around line 110 in pacman.c. We will also want to document it in the doc/pacman.8.txt manpage. Yes I ignored the doc part when I wrote the code, it's done now.
Please stick with the naming conventions in the rest of the code- no camel case necessary. Just 'st' is the preferred name for a struct stat. This also can be made local to the for(files) loop. Fixed.
No automatic GC in C. You are leaking like a sieve here: ==26832== 43,564 bytes in 1,512 blocks are definitely lost in loss record 3 of 3 ==26832== at 0x4C2391E: malloc (in /usr/lib/valgrind/amd64-linux/vgpreload_memcheck.so) ==26832== by 0x40852C: verify (query.c:353) ==26832== by 0x408B69: pacman_query (query.c:485) ==26832== by 0x407A6B: main (pacman.c:944)
Valgrind is awesome. Compile pacman, run ./src/pacman/pacman at least once, then do the following:
dmcgee@galway ~/projects/pacman (master) $ valgrind ./src/pacman/.libs/lt-pacman -Qy glibc pacman-git
And it should give you a fairly concise summary of memory leaks. See here for more: http://www.toofishes.net/blog/using-valgrind-c-programming/
Thanks, you're right it's awesome. :)
This logic isn't always correct. See lib/libalpm/add.c, line 309: snprintf(filename, PATH_MAX, "%s%s", handle->root, entryname);
You cannot assume the root path is always '/'. In addition, it would probably be better to just use a static buffer of length PATH_MAX like that code does or you will be spending a tremendous amount of time mallocing (and currently leaking memory). Fixed.
What has files in /tmp? Well they are temporary files so you can't consider that the package is damaged if they are not here. I added this because of some fonts package which has files in /tmp.
Hmm. I like the interactivity of this, but I'm not completely sure what you having going on here. You write to stderr but flush stdout? Those were just errors due to the fact that I only wrote in stdout when I first wrote the code and I forgot to change.
In addition, the '°' symbol means nothing to me- this could be a cultural thing though. :) Yes I didn't realise it was French, and according to Wikipedia it isn't even correct in French ... I removed it.
It would probably also be best to use a format like we do in callback.c, line 399, which is something like this: checking package integrity... (1/1) checking for file conflicts [#####################] 100% (1/1) installing emacs [#####################] 100%
I am referring to the last two lines here. We can probably even use this callback function to do the dirty work of printing status and a progress bar for us.
I am also a fan of output only when errors happen, besides a progress bar. For instance, look at the output of testdb: $ testdb Checking the integrity of the local database in /var/lib/pacman/ missing dependency for mysql : mysql-clients>=5.1.32
If there is any output at all, it is important as it is telling you problems. If we go for progress bars then I guess it would be a line by package and the bar would be for the files. I'll look into that and post a new version of the patch.
Since we don't actually do any repair, can we just call pkgs2repair "damaged"? That also kills the numeral from the variable name, which I at least don't find very appealing.
pkgs2repair is also never freed. Memory leak.
Memory leak! All fixed.
I think your logic is a bit weird here, as you are trying to incorporate both filters and a verify- note that we end up displaying both pieces of information:
$ ./src/pacman/.libs/lt-pacman -Qy glibc pacman-git glibc 2.9-4 pacman-git 20090116-1 file n°1512 (package 2/2): OK No damaged packages.
We should probably only do verification and not print the names of the packages and run them through filters, unless anyone else has an opinion. Good point, fixed.
I commented a lot, but don't think I hate your patch. I definitely like this idea. I get that and it's me who has to thank you for commenting because it helps me get better in C.