Re: [PATCH v2 07/14] match-trees: convert splice_tree to object_id

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 23, 2018, 01:29 UTC
Message-ID: <20180123012933.GA15378@duynguyen.dek-tpc.internal>
In-Reply-To: <CAJfL8+TrFV=DkJPZAYYYc69recsYiOGdeV8Ev0r5xfcmmqP-Hw@mail.gmail.com>

On Mon, Jan 22, 2018 at 02:12:56PM +0100, Patryk Obara wrote:

Show 48 quoted lines

> >> @@ -197,26 +195,26 @@ static int splice_tree(const unsigned char *hash1,
> >>               if (strlen(name) == toplen &&
> >>                   !memcmp(name, prefix, toplen)) {
> >>                       if (!S_ISDIR(mode))
> >> -                             die("entry %s in tree %s is not a tree",
> >> -                                 name, sha1_to_hex(hash1));
> >> -                     rewrite_here = (unsigned char *) oid->hash;
> >> +                             die("entry %s in tree %s is not a tree", name,
> >> +                                 oid_to_hex(hash1));
> >> +                     rewrite_here = (struct object_id *)oid;
> >
> > You don't need the typecast here anymore, do you?
> 
> Unfortunately, I do :(
> 
> Few lines above:
> 192: const struct object_id *oid;
> 194: oid = tree_entry_extract(&desc, &name, &mode);
> 
> Function tree_entry_extract returns const pointer, which leads to
> compiler warning:
> "assigning to 'struct object_id *' from 'const struct object_id *'
> discards qualifiers".
> 
> On the other hand, if I change const qualifier for 'rewrite_here'
> variable - warning will
> appear in line 216:
> 
> 216: oidcpy(rewrite_here, rewrite_with);
> 
> So the question here is rather: is it ok to overwrite buffer returned
> by tree_entry_extract?
> 
> When writing this I opted to preserve cv-qualifiers despite changing
> pointer type (which implied preservation of typecast) - partly
> because parameter 'desc' of tree_entry_extract is NOT const (which
> suggests to me, that it's ok).
> 
> But this cast might be indication of unintended modification inside
> tree description structure and might lead to an error is some other
> place, if there's an assumption, that this buffer is not
> overwritable.
> 
> Maybe const should be removed from return type of tree_entry_extract
> (and maybe from oid field of struct name_entry)?
>
> I will give it some more thought - maybe oidcpy from line 216 could
> be replaced.

I've read this code a bit more (sorry I didn't see the "const struct object_id *oid" line when I read this patch). I think the typecast is very much on purpose. Junio wanted to make a new tree with one different hash in 68faf68938 (A new merge stragety 'subtree'. - 2007-02-15) but I think he kinda abused the tree walker for this task.

A cleaner way is create a new tree by copying unmodified entries and replacing just one entry. I think the old way was ok when we dealt with SHA-1 directly, but with the object_id abstraction in place, this kind of update looks iffy.

Alternatively, perhaps we can do something like this to keep tree manipulation in tree-walk.c, one of the two places that know about tree object on-disk format (the other one is cache-tree.c)

-- 8< --

diff --git a/match-trees.c b/match-trees.c
index 396b7338df..a8dc8a53d9 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -171,7 +171,7 @@ static int splice_tree(const unsigned char *hash1,
 	char *buf;
 	unsigned long sz;
 	struct tree_desc desc;
-	unsigned char *rewrite_here;
+	const object_id *rewrite_here;
 	const unsigned char *rewrite_with;
 	unsigned char subtree[20];
 	enum object_type type;
@@ -199,7 +199,7 @@ static int splice_tree(const unsigned char *hash1,
 			if (!S_ISDIR(mode))
 				die("entry %s in tree %s is not a tree",
 				    name, sha1_to_hex(hash1));
-			rewrite_here = (unsigned char *) oid->hash;
+			rewrite_here = oid->hash;
 			break;
 		}
 		update_tree_entry(&desc);
@@ -215,7 +215,7 @@ static int splice_tree(const unsigned char *hash1,
 	}
 	else
 		rewrite_with = hash2;
-	hashcpy(rewrite_here, rewrite_with);
+	replace_tree_entry_hash(&desc, rewrite_with, buf, sz);
 	status = write_sha1_file(buf, sz, tree_type, result);
 	free(buf);
 	return status;
diff --git a/tree-walk.c b/tree-walk.c
index 63a87ed666..f31a03569f 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -164,6 +164,17 @@ int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
 	return 1;
 }
 
+void replace_tree_entry_hash(struct tree_desc *desc,
+			     const unsigned char *sha1,
+			     char *buf, unsigned long size)
+{
+	unsigned long offset = (const char *)desc->buffer - buf;
+	unsigned char *to_update;
+
+	to_update = (unsigned char *)buf + offset + tree_entry_len(&desc->entry);
+	hashcpy(to_update, sha1);
+}
+
 void setup_traverse_info(struct traverse_info *info, const char *base)
 {
 	int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index b6bd1b4ccf..9a7d133d68 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -35,6 +35,9 @@ int update_tree_entry_gently(struct tree_desc *);
 void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
 int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
 
+void replace_tree_entry_hash(struct tree_desc *desc,
+			     const unsigned char *sha1,
+			     char *buf, unsigned long size);
 /*
  * Helper function that does both tree_entry_extract() and update_tree_entry()
  * and returns true for success
-- 8< --