patchFix crlf attribute handling to match documentation

gitattributes.txt says, of the crlf attribute:

 Set::
    Setting the `crlf` attribute on a path is meant to mark
    the path as a "text" file.  'core.autocrlf' conversion
    takes place without guessing the content type by
    inspection.

That is to say that the crlf attribute does not force the file to have CRLF line endings, instead it removes the autocrlf guesswork and forces the file to be treated as text. Then, whatever line ending is defined by the autocrlf setting is applied.

However, that is not what convert.c was doing. The conversion to CRLF was being skipped in crlf_to_worktree() when the following condition was true:

 action == CRLF_GUESS && auto_crlf <= 0

That is to say conversion took place when not in guess mode (crlf attribute not specified) or core.autocrlf set to true. This was wrong. It meant that the crlf attribute being on for a given file _forced_ CRLF conversion, when actually it should force the file to be treated as text, and converted accordingly. The real test should simply be

 auto_crlf <= 0

That is to say, if core.autocrlf is falsei (or input), conversion from LF to CRLF is never done. When core.autocrlf is true, conversion from LF to CRLF is done only when in CRLF_GUESS (and the guess is "text"), or CRLF_TEXT mode.

Similarly for crlf_to_worktree(), if core.autocrlf is false, no conversion should _ever_ take place. In reality it was only not taking place if core.autocrlf was false _and_ the crlf attribute was unspecified.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 convert.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Andy Parkins <andyparkins@gmail.com> writes:

Thanks.  I'll take a look later.

Some addition to the existing test t/t0020-crlf.sh to demonstrate this bug and your fix would be appropriate.

Andy Parkins <andyparkins@gmail.com> writes:

Thanks; I looked at the patch (although I am still _physically_ at work ;-). I think your code is correct but the explanation is slightly misleading.

The check you modified in the first hunk is not the above '<=' comparison but is this:

	(action == CRLF_GUESS && !auto_crlf)

and "core.autocrlf = input" makes "auto_crlf = -1", so when action is not GUESS, or even when action is GUESS, if the config is set to "input", the if() statement you patched in the first hunk should not trigger. The above description is different from what the code was doing.

The logic (in crlf_to_git, which is the input codepath) should be:

	* if action is BINARY, do nothing (obviously -- and the
          code gets this right).

	* if action is GUESS, do nothing if config says false
          (we want 'input' and 'true' to apply the munging after
          guessing).

	* if action is TEXT or INPUT, do not guess but do CRLF
          only on platforms that need it -- which means where
          auto_crlf is -1 (input) or 1 (true).  Otherwise do not
          do the conversion.

The original code gets the third case incorrectly, and your patch fixes it by returning early in that case as well.

The output codepath is the same. Regardless of action (GUESS/TEXT/INPUT), we will not do anything if config says 'false' or 'input', so removing the check with "action == GUESS" is the right thing.

On Saturday 2007, May 19, Junio C Hamano wrote:

I was talking about the second hunk there though, which I mentioned with "...skipped in crlf_to_worktree()...". In which case the description matches.

As I say, the description was for the second hunk.

The fault was my last paragraph, "Similarly for crlf_to_worktree(), if core.autocrlf is false, no", which should have said "Similarly for crlf_to_git()". Sorry.

With that small change I think my description was correct. However, it obviously wasn't clear - apologies.

Andy