read-cache.c: reduce unnecessary cache entry name copying
PATCH 1/1 read-cache.c: reduce unnecessary cache entry name copying
From: ZheNing Hu <[email protected]>
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.
---
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(-)
read-cache.c +0 -2
@@ -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
Signature · 1 line
gitgitgadget
Am 05.06.22 um 15:37 schrieb ZheNing Hu via GitGitGadget:
Quoted 25 lines
From: ZheNing Hu <[email protected]>
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 <[email protected]>---
read-cache.c: reduce unnecessary cache entry name copyingIndex 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/1249read-cache.c | 2 -- 1 file changed, 2 deletions(-)read-cache.c +0
-0
read-cache.c static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, · line 1948
Quoted 5 lines
read-cache.c +0
-2@@ -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.
read-cache.c static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, · line 1949
Quoted 3 lines
read-cache.c +0
-0if (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);
}Quoted 2 lines
base-commit:
ab336e8f1c8009c8b1aab8deb592148e69217085
Quoted 6 lines · René Scharfe
Quoted 6 lines
read-cache.c +0
-0@@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, ce->ce_namelen = len;
Quoted 8 lines
read-cache.c static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, · line 1948
Quoted 4 lines
read-cache.c +0
-2ce->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.
Quoted 18 lines
read-cache.c static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, · line 1949
Quoted 3 lines
read-cache.c +0
-0if (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); }Quoted 2 lines
base-commit:
ab336e8f1c8009c8b1aab8deb592148e69217085