{"thread":{"id":"56359","subject":"git log --encoding=HTML is not supported","startedAt":"2021-08-24T09:09:32Z","lastAt":"2021-10-29T21:10:06Z","messageCount":33,"participants":["Krzysztof Żelechowski","Bagas Sanjaya","Junio C Hamano","Jeff King","Bryan Turner","Ævar Arnfjörð Bjarmason","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"433514","messageId":"9896630.2IqcCWsCYL@localhost.localdomain","threadId":"56359","inReplyTo":null,"subject":"git log --encoding=HTML is not supported","fromName":"Krzysztof Żelechowski","fromEmail":"giecrilj@stegny.2a.pl","sentAt":"2021-08-24T09:00:03Z","receivedAt":"2021-08-24T09:09:32Z","isPatch":false,"sender":{"key":"giecrilj@stegny.2a.pl","avatar":"https://gravatar.com/avatar/419ea28e22b430cae87994078ab5203f95eb2146ac1fe20739b0521530b26a25?d=mp&s=160"},"body":"Co robiłeś/-aś zanim pojawił się błąd? (Kroki, aby odtworzyć problem)\n{ git log --oneline --encoding=HTML stl_function.h; }\n\nCo powinno się stać? (Oczekiwane zachowanie)\n828176ba490 libstdc++: Improve doxygen comments in <bits/stl_function.h>\n\nCo stało się zamiast tego? (Rzeczywiste zachowanie)\n828176ba490 libstdc++: Improve doxygen comments in \n\nJaka jest różnica między tym, co powinno się stać, a tym, co się stało?\nZnak początku nazwy pliku jest interpretowany jako znak otwierający znacznik.\n\nInne cenne uwagi:\nBłąd u klienta:\n\n\nImplementacja wtyczki:\n \"--pretty=format:\n \n%h %ad %s %an \n\"\n\nPodobne zgłoszenie:\n\n\nProponowane rozwiązanie:\nW odróżnieniu od omawianych powyżej trudności używaniem formatu wynikowego \nJSON, moim zdaniem w tym przypadku wystarczyłoby zakodować znaki [<] i [&] \nw treści w sposób odpowiedni dla HTML.\n(To rozwiązanie nie zakłada wykrywania i odrzucania znaków nieprawidłowych.)\n\n[Informacje o systemie]\nwersja gita:\ngit version 2.32.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.13.12-1-default #1 SMP Wed Aug 18 08:01:38 UTC 2021 (999e604) \nx86_64\ninformacje o kompilacji: gnuc: 11.1\ninformacje o bibliotece libc: glibc: 2.33\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Włączone skrypty Gita]\n\n\n\n"},{"id":"433527","messageId":"22496693-cf63-a278-c85e-d9e4376e2a59@gmail.com","threadId":"56359","inReplyTo":"9896630.2IqcCWsCYL@localhost.localdomain","subject":"Re: git log --encoding=HTML is not supported","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-24T10:31:14Z","receivedAt":"2021-08-24T10:32:14Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 24/08/21 16.00, Krzysztof Żelechowski wrote:\n> Co robiłeś/-aś zanim pojawił się błąd? (Kroki, aby odtworzyć problem)\n> { git log --oneline --encoding=HTML stl_function.h; }\n> \n> Co powinno się stać? (Oczekiwane zachowanie)\n> 828176ba490 libstdc++: Improve doxygen comments in <bits/stl_function.h>\n> \n> Co stało się zamiast tego? (Rzeczywiste zachowanie)\n> 828176ba490 libstdc++: Improve doxygen comments in \n> \n> Jaka jest różnica między tym, co powinno się stać, a tym, co się stało?\n> Znak początku nazwy pliku jest interpretowany jako znak otwierający znacznik.\n> \n> Inne cenne uwagi:\n> Błąd u klienta:\n> \n> \n> Implementacja wtyczki:\n> \"--pretty=format:\n> \n> %h %ad %s %an\n> \"\n> \n> Podobne zgłoszenie:\n> \n> \n> Proponowane rozwiązanie:\n> W odróżnieniu od omawianych powyżej trudności używaniem formatu wynikowego\n> JSON, moim zdaniem w tym przypadku wystarczyłoby zakodować znaki [<] i [&]\n> w treści w sposób odpowiedni dla HTML.\n> (To rozwiązanie nie zakłada wykrywania i odrzucania znaków nieprawidłowych.)\n> \n> [Informacje o systemie]\n> wersja gita:\n> git version 2.32.0\n> cpu: x86_64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> uname: Linux 5.13.12-1-default #1 SMP Wed Aug 18 08:01:38 UTC 2021 (999e604)\n> x86_64\n> informacje o kompilacji: gnuc: 11.1\n> informacje o bibliotece libc: glibc: 2.33\n> $SHELL (typically, interactive shell): /bin/bash\n> \n> \n> [Włączone skrypty Gita]\n> \n> \n> \n\nPlease speak English here (in other words, re-submit git-bugreport \nwithout l10n).\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"433528","messageId":"2197959.ZqlxZjeC1n@localhost.localdomain","threadId":"56359","inReplyTo":"22496693-cf63-a278-c85e-d9e4376e2a59@gmail.com","subject":"Re: git log --encoding=HTML is not supported","fromName":"Krzysztof Żelechowski","fromEmail":"giecrilj@stegny.2a.pl","sentAt":"2021-08-24T10:33:10Z","receivedAt":"2021-08-24T10:33:18Z","isPatch":false,"sender":{"key":"giecrilj@stegny.2a.pl","avatar":"https://gravatar.com/avatar/419ea28e22b430cae87994078ab5203f95eb2146ac1fe20739b0521530b26a25?d=mp&s=160"},"body":"Dnia wtorek, 24 sierpnia 2021 12:31:14 CEST Bagas Sanjaya pisze:\n\n> Please speak English here (in other words, re-submit git-bugreport\n> without l10n).\n\nHow do I do that?\n\nChris\n\n\n"},{"id":"433539","messageId":"05ffcc36-f473-14f3-d7df-1efa0dcfcade@gmail.com","threadId":"56359","inReplyTo":"2197959.ZqlxZjeC1n@localhost.localdomain","subject":"Re: git log --encoding=HTML is not supported","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-24T10:46:28Z","receivedAt":"2021-08-24T10:46:34Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 24/08/21 17.33, Krzysztof Żelechowski wrote:\n> Dnia wtorek, 24 sierpnia 2021 12:31:14 CEST Bagas Sanjaya pisze:\n> \n>> Please speak English here (in other words, re-submit git-bugreport\n>> without l10n).\n> \n> How do I do that?\n\nYou need to set locale to English when executing `git bugreport`:\n\n```\nLANGUAGE=en_US.UTF-8 LC_ALL=en_US.UTF-8 /path/to/git bugreport\n```\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"433614","messageId":"xmqqeeaiodxc.fsf@gitster.g","threadId":"56359","inReplyTo":"05ffcc36-f473-14f3-d7df-1efa0dcfcade@gmail.com","subject":"Re: git log --encoding=HTML is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-24T19:11:59Z","receivedAt":"2021-08-24T19:12:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya writes:\n\n> On 24/08/21 17.33, Krzysztof Żelechowski wrote:\n>> Dnia wtorek, 24 sierpnia 2021 12:31:14 CEST Bagas Sanjaya pisze:\n>> \n>>> Please speak English here (in other words, re-submit git-bugreport\n>>> without l10n).\n>> How do I do that?\n>\n> You need to set locale to English when executing `git bugreport`:\n>\n> ```\n> LANGUAGE=en_US.UTF-8 LC_ALL=en_US.UTF-8 /path/to/git bugreport\n> ```\n\nEmily, what's your take on this exchange?\n\nI recall that many people (me included) went for \"user friendlyness\"\nby pushing to localize the questionnaire, but here, it seems to be\nbackfiring at us.\n\nI personally think that it is OK to give a localized questionnaire\nand let volunteers who can speak the language help non-English\nspeakers, but at the same time, it may be a good idea to hint that\nfilling in the answers in English would give a better chance for\ntheir problems to be looked at (in a localized message).\n\n"},{"id":"433684","messageId":"YSWVi8uLHZCTtG0l@coredump.intra.peff.net","threadId":"56359","inReplyTo":"9896630.2IqcCWsCYL@localhost.localdomain","subject":"Re: git log --encoding=HTML is not supported","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-25T00:57:47Z","receivedAt":"2021-08-25T00:57:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 24, 2021 at 11:00:03AM +0200, Krzysztof Żelechowski wrote:\n\n> Co robiłeś/-aś zanim pojawił się błąd? (Kroki, aby odtworzyć problem)\n> { git log --oneline --encoding=HTML stl_function.h; }\n> \n> Co powinno się stać? (Oczekiwane zachowanie)\n> 828176ba490 libstdc++: Improve doxygen comments in <bits/stl_function.h>\n> \n> Co stało się zamiast tego? (Rzeczywiste zachowanie)\n> 828176ba490 libstdc++: Improve doxygen comments in \n\nI can't read the non-English parts of the email, but I gather you were\nexpecting \"--encoding=HTML\" to escape syntactically significant HTML\ncharacters. It's not that kind of \"encoding\", but more \"which character\nset are you using\" (utf8 vs iso8859-1, etc).\n\nWe feed the encoding \"HTML\" to iconv_open(), which of course has no idea\nwhat that is. It's unfortunate, though, that we don't even print a\nwarning, and instead just quietly leave the text intact. I wonder if we\nshould do something like:\n\ndiff --git a/pretty.c b/pretty.c\nindex 535eb97fa6..708b618cfe 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -672,7 +672,11 @@ const char *repo_logmsg_reencode(struct repository *r,\n \t * If the re-encoding failed, out might be NULL here; in that\n \t * case we just return the commit message verbatim.\n \t */\n-\treturn out ? out : msg;\n+\tif (!out) {\n+\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n+\t\treturn msg;\n+\t}\n+\treturn out;\n }\n \n static int mailmap_name(const char **email, size_t *email_len,\n\nAs far as what you're trying to accomplish, HTML-escaping isn't\nsomething Git supports. You'll have to run the output through an\nexternal escaping mechanism.\n\n-Peff\n"},{"id":"433751","messageId":"xmqqeeahjxj4.fsf@gitster.g","threadId":"56359","inReplyTo":"YSWVi8uLHZCTtG0l@coredump.intra.peff.net","subject":"Re: git log --encoding=HTML is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T16:31:59Z","receivedAt":"2021-08-25T16:32:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We feed the encoding \"HTML\" to iconv_open(), which of course has no idea\n> what that is. It's unfortunate, though, that we don't even print a\n> warning, and instead just quietly leave the text intact. I wonder if we\n> should do something like:\n>\n> diff --git a/pretty.c b/pretty.c\n> index 535eb97fa6..708b618cfe 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -672,7 +672,11 @@ const char *repo_logmsg_reencode(struct repository *r,\n> \t * If the re-encoding failed, out might be NULL here; in that\n> \t * case we just return the commit message verbatim.\n> \t */\n> -\treturn out ? out : msg;\n> +\tif (!out) {\n> +\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n> +\t\treturn msg;\n> +\t}\n> +\treturn out;\n> }\n> \n> static int mailmap_name(const char **email, size_t *email_len,\n\nThis addition sounds quite sensible to me.\n\n\"git log --encoding=bogus\" would issue this warning for each and\nevery commit and that may be a bit irritating, but being irritating\nmay be a good characteristic for a warning message that is given to\nan easily correctable condition.\n\nI originally thought that the warning would be lost to the pager,\nbut apparently I forgot what I did eons ago at 61b80509 (sending\nerrors to stdout under $PAGER, 2008-02-16) ;-).\n"},{"id":"433772","messageId":"1790169.Z4XVHNUiN4@localhost.localdomain","threadId":"56359","inReplyTo":"YSWVi8uLHZCTtG0l@coredump.intra.peff.net","subject":"Re: git log --encoding=HTML is not supported","fromName":"Krzysztof Żelechowski","fromEmail":"giecrilj@stegny.2a.pl","sentAt":"2021-08-25T23:00:44Z","receivedAt":"2021-08-25T23:00:57Z","isPatch":false,"sender":{"key":"giecrilj@stegny.2a.pl","avatar":"https://gravatar.com/avatar/419ea28e22b430cae87994078ab5203f95eb2146ac1fe20739b0521530b26a25?d=mp&s=160"},"body":"Dnia środa, 25 sierpnia 2021 02:57:47 CEST Jeff King pisze:\n> As far as what you're trying to accomplish, HTML-escaping isn't\n> something Git supports. You'll have to run the output through an\n> external escaping mechanism.\n\nHave you looked at the format? It is a HTML fragment with placeholders to be \nfilled by git log. I cannot run the output through an external escaping \nmechanism because it will kill the markup that is already there.\n\nChris\n\n\n\n\n"},{"id":"433783","messageId":"24330338.EZKKyuarjD@localhost.localdomain","threadId":"56359","inReplyTo":"YSWVi8uLHZCTtG0l@coredump.intra.peff.net","subject":"Re: git log --encoding=HTML is not supported","fromName":"Krzysztof Żelechowski","fromEmail":"giecrilj@stegny.2a.pl","sentAt":"2021-08-25T23:28:58Z","receivedAt":"2021-08-25T23:29:07Z","isPatch":false,"sender":{"key":"giecrilj@stegny.2a.pl","avatar":"https://gravatar.com/avatar/419ea28e22b430cae87994078ab5203f95eb2146ac1fe20739b0521530b26a25?d=mp&s=160"},"body":"Dnia środa, 25 sierpnia 2021 02:57:47 CEST Jeff King pisze:\n> diff --git a/pretty.c b/pretty.c\n\nPlease fix the manual for git log. It should say what encoding is recognised \n(namely if supported by iconv(1), except that POSIX character maps of \niconv(1p) are not supported), and that an unrecognised encoding is ignored.\n\nI would also like to see the HTML encoding supported independently of iconv, \nwhich seems like a pretty easy thing to do. Dream on, I guess?\n\nChris\n\n\n"},{"id":"433789","messageId":"CAGyf7-FhLQKQyCOishyrZHg0J+jk6=aszE3hMoH=L0j+0+TP7Q@mail.gmail.com","threadId":"56359","inReplyTo":"24330338.EZKKyuarjD@localhost.localdomain","subject":"Re: git log --encoding=HTML is not supported","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2021-08-25T23:47:49Z","receivedAt":"2021-08-25T23:48:02Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Aug 25, 2021 at 4:29 PM Krzysztof Żelechowski\n wrote:\n>\n> Dnia środa, 25 sierpnia 2021 02:57:47 CEST Jeff King pisze:\n> > diff --git a/pretty.c b/pretty.c\n>\n> Please fix the manual for git log. It should say what encoding is recognised\n> (namely if supported by iconv(1), except that POSIX character maps of\n> iconv(1p) are not supported), and that an unrecognised encoding is ignored.\n>\n> I would also like to see the HTML encoding supported independently of iconv,\n> which seems like a pretty easy thing to do. Dream on, I guess?\n\nI suspect the answer is less \"Dream on\" and more \"Patches welcome.\"\n\n>\n> Chris\n>\n>\n"},{"id":"433841","messageId":"xmqq5yvsgqt7.fsf@gitster.g","threadId":"56359","inReplyTo":"CAGyf7-FhLQKQyCOishyrZHg0J+jk6=aszE3hMoH=L0j+0+TP7Q@mail.gmail.com","subject":"Re: git log --encoding=HTML is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T15:37:40Z","receivedAt":"2021-08-26T15:37:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bryan Turner writes:\n\n> On Wed, Aug 25, 2021 at 4:29 PM Krzysztof Żelechowski\n> wrote:\n>>\n>> Dnia środa, 25 sierpnia 2021 02:57:47 CEST Jeff King pisze:\n>> > diff --git a/pretty.c b/pretty.c\n>>\n>> Please fix the manual for git log. It should say what encoding is recognised\n>> (namely if supported by iconv(1), except that POSIX character maps of\n>> iconv(1p) are not supported), and that an unrecognised encoding is ignored.\n>>\n>> I would also like to see the HTML encoding supported independently of iconv,\n>> which seems like a pretty easy thing to do. Dream on, I guess?\n>\n> I suspect the answer is less \"Dream on\" and more \"Patches welcome.\"\n\nPatches are welcomed but not before a proposed design is freshed\nout. I am sure people do welcome the design discussion.\n\nPieces taken from the contents stored in Git (like \"the title of the\ncommit\", \"the name of the author of the commit\") may need quoting\nand/or escaping when they are incorporated into a string to become\nparts of \"output\", and the way the quoting/escaping must be done\nwould depend on the \"host\" language/format. HTML has its own\nrequirements for how these pieces coming from Git contents are\nquoted, but it will not be the only \"host\" language that needs\nquoting.\n\nThe requirement for the feature we are \"Dreaming on\" may be much\ncloser to the \"host language\" options (e.g. --tcl, --perl ...) the\n\"git for-each-ref\" command has. These options tells us to format\neach piece of information (e.g. \"%(subject)\") taken from Git as a\nnatural 'string' constant in the host language, so that\n\n\tgit for-each-ref --shell \\\n\t --format='do_something %(authorname) %(authoremail)'\n\nwould write a shell script that calls \"do_something\" command with\ntwo arguments for each ref enumerated by the command, without having\nto worry about whitespaces and quote characters that may appear in\nthe interpolated pieces. It is immediately obvious that within the\ncontext of the for-each-ref command, the follwoing would equally be\nuseful (note: this is already \"dreaming on\" and does not exist yet):\n\n\techo \"
    \"\n\tgit for-each-ref --html \\\n\t\t--format='
  • %(authoremail)
    • '\n\techo \"
\"\n\nAs we have been seeing efforts to port features around the --format\noption between the for-each-ref family of commands and the log\nfamily of commands, I would also imagine that it would be natural\nfuture direction to extend it to the latter and eventually allow\n\n\tgit log --html \\\n\t\t--format='%h%s...'\n\nto format each commit into a single row in HTML table, and things\nlike that.\n\n"},{"id":"433925","messageId":"3883941.fE8Og5qy2N@localhost.localdomain","threadId":"56359","inReplyTo":"xmqq5yvsgqt7.fsf@gitster.g","subject":"Re: git log --encoding=HTML is not supported","fromName":"Krzysztof Żelechowski","fromEmail":"giecrilj@stegny.2a.pl","sentAt":"2021-08-26T20:52:36Z","receivedAt":"2021-08-27T11:40:16Z","isPatch":false,"sender":{"key":"giecrilj@stegny.2a.pl","avatar":"https://gravatar.com/avatar/419ea28e22b430cae87994078ab5203f95eb2146ac1fe20739b0521530b26a25?d=mp&s=160"},"body":"Dnia czwartek, 26 sierpnia 2021 17:37:40 CEST Junio C Hamano pisze:\n> git log --html \\\n> --format='%h%s...'\n\nI would like to be able to say:\n { git config i18n.logOutputEscape HTML; }\n\nWhat do you think?\n\nChris\n\n\n\n\n"},{"id":"433933","messageId":"xmqqk0k6c1zn.fsf@gitster.g","threadId":"56359","inReplyTo":"3883941.fE8Og5qy2N@localhost.localdomain","subject":"Re: git log --encoding=HTML is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-27T15:59:40Z","receivedAt":"2021-08-27T15:59:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Krzysztof Żelechowski writes:\n\n> Dnia czwartek, 26 sierpnia 2021 17:37:40 CEST Junio C Hamano pisze:\n>> git log --html \\\n>> --format='%h%s...'\n>\n> I would like to be able to say:\n> { git config i18n.logOutputEscape HTML; }\n>\n> What do you think?\n\nIt depends on what it does.\n\nIf the configuration means that \"git log\" output (with any supported\noptions, like \"-p\") will be given with '<' written to '^lt;' etc. so\nthat it becomes safe to dump it in HTML, it fails to interest me at\nall.\n"},{"id":"433943","messageId":"YSkvNyR4uT52de13@coredump.intra.peff.net","threadId":"56359","inReplyTo":"xmqqeeahjxj4.fsf@gitster.g","subject":"Re: git log --encoding=HTML is not supported","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-27T18:30:15Z","receivedAt":"2021-08-27T18:30:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 25, 2021 at 09:31:59AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We feed the encoding \"HTML\" to iconv_open(), which of course has no idea\n> > what that is. It's unfortunate, though, that we don't even print a\n> > warning, and instead just quietly leave the text intact. I wonder if we\n> > should do something like:\n> [...]\n> This addition sounds quite sensible to me.\n> \n> \"git log --encoding=bogus\" would issue this warning for each and\n> every commit and that may be a bit irritating, but being irritating\n> may be a good characteristic for a warning message that is given to\n> an easily correctable condition.\n> \n> I originally thought that the warning would be lost to the pager,\n> but apparently I forgot what I did eons ago at 61b80509 (sending\n> errors to stdout under $PAGER, 2008-02-16) ;-).\n\nHere it is polished into a real commit.\n\n-- >8 --\nSubject: [PATCH] logmsg_reencode(): warn when iconv() fails\n\nIf the user asks for a pretty-printed commit to be converted (either\nexplicitly with --encoding=foo, or implicitly because the commit is\nnon-utf8 and we want to convert it), we pass it through iconv(). If that\nfails, we fall back to showing the input verbatim, but don't tell the\nuser that the output may be bogus.\n\nLet's add a warning to do so, along with a mention in the documentation\nfor --encoding. Two things to note about the implementation:\n\n - we could produce the warning closer to the call to iconv() in\n reencode_string_len(), which would let us relay the value of errno.\n But this is not actually very helpful. reencode_string_len() does\n not know we are operating on a commit, and indeed does not know that\n the caller won't produce an error of its own. And the errno values\n from iconv() are seldom helpful (iconv_open() only ever produces\n EINVAL; perhaps EILSEQ from iconv() might be illuminating, but it\n can also return EINVAL for incomplete sequences).\n\n - if the reason for the failure is that the output charset is not\n supported, then the user will see this warning for every commit we\n try to display. That might be ugly and overwhelming, but on the\n other hand it is making it clear that every one of them has not been\n converted (and the likely outcome anyway is to re-try the command\n with a supported output encoding).\n\nSigned-off-by: Jeff King \n---\n Documentation/pretty-options.txt | 4 +++-\n pretty.c | 6 +++++-\n t/t4210-log-i18n.sh | 7 +++++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex 27ddaf84a1..42b227bc40 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -40,7 +40,9 @@ people using 80-column terminals.\n \tdefaults to UTF-8. Note that if an object claims to be encoded\n \tin `X` and we are outputting in `X`, we will output the object\n \tverbatim; this means that invalid sequences in the original\n-\tcommit may be copied to the output.\n+\tcommit may be copied to the output. Likewise, if iconv(3) fails\n+\tto convert the commit, we will output the original object\n+\tverbatim, along with a warning.\n \n --expand-tabs=::\n --expand-tabs::\ndiff --git a/pretty.c b/pretty.c\nindex 9631529c10..73b5ead509 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -671,7 +671,11 @@ const char *repo_logmsg_reencode(struct repository *r,\n \t * If the re-encoding failed, out might be NULL here; in that\n \t * case we just return the commit message verbatim.\n \t */\n-\treturn out ? out : msg;\n+\tif (!out) {\n+\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n+\t\treturn msg;\n+\t}\n+\treturn out;\n }\n \n static int mailmap_name(const char **email, size_t *email_len,\ndiff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\nindex d2dfcf164e..0141f36e33 100755\n--- a/t/t4210-log-i18n.sh\n+++ b/t/t4210-log-i18n.sh\n@@ -131,4 +131,11 @@ do\n \tfi\n done\n \n+test_expect_success 'log shows warning when conversion fails' '\n+\tenc=this-encoding-does-not-exist &&\n+\tgit log -1 --encoding=$enc 2>err &&\n+\techo \"warning: unable to reencode commit to ${SQ}${enc}${SQ}\" >expect &&\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.33.0.396.g72f622fe47\n\n"},{"id":"433944","messageId":"YSkvpqiW09DugNoz@coredump.intra.peff.net","threadId":"56359","inReplyTo":"YSkvNyR4uT52de13@coredump.intra.peff.net","subject":"Re: git log --encoding=HTML is not supported","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-27T18:32:06Z","receivedAt":"2021-08-27T18:32:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 27, 2021 at 02:30:16PM -0400, Jeff King wrote:\n\n> Here it is polished into a real commit.\n> \n> Subject: [PATCH] logmsg_reencode(): warn when iconv() fails\n\nAnd here's a minimal documentation I'd suggest on top. We can discuss\ngoing further in discussing subtleties of iconv() if we want, but IMHO\nit would work to stop here.\n\n-- >8 --\nSubject: [PATCH] docs: use \"character encoding\" to refer to commit-object\n encoding\n\nThe word \"encoding\" can mean a lot of things (e.g., base64 or\nquoted-printable encoding in emails, HTML entities, URL encoding, and so\non). The documentation for i18n.commitEncoding and i18n.logOutputEncoding\nuses the phrase \"character encoding\" to make this more clear.\n\nLet's use that phrase in other places to make it clear what kind of\nencoding we are talking about. This patch covers the gui.encoding\noption, as well as the --encoding option for git-log, etc (in this\nlatter case, I word-smithed the sentence a little at the same time).\nThat, coupled with the mention of iconv in the --encoding description,\nshould make this more clear.\n\nThe other spot I looked at is the working-tree-encoding section of\ngitattributes(5). But it gives specific examples of encodings that I\nthink make the meaning pretty clear already.\n\nSigned-off-by: Jeff King \n---\n Documentation/config/gui.txt | 2 +-\n Documentation/pretty-options.txt | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/gui.txt b/Documentation/config/gui.txt\nindex d30831a130..0c087fd8c9 100644\n--- a/Documentation/config/gui.txt\n+++ b/Documentation/config/gui.txt\n@@ -11,7 +11,7 @@ gui.displayUntracked::\n \tin the file list. The default is \"true\".\n \n gui.encoding::\n-\tSpecifies the default encoding to use for displaying of\n+\tSpecifies the default character encoding to use for displaying of\n \tfile contents in linkgit:git-gui[1] and linkgit:gitk[1].\n \tIt can be overridden by setting the 'encoding' attribute\n \tfor relevant files (see linkgit:gitattributes[5]).\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex 42b227bc40..b3af850608 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -33,7 +33,7 @@ people using 80-column terminals.\n \tused together.\n \n --encoding=::\n-\tThe commit objects record the encoding used for the log message\n+\tCommit objects record the character encoding used for the log message\n \tin their encoding header; this option can be used to tell the\n \tcommand to re-code the commit log message in the encoding\n \tpreferred by the user. For non plumbing commands this\n-- \n2.33.0.396.g72f622fe47\n\n"},{"id":"433945","messageId":"YSkv8jTgR8nxZ+QZ@coredump.intra.peff.net","threadId":"56359","inReplyTo":"1790169.Z4XVHNUiN4@localhost.localdomain","subject":"Re: git log --encoding=HTML is not supported","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-27T18:33:22Z","receivedAt":"2021-08-27T18:33:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 26, 2021 at 01:00:44AM +0200, Krzysztof Żelechowski wrote:\n\n> Dnia środa, 25 sierpnia 2021 02:57:47 CEST Jeff King pisze:\n> > As far as what you're trying to accomplish, HTML-escaping isn't\n> > something Git supports. You'll have to run the output through an\n> > external escaping mechanism.\n> \n> Have you looked at the format? It is a HTML fragment with placeholders to be \n> filled by git log. I cannot run the output through an external escaping \n> mechanism because it will kill the markup that is already there.\n\nRight, what I mean is that you'd have to pull the output out of Git, and\nthen format (and escape) it separately.\n\n-Peff\n"},{"id":"433947","messageId":"YSkw6DSHenXHGx0b@coredump.intra.peff.net","threadId":"56359","inReplyTo":"xmqqk0k6c1zn.fsf@gitster.g","subject":"Re: git log --encoding=HTML is not supported","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-27T18:37:28Z","receivedAt":"2021-08-27T18:37:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 27, 2021 at 08:59:40AM -0700, Junio C Hamano wrote:\n\n> Krzysztof Żelechowski writes:\n> \n> > Dnia czwartek, 26 sierpnia 2021 17:37:40 CEST Junio C Hamano pisze:\n> >> git log --html \\\n> >> --format='%h%s...'\n> >\n> > I would like to be able to say:\n> > { git config i18n.logOutputEscape HTML; }\n> >\n> > What do you think?\n> \n> It depends on what it does.\n> \n> If the configuration means that \"git log\" output (with any supported\n> options, like \"-p\") will be given with '<' written to '^lt;' etc. so\n> that it becomes safe to dump it in HTML, it fails to interest me at\n> all.\n\nYeah, I think things get pretty weird when you start thinking about\ndumping whole filenames and diff contents.\n\nI wouldn't be opposed to an option for the pretty formatter to have\nencodings. Something like:\n\n git log --format='%(authorname:quote=html)'\n\nI'd probably put off implementing that until we actually unify the\nfor-each-ref and pretty formats, though (we do not even have\n%(authorname) at this point!). The latter already has a quoting\nmechanism for shell/perl/python/tcl (though it is not per-atom, and I\nwouldn't be opposed to a --format-quote option that quoted all pretty.c\nplaceholders).\n\n-Peff\n"},{"id":"433951","messageId":"xmqqo89iacw3.fsf@gitster.g","threadId":"56359","inReplyTo":"YSkvpqiW09DugNoz@coredump.intra.peff.net","subject":"Re: git log --encoding=HTML is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-27T19:47:08Z","receivedAt":"2021-08-27T19:47:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 27, 2021 at 02:30:16PM -0400, Jeff King wrote:\n>\n>> Here it is polished into a real commit.\n>> \n>> Subject: [PATCH] logmsg_reencode(): warn when iconv() fails\n>\n> And here's a minimal documentation I'd suggest on top. We can discuss\n> going further in discussing subtleties of iconv() if we want, but IMHO\n> it would work to stop here.\n\nThanks, both patches look sensible.\n\nWill queue.\n"},{"id":"433969","messageId":"xmqqczpya750.fsf@gitster.g","threadId":"56359","inReplyTo":"YSkw6DSHenXHGx0b@coredump.intra.peff.net","subject":"Re: git log --encoding=HTML is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-27T21:51:23Z","receivedAt":"2021-08-27T21:51:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wouldn't be opposed to an option for the pretty formatter to have\n> encodings. Something like:\n>\n> git log --format='%(authorname:quote=html)'\n>\n> I'd probably put off implementing that until we actually unify the\n> for-each-ref and pretty formats, though (we do not even have\n> %(authorname) at this point!). The latter already has a quoting\n> mechanism for shell/perl/python/tcl (though it is not per-atom, and I\n> wouldn't be opposed to a --format-quote option that quoted all pretty.c\n> placeholders).\n\nYeah, per-atom would be nice, as we can specify which piece needs\nwhat kind of quoting, e.g.\n\n git log --format='\n\tif test %(authoremail:quote=shell) != \"gitster@pobox.com\"\n\tthen\n\t echo %(authorname:quote=html+shell)\n\tfi\n ' | sh\n\ncan be used to write a script to produce an \"echo\" command with a\nshell literal string as its argument, where that literal string\nwrites author's name in a way that can be inserted in an HTML\ndocument, but omitting the commits by me.\n"},{"id":"438385","messageId":"87ily7m1mv.fsf@evledraar.gmail.com","threadId":"56359","inReplyTo":"YSkvNyR4uT52de13@coredump.intra.peff.net","subject":"*Really* noisy encoding warnings post-v2.33.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T00:58:10Z","receivedAt":"2021-10-09T01:24:13Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 27 2021, Jeff King wrote:\n\n$subject because I think we should really consider backing this out\nbefore it gets to a real release.\n\nI ran into this while testing the grep coloring patch[1] (but it's\nunrelated). Before this commit e.g.:\n\n LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100|wc -l\n 28333\n\nSo ~3k lines for my last 100 commits, but then:\n\n $ LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100 2>&1|grep -c ^warning\n 299\n\nAt first I thought it was spewing warnings for every failed re-encoded\nline in some cases, because I get hundreds at a time sometimes, but it's\nbecause stderr and stdout I/O buffering is different (a common\ncase). Adding a \"fflush(stderr)\" \"fixes\" that.\n\nBut anyway, I think we've got a lot of users who say *do* want to\nreencode something from say UTF-8 to latin1, but then might have the\noccasional non-latin1 representable data. The old behavior of silently\nfalling back is going to be much better for those users, or maybe show\none warning at the end or something, if you feel it really needs to be\nkept.\n\n> On Wed, Aug 25, 2021 at 09:31:59AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > We feed the encoding \"HTML\" to iconv_open(), which of course has no idea\n>> > what that is. It's unfortunate, though, that we don't even print a\n>> > warning, and instead just quietly leave the text intact. I wonder if we\n>> > should do something like:\n>> [...]\n>> This addition sounds quite sensible to me.\n>> \n>> \"git log --encoding=bogus\" would issue this warning for each and\n>> every commit and that may be a bit irritating, but being irritating\n>> may be a good characteristic for a warning message that is given to\n>> an easily correctable condition.\n>> \n>> I originally thought that the warning would be lost to the pager,\n>> but apparently I forgot what I did eons ago at 61b80509 (sending\n>> errors to stdout under $PAGER, 2008-02-16) ;-).\n>\n> Here it is polished into a real commit.\n>\n> -- >8 --\n> Subject: [PATCH] logmsg_reencode(): warn when iconv() fails\n>\n> If the user asks for a pretty-printed commit to be converted (either\n> explicitly with --encoding=foo, or implicitly because the commit is\n> non-utf8 and we want to convert it), we pass it through iconv(). If that\n> fails, we fall back to showing the input verbatim, but don't tell the\n> user that the output may be bogus.\n>\n> Let's add a warning to do so, along with a mention in the documentation\n> for --encoding. Two things to note about the implementation:\n>\n> - we could produce the warning closer to the call to iconv() in\n> reencode_string_len(), which would let us relay the value of errno.\n> But this is not actually very helpful. reencode_string_len() does\n> not know we are operating on a commit, and indeed does not know that\n> the caller won't produce an error of its own. And the errno values\n> from iconv() are seldom helpful (iconv_open() only ever produces\n> EINVAL; perhaps EILSEQ from iconv() might be illuminating, but it\n> can also return EINVAL for incomplete sequences).\n>\n> - if the reason for the failure is that the output charset is not\n> supported, then the user will see this warning for every commit we\n> try to display. That might be ugly and overwhelming, but on the\n> other hand it is making it clear that every one of them has not been\n> converted (and the likely outcome anyway is to re-try the command\n> with a supported output encoding).\n>\n> Signed-off-by: Jeff King \n> ---\n> Documentation/pretty-options.txt | 4 +++-\n> pretty.c | 6 +++++-\n> t/t4210-log-i18n.sh | 7 +++++++\n> 3 files changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\n> index 27ddaf84a1..42b227bc40 100644\n> --- a/Documentation/pretty-options.txt\n> +++ b/Documentation/pretty-options.txt\n> @@ -40,7 +40,9 @@ people using 80-column terminals.\n> \tdefaults to UTF-8. Note that if an object claims to be encoded\n> \tin `X` and we are outputting in `X`, we will output the object\n> \tverbatim; this means that invalid sequences in the original\n> -\tcommit may be copied to the output.\n> +\tcommit may be copied to the output. Likewise, if iconv(3) fails\n> +\tto convert the commit, we will output the original object\n> +\tverbatim, along with a warning.\n> \n> --expand-tabs=::\n> --expand-tabs::\n> diff --git a/pretty.c b/pretty.c\n> index 9631529c10..73b5ead509 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -671,7 +671,11 @@ const char *repo_logmsg_reencode(struct repository *r,\n> \t * If the re-encoding failed, out might be NULL here; in that\n> \t * case we just return the commit message verbatim.\n> \t */\n> -\treturn out ? out : msg;\n> +\tif (!out) {\n> +\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n> +\t\treturn msg;\n> +\t}\n> +\treturn out;\n> }\n> \n> static int mailmap_name(const char **email, size_t *email_len,\n> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\n> index d2dfcf164e..0141f36e33 100755\n> --- a/t/t4210-log-i18n.sh\n> +++ b/t/t4210-log-i18n.sh\n> @@ -131,4 +131,11 @@ do\n> \tfi\n> done\n> \n> +test_expect_success 'log shows warning when conversion fails' '\n> +\tenc=this-encoding-does-not-exist &&\n> +\tgit log -1 --encoding=$enc 2>err &&\n> +\techo \"warning: unable to reencode commit to ${SQ}${enc}${SQ}\" >expect &&\n> +\ttest_cmp expect err\n> +'\n> +\n> test_done\n\n"},{"id":"438386","messageId":"87ee8vm19j.fsf@evledraar.gmail.com","threadId":"56359","inReplyTo":"87ily7m1mv.fsf@evledraar.gmail.com","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T01:29:45Z","receivedAt":"2021-10-09T01:32:14Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 09 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Aug 27 2021, Jeff King wrote:\n>\n> $subject because I think we should really consider backing this out\n> before it gets to a real release.\n>\n> I ran into this while testing the grep coloring patch[1] (but it's\n> unrelated). Before this commit e.g.:\n>\n> LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100|wc -l\n> 28333\n>\n> So ~3k lines for my last 100 commits, but then:\n>\n> $ LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100 2>&1|grep -c ^warning\n> 299\n>\n> At first I thought it was spewing warnings for every failed re-encoded\n> line in some cases, because I get hundreds at a time sometimes, but it's\n> because stderr and stdout I/O buffering is different (a common\n> case). Adding a \"fflush(stderr)\" \"fixes\" that.\n\nIt's partially that, but also more pathologically:\n\n $ git -P -c i18n.commitEncoding=ascii log --author=doesnotexist -1 | wc -l\n 0\n $ git -P -c i18n.commitEncoding=ascii log --author=doesnotexist -1 2>&1 |wc -l\n 6688\n\nI.e. even if we don't end up emitting anything we'll warn, of course we\nmight not match *because* we failed, e.g. if you had a non-ascii --grep\nstring, but in this case it's rather noisy.\n\n> But anyway, I think we've got a lot of users who say *do* want to\n> reencode something from say UTF-8 to latin1, but then might have the\n> occasional non-latin1 representable data. The old behavior of silently\n> falling back is going to be much better for those users, or maybe show\n> one warning at the end or something, if you feel it really needs to be\n> kept.\n"},{"id":"438390","messageId":"YWEAEjIN0HVHbIpg@coredump.intra.peff.net","threadId":"56359","inReplyTo":"87ily7m1mv.fsf@evledraar.gmail.com","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-09T02:36:02Z","receivedAt":"2021-10-09T02:36:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 09, 2021 at 02:58:10AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I ran into this while testing the grep coloring patch[1] (but it's\n> unrelated). Before this commit e.g.:\n> \n> LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100|wc -l\n> 28333\n> \n> So ~3k lines for my last 100 commits, but then:\n> \n> $ LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100 2>&1|grep -c ^warning\n> 299\n> \n> At first I thought it was spewing warnings for every failed re-encoded\n> line in some cases, because I get hundreds at a time sometimes, but it's\n> because stderr and stdout I/O buffering is different (a common\n> case). Adding a \"fflush(stderr)\" \"fixes\" that.\n\nI don't think the buffering is the issue. By default stderr flushes on\nlines, and we flush commits after showing them. If you take away \"-P\"\n(or look at the combined 2>&1 output in order), you'll see that they are\ngrouped.\n\nNow one thing you might notice is that there may be multiple warnings\nbetween output commits. But that's because we really are re-encoding\neach of those intermediate commits to do your --author grep. And if that\nre-encoding fails, we may well be producing the wrong output, because\nthe matching won't be correct (in your case, presumably the correct\noutput should be _nothing_, because Æ is not an ascii character).\n\nI do think the current warning is particularly bad there, because it\ndoesn't even mention the commit oid. So something like:\n\ndiff --git a/pretty.c b/pretty.c\nindex 708b618cfe..ddf501632d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -673,7 +673,8 @@ const char *repo_logmsg_reencode(struct repository *r,\n \t * case we just return the commit message verbatim.\n \t */\n \tif (!out) {\n-\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n+\t\twarning(\"unable to reencode commit %s to '%s'\",\n+\t\t\toid_to_hex(&commit->object.oid), output_encoding);\n \t\treturn msg;\n \t}\n \treturn out;\n\nmeans you get output like:\n\n $ git -c i18n.commitEncoding=ascii log --format='%h %s' --author=Ævar -100\n warning: unable to reencode commit c90cfc225baaf64af311f7e2953267e4de636205 to 'ascii'\n warning: unable to reencode commit 1d1d731d30cbcd5f3a6a5cbac1fe218e4d4db72b to 'ascii'\n warning: unable to reencode commit 66237bcf60df357f188551e1ea4db90f94c519ae to 'ascii'\n warning: unable to reencode commit 100c2da2d3a330366588143d720f09a88926972a to 'ascii'\n warning: unable to reencode commit 59580685bee17de3efff614df7f508133d1e4a7a to 'ascii'\n 59580685be config.h: remove unused git_config_get_untracked_cache() declaration\n warning: unable to reencode commit 067e73c8aee9aeb05eac939205274cd2ad8b7cae to 'ascii'\n 067e73c8ae log-tree.h: remove unused function declarations\n [...etc...]\n\nIf that were coupled with, say, an advise() call to explain that output\nand matching might be inaccurate (and show that _once_), that might\nmight it more clear what's going on.\n\nNow I am sympathetic to flooding the user with too many messages, and\nmaybe reducing this to a single instance of \"some commit messages could\nnot be re-encoded; output and matching might be inaccurate\" is the right\nthing. But in a sense, it's also working as designed: what you asked for\nis producing wrong output over and over, and Git is saying so.\n\nI'm not even sure what you're trying to do with that command. It could\nnever output a single correct commit, because you've asked to match only\ncommits that will be shown in the wrong encoding.\n\n> But anyway, I think we've got a lot of users who say *do* want to\n> reencode something from say UTF-8 to latin1, but then might have the\n> occasional non-latin1 representable data. The old behavior of silently\n> falling back is going to be much better for those users, or maybe show\n> one warning at the end or something, if you feel it really needs to be\n> kept.\n\nIf there are real-world cases where the quantity of errors is really\ngetting in the way, I'm open to the idea of having a single error\nmessage. And personally, I don't really have any experience working with\nbroken encodings (all my commits are in utf8, and that's what I use as\noutput). It just seems weird to me that 'git log --encoding=foo' would\nquietly ignore the option entirely (i.e., the old behavior, which did\nlead to a confused user and a post to the list).\n\n-Peff\n"},{"id":"438391","messageId":"YWEBmJk0aENR5Yeo@coredump.intra.peff.net","threadId":"56359","inReplyTo":"YWEAEjIN0HVHbIpg@coredump.intra.peff.net","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-09T02:42:32Z","receivedAt":"2021-10-09T02:42:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 08, 2021 at 10:36:02PM -0400, Jeff King wrote:\n\n> If that were coupled with, say, an advise() call to explain that output\n> and matching might be inaccurate (and show that _once_), that might\n> might it more clear what's going on.\n> \n> Now I am sympathetic to flooding the user with too many messages, and\n> maybe reducing this to a single instance of \"some commit messages could\n> not be re-encoded; output and matching might be inaccurate\" is the right\n> thing. But in a sense, it's also working as designed: what you asked for\n> is producing wrong output over and over, and Git is saying so.\n\nThe single-output version would perhaps be something like this:\n\ndiff --git a/pretty.c b/pretty.c\nindex 708b618cfe..c86f41bae7 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -606,6 +606,21 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \treturn strbuf_detach(&tmp, NULL);\n }\n \n+static void show_encoding_warning(const char *output_encoding)\n+{\n+\tstatic int seen_warning;\n+\n+\tif (seen_warning)\n+\t\treturn;\n+\n+\tseen_warning = 1;\n+\twarning(\"one or more commits could not be re-encoded to '%s'\",\n+\t\toutput_encoding);\n+\tadvise(\"When re-encoding fails, some output may be in an unexpected\\n\"\n+\t \"encoding, and pattern matches against commit data may be\\n\"\n+\t \"inaccurate.\");\n+}\n+\n const char *repo_logmsg_reencode(struct repository *r,\n \t\t\t\t const struct commit *commit,\n \t\t\t\t char **commit_encoding,\n@@ -673,7 +688,7 @@ const char *repo_logmsg_reencode(struct repository *r,\n \t * case we just return the commit message verbatim.\n \t */\n \tif (!out) {\n-\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n+\t\tshow_encoding_warning(output_encoding);\n \t\treturn msg;\n \t}\n \treturn out;\n"},{"id":"438407","messageId":"871r4umfnm.fsf@evledraar.gmail.com","threadId":"56359","inReplyTo":"YWEBmJk0aENR5Yeo@coredump.intra.peff.net","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T13:47:16Z","receivedAt":"2021-10-09T14:33:39Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 08 2021, Jeff King wrote:\n\n> On Fri, Oct 08, 2021 at 10:36:02PM -0400, Jeff King wrote:\n>\n>> If that were coupled with, say, an advise() call to explain that output\n>> and matching might be inaccurate (and show that _once_), that might\n>> might it more clear what's going on.\n>> \n>> Now I am sympathetic to flooding the user with too many messages, and\n>> maybe reducing this to a single instance of \"some commit messages could\n>> not be re-encoded; output and matching might be inaccurate\" is the right\n>> thing. But in a sense, it's also working as designed: what you asked for\n>> is producing wrong output over and over, and Git is saying so.\n>\n> The single-output version would perhaps be something like this:\n>\n> diff --git a/pretty.c b/pretty.c\n> index 708b618cfe..c86f41bae7 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -606,6 +606,21 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n> \treturn strbuf_detach(&tmp, NULL);\n> }\n> \n> +static void show_encoding_warning(const char *output_encoding)\n> +{\n> +\tstatic int seen_warning;\n> +\n> +\tif (seen_warning)\n> +\t\treturn;\n> +\n> +\tseen_warning = 1;\n> +\twarning(\"one or more commits could not be re-encoded to '%s'\",\n> +\t\toutput_encoding);\n> +\tadvise(\"When re-encoding fails, some output may be in an unexpected\\n\"\n> +\t \"encoding, and pattern matches against commit data may be\\n\"\n> +\t \"inaccurate.\");\n> +}\n> +\n> const char *repo_logmsg_reencode(struct repository *r,\n> \t\t\t\t const struct commit *commit,\n> \t\t\t\t char **commit_encoding,\n> @@ -673,7 +688,7 @@ const char *repo_logmsg_reencode(struct repository *r,\n> \t * case we just return the commit message verbatim.\n> \t */\n> \tif (!out) {\n> -\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n> +\t\tshow_encoding_warning(output_encoding);\n> \t\treturn msg;\n> \t}\n> \treturn out;\n\nI'm not categorically opposed to having this warning stay, because I can\nimagine an implementation of it that isn't so overmatching, but I think\nneither one of us is going to work on that, so ....\n\nWe have the exact same edge case in the grep-a-file code, and it's a\ndelibirate decision to stay quiet about it.\n\nLet's assume your pattern contains non-ASCII, you've asked for\nlocale-aware grepping, you want \\d+ to mean all sorts of Unicode\nfuzzyness instead of just [0-9] etc. (not yet implemented, PCRE_UCP).\n\nIt would still be annoying to see a warning every time you grep without\nproviding a pathspec that blacklists say the 100 '*.png' files that are\nin your tree.\n\nAnd that's a case where we *could* say that the user should mark them\nwith .gitattributes or whatever, but making every git user go through\nthat would be annoying to them, so we just do our best and silently fall\nback.\n\nSimilarly with this, let's say I'm on an OS that likes UTF-16 better, as\nsome of our users do, I have the relevant settings to re-encode git.git\nor linux.git. Now run:\n\n git -c i18n.logOutputEncoding=utf16 log --author=foobarbaz\n\nAnd it's 2 warnings in git.git, and 157 in linux.git. Anyway, your\ncommit above makes that 1 in both cases, which is certainly a *lot*\nbetter.\n\nBut I think similar to the grep-a-file case it's still way to much, now\njust because I've got some old badly encoded commits in my history I'll\nsee one warning every time a log revision walk/grep comes across those.\n\nOn the \"not categorically opposed\" I think that this sort of warning\n/might/ be good if:\n\n * It weren't enabled by default, or at least as a transition had\n something like a advise() message pointing at a fsck.skipList-like\n (or other instructions, replace?) about how to quiet it.\n\n * We're realy dumb with how we chain data->iconv->PCRE here. I.e. we'll\n whine that we can't reencode just to match my \"foobarbaz\", but we\n could just keep walking past bad bytes. We should ideally say \"we\n might have matched your data, but *because* of the encoding failure\n we couldn't.\n\n We can easily know with something like \"foobarbaz\" that that's not\n the case.\n\nAnyway, I think all of that we can leave for the future, because I'd\nsimply assumed that this was based on some report that had to do with\nsomeone not matching with --grep or whatever because of the details of\nthe encoding going wrong, e.g. a string that's later in a commit\nmessage, but a misencoded character tripped it up.\n\nBut in this case this seems to have been because someone tried to feed\n\"HTML\" to it, which is not an encoding, and something iconv_open() has\n(I daresay) always and will always error on. It returns -1 and sets\nerrno=EINVAL.\n\nSo having a warning or other detection in the revision loop seems\nbackwards to me, surely we want something like the below instead?\nI.e. die as close to bad option parsing as possible?\n\nNote that this will now die if we have NO_ICONV=Y, even with your patch,\nthat seems like a feature. Now we'll silently ignore it. I.e. we'll warn\nbecause we failed to re-encode, but we're using a stub function whose\nbody is:\n\n { if (e) *e = 0; return NULL; }\n\nSo ditto the garbage encoding name we should have died a lot earlier.\n\nAside from your warning test the below makes tests in t4201-shortlog.sh\nfail, but those just seem broken to me. I.e. they seem to rely on git\nstaying quiet if i18n.commitencoding is set to garbage.\n\ndiff --git a/environment.c b/environment.c\nindex 43bb1b35ffe..c26b18f8e5c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -357,8 +357,18 @@ void set_git_dir(const char *path, int make_realpath)\n \n const char *get_log_output_encoding(void)\n {\n-\treturn git_log_output_encoding ? git_log_output_encoding\n+\tconst char *encoding = git_log_output_encoding ? git_log_output_encoding\n \t\t: get_commit_output_encoding();\n+#ifndef NO_ICONV\n+\ticonv_t conv;\n+\tconv = iconv_open(encoding, \"UTF-8\");\n+\tif (conv == (iconv_t) -1 && errno == EINVAL)\n+\t\tdie_errno(\"the '%s' encoding is not known to iconv\", encoding);\n+#else\n+\tif (strcmp(encoding, \"UTF-8\"))\n+\t\tdie(\"compiled with NO_ICONV=Y, can't re-encode to '%s'\", encoding);\n+#endif\n+\treturn encoding;\n }\n \n const char *get_commit_output_encoding(void)\n"},{"id":"438417","messageId":"5eca71b7-e4df-92a1-35bf-5a99550e558e@kdbg.org","threadId":"56359","inReplyTo":"YWEAEjIN0HVHbIpg@coredump.intra.peff.net","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-10-10T13:53:31Z","receivedAt":"2021-10-10T13:53:38Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.10.21 um 04:36 schrieb Jeff King:\n> On Sat, Oct 09, 2021 at 02:58:10AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n>> I ran into this while testing the grep coloring patch[1] (but it's\n>> unrelated). Before this commit e.g.:\n>>\n>> LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100|wc -l\n>> 28333\n>>\n>> So ~3k lines for my last 100 commits, but then:\n>>\n>> $ LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100 2>&1|grep -c ^warning\n>> 299\n>>\n>> At first I thought it was spewing warnings for every failed re-encoded\n>> line in some cases, because I get hundreds at a time sometimes, but it's\n>> because stderr and stdout I/O buffering is different (a common\n>> case). Adding a \"fflush(stderr)\" \"fixes\" that.\n> \n> I don't think the buffering is the issue. By default stderr flushes on\n> lines, and we flush commits after showing them. If you take away \"-P\"\n> (or look at the combined 2>&1 output in order), you'll see that they are\n> grouped.\n> \n> Now one thing you might notice is that there may be multiple warnings\n> between output commits. But that's because we really are re-encoding\n> each of those intermediate commits to do your --author grep. And if that\n> re-encoding fails, we may well be producing the wrong output, because\n> the matching won't be correct (in your case, presumably the correct\n> output should be _nothing_, because Æ is not an ascii character).\n\nI don't understand why i18n.commitEncoding plays a role here. Isn't it\nan instruction \"when you make a commit, mark the commit message having\nthis encoding\". But grep does not make a commit.\n\nIf this were i18n.logOuputEncoding it would make much more sense.\n\nHave I misunderstood the meaning of the two options?\n\n-- Hannes\n"},{"id":"438418","messageId":"87sfx8lw8j.fsf@evledraar.gmail.com","threadId":"56359","inReplyTo":"5eca71b7-e4df-92a1-35bf-5a99550e558e@kdbg.org","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-10T15:43:42Z","receivedAt":"2021-10-10T15:45:23Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Oct 10 2021, Johannes Sixt wrote:\n\n> Am 09.10.21 um 04:36 schrieb Jeff King:\n>> On Sat, Oct 09, 2021 at 02:58:10AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> \n>>> I ran into this while testing the grep coloring patch[1] (but it's\n>>> unrelated). Before this commit e.g.:\n>>>\n>>> LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100|wc -l\n>>> 28333\n>>>\n>>> So ~3k lines for my last 100 commits, but then:\n>>>\n>>> $ LC_ALL=C ~/g/git/git -P -c i18n.commitEncoding=ascii log --author=Ævar -100 2>&1|grep -c ^warning\n>>> 299\n>>>\n>>> At first I thought it was spewing warnings for every failed re-encoded\n>>> line in some cases, because I get hundreds at a time sometimes, but it's\n>>> because stderr and stdout I/O buffering is different (a common\n>>> case). Adding a \"fflush(stderr)\" \"fixes\" that.\n>> \n>> I don't think the buffering is the issue. By default stderr flushes on\n>> lines, and we flush commits after showing them. If you take away \"-P\"\n>> (or look at the combined 2>&1 output in order), you'll see that they are\n>> grouped.\n>> \n>> Now one thing you might notice is that there may be multiple warnings\n>> between output commits. But that's because we really are re-encoding\n>> each of those intermediate commits to do your --author grep. And if that\n>> re-encoding fails, we may well be producing the wrong output, because\n>> the matching won't be correct (in your case, presumably the correct\n>> output should be _nothing_, because Æ is not an ascii character).\n>\n> I don't understand why i18n.commitEncoding plays a role here. Isn't it\n> an instruction \"when you make a commit, mark the commit message having\n> this encoding\". But grep does not make a commit.\n>\n> If this were i18n.logOuputEncoding it would make much more sense.\n>\n> Have I misunderstood the meaning of the two options?\n\nIt doesn't, see my later <871r4umfnm.fsf@evledraar.gmail.com> for when I\ngot it right.\n\nFor the amount of warnings etc. it's the same, whether we call iconv\nbecause it's e.g. ascii->utf-8 and that triggers iconv() issues, or\n(with i18n.logOuputEncoding) utf-8->ascii.\n"},{"id":"439435","messageId":"211023.86sfwsis1i.gmgdl@evledraar.gmail.com","threadId":"56359","inReplyTo":"YWEBmJk0aENR5Yeo@coredump.intra.peff.net","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T22:58:34Z","receivedAt":"2021-10-22T23:00:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 08 2021, Jeff King wrote:\n\n> On Fri, Oct 08, 2021 at 10:36:02PM -0400, Jeff King wrote:\n>\n>> If that were coupled with, say, an advise() call to explain that output\n>> and matching might be inaccurate (and show that _once_), that might\n>> might it more clear what's going on.\n>> \n>> Now I am sympathetic to flooding the user with too many messages, and\n>> maybe reducing this to a single instance of \"some commit messages could\n>> not be re-encoded; output and matching might be inaccurate\" is the right\n>> thing. But in a sense, it's also working as designed: what you asked for\n>> is producing wrong output over and over, and Git is saying so.\n>\n> The single-output version would perhaps be something like this:\n>\n> diff --git a/pretty.c b/pretty.c\n> index 708b618cfe..c86f41bae7 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -606,6 +606,21 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n> \treturn strbuf_detach(&tmp, NULL);\n> }\n> \n> +static void show_encoding_warning(const char *output_encoding)\n> +{\n> +\tstatic int seen_warning;\n> +\n> +\tif (seen_warning)\n> +\t\treturn;\n> +\n> +\tseen_warning = 1;\n> +\twarning(\"one or more commits could not be re-encoded to '%s'\",\n> +\t\toutput_encoding);\n> +\tadvise(\"When re-encoding fails, some output may be in an unexpected\\n\"\n> +\t \"encoding, and pattern matches against commit data may be\\n\"\n> +\t \"inaccurate.\");\n> +}\n> +\n> const char *repo_logmsg_reencode(struct repository *r,\n> \t\t\t\t const struct commit *commit,\n> \t\t\t\t char **commit_encoding,\n> @@ -673,7 +688,7 @@ const char *repo_logmsg_reencode(struct repository *r,\n> \t * case we just return the commit message verbatim.\n> \t */\n> \tif (!out) {\n> -\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n> +\t\tshow_encoding_warning(output_encoding);\n> \t\treturn msg;\n> \t}\n> \treturn out;\n\n*Poke* about this. We're getting pretty close to release. I think the\nWIP hunk I posted in\nhttps://lore.kernel.org/git/871r4umfnm.fsf@evledraar.gmail.com/ presents\na good way forward with this.\n\nI.e. aside from how wise it is to warn about this in general, I think\nthere's a pretty bad bug in your implementation where what should\neffectively be a parse_options() or git_config()-time one-off warning is\nbeing fired off for every single commit in \"git log\" in some cases.\n"},{"id":"439749","messageId":"YXkx6WzoF+B1id5T@coredump.intra.peff.net","threadId":"56359","inReplyTo":"871r4umfnm.fsf@evledraar.gmail.com","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-27T11:03:05Z","receivedAt":"2021-10-27T11:03:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 09, 2021 at 03:47:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> But in this case this seems to have been because someone tried to feed\n> \"HTML\" to it, which is not an encoding, and something iconv_open() has\n> (I daresay) always and will always error on. It returns -1 and sets\n> errno=EINVAL.\n> \n> So having a warning or other detection in the revision loop seems\n> backwards to me, surely we want something like the below instead?\n> I.e. die as close to bad option parsing as possible?\n\nSorry for the slow response; this got thrown on my \"to think about and\nlook at later\" pile.\n\nYeah, I agree that if we sanity-checked the encoding up front, that\nwould cover the case we saw in practice, and goes a long way towards\ncatching any practical errors.\n\nBut I think this patch is tricky:\n\n> diff --git a/environment.c b/environment.c\n> index 43bb1b35ffe..c26b18f8e5c 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -357,8 +357,18 @@ void set_git_dir(const char *path, int make_realpath)\n> \n> const char *get_log_output_encoding(void)\n> {\n> -\treturn git_log_output_encoding ? git_log_output_encoding\n> +\tconst char *encoding = git_log_output_encoding ? git_log_output_encoding\n> \t\t: get_commit_output_encoding();\n> +#ifndef NO_ICONV\n> +\ticonv_t conv;\n> +\tconv = iconv_open(encoding, \"UTF-8\");\n> +\tif (conv == (iconv_t) -1 && errno == EINVAL)\n> +\t\tdie_errno(\"the '%s' encoding is not known to iconv\", encoding);\n> +#else\n> +\tif (strcmp(encoding, \"UTF-8\"))\n> +\t\tdie(\"compiled with NO_ICONV=Y, can't re-encode to '%s'\", encoding);\n> +#endif\n> +\treturn encoding;\n> }\n\nSo one obvious problem here is that we call this function once per\ncommit, so it's a lot of extra iconv_open() calls. But obviously we\ncould use a static flag to do it once per process.\n\nThe other issue is that it is assuming UTF-8 on one end of the\nconversion. But we aren't necessarily doing such a conversion; it\ndepends on the commit's on-disk encoding, and the requested output\nencoding. In particular:\n\n - if both of those match, we do not need to call iconv at all (see the\n same_encoding() check in repo_logmsg_reencode()). With the patch\n above, the NO_ICONV case would start to die() when both are say\n iso8859-1, even though it currently works.\n\n - likewise, even if you have iconv support, it's possible that your\n preferred encoding is not compatible with utf8. In which case\n iconv_open() may complain, even though the actual conversion we'd\n ask it to do would succeed.\n\nI.e., I don't think there's a way to just ask iconv \"does this encoding\nname by itself make any sense\". You can only ask it about to/from\ncombos.\n\nSo I think a much better version of this is to catch the _actual_\niconv_open() call we make. And if it fails, say \"woah, this combo of\nencodings isn't supported\". The reason I didn't do that in the earlier\npatch is that all of this is obscured inside reencode_string_len(),\nwhich does both the iconv_open() and the iconv() call. We could surface\nthat error information.\n\nBut I'm not sure it would make sense to die() in that case. While for\nsomething like \"git log --encoding=nonsense\" every commit is going to\nfail to re-encode, it's still possible that iconv_open() failures are\ncommit-specific. I.e., you could have some garbage commit in your\nhistory with an unsupported encoding, and you wouldn't want to die() for\nit (it's the same case you are complaining about having a warning for,\nbut much worse).\n\nI suspect the best we could do along these lines is to wait until a real\niconv_open(to, from) fails, and then as a fallback try:\n\n iconv_open(\"UTF-8\", from);\n iconv_open(to, \"UTF-8\");\n\nto sanity-check them individually, and guess that one of them is broken\nif it can't go to/from UTF-8. But even that feels like it's making\nassumptions about both the system iconv, and the charsets people use.\n\nTo be clear, I'd expect that most people just use utf-8 in the first\nplace, and even if they don't that their system has some basic utf-8\nsupport. But we are deep into the realm of weird corner cases here, and\nthe utility of this warning / error-checking doesn't seem high enough to\nmerit the possible regressions we'd get by trying to make too many\nassumptions.\n\n-Peff\n"},{"id":"439992","messageId":"211029.86bl38w124.gmgdl@evledraar.gmail.com","threadId":"56359","inReplyTo":"YXkx6WzoF+B1id5T@coredump.intra.peff.net","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-29T10:47:36Z","receivedAt":"2021-10-29T10:57:45Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 27 2021, Jeff King wrote:\n\n> On Sat, Oct 09, 2021 at 03:47:16PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> But in this case this seems to have been because someone tried to feed\n>> \"HTML\" to it, which is not an encoding, and something iconv_open() has\n>> (I daresay) always and will always error on. It returns -1 and sets\n>> errno=EINVAL.\n>> \n>> So having a warning or other detection in the revision loop seems\n>> backwards to me, surely we want something like the below instead?\n>> I.e. die as close to bad option parsing as possible?\n>\n> Sorry for the slow response; this got thrown on my \"to think about and\n> look at later\" pile.\n>\n> Yeah, I agree that if we sanity-checked the encoding up front, that\n> would cover the case we saw in practice, and goes a long way towards\n> catching any practical errors.\n>\n> But I think this patch is tricky:\n>\n>> diff --git a/environment.c b/environment.c\n>> index 43bb1b35ffe..c26b18f8e5c 100644\n>> --- a/environment.c\n>> +++ b/environment.c\n>> @@ -357,8 +357,18 @@ void set_git_dir(const char *path, int make_realpath)\n>> \n>> const char *get_log_output_encoding(void)\n>> {\n>> -\treturn git_log_output_encoding ? git_log_output_encoding\n>> +\tconst char *encoding = git_log_output_encoding ? git_log_output_encoding\n>> \t\t: get_commit_output_encoding();\n>> +#ifndef NO_ICONV\n>> +\ticonv_t conv;\n>> +\tconv = iconv_open(encoding, \"UTF-8\");\n>> +\tif (conv == (iconv_t) -1 && errno == EINVAL)\n>> +\t\tdie_errno(\"the '%s' encoding is not known to iconv\", encoding);\n>> +#else\n>> +\tif (strcmp(encoding, \"UTF-8\"))\n>> +\t\tdie(\"compiled with NO_ICONV=Y, can't re-encode to '%s'\", encoding);\n>> +#endif\n>> +\treturn encoding;\n>> }\n>\n> So one obvious problem here is that we call this function once per\n> commit, so it's a lot of extra iconv_open() calls. But obviously we\n> could use a static flag to do it once per process.\n\nYes, or the diff below, which seems like a better idea. I.e. stop\ncalling this in a loop if we know we'll need it, have setup_revisions()\npopulate it once.\n\n> The other issue is that it is assuming UTF-8 on one end of the\n> conversion. But we aren't necessarily doing such a conversion; it\n> depends on the commit's on-disk encoding, and the requested output\n> encoding. In particular:\n>\n> - if both of those match, we do not need to call iconv at all (see the\n> same_encoding() check in repo_logmsg_reencode()). With the patch\n> above, the NO_ICONV case would start to die() when both are say\n> iso8859-1, even though it currently works.\n>\n> - likewise, even if you have iconv support, it's possible that your\n> preferred encoding is not compatible with utf8. In which case\n> iconv_open() may complain, even though the actual conversion we'd\n> ask it to do would succeed.\n>\n> I.e., I don't think there's a way to just ask iconv \"does this encoding\n> name by itself make any sense\". You can only ask it about to/from\n> combos.\n\nYes, I'm not saying it covers the general problem, but that it covers\nthe specific complained-about issue of a completely nonsensical encoding\nlike \"HTML\". We should simply error on that on command startup, whether\nor not we have any commits to visit.\n\nBut yes, if we want to cover specific encoding issues, e.g. not being\nable to squash CJK UTF-8 into US-ASCII that needs to be per-commit,\nbut...\n\n> So I think a much better version of this is to catch the _actual_\n> iconv_open() call we make. And if it fails, say \"woah, this combo of\n> encodings isn't supported\". The reason I didn't do that in the earlier\n> patch is that all of this is obscured inside reencode_string_len(),\n> which does both the iconv_open() and the iconv() call. We could surface\n> that error information.\n>\n> But I'm not sure it would make sense to die() in that case. While for\n> something like \"git log --encoding=nonsense\" every commit is going to\n> fail to re-encode, it's still possible that iconv_open() failures are\n> commit-specific. I.e., you could have some garbage commit in your\n> history with an unsupported encoding, and you wouldn't want to die() for\n> it (it's the same case you are complaining about having a warning for,\n> but much worse).\n>\n> I suspect the best we could do along these lines is to wait until a real\n> iconv_open(to, from) fails, and then as a fallback try:\n>\n> iconv_open(\"UTF-8\", from);\n> iconv_open(to, \"UTF-8\");\n>\n> to sanity-check them individually, and guess that one of them is broken\n> if it can't go to/from UTF-8. But even that feels like it's making\n> assumptions about both the system iconv, and the charsets people use.\n>\n> To be clear, I'd expect that most people just use utf-8 in the first\n> place, and even if they don't that their system has some basic utf-8\n> support. But we are deep into the realm of weird corner cases here, and\n> the utility of this warning / error-checking doesn't seem high enough to\n> merit the possible regressions we'd get by trying to make too many\n> assumptions.\n\nI still think the right move here is to just revert your patch. Yes we\ncan think of a bunch of tricky edge cases that need to be fixed, but\nthis is a long-standing problem, and the change as-is has some pretty\nbad UI problems.\n\nI think any change that solves those (probably starting with the below)\nis going to be relatively major this close to release.\n\nSo per <87ily7m1mv.fsf@evledraar.gmail.com> why can't we just revert the\nwarning(), and then consider a good way forward that covers some/all of\nthese cases we've noted?\n\nWhich should probably start with the below diff, so we can clearly\ndistinguish startup bad encoding names from runtime ones (we'd put that\nproposed \"die if iconv_open() doesn't like it\" somewhere in\nsetup_revisions()).\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f75d87e8d7f..2b3a607e947 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -539,7 +539,7 @@ static void show_tagger(const char *buf, struct rev_info *rev)\n \n \tpp.fmt = rev->commit_format;\n \tpp.date_mode = rev->date_mode;\n-\tpp_user_info(&pp, \"Tagger\", &out, buf, get_log_output_encoding());\n+\tpp_user_info(&pp, \"Tagger\", &out, buf, rev->log_output_encoding);\n \tfprintf(rev->diffopt.file, \"%s\", out.buf);\n \tstrbuf_release(&out);\n }\n@@ -1208,7 +1208,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tlog.file = rev->diffopt.file;\n \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n \tfor (i = 0; i < nr; i++)\n-\t\tshortlog_add_commit(&log, list[i]);\n+\t\tshortlog_add_commit(rev, &log, list[i]);\n \n \tshortlog_output(&log);\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex ea3112e0c0b..1a8fbafc149 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -431,6 +431,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \tctx.abbrev = rev.abbrev;\n \tctx.date_mode = rev.date_mode;\n \tctx.fmt = rev.commit_format;\n+\tctx.output_encoding = rev.log_output_encoding;\n \n \tstrbuf_addstr(&out, \"Squashed commit of the following:\\n\");\n \twhile ((commit = get_revision(&rev)) != NULL) {\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 36cb909ebaa..905ba7462f3 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -165,7 +165,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\tctx.date_mode = revs->date_mode;\n \t\tctx.date_mode_explicit = revs->date_mode_explicit;\n \t\tctx.fmt = revs->commit_format;\n-\t\tctx.output_encoding = get_log_output_encoding();\n+\t\tctx.output_encoding = revs->log_output_encoding;\n \t\tctx.color = revs->diffopt.use_color;\n \t\tpretty_print_commit(&ctx, commit, &buf);\n \t\tif (buf.len) {\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex e7f7af5de3f..2e5409a4fcc 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -198,7 +198,8 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n-void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n+void shortlog_add_commit(struct rev_info *rev, struct shortlog *log,\n+\t\t\t struct commit *commit)\n {\n \tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n@@ -210,7 +211,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.abbrev = log->abbrev;\n \tctx.print_email_subject = 1;\n \tctx.date_mode.type = DATE_NORMAL;\n-\tctx.output_encoding = get_log_output_encoding();\n+\tctx.output_encoding = rev->log_output_encoding;\n \n \tif (!log->summary) {\n \t\tif (log->user_format)\n@@ -254,7 +255,7 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n \tif (prepare_revision_walk(rev))\n \t\tdie(_(\"revision walk setup failed\"));\n \twhile ((commit = get_revision(rev)) != NULL)\n-\t\tshortlog_add_commit(log, commit);\n+\t\tshortlog_add_commit(rev, log, commit);\n }\n \n static int parse_uint(char const **arg, int comma, int defval)\ndiff --git a/bundle.c b/bundle.c\nindex a0bb687b0f4..32d74ab0bee 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -448,6 +448,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \n struct bundle_prerequisites_info {\n \tstruct object_array *pending;\n+\tstruct rev_info *rev;\n \tint fd;\n };\n \n@@ -464,7 +465,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)\n \twrite_or_die(bpi->fd, buf.buf, buf.len);\n \n \tctx.fmt = CMIT_FMT_ONELINE;\n-\tctx.output_encoding = get_log_output_encoding();\n+\tctx.output_encoding = bpi->rev->log_output_encoding;\n \tstrbuf_reset(&buf);\n \tpretty_print_commit(&ctx, commit, &buf);\n \tstrbuf_trim(&buf);\n@@ -544,6 +545,7 @@ int create_bundle(struct repository *r, const char *path,\n \t\tdie(\"revision walk setup failed\");\n \tbpi.fd = bundle_fd;\n \tbpi.pending = &revs_copy.pending;\n+\tbpi.rev = &revs;\n \ttraverse_commit_list(&revs, write_bundle_prerequisites, NULL, &bpi);\n \tobject_array_remove_duplicates(&revs_copy.pending);\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 644893fd8cf..d79324d5bfd 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -737,7 +737,7 @@ void show_log(struct rev_info *opt)\n \n \t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT);\n \t\tformat_display_notes(&commit->object.oid, ¬ebuf,\n-\t\t\t\t get_log_output_encoding(), raw);\n+\t\t\t\t opt->log_output_encoding, raw);\n \t\tctx.notes_message = strbuf_detach(¬ebuf, NULL);\n \t}\n \n@@ -758,7 +758,7 @@ void show_log(struct rev_info *opt)\n \tctx.mailmap = opt->mailmap;\n \tctx.color = opt->diffopt.use_color;\n \tctx.expand_tabs_in_log = opt->expand_tabs_in_log;\n-\tctx.output_encoding = get_log_output_encoding();\n+\tctx.output_encoding = opt->log_output_encoding;\n \tctx.rev = opt;\n \tif (opt->from_ident.mail_begin && opt->from_ident.name_begin)\n \t\tctx.from_ident = &opt->from_ident;\ndiff --git a/pretty.c b/pretty.c\nindex fe95107ae5a..6eb64e8189d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -2048,15 +2048,17 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tint indent = 4;\n \tconst char *msg;\n \tconst char *reencoded;\n-\tconst char *encoding;\n+\tconst char *encoding = pp->output_encoding;\n \tint need_8bit_cte = pp->need_8bit_cte;\n \n+\tif (!encoding)\n+\t\tBUG(\"should have .output_encoding\");\n+\n \tif (pp->fmt == CMIT_FMT_USERFORMAT) {\n \t\tformat_commit_message(commit, user_format, sb, pp);\n \t\treturn;\n \t}\n \n-\tencoding = get_log_output_encoding();\n \tmsg = reencoded = logmsg_reencode(commit, NULL, encoding);\n \n \tif (pp->fmt == CMIT_FMT_ONELINE || cmit_fmt_is_mail(pp->fmt))\n@@ -2121,6 +2123,14 @@ void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,\n \t\t struct strbuf *sb)\n {\n \tstruct pretty_print_context pp = {0};\n+\tstatic const char *output_encoding;\n+\n+\tif (!output_encoding) {\n+\t\tconst char *tmp = get_log_output_encoding();\n+\t\toutput_encoding = tmp;\n+\t}\n+\t\n \tpp.fmt = fmt;\n+\tpp.output_encoding = output_encoding;\n \tpretty_print_commit(&pp, commit, sb);\n }\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d69156312bd..733a525bc73 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -345,8 +345,14 @@ static void free_discovery(struct discovery *d)\n static int show_http_message(struct strbuf *type, struct strbuf *charset,\n \t\t\t struct strbuf *msg)\n {\n+\tstatic const char *output_encoding;\n \tconst char *p, *eol;\n \n+\tif (!output_encoding) {\n+\t\tconst char *tmp = get_log_output_encoding();\n+\t\toutput_encoding = tmp;\n+\t}\n+\n \t/*\n \t * We only show text/plain parts, as other types are likely\n \t * to be ugly to look at on the user's terminal.\n@@ -354,7 +360,7 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,\n \tif (strcmp(type->buf, \"text/plain\"))\n \t\treturn -1;\n \tif (charset->len)\n-\t\tstrbuf_reencode(msg, charset->buf, get_log_output_encoding());\n+\t\tstrbuf_reencode(msg, charset->buf, output_encoding);\n \n \tstrbuf_trim(msg);\n \tif (!msg->len)\ndiff --git a/revision.c b/revision.c\nindex ab7c1358042..0b2ad87f28e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2866,7 +2866,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tgrep_commit_pattern_type(GREP_PATTERN_TYPE_UNSPECIFIED,\n \t\t\t\t &revs->grep_filter);\n-\tif (!is_encoding_utf8(get_log_output_encoding()))\n+\trevs->log_output_encoding = get_log_output_encoding();\n+\tif (!is_encoding_utf8(revs->log_output_encoding))\n \t\trevs->grep_filter.ignore_locale = 1;\n \tcompile_grep_patterns(&revs->grep_filter);\n \ndiff --git a/revision.h b/revision.h\nindex 5578bb4720a..bf9dbca727e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -237,6 +237,7 @@ struct rev_info {\n \tstruct string_list *ref_message_ids;\n \tint\t\tadd_signoff;\n \tconst char\t*extra_headers;\n+\tconst char\t*log_output_encoding;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n \tint\t\tpatch_name_max;\ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabcae..f809617f8a0 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -30,7 +30,9 @@ struct shortlog {\n \n void shortlog_init(struct shortlog *log);\n \n-void shortlog_add_commit(struct shortlog *log, struct commit *commit);\n+struct rev_info;\n+void shortlog_add_commit(struct rev_info *rev, struct shortlog *log,\n+\t\t\t struct commit *commit);\n \n void shortlog_output(struct shortlog *log);\n \ndiff --git a/submodule.c b/submodule.c\nindex c6890705241..f10e9c34ff6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -501,7 +501,7 @@ static void print_submodule_diff_summary(struct repository *r, struct rev_info *\n \twhile ((commit = get_revision(rev))) {\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.date_mode = rev->date_mode;\n-\t\tctx.output_encoding = get_log_output_encoding();\n+\t\tctx.output_encoding = rev->log_output_encoding;\n \t\tstrbuf_setlen(&sb, 0);\n \t\trepo_format_commit_message(r, commit, format, &sb,\n \t\t\t\t &ctx);\n"},{"id":"440062","messageId":"YXxcIQQS7GQzRwUa@coredump.intra.peff.net","threadId":"56359","inReplyTo":"211029.86bl38w124.gmgdl@evledraar.gmail.com","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-29T20:40:01Z","receivedAt":"2021-10-29T20:40:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 29, 2021 at 12:47:36PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > The other issue is that it is assuming UTF-8 on one end of the\n> > conversion. But we aren't necessarily doing such a conversion; it\n> > depends on the commit's on-disk encoding, and the requested output\n> > encoding. In particular:\n> >\n> > - if both of those match, we do not need to call iconv at all (see the\n> > same_encoding() check in repo_logmsg_reencode()). With the patch\n> > above, the NO_ICONV case would start to die() when both are say\n> > iso8859-1, even though it currently works.\n> >\n> > - likewise, even if you have iconv support, it's possible that your\n> > preferred encoding is not compatible with utf8. In which case\n> > iconv_open() may complain, even though the actual conversion we'd\n> > ask it to do would succeed.\n> >\n> > I.e., I don't think there's a way to just ask iconv \"does this encoding\n> > name by itself make any sense\". You can only ask it about to/from\n> > combos.\n> \n> Yes, I'm not saying it covers the general problem, but that it covers\n> the specific complained-about issue of a completely nonsensical encoding\n> like \"HTML\". We should simply error on that on command startup, whether\n> or not we have any commits to visit.\n\nI definitely agree with you on the direction, and I don't mind if we\ndon't cover every case. What I was trying to point out above though is\nthat the patch you showed actually _regresses_ some cases, and it's hard\nto robustly avoid that.\n\n> So per <87ily7m1mv.fsf@evledraar.gmail.com> why can't we just revert the\n> warning(), and then consider a good way forward that covers some/all of\n> these cases we've noted?\n\nRight, I agreed with that in the other thread. You may need to convince\nJunio. ;)\n\nTBH I am not even sure it is worth spending a lot of brain cells on the\n\"and then consider...\" part. Over all these years, we've had one report,\nand it simply misunderstand what \"--encoding\" was for. I thought it was\nsomething we could fix up easily by checking a return value, but IMHO\ndoing it right is quite tricky because of iconv()'s limited interface,\nand the risk of regression outweighs the potential benefit.\n\n-Peff\n"},{"id":"440064","messageId":"xmqqk0hvk1bc.fsf@gitster.g","threadId":"56359","inReplyTo":"YXxcIQQS7GQzRwUa@coredump.intra.peff.net","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T20:45:11Z","receivedAt":"2021-10-29T20:45:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> TBH I am not even sure it is worth spending a lot of brain cells on the\n> \"and then consider...\" part. Over all these years, we've had one report,\n> and it simply misunderstand what \"--encoding\" was for. I thought it was\n> something we could fix up easily by checking a return value, but IMHO\n> doing it right is quite tricky because of iconv()'s limited interface,\n> and the risk of regression outweighs the potential benefit.\n\nI tend to agree with the above. Let's not over-engineer things.\n"},{"id":"440066","messageId":"xmqqee83k0zx.fsf@gitster.g","threadId":"56359","inReplyTo":"xmqqk0hvk1bc.fsf@gitster.g","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T20:52:02Z","receivedAt":"2021-10-29T20:52:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> TBH I am not even sure it is worth spending a lot of brain cells on the\n>> \"and then consider...\" part. Over all these years, we've had one report,\n>> and it simply misunderstand what \"--encoding\" was for. I thought it was\n>> something we could fix up easily by checking a return value, but IMHO\n>> doing it right is quite tricky because of iconv()'s limited interface,\n>> and the risk of regression outweighs the potential benefit.\n>\n> I tend to agree with the above. Let's not over-engineer things.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nFrom: Junio C Hamano \nDate: Fri, 29 Oct 2021 13:48:58 -0700\nSubject: [PATCH] Revert \"logmsg_reencode(): warn when iconv() fails\"\n\nThis reverts commit fd680bc5 (logmsg_reencode(): warn when iconv()\nfails, 2021-08-27). Throwing a warning for each and every commit\nthat gets reencoded, without allowing a way to squelch, would make\nit unpleasant for folks who have to deal with an ancient part of the\nhistory in an old project that used wrong encoding in the commits.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/pretty-options.txt | 4 +---\n pretty.c | 6 +-----\n t/t4210-log-i18n.sh | 7 -------\n 3 files changed, 2 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex b3af850608..54d8bb3db0 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -40,9 +40,7 @@ people using 80-column terminals.\n \tdefaults to UTF-8. Note that if an object claims to be encoded\n \tin `X` and we are outputting in `X`, we will output the object\n \tverbatim; this means that invalid sequences in the original\n-\tcommit may be copied to the output. Likewise, if iconv(3) fails\n-\tto convert the commit, we will output the original object\n-\tverbatim, along with a warning.\n+\tcommit may be copied to the output.\n \n --expand-tabs=::\n --expand-tabs::\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..9631529c10 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -671,11 +671,7 @@ const char *repo_logmsg_reencode(struct repository *r,\n \t * If the re-encoding failed, out might be NULL here; in that\n \t * case we just return the commit message verbatim.\n \t */\n-\tif (!out) {\n-\t\twarning(\"unable to reencode commit to '%s'\", output_encoding);\n-\t\treturn msg;\n-\t}\n-\treturn out;\n+\treturn out ? out : msg;\n }\n \n static int mailmap_name(const char **email, size_t *email_len,\ndiff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\nindex 0141f36e33..d2dfcf164e 100755\n--- a/t/t4210-log-i18n.sh\n+++ b/t/t4210-log-i18n.sh\n@@ -131,11 +131,4 @@ do\n \tfi\n done\n \n-test_expect_success 'log shows warning when conversion fails' '\n-\tenc=this-encoding-does-not-exist &&\n-\tgit log -1 --encoding=$enc 2>err &&\n-\techo \"warning: unable to reencode commit to ${SQ}${enc}${SQ}\" >expect &&\n-\ttest_cmp expect err\n-'\n-\n test_done\n-- \n2.33.1-1007-g607b33ccc6\n\n"},{"id":"440070","messageId":"YXxjK8FL3HraUEd0@coredump.intra.peff.net","threadId":"56359","inReplyTo":"xmqqee83k0zx.fsf@gitster.g","subject":"Re: *Really* noisy encoding warnings post-v2.33.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-29T21:10:03Z","receivedAt":"2021-10-29T21:10:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 29, 2021 at 01:52:02PM -0700, Junio C Hamano wrote:\n\n> > I tend to agree with the above. Let's not over-engineer things.\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> From: Junio C Hamano \n> Date: Fri, 29 Oct 2021 13:48:58 -0700\n> Subject: [PATCH] Revert \"logmsg_reencode(): warn when iconv() fails\"\n> \n> This reverts commit fd680bc5 (logmsg_reencode(): warn when iconv()\n> fails, 2021-08-27). Throwing a warning for each and every commit\n> that gets reencoded, without allowing a way to squelch, would make\n> it unpleasant for folks who have to deal with an ancient part of the\n> history in an old project that used wrong encoding in the commits.\n\nThanks for tying this up.\n\nI do think fd680bc5's documentation was good, though. So I'd suggest\nsquashing this in, or applying it on top.\n\n-- >8 --\nSubject: [PATCH] log: document --encoding behavior on iconv() failure\n\nWe already note that we may produce invalid output when we skip calling\niconv() altogether. But we may also do so if iconv() fails, and we have\nno good alternative. Let's document this to avoid surprising users.\n\nSigned-off-by: Jeff King \n---\n Documentation/pretty-options.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex 54d8bb3db0..dc685be363 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -40,7 +40,9 @@ people using 80-column terminals.\n \tdefaults to UTF-8. Note that if an object claims to be encoded\n \tin `X` and we are outputting in `X`, we will output the object\n \tverbatim; this means that invalid sequences in the original\n-\tcommit may be copied to the output.\n+\tcommit may be copied to the output. Likewise, if iconv(3) fails\n+\tto convert the commit, we will quietly output the original\n+\tobject verbatim.\n \n --expand-tabs=::\n --expand-tabs::\n-- \n2.33.1.1446.g3ed047b518\n"}]}