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

tboegi@web.de writes:

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.