Re: [PATCH v2 1/4] sha1_name: add get_sha1_with_context()

["Clément Poulain" <clement.poulain@ensimag.imag.fr>]

Le 8 juin 2010 19:57, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> This patch produces uncompilable code for me:
>
> cc1: warnings being treated as errors
> In file included from builtin.h:6,
>                 from fast-import.c:147:
> cache.h: In function ‘get_sha1_with_context’:
> cache.h:748: error: implicit declaration of function ‘get_sha1_with_context_1’
>
> Forgot to add get_sha1_with_context_1 to cache.h?

Uh, we compiled it almost ten times on both our pc and ensibm (our
school server), whithout any problems. Seems that we need to check our
compilation configurations.

> I'm not an expert in struct initializers, but after doing experiments
> with GCC, this raises a warning
>
> builtin/cat-file.c:90: error: missing braces around initializer
> builtin/cat-file.c:90: error: (near initialization for ‘obj_context.tree’)
>
> and the behavior is to flatten the arrays contained inside the
> structure. So, your OBJECT_CONTEXT_INIT initializes the 3 first bytes
> of tree to 0, and leaves other fields uninitialized.
>
> You probably want something like this instead if you want to
> initialize the whole struct:
>
> {{0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>  0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, "", 0}

As you pointed out in your second answer, initialization is maybe no
required, we have to check it tomorrow.
Otherwise, an easy way to do it can be something like :
void object_context_init(struct object_context *oc)
{
	memset(oc, 0, sizeof(*oc));
}

>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -933,8 +933,8 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>>   */
>>  int get_sha1(const char *name, unsigned char *sha1)
>>  {
>> -     unsigned unused;
>> -     return get_sha1_with_mode(name, sha1, &unused);
>> +     struct object_context unused;
>> +     return get_sha1_with_context(name, sha1, &unused);
>>  }
>
> This changes doesn't seem harmful, but it doesn't seem useful to me
> either: get_sha1_with_mode still exists, right?

Right. But the aim was to skip one function call (see the call-stack below)
_with_mode => _with_mode_1 => _with_context_1
whereas:
 _with_context => _with_context_1

> What does orc stand for? I understand "oc" for "object context", but
> I'm curious about the r ;-).

"orc" was for "object resolve context". This is an artifact of our
previous version. We'll change it, it won't bother you no more ;-)

>> +             orc->path[sizeof(orc->path)] = '\0';
>> +
>
> Isn't this an off-by-one? The last element of an array of size N is
> array[N-1] ...
>
>> +                     orc->path[sizeof(orc->path)] = '\0';
>
> Same here.

That's true. Stupid error, we copied this line without checking it.