From: Torsten Bögershausen <tboegi@web.de>
Date: Wed, 18 May 2016 04:26:23 GMT
Subject: Re: [PATCH v4 2/2] convert: ce_compare_data() checks for a sha1 of a path
Message-ID: <573BEEEF.8040305@web.de>
In-Reply-To: <xmqq60ucwlz8.fsf@gitster.mtv.corp.google.com>

On 05/17/2016 08:58 PM, Junio C Hamano wrote:
> tboegi@web.de writes:
>
>>   #define HASH_WRITE_OBJECT 1
>>   #define HASH_FORMAT_CHECK 2
>> +#define HASH_CE_HAS_SHA1  4
>
> How does one pronounce the words in this constant?  Does it make a
> listener understand what this constant means?
How about
HASH_USE_SHA1_FROM_CE
or
HASH_CE_HAS_VALID_SHA1


>
>
> /*
>   * We need a comment around here to say what these two
>   * parameters mean.  I am guessing that (1) if sha1 is not NULL,
>   * path is ignored and the function inspects if it has CR; (2)
>   * otherwise it checks the index entry at path and inspects if
>   * it has CR.
>   */
>>
>> -static int has_cr_in_index(const char *path)
>> +static int has_cr_in_index(const char *path, const unsigned char *sha1)
>>   {
>
> This makes me seriously wonder if it is a good idea to modify this
> function like this.  (1) means this function is not about IN INDEX
> at all!
>
> Perhaps add a "static int blob_has_cr(const unsigned char *sha1)"
> and call it from the real caller you added that wants to call this
> butchered two-param version that has sha1 is a better solution?
>
>> -static int crlf_to_git(const char *path, const char *src, size_t len,
>> +static int crlf_to_git(const char *path, const unsigned char *sha1,
>> +		       const char *src, size_t len,
>>   		       struct strbuf *buf,
>>   		       enum crlf_action crlf_action, enum safe_crlf checksafe)
>>   {
>> @@ -260,7 +267,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>>   			 * If the file in the index has any CR in it, do not convert.
>>   			 * This is the new safer autocrlf handling.
>>   			 */
>> -			if (has_cr_in_index(path))
>> +			if (has_cr_in_index(path, sha1))

>
> I think this change is too ugly.  The new "sha1" parameter is
> telling us that "in order to see if the indexed version has CR, do
> not look at the index, but look at the contents of this blob".
Thanks for some fresh eyes, I guess that
blob_has_cr(sha1)
would make most sense.