Re: [PATCH v4 2/2] convert: ce_compare_data() checks for a sha1 of a path

From: Junio C Hamano <gitster@pobox.com>
Date: May 17, 2016, 18:58 UTC
Message-ID: <xmqq60ucwlz8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1463503301-3634-1-git-send-email-tboegi@web.de>

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?

/*
 * 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?

Show 12 quoted lines

> -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".

But wouldn't the result become more understandable if instead we passed a new parameter "cr-state-for-conversion" that takes UNKNOWN, HAS_CR, or NO_CR to this function?

In other words, what if, when the caller knows it does not care what's in the index but wants to instead see if a different blob has CR, we make it the caller's responsibility to figure it out by calling blob_has_cr() before calling into this codepath and pass the result of that check down? When cr-state-for-conversion is UNKNOWN, this code knows that it needs to call has_cr_in_index(path) to figure it out itself. Otherwise, it can and should use the caller-supplied value without asking has_cr_in_index(path).