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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 05/23/2015 01:51 AM, Jeff King wrote:
> The stat_validity code was originally written to avoid
> re-reading the packed-refs file when it has not changed. It
> makes sure that the file continues to match S_ISREG() when
> we check it.
> 
> However, we can use the same concept on a directory to see
> whether it has been modified. Even though we still have to
> touch the filesystem to do the stat, this can provide a
> speedup even over opendir/readdir/closedir, especially on
> high-latency filesystems like NFS.
> 
> This patch adds a "mode" field to stat_validity, which lets
> us check that the mode has stayed the same (rather than
> explicitly checking that it is a regular file).

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?

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

> As a bonus cleanup, we can stop allocating the embedded
> "stat_data" on the heap. Prior to this patch, we needed to
> represent the case where the file did not exist, and we used
> "sv->sd == NULL" for that. Now we can simply check that
> "sv->mode" is 0.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  cache.h      |  3 ++-
>  read-cache.c | 16 ++++++----------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index c97d807..2941e7e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1660,7 +1660,8 @@ int sane_execvp(const char *file, char *const argv[]);
>   * for the index.
>   */
>  struct stat_validity {
> -	struct stat_data *sd;
> +	struct stat_data sd;
> +	unsigned mode;
>  };

Could the mode be stored directly in stat_data?

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.

> [...]

Otherwise, looks OK.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu