Re: [PATCH] sha1_file: use access(), not lstat(), if possible
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 20, 2017, 21:48 UTC
- Message-ID
- <xmqqbmoekce5.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170719171251.11906-1-jonathantanmy@google.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.
Show 46 quoted lines
>
> [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);