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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
