{"thread":{"id":"57955","subject":"[PATCH] read-cache.c: reduce unnecessary cache entry name copying","startedAt":"2022-06-05T13:37:34Z","lastAt":"2022-06-06T17:23:18Z","messageCount":3,"participants":["ZheNing Hu via GitGitGadget","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"456698","messageId":"pull.1249.git.1654436248249.gitgitgadget@gmail.com","threadId":"57955","inReplyTo":null,"subject":"[PATCH] read-cache.c: reduce unnecessary cache entry name copying","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-05T13:37:28Z","receivedAt":"2022-06-05T13:37:34Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIn function create_from_disk, we have already copy the cache\nentries name from disk or previous cache entry, we can reduce\nunnecessary copy before that.\n\nSigned-off-by: ZheNing Hu \n---\n read-cache.c: reduce unnecessary cache entry name copying\n \n Index cache entries name are copied twice wrongly, so reduce the first\n one to fix it.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1249\n\n read-cache.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 96ce489c7c5..e61af3a3d4d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \tce->ce_namelen = len;\n \tce->index = 0;\n \toidread(&ce->oid, ondisk->data);\n-\tmemcpy(ce->name, name, len);\n-\tce->name[len] = '\\0';\n \n \tif (expand_name_field) {\n \t\tif (copy_len)\n\nbase-commit: ab336e8f1c8009c8b1aab8deb592148e69217085\n-- \ngitgitgadget\n"},{"id":"456707","messageId":"9195fe51-a696-1fb7-3617-a5b65358a806@web.de","threadId":"57955","inReplyTo":"pull.1249.git.1654436248249.gitgitgadget@gmail.com","subject":"Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-06T09:48:13Z","receivedAt":"2022-06-06T09:48:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.06.22 um 15:37 schrieb ZheNing Hu via GitGitGadget:\n> From: ZheNing Hu \n>\n> In function create_from_disk, we have already copy the cache\n> entries name from disk or previous cache entry, we can reduce\n> unnecessary copy before that.\n>\n> Signed-off-by: ZheNing Hu \n> ---\n> read-cache.c: reduce unnecessary cache entry name copying\n>\n> Index cache entries name are copied twice wrongly, so reduce the first\n> one to fix it.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1249\n>\n> read-cache.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 96ce489c7c5..e61af3a3d4d 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n> \tce->ce_namelen = len;\n> \tce->index = 0;\n> \toidread(&ce->oid, ondisk->data);\n> -\tmemcpy(ce->name, name, len);\n> -\tce->name[len] = '\\0';\n\nThis removal looks correct to me. The extra copying was added by\n575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)\nbut its commit message doesn't mention it.\n\n>\n> \tif (expand_name_field) {\n> \t\tif (copy_len)\n\nSome more context lines would help to see the redundancy; here they are:\n\n\tif (expand_name_field) {\n\t\tif (copy_len)\n\t\t\tmemcpy(ce->name, previous_ce->name, copy_len);\n\t\tmemcpy(ce->name + copy_len, name, len + 1 - copy_len);\n\t\t*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;\n\t} else {\n\t\tmemcpy(ce->name, name, len + 1);\n\t\t*ent_size = ondisk_ce_size(ce);\n\t}\n\n>\n> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085\n"},{"id":"456727","messageId":"xmqqy1y9905h.fsf@gitster.g","threadId":"57955","inReplyTo":"9195fe51-a696-1fb7-3617-a5b65358a806@web.de","subject":"Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T17:23:06Z","receivedAt":"2022-06-06T17:23:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 96ce489c7c5..e61af3a3d4d 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>> \tce->ce_namelen = len;\n>> \tce->index = 0;\n>> \toidread(&ce->oid, ondisk->data);\n>> -\tmemcpy(ce->name, name, len);\n>> -\tce->name[len] = '\\0';\n>\n> This removal looks correct to me. The extra copying was added by\n> 575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)\n> but its commit message doesn't mention it.\n\nI agree with the analysis. When we are prefix-compressing the\nentry, name[] may be much shorter than len, so this memcpy() may\npotentially be reading beyond the end of the on-disk buffer, so the\noriginal code is not just redundant, but it may simply be wrong.\n\nThanks, both. Will queue.\n\n>>\n>> \tif (expand_name_field) {\n>> \t\tif (copy_len)\n>\n> Some more context lines would help to see the redundancy; here they are:\n>\n> \tif (expand_name_field) {\n> \t\tif (copy_len)\n> \t\t\tmemcpy(ce->name, previous_ce->name, copy_len);\n> \t\tmemcpy(ce->name + copy_len, name, len + 1 - copy_len);\n> \t\t*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;\n> \t} else {\n> \t\tmemcpy(ce->name, name, len + 1);\n> \t\t*ent_size = ondisk_ce_size(ce);\n> \t}\n>\n>>\n>> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085\n"}]}