Date: Wed, 9 May 2012 14:27:04 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v2 2/2] new: Centralize file type stat-ing logic
Message-ID: <20120509182704.GC11804@mit.edu>
References: <1336414186-15293-1-git-send-email-amdragon@mit.edu>
	<1336429240-1114-1-git-send-email-amdragon@mit.edu>
	<1336429240-1114-3-git-send-email-amdragon@mit.edu>
	<87r4uvdryz.fsf@nikula.org> <87obpzdqcz.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87obpzdqcz.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org, Vladimir Marek <vlmarek@volny.cz>
Precedence: list

Quoth Jani Nikula on May 08 at  8:33 am:
> On Tue, 08 May 2012 07:58:28 +0000, Jani Nikula <jani@nikula.org> wrote:
> > On Mon,  7 May 2012 18:20:40 -0400, Austin Clements <amdragon@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.