Re: [PATCH] archive-zip: add --text parameter

From: René Scharfe <l.s.r@web.de>
Date: Mar 5, 2015, 15:27 UTC
Message-ID: <54F875EC.2020600@web.de>
In-Reply-To: <xmqq7fuw5guc.fsf@gitster.dls.corp.google.com>

Am 05.03.2015 um 03:16 schrieb Junio C Hamano:

Show 11 quoted lines

> René Scharfe <l.s.r@web.de> writes:
>
>> No sign-off, yet, because I'm not sure we really need another option.
>> E.g. --text=all doesn't seem to be actually useful, but it was easy to
>> implement.  Info-ZIP's zip always creates archives like --text=auto
>> does, so perhaps we should make that our default behavior as well?
>
> My knee-jerk reaction is "yeah, why not? what are the downsides,
> other than the result will not be bit-for-bit identical to the
> output from older Git".  I am sure I am missing something as I do
> not regularly use this format.

AFAICS there won't be any other downsides. And archive stability is harder to achieve for ZIP anyway because it depends on compression level and (more fundamentally) on libz version.

Show 12 quoted lines

>> @@ -256,6 +264,8 @@ static int write_zip_entry(struct archiver_args *args,
>>   				return error("cannot read %s",
>>   					     sha1_to_hex(sha1));
>>   			crc = crc32(crc, buffer, size);
>> +			if (is_binary < 0)
>> +				is_binary = buffer_is_binary(buffer, size);
>
> In this codepath, do you have the path of the thing the buffer
> contents came from?  I am wondering if consulting the attributes
> system is a better idea. Anything that is explicitly marked as
> "binary" or "-diff" is definitely binary, and anything that is not
> marked as "binary" is text to us for all practical purposes, no?

Yes, attributes can help, especially to allow users to correct wrong guesses of the heuristic. Offering automatic detection of binary files by default like git diff and git grep is still a good idea, I think. buffer_is_binary() doesn't add a lot of overhead since it only looks at the first few bytes of the buffer.

René