On 05/17/2016 08:58 PM, Junio C Hamano wrote:
tboegi@xxxxxx 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html