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

["René Scharfe" <l.s.r@web.de>]

Am 05.03.2015 um 03:16 schrieb Junio C Hamano:
> 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.

>> @@ -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é