{"thread":{"id":"36024","subject":"[PATCH] Replace memcpy with hashcpy when lengths defined","startedAt":"2014-03-02T19:19:37Z","lastAt":"2014-03-02T22:39:07Z","messageCount":3,"participants":["Alberto","Philip Oakley","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235806","messageId":"1393787977-13164-1-git-send-email-albcoron@gmail.com","threadId":"36024","inReplyTo":null,"subject":"[PATCH] Replace memcpy with hashcpy when lengths defined","fromName":"Alberto","fromEmail":"albcoron@gmail.com","sentAt":"2014-03-02T19:19:37Z","receivedAt":"2014-03-02T19:19:37Z","isPatch":true,"sender":{"key":"albcoron@gmail.com","avatar":null},"body":"From: Alberto Corona \n\nReplaced memcpy with hashcpy where lengts in memcpy\nare already defined.\n\nSigned-off-by: Alberto Corona \n---\n bundle.c | 2 +-\n grep.c | 2 +-\n refs.c | 2 +-\n sha1_name.c | 4 ++--\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex e99065c..7809fbb 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -19,7 +19,7 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \t\tlist->list = xrealloc(list->list,\n \t\t\t\tlist->alloc * sizeof(list->list[0]));\n \t}\n-\tmemcpy(list->list[list->nr].sha1, sha1, 20);\n+\thashcpy(list->list[list->nr].sha1, sha1);\n \tlist->list[list->nr].name = xstrdup(name);\n \tlist->nr++;\n }\ndiff --git a/grep.c b/grep.c\nindex c668034..f5101f7 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1650,7 +1650,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n \t\tbreak;\n \tcase GREP_SOURCE_SHA1:\n \t\tgs->identifier = xmalloc(20);\n-\t\tmemcpy(gs->identifier, identifier, 20);\n+\t\thashcpy(gs->identifier, identifier);\n \t\tbreak;\n \tcase GREP_SOURCE_BUF:\n \t\tgs->identifier = NULL;\ndiff --git a/refs.c b/refs.c\nindex 89228e2..f90b7ea 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1222,7 +1222,7 @@ static int resolve_gitlink_packed_ref(struct ref_cache *refs,\n \tif (ref == NULL)\n \t\treturn -1;\n \n-\tmemcpy(sha1, ref->u.value.sha1, 20);\n+\thashcpy(sha1, ref->u.value.sha1);\n \treturn 0;\n }\n \ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6fca869..3f5010f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -111,7 +111,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa\n \t\t\t\tcontinue;\n \t\t\tif (memcmp(de->d_name, hex_pfx + 2, len - 2))\n \t\t\t\tcontinue;\n-\t\t\tmemcpy(hex + 2, de->d_name, 38);\n+\t\t\thashcpy(hex + 2, de->d_name);\n \t\t\tif (!get_sha1_hex(hex, sha1))\n \t\t\t\tupdate_candidates(ds, sha1);\n \t\t}\n@@ -373,7 +373,7 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n \tstatic char hex[41];\n \n \texists = has_sha1_file(sha1);\n-\tmemcpy(hex, sha1_to_hex(sha1), 40);\n+\thashcpy(hex, sha1_to_hex(sha1));\n \tif (len == 40 || !len)\n \t\treturn hex;\n \twhile (len < 40) {\n-- \n1.9.0.138.g2de3478.dirty\n"},{"id":"235807","messageId":"BD20EE393DBA43E39E6390ECB511EDD2@PhilipOakley","threadId":"36024","inReplyTo":"1393787977-13164-1-git-send-email-albcoron@gmail.com","subject":"Re: [PATCH] Replace memcpy with hashcpy when lengths defined","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-03-02T19:19:37Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Alberto\" \n> From: Alberto Corona \n>\n> Replaced memcpy with hashcpy where lengts in memcpy\n\ns/lengts/lengths/\n\n> are already defined.\n>\n> Signed-off-by: Alberto Corona \n> ---\n> bundle.c | 2 +-\n> grep.c | 2 +-\n> refs.c | 2 +-\n> sha1_name.c | 4 ++--\n> 4 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index e99065c..7809fbb 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -19,7 +19,7 @@ static void add_to_ref_list(const unsigned char \n> *sha1, const char *name,\n> list->list = xrealloc(list->list,\n> list->alloc * sizeof(list->list[0]));\n> }\n> - memcpy(list->list[list->nr].sha1, sha1, 20);\n> + hashcpy(list->list[list->nr].sha1, sha1);\n> list->list[list->nr].name = xstrdup(name);\n> list->nr++;\n> }\n> diff --git a/grep.c b/grep.c\n> index c668034..f5101f7 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1650,7 +1650,7 @@ void grep_source_init(struct grep_source *gs, \n> enum grep_source_type type,\n> break;\n> case GREP_SOURCE_SHA1:\n> gs->identifier = xmalloc(20);\n> - memcpy(gs->identifier, identifier, 20);\n> + hashcpy(gs->identifier, identifier);\n> break;\n> case GREP_SOURCE_BUF:\n> gs->identifier = NULL;\n> diff --git a/refs.c b/refs.c\n> index 89228e2..f90b7ea 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1222,7 +1222,7 @@ static int resolve_gitlink_packed_ref(struct \n> ref_cache *refs,\n> if (ref == NULL)\n> return -1;\n>\n> - memcpy(sha1, ref->u.value.sha1, 20);\n> + hashcpy(sha1, ref->u.value.sha1);\n> return 0;\n> }\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 6fca869..3f5010f 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -111,7 +111,7 @@ static void find_short_object_filename(int len, \n> const char *hex_pfx, struct disa\n> continue;\n> if (memcmp(de->d_name, hex_pfx + 2, len - 2))\n> continue;\n> - memcpy(hex + 2, de->d_name, 38);\n> + hashcpy(hex + 2, de->d_name);\n> if (!get_sha1_hex(hex, sha1))\n> update_candidates(ds, sha1);\n> }\n> @@ -373,7 +373,7 @@ const char *find_unique_abbrev(const unsigned char \n> *sha1, int len)\n> static char hex[41];\n>\n> exists = has_sha1_file(sha1);\n> - memcpy(hex, sha1_to_hex(sha1), 40);\n> + hashcpy(hex, sha1_to_hex(sha1));\n> if (len == 40 || !len)\n> return hex;\n> while (len < 40) {\n> -- \n> 1.9.0.138.g2de3478.dirty\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"235813","messageId":"CAPig+cRVNP0Jdid33j8avMDkkf3XqVHxNLB6Fp3dgzOWw7i2Aw@mail.gmail.com","threadId":"36024","inReplyTo":"1393787977-13164-1-git-send-email-albcoron@gmail.com","subject":"Re: [PATCH] Replace memcpy with hashcpy when lengths defined","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-02T22:39:07Z","receivedAt":"2014-03-02T22:39:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 2, 2014 at 2:19 PM, Alberto wrote:\n> From: Alberto Corona \n>\n> Replaced memcpy with hashcpy where lengts in memcpy\n> are already defined.\n\nThis doesn't really explain what this patch is attempting to do. What\ndoes \"lengths already defined\" mean? It's also misleading if taken\nliterally (as seen below).\n\nInstead, you should say something about the purpose of the change. For\ninstance, explain that the change takes advantage of the abstraction\nprovided by hashcpy() rather than hardcoding knowledge about a\nparticular hash representation.\n\nMore below.\n\n> Signed-off-by: Alberto Corona \n> ---\n> bundle.c | 2 +-\n> grep.c | 2 +-\n> refs.c | 2 +-\n> sha1_name.c | 4 ++--\n> 4 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index e99065c..7809fbb 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -19,7 +19,7 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n> list->list = xrealloc(list->list,\n> list->alloc * sizeof(list->list[0]));\n> }\n> - memcpy(list->list[list->nr].sha1, sha1, 20);\n> + hashcpy(list->list[list->nr].sha1, sha1);\n> list->list[list->nr].name = xstrdup(name);\n> list->nr++;\n> }\n> diff --git a/grep.c b/grep.c\n> index c668034..f5101f7 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1650,7 +1650,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n> break;\n> case GREP_SOURCE_SHA1:\n> gs->identifier = xmalloc(20);\n> - memcpy(gs->identifier, identifier, 20);\n> + hashcpy(gs->identifier, identifier);\n> break;\n> case GREP_SOURCE_BUF:\n> gs->identifier = NULL;\n> diff --git a/refs.c b/refs.c\n> index 89228e2..f90b7ea 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1222,7 +1222,7 @@ static int resolve_gitlink_packed_ref(struct ref_cache *refs,\n> if (ref == NULL)\n> return -1;\n>\n> - memcpy(sha1, ref->u.value.sha1, 20);\n> + hashcpy(sha1, ref->u.value.sha1);\n> return 0;\n> }\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 6fca869..3f5010f 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -111,7 +111,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa\n> continue;\n> if (memcmp(de->d_name, hex_pfx + 2, len - 2))\n> continue;\n> - memcpy(hex + 2, de->d_name, 38);\n> + hashcpy(hex + 2, de->d_name);\n\nThis can't be correct. hashcpy() copies the binary representation of a\nhash (which is currently 20 bytes, as seen in the implementation of\nhashcpy() in cache.h). The fact that this particular memcpy() is\ncopying 38 bytes should be a clue that something is different here. In\nfact, for this case, 'hex' is a 40-character textual representation of\nthe hash, thus not suitable for hashcpy().\n\n> if (!get_sha1_hex(hex, sha1))\n> update_candidates(ds, sha1);\n> }\n> @@ -373,7 +373,7 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)\n> static char hex[41];\n>\n> exists = has_sha1_file(sha1);\n> - memcpy(hex, sha1_to_hex(sha1), 40);\n> + hashcpy(hex, sha1_to_hex(sha1));\n\nSame as above.\n\n> if (len == 40 || !len)\n> return hex;\n> while (len < 40) {\n> --\n> 1.9.0.138.g2de3478.dirty\n"}]}