{"thread":{"id":"47213","subject":"[RFC] Indicate that Git waits for user input via editor","startedAt":"2017-11-15T15:08:32Z","lastAt":"2017-11-19T01:07:37Z","messageCount":16,"participants":["Lars Schneider","Stefan Beller","Junio C Hamano","Kaartic Sivaraam"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"332605","messageId":"274B4850-2EB7-4BFA-A42C-25A573254969@gmail.com","threadId":"47213","inReplyTo":null,"subject":"[RFC] Indicate that Git waits for user input via editor","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-15T15:08:23Z","receivedAt":"2017-11-15T15:08:32Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nGit gathers user input via text editor in certain commands (e.g. \"git commit\").\nIf you configure a text based editor, such as vi, then this works great as the \neditor is opened in your terminal window in the foreground and in focus.\n\nHowever, if you configure an editor that runs outside your terminal window then\nyou might run into the following problem: \nGit opens the editor but the editor is the background or on another screen and \nconsequently you don't see the editor. You only see the Git command line \ninterface which appears to hang.\n\nI wonder if would make sense to print \"Opening editor for user input...\" or\nsomething to the screen to make the user aware of the action. Does this sound\nsensible to you? Am I missing an existing solution to this problem?\n\nThanks,\nLars"},{"id":"332610","messageId":"CAGZ79kaqyRTaj6=-7Jmb1K6_aSH5-BaEhm7PPyRgJLsHuDF-tw@mail.gmail.com","threadId":"47213","inReplyTo":"274B4850-2EB7-4BFA-A42C-25A573254969@gmail.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-15T17:51:46Z","receivedAt":"2017-11-15T17:51:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 15, 2017 at 7:08 AM, Lars Schneider\n wrote:\n> Hi,\n>\n> Git gathers user input via text editor in certain commands (e.g. \"git commit\").\n> If you configure a text based editor, such as vi, then this works great as the\n> editor is opened in your terminal window in the foreground and in focus.\n>\n> However, if you configure an editor that runs outside your terminal window then\n> you might run into the following problem:\n> Git opens the editor but the editor is the background or on another screen and\n> consequently you don't see the editor. You only see the Git command line\n> interface which appears to hang.\n>\n> I wonder if would make sense to print \"Opening editor for user input...\" or\n> something to the screen to make the user aware of the action. Does this sound\n> sensible to you? Am I missing an existing solution to this problem?\n\nCan this be put in a wrapper that opens the text editor?\nThe wrapper would print these lines and then open the text editor;\ndepending on the operating system, there might even be a command to\nfocus on that editor window.\n\nRegarding Git, maybe improve the documentation for how to set the editor\nvariable?\n\n\n\n>\n> Thanks,\n> Lars\n"},{"id":"332611","messageId":"7CF0CF44-3772-48D9-A2EF-E501645A6211@gmail.com","threadId":"47213","inReplyTo":"CAGZ79kaqyRTaj6=-7Jmb1K6_aSH5-BaEhm7PPyRgJLsHuDF-tw@mail.gmail.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-15T18:07:26Z","receivedAt":"2017-11-15T18:07:35Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 15 Nov 2017, at 18:51, Stefan Beller wrote:\n> \n> On Wed, Nov 15, 2017 at 7:08 AM, Lars Schneider\n> wrote:\n>> Hi,\n>> \n>> Git gathers user input via text editor in certain commands (e.g. \"git commit\").\n>> If you configure a text based editor, such as vi, then this works great as the\n>> editor is opened in your terminal window in the foreground and in focus.\n>> \n>> However, if you configure an editor that runs outside your terminal window then\n>> you might run into the following problem:\n>> Git opens the editor but the editor is the background or on another screen and\n>> consequently you don't see the editor. You only see the Git command line\n>> interface which appears to hang.\n>> \n>> I wonder if would make sense to print \"Opening editor for user input...\" or\n>> something to the screen to make the user aware of the action. Does this sound\n>> sensible to you? Am I missing an existing solution to this problem?\n> \n> Can this be put in a wrapper that opens the text editor?\n> The wrapper would print these lines and then open the text editor;\n> depending on the operating system, there might even be a command to\n> focus on that editor window.\n\nYeah, that would be a workaround. However, in order to take these steps (write the \nwrapper, enable the focus command) the users needs to understand the problem.\nThat's quite a leap especially for new Git users. They just get the feeling \"Git \nis broken because it hangs\".\n\nCan you imagine any downside for an \"Opening editor for user input...\" message?\n\n\n> Regarding Git, maybe improve the documentation for how to set the editor\n> variable?\n\nThe sad truth is that 98% of the users do not read the documentation\n(made up number but close to my observed reality).\n\n\n- Lars"},{"id":"332612","messageId":"CAGZ79kb4DdV3P0z9qQkHMt9oj40sddfRWdT3k-fMj2-CJkwXiA@mail.gmail.com","threadId":"47213","inReplyTo":"7CF0CF44-3772-48D9-A2EF-E501645A6211@gmail.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-15T18:37:56Z","receivedAt":"2017-11-15T18:38:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 15, 2017 at 10:07 AM, Lars Schneider\n wrote:\n\n>> Can this be put in a wrapper that opens the text editor?\n>> The wrapper would print these lines and then open the text editor;\n>> depending on the operating system, there might even be a command to\n>> focus on that editor window.\n>\n> Yeah, that would be a workaround. However, in order to take these steps (write the\n> wrapper, enable the focus command) the users needs to understand the problem.\n> That's quite a leap especially for new Git users. They just get the feeling \"Git\n> is broken because it hangs\".\n>\n> Can you imagine any downside for an \"Opening editor for user input...\" message?\n\n\"Wasting precious screen real estate for power users\" (tm)\nSo maybe if one can turn it off, this would be ok? Or even for known inline\neditors?\n\n>> Regarding Git, maybe improve the documentation for how to set the editor\n>> variable?\n>\n> The sad truth is that 98% of the users do not read the documentation\n> (made up number but close to my observed reality).\n\nYeah that is an excellent point, but you forgot the people who\nlook at stackoverflow which of course links back to the docs in\nthe first excellent reply.\n\nIn a way this suggestion of bettering the docs was a cheap way out, sorry.\n"},{"id":"332625","messageId":"xmqq8tf7yxzn.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"274B4850-2EB7-4BFA-A42C-25A573254969@gmail.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-15T22:32:12Z","receivedAt":"2017-11-15T22:32:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> However, if you configure an editor that runs outside your terminal window then\n> you might run into the following problem: \n> Git opens the editor but the editor is the background or on another screen and \n> consequently you don't see the editor. You only see the Git command line \n> interface which appears to hang.\n>\n> I wonder if would make sense to print \"Opening editor for user input...\" or\n> something to the screen to make the user aware of the action. Does this sound\n> sensible to you? Am I missing an existing solution to this problem?\n\nMy knee-jerk reaction was: for such a user who has EDITOR set to a\nprogram that pops under, wouldn't any program, not just Git, that\nuses the editor to open a file for editing/viewing look broken?\nWould we care if we are called \"broken\" by such a clueless user who\ncannot tell a (non-)broken caller of an editor and a broken editor?\n\nBut that is true only when the user does realize/expect that the\nprogram s/he is running _will_ open an editor at the point of the\nworkflow. If s/he types \"git merge\" or \"git rebase -i @{u}\", for\nexample, it is true that the world would be a better place if s/he\nknows that would ask a file to be edited with an editor, but it is\nunrealisic to expect that everybody knows how to operate these\ncommands. Everybody is a newbie at least once.\n\nI wonder if we can do something like\n\n\tgit_spawn_editor()\n\t{\n\t\tconst char *EL = \"\\033[K\"; /* Erase in Line */\n\n\t\t/* notice the lack of terminating LF */\n\t\tfprintf(stderr, \"Launching your editor...\");\n\t\tfflush(stderr);\n\n\t\tif (!run_command(... spawn the editor ...)) {\n\t\t\t/* Success! - go back and erase the whole line */\n\t\t\tfprintf(stderr, \"\\r%s\", EL);\n\t\t} else {\n\t\t\tfprintf(stderr, \"failed (%s)\\n\", strerror(errno));\n\t\t}\n\t\tfflush(stderr);\n\t}\n\nto tentatively give a message without permanently wasting the\nvertical space.\n"},{"id":"332626","messageId":"CAGZ79kZbm8SGY4rXKZHV82E-HX9qbQ4iyCbMgJEBFQf4fj3u=Q@mail.gmail.com","threadId":"47213","inReplyTo":"xmqq8tf7yxzn.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-16T00:06:25Z","receivedAt":"2017-11-16T00:06:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 15, 2017 at 2:32 PM, Junio C Hamano wrote:\n> Lars Schneider writes:\n>\n>> However, if you configure an editor that runs outside your terminal window then\n>> you might run into the following problem:\n>> Git opens the editor but the editor is the background or on another screen and\n>> consequently you don't see the editor. You only see the Git command line\n>> interface which appears to hang.\n>>\n>> I wonder if would make sense to print \"Opening editor for user input...\" or\n>> something to the screen to make the user aware of the action. Does this sound\n>> sensible to you? Am I missing an existing solution to this problem?\n>\n> My knee-jerk reaction was: for such a user who has EDITOR set to a\n> program that pops under, wouldn't any program, not just Git, that\n> uses the editor to open a file for editing/viewing look broken?\n> Would we care if we are called \"broken\" by such a clueless user who\n> cannot tell a (non-)broken caller of an editor and a broken editor?\n>\n> But that is true only when the user does realize/expect that the\n> program s/he is running _will_ open an editor at the point of the\n> workflow. If s/he types \"git merge\" or \"git rebase -i @{u}\", for\n> example, it is true that the world would be a better place if s/he\n> knows that would ask a file to be edited with an editor, but it is\n> unrealisic to expect that everybody knows how to operate these\n> commands. Everybody is a newbie at least once.\n>\n> I wonder if we can do something like\n>\n> git_spawn_editor()\n> {\n> const char *EL = \"\\033[K\"; /* Erase in Line */\n>\n> /* notice the lack of terminating LF */\n> fprintf(stderr, \"Launching your editor...\");\n\n\"It takes quite some time to launch this special Git Editor\"\n\nAs Lars pointed out, the editor may be launched in the background,\nthat the user would not know, but they might expect a thing to\npop up as a modal dialog as is always with UIs.\n\nSo despite it being technically wrong at this point in time,\nI would phrase it in past tense or in a way that indicates that the\nuser needs to take action already.\n\nThe \"Launching...\" sounds as if I need to wait for an event to occur.\n\n> fflush(stderr);\n>\n> if (!run_command(... spawn the editor ...)) {\n> /* Success! - go back and erase the whole line */\n> fprintf(stderr, \"\\r%s\", EL);\n> } else {\n> fprintf(stderr, \"failed (%s)\\n\", strerror(errno));\n> }\n> fflush(stderr);\n> }\n>\n> to tentatively give a message without permanently wasting the\n> vertical space.\n"},{"id":"332630","messageId":"xmqqpo8jxdto.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"CAGZ79kZbm8SGY4rXKZHV82E-HX9qbQ4iyCbMgJEBFQf4fj3u=Q@mail.gmail.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T00:33:07Z","receivedAt":"2017-11-16T00:33:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I wonder if we can do something like\n>>\n>> git_spawn_editor()\n>> {\n>> const char *EL = \"\\033[K\"; /* Erase in Line */\n>>\n>> /* notice the lack of terminating LF */\n>> fprintf(stderr, \"Launching your editor...\");\n>\n> \"It takes quite some time to launch this special Git Editor\"\n>\n> As Lars pointed out, the editor may be launched in the background,\n> that the user would not know, but they might expect a thing to\n> pop up as a modal dialog as is always with UIs.\n>\n> So despite it being technically wrong at this point in time,\n> I would phrase it in past tense or in a way that indicates that the\n> user needs to take action already.\n>\n> The \"Launching...\" sounds as if I need to wait for an event to occur.\n\nHeh, I wasn't expecting phrasing nitpicks when I was trying to help\nthe thread by trying to come up with a way to avoid vertical space\nwastage.\n"},{"id":"332631","messageId":"CAGZ79kavG+gND_2oqMsB0HgZHbaCx2tRvB++8y7KvLc38mnUiQ@mail.gmail.com","threadId":"47213","inReplyTo":"xmqqpo8jxdto.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-16T00:37:57Z","receivedAt":"2017-11-16T00:38:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 15, 2017 at 4:33 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> I wonder if we can do something like\n>>>\n>>> git_spawn_editor()\n>>> {\n>>> const char *EL = \"\\033[K\"; /* Erase in Line */\n>>>\n>>> /* notice the lack of terminating LF */\n>>> fprintf(stderr, \"Launching your editor...\");\n>>\n>> \"It takes quite some time to launch this special Git Editor\"\n>>\n>> As Lars pointed out, the editor may be launched in the background,\n>> that the user would not know, but they might expect a thing to\n>> pop up as a modal dialog as is always with UIs.\n>>\n>> So despite it being technically wrong at this point in time,\n>> I would phrase it in past tense or in a way that indicates that the\n>> user needs to take action already.\n>>\n>> The \"Launching...\" sounds as if I need to wait for an event to occur.\n>\n> Heh, I wasn't expecting phrasing nitpicks when I was trying to help\n> the thread by trying to come up with a way to avoid vertical space\n> wastage.\n\nI know you weren't, but maybe it is helpful for the author of the patch\n(I presume you may not be the author, after all).\n\nBut you are right, I should have started with a more fundamental\nanswer stating this is a good idea, and I cannot think of a negative\nside effect currently.\n"},{"id":"332652","messageId":"xmqq375ewzqk.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"xmqq8tf7yxzn.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] Indicate that Git waits for user input via editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T05:37:23Z","receivedAt":"2017-11-16T05:37:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I wonder if we can do something like\n> ...\n> to tentatively give a message without permanently wasting the\n> vertical space.\n\nLearning from 13e4760a (\"recv_sideband: Do not use ANSI escape\nsequence on dumb terminals.\", 2008-01-08), I think the above should\nbe more like this:\n\n\tgit_spawn_editor()\n\t{\n\t\tstatic const char *finish;\n\n\t\tif (!finish) {\n\t\t\tconst char ANSI_FINISH[] = \"\\r\\033[K\";\n\t\t\tconst char DUMB_FINISH[] = \"done.\\n\";\n\t\t\tchar *term = getenv(\"TERM\");\n\n\t\t\tif (term && strcmp(term, \"dumb\"))\n\t\t\t\tfinish = ANSI_FINISH;\n\t\t\telse\n\t\t\t\tfinish = DUMB_FINISH;\n\t\t}\n\n\t\t/* notice the lack of terminating LF */\n\t\tfprintf(stderr, \"Launched your editor, waiting...\");\n\t\tfflush(stderr);\n\n\t\tif (!run_command(... spawn the editor ...))\n\t\t\tfputs(finish, stderr);\n\t\telse\n\t\t\tfprintf(stderr, \"failed (%s)\\n\", strerror(errno));\n\t\tfflush(stderr);\n\t}\n"},{"id":"332653","messageId":"xmqqr2syvjxb.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"274B4850-2EB7-4BFA-A42C-25A573254969@gmail.com","subject":"[PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T06:04:16Z","receivedAt":"2017-11-16T06:04:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When a graphical GIT_EDITOR is spawned by a Git command that opens\nand waits for it for the user input (e.g. \"git rebase -i\") pops its\nwindow elsewhere that is obscure, the user may be left staring the\noriginal terminal window s/he invoked the Git command from, without\neven realizing that now s/he needs to interact with another window\nthe editor is waiting in, before Git can proceed.\n\nShow a message to the original terminal, and get rid of it when the\neditor returns.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I tried this with \"emacsclient\" but it turns out that Emacs folks\n have already thought about this issue, and the program says\n \"Waiting for Emacs...\" while it does its thing, so from that\n point of view, perhaps as Stefan said originally, the editor Lars\n had trouble with is what is at fault and needs fixing? I dunno.\n\n Also, emacsclient seems to conclude its message, once the editing\n is done, by emitting LF _before_ we have a chance to do the \"go\n back to the beginning and clear\" dance, so it probably is not a\n very good example to emulate the situation Lars had trouble with.\n You cannot observe the nuking of the \"launched, waiting...\" with\n it.\n\n editor.c | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/editor.c b/editor.c\nindex 7519edecdc..1321944716 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -40,6 +40,28 @@ 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(stderr, \"Launched your editor, waiting...\");\n+\t\t\tfflush(stderr);\n+\t\t}\n \n \t\tp.argv = args;\n \t\tp.env = env;\n@@ -53,11 +75,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-- \n2.15.0-360-gf2497ca0e5\n\n"},{"id":"332655","messageId":"DAEC36C7-AE09-4C9B-ACC4-07F2C5F2B97F@gmail.com","threadId":"47213","inReplyTo":"xmqqr2syvjxb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-16T10:25:50Z","receivedAt":"2017-11-16T10:26:19Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 16 Nov 2017, at 07:04, Junio C Hamano wrote:\n\nWow. Thanks for the quick patch :-)\n\n\n> When a graphical GIT_EDITOR is spawned by a Git command that opens\n> and waits for it for the user input (e.g. \"git rebase -i\") pops its\n> window elsewhere that is obscure, the user may be left staring the\n> original terminal window s/he invoked the Git command from, without\n> even realizing that now s/he needs to interact with another window\n> the editor is waiting in, before Git can proceed.\n\nMaybe:\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\n\n> Show a message to the original terminal, and get rid of it when the\ns/to/in/ ?\n\n> editor returns.\n\n\n\n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * I tried this with \"emacsclient\" but it turns out that Emacs folks\n> have already thought about this issue, and the program says\n> \"Waiting for Emacs...\" while it does its thing, so from that\n> point of view, perhaps as Stefan said originally, the editor Lars\n> had trouble with is what is at fault and needs fixing? I dunno.\n\nBased on my (not too extensive) testing, Emacs is probably the only \neditor with this clever behavior.\n\n\n> Also, emacsclient seems to conclude its message, once the editing\n> is done, by emitting LF _before_ we have a chance to do the \"go\n> back to the beginning and clear\" dance, so it probably is not a\n> very good example to emulate the situation Lars had trouble with.\n> You cannot observe the nuking of the \"launched, waiting...\" with\n> it.\n> \n> editor.c | 25 +++++++++++++++++++++++++\n> 1 file changed, 25 insertions(+)\n> \n> diff --git a/editor.c b/editor.c\n> index 7519edecdc..1321944716 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -40,6 +40,28 @@ 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(stderr, \"Launched your editor, waiting...\");\n\nI also noticed that some people don't get that they need to save and close the\nfile to continue. Plus, we should tell them that Git is waiting for *them* and\nnot anything else. Maybe we should also tell them the editor name, but that might\nbe too verbose. I dunno. How about this?\n\n\tfprintf(stderr, \"Launched your editor ('%s'). Adjust, save, and close the file to continue. Waiting for you input... \", editor);\n\t\t\n\n- Lars"},{"id":"332666","messageId":"xmqqh8tuuv6l.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"DAEC36C7-AE09-4C9B-ACC4-07F2C5F2B97F@gmail.com","subject":"Re: [PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T14:58:42Z","receivedAt":"2017-11-16T14:58:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 16 Nov 2017, at 07:04, Junio C Hamano wrote:\n>\n> Wow. Thanks for the quick patch :-)\n\nHeh, this is not exactly my itch, so if you are inclined to, can you\ntake it over from here on?\n\nThanks.\n"},{"id":"332667","messageId":"DAA0F91D-5180-4C3B-B0E8-7C917915AC6F@gmail.com","threadId":"47213","inReplyTo":"xmqqh8tuuv6l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-16T15:24:55Z","receivedAt":"2017-11-16T15:25:03Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 16 Nov 2017, at 15:58, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> On 16 Nov 2017, at 07:04, Junio C Hamano wrote:\n>> \n>> Wow. Thanks for the quick patch :-)\n> \n> Heh, this is not exactly my itch, so if you are inclined to, can you\n> take it over from here on?\n\nAbsolutely! What is the proper way to proceed here? Should I send a v2\nwith the changes I suggested? How do I attribute you correctly? I\nassume I need to remove your 'Signed-off-by:'?\n\nThanks,\nLars\n"},{"id":"332730","messageId":"xmqq8tf5vhh4.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"DAA0F91D-5180-4C3B-B0E8-7C917915AC6F@gmail.com","subject":"Re: [PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-17T01:09:27Z","receivedAt":"2017-11-17T01:09:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 16 Nov 2017, at 15:58, Junio C Hamano wrote:\n>> \n>> Lars Schneider writes:\n>> \n>>>> On 16 Nov 2017, at 07:04, Junio C Hamano wrote:\n>>> \n>>> Wow. Thanks for the quick patch :-)\n>> \n>> Heh, this is not exactly my itch, so if you are inclined to, can you\n>> take it over from here on?\n>\n> Absolutely! What is the proper way to proceed here? Should I send a v2\n> with the changes I suggested? How do I attribute you correctly? I\n> assume I need to remove your 'Signed-off-by:'?\n\nIf you are making major changes over the original patch, you'd\nprobably want to take the authorship and mention me on Helped-by:\nand/or add \"This is based on a previous work by...\", if you want to\ncredit me.\n\nIf you are not taking the authorship over, then doing something like\n\n https://public-inbox.org/git/xmqqshdex0ff.fsf@gitster.mtv.corp.google.com/\n\nwould be sufficient, I would think.\n\nThanks.\n"},{"id":"332808","messageId":"26256555-4463-7bc9-5d07-8617932cf6cb@gmail.com","threadId":"47213","inReplyTo":"xmqqr2syvjxb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-18T17:47:14Z","receivedAt":"2017-11-18T17:47:28Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Thursday 16 November 2017 11:34 AM, Junio C Hamano wrote:\n> \n> * I tried this with \"emacsclient\" but it turns out that Emacs folks\n> have already thought about this issue, and the program says\n> \"Waiting for Emacs...\" while it does its thing, so from that\n> point of view, perhaps as Stefan said originally, the editor Lars\n> had trouble with is what is at fault and needs fixing? I dunno.\n> \n> Also, emacsclient seems to conclude its message, once the editing\n> is done, by emitting LF _before_ we have a chance to do the \"go\n> back to the beginning and clear\" dance, so it probably is not a\n> very good example to emulate the situation Lars had trouble with.\n> You cannot observe the nuking of the \"launched, waiting...\" with\n> it.\n>\n\nI tried this patch with 'gedit' and it seems to work fine except for one \nparticular case. When the launched editor gets killed somehow when \nwaiting for user input, the error message shown in the terminal seems to \nbe following the \"Launched editor...\" message immediately, making it \nhard to identify it.\n\n$ GIT_EDITOR=gedit ./git commit --allow-empty\nLaunched your editor, waiting...error: gedit died of signal 15\nerror: There was a problem with the editor 'gedit'.\nPlease supply the message using either -m or -F option.\n\nThough this is not something that's going to happen very often, I'm not \nsure if this is to be considered. Just wanted to note what I observed.\n\n\n> editor.c | 25 +++++++++++++++++++++++++\n> 1 file changed, 25 insertions(+)\n> \n> diff --git a/editor.c b/editor.c\n> index 7519edecdc..1321944716 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -40,6 +40,28 @@ 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\nJust a little curious, what's the advantage of making 'close_notice' \n'static' over 'auto' ?\n\n\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(stderr, \"Launched your editor, waiting...\");\n> +\t\t\tfflush(stderr);\n\nBeing curious again, is flushing 'stderr' required ? AFAIK, 'stderr' is \nunbuffered by default and I didn't notice any calls that changed the \nbuffering mode of it along this code path.\n\n\n> +\t\t}\n> \n> \t\tp.argv = args;\n> \t\tp.env = env;\n> @@ -53,11 +75,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> \n\n"},{"id":"332836","messageId":"xmqq8tf3oz3n.fsf@gitster.mtv.corp.google.com","threadId":"47213","inReplyTo":"26256555-4463-7bc9-5d07-8617932cf6cb@gmail.com","subject":"Re: [PATCH] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-19T01:07:24Z","receivedAt":"2017-11-19T01:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> $ GIT_EDITOR=gedit ./git commit --allow-empty\n> Launched your editor, waiting...error: gedit died of signal 15\n> error: There was a problem with the editor 'gedit'.\n> Please supply the message using either -m or -F option.\n>\n> Though this is not something that's going to happen very often, I'm\n> not sure if this is to be considered. Just wanted to note what I\n> observed.\n\nSee my earlier response to Eric Sunshine.\n\n>> +\t\tstatic const char *close_notice = NULL;\n>\n> Just a little curious, what's the advantage of making 'close_notice'\n> 'static' over 'auto' ?\n\nA second call, if an earlier call to the function already determined\nthat our standard error output is going to a terminal and what kind\nof terminal we are on, would just use the result from the earliser\ncall.\n\n> Being curious again, is flushing 'stderr' required ? AFAIK, 'stderr'\n> is unbuffered by default and I didn't notice any calls that changed\n> the buffering mode of it along this code path.\n\n\"By default\" is the key phrase. The code is merely being defensive\nto changes in other area of the code.\n"}]}