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

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2016, 17:46 UTC
Message-ID: <xmqqshxcei66.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1463764366-21683-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

Show 29 quoted lines

> From: Torsten Bögershausen <tboegi@web.de>
>
> To compare a file in working tree with the index, convert_to_git() is used,
> the result is hashed and the hash value compared with ce->sha1.
>
> Deep down would_convert_crlf_at_commit() is invoked, to check if CRLF
> are converted or not: When a CRLF had been in the index before, CRLF in
> the working tree are not converted.
>
> While in a merge, a file name in the working tree has different blobs
> in the index with different hash values.
> Forwarding ce->sha1 from ce_compare_data() into crlf_to_git() makes sure
> the would_convert_crlf_at_commit() looks at the appropriate blob.
>
> Forward sha1 from ce_compare_data() into convert_to_git().
> All other callers use NULL, and the sha1 it is determined from path using
> get_sha1_from_cache(path), this is the same handling as before.
>
> Re-order the arguments for convert_to_git() according to their importance:
>   `src`, `len` and `dst` are the place in memory, where the conversion is done
>   `path` is the file name to look up the attributes.
>   `sha1` is needed by the "new safer autocrlf handling".
>   `checksafe` determines, if a warning is printed or an error is raised.
>
> In the same spirit, forward the sha1 into would_convert_to_git().
>
> While at it, rename has_cr_in_index() into blob_has_cr()
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

> Changes sinve v6:
>  decrease the messiness with 12 %

What does that mean????

>  convert_to_git() has a re-ordered parameter list.

That is not what I suggested, though. <path, src, len> being the first three would be fine, and that would be in line with helpers ${frotz}_to_git() that are used from there.

The primary thing that made me worried was a new parameter with a bland name "sha1" whose purpose is unclear was added near the beginning, leading readers to confusion.

Whether you keep <path> at the beginning of move it to later together with <sha1>, helpers like crlf_to_git() need to be updated to match. E.g. I would say that this

> -	ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
> +	ret |= crlf_to_git(path, sha1, src, len, dst, ca.crlf_action, checksafe);

would want to be ordered more like this:

    ret |= crlf_to_git(path, src, len, dst,
                       ca.crlf_action, checksafe, index_blob_sha1);

if you choose to keep the first four intact for convert_to_git(), that is.

>  Describe whats going on better in the commit msg.

The suggestion to rename the parameter was to allow readers of the code to immediately know what kind of SHA1 it is. They cannot afford to run "git blame" every time to find the commit to read the commit log message; for a public function like convert_to_git(), in-code comment is also necessary.