Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id
From: Jeff King <hidden>
Date: 2017-02-18 03:15:56
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Fri, Feb 17, 2017 at 08:42:17PM -0500, Jeff King wrote:
quoted
I'm wondering if parse_oid_hex could be useful here as well.I know I haven't looked at this chunk nearly as carefully as you have, but it seems somewhat crazy to me that these functions get the original "line" in the first place. Shouldn't they get line+40 from the caller (who in turn should be using parse_oid_hex to compute that)? And then each function should subsequently parse left-to-right with a mix of isspace() and parse_oid_hex(), and probably doesn't even need to care about the original "len" at all (yes, you can quit early if you know your len isn't long enough, but that's the unusual error case anyway; it's not a big deal to find that out while parsing). In general, I think this sort of left-to-right incremental pointer movement is safe and simple. There may be a few cases where it doesn't apply (i.e., where you need to look at the end of the string to know how to parse the beginning), but that should be relatively rare.
So for this case, something like the patch below. Incidentally, there's an off-by-one in the original loop of stdin_diff_commit that reads past the end of the trailing NUL for the final sha1 on the line. The problem is the: pos += GIT_SHA1_HEXSZ + 1; which assumes we're slurping up the trailing space. This works in practice because the caller will only permit a string which had a newline (which it converted into a NUL). I suspect that function could be more aggressive about complaining about nonsense on the line, rather than silently ignoring it. builtin/diff-tree.c | 43 ++++++++++++++++------------------- 1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 1f1573bb2..222c671f2 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c@@ -16,37 +16,33 @@ static int diff_tree_commit_sha1(const struct object_id *oid) } /* Diff one or more commits. */ -static int stdin_diff_commit(struct commit *commit, char *line, int len) +static int stdin_diff_commit(struct commit *commit, const char *p) { struct object_id oid; - if (isspace(line[GIT_SHA1_HEXSZ]) && !get_oid_hex(line+GIT_SHA1_HEXSZ+1, &oid)) { - /* Graft the fake parents locally to the commit */ - int pos = GIT_SHA1_HEXSZ + 1; - struct commit_list **pptr; - - /* Free the real parent list */ - free_commit_list(commit->parents); - commit->parents = NULL; - pptr = &(commit->parents); - while (line[pos] && !get_oid_hex(line + pos, &oid)) { - struct commit *parent = lookup_commit(oid.hash); - if (parent) { - pptr = &commit_list_insert(parent, pptr)->next; - } - pos += GIT_SHA1_HEXSZ + 1; + struct commit_list **pptr = NULL; + + /* Graft the fake parents locally to the commit */ + while (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) { + struct commit *parent = lookup_commit(oid.hash); + if (!pptr) { + /* Free the real parent list */ + free_commit_list(commit->parents); + commit->parents = NULL; + pptr = &(commit->parents); + } + if (parent) { + pptr = &commit_list_insert(parent, pptr)->next; } } return log_tree_commit(&log_tree_opt, commit); } /* Diff two trees. */ -static int stdin_diff_trees(struct tree *tree1, char *line, int len) +static int stdin_diff_trees(struct tree *tree1, const char *p) { struct object_id oid; struct tree *tree2; - const int chunksz = GIT_SHA1_HEXSZ + 1; - if (len != 2 * chunksz || !isspace(line[chunksz-1]) || - get_sha1_hex(line + chunksz, oid.hash)) + if (!isspace(*p++) || parse_oid_hex(p, &oid, &p) || *p) return error("Need exactly two trees, separated by a space"); tree2 = lookup_tree(oid.hash); if (!tree2 || parse_tree(tree2))
@@ -64,19 +60,20 @@ static int diff_tree_stdin(char *line) int len = strlen(line); struct object_id oid; struct object *obj; + const char *rest; if (!len || line[len-1] != '\n') return -1; line[len-1] = 0; - if (get_oid_hex(line, &oid)) + if (parse_oid_hex(line, &oid, &rest)) return -1; obj = parse_object(oid.hash); if (!obj) return -1; if (obj->type == OBJ_COMMIT) - return stdin_diff_commit((struct commit *)obj, line, len); + return stdin_diff_commit((struct commit *)obj, rest); if (obj->type == OBJ_TREE) - return stdin_diff_trees((struct tree *)obj, line, len); + return stdin_diff_trees((struct tree *)obj, rest); error("Object %s is a %s, not a commit or tree", oid_to_hex(&oid), typename(obj->type)); return -1;