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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.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:

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

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