{"thread":{"id":"61296","subject":"[PATCH] launch_editor: waiting message on error","startedAt":"2024-04-08T21:02:18Z","lastAt":"2024-04-15T18:44:43Z","messageCount":26,"participants":["Rubén Justo","Junio C Hamano","rsbecker@nexbridge.com","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492582","messageId":"e60c1217-aeb6-48ce-9aa5-7c0b13396e33@gmail.com","threadId":"61296","inReplyTo":null,"subject":"[PATCH] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-08T21:02:15Z","receivedAt":"2024-04-08T21:02:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have the hint we're touching in this commit since abfb04d0c7\n(launch_editor(): indicate that Git waits for user input, 2017-12-07).\n\nAdding a new line after the hint when the editor returns error was\ndiscussed in the list, but finally it was considered not necessary\nbecause a shorter message is used [1].\n\nHowever, even with a short message, feeding that LF makes our next\n\"error: There was a problem with the...\" more clear. For example, the\neditor could print messages that would be mixed with our error message.\nSo, add that LF.\n\nWhile we're here, make the error message follow our CodingGuideLines.\n\n [1] https://public-inbox.org/git/20171127134716.69471-1-lars.schneider@autodesk.com/T/#u\n\nSigned-off-by: Rubén Justo \n---\n editor.c | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex b67b802ddf..a658090a65 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -104,16 +104,25 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_pop(SIGQUIT);\n \t\tif (sig == SIGINT || sig == SIGQUIT)\n \t\t\traise(sig);\n-\t\tif (ret)\n-\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n-\t\t\t\t\teditor);\n \n-\t\tif (print_waiting_for_editor && !is_terminal_dumb())\n-\t\t\t/*\n-\t\t\t * Erase the entire line to avoid wasting the\n-\t\t\t * vertical space.\n-\t\t\t */\n-\t\t\tterm_clear_line();\n+\t\tif (print_waiting_for_editor && !is_terminal_dump())\n+\t\t\tif (!ret)\n+\t\t\t\t/*\n+\t\t\t \t * Erase the entire line to avoid wasting\n+\t\t\t \t * the vertical space.\n+\t\t\t \t */\n+\t\t\t\tterm_clear_line();\n+\t\t\telse\n+\t\t\t\t/*\n+\t\t\t\t * We don't want term_clear_line() here\n+\t\t\t\t * because the editor could have written\n+\t\t\t\t * some useful messages to the user.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"\\n\");\n+\n+\t\tif (ret) \n+\t\t\treturn error(\"there was a problem with the editor '%s'\",\n+\t\t\t\t\teditor);\n \t}\n \n \tif (!buffer)\n-- \n2.43.0\n"},{"id":"492583","messageId":"96bef5f9-1286-4938-99ec-6beed13ee68d@gmail.com","threadId":"61296","inReplyTo":"e60c1217-aeb6-48ce-9aa5-7c0b13396e33@gmail.com","subject":"[PATCH] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-08T21:07:37Z","receivedAt":"2024-04-08T21:07:40Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have the hint we're touching in this commit since abfb04d0c7\n(launch_editor(): indicate that Git waits for user input, 2017-12-07).\n\nAdding a new line after the hint when the editor returns error was\ndiscussed in the list, but finally it was considered not necessary\nbecause a shorter message is used [1].\n\nHowever, even with a short message, feeding that LF makes out next\n\"error: There was a problem with the...\" more clear. For example, the\neditor could print messages that would be mixed with our error message.\nSo, add that LF.\n\nWhile we're here, make the error message follow our CodingGuideLines.\n\n [1] https://public-inbox.org/git/20171127134716.69471-1-lars.schneider@autodesk.com/T/#u\n\nSigned-off-by: Rubén Justo \n---\n editor.c | 28 +++++++++++++++++++---------\n 1 file changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex b67b802ddf..0d4c6f94a6 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -104,16 +104,26 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_pop(SIGQUIT);\n \t\tif (sig == SIGINT || sig == SIGQUIT)\n \t\t\traise(sig);\n-\t\tif (ret)\n-\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n-\t\t\t\t\teditor);\n \n-\t\tif (print_waiting_for_editor && !is_terminal_dumb())\n-\t\t\t/*\n-\t\t\t * Erase the entire line to avoid wasting the\n-\t\t\t * vertical space.\n-\t\t\t */\n-\t\t\tterm_clear_line();\n+\t\tif (print_waiting_for_editor && !is_terminal_dumb()) {\n+\t\t\tif (!ret)\n+\t\t\t\t/*\n+\t\t\t \t * Erase the entire line to avoid wasting\n+\t\t\t \t * the vertical space.\n+\t\t\t \t */\n+\t\t\t\tterm_clear_line();\n+\t\t\telse\n+\t\t\t\t/*\n+\t\t\t\t * We don't want term_clear_line() here\n+\t\t\t\t * because the editor could have written\n+\t\t\t\t * some useful messages to the user.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"\\n\");\n+\t\t}\n+\n+\t\tif (ret) \n+\t\t\treturn error(\"there was a problem with the editor '%s'\",\n+\t\t\t\t\teditor);\n \t}\n \n \tif (!buffer)\n-- \n2.43.0\n"},{"id":"492592","messageId":"0258a583-a90a-4434-bb4e-a1672d574b9c@gmail.com","threadId":"61296","inReplyTo":"96bef5f9-1286-4938-99ec-6beed13ee68d@gmail.com","subject":"[PATCH v2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-08T23:09:21Z","receivedAt":"2024-04-08T23:09:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have the hint we're touching in this commit since abfb04d0c7\n(launch_editor(): indicate that Git waits for user input, 2017-12-07).\n\nAdding a new line after the hint when the editor returns error was\ndiscussed in the list, but finally it was considered not necessary\nbecause a shorter message is used [1].\n\nHowever, even with a short message, feeding that LF makes the following\n\"error: There was a problem with the...\" clearer, separating it from\npossible messages that the editor could have printed. So, add that LF.\n\nWhile we're here, make the error message follow our CodingGuideLines.\n\n [1] https://public-inbox.org/git/20171127134716.69471-1-lars.schneider@autodesk.com/T/#u\n\nSigned-off-by: Rubén Justo \n---\n\nThis v2 fixes some whitespaces I didn't notice.\n\nSorry for the mess.\n\n editor.c | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex b67b802ddf..8f224747d9 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -104,16 +104,26 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_pop(SIGQUIT);\n \t\tif (sig == SIGINT || sig == SIGQUIT)\n \t\t\traise(sig);\n+\n+\t\tif (print_waiting_for_editor && !is_terminal_dumb()) {\n+\t\t\tif (!ret)\n+\t\t\t\t/*\n+\t\t\t\t * Erase the entire line to avoid wasting\n+\t\t\t\t * the vertical space.\n+\t\t\t\t */\n+\t\t\t\tterm_clear_line();\n+\t\t\telse\n+\t\t\t\t/*\n+\t\t\t\t * We don't want term_clear_line() here\n+\t\t\t\t * because the editor could have written\n+\t\t\t\t * some useful messages to the user.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"\\n\");\n+\t\t}\n+\n \t\tif (ret)\n-\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n+\t\t\treturn error(\"there was a problem with the editor '%s'\",\n \t\t\t\t\teditor);\n-\n-\t\tif (print_waiting_for_editor && !is_terminal_dumb())\n-\t\t\t/*\n-\t\t\t * Erase the entire line to avoid wasting the\n-\t\t\t * vertical space.\n-\t\t\t */\n-\t\t\tterm_clear_line();\n \t}\n \n \tif (!buffer)\n-- \n2.44.0.502.g3e6838d230\n"},{"id":"492604","messageId":"xmqq4jcb495u.fsf@gitster.g","threadId":"61296","inReplyTo":"0258a583-a90a-4434-bb4e-a1672d574b9c@gmail.com","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-09T01:27:57Z","receivedAt":"2024-04-09T01:28:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> However, even with a short message, feeding that LF makes the following\n> \"error: There was a problem with the...\" clearer, separating it from\n> possible messages that the editor could have printed. So, add that LF.\n\nSounds sensible.\n\n> +\t\tif (print_waiting_for_editor && !is_terminal_dumb()) {\n> +\t\t\tif (!ret)\n> +\t\t\t\t/*\n> +\t\t\t\t * Erase the entire line to avoid wasting\n> +\t\t\t\t * the vertical space.\n> +\t\t\t\t */\n> +\t\t\t\tterm_clear_line();\n\nI know this was inherited from the original, but overly verbose\ncomment is not being very useful here.\n\n> +\t\t\telse\n> +\t\t\t\t/*\n> +\t\t\t\t * We don't want term_clear_line() here\n> +\t\t\t\t * because the editor could have written\n> +\t\t\t\t * some useful messages to the user.\n> +\t\t\t\t */\n> +\t\t\t\tfprintf(stderr, \"\\n\");\n\nBut I do not think this is emitting the newline at the right place.\nThe sequence would be (1) we say \"we are waiting\" on an incomplete\nline, and then (2) the editor may say \"There was a problem\" without\nfirst adding LF _before_ saying so. Isn't adding a LF here too late\nto let the editor emit its message on its own line, instead of\nhaving it _after_ the short \"hint\" message?\n\nOf course, after these two messages (one from us, and then the\nerror message from the editor) concatenated on the same line, we\nwould want to have the next error message on its own line, but\ndo we need to add an extra newline here for that purpose? Unlike\nour \"hint: we are waiting\" that we fully intend to clean-up by\nusing term_clear_line(), the editor that exits upon failure has no\nreason to keep its final error message \"There was a problem\" on an\nincomplete line without emitting the terminating LF before giving\ncontrol back to us.\n\nThe \"I do not know if it is bad enough to have these two on the same\nline\" you seem to refer to indirectly by citing Lars's message\n<20171127134716.69471-1-lars.schneider@autodesk.com> is my\n<20171127134716.69471-1-lars.schneider@autodesk.com>, I think. But\nin that utterance, \"these two\" refers to \"hint: we are waiting...\"\nand whatever the message the editor emits upon seeing an error. The\nsuggestion I made 7 years ago has nothing to do with the behaviour\nchange this patch is making.\n\nI think the code is doing the right thing. It is doing something\ndifferent from what the proposed commit log message said it is\ndoing. Let me try to summarize what I think this patch does:\n\n\tWhen advice.waitingForEditor configuration is not set to\n\tfalse, we show a hint telling that we are waiting for user's\n\teditor to close the file when we launch an editor and wait\n\tfor it to return control back to us. We give the message on\n\tan incomplete line, expecting that we can go back to the\n\tline and clear the message when the editor returns\n\tsuccessfully.\n\n\tHowever, it is possible that the editor exits with an error\n\tstatus, in which case we show an error message and then\n\treturn to our caller. In such a case, the error message is\n\tgiven where the terminal cursor happens to be, which is most\n\tlikely after the \"we are waiting for your editor\" message on\n\tthe same line.\n\n\tOnly clear the line when the editor returned cleanly, and\n\totherwise, complete the message on the incomplete line with\n\ta newline before giving the error message.\n\nHopefully the above is a more reasonable explanation of what is\nhappening in this patch, I think?\n\nActually, having thought it through in order to write the above\nexplanation, I wonder if we can just call term_clear_line()\nregardless of the value of ret. Either case, the waiting is already\nover and in the error case, we show another message after it.\n\nThere is another error message when we fail to start the editor.\nDoesn't that codepath have the same problem?\n\nI wonder:\n\n - moving the code to show \"hint\" down below start_command() where\n it could return error(\"unable to start\");\n\n - moving the \"if (ret) return error(\"There was a problem\")\" after\n the block that calls term_clear_line();\n\nwould be a better and sufficient fix?\n\nThanks.\n"},{"id":"492668","messageId":"54dcd4f1-5a79-47ba-9650-7115b7170a84@gmail.com","threadId":"61296","inReplyTo":"xmqq4jcb495u.fsf@gitster.g","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-09T23:38:10Z","receivedAt":"2024-04-09T23:38:19Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Apr 08, 2024 at 06:27:57PM -0700, Junio C Hamano wrote:\n\n> I wonder if we can just call term_clear_line()\n> regardless of the value of ret. Either case, the waiting is already\n> over and in the error case, we show another message after it.\n\nMy concern is that perhaps term_clear_line() might clear some useful\ninformation for the user. Although I am not sure that this concern is\nsensible.\n\nStepping back a bit, how painful it would be to drop the\nterm_clear_line() and start using advice_if_enabled() here?\n\nThis is what I'm thinking about now.\n\n\t$ GIT_EDITOR=false git commit -a\n\thint: A-good-explanation-to-say-we-run-'editor'\n\thint: Disable this message with \"git config advice.waitingForEditor false\"\n\terror: There was a problem with the editor 'false'.\n\tPlease supply the message using either -m or -F option.\n\n> There is another error message when we fail to start the editor.\n> Doesn't that codepath have the same problem?\n\nOf course.\n\nMy itch is:\n\n\t$ GIT_EDITOR=false git commit -a\n\thint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'.\n\tPlease supply the message using either -m or -F option.\n\nBut, yes, while we're here we can also fix:\n\n\t$ GIT_EDITOR=falso git commit -a\n\thint: Waiting for your editor to close the file... error: cannot run falso: No such file or directory\n\terror: unable to start editor 'falso'\n\tPlease supply the message using either -m or -F option.\n"},{"id":"492719","messageId":"xmqqle5lxlwm.fsf@gitster.g","threadId":"61296","inReplyTo":"54dcd4f1-5a79-47ba-9650-7115b7170a84@gmail.com","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-10T15:44:25Z","receivedAt":"2024-04-10T15:44:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> On Mon, Apr 08, 2024 at 06:27:57PM -0700, Junio C Hamano wrote:\n>\n>> I wonder if we can just call term_clear_line()\n>> regardless of the value of ret. Either case, the waiting is already\n>> over and in the error case, we show another message after it.\n>\n> My concern is that perhaps term_clear_line() might clear some useful\n> information for the user. Although I am not sure that this concern is\n> sensible.\n\nIt happens ONLY when the error message the editor itself emits\n(which comes later on the same line as \"We are waiting for your\neditor...\") without terminating newline itself. Otherwise, we'd\nhave\n\n We are waiting ... THE EDITOR SAYS I FAILED\n _\n\non the screen, and the cursor is at the _ position. term_clear_line()\nwould not clear anything.\n\n> Stepping back a bit, how painful it would be to drop the\n> term_clear_line() and start using advice_if_enabled() here?\n>\n> This is what I'm thinking about now.\n>\n> \t$ GIT_EDITOR=false git commit -a\n> \thint: A-good-explanation-to-say-we-run-'editor'\n> \thint: Disable this message with \"git config advice.waitingForEditor false\"\n> \terror: There was a problem with the editor 'false'.\n> \tPlease supply the message using either -m or -F option.\n\nI do not think the problem is in the case where the editor\nimmediately exits with an error. It is when the editor opens\nelsewhere (or more likely, opens another tab to let you edit in an\nexisting GUI editor session, but the editor's window is buried under\nother windows) and your \"git commit\" will stay silently without\ngiving you back a terminal prompt, leaving you wondering why it is\ntaking so much time.\n\nSo I am not sure if the advice mechanism is a good fit.\n\n>> There is another error message when we fail to start the editor.\n>> Doesn't that codepath have the same problem?\n>\n> Of course.\n>\n> My itch is:\n>\n> \t$ GIT_EDITOR=false git commit -a\n> \thint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'.\n> \tPlease supply the message using either -m or -F option.\n\nI do not think we want to encourage \"-m\" when the end user did not\nsay so. Instead we should let them fix their editor to keep them\nmore productive.\n\nThanks.\n"},{"id":"492814","messageId":"d485c4cd-f963-45b7-9fa6-801738c7c066@gmail.com","threadId":"61296","inReplyTo":"xmqqle5lxlwm.fsf@gitster.g","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-11T23:18:55Z","receivedAt":"2024-04-11T23:18:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Apr 10, 2024 at 08:44:25AM -0700, Junio C Hamano wrote:\n\n> > My concern is that perhaps term_clear_line() might clear some useful\n> > information for the user. Although I am not sure that this concern is\n> > sensible.\n> \n> It happens ONLY when the error message the editor itself emits\n> (which comes later on the same line as \"We are waiting for your\n> editor...\") without terminating newline itself. Otherwise, we'd\n> have\n> \n> We are waiting ... THE EDITOR SAYS I FAILED\n> _\n> \n> on the screen, and the cursor is at the _ position. term_clear_line()\n> would not clear anything.\n\nNot with a careless editor:\n\n--- >8 ---\n#include \n#include \n\nint main(void)\n{\n\tfprintf(\"editing the file...\");\n\texit(1); /* unexpected exit */\n\tfprintf(\"\\n\");\n\treturn 0;\n}\n--- 8< ---\n\n> > Stepping back a bit, how painful it would be to drop the\n> > term_clear_line() and start using advice_if_enabled() here?\n> >\n> > This is what I'm thinking about now.\n> >\n> > \t$ GIT_EDITOR=false git commit -a\n> > \thint: A-good-explanation-to-say-we-run-'editor'\n> > \thint: Disable this message with \"git config advice.waitingForEditor false\"\n> > \terror: There was a problem with the editor 'false'.\n> > \tPlease supply the message using either -m or -F option.\n> \n> I do not think the problem is in the case where the editor\n> immediately exits with an error. It is when the editor opens\n> elsewhere (or more likely, opens another tab to let you edit in an\n> existing GUI editor session, but the editor's window is buried under\n> other windows) and your \"git commit\" will stay silently without\n> giving you back a terminal prompt, leaving you wondering why it is\n> taking so much time.\n\nYes ...\n\n>\n> So I am not sure if the advice mechanism is a good fit.\n\nand I'm not sure either.\n\nHowever, two things that I like: by using the advice mechanism we avoid\nthe term_clear_line() and we advertise better the knob. I discovered\nadvice.waitingForEditor while inspecting the code.\n\nUsing the advice mechanism here is just a thought.\n\n> > \t$ GIT_EDITOR=false git commit -a\n> > \thint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'.\n> > \tPlease supply the message using either -m or -F option.\n\n> I do not think we want to encourage \"-m\" when the end user did not\n> say so. Instead we should let them fix their editor to keep them\n> more productive.\n\nThat message can be traced back to 62e09ce998 (Make git tag a builtin.,\n2007-07-20). My understanding is that we suggest -m or -F because using\nthe editor failed.\n\nAre you saying we should stop giving that \"Please supply ...\"?\n\nI see that message when I'm editing a commit message and decide to abort\nthe commit by making the editor end with an error. So, that message is\nmisleading to me.\n\nTherefore I'm fine with removing that message, however, I'm not sure\nthat's what you're suggesting.\n\nAt any rate, this is tangential to this series.\n"},{"id":"492873","messageId":"xmqqwmp2posk.fsf@gitster.g","threadId":"61296","inReplyTo":"d485c4cd-f963-45b7-9fa6-801738c7c066@gmail.com","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-12T15:46:03Z","receivedAt":"2024-04-12T15:46:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> On Wed, Apr 10, 2024 at 08:44:25AM -0700, Junio C Hamano wrote:\n>...\n>> It happens ONLY when the error message the editor itself emits\n>> (which comes later on the same line as \"We are waiting for your\n>> editor...\") without terminating newline itself. Otherwise, we'd\n>> have\n>> \n>> We are waiting ... THE EDITOR SAYS I FAILED\n>> _\n>> \n>> on the screen, and the cursor is at the _ position. term_clear_line()\n>> would not clear anything.\n>\n> Not with a careless editor:\n\nThat is why I said \"Otherwise\". Of course, a broken editor would\ngive broken output. What else is new? And more importantly, if you\nwrote such an editor, would you release it in such a buggy form to\nthe outside world? Does it still look like a problem worth spending\nour brain cycles on?\n\n"},{"id":"492878","messageId":"40075168-24d8-4c46-89fb-30fe2ab2581b@gmail.com","threadId":"61296","inReplyTo":"xmqqwmp2posk.fsf@gitster.g","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-12T17:03:24Z","receivedAt":"2024-04-12T17:03:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Apr 12, 2024 at 08:46:03AM -0700, Junio C Hamano wrote:\n\n> >> It happens ONLY when the error message the editor itself emits\n> >> (which comes later on the same line as \"We are waiting for your\n> >> editor...\") without terminating newline itself. Otherwise, we'd\n> >> have\n> >> \n> >> We are waiting ... THE EDITOR SAYS I FAILED\n> >> _\n> >> \n> >> on the screen, and the cursor is at the _ position. term_clear_line()\n> >> would not clear anything.\n> >\n> > Not with a careless editor:\n> \n> That is why I said \"Otherwise\". Of course, a broken editor would\n> give broken output. What else is new? And more importantly, if you\n> wrote such an editor, would you release it in such a buggy form to\n> the outside world? Does it still look like a problem worth spending\n> our brain cycles on?\n> \n\nYes, but I also see it from another perspective; I don't want to worry\nabout a possible inconvenience. And since it is perhaps an unexpected\nprecaution, for a future reviewer, hence the explicit comment in the\ncode.\n"},{"id":"492879","messageId":"ccbd77a1-d334-4d8f-8de0-b542c79330fd@gmail.com","threadId":"61296","inReplyTo":"0258a583-a90a-4434-bb4e-a1672d574b9c@gmail.com","subject":"[PATCH v3 0/2] launch_editor: waiting message","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-12T17:05:41Z","receivedAt":"2024-04-12T17:05:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Improve the hint message we have in editor.c.\n\nRubén Justo (2):\n launch_editor: waiting for editor message\n launch_editor: waiting message on error\n\n editor.c | 54 ++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 32 insertions(+), 22 deletions(-)\n\n-- \n2.44.0.771.gbd07cf668b\n"},{"id":"492881","messageId":"83b34572-498b-438c-8437-dfbb837e60ba@gmail.com","threadId":"61296","inReplyTo":"ccbd77a1-d334-4d8f-8de0-b542c79330fd@gmail.com","subject":"[PATCH v3 1/2] launch_editor: waiting for editor message","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-12T17:15:02Z","receivedAt":"2024-04-12T17:15:05Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have a hint shown when we are waiting for user's editor since\nabfb04d0c7 (launch_editor(): indicate that Git waits for user input,\n2017-12-07).\n\nAfter showing the hint, we call start_command() which can return with an\nerror. Then we'll show \"unable to start editor...\", after having said\n\"Waiting for your editor...\", which may be confusing.\n\nMove the code to show the hint below the start_command().\n\nSigned-off-by: Rubén Justo \n---\n editor.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex b67b802ddf..1da3a26f5d 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -64,9 +64,21 @@ static int launch_specified_editor(const char *editor, const char *path,\n \tif (strcmp(editor, \":\")) {\n \t\tstruct strbuf realpath = STRBUF_INIT;\n \t\tstruct child_process p = CHILD_PROCESS_INIT;\n-\t\tint ret, sig;\n-\t\tint print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);\n+\t\tint ret, sig, print_waiting_for_editor;\n \n+\t\tstrbuf_realpath(&realpath, path, 1);\n+\n+\t\tstrvec_pushl(&p.args, editor, realpath.buf, NULL);\n+\t\tif (env)\n+\t\t\tstrvec_pushv(&p.env, (const char **)env);\n+\t\tp.use_shell = 1;\n+\t\tp.trace2_child_class = \"editor\";\n+\t\tif (start_command(&p) < 0) {\n+\t\t\tstrbuf_release(&realpath);\n+\t\t\treturn error(\"unable to start editor '%s'\", editor);\n+\t\t}\n+\n+\t\tprint_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);\n \t\tif (print_waiting_for_editor) {\n \t\t\t/*\n \t\t\t * A dumb terminal cannot erase the line later on. Add a\n@@ -83,18 +95,6 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\t\tfflush(stderr);\n \t\t}\n \n-\t\tstrbuf_realpath(&realpath, path, 1);\n-\n-\t\tstrvec_pushl(&p.args, editor, realpath.buf, NULL);\n-\t\tif (env)\n-\t\t\tstrvec_pushv(&p.env, (const char **)env);\n-\t\tp.use_shell = 1;\n-\t\tp.trace2_child_class = \"editor\";\n-\t\tif (start_command(&p) < 0) {\n-\t\t\tstrbuf_release(&realpath);\n-\t\t\treturn error(\"unable to start editor '%s'\", editor);\n-\t\t}\n-\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n-- \n2.44.0.771.gbd07cf668b\n"},{"id":"492882","messageId":"c430a194-32ac-403c-a381-801556275f66@gmail.com","threadId":"61296","inReplyTo":"ccbd77a1-d334-4d8f-8de0-b542c79330fd@gmail.com","subject":"[PATCH v3 2/2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-12T17:15:08Z","receivedAt":"2024-04-12T17:15:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When advice.waitingForEditor configuration is not set to false, we show\na hint telling that we are waiting for user's editor to close the file\nwhen we launch an editor and wait for it to return control back to us.\nWe give the message on an incomplete line, expecting that we can go back\nto the line and clear the message when the editor returns successfully.\n\nHowever, it is possible that the editor exits with an error status, in\nwhich case we show an error message and then return to our caller. In\nsuch a case, the error message is given where the terminal cursor\nhappens to be, which is most likely after the \"we are waiting for your\neditor\" message on the same line.\n\nOnly clear the line when the editor returned cleanly, and otherwise,\ncomplete the message on the incomplete line with a newline before giving\nthe error message.\n\nWhile we're here, make the error message follow our CodingGuideLines.\n\nSigned-off-by: Rubén Justo \n---\n editor.c | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex 1da3a26f5d..eb0cfe4a28 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -104,16 +104,26 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_pop(SIGQUIT);\n \t\tif (sig == SIGINT || sig == SIGQUIT)\n \t\t\traise(sig);\n+\n+\t\tif (print_waiting_for_editor && !is_terminal_dumb()) {\n+\t\t\tif (!ret)\n+\t\t\t\t/*\n+\t\t\t\t * Erase the entire line to avoid wasting\n+\t\t\t\t * the vertical space.\n+\t\t\t\t */\n+\t\t\t\tterm_clear_line();\n+\t\t\telse\n+\t\t\t\t/*\n+\t\t\t\t * We don't want term_clear_line() here\n+\t\t\t\t * because the editor could have written\n+\t\t\t\t * some useful messages to the user.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"\\n\");\n+\t\t}\n+\n \t\tif (ret)\n-\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n+\t\t\treturn error(\"there was a problem with the editor '%s'\",\n \t\t\t\t\teditor);\n-\n-\t\tif (print_waiting_for_editor && !is_terminal_dumb())\n-\t\t\t/*\n-\t\t\t * Erase the entire line to avoid wasting the\n-\t\t\t * vertical space.\n-\t\t\t */\n-\t\t\tterm_clear_line();\n \t}\n \n \tif (!buffer)\n-- \n2.44.0.771.gbd07cf668b\n"},{"id":"492885","messageId":"02b701da8cfe$59be3370$0d3a9a50$@nexbridge.com","threadId":"61296","inReplyTo":"83b34572-498b-438c-8437-dfbb837e60ba@gmail.com","subject":"RE: [PATCH v3 1/2] launch_editor: waiting for editor message","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-12T17:24:55Z","receivedAt":"2024-04-12T17:25:06Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote:\n>Subject: [PATCH v3 1/2] launch_editor: waiting for editor message\n>\n>We have a hint shown when we are waiting for user's editor since\n>abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07).\n>\n>After showing the hint, we call start_command() which can return with an error.\n>Then we'll show \"unable to start editor...\", after having said \"Waiting for your\n>editor...\", which may be confusing.\n>\n>Move the code to show the hint below the start_command().\n\nMy thought on this move is for esoteric (but commonly used) terminal emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for example, the emulator switches modes from text on the POSIX side to block/full screen mode when the editor is launched. Printing a message after the editor has launched has the potential to dump the message into the terminal emulation buffer and get caught in the commit text comment. This is not desirable. This change could have seriously undesirable side-effects.\n\nOn the other side, if the message is not displayed in the emulation buffer, it is deferred until after the editor closes, which makes the message a bit pointless.\n--Randall\n\n\n"},{"id":"492887","messageId":"xmqqbk6emql1.fsf@gitster.g","threadId":"61296","inReplyTo":"40075168-24d8-4c46-89fb-30fe2ab2581b@gmail.com","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-12T17:35:38Z","receivedAt":"2024-04-12T17:35:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Yes, but I also see it from another perspective; I don't want to worry\n> about a possible inconvenience. And since it is perhaps an unexpected\n> precaution, for a future reviewer, hence the explicit comment in the\n> code.\n\nBut then the comment should say it only matters if the editor left\nits message incomplete, shouldn't it? If the editor did the right\nthing and terminated its message before it exits with a newline, the\nextra LF we emit after it will only waste the vertical screen real\nestate.\n"},{"id":"492888","messageId":"f473894d-caf9-4a50-962e-dc884f29e174@gmail.com","threadId":"61296","inReplyTo":"02b701da8cfe$59be3370$0d3a9a50$@nexbridge.com","subject":"Re: [PATCH v3 1/2] launch_editor: waiting for editor message","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-12T17:37:16Z","receivedAt":"2024-04-12T17:37:19Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Apr 12, 2024 at 01:24:55PM -0400, rsbecker@nexbridge.com wrote:\n> On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote:\n> >Subject: [PATCH v3 1/2] launch_editor: waiting for editor message\n> >\n> >We have a hint shown when we are waiting for user's editor since\n> >abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07).\n> >\n> >After showing the hint, we call start_command() which can return with an error.\n> >Then we'll show \"unable to start editor...\", after having said \"Waiting for your\n> >editor...\", which may be confusing.\n> >\n> >Move the code to show the hint below the start_command().\n> \n> My thought on this move is for esoteric (but commonly used) terminal\n> emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for\n> example, the emulator switches modes from text on the POSIX side to\n> block/full screen mode when the editor is launched. Printing a message\n> after the editor has launched has the potential to dump the message\n> into the terminal emulation buffer and get caught in the commit text\n> comment. This is not desirable. This change could have seriously\n> undesirable side-effects.\n\nThat's a good point. Thanks for bringing it up.\n\nOf course, in such a situation the user has the opportunity to disable\nthe hint.\n\nHowever, can you think of a way in which we could do this, not showing\nthe \"Waiting...\" before the \"unable to start\", better?\n\nThanks.\n"},{"id":"492890","messageId":"02ba01da8d01$78bd5a50$6a380ef0$@nexbridge.com","threadId":"61296","inReplyTo":"f473894d-caf9-4a50-962e-dc884f29e174@gmail.com","subject":"RE: [PATCH v3 1/2] launch_editor: waiting for editor message","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-12T17:47:16Z","receivedAt":"2024-04-12T17:47:24Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Friday, April 12, 2024 1:37 PM, Rubén Justo wrote:\n>On Fri, Apr 12, 2024 at 01:24:55PM -0400, rsbecker@nexbridge.com wrote:\n>> On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote:\n>> >Subject: [PATCH v3 1/2] launch_editor: waiting for editor message\n>> >\n>> >We have a hint shown when we are waiting for user's editor since\n>> >abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07).\n>> >\n>> >After showing the hint, we call start_command() which can return with an error.\n>> >Then we'll show \"unable to start editor...\", after having said\n>> >\"Waiting for your editor...\", which may be confusing.\n>> >\n>> >Move the code to show the hint below the start_command().\n>>\n>> My thought on this move is for esoteric (but commonly used) terminal\n>> emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for\n>> example, the emulator switches modes from text on the POSIX side to\n>> block/full screen mode when the editor is launched. Printing a message\n>> after the editor has launched has the potential to dump the message\n>> into the terminal emulation buffer and get caught in the commit text\n>> comment. This is not desirable. This change could have seriously\n>> undesirable side-effects.\n>\n>That's a good point. Thanks for bringing it up.\n>\n>Of course, in such a situation the user has the opportunity to disable the hint.\n>\n>However, can you think of a way in which we could do this, not showing the\n>\"Waiting...\" before the \"unable to start\", better?\n\nI do not have a good solution. One thought was to run the Waiting message in a separate thread, but that is dangerous. Terminal I/O APIs are generally not thread aware and random results are frequent when writing from two threads, particularly in different processes. Polluting stdout is never a good idea, and in this case the encoding and terminal type could also change between git and editor, even in Linux. The only potential way to do this is with an editor aware mutex (there isn't a portable on) that would block the editor, poll the terminal state, change to UTF-8 or US-ASCII or... and write the Waiting message, switch back, then release the mutex.\n\n"},{"id":"492892","messageId":"701c23e2-ed88-4ad8-95b6-1ce73e671709@gmail.com","threadId":"61296","inReplyTo":"xmqqbk6emql1.fsf@gitster.g","subject":"Re: [PATCH v2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-12T18:24:33Z","receivedAt":"2024-04-12T18:24:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Apr 12, 2024 at 10:35:38AM -0700, Junio C Hamano wrote:\n\n> > Yes, but I also see it from another perspective; I don't want to worry\n> > about a possible inconvenience. And since it is perhaps an unexpected\n> > precaution, for a future reviewer, hence the explicit comment in the\n> > code.\n> \n> But then the comment should say it only matters if the editor left\n> its message incomplete, shouldn't it? If the editor did the right\n> thing and terminated its message before it exits with a newline, the\n> extra LF we emit after it will only waste the vertical screen real\n> estate.\n\nNot sure if that needs to be noted in the comment.\n\nThis, and the other point raised by Randall [1], certainly makes me more\nin favor of using the advise_if_enabled().\n\nInstead of \"Waiting...\", using a message such as \"Started...\" can be\njust as good for user guidance and less prone to error.\n\nI think the v3 I posted is an improvement. But I believe we should\nconsider moving towards using the advise API here, at some point. \n\n [1] https://lore.kernel.org/git/96bef5f9-1286-4938-99ec-6beed13ee68d@gmail.com/T/#m906fca9d24baf343326e134ac08370a77d69a603\n"},{"id":"492905","messageId":"eff5b9b5-1efa-4a63-8a98-a5630a17fd94@gmail.com","threadId":"61296","inReplyTo":"02b701da8cfe$59be3370$0d3a9a50$@nexbridge.com","subject":"Re: [PATCH v3 1/2] launch_editor: waiting for editor message","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-13T15:06:35Z","receivedAt":"2024-04-13T15:06:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/04/2024 18:24, rsbecker@nexbridge.com wrote:\n> On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote:\n>> Subject: [PATCH v3 1/2] launch_editor: waiting for editor message\n>>\n>> We have a hint shown when we are waiting for user's editor since\n>> abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07).\n>>\n>> After showing the hint, we call start_command() which can return with an error.\n>> Then we'll show \"unable to start editor...\", after having said \"Waiting for your\n>> editor...\", which may be confusing.\n>>\n>> Move the code to show the hint below the start_command().\n> \n> My thought on this move is for esoteric (but commonly used) terminal emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for example, the emulator switches modes from text on the POSIX side to block/full screen mode when the editor is launched. Printing a message after the editor has launched has the potential to dump the message into the terminal emulation buffer and get caught in the commit text comment. This is not desirable. This change could have seriously undesirable side-effects.\n\nWriting to the terminal after starting the editor is a bad idea for the \nreason you describe regardless of the terminal type. I don't think there \nis a sensible way to avoid showing the hint before we know whether the \neditor started successfully or not.\n\n> On the other side, if the message is not displayed in the emulation buffer, it is deferred until after the editor closes, which makes the message a bit pointless.\n\nI think the message is there for gui editors, with a terminal editor it \nis obvious that git has started the editor and the user doesn't really \nsee the message because it is cleared when the editor exits successfully.\n\nBest Wishes\n\nPhillip\n\n> --Randall\n> \n> \n> \n"},{"id":"492906","messageId":"782c7127-45d7-4bd6-930f-941a6f43088d@gmail.com","threadId":"61296","inReplyTo":"c430a194-32ac-403c-a381-801556275f66@gmail.com","subject":"Re: [PATCH v3 2/2] launch_editor: waiting message on error","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-13T15:09:27Z","receivedAt":"2024-04-13T15:09:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 12/04/2024 18:15, Rubén Justo wrote:\n> When advice.waitingForEditor configuration is not set to false, we show\n> a hint telling that we are waiting for user's editor to close the file\n> when we launch an editor and wait for it to return control back to us.\n> We give the message on an incomplete line, expecting that we can go back\n> to the line and clear the message when the editor returns successfully.\n> \n> However, it is possible that the editor exits with an error status, in\n> which case we show an error message and then return to our caller. In\n> such a case, the error message is given where the terminal cursor\n> happens to be, which is most likely after the \"we are waiting for your\n> editor\" message on the same line.\n\nI think it is very likely that the editor has printed an error message \nif it exits with a non-zero exit code and if that message does not end \nwith a newline that is a bug in the editor. Do you have a real-world \nexample of the problem you are seeking to fix?\n\nBest Wishes\n\nPhillip\n\n> Only clear the line when the editor returned cleanly, and otherwise,\n> complete the message on the incomplete line with a newline before giving\n> the error message.\n> \n> While we're here, make the error message follow our CodingGuideLines.\n> \n> Signed-off-by: Rubén Justo \n> ---\n> editor.c | 26 ++++++++++++++++++--------\n> 1 file changed, 18 insertions(+), 8 deletions(-)\n> \n> diff --git a/editor.c b/editor.c\n> index 1da3a26f5d..eb0cfe4a28 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -104,16 +104,26 @@ static int launch_specified_editor(const char *editor, const char *path,\n> \t\tsigchain_pop(SIGQUIT);\n> \t\tif (sig == SIGINT || sig == SIGQUIT)\n> \t\t\traise(sig);\n> +\n> +\t\tif (print_waiting_for_editor && !is_terminal_dumb()) {\n> +\t\t\tif (!ret)\n> +\t\t\t\t/*\n> +\t\t\t\t * Erase the entire line to avoid wasting\n> +\t\t\t\t * the vertical space.\n> +\t\t\t\t */\n> +\t\t\t\tterm_clear_line();\n> +\t\t\telse\n> +\t\t\t\t/*\n> +\t\t\t\t * We don't want term_clear_line() here\n> +\t\t\t\t * because the editor could have written\n> +\t\t\t\t * some useful messages to the user.\n> +\t\t\t\t */\n> +\t\t\t\tfprintf(stderr, \"\\n\");\n> +\t\t}\n> +\n> \t\tif (ret)\n> -\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n> +\t\t\treturn error(\"there was a problem with the editor '%s'\",\n> \t\t\t\t\teditor);\n> -\n> -\t\tif (print_waiting_for_editor && !is_terminal_dumb())\n> -\t\t\t/*\n> -\t\t\t * Erase the entire line to avoid wasting the\n> -\t\t\t * vertical space.\n> -\t\t\t */\n> -\t\t\tterm_clear_line();\n> \t}\n> \n> \tif (!buffer)\n"},{"id":"492920","messageId":"66b3192a-e0db-4df9-bcec-066adaa3e083@gmail.com","threadId":"61296","inReplyTo":"782c7127-45d7-4bd6-930f-941a6f43088d@gmail.com","subject":"Re: [PATCH v3 2/2] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-14T07:23:49Z","receivedAt":"2024-04-14T07:23:56Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Apr 13, 2024 at 04:09:27PM +0100, Phillip Wood wrote:\n\n> I think it is very likely that the editor has printed an error message if it\n> exits with a non-zero exit code and if that message does not end with a\n> newline that is a bug in the editor. Do you have a real-world example of the\n> problem you are seeking to fix?\n\nPerhaps I am being too cautious. I'll follow Junio's suggestion and use \nterm_clear_line() in all cases.\n\nThanks.\n"},{"id":"492921","messageId":"e208da74-8f16-44ae-912e-ae968da82057@gmail.com","threadId":"61296","inReplyTo":"ccbd77a1-d334-4d8f-8de0-b542c79330fd@gmail.com","subject":"[PATCH v4] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-14T07:39:44Z","receivedAt":"2024-04-14T07:39:48Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When advice.waitingForEditor configuration is not set to false, we show\na hint telling that we are waiting for user's editor to close the file\nwhen we launch an editor and wait for it to return control back to us.\nWe give the message on an incomplete line, expecting that we can go back\nto the line and clear the message when the editor returns.\n\nHowever, it is possible that the editor exits with an error status, in\nwhich case we show an error message and then return to our caller. In\nsuch a case, the error message is given where the terminal cursor\nhappens to be, which is most likely after the \"we are waiting for your\neditor\" message on the same line.\n\nClear the line before showing the error.\n\nWhile we're here, make the error message follow our CodingGuideLines.\n\nSigned-off-by: Rubén Justo \n---\n editor.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex b67b802ddf..d1ba2d7c34 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -104,16 +104,15 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_pop(SIGQUIT);\n \t\tif (sig == SIGINT || sig == SIGQUIT)\n \t\t\traise(sig);\n-\t\tif (ret)\n-\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n-\t\t\t\t\teditor);\n-\n \t\tif (print_waiting_for_editor && !is_terminal_dumb())\n \t\t\t/*\n \t\t\t * Erase the entire line to avoid wasting the\n \t\t\t * vertical space.\n \t\t\t */\n \t\t\tterm_clear_line();\n+\t\tif (ret)\n+\t\t\treturn error(\"there was a problem with the editor '%s'\",\n+\t\t\t\t\teditor);\n \t}\n \n \tif (!buffer)\n-- \n2.44.0.782.g038d277106\n"},{"id":"492945","messageId":"beebf170-31d0-4ecf-b029-d6bc499f6da8@gmail.com","threadId":"61296","inReplyTo":"e208da74-8f16-44ae-912e-ae968da82057@gmail.com","subject":"Re: [PATCH v4] launch_editor: waiting message on error","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-15T14:05:32Z","receivedAt":"2024-04-15T14:05:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 14/04/2024 08:39, Rubén Justo wrote:\n> When advice.waitingForEditor configuration is not set to false, we show\n> a hint telling that we are waiting for user's editor to close the file\n> when we launch an editor and wait for it to return control back to us.\n> We give the message on an incomplete line, expecting that we can go back\n> to the line and clear the message when the editor returns.\n> \n> However, it is possible that the editor exits with an error status, in\n> which case we show an error message and then return to our caller. In\n> such a case, the error message is given where the terminal cursor\n> happens to be, which is most likely after the \"we are waiting for your\n> editor\" message on the same line.\n\nAs I've said before I'm not sure how likely that is as I think the \neditor will probably have printed a message if there was an error. \nAssuming the message from the editor ends with a newline the proposed \nchange wont do any harm so I don't object to it.\n\nBest Wishes\n\nPhillip\n\n> Clear the line before showing the error.\n> \n> While we're here, make the error message follow our CodingGuideLines.\n> \n> Signed-off-by: Rubén Justo \n> ---\n> editor.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/editor.c b/editor.c\n> index b67b802ddf..d1ba2d7c34 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -104,16 +104,15 @@ static int launch_specified_editor(const char *editor, const char *path,\n> \t\tsigchain_pop(SIGQUIT);\n> \t\tif (sig == SIGINT || sig == SIGQUIT)\n> \t\t\traise(sig);\n> -\t\tif (ret)\n> -\t\t\treturn error(\"There was a problem with the editor '%s'.\",\n> -\t\t\t\t\teditor);\n> -\n> \t\tif (print_waiting_for_editor && !is_terminal_dumb())\n> \t\t\t/*\n> \t\t\t * Erase the entire line to avoid wasting the\n> \t\t\t * vertical space.\n> \t\t\t */\n> \t\t\tterm_clear_line();\n> +\t\tif (ret)\n> +\t\t\treturn error(\"there was a problem with the editor '%s'\",\n> +\t\t\t\t\teditor);\n> \t}\n> \n> \tif (!buffer)\n"},{"id":"492948","messageId":"c9f3b358-bce0-41dd-b717-e845f0b2dab3@gmail.com","threadId":"61296","inReplyTo":"beebf170-31d0-4ecf-b029-d6bc499f6da8@gmail.com","subject":"Re: [PATCH v4] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-15T17:03:35Z","receivedAt":"2024-04-15T17:03:39Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Apr 15, 2024 at 03:05:32PM +0100, Phillip Wood wrote:\n\n> > However, it is possible that the editor exits with an error status, in\n> > which case we show an error message and then return to our caller. In\n> > such a case, the error message is given where the terminal cursor\n> > happens to be, which is most likely after the \"we are waiting for your\n> > editor\" message on the same line.\n> \n> As I've said before I'm not sure how likely that is as I think the editor\n> will probably have printed a message if there was an error.\n\nFor instance, Vim with \":cq\" does not emit any error message.\n\n> Assuming the\n> message from the editor ends with a newline the proposed change wont do any\n> harm so I don't object to it.\n\nThanks.\n"},{"id":"492949","messageId":"8ef912a9-78f2-411a-a055-f86f69d78b90@gmail.com","threadId":"61296","inReplyTo":"e208da74-8f16-44ae-912e-ae968da82057@gmail.com","subject":"Re: [PATCH v4] launch_editor: waiting message on error","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-15T17:07:14Z","receivedAt":"2024-04-15T17:07:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sun, Apr 14, 2024 at 09:39:44AM +0200, Rubén Justo wrote:\n> When advice.waitingForEditor configuration is not set to false, we show\n> a hint telling that we are waiting for user's editor to close the file\n> when we launch an editor and wait for it to return control back to us.\n> We give the message on an incomplete line, expecting that we can go back\n> to the line and clear the message when the editor returns.\n> \n> However, it is possible that the editor exits with an error status, in\n> which case we show an error message and then return to our caller. In\n> such a case, the error message is given where the terminal cursor\n> happens to be, which is most likely after the \"we are waiting for your\n> editor\" message on the same line.\n> \n> Clear the line before showing the error.\n> \n> While we're here, make the error message follow our CodingGuideLines.\n> \n> Signed-off-by: Rubén Justo \n> ---\n\nThe changes since v3 are:\n\n- dropped [v3 1/2] because, as noted by Randall and Phillip, it is not a\n good idea.\n\n The message stays like:\n\n\t$ GIT_EDITOR=falso git commit -a\n\thint: Waiting for your editor to close the file... error: cannot run falso: No such file or directory\n\terror: unable to start editor 'falso'\n\tPlease supply the message using either -m or -F option.\n\n The \"error: unable to start...\" at the beginning of the line makes it\n less prone to confusion than the other error message considered in\n this series.\n\n- term_clear_line() is now used in all cases as it is unlikely that any\n sane editor emits an error message without ending it with a newline.\n\n This:\n\n\t$ GIT_EDITOR=false git commit -a\n\thint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'.\n\tPlease supply the message using either -m or -F option.\n\n becomes:\n\n\t$ GIT_EDITOR=false git commit -a\n\terror: There was a problem with the editor 'false'.\n\tPlease supply the message using either -m or -F option.\n\n"},{"id":"492953","messageId":"xmqqa5lupmo7.fsf@gitster.g","threadId":"61296","inReplyTo":"beebf170-31d0-4ecf-b029-d6bc499f6da8@gmail.com","subject":"Re: [PATCH v4] launch_editor: waiting message on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T17:20:56Z","receivedAt":"2024-04-15T17:21:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> As I've said before I'm not sure how likely that is as I think the\n> editor will probably have printed a message if there was an\n> error. Assuming the message from the editor ends with a newline the\n> proposed change wont do any harm so I don't object to it.\n\nYup, I think it is a no-op for a well-behaving editor that emits an\nerror message with terminating newline. An editor that leaves its\nown error message incomplete, its error message together with our\n\"we are waiting\" will be erased together, but we probably do not\ncare for such an editor. If an editor silently errors out, then\nthis will still clear \"we are waiting\" we printed earlier and say\n\"there was a problem\", which is what we want to see happen.\n\nSo, this looks good to me; let's queue it.\n\nThanks, both.\n"},{"id":"492959","messageId":"xmqqedb6mpns.fsf@gitster.g","threadId":"61296","inReplyTo":"8ef912a9-78f2-411a-a055-f86f69d78b90@gmail.com","subject":"Re: [PATCH v4] launch_editor: waiting message on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T18:44:39Z","receivedAt":"2024-04-15T18:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> - term_clear_line() is now used in all cases as it is unlikely that any\n> sane editor emits an error message without ending it with a newline.\n>\n> This:\n>\n> \t$ GIT_EDITOR=false git commit -a\n> \thint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'.\n> \tPlease supply the message using either -m or -F option.\n>\n> becomes:\n>\n> \t$ GIT_EDITOR=false git commit -a\n> \terror: There was a problem with the editor 'false'.\n> \tPlease supply the message using either -m or -F option.\n\nNobody uses 'false' as their editor, but as you said ':cq' in vim\nmay be a real-world example of a use case that may benefit from this\nchange.\n\nWill queue. Thanks.\n"}]}