Quoth Jani Nikula on May 08 at 8:33 am: > On Tue, 08 May 2012 07:58:28 +0000, Jani Nikula <jani at nikula.org> wrote: > > On Mon, 7 May 2012 18:20:40 -0400, Austin Clements <amdragon at MIT.EDU> > > wrote: > > > This moves our logic to get a file's type into one function. This has > > > several benefits: we can support OSes and file systems that do not > > > provide dirent.d_type or always return DT_UNKNOWN, complex > > > symlink-handling logic has been replaced by a simple stat fall-through > > > in one place, and the error message for un-stat-able file is more > > > accurate (previously, the error always mentioned directories, even > > > though a broken symlink is not a directory). > > > > LGTM. > > Okay, it's good, but I think you can make it even better: > > add_files_recursive() has check for "! S_ISDIR (st.st_mode)" in the > beginning, returning silently in case it recursed based on a symlink to > regular file. IIUC, this will no longer happen with your patch, as > symlinks are resolved and stat'ed before recursing. > > add_files() exists to fail loudly in the same situation, and has > otherwise the same checks in the beginning. I think you could now use > the checks from add_files() to replace the ones in > add_files_recursive(), and get rid of add_files() altogether. > > Please double check my thinking. Also this should probably be a separate > patch, no need to change the current one.
Excellent idea. It works fantastically. I'll wait to send the patches until this series gets pushed, both the avoid dependent series confusion and so I can refer to the appropriate commit ID in the commit message.
