{"thread":{"id":"42319","subject":"[PATCH] crlf: Add test showing double warning on commit","startedAt":"2016-05-14T11:17:55Z","lastAt":"2016-05-15T18:55:22Z","messageCount":6,"participants":["Adam Dinwoodie","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"286530","messageId":"20160514111755.GG2345@dinwoodie.org","threadId":"42319","inReplyTo":null,"subject":"[PATCH] crlf: Add test showing double warning on commit","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2016-05-14T11:17:55Z","receivedAt":"2016-05-14T11:17:55Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"Add failing test case showing CRLF -> LF rewrite warnings being printed\nmultiple times when running \"git commit\".\n\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n t/t0020-crlf.sh | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex f94120a..188b1dd 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -74,7 +74,7 @@ test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '\n \ttest_must_fail git add mixed\n '\n \n-test_expect_success 'safecrlf: print warning only once' '\n+test_expect_success 'safecrlf: print warning only once on add' '\n \n \tgit config core.autocrlf input &&\n \tgit config core.safecrlf warn &&\n@@ -87,6 +87,20 @@ test_expect_success 'safecrlf: print warning only once' '\n '\n \n \n+test_expect_failure 'safecrlf: print warning only once on commit' '\n+\n+\tgit config core.autocrlf input &&\n+\tgit config core.safecrlf warn &&\n+\n+\tfor w in I am all LF; do echo $w; done >doublewarn2 &&\n+\tgit add doublewarn2 &&\n+\tgit commit -m \"nowarn\" &&\n+\tfor w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn2 &&\n+\tgit add doublewarn2 2>&1 &&\n+\ttest $(git commit -m Message | grep \"CRLF will be replaced by LF\" | wc -l) = 1\n+'\n+\n+\n test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '\n \tgit config core.autocrlf input &&\n \tgit config core.safecrlf true &&\n-- \n2.8.1\n"},{"id":"286537","messageId":"baea5604-e786-5773-aa68-f311efb23752@web.de","threadId":"42319","inReplyTo":"20160514111755.GG2345@dinwoodie.org","subject":"Re: [PATCH] crlf: Add test showing double warning on commit","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-14T16:20:22Z","receivedAt":"2016-05-14T16:20:22Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 14.05.16 13:17, Adam Dinwoodie wrote:\n> Add failing test case showing CRLF -> LF rewrite warnings being printed\n> multiple times when running \"git commit\".\n>\n\nThe problem seems to come from this line:\n\nindex 5473493..59d4106 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -61,9 +61,18 @@ static int should_break(struct diff_filespec *src,\n            !hashcmp(src->sha1, dst->sha1))\n                return 0; /* they are the same */\n \n+       fprintf(stderr, \"%s:%d src-path=%s dst-path=%s\\n\",\n+               __FILE__, __LINE__, src->path, dst->path);\n+#if 0\n        if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))\n                return 0; /* error but caught downstream */\n+#else\n \n+       if (diff_populate_filespec(src, 0))\n+               return 0; /* error but caught downstream */\n+       if (strcmp(src->path, dst->path) && diff_populate_filespec(dst, 0))\n+               return 0; /* error but caught downstream */\n+#endif\n        max_size = ((src->size > dst->size) ? src->size : dst->size);\n\nDo we need to run diff_populate_filespec() twice when src==dst ?\nIf yes, we may need to introduce a flag besides\n#define CHECK_SIZE_ONLY 1\n#define CHECK_BINARY    2\nto suppress the conversion warning ??\n\nIf no, the fix from above should do ?\n"},{"id":"286545","messageId":"xmqqoa887a25.fsf@gitster.mtv.corp.google.com","threadId":"42319","inReplyTo":"baea5604-e786-5773-aa68-f311efb23752@web.de","subject":"Re: [PATCH] crlf: Add test showing double warning on commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-14T18:45:38Z","receivedAt":"2016-05-14T18:45:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Do we need to run diff_populate_filespec() twice when src==dst ?\n\nOf course we do.\n\nsrc and dst may have the same path, but are coming from different\nplaces (src may be an indexed blob while dst may be a file in the\nworking tree).\n\n> If yes, we may need to introduce a flag besides\n> #define CHECK_SIZE_ONLY 1\n> #define CHECK_BINARY    2\n> to suppress the conversion warning ??\n\nI do not think that belongs to diff_populate_filespec() at all.\n\nWhy should conversion routine give this warning when called by\ndiff_populate_filespec() in the first place?  Shouldn't it be silent\nby default, and is allowed to talk _ONLY_ when we are attempting to\nactually replace the data in the index, e.g. \"git add\" and \"git\ncommit -a\"?\n"},{"id":"286563","messageId":"55965a71-7078-60ad-0746-6f677dbb3f06@web.de","threadId":"42319","inReplyTo":"xmqqoa887a25.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] crlf: Add test showing double warning on commit","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-15T06:39:31Z","receivedAt":"2016-05-15T06:39:31Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 14.05.16 20:45, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n> \n>> Do we need to run diff_populate_filespec() twice when src==dst ?\n> \n> Of course we do.\n> \n> src and dst may have the same path, but are coming from different\n> places (src may be an indexed blob while dst may be a file in the\n> working tree).\n> \n>> If yes, we may need to introduce a flag besides\n>> #define CHECK_SIZE_ONLY 1\n>> #define CHECK_BINARY    2\n>> to suppress the conversion warning ??\n> \n> I do not think that belongs to diff_populate_filespec() at all.\n> \n> Why should conversion routine give this warning when called by\n> diff_populate_filespec() in the first place?  Shouldn't it be silent\n> by default, and is allowed to talk _ONLY_ when we are attempting to\n> actually replace the data in the index, e.g. \"git add\" and \"git\n> commit -a\"?\n> \n\nNja, (Or Nyes in English), the old handling tried to be \"nice\" to the user:\n$ git add text # gave warning\n#User forgets, does other things, git reset HEAD....\n$ git commit # Gave the warning one more time, to remind the user, \n             # what he did, and what is really commited.\n\nBut it may be, that diff_populate_filespec() is the wrong place for speaches ?\n"},{"id":"286566","messageId":"5e47eb63-41b3-31d0-7781-04d4e6eeb4b8@web.de","threadId":"42319","inReplyTo":"xmqqoa887a25.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] crlf: Add test showing double warning on commit","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-15T06:46:32Z","receivedAt":"2016-05-15T06:46:32Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 14.05.16 20:45, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n> \n>> Do we need to run diff_populate_filespec() twice when src==dst ?\n> \n> Of course we do.\n> \n> src and dst may have the same path, but are coming from different\n> places (src may be an indexed blob while dst may be a file in the\n> working tree).\n> \n>> If yes, we may need to introduce a flag besides\n>> #define CHECK_SIZE_ONLY 1\n>> #define CHECK_BINARY    2\n>> to suppress the conversion warning ??\n> \n> I do not think that belongs to diff_populate_filespec() at all.\n\nJust to remind myself:\nsha1_file.c:\nThe warning should probably triggered from here, depending on the flags ?\n\nint index_fd(unsigned char *sha1, int fd, struct stat *st,\n\t     enum object_type type, const char *path, unsigned flags)\n"},{"id":"286599","messageId":"xmqqshxj5exx.fsf@gitster.mtv.corp.google.com","threadId":"42319","inReplyTo":"55965a71-7078-60ad-0746-6f677dbb3f06@web.de","subject":"Re: [PATCH] crlf: Add test showing double warning on commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-15T18:55:22Z","receivedAt":"2016-05-15T18:55:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Nja, (Or Nyes in English), the old handling tried to be \"nice\" to the user:\n> $ git add text # gave warning\n> #User forgets, does other things, git reset HEAD....\n> $ git commit # Gave the warning one more time, to remind the user, \n>              # what he did, and what is really commited.\n>\n> But it may be, that diff_populate_filespec() is the wrong place for speaches ?\n\nIt is *not* necessarily a wrong place for speeches, but certainly it\nis wrong place to talk about whatever convert_to_git() does.\n\nLet's step back and think what the warning is about.\n\nI personally feel that \"CRLF will be turned into LF\" is pointless\nwhen the user expressed her preference \"I want to have LF in the\nrepository and CRLF in the working tree, when it makes sense\".  It\nis not even a warning but merely reports \"we did what we were told\nto do.\"  But for those who set safecrlf=warn to get that warning, we\nshould give the warning when we actually convert the the working tree\ncontents and strip CR from CRLF before recording it as an object.\n\nAn as-is \"git commit\" (nothing on command line affects the index\nwith working tree contents, e.g. \"-a\" or pathspec) does not do any\nsuch conversion.  The \"damage\" the user requested to be warned has\nlong been done before the command is run.\n\nSo the warning from \"git add\" is good, but it shouldn't appear from\nanything that does \"diff\".\n"}]}