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

From: Rubén Justo <rjusto@gmail.com>
Date: Apr 11, 2024, 23:18 UTC
Message-ID: <d485c4cd-f963-45b7-9fa6-801738c7c066@gmail.com>
In-Reply-To: <xmqqle5lxlwm.fsf@gitster.g>

On Wed, Apr 10, 2024 at 08:44:25AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

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

Show 18 quoted lines

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