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: > > dmc...@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: > > dmc...@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. _______________________________________________ pacman-dev mailing list [email protected] http://www.archlinux.org/mailman/listinfo/pacman-dev
