{"thread":{"id":"57656","subject":"[PATCH] convert: clarify line ending conversion warning","startedAt":"2022-04-04T05:52:06Z","lastAt":"2022-04-05T04:03:43Z","messageCount":8,"participants":["Alex Henrie","Ævar Arnfjörð Bjarmason","Tao Klerks","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453020","messageId":"20220404055151.160184-1-alexhenrie24@gmail.com","threadId":"57656","inReplyTo":null,"subject":"[PATCH] convert: clarify line ending conversion warning","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-04-04T05:51:51Z","receivedAt":"2022-04-04T05:52:06Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The warning about converting line endings is extremely confusing. Its\ntwo sentences each use the word \"will\" without specifying a timeframe,\nwhich makes it sound like both sentences are referring to the same\ntimeframe. On top of that, it uses the term \"original line endings\"\nwithout saying whether \"original\" means LF or CRLF.\n\nRephrase the warning to be clear that line endings have not been changed\nin the working directory but will be changed on the next checkout, and\nexplicitly say which line endings the file currently has in the working\ndirectory.\n\nExample commands to trigger the warning on Linux:\n\ngit config core.autocrlf true\necho 'Hello world!' > hello.txt\ngit add hello.txt\ngit commit -m \"Add hello.txt\"\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n convert.c            | 14 ++++++++------\n t/t0027-auto-crlf.sh |  2 +-\n 2 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 8e39731efb..b024d74222 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -195,9 +195,10 @@ static void check_global_conv_flags_eol(const char *path,\n \t\tif (conv_flags & CONV_EOL_RNDTRP_DIE)\n \t\t\tdie(_(\"CRLF would be replaced by LF in %s\"), path);\n \t\telse if (conv_flags & CONV_EOL_RNDTRP_WARN)\n-\t\t\twarning(_(\"CRLF will be replaced by LF in %s.\\n\"\n-\t\t\t\t  \"The file will have its original line\"\n-\t\t\t\t  \" endings in your working directory\"), path);\n+\t\t\twarning(_(\"CRLF will be replaced by LF in %s the next\"\n+\t\t\t\t  \" time you check it out.\\n\"\n+\t\t\t\t  \"For now, the file still has CRLF line\"\n+\t\t\t\t  \" endings in your working directory.\"), path);\n \t} else if (old_stats->lonelf && !new_stats->lonelf ) {\n \t\t/*\n \t\t * CRLFs would be added by checkout\n@@ -205,9 +206,10 @@ static void check_global_conv_flags_eol(const char *path,\n \t\tif (conv_flags & CONV_EOL_RNDTRP_DIE)\n \t\t\tdie(_(\"LF would be replaced by CRLF in %s\"), path);\n \t\telse if (conv_flags & CONV_EOL_RNDTRP_WARN)\n-\t\t\twarning(_(\"LF will be replaced by CRLF in %s.\\n\"\n-\t\t\t\t  \"The file will have its original line\"\n-\t\t\t\t  \" endings in your working directory\"), path);\n+\t\t\twarning(_(\"LF will be replaced by CRLF in %s the next\"\n+\t\t\t\t  \" time you check it out.\\n\"\n+\t\t\t\t  \"For now, the file still has LF line\"\n+\t\t\t\t  \" endings in your working directory.\"), path);\n \t}\n }\n \ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 0feb41a23f..31f471c4b1 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -82,7 +82,7 @@ check_warning () {\n \t'')\t                                                 >\"$2\".expect ;;\n \t*) echo >&2 \"Illegal 1\": \"$1\" ; return false ;;\n \tesac\n-\tgrep \"will be replaced by\" \"$2\" | sed -e \"s/\\(.*\\) in [^ ]*$/\\1/\" | uniq  >\"$2\".actual\n+\tgrep \"will be replaced by\" \"$2\" | sed -e \"s/\\(.*\\) in .*$/\\1/\" | uniq  >\"$2\".actual\n \ttest_cmp \"$2\".expect \"$2\".actual\n }\n \n-- \n2.35.1\n\n"},{"id":"453021","messageId":"220404.867d85l6ok.gmgdl@evledraar.gmail.com","threadId":"57656","inReplyTo":"20220404055151.160184-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-04T06:06:30Z","receivedAt":"2022-04-04T06:11:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 03 2022, Alex Henrie wrote:\n\n> The warning about converting line endings is extremely confusing. Its\n> two sentences each use the word \"will\" without specifying a timeframe,\n> which makes it sound like both sentences are referring to the same\n> timeframe. On top of that, it uses the term \"original line endings\"\n> without saying whether \"original\" means LF or CRLF.\n>\n> Rephrase the warning to be clear that line endings have not been changed\n> in the working directory but will be changed on the next checkout, and\n> explicitly say which line endings the file currently has in the working\n> directory.\n>\n> Example commands to trigger the warning on Linux:\n>\n> git config core.autocrlf true\n> echo 'Hello world!' > hello.txt\n> git add hello.txt\n> git commit -m \"Add hello.txt\"\n\nJust eyeballing this, while there might be some improvements here one\nthing to consider is atht now %s isn't at the end of the line.\n\nIt's fine for hello.txt, but consider longer paths, before:\n\n\tLF will be replaced by CRLF in /home/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello.txt.\n\nV.s.:\n\n\tLF will be replaced by CRLF in /home/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello.txt the next time you check it out.\n"},{"id":"453036","messageId":"CAPMMpohXLG2BsO2N1He9aBxm_VuQGCtf32AC67jVkuHQnwVw8A@mail.gmail.com","threadId":"57656","inReplyTo":"20220404055151.160184-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-04T15:22:16Z","receivedAt":"2022-04-04T15:22:32Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Apr 4, 2022 at 7:52 AM Alex Henrie <alexhenrie24@gmail.com> wrote:\n>\n> The warning about converting line endings is extremely confusing. Its\n> two sentences each use the word \"will\" without specifying a timeframe,\n> which makes it sound like both sentences are referring to the same\n> timeframe. On top of that, it uses the term \"original line endings\"\n> without saying whether \"original\" means LF or CRLF.\n>\n> Rephrase the warning to be clear that line endings have not been changed\n> in the working directory but will be changed on the next checkout, and\n> explicitly say which line endings the file currently has in the working\n> directory.\n>\n\nI think this change is generally a great idea (I agree the existing\nmessage is confusing / not as helpful as it could be).\n\nI think we could do slightly better than \"the next time you check it\nout\", however, in terms of clarity. I just tested and the way I\npersonally would inelegantly describe it is \"the next time you switch\nto, merge in, or rebase a branch that has different content for this\nfile, or explicitly check the file out\". I understand \"check it out\"\nis probably technically correct, but as a user, I'm left wondering\nwhether that's the next time I check it out from scratch (i.e., switch\nto a branch that does not have the file, and then switch back - which,\nincidentally, is what I originally *assumed* would happen before your\npatch - I had never checked), or the next time I switch branches at\nall, or the next time I switch to (/merge in /rebase onto) a branch\nthat has different content for that *specific file*.\n"},{"id":"453044","messageId":"xmqqtub8prm1.fsf@gitster.g","threadId":"57656","inReplyTo":"CAMMLpeQUZ3OfP=G-qKRjUVON=hninYEDQeeCrAdfM9maAZ3j7Q@mail.gmail.com","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-04T19:37:26Z","receivedAt":"2022-04-04T21:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n>> I have mixed feelings with this change, even though I agree that the\n>> original is not good.  The first sentence of the updated text\n>> already says that right now, the file ends with CRLF, and that the\n>> conversion happen the next time you check out the file to the\n>> working tree.  And that makes \"For now ... still\" totally redundant.\n>>\n>> Perhaps a single sentence, nothing more than\n>>\n>>         warning: in '%s', CRLF will be replaced by LF the next time\n>>         you check it out\n>>\n>> is sufficient?  I dunno.\n>\n> I like your idea to move the file name to the start of the message,\n> and I think that will address Ævar's concern as well.\n>\n> It doesn't matter to me whether the message is one sentence or two\n> sentences. If you have a slight preference for one sentence then let's\n> just do that.\n\n\"For now\" smelled like sending a wrong and/or confusing message.  Is\nit \"for now\" until we update the software, and with future versions\nof Git, \"git add\" will pretend as if the user immediately checked it\nout?\n\nWhile I do not care too deeply between one and two sentences,\ngetting rid of the seemingly redundant \"For now\" sentence will\nremove the source of such confusion, so ....\n\n\n"},{"id":"453045","messageId":"CAMMLpeQUZ3OfP=G-qKRjUVON=hninYEDQeeCrAdfM9maAZ3j7Q@mail.gmail.com","threadId":"57656","inReplyTo":"xmqqtub8rdrw.fsf@gitster.g","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-04-04T17:21:24Z","receivedAt":"2022-04-04T21:23:12Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Apr 4, 2022 at 10:53 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>\n> > Rephrase the warning to be clear that line endings have not been changed\n> > in the working directory but will be changed on the next checkout, and\n> > explicitly say which line endings the file currently has in the working\n> > directory.\n> >\n> > Example commands to trigger the warning on Linux:\n> >\n> > git config core.autocrlf true\n> > echo 'Hello world!' > hello.txt\n> > git add hello.txt\n> > git commit -m \"Add hello.txt\"\n>\n> While the \"example\" does not hurt, because the log message is not\n> executable, it would not help to its potential unless you add its\n> expected output to go with it.\n>\n>     On a platform whose native line endings are not CRLF\n>     (e.g. Linux), the \"git add\" step in the sequence triggers the\n>     waring in question.\n>\n>     $ git config core.autocrlf true\n>     $ echo 'Hello world!' >hello.txt\n>     $ git add hello.txt\n>     warning: LF will be replaced by CRLF in hello.txt\n>     The file will have its original line endings in your working directory\n>\n> or something like that.\n>\n> I think the recent trend is to enclose end-user supplied strings\n> (like misspelt option names, arguments to options, and pathnames)\n> inside single quote, so\n>\n>     warning: LF will be replaced by CRLF in 'hello.txt'.\n>\n> would probably be a good idea, on top of what you are aiming for.\n> Also, in a multi-sentence warning message like this, I do not think\n> it makes sense to omit the end-of-sentence full-stop.\n\nSure. I'll do all of that in the v2.\n\n> I have mixed feelings with this change, even though I agree that the\n> original is not good.  The first sentence of the updated text\n> already says that right now, the file ends with CRLF, and that the\n> conversion happen the next time you check out the file to the\n> working tree.  And that makes \"For now ... still\" totally redundant.\n>\n> Perhaps a single sentence, nothing more than\n>\n>         warning: in '%s', CRLF will be replaced by LF the next time\n>         you check it out\n>\n> is sufficient?  I dunno.\n\nI like your idea to move the file name to the start of the message,\nand I think that will address Ævar's concern as well.\n\nIt doesn't matter to me whether the message is one sentence or two\nsentences. If you have a slight preference for one sentence then let's\njust do that.\n\nThanks for the feedback!\n\n-Alex\n"},{"id":"453049","messageId":"xmqqtub8rdrw.fsf@gitster.g","threadId":"57656","inReplyTo":"20220404055151.160184-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-04T16:53:23Z","receivedAt":"2022-04-04T21:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> Rephrase the warning to be clear that line endings have not been changed\n> in the working directory but will be changed on the next checkout, and\n> explicitly say which line endings the file currently has in the working\n> directory.\n>\n> Example commands to trigger the warning on Linux:\n>\n> git config core.autocrlf true\n> echo 'Hello world!' > hello.txt\n> git add hello.txt\n> git commit -m \"Add hello.txt\"\n\nWhile the \"example\" does not hurt, because the log message is not\nexecutable, it would not help to its potential unless you add its\nexpected output to go with it.\n\n    On a platform whose native line endings are not CRLF\n    (e.g. Linux), the \"git add\" step in the sequence triggers the\n    waring in question.\n\n    $ git config core.autocrlf true\n    $ echo 'Hello world!' >hello.txt\n    $ git add hello.txt\n    warning: LF will be replaced by CRLF in hello.txt\n    The file will have its original line endings in your working directory\n\nor something like that.\n\nI think the recent trend is to enclose end-user supplied strings\n(like misspelt option names, arguments to options, and pathnames)\ninside single quote, so\n\n    warning: LF will be replaced by CRLF in 'hello.txt'.\n\nwould probably be a good idea, on top of what you are aiming for.\nAlso, in a multi-sentence warning message like this, I do not think\nit makes sense to omit the end-of-sentence full-stop.\n\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n> diff --git a/convert.c b/convert.c\n> index 8e39731efb..b024d74222 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -195,9 +195,10 @@ static void check_global_conv_flags_eol(const char *path,\n>  \t\tif (conv_flags & CONV_EOL_RNDTRP_DIE)\n>  \t\t\tdie(_(\"CRLF would be replaced by LF in %s\"), path);\n>  \t\telse if (conv_flags & CONV_EOL_RNDTRP_WARN)\n> -\t\t\twarning(_(\"CRLF will be replaced by LF in %s.\\n\"\n> -\t\t\t\t  \"The file will have its original line\"\n> -\t\t\t\t  \" endings in your working directory\"), path);\n> +\t\t\twarning(_(\"CRLF will be replaced by LF in %s the next\"\n> +\t\t\t\t  \" time you check it out.\\n\"\n> +\t\t\t\t  \"For now, the file still has CRLF line\"\n> +\t\t\t\t  \" endings in your working directory.\"), path);\n\nI have mixed feelings with this change, even though I agree that the\noriginal is not good.  The first sentence of the updated text\nalready says that right now, the file ends with CRLF, and that the\nconversion happen the next time you check out the file to the\nworking tree.  And that makes \"For now ... still\" totally redundant.\n\nPerhaps a single sentence, nothing more than\n\n\twarning: in '%s', CRLF will be replaced by LF the next time\n\tyou check it out\n\nis sufficient?  I dunno.\n\nThanks.\n"},{"id":"453057","messageId":"CAMMLpeRQm=PwvmvEWy387oFsjQXNgPnbgqSnre8Hz7iLJxPM_g@mail.gmail.com","threadId":"57656","inReplyTo":"CAPMMpohXLG2BsO2N1He9aBxm_VuQGCtf32AC67jVkuHQnwVw8A@mail.gmail.com","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-04-04T17:23:02Z","receivedAt":"2022-04-04T21:23:49Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Apr 4, 2022 at 9:22 AM Tao Klerks <tao@klerks.biz> wrote:\n>\n> On Mon, Apr 4, 2022 at 7:52 AM Alex Henrie <alexhenrie24@gmail.com> wrote:\n> >\n> > The warning about converting line endings is extremely confusing. Its\n> > two sentences each use the word \"will\" without specifying a timeframe,\n> > which makes it sound like both sentences are referring to the same\n> > timeframe. On top of that, it uses the term \"original line endings\"\n> > without saying whether \"original\" means LF or CRLF.\n> >\n> > Rephrase the warning to be clear that line endings have not been changed\n> > in the working directory but will be changed on the next checkout, and\n> > explicitly say which line endings the file currently has in the working\n> > directory.\n> >\n>\n> I think this change is generally a great idea (I agree the existing\n> message is confusing / not as helpful as it could be).\n>\n> I think we could do slightly better than \"the next time you check it\n> out\", however, in terms of clarity. I just tested and the way I\n> personally would inelegantly describe it is \"the next time you switch\n> to, merge in, or rebase a branch that has different content for this\n> file, or explicitly check the file out\". I understand \"check it out\"\n> is probably technically correct, but as a user, I'm left wondering\n> whether that's the next time I check it out from scratch (i.e., switch\n> to a branch that does not have the file, and then switch back - which,\n> incidentally, is what I originally *assumed* would happen before your\n> patch - I had never checked), or the next time I switch branches at\n> all, or the next time I switch to (/merge in /rebase onto) a branch\n> that has different content for that *specific file*.\n\nI would prefer a short message to a long-winded one. How about \"the\nnext time Git touches it\"?\n\n-Alex\n"},{"id":"453081","messageId":"CAPMMpojFg7x7O3YRSDkKa31eM4U-fbKSPA9he6XXG+TQngwtVg@mail.gmail.com","threadId":"57656","inReplyTo":"CAMMLpeRQm=PwvmvEWy387oFsjQXNgPnbgqSnre8Hz7iLJxPM_g@mail.gmail.com","subject":"Re: [PATCH] convert: clarify line ending conversion warning","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-05T04:03:05Z","receivedAt":"2022-04-05T04:03:43Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Apr 4, 2022 at 7:23 PM Alex Henrie <alexhenrie24@gmail.com> wrote:\n>\n> On Mon, Apr 4, 2022 at 9:22 AM Tao Klerks <tao@klerks.biz> wrote:\n> >\n> > I think we could do slightly better than \"the next time you check it\n> > out\", however, in terms of clarity.\n>\n> I would prefer a short message to a long-winded one. How about \"the\n> next time Git touches it\"?\n>\n> -Alex\n\nWorks for me!\n"}]}