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

From: Jeff King <peff@peff.net>
Date: May 24, 2015, 08:29 UTC
Message-ID: <20150524082935.GB8718@peff.net>
In-Reply-To: <55605DB6.6040909@alum.mit.edu>

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

Show 8 quoted lines

> 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).

Show 7 quoted lines

> >  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.

Show 5 quoted lines

> 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