Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

On Sat, Feb 18, 2017 at 01:18:11AM +0000, Ramsay Jones wrote:

On 18/02/17 00:06, brian m. carlson wrote:

quoted

Convert most leaf functions to struct object_id.  Rewrite several
hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
variable where that makes sense.

Signed-off-by: brian m. carlson <redacted>
---
 builtin/diff-tree.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..1f1573bb2a 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c

@@ -7,9 +7,9 @@
 
 static struct rev_info log_tree_opt;
 
-static int diff_tree_commit_sha1(const unsigned char *sha1)
+static int diff_tree_commit_sha1(const struct object_id *oid)
 {
-	struct commit *commit = lookup_commit_reference(sha1);
+	struct commit *commit = lookup_commit_reference(oid->hash);
 	if (!commit)
 		return -1;
 	return log_tree_commit(&log_tree_opt, commit);

@@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)
 /* Diff one or more commits. */
 static int stdin_diff_commit(struct commit *commit, char *line, int len)
 {
-	unsigned char sha1[20];
-	if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
+	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 = 41;
+		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_sha1_hex(line + pos, sha1)) {
-			struct commit *parent = lookup_commit(sha1);
+		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 += 41;
+			pos += GIT_SHA1_HEXSZ + 1;
 		}
 	}
 	return log_tree_commit(&log_tree_opt, commit);

@@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)
 /* Diff two trees. */
 static int stdin_diff_trees(struct tree *tree1, char *line, int len)
 {
-	unsigned char sha1[20];
+	struct object_id oid;
 	struct tree *tree2;
-	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+	const int chunksz = GIT_SHA1_HEXSZ + 1;
+	if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
+		get_sha1_hex(line + chunksz, oid.hash))

I'm not sure that this is an improvement. The input expected in 'line'
is supposed to look like: '<sha1> + <space> + <sha1> + <\n>'. So your
'chunk' would be a <sha1> plus one 'char' of some sort. Except that the
caller of this function has already replaced the newline character with
a '\0' char (so strlen(line) would return 81), but still passes the
original line length! Also, note that this (and other functions in this
file) actually test for 'isspace(char)' rather than for a ' ' char!

Hmm, maybe just:

if (len < (2 * GIT_SHA1_HEXSZ + 1) || line[GIT_SHA1_HEXSZ] != ' ' ||
    get_sha1_hex(line + GIT_SHA1_HEXSZ + 1, oid.hash))

(or, perhaps, still call isspace() in this patch ...)

Well, I think it's strictly an improvement in that we have avoided
writing hardcoded constants[0].  I did intend it as a "hash plus one"
chunk, which is actually quite common throughout the code.

I'm wondering if parse_oid_hex could be useful here as well.

[0] If we change the hash size, all of the GIT_SHA1_HEXSZ constants can
be replaced with a variable that varies based on hash size, and the code
still works.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204