{"thread":{"id":"46588","subject":"[PATCH 0/5] Modernize read_graft_line implementation","startedAt":"2017-08-15T11:49:37Z","lastAt":"2017-08-18T19:47:08Z","messageCount":59,"participants":["Patryk Obara","Stefan Beller","Junio C Hamano","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"326382","messageId":"cover.1502796628.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":null,"subject":"[PATCH 0/5] Modernize read_graft_line implementation","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-15T11:49:01Z","receivedAt":"2017-08-15T11:49:37Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"I experimented with using a different hash algorithm (I am aware of\nexisting \"Git hash function transition plan\", I just want to push\nthings forward a bit) - and immediately hit a small issue - changing\nthe size of object_id hash buffer leads to compilation issues and\nbreaks graft-related tests.\n\nI am sending patch 1 only to show a modification, that I did to\nincrease buffer size - it's not intended to be merged.\n\nPatch 2 fixes trivial compilation issue.\n\nPatches 3, 4, and 5 touch graft implementation to remove calculations\nusing GIT_SHA1_*, that lead to broken tests. I replaced FLEX_ARRAY of\nobject_id's representing parents with oid_array. New implementation\nshould be more future-proof, I think.\n\nNew implementation has tiny behaviour change: previously parents in\ngraft line needed to be separated with single space - now any number\nof whitespace characters will do.\n\nAlternative implementation approaches\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nStrbuf could be replaced with string_list with\nstring_list_split_in_place instead of while loop in read_graft_line.\nI didn't implement it this way because I learned\nabout string_list_split_in_place after finishing this implementation\ndraft. Right now I'm not sure which approach is better.\n\nAnother possibility is dropping graft feature altogether - that would\nmean removing code for parsing grafts and 'parent' field in the struct,\nbut preserving the struct itself as a shallow clone marker. Grafts are\na little-known feature with modern replacement, but this seems like\nbigger task and rather out of the scope of transition to the new\nhashing algorithm.\n\nI considered making function read_graft_line a static one and\nread_graft_file non-static, but read_graft_line is used in\n'builtin/blame.c' in function read_ancestry, which is almost a copy of\nread_graft_file (difference of single boolean flag passed to\nregister_commit_graft). Removal of this duplication may be worthwhile,\nbut I think it's out of scope.\n\nPatryk Obara (5):\n cache: extend object_id size to sha3-256\n sha1_file: fix hardcoded size in null_sha1\n commit: replace the raw buffer with strbuf in read_graft_line\n commit: implement free_commit_graft\n commit: rewrite read_graft_line\n\n builtin/blame.c | 2 +-\n cache.h | 8 ++++++--\n commit.c | 55 ++++++++++++++++++++++++++++++++-----------------------\n commit.h | 5 +++--\n sha1_file.c | 2 +-\n shallow.c | 1 +\n 6 files changed, 44 insertions(+), 29 deletions(-)\n\n-- \n2.9.5\n\n"},{"id":"326383","messageId":"c6d4d0a52c5d33a2c6e3e8249fcf5696f06e3e0c.1502796628.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"[PATCH 1/5] cache: extend object_id size to sha3-256","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-15T11:49:02Z","receivedAt":"2017-08-15T11:49:39Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This commit is not intended to be merged - it serves only as context for\nnext patches in this thread.\n\nSigned-off-by: Patryk Obara \n---\n cache.h | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 1c69d2a..ad8a57c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -68,9 +68,13 @@ unsigned long git_deflate_bound(git_zstream *, unsigned long);\n #define GIT_SHA1_RAWSZ 20\n #define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)\n \n+/* The length in bytes and in hex digits of an object name (SHA3-256 value). */\n+#define GIT_SHA3_256_RAWSZ 32\n+#define GIT_SHA3_256_HEXSZ (2 * GIT_SHA3_256_RAWSZ)\n+\n /* The length in byte and in hex digits of the largest possible hash value. */\n-#define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ\n-#define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ\n+#define GIT_MAX_RAWSZ GIT_SHA3_256_RAWSZ\n+#define GIT_MAX_HEXSZ GIT_SHA3_256_HEXSZ\n \n struct object_id {\n \tunsigned char hash[GIT_MAX_RAWSZ];\n-- \n2.9.5\n\n"},{"id":"326384","messageId":"a21088f049390828cdee957f88503e8466e1d34e.1502796628.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"[PATCH 2/5] sha1_file: fix hardcoded size in null_sha1","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-15T11:49:03Z","receivedAt":"2017-08-15T11:49:41Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This prevents compilation error if GIT_MAX_RAWSZ is different than 20.\n\nSigned-off-by: Patryk Obara \n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b60ae15..f5b5bec 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -32,7 +32,7 @@\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n-const unsigned char null_sha1[20];\n+const unsigned char null_sha1[GIT_MAX_RAWSZ];\n const struct object_id null_oid;\n const struct object_id empty_tree_oid = {\n \tEMPTY_TREE_SHA1_BIN_LITERAL\n-- \n2.9.5\n\n"},{"id":"326385","messageId":"945cc94bedab645885f9025cee51efd8205a69a4.1502796628.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"[PATCH 4/5] commit: implement free_commit_graft","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-15T11:49:05Z","receivedAt":"2017-08-15T11:49:43Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Signed-off-by: Patryk Obara \n---\n commit.c | 11 ++++++++---\n commit.h | 1 +\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 499fb14..6a145f1 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -109,15 +109,20 @@ static int commit_graft_pos(const unsigned char *sha1)\n \t\t\tcommit_graft_sha1_access);\n }\n \n+void free_commit_graft(struct commit_graft *graft)\n+{\n+\tfree(graft);\n+}\n+\n int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n {\n \tint pos = commit_graft_pos(graft->oid.hash);\n \n \tif (0 <= pos) {\n \t\tif (ignore_dups)\n-\t\t\tfree(graft);\n+\t\t\tfree_commit_graft(graft);\n \t\telse {\n-\t\t\tfree(commit_graft[pos]);\n+\t\t\tfree_commit_graft(commit_graft[pos]);\n \t\t\tcommit_graft[pos] = graft;\n \t\t}\n \t\treturn 1;\n@@ -163,7 +168,7 @@ struct commit_graft *read_graft_line(struct strbuf *line)\n \n bad_graft_data:\n \terror(\"bad graft data: %s\", buf);\n-\tfree(graft);\n+\tfree_commit_graft(graft);\n \treturn NULL;\n }\n \ndiff --git a/commit.h b/commit.h\nindex baecc0a..c1b319f 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -247,6 +247,7 @@ struct commit_graft {\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n+void free_commit_graft(struct commit_graft *);\n struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n-- \n2.9.5\n\n"},{"id":"326386","messageId":"b5633a5425c623f3d2204325e99332b5bb511582.1502796628.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"[PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-15T11:49:04Z","receivedAt":"2017-08-15T11:49:49Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This simplifies function declaration and allows for use of strbuf_rtrim\ninstead of modifying buffer directly.\n\nSigned-off-by: Patryk Obara \n---\n builtin/blame.c | 2 +-\n commit.c | 11 ++++++-----\n commit.h | 2 +-\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bda1a78..d4472e9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -488,7 +488,7 @@ static int read_ancestry(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (graft)\n \t\t\tregister_commit_graft(graft, 0);\n \t}\ndiff --git a/commit.c b/commit.c\nindex 8b28415..499fb14 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -134,15 +134,16 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \treturn 0;\n }\n \n-struct commit_graft *read_graft_line(char *buf, int len)\n+struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i;\n+\tint i, len;\n+\tchar *buf = line->buf;\n \tstruct commit_graft *graft = NULL;\n \tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n \n-\twhile (len && isspace(buf[len-1]))\n-\t\tbuf[--len] = '\\0';\n+\tstrbuf_rtrim(line);\n+\tlen = line->len;\n \tif (buf[0] == '#' || buf[0] == '\\0')\n \t\treturn NULL;\n \tif ((len + 1) % entry_size)\n@@ -174,7 +175,7 @@ static int read_graft_file(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (!graft)\n \t\t\tcontinue;\n \t\tif (register_commit_graft(graft, 1))\ndiff --git a/commit.h b/commit.h\nindex 6d857f0..baecc0a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -247,7 +247,7 @@ struct commit_graft {\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n-struct commit_graft *read_graft_line(char *buf, int len);\n+struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n \n-- \n2.9.5\n\n"},{"id":"326387","messageId":"ee8edaf08864d5983ff1a5150077d29a4ee17796.1502796628.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"[PATCH 5/5] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-15T11:49:06Z","receivedAt":"2017-08-15T11:49:52Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"The previous implementation of read_graft_line used calculations based\non GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ to determine the number of commit\nids in a single graft line. New implementation does not depend on these\nconstants, so it adapts to any object_id buffer size.\n\nTo make this possible, FLEX_ARRAY of object_id in struct was replaced\nby an oid_array.\n\nCode allocating graft now needs to use memset to zero the memory before\nuse to start with oid_array in a consistent state.\n\nUpdates free_graft function implemented in the previous patch to\nproperly cleanup an oid_array storing parents.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 39 +++++++++++++++++++++------------------\n commit.h | 2 +-\n shallow.c | 1 +\n 3 files changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 6a145f1..75dd45d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -111,6 +111,7 @@ static int commit_graft_pos(const unsigned char *sha1)\n \n void free_commit_graft(struct commit_graft *graft)\n {\n+\toid_array_clear(&graft->parents);\n \tfree(graft);\n }\n \n@@ -139,35 +140,37 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \treturn 0;\n }\n \n+static int parse_next_oid_hex(const char *buf, struct object_id *oid, const char **end)\n+{\n+\twhile (isspace(buf[0]))\n+\t\tbuf++;\n+\treturn parse_oid_hex(buf, oid, end);\n+}\n+\n struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i, len;\n-\tchar *buf = line->buf;\n \tstruct commit_graft *graft = NULL;\n-\tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n+\tstruct object_id oid;\n+\tconst char *tail = NULL;\n \n \tstrbuf_rtrim(line);\n-\tlen = line->len;\n-\tif (buf[0] == '#' || buf[0] == '\\0')\n+\tif (line->buf[0] == '#' || line->len == 0)\n \t\treturn NULL;\n-\tif ((len + 1) % entry_size)\n+\tgraft = xmalloc(sizeof(*graft));\n+\tmemset(graft, 0, sizeof(*graft));\n+\tif (parse_oid_hex(line->buf, &graft->oid, &tail))\n \t\tgoto bad_graft_data;\n-\ti = (len + 1) / entry_size - 1;\n-\tgraft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n-\tgraft->nr_parent = i;\n-\tif (get_oid_hex(buf, &graft->oid))\n+\twhile (!parse_next_oid_hex(tail, &oid, &tail))\n+\t\toid_array_append(&graft->parents, &oid);\n+\tif (tail[0] != '\\0')\n \t\tgoto bad_graft_data;\n-\tfor (i = GIT_SHA1_HEXSZ; i < len; i += entry_size) {\n-\t\tif (buf[i] != ' ')\n-\t\t\tgoto bad_graft_data;\n-\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/entry_size].hash))\n-\t\t\tgoto bad_graft_data;\n-\t}\n+\tgraft->nr_parent = graft->parents.nr;\n+\n \treturn graft;\n \n bad_graft_data:\n-\terror(\"bad graft data: %s\", buf);\n+\terror(\"bad graft data: %s\", line->buf);\n \tfree_commit_graft(graft);\n \treturn NULL;\n }\n@@ -363,7 +366,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->parents.oid[i]);\n \t\t\tif (!new_parent)\n \t\t\t\tcontinue;\n \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\ndiff --git a/commit.h b/commit.h\nindex c1b319f..070d45d 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -243,7 +243,7 @@ void sort_in_topological_order(struct commit_list **, enum rev_sort_order);\n struct commit_graft {\n \tstruct object_id oid;\n \tint nr_parent; /* < 0 if shallow commit */\n-\tstruct object_id parent[FLEX_ARRAY]; /* more */\n+\tstruct oid_array parents;\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \ndiff --git a/shallow.c b/shallow.c\nindex f5591e5..892cd90 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -33,6 +33,7 @@ int register_shallow(const struct object_id *oid)\n \t\txmalloc(sizeof(struct commit_graft));\n \tstruct commit *commit = lookup_commit(oid);\n \n+\tmemset(graft, 0, sizeof(*graft));\n \toidcpy(&graft->oid, oid);\n \tgraft->nr_parent = -1;\n \tif (commit && commit->object.parsed)\n-- \n2.9.5\n\n"},{"id":"326408","messageId":"CAGZ79kaCtbuDEUJqJ+nVUW78ksLMyHZ2xhnbunUqzjkGRuYg+A@mail.gmail.com","threadId":"46588","inReplyTo":"b5633a5425c623f3d2204325e99332b5bb511582.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T17:02:56Z","receivedAt":"2017-08-15T17:03:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 4:49 AM, Patryk Obara wrote:\n> This simplifies function declaration and allows for use of strbuf_rtrim\n> instead of modifying buffer directly.\n>\n> Signed-off-by: Patryk Obara \n> ---\n> builtin/blame.c | 2 +-\n> commit.c | 11 ++++++-----\n> commit.h | 2 +-\n> 3 files changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index bda1a78..d4472e9 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -488,7 +488,7 @@ static int read_ancestry(const char *graft_file)\n> return -1;\n> while (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> /* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> - struct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n> + struct commit_graft *graft = read_graft_line(&buf);\n> if (graft)\n> register_commit_graft(graft, 0);\n> }\n> diff --git a/commit.c b/commit.c\n> index 8b28415..499fb14 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -134,15 +134,16 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n> return 0;\n> }\n>\n> -struct commit_graft *read_graft_line(char *buf, int len)\n> +struct commit_graft *read_graft_line(struct strbuf *line)\n> {\n> /* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> - int i;\n> + int i, len;\n> + char *buf = line->buf;\n> struct commit_graft *graft = NULL;\n> const int entry_size = GIT_SHA1_HEXSZ + 1;\n\noutside the scope of this patch:\nIs GIT_SHA1_HEXSZ or GIT_MAX_HEXSZ the right call here?\n\n>\n> - while (len && isspace(buf[len-1]))\n> - buf[--len] = '\\0';\n> + strbuf_rtrim(line);\n> + len = line->len;\n> if (buf[0] == '#' || buf[0] == '\\0')\n> return NULL;\n> if ((len + 1) % entry_size)\n> @@ -174,7 +175,7 @@ static int read_graft_file(const char *graft_file)\n> return -1;\n> while (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> /* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> - struct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n> + struct commit_graft *graft = read_graft_line(&buf);\n> if (!graft)\n> continue;\n> if (register_commit_graft(graft, 1))\n> diff --git a/commit.h b/commit.h\n> index 6d857f0..baecc0a 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -247,7 +247,7 @@ struct commit_graft {\n> };\n> typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n>\n> -struct commit_graft *read_graft_line(char *buf, int len);\n> +struct commit_graft *read_graft_line(struct strbuf *line);\n> int register_commit_graft(struct commit_graft *, int);\n> struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n>\n> --\n> 2.9.5\n>\n"},{"id":"326409","messageId":"CAGZ79kbT7MZcWnWiQOWt_SkFMpK-u5K2=9ktXK6FaaHypt7+Nw@mail.gmail.com","threadId":"46588","inReplyTo":"945cc94bedab645885f9025cee51efd8205a69a4.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 4/5] commit: implement free_commit_graft","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T17:04:26Z","receivedAt":"2017-08-15T17:04:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 4:49 AM, Patryk Obara wrote:\n\nHere is a good place to explain why this is a good patch,\n(which is not immediately obvious to me at least).\n\n> Signed-off-by: Patryk Obara \n> ---\n> commit.c | 11 ++++++++---\n> commit.h | 1 +\n> 2 files changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index 499fb14..6a145f1 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -109,15 +109,20 @@ static int commit_graft_pos(const unsigned char *sha1)\n> commit_graft_sha1_access);\n> }\n>\n> +void free_commit_graft(struct commit_graft *graft)\n> +{\n> + free(graft);\n> +}\n> +\n> int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n> {\n> int pos = commit_graft_pos(graft->oid.hash);\n>\n> if (0 <= pos) {\n> if (ignore_dups)\n> - free(graft);\n> + free_commit_graft(graft);\n> else {\n> - free(commit_graft[pos]);\n> + free_commit_graft(commit_graft[pos]);\n> commit_graft[pos] = graft;\n> }\n> return 1;\n> @@ -163,7 +168,7 @@ struct commit_graft *read_graft_line(struct strbuf *line)\n>\n> bad_graft_data:\n> error(\"bad graft data: %s\", buf);\n> - free(graft);\n> + free_commit_graft(graft);\n> return NULL;\n> }\n>\n> diff --git a/commit.h b/commit.h\n> index baecc0a..c1b319f 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -247,6 +247,7 @@ struct commit_graft {\n> };\n> typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n>\n> +void free_commit_graft(struct commit_graft *);\n> struct commit_graft *read_graft_line(struct strbuf *line);\n> int register_commit_graft(struct commit_graft *, int);\n> struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n> --\n> 2.9.5\n>\n"},{"id":"326412","messageId":"CAGZ79kbsKLOiLvr8NxRzBDkU44SPtVozUoV7fYUyt3vC=QxWyQ@mail.gmail.com","threadId":"46588","inReplyTo":"ee8edaf08864d5983ff1a5150077d29a4ee17796.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 5/5] commit: rewrite read_graft_line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T17:11:23Z","receivedAt":"2017-08-15T17:11:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 4:49 AM, Patryk Obara wrote:\n> The previous implementation of read_graft_line used calculations based\n> on GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ to determine the number of commit\n> ids in a single graft line. New implementation does not depend on these\n> constants, so it adapts to any object_id buffer size.\n>\n> To make this possible, FLEX_ARRAY of object_id in struct was replaced\n> by an oid_array.\n>\n> Code allocating graft now needs to use memset to zero the memory before\n> use to start with oid_array in a consistent state.\n>\n> Updates free_graft function implemented in the previous patch to\n> properly cleanup an oid_array storing parents.\n>\n> Signed-off-by: Patryk Obara \n> ---\n> commit.c | 39 +++++++++++++++++++++------------------\n> commit.h | 2 +-\n> shallow.c | 1 +\n> 3 files changed, 23 insertions(+), 19 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index 6a145f1..75dd45d 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -111,6 +111,7 @@ static int commit_graft_pos(const unsigned char *sha1)\n>\n> void free_commit_graft(struct commit_graft *graft)\n> {\n> + oid_array_clear(&graft->parents);\n\nOh patch 4/5 could just say it's in preparation of this patch.\nNevermind the comment on patch 4.\n"},{"id":"326414","messageId":"CAGZ79kbGNMHVjfzZBiEAkUV+WVY=a5aQsV3Da=1yKcCkFR-ewA@mail.gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 0/5] Modernize read_graft_line implementation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T17:19:50Z","receivedAt":"2017-08-15T17:19:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 4:49 AM, Patryk Obara wrote:\n\nWelcome (back?) to the git mailing list!\n\n> I experimented with using a different hash algorithm (I am aware of\n> existing \"Git hash function transition plan\", I just want to push\n> things forward a bit) - and immediately hit a small issue - changing\n> the size of object_id hash buffer leads to compilation issues and\n> breaks graft-related tests.\n\nThanks for advancing this frontier. :)\n\n>\n> I am sending patch 1 only to show a modification, that I did to\n> increase buffer size - it's not intended to be merged.\n>\n> Patch 2 fixes trivial compilation issue.\n>\n> Patches 3, 4, and 5 touch graft implementation to remove calculations\n> using GIT_SHA1_*, that lead to broken tests. I replaced FLEX_ARRAY of\n> object_id's representing parents with oid_array. New implementation\n> should be more future-proof, I think.\n\nI would think so, too.\n\nparse_oid_hex currently only reads sha1, but once it can read a new\nhash (or both old and new hash), it would solve the graft problems.\n\n> New implementation has tiny behaviour change: previously parents in\n> graft line needed to be separated with single space - now any number\n> of whitespace characters will do.\n\nYeah that is because of parse_next_oid_hex in patch 5 is pretty smart\n(and if we'd want to preserve behavior we'd need to just skip one SP\nand in case of more SP \"goto bad_graft_data\" that is in the\ncaller function.\n\n>\n> Alternative implementation approaches\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>\n> Strbuf could be replaced with string_list with\n> string_list_split_in_place instead of while loop in read_graft_line.\n> I didn't implement it this way because I learned\n> about string_list_split_in_place after finishing this implementation\n> draft. Right now I'm not sure which approach is better.\n>\n> Another possibility is dropping graft feature altogether - that would\n> mean removing code for parsing grafts and 'parent' field in the struct,\n> but preserving the struct itself as a shallow clone marker. Grafts are\n> a little-known feature with modern replacement, but this seems like\n> bigger task and rather out of the scope of transition to the new\n> hashing algorithm.\n>\n> I considered making function read_graft_line a static one and\n> read_graft_file non-static, but read_graft_line is used in\n> 'builtin/blame.c' in function read_ancestry, which is almost a copy of\n> read_graft_file (difference of single boolean flag passed to\n> register_commit_graft). Removal of this duplication may be worthwhile,\n> but I think it's out of scope.\n\nI think the grafts may be still in use in Linux, to fault in the\nhistory before git was used, which cannot be replaced by the\nshallow mechanism.\n\nThanks for the patches 2-5!\n\nStefan\n"},{"id":"326428","messageId":"xmqq1socsnay.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"a21088f049390828cdee957f88503e8466e1d34e.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 2/5] sha1_file: fix hardcoded size in null_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T18:23:01Z","receivedAt":"2017-08-15T18:23:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> This prevents compilation error if GIT_MAX_RAWSZ is different than 20.\n>\n> Signed-off-by: Patryk Obara \n> ---\n> sha1_file.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nI think this is OK for \"null\" thing, but in general I feel\nambivalent when I see the use of \"MAX\" thing.\n\nUse of \"MAX\" here implies that we wish to support multiple hashes at\nthe same time in a single binary, so that longer or shorter hashes\nfit there. But in such a case, the callers would want a union of\nsome sort so that they can say \"I am using SHA2, give me the null\nthing\"? \n\nI said this is OK for \"null\" because we assume we will use ^\\0{len}$\nfor any hash function we choose as the \"impossible\" value, and for\nthat particular use pattern, we do not need such a union. Just\nletting the caller peek at an appropriate number of bytes at the\nbeginning of that NUL buffer for hash the caller wants to use is\nsufficient.\n\nThanks.\n\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index b60ae15..f5b5bec 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -32,7 +32,7 @@\n> #define SZ_FMT PRIuMAX\n> static inline uintmax_t sz_fmt(size_t s) { return s; }\n> \n> -const unsigned char null_sha1[20];\n> +const unsigned char null_sha1[GIT_MAX_RAWSZ];\n> const struct object_id null_oid;\n> const struct object_id empty_tree_oid = {\n> \tEMPTY_TREE_SHA1_BIN_LITERAL\n"},{"id":"326429","messageId":"xmqqwp64r8n0.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"b5633a5425c623f3d2204325e99332b5bb511582.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T18:25:07Z","receivedAt":"2017-08-15T18:25:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> This simplifies function declaration and allows for use of strbuf_rtrim\n> instead of modifying buffer directly.\n>\n> Signed-off-by: Patryk Obara \n> ---\n> builtin/blame.c | 2 +-\n> commit.c | 11 ++++++-----\n> commit.h | 2 +-\n> 3 files changed, 8 insertions(+), 7 deletions(-)\n\nLooks good; both existing callers already have strbuf, and we do not\nexpect we will gain a lot more callers to this ancient facility, so\nthere is no point to have a more accomodating API that separately\ntakes .\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index bda1a78..d4472e9 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -488,7 +488,7 @@ static int read_ancestry(const char *graft_file)\n> \t\treturn -1;\n> \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> -\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n> +\t\tstruct commit_graft *graft = read_graft_line(&buf);\n> \t\tif (graft)\n> \t\t\tregister_commit_graft(graft, 0);\n> \t}\n> diff --git a/commit.c b/commit.c\n> index 8b28415..499fb14 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -134,15 +134,16 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n> \treturn 0;\n> }\n> \n> -struct commit_graft *read_graft_line(char *buf, int len)\n> +struct commit_graft *read_graft_line(struct strbuf *line)\n> {\n> \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> -\tint i;\n> +\tint i, len;\n> +\tchar *buf = line->buf;\n> \tstruct commit_graft *graft = NULL;\n> \tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n> \n> -\twhile (len && isspace(buf[len-1]))\n> -\t\tbuf[--len] = '\\0';\n> +\tstrbuf_rtrim(line);\n> +\tlen = line->len;\n> \tif (buf[0] == '#' || buf[0] == '\\0')\n> \t\treturn NULL;\n> \tif ((len + 1) % entry_size)\n> @@ -174,7 +175,7 @@ static int read_graft_file(const char *graft_file)\n> \t\treturn -1;\n> \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> -\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n> +\t\tstruct commit_graft *graft = read_graft_line(&buf);\n> \t\tif (!graft)\n> \t\t\tcontinue;\n> \t\tif (register_commit_graft(graft, 1))\n> diff --git a/commit.h b/commit.h\n> index 6d857f0..baecc0a 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -247,7 +247,7 @@ struct commit_graft {\n> };\n> typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n> \n> -struct commit_graft *read_graft_line(char *buf, int len);\n> +struct commit_graft *read_graft_line(struct strbuf *line);\n> int register_commit_graft(struct commit_graft *, int);\n> struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n"},{"id":"326430","messageId":"xmqqshgsr8kn.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"945cc94bedab645885f9025cee51efd8205a69a4.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 4/5] commit: implement free_commit_graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T18:26:32Z","receivedAt":"2017-08-15T18:26:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Signed-off-by: Patryk Obara \n> ---\n> commit.c | 11 ++++++++---\n> commit.h | 1 +\n> 2 files changed, 9 insertions(+), 3 deletions(-)\n\nI do not see a need to make this new function extern. Shouldn't it\nbe made \"static\" and revert the change to commit.h?\n\n> diff --git a/commit.c b/commit.c\n> index 499fb14..6a145f1 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -109,15 +109,20 @@ static int commit_graft_pos(const unsigned char *sha1)\n> \t\t\tcommit_graft_sha1_access);\n> }\n> \n> +void free_commit_graft(struct commit_graft *graft)\n> +{\n> +\tfree(graft);\n> +}\n> +\n> int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n> {\n> \tint pos = commit_graft_pos(graft->oid.hash);\n> \n> \tif (0 <= pos) {\n> \t\tif (ignore_dups)\n> -\t\t\tfree(graft);\n> +\t\t\tfree_commit_graft(graft);\n> \t\telse {\n> -\t\t\tfree(commit_graft[pos]);\n> +\t\t\tfree_commit_graft(commit_graft[pos]);\n> \t\t\tcommit_graft[pos] = graft;\n> \t\t}\n> \t\treturn 1;\n> @@ -163,7 +168,7 @@ struct commit_graft *read_graft_line(struct strbuf *line)\n> \n> bad_graft_data:\n> \terror(\"bad graft data: %s\", buf);\n> -\tfree(graft);\n> +\tfree_commit_graft(graft);\n> \treturn NULL;\n> }\n> \n> diff --git a/commit.h b/commit.h\n> index baecc0a..c1b319f 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -247,6 +247,7 @@ struct commit_graft {\n> };\n> typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n> \n> +void free_commit_graft(struct commit_graft *);\n> struct commit_graft *read_graft_line(struct strbuf *line);\n> int register_commit_graft(struct commit_graft *, int);\n> struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n"},{"id":"326431","messageId":"CAGZ79kaCA29j6ON4KSsB=EH8FPfZGE56hVGSAAepcPiH952v6g@mail.gmail.com","threadId":"46588","inReplyTo":"xmqq1socsnay.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] sha1_file: fix hardcoded size in null_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T18:29:55Z","receivedAt":"2017-08-15T18:30:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 11:23 AM, Junio C Hamano wrote:\n> Patryk Obara writes:\n>\n>> This prevents compilation error if GIT_MAX_RAWSZ is different than 20.\n>>\n>> Signed-off-by: Patryk Obara \n>> ---\n>> sha1_file.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> I think this is OK for \"null\" thing, but in general I feel\n> ambivalent when I see the use of \"MAX\" thing.\n>\n> Use of \"MAX\" here implies that we wish to support multiple hashes at\n> the same time in a single binary, so that longer or shorter hashes\n> fit there. But in such a case, the callers would want a union of\n> some sort so that they can say \"I am using SHA2, give me the null\n> thing\"?\n>\n> I said this is OK for \"null\" because we assume we will use ^\\0{len}$\n> for any hash function we choose as the \"impossible\" value, and for\n> that particular use pattern, we do not need such a union. Just\n> letting the caller peek at an appropriate number of bytes at the\n> beginning of that NUL buffer for hash the caller wants to use is\n> sufficient.\n>\n> Thanks.\n\nOnce we have 2 hash functions usable in a local Git installation,\nthis would be wasteful for the smaller hash function (and the\nrelated grafts).\n\nI think Jonathan once envisioned an 'optimized' version as a\nsecond step, maybe this is a good time to discuss how we'd\nget the right size for e.g. allocating memory, as _MAX_ seems\nto be not the correct solution long term?\n"},{"id":"326432","messageId":"xmqqo9rgr8eh.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"ee8edaf08864d5983ff1a5150077d29a4ee17796.1502796628.git.patryk.obara@gmail.com","subject":"Re: [PATCH 5/5] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T18:30:14Z","receivedAt":"2017-08-15T18:30:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> The previous implementation of read_graft_line used calculations based\n> on GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ to determine the number of commit\n> ids in a single graft line. New implementation does not depend on these\n> constants, so it adapts to any object_id buffer size.\n\nI am not sure if this is a good approach. Just like in 2/5 you can\nuse the MAX thing instead of 20, instead of having each graft entry\nallocate a separate oid_array.oid[].\n\nIs this because you expect more than one _kind_ of hashes are used\nat the same time?\n\n"},{"id":"326441","messageId":"xmqqbmngr6wq.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAGZ79kaCA29j6ON4KSsB=EH8FPfZGE56hVGSAAepcPiH952v6g@mail.gmail.com","subject":"Re: [PATCH 2/5] sha1_file: fix hardcoded size in null_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T19:02:29Z","receivedAt":"2017-08-15T19:02:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Once we have 2 hash functions usable in a local Git installation,\n> this would be wasteful for the smaller hash function (and the\n> related grafts).\n>\n> I think Jonathan once envisioned an 'optimized' version as a\n> second step, maybe this is a good time to discuss how we'd\n> get the right size for e.g. allocating memory, as _MAX_ seems\n> to be not the correct solution long term?\n\nMAX is inevitable only if we envision that we have to handle objects\nnamed using two or more hashing schemes at the same time, with the\nsame binary and during the same run inside a single process.\n\nUsing MAX may be nicer even if we use only one hashing scheme at a\ntime, though.\n"},{"id":"326497","messageId":"CAJfL8+R6SK3RGEGXcr5N-btKKjHCUcT95r7oOOsWgY1RXwEEtA@mail.gmail.com","threadId":"46588","inReplyTo":"xmqqbmngr6wq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] sha1_file: fix hardcoded size in null_sha1","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T12:11:19Z","receivedAt":"2017-08-16T12:11:56Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Junio C Hamano wrote:\n\n> I said this is OK for \"null\" because we assume we will use ^\\0{len}$\n> for any hash function we choose as the \"impossible\" value, and for\n> that particular use pattern, we do not need such a union. Just\n> letting the caller peek at an appropriate number of bytes at the\n> beginning of that NUL buffer for hash the caller wants to use is\n> sufficient.\n\nDo you think I should record this explanation as either commit message\nor comment in sha1_file.c?\n\n> MAX is inevitable only if we envision that we have to handle objects\n> named using two or more hashing schemes at the same time, with the\n> same binary and during the same run inside a single process.\n\nI think this will be the case if \"transition one local repository at\na time\" from Jonathan Nieder's transition plan will be followed.\nThis plan assumes object_id translation happening e.g. during fetch\noperation.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326500","messageId":"CAJfL8+QreNvRqVZ0t1Sw=+o4nFK6WuvuOWix_C0MNFik6Cc+rA@mail.gmail.com","threadId":"46588","inReplyTo":"CAGZ79kaCtbuDEUJqJ+nVUW78ksLMyHZ2xhnbunUqzjkGRuYg+A@mail.gmail.com","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T12:24:27Z","receivedAt":"2017-08-16T12:25:02Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"On Tue, Aug 15, 2017 at 7:02 PM, Stefan Beller wrote:\n>> const int entry_size = GIT_SHA1_HEXSZ + 1;\n>\n> outside the scope of this patch:\n> Is GIT_SHA1_HEXSZ or GIT_MAX_HEXSZ the right call here?\n\nI think neither one. In my opinion, this code should not be so closely\ncoupled to hash parsing code - it should be tasked with parsing\nwhitespace separated list of commit ids without relying on specific\ncommit id length or format.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326501","messageId":"CAJfL8+QBO261M-twofEqaXA6i77n91AK_vF7a1JYh_6aith0qw@mail.gmail.com","threadId":"46588","inReplyTo":"xmqqshgsr8kn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] commit: implement free_commit_graft","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T12:28:04Z","receivedAt":"2017-08-16T12:28:40Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Junio C Hamano wrote:\n> I do not see a need to make this new function extern. Shouldn't it\n> be made \"static\" and revert the change to commit.h?\n\nAh, of course :) I was anticipating to find free(graft); in more places\nthroughout code but forgot to get back to it once there was none\nleft.\n\nI will change it in v2.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326506","messageId":"CAJfL8+RVNYh1ryZ5EkiWxiZRo3Loq-MjujD94zGBMEGykmSWeg@mail.gmail.com","threadId":"46588","inReplyTo":"xmqqo9rgr8eh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T13:07:54Z","receivedAt":"2017-08-16T13:08:31Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Junio C Hamano wrote:\n> I am not sure if this is a good approach. Just like in 2/5 you can\n> use the MAX thing instead of 20, instead of having each graft entry\n> allocate a separate oid_array.oid[].\n\nOnce MAX values were increased memory corruption was caused exactly by\nthis line:\n\n> - graft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n\nI could've replaced it by:\n\n graft = xmalloc(st_add(sizeof(*graft), st_mult(sizeof(struct\nobject_id), i)));\n\nBut it seemed to me like short-sighted solution (code might be broken if\nobject_id will be modified again in future).\n\n> Is this because you expect more than one _kind_ of hashes are used\n> at the same time?\n\nYes - I think more than one kind of hashes will be used at the same time\n(meaning in single git binary, not necessarily in a single process), at\nthe very least to allow read access to repositories in \"old\" format(s), with\nsha1-based grafts. Using MAX for parsing here would prevent this,\nrequiring either modifying grafts code again in future or forcing right now\nsome unwelcome design decisions down the line.\n\nMy goal here was to make this code compatible with whatever hash\nfunction transition plan will be chosen.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326514","messageId":"cover.1502905085.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502796628.git.patryk.obara@gmail.com","subject":"[PATCH v2 0/4] Modernize read_graft_line implementation","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T17:58:21Z","receivedAt":"2017-08-16T17:59:20Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Compared to v1:\n- the first patch is dropped to make it easier to merge\n- free_graft is now static function in commit.c\n\nI don't know, what are exact rules about adding Reviewed-by footer, so\nI didn't add any.\n\n\nPatryk Obara (4):\n sha1_file: fix hardcoded size in null_sha1\n commit: replace the raw buffer with strbuf in read_graft_line\n commit: implement free_commit_graft\n commit: rewrite read_graft_line\n\n builtin/blame.c | 2 +-\n commit.c | 55 ++++++++++++++++++++++++++++++++-----------------------\n commit.h | 4 ++--\n sha1_file.c | 2 +-\n shallow.c | 1 +\n 5 files changed, 37 insertions(+), 27 deletions(-)\n\n-- \n2.9.5\n\n"},{"id":"326515","messageId":"bc6dc9fd13dfb6539af994c662eebf2474e731fb.1502905085.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502905085.git.patryk.obara@gmail.com","subject":"[PATCH v2 1/4] sha1_file: fix hardcoded size in null_sha1","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T17:58:22Z","receivedAt":"2017-08-16T17:59:21Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This prevents compilation error if GIT_MAX_RAWSZ is different than 20.\n\nSigned-off-by: Patryk Obara \n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b60ae15..f5b5bec 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -32,7 +32,7 @@\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n-const unsigned char null_sha1[20];\n+const unsigned char null_sha1[GIT_MAX_RAWSZ];\n const struct object_id null_oid;\n const struct object_id empty_tree_oid = {\n \tEMPTY_TREE_SHA1_BIN_LITERAL\n-- \n2.9.5\n\n"},{"id":"326516","messageId":"1aefab7d155cffded7184d7c79ae08d6db721d70.1502905085.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502905085.git.patryk.obara@gmail.com","subject":"[PATCH v2 3/4] commit: implement free_commit_graft","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T17:58:24Z","receivedAt":"2017-08-16T17:59:22Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"In preparation for new graft struct version introduced in next commit.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 499fb14..4d23e72 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -109,15 +109,20 @@ static int commit_graft_pos(const unsigned char *sha1)\n \t\t\tcommit_graft_sha1_access);\n }\n \n+static void free_commit_graft(struct commit_graft *graft)\n+{\n+\tfree(graft);\n+}\n+\n int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n {\n \tint pos = commit_graft_pos(graft->oid.hash);\n \n \tif (0 <= pos) {\n \t\tif (ignore_dups)\n-\t\t\tfree(graft);\n+\t\t\tfree_commit_graft(graft);\n \t\telse {\n-\t\t\tfree(commit_graft[pos]);\n+\t\t\tfree_commit_graft(commit_graft[pos]);\n \t\t\tcommit_graft[pos] = graft;\n \t\t}\n \t\treturn 1;\n@@ -163,7 +168,7 @@ struct commit_graft *read_graft_line(struct strbuf *line)\n \n bad_graft_data:\n \terror(\"bad graft data: %s\", buf);\n-\tfree(graft);\n+\tfree_commit_graft(graft);\n \treturn NULL;\n }\n \n-- \n2.9.5\n\n"},{"id":"326517","messageId":"db36e425b29855e31f11f511a27c04cd8b4a19dd.1502905085.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502905085.git.patryk.obara@gmail.com","subject":"[PATCH v2 2/4] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T17:58:23Z","receivedAt":"2017-08-16T17:59:24Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This simplifies function declaration and allows for use of strbuf_rtrim\ninstead of modifying buffer directly.\n\nSigned-off-by: Patryk Obara \n---\n builtin/blame.c | 2 +-\n commit.c | 11 ++++++-----\n commit.h | 2 +-\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bda1a78..d4472e9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -488,7 +488,7 @@ static int read_ancestry(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (graft)\n \t\t\tregister_commit_graft(graft, 0);\n \t}\ndiff --git a/commit.c b/commit.c\nindex 8b28415..499fb14 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -134,15 +134,16 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \treturn 0;\n }\n \n-struct commit_graft *read_graft_line(char *buf, int len)\n+struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i;\n+\tint i, len;\n+\tchar *buf = line->buf;\n \tstruct commit_graft *graft = NULL;\n \tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n \n-\twhile (len && isspace(buf[len-1]))\n-\t\tbuf[--len] = '\\0';\n+\tstrbuf_rtrim(line);\n+\tlen = line->len;\n \tif (buf[0] == '#' || buf[0] == '\\0')\n \t\treturn NULL;\n \tif ((len + 1) % entry_size)\n@@ -174,7 +175,7 @@ static int read_graft_file(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (!graft)\n \t\t\tcontinue;\n \t\tif (register_commit_graft(graft, 1))\ndiff --git a/commit.h b/commit.h\nindex 6d857f0..baecc0a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -247,7 +247,7 @@ struct commit_graft {\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n-struct commit_graft *read_graft_line(char *buf, int len);\n+struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n \n-- \n2.9.5\n\n"},{"id":"326518","messageId":"b63b2148b7d79ebe5c57b876c7077a9ac42d2869.1502905085.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502905085.git.patryk.obara@gmail.com","subject":"[PATCH v2 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-16T17:58:25Z","receivedAt":"2017-08-16T17:59:25Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"The previous implementation of read_graft_line used calculations based\non GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ to determine the number of commit\nids in a single graft line. New implementation does not depend on these\nconstants, so it adapts to any object_id buffer size.\n\nTo make this possible, FLEX_ARRAY of object_id in struct was replaced\nby an oid_array.\n\nCode allocating graft now needs to use memset to zero the memory before\nuse to start with oid_array in a consistent state.\n\nUpdates free_graft function implemented in the previous patch to\nproperly cleanup an oid_array storing parents.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 39 +++++++++++++++++++++------------------\n commit.h | 2 +-\n shallow.c | 1 +\n 3 files changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 4d23e72..8bdce36 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -111,6 +111,7 @@ static int commit_graft_pos(const unsigned char *sha1)\n \n static void free_commit_graft(struct commit_graft *graft)\n {\n+\toid_array_clear(&graft->parents);\n \tfree(graft);\n }\n \n@@ -139,35 +140,37 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \treturn 0;\n }\n \n+static int parse_next_oid_hex(const char *buf, struct object_id *oid, const char **end)\n+{\n+\twhile (isspace(buf[0]))\n+\t\tbuf++;\n+\treturn parse_oid_hex(buf, oid, end);\n+}\n+\n struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i, len;\n-\tchar *buf = line->buf;\n \tstruct commit_graft *graft = NULL;\n-\tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n+\tstruct object_id oid;\n+\tconst char *tail = NULL;\n \n \tstrbuf_rtrim(line);\n-\tlen = line->len;\n-\tif (buf[0] == '#' || buf[0] == '\\0')\n+\tif (line->buf[0] == '#' || line->len == 0)\n \t\treturn NULL;\n-\tif ((len + 1) % entry_size)\n+\tgraft = xmalloc(sizeof(*graft));\n+\tmemset(graft, 0, sizeof(*graft));\n+\tif (parse_oid_hex(line->buf, &graft->oid, &tail))\n \t\tgoto bad_graft_data;\n-\ti = (len + 1) / entry_size - 1;\n-\tgraft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n-\tgraft->nr_parent = i;\n-\tif (get_oid_hex(buf, &graft->oid))\n+\twhile (!parse_next_oid_hex(tail, &oid, &tail))\n+\t\toid_array_append(&graft->parents, &oid);\n+\tif (tail[0] != '\\0')\n \t\tgoto bad_graft_data;\n-\tfor (i = GIT_SHA1_HEXSZ; i < len; i += entry_size) {\n-\t\tif (buf[i] != ' ')\n-\t\t\tgoto bad_graft_data;\n-\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/entry_size].hash))\n-\t\t\tgoto bad_graft_data;\n-\t}\n+\tgraft->nr_parent = graft->parents.nr;\n+\n \treturn graft;\n \n bad_graft_data:\n-\terror(\"bad graft data: %s\", buf);\n+\terror(\"bad graft data: %s\", line->buf);\n \tfree_commit_graft(graft);\n \treturn NULL;\n }\n@@ -363,7 +366,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->parents.oid[i]);\n \t\t\tif (!new_parent)\n \t\t\t\tcontinue;\n \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\ndiff --git a/commit.h b/commit.h\nindex baecc0a..96ff375 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -243,7 +243,7 @@ void sort_in_topological_order(struct commit_list **, enum rev_sort_order);\n struct commit_graft {\n \tstruct object_id oid;\n \tint nr_parent; /* < 0 if shallow commit */\n-\tstruct object_id parent[FLEX_ARRAY]; /* more */\n+\tstruct oid_array parents;\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \ndiff --git a/shallow.c b/shallow.c\nindex f5591e5..892cd90 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -33,6 +33,7 @@ int register_shallow(const struct object_id *oid)\n \t\txmalloc(sizeof(struct commit_graft));\n \tstruct commit *commit = lookup_commit(oid);\n \n+\tmemset(graft, 0, sizeof(*graft));\n \toidcpy(&graft->oid, oid);\n \tgraft->nr_parent = -1;\n \tif (commit && commit->object.parsed)\n-- \n2.9.5\n\n"},{"id":"326532","messageId":"xmqqr2wbjol8.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAJfL8+R6SK3RGEGXcr5N-btKKjHCUcT95r7oOOsWgY1RXwEEtA@mail.gmail.com","subject":"Re: [PATCH 2/5] sha1_file: fix hardcoded size in null_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-16T19:32:19Z","receivedAt":"2017-08-16T19:32:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Junio C Hamano wrote:\n>\n>> I said this is OK for \"null\" because we assume we will use ^\\0{len}$\n>> for any hash function we choose as the \"impossible\" value, and for\n>> that particular use pattern, we do not need such a union. Just\n>> letting the caller peek at an appropriate number of bytes at the\n>> beginning of that NUL buffer for hash the caller wants to use is\n>> sufficient.\n>\n> Do you think I should record this explanation as either commit message\n> or comment in sha1_file.c?\n>\n>> MAX is inevitable only if we envision that we have to handle objects\n>> named using two or more hashing schemes at the same time, with the\n>> same binary and during the same run inside a single process.\n>\n> I think this will be the case if \"transition one local repository at\n> a time\" from Jonathan Nieder's transition plan will be followed.\n> This plan assumes object_id translation happening e.g. during fetch\n> operation.\n\nIt would be good if that assumption is made explicit.\n\nThanks.\n"},{"id":"326533","messageId":"xmqqmv6zjo9x.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAJfL8+RVNYh1ryZ5EkiWxiZRo3Loq-MjujD94zGBMEGykmSWeg@mail.gmail.com","subject":"Re: [PATCH 5/5] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-16T19:39:06Z","receivedAt":"2017-08-16T19:39:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Junio C Hamano wrote:\n>> I am not sure if this is a good approach. Just like in 2/5 you can\n>> use the MAX thing instead of 20, instead of having each graft entry\n>> allocate a separate oid_array.oid[].\n>\n> Once MAX values were increased memory corruption was caused exactly by\n> this line:\n>\n>> - graft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n>\n> I could've replaced it by:\n>\n> graft = xmalloc(st_add(sizeof(*graft), st_mult(sizeof(struct\n> object_id), i)));\n>\n> But it seemed to me like short-sighted solution (code might be broken if\n> object_id will be modified again in future).\n\nWhy? \n\nAssuming that \"struct object_id\" would have to be prepared to handle\nmore than one types of hash functions at the same time, it would be\nsufficiently large to hold any type of hash, plus possibly another\nmember that tells which kind.\n\nThe decision to choose between a flex array and a separately\nallocatable structure primarily should come from how the enclosing\nstruct (i.e. the commti_graft structure) is meant to be used. It is\nnot meant to be tweaked by adding more parents or removing parents\nonce it is constructed, which argues for having a flex array to hold\nthe known and fixed number of parents once it is constructed.\n"},{"id":"326534","messageId":"xmqqinhnjnxr.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"bc6dc9fd13dfb6539af994c662eebf2474e731fb.1502905085.git.patryk.obara@gmail.com","subject":"Re: [PATCH v2 1/4] sha1_file: fix hardcoded size in null_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-16T19:46:24Z","receivedAt":"2017-08-16T19:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> This prevents compilation error if GIT_MAX_RAWSZ is different than 20.\n\nThe above made me scratch my head wondering why because it does not\nsay what the root cause of the issue is. It would have avoided a\nfew strand of lost hair if it were more like this, perhaps:\n\n\tThe array is declared in cache.h as\n\n\t extern const unsigned char null_sha1[GIT_MAX_RAWSZ];\n\n\tThe definition of it we have in sha1_file.c must match.\n\n\n>\n> Signed-off-by: Patryk Obara \n> ---\n> sha1_file.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index b60ae15..f5b5bec 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -32,7 +32,7 @@\n> #define SZ_FMT PRIuMAX\n> static inline uintmax_t sz_fmt(size_t s) { return s; }\n> \n> -const unsigned char null_sha1[20];\n> +const unsigned char null_sha1[GIT_MAX_RAWSZ];\n> const struct object_id null_oid;\n> const struct object_id empty_tree_oid = {\n> \tEMPTY_TREE_SHA1_BIN_LITERAL\n"},{"id":"326568","messageId":"20170816225901.dbpzvsie2zgetunu@genre.crustytoothpaste.net","threadId":"46588","inReplyTo":"CAJfL8+QreNvRqVZ0t1Sw=+o4nFK6WuvuOWix_C0MNFik6Cc+rA@mail.gmail.com","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-08-16T22:59:02Z","receivedAt":"2017-08-16T22:59:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Aug 16, 2017 at 02:24:27PM +0200, Patryk Obara wrote:\n> On Tue, Aug 15, 2017 at 7:02 PM, Stefan Beller wrote:\n> >> const int entry_size = GIT_SHA1_HEXSZ + 1;\n> >\n> > outside the scope of this patch:\n> > Is GIT_SHA1_HEXSZ or GIT_MAX_HEXSZ the right call here?\n> \n> I think neither one. In my opinion, this code should not be so closely\n> coupled to hash parsing code - it should be tasked with parsing\n> whitespace separated list of commit ids without relying on specific\n> commit id length or format.\n\nWhat I had intended, although maybe I have not explained this well, was\nthat we would have one binary that set up hash functionality as part of\nearly setup. GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ would turn into\nsomething like current_hash->rawsz and current_hash->hexsz at that\npoint. The reason I introduced the GIT_MAX constants was to allocate\nmemory suitable for whatever hash we picked.\n\nHowever, this is only what I had considered for design, and others might\nhave different views going forward. I have, however, based my patches\non that assumption, and responded to others' comments with those\nstatements.\n\nI agree that ideally we should make as much of the code as possible\nignorant of the hash size, because that will generally result in more\nrobust, less brittle code. I've noticed in this series the use of\nparse_oid_hex, and I agree that's one tool we can use to accomplish that\ngoal.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"326578","messageId":"20170817055516.4zz3ucvx4mgr6qus@sigill.intra.peff.net","threadId":"46588","inReplyTo":"20170816225901.dbpzvsie2zgetunu@genre.crustytoothpaste.net","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T05:55:16Z","receivedAt":"2017-08-17T05:55:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 16, 2017 at 10:59:02PM +0000, brian m. carlson wrote:\n\n> On Wed, Aug 16, 2017 at 02:24:27PM +0200, Patryk Obara wrote:\n> > On Tue, Aug 15, 2017 at 7:02 PM, Stefan Beller wrote:\n> > >> const int entry_size = GIT_SHA1_HEXSZ + 1;\n> > >\n> > > outside the scope of this patch:\n> > > Is GIT_SHA1_HEXSZ or GIT_MAX_HEXSZ the right call here?\n> > \n> > I think neither one. In my opinion, this code should not be so closely\n> > coupled to hash parsing code - it should be tasked with parsing\n> > whitespace separated list of commit ids without relying on specific\n> > commit id length or format.\n> \n> What I had intended, although maybe I have not explained this well, was\n> that we would have one binary that set up hash functionality as part of\n> early setup. GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ would turn into\n> something like current_hash->rawsz and current_hash->hexsz at that\n> point. The reason I introduced the GIT_MAX constants was to allocate\n> memory suitable for whatever hash we picked.\n> \n> However, this is only what I had considered for design, and others might\n> have different views going forward. I have, however, based my patches\n> on that assumption, and responded to others' comments with those\n> statements.\n\nWhat you wrote here matches my understanding of the general plan. IOW,\nwe'd expect to \"waste\" 12 bytes when dealing with a 160-bit sha1 in a\nGit binary that's aware of 256-bit hashes. But that seems like a small\nprice to pay to be able to continue using automatic allocations, versus\nrewriting each site to call xmalloc(current_hash->rawsz).\n\nI'd expect most of the GIT_MAX constants to eventually go away in favor\nof \"struct object_id\", but that will still be using the same \"big enough\nto hold any hash\" size under the hood.\n\n> I agree that ideally we should make as much of the code as possible\n> ignorant of the hash size, because that will generally result in more\n> robust, less brittle code. I've noticed in this series the use of\n> parse_oid_hex, and I agree that's one tool we can use to accomplish that\n> goal.\n\nAgreed. Most code should be dealing with the abstract concept of a hash\nand shouldn't have to care about the size. I really like parse_oid_hex()\nfor that reason (and I think parsing is the main place we've found that\nneeds to care).\n\n-Peff\n"},{"id":"326642","messageId":"xmqq7ey1gai3.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"20170817055516.4zz3ucvx4mgr6qus@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-17T21:17:08Z","receivedAt":"2017-08-17T21:17:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd expect most of the GIT_MAX constants to eventually go away in favor\n> of \"struct object_id\", but that will still be using the same \"big enough\n> to hold any hash\" size under the hood.\n\nIndeed. It is good to see major contributors are in agreement ;-)\nI'd expect that an array of \"struct object_id\" would be how a fixed\nnumber of object names would be represented, i.e.\n\n\tstruct object_id thing[num_elements];\n\ninstead of an array of uchar that is MAX bytes long, i.e.\n\n\tunsigned char name[GIT_MAX_RAWSZ][num_elements];\n\nIn fact, the former is already how we represent the list of fake\nparents in the commit_graft structure, so I think patch 5/5 in this\nseries does two unrelated things, one of which is bad (i.e. use of\nparse_oid_hex() is good; turning the FLEX_ARRAY at the end into a\noid_array that requires a separate allocation of the array is bad).\n\n> Agreed. Most code should be dealing with the abstract concept of a hash\n> and shouldn't have to care about the size. I really like parse_oid_hex()\n> for that reason (and I think parsing is the main place we've found that\n> needs to care).\n\nYes.\n"},{"id":"326643","messageId":"xmqq378pgac1.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"b63b2148b7d79ebe5c57b876c7077a9ac42d2869.1502905085.git.patryk.obara@gmail.com","subject":"Re: [PATCH v2 4/4] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-17T21:20:46Z","receivedAt":"2017-08-17T21:20:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> The previous implementation of read_graft_line used calculations based\n> on GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ to determine the number of commit\n> ids in a single graft line. New implementation does not depend on these\n> constants, so it adapts to any object_id buffer size.\n>\n> To make this possible, FLEX_ARRAY of object_id in struct was replaced\n> by an oid_array.\n\nThere is a leap in logic between the two paragraphs. Your use of\nparse_oid_hex() is good. But I do not think moving the array body\nto outside commit_graft structure and forcing it to be separately\nallocated is necessary or beneficial. When we got a single line, we\nknow how many fake parents a child described by that graft line has,\nand you can still use of FLEX_ARRAY to avoid separate allocation\nand need for separate freeing of it.\n"},{"id":"326650","messageId":"CAJfL8+T0sC9TnYfdut1nqiE9e2misnjK6X0WFDkj-mPcuKB4Tw@mail.gmail.com","threadId":"46588","inReplyTo":"xmqq7ey1gai3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/5] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-17T21:38:56Z","receivedAt":"2017-08-17T21:39:35Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"> In fact, the former is already how we represent the list of fake\n> parents in the commit_graft structure, so I think patch 5/5 in this\n> series does two unrelated things, one of which is bad (i.e. use of\n> parse_oid_hex() is good; turning the FLEX_ARRAY at the end into a\n> oid_array that requires a separate allocation of the array is bad).\n\nAgreed; I already split patch 5 into two separate changes (one fixing\nmemory allocation issue, one parsing object_ids into FLEX_ARRAY,\nwithout modifying graft struct). In result patch 4 (free_graft) can\nbe dropped. I will send these changes as v3.\n\n\nOn Thu, Aug 17, 2017 at 11:17 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I'd expect most of the GIT_MAX constants to eventually go away in favor\n>> of \"struct object_id\", but that will still be using the same \"big enough\n>> to hold any hash\" size under the hood.\n>\n> Indeed. It is good to see major contributors are in agreement ;-)\n> I'd expect that an array of \"struct object_id\" would be how a fixed\n> number of object names would be represented, i.e.\n>\n> struct object_id thing[num_elements];\n>\n> instead of an array of uchar that is MAX bytes long, i.e.\n>\n> unsigned char name[GIT_MAX_RAWSZ][num_elements];\n>\n> In fact, the former is already how we represent the list of fake\n> parents in the commit_graft structure, so I think patch 5/5 in this\n> series does two unrelated things, one of which is bad (i.e. use of\n> parse_oid_hex() is good; turning the FLEX_ARRAY at the end into a\n> oid_array that requires a separate allocation of the array is bad).\n>\n>> Agreed. Most code should be dealing with the abstract concept of a hash\n>> and shouldn't have to care about the size. I really like parse_oid_hex()\n>> for that reason (and I think parsing is the main place we've found that\n>> needs to care).\n>\n> Yes.\n\n\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326652","messageId":"CAJfL8+SMHQTH4Y9TFYww2WRgSfag5+sXvKniA40Qb0UEwUkrLw@mail.gmail.com","threadId":"46588","inReplyTo":"xmqq378pgac1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-17T21:42:15Z","receivedAt":"2017-08-17T21:42:51Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Understood :) - yes, oid_array is not completely necessary here - and it\ngives the wrong impression about usage of this struct.\n\nFLEX_ARRAY will be brought back in v3.\n\nOn Thu, Aug 17, 2017 at 11:20 PM, Junio C Hamano wrote:\n> Patryk Obara writes:\n>\n>> The previous implementation of read_graft_line used calculations based\n>> on GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ to determine the number of commit\n>> ids in a single graft line. New implementation does not depend on these\n>> constants, so it adapts to any object_id buffer size.\n>>\n>> To make this possible, FLEX_ARRAY of object_id in struct was replaced\n>> by an oid_array.\n>\n> There is a leap in logic between the two paragraphs. Your use of\n> parse_oid_hex() is good. But I do not think moving the array body\n> to outside commit_graft structure and forcing it to be separately\n> allocated is necessary or beneficial. When we got a single line, we\n> know how many fake parents a child described by that graft line has,\n> and you can still use of FLEX_ARRAY to avoid separate allocation\n> and need for separate freeing of it.\n\n\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326661","messageId":"cover.1503020338.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1502905085.git.patryk.obara@gmail.com","subject":"[PATCH v3 0/4] Modernize read_graft_line implementation","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T01:59:34Z","receivedAt":"2017-08-18T01:59:47Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Changes since v2:\n- commit implementing free_graft dropped (no longer needed)\n- several more lines moved from last commit to commit replacing\n raw buffer with strbuf\n- fix for memory allocation separated from change in hash\n parsing\n- commit_graft struct uses FLEX_ARRAY again, meaning free_graft,\n memset, nor oid_array were not needed after all\n\nPatryk Obara (4):\n sha1_file: fix definition of null_sha1\n commit: replace the raw buffer with strbuf in read_graft_line\n commit: allocate array using object_id size\n commit: rewrite read_graft_line\n\n builtin/blame.c | 2 +-\n commit.c | 38 +++++++++++++++++++-------------------\n commit.h | 2 +-\n sha1_file.c | 2 +-\n 4 files changed, 22 insertions(+), 22 deletions(-)\n\n-- \n2.9.5\n\nbase-commit: b3622a4ee94e4916cd05e6d96e41eeb36b941182\n"},{"id":"326662","messageId":"65f84c5eb94e8b6f5cbce31f56810fdb71a58bf9.1503020338.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503020338.git.patryk.obara@gmail.com","subject":"[PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T01:59:36Z","receivedAt":"2017-08-18T01:59:52Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This simplifies function declaration and allows for use of strbuf_rtrim\ninstead of modifying buffer directly.\n\nSigned-off-by: Patryk Obara \n---\n builtin/blame.c | 2 +-\n commit.c | 15 ++++++++-------\n commit.h | 2 +-\n 3 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bda1a78..d4472e9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -488,7 +488,7 @@ static int read_ancestry(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (graft)\n \t\t\tregister_commit_graft(graft, 0);\n \t}\ndiff --git a/commit.c b/commit.c\nindex 8b28415..019e733 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -134,17 +134,18 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \treturn 0;\n }\n \n-struct commit_graft *read_graft_line(char *buf, int len)\n+struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i;\n+\tint i, len;\n+\tchar *buf = line->buf;\n \tstruct commit_graft *graft = NULL;\n \tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n \n-\twhile (len && isspace(buf[len-1]))\n-\t\tbuf[--len] = '\\0';\n-\tif (buf[0] == '#' || buf[0] == '\\0')\n+\tstrbuf_rtrim(line);\n+\tif (line->buf[0] == '#' || line->len == 0)\n \t\treturn NULL;\n+\tlen = line->len;\n \tif ((len + 1) % entry_size)\n \t\tgoto bad_graft_data;\n \ti = (len + 1) / entry_size - 1;\n@@ -161,7 +162,7 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \treturn graft;\n \n bad_graft_data:\n-\terror(\"bad graft data: %s\", buf);\n+\terror(\"bad graft data: %s\", line->buf);\n \tfree(graft);\n \treturn NULL;\n }\n@@ -174,7 +175,7 @@ static int read_graft_file(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (!graft)\n \t\t\tcontinue;\n \t\tif (register_commit_graft(graft, 1))\ndiff --git a/commit.h b/commit.h\nindex 6d857f0..baecc0a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -247,7 +247,7 @@ struct commit_graft {\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n-struct commit_graft *read_graft_line(char *buf, int len);\n+struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n \n-- \n2.9.5\n\n"},{"id":"326663","messageId":"08a1c3de7d35243c36296d88ed17e8307aa9fff2.1503020338.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503020338.git.patryk.obara@gmail.com","subject":"[PATCH v3 1/4] sha1_file: fix definition of null_sha1","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T01:59:35Z","receivedAt":"2017-08-18T01:59:54Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"The array is declared in cache.h as:\n\n extern const unsigned char null_sha1[GIT_MAX_RAWSZ];\n\nDefinition in sha1_file.c must match.\n\nSigned-off-by: Patryk Obara \n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b60ae15..f5b5bec 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -32,7 +32,7 @@\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n-const unsigned char null_sha1[20];\n+const unsigned char null_sha1[GIT_MAX_RAWSZ];\n const struct object_id null_oid;\n const struct object_id empty_tree_oid = {\n \tEMPTY_TREE_SHA1_BIN_LITERAL\n-- \n2.9.5\n\n"},{"id":"326664","messageId":"cb98970b3f6c175321f52efb65deb48f9cfeabae.1503020338.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503020338.git.patryk.obara@gmail.com","subject":"[PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T01:59:38Z","receivedAt":"2017-08-18T01:59:55Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Determine the number of object_id's to parse in a single graft line by\ncounting separators (whitespace characters) instead of dividing by\nlength of hash representation.\n\nThis way graft parsing code can support different sizes of hashes\nwithout any further code adaptations.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 28 +++++++++++++---------------\n 1 file changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 61528a5..46ee2db 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -137,29 +137,27 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i, len;\n-\tchar *buf = line->buf;\n+\tint i, n;\n+\tconst char *tail = NULL;\n \tstruct commit_graft *graft = NULL;\n-\tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n \n \tstrbuf_rtrim(line);\n \tif (line->buf[0] == '#' || line->len == 0)\n \t\treturn NULL;\n-\tlen = line->len;\n-\tif ((len + 1) % entry_size)\n-\t\tgoto bad_graft_data;\n-\ti = (len + 1) / entry_size - 1;\n+\t/* count number of blanks to determine size of array to allocate */\n+\tfor (i = 0, n = 0; i < line->len; i++)\n+\t\tif (isspace(line->buf[i]))\n+\t\t\tn++;\n \tgraft = xmalloc(st_add(sizeof(*graft),\n-\t st_mult(sizeof(struct object_id), i)));\n-\tgraft->nr_parent = i;\n-\tif (get_oid_hex(buf, &graft->oid))\n+\t st_mult(sizeof(struct object_id), n)));\n+\tgraft->nr_parent = n;\n+\tif (parse_oid_hex(line->buf, &graft->oid, &tail))\n \t\tgoto bad_graft_data;\n-\tfor (i = GIT_SHA1_HEXSZ; i < len; i += entry_size) {\n-\t\tif (buf[i] != ' ')\n-\t\t\tgoto bad_graft_data;\n-\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/entry_size].hash))\n+\tfor (i = 0; i < graft->nr_parent; i++)\n+\t\tif (!isspace(*tail++) || parse_oid_hex(tail, &graft->parent[i], &tail))\n \t\t\tgoto bad_graft_data;\n-\t}\n+\tif (tail[0] != '\\0')\n+\t\tgoto bad_graft_data;\n \treturn graft;\n \n bad_graft_data:\n-- \n2.9.5\n\n"},{"id":"326665","messageId":"26cb2ccae6c5b66e55dc3868e47b2f1473acae07.1503020338.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503020338.git.patryk.obara@gmail.com","subject":"[PATCH v3 3/4] commit: allocate array using object_id size","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T01:59:37Z","receivedAt":"2017-08-18T01:59:56Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"struct commit_graft aggregates an array of object_id's, which have\nsize >= GIT_MAX_RAWSZ bytes. This change prevents memory allocation\nerror when size of object_id is larger than GIT_SHA1_RAWSZ.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 019e733..61528a5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -149,7 +149,8 @@ struct commit_graft *read_graft_line(struct strbuf *line)\n \tif ((len + 1) % entry_size)\n \t\tgoto bad_graft_data;\n \ti = (len + 1) / entry_size - 1;\n-\tgraft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n+\tgraft = xmalloc(st_add(sizeof(*graft),\n+\t st_mult(sizeof(struct object_id), i)));\n \tgraft->nr_parent = i;\n \tif (get_oid_hex(buf, &graft->oid))\n \t\tgoto bad_graft_data;\n-- \n2.9.5\n\n"},{"id":"326667","messageId":"xmqq8tihehvr.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"cover.1503020338.git.patryk.obara@gmail.com","subject":"Re: [PATCH v3 0/4] Modernize read_graft_line implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T02:20:40Z","receivedAt":"2017-08-18T02:20:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will queue.\n"},{"id":"326674","messageId":"20170818062929.f4zitbtaeii4xiko@sigill.intra.peff.net","threadId":"46588","inReplyTo":"65f84c5eb94e8b6f5cbce31f56810fdb71a58bf9.1503020338.git.patryk.obara@gmail.com","subject":"Re: [PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-18T06:29:30Z","receivedAt":"2017-08-18T06:29:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2017 at 03:59:36AM +0200, Patryk Obara wrote:\n\n> diff --git a/commit.c b/commit.c\n> index 8b28415..019e733 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -134,17 +134,18 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n> \treturn 0;\n> }\n> \n> -struct commit_graft *read_graft_line(char *buf, int len)\n> +struct commit_graft *read_graft_line(struct strbuf *line)\n> {\n> \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n> -\tint i;\n> +\tint i, len;\n> +\tchar *buf = line->buf;\n\nCopying a pointer to a strbuf's buffer is a dangerous habit. The strbuf\nis free to re-allocate the buffer under the hood during any operation it\nlikes, potentially leaving you pointing to freed memory.\n\nIn this case it's OK because the only function you call is\nstrbuf_rtrim(), which never reallocates. But I feel like this is setting\nup a maintenance trap for the next person to touch the function.\n\nAFAICT this is only here to avoid having to s/buf/line->buf/ in the rest\nof the function. But I think we should just make that change (you\nalready did in some of the spots). And IMHO we should do the same for\nline->len. When there are two names for the same value, it increases the\nchances of a bug where the two end up diverging.\n\n> -\twhile (len && isspace(buf[len-1]))\n> -\t\tbuf[--len] = '\\0';\n> -\tif (buf[0] == '#' || buf[0] == '\\0')\n> +\tstrbuf_rtrim(line);\n> +\tif (line->buf[0] == '#' || line->len == 0)\n> \t\treturn NULL;\n\nI find it funny to look at line->buf[0] before line->len, because it\nmeans we're reading pas the end of the buffer. It's OK here because we\nknow there's a NUL terminator, but I think short-circuiting like:\n\n if (!line->len || line->buf[0] == '#')\n\nis better (I also think \"!\" instead of \"== 0\" is our usual style, but\nthat's much less important).\n\n-Peff\n"},{"id":"326675","messageId":"20170818064335.h5sr5iz7mh64axji@sigill.intra.peff.net","threadId":"46588","inReplyTo":"cb98970b3f6c175321f52efb65deb48f9cfeabae.1503020338.git.patryk.obara@gmail.com","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-18T06:43:36Z","receivedAt":"2017-08-18T06:43:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2017 at 03:59:38AM +0200, Patryk Obara wrote:\n\n> Determine the number of object_id's to parse in a single graft line by\n> counting separators (whitespace characters) instead of dividing by\n> length of hash representation.\n> \n> This way graft parsing code can support different sizes of hashes\n> without any further code adaptations.\n\nSounds like a reasonable approach, though I wonder what happens if our\ncounting pass differs in its behavior from the actual parse.\n\nE.g., here:\n\n> +\t/* count number of blanks to determine size of array to allocate */\n> +\tfor (i = 0, n = 0; i < line->len; i++)\n> +\t\tif (isspace(line->buf[i]))\n> +\t\t\tn++;\n\nIf we see multiple spaces like \"1234abcd 5678abcd\" we'll allocate a\nslot for each. I think that's OK because here:\n\n> +\tfor (i = 0; i < graft->nr_parent; i++)\n> +\t\tif (!isspace(*tail++) || parse_oid_hex(tail, &graft->parent[i], &tail))\n> \t\t\tgoto bad_graft_data;\n\nWe'd reject such an input totally (though as an interesting side effect,\nyou can convince the parser to allocate 20x as much RAM as you send it;\none oid for each space).\n\nSo we're probably fine. The two parsing passes are right next to each\nother and are sufficiently simple and strict that we don't have to\nworry about them diverging.\n\nThe single-pass alternative would probably be to read into a dynamic\nstructure like an oid_array, and then copy the result into the flex\nstructure.\n\nOr of course to stop using a flex structure, as your original pass did.\nI agree with Junio that the use of object_id's is orthogonal to using a\nFLEX_ARRAY. But I could also see an argument that the complexity the\nflex array adds here isn't worth the savings. The main benefits of a\nflex array are:\n\n 1. Less memory used. But we don't expect to see a large enough number\n of grafts for this to matter.\n\n 2. A more compact memory representation, which can be faster. But\n accessing the parent list of a graft isn't going to be the hot code\n path. It probably only happens once in a program run when we\n rewrite the parents (versus checking the grafted commit, which is\n looked up once per commit we access).\n\n 3. It's easier to free the struct and its associated resources in a\n single free(). But we never free the graft list.\n\nReading your original, my thought was \"why _not_ keep doing it as a\nFLEX_ARRAY, as it saves a little memory, which can't hurt\". But seeing\nthis pre-counting phase, I think it does make the code a little more\ncomplicated. But I'd be OK with doing it either way.\n\n-Peff\n"},{"id":"326680","messageId":"xmqqziaxcobp.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"20170818064335.h5sr5iz7mh64axji@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T07:44:26Z","receivedAt":"2017-08-18T07:44:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So we're probably fine. The two parsing passes are right next to each\n> other and are sufficiently simple and strict that we don't have to\n> worry about them diverging.\n\nIf I were doing the two-pass thing, I'd probably write a for loop\nthat runs exactly twice, where the first iteration parses into a\nsingle throw-away oid struct only to count, and the second iteration\nparses the same input into the allocated array of oid struct. That\nway, you do not have to worry about two phrases going out of sync.\n"},{"id":"326683","messageId":"CAJfL8+TRZQrfF9Y9PdBZTptEf_O9u9irVRzb0bBVAnTRga2xmw@mail.gmail.com","threadId":"46588","inReplyTo":"20170818062929.f4zitbtaeii4xiko@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T10:12:37Z","receivedAt":"2017-08-18T10:13:14Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Jeff King wrote:\n\n> AFAICT this is only here to avoid having to s/buf/line->buf/ in the rest\n> of the function. But I think we should just make that change (you\n> already did in some of the spots). And IMHO we should do the same for\n> line->len. When there are two names for the same value, it increases the\n> chances of a bug where the two end up diverging.\n\nMy motivation was rather to keep patch(es) as small as possible because every\nline using buf will be replaced in a later patch in series. But it will make\ncommit better (it will stand on its own), so why not to do it? :)\n\n> (…) I think short-circuiting like:\n>\n> if (!line->len || line->buf[0] == '#')\n>\n> is better (I also think \"!\" instead of \"== 0\" is our usual style, but\n> that's much less important).\n\nAh, I only replaced comparison to NULL terminator with length check because\nI thought it better shows intention of the code and I didn't notice, that\nreversing order will result in better code overall.\n\nI will include both changes in v4.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326684","messageId":"CAJfL8+SHSAhgrMY6ONVHLMWEHcT0mhm4oKMmq6D=89SErDKiMA@mail.gmail.com","threadId":"46588","inReplyTo":"xmqqziaxcobp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T11:30:23Z","receivedAt":"2017-08-18T11:31:01Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Jeff King wrote:\n>\n> So we're probably fine. The two parsing passes are right next to each\n> other and are sufficiently simple and strict that we don't have to\n> worry about them diverging.\n\nThat was my conclusion as well. I added comment before the first pass and\navoided any \"cleverness\" to make it perfectly clear to a reader.\n\n> We'd reject such an input totally (though as an interesting side effect,\n> you can convince the parser to allocate 20x as much RAM as you send it;\n> one oid for each space).\n\nGrafts are not populated during clone operation, so it really would be user\nmaking his life miserable. I could allocate FLEXI_ARRAY of size\nmin(n, line->len / (GIT_*MIN*_HEXSZ+1)) instead… but I think it's not even\nworth the cost of making the code more complicated (and I don't want\nto reintroduce these size macros in here.\n\nWe _could_ put an artificial limit on graft parents, though (e.g. 10) and\ndisplay an error message urging the user to stop using grafts?\n\n> The single-pass alternative would probably be to read into a dynamic\n> structure like an oid_array, and then copy the result into the flex\n> structure.\n\nBefore sending v3 I tried two other alternative implementations (perhaps I\nshould've listed them in the v3 cover letter):\n\n 1. Using string_list_split_in_place. I resigned from this approach as soon\n as I noticed, that line->buf needs to be preserved for possible\n error message. string_list_split would have no benefits over using\n oid_array.\n\n 2. Parsing into temporary oid_array and then copying memory to FLEXI_ARRAY.\n Throw-away oid_array still needs to be cleaned, which means we have\n new/different return path (one before xmalloc and one after xmalloc),\n which means \"bad_graft_data\" label needs to be changed into \"cleanup\"\n label (or removed), which means error description needs be conditionally\n put in earlier code… and at this point, I decided these changes are not\n making code cleaner nor more readable at all :)\n\nJunio C Hamano wrote:\n>\n> If I were doing the two-pass thing, I'd probably write a for loop\n> that runs exactly twice, where the first iteration parses into a\n> single throw-away oid struct only to count, and the second iteration\n> parses the same input into the allocated array of oid struct. That\n> way, you do not have to worry about two phrases going out of sync.\n\nTwo passes would still differ in error handling due to xmalloc between them…\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326685","messageId":"20170818114530.hiks5iiljtxeyrha@sigill.intra.peff.net","threadId":"46588","inReplyTo":"CAJfL8+SHSAhgrMY6ONVHLMWEHcT0mhm4oKMmq6D=89SErDKiMA@mail.gmail.com","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-18T11:45:30Z","receivedAt":"2017-08-18T11:45:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2017 at 01:30:23PM +0200, Patryk Obara wrote:\n\n> > We'd reject such an input totally (though as an interesting side effect,\n> > you can convince the parser to allocate 20x as much RAM as you send it;\n> > one oid for each space).\n> \n> Grafts are not populated during clone operation, so it really would be user\n> making his life miserable. I could allocate FLEXI_ARRAY of size\n> min(n, line->len / (GIT_*MIN*_HEXSZ+1)) instead… but I think it's not even\n> worth the cost of making the code more complicated (and I don't want\n> to reintroduce these size macros in here.\n> \n> We _could_ put an artificial limit on graft parents, though (e.g. 10) and\n> display an error message urging the user to stop using grafts?\n\nYeah, sorry, I should have made more clear that this is fine. I always\ntry to read parsing code with my paranoid hat on, but I agree that\ngrafts aren't really exposed to untrusted entities.\n\nIn general I'd prefer to avoid artificial limits unless there's a need\nfor them. There are already spots (like receive-pack) where you can ask\nGit to store bytes in RAM as fast as you can send them. What I found\ninteresting about this one was the 20:1 amplification. :)\n\n> Before sending v3 I tried two other alternative implementations (perhaps I\n> should've listed them in the v3 cover letter):\n\nIt might even be worth listing them in the commit message. Somebody\nfinding your commit 3 years from via \"git log -S\" or \"git blame\" might\nsay \"yes, but why didn't they just do it like...\". You can respond to\nthem preemptively. :)\n\n-Peff\n"},{"id":"326686","messageId":"20170818115017.cji4r5nrdetumtqo@sigill.intra.peff.net","threadId":"46588","inReplyTo":"CAJfL8+TRZQrfF9Y9PdBZTptEf_O9u9irVRzb0bBVAnTRga2xmw@mail.gmail.com","subject":"Re: [PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-18T11:50:17Z","receivedAt":"2017-08-18T11:50:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2017 at 12:12:37PM +0200, Patryk Obara wrote:\n\n> Jeff King wrote:\n> \n> > AFAICT this is only here to avoid having to s/buf/line->buf/ in the rest\n> > of the function. But I think we should just make that change (you\n> > already did in some of the spots). And IMHO we should do the same for\n> > line->len. When there are two names for the same value, it increases the\n> > chances of a bug where the two end up diverging.\n> \n> My motivation was rather to keep patch(es) as small as possible because every\n> line using buf will be replaced in a later patch in series. But it will make\n> commit better (it will stand on its own), so why not to do it? :)\n\nAh, I didn't notice those lines went away. That does make it less bad,\nbut I do think it's easier to review if each commit stands on its own.\n\nIn some cases, if it's really painful to do the intermediate cleanup, I\nmight say something in the commit message like \"this leaves X that is\nnot ideal, but we'll be getting rid of it soon anyway\". But in this case\nI think just creating that intermediate state is simple enough.\n\n> Ah, I only replaced comparison to NULL terminator with length check because\n> I thought it better shows intention of the code and I didn't notice, that\n> reversing order will result in better code overall.\n> \n> I will include both changes in v4.\n\nThanks.\n\n-Peff\n"},{"id":"326697","messageId":"xmqqd17sddvz.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAJfL8+SHSAhgrMY6ONVHLMWEHcT0mhm4oKMmq6D=89SErDKiMA@mail.gmail.com","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T16:44:32Z","receivedAt":"2017-08-18T16:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Junio C Hamano wrote:\n>>\n>> If I were doing the two-pass thing, I'd probably write a for loop\n>> that runs exactly twice, where the first iteration parses into a\n>> single throw-away oid struct only to count, and the second iteration\n>> parses the same input into the allocated array of oid struct. That\n>> way, you do not have to worry about two phrases going out of sync.\n>\n> Two passes would still differ in error handling due to xmalloc between them…\n\nI am not sure if I follow. What I meant was something along these\nlines:\n\n\tstruct commit_graft *graft = NULL;\n char *line = ... what you read from the file ...;\n int phase; /* phase #0 counts, phase #1 fills */\n\n\tfor (phase = 0; phase < 2; phase++) {\n\t\tint count;\n\t\tchar *scan;\n\t\tstrucut object_id dummy_oid, *oid;\n\n\t\tfor (scan = line, count = 0;\n *scan;\n\t\t count++) {\n \toid = graft ? &graft->parent[count] : &dummy_oid;\n\t\t\tif (parse_oid_hex(scan, oid, &scan))\n\t\t\t\treturn error(...);\n\t\t\tswitch (*scan) {\n\t\t\tcase ' ':\n\t\t\t\tscan++;\n\t\t\t\tcontinue; /* there are more */\n\t\t\tcase '\\0':\n\t\t\t\tbreak; /* we are done */\n\t\t\tdefault:\n\t\t\t\treturn error(...);\n\t\t\t}\n\t\t}\n\n\t\tif (!graft)\n\t\t\tgraft = xmalloc(... with 'count' parentes ...);\n\t}\n\n /* now we have graft with parent[count] all filled */\n\treturn graft;\n\nThe inner for() loop will do the same parsing for both passes,\nleaving little chance for programming errors to make the two passes\ndecide there are different number of fake parents. I suspect I may\nhave botched some details in that loop, but both passes will even\nshare the same buggy counting when the code is structured that\nway ;-)\n\nThat is what I meant by \"not have to worry about two phases going\nout of sync\".\n"},{"id":"326701","messageId":"CAJfL8+Tkb1KOn6bTGd8QPwr3=GgxKNZbp9OD_RmNeN4w-Os-iw@mail.gmail.com","threadId":"46588","inReplyTo":"xmqqd17sddvz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T17:05:56Z","receivedAt":"2017-08-18T17:06:32Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Ah! I presumed two separate loops, one throwing away oids and second\none actually filling a table - this makes more sense. I was just about\nto send v4, but will rewrite the last patch and we'll see how it looks\nlike.\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326704","messageId":"xmqqtw14bugt.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAJfL8+Tkb1KOn6bTGd8QPwr3=GgxKNZbp9OD_RmNeN4w-Os-iw@mail.gmail.com","subject":"Re: [PATCH v3 4/4] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T18:29:22Z","receivedAt":"2017-08-18T18:29:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Ah! I presumed two separate loops, one throwing away oids and second\n> one actually filling a table - this makes more sense. I was just about\n> to send v4, but will rewrite the last patch and we'll see how it looks\n> like.\n\nYeah, it is understandable if you missed my \"a loop that runs\nexactly twice\", as that pattern, while we do use it in a few places\nin our codebase, is of limited applicability in general---the cost\nof discarded computation in the first pass need to be low enough for\nthe improved maintainability to make sense.\n"},{"id":"326705","messageId":"cover.1503079879.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503020338.git.patryk.obara@gmail.com","subject":"[PATCH v4 0/4] Modernize read_graft_line implementation","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T18:33:10Z","receivedAt":"2017-08-18T18:33:24Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Changes since v3:\n\n- Commit replacing raw buffer does not store temporary pointer to\n strbuf internals any more.\n\n- Commit message of patch 4 explains all alternative approaches\n considered so far.\n\n- Patch 4 uses two-phases to parse graft line, without code repetition.\n\nI have my reservations about patch 4 from readability standpoint\n(it's not immediately clear why parsing code can skip freeing of graft\nin phase 2), but this implementation seems to address every issue raised\nin review so far. If you'll prefer me to go back to impementation\nfrom v3, I have it prepared ;)\n\n\nPatryk Obara (4):\n sha1_file: fix definition of null_sha1\n commit: replace the raw buffer with strbuf in read_graft_line\n commit: allocate array using object_id size\n commit: rewrite read_graft_line\n\n builtin/blame.c | 2 +-\n commit.c | 45 +++++++++++++++++++++++++--------------------\n commit.h | 2 +-\n sha1_file.c | 2 +-\n 4 files changed, 28 insertions(+), 23 deletions(-)\n\n-- \n2.9.5\n\n"},{"id":"326706","messageId":"f94a5bb774ef635924ea35895b2453aad329d069.1503079879.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503079879.git.patryk.obara@gmail.com","subject":"[PATCH v4 2/4] commit: replace the raw buffer with strbuf in read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T18:33:12Z","receivedAt":"2017-08-18T18:33:27Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"This simplifies function declaration and allows for use of strbuf_rtrim\ninstead of modifying buffer directly.\n\nSigned-off-by: Patryk Obara \n---\n builtin/blame.c | 2 +-\n commit.c | 23 +++++++++++------------\n commit.h | 2 +-\n 3 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bda1a78..d4472e9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -488,7 +488,7 @@ static int read_ancestry(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (graft)\n \t\t\tregister_commit_graft(graft, 0);\n \t}\ndiff --git a/commit.c b/commit.c\nindex 8b28415..1a0a9f2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -134,34 +134,33 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n \treturn 0;\n }\n \n-struct commit_graft *read_graft_line(char *buf, int len)\n+struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \tint i;\n \tstruct commit_graft *graft = NULL;\n \tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n \n-\twhile (len && isspace(buf[len-1]))\n-\t\tbuf[--len] = '\\0';\n-\tif (buf[0] == '#' || buf[0] == '\\0')\n+\tstrbuf_rtrim(line);\n+\tif (!line->len || line->buf[0] == '#')\n \t\treturn NULL;\n-\tif ((len + 1) % entry_size)\n+\tif ((line->len + 1) % entry_size)\n \t\tgoto bad_graft_data;\n-\ti = (len + 1) / entry_size - 1;\n+\ti = (line->len + 1) / entry_size - 1;\n \tgraft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n \tgraft->nr_parent = i;\n-\tif (get_oid_hex(buf, &graft->oid))\n+\tif (get_oid_hex(line->buf, &graft->oid))\n \t\tgoto bad_graft_data;\n-\tfor (i = GIT_SHA1_HEXSZ; i < len; i += entry_size) {\n-\t\tif (buf[i] != ' ')\n+\tfor (i = GIT_SHA1_HEXSZ; i < line->len; i += entry_size) {\n+\t\tif (line->buf[i] != ' ')\n \t\t\tgoto bad_graft_data;\n-\t\tif (get_sha1_hex(buf + i + 1, graft->parent[i/entry_size].hash))\n+\t\tif (get_sha1_hex(line->buf + i + 1, graft->parent[i/entry_size].hash))\n \t\t\tgoto bad_graft_data;\n \t}\n \treturn graft;\n \n bad_graft_data:\n-\terror(\"bad graft data: %s\", buf);\n+\terror(\"bad graft data: %s\", line->buf);\n \tfree(graft);\n \treturn NULL;\n }\n@@ -174,7 +173,7 @@ static int read_graft_file(const char *graft_file)\n \t\treturn -1;\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n+\t\tstruct commit_graft *graft = read_graft_line(&buf);\n \t\tif (!graft)\n \t\t\tcontinue;\n \t\tif (register_commit_graft(graft, 1))\ndiff --git a/commit.h b/commit.h\nindex 6d857f0..baecc0a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -247,7 +247,7 @@ struct commit_graft {\n };\n typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n-struct commit_graft *read_graft_line(char *buf, int len);\n+struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const struct object_id *oid);\n \n-- \n2.9.5\n\n"},{"id":"326707","messageId":"9a4548f1d0832d036cad152771339d853b5885f3.1503079879.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503079879.git.patryk.obara@gmail.com","subject":"[PATCH v4 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T18:33:14Z","receivedAt":"2017-08-18T18:33:29Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Old implementation determined number of hashes by dividing length of\nline by length of hash, which works only if all hash representations\nhave same length.\n\nNew graft line parser works in two phases:\n\n 1. In first phase line is scanned to verify correctness and compute\n number of hashes, then graft struct is allocated.\n\n 2. In second phase line is scanned again to fill up already allocated\n graft struct.\n\nThis way graft parsing code can support different sizes of hashes\nwithout any further code adaptations.\n\nA number of alternative implementations were considered and discarded:\n\n - Modifying graft structure to store oid_array instead of FLEXI_ARRAY\n indicates undesirable usage of struct to readers.\n\n - Parsing into temporary string_list or oid_array complicates code\n by adding more return paths, as these structures needs to be\n cleared before returning from function.\n\n - Determining number of hashes by counting separators might cause\n maintenance issues, if this function needs to be modified in future\n again.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 35 ++++++++++++++++++++---------------\n 1 file changed, 20 insertions(+), 15 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 436eb34..3eefd9d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -137,32 +137,37 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)\n struct commit_graft *read_graft_line(struct strbuf *line)\n {\n \t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n-\tint i;\n+\tint i, phase;\n+\tconst char *tail = NULL;\n \tstruct commit_graft *graft = NULL;\n-\tconst int entry_size = GIT_SHA1_HEXSZ + 1;\n+\tstruct object_id dummy_oid, *oid;\n \n \tstrbuf_rtrim(line);\n \tif (!line->len || line->buf[0] == '#')\n \t\treturn NULL;\n-\tif ((line->len + 1) % entry_size)\n-\t\tgoto bad_graft_data;\n-\ti = (line->len + 1) / entry_size - 1;\n-\tgraft = xmalloc(st_add(sizeof(*graft),\n-\t st_mult(sizeof(struct object_id), i)));\n-\tgraft->nr_parent = i;\n-\tif (get_oid_hex(line->buf, &graft->oid))\n-\t\tgoto bad_graft_data;\n-\tfor (i = GIT_SHA1_HEXSZ; i < line->len; i += entry_size) {\n-\t\tif (line->buf[i] != ' ')\n-\t\t\tgoto bad_graft_data;\n-\t\tif (get_sha1_hex(line->buf + i + 1, graft->parent[i/entry_size].hash))\n+\t/*\n+\t * phase 0 verifies line, counts hashes in line and allocates graft\n+\t * phase 1 fills graft\n+\t */\n+\tfor (phase = 0; phase < 2; phase++) {\n+\t\toid = graft ? &graft->oid : &dummy_oid;\n+\t\tif (parse_oid_hex(line->buf, oid, &tail))\n \t\t\tgoto bad_graft_data;\n+\t\tfor (i = 0; *tail != '\\0'; i++) {\n+\t\t\toid = graft ? &graft->parent[i] : &dummy_oid;\n+\t\t\tif (!isspace(*tail++) || parse_oid_hex(tail, oid, &tail))\n+\t\t\t\tgoto bad_graft_data;\n+\t\t}\n+\t\tif (!graft) {\n+\t\t\tgraft = xmalloc(st_add(sizeof(*graft),\n+\t\t\t st_mult(sizeof(struct object_id), i)));\n+\t\t\tgraft->nr_parent = i;\n+\t\t}\n \t}\n \treturn graft;\n \n bad_graft_data:\n \terror(\"bad graft data: %s\", line->buf);\n-\tfree(graft);\n \treturn NULL;\n }\n \n-- \n2.9.5\n\n"},{"id":"326708","messageId":"08a1c3de7d35243c36296d88ed17e8307aa9fff2.1503079879.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503079879.git.patryk.obara@gmail.com","subject":"[PATCH v4 1/4] sha1_file: fix definition of null_sha1","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T18:33:11Z","receivedAt":"2017-08-18T18:33:31Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"The array is declared in cache.h as:\n\n extern const unsigned char null_sha1[GIT_MAX_RAWSZ];\n\nDefinition in sha1_file.c must match.\n\nSigned-off-by: Patryk Obara \n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b60ae15..f5b5bec 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -32,7 +32,7 @@\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n-const unsigned char null_sha1[20];\n+const unsigned char null_sha1[GIT_MAX_RAWSZ];\n const struct object_id null_oid;\n const struct object_id empty_tree_oid = {\n \tEMPTY_TREE_SHA1_BIN_LITERAL\n-- \n2.9.5\n\n"},{"id":"326709","messageId":"d3003634866f18fee7c05f78b6e170a87f62f041.1503079879.git.patryk.obara@gmail.com","threadId":"46588","inReplyTo":"cover.1503079879.git.patryk.obara@gmail.com","subject":"[PATCH v4 3/4] commit: allocate array using object_id size","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T18:33:13Z","receivedAt":"2017-08-18T18:33:32Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"struct commit_graft aggregates an array of object_id's, which have\nsize >= GIT_MAX_RAWSZ bytes. This change prevents memory allocation\nerror when size of object_id is larger than GIT_SHA1_RAWSZ.\n\nSigned-off-by: Patryk Obara \n---\n commit.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 1a0a9f2..436eb34 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -147,7 +147,8 @@ struct commit_graft *read_graft_line(struct strbuf *line)\n \tif ((line->len + 1) % entry_size)\n \t\tgoto bad_graft_data;\n \ti = (line->len + 1) / entry_size - 1;\n-\tgraft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i)));\n+\tgraft = xmalloc(st_add(sizeof(*graft),\n+\t st_mult(sizeof(struct object_id), i)));\n \tgraft->nr_parent = i;\n \tif (get_oid_hex(line->buf, &graft->oid))\n \t\tgoto bad_graft_data;\n-- \n2.9.5\n\n"},{"id":"326710","messageId":"CAJfL8+T3vqnmFJmx19H-v8yGiY4Se78SM+ax_q07_PF4VHDv3Q@mail.gmail.com","threadId":"46588","inReplyTo":"9a4548f1d0832d036cad152771339d853b5885f3.1503079879.git.patryk.obara@gmail.com","subject":"Re: [PATCH v4 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T18:38:25Z","receivedAt":"2017-08-18T18:39:03Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Actually, I don't think I needed to remove free(graft) line, but I don't\nknow if freeing NULL is considered ok in git code. Let me know if I\nshould bring it back, please.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326714","messageId":"xmqqlgmgbshg.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAJfL8+T3vqnmFJmx19H-v8yGiY4Se78SM+ax_q07_PF4VHDv3Q@mail.gmail.com","subject":"Re: [PATCH v4 4/4] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T19:12:11Z","receivedAt":"2017-08-18T19:12:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Actually, I don't think I needed to remove free(graft) line, but I don't\n> know if freeing NULL is considered ok in git code. Let me know if I\n> should bring it back, please.\n\nCalling free(var) when var may or may not be NULL is perfectly fine.\n\nWe even discourage people from writing:\n\n\tif (var)\n\t\tfree(var);\n\nbecause an unconditional call to free(var) is sufficient.\n\n"},{"id":"326716","messageId":"CAJfL8+TdKm6ScPTG2gD4R39cagxN3=+RSA-KGTpMP6fYjoK0VA@mail.gmail.com","threadId":"46588","inReplyTo":"xmqqlgmgbshg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 4/4] commit: rewrite read_graft_line","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2017-08-18T19:33:34Z","receivedAt":"2017-08-18T19:34:10Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Ok, so that's an option - in this instance free is not actually needed\nbecause it can be triggered only in phase 0, but it would add a bit of\nrobustness.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"326717","messageId":"xmqqefs8bqvf.fsf@gitster.mtv.corp.google.com","threadId":"46588","inReplyTo":"CAJfL8+T3vqnmFJmx19H-v8yGiY4Se78SM+ax_q07_PF4VHDv3Q@mail.gmail.com","subject":"Re: [PATCH v4 4/4] commit: rewrite read_graft_line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T19:47:00Z","receivedAt":"2017-08-18T19:47:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patryk Obara writes:\n\n> Actually, I don't think I needed to remove free(graft) line, but I don't\n> know if freeing NULL is considered ok in git code. Let me know if I\n> should bring it back, please.\n\nEither free(graft) or assert(!graft) is fine, but we should have one\nof them there. I'll add assert(!graft) there while queuing, at\nleast for now.\n\nIn the current code, when the control reaches the bad_graft_data\nlabel, 'graft' must be NULL, or there is a bug in our code. Because\nwe are parsing exactly the same input using the same helper routines\nin both passes, we should see failure during the first pass before\n'graft' points to an allocated piece of memory. So it may be a good\nidea to have assert(!graft) there than free(graft); the latter would\nsweep a potential bug under the carpet.\n\nIf this were a part of the system whose design is still fluid (it is\nnot), it is not implausible that we would later want to add new test\nthat jumps to the bad_graft_data label. For example, after the\n\"runs exactly twice\" loop, we may add a new test that iterates over\nthe graft->parents[] to ensure that there is no duplicate and jumps\nto bad_graft_data when we find one.\n\nIf we add assert(!graft) there today, and if such an enhancement\nforgets to replace it with free(graft), the assert() will catch the\nmistake. If we have neither, it makes it more likely that such an\nenhancement leaves a possible memory leak in its error codepath.\n\nThanks.\n"}]}