Hi again Andreas, On 2006-01-26, Andreas Gruenbacher wrote: > I have removed our reliance on ftw and implemented directory walking in > lib/backup-files.c itself based on Gary's proposal, > <http://lists.gnu.org/archive/html/quilt-dev/2005-09/msg00133.html>. > Gary, it took a while, but eventually you've won -- thank you ;) > > Could you guys please review the changes, and check if backup-files now > builds on the platforms we are targeting?
I just took a few minutes to review the code. Given the relatively small amount of code you had to add, it was definitely not worth the additional pain of relying of a hardly standard library like we did before. Smart move. The code looks mostly OK to me (although I didn't get a chance of testing it yet) except for a few details: * perror(NULL) should be perror(progname) or something. It's always very frustrating when error messages don't provide any hint at their source. * The while loop in foreachdir_rec promises to be quite slow, due to the repeated use of realloc and strlen. Given that path never changes, we could store the result of strlen(path) in a local variable (or even better, 1 + strlen(path) + 1) and use it when needed. This would be a first improvement. However, I think it would be even better to rely on PATH_MAX + 1 for the allocation of p (or 4096 where PATH_MAX is not defined) and only allocate it once for a given directory. We can then use snprintf instead of sprintf and check the return value to detect errors. On error, we can realloc with the needed size, or just fail - I really don't see anyone using such long names, and as I understand it PATH_MAX is a system limit so it probably wouldn't work anyway. Opinions? Thanks, -- Jean Delvare _______________________________________________ Quilt-dev mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/quilt-dev
