| Message ID | 83b34572-498b-438c-8437-dfbb837e60ba@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | launch_editor: waiting message | expand |
On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote: >Subject: [PATCH v3 1/2] launch_editor: waiting for editor message > >We have a hint shown when we are waiting for user's editor since >abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07). > >After showing the hint, we call start_command() which can return with an error. >Then we'll show "unable to start editor...", after having said "Waiting for your >editor...", which may be confusing. > >Move the code to show the hint below the start_command(). My thought on this move is for esoteric (but commonly used) terminal emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for example, the emulator switches modes from text on the POSIX side to block/full screen mode when the editor is launched. Printing a message after the editor has launched has the potential to dump the message into the terminal emulation buffer and get caught in the commit text comment. This is not desirable. This change could have seriously undesirable side-effects. On the other side, if the message is not displayed in the emulation buffer, it is deferred until after the editor closes, which makes the message a bit pointless. --Randall
On Fri, Apr 12, 2024 at 01:24:55PM -0400, rsbecker@nexbridge.com wrote: > On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote: > >Subject: [PATCH v3 1/2] launch_editor: waiting for editor message > > > >We have a hint shown when we are waiting for user's editor since > >abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07). > > > >After showing the hint, we call start_command() which can return with an error. > >Then we'll show "unable to start editor...", after having said "Waiting for your > >editor...", which may be confusing. > > > >Move the code to show the hint below the start_command(). > > My thought on this move is for esoteric (but commonly used) terminal > emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for > example, the emulator switches modes from text on the POSIX side to > block/full screen mode when the editor is launched. Printing a message > after the editor has launched has the potential to dump the message > into the terminal emulation buffer and get caught in the commit text > comment. This is not desirable. This change could have seriously > undesirable side-effects. That's a good point. Thanks for bringing it up. Of course, in such a situation the user has the opportunity to disable the hint. However, can you think of a way in which we could do this, not showing the "Waiting..." before the "unable to start", better? Thanks.
On Friday, April 12, 2024 1:37 PM, Rubén Justo wrote: >On Fri, Apr 12, 2024 at 01:24:55PM -0400, rsbecker@nexbridge.com wrote: >> On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote: >> >Subject: [PATCH v3 1/2] launch_editor: waiting for editor message >> > >> >We have a hint shown when we are waiting for user's editor since >> >abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07). >> > >> >After showing the hint, we call start_command() which can return with an error. >> >Then we'll show "unable to start editor...", after having said >> >"Waiting for your editor...", which may be confusing. >> > >> >Move the code to show the hint below the start_command(). >> >> My thought on this move is for esoteric (but commonly used) terminal >> emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for >> example, the emulator switches modes from text on the POSIX side to >> block/full screen mode when the editor is launched. Printing a message >> after the editor has launched has the potential to dump the message >> into the terminal emulation buffer and get caught in the commit text >> comment. This is not desirable. This change could have seriously >> undesirable side-effects. > >That's a good point. Thanks for bringing it up. > >Of course, in such a situation the user has the opportunity to disable the hint. > >However, can you think of a way in which we could do this, not showing the >"Waiting..." before the "unable to start", better? I do not have a good solution. One thought was to run the Waiting message in a separate thread, but that is dangerous. Terminal I/O APIs are generally not thread aware and random results are frequent when writing from two threads, particularly in different processes. Polluting stdout is never a good idea, and in this case the encoding and terminal type could also change between git and editor, even in Linux. The only potential way to do this is with an editor aware mutex (there isn't a portable on) that would block the editor, poll the terminal state, change to UTF-8 or US-ASCII or... and write the Waiting message, switch back, then release the mutex.
On 12/04/2024 18:24, rsbecker@nexbridge.com wrote: > On Friday, April 12, 2024 1:15 PM, Rubén Justo wrote: >> Subject: [PATCH v3 1/2] launch_editor: waiting for editor message >> >> We have a hint shown when we are waiting for user's editor since >> abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07). >> >> After showing the hint, we call start_command() which can return with an error. >> Then we'll show "unable to start editor...", after having said "Waiting for your >> editor...", which may be confusing. >> >> Move the code to show the hint below the start_command(). > > My thought on this move is for esoteric (but commonly used) terminal emulators. If one is on a t6530, tn3270, or w3270/9 emulator, for example, the emulator switches modes from text on the POSIX side to block/full screen mode when the editor is launched. Printing a message after the editor has launched has the potential to dump the message into the terminal emulation buffer and get caught in the commit text comment. This is not desirable. This change could have seriously undesirable side-effects. Writing to the terminal after starting the editor is a bad idea for the reason you describe regardless of the terminal type. I don't think there is a sensible way to avoid showing the hint before we know whether the editor started successfully or not. > On the other side, if the message is not displayed in the emulation buffer, it is deferred until after the editor closes, which makes the message a bit pointless. I think the message is there for gui editors, with a terminal editor it is obvious that git has started the editor and the user doesn't really see the message because it is cleared when the editor exits successfully. Best Wishes Phillip > --Randall > > >
diff --git a/editor.c b/editor.c index b67b802ddf..1da3a26f5d 100644 --- a/editor.c +++ b/editor.c @@ -64,9 +64,21 @@ static int launch_specified_editor(const char *editor, const char *path, if (strcmp(editor, ":")) { struct strbuf realpath = STRBUF_INIT; struct child_process p = CHILD_PROCESS_INIT; - int ret, sig; - int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2); + int ret, sig, print_waiting_for_editor; + strbuf_realpath(&realpath, path, 1); + + strvec_pushl(&p.args, editor, realpath.buf, NULL); + if (env) + strvec_pushv(&p.env, (const char **)env); + p.use_shell = 1; + p.trace2_child_class = "editor"; + if (start_command(&p) < 0) { + strbuf_release(&realpath); + return error("unable to start editor '%s'", editor); + } + + print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2); if (print_waiting_for_editor) { /* * A dumb terminal cannot erase the line later on. Add a @@ -83,18 +95,6 @@ static int launch_specified_editor(const char *editor, const char *path, fflush(stderr); } - strbuf_realpath(&realpath, path, 1); - - strvec_pushl(&p.args, editor, realpath.buf, NULL); - if (env) - strvec_pushv(&p.env, (const char **)env); - p.use_shell = 1; - p.trace2_child_class = "editor"; - if (start_command(&p) < 0) { - strbuf_release(&realpath); - return error("unable to start editor '%s'", editor); - } - sigchain_push(SIGINT, SIG_IGN); sigchain_push(SIGQUIT, SIG_IGN); ret = finish_command(&p);
We have a hint shown when we are waiting for user's editor since abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07). After showing the hint, we call start_command() which can return with an error. Then we'll show "unable to start editor...", after having said "Waiting for your editor...", which may be confusing. Move the code to show the hint below the start_command(). Signed-off-by: Rubén Justo <rjusto@gmail.com> --- editor.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)