Re: [PATCH 3/5] combine-diff: handle binary files as binary

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2011, 06:33 UTC
Message-ID: <7vpqn0ofy5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110523202734.GC6298@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

> @@ -852,6 +862,29 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
>  		}
>  	}
>  
> +	if (userdiff->binary != -1)
> +		is_binary = userdiff->binary;
> +	else {
> +		is_binary = buffer_is_binary(result, result_size);
> +		for (i = 0; !is_binary && i < num_parent; i++) {
> +			char *buf;
> +			unsigned long size;
> +			buf = grab_blob(elem->parent[i].sha1,
> +					elem->parent[i].mode,
> +					&size);
> +			if (buffer_is_binary(buf, size))
> +				is_binary = 1;
> +			free(buf);
> +		}
> +	}

Two comments.

 - This loop will grab the blob from all parents just to peek and discard
   for most of commits. It feels somewhat wasteful, especially because
   "binary" that is not marked as binary is a rare minor case (most of the
   paths are text). Stopping immediately at the first binary blob does not
   optimize the loop even though it is better than not having one.

 - It may make sense to compare [i].sha1 with earlier [j].sha1 (j < i) and
   avoid grab_blob() altogether?  Cf. 3c39e9b (combine-diff: reuse diff
   from the same blob., 2006-02-01).