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

[Duy Nguyen]

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(, , );
> 
> 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();
@@ -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(, 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(>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, 

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

[Patryk Obara]

On 22 January 2018 at 12:56, Duy Nguyen  wrote:
> On Mon, Jan 22, 2018 at 12:04:30PM +0100, Patryk Obara wrote:
>> Convert the definition of static recursive splice_tree function to use
>> struct object_id and adjust single caller.
>>
>> Signed-off-by: Patryk Obara 
>> ---
>>  match-trees.c | 42 --
>>  1 file changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/match-trees.c b/match-trees.c
>> index 396b7338df..0f899a7212 100644
>> --- a/match-trees.c
>> +++ b/match-trees.c
>> @@ -161,19 +161,17 @@ static void match_trees(const struct object_id *hash1,
>>   * A tree "hash1" has a subdirectory at "prefix".  Come up with a
>>   * tree object by replacing it with another tree "hash2".
>>   */
>> -static int splice_tree(const unsigned char *hash1,
>> -const char *prefix,
>> -const unsigned char *hash2,
>> -unsigned char *result)
>> +static int splice_tree(const struct object_id *hash1, const char *prefix,
>> +const struct object_id *hash2, struct object_id *result)
>
> Maybe change the names to oid1 and oid2 too. I had a "what?" moment
> when I read hash1->hash below.

OK

>> @@ -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(, , );

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.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

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

[Duy Nguyen]

On Mon, Jan 22, 2018 at 12:04:30PM +0100, Patryk Obara wrote:
> Convert the definition of static recursive splice_tree function to use
> struct object_id and adjust single caller.
> 
> Signed-off-by: Patryk Obara 
> ---
>  match-trees.c | 42 --
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/match-trees.c b/match-trees.c
> index 396b7338df..0f899a7212 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -161,19 +161,17 @@ static void match_trees(const struct object_id *hash1,
>   * A tree "hash1" has a subdirectory at "prefix".  Come up with a
>   * tree object by replacing it with another tree "hash2".
>   */
> -static int splice_tree(const unsigned char *hash1,
> -const char *prefix,
> -const unsigned char *hash2,
> -unsigned char *result)
> +static int splice_tree(const struct object_id *hash1, const char *prefix,
> +const struct object_id *hash2, struct object_id *result)

Maybe change the names to oid1 and oid2 too. I had a "what?" moment
when I read hash1->hash below.

> @@ -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?

--
Duy

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

[Patryk Obara]

Convert the definition of static recursive splice_tree function to use
struct object_id and adjust single caller.

Signed-off-by: Patryk Obara 
---
 match-trees.c | 42 --
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 396b7338df..0f899a7212 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -161,19 +161,17 @@ static void match_trees(const struct object_id *hash1,
  * A tree "hash1" has a subdirectory at "prefix".  Come up with a
  * tree object by replacing it with another tree "hash2".
  */
-static int splice_tree(const unsigned char *hash1,
-  const char *prefix,
-  const unsigned char *hash2,
-  unsigned char *result)
+static int splice_tree(const struct object_id *hash1, const char *prefix,
+  const struct object_id *hash2, struct object_id *result)
 {
char *subpath;
int toplen;
char *buf;
unsigned long sz;
struct tree_desc desc;
-   unsigned char *rewrite_here;
-   const unsigned char *rewrite_with;
-   unsigned char subtree[20];
+   struct object_id *rewrite_here;
+   const struct object_id *rewrite_with;
+   struct object_id subtree;
enum object_type type;
int status;
 
@@ -182,9 +180,9 @@ static int splice_tree(const unsigned char *hash1,
if (*subpath)
subpath++;
 
-   buf = read_sha1_file(hash1, , );
+   buf = read_sha1_file(hash1->hash, , );
if (!buf)
-   die("cannot read tree %s", sha1_to_hex(hash1));
+   die("cannot read tree %s", oid_to_hex(hash1));
init_tree_desc(, buf, sz);
 
rewrite_here = NULL;
@@ -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;
break;
}
update_tree_entry();
}
if (!rewrite_here)
-   die("entry %.*s not found in tree %s",
-   toplen, prefix, sha1_to_hex(hash1));
+   die("entry %.*s not found in tree %s", toplen, prefix,
+   oid_to_hex(hash1));
if (*subpath) {
-   status = splice_tree(rewrite_here, subpath, hash2, subtree);
+   status = splice_tree(rewrite_here, subpath, hash2, );
if (status)
return status;
-   rewrite_with = subtree;
-   }
-   else
+   rewrite_with = 
+   } else {
rewrite_with = hash2;
-   hashcpy(rewrite_here, rewrite_with);
-   status = write_sha1_file(buf, sz, tree_type, result);
+   }
+   oidcpy(rewrite_here, rewrite_with);
+   status = write_sha1_file(buf, sz, tree_type, result->hash);
free(buf);
return status;
 }
@@ -280,7 +278,7 @@ void shift_tree(const struct object_id *hash1,
if (!*add_prefix)
return;
 
-   splice_tree(hash1->hash, add_prefix, hash2->hash, shifted->hash);
+   splice_tree(hash1, add_prefix, hash2, shifted);
 }
 
 /*
@@ -334,7 +332,7 @@ void shift_tree_by(const struct object_id *hash1,
 * shift tree2 down by adding shift_prefix above it
 * to match tree1.
 */
-   splice_tree(hash1->hash, shift_prefix, hash2->hash, 
shifted->hash);
+   splice_tree(hash1, shift_prefix, hash2, shifted);
else
/*
 * shift tree2 up by removing shift_prefix from it
-- 
2.14.3