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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 8, 2010, 17:57 UTC
Message-ID: <vpqiq5t5rvd.fsf@bauges.imag.fr>
In-Reply-To: <1276004958-13540-2-git-send-email-clement.poulain@ensimag.imag.fr>

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?

Clément Poulain <clement.poulain@ensimag.imag.fr> writes:

Show 7 quoted lines

> +struct object_context {
> +	unsigned char tree[20];
> +	char path[PATH_MAX];
> +	unsigned mode;
> +};
> +#define OBJECT_CONTEXT_INIT  { 0, 0, 0 }
> +

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}

Show 11 quoted lines

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

>  int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
>  {
> +	struct object_context orc;

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

> +		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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/