| Message ID | 0258a583-a90a-4434-bb4e-a1672d574b9c@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] launch_editor: waiting message on error | expand |
Rubén Justo <rjusto@gmail.com> writes: > However, even with a short message, feeding that LF makes the following > "error: There was a problem with the..." clearer, separating it from > possible messages that the editor could have printed. So, add that LF. Sounds sensible. > + if (print_waiting_for_editor && !is_terminal_dumb()) { > + if (!ret) > + /* > + * Erase the entire line to avoid wasting > + * the vertical space. > + */ > + term_clear_line(); I know this was inherited from the original, but overly verbose comment is not being very useful here. > + else > + /* > + * We don't want term_clear_line() here > + * because the editor could have written > + * some useful messages to the user. > + */ > + fprintf(stderr, "\n"); But I do not think this is emitting the newline at the right place. The sequence would be (1) we say "we are waiting" on an incomplete line, and then (2) the editor may say "There was a problem" without first adding LF _before_ saying so. Isn't adding a LF here too late to let the editor emit its message on its own line, instead of having it _after_ the short "hint" message? Of course, after these two messages (one from us, and then the error message from the editor) concatenated on the same line, we would want to have the next error message on its own line, but do we need to add an extra newline here for that purpose? Unlike our "hint: we are waiting" that we fully intend to clean-up by using term_clear_line(), the editor that exits upon failure has no reason to keep its final error message "There was a problem" on an incomplete line without emitting the terminating LF before giving control back to us. The "I do not know if it is bad enough to have these two on the same line" you seem to refer to indirectly by citing Lars's message <20171127134716.69471-1-lars.schneider@autodesk.com> is my <20171127134716.69471-1-lars.schneider@autodesk.com>, I think. But in that utterance, "these two" refers to "hint: we are waiting..." and whatever the message the editor emits upon seeing an error. The suggestion I made 7 years ago has nothing to do with the behaviour change this patch is making. I think the code is doing the right thing. It is doing something different from what the proposed commit log message said it is doing. Let me try to summarize what I think this patch does: 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 successfully. 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. Only clear the line when the editor returned cleanly, and otherwise, complete the message on the incomplete line with a newline before giving the error message. Hopefully the above is a more reasonable explanation of what is happening in this patch, I think? Actually, having thought it through in order to write the above explanation, I wonder if we can just call term_clear_line() regardless of the value of ret. Either case, the waiting is already over and in the error case, we show another message after it. There is another error message when we fail to start the editor. Doesn't that codepath have the same problem? I wonder: - moving the code to show "hint" down below start_command() where it could return error("unable to start"); - moving the "if (ret) return error("There was a problem")" after the block that calls term_clear_line(); would be a better and sufficient fix? Thanks.
On Mon, Apr 08, 2024 at 06:27:57PM -0700, Junio C Hamano wrote: > I wonder if we can just call term_clear_line() > regardless of the value of ret. Either case, the waiting is already > over and in the error case, we show another message after it. My concern is that perhaps term_clear_line() might clear some useful information for the user. Although I am not sure that this concern is sensible. Stepping back a bit, how painful it would be to drop the term_clear_line() and start using advice_if_enabled() here? This is what I'm thinking about now. $ GIT_EDITOR=false git commit -a hint: A-good-explanation-to-say-we-run-'editor' hint: Disable this message with "git config advice.waitingForEditor false" error: There was a problem with the editor 'false'. Please supply the message using either -m or -F option. > There is another error message when we fail to start the editor. > Doesn't that codepath have the same problem? Of course. My itch is: $ 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. But, yes, while we're here we can also fix: $ 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.
Rubén Justo <rjusto@gmail.com> writes: > On Mon, Apr 08, 2024 at 06:27:57PM -0700, Junio C Hamano wrote: > >> I wonder if we can just call term_clear_line() >> regardless of the value of ret. Either case, the waiting is already >> over and in the error case, we show another message after it. > > My concern is that perhaps term_clear_line() might clear some useful > information for the user. Although I am not sure that this concern is > sensible. It happens ONLY when the error message the editor itself emits (which comes later on the same line as "We are waiting for your editor...") without terminating newline itself. Otherwise, we'd have We are waiting ... THE EDITOR SAYS I FAILED _ on the screen, and the cursor is at the _ position. term_clear_line() would not clear anything. > Stepping back a bit, how painful it would be to drop the > term_clear_line() and start using advice_if_enabled() here? > > This is what I'm thinking about now. > > $ GIT_EDITOR=false git commit -a > hint: A-good-explanation-to-say-we-run-'editor' > hint: Disable this message with "git config advice.waitingForEditor false" > error: There was a problem with the editor 'false'. > Please supply the message using either -m or -F option. I do not think the problem is in the case where the editor immediately exits with an error. It is when the editor opens elsewhere (or more likely, opens another tab to let you edit in an existing GUI editor session, but the editor's window is buried under other windows) and your "git commit" will stay silently without giving you back a terminal prompt, leaving you wondering why it is taking so much time. So I am not sure if the advice mechanism is a good fit. >> There is another error message when we fail to start the editor. >> Doesn't that codepath have the same problem? > > Of course. > > My itch is: > > $ 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. I do not think we want to encourage "-m" when the end user did not say so. Instead we should let them fix their editor to keep them more productive. Thanks.
On Wed, Apr 10, 2024 at 08:44:25AM -0700, Junio C Hamano wrote: > > My concern is that perhaps term_clear_line() might clear some useful > > information for the user. Although I am not sure that this concern is > > sensible. > > It happens ONLY when the error message the editor itself emits > (which comes later on the same line as "We are waiting for your > editor...") without terminating newline itself. Otherwise, we'd > have > > We are waiting ... THE EDITOR SAYS I FAILED > _ > > on the screen, and the cursor is at the _ position. term_clear_line() > would not clear anything. Not with a careless editor: --- >8 --- #include <stdio.h> #include <stdlib.h> int main(void) { fprintf("editing the file..."); exit(1); /* unexpected exit */ fprintf("\n"); return 0; } --- 8< --- > > Stepping back a bit, how painful it would be to drop the > > term_clear_line() and start using advice_if_enabled() here? > > > > This is what I'm thinking about now. > > > > $ GIT_EDITOR=false git commit -a > > hint: A-good-explanation-to-say-we-run-'editor' > > hint: Disable this message with "git config advice.waitingForEditor false" > > error: There was a problem with the editor 'false'. > > Please supply the message using either -m or -F option. > > I do not think the problem is in the case where the editor > immediately exits with an error. It is when the editor opens > elsewhere (or more likely, opens another tab to let you edit in an > existing GUI editor session, but the editor's window is buried under > other windows) and your "git commit" will stay silently without > giving you back a terminal prompt, leaving you wondering why it is > taking so much time. Yes ... > > So I am not sure if the advice mechanism is a good fit. and I'm not sure either. However, two things that I like: by using the advice mechanism we avoid the term_clear_line() and we advertise better the knob. I discovered advice.waitingForEditor while inspecting the code. Using the advice mechanism here is just a thought. > > $ 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. > I do not think we want to encourage "-m" when the end user did not > say so. Instead we should let them fix their editor to keep them > more productive. That message can be traced back to 62e09ce998 (Make git tag a builtin., 2007-07-20). My understanding is that we suggest -m or -F because using the editor failed. Are you saying we should stop giving that "Please supply ..."? I see that message when I'm editing a commit message and decide to abort the commit by making the editor end with an error. So, that message is misleading to me. Therefore I'm fine with removing that message, however, I'm not sure that's what you're suggesting. At any rate, this is tangential to this series.
Rubén Justo <rjusto@gmail.com> writes: > On Wed, Apr 10, 2024 at 08:44:25AM -0700, Junio C Hamano wrote: >... >> It happens ONLY when the error message the editor itself emits >> (which comes later on the same line as "We are waiting for your >> editor...") without terminating newline itself. Otherwise, we'd >> have >> >> We are waiting ... THE EDITOR SAYS I FAILED >> _ >> >> on the screen, and the cursor is at the _ position. term_clear_line() >> would not clear anything. > > Not with a careless editor: That is why I said "Otherwise". Of course, a broken editor would give broken output. What else is new? And more importantly, if you wrote such an editor, would you release it in such a buggy form to the outside world? Does it still look like a problem worth spending our brain cycles on?
On Fri, Apr 12, 2024 at 08:46:03AM -0700, Junio C Hamano wrote: > >> It happens ONLY when the error message the editor itself emits > >> (which comes later on the same line as "We are waiting for your > >> editor...") without terminating newline itself. Otherwise, we'd > >> have > >> > >> We are waiting ... THE EDITOR SAYS I FAILED > >> _ > >> > >> on the screen, and the cursor is at the _ position. term_clear_line() > >> would not clear anything. > > > > Not with a careless editor: > > That is why I said "Otherwise". Of course, a broken editor would > give broken output. What else is new? And more importantly, if you > wrote such an editor, would you release it in such a buggy form to > the outside world? Does it still look like a problem worth spending > our brain cycles on? > Yes, but I also see it from another perspective; I don't want to worry about a possible inconvenience. And since it is perhaps an unexpected precaution, for a future reviewer, hence the explicit comment in the code.
Rubén Justo <rjusto@gmail.com> writes: > Yes, but I also see it from another perspective; I don't want to worry > about a possible inconvenience. And since it is perhaps an unexpected > precaution, for a future reviewer, hence the explicit comment in the > code. But then the comment should say it only matters if the editor left its message incomplete, shouldn't it? If the editor did the right thing and terminated its message before it exits with a newline, the extra LF we emit after it will only waste the vertical screen real estate.
On Fri, Apr 12, 2024 at 10:35:38AM -0700, Junio C Hamano wrote: > > Yes, but I also see it from another perspective; I don't want to worry > > about a possible inconvenience. And since it is perhaps an unexpected > > precaution, for a future reviewer, hence the explicit comment in the > > code. > > But then the comment should say it only matters if the editor left > its message incomplete, shouldn't it? If the editor did the right > thing and terminated its message before it exits with a newline, the > extra LF we emit after it will only waste the vertical screen real > estate. Not sure if that needs to be noted in the comment. This, and the other point raised by Randall [1], certainly makes me more in favor of using the advise_if_enabled(). Instead of "Waiting...", using a message such as "Started..." can be just as good for user guidance and less prone to error. I think the v3 I posted is an improvement. But I believe we should consider moving towards using the advise API here, at some point. [1] https://lore.kernel.org/git/96bef5f9-1286-4938-99ec-6beed13ee68d@gmail.com/T/#m906fca9d24baf343326e134ac08370a77d69a603
diff --git a/editor.c b/editor.c index b67b802ddf..8f224747d9 100644 --- a/editor.c +++ b/editor.c @@ -104,16 +104,26 @@ static int launch_specified_editor(const char *editor, const char *path, sigchain_pop(SIGQUIT); if (sig == SIGINT || sig == SIGQUIT) raise(sig); + + if (print_waiting_for_editor && !is_terminal_dumb()) { + if (!ret) + /* + * Erase the entire line to avoid wasting + * the vertical space. + */ + term_clear_line(); + else + /* + * We don't want term_clear_line() here + * because the editor could have written + * some useful messages to the user. + */ + fprintf(stderr, "\n"); + } + if (ret) - return error("There was a problem with the editor '%s'.", + 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 (!buffer)
We have the hint we're touching in this commit since abfb04d0c7 (launch_editor(): indicate that Git waits for user input, 2017-12-07). Adding a new line after the hint when the editor returns error was discussed in the list, but finally it was considered not necessary because a shorter message is used [1]. However, even with a short message, feeding that LF makes the following "error: There was a problem with the..." clearer, separating it from possible messages that the editor could have printed. So, add that LF. While we're here, make the error message follow our CodingGuideLines. [1] https://public-inbox.org/git/20171127134716.69471-1-lars.schneider@autodesk.com/T/#u Signed-off-by: Rubén Justo <rjusto@gmail.com> --- This v2 fixes some whitespaces I didn't notice. Sorry for the mess. editor.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)