{"thread":{"id":"8201","subject":"[PATCH] Fix crlf attribute handling to match documentation","startedAt":"2007-05-18T12:33:32Z","lastAt":"2007-05-19T09:58:29Z","messageCount":4,"participants":["Andy Parkins","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"42476","messageId":"200705181333.32719.andyparkins@gmail.com","threadId":"8201","inReplyTo":null,"subject":"[PATCH] Fix crlf attribute handling to match documentation","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-05-18T12:33:32Z","receivedAt":"2007-05-18T12:33:32Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"gitattributes.txt says, of the crlf attribute:\n\n Set::\n    Setting the `crlf` attribute on a path is meant to mark\n    the path as a \"text\" file.  'core.autocrlf' conversion\n    takes place without guessing the content type by\n    inspection.\n\nThat is to say that the crlf attribute does not force the file to have\nCRLF line endings, instead it removes the autocrlf guesswork and forces\nthe file to be treated as text.  Then, whatever line ending is defined\nby the autocrlf setting is applied.\n\nHowever, that is not what convert.c was doing.  The conversion to CRLF\nwas being skipped in crlf_to_worktree() when the following condition was\ntrue:\n\n action == CRLF_GUESS && auto_crlf <= 0\n\nThat is to say conversion took place when not in guess mode (crlf attribute\nnot specified) or core.autocrlf set to true.  This was wrong.  It meant\nthat the crlf attribute being on for a given file _forced_ CRLF\nconversion, when actually it should force the file to be treated as\ntext, and converted accordingly.  The real test should simply be\n\n auto_crlf <= 0\n\nThat is to say, if core.autocrlf is falsei (or input), conversion from\nLF to CRLF is never done.  When core.autocrlf is true, conversion from\nLF to CRLF is done only when in CRLF_GUESS (and the guess is \"text\"), or\nCRLF_TEXT mode.\n\nSimilarly for crlf_to_worktree(), if core.autocrlf is false, no conversion\nshould _ever_ take place.  In reality it was only not taking place if\ncore.autocrlf was false _and_ the crlf attribute was unspecified.\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n convert.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 12abdaf..86d8167 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -86,7 +86,7 @@ static char *crlf_to_git(const char *path, const char *src, unsigned long *sizep\n \tunsigned long size, nsize;\n \tstruct text_stat stats;\n \n-\tif ((action == CRLF_BINARY) || (action == CRLF_GUESS && !auto_crlf))\n+\tif ((action == CRLF_BINARY) || !auto_crlf)\n \t\treturn NULL;\n \n \tsize = *sizep;\n@@ -154,7 +154,7 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *\n \tunsigned char last;\n \n \tif ((action == CRLF_BINARY) || (action == CRLF_INPUT) ||\n-\t    (action == CRLF_GUESS && auto_crlf <= 0))\n+\t    auto_crlf <= 0)\n \t\treturn NULL;\n \n \tsize = *sizep;\n-- \n1.5.2.rc3.51.gd07bc\n"},{"id":"42500","messageId":"7v1whe6l5z.fsf@assigned-by-dhcp.cox.net","threadId":"8201","inReplyTo":"200705181333.32719.andyparkins@gmail.com","subject":"Re: [PATCH] Fix crlf attribute handling to match documentation","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-18T18:28:40Z","receivedAt":"2007-05-18T18:28:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> gitattributes.txt says, of the crlf attribute:\n> ...\n> However, that is not what convert.c was doing.\n\nThanks.  I'll take a look later.\n\nSome addition to the existing test t/t0020-crlf.sh to\ndemonstrate this bug and your fix would be appropriate.\n"},{"id":"42559","messageId":"7v646p4r50.fsf@assigned-by-dhcp.cox.net","threadId":"8201","inReplyTo":"200705181333.32719.andyparkins@gmail.com","subject":"Re: [PATCH] Fix crlf attribute handling to match documentation","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-19T00:02:35Z","receivedAt":"2007-05-19T00:02:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> gitattributes.txt says, of the crlf attribute:\n>\n>  Set::\n>     Setting the `crlf` attribute on a path is meant to mark\n>     the path as a \"text\" file.  'core.autocrlf' conversion\n>     takes place without guessing the content type by\n>     inspection.\n>\n> That is to say that the crlf attribute does not force the file to have\n> CRLF line endings, instead it removes the autocrlf guesswork and forces\n> the file to be treated as text.  Then, whatever line ending is defined\n> by the autocrlf setting is applied.\n\nThanks; I looked at the patch (although I am still _physically_\nat work ;-).  I think your code is correct but the explanation\nis slightly misleading.\n\n> However, that is not what convert.c was doing.  The conversion to CRLF\n> was being skipped in crlf_to_worktree() when the following condition was\n> true:\n>\n>  action == CRLF_GUESS && auto_crlf <= 0\n\nThe check you modified in the first hunk is not the above '<='\ncomparison but is this:\n\n\t(action == CRLF_GUESS && !auto_crlf)\n\nand \"core.autocrlf = input\" makes \"auto_crlf = -1\", so when\naction is not GUESS, or even when action is GUESS, if the config\nis set to \"input\", the if() statement you patched in the first\nhunk should not trigger.  The above description is different from\nwhat the code was doing.\n\nThe logic (in crlf_to_git, which is the input codepath) should be:\n\n\t* if action is BINARY, do nothing (obviously -- and the\n          code gets this right).\n\n\t* if action is GUESS, do nothing if config says false\n          (we want 'input' and 'true' to apply the munging after\n          guessing).\n\n\t* if action is TEXT or INPUT, do not guess but do CRLF\n          only on platforms that need it -- which means where\n          auto_crlf is -1 (input) or 1 (true).  Otherwise do not\n          do the conversion.\n\nThe original code gets the third case incorrectly, and your\npatch fixes it by returning early in that case as well.\n\nThe output codepath is the same.  Regardless of action\n(GUESS/TEXT/INPUT), we will not do anything if config says 'false'\nor 'input', so removing the check with \"action == GUESS\" is the\nright thing.\n"},{"id":"42609","messageId":"200705191058.32473.andyparkins@gmail.com","threadId":"8201","inReplyTo":"7v646p4r50.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix crlf attribute handling to match documentation","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-05-19T09:58:29Z","receivedAt":"2007-05-19T09:58:29Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Saturday 2007, May 19, Junio C Hamano wrote:\n\n> The check you modified in the first hunk is not the above '<='\n> comparison but is this:\n\nI was talking about the second hunk there though, which I mentioned \nwith \"...skipped in crlf_to_worktree()...\".   In which case the \ndescription matches.\n\n> \t(action == CRLF_GUESS && !auto_crlf)\n>\n> and \"core.autocrlf = input\" makes \"auto_crlf = -1\", so when\n> action is not GUESS, or even when action is GUESS, if the config\n> is set to \"input\", the if() statement you patched in the first\n> hunk should not trigger.  The above description is different from\n> what the code was doing.\n\nAs I say, the description was for the second hunk.\n\nThe fault was my last paragraph, \"Similarly for crlf_to_worktree(), if \ncore.autocrlf is false, no\", which should have said \"Similarly for \ncrlf_to_git()\".  Sorry.\n\nWith that small change I think my description was correct.  However, it \nobviously wasn't clear - apologies.\n\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"}]}