{"thread":{"id":"47229","subject":"[PATCH v2] launch_editor(): indicate that Git waits for user input","startedAt":"2017-11-17T13:51:50Z","lastAt":"2017-11-24T06:38:46Z","messageCount":12,"participants":["lars.schneider@autodesk.com","Junio C Hamano","Eric Sunshine","Kaartic Sivaraam","Lars Schneider"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"332757","messageId":"20171117135109.18071-1-lars.schneider@autodesk.com","threadId":"47229","inReplyTo":null,"subject":"[PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2017-11-17T13:51:09Z","receivedAt":"2017-11-17T13:51:50Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Junio C Hamano \n\nWhen a graphical GIT_EDITOR is spawned by a Git command that opens\nand waits for user input (e.g. \"git rebase -i\"), then the editor window\nmight be obscured by other windows. The user may be left staring at the\noriginal Git terminal window without even realizing that s/he needs to\ninteract with another window before Git can proceed. To this user Git\nappears hanging.\n\nShow a message in the original terminal and get rid of it when the\neditor returns.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Lars Schneider \n---\n\nJunio posted the original version of this patch [1] as response to my RFC [2].\nI took Junio's patch and slightly changed the commit message as well as the\nmessage printed to the user after GIT_EDITOR is invoked [3].\n\nThanks,\nLars\n\n\n[1] https://public-inbox.org/git/xmqqr2syvjxb.fsf@gitster.mtv.corp.google.com/\n[2] https://public-inbox.org/git/274B4850-2EB7-4BFA-A42C-25A573254969@gmail.com/\n[3] https://public-inbox.org/git/DAEC36C7-AE09-4C9B-ACC4-07F2C5F2B97F@gmail.com/\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/larsxschneider/git/commit/6fd6d8e682\n Checkout: git fetch https://github.com/larsxschneider/git editor-v2 && git checkout 6fd6d8e682\n\n\n editor.c | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/editor.c b/editor.c\nindex 7519edecdc..23db92d8c6 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\tconst char *args[] = { editor, real_path(path), NULL };\n \t\tstruct child_process p = CHILD_PROCESS_INIT;\n \t\tint ret, sig;\n+\t\tstatic const char *close_notice = NULL;\n+\n+\t\tif (isatty(2) && !close_notice) {\n+\t\t\tchar *term = getenv(\"TERM\");\n+\n+\t\t\tif (term && strcmp(term, \"dumb\"))\n+\t\t\t\t/*\n+\t\t\t\t * go back to the beginning and erase the\n+\t\t\t\t * entire line if the terminal is capable\n+\t\t\t\t * to do so, to avoid wasting the vertical\n+\t\t\t\t * space.\n+\t\t\t\t */\n+\t\t\t\tclose_notice = \"\\r\\033[K\";\n+\t\t\telse\n+\t\t\t\t/* otherwise, complete and waste the line */\n+\t\t\t\tclose_notice = \"done.\\n\";\n+\t\t}\n+\n+\t\tif (close_notice) {\n+\t\t\tfprintf(\n+\t\t\t\tstderr,\n+\t\t\t\t\"Launched your editor ('%s'). Adjust, save, and close the \"\n+\t\t\t\t\"file to continue. Waiting for your input... \", editor\n+\t\t\t);\n+\t\t\tfflush(stderr);\n+\t\t}\n\n \t\tp.argv = args;\n \t\tp.env = env;\n@@ -53,11 +79,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n \t\tsigchain_pop(SIGQUIT);\n+\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+\t\tif (close_notice)\n+\t\t\tfputs(close_notice, stderr);\n \t}\n\n \tif (!buffer)\n\nbase-commit: 89ea799ffcc5c8a0547d3c9075eb979256ee95b8\n--\n2.15.0\n\n"},{"id":"332766","messageId":"xmqqy3n4rbnr.fsf@gitster.mtv.corp.google.com","threadId":"47229","inReplyTo":"20171117135109.18071-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-17T18:40:56Z","receivedAt":"2017-11-17T18:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lars.schneider@autodesk.com writes:\n\n> Junio posted the original version of this patch [1] as response to my RFC [2].\n> I took Junio's patch and slightly changed the commit message as well as the\n> message printed to the user after GIT_EDITOR is invoked [3].\n>\n> Thanks,\n> Lars\n\nThanks.\n\n> diff --git a/editor.c b/editor.c\n> index 7519edecdc..23db92d8c6 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf...\n> ...\n> +\t\tif (close_notice) {\n> +\t\t\tfprintf(\n> +\t\t\t\tstderr,\n> +\t\t\t\t\"Launched your editor ('%s'). Adjust, save, and close the \"\n> +\t\t\t\t\"file to continue. Waiting for your input... \", editor\n\nHow wide is your typical terminal window? With message this long, a\nsample standalone program I used while developing the prototype of\nthis feature no longer can retract this \"temporary\" message.\n\nWould something shorter like \"Waiting for you to finish editing...\"\nwork well enough?\n\n-- -- --\n#include \n\nint main(void)\n{\n\tconst char *EL = \"\\033[K\"; /* Erase in Line */\n\tconst char *editor = \"emacsclient\";\n\n\tfprintf(\n\t\tstderr,\n\t\t\"Launched your editor ('%s'). Adjust, save, and close the \"\n\t\t\"file to continue. Waiting for your input... \", editor);\n\tfflush(stderr);\n\tsleep(2);\n\tfprintf(stderr, \"\\r%s\", EL);\n\tfflush(stderr);\n\treturn 0;\n}\n-- -- --\n\n"},{"id":"332767","messageId":"CAPig+cQ3a0guJUhnbktrjs6fL6mSrUXmPqR0BafEAOhVr7Sy-w@mail.gmail.com","threadId":"47229","inReplyTo":"20171117135109.18071-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-17T19:41:18Z","receivedAt":"2017-11-17T19:41:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 17, 2017 at 8:51 AM, wrote:\n> When a graphical GIT_EDITOR is spawned by a Git command that opens\n> and waits for user input (e.g. \"git rebase -i\"), then the editor window\n> might be obscured by other windows. The user may be left staring at the\n> original Git terminal window without even realizing that s/he needs to\n> interact with another window before Git can proceed. To this user Git\n> appears hanging.\n>\n> Show a message in the original terminal and get rid of it when the\n> editor returns.\n>\n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/editor.c b/editor.c\n> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n> + static const char *close_notice = NULL;\n> +\n> + if (isatty(2) && !close_notice) {\n\nIf you reverse this condition to say (!close_notice && isatty(2)),\nthen you save an isatty() invocation each time if close_notice is\nalready assigned.\n\nHowever, it's not clear how much benefit you gain from stashing this\naway in a static variable. Premature optimization?\n\n> + char *term = getenv(\"TERM\");\n> +\n> + if (term && strcmp(term, \"dumb\"))\n> + /*\n> + * go back to the beginning and erase the\n> + * entire line if the terminal is capable\n> + * to do so, to avoid wasting the vertical\n> + * space.\n> + */\n> + close_notice = \"\\r\\033[K\";\n> + else\n> + /* otherwise, complete and waste the line */\n> + close_notice = \"done.\\n\";\n> + }\n> +\n> + if (close_notice) {\n> + fprintf(\n> + stderr,\n> + \"Launched your editor ('%s'). Adjust, save, and close the \"\n> + \"file to continue. Waiting for your input... \", editor\n> + );\n\nHere's what this looks like for me:\n\n--- 8< ---\nLaunched your editor\n('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,\nsave, and close the file to continue. Waiting for your input...\nWaiting for Emacs...\n--- 8< ---\n\nVery, very noisy, so much so that it's almost unreadable. There are at\nleast three reasons for the noise:\n\n* The raw message itself is already overly long. Do we really need to\nassume that newcomers are so clueless that they need it spelled out to\nsuch a level of detail? \"Launched editor\" should be enough for most\npeople, and one would hope that \"Launched editor; waiting for\ninput...\" would be enough for the rest.\n\n* Does not take into consideration that EDITOR might be very long;\nperhaps you could just print the basename and strip arguments (i.e.\n\"/my/long/path/edit -x --foo --zap\" becomes \"edit\"). Or, just omit the\neditor altogether.\n\n* emacsclient already prints its own message (\"Waiting for Emacs...\",\nwhich runs together with Git's message). Perhaps treat emacsclient as\na special case and skip printing this message if emacsclient is in\nuse: if (strstr(...,\"emacsclient\"))\n\nAnd, of course, with a \"dumb\" terminal, it's even noisier with the\nextra \"done.\" at the end:\n\n--- 8< ---\nLaunched your editor\n('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,\nsave, and close the file to continue. Waiting for your input...\nWaiting for Emacs...\ndone.\n--- 8< ---\n\nAs Junio pointed out in [1], emacsclient has already emitted a\nnewline, so the clear-line sequence is ineffective; likewise, for a\ndumb terminal, \"done.\" ends up on its own line. Aside from the noise,\nthis also suggests making a special case for emacsclient.\n\nAnd, as Junio pointed out in [2], with a message so long, once it has\nwrapped, the clear-line sequence does not work as intended. For those\nof us with 80-column terminals, we're left with a bunch of noise on\nthe screen.\n\n> + fflush(stderr);\n> + }\n>\n> p.argv = args;\n> p.env = env;\n> @@ -53,11 +79,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n> sig = ret - 128;\n> sigchain_pop(SIGINT);\n> sigchain_pop(SIGQUIT);\n> +\n> if (sig == SIGINT || sig == SIGQUIT)\n> raise(sig);\n> if (ret)\n> return error(\"There was a problem with the editor '%s'.\",\n> editor);\n> + if (close_notice)\n> + fputs(close_notice, stderr);\n\nShould printing of close_notice be done before the error()? Otherwise,\nyou get this:\n\n--- 8< ---\nLaunched your editor (...) ...There was a problem...\n--- 8< ---\n\n> }\n\n[1]: https://public-inbox.org/git/xmqqr2syvjxb.fsf@gitster.mtv.corp.google.com/\n[2]: https://public-inbox.org/git/xmqqy3n4rbnr.fsf@gitster.mtv.corp.google.com/\n"},{"id":"332784","messageId":"xmqqh8tsqs83.fsf@gitster.mtv.corp.google.com","threadId":"47229","inReplyTo":"CAPig+cQ3a0guJUhnbktrjs6fL6mSrUXmPqR0BafEAOhVr7Sy-w@mail.gmail.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-18T01:40:44Z","receivedAt":"2017-11-18T01:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n>> + static const char *close_notice = NULL;\n>> +\n>> + if (isatty(2) && !close_notice) {\n>\n> If you reverse this condition to say (!close_notice && isatty(2)),\n> then you save an isatty() invocation each time if close_notice is\n> already assigned.\n>\n> However, it's not clear how much benefit you gain from stashing this\n> away in a static variable. Premature optimization?\n\nThe variable being \"static\" could be (but it was done primarily\nbecause it allowed me not to worry about freeing), but during a\nsingle invocation, it primarily is used as a flag \"are we showing\nthe editor invocation notice?\" two places in the function can use\nwithout having to do the same checks twice.\n\nIf we want this as an optimization \"we've already checked the\ncondition in our previous call, so let's use the result without\nchecking again\", then this has to become tristate:\n\n - We haven't checked, and needs checking (probably NULL);\n\n - We have checked, and we know we want to show the notice---here is\n a string to use when we clear the notice (what we have here);\n\n - We have checked, and we know we do not want to show the notice\n (there is no provision for this in the posted code---that makes\n this not an optimization yet).\n\nPerhaps an empty string can be used for the last one, but if/when it\nis done, the above needs to go in a comment near the definition of\nthe variable.\n\n> Should printing of close_notice be done before the error()? Otherwise,\n> you get this:\n>\n> --- 8< ---\n> Launched your editor (...) ...There was a problem...\n> --- 8< ---\n\nIn my version with a far shorter message, I deliberately chose not\nto clear the notice. We ran the editor, and we saw a problem. That\nis what happened and that is what will be left on the terminal.\n\nI agree that the verbose message Lars substituted makes it harder to\nread.\n"},{"id":"332862","messageId":"d53a655b-f51e-3f44-23db-581071010fd2@gmail.com","threadId":"47229","inReplyTo":"xmqqh8tsqs83.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-19T17:49:51Z","receivedAt":"2017-11-19T17:50:12Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Saturday 18 November 2017 07:10 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>>> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n>>> + static const char *close_notice = NULL;\n>>> +\n>>> + if (isatty(2) && !close_notice) {\n>>\n>> If you reverse this condition to say (!close_notice && isatty(2)),\n>> then you save an isatty() invocation each time if close_notice is\n>> already assigned.\n>>\n>> However, it's not clear how much benefit you gain from stashing this\n>> away in a static variable. Premature optimization?\n> \n> The variable being \"static\" could be (but it was done primarily\n> because it allowed me not to worry about freeing),\n\nAFAIK, observing the way the variable 'close_notice' is used, I guess \nyou don't need to worry about freeing it up even if it wasn't \"static\".\nThat's my interpretation of the following section of the C standard,\n\n========================\n6.5.2.5 Compound literals\n\n...\n\n9 EXAMPLE 2\nIn contrast, in\nvoid f(void)\n{\nint *p;\n/*...*/\np = (int [2]){*p};\n/*...*/\n}\np is assigned the address of the first element of an array of two ints, \nthe first having the value previously pointed to by p and the second, \nzero. The expressions in this compound literal need not be constant. The\nunnamed object has automatic storage duration.\n=========================\n\nSo the unnamed objects created as a consequence of the string literals \nassigned to 'close_notice' should have \"automatic\" storage duration \nwhich means you don't have to worry about allocating memory for them \nwhich would make you worry about freeing it up. If I'm stating something \nwrong, let me know.\n\nBTW, I find making the variable 'close_notice' to be 'static' unwanted \nas I couldn't find any piece of code that invokes 'launch_editor' more \nthan once within a single run. What could be the possible cases in which \n'launch_editor' could be called twice ?\n\n\n>> Should printing of close_notice be done before the error()? Otherwise,\n>> you get this:\n>>\n>> --- 8< ---\n>> Launched your editor (...) ...There was a problem...\n>> --- 8< ---\n> \n> In my version with a far shorter message, I deliberately chose not\n> to clear the notice. We ran the editor, and we saw a problem. That\n> is what happened and that is what will be left on the terminal.\n> \n\nIt might be a good thing to keep the notice but I think it would be \nbetter to have that error message in a \"new line\". I'm not sure if it's \npossible or not.\n\n\n---\nKaartic\n"},{"id":"332880","messageId":"xmqqvai5ollx.fsf@gitster.mtv.corp.google.com","threadId":"47229","inReplyTo":"d53a655b-f51e-3f44-23db-581071010fd2@gmail.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-20T00:11:06Z","receivedAt":"2017-11-20T00:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n>>> However, it's not clear how much benefit you gain from stashing this\n>>> away in a static variable. Premature optimization?\n>>\n>> The variable being \"static\" could be (but it was done primarily\n>> because it allowed me not to worry about freeing),\n\nThe current code happens to be safe because I do not allocate. I do\nnot know what others will do to the code in the future, and at that\npoint, instinct kicks in to futureproof against the worst ;-).\n\n>>> Should printing of close_notice be done before the error()? Otherwise,\n>>> you get this:\n>>>\n>>> --- 8< ---\n>>> Launched your editor (...) ...There was a problem...\n>>> --- 8< ---\n>>\n>> In my version with a far shorter message, I deliberately chose not\n>> to clear the notice. We ran the editor, and we saw a problem. That\n>> is what happened and that is what will be left on the terminal.\n>>\n>\n> It might be a good thing to keep the notice but I think it would be\n> better to have that error message in a \"new line\". I'm not sure if\n> it's possible or not.\n\nOf course it is possible, if you really wanted to. The code knows\nif it gave the \"we launched and waiting for you\" notice, so it can\nmaintain not just one (i.e. \"how I close the notice?\") but another\none (i.e. \"how I do so upon an error?\") and use it in the error\ncodepath.\n\n"},{"id":"333292","messageId":"0DD1EE8A-47F1-41AA-8F86-C9FDF99D22A0@gmail.com","threadId":"47229","inReplyTo":"xmqqy3n4rbnr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-22T16:47:09Z","receivedAt":"2017-11-22T16:47:17Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Nov 2017, at 19:40, Junio C Hamano wrote:\n> \n> lars.schneider@autodesk.com writes:\n> \n>> Junio posted the original version of this patch [1] as response to my RFC [2].\n>> I took Junio's patch and slightly changed the commit message as well as the\n>> message printed to the user after GIT_EDITOR is invoked [3].\n>> \n>> Thanks,\n>> Lars\n> \n> Thanks.\n> \n>> diff --git a/editor.c b/editor.c\n>> index 7519edecdc..23db92d8c6 100644\n>> --- a/editor.c\n>> +++ b/editor.c\n>> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf...\n>> ...\n>> +\t\tif (close_notice) {\n>> +\t\t\tfprintf(\n>> +\t\t\t\tstderr,\n>> +\t\t\t\t\"Launched your editor ('%s'). Adjust, save, and close the \"\n>> +\t\t\t\t\"file to continue. Waiting for your input... \", editor\n> \n> How wide is your typical terminal window? With message this long, a\n> sample standalone program I used while developing the prototype of\n> this feature no longer can retract this \"temporary\" message.\n> \n> Would something shorter like \"Waiting for you to finish editing...\"\n> work well enough?\n\nYeah, Eric criticized the verbosity elsewhere, too. I understand your point\nof view. Let's revert it to your initial short version. \n\n- Lars\n\n> \n> -- -- --\n> #include \n> \n> int main(void)\n> {\n> \tconst char *EL = \"\\033[K\"; /* Erase in Line */\n> \tconst char *editor = \"emacsclient\";\n> \n> \tfprintf(\n> \t\tstderr,\n> \t\t\"Launched your editor ('%s'). Adjust, save, and close the \"\n> \t\t\"file to continue. Waiting for your input... \", editor);\n> \tfflush(stderr);\n> \tsleep(2);\n> \tfprintf(stderr, \"\\r%s\", EL);\n> \tfflush(stderr);\n> \treturn 0;\n> }\n> -- -- --\n> \n\n"},{"id":"333293","messageId":"919EA31C-FDC5-4694-9614-157042D2DA82@gmail.com","threadId":"47229","inReplyTo":"CAPig+cQ3a0guJUhnbktrjs6fL6mSrUXmPqR0BafEAOhVr7Sy-w@mail.gmail.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-22T16:53:32Z","receivedAt":"2017-11-22T16:53:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Nov 2017, at 20:41, Eric Sunshine wrote:\n> \n> On Fri, Nov 17, 2017 at 8:51 AM, wrote:\n> \n>> + char *term = getenv(\"TERM\");\n>> +\n>> + if (term && strcmp(term, \"dumb\"))\n>> + /*\n>> + * go back to the beginning and erase the\n>> + * entire line if the terminal is capable\n>> + * to do so, to avoid wasting the vertical\n>> + * space.\n>> + */\n>> + close_notice = \"\\r\\033[K\";\n>> + else\n>> + /* otherwise, complete and waste the line */\n>> + close_notice = \"done.\\n\";\n>> + }\n>> +\n>> + if (close_notice) {\n>> + fprintf(\n>> + stderr,\n>> + \"Launched your editor ('%s'). Adjust, save, and close the \"\n>> + \"file to continue. Waiting for your input... \", editor\n>> + );\n> \n> Here's what this looks like for me:\n> \n> --- 8< ---\n> Launched your editor\n> ('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,\n> save, and close the file to continue. Waiting for your input...\n> Waiting for Emacs...\n> --- 8< ---\n> \n> Very, very noisy, so much so that it's almost unreadable. There are at\n> least three reasons for the noise:\n> \n> * The raw message itself is already overly long. Do we really need to\n> assume that newcomers are so clueless that they need it spelled out to\n> such a level of detail? \"Launched editor\" should be enough for most\n> people, and one would hope that \"Launched editor; waiting for\n> input...\" would be enough for the rest.\n> \n> * Does not take into consideration that EDITOR might be very long;\n> perhaps you could just print the basename and strip arguments (i.e.\n> \"/my/long/path/edit -x --foo --zap\" becomes \"edit\"). Or, just omit the\n> editor altogether.\n\nAgreed already in another reply here:\nhttps://public-inbox.org/git/0DD1EE8A-47F1-41AA-8F86-C9FDF99D22A0@gmail.com/\n\n> \n> * emacsclient already prints its own message (\"Waiting for Emacs...\",\n> which runs together with Git's message). Perhaps treat emacsclient as\n> a special case and skip printing this message if emacsclient is in\n> use: if (strstr(...,\"emacsclient\"))\n\nIf Junio et al. are ok with the special handling of emacs, then I am happy \nto add this change in v3. If we look for \"emacsclient\", then would this \ncover emacs on Linux and Windows, too? (I am no emacs user)\n\n\n> \n> And, of course, with a \"dumb\" terminal, it's even noisier with the\n> extra \"done.\" at the end:\n> \n> --- 8< ---\n> Launched your editor\n> ('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,\n> save, and close the file to continue. Waiting for your input...\n> Waiting for Emacs...\n> done.\n> --- 8< ---\n> \n> As Junio pointed out in [1], emacsclient has already emitted a\n> newline, so the clear-line sequence is ineffective; likewise, for a\n> dumb terminal, \"done.\" ends up on its own line. Aside from the noise,\n> this also suggests making a special case for emacsclient.\n> \n> And, as Junio pointed out in [2], with a message so long, once it has\n> wrapped, the clear-line sequence does not work as intended. For those\n> of us with 80-column terminals, we're left with a bunch of noise on\n> the screen.\n> \n>> + fflush(stderr);\n>> + }\n>> \n>> p.argv = args;\n>> p.env = env;\n>> @@ -53,11 +79,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n>> sig = ret - 128;\n>> sigchain_pop(SIGINT);\n>> sigchain_pop(SIGQUIT);\n>> +\n>> if (sig == SIGINT || sig == SIGQUIT)\n>> raise(sig);\n>> if (ret)\n>> return error(\"There was a problem with the editor '%s'.\",\n>> editor);\n>> + if (close_notice)\n>> + fputs(close_notice, stderr);\n> \n> Should printing of close_notice be done before the error()? Otherwise,\n> you get this:\n> \n> --- 8< ---\n> Launched your editor (...) ...There was a problem...\n> --- 8< ---\n\nI think we should keep it as I agree with Junio's argument here:\nhttps://public-inbox.org/git/xmqqh8tsqs83.fsf@gitster.mtv.corp.google.com/\n\n- Lars"},{"id":"333294","messageId":"998E3A52-C710-447A-82A3-50C58354CC31@gmail.com","threadId":"47229","inReplyTo":"xmqqvai5ollx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-22T16:55:29Z","receivedAt":"2017-11-22T16:55:36Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 20 Nov 2017, at 01:11, Junio C Hamano wrote:\n> \n> Kaartic Sivaraam writes:\n> \n>>>> However, it's not clear how much benefit you gain from stashing this\n>>>> away in a static variable. Premature optimization?\n>>> \n>>> The variable being \"static\" could be (but it was done primarily\n>>> because it allowed me not to worry about freeing),\n> \n> The current code happens to be safe because I do not allocate. I do\n> not know what others will do to the code in the future, and at that\n> point, instinct kicks in to futureproof against the worst ;-).\n> \n>>>> Should printing of close_notice be done before the error()? Otherwise,\n>>>> you get this:\n>>>> \n>>>> --- 8< ---\n>>>> Launched your editor (...) ...There was a problem...\n>>>> --- 8< ---\n>>> \n>>> In my version with a far shorter message, I deliberately chose not\n>>> to clear the notice. We ran the editor, and we saw a problem. That\n>>> is what happened and that is what will be left on the terminal.\n>>> \n>> \n>> It might be a good thing to keep the notice but I think it would be\n>> better to have that error message in a \"new line\". I'm not sure if\n>> it's possible or not.\n> \n> Of course it is possible, if you really wanted to. The code knows\n> if it gave the \"we launched and waiting for you\" notice, so it can\n> maintain not just one (i.e. \"how I close the notice?\") but another\n> one (i.e. \"how I do so upon an error?\") and use it in the error\n> codepath.\n\nI think a newline makes sense. I'll look into this for v3.\n\nThanks,\nLars\n"},{"id":"333302","messageId":"CAPig+cQ=E4Ohybf_zPKEPMQg5iW+eqG4Y4Jcd4pNz9pk9i4+sA@mail.gmail.com","threadId":"47229","inReplyTo":"919EA31C-FDC5-4694-9614-157042D2DA82@gmail.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-22T17:33:42Z","receivedAt":"2017-11-22T17:33:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 22, 2017 at 11:53 AM, Lars Schneider\n wrote:\n>> On 17 Nov 2017, at 20:41, Eric Sunshine wrote:\n>> * emacsclient already prints its own message (\"Waiting for Emacs...\",\n>> which runs together with Git's message). Perhaps treat emacsclient as\n>> a special case and skip printing this message if emacsclient is in\n>> use: if (strstr(...,\"emacsclient\"))\n>\n> If Junio et al. are ok with the special handling of emacs, then I am happy\n> to add this change in v3. If we look for \"emacsclient\", then would this\n> cover emacs on Linux and Windows, too? (I am no emacs user)\n\nYes, searching for \"emacsclient\" should work on all platforms (Linux,\nMacOS, Windows, BSD). Most of the time, the full value of EDITOR is\njust \"emacsclient\", but sometimes emacsclient is located at an\nout-of-the-way location, not in PATH, such as in my example\n(/long/path/to/emacsclient). On Windows, it's actually called\n\"emacsclientw\", but that should be matched just fine when searching\nfor \"emacsclient\".\n"},{"id":"333307","messageId":"96f9c87c-d2c3-ebf3-bcca-da538c946733@gmail.com","threadId":"47229","inReplyTo":"998E3A52-C710-447A-82A3-50C58354CC31@gmail.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-22T17:58:55Z","receivedAt":"2017-11-22T17:59:08Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 22 November 2017 10:25 PM, Lars Schneider wrote:\n> \n>> On 20 Nov 2017, at 01:11, Junio C Hamano wrote:\n>>\n>> Kaartic Sivaraam writes:\n>>> It might be a good thing to keep the notice but I think it would be\n>>> better to have that error message in a \"new line\". I'm not sure if\n>>> it's possible or not.\n>>\n>> Of course it is possible, if you really wanted to. The code knows\n>> if it gave the \"we launched and waiting for you\" notice, so it can\n>> maintain not just one (i.e. \"how I close the notice?\") but another\n>> one (i.e. \"how I do so upon an error?\") and use it in the error\n>> codepath.\n> \n> I think a newline makes sense. I'll look into this for v3.\n>\n\nIf I remember correctly, I don't think it's as simple as printing a \nnewline character in case of an error. That's because the error message \nthat shows up in the same line as \"Launched your ...\" comes from a \ndifferent function (possibly finish_command() though I'm not sure)\n\n> Thanks,\n> Lars\n> \n\n"},{"id":"333415","messageId":"xmqqk1ygnpu7.fsf@gitster.mtv.corp.google.com","threadId":"47229","inReplyTo":"998E3A52-C710-447A-82A3-50C58354CC31@gmail.com","subject":"Re: [PATCH v2] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-24T06:38:40Z","receivedAt":"2017-11-24T06:38:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> Of course it is possible, if you really wanted to. The code knows\n>> if it gave the \"we launched and waiting for you\" notice, so it can\n>> maintain not just one (i.e. \"how I close the notice?\") but another\n>> one (i.e. \"how I do so upon an error?\") and use it in the error\n>> codepath.\n>\n> I think a newline makes sense. I'll look into this for v3.\n\nAs this is an error codepath, I am OK to waste one more line with a\nline break after the \"we launched and are waiting...\" message, but\nwith a shorter \"we are waiting...\" message, I do not know if it is\nbad enough to have these two on the same line, so I am on the fence.\n"}]}