On Mon, Jan 22, 2018 at 02:12:56PM +0100, Patryk Obara wrote:
> >> @@ -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< --
