Re: [PATCH 1/3] stat_validity: handle non-regular files

[Jeff King <peff@peff.net>]

On Sat, May 23, 2015 at 01:00:06PM +0200, Michael Haggerty wrote:

> I don't have any insight about whether mtimes are reliable change
> indicators for directories.
> 
> But if you make this change, you are changing the contract of the
> stat_validity functions:
> 
> * Have you verified that no callers rely on the old stat_validity's
> check that the file is a regular file?

I think they are OK if a directory appears (they notice the error in
reading and call stat_validity_clear to throw away the result). But it
would be a problem, I suppose, if you put a FIFO or something into
$GIT_DIR/packed-refs. OTOH, that would suffer from the stat data
changing constantly, so we would fall back to safe behavior.

I don't know how careful we want to be. I guess the conservative choice
would be to barf if it's not a file or directory, both of which can be
handled pretty sanely.

> * The docstrings in cache.h need to be updated.

Thanks, will fix if I re-roll (I'm still not convinced this is a robust
thing to be doing overall).

> >  struct stat_validity {
> > -	struct stat_data *sd;
> > +	struct stat_data sd;
> > +	unsigned mode;
> >  };
> 
> Could the mode be stored directly in stat_data?

I'd rather not. stat_data is shared with the cache_entry code, which
holds the mode separately. I'd like to avoid impacting the index code at
all.

> By comparing modes, you are not only comparing file type (S_ISREG vs
> S_ISDIR etc.) but also all of the file permissions. That seems OK with
> me but you might want to document that fact. Plus, I don't know whether
> POSIX allows additional, implementation-defined information to be stored
> in st_mode. If so, you would be comparing that, too.

Yeah, I considered that. My feeling is that testing _more_ information
is probably OK. Even if there is extra data there, it presumably doesn't
change from call to call of stat() unless the directory changes. And if
we are wrong, we fail safely (e.g., if the permissions change we don't
technically need to re-read the pack directory, but it is OK to do so).

-Peff