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

Rubén Justo <rjusto@gmail.com> writes:

Sounds sensible.

I know this was inherited from the original, but overly verbose comment is not being very useful here.

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.