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

[Patryk Obara <patryk.obara@gmail.com>]

On 22 January 2018 at 12:56, Duy Nguyen <pclouds@gmail.com> 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 <patryk.obara@gmail.com>
>> ---
>>  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(&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.

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