On 10/19/06, VMiklos <vmiklos@frugalware.org> wrote:
1) po4a detection is added 3 times to configure.ac
Fixed. I've noticed one or two other instances of triple-insertions...
2) you forgot to cvs add the bindings directory, while you added the bindings part to configure.ac
Bindings are here: http://cvs.archlinux.org/cgi-bin/viewcvs.cgi/bindings/?cvsroot=Pacman What do you mean?
3) po4a support as been added to doc/Makefile.am, but the translations in /doc/po/ are missing
Not sure what you mean - the only translations I have from the patch are hungarian.
4) you left several NoUpgrade files in pacman.conf while that directive should not be used by default (it is something for users)
According to CVS, NoUpgrade was never removed. This is the latest: http://cvs.archlinux.org/cgi-bin/viewcvs.cgi/etc/Attic/pacman.conf?rev=HEAD&search=None&hideattic=1&cvsroot=Pacman&only_with_tag=HEAD&content-type=text/vnd.viewcvs-markup
5) libalpm/add.c: search for "Cleaning up", added 2 times
Fixed in CVS.
6) you've added a new callback parameter to alpm_db_register() which is totally useless imho. the callback is called with the treename (which is a parameter, too) and the database pointer, which is returned. so what's the point of it?
The point is that I changed this function to return the existing DB in an attempt to reregister, in place of returning null. It seems stupid to let this fail: [current] Server = a [current] Server = b Every config file parser I've seen for sectioned configs ([section name]) parses this as if they were all one section. As such, the change requires the callback because only db_register knows when the database is new or old. It's a rather trivial change, and not "totally useless", as it allows for more valid config file handling.
7) in alpm.c:
- * @return a void* on success (the value), NULL on error + * @return a char* on success (the value), NULL on error */ void *alpm_conflict_getinfo(pmconflict_t *conflict, unsigned char parm)
is this a typo? :)
I didn't touch those comments... fixed in CVS though
8) i think you've reverted judd's following change: 2006-07-04 19:48 judd
* lib/libalpm/deps.c: bugfix: when looking at provides, defer to the new, to-be-installed package's provisios instead of the the existing package's
Ah crap... didn't notice that one, I assumed your patches took this into account. I will fix this later, as it's a shade more complicated than editing a few lines.
9) what is the benefit of this change in Makefile.am?
- cd pactest; python pactest.py --test=tests/*.py -p ../src/pacman/pacman --debug=-1 + cd pactest; python pactest.py --test=tests/*.py -p ../src/pacman/pacman --debug=1
That was a mistake on my part that shouldn't have been checked in. I was not getting debug output, so messed with the param.... later I found the 2>&1 > logfile redirection and therefore added the nolog parameter. Fixed in CVS.
10) i think you forgot to cvs add the /pactest/tests/ directory They show up for me: http://cvs.archlinux.org/cgi-bin/viewcvs.cgi/pactest/tests/?search=None&hideattic=1&cvsroot=Pacman&only_with_tag=HEAD
11) i think you forgot to add the relevant copyright lines to scripts/makepkg
Ah, I didn't change mackepkg so didn't check that - fixed in CVS
12) you haven't removed src/pacman/db.[ch] while noone uses the functions provided by them
Done. Thanks alot.