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

[Junio C Hamano <gitster@pobox.com>]

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?

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