| Message ID | pull.1249.git.1654436248249.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6d858341d284e08320dc2dbf1952d7a37884e5f3 |
| Headers | show |
| Series | read-cache.c: reduce unnecessary cache entry name copying | expand |
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
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
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)