Re: [PATCH 1/2] removes the artificial restriction tagsize < 8kb from git-mktag

From: Junio C Hamano <junkio@cox.net>
Date: May 22, 2006, 19:19 UTC
Message-ID: <7v1wulc1fe.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <4471CF5A.702@gmx.de>

Björn Engelmann <BjEngelmann@gmx.de> writes:

Show 10 quoted lines

> @@ -154,6 +154,7 @@ extern int ce_match_stat(struct cache_en
>  extern int ce_modified(struct cache_entry *ce, struct stat *st, int);
>  extern int ce_path_match(const struct cache_entry *ce, const char
> **pathspec);
>  extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int
> write_object, const char *type);
> +extern int read_pipe(int fd, char** return_buf, unsigned long*
> return_size);
>  extern int index_pipe(unsigned char *sha1, int fd, const char *type,
> int write_object);

I smell whitespace breakage around here...

Show 22 quoted lines

> diff --git a/mktag.c b/mktag.c
> index 2328878..79c466c 100644
> --- a/mktag.c
> +++ b/mktag.c
>...
> @@ -114,21 +114,24 @@ int main(int argc, char **argv)
>  
>      setup_git_directory();
>  
> -    // Read the signature
> -    size = 0;
> -    for (;;) {
> -        int ret = xread(0, buffer + size, MAXSIZE - size);
> -        if (ret <= 0)
> -            break;
> -        size += ret;
> +    if (read_pipe(0, &buffer, &size)) {
> +        free(buffer);
> +        die("could not read from stdin");
>      }
> -
> +

Please do not introduce lines with trailing whitespaces.

Show 7 quoted lines

>      // Verify it for some basic sanity: it needs to start with "object
> <sha1>\ntype\ntagger "
> -    if (verify_tag(buffer, size) < 0)
> +    if (verify_tag(buffer, size) < 0) {
> +        free(buffer);
>          die("invalid tag signature file");
> +    }

You seem to be striving for extra cleanliness, but I personally consider it is not worth calling free() immediately before you call die().

Show 12 quoted lines

>  
> -    if (write_sha1_file(buffer, size, tag_type, result_sha1) < 0)
> +    if (write_sha1_file(buffer, size, tag_type, result_sha1) < 0) {
> +        free(buffer);
>          die("unable to write tag file");
> +    }
> +        
> +    free(buffer);
> +    
>      printf("%s\n", sha1_to_hex(result_sha1));
>      return 0;
>  }

A call to free() immediately before returning from main() might look similar to the die() issue I mentioned above, but we might extend it to do a lot more after writing the tag in the future, so this one is very good to keep.