Rubén Justo <rjusto@xxxxxxxxx> 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@xxxxxxxxxxxx> is my
<20171127134716.69471-1-lars.schneider@xxxxxxxxxxxx>, 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.
