{"thread":{"id":"30850","subject":"[PATCH 1/2] submodule: Demonstrate failure to add with auto/safecrlf","startedAt":"2012-06-20T14:43:31Z","lastAt":"2012-06-21T19:06:28Z","messageCount":12,"participants":["Brad King","Junio C Hamano","Jens Lehmann","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"193935","messageId":"cover.1340202515.git.brad.king@kitware.com","threadId":"30850","inReplyTo":null,"subject":"[PATCH 0/2] submodule add + autocrlf + safecrlf","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2012-06-20T14:43:31Z","receivedAt":"2012-06-20T14:43:31Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"Hi Folks,\n\nWhen 'git submodule add' uses 'git config' to create a\n'.gitmodules' file it gets LF newlines that the subsequent\n'git add --force .gitmodules' rejects if autocrlf and\nsafecrlf are both enabled. This series adds a test and\nproposes a fix that simply uses '-c core.safecrlf=false'\nto disable safecrlf when adding '.gitmodules'.\n\nI'm not excited by allowing a LF file in work tree that\nhas clearly been configured to prefer CRLF, but avoiding\nthat for .gitmodules is probably a separate issue.\n\n-Brad\n\nBrad King (2):\n submodule: Demonstrate failure to add with auto/safecrlf\n submodule: Tolerate auto/safecrlf when adding .gitmodules\n\n git-submodule.sh | 2 +-\n t/t7400-submodule-basic.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\n-- \n1.7.10\n"},{"id":"193933","messageId":"7b51d857a351dd0d4972b62b5ba52a7df6bfd988.1340202515.git.brad.king@kitware.com","threadId":"30850","inReplyTo":"cover.1340202515.git.brad.king@kitware.com","subject":"[PATCH 1/2] submodule: Demonstrate failure to add with auto/safecrlf","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2012-06-20T14:43:32Z","receivedAt":"2012-06-20T14:43:32Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"If 'core.autocrlf' and 'core.safecrlf' are both enabled then\n'git submodule add' fails with an error such as\n\n fatal: LF would be replaced by CRLF in .gitmodules\n Failed to register submodule 'submod'\n\nbecause it generates a '.gitmodules' file with LF newlines that are\nrejected by 'git add' under this configuration. Demonstrate this known\nbreakage with a new test in t7400-submodule-basic covering the case.\n\nSigned-off-by: Brad King \n---\n t/t7400-submodule-basic.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 81827e6..5eaeb04 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -43,6 +43,7 @@ test_expect_success 'setup - hide init subdirectory' '\n \n test_expect_success 'setup - repository to add submodules to' '\n \tgit init addtest &&\n+\tgit init addtest-crlf &&\n \tgit init addtest-ignore\n '\n \n@@ -98,6 +99,18 @@ test_expect_success 'submodule add' '\n \ttest_cmp empty untracked\n '\n \n+test_expect_failure 'submodule add with core.autocrlf and core.safecrlf' '\n+\t(\n+\t\tcd addtest-crlf &&\n+\t\tgit config core.autocrlf true &&\n+\t\tgit config core.safecrlf true &&\n+\t\tgit submodule add \"$submodurl\" submod &&\n+\t\techo \".gitmodules\" >expect &&\n+\t\tgit ls-files -- .gitmodules >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'submodule add to .gitignored path fails' '\n \t(\n \t\tcd addtest-ignore &&\n-- \n1.7.10\n"},{"id":"193934","messageId":"eebc8b3692f8fcb95cf75278f7c9f9982e8f2cd6.1340202515.git.brad.king@kitware.com","threadId":"30850","inReplyTo":"cover.1340202515.git.brad.king@kitware.com","subject":"[PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2012-06-20T14:43:33Z","receivedAt":"2012-06-20T14:43:33Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"Temporarily disable 'core.safecrlf' to add '.gitmodules' so that\n'git add' does not reject the LF newlines we write to the file\neven if both 'core.autocrlf' and 'core.safecrlf' are enabled.\nThis fixes known breakage tested in t7400-submodule-basic.\n\nSigned-off-by: Brad King \n---\n git-submodule.sh | 2 +-\n t/t7400-submodule-basic.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5c61ae2..ed9a54a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -303,7 +303,7 @@ Use -f if you really want to add it.\" >&2\n \n \tgit config -f .gitmodules submodule.\"$sm_path\".path \"$sm_path\" &&\n \tgit config -f .gitmodules submodule.\"$sm_path\".url \"$repo\" &&\n-\tgit add --force .gitmodules ||\n+\tgit -c core.safecrlf=false add --force .gitmodules ||\n \tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n }\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 5eaeb04..9a4da9b 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -99,7 +99,7 @@ test_expect_success 'submodule add' '\n \ttest_cmp empty untracked\n '\n \n-test_expect_failure 'submodule add with core.autocrlf and core.safecrlf' '\n+test_expect_success 'submodule add with core.autocrlf and core.safecrlf' '\n \t(\n \t\tcd addtest-crlf &&\n \t\tgit config core.autocrlf true &&\n-- \n1.7.10\n"},{"id":"193941","messageId":"7vipelx49g.fsf@alter.siamese.dyndns.org","threadId":"30850","inReplyTo":"cover.1340202515.git.brad.king@kitware.com","subject":"Re: [PATCH 0/2] submodule add + autocrlf + safecrlf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-20T17:49:31Z","receivedAt":"2012-06-20T17:49:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brad King writes:\n\n> When 'git submodule add' uses 'git config' to create a\n> '.gitmodules' file it gets LF newlines that the subsequent\n> 'git add --force .gitmodules' rejects if autocrlf and\n> safecrlf are both enabled. This series adds a test and\n> proposes a fix that simply uses '-c core.safecrlf=false'\n> to disable safecrlf when adding '.gitmodules'.\n>\n> I'm not excited by allowing a LF file in work tree that\n> has clearly been configured to prefer CRLF, but avoiding\n> that for .gitmodules is probably a separate issue.\n\nI have a suspicion that \"git config\" should be taught about this\nkind of thing instead.\n\nShoudn't your .git/config file that is outside the revision control\nalso end with CRLF if your platform and project prefer CRLF over LF?\n"},{"id":"193943","messageId":"4FE20DD3.6040607@web.de","threadId":"30850","inReplyTo":"eebc8b3692f8fcb95cf75278f7c9f9982e8f2cd6.1340202515.git.brad.king@kitware.com","subject":"Re: [PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-06-20T17:52:19Z","receivedAt":"2012-06-20T17:52:19Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 20.06.2012 16:43, schrieb Brad King:\n> Temporarily disable 'core.safecrlf' to add '.gitmodules' so that\n> 'git add' does not reject the LF newlines we write to the file\n> even if both 'core.autocrlf' and 'core.safecrlf' are enabled.\n> This fixes known breakage tested in t7400-submodule-basic.\n\nHmm, I have no objections against the intention of the patch. But\nas I understand it this message will reoccur when the user e.g.\nedits the .gitmodules file later with any editor who just writes\nlfs and adds it.\n\nI don't know terribly much about crlf support but maybe flagging\nthe .gitmodules file in .gitattributes would be a better solution\nhere? Opinions?\n\n> Signed-off-by: Brad King \n> ---\n> git-submodule.sh | 2 +-\n> t/t7400-submodule-basic.sh | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 5c61ae2..ed9a54a 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -303,7 +303,7 @@ Use -f if you really want to add it.\" >&2\n> \n> \tgit config -f .gitmodules submodule.\"$sm_path\".path \"$sm_path\" &&\n> \tgit config -f .gitmodules submodule.\"$sm_path\".url \"$repo\" &&\n> -\tgit add --force .gitmodules ||\n> +\tgit -c core.safecrlf=false add --force .gitmodules ||\n> \tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n> }\n> \n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 5eaeb04..9a4da9b 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -99,7 +99,7 @@ test_expect_success 'submodule add' '\n> \ttest_cmp empty untracked\n> '\n> \n> -test_expect_failure 'submodule add with core.autocrlf and core.safecrlf' '\n> +test_expect_success 'submodule add with core.autocrlf and core.safecrlf' '\n> \t(\n> \t\tcd addtest-crlf &&\n> \t\tgit config core.autocrlf true &&\n> \n"},{"id":"193946","messageId":"4FE21133.2030001@kitware.com","threadId":"30850","inReplyTo":"4FE20DD3.6040607@web.de","subject":"Re: [PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2012-06-20T18:06:43Z","receivedAt":"2012-06-20T18:06:43Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 06/20/2012 01:52 PM, Jens Lehmann wrote:\n> Am 20.06.2012 16:43, schrieb Brad King:\n>> Temporarily disable 'core.safecrlf' to add '.gitmodules' so that\n>> 'git add' does not reject the LF newlines we write to the file\n>> even if both 'core.autocrlf' and 'core.safecrlf' are enabled.\n>> This fixes known breakage tested in t7400-submodule-basic.\n> \n> Hmm, I have no objections against the intention of the patch. But\n> as I understand it this message will reoccur when the user e.g.\n> edits the .gitmodules file later with any editor who just writes\n> lfs and adds it.\n> \n> I don't know terribly much about crlf support but maybe flagging\n> the .gitmodules file in .gitattributes would be a better solution\n> here? Opinions?\n\nOnce a user edits the file with an outside tool it is his/her\nresponsibility to add .gitattributes for the file. In the reported\ncase Git is creating the file and already knows the crlf mode when\ncreating it.\n\nI think Junio's proposal to teach \"git config\" to respect crlf\nconfiguration is a more general solution.\n\n-Brad\n"},{"id":"193948","messageId":"4FE211C8.3080007@kitware.com","threadId":"30850","inReplyTo":"7vipelx49g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] submodule add + autocrlf + safecrlf","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2012-06-20T18:09:12Z","receivedAt":"2012-06-20T18:09:12Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 06/20/2012 01:49 PM, Junio C Hamano wrote:\n> I have a suspicion that \"git config\" should be taught about this\n> kind of thing instead.\n> \n> Shoudn't your .git/config file that is outside the revision control\n> also end with CRLF if your platform and project prefer CRLF over LF?\n\nThat would be reasonable, but is beyond the scope I'm willing to\ntackle myself.\n\nI don't actually have a project like this so I have no strong\nopinion on this issue. I discovered the problem by accident\nand have already worked around it in the obscure case it matters\nfor me.\n\nPerhaps only the first patch in the series is worth inclusion.\nIt can become the beginning of a series if someone wants to address\nhandling crlf in config files. Note that in the case I discovered\nthis the crlf configuration was in ~/.gitconfig so the project\nknew nothing about it and had no .gitattributes.\n\n-Brad\n"},{"id":"193964","messageId":"4FE21493.8000206@web.de","threadId":"30850","inReplyTo":"4FE21133.2030001@kitware.com","subject":"Re: [PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-06-20T18:21:07Z","receivedAt":"2012-06-20T18:21:07Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 20.06.2012 20:06, schrieb Brad King:\n> On 06/20/2012 01:52 PM, Jens Lehmann wrote:\n>> Am 20.06.2012 16:43, schrieb Brad King:\n>>> Temporarily disable 'core.safecrlf' to add '.gitmodules' so that\n>>> 'git add' does not reject the LF newlines we write to the file\n>>> even if both 'core.autocrlf' and 'core.safecrlf' are enabled.\n>>> This fixes known breakage tested in t7400-submodule-basic.\n>>\n>> Hmm, I have no objections against the intention of the patch. But\n>> as I understand it this message will reoccur when the user e.g.\n>> edits the .gitmodules file later with any editor who just writes\n>> lfs and adds it.\n>>\n>> I don't know terribly much about crlf support but maybe flagging\n>> the .gitmodules file in .gitattributes would be a better solution\n>> here? Opinions?\n> \n> Once a user edits the file with an outside tool it is his/her\n> responsibility to add .gitattributes for the file. In the reported\n> case Git is creating the file and already knows the crlf mode when\n> creating it.\n> \n> I think Junio's proposal to teach \"git config\" to respect crlf\n> configuration is a more general solution.\n\nYep.\n"},{"id":"193967","messageId":"20120620191132.GB31520@sigill.intra.peff.net","threadId":"30850","inReplyTo":"4FE21133.2030001@kitware.com","subject":"Re: [PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-20T19:11:32Z","receivedAt":"2012-06-20T19:11:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 20, 2012 at 02:06:43PM -0400, Brad King wrote:\n\n> > Hmm, I have no objections against the intention of the patch. But\n> > as I understand it this message will reoccur when the user e.g.\n> > edits the .gitmodules file later with any editor who just writes\n> > lfs and adds it.\n> > \n> > I don't know terribly much about crlf support but maybe flagging\n> > the .gitmodules file in .gitattributes would be a better solution\n> > here? Opinions?\n> \n> Once a user edits the file with an outside tool it is his/her\n> responsibility to add .gitattributes for the file. In the reported\n> case Git is creating the file and already knows the crlf mode when\n> creating it.\n> \n> I think Junio's proposal to teach \"git config\" to respect crlf\n> configuration is a more general solution.\n\nI don't think so. It should not be an issue at all that .gitmodules has\nCRLF or even mixed line endings; the config parser knows that its input\nis a text file and ignores the CRs already.\n\nThe only problem is when adding the file, because git is not aware that\n.gitmodules is, by definition, a text file. The point of safecrlf was\nto prevent accidents with files which are really binary, or for which\nmixed line endings must be preserved; .gitmodules is not such a file. We\nknow that, but we never tell git.\n\nWe can paper over the problem by normalizing line endings when config\nwrites it out, but that does not cover the case of somebody editing it\nmanually and introducing mixed line endings. Nor does it help if\nsomebody checks in a .gitmodules file with CRLF, which we then checkout\non a LF-based system. Or if we do a merge between two versions with\ndifferent line endings.\n\nThe only sane thing is to have a canonical in-repo representation.\nFortunately we already have the infrastructure for that, and in theory\nit should be as easy as adding \".gitmodules text\" to our built-in\ngitattributes (you could even do \"eol=lf\", but I don't see a reason not\nto respect the native line endings in the working tree, given that git\ncan handle the CRLFs just fine).\n\nI say \"in theory\" there because I am not sure whether specifying a file\nas definitely text via attributes will actually suppress the safecrlf\ncheck or not. IMHO, it should, since safecrlf is really about preventing\nfalse positives via autocrlf or text=auto.\n\nI don't see any reason for each individual repo to have to add these\nattributes manually. This is a git-specific file, and the format is\ndictated by git. We know that it's a text file, so why not help out the\nuser? We should possibly do the same thing for .gitattributes and\n.gitignore.\n\n-Peff\n"},{"id":"193969","messageId":"7vehp9vlbf.fsf@alter.siamese.dyndns.org","threadId":"30850","inReplyTo":"4FE211C8.3080007@kitware.com","subject":"Re: [PATCH 0/2] submodule add + autocrlf + safecrlf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-20T19:24:04Z","receivedAt":"2012-06-20T19:24:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brad King writes:\n\n> On 06/20/2012 01:49 PM, Junio C Hamano wrote:\n>> I have a suspicion that \"git config\" should be taught about this\n>> kind of thing instead.\n>> \n>> Shoudn't your .git/config file that is outside the revision control\n>> also end with CRLF if your platform and project prefer CRLF over LF?\n>\n> That would be reasonable, but is beyond the scope I'm willing to\n> tackle myself.\n>\n> I don't actually have a project like this so I have no strong\n> opinion on this issue.\n\nIf that is the case, my preference would be to wait until that \"git\nconfig\" change happens (provided that it is a reasonably way\nforward---it may turn out to be a bad idea) and do nothing to\nclutter \"git submodule\" at all. In the meantime, if there are real\nprojects that are hurt by this, we can tell them to explicitly\nspecify that .gitmodules is a LF text file in their .gitattributes.\n"},{"id":"193984","messageId":"7v1ul9vjyn.fsf@alter.siamese.dyndns.org","threadId":"30850","inReplyTo":"20120620191132.GB31520@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-20T19:53:20Z","receivedAt":"2012-06-20T19:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is a git-specific file, and the format is\n> dictated by git. We know that it's a text file, so why not help out the\n> user? We should possibly do the same thing for .gitattributes and\n> .gitignore.\n\nOK.\n"},{"id":"194048","messageId":"4FE370B4.60404@web.de","threadId":"30850","inReplyTo":"20120620191132.GB31520@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] submodule: Tolerate auto/safecrlf when adding .gitmodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-06-21T19:06:28Z","receivedAt":"2012-06-21T19:06:28Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 20.06.2012 21:11, schrieb Jeff King:\n> The only sane thing is to have a canonical in-repo representation.\n> Fortunately we already have the infrastructure for that, and in theory\n> it should be as easy as adding \".gitmodules text\" to our built-in\n> gitattributes (you could even do \"eol=lf\", but I don't see a reason not\n> to respect the native line endings in the working tree, given that git\n> can handle the CRLFs just fine).\n> \n> I say \"in theory\" there because I am not sure whether specifying a file\n> as definitely text via attributes will actually suppress the safecrlf\n> check or not. IMHO, it should, since safecrlf is really about preventing\n> false positives via autocrlf or text=auto.\n\nA quick test shows that unfortunately theory differs from practice here.\nAdding \".gitmodules text\" to the built-in gitattributes lets the test\nBrad wrote still fail. You have to use \".gitmodules eol=lf\" to make it\npass. I stopped digging deeper at this point.\n\n> I don't see any reason for each individual repo to have to add these\n> attributes manually. This is a git-specific file, and the format is\n> dictated by git. We know that it's a text file, so why not help out the\n> user? We should possibly do the same thing for .gitattributes and\n> .gitignore.\n\nI really like this approach. (And in the long run would like to see a\nini-file aware merge driver being used for the .gitmodules file too,\nwhich would just merge submodules added in different branches instead\nof producing a conflict)\n"}]}