[PATCH] read-cache.c: reduce unnecessary cache entry name copying

[PATCH] read-cache.c: reduce unnecessary cache entry name copying

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

In function create_from_disk, we have already copy the cache
entries name from disk or previous cache entry, we can reduce
unnecessary copy before that.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    read-cache.c: reduce unnecessary cache entry name copying
    
    Index cache entries name are copied twice wrongly, so reduce the first
    one to fix it.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1249

 read-cache.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 96ce489c7c5..e61af3a3d4d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 	ce->ce_namelen = len;
 	ce->index = 0;
 	oidread(&ce->oid, ondisk->data);
-	memcpy(ce->name, name, len);
-	ce->name[len] = '\0';
 
 	if (expand_name_field) {
 		if (copy_len)

base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085
-- 
gitgitgadget

Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying

[René Scharfe]

Am 05.06.22 um 15:37 schrieb ZheNing Hu via GitGitGadget:
> From: ZheNing Hu <adlternative@gmail.com>
>
> In function create_from_disk, we have already copy the cache
> entries name from disk or previous cache entry, we can reduce
> unnecessary copy before that.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     read-cache.c: reduce unnecessary cache entry name copying
>
>     Index cache entries name are copied twice wrongly, so reduce the first
>     one to fix it.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1249
>
>  read-cache.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 96ce489c7c5..e61af3a3d4d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>  	ce->ce_namelen = len;
>  	ce->index = 0;
>  	oidread(&ce->oid, ondisk->data);
> -	memcpy(ce->name, name, len);
> -	ce->name[len] = '\0';

This removal looks  correct to me.  The extra copying was added by
575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
but its commit message doesn't mention it.

>
>  	if (expand_name_field) {
>  		if (copy_len)

Some more context lines would help to see the redundancy; here they are:

	if (expand_name_field) {
		if (copy_len)
			memcpy(ce->name, previous_ce->name, copy_len);
		memcpy(ce->name + copy_len, name, len + 1 - copy_len);
		*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
	} else {
		memcpy(ce->name, name, len + 1);
		*ent_size = ondisk_ce_size(ce);
	}

>
> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085

Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> diff --git a/read-cache.c b/read-cache.c
>> index 96ce489c7c5..e61af3a3d4d 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>>  	ce->ce_namelen = len;
>>  	ce->index = 0;
>>  	oidread(&ce->oid, ondisk->data);
>> -	memcpy(ce->name, name, len);
>> -	ce->name[len] = '\0';
>
> This removal looks  correct to me.  The extra copying was added by
> 575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
> but its commit message doesn't mention it.

I agree with the analysis.  When we are prefix-compressing the
entry, name[] may be much shorter than len, so this memcpy() may
potentially be reading beyond the end of the on-disk buffer, so the
original code is not just redundant, but it may simply be wrong.

Thanks, both.  Will queue.

>>
>>  	if (expand_name_field) {
>>  		if (copy_len)
>
> Some more context lines would help to see the redundancy; here they are:
>
> 	if (expand_name_field) {
> 		if (copy_len)
> 			memcpy(ce->name, previous_ce->name, copy_len);
> 		memcpy(ce->name + copy_len, name, len + 1 - copy_len);
> 		*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
> 	} else {
> 		memcpy(ce->name, name, len + 1);
> 		*ent_size = ondisk_ce_size(ce);
> 	}
>
>>
>> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085