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

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

tboegi@web.de writes:

> +int convert_to_git_ce_sha1(const char *path, const unsigned char *sha1,
> +			   const char *src, size_t len,
> +			   struct strbuf *dst, enum safe_crlf checksafe)

That's a strange name for the function, as "ce" and "sha1" gives no
hints what they are about.

If I understand correctly:

 - convert_to_git() is about converting <src, len> into <dst>, and
   "path" is not for reading the contents to be converted.  It is
   used to tell crlf_to_git() the index entry to pay attention to
   when defeating the usual conversion rules due to strange contents
   in the index (it also is used to report errors for safe-crlf
   check).

 - This one adds "sha1", and that is not about the contents to be
   converted, either.  Like "path", "sha1" is used to tell what blob
   to check when disabling the usual conversion rules.

Does the above description help in coming up with a better
description for the ce/sha1 thing?  The comment near the code that
uses them reads like so:

	/*
	 * If the file in the index has any CR in it, do not convert.
	 * This is the new safer autocrlf handling.
	 */

What is a good name to call the input to that logic?  "This
function, in addition to convert_to_git(), takes an extra parameter,
that tells it an extra piece of information 'X'"; what is X?

At the same time, the parameter "sha1" needs to be renamed to
clarify what object it is and what purpose it serves.  "sha1" alone
is an overly generic name, and it does not hint that it may not even
be given to the function, and that it doesn't have anything to do
with the contents <src, len> points at.

	Note. Perhaps you wanted _ce_sha1 suffix to tell the readers
	that it takes "an object name of the cache entry" that
	further affects the conversion?  If so the sha1 parameter
	should be renamed to match (and make it clear to readers
	that is what you meant).

The "sha1" is pretending to be the more important input for the
primary function of this function by being in early part of the
parameter list.  This may need to be rethought; we probably should
have done so as part of your previous refactoring of this file.

convert_to_git() takes the data for <path> in <src, len> and gives
result in <dst>, so these four should be its first parameters.  The
detail of the way the conversion works may be affected by additional
parameters, e.g. <checksafe> controls if extra warnings are given.

The <sha1> is to influence the conversion logic further to disable
the crlf-to-git conversion by inspecting a blob, and it tells the
function that the blob comes from an unusual place (as opposed to
the index entry at <path>).  So it should sit next to checksafe as
an auxiliary input parameter.

The logic implemented by the patch looks correct, but I'd have to
say that the result is an unmaintainable mess.  Right now, I can see
what is going on in the new code.  I am sure that in 6 months, with
poorly named helper functions and parameters, I will have hard time
recalling how the new code works.