From: Junio C Hamano <gitster@pobox.com>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH v2] launch_editor: waiting message on error
Date: Mon, 08 Apr 2024 18:27:57 -0700 [thread overview]
Message-ID: <xmqq4jcb495u.fsf@gitster.g> (raw)
In-Reply-To: <0258a583-a90a-4434-bb4e-a1672d574b9c@gmail.com> ("Rubén Justo"'s message of "Tue, 9 Apr 2024 01:09:21 +0200")
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.
next prev parent reply other threads:[~2024-04-09 1:28 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-08 21:02 [PATCH] launch_editor: waiting message on error Rubén Justo
2024-04-08 21:07 ` Rubén Justo
2024-04-08 23:09 ` [PATCH v2] " Rubén Justo
2024-04-09 1:27 ` Junio C Hamano [this message]
2024-04-09 23:38 ` Rubén Justo
2024-04-10 15:44 ` Junio C Hamano
2024-04-11 23:18 ` Rubén Justo
2024-04-12 15:46 ` Junio C Hamano
2024-04-12 17:03 ` Rubén Justo
2024-04-12 17:35 ` Junio C Hamano
2024-04-12 18:24 ` Rubén Justo
2024-04-12 17:05 ` [PATCH v3 0/2] launch_editor: waiting message Rubén Justo
2024-04-12 17:15 ` [PATCH v3 1/2] launch_editor: waiting for editor message Rubén Justo
2024-04-12 17:24 ` rsbecker
2024-04-12 17:37 ` Rubén Justo
2024-04-12 17:47 ` rsbecker
2024-04-13 15:06 ` Phillip Wood
2024-04-12 17:15 ` [PATCH v3 2/2] launch_editor: waiting message on error Rubén Justo
2024-04-13 15:09 ` Phillip Wood
2024-04-14 7:23 ` Rubén Justo
2024-04-14 7:39 ` [PATCH v4] " Rubén Justo
2024-04-15 14:05 ` Phillip Wood
2024-04-15 17:03 ` Rubén Justo
2024-04-15 17:20 ` Junio C Hamano
2024-04-15 17:07 ` Rubén Justo
2024-04-15 18:44 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4jcb495u.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).