{"thread":{"id":"47328","subject":"[PATCH v3] launch_editor(): indicate that Git waits for user input","startedAt":"2017-11-27T13:47:55Z","lastAt":"2017-11-29T02:06:28Z","messageCount":11,"participants":["lars.schneider@autodesk.com","Kaartic Sivaraam","Eric Sunshine","Lars Schneider","brian m. carlson","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"333606","messageId":"20171127134716.69471-1-lars.schneider@autodesk.com","threadId":"47328","inReplyTo":null,"subject":"[PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2017-11-27T13:47:16Z","receivedAt":"2017-11-27T13:47:55Z","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\n\nHi,\n\ndiff to v2:\n - shortened and localized the \"waiting\" message\n - detect \"emacsclient\" and suppress \"waiting\" message\n\nI did not add a newline after the \"waiting\" message in case of an error.\nJunio indicated [1] that it is not necessary with the shorter \"waiting\"\nmessage.\n\nThanks,\nLars\n\n\n[1] https://public-inbox.org/git/xmqqk1ygnpu7.fsf@gitster.mtv.corp.google.com/\n\nNotes:\n Base Commit: 89ea799ffc (89ea799ffcc5c8a0547d3c9075eb979256ee95b8)\n Diff on Web: https://github.com/larsxschneider/git/commit/0d43814931\n Checkout: git fetch https://github.com/larsxschneider/git editor-v3 && git checkout 0d43814931\n\n editor.c | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/editor.c b/editor.c\nindex 7519edecdc..4251ae9d7a 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -40,6 +40,35 @@ 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 if (term && strstr(term, \"emacsclient\"))\n+\t\t\t\t/*\n+\t\t\t\t * `emacsclient` (or `emacsclientw` on Windows) already prints\n+\t\t\t\t * (\"Waiting for Emacs...\") if a file is opened for editing.\n+\t\t\t\t * Therefore, we don't need to print the editor launch info.\n+\t\t\t\t */\n+\t\t\t\t;\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 editor. Waiting for your input... \"));\n+\t\t\tfflush(stderr);\n+\t\t}\n\n \t\tp.argv = args;\n \t\tp.env = env;\n@@ -53,11 +82,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\n\n"},{"id":"333615","messageId":"1bb4eeb1-eaaf-11ec-8215-08d3490577a8@gmail.com","threadId":"47328","inReplyTo":"20171127134716.69471-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-27T17:17:59Z","receivedAt":"2017-11-27T17:18:17Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Monday 27 November 2017 07:17 PM, lars.schneider@autodesk.com wrote:\n\n> Show a message in the original terminal and get rid of it when the\n> editor returns.\n> \n\n\"... except in the case when an error occurs.\" could be included if needed.\n> +\t\tstatic const char *close_notice = NULL;\n> +\n\nIIRC, this variable is bound to be `static` for sake of future proofing. \nSo, I guess you could use the following suggestion of Eric Sunshine in \nthe below conditional.\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> +\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 if (term && strstr(term, \"emacsclient\"))\n> +\t\t\t\t/*\n> +\t\t\t\t * `emacsclient` (or `emacsclientw` on Windows) already prints\n> +\t\t\t\t * (\"Waiting for Emacs...\") if a file is opened for editing.\n> +\t\t\t\t * Therefore, we don't need to print the editor launch info.\n> +\t\t\t\t */\n> +\t\t\t\t;\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 editor. Waiting for your input... \"));\n> +\t\t\tfflush(stderr);\n> +\t\t}\n> \n> \t\tp.argv = args;\n> \t\tp.env = env;\n> @@ -53,11 +82,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> 2.15.0\n> \n\n"},{"id":"333625","messageId":"CAPig+cSStSbckFi5K33z0WZThjNZC5Eb1Fb+ikrd5ZajM6mV3Q@mail.gmail.com","threadId":"47328","inReplyTo":"20171127134716.69471-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-27T18:36:00Z","receivedAt":"2017-11-27T18:36:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 27, 2017 at 8:47 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,35 @@ 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> + 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 if (term && strstr(term, \"emacsclient\"))\n\nYou need to check 'editor' here, not 'term', and you should do it\nbefore the \"not dumb\" terminal check, otherwise you'll never get this\nfar.\n\n> + /*\n> + * `emacsclient` (or `emacsclientw` on Windows) already prints\n> + * (\"Waiting for Emacs...\") if a file is opened for editing.\n> + * Therefore, we don't need to print the editor launch info.\n> + */\n> + ;\n> + else\n> + /* otherwise, complete and waste the line */\n> + close_notice = _(\"done.\\n\");\n> + }\n"},{"id":"333629","messageId":"BF8D6B02-37C1-40B3-9FDA-3B1055389B85@gmail.com","threadId":"47328","inReplyTo":"CAPig+cSStSbckFi5K33z0WZThjNZC5Eb1Fb+ikrd5ZajM6mV3Q@mail.gmail.com","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-27T20:04:42Z","receivedAt":"2017-11-27T20:04:52Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 27 Nov 2017, at 19:36, Eric Sunshine wrote:\n> \n> On Mon, Nov 27, 2017 at 8:47 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,35 @@ 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>> + 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 if (term && strstr(term, \"emacsclient\"))\n> \n> You need to check 'editor' here, not 'term', and you should do it\n> before the \"not dumb\" terminal check, otherwise you'll never get this\n> far.\n\nOuch. That happens if I try to do two things at once. Embarrassing.\n\nThanks,\nLars\n\n\n> \n>> + /*\n>> + * `emacsclient` (or `emacsclientw` on Windows) already prints\n>> + * (\"Waiting for Emacs...\") if a file is opened for editing.\n>> + * Therefore, we don't need to print the editor launch info.\n>> + */\n>> + ;\n>> + else\n>> + /* otherwise, complete and waste the line */\n>> + close_notice = _(\"done.\\n\");\n>> + }\n\n"},{"id":"333630","messageId":"20171127200932.GA558049@genre.crustytoothpaste.net","threadId":"47328","inReplyTo":"20171127134716.69471-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-11-27T20:09:32Z","receivedAt":"2017-11-27T20:09:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Nov 27, 2017 at 02:47:16PM +0100, lars.schneider@autodesk.com wrote:\n> From: Junio C Hamano \n> \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> diff --git a/editor.c b/editor.c\n> index 7519edecdc..4251ae9d7a 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -40,6 +40,35 @@ 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\nSorry for coming to the topic so late, but it occurred to me that we\nmight want to conditionalize this on an advice.* flag. I expect there\nare some people who will never want to see this, and letting them turn\nit off would be good.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"333647","messageId":"20171127230520.GA29636@sigill.intra.peff.net","threadId":"47328","inReplyTo":"20171127200932.GA558049@genre.crustytoothpaste.net","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-27T23:05:20Z","receivedAt":"2017-11-27T23:05:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 27, 2017 at 08:09:32PM +0000, brian m. carlson wrote:\n\n> > Show a message in the original terminal and get rid of it when the\n> > editor returns.\n> [...]\n> \n> Sorry for coming to the topic so late, but it occurred to me that we\n> might want to conditionalize this on an advice.* flag. I expect there\n> are some people who will never want to see this, and letting them turn\n> it off would be good.\n\nI am torn between saying \"yes please, I would absolutely set such an\noption myself\" and \"if we need advice.*, that is a good sign that the\nfeature is mis-designed\".\n\nLet me elaborate a bit on the latter.\n\nMy gut feeling is that this is absolutely the wrong place to put a\nmessage like this. We don't know enough about what the editor is doing,\nso we have to take pains to avoid a crufty message in the terminal,\nincluding:\n\n - playing ANSI-term trickery to erase the message\n\n - hard-coding (!) emacsclient as a special case\n\nAnd that's why I say that \"advice.*\" is a bad sign, because it means\nthose other techniques are failing, and somebody is seeing and being\nannoyed by the cruft.\n\nThe right place for this message, IMHO, is for the editor itself (or a\nwrapper script) to say \"hey, I'm opening a new window\" (like emacsclient\ndoes).\n\nBut I also recognize that the world isn't perfect. Not all editors will\nget this right, and not all users are savvy enough to set up a wrapper\nscript for editors which don't. So defaulting this to \"on\" tries to help\nthose cases.\n\nIf the anti-cruft techniques I mentioned above work well in practice,\nthen we get to have our cake and eat it, too. If they don't, then I'm\nnot sure if the tradeoff is worth it.\n\n-Peff\n"},{"id":"333650","messageId":"xmqqefojb9a1.fsf@gitster.mtv.corp.google.com","threadId":"47328","inReplyTo":"20171127134716.69471-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-27T23:18:30Z","receivedAt":"2017-11-27T23:18:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lars.schneider@autodesk.com writes:\n\n> diff to v2:\n> - shortened and localized the \"waiting\" message\n> - detect \"emacsclient\" and suppress \"waiting\" message\n\nThanks for moving this forward.\n\n\n> +\t\tstatic const char *close_notice = NULL;\n\nBecause this thing is \"static\", the variable is NULL when the first\ncall to this function is made, and the value left in the variable\nwhen a call returns will be seen by the next call.\n\n> +\t\tif (isatty(2) && !close_notice) {\n\nDeclaring a \"static\" variable initialized to NULL and checking its\nNULL-ness upfront is a common pattern to make sure that the code\navoids repeated computation of the same thing. The body of the if\nstatement is run only when standard error stream is a tty (hinting\nan interactive session) *and* close_notice is (still) NULL.\n\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 if (term && strstr(term, \"emacsclient\"))\n> +\t\t\t\t/*\n> +\t\t\t\t * `emacsclient` (or `emacsclientw` on Windows) already prints\n> +\t\t\t\t * (\"Waiting for Emacs...\") if a file is opened for editing.\n> +\t\t\t\t * Therefore, we don't need to print the editor launch info.\n> +\t\t\t\t */\n> +\t\t\t\t;\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\nIt assigns a non-NULL value to close_notice unless the editor is\nemacsclient (modulo the bug that \"emacsclient\" is to be compared\nwith EDITOR, GIT_EDITOR, core.editor etc. -- git_editor() can be\nused to pick the right one). For a user of that particular editor,\nit is left as NULL. Because it is unlikely that EDITOR etc. would\nchange across calls to this function, for them, and only for them,\nthe above is computed to yield the same result every time this\nfunction is called.\n\nThat feels a bit uneven, doesn't it?\n\nThere are two possible solutions:\n\n1. drop \"static\" from the declaration to stress the fact that the\n variable and !close_notice in the upfront if() statement is not\n avoiding repeated computation of the same thing, or\n\n2. arrange that \"emacsclient\" case also participates in \"avoid\n repeated computation\" dance. While at it, swap the order of\n checking isatty(2) and !close_notice (aka \"have we done this\n already?)--because we do not expect us swapping file descriptor\n #2 inside this single process, we'd be repeatedly asking\n isatty(2) for the same answer.\n\nThe former may be simpler and easier, as an editor invocation would\nnot be a performance critical codepath.\n\nIf we want to do the latter, a cleaner way may be to have another\nstatic \"static int use_notice_checked = 0;\" declared, and then\n\n\tif (!use_notice_checked && isatty(2)) {\n\t\t... what you currently have, modulo the\n\t\t... fix for the editor thing, and set\n\t\t... close_notice to a string (or NULL).\n use_notice_checked = 1;\n\t}\n\nThe users of close_notice after this part that use !close_notice\nas \"do not give the notice at all\" flag and also as \"this is the\nstring to show after editor comes back\" can stay the same if you go\nthis route. That would be solution 2a.\n\nOf course, you can instead use close_notice = \"\" (empty string) as a\nsignal \"we checked and we know that we are not using the notice\nthing\". If you go that route, then the users after this if statement\nthat sets up close_notice upon the first call would say !*close_notice\ninstead of !close_notice when they try to see if the notice is in use.\nThat would be solution 2b.\n\nI personally think any one of 1., 2a., or 2b. is fine.\n\n"},{"id":"333679","messageId":"xmqqpo836n6u.fsf@gitster.mtv.corp.google.com","threadId":"47328","inReplyTo":"20171127230520.GA29636@sigill.intra.peff.net","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-28T04:29:13Z","receivedAt":"2017-11-28T04:29:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... \"if we need advice.*, that is a good sign that the\n> feature is mis-designed\".\n>\n> Let me elaborate a bit on the latter.\n>\n> My gut feeling is that this is absolutely the wrong place to put a\n> message like this....\n> The right place for this message, IMHO, is for the editor itself (or a\n> wrapper script) to say \"hey, I'm opening a new window\" (like emacsclient\n> does).\n\nYes, I think we already had that discussion and if you recall those\ninvolved in the thread except Lars were in favor of writing this off\nas \"here is a nickel--get a better editor\".\n\nOne thing that may sway you slightly in favor of doing this in Git\nis that a new user _might_ not be expecting Git to open an editor in\nresponse to the command s/he just gave it [*1*].\n\nIt is one thing that the user _knowingly_ runs an editor and fails\nto locate which window the editor opened. It is a bit different if\nthe user didn't even know that s/he is supposed to be interacting\nwith an editor.\n\n> But I also recognize that the world isn't perfect. Not all editors will\n> get this right, and not all users are savvy enough to set up a wrapper\n> script for editors which don't. So defaulting this to \"on\" tries to help\n> those cases.\n>\n> If the anti-cruft techniques I mentioned above work well in practice,\n> then we get to have our cake and eat it, too. If they don't, then I'm\n> not sure if the tradeoff is worth it.\n\nI agree with all of the above; I do not know if the \"go back and\nerase to the end of the line\" is good enough in practice, and that\nis why I sent \"here is a possible anti-cruft\" but did not push it\nstrongly forward myself.\n\n\n[Footnote]\n\n*1* \"git merge topic\" used to just complete the merge with canned\n log message and people were not all that unhappy--they just\n thought it is an OK design that a clean merge is a\n non-interactive operation. Perhaps a person new to Git may be\n expecting that behaviour (without knowing anything like (1) that\n was what we used to do or (2) we open an editor by default these\n days), wondering why nothing seems to be happening, staring at\n an inactive window, after typing \"git merge topic\".\n"},{"id":"333686","messageId":"33DF54BF-8483-4DFC-99B8-4B25883FE28C@gmail.com","threadId":"47328","inReplyTo":"20171127230520.GA29636@sigill.intra.peff.net","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-28T12:31:05Z","receivedAt":"2017-11-28T12:31:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 28 Nov 2017, at 00:05, Jeff King wrote:\n> \n> On Mon, Nov 27, 2017 at 08:09:32PM +0000, brian m. carlson wrote:\n> \n>>> Show a message in the original terminal and get rid of it when the\n>>> editor returns.\n>> [...]\n>> \n>> Sorry for coming to the topic so late, but it occurred to me that we\n>> might want to conditionalize this on an advice.* flag. I expect there\n>> are some people who will never want to see this, and letting them turn\n>> it off would be good.\n> \n> I am torn between saying \"yes please, I would absolutely set such an\n> option myself\" and \"if we need advice.*, that is a good sign that the\n> feature is mis-designed\".\n> \n> Let me elaborate a bit on the latter.\n> \n> My gut feeling is that this is absolutely the wrong place to put a\n> message like this. We don't know enough about what the editor is doing,\n> so we have to take pains to avoid a crufty message in the terminal,\n> including:\n> \n> - playing ANSI-term trickery to erase the message\n> \n> - hard-coding (!) emacsclient as a special case\n> \n> And that's why I say that \"advice.*\" is a bad sign, because it means\n> those other techniques are failing, and somebody is seeing and being\n> annoyed by the cruft.\n\nI agree with your cruft assessments, especially regarding the hard-coded \nemacsclient thingy. \n\nHowever, I really like Brian's \"advice\" suggestion. Would you be more\nin favor of this change if we don't do emacsclient hardcoding and rely on \n\"advice.openEditor\"? Maybe we could also remove the term trickery but\nthis seems to be convenient in practice. \n\nAccording to the docs advice.* defaults to \"true\". For my use case it would\nbe ok if \"advice.openEditor\" would default to \"false\" as I distribute\na common Git config via \"include.path\" to my users. However, that is likely\nconfusing to the \"advice\" machinery and users.\n\n\n> The right place for this message, IMHO, is for the editor itself (or a\n> wrapper script) to say \"hey, I'm opening a new window\" (like emacsclient\n> does).\n\nI 100% agree. However, as you mentioned, the world isn't perfect.\n\nGit's core concepts are pretty simple and most people understand them\nif you explain them visually. However, applying/using these concepts\nis often the problem. If you want to use Git efficiently then you need\nto know lots of other things. Being command-line savvy is one of them.\nFrom experience I know that this is hard for many people. Especially\nWindows users as they are not used to BASH and Unix-like command-line\ntools. That's why I think \"advice.openEditor\" could help a few people.\n\n- Lars"},{"id":"333687","messageId":"B7DA73AD-CD9E-4EB1-AD6D-AAF2B11F93D8@gmail.com","threadId":"47328","inReplyTo":"xmqqefojb9a1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-11-28T12:39:57Z","receivedAt":"2017-11-28T12:40:06Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 28 Nov 2017, at 00:18, Junio C Hamano wrote:\n> \n> lars.schneider@autodesk.com writes:\n> \n>> diff to v2:\n>> - shortened and localized the \"waiting\" message\n>> - detect \"emacsclient\" and suppress \"waiting\" message\n> \n> Thanks for moving this forward.\n> \n> \n>> +\t\tstatic const char *close_notice = NULL;\n> \n> Because this thing is \"static\", the variable is NULL when the first\n> call to this function is made, and the value left in the variable\n> when a call returns will be seen by the next call.\n> \n>> +\t\tif (isatty(2) && !close_notice) {\n> \n> Declaring a \"static\" variable initialized to NULL and checking its\n> NULL-ness upfront is a common pattern to make sure that the code\n> avoids repeated computation of the same thing. The body of the if\n> statement is run only when standard error stream is a tty (hinting\n> an interactive session) *and* close_notice is (still) NULL.\n> \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 if (term && strstr(term, \"emacsclient\"))\n>> +\t\t\t\t/*\n>> +\t\t\t\t * `emacsclient` (or `emacsclientw` on Windows) already prints\n>> +\t\t\t\t * (\"Waiting for Emacs...\") if a file is opened for editing.\n>> +\t\t\t\t * Therefore, we don't need to print the editor launch info.\n>> +\t\t\t\t */\n>> +\t\t\t\t;\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> It assigns a non-NULL value to close_notice unless the editor is\n> emacsclient (modulo the bug that \"emacsclient\" is to be compared\n> with EDITOR, GIT_EDITOR, core.editor etc. -- git_editor() can be\n> used to pick the right one). For a user of that particular editor,\n> it is left as NULL. Because it is unlikely that EDITOR etc. would\n> change across calls to this function, for them, and only for them,\n> the above is computed to yield the same result every time this\n> function is called.\n> \n> That feels a bit uneven, doesn't it?\n> \n> There are two possible solutions:\n> \n> 1. drop \"static\" from the declaration to stress the fact that the\n> variable and !close_notice in the upfront if() statement is not\n> avoiding repeated computation of the same thing, or\n> \n> 2. arrange that \"emacsclient\" case also participates in \"avoid\n> repeated computation\" dance. While at it, swap the order of\n> checking isatty(2) and !close_notice (aka \"have we done this\n> already?)--because we do not expect us swapping file descriptor\n> #2 inside this single process, we'd be repeatedly asking\n> isatty(2) for the same answer.\n> \n> The former may be simpler and easier, as an editor invocation would\n> not be a performance critical codepath.\n> \n> If we want to do the latter, a cleaner way may be to have another\n> static \"static int use_notice_checked = 0;\" declared, and then\n> \n> \tif (!use_notice_checked && isatty(2)) {\n> \t\t... what you currently have, modulo the\n> \t\t... fix for the editor thing, and set\n> \t\t... close_notice to a string (or NULL).\n> use_notice_checked = 1;\n> \t}\n> \n> The users of close_notice after this part that use !close_notice\n> as \"do not give the notice at all\" flag and also as \"this is the\n> string to show after editor comes back\" can stay the same if you go\n> this route. That would be solution 2a.\n> \n> Of course, you can instead use close_notice = \"\" (empty string) as a\n> signal \"we checked and we know that we are not using the notice\n> thing\". If you go that route, then the users after this if statement\n> that sets up close_notice upon the first call would say !*close_notice\n> instead of !close_notice when they try to see if the notice is in use.\n> That would be solution 2b.\n> \n> I personally think any one of 1., 2a., or 2b. is fine.\n\nThanks for your thoughts! I will go with 1. I also think that this is \nno performance critical codepath and therefore we should go with the\nsimplest/most maintainable solution.\n\n\nThanks,\nLars\n"},{"id":"333759","messageId":"20171129020616.GC598034@genre.crustytoothpaste.net","threadId":"47328","inReplyTo":"20171127230520.GA29636@sigill.intra.peff.net","subject":"Re: [PATCH v3] launch_editor(): indicate that Git waits for user input","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-11-29T02:06:16Z","receivedAt":"2017-11-29T02:06:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Nov 27, 2017 at 06:05:20PM -0500, Jeff King wrote:\n> On Mon, Nov 27, 2017 at 08:09:32PM +0000, brian m. carlson wrote:\n> \n> > > Show a message in the original terminal and get rid of it when the\n> > > editor returns.\n> > [...]\n> > \n> > Sorry for coming to the topic so late, but it occurred to me that we\n> > might want to conditionalize this on an advice.* flag. I expect there\n> > are some people who will never want to see this, and letting them turn\n> > it off would be good.\n> \n> I am torn between saying \"yes please, I would absolutely set such an\n> option myself\" and \"if we need advice.*, that is a good sign that the\n> feature is mis-designed\".\n\nI myself would also set such an option. More importantly, I think there\nare other developers who would complain about such a message, and I'd\nlike to give them an easy way to turn it off.\n\nNote that I am not altogether opposed to advice.*, since I personally\nfind it helpful in certain cases as an aide-mémoire of what state my\nbranch is in.\n\n> Let me elaborate a bit on the latter.\n> \n> My gut feeling is that this is absolutely the wrong place to put a\n> message like this. We don't know enough about what the editor is doing,\n> so we have to take pains to avoid a crufty message in the terminal,\n> including:\n> \n> - playing ANSI-term trickery to erase the message\n> \n> - hard-coding (!) emacsclient as a special case\n\nI agree that the editor is the right place to put this, but I also\nunderstand that the people most likely to be helped by this are the\nleast likely to write such scripting. I think this is especially so\nbecause in my experience newer, less advanced users are more likely than\nnot to use graphical editors.\n\nGit is an extraordinarily powerful piece of software, but it's also hard\nfor people to use (judging by my Twitter feed), so if we can make it\nless painful for new users, I'm okay with an advice.* setting.\n\nI'm slightly negative on hard-coding emacsclient, since I'm sure someone\nwill come up with another editor that does the same thing, and then we'd\nhave to update it.\n\n> If the anti-cruft techniques I mentioned above work well in practice,\n> then we get to have our cake and eat it, too. If they don't, then I'm\n> not sure if the tradeoff is worth it.\n\nI have a feeling that this may not work properly in editors that don't\nsupport the concept of xterm alternate screens (such as the Linux\nconsole) where the editor window is left on the screen, but it may be\nthat it works fine and I've just misunderstood how it's supposed to\nwork. It may also be that we don't care about such cases, as any cruft\nwould have already scrolled off the screen.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"}]}