Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

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

quoted text
> -{
> -	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?

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

quoted text
> -	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.

quoted text
> -	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] = '__PLACEHOLDER__4_';

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

quoted text
>  	/* 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?

quoted text
> @@ -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.

quoted text
> -        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.

quoted text
> @@ -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?

quoted text
> -	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).

quoted text
> +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

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html