{"thread":{"id":"36033","subject":"[PATCH v3] Replace memcpy with hashcpy when dealing hash copy globally","startedAt":"2014-03-03T09:39:59Z","lastAt":"2014-03-05T21:48:29Z","messageCount":2,"participants":["Sun He","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"235857","messageId":"1393839599-6955-1-git-send-email-sunheehnus@gmail.com","threadId":"36033","inReplyTo":null,"subject":"[PATCH v3] Replace memcpy with hashcpy when dealing hash copy globally","fromName":"Sun He","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-03T09:39:59Z","receivedAt":"2014-03-03T09:39:59Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"Replacing memcpy with hashcpy is more directly and elegant.\n\nLeave ppc/sha1.c alone, as it is an isolated component.\nPull cache.h(actually ../cache.h) in just for one memcpy\nthere is not proper.\n\nHelped-by: Michael Haggerty <mhagger@alum.mit.edu>\nHelped-by: Duy Nguyen <pclouds@gmail.com>\nSigned-off-by: Sun He <sunheehnus@gmail.com>\n---\n\nPATCH v3 delete the one-space indentation on each line of commit message\nas is suggested by Eric Sunshine.\n Thanks to Eric Sunshine.\n\nPATCH v2 leave ppc/sha1.c alone.\n\nThe general rule is if cache.h or git-compat-util.h is included,\nit is the first #include, and system includes will be always in\ngit-compat-tuil.h.\n       \t\t\t\tvia Duy Nguyen\n\nThe change in PATCH v1 is not proper because I placed cache.h\nin the end.\nAnd adding it to the head is not a good way to achieve the goal,\nas is said above \"---\".\n Thanks to Duy Nguyen.\n\nFind the potential places with memcpy by the bash command:\n $ find . | xargs grep \"memcpy.*\\(.*20.*\\)\"\n\nppc/sha1.c doesn't include cache.h and it cannot use hashcpy().\nSo just leave memcpy(in ppc/sha1.c) alone.\n\n bundle.c            | 2 +-\n grep.c              | 2 +-\n pack-bitmap-write.c | 2 +-\n reflog-walk.c       | 4 ++--\n refs.c              | 2 +-\n 5 files changed, 6 insertions(+), 6 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/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 1218bef..5f1791a 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -530,7 +530,7 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \theader.version = htons(default_version);\n \theader.options = htons(flags | options);\n \theader.entry_count = htonl(writer.selected_nr);\n-\tmemcpy(header.checksum, writer.pack_checksum, 20);\n+\thashcpy(header.checksum, writer.pack_checksum);\n \n \tsha1write(f, &header, sizeof(header));\n \tdump_bitmap(f, writer.commits);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex b2fbdb2..d490f7d 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -32,8 +32,8 @@ static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1,\n \t\t\tsizeof(struct reflog_info));\n \t}\n \titem = array->items + array->nr;\n-\tmemcpy(item->osha1, osha1, 20);\n-\tmemcpy(item->nsha1, nsha1, 20);\n+\thashcpy(item->osha1, osha1);\n+\thashcpy(item->nsha1, nsha1);\n \titem->email = xstrdup(email);\n \titem->timestamp = timestamp;\n \titem->tz = tz;\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 \n-- \n1.9.0.138.g2de3478.dirty\n"},{"id":"236132","messageId":"xmqqfvmwfhaq.fsf@gitster.dls.corp.google.com","threadId":"36033","inReplyTo":"1393839599-6955-1-git-send-email-sunheehnus@gmail.com","subject":"Re: [PATCH v3] Replace memcpy with hashcpy when dealing hash copy globally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T21:48:29Z","receivedAt":"2014-03-05T21:48:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sun He <sunheehnus@gmail.com> writes:\n\n> Replacing memcpy with hashcpy is more directly and elegant.\n\nCan we justify the change without being subjective?\n\n> Leave ppc/sha1.c alone, as it is an isolated component.\n> Pull cache.h(actually ../cache.h) in just for one memcpy\n> there is not proper.\n\nThat is not the reason why that memcpy of 20-byte must stay as it\nis.  If for whatever reason we later choose to switch to using\nanother hash function, say MD5, to come up with the object names,\nthe majority of memcpy(..., 20) must change to copy 16 bytes, and it\nmakes sense to contain that implementation-specific knowledge of\ntwenty behind the hashcpy() abstraction.  The 20-byte memcpy() call\nin ppc/sha1.c, however, is an implementation of *THE* SHA-1\nalgorithm, whose output is and will always be 20-byte.  It will not\nchange when we decide to replace what hash function is used to name\nour objects (which would result in updating the implementation of\nhashcpy()).  That is the reason why you shouldn't touch that one.\nIt has to be explicitly 20 byte, without ever getting affected by\nwhat length our hashcpy() may choose to copy.\n\nPerhaps...\n\n\tWe invented hashcpy() to keep the abstraction of \"object\n\tname\" behind it.  Use it instead of calling memcpy() with\n\thard-coded 20-byte length when moving object names between\n        pieces of memory.\n\n\tLeave ppc/sha1.c as-is, because the function *is* about\n\t*the* SHA-1 hash algorithm whose output is and will always\n\tbe 20-byte.\n\nor something.\n\n> Find the potential places with memcpy by the bash command:\n>  $ find . | xargs grep \"memcpy.*\\(.*20.*\\)\"\n\nIf you are planning to hack on git, learn how to use it first ;-)\n\n    $ git grep 'memcpy.*, 20)'\n"}]}