{"thread":{"id":"38745","subject":"[PATCH v2 00/10] Use a structure for object IDs.","startedAt":"2015-03-07T23:23:55Z","lastAt":"2015-03-15T02:32:29Z","messageCount":36,"participants":["brian m. carlson","Junio C Hamano","Duy Nguyen","Kyle J. McKay","Michael Haggerty"],"isPatch":true,"patchVersion":2,"patchTotal":10},"messages":[{"id":"257244","messageId":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":null,"subject":"[PATCH v2 00/10] Use a structure for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:23:55Z","receivedAt":"2015-03-07T23:23:55Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This is a patch series to convert some of the relevant uses of unsigned\nchar [20] to struct object_id.\n\nThe goal of this series to improve type-checking in the codebase and to\nmake it easier to move to a different hash function if the project\ndecides to do that. This series does not convert all of the codebase,\nbut only parts. I've dropped some of the patches from earlier (which no\nlonger apply) and added others.\n\nCertain parts of the code have to be converted before others to keep the\npatch sizes small, maintainable, and bisectable, so functions and\nstructures that are used across the codebase (e.g. struct object) will\nbe converted later. Conversion has been done in a somewhat haphazard\nmanner by converting modules with leaf functions and less commonly used\nstructs first.\n\nSince part of the goal is to ease a move to a different hash function if\nthe project decides to do that, I've adopted Michael Haggerty's\nsuggestion of using variables named \"oid\", naming the structure member\n\"sha1\", and using GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ as the constants.\n\nI've been holding on to this series for a long time in hopes of\nconverting more of the code before submitting, but realized that this\ndeprived others of the ability to use the new structure and required me\nto rebase extremely frequently.\n\nbrian m. carlson (10):\n Define a structure for object IDs.\n Define utility functions for object IDs.\n bisect.c: convert leaf functions to use struct object_id\n archive.c: convert to use struct object_id\n zip: use GIT_SHA1_HEXSZ for trailers\n bulk-checkin.c: convert to use struct object_id\n diff: convert struct combine_diff_path to object_id\n commit: convert parts to struct object_id\n patch-id: convert to use struct object_id\n apply: convert threeway_stage to object_id\n\n archive-zip.c | 4 ++--\n archive.c | 18 +++++++++---------\n bisect.c | 40 +++++++++++++++++++-------------------\n builtin/apply.c | 14 +++++++-------\n builtin/patch-id.c | 34 ++++++++++++++++-----------------\n bulk-checkin.c | 12 ++++++------\n cache.h | 37 ++++++++++++++++++++++++++++++++----\n combine-diff.c | 56 +++++++++++++++++++++++++++---------------------------\n commit.c | 32 +++++++++++++++----------------\n commit.h | 4 ++--\n diff-lib.c | 10 +++++-----\n diff.h | 5 +++--\n hex.c | 16 +++++++++++++---\n log-tree.c | 2 +-\n object.h | 8 ++++++++\n send-pack.c | 2 +-\n shallow.c | 8 ++++----\n tree-diff.c | 10 +++++-----\n upload-pack.c | 2 +-\n 19 files changed, 181 insertions(+), 133 deletions(-)\n\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257245","messageId":"1425770645-628957-2-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 01/10] Define a structure for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:23:56Z","receivedAt":"2015-03-07T23:23:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Many places throughout the code use \"unsigned char [20]\" to store object IDs\n(SHA-1 values). This leads to lots of hardcoded numbers throughout the\ncodebase. It also leads to confusion about the purposes of a buffer.\n\nIntroduce a structure for object IDs. This allows us to obtain the benefits\nof compile-time checking for misuse. The structure is expected to remain\nthe same size and have the same alignment requirements on all known\nplatforms, compared to the array of unsigned char, although this is not\nrequired for correctness.\n\nSigned-off-by: brian m. carlson \n---\n object.h | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/object.h b/object.h\nindex 6416247..f99b502 100644\n--- a/object.h\n+++ b/object.h\n@@ -1,6 +1,14 @@\n #ifndef OBJECT_H\n #define OBJECT_H\n \n+/* The length in bytes and in hex digits of an object name (SHA-1 value). */\n+#define GIT_SHA1_RAWSZ 20\n+#define GIT_SHA1_HEXSZ 40\n+\n+struct object_id {\n+\tunsigned char sha1[GIT_SHA1_RAWSZ];\n+};\n+\n struct object_list {\n \tstruct object *item;\n \tstruct object_list *next;\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257246","messageId":"1425770645-628957-3-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 02/10] Define utility functions for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:23:57Z","receivedAt":"2015-03-07T23:23:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"There are several utility functions (hashcmp and friends) that are used\nfor comparing object IDs (SHA-1 values). Using these functions, which\ntake pointers to unsigned char, with struct object_id requires tiresome\naccess to the sha1 member, which bloats code and violates the desired\nencapsulation. Provide wrappers around these functions for struct\nobject_id for neater, more maintainable code. Use the new constants to\navoid the hard-coded 20s and 40s throughout the original functions.\n\nThese functions simply call the underlying pointer-to-unsigned-char\nversions to ensure that any performance improvements will be passed\nthrough to the new functions.\n\nSigned-off-by: brian m. carlson \n---\nI'm not very excited about having to put the #include in the middle of\ncache.h. The alternative, of course, is to move enum object_type up,\nwhich I can do if necessary. Another alternative is to simply move the\nstruct object_id definitions to cache.h instead of object.h, which might\nbe cleaner.\n\nI'm interested in hearing opinions about the best way to go forward.\n\n cache.h | 37 +++++++++++++++++++++++++++++++++----\n hex.c | 16 +++++++++++++---\n 2 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 761c570..f9addcc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -368,6 +368,11 @@ static inline enum object_type object_type(unsigned int mode)\n \t\tOBJ_BLOB;\n }\n \n+/* This must go before the oid* functions, but after the declaration of enum\n+ * object_type.\n+ */\n+#include \"object.h\"\n+\n /* Double-check local_repo_env below if you add to this list. */\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n@@ -710,13 +715,13 @@ extern char *sha1_pack_name(const unsigned char *sha1);\n extern char *sha1_pack_index_name(const unsigned char *sha1);\n \n extern const char *find_unique_abbrev(const unsigned char *sha1, int);\n-extern const unsigned char null_sha1[20];\n+extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n \tint i;\n \n-\tfor (i = 0; i < 20; i++, sha1++, sha2++) {\n+\tfor (i = 0; i < GIT_SHA1_RAWSZ; i++, sha1++, sha2++) {\n \t\tif (*sha1 != *sha2)\n \t\t\treturn *sha1 - *sha2;\n \t}\n@@ -724,20 +729,42 @@ static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n \treturn 0;\n }\n \n+static inline int oidcmp(const struct object_id *o1, const struct object_id *o2)\n+{\n+\treturn hashcmp(o1->sha1, o2->sha1);\n+}\n+\n static inline int is_null_sha1(const unsigned char *sha1)\n {\n \treturn !hashcmp(sha1, null_sha1);\n }\n \n+static inline int is_null_oid(const struct object_id *oid)\n+{\n+\treturn !hashcmp(oid->sha1, null_sha1);\n+}\n+\n static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src)\n {\n-\tmemcpy(sha_dst, sha_src, 20);\n+\tmemcpy(sha_dst, sha_src, GIT_SHA1_RAWSZ);\n }\n+\n+static inline void oidcpy(struct object_id *dst, const struct object_id *src)\n+{\n+\thashcpy(dst->sha1, src->sha1);\n+}\n+\n static inline void hashclr(unsigned char *hash)\n {\n-\tmemset(hash, 0, 20);\n+\tmemset(hash, 0, GIT_SHA1_RAWSZ);\n }\n \n+static inline void oidclr(struct object_id *oid)\n+{\n+\thashclr(oid->sha1);\n+}\n+\n+\n #define EMPTY_TREE_SHA1_HEX \\\n \t\"4b825dc642cb6eb9a060e54bf8d69288fbee4904\"\n #define EMPTY_TREE_SHA1_BIN_LITERAL \\\n@@ -944,8 +971,10 @@ extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);\n * null-terminated string.\n */\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n+extern int get_oid_hex(const char *hex, struct object_id *sha1);\n \n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n+extern char *oid_to_hex(const struct object_id *oid);\t/* same static buffer as sha1_to_hex */\n extern int read_ref_full(const char *refname, int resolve_flags,\n \t\t\t unsigned char *sha1, int *flags);\n extern int read_ref(const char *refname, unsigned char *sha1);\ndiff --git a/hex.c b/hex.c\nindex cfd9d72..7e25a88 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -38,7 +38,7 @@ const signed char hexval_table[256] = {\n int get_sha1_hex(const char *hex, unsigned char *sha1)\n {\n \tint i;\n-\tfor (i = 0; i < 20; i++) {\n+\tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n \t\tunsigned int val;\n \t\t/*\n \t\t * hex[1]=='\\0' is caught when val is checked below,\n@@ -56,15 +56,20 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)\n \treturn 0;\n }\n \n+int get_oid_hex(const char *hex, struct object_id *oid)\n+{\n+\treturn get_sha1_hex(hex, oid->sha1);\n+}\n+\n char *sha1_to_hex(const unsigned char *sha1)\n {\n \tstatic int bufno;\n-\tstatic char hexbuffer[4][41];\n+\tstatic char hexbuffer[4][GIT_SHA1_HEXSZ + 1];\n \tstatic const char hex[] = \"0123456789abcdef\";\n \tchar *buffer = hexbuffer[3 & ++bufno], *buf = buffer;\n \tint i;\n \n-\tfor (i = 0; i < 20; i++) {\n+\tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n \t\tunsigned int val = *sha1++;\n \t\t*buf++ = hex[val >> 4];\n \t\t*buf++ = hex[val & 0xf];\n@@ -73,3 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)\n \n \treturn buffer;\n }\n+\n+char *oid_to_hex(const struct object_id *oid)\n+{\n+\treturn sha1_to_hex(oid->sha1);\n+}\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257252","messageId":"1425770645-628957-4-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 03/10] bisect.c: convert leaf functions to use struct object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:23:58Z","receivedAt":"2015-03-07T23:23:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Convert some constants to GIT_SHA1_HEXSZ.\n\nSigned-off-by: brian m. carlson \n---\n bisect.c | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 8c6d843..2692d54 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -15,7 +15,7 @@\n static struct sha1_array good_revs;\n static struct sha1_array skipped_revs;\n \n-static unsigned char *current_bad_sha1;\n+static struct object_id *current_bad_oid;\n \n static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n@@ -404,8 +404,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,\n \t\t\tint flags, void *cb_data)\n {\n \tif (!strcmp(refname, \"bad\")) {\n-\t\tcurrent_bad_sha1 = xmalloc(20);\n-\t\thashcpy(current_bad_sha1, sha1);\n+\t\tcurrent_bad_oid = xmalloc(sizeof(*current_bad_oid));\n+\t\thashcpy(current_bad_oid->sha1, sha1);\n \t} else if (starts_with(refname, \"good-\")) {\n \t\tsha1_array_append(&good_revs, sha1);\n \t} else if (starts_with(refname, \"skip-\")) {\n@@ -564,7 +564,7 @@ static struct commit_list *skip_away(struct commit_list *list, int count)\n \n \tfor (i = 0; cur; cur = cur->next, i++) {\n \t\tif (i == index) {\n-\t\t\tif (hashcmp(cur->item->object.sha1, current_bad_sha1))\n+\t\t\tif (hashcmp(cur->item->object.sha1, current_bad_oid->sha1))\n \t\t\t\treturn cur;\n \t\t\tif (previous)\n \t\t\t\treturn previous;\n@@ -607,7 +607,7 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix,\n \n \t/* rev_argv.argv[0] will be ignored by setup_revisions */\n \targv_array_push(&rev_argv, \"bisect_rev_setup\");\n-\targv_array_pushf(&rev_argv, bad_format, sha1_to_hex(current_bad_sha1));\n+\targv_array_pushf(&rev_argv, bad_format, sha1_to_hex(current_bad_oid->sha1));\n \tfor (i = 0; i < good_revs.nr; i++)\n \t\targv_array_pushf(&rev_argv, good_format,\n \t\t\t\t sha1_to_hex(good_revs.sha1[i]));\n@@ -628,7 +628,7 @@ static void bisect_common(struct rev_info *revs)\n }\n \n static void exit_if_skipped_commits(struct commit_list *tried,\n-\t\t\t\t const unsigned char *bad)\n+\t\t\t\t const struct object_id *bad)\n {\n \tif (!tried)\n \t\treturn;\n@@ -637,12 +637,12 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \t \"The first bad commit could be any of:\\n\");\n \tprint_commit_list(tried, \"%s\\n\", \"%s\\n\");\n \tif (bad)\n-\t\tprintf(\"%s\\n\", sha1_to_hex(bad));\n+\t\tprintf(\"%s\\n\", oid_to_hex(bad));\n \tprintf(\"We cannot bisect more!\\n\");\n \texit(2);\n }\n \n-static int is_expected_rev(const unsigned char *sha1)\n+static int is_expected_rev(const struct object_id *oid)\n {\n \tconst char *filename = git_path(\"BISECT_EXPECTED_REV\");\n \tstruct stat st;\n@@ -658,7 +658,7 @@ static int is_expected_rev(const unsigned char *sha1)\n \t\treturn 0;\n \n \tif (strbuf_getline(&str, fp, '\\n') != EOF)\n-\t\tres = !strcmp(str.buf, sha1_to_hex(sha1));\n+\t\tres = !strcmp(str.buf, oid_to_hex(oid));\n \n \tstrbuf_release(&str);\n \tfclose(fp);\n@@ -719,7 +719,7 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)\n \tstruct commit **rev = xmalloc(len * sizeof(*rev));\n \tint i, n = 0;\n \n-\trev[n++] = get_commit_reference(current_bad_sha1);\n+\trev[n++] = get_commit_reference(current_bad_oid->sha1);\n \tfor (i = 0; i < good_revs.nr; i++)\n \t\trev[n++] = get_commit_reference(good_revs.sha1[i]);\n \t*rev_nr = n;\n@@ -729,8 +729,8 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)\n \n static void handle_bad_merge_base(void)\n {\n-\tif (is_expected_rev(current_bad_sha1)) {\n-\t\tchar *bad_hex = sha1_to_hex(current_bad_sha1);\n+\tif (is_expected_rev(current_bad_oid)) {\n+\t\tchar *bad_hex = oid_to_hex(current_bad_oid);\n \t\tchar *good_hex = join_sha1_array_hex(&good_revs, ' ');\n \n \t\tfprintf(stderr, \"The merge base %s is bad.\\n\"\n@@ -750,7 +750,7 @@ static void handle_bad_merge_base(void)\n static void handle_skipped_merge_base(const unsigned char *mb)\n {\n \tchar *mb_hex = sha1_to_hex(mb);\n-\tchar *bad_hex = sha1_to_hex(current_bad_sha1);\n+\tchar *bad_hex = sha1_to_hex(current_bad_oid->sha1);\n \tchar *good_hex = join_sha1_array_hex(&good_revs, ' ');\n \n \twarning(\"the merge base between %s and [%s] \"\n@@ -781,7 +781,7 @@ static void check_merge_bases(int no_checkout)\n \n \tfor (; result; result = result->next) {\n \t\tconst unsigned char *mb = result->item->object.sha1;\n-\t\tif (!hashcmp(mb, current_bad_sha1)) {\n+\t\tif (!hashcmp(mb, current_bad_oid->sha1)) {\n \t\t\thandle_bad_merge_base();\n \t\t} else if (0 <= sha1_array_lookup(&good_revs, mb)) {\n \t\t\tcontinue;\n@@ -838,7 +838,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)\n \tstruct stat st;\n \tint fd;\n \n-\tif (!current_bad_sha1)\n+\tif (!current_bad_oid)\n \t\tdie(\"a bad revision is needed\");\n \n \t/* Check if file BISECT_ANCESTORS_OK exists. */\n@@ -903,7 +903,7 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n \tconst unsigned char *bisect_rev;\n-\tchar bisect_rev_hex[41];\n+\tchar bisect_rev_hex[GIT_SHA1_HEXSZ + 1];\n \n \tif (read_bisect_refs())\n \t\tdie(\"reading bisect refs failed\");\n@@ -927,7 +927,7 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \t\texit_if_skipped_commits(tried, NULL);\n \n \t\tprintf(\"%s was both good and bad\\n\",\n-\t\t sha1_to_hex(current_bad_sha1));\n+\t\t oid_to_hex(current_bad_oid));\n \t\texit(1);\n \t}\n \n@@ -938,10 +938,10 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \t}\n \n \tbisect_rev = revs.commits->item->object.sha1;\n-\tmemcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), 41);\n+\tmemcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);\n \n-\tif (!hashcmp(bisect_rev, current_bad_sha1)) {\n-\t\texit_if_skipped_commits(tried, current_bad_sha1);\n+\tif (!hashcmp(bisect_rev, current_bad_oid->sha1)) {\n+\t\texit_if_skipped_commits(tried, current_bad_oid);\n \t\tprintf(\"%s is the first bad commit\\n\", bisect_rev_hex);\n \t\tshow_diff_tree(prefix, revs.commits->item);\n \t\t/* This means the bisection process succeeded. */\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257249","messageId":"1425770645-628957-5-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 04/10] archive.c: convert to use struct object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:23:59Z","receivedAt":"2015-03-07T23:23:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Convert a hard-coded 20 as well.\n\nSigned-off-by: brian m. carlson \n---\n archive.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 96057ed..46d9025 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -101,7 +101,7 @@ static void setup_archive_check(struct git_attr_check *check)\n \n struct directory {\n \tstruct directory *up;\n-\tunsigned char sha1[20];\n+\tunsigned char sha1[GIT_SHA1_RAWSZ];\n \tint baselen, len;\n \tunsigned mode;\n \tint stage;\n@@ -354,7 +354,7 @@ static void parse_treeish_arg(const char **argv,\n \ttime_t archive_time;\n \tstruct tree *tree;\n \tconst struct commit *commit;\n-\tunsigned char sha1[20];\n+\tstruct object_id oid;\n \n \t/* Remotes are only allowed to fetch actual refs */\n \tif (remote && !remote_allow_unreachable) {\n@@ -362,15 +362,15 @@ static void parse_treeish_arg(const char **argv,\n \t\tconst char *colon = strchrnul(name, ':');\n \t\tint refnamelen = colon - name;\n \n-\t\tif (!dwim_ref(name, refnamelen, sha1, &ref))\n+\t\tif (!dwim_ref(name, refnamelen, oid.sha1, &ref))\n \t\t\tdie(\"no such ref: %.*s\", refnamelen, name);\n \t\tfree(ref);\n \t}\n \n-\tif (get_sha1(name, sha1))\n+\tif (get_sha1(name, oid.sha1))\n \t\tdie(\"Not a valid object name\");\n \n-\tcommit = lookup_commit_reference_gently(sha1, 1);\n+\tcommit = lookup_commit_reference_gently(oid.sha1, 1);\n \tif (commit) {\n \t\tcommit_sha1 = commit->object.sha1;\n \t\tarchive_time = commit->date;\n@@ -379,21 +379,21 @@ static void parse_treeish_arg(const char **argv,\n \t\tarchive_time = time(NULL);\n \t}\n \n-\ttree = parse_tree_indirect(sha1);\n+\ttree = parse_tree_indirect(oid.sha1);\n \tif (tree == NULL)\n \t\tdie(\"not a tree object\");\n \n \tif (prefix) {\n-\t\tunsigned char tree_sha1[20];\n+\t\tstruct object_id tree_oid;\n \t\tunsigned int mode;\n \t\tint err;\n \n \t\terr = get_tree_entry(tree->object.sha1, prefix,\n-\t\t\t\t tree_sha1, &mode);\n+\t\t\t\t tree_oid.sha1, &mode);\n \t\tif (err || !S_ISDIR(mode))\n \t\t\tdie(\"current working directory is untracked\");\n \n-\t\ttree = parse_tree_indirect(tree_sha1);\n+\t\ttree = parse_tree_indirect(tree_oid.sha1);\n \t}\n \tar_args->tree = tree;\n \tar_args->commit_sha1 = commit_sha1;\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257254","messageId":"1425770645-628957-6-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 05/10] zip: use GIT_SHA1_HEXSZ for trailers","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:24:00Z","receivedAt":"2015-03-07T23:24:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Signed-off-by: brian m. carlson \n---\n archive-zip.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex 4bde019..b669e50 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -427,12 +427,12 @@ static void write_zip_trailer(const unsigned char *sha1)\n \tcopy_le16(trailer.entries, zip_dir_entries);\n \tcopy_le32(trailer.size, zip_dir_offset);\n \tcopy_le32(trailer.offset, zip_offset);\n-\tcopy_le16(trailer.comment_length, sha1 ? 40 : 0);\n+\tcopy_le16(trailer.comment_length, sha1 ? GIT_SHA1_HEXSZ : 0);\n \n \twrite_or_die(1, zip_dir, zip_dir_offset);\n \twrite_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);\n \tif (sha1)\n-\t\twrite_or_die(1, sha1_to_hex(sha1), 40);\n+\t\twrite_or_die(1, sha1_to_hex(sha1), GIT_SHA1_HEXSZ);\n }\n \n static void dos_time(time_t *time, int *dos_date, int *dos_time)\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257251","messageId":"1425770645-628957-7-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 06/10] bulk-checkin.c: convert to use struct object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:24:01Z","receivedAt":"2015-03-07T23:24:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Signed-off-by: brian m. carlson \n---\n bulk-checkin.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 0c4b8a7..e50f60e 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -24,7 +24,7 @@ static struct bulk_checkin_state {\n \n static void finish_bulk_checkin(struct bulk_checkin_state *state)\n {\n-\tunsigned char sha1[20];\n+\tstruct object_id oid;\n \tstruct strbuf packname = STRBUF_INIT;\n \tint i;\n \n@@ -36,11 +36,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n \t\tunlink(state->pack_tmp_name);\n \t\tgoto clear_exit;\n \t} else if (state->nr_written == 1) {\n-\t\tsha1close(state->f, sha1, CSUM_FSYNC);\n+\t\tsha1close(state->f, oid.sha1, CSUM_FSYNC);\n \t} else {\n-\t\tint fd = sha1close(state->f, sha1, 0);\n-\t\tfixup_pack_header_footer(fd, sha1, state->pack_tmp_name,\n-\t\t\t\t\t state->nr_written, sha1,\n+\t\tint fd = sha1close(state->f, oid.sha1, 0);\n+\t\tfixup_pack_header_footer(fd, oid.sha1, state->pack_tmp_name,\n+\t\t\t\t\t state->nr_written, oid.sha1,\n \t\t\t\t\t state->offset);\n \t\tclose(fd);\n \t}\n@@ -48,7 +48,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)\n \tstrbuf_addf(&packname, \"%s/pack/pack-\", get_object_directory());\n \tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n-\t\t\t &state->pack_idx_opts, sha1);\n+\t\t\t &state->pack_idx_opts, oid.sha1);\n \tfor (i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257253","messageId":"1425770645-628957-8-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 07/10] diff: convert struct combine_diff_path to object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:24:02Z","receivedAt":"2015-03-07T23:24:02Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Also, convert a constant to GIT_SHA1_HEXSZ.\n\nSigned-off-by: brian m. carlson \n---\n combine-diff.c | 56 ++++++++++++++++++++++++++++----------------------------\n diff-lib.c | 10 +++++-----\n diff.h | 5 +++--\n tree-diff.c | 10 +++++-----\n 4 files changed, 41 insertions(+), 40 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 91edce5..ec25202 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -44,9 +44,9 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,\n \t\t\tmemset(p->parent, 0,\n \t\t\t sizeof(p->parent[0]) * num_parent);\n \n-\t\t\thashcpy(p->sha1, q->queue[i]->two->sha1);\n+\t\t\thashcpy(p->oid.sha1, q->queue[i]->two->sha1);\n \t\t\tp->mode = q->queue[i]->two->mode;\n-\t\t\thashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);\n+\t\t\thashcpy(p->parent[n].oid.sha1, q->queue[i]->one->sha1);\n \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\tp->parent[n].status = q->queue[i]->status;\n \t\t\t*tail = p;\n@@ -77,7 +77,7 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,\n \t\t\tcontinue;\n \t\t}\n \n-\t\thashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);\n+\t\thashcpy(p->parent[n].oid.sha1, q->queue[i]->one->sha1);\n \t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\tp->parent[n].status = q->queue[i]->status;\n \n@@ -284,7 +284,7 @@ static struct lline *coalesce_lines(struct lline *base, int *lenbase,\n \treturn base;\n }\n \n-static char *grab_blob(const unsigned char *sha1, unsigned int mode,\n+static char *grab_blob(const struct object_id *oid, unsigned int mode,\n \t\t unsigned long *size, struct userdiff_driver *textconv,\n \t\t const char *path)\n {\n@@ -294,20 +294,20 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,\n \tif (S_ISGITLINK(mode)) {\n \t\tblob = xmalloc(100);\n \t\t*size = snprintf(blob, 100,\n-\t\t\t\t \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n-\t} else if (is_null_sha1(sha1)) {\n+\t\t\t\t \"Subproject commit %s\\n\", oid_to_hex(oid));\n+\t} else if (is_null_oid(oid)) {\n \t\t/* deleted blob */\n \t\t*size = 0;\n \t\treturn xcalloc(1, 1);\n \t} else if (textconv) {\n \t\tstruct diff_filespec *df = alloc_filespec(path);\n-\t\tfill_filespec(df, sha1, 1, mode);\n+\t\tfill_filespec(df, oid->sha1, 1, mode);\n \t\t*size = fill_textconv(textconv, df, &blob);\n \t\tfree_filespec(df);\n \t} else {\n-\t\tblob = read_sha1_file(sha1, &type, size);\n+\t\tblob = read_sha1_file(oid->sha1, &type, size);\n \t\tif (type != OBJ_BLOB)\n-\t\t\tdie(\"object '%s' is not a blob!\", sha1_to_hex(sha1));\n+\t\t\tdie(\"object '%s' is not a blob!\", oid_to_hex(oid));\n \t}\n \treturn blob;\n }\n@@ -389,7 +389,7 @@ static void consume_line(void *state_, char *line, unsigned long len)\n \t}\n }\n \n-static void combine_diff(const unsigned char *parent, unsigned int mode,\n+static void combine_diff(const struct object_id *parent, unsigned int mode,\n \t\t\t mmfile_t *result_file,\n \t\t\t struct sline *sline, unsigned int cnt, int n,\n \t\t\t int num_parent, int result_deleted,\n@@ -897,7 +897,7 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t\t int show_file_header)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n-\tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n+\tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? GIT_SHA1_HEXSZ : DEFAULT_ABBREV;\n \tconst char *a_prefix = opt->a_prefix ? opt->a_prefix : \"a/\";\n \tconst char *b_prefix = opt->b_prefix ? opt->b_prefix : \"b/\";\n \tconst char *c_meta = diff_get_color_opt(opt, DIFF_METAINFO);\n@@ -914,11 +914,11 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t \"\", elem->path, line_prefix, c_meta, c_reset);\n \tprintf(\"%s%sindex \", line_prefix, c_meta);\n \tfor (i = 0; i < num_parent; i++) {\n-\t\tabb = find_unique_abbrev(elem->parent[i].sha1,\n+\t\tabb = find_unique_abbrev(elem->parent[i].oid.sha1,\n \t\t\t\t\t abbrev);\n \t\tprintf(\"%s%s\", i ? \",\" : \"\", abb);\n \t}\n-\tabb = find_unique_abbrev(elem->sha1, abbrev);\n+\tabb = find_unique_abbrev(elem->oid.sha1, abbrev);\n \tprintf(\"..%s%s\\n\", abb, c_reset);\n \n \tif (mode_differs) {\n@@ -991,7 +991,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n-\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size,\n+\t\tresult = grab_blob(&elem->oid, elem->mode, &result_size,\n \t\t\t\t textconv, elem->path);\n \telse {\n \t\t/* Used by diff-tree to read from the working tree */\n@@ -1013,12 +1013,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tresult = strbuf_detach(&buf, NULL);\n \t\t\telem->mode = canon_mode(st.st_mode);\n \t\t} else if (S_ISDIR(st.st_mode)) {\n-\t\t\tunsigned char sha1[20];\n-\t\t\tif (resolve_gitlink_ref(elem->path, \"HEAD\", sha1) < 0)\n-\t\t\t\tresult = grab_blob(elem->sha1, elem->mode,\n+\t\t\tstruct object_id oid;\n+\t\t\tif (resolve_gitlink_ref(elem->path, \"HEAD\", oid.sha1) < 0)\n+\t\t\t\tresult = grab_blob(&elem->oid, elem->mode,\n \t\t\t\t\t\t &result_size, NULL, NULL);\n \t\t\telse\n-\t\t\t\tresult = grab_blob(sha1, elem->mode,\n+\t\t\t\tresult = grab_blob(&oid, elem->mode,\n \t\t\t\t\t\t &result_size, NULL, NULL);\n \t\t} else if (textconv) {\n \t\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n@@ -1090,7 +1090,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\tfor (i = 0; !is_binary && i < num_parent; i++) {\n \t\t\tchar *buf;\n \t\t\tunsigned long size;\n-\t\t\tbuf = grab_blob(elem->parent[i].sha1,\n+\t\t\tbuf = grab_blob(&elem->parent[i].oid,\n \t\t\t\t\telem->parent[i].mode,\n \t\t\t\t\t&size, NULL, NULL);\n \t\t\tif (buffer_is_binary(buf, size))\n@@ -1139,14 +1139,14 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tfor (i = 0; i < num_parent; i++) {\n \t\tint j;\n \t\tfor (j = 0; j < i; j++) {\n-\t\t\tif (!hashcmp(elem->parent[i].sha1,\n-\t\t\t\t elem->parent[j].sha1)) {\n+\t\t\tif (!hashcmp(elem->parent[i].oid.sha1,\n+\t\t\t\t elem->parent[j].oid.sha1)) {\n \t\t\t\treuse_combine_diff(sline, cnt, i, j);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t\tif (i <= j)\n-\t\t\tcombine_diff(elem->parent[i].sha1,\n+\t\t\tcombine_diff(&elem->parent[i].oid,\n \t\t\t\t elem->parent[i].mode,\n \t\t\t\t &result_file, sline,\n \t\t\t\t cnt, i, num_parent, result_deleted,\n@@ -1206,9 +1206,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \n \t\t/* Show sha1's */\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tprintf(\" %s\", diff_unique_abbrev(p->parent[i].sha1,\n+\t\t\tprintf(\" %s\", diff_unique_abbrev(p->parent[i].oid.sha1,\n \t\t\t\t\t\t\t opt->abbrev));\n-\t\tprintf(\" %s \", diff_unique_abbrev(p->sha1, opt->abbrev));\n+\t\tprintf(\" %s \", diff_unique_abbrev(p->oid.sha1, opt->abbrev));\n \t}\n \n \tif (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {\n@@ -1271,16 +1271,16 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n \tfor (i = 0; i < num_parent; i++) {\n \t\tpair->one[i].path = p->path;\n \t\tpair->one[i].mode = p->parent[i].mode;\n-\t\thashcpy(pair->one[i].sha1, p->parent[i].sha1);\n-\t\tpair->one[i].sha1_valid = !is_null_sha1(p->parent[i].sha1);\n+\t\thashcpy(pair->one[i].sha1, p->parent[i].oid.sha1);\n+\t\tpair->one[i].sha1_valid = !is_null_oid(&p->parent[i].oid);\n \t\tpair->one[i].has_more_entries = 1;\n \t}\n \tpair->one[num_parent - 1].has_more_entries = 0;\n \n \tpair->two->path = p->path;\n \tpair->two->mode = p->mode;\n-\thashcpy(pair->two->sha1, p->sha1);\n-\tpair->two->sha1_valid = !is_null_sha1(p->sha1);\n+\thashcpy(pair->two->sha1, p->oid.sha1);\n+\tpair->two->sha1_valid = !is_null_oid(&p->oid);\n \treturn pair;\n }\n \ndiff --git a/diff-lib.c b/diff-lib.c\nindex a85c497..60a7d6f 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -125,7 +125,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tdpath->next = NULL;\n \t\t\tmemcpy(dpath->path, ce->name, path_len);\n \t\t\tdpath->path[path_len] = '\\0';\n-\t\t\thashclr(dpath->sha1);\n+\t\t\toidclr(&dpath->oid);\n \t\t\tmemset(&(dpath->parent[0]), 0,\n \t\t\t sizeof(struct combine_diff_parent)*5);\n \n@@ -155,7 +155,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\tif (2 <= stage) {\n \t\t\t\t\tint mode = nce->ce_mode;\n \t\t\t\t\tnum_compare_stages++;\n-\t\t\t\t\thashcpy(dpath->parent[stage-2].sha1, nce->sha1);\n+\t\t\t\t\thashcpy(dpath->parent[stage-2].oid.sha1, nce->sha1);\n \t\t\t\t\tdpath->parent[stage-2].mode = ce_mode_from_stat(nce, mode);\n \t\t\t\t\tdpath->parent[stage-2].status =\n \t\t\t\t\t\tDIFF_STATUS_MODIFIED;\n@@ -339,14 +339,14 @@ static int show_modified(struct rev_info *revs,\n \t\tmemcpy(p->path, new->name, pathlen);\n \t\tp->path[pathlen] = 0;\n \t\tp->mode = mode;\n-\t\thashclr(p->sha1);\n+\t\toidclr(&p->oid);\n \t\tmemset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));\n \t\tp->parent[0].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[0].mode = new->ce_mode;\n-\t\thashcpy(p->parent[0].sha1, new->sha1);\n+\t\thashcpy(p->parent[0].oid.sha1, new->sha1);\n \t\tp->parent[1].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[1].mode = old->ce_mode;\n-\t\thashcpy(p->parent[1].sha1, old->sha1);\n+\t\thashcpy(p->parent[1].oid.sha1, old->sha1);\n \t\tshow_combined_diff(p, 2, revs->dense_combined_merges, revs);\n \t\tfree(p);\n \t\treturn 0;\ndiff --git a/diff.h b/diff.h\nindex b4a624d..f6fdf49 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -6,6 +6,7 @@\n \n #include \"tree-walk.h\"\n #include \"pathspec.h\"\n+#include \"object.h\"\n \n struct rev_info;\n struct diff_options;\n@@ -207,11 +208,11 @@ struct combine_diff_path {\n \tstruct combine_diff_path *next;\n \tchar *path;\n \tunsigned int mode;\n-\tunsigned char sha1[20];\n+\tstruct object_id oid;\n \tstruct combine_diff_parent {\n \t\tchar status;\n \t\tunsigned int mode;\n-\t\tunsigned char sha1[20];\n+\t\tstruct object_id oid;\n \t} parent[FLEX_ARRAY];\n };\n #define combine_diff_path_size(n, l) \\\ndiff --git a/tree-diff.c b/tree-diff.c\nindex e7b378c..8631c28 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -64,7 +64,7 @@ static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_\n {\n \tstruct combine_diff_parent *p0 = &p->parent[0];\n \tif (p->mode && p0->mode) {\n-\t\topt->change(opt, p0->mode, p->mode, p0->sha1, p->sha1,\n+\t\topt->change(opt, p0->mode, p->mode, p0->oid.sha1, p->oid.sha1,\n \t\t\t1, 1, p->path, 0, 0);\n \t}\n \telse {\n@@ -74,11 +74,11 @@ static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_\n \n \t\tif (p->mode) {\n \t\t\taddremove = '+';\n-\t\t\tsha1 = p->sha1;\n+\t\t\tsha1 = p->oid.sha1;\n \t\t\tmode = p->mode;\n \t\t} else {\n \t\t\taddremove = '-';\n-\t\t\tsha1 = p0->sha1;\n+\t\t\tsha1 = p0->oid.sha1;\n \t\t\tmode = p0->mode;\n \t\t}\n \n@@ -151,7 +151,7 @@ static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,\n \tmemcpy(p->path + base->len, path, pathlen);\n \tp->path[len] = 0;\n \tp->mode = mode;\n-\thashcpy(p->sha1, sha1 ? sha1 : null_sha1);\n+\thashcpy(p->oid.sha1, sha1 ? sha1 : null_sha1);\n \n \treturn p;\n }\n@@ -238,7 +238,7 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,\n \t\t\t}\n \n \t\t\tp->parent[i].mode = mode_i;\n-\t\t\thashcpy(p->parent[i].sha1, sha1_i ? sha1_i : null_sha1);\n+\t\t\thashcpy(p->parent[i].oid.sha1, sha1_i ? sha1_i : null_sha1);\n \t\t}\n \n \t\tkeep = 1;\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257248","messageId":"1425770645-628957-9-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 08/10] commit: convert parts to struct object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:24:03Z","receivedAt":"2015-03-07T23:24:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Convert struct commit_graft and necessary local parts of commit.c.\n\nSigned-off-by: brian m. carlson \n---\n commit.c | 32 ++++++++++++++++----------------\n commit.h | 4 ++--\n log-tree.c | 2 +-\n send-pack.c | 2 +-\n shallow.c | 8 ++++----\n upload-pack.c | 2 +-\n 6 files changed, 25 insertions(+), 25 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex a8c7577..89c207e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -55,12 +55,12 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \n struct commit *lookup_commit_reference_by_name(const char *name)\n {\n-\tunsigned char sha1[20];\n+\tstruct object_id oid;\n \tstruct commit *commit;\n \n-\tif (get_sha1_committish(name, sha1))\n+\tif (get_sha1_committish(name, oid.sha1))\n \t\treturn NULL;\n-\tcommit = lookup_commit_reference(sha1);\n+\tcommit = lookup_commit_reference(oid.sha1);\n \tif (parse_commit(commit))\n \t\treturn NULL;\n \treturn commit;\n@@ -99,7 +99,7 @@ static int commit_graft_alloc, commit_graft_nr;\n static const unsigned char *commit_graft_sha1_access(size_t index, void *table)\n {\n \tstruct commit_graft **commit_graft_table = table;\n-\treturn commit_graft_table[index]->sha1;\n+\treturn commit_graft_table[index]->oid.sha1;\n }\n \n static int commit_graft_pos(const unsigned char *sha1)\n@@ -110,7 +110,7 @@ static int commit_graft_pos(const unsigned char *sha1)\n \n int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n {\n-\tint pos = commit_graft_pos(graft->sha1);\n+\tint pos = commit_graft_pos(graft->oid.sha1);\n \n \tif (0 <= pos) {\n \t\tif (ignore_dups)\n@@ -148,12 +148,12 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \ti = (len + 1) / 41 - 1;\n \tgraft = xmalloc(sizeof(*graft) + 20 * i);\n \tgraft->nr_parent = i;\n-\tif (get_sha1_hex(buf, graft->sha1))\n+\tif (get_oid_hex(buf, &graft->oid))\n \t\tgoto bad_graft_data;\n \tfor (i = 40; i < len; i += 41) {\n \t\tif (buf[i] != ' ')\n \t\t\tgoto bad_graft_data;\n-\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/41]))\n+\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/41].sha1))\n \t\t\tgoto bad_graft_data;\n \t}\n \treturn graft;\n@@ -302,7 +302,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n {\n \tconst char *tail = buffer;\n \tconst char *bufptr = buffer;\n-\tunsigned char parent[20];\n+\tstruct object_id parent;\n \tstruct commit_list **pptr;\n \tstruct commit_graft *graft;\n \n@@ -312,10 +312,10 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \ttail += size;\n \tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n-\tif (get_sha1_hex(bufptr + 5, parent) < 0)\n+\tif (get_sha1_hex(bufptr + 5, parent.sha1) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t sha1_to_hex(item->object.sha1));\n-\titem->tree = lookup_tree(parent);\n+\titem->tree = lookup_tree(parent.sha1);\n \tbufptr += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \tpptr = &item->parents;\n \n@@ -324,7 +324,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \t\tstruct commit *new_parent;\n \n \t\tif (tail <= bufptr + 48 ||\n-\t\t get_sha1_hex(bufptr + 7, parent) ||\n+\t\t get_sha1_hex(bufptr + 7, parent.sha1) ||\n \t\t bufptr[47] != '\\n')\n \t\t\treturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n \t\tbufptr += 48;\n@@ -334,7 +334,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \t\t */\n \t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n \t\t\tcontinue;\n-\t\tnew_parent = lookup_commit(parent);\n+\t\tnew_parent = lookup_commit(parent.sha1);\n \t\tif (new_parent)\n \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n \t}\n@@ -342,7 +342,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \t\tint i;\n \t\tstruct commit *new_parent;\n \t\tfor (i = 0; i < graft->nr_parent; i++) {\n-\t\t\tnew_parent = lookup_commit(graft->parent[i]);\n+\t\t\tnew_parent = lookup_commit(graft->parent[i].sha1);\n \t\t\tif (!new_parent)\n \t\t\t\tcontinue;\n \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n@@ -1580,10 +1580,10 @@ struct commit *get_merge_parent(const char *name)\n {\n \tstruct object *obj;\n \tstruct commit *commit;\n-\tunsigned char sha1[20];\n-\tif (get_sha1(name, sha1))\n+\tstruct object_id oid;\n+\tif (get_sha1(name, oid.sha1))\n \t\treturn NULL;\n-\tobj = parse_object(sha1);\n+\tobj = parse_object(oid.sha1);\n \tcommit = (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n \tif (commit && !commit->util) {\n \t\tstruct merge_remote_desc *desc;\ndiff --git a/commit.h b/commit.h\nindex 9f189cb..ed3a1d5 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -226,9 +226,9 @@ enum rev_sort_order {\n void sort_in_topological_order(struct commit_list **, enum rev_sort_order);\n \n struct commit_graft {\n-\tunsigned char sha1[20];\n+\tstruct object_id oid;\n \tint nr_parent; /* < 0 if shallow commit */\n-\tunsigned char parent[FLEX_ARRAY][20]; /* more */\n+\tstruct object_id parent[FLEX_ARRAY]; /* more */\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 7f0890e..f6b7801 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -137,7 +137,7 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in\n \n static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n {\n-\tstruct commit *commit = lookup_commit(graft->sha1);\n+\tstruct commit *commit = lookup_commit(graft->oid.sha1);\n \tif (!commit)\n \t\treturn 0;\n \tadd_name_decoration(DECORATION_GRAFTED, \"grafted\", &commit->object);\ndiff --git a/send-pack.c b/send-pack.c\nindex 9d2b0c5..e8389a4 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -182,7 +182,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c\n {\n \tstruct strbuf *sb = cb;\n \tif (graft->nr_parent == -1)\n-\t\tpacket_buf_write(sb, \"shallow %s\\n\", sha1_to_hex(graft->sha1));\n+\t\tpacket_buf_write(sb, \"shallow %s\\n\", sha1_to_hex(graft->oid.sha1));\n \treturn 0;\n }\n \ndiff --git a/shallow.c b/shallow.c\nindex d8bf40a..9ff2d4c 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -31,7 +31,7 @@ int register_shallow(const unsigned char *sha1)\n \t\txmalloc(sizeof(struct commit_graft));\n \tstruct commit *commit = lookup_commit(sha1);\n \n-\thashcpy(graft->sha1, sha1);\n+\thashcpy(graft->oid.sha1, sha1);\n \tgraft->nr_parent = -1;\n \tif (commit && commit->object.parsed)\n \t\tcommit->parents = NULL;\n@@ -159,11 +159,11 @@ struct write_shallow_data {\n static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n {\n \tstruct write_shallow_data *data = cb_data;\n-\tconst char *hex = sha1_to_hex(graft->sha1);\n+\tconst char *hex = oid_to_hex(&graft->oid);\n \tif (graft->nr_parent != -1)\n \t\treturn 0;\n \tif (data->flags & SEEN_ONLY) {\n-\t\tstruct commit *c = lookup_commit(graft->sha1);\n+\t\tstruct commit *c = lookup_commit(graft->oid.sha1);\n \t\tif (!c || !(c->object.flags & SEEN)) {\n \t\t\tif (data->flags & VERBOSE)\n \t\t\t\tprintf(\"Removing %s from .git/shallow\\n\",\n@@ -282,7 +282,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c\n {\n \tint fd = *(int *)cb;\n \tif (graft->nr_parent == -1)\n-\t\tpacket_write(fd, \"shallow %s\\n\", sha1_to_hex(graft->sha1));\n+\t\tpacket_write(fd, \"shallow %s\\n\", oid_to_hex(&graft->oid));\n \treturn 0;\n }\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex b531a32..0566ce0 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -74,7 +74,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n {\n \tFILE *fp = cb_data;\n \tif (graft->nr_parent == -1)\n-\t\tfprintf(fp, \"--shallow %s\\n\", sha1_to_hex(graft->sha1));\n+\t\tfprintf(fp, \"--shallow %s\\n\", oid_to_hex(&graft->oid));\n \treturn 0;\n }\n \n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257250","messageId":"1425770645-628957-10-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 09/10] patch-id: convert to use struct object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:24:04Z","receivedAt":"2015-03-07T23:24:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Signed-off-by: brian m. carlson \n---\n builtin/patch-id.c | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 77db873..c208e7e 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -1,14 +1,14 @@\n #include \"builtin.h\"\n \n-static void flush_current_id(int patchlen, unsigned char *id, unsigned char *result)\n+static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n \tchar name[50];\n \n \tif (!patchlen)\n \t\treturn;\n \n-\tmemcpy(name, sha1_to_hex(id), 41);\n-\tprintf(\"%s %s\\n\", sha1_to_hex(result), name);\n+\tmemcpy(name, oid_to_hex(id), GIT_SHA1_HEXSZ + 1);\n+\tprintf(\"%s %s\\n\", oid_to_hex(result), name);\n }\n \n static int remove_space(char *line)\n@@ -53,23 +53,23 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n-static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)\n+static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)\n {\n-\tunsigned char hash[20];\n+\tunsigned char hash[GIT_SHA1_RAWSZ];\n \tunsigned short carry = 0;\n \tint i;\n \n \tgit_SHA1_Final(hash, ctx);\n \tgit_SHA1_Init(ctx);\n \t/* 20-byte sum, with carry */\n-\tfor (i = 0; i < 20; ++i) {\n-\t\tcarry += result[i] + hash[i];\n-\t\tresult[i] = carry;\n+\tfor (i = 0; i < GIT_SHA1_RAWSZ; ++i) {\n+\t\tcarry += result->sha1[i] + hash[i];\n+\t\tresult->sha1[i] = carry;\n \t\tcarry >>= 8;\n \t}\n }\n \n-static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n+static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t struct strbuf *line_buf, int stable)\n {\n \tint patchlen = 0, found_next = 0;\n@@ -77,7 +77,7 @@ static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n \tgit_SHA_CTX ctx;\n \n \tgit_SHA1_Init(&ctx);\n-\thashclr(result);\n+\toidclr(result);\n \n \twhile (strbuf_getwholeline(line_buf, stdin, '\\n') != EOF) {\n \t\tchar *line = line_buf->buf;\n@@ -93,7 +93,7 @@ static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n \t\telse if (!memcmp(line, \"\\\\ \", 2) && 12 < strlen(line))\n \t\t\tcontinue;\n \n-\t\tif (!get_sha1_hex(p, next_sha1)) {\n+\t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n \t\t\tbreak;\n \t\t}\n@@ -143,7 +143,7 @@ static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n \t}\n \n \tif (!found_next)\n-\t\thashclr(next_sha1);\n+\t\toidclr(next_oid);\n \n \tflush_one_hunk(result, &ctx);\n \n@@ -152,15 +152,15 @@ static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n \n static void generate_id_list(int stable)\n {\n-\tunsigned char sha1[20], n[20], result[20];\n+\tstruct object_id oid, n, result;\n \tint patchlen;\n \tstruct strbuf line_buf = STRBUF_INIT;\n \n-\thashclr(sha1);\n+\toidclr(&oid);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(n, result, &line_buf, stable);\n-\t\tflush_current_id(patchlen, sha1, result);\n-\t\thashcpy(sha1, n);\n+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable);\n+\t\tflush_current_id(patchlen, &oid, &result);\n+\t\toidcpy(&oid, &n);\n \t}\n \tstrbuf_release(&line_buf);\n }\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257247","messageId":"1425770645-628957-11-git-send-email-sandals@crustytoothpaste.net","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 10/10] apply: convert threeway_stage to object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-07T23:24:05Z","receivedAt":"2015-03-07T23:24:05Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Signed-off-by: brian m. carlson \n---\n builtin/apply.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 65b97ee..75c5342 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -208,7 +208,7 @@ struct patch {\n \tstruct patch *next;\n \n \t/* three-way fallback result */\n-\tunsigned char threeway_stage[3][20];\n+\tstruct object_id threeway_stage[3];\n };\n \n static void free_fragment_list(struct fragment *list)\n@@ -3424,11 +3424,11 @@ static int try_threeway(struct image *image, struct patch *patch,\n \tif (status) {\n \t\tpatch->conflicted_threeway = 1;\n \t\tif (patch->is_new)\n-\t\t\thashclr(patch->threeway_stage[0]);\n+\t\t\thashclr(patch->threeway_stage[0].sha1);\n \t\telse\n-\t\t\thashcpy(patch->threeway_stage[0], pre_sha1);\n-\t\thashcpy(patch->threeway_stage[1], our_sha1);\n-\t\thashcpy(patch->threeway_stage[2], post_sha1);\n+\t\t\thashcpy(patch->threeway_stage[0].sha1, pre_sha1);\n+\t\thashcpy(patch->threeway_stage[1].sha1, our_sha1);\n+\t\thashcpy(patch->threeway_stage[2].sha1, post_sha1);\n \t\tfprintf(stderr, \"Applied patch to '%s' with conflicts.\\n\", patch->new_name);\n \t} else {\n \t\tfprintf(stderr, \"Applied patch to '%s' cleanly.\\n\", patch->new_name);\n@@ -4184,14 +4184,14 @@ static void add_conflicted_stages_file(struct patch *patch)\n \n \tremove_file_from_cache(patch->new_name);\n \tfor (stage = 1; stage < 4; stage++) {\n-\t\tif (is_null_sha1(patch->threeway_stage[stage - 1]))\n+\t\tif (is_null_oid(&patch->threeway_stage[stage - 1]))\n \t\t\tcontinue;\n \t\tce = xcalloc(1, ce_size);\n \t\tmemcpy(ce->name, patch->new_name, namelen);\n \t\tce->ce_mode = create_ce_mode(mode);\n \t\tce->ce_flags = create_ce_flags(stage);\n \t\tce->ce_namelen = namelen;\n-\t\thashcpy(ce->sha1, patch->threeway_stage[stage - 1]);\n+\t\thashcpy(ce->sha1, patch->threeway_stage[stage - 1].sha1);\n \t\tif (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)\n \t\t\tdie(_(\"unable to add cache entry for %s\"), patch->new_name);\n \t}\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"257270","messageId":"xmqqa8zoszmj.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 00/10] Use a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T07:43:32Z","receivedAt":"2015-03-08T07:43:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Certain parts of the code have to be converted before others to keep the\n> patch sizes small, maintainable, and bisectable, so functions and\n> structures that are used across the codebase (e.g. struct object) will\n> be converted later. Conversion has been done in a somewhat haphazard\n> manner by converting modules with leaf functions and less commonly used\n> structs first.\n\nThat \"leaf-first\" approach sounds very sensible.\n\nIn the medium term, I wonder if the changes can progress faster and\nin a less error prone way if you first used GIT_SHA1_RAWSZ in places\nthat cannot be immediately converted to the struct yet. That way,\nwe will be easily tell by \"git grep GIT_SHA1_RAWSZ\" how many more\nplaces need treatment. I do not know if that is all that useful\noffhand, though. Those places need to be touched in the second pass\nto use the struct again, after the \"s/\\[20\\]/[GIT_SHA1_RAWSZ]/\"\nfirst pass.\n"},{"id":"257285","messageId":"CACsJy8DBpRPq34=JwJJEOp=DoCP_dkRQB60xPCyqoBFjui416A@mail.gmail.com","threadId":"38745","inReplyTo":"1425770645-628957-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 02/10] Define utility functions for object IDs.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-08T09:57:36Z","receivedAt":"2015-03-08T09:57:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 8, 2015 at 6:23 AM, brian m. carlson\n wrote:\n> There are several utility functions (hashcmp and friends) that are used\n> for comparing object IDs (SHA-1 values). Using these functions, which\n> take pointers to unsigned char, with struct object_id requires tiresome\n> access to the sha1 member, which bloats code and violates the desired\n> encapsulation. Provide wrappers around these functions for struct\n> object_id for neater, more maintainable code. Use the new constants to\n> avoid the hard-coded 20s and 40s throughout the original functions.\n>\n> These functions simply call the underlying pointer-to-unsigned-char\n> versions to ensure that any performance improvements will be passed\n> through to the new functions.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> I'm not very excited about having to put the #include in the middle of\n> cache.h. The alternative, of course, is to move enum object_type up,\n> which I can do if necessary. Another alternative is to simply move the\n> struct object_id definitions to cache.h instead of object.h, which might\n> be cleaner.\n>\n> I'm interested in hearing opinions about the best way to go forward.\n\nI think declaring struct object_id in cache.h would be simplest.\nAnother alternative is do it in git-compat-util.h. This is not the\nfirst abuse of git-compat-util.h (because it's included everywhere).\nstarts_with(), ends_with() and friends are already in\ngit-compat-util.h\n-- \nDuy\n"},{"id":"257323","messageId":"20150308144816.GB4245@vauxhall.crustytoothpaste.net","threadId":"38745","inReplyTo":"CACsJy8DBpRPq34=JwJJEOp=DoCP_dkRQB60xPCyqoBFjui416A@mail.gmail.com","subject":"Re: [PATCH v2 02/10] Define utility functions for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-08T14:48:16Z","receivedAt":"2015-03-08T14:48:16Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Mar 08, 2015 at 04:57:36PM +0700, Duy Nguyen wrote:\n>On Sun, Mar 8, 2015 at 6:23 AM, brian m. carlson\n> wrote:\n>> I'm not very excited about having to put the #include in the middle of\n>> cache.h. The alternative, of course, is to move enum object_type up,\n>> which I can do if necessary. Another alternative is to simply move the\n>> struct object_id definitions to cache.h instead of object.h, which might\n>> be cleaner.\n>>\n>> I'm interested in hearing opinions about the best way to go forward.\n>\n>I think declaring struct object_id in cache.h would be simplest.\n>Another alternative is do it in git-compat-util.h. This is not the\n>first abuse of git-compat-util.h (because it's included everywhere).\n>starts_with(), ends_with() and friends are already in\n>git-compat-util.h\n\ncache.h is so ubiquitous due to the hash* functions that I didn't need \nto include it anywhere, so I don't see a reason to abuse \ngit-compat-util.h for that purpose.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"257525","messageId":"82A92572-98E5-4911-87A6-DF5377579436@gmail.com","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 00/10] Use a structure for object IDs.","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-11T02:38:28Z","receivedAt":"2015-03-11T02:38:28Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Mar 7, 2015, at 15:23, brian m. carlson wrote:\n> This is a patch series to convert some of the relevant uses of \n> unsigned\n> char [20] to struct object_id.\n>\n> brian m. carlson (10):\n\nAll patches applied for me (to master) and all tests pass.\n\nTested-by: Kyle J. McKay\n\n\n> Define a structure for object IDs.\n\nComments in reply to the patch.\n\n\n> Define utility functions for object IDs.\n> bisect.c: convert leaf functions to use struct object_id\n> archive.c: convert to use struct object_id\n> zip: use GIT_SHA1_HEXSZ for trailers\n> bulk-checkin.c: convert to use struct object_id\n> diff: convert struct combine_diff_path to object_id\n> commit: convert parts to struct object_id\n> patch-id: convert to use struct object_id\n> apply: convert threeway_stage to object_id\n\nThese all look good, the conversions are simple and easy to follow.\n\n\nOn Mar 7, 2015, at 23:43, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n>\n>> Certain parts of the code have to be converted before others to \n>> keep the\n>> patch sizes small, maintainable, and bisectable, so functions and\n>> structures that are used across the codebase (e.g. struct object) \n>> will\n>> be converted later. Conversion has been done in a somewhat haphazard\n>> manner by converting modules with leaf functions and less commonly \n>> used\n>> structs first.\n>\n> That \"leaf-first\" approach sounds very sensible.\n>\n> In the medium term, I wonder if the changes can progress faster and\n> in a less error prone way if you first used GIT_SHA1_RAWSZ in places\n> that cannot be immediately converted to the struct yet. That way,\n> we will be easily tell by \"git grep GIT_SHA1_RAWSZ\" how many more\n> places need treatment. I do not know if that is all that useful\n> offhand, though. Those places need to be touched in the second pass\n> to use the struct again, after the \"s/\\[20\\]/[GIT_SHA1_RAWSZ]/\"\n> first pass.\n\nI definitely noticed the leaf-first approach as I was looking through \nthe patches where, for example (03/10), this prototype was left \nunchanged:\n\nstatic int register_ref(const char *refname, const unsigned char *sha1,\n int flags, void *cb_data)\n\nbut its contents got the update leaving it half converted. As \nmentioned above this makes the patches more manageable, maintainable \nand bisectable. However, these functions could be converted to take a \ntypedef (a quick grep of 'CodingGuidelines' does not mention typedef) \nand perhaps, as Junio mentions above, help the changes progress faster \nby making it easier to find the affected code (e.g. changing or \nremoving the typedef would make the compiler find them for you).\n\nFor example, if we added this to object.h:\n\n typedef unsigned char sha1raw_t[GIT_SHA1_RAWSZ];\n\nthen the above prototype could be immediately converted to (and this \ndoes compile and pass all the tests):\n\nstatic int register_ref(const char *refname, const sha1raw_t sha,\n int flags, void *cb_data)\n\nSo that together with Junio's suggestion above (and perhaps also a \nsha1hex_t type) would help mark everything in the first pass that \nneeds to be touched again in the second pass. (I'm just throwing out \nsome typedef names as an example, there may be more preferable names \nto \"sha1raw_t\" and \"sha1hex_t\", but those names would end up being \nreplaced eventually anyway.)\n\n-Kyle \n"},{"id":"257555","messageId":"550038C8.80601@alum.mit.edu","threadId":"38745","inReplyTo":"1425770645-628957-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 02/10] Define utility functions for object IDs.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-11T12:44:56Z","receivedAt":"2015-03-11T12:44:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/08/2015 12:23 AM, brian m. carlson wrote:\n> There are several utility functions (hashcmp and friends) that are used\n> for comparing object IDs (SHA-1 values). Using these functions, which\n> take pointers to unsigned char, with struct object_id requires tiresome\n> access to the sha1 member, which bloats code and violates the desired\n> encapsulation. Provide wrappers around these functions for struct\n> object_id for neater, more maintainable code. Use the new constants to\n> avoid the hard-coded 20s and 40s throughout the original functions.\n> \n> These functions simply call the underlying pointer-to-unsigned-char\n> versions to ensure that any performance improvements will be passed\n> through to the new functions.\n> \n> Signed-off-by: brian m. carlson \n> ---\n> I'm not very excited about having to put the #include in the middle of\n> cache.h. The alternative, of course, is to move enum object_type up,\n> which I can do if necessary. Another alternative is to simply move the\n> struct object_id definitions to cache.h instead of object.h, which might\n> be cleaner.\n> \n> I'm interested in hearing opinions about the best way to go forward.\n> \n> cache.h | 37 +++++++++++++++++++++++++++++++++----\n> hex.c | 16 +++++++++++++---\n> 2 files changed, 46 insertions(+), 7 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index 761c570..f9addcc 100644\n> --- a/cache.h\n> +++ b/cache.h\n> [...]\n> @@ -724,20 +729,42 @@ static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> \treturn 0;\n> }\n> \n> +static inline int oidcmp(const struct object_id *o1, const struct object_id *o2)\n> +{\n> +\treturn hashcmp(o1->sha1, o2->sha1);\n> +}\n> +\n\nMaybe rename o1 -> oid1 and o2 -> oid2 just to make things blindingly\nobvious?\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257557","messageId":"55004F18.4070609@alum.mit.edu","threadId":"38745","inReplyTo":"1425770645-628957-5-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 04/10] archive.c: convert to use struct object_id","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-11T14:20:08Z","receivedAt":"2015-03-11T14:20:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/08/2015 12:23 AM, brian m. carlson wrote:\n> Convert a hard-coded 20 as well.\n> \n> Signed-off-by: brian m. carlson \n> ---\n> archive.c | 18 +++++++++---------\n> 1 file changed, 9 insertions(+), 9 deletions(-)\n> \n> diff --git a/archive.c b/archive.c\n> index 96057ed..46d9025 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -101,7 +101,7 @@ static void setup_archive_check(struct git_attr_check *check)\n> \n> struct directory {\n> \tstruct directory *up;\n> -\tunsigned char sha1[20];\n> +\tunsigned char sha1[GIT_SHA1_RAWSZ];\n\nThis is a local struct, and I think it is only a three-line change to\nchange the sha1 member to\n\n struct object_id oid;\n\nThough perhaps you are delaying that change for some concrete reason.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257558","messageId":"55005553.9060704@alum.mit.edu","threadId":"38745","inReplyTo":"1425770645-628957-9-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 08/10] commit: convert parts to struct object_id","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-11T14:46:43Z","receivedAt":"2015-03-11T14:46:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/08/2015 12:24 AM, brian m. carlson wrote:\n> Convert struct commit_graft and necessary local parts of commit.c.\n> \n> Signed-off-by: brian m. carlson \n> ---\n> commit.c | 32 ++++++++++++++++----------------\n> commit.h | 4 ++--\n> log-tree.c | 2 +-\n> send-pack.c | 2 +-\n> shallow.c | 8 ++++----\n> upload-pack.c | 2 +-\n> 6 files changed, 25 insertions(+), 25 deletions(-)\n> \n> diff --git a/commit.c b/commit.c\n> index a8c7577..89c207e 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -55,12 +55,12 @@ struct commit *lookup_commit(const unsigned char *sha1)\n> \n> struct commit *lookup_commit_reference_by_name(const char *name)\n> {\n> -\tunsigned char sha1[20];\n> +\tstruct object_id oid;\n> \tstruct commit *commit;\n> \n> -\tif (get_sha1_committish(name, sha1))\n> +\tif (get_sha1_committish(name, oid.sha1))\n> \t\treturn NULL;\n> -\tcommit = lookup_commit_reference(sha1);\n> +\tcommit = lookup_commit_reference(oid.sha1);\n> \tif (parse_commit(commit))\n> \t\treturn NULL;\n> \treturn commit;\n> @@ -99,7 +99,7 @@ static int commit_graft_alloc, commit_graft_nr;\n> static const unsigned char *commit_graft_sha1_access(size_t index, void *table)\n> {\n> \tstruct commit_graft **commit_graft_table = table;\n> -\treturn commit_graft_table[index]->sha1;\n> +\treturn commit_graft_table[index]->oid.sha1;\n> }\n> \n> static int commit_graft_pos(const unsigned char *sha1)\n> @@ -110,7 +110,7 @@ static int commit_graft_pos(const unsigned char *sha1)\n> \n> int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n> {\n> -\tint pos = commit_graft_pos(graft->sha1);\n> +\tint pos = commit_graft_pos(graft->oid.sha1);\n> \n> \tif (0 <= pos) {\n> \t\tif (ignore_dups)\n> @@ -148,12 +148,12 @@ struct commit_graft *read_graft_line(char *buf, int len)\n> \ti = (len + 1) / 41 - 1;\n> \tgraft = xmalloc(sizeof(*graft) + 20 * i);\n> \tgraft->nr_parent = i;\n> -\tif (get_sha1_hex(buf, graft->sha1))\n> +\tif (get_oid_hex(buf, &graft->oid))\n> \t\tgoto bad_graft_data;\n> \tfor (i = 40; i < len; i += 41) {\n> \t\tif (buf[i] != ' ')\n> \t\t\tgoto bad_graft_data;\n> -\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/41]))\n> +\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/41].sha1))\n> \t\t\tgoto bad_graft_data;\n> \t}\n\nThere are a bunch of constants in this function related to\nGIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ.\n\n> \treturn graft;\n> @@ -302,7 +302,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> {\n> \tconst char *tail = buffer;\n> \tconst char *bufptr = buffer;\n> -\tunsigned char parent[20];\n> +\tstruct object_id parent;\n> \tstruct commit_list **pptr;\n> \tstruct commit_graft *graft;\n> \n> @@ -312,10 +312,10 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> \ttail += size;\n> \tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n> \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n> -\tif (get_sha1_hex(bufptr + 5, parent) < 0)\n> +\tif (get_sha1_hex(bufptr + 5, parent.sha1) < 0)\n> \t\treturn error(\"bad tree pointer in commit %s\",\n> \t\t\t sha1_to_hex(item->object.sha1));\n> -\titem->tree = lookup_tree(parent);\n> +\titem->tree = lookup_tree(parent.sha1);\n> \tbufptr += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n> \tpptr = &item->parents;\n> \n\nHere, too; e.g.,\n\n* \"45\" -> GIT_SHA1_HEXSZ + 5\n* \"46\" -> GIT_SHA1_HEXSZ + 5 + 1\n\nand below,\n\n* \"47\" -> GIT_SHA1_HEXSZ + 7\n* \"48\" -> GIT_SHA1_HEXSZ + 7 + 1\n\n\n> @@ -324,7 +324,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> \t\tstruct commit *new_parent;\n> \n> \t\tif (tail <= bufptr + 48 ||\n> -\t\t get_sha1_hex(bufptr + 7, parent) ||\n> +\t\t get_sha1_hex(bufptr + 7, parent.sha1) ||\n> \t\t bufptr[47] != '\\n')\n> \t\t\treturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n> \t\tbufptr += 48;\n> @@ -334,7 +334,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> \t\t */\n> \t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n> \t\t\tcontinue;\n> -\t\tnew_parent = lookup_commit(parent);\n> +\t\tnew_parent = lookup_commit(parent.sha1);\n> \t\tif (new_parent)\n> \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n> \t}\n> @@ -342,7 +342,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> \t\tint i;\n> \t\tstruct commit *new_parent;\n> \t\tfor (i = 0; i < graft->nr_parent; i++) {\n> -\t\t\tnew_parent = lookup_commit(graft->parent[i]);\n> +\t\t\tnew_parent = lookup_commit(graft->parent[i].sha1);\n> \t\t\tif (!new_parent)\n> \t\t\t\tcontinue;\n> \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n> @@ -1580,10 +1580,10 @@ struct commit *get_merge_parent(const char *name)\n> {\n> \tstruct object *obj;\n> \tstruct commit *commit;\n> -\tunsigned char sha1[20];\n> -\tif (get_sha1(name, sha1))\n> +\tstruct object_id oid;\n> +\tif (get_sha1(name, oid.sha1))\n> \t\treturn NULL;\n> -\tobj = parse_object(sha1);\n> +\tobj = parse_object(oid.sha1);\n> \tcommit = (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n> \tif (commit && !commit->util) {\n> \t\tstruct merge_remote_desc *desc;\n> diff --git a/commit.h b/commit.h\n> index 9f189cb..ed3a1d5 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -226,9 +226,9 @@ enum rev_sort_order {\n> void sort_in_topological_order(struct commit_list **, enum rev_sort_order);\n> \n> struct commit_graft {\n> -\tunsigned char sha1[20];\n> +\tstruct object_id oid;\n> \tint nr_parent; /* < 0 if shallow commit */\n> -\tunsigned char parent[FLEX_ARRAY][20]; /* more */\n> +\tstruct object_id parent[FLEX_ARRAY]; /* more */\n> };\n> typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n> \n> diff --git a/log-tree.c b/log-tree.c\n> index 7f0890e..f6b7801 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -137,7 +137,7 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in\n> \n> static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n> {\n> -\tstruct commit *commit = lookup_commit(graft->sha1);\n> +\tstruct commit *commit = lookup_commit(graft->oid.sha1);\n> \tif (!commit)\n> \t\treturn 0;\n> \tadd_name_decoration(DECORATION_GRAFTED, \"grafted\", &commit->object);\n> diff --git a/send-pack.c b/send-pack.c\n> index 9d2b0c5..e8389a4 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -182,7 +182,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c\n> {\n> \tstruct strbuf *sb = cb;\n> \tif (graft->nr_parent == -1)\n> -\t\tpacket_buf_write(sb, \"shallow %s\\n\", sha1_to_hex(graft->sha1));\n> +\t\tpacket_buf_write(sb, \"shallow %s\\n\", sha1_to_hex(graft->oid.sha1));\n> \treturn 0;\n> }\n> \n> diff --git a/shallow.c b/shallow.c\n> index d8bf40a..9ff2d4c 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -31,7 +31,7 @@ int register_shallow(const unsigned char *sha1)\n> \t\txmalloc(sizeof(struct commit_graft));\n> \tstruct commit *commit = lookup_commit(sha1);\n> \n> -\thashcpy(graft->sha1, sha1);\n> +\thashcpy(graft->oid.sha1, sha1);\n> \tgraft->nr_parent = -1;\n> \tif (commit && commit->object.parsed)\n> \t\tcommit->parents = NULL;\n> @@ -159,11 +159,11 @@ struct write_shallow_data {\n> static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> {\n> \tstruct write_shallow_data *data = cb_data;\n> -\tconst char *hex = sha1_to_hex(graft->sha1);\n> +\tconst char *hex = oid_to_hex(&graft->oid);\n> \tif (graft->nr_parent != -1)\n> \t\treturn 0;\n> \tif (data->flags & SEEN_ONLY) {\n> -\t\tstruct commit *c = lookup_commit(graft->sha1);\n> +\t\tstruct commit *c = lookup_commit(graft->oid.sha1);\n> \t\tif (!c || !(c->object.flags & SEEN)) {\n> \t\t\tif (data->flags & VERBOSE)\n> \t\t\t\tprintf(\"Removing %s from .git/shallow\\n\",\n> @@ -282,7 +282,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c\n> {\n> \tint fd = *(int *)cb;\n> \tif (graft->nr_parent == -1)\n> -\t\tpacket_write(fd, \"shallow %s\\n\", sha1_to_hex(graft->sha1));\n> +\t\tpacket_write(fd, \"shallow %s\\n\", oid_to_hex(&graft->oid));\n> \treturn 0;\n> }\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index b531a32..0566ce0 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -74,7 +74,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> {\n> \tFILE *fp = cb_data;\n> \tif (graft->nr_parent == -1)\n> -\t\tfprintf(fp, \"--shallow %s\\n\", sha1_to_hex(graft->sha1));\n> +\t\tfprintf(fp, \"--shallow %s\\n\", oid_to_hex(&graft->oid));\n> \treturn 0;\n> }\n> \n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257559","messageId":"5500689A.5090101@alum.mit.edu","threadId":"38745","inReplyTo":"1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 00/10] Use a structure for object IDs.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-11T16:08:58Z","receivedAt":"2015-03-11T16:08:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/08/2015 12:23 AM, brian m. carlson wrote:\n> This is a patch series to convert some of the relevant uses of unsigned\n> char [20] to struct object_id.\n> \n> The goal of this series to improve type-checking in the codebase and to\n> make it easier to move to a different hash function if the project\n> decides to do that. This series does not convert all of the codebase,\n> but only parts. I've dropped some of the patches from earlier (which no\n> longer apply) and added others.\n> \n> Certain parts of the code have to be converted before others to keep the\n> patch sizes small, maintainable, and bisectable, so functions and\n> structures that are used across the codebase (e.g. struct object) will\n> be converted later. Conversion has been done in a somewhat haphazard\n> manner by converting modules with leaf functions and less commonly used\n> structs first.\n> \n> Since part of the goal is to ease a move to a different hash function if\n> the project decides to do that, I've adopted Michael Haggerty's\n> suggestion of using variables named \"oid\", naming the structure member\n> \"sha1\", and using GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ as the constants.\n> \n> I've been holding on to this series for a long time in hopes of\n> converting more of the code before submitting, but realized that this\n> deprived others of the ability to use the new structure and required me\n> to rebase extremely frequently.\n> [...]\n\nI've added CC to several people who commented on v1.\n\nI think this is a really interesting project and I hope that it works out.\n\nIn my opinion, the biggest risk (aside from the sheer amount of work\nrequired) is the issue that was brought up on the mailing list when you\nsubmitted v1 [1]: Converting an arbitrary (unsigned char *) to a (struct\nobject_id *) is not allowed, because the alignment requirements of the\nlatter might be stricter than those of the former. This means that if,\nfor example, we are reading some data from disk or from the network, and\nwe expect the 20 bytes starting with byte number 17 to be a SHA-1 in\nbinary format, we used to be able to do\n\n unsigned char *sha1 = buf + 17;\n\nand use sha1 like any other SHA-1, without the need for any copying. But\nwe can't do the analogous\n\n struct object_id *oid = (struct object_id *)(buf + 17);\n\nbecause the alignment is not necessarily correct. So in a pure \"struct\nobject_id\" world, I think we would be forced to change such code to\n\n struct object_id oid;\n hashcpy(oid.sha1, buf + 17);\n\nThis uses additional memory and introduces copying overhead. Also, the\nlifetime of oid might exceed the scope of a function, so oid might have\nto be allocated on the heap and later freed. This adds more\ncomputational overhead, more memory overhead, and more programming\neffort to get it all right.\n\nSo as much as I like the idea of wrapping SHA-1s in objects, I think it\nwould be a good idea to first make sure that we can all agree on a plan\nfor dealing with situations like this. I can think of the following\npossibilities:\n\n1. Maybe code that needs to handle SHA-1s with screwy alignment does not\nexist, or maybe it does the copying anyway. I haven't actually checked.\n\n2. Maybe somebody can prove that\n\n struct object_id *oid = (struct object_id *)(buf + 17);\n\n somehow *is* allowed by the C standard, or at least that it is\nsufficiently portable for our purposes.\n\n3. We accept the additional runtime costs and development effort for the\nextra copies. To accept this approach, we would need some idea of which\nareas of the code will be affected, and some estimate of how much\nadditional memory it would cost.\n\n4. We continue to support working with SHA-1s declared to be (unsigned\nchar *) in some performance-critical code, even as we migrate most other\ncode to using SHA-1s embedded within a (struct object_id). This will\ncost some duplication of code. To accept this approach, we would need an\nidea of *how much* code duplication would be needed. E.g., how many\nfunctions will need both (unsigned char *) versions and (struct\nobject_id *) versions?\n\n5. We only make the change opportunistically. Whenever we find a\nfunction that needs to work with non-struct-aligned SHA-1s, we leave the\nfunction as-is rather than converting it or creating a second version\nthat works with object_id objects. This approach would leave the\ncodebase somewhat schizophrenic.\n\nI'm not trying to dissuade you from this project, but I think that for\nyour project to have a chance of success, we need an answer to this\nquestion. So let's confront it now rather than after you have invested\nlots more time and/or gotten patches merged.\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/248054\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257568","messageId":"xmqqk2yn5l39.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"5500689A.5090101@alum.mit.edu","subject":"Re: [PATCH v2 00/10] Use a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T20:35:06Z","receivedAt":"2015-03-11T20:35:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I think this is a really interesting project and I hope that it works out.\n\nCount me in ;-)\n\n> In my opinion, the biggest risk (aside from the sheer amount of work\n> required) is the issue that was brought up on the mailing list when you\n> submitted v1 [1]: Converting an arbitrary (unsigned char *) to a (struct\n> object_id *) is not allowed, because the alignment requirements of the\n> latter might be stricter than those of the former. This means that if,\n> for example, we are reading some data from disk or from the network, and\n> we expect the 20 bytes starting with byte number 17 to be a SHA-1 in\n> binary format, we used to be able to do\n>\n> unsigned char *sha1 = buf + 17;\n>\n> and use sha1 like any other SHA-1, without the need for any copying. But\n> we can't do the analogous\n>\n> struct object_id *oid = (struct object_id *)(buf + 17);\n>\n> because the alignment is not necessarily correct. So in a pure \"struct\n> object_id\" world, I think we would be forced to change such code to\n>\n> struct object_id oid;\n> hashcpy(oid.sha1, buf + 17);\n>\n> This uses additional memory and introduces copying overhead. Also, the\n> lifetime of oid might exceed the scope of a function, so oid might have\n> to be allocated on the heap and later freed. This adds more\n> computational overhead, more memory overhead, and more programming\n> effort to get it all right.\n\nBecause we use abstraction to reduce burden on programmers, the last\npoint is really what defeats this approach.\n\nI wonder if there is any way to make the new \"oid wrapped in a\nstruct\" result in identical binary---that would be a reasonable\ncriterion to judge the goodness of a change line this. Any\ndifference could be (1) compiler being (a) stupid and not taking\noptimization opportunity it notices for a bare byte array but not\nfor a byte array in struct or (b) clever and taking optimization\nopportunity the other way around, or (2) breakage in the conversion\ncausing new bugs, perhaps coming from the \"alignment\" worries you\ncited above. (1-a) may or may not be an acceptable price to pay for\na cleaner abstraction, but that depends on the extent of damage.\n(1-b) may be unlikely but if there is such a gain, that's nice ;-).\nAnd (2) is obviously a show-stopper X-<.\n\n> So as much as I like the idea of wrapping SHA-1s in objects, I think it\n> would be a good idea to first make sure that we can all agree on a plan\n> for dealing with situations like this. I can think of the following\n> possibilities:\n\nThese all look sensible to me.\n\n> 4. We continue to support working with SHA-1s declared to be (unsigned\n> char *) in some performance-critical code, even as we migrate most other\n> code to using SHA-1s embedded within a (struct object_id). This will\n> cost some duplication of code. To accept this approach, we would need an\n> idea of *how much* code duplication would be needed. E.g., how many\n> functions will need both (unsigned char *) versions and (struct\n> object_id *) versions?\n\nIdeally, only the ones that knows the underlying hash function is\nSHA-1 (i.e. anybody who mentions git_SHA_CTX), moves bytes from/to\nin-core object name field and raw range of bytes (e.g. sha1hash());\neverybody else like hashcpy() and hashcmp() should be able to do its\nthing only on structs and a generic-looking constant that denotes\nhow many bytes there are in the hash (or even sizeof(struct oid)).\n\nI do not know what kind of code duplication you are worried about,\nthough. If a callee needs \"unsigned char *\", the caller that has a\n\"struct object_id *o\" should pass o->hash to the callee. We should\nnot add another variant of the same callee that takes a pointer to\n\"struct object_id\"---I think that leads to insanty, e.g.\n\n int hashcmp_o_o(struct object_id *, struct object_id *);\n int hashcmp_o_b(struct object_id *, unsigned char *);\n int hashcmp_b_o(unsigned char *, struct object_id *);\n int hashcmp_b_b(unsigned char *, unsigned char *);\n\nAnd please do not suggest switching to C++; all it would do to\noverload these into a single name is to make the callers harder to\nread.\n"},{"id":"257574","messageId":"20150311220825.GB46326@vauxhall.crustytoothpaste.net","threadId":"38745","inReplyTo":"CEA07500-9F47-4B24-AD5D-1423A601A4DD@gmail.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-11T22:08:25Z","receivedAt":"2015-03-11T22:08:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Mar 10, 2015 at 07:38:42PM -0700, Kyle J. McKay wrote:\n>GIT_SHA1_HEXSZ will always be exactly 2 * GIT_SHA1_RAWSZ, right? In\n>fact, if it's not things will almost certainly break, yes?\n>\n>Does it make more sense then to reflect this requirement by using:\n>\n> #define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)\n>\n>instead?\n\nYes. I'll make that change in the next version.\n\n>I don't see anything wrong with this. However, in part 02/10 the\n>utility functions all use \"oid\" in their names, so I'm thinking that\n>it may make more sense to just go with:\n>\n>struct object_id {\n>\tunsigned char oid[GIT_SHA1_RAWSZ];\n>};\n>\n>to match?\n\nMichael Haggerty recommended that I call the structure element sha1\ninstead of oid in case we want to turn this into a union if we decide to\ngo the additional hash route.\n\nI think it can also improve readability if we use \"oid\" only for the\ninstances of the struct itself, especially since it makes it more\nobvious what code has been converted already.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"257575","messageId":"20150311221229.GC46326@vauxhall.crustytoothpaste.net","threadId":"38745","inReplyTo":"55004F18.4070609@alum.mit.edu","subject":"Re: [PATCH v2 04/10] archive.c: convert to use struct object_id","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-11T22:12:30Z","receivedAt":"2015-03-11T22:12:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Mar 11, 2015 at 03:20:08PM +0100, Michael Haggerty wrote:\n>On 03/08/2015 12:23 AM, brian m. carlson wrote:\n>> struct directory {\n>> \tstruct directory *up;\n>> -\tunsigned char sha1[20];\n>> +\tunsigned char sha1[GIT_SHA1_RAWSZ];\n>\n>This is a local struct, and I think it is only a three-line change to\n>change the sha1 member to\n>\n> struct object_id oid;\n>\n>Though perhaps you are delaying that change for some concrete reason.\n\nNo, I just overlooked it. I'll fix that in the re-roll.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"257577","messageId":"xmqqr3sv3vsf.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"20150311220825.GB46326@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-12T00:26:56Z","receivedAt":"2015-03-12T00:26:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Michael Haggerty recommended that I call the structure element sha1\n> instead of oid in case we want to turn this into a union if we decide to\n> go the additional hash route.\n\nI'd advise against it.\n\nAs I wrote in $gmane/265337 in response to Michael:\n\n > 4. We continue to support working with SHA-1s declared to be (unsigned\n > char *) in some performance-critical code, even as we migrate most other\n > code to using SHA-1s embedded within a (struct object_id). This will\n > cost some duplication of code. To accept this approach, we would need an\n > idea of *how much* code duplication would be needed. E.g., how many\n > functions will need both (unsigned char *) versions and (struct\n > object_id *) versions?\n\n ...\n\n I do not know what kind of code duplication you are worried about,\n though. If a callee needs \"unsigned char *\", the caller that has a\n \"struct object_id *o\" should pass o->hash to the callee.\n\nAnd that would break the abstraction effort if you start calling the\nfield with a name that is specific to the underlying hash function.\nThe caller has to change o->sha1 to o->sha256 instead of keeping\nthat as o->oid and letting the callee handle the implementation\ndetails when calling\n\n if (!hashcmp(o1->oid, o2->oid))\n ; /* they are the same */\n else\n ; /* they are different */\n\nThe only folks that need to _know_ what hash function is used or\nhow long the field is are the ones that have raw bytes of the hash\nobtained from files (e.g. from the index) and they would do\nsomething like this to implement a function that checks the file\nrecords an object name that is expected by the caller:\n\n void check_oid(int fd, struct object_id *expected)\n {\n unsigned char object_name[GIT_HASH_RAWSZ];\n\n ...\n read(fd, object_name, GIT_HASH_RAWSZ);\n if (hashcmp(object_name, expected->oid))\n die(\"object name mismatch???\");\n }\n\nor when they do know they are using a specific function, do:\n\n void compute_object_name(struct object_id *id,\n unsignd char*data,\n size_t len)\n {\n git_SHA_CTX c;\n unsigned char *sha1 = &(id->oid);\n\n /* if we are paranoid... */\n assert(sizeof(id->oid) >= 20);\n\n ...\n git_SHA1_Init(&c);\n git_SHA1_Update(&c, data, len);\n ...\n git_SHA1_Final(sha1, &c);\n }\n\nEven the latter would not be helped if the field to store the hash\nwere named id->sha1 very much, I would think.\n"},{"id":"257586","messageId":"20150312093404.GE46326@vauxhall.crustytoothpaste.net","threadId":"38745","inReplyTo":"xmqqr3sv3vsf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-12T09:34:05Z","receivedAt":"2015-03-12T09:34:05Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Mar 11, 2015 at 05:26:56PM -0700, Junio C Hamano wrote:\n>\"brian m. carlson\" writes:\n>\n>> Michael Haggerty recommended that I call the structure element sha1\n>> instead of oid in case we want to turn this into a union if we decide to\n>> go the additional hash route.\n>\n>I'd advise against it.\n\nYeah, after re-reading this, I think \"hash\" is the best choice for the\nunderlying element name. That way, it's different from \"oid\", which\nwould be the instances of the struct itself, and it's sufficiently\ndifferent from unconverted places in the code, which don't typically use\nthat term.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"257588","messageId":"55016A3A.6010100@alum.mit.edu","threadId":"38745","inReplyTo":"xmqqr3sv3vsf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-12T10:28:10Z","receivedAt":"2015-03-12T10:28:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/12/2015 01:26 AM, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n>> Michael Haggerty recommended that I call the structure element sha1\n>> instead of oid in case we want to turn this into a union if we decide to\n>> go the additional hash route.\n> \n> I'd advise against it.\n> \n> As I wrote in $gmane/265337 in response to Michael:\n> \n> > 4. We continue to support working with SHA-1s declared to be (unsigned\n> > char *) in some performance-critical code, even as we migrate most other\n> > code to using SHA-1s embedded within a (struct object_id). This will\n> > cost some duplication of code. To accept this approach, we would need an\n> > idea of *how much* code duplication would be needed. E.g., how many\n> > functions will need both (unsigned char *) versions and (struct\n> > object_id *) versions?\n> \n> ...\n> \n> I do not know what kind of code duplication you are worried about,\n> though. If a callee needs \"unsigned char *\", the caller that has a\n> \"struct object_id *o\" should pass o->hash to the callee.\n> \n> And that would break the abstraction effort if you start calling the\n> field with a name that is specific to the underlying hash function.\n> The caller has to change o->sha1 to o->sha256 instead of keeping\n> that as o->oid and letting the callee handle the implementation\n> details when calling\n> \n> if (!hashcmp(o1->oid, o2->oid))\n> ; /* they are the same */\n> else\n> ; /* they are different */\n> [...]\n\nHmm, I guess you imagine that we might sometimes pack SHA-1s, sometimes\nSHA-256s (or whatever) in the \"oid\" field, which would be dimensioned\nlarge enough for either one (with, say, SHA-1s padded with zeros).\n\nI was imagining that this would evolve into a union (or maybe struct) of\ndifferent hash types, like\n\n struct object_id {\n unsigned char hash_type;\n union {\n unsigned char sha1[GIT_SHA1_RAWSZ];\n unsigned char sha256[GIT_SHA256_RAWSZ];\n } hash;\n };\n\nBTW in either case, any hopes of mapping object_id objects directly on\ntop of buffer memory would disappear.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257589","messageId":"20150312104651.GF46326@vauxhall.crustytoothpaste.net","threadId":"38745","inReplyTo":"55016A3A.6010100@alum.mit.edu","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-12T10:46:52Z","receivedAt":"2015-03-12T10:46:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Mar 12, 2015 at 11:28:10AM +0100, Michael Haggerty wrote:\n>On 03/12/2015 01:26 AM, Junio C Hamano wrote:\n>> And that would break the abstraction effort if you start calling the\n>> field with a name that is specific to the underlying hash function.\n>> The caller has to change o->sha1 to o->sha256 instead of keeping\n>> that as o->oid and letting the callee handle the implementation\n>> details when calling\n>>\n>> if (!hashcmp(o1->oid, o2->oid))\n>> ; /* they are the same */\n>> else\n>> ; /* they are different */\n>> [...]\n>\n>Hmm, I guess you imagine that we might sometimes pack SHA-1s, sometimes\n>SHA-256s (or whatever) in the \"oid\" field, which would be dimensioned\n>large enough for either one (with, say, SHA-1s padded with zeros).\n>\n>I was imagining that this would evolve into a union (or maybe struct) of\n>different hash types, like\n>\n> struct object_id {\n> unsigned char hash_type;\n> union {\n> unsigned char sha1[GIT_SHA1_RAWSZ];\n> unsigned char sha256[GIT_SHA256_RAWSZ];\n> } hash;\n> };\n>\n>BTW in either case, any hopes of mapping object_id objects directly on\n>top of buffer memory would disappear.\n\nWhat I think might be more beneficial is to make the hash function\nspecific to a particular repository, and therefore you could maintain\nsomething like this:\n\n struct object_id {\n unsigned char hash[GIT_MAX_RAWSZ];\n };\n\nand make hash_type (or hash_length) a global[0]. I don't think it's\nvery worthwhile to try to mix two different hash functions in the same\nrepository, so we could still map directly onto buffer memory if we\ndecide that's portable enough. I expect the cases where we need to do\nthat will be relatively limited.\n\nRegardless, it seems that this solution has the most support (including\nJunio's) and it's more self-documenting than my current set of patches,\nso I'm going to go with it for now. It should be easy to change if the\nconsensus goes back the other way.\n\n[0] I personally think globals are a bit gross, but they don't seem to\nhave the problems that they would if git were a shared library.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"257590","messageId":"CACsJy8AL0jGwzTfWDSszkvyqzNtPA7o7vQRT5NFaBYrVU02nOA@mail.gmail.com","threadId":"38745","inReplyTo":"20150312104651.GF46326@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-12T11:16:49Z","receivedAt":"2015-03-12T11:16:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 12, 2015 at 5:46 PM, brian m. carlson\n wrote:\n> On Thu, Mar 12, 2015 at 11:28:10AM +0100, Michael Haggerty wrote:\n>>\n>> On 03/12/2015 01:26 AM, Junio C Hamano wrote:\n>>>\n>>> And that would break the abstraction effort if you start calling the\n>>> field with a name that is specific to the underlying hash function.\n>>> The caller has to change o->sha1 to o->sha256 instead of keeping\n>>> that as o->oid and letting the callee handle the implementation\n>>> details when calling\n>>>\n>>> if (!hashcmp(o1->oid, o2->oid))\n>>> ; /* they are the same */\n>>> else\n>>> ; /* they are different */\n>>> [...]\n>>\n>>\n>> Hmm, I guess you imagine that we might sometimes pack SHA-1s, sometimes\n>> SHA-256s (or whatever) in the \"oid\" field, which would be dimensioned\n>> large enough for either one (with, say, SHA-1s padded with zeros).\n>>\n>> I was imagining that this would evolve into a union (or maybe struct) of\n>> different hash types, like\n>>\n>> struct object_id {\n>> unsigned char hash_type;\n>> union {\n>> unsigned char sha1[GIT_SHA1_RAWSZ];\n>> unsigned char sha256[GIT_SHA256_RAWSZ];\n>> } hash;\n>> };\n>>\n>> BTW in either case, any hopes of mapping object_id objects directly on\n>> top of buffer memory would disappear.\n>\n>\n> What I think might be more beneficial is to make the hash function\n> specific to a particular repository, and therefore you could maintain\n> something like this:\n>\n> struct object_id {\n> unsigned char hash[GIT_MAX_RAWSZ];\n> };\n>\n> and make hash_type (or hash_length) a global[0]. I don't think it's\n> very worthwhile to try to mix two different hash functions in the same\n> repository,\n\nThis may or may not fall into the \"mix different hash functions\"\ncategory. In pack files version 4, trees are encoded to point to other\ntrees or blobs by a (pack, offset) tuple. It would be great if the new\nobject_id could support carrying this kind of object id around because\nit could help reduce object lookup cost a lot. (pack, offset) can be\nconverted back to SHA-1 so no info is lost and hashcmp() can compare\n(pack, tuple) against an SHA-1 just fine.\n\n> so we could still map directly onto buffer memory if we\n> decide that's portable enough. I expect the cases where we need to do\n> that will be relatively limited.\n>\n> Regardless, it seems that this solution has the most support (including\n> Junio's) and it's more self-documenting than my current set of patches,\n> so I'm going to go with it for now. It should be easy to change if the\n> consensus goes back the other way.\n>\n> [0] I personally think globals are a bit gross, but they don't seem to\n> have the problems that they would if git were a shared library.\n>\n> --\n> brian m. carlson / brian with sandals: Houston, Texas, US\n> +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\n> OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n\n\n\n-- \nDuy\n"},{"id":"257595","messageId":"xmqqwq2m2hwf.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"CACsJy8AL0jGwzTfWDSszkvyqzNtPA7o7vQRT5NFaBYrVU02nOA@mail.gmail.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-12T18:24:32Z","receivedAt":"2015-03-12T18:24:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> This may or may not fall into the \"mix different hash functions\"\n> category. In pack files version 4, trees are encoded to point to other\n> trees or blobs by a (pack, offset) tuple. It would be great if the new\n> object_id could support carrying this kind of object id around because\n> it could help reduce object lookup cost a lot. (pack, offset) can be\n> converted back to SHA-1 so no info is lost and hashcmp() can compare\n> (pack, tuple) against an SHA-1 just fine.\n\nYou mean \"if it came in format, convert it down to\n until the last second that it is needed (e.g. need to put\nthat in a tree object in order to compute the object name of the\ncontaining tree object)\"?\n\nAfter converting an object name originally represented as , if we are doing the \"union in struct\" thing, to \nrepresentation, you would have to look it up from .idx in order to\nread the contents the usual way. If that happens often enough, then\nit may not be worth adding complexity to the code to carry the\n pair around.\n\nUnless you fix that \"union in struct\" assumption, that is.\n\nTo me, information smells to belong more to a \"struct\nobject\" (or its subclass) as an optional annotation---when a caller\nis asked to parse_object(), you would bypass the sha1_read_file()\nthat goes and looks the object name up from the list of pack .idx\nand instead go there straight using that annotation.\n"},{"id":"257598","messageId":"xmqqa8zi2gz4.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"xmqqwq2m2hwf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-12T18:44:31Z","receivedAt":"2015-03-12T18:44:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Duy Nguyen writes:\n>\n>> This may or may not fall into the \"mix different hash functions\"\n>> category. In pack files version 4, trees are encoded to point to other\n>> trees or blobs by a (pack, offset) tuple. It would be great if the new\n>> object_id could support carrying this kind of object id around because\n>> it could help reduce object lookup cost a lot. (pack, offset) can be\n>> converted back to SHA-1 so no info is lost and hashcmp() can compare\n>> (pack, tuple) against an SHA-1 just fine.\n>\n> You mean \"if it came in format, convert it down to\n> until the last second that it is needed (e.g. need to put\n> that in a tree object in order to compute the object name of the\n> containing tree object)\"?\n\nSorry, obviously I meant \"do not convert until the last second\".\n\n> To me, information smells to belong more to a \"struct\n> object\" (or its subclass) as an optional annotation---when a caller\n> is asked to parse_object(), you would bypass the sha1_read_file()\n> that goes and looks the object name up from the list of pack .idx\n> and instead go there straight using that annotation.\n"},{"id":"257615","messageId":"CACsJy8CMZbwyBayX-bbWmGwV=AWC000Yx6LfzOcB2irq2X6qHQ@mail.gmail.com","threadId":"38745","inReplyTo":"xmqqwq2m2hwf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-13T00:58:20Z","receivedAt":"2015-03-13T00:58:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 13, 2015 at 1:24 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> This may or may not fall into the \"mix different hash functions\"\n>> category. In pack files version 4, trees are encoded to point to other\n>> trees or blobs by a (pack, offset) tuple. It would be great if the new\n>> object_id could support carrying this kind of object id around because\n>> it could help reduce object lookup cost a lot. (pack, offset) can be\n>> converted back to SHA-1 so no info is lost and hashcmp() can compare\n>> (pack, tuple) against an SHA-1 just fine.\n>\n> You mean \"if it came in format, convert it down to\n> until the last second that it is needed (e.g. need to put\n> that in a tree object in order to compute the object name of the\n> containing tree object)\"?\n\nI picked my words poorly. It should be \ninstead of the _byte_ offset. This index ranges from 0 to the number\nof objects minus one. This is essentially what pack v4 stores in\nplaces where pack v2 would store SHA-1, which makes me make the\nconnection: both are different ways of identifying an object.\n\n> After converting an object name originally represented as offset>, if we are doing the \"union in struct\" thing, to \n> representation, you would have to look it up from .idx in order to\n> read the contents the usual way. If that happens often enough, then\n> it may not be worth adding complexity to the code to carry the\n> pair around.\n\nI'd keep it in for as long as possible (or until higher\nlayer decides that it's not worth keeping in this representation\nanymore). I can only see two use cases where actual SHA-1 is involved:\nsorting by SHA-1 (rarely done, probably only in index-pack and\npack-objects) and output to the screen (or producing v2 packs).\n\n> Unless you fix that \"union in struct\" assumption, that is.\n>\n> To me, information smells to belong more to a \"struct\n> object\" (or its subclass) as an optional annotation---when a caller\n> is asked to parse_object(), you would bypass the sha1_read_file()\n> that goes and looks the object name up from the list of pack .idx\n> and instead go there straight using that annotation.\n\nFor pack v4, commits and trees can be encoded this way. parse_object()\ncould help the commit case (maybe, maybe not, I haven't looked at\ncommit walkers), not recursive tree walking where we now pass SHA-1\naround to user callback and all. Carrying optional annotation around\nwould impact the code in many places. Also, storing this info in\nstruct object seems conflict with v4 goal of reducing \"struct object\"\nlookup cost. Maybe I'm missing something here.\n\nThen again, we don't know if pack v4 will get merged in the end (I do\nhope it will). And we have an option of making specialized commit/tree\nwalkers that are aware of pack v4 and only use them in hot places to\nreduce impact to the rest of the code base. If hash[GIT_MAX_RAWSZ]\nlooks like a good enough solution, we can go with that and worry about\npack v4 later when/if it comes.\n-- \nDuy\n"},{"id":"257630","messageId":"xmqqwq2lzb6u.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"CACsJy8CMZbwyBayX-bbWmGwV=AWC000Yx6LfzOcB2irq2X6qHQ@mail.gmail.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-13T06:03:05Z","receivedAt":"2015-03-13T06:03:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> You mean \"if it came in format, convert it down to\n>> until the last second that it is needed (e.g. need to put\n>> that in a tree object in order to compute the object name of the\n>> containing tree object)\"?\n>\n> I picked my words poorly. It should be \n> instead of the _byte_ offset.\n\nThanks for a clarification, but I do not think it affects the main\npoint of the discussion very much. If we use \"union in struct\",\nwhere we can store either an SHA-1 hash or some other identifying\ninformation for the object, but not both, then at some point you\nwould need to convert to in a codepath that\nneeds the sha-1 hash value (e.g. if the object A, that is known to\nyou as , is placed in a tree object B, you need the\nobject name of A in representation in order to compute the\nobject name of the tree object B. You can choose to keep it in\n form in \"struct object_id { union }\" and look up the\n from the pack index every time, or you can choose to give\nup the form and upgrade the \"struct object_id\" to store\n at that point.\n\nIf you keep both *and* in \"struct object_id\" at\nthe same time, you can choose whichever is convenient, but it would\nbloat everything in core. Not just it bloats \"struct object\" and\nits subclasses, the in-core index entries, which is what I meant\nby ...\n\n>> Unless you fix that \"union in struct\" assumption, that is.\n\n... this.\n\n>> To me, information smells to belong more to a \"struct\n>> object\" (or its subclass) as an optional annotation---when a caller\n>> is asked to parse_object(), you would bypass the sha1_read_file()\n>> that goes and looks the object name up from the list of pack .idx\n>> and instead go there straight using that annotation.\n>\n> For pack v4, commits and trees can be encoded this way.\n\nEven if your in-pack representation of a commit object allowed to\nstore the tree pointer in format, its object name must\nbe computed as if you have the commit object in the traditional\nformat and computed the hash of that (together with the standard\n\" \\0\" header), and at that point, you need the contained\nobject's name in format (imagine how you would implement the\ncommit-tree command). Hence, I do not think the v4 encoding changes\nthe discussion very much. I see the primary value of v4 encoding is\nto shorten the length of various fields take on-disk and in-pack.\nIf it were , it could be argued that it would also be\nfaster to get to the packed data in the packfile, and going from\n to the .idx file and then going to the location in the\ndata in the packfile would be faster than going from to a\nparticular pack and its in-pack offset, with the difference of cost\naround log(n)*m where n is the number of objects in a pack and m is\nthe total number of packs in the repository.\n\nIt is true that format (force that the referred-to object\nlives in the same pack as the referrer) can help speed up\ninterpretation of extended SHA-1 expression, e.g. \"v1.0^0:t\", which\ncan read v1.0 tag in v4 format, find the info for the commit\npointed by the tag and get to that data in the pack, find the \ninfo for the top-tree recorded in that commit and directly get to\nthe data of that tree, and then find the entry for \"t\", which will\ngive the object name for that subtree again in format, and at\nthat point you can find the of that final object, without\nhaving to know any object names of the intermediate objects\n(i.e. you must start from of the tag you obtain from the\nrefs API, but you didn't use the object name of the commit and its\ntop-level tree). So for such a codepath, I would say it would be\nsufficient to use a \"union in struct\" people have been envisioning\nand convert to when the latter form becomes\nnecessary for the first time for the object.\n\nAnyway, wouldn't this be all academic? I do not see how you would\nkeep the object name in the format in-core, as the\nobj_hash[] is a hashtable keyed by , and even when we switch\nto a different hash, I cannot see how such a table to ensure the\nsingleton-ness of in-core objects can be keyed sometimes by \nand by in some other time.\n"},{"id":"257650","messageId":"20150313224524.GG46326@vauxhall.crustytoothpaste.net","threadId":"38745","inReplyTo":"xmqqk2yn5l39.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 00/10] Use a structure for object IDs.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-03-13T22:45:24Z","receivedAt":"2015-03-13T22:45:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Mar 11, 2015 at 01:35:06PM -0700, Junio C Hamano wrote:\n>Michael Haggerty writes:\n>> 4. We continue to support working with SHA-1s declared to be (unsigned\n>> char *) in some performance-critical code, even as we migrate most other\n>> code to using SHA-1s embedded within a (struct object_id). This will\n>> cost some duplication of code. To accept this approach, we would need an\n>> idea of *how much* code duplication would be needed. E.g., how many\n>> functions will need both (unsigned char *) versions and (struct\n>> object_id *) versions?\n>\n>Ideally, only the ones that knows the underlying hash function is\n>SHA-1 (i.e. anybody who mentions git_SHA_CTX), moves bytes from/to\n>in-core object name field and raw range of bytes (e.g. sha1hash());\n>everybody else like hashcpy() and hashcmp() should be able to do its\n>thing only on structs and a generic-looking constant that denotes\n>how many bytes there are in the hash (or even sizeof(struct oid)).\n>\n>I do not know what kind of code duplication you are worried about,\n>though. If a callee needs \"unsigned char *\", the caller that has a\n>\"struct object_id *o\" should pass o->hash to the callee.\n\nThat's the plan. My goal (which may or may not be achievable) is to\nmake hashcpy, hashcmp, and similar functions an implementation detail of\nstruct object_id functions. If we have to use o->hash somewhere, okay.\nI'm much more interested in practicality than theoretical purity. I\nwant these changes to provide easier-to-change, more maintainable code,\nnot more complex and difficult code.\n\n>And please do not suggest switching to C++; all it would do to\n>overload these into a single name is to make the callers harder to\n>read.\n\nI'm not considering that. I've attempted to compile git with g++\nbefore as an idle curiosity, and I gave up almost immediately because it\nlooked like a bunch of work.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"257675","messageId":"CACsJy8BeFO3RQJ6pU5+q6E4n3A36X9PgYoi7xB_NQ4C_JYaP5w@mail.gmail.com","threadId":"38745","inReplyTo":"xmqqwq2lzb6u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-14T11:49:51Z","receivedAt":"2015-03-14T11:49:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 13, 2015 at 1:03 PM, Junio C Hamano wrote:\n>>> To me, information smells to belong more to a \"struct\n>>> object\" (or its subclass) as an optional annotation---when a caller\n>>> is asked to parse_object(), you would bypass the sha1_read_file()\n>>> that goes and looks the object name up from the list of pack .idx\n>>> and instead go there straight using that annotation.\n>>\n>> For pack v4, commits and trees can be encoded this way.\n>\n> Even if your in-pack representation of a commit object allowed to\n> store the tree pointer in format, its object name must\n> be computed as if you have the commit object in the traditional\n> format and computed the hash of that (together with the standard\n> \" \\0\" header), and at that point, you need the contained\n> object's name in format (imagine how you would implement the\n> commit-tree command).\n\nTrue. But commit-tree is not often executed as, say, rev-list, where\nthe hash is used as 20-byte key to some pieces somewhere (pack data,\nstruct object *). If we keep in object_id (the union\napproach), v4-aware code has the chance to go on a fast path.\n\n> Hence, I do not think the v4 encoding changes\n> the discussion very much. I see the primary value of v4 encoding is\n> to shorten the length of various fields take on-disk and in-pack.\n> If it were , it could be argued that it would also be\n> faster to get to the packed data in the packfile, and going from\n> to the .idx file and then going to the location in the\n> data in the packfile would be faster than going from to a\n> particular pack and its in-pack offset, with the difference of cost\n> around log(n)*m where n is the number of objects in a pack and m is\n> the total number of packs in the repository.\n>\n> It is true that format (force that the referred-to object\n> lives in the same pack as the referrer) can help speed up\n> interpretation of extended SHA-1 expression, e.g. \"v1.0^0:t\", which\n> can read v1.0 tag in v4 format, find the info for the commit\n> pointed by the tag and get to that data in the pack, find the \n> info for the top-tree recorded in that commit and directly get to\n> the data of that tree, and then find the entry for \"t\", which will\n> give the object name for that subtree again in format, and at\n> that point you can find the of that final object, without\n> having to know any object names of the intermediate objects\n> (i.e. you must start from of the tag you obtain from the\n> refs API, but you didn't use the object name of the commit and its\n> top-level tree). So for such a codepath, I would say it would be\n> sufficient to use a \"union in struct\" people have been envisioning\n> and convert to when the latter form becomes\n> necessary for the first time for the object.\n\nIt's the point: faster access (to either pack data, or \"struct object\n*\"). Using a separate union from struct object_id (iow, object_id\nonly contains SHA-1) works. But imagine to independent code islands\ncan perform this type of fast access. In order to pass this union from\none island to another, we need to update pretty much every function\ncall between them. The change to using object_id has a similar impact.\nIf we go with \"union in object_id\", we may be able to avoid the second\nmass change to make use of .\n\n> Anyway, wouldn't this be all academic? I do not see how you would\n> keep the object name in the format in-core, as the\n> obj_hash[] is a hashtable keyed by , and even when we switch\n> to a different hash, I cannot see how such a table to ensure the\n> singleton-ness of in-core objects can be keyed sometimes by \n> and by in some other time.\n\nI'm implementing something to see how much we gain by avoiding object\nlookup. The current approach is having \"struct object ** obj\" in\n\"struct packed_git\", indexed by \"nth\". So when you have \nand pack->obj[nth] is valid, you'll get to \"struct object *\" without\nhashing. If pack->obj[nth] is NULL, hashing and looking up are\nrequired the first time using SHA-1, then the result is cached in\npack->obj[]. So no, it's not academic :)\n-- \nDuy\n"},{"id":"257691","messageId":"xmqqbnjvw7c8.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"CACsJy8BeFO3RQJ6pU5+q6E4n3A36X9PgYoi7xB_NQ4C_JYaP5w@mail.gmail.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-14T22:19:03Z","receivedAt":"2015-03-14T22:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> Anyway, wouldn't this be all academic? I do not see how you would\n>> keep the object name in the format in-core, as the\n>> obj_hash[] is a hashtable keyed by , and even when we switch\n>> to a different hash, I cannot see how such a table to ensure the\n>> singleton-ness of in-core objects can be keyed sometimes by \n>> and by in some other time.\n>\n> I'm implementing something to see how much we gain by avoiding object\n> lookup. The current approach is having \"struct object ** obj\" in\n> \"struct packed_git\", indexed by \"nth\". So when you have \n> and pack->obj[nth] is valid, you'll get to \"struct object *\" without\n> hashing.\n\nBut do you realize that the hashtable serves two purposes? Grab the\nobject from its name is one thing, and the other one I am not seeing\nhow you will make it work with \"sometimes sometimes \"\nis to ensure that we will have only one in-core copy for the same object.\nWe even walk the hashtable when we want to drop the flag bits from\nall in-core objects, so even if you instanciated an in-core object\nwithout going through the object name layer, the hashtable needs to\nhave a pointer to such a pointer, no?\n"},{"id":"257697","messageId":"CACsJy8CK=0QpsMpZKN6Js99xFRD6mpJW1id2BBQDMsV1DaHiPg@mail.gmail.com","threadId":"38745","inReplyTo":"xmqqbnjvw7c8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-15T00:17:07Z","receivedAt":"2015-03-15T00:17:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 15, 2015 at 5:19 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>>> Anyway, wouldn't this be all academic? I do not see how you would\n>>> keep the object name in the format in-core, as the\n>>> obj_hash[] is a hashtable keyed by , and even when we switch\n>>> to a different hash, I cannot see how such a table to ensure the\n>>> singleton-ness of in-core objects can be keyed sometimes by \n>>> and by in some other time.\n>>\n>> I'm implementing something to see how much we gain by avoiding object\n>> lookup. The current approach is having \"struct object ** obj\" in\n>> \"struct packed_git\", indexed by \"nth\". So when you have \n>> and pack->obj[nth] is valid, you'll get to \"struct object *\" without\n>> hashing.\n>\n> But do you realize that the hashtable serves two purposes? Grab the\n> object from its name is one thing, and the other one I am not seeing\n> how you will make it work with \"sometimes sometimes \"\n> is to ensure that we will have only one in-core copy for the same object.\n> We even walk the hashtable when we want to drop the flag bits from\n> all in-core objects, so even if you instanciated an in-core object\n> without going through the object name layer, the hashtable needs to\n> have a pointer to such a pointer, no?\n\nNotice that the first time pack->obj[] is filled using\nlookup_object(). So yes, the hash table has all the pointers that\npack->obj[] has. If somebody wants to remove an object out of hash\ntable, then all pack->obj[] are invalidated, but I don't think we ever\ndelete objects from hash table.\n-- \nDuy\n"},{"id":"257704","messageId":"xmqqy4mzuh1e.fsf@gitster.dls.corp.google.com","threadId":"38745","inReplyTo":"CACsJy8CK=0QpsMpZKN6Js99xFRD6mpJW1id2BBQDMsV1DaHiPg@mail.gmail.com","subject":"Re: [PATCH v2 01/10] Define a structure for object IDs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T02:32:29Z","receivedAt":"2015-03-15T02:32:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Notice that the first time pack->obj[] is filled using\n> lookup_object(). So yes, the hash table has all the pointers that\n> pack->obj[] has.\n\nAre we talking about the same thing?\n\nBy \"the hash table\", I mean **obj_hash that is a hashtable that uses\n form of identifier as the key. So \"the hash table has all\nthe pointers\" sounds like all the objects you instantiate from a v4\npackfile needs their object name known in the form anyway\nwhen it gets instantiated (or more importantly, when you realize\nthere is another copy of it already in the **obj_hash hashtable, you\nmay have to free or refrain from creating it in the first place).\n\nAs long as we use **obj_hash hashtable as \"all the objects this\nprocess cares about in the world\" and \"struct object_id.hash\" as the\nkey into it, I do not think representation belongs to\nthat layer.\n\nI do think representation has its use as a short-cut\nmechanism to reach an indirectly referenced object directly without\ninstantiating intermediate objects when dealing with an extended\nSHA-1 expression (cf. \"v1.0^0:t\" example in a few messages\nupthread). I just think it does not belong to \"struct object_id\"\nthat are used to refer to in-core objects.\n"}]}