[Quilt-dev] Re: ftw removed; emacs mode; translations; ready for 0.43?

[Jean Delvare]

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