Re: [PATCH] sha1_file: use access(), not lstat(), if possible

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> In sha1_loose_object_info(), use access() (indirectly invoked through
> has_loose_object()) instead of lstat() if we do not need the on-disk
> size, as it should be faster on Windows [1].

That sounds as if Windows is the only thing that matters.  "It is
faster in general, and is much faster on Windows" would have been
more convincing, and "It isn't slower, and is much faster on
Windows" would also have been OK.  Do we have any measurement, or
this patch does not yield any measuable gain?  

By the way, the special casing of disk_sizep (which is only used by
the batch-check feature of cat-file) is somewhat annoying with or
without this patch, but this change makes it even more so by adding
an extra indentation level.  I do not think of a way to make it less
annoying offhand, and I do not think this change needs to address it
in any way, but I am mentioning this as a hint to bystanders who may
want to find something small that can be cleaned up ;-)

Thanks.

>
> [1] https://public-inbox.org/git/alpine.DEB.2.21.1.1707191450570.4193@virtualbox/
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks for the information - here's a patch. Do you, by any chance, know
> of a web page (or similar thing) that I can cite for this?
> ---
>  sha1_file.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index fca165f13..81962b019 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2920,20 +2920,19 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>  
>  	/*
>  	 * If we don't care about type or size, then we don't
> -	 * need to look inside the object at all. Note that we
> -	 * do not optimize out the stat call, even if the
> -	 * caller doesn't care about the disk-size, since our
> -	 * return value implicitly indicates whether the
> -	 * object even exists.
> +	 * need to look inside the object at all. We only check
> +	 * for its existence.
>  	 */
>  	if (!oi->typep && !oi->typename && !oi->sizep && !oi->contentp) {
> -		const char *path;
> -		struct stat st;
> -		if (stat_sha1_file(sha1, &st, &path) < 0)
> -			return -1;
> -		if (oi->disk_sizep)
> +		if (oi->disk_sizep) {
> +			const char *path;
> +			struct stat st;
> +			if (stat_sha1_file(sha1, &st, &path) < 0)
> +				return -1;
>  			*oi->disk_sizep = st.st_size;
> -		return 0;
> +			return 0;
> +		}
> +		return has_loose_object(sha1) ? 0 : -1;
>  	}
>  
>  	map = map_sha1_file(sha1, &mapsize);