{"thread":{"id":"46672","subject":"[PATCH 00/12] Clean up notes-related code around `load_subtree()`","startedAt":"2017-08-26T08:28:31Z","lastAt":"2017-09-12T11:55:55Z","messageCount":23,"participants":["Michael Haggerty","Junio C Hamano","Johan Herland","Jeff King","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"327230","messageId":"cover.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":null,"subject":"[PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:00Z","receivedAt":"2017-08-26T08:28:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"While putzing around in the notes code quite some time ago, I found\nthis comment:\n\n    /*\n     * Determine full path for this non-note entry:\n     * The filename is already found in entry.path, but the\n     * directory part of the path must be deduced from the subtree\n     * containing this entry. We assume here that the overall notes\n     * tree follows a strict byte-based progressive fanout\n     * structure (i.e. using 2/38, 2/2/36, etc. fanouts, and not\n     * e.g. 4/36 fanout). This means that if a non-note is found at\n     * path \"dead/beef\", the following code will register it as\n     * being found on \"de/ad/beef\".\n     * On the other hand, if you use such non-obvious non-note\n     * paths in the middle of a notes tree, you deserve what's\n     * coming to you ;). Note that for non-notes that are not\n     * SHA1-like at the top level, there will be no problems.\n     *\n     * To conclude, it is strongly advised to make sure non-notes\n     * have at least one non-hex character in the top-level path\n     * component.\n     */\n\nThis was enough of a nerd snipe to get me to dig into the code.\n\nIt turns out that the comment is incorrect, but there was nevertheless\nplenty that could be cleaned up in the area:\n\n* Make macro `GIT_NIBBLE` safer by adding some parentheses\n* Remove some dead code\n* Fix some memory leaks\n* Fix some obsolete and incorrect comments\n* Reject \"notes\" that are not blobs\n\nI hope the result is also easier to understand.\n\nThis branch is also available from my Git fork [1] as branch\n`load-subtree-cleanup`.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nMichael Haggerty (12):\n  notes: make GET_NIBBLE macro more robust\n  load_subtree(): remove unnecessary conditional\n  load_subtree(): reduce the scope of some local variables\n  load_subtree(): fix incorrect comment\n  load_subtree(): separate logic for internal vs. terminal entries\n  load_subtree(): check earlier whether an internal node is a tree entry\n  load_subtree(): only consider blobs to be potential notes\n  get_oid_hex_segment(): return 0 on success\n  load_subtree(): combine some common code\n  get_oid_hex_segment(): don't pad the rest of `oid`\n  hex_to_bytes(): simpler replacement for `get_oid_hex_segment()`\n  load_subtree(): declare some variables to be `size_t`\n\n notes.c | 136 +++++++++++++++++++++++++++++++---------------------------------\n 1 file changed, 66 insertions(+), 70 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"327231","messageId":"df9d90032a9bc36333b4c5d19edf7135bf862c19.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 01/12] notes: make GET_NIBBLE macro more robust","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:01Z","receivedAt":"2017-08-26T08:28:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Put parentheses around sha1. Otherwise it could fail for something\nlike\n\n    GET_NIBBLE(n, (unsigned char *)data);\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/notes.c b/notes.c\nindex f090c88363..00630a9396 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -64,7 +64,7 @@ struct non_note {\n #define CLR_PTR_TYPE(ptr)       ((void *) ((uintptr_t) (ptr) & ~3))\n #define SET_PTR_TYPE(ptr, type) ((void *) ((uintptr_t) (ptr) | (type)))\n \n-#define GET_NIBBLE(n, sha1) (((sha1[(n) >> 1]) >> ((~(n) & 0x01) << 2)) & 0x0f)\n+#define GET_NIBBLE(n, sha1) ((((sha1)[(n) >> 1]) >> ((~(n) & 0x01) << 2)) & 0x0f)\n \n #define KEY_INDEX (GIT_SHA1_RAWSZ - 1)\n #define FANOUT_PATH_SEPARATORS ((GIT_SHA1_HEXSZ / 2) - 1)\n-- \n2.11.0\n\n"},{"id":"327232","messageId":"c92a4da68db645185a7a3fa2a1584724882ad268.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 03/12] load_subtree(): reduce the scope of some local variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:03Z","receivedAt":"2017-08-26T08:28:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Declare the variables inside the loop, to make it more obvious that\ntheir values are not carried across loop iterations.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex f7ce64ff48..fbed8c3013 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -421,9 +421,6 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \tvoid *buf;\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n-\tint len, path_len;\n-\tunsigned char type;\n-\tstruct leaf_node *l;\n \n \tbuf = fill_tree_descriptor(&desc, &subtree->val_oid);\n \tif (!buf)\n@@ -434,7 +431,10 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \tassert(prefix_len * 2 >= n);\n \tmemcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);\n \twhile (tree_entry(&desc, &entry)) {\n-\t\tpath_len = strlen(entry.path);\n+\t\tunsigned char type;\n+\t\tstruct leaf_node *l;\n+\t\tint len, path_len = strlen(entry.path);\n+\n \t\tlen = get_oid_hex_segment(entry.path, path_len,\n \t\t\t\tobject_oid.hash + prefix_len, GIT_SHA1_RAWSZ - prefix_len);\n \t\tif (len < 0)\n-- \n2.11.0\n\n"},{"id":"327233","messageId":"3eb7cba0331bfaf46f60d9a44a74a931272c5304.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 04/12] load_subtree(): fix incorrect comment","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:04Z","receivedAt":"2017-08-26T08:28:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This comment was added in 851c2b3791 (Teach notes code to properly\npreserve non-notes in the notes tree, 2010-02-13) when the\ncorresponding code was added. But I believe it was incorrect even\nthen. The condition `path_len != 2` a dozen lines up prevents a path\nlike \"dead/beef\" from being converted to \"de/ad/beef\", and indeed the\ntest added in commit 851c2b3 verifies that this case works correctly.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 24 +++++++-----------------\n 1 file changed, 7 insertions(+), 17 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex fbed8c3013..62ab3f4ce3 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -468,23 +468,13 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \n handle_non_note:\n \t\t/*\n-\t\t * Determine full path for this non-note entry:\n-\t\t * The filename is already found in entry.path, but the\n-\t\t * directory part of the path must be deduced from the subtree\n-\t\t * containing this entry. We assume here that the overall notes\n-\t\t * tree follows a strict byte-based progressive fanout\n-\t\t * structure (i.e. using 2/38, 2/2/36, etc. fanouts, and not\n-\t\t * e.g. 4/36 fanout). This means that if a non-note is found at\n-\t\t * path \"dead/beef\", the following code will register it as\n-\t\t * being found on \"de/ad/beef\".\n-\t\t * On the other hand, if you use such non-obvious non-note\n-\t\t * paths in the middle of a notes tree, you deserve what's\n-\t\t * coming to you ;). Note that for non-notes that are not\n-\t\t * SHA1-like at the top level, there will be no problems.\n-\t\t *\n-\t\t * To conclude, it is strongly advised to make sure non-notes\n-\t\t * have at least one non-hex character in the top-level path\n-\t\t * component.\n+\t\t * Determine full path for this non-note entry. The\n+\t\t * filename is already found in entry.path, but the\n+\t\t * directory part of the path must be deduced from the\n+\t\t * subtree containing this entry based on our\n+\t\t * knowledge that the overall notes tree follows a\n+\t\t * strict byte-based progressive fanout structure\n+\t\t * (i.e. using 2/38, 2/2/36, etc. fanouts).\n \t\t */\n \t\t{\n \t\t\tstruct strbuf non_note_path = STRBUF_INIT;\n-- \n2.11.0\n\n"},{"id":"327234","messageId":"22e8ace89f28fd2fae31c08b8e86e03a4833e694.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 06/12] load_subtree(): check earlier whether an internal node is a tree entry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:06Z","receivedAt":"2017-08-26T08:28:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If an entry is not a tree entry, then it cannot possibly be an\ninternal node. But the old code checked this condition only after\nallocating a leaf_node object and therefore leaked that memory.\nInstead, check before even entering this branch of the code.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 768902055e..ac69c5aa18 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -449,6 +449,11 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\toidcpy(&l->val_oid, entry.oid);\n \t\t} else if (path_len == 2) {\n \t\t\t/* This is potentially an internal node */\n+\n+\t\t\tif (!S_ISDIR(entry.mode))\n+\t\t\t\t/* internal nodes must be trees */\n+\t\t\t\tgoto handle_non_note;\n+\n \t\t\tif (get_oid_hex_segment(entry.path, 2,\n \t\t\t\t\t\tobject_oid.hash + prefix_len,\n \t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len) < 0)\n@@ -459,8 +464,6 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\t\txcalloc(1, sizeof(struct leaf_node));\n \t\t\toidcpy(&l->key_oid, &object_oid);\n \t\t\toidcpy(&l->val_oid, entry.oid);\n-\t\t\tif (!S_ISDIR(entry.mode))\n-\t\t\t\tgoto handle_non_note; /* not subtree */\n \t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1);\n \t\t} else {\n \t\t\t/* This can't be part of a note */\n-- \n2.11.0\n\n"},{"id":"327235","messageId":"1a421226c8f7cb3cb7609cb4bd06ed009430a543.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 08/12] get_oid_hex_segment(): return 0 on success","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:08Z","receivedAt":"2017-08-26T08:28:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Nobody cares about the return value of get_oid_hex_segment() except to\ncheck whether it failed. So just return 0 on success.\n\nAnd while we're updating its docstring, update it for some argument\nrenaming that happened a while ago.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 46ab15b83a..6ce71bfedb 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -338,11 +338,10 @@ static void note_tree_free(struct int_node *tree)\n  * Convert a partial SHA1 hex string to the corresponding partial SHA1 value.\n  * - hex      - Partial SHA1 segment in ASCII hex format\n  * - hex_len  - Length of above segment. Must be multiple of 2 between 0 and 40\n- * - sha1     - Partial SHA1 value is written here\n- * - sha1_len - Max #bytes to store in sha1, Must be >= hex_len / 2, and < 20\n- * Returns -1 on error (invalid arguments or invalid SHA1 (not in hex format)).\n- * Otherwise, returns number of bytes written to sha1 (i.e. hex_len / 2).\n- * Pads sha1 with NULs up to sha1_len (not included in returned length).\n+ * - oid      - Partial SHA1 value is written here\n+ * - oid_len  - Max #bytes to store in sha1, Must be >= hex_len / 2, and < 20\n+ * Return 0 on success or -1 on error (invalid arguments or input not\n+ * in hex format). Pad oid with NULs up to oid_len.\n  */\n static int get_oid_hex_segment(const char *hex, unsigned int hex_len,\n \t\tunsigned char *oid, unsigned int oid_len)\n@@ -359,7 +358,7 @@ static int get_oid_hex_segment(const char *hex, unsigned int hex_len,\n \t}\n \tfor (; i < oid_len; i++)\n \t\t*oid++ = 0;\n-\treturn len;\n+\treturn 0;\n }\n \n static int non_note_cmp(const struct non_note *a, const struct non_note *b)\n@@ -444,7 +443,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \n \t\t\tif (get_oid_hex_segment(entry.path, path_len,\n \t\t\t\t\t\tobject_oid.hash + prefix_len,\n-\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len) < 0)\n+\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n \t\t\ttype = PTR_TYPE_NOTE;\n@@ -461,7 +460,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \n \t\t\tif (get_oid_hex_segment(entry.path, 2,\n \t\t\t\t\t\tobject_oid.hash + prefix_len,\n-\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len) < 0)\n+\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n \t\t\ttype = PTR_TYPE_SUBTREE;\n-- \n2.11.0\n\n"},{"id":"327236","messageId":"ba5c439f990752a7768ed82c04a387aabd75558a.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 10/12] get_oid_hex_segment(): don't pad the rest of `oid`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:10Z","receivedAt":"2017-08-26T08:28:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Remove the feature of `get_oid_hex_segment()` that it pads the rest of\nthe `oid` argument with zeros. Instead, do this at the caller who\nneeds it.\n\nThis makes the functionality of this function more coherent and\nremoves the need for its `oid_len` argument.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 534fda007e..ce9ba36179 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -339,15 +339,14 @@ static void note_tree_free(struct int_node *tree)\n  * - hex      - Partial SHA1 segment in ASCII hex format\n  * - hex_len  - Length of above segment. Must be multiple of 2 between 0 and 40\n  * - oid      - Partial SHA1 value is written here\n- * - oid_len  - Max #bytes to store in sha1, Must be >= hex_len / 2, and < 20\n  * Return 0 on success or -1 on error (invalid arguments or input not\n- * in hex format). Pad oid with NULs up to oid_len.\n+ * in hex format).\n  */\n static int get_oid_hex_segment(const char *hex, unsigned int hex_len,\n-\t\tunsigned char *oid, unsigned int oid_len)\n+\t\tunsigned char *oid)\n {\n \tunsigned int i, len = hex_len >> 1;\n-\tif (hex_len % 2 != 0 || len > oid_len)\n+\tif (hex_len % 2 != 0)\n \t\treturn -1;\n \tfor (i = 0; i < len; i++) {\n \t\tunsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);\n@@ -356,8 +355,6 @@ static int get_oid_hex_segment(const char *hex, unsigned int hex_len,\n \t\t*oid++ = val;\n \t\thex += 2;\n \t}\n-\tfor (; i < oid_len; i++)\n-\t\t*oid++ = 0;\n \treturn 0;\n }\n \n@@ -442,24 +439,29 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\t\tgoto handle_non_note;\n \n \t\t\tif (get_oid_hex_segment(entry.path, path_len,\n-\t\t\t\t\t\tobject_oid.hash + prefix_len,\n-\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len))\n+\t\t\t\t\t\tobject_oid.hash + prefix_len))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n \t\t\ttype = PTR_TYPE_NOTE;\n \t\t} else if (path_len == 2) {\n \t\t\t/* This is potentially an internal node */\n+\t\t\tsize_t len = prefix_len;\n \n \t\t\tif (!S_ISDIR(entry.mode))\n \t\t\t\t/* internal nodes must be trees */\n \t\t\t\tgoto handle_non_note;\n \n \t\t\tif (get_oid_hex_segment(entry.path, 2,\n-\t\t\t\t\t\tobject_oid.hash + prefix_len,\n-\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len))\n+\t\t\t\t\t\tobject_oid.hash + len++))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n-\t\t\tobject_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1);\n+\t\t\t/*\n+\t\t\t * Pad the rest of the SHA-1 with zeros,\n+\t\t\t * except for the last byte, where we write\n+\t\t\t * the length:\n+\t\t\t */\n+\t\t\tmemset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ - len - 1);\n+\t\t\tobject_oid.hash[KEY_INDEX] = (unsigned char)len;\n \n \t\t\ttype = PTR_TYPE_SUBTREE;\n \t\t} else {\n-- \n2.11.0\n\n"},{"id":"327237","messageId":"86a8617cf78e456b6f119d8820479b6d82094124.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 11/12] hex_to_bytes(): simpler replacement for `get_oid_hex_segment()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:11Z","receivedAt":"2017-08-26T08:28:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that `get_oid_hex_segment()` does less, it makes sense to rename\nit and simplify its semantics:\n\n* Instead of a `hex_len` parameter, which was the number of hex\n  characters (and had to be even), use a `len` parameter, which is the\n  number of resulting bytes. This removes then need for the check that\n  `hex_len` is even and to divide it by two to determine the number of\n  bytes. For good hygiene, declare the `len` parameter to be `size_t`\n  instead of `unsigned int`.\n\n* Change the order of the arguments to the more traditional (dst,\n  src, len).\n\n* Rename the function to `hex_to_bytes()`.\n\n* Remove a loop variable: just count `len` down instead.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 28 ++++++++++------------------\n 1 file changed, 10 insertions(+), 18 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex ce9ba36179..d5409b55e3 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -335,25 +335,18 @@ static void note_tree_free(struct int_node *tree)\n }\n \n /*\n- * Convert a partial SHA1 hex string to the corresponding partial SHA1 value.\n- * - hex      - Partial SHA1 segment in ASCII hex format\n- * - hex_len  - Length of above segment. Must be multiple of 2 between 0 and 40\n- * - oid      - Partial SHA1 value is written here\n- * Return 0 on success or -1 on error (invalid arguments or input not\n- * in hex format).\n+ * Read `len` pairs of hexadecimal digits from `hex` and write the\n+ * values to `binary` as `len` bytes. Return 0 on success, or -1 if\n+ * the input does not consist of hex digits).\n  */\n-static int get_oid_hex_segment(const char *hex, unsigned int hex_len,\n-\t\tunsigned char *oid)\n+static int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)\n {\n-\tunsigned int i, len = hex_len >> 1;\n-\tif (hex_len % 2 != 0)\n-\t\treturn -1;\n-\tfor (i = 0; i < len; i++) {\n+\tfor (; len; len--, hex += 2) {\n \t\tunsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);\n+\n \t\tif (val & ~0xff)\n \t\t\treturn -1;\n-\t\t*oid++ = val;\n-\t\thex += 2;\n+\t\t*binary++ = val;\n \t}\n \treturn 0;\n }\n@@ -438,8 +431,8 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\t\t/* notes must be blobs */\n \t\t\t\tgoto handle_non_note;\n \n-\t\t\tif (get_oid_hex_segment(entry.path, path_len,\n-\t\t\t\t\t\tobject_oid.hash + prefix_len))\n+\t\t\tif (hex_to_bytes(object_oid.hash + prefix_len, entry.path,\n+\t\t\t\t\t GIT_SHA1_RAWSZ - prefix_len))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n \t\t\ttype = PTR_TYPE_NOTE;\n@@ -451,8 +444,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\t\t/* internal nodes must be trees */\n \t\t\t\tgoto handle_non_note;\n \n-\t\t\tif (get_oid_hex_segment(entry.path, 2,\n-\t\t\t\t\t\tobject_oid.hash + len++))\n+\t\t\tif (hex_to_bytes(object_oid.hash + len++, entry.path, 1))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n \t\t\t/*\n-- \n2.11.0\n\n"},{"id":"327238","messageId":"03eba8f14c048d276249bcb64bd9d97f5c2e55f9.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 12/12] load_subtree(): declare some variables to be `size_t`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:12Z","receivedAt":"2017-08-26T08:28:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* `prefix_len`\n* `path_len`\n* `i`\n\nIt's good hygiene.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex d5409b55e3..7f5bfa19c7 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -406,7 +406,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\tstruct int_node *node, unsigned int n)\n {\n \tstruct object_id object_oid;\n-\tunsigned int prefix_len;\n+\tsize_t prefix_len;\n \tvoid *buf;\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n@@ -422,7 +422,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \twhile (tree_entry(&desc, &entry)) {\n \t\tunsigned char type;\n \t\tstruct leaf_node *l;\n-\t\tint path_len = strlen(entry.path);\n+\t\tsize_t path_len = strlen(entry.path);\n \n \t\tif (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {\n \t\t\t/* This is potentially the remainder of the SHA-1 */\n@@ -486,7 +486,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t{\n \t\t\tstruct strbuf non_note_path = STRBUF_INIT;\n \t\t\tconst char *q = oid_to_hex(&subtree->key_oid);\n-\t\t\tint i;\n+\t\t\tsize_t i;\n \t\t\tfor (i = 0; i < prefix_len; i++) {\n \t\t\t\tstrbuf_addch(&non_note_path, *q++);\n \t\t\t\tstrbuf_addch(&non_note_path, *q++);\n-- \n2.11.0\n\n"},{"id":"327239","messageId":"f57e3132fcb98b1a9f36b07886d3620bd224e377.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 07/12] load_subtree(): only consider blobs to be potential notes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:07Z","receivedAt":"2017-08-26T08:28:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code converted any entry whose path constituted a full SHA-1\nas a leaf node, without regard for the type of the entry. But only\nblobs can be notes. So treat entries whose paths *look like* notes\npaths but that are not blobs as non-notes.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/notes.c b/notes.c\nindex ac69c5aa18..46ab15b83a 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -437,6 +437,11 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \n \t\tif (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {\n \t\t\t/* This is potentially the remainder of the SHA-1 */\n+\n+\t\t\tif (!S_ISREG(entry.mode))\n+\t\t\t\t/* notes must be blobs */\n+\t\t\t\tgoto handle_non_note;\n+\n \t\t\tif (get_oid_hex_segment(entry.path, path_len,\n \t\t\t\t\t\tobject_oid.hash + prefix_len,\n \t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len) < 0)\n-- \n2.11.0\n\n"},{"id":"327240","messageId":"b76c9b2596b4385ee40ea4da7f56190cf6d225f2.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 05/12] load_subtree(): separate logic for internal vs. terminal entries","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:05Z","receivedAt":"2017-08-26T08:29:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There are only two legitimate notes path components:\n\n* A hexadecimal string that fills the rest of the SHA-1\n\n* A two-digit hexadecimal string that constitutes another internal\n  node.\n\nSo handle those two cases at the top level, and reject others as\nnon-notes without trying to parse them. The logic separation also\nsimplifies upcoming changes.\n\nThis prevents us from leaking memory for a leaf_node in the case of\nwrong-sized paths. There are still memory leaks in this code; they will\nbe fixed in upcoming commits.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 52 +++++++++++++++++++++++++++++++---------------------\n 1 file changed, 31 insertions(+), 21 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 62ab3f4ce3..768902055e 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -433,30 +433,40 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \twhile (tree_entry(&desc, &entry)) {\n \t\tunsigned char type;\n \t\tstruct leaf_node *l;\n-\t\tint len, path_len = strlen(entry.path);\n+\t\tint path_len = strlen(entry.path);\n+\n+\t\tif (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {\n+\t\t\t/* This is potentially the remainder of the SHA-1 */\n+\t\t\tif (get_oid_hex_segment(entry.path, path_len,\n+\t\t\t\t\t\tobject_oid.hash + prefix_len,\n+\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len) < 0)\n+\t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n+\n+\t\t\ttype = PTR_TYPE_NOTE;\n+\t\t\tl = (struct leaf_node *)\n+\t\t\t\txcalloc(1, sizeof(struct leaf_node));\n+\t\t\toidcpy(&l->key_oid, &object_oid);\n+\t\t\toidcpy(&l->val_oid, entry.oid);\n+\t\t} else if (path_len == 2) {\n+\t\t\t/* This is potentially an internal node */\n+\t\t\tif (get_oid_hex_segment(entry.path, 2,\n+\t\t\t\t\t\tobject_oid.hash + prefix_len,\n+\t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len) < 0)\n+\t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n-\t\tlen = get_oid_hex_segment(entry.path, path_len,\n-\t\t\t\tobject_oid.hash + prefix_len, GIT_SHA1_RAWSZ - prefix_len);\n-\t\tif (len < 0)\n-\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n-\t\tlen += prefix_len;\n-\n-\t\t/*\n-\t\t * If object SHA1 is complete (len == 20), assume note object\n-\t\t * If object SHA1 is incomplete (len < 20), and current\n-\t\t * component consists of 2 hex chars, assume note subtree\n-\t\t */\n-\t\ttype = PTR_TYPE_NOTE;\n-\t\tl = (struct leaf_node *)\n-\t\t\txcalloc(1, sizeof(struct leaf_node));\n-\t\toidcpy(&l->key_oid, &object_oid);\n-\t\toidcpy(&l->val_oid, entry.oid);\n-\t\tif (len < GIT_SHA1_RAWSZ) {\n-\t\t\tif (!S_ISDIR(entry.mode) || path_len != 2)\n-\t\t\t\tgoto handle_non_note; /* not subtree */\n-\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) len;\n \t\t\ttype = PTR_TYPE_SUBTREE;\n+\t\t\tl = (struct leaf_node *)\n+\t\t\t\txcalloc(1, sizeof(struct leaf_node));\n+\t\t\toidcpy(&l->key_oid, &object_oid);\n+\t\t\toidcpy(&l->val_oid, entry.oid);\n+\t\t\tif (!S_ISDIR(entry.mode))\n+\t\t\t\tgoto handle_non_note; /* not subtree */\n+\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1);\n+\t\t} else {\n+\t\t\t/* This can't be part of a note */\n+\t\t\tgoto handle_non_note;\n \t\t}\n+\n \t\tif (note_tree_insert(t, node, n, l, type,\n \t\t\t\t     combine_notes_concatenate))\n \t\t\tdie(\"Failed to load %s %s into notes tree \"\n-- \n2.11.0\n\n"},{"id":"327241","messageId":"f0077bb814ea465dbd5c0b8b2e7fe36fdde56070.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 09/12] load_subtree(): combine some common code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:09Z","receivedAt":"2017-08-26T08:29:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Write the length into `object_oid` (before copying) rather than\n`l->key_oid` (after copying). Then combine some code from the two `if`\nblocks.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 6ce71bfedb..534fda007e 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -447,10 +447,6 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n \t\t\ttype = PTR_TYPE_NOTE;\n-\t\t\tl = (struct leaf_node *)\n-\t\t\t\txcalloc(1, sizeof(struct leaf_node));\n-\t\t\toidcpy(&l->key_oid, &object_oid);\n-\t\t\toidcpy(&l->val_oid, entry.oid);\n \t\t} else if (path_len == 2) {\n \t\t\t/* This is potentially an internal node */\n \n@@ -463,17 +459,17 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t\t\t\t\tGIT_SHA1_RAWSZ - prefix_len))\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n \n+\t\t\tobject_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1);\n+\n \t\t\ttype = PTR_TYPE_SUBTREE;\n-\t\t\tl = (struct leaf_node *)\n-\t\t\t\txcalloc(1, sizeof(struct leaf_node));\n-\t\t\toidcpy(&l->key_oid, &object_oid);\n-\t\t\toidcpy(&l->val_oid, entry.oid);\n-\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) (prefix_len + 1);\n \t\t} else {\n \t\t\t/* This can't be part of a note */\n \t\t\tgoto handle_non_note;\n \t\t}\n \n+\t\tl = xcalloc(1, sizeof(*l));\n+\t\toidcpy(&l->key_oid, &object_oid);\n+\t\toidcpy(&l->val_oid, entry.oid);\n \t\tif (note_tree_insert(t, node, n, l, type,\n \t\t\t\t     combine_notes_concatenate))\n \t\t\tdie(\"Failed to load %s %s into notes tree \"\n-- \n2.11.0\n\n"},{"id":"327242","messageId":"c21bedbee9487792f4a336a417aa9874578aaac2.1503734566.git.mhagger@alum.mit.edu","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"[PATCH 02/12] load_subtree(): remove unnecessary conditional","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-26T08:28:02Z","receivedAt":"2017-08-26T08:29:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"At this point in the code, len is *always* <= 20.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n notes.c | 35 +++++++++++++++++------------------\n 1 file changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 00630a9396..f7ce64ff48 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -446,25 +446,24 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\t * If object SHA1 is incomplete (len < 20), and current\n \t\t * component consists of 2 hex chars, assume note subtree\n \t\t */\n-\t\tif (len <= GIT_SHA1_RAWSZ) {\n-\t\t\ttype = PTR_TYPE_NOTE;\n-\t\t\tl = (struct leaf_node *)\n-\t\t\t\txcalloc(1, sizeof(struct leaf_node));\n-\t\t\toidcpy(&l->key_oid, &object_oid);\n-\t\t\toidcpy(&l->val_oid, entry.oid);\n-\t\t\tif (len < GIT_SHA1_RAWSZ) {\n-\t\t\t\tif (!S_ISDIR(entry.mode) || path_len != 2)\n-\t\t\t\t\tgoto handle_non_note; /* not subtree */\n-\t\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) len;\n-\t\t\t\ttype = PTR_TYPE_SUBTREE;\n-\t\t\t}\n-\t\t\tif (note_tree_insert(t, node, n, l, type,\n-\t\t\t\t\t     combine_notes_concatenate))\n-\t\t\t\tdie(\"Failed to load %s %s into notes tree \"\n-\t\t\t\t    \"from %s\",\n-\t\t\t\t    type == PTR_TYPE_NOTE ? \"note\" : \"subtree\",\n-\t\t\t\t    oid_to_hex(&l->key_oid), t->ref);\n+\t\ttype = PTR_TYPE_NOTE;\n+\t\tl = (struct leaf_node *)\n+\t\t\txcalloc(1, sizeof(struct leaf_node));\n+\t\toidcpy(&l->key_oid, &object_oid);\n+\t\toidcpy(&l->val_oid, entry.oid);\n+\t\tif (len < GIT_SHA1_RAWSZ) {\n+\t\t\tif (!S_ISDIR(entry.mode) || path_len != 2)\n+\t\t\t\tgoto handle_non_note; /* not subtree */\n+\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) len;\n+\t\t\ttype = PTR_TYPE_SUBTREE;\n \t\t}\n+\t\tif (note_tree_insert(t, node, n, l, type,\n+\t\t\t\t     combine_notes_concatenate))\n+\t\t\tdie(\"Failed to load %s %s into notes tree \"\n+\t\t\t    \"from %s\",\n+\t\t\t    type == PTR_TYPE_NOTE ? \"note\" : \"subtree\",\n+\t\t\t    oid_to_hex(&l->key_oid), t->ref);\n+\n \t\tcontinue;\n \n handle_non_note:\n-- \n2.11.0\n\n"},{"id":"327247","messageId":"xmqqh8wuqo6e.fsf@gitster.mtv.corp.google.com","threadId":"46672","inReplyTo":"c21bedbee9487792f4a336a417aa9874578aaac2.1503734566.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-26T16:38:49Z","receivedAt":"2017-08-26T16:38:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> At this point in the code, len is *always* <= 20.\n\nThis is the kind of log message that makes me unconfortable, as it\nlacks \"because\", and the readers would need to find out themselves\nby following the same codepath the patch author already followed.\n\nThere is an assert earlier before the control gets in this loop\n\n\tprefix_len = subtree->key_oid.hash[KEY_INDEX];\n\tassert(prefix_len * 2 >= n);\n\tmemcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);\n\nthat tries to ensure there is sufficient number of prefix defined in\nthat key, and the codeflow may ensure that prefix_len is both an\neven number and shorter than 20 (the correctness of the code depends\non these, it seems, and if for some reason prefix_len is much\nlarger, calls to get_oid_hex_segment() will overflow the oid.hash[]\narray without checking).  I'd at least feel safer to have an assert\nnext to the existing one that catches a bug to throw a randomly\nlarge value into subtree->key_oid.hash[KEY_INDEX].  Then we can\nsafely say \"at this point in the code, len is always <= 20\", as that\nassert will makes it obvious without looking at anything other than\nthis code and get_oid_hex_segment() implementaiton (combined with\nthe fact that this function is the only one that coerces len and\nputs it into ->key_oid.hash[KEY_INDEX], but that is a weak assurance\nas we cannot tell where \"subtree\" came from---it may have full\n20-byte oid in its key_oid field---without following the callchain a\nlot more widely).\n\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  notes.c | 35 +++++++++++++++++------------------\n>  1 file changed, 17 insertions(+), 18 deletions(-)\n>\n> diff --git a/notes.c b/notes.c\n> index 00630a9396..f7ce64ff48 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -446,25 +446,24 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n>  \t\t * If object SHA1 is incomplete (len < 20), and current\n>  \t\t * component consists of 2 hex chars, assume note subtree\n>  \t\t */\n> -\t\tif (len <= GIT_SHA1_RAWSZ) {\n> -\t\t\ttype = PTR_TYPE_NOTE;\n> -\t\t\tl = (struct leaf_node *)\n> -\t\t\t\txcalloc(1, sizeof(struct leaf_node));\n> -\t\t\toidcpy(&l->key_oid, &object_oid);\n> -\t\t\toidcpy(&l->val_oid, entry.oid);\n> -\t\t\tif (len < GIT_SHA1_RAWSZ) {\n> -\t\t\t\tif (!S_ISDIR(entry.mode) || path_len != 2)\n> -\t\t\t\t\tgoto handle_non_note; /* not subtree */\n> -\t\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) len;\n> -\t\t\t\ttype = PTR_TYPE_SUBTREE;\n> -\t\t\t}\n> -\t\t\tif (note_tree_insert(t, node, n, l, type,\n> -\t\t\t\t\t     combine_notes_concatenate))\n> -\t\t\t\tdie(\"Failed to load %s %s into notes tree \"\n> -\t\t\t\t    \"from %s\",\n> -\t\t\t\t    type == PTR_TYPE_NOTE ? \"note\" : \"subtree\",\n> -\t\t\t\t    oid_to_hex(&l->key_oid), t->ref);\n> +\t\ttype = PTR_TYPE_NOTE;\n> +\t\tl = (struct leaf_node *)\n> +\t\t\txcalloc(1, sizeof(struct leaf_node));\n> +\t\toidcpy(&l->key_oid, &object_oid);\n> +\t\toidcpy(&l->val_oid, entry.oid);\n> +\t\tif (len < GIT_SHA1_RAWSZ) {\n> +\t\t\tif (!S_ISDIR(entry.mode) || path_len != 2)\n> +\t\t\t\tgoto handle_non_note; /* not subtree */\n> +\t\t\tl->key_oid.hash[KEY_INDEX] = (unsigned char) len;\n> +\t\t\ttype = PTR_TYPE_SUBTREE;\n>  \t\t}\n> +\t\tif (note_tree_insert(t, node, n, l, type,\n> +\t\t\t\t     combine_notes_concatenate))\n> +\t\t\tdie(\"Failed to load %s %s into notes tree \"\n> +\t\t\t    \"from %s\",\n> +\t\t\t    type == PTR_TYPE_NOTE ? \"note\" : \"subtree\",\n> +\t\t\t    oid_to_hex(&l->key_oid), t->ref);\n> +\n>  \t\tcontinue;\n>  \n>  handle_non_note:\n"},{"id":"327251","messageId":"CALKQrgdRwch4d837OwOJUxPPmUc43janx1VROvAoEJG2ef3SJA@mail.gmail.com","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2017-08-26T23:36:32Z","receivedAt":"2017-08-26T23:56:20Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sat, Aug 26, 2017 at 10:28 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n[...]\n> plenty that could be cleaned up in the area:\n>\n> * Make macro `GIT_NIBBLE` safer by adding some parentheses\n> * Remove some dead code\n> * Fix some memory leaks\n> * Fix some obsolete and incorrect comments\n> * Reject \"notes\" that are not blobs\n>\n> I hope the result is also easier to understand.\n\nI looked through the series, and the patches look good to me, although\nI do agree with Junio's comments on #2.\n\nThanks for a long-overdue cleanup in one of the hairier parts of\nthe notes code. The end result reads a lot better IMHO.\n\n...Johan\n"},{"id":"327254","messageId":"CAMy9T_HYV9=HvrAnAxHgzRvUy__3o99PxQSOe2iCE_swtk_8VQ@mail.gmail.com","threadId":"46672","inReplyTo":"xmqqh8wuqo6e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-27T06:37:47Z","receivedAt":"2017-08-27T06:37:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Sat, Aug 26, 2017 at 6:38 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>\n>> At this point in the code, len is *always* <= 20.\n>\n> This is the kind of log message that makes me unconfortable, as it\n> lacks \"because\", and the readers would need to find out themselves\n> by following the same codepath the patch author already followed.\n> [...]\n\nThat's a valid complaint. I've adjusted the patch series to add the\nassertion and explain the reasoning better in the commit message. I've\npushed the revised series to GitHub, but I'll wait a couple of days to\nsee if there's more feedback before resubmitting.\n\nThanks,\nMichael\n"},{"id":"327271","messageId":"CAMy9T_Gt=p==jHmx5nf8GeZBULACkjqC2zZqU+F31yx1xVaPBw@mail.gmail.com","threadId":"46672","inReplyTo":"CAMy9T_HYV9=HvrAnAxHgzRvUy__3o99PxQSOe2iCE_swtk_8VQ@mail.gmail.com","subject":"Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-28T06:55:39Z","receivedAt":"2017-08-28T06:55:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Junio, I'm surprised that you have merged the `mh/notes-cleanup`\nbranch into `next` already. Was that intentional? Aside from the fact\nthat the topic has had very little cooking time, there's the issue of\nthe assertion that you asked for. I have implemented the assertion in\na new version of the branch that I pushed to GitHub but haven't yet\nsubmitted to the list.\n\nHow would you like to proceed? Do you want me to submit a patch\n(adding the assertion) that applies on top of this branch?\n\nMichael\n"},{"id":"327511","messageId":"xmqqr2vqhyri.fsf@gitster.mtv.corp.google.com","threadId":"46672","inReplyTo":"CAMy9T_Gt=p==jHmx5nf8GeZBULACkjqC2zZqU+F31yx1xVaPBw@mail.gmail.com","subject":"Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-01T21:53:05Z","receivedAt":"2017-09-01T21:53:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Junio, I'm surprised that you have merged the `mh/notes-cleanup`\n> branch into `next` already. Was that intentional?\n\nYup, I was clearing the deck as much as possible before I go\noffline, as there didn't seem to be any glaring problem that we do\nnot even want to see in the history of our codebase in the series,\nand I thought it would be better to give wider exposure early, as\nlong as small improvements are done incrementally.\n\nThanks.\n\nps. I'll still be offline for a bit more, so please do not get\ndisappointed if your updates do not show up in my tree until I come\nback.\n\n\n\n"},{"id":"327813","messageId":"20170909103131.pppm346qbj2cdxuo@sigill.intra.peff.net","threadId":"46672","inReplyTo":"cover.1503734566.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-09T10:31:31Z","receivedAt":"2017-09-09T10:31:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 26, 2017 at 10:28:00AM +0200, Michael Haggerty wrote:\n\n> It turns out that the comment is incorrect, but there was nevertheless\n> plenty that could be cleaned up in the area:\n> \n> * Make macro `GIT_NIBBLE` safer by adding some parentheses\n> * Remove some dead code\n> * Fix some memory leaks\n> * Fix some obsolete and incorrect comments\n> * Reject \"notes\" that are not blobs\n> \n> I hope the result is also easier to understand.\n> \n> This branch is also available from my Git fork [1] as branch\n> `load-subtree-cleanup`.\n\nFYI, Coverity seems to complain about \"pu\" after this series is merged, but\nI think it's wrong.  It says:\n\n  *** CID 1417630:  Memory - illegal accesses  (OVERRUN)\n  /notes.c: 458 in load_subtree()\n  452     \n  453     \t\t\t/*\n  454     \t\t\t * Pad the rest of the SHA-1 with zeros,\n  455     \t\t\t * except for the last byte, where we write\n  456     \t\t\t * the length:\n  457     \t\t\t */\n  >>>     CID 1417630:  Memory - illegal accesses  (OVERRUN)\n  >>>     Overrunning array of 20 bytes at byte offset 20 by dereferencing pointer \"&object_oid.hash[len]\".\n  458     \t\t\tmemset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ - len - 1);\n  459     \t\t\tobject_oid.hash[KEY_INDEX] = (unsigned char)len;\n  460     \n  461     \t\t\ttype = PTR_TYPE_SUBTREE;\n  462     \t\t} else {\n  463     \t\t\t/* This can't be part of a note */\n\nI agree that if \"len\" were 20 here that would be a problem, but I don't\nthink that's possible.\n\nThe tool correctly claims that prefix_len can be up to 19, due to the\nassert:\n\n     3. cond_at_most: Checking prefix_len >= 20UL implies that prefix_len may be up to 19 on the false branch.\n  420        if (prefix_len >= GIT_SHA1_RAWSZ)\n  421                BUG(\"prefix_len (%\"PRIuMAX\") is out of range\", (uintmax_t)prefix_len);\n\nThen it claims:\n\n    13. Condition path_len == 2 * (20 - prefix_len), taking false branch.\n  430                if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {\n  431                        /* This is potentially the remainder of the SHA-1 */\n\nSo we know that either prefix_len is not 19, or that path_len is not 2\n(since that combination would cause us to take the true branch here).\nBut then it goes on to say:\n\n    14. Condition path_len == 2, taking true branch.\n  442                } else if (path_len == 2) {\n  443                        /* This is potentially an internal node */\n\nwhich I believe must mean that prefix_len cannot be 19 here. And yet it\nsays:\n\n    15. assignment: Assigning: len = prefix_len. The value of len may now be up to 19.\n  444                        size_t len = prefix_len;\n  445\n  [...]\n     17. incr: Incrementing len. The value of len may now be up to 20.\n     18. Condition hex_to_bytes(&object_oid.hash[len++], entry.path, 1), taking false branch.\n  450                        if (hex_to_bytes(object_oid.hash + len++, entry.path, 1))\n  451                                goto handle_non_note; /* entry.path is not a SHA1 */\n\nI think that's impossible, and Coverity simply isn't smart enough to\nshrink the set of possible values for prefix_len based on the set of\nif-else conditions.\n\nSo nothing to see here, but since I spent 20 minutes scratching my head\n(and I know others look at Coverity output and may scratch their heads\ntoo), I thought it was worth writing up. And also if I'm wrong, it would\nbe good to know. ;)\n\n-Peff\n"},{"id":"327819","messageId":"2b7c0053-bf7a-fbdd-3cf9-39b5d9a962c3@alum.mit.edu","threadId":"46672","inReplyTo":"20170909103131.pppm346qbj2cdxuo@sigill.intra.peff.net","subject":"Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-10T04:45:08Z","receivedAt":"2017-09-10T04:45:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2017 12:31 PM, Jeff King wrote:\n> On Sat, Aug 26, 2017 at 10:28:00AM +0200, Michael Haggerty wrote:\n> \n>> It turns out that the comment is incorrect, but there was nevertheless\n>> plenty that could be cleaned up in the area:\n>>\n>> * Make macro `GIT_NIBBLE` safer by adding some parentheses\n>> * Remove some dead code\n>> * Fix some memory leaks\n>> * Fix some obsolete and incorrect comments\n>> * Reject \"notes\" that are not blobs\n>>\n>> I hope the result is also easier to understand.\n>>\n>> This branch is also available from my Git fork [1] as branch\n>> `load-subtree-cleanup`.\n> \n> FYI, Coverity seems to complain about \"pu\" after this series is merged, but\n> I think it's wrong.  It says:\n> \n>   *** CID 1417630:  Memory - illegal accesses  (OVERRUN)\n>   /notes.c: 458 in load_subtree()\n>   452     \n>   453     \t\t\t/*\n>   454     \t\t\t * Pad the rest of the SHA-1 with zeros,\n>   455     \t\t\t * except for the last byte, where we write\n>   456     \t\t\t * the length:\n>   457     \t\t\t */\n>   >>>     CID 1417630:  Memory - illegal accesses  (OVERRUN)\n>   >>>     Overrunning array of 20 bytes at byte offset 20 by dereferencing pointer \"&object_oid.hash[len]\".\n>   458     \t\t\tmemset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ - len - 1);\n>   459     \t\t\tobject_oid.hash[KEY_INDEX] = (unsigned char)len;\n>   460     \n>   461     \t\t\ttype = PTR_TYPE_SUBTREE;\n>   462     \t\t} else {\n>   463     \t\t\t/* This can't be part of a note */\n> \n> I agree that if \"len\" were 20 here that would be a problem, but I don't\n> think that's possible.\n> \n> The tool correctly claims that prefix_len can be up to 19, due to the\n> assert:\n> \n>      3. cond_at_most: Checking prefix_len >= 20UL implies that prefix_len may be up to 19 on the false branch.\n>   420        if (prefix_len >= GIT_SHA1_RAWSZ)\n>   421                BUG(\"prefix_len (%\"PRIuMAX\") is out of range\", (uintmax_t)prefix_len);\n> \n> Then it claims:\n> \n>     13. Condition path_len == 2 * (20 - prefix_len), taking false branch.\n>   430                if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {\n>   431                        /* This is potentially the remainder of the SHA-1 */\n> \n> So we know that either prefix_len is not 19, or that path_len is not 2\n> (since that combination would cause us to take the true branch here).\n> But then it goes on to say:\n> \n>     14. Condition path_len == 2, taking true branch.\n>   442                } else if (path_len == 2) {\n>   443                        /* This is potentially an internal node */\n> \n> which I believe must mean that prefix_len cannot be 19 here. And yet it\n> says:\n> \n>     15. assignment: Assigning: len = prefix_len. The value of len may now be up to 19.\n>   444                        size_t len = prefix_len;\n>   445\n>   [...]\n>      17. incr: Incrementing len. The value of len may now be up to 20.\n>      18. Condition hex_to_bytes(&object_oid.hash[len++], entry.path, 1), taking false branch.\n>   450                        if (hex_to_bytes(object_oid.hash + len++, entry.path, 1))\n>   451                                goto handle_non_note; /* entry.path is not a SHA1 */\n> \n> I think that's impossible, and Coverity simply isn't smart enough to\n> shrink the set of possible values for prefix_len based on the set of\n> if-else conditions.\n> \n> So nothing to see here, but since I spent 20 minutes scratching my head\n> (and I know others look at Coverity output and may scratch their heads\n> too), I thought it was worth writing up. And also if I'm wrong, it would\n> be good to know. ;)\n\nThanks for looking into this. I agree with your analysis.\n\nI wonder whether it is the factor of two between path lengths and byte\nlengths that is confusing Coverity. Perhaps the patch below would help.\nIt requires an extra, superfluous, check, but perhaps makes the code a\ntad more readable. I'm neutral on whether we would want to make the change.\n\nIs there a way to ask Coverity whether a hypothetical change would\nremove the warning, short of merging the change to master?\n\nMichael\n\ndiff --git a/notes.c b/notes.c\nindex 27d232f294..34f623f7b1 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -426,8 +426,14 @@ static void load_subtree(struct notes_tree *t,\nstruct leaf_node *subtree,\n \t\tunsigned char type;\n \t\tstruct leaf_node *l;\n \t\tsize_t path_len = strlen(entry.path);\n+\t\tsize_t path_bytes;\n\n-\t\tif (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {\n+\t\tif (path_len % 2 != 0)\n+\t\t\t/* Path chunks must come in pairs of hex characters */\n+\t\t\tgoto handle_non_note;\n+\n+\t\tpath_bytes = path_len / 2;\n+\t\tif (path_bytes == GIT_SHA1_RAWSZ - prefix_len) {\n \t\t\t/* This is potentially the remainder of the SHA-1 */\n\n \t\t\tif (!S_ISREG(entry.mode))\n@@ -439,7 +445,7 @@ static void load_subtree(struct notes_tree *t,\nstruct leaf_node *subtree,\n \t\t\t\tgoto handle_non_note; /* entry.path is not a SHA1 */\n\n \t\t\ttype = PTR_TYPE_NOTE;\n-\t\t} else if (path_len == 2) {\n+\t\t} else if (path_bytes == 1) {\n \t\t\t/* This is potentially an internal node */\n \t\t\tsize_t len = prefix_len;\n\n"},{"id":"327825","messageId":"20170910073928.ys4nbap76tmiurjh@sigill.intra.peff.net","threadId":"46672","inReplyTo":"2b7c0053-bf7a-fbdd-3cf9-39b5d9a962c3@alum.mit.edu","subject":"Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-10T07:39:28Z","receivedAt":"2017-09-10T07:39:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 10, 2017 at 06:45:08AM +0200, Michael Haggerty wrote:\n\n> > So nothing to see here, but since I spent 20 minutes scratching my head\n> > (and I know others look at Coverity output and may scratch their heads\n> > too), I thought it was worth writing up. And also if I'm wrong, it would\n> > be good to know. ;)\n> \n> Thanks for looking into this. I agree with your analysis.\n> \n> I wonder whether it is the factor of two between path lengths and byte\n> lengths that is confusing Coverity. Perhaps the patch below would help.\n> It requires an extra, superfluous, check, but perhaps makes the code a\n> tad more readable. I'm neutral on whether we would want to make the change.\n\nYeah, I do agree that it makes the code's assumptions a bit easier to\nfollow.\n\n> Is there a way to ask Coverity whether a hypothetical change would\n> remove the warning, short of merging the change to master?\n\nYou can download and run the build portion of the coverity tools\nyourself. IIRC, that pushes the build up to their servers which then do\nthe analysis (you can make your own \"project\", or use the existing \"git\"\nproject -- I checked and you are already listed as an admin). I recall\nit being a minor pain to get it set up, but not too bad.\n\nStefan runs it against \"pu\" on a regular basis, which is where the\nemailed results come from. So just having Junio merge it to \"pu\" would\nbe enough to get results.\n\nI noticed that they now have some GitHub/Travis integration:\n\n  https://scan.coverity.com/github\n\nI'm not sure if that is new, or if we just didn't notice it before. ;)\nBut that probably makes more sense to use than ad-hoc uploading (and\nmaybe it would make it easy for you to test personal branches, too).\n\n-Peff\n"},{"id":"327875","messageId":"CAMy9T_GGrb3+9n2nMT78mk8_4SGdf=EbS4gDYtvM1CA-i3+xXQ@mail.gmail.com","threadId":"46672","inReplyTo":"20170910073928.ys4nbap76tmiurjh@sigill.intra.peff.net","subject":"Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-12T06:47:14Z","receivedAt":"2017-09-12T06:47:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Sun, Sep 10, 2017 at 9:39 AM, Jeff King <peff@peff.net> wrote:\n> On Sun, Sep 10, 2017 at 06:45:08AM +0200, Michael Haggerty wrote:\n>\n>> > So nothing to see here, but since I spent 20 minutes scratching my head\n>> > (and I know others look at Coverity output and may scratch their heads\n>> > too), I thought it was worth writing up. And also if I'm wrong, it would\n>> > be good to know. ;)\n>>\n>> Thanks for looking into this. I agree with your analysis.\n>>\n>> I wonder whether it is the factor of two between path lengths and byte\n>> lengths that is confusing Coverity. Perhaps the patch below would help.\n>> It requires an extra, superfluous, check, but perhaps makes the code a\n>> tad more readable. I'm neutral on whether we would want to make the change.\n>\n> Yeah, I do agree that it makes the code's assumptions a bit easier to\n> follow.\n>\n>> Is there a way to ask Coverity whether a hypothetical change would\n>> remove the warning, short of merging the change to master?\n>\n> You can download and run the build portion of the coverity tools\n> yourself. [...]\n\nThanks for the info.\n\nMy suggested tweak doesn't appease Coverity. Given that, I don't think\nI'll bother adding it to the patch series.\n\nMichael\n"},{"id":"327886","messageId":"85A146BA-6AF8-455A-96F5-A81CE25FE220@gmail.com","threadId":"46672","inReplyTo":"20170910073928.ys4nbap76tmiurjh@sigill.intra.peff.net","subject":"Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-09-12T11:55:47Z","receivedAt":"2017-09-12T11:55:55Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Sep 2017, at 09:39, Jeff King <peff@peff.net> wrote:\n> \n> On Sun, Sep 10, 2017 at 06:45:08AM +0200, Michael Haggerty wrote:\n> \n>>> So nothing to see here, but since I spent 20 minutes scratching my head\n>>> (and I know others look at Coverity output and may scratch their heads\n>>> too), I thought it was worth writing up. And also if I'm wrong, it would\n>>> be good to know. ;)\n>> \n>> Thanks for looking into this. I agree with your analysis.\n>> \n>> I wonder whether it is the factor of two between path lengths and byte\n>> lengths that is confusing Coverity. Perhaps the patch below would help.\n>> It requires an extra, superfluous, check, but perhaps makes the code a\n>> tad more readable. I'm neutral on whether we would want to make the change.\n> \n> Yeah, I do agree that it makes the code's assumptions a bit easier to\n> follow.\n> \n>> Is there a way to ask Coverity whether a hypothetical change would\n>> remove the warning, short of merging the change to master?\n> \n> You can download and run the build portion of the coverity tools\n> yourself. IIRC, that pushes the build up to their servers which then do\n> the analysis (you can make your own \"project\", or use the existing \"git\"\n> project -- I checked and you are already listed as an admin). I recall\n> it being a minor pain to get it set up, but not too bad.\n> \n> Stefan runs it against \"pu\" on a regular basis, which is where the\n> emailed results come from. So just having Junio merge it to \"pu\" would\n> be enough to get results.\n> \n> I noticed that they now have some GitHub/Travis integration:\n> \n>  https://scan.coverity.com/github\n> \n> I'm not sure if that is new, or if we just didn't notice it before. ;)\n> But that probably makes more sense to use than ad-hoc uploading (and\n> maybe it would make it easy for you to test personal branches, too).\n\nCoverity scans Git already:\nhttps://scan.coverity.com/projects/70\n\nI requested access to this Coverity project to integrate into our TravisCI\nbuild.\n\n- Lars"}]}