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

From: Patryk Obara <patryk.obara@gmail.com>
Date: Jan 22, 2018, 13:12 UTC
Message-ID: <CAJfL8+TrFV=DkJPZAYYYc69recsYiOGdeV8Ev0r5xfcmmqP-Hw@mail.gmail.com>
In-Reply-To: <20180122115642.GB5980@ash>

On 22 January 2018 at 12:56, Duy Nguyen <pclouds@gmail.com> wrote:

Show 26 quoted lines

> 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

Show 12 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.

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