| Message ID | e208da74-8f16-44ae-912e-ae968da82057@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 48e1ca27b1fed9d71779bf93d524b4aed9d66da3 |
| Headers | show |
| Series | [v4] launch_editor: waiting message on error | expand |
Hi Rubén On 14/04/2024 08:39, Rubén Justo wrote: > When advice.waitingForEditor configuration is not set to false, we show > a hint telling that we are waiting for user's editor to close the file > when we launch an editor and wait for it to return control back to us. > We give the message on an incomplete line, expecting that we can go back > to the line and clear the message when the editor returns. > > However, it is possible that the editor exits with an error status, in > which case we show an error message and then return to our caller. In > such a case, the error message is given where the terminal cursor > happens to be, which is most likely after the "we are waiting for your > editor" message on the same line. As I've said before I'm not sure how likely that is as I think the editor will probably have printed a message if there was an error. Assuming the message from the editor ends with a newline the proposed change wont do any harm so I don't object to it. Best Wishes Phillip > Clear the line before showing the error. > > While we're here, make the error message follow our CodingGuideLines. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > editor.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/editor.c b/editor.c > index b67b802ddf..d1ba2d7c34 100644 > --- a/editor.c > +++ b/editor.c > @@ -104,16 +104,15 @@ static int launch_specified_editor(const char *editor, const char *path, > sigchain_pop(SIGQUIT); > if (sig == SIGINT || sig == SIGQUIT) > raise(sig); > - if (ret) > - return error("There was a problem with the editor '%s'.", > - editor); > - > if (print_waiting_for_editor && !is_terminal_dumb()) > /* > * Erase the entire line to avoid wasting the > * vertical space. > */ > term_clear_line(); > + if (ret) > + return error("there was a problem with the editor '%s'", > + editor); > } > > if (!buffer)
On Mon, Apr 15, 2024 at 03:05:32PM +0100, Phillip Wood wrote: > > However, it is possible that the editor exits with an error status, in > > which case we show an error message and then return to our caller. In > > such a case, the error message is given where the terminal cursor > > happens to be, which is most likely after the "we are waiting for your > > editor" message on the same line. > > As I've said before I'm not sure how likely that is as I think the editor > will probably have printed a message if there was an error. For instance, Vim with ":cq" does not emit any error message. > Assuming the > message from the editor ends with a newline the proposed change wont do any > harm so I don't object to it. Thanks.
On Sun, Apr 14, 2024 at 09:39:44AM +0200, Rubén Justo wrote: > When advice.waitingForEditor configuration is not set to false, we show > a hint telling that we are waiting for user's editor to close the file > when we launch an editor and wait for it to return control back to us. > We give the message on an incomplete line, expecting that we can go back > to the line and clear the message when the editor returns. > > However, it is possible that the editor exits with an error status, in > which case we show an error message and then return to our caller. In > such a case, the error message is given where the terminal cursor > happens to be, which is most likely after the "we are waiting for your > editor" message on the same line. > > Clear the line before showing the error. > > While we're here, make the error message follow our CodingGuideLines. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- The changes since v3 are: - dropped [v3 1/2] because, as noted by Randall and Phillip, it is not a good idea. The message stays like: $ GIT_EDITOR=falso git commit -a hint: Waiting for your editor to close the file... error: cannot run falso: No such file or directory error: unable to start editor 'falso' Please supply the message using either -m or -F option. The "error: unable to start..." at the beginning of the line makes it less prone to confusion than the other error message considered in this series. - term_clear_line() is now used in all cases as it is unlikely that any sane editor emits an error message without ending it with a newline. This: $ GIT_EDITOR=false git commit -a hint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'. Please supply the message using either -m or -F option. becomes: $ GIT_EDITOR=false git commit -a error: There was a problem with the editor 'false'. Please supply the message using either -m or -F option.
Phillip Wood <phillip.wood123@gmail.com> writes: > As I've said before I'm not sure how likely that is as I think the > editor will probably have printed a message if there was an > error. Assuming the message from the editor ends with a newline the > proposed change wont do any harm so I don't object to it. Yup, I think it is a no-op for a well-behaving editor that emits an error message with terminating newline. An editor that leaves its own error message incomplete, its error message together with our "we are waiting" will be erased together, but we probably do not care for such an editor. If an editor silently errors out, then this will still clear "we are waiting" we printed earlier and say "there was a problem", which is what we want to see happen. So, this looks good to me; let's queue it. Thanks, both.
Rubén Justo <rjusto@gmail.com> writes: > - term_clear_line() is now used in all cases as it is unlikely that any > sane editor emits an error message without ending it with a newline. > > This: > > $ GIT_EDITOR=false git commit -a > hint: Waiting for your editor to close the file... error: There was a problem with the editor 'false'. > Please supply the message using either -m or -F option. > > becomes: > > $ GIT_EDITOR=false git commit -a > error: There was a problem with the editor 'false'. > Please supply the message using either -m or -F option. Nobody uses 'false' as their editor, but as you said ':cq' in vim may be a real-world example of a use case that may benefit from this change. Will queue. Thanks.
diff --git a/editor.c b/editor.c index b67b802ddf..d1ba2d7c34 100644 --- a/editor.c +++ b/editor.c @@ -104,16 +104,15 @@ static int launch_specified_editor(const char *editor, const char *path, sigchain_pop(SIGQUIT); if (sig == SIGINT || sig == SIGQUIT) raise(sig); - if (ret) - return error("There was a problem with the editor '%s'.", - editor); - if (print_waiting_for_editor && !is_terminal_dumb()) /* * Erase the entire line to avoid wasting the * vertical space. */ term_clear_line(); + if (ret) + return error("there was a problem with the editor '%s'", + editor); } if (!buffer)
When advice.waitingForEditor configuration is not set to false, we show a hint telling that we are waiting for user's editor to close the file when we launch an editor and wait for it to return control back to us. We give the message on an incomplete line, expecting that we can go back to the line and clear the message when the editor returns. However, it is possible that the editor exits with an error status, in which case we show an error message and then return to our caller. In such a case, the error message is given where the terminal cursor happens to be, which is most likely after the "we are waiting for your editor" message on the same line. Clear the line before showing the error. While we're here, make the error message follow our CodingGuideLines. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- editor.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)