[PATCH] launch_editor: waiting message on error

[PATCH] launch_editor: waiting message on error

[Rubén Justo]

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 our next
"error: There was a problem with the..." more clear.  For example, the
editor could print messages that would be mixed with our error message.
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>
---
 editor.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/editor.c b/editor.c
index b67b802ddf..a658090a65 100644
--- a/editor.c
+++ b/editor.c
@@ -104,16 +104,25 @@ 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 (print_waiting_for_editor && !is_terminal_dump())
+			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'",
+					editor);
 	}
 
 	if (!buffer)
-- 
2.43.0

[PATCH] launch_editor: waiting message on error

[Rubén Justo]

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 out next
"error: There was a problem with the..." more clear.  For example, the
editor could print messages that would be mixed with our error message.
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>
---
 editor.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/editor.c b/editor.c
index b67b802ddf..0d4c6f94a6 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 (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 (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'",
+					editor);
 	}
 
 	if (!buffer)
-- 
2.43.0

[PATCH v2] launch_editor: waiting message on error

[Rubén Justo]

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(-)

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)
-- 
2.44.0.502.g3e6838d230

Re: [PATCH v2] launch_editor: waiting message on error

[Junio C Hamano]

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.

Re: [PATCH v2] launch_editor: waiting message on error

[Rubén Justo]

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.

Re: [PATCH v2] launch_editor: waiting message on error

[Junio C Hamano]

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.

Re: [PATCH v2] launch_editor: waiting message on error

[Rubén Justo]

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.

Re: [PATCH v2] launch_editor: waiting message on error

[Junio C Hamano]

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?

Re: [PATCH v2] launch_editor: waiting message on error

[Rubén Justo]

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.

Re: [PATCH v2] launch_editor: waiting message on error

[Junio C Hamano]

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.

Re: [PATCH v2] launch_editor: waiting message on error

[Rubén Justo]

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

[PATCH v3 0/2] launch_editor: waiting message

[Rubén Justo]

Improve the hint message we have in editor.c.

Rubén Justo (2):
  launch_editor: waiting for editor message
  launch_editor: waiting message on error

 editor.c | 54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

-- 
2.44.0.771.gbd07cf668b

[PATCH v3 1/2] launch_editor: waiting for editor message

[Rubén Justo]

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(-)

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);
-- 
2.44.0.771.gbd07cf668b

RE: [PATCH v3 1/2] launch_editor: waiting for editor message

[rsbecker]

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

Re: [PATCH v3 1/2] launch_editor: waiting for editor message

[Rubén Justo]

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.

RE: [PATCH v3 1/2] launch_editor: waiting for editor message

[rsbecker]

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.

Re: [PATCH v3 1/2] launch_editor: waiting for editor message

[Phillip Wood]

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
> 
> 
> 

[PATCH v3 2/2] launch_editor: waiting message on error

[Rubén Justo]

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.

While we're here, make the error message follow our CodingGuideLines.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 editor.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/editor.c b/editor.c
index 1da3a26f5d..eb0cfe4a28 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)
-- 
2.44.0.771.gbd07cf668b

Re: [PATCH v3 2/2] launch_editor: waiting message on error

[Phillip Wood]

Hi Rubén

On 12/04/2024 18:15, 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 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.

I think it is very likely that the editor has printed an error message 
if it exits with a non-zero exit code and if that message does not end 
with a newline that is a bug in the editor. Do you have a real-world 
example of the problem you are seeking to fix?

Best Wishes

Phillip

> 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.
> 
> While we're here, make the error message follow our CodingGuideLines.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   editor.c | 26 ++++++++++++++++++--------
>   1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/editor.c b/editor.c
> index 1da3a26f5d..eb0cfe4a28 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)

Re: [PATCH v3 2/2] launch_editor: waiting message on error

[Rubén Justo]

On Sat, Apr 13, 2024 at 04:09:27PM +0100, Phillip Wood wrote:

> I think it is very likely that the editor has printed an error message if it
> exits with a non-zero exit code and if that message does not end with a
> newline that is a bug in the editor. Do you have a real-world example of the
> problem you are seeking to fix?

Perhaps I am being too cautious.  I'll follow Junio's suggestion and use 
term_clear_line() in all cases.

Thanks.

[PATCH v4] launch_editor: waiting message on error

[Rubén Justo]

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(-)

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)
-- 
2.44.0.782.g038d277106

Re: [PATCH v4] launch_editor: waiting message on error

[Phillip Wood]

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)

Re: [PATCH v4] launch_editor: waiting message on error

[Rubén Justo]

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.

Re: [PATCH v4] launch_editor: waiting message on error

[Junio C Hamano]

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.

Re: [PATCH v4] launch_editor: waiting message on error

[Rubén Justo]

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.

Re: [PATCH v4] launch_editor: waiting message on error

[Junio C Hamano]

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.