Re: [PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> diff --git a/mktag.c b/mktag.c
> index b82e377..0bc20c8 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -32,52 +32,48 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
>  	return ret;
>  }
>  
> +static int verify_tag(char *data, unsigned long size)
> +{
>  #ifdef NO_C99_FORMAT
>  #define PD_FMT "%d"
>  #else
>  #define PD_FMT "%td"
>  #endif
>  
> -static int verify_tag(char *buffer, unsigned long size)

Why this rename from buffer to data?

> -{
> -	int typelen;
> -	char type[20];
>  	unsigned char sha1[20];
> -	const char *object, *type_line, *tag_line, *tagger_line;
> +	char type[20];
> +	const char *type_line, *tag_line, *tagger_line;
> +	unsigned long type_len;

Why this change of order?

>  	if (size < 64)
>  		return error("wanna fool me ? you obviously got the size wrong !");
>  
> -	buffer[size] = 0;

Are you sure that your buffer is always NUL terminated?

> -	type_line = object + 48;
> +	type_line = data + 48;

Quite a lot of changes seem to do this object->data. The patch would have 
been much more compact if you just had renamed buffer to object instead of 
data.

> -	typelen = tag_line - type_line - strlen("type \n");
> -	if (typelen >= sizeof(type))
> -		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
> -
> -	memcpy(type, type_line+5, typelen);
> -	type[typelen] = 0;
> +	type_len = tag_line - type_line - strlen("type \n");
> +	if (type_len >= sizeof(type))
> +		return error("char" PD_FMT ": type too long", type_line + 5 - data);
> +	memcpy(type, type_line + 5, type_len);
> +	type[type_len] = '\0';

This renaming variables has nothing to do with refactoring. In fact, I 
have a hard time to find code changes (which your subject suggests, as you 
want to make two functions more similar).

>  	/* TODO: check for committer info + blank line? */
>  	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
>  
>  	/* The actual stuff afterwards we don't care about.. */
>  	return 0;
> -}
>  
>  #undef PD_FMT
> +}

Any particular reason for this?

> @@ -124,6 +120,7 @@ int main(int argc, char **argv)
>  		free(buffer);
>  		die("could not read from stdin");
>  	}
> +	buffer[size] = 0;

Ah, so you terminate the buffer here. From the patch, it is relatively 
hard to see if this line is always hit _before_ the function is called 
that evidently relies on NUL termination. By moving it here, I think it is 
much more likely to overlook the fact that the function, which made sure 
that its assumption was met, needs this line. Whereas if you left it where 
it was, the assumption would always be met.

> -        if (item->object.parsed)
> -                return 0;
> -        item->object.parsed = 1;
> +	if (item->object.parsed)
> +		return 0;
> +	item->object.parsed = 1;

Again, this has nothing to do with refactoring.

> @@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
>  	if (memcmp(data, "object ", 7))
>  		return error("char%d: does not start with \"object \"", 0);
>  
> -	if (get_sha1_hex((char *) data + 7, sha1))
> +	if (get_sha1_hex(data + 7, sha1))

Is this really necessary? Even if (which I doubt), it has nothing to do 
with refactoring.

If you _want_ to _explicitely_ do arithmetic on a char* instead of void*, 
why not DRT and change the function signature?

> -	sig_line++;
> +	tagger_line++;

I am really reluctant with renamings like these. IMHO they don't buy you 
much, except for possible confusion. It is evident that sig means the 
signer (and it is obvious in the case of an unsigned tag, who is meant, 
too).

> +int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
> +{
> +	return parse_tag_buffer_internal(item, (const char *) data, size);
> +}

This cast (and indeed, this function, if you ask me) is unnecessary.

I reviewed only this patch out of your long series, mostly because I found 
the subject line interesting. But IMHO the patch does not what the subject 
line suggests.

Unfortunately, it's unlikely that I will have time until Monday night to 
continue with this patch series.

Ciao,
Dscho