Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input

On 30 Nov 2017, at 21:51, Jeff King [off-list ref] wrote:

On Wed, Nov 29, 2017 at 03:37:52PM +0100, lars.schneider@autodesk.com wrote:
...

quoted

The standard advise() function is not used here as it would always add
a newline which would make deleting the message harder.

I tried to think of ways this "show a message and then delete it" could
go wrong. It should work OK with editors that just do curses-like
things, taking over the terminal and then restoring it at the end.

It does behave in a funny way if the editor produces actual lines of
output outside of the curses handling. E.g. (I just quit vim
immediately, hence the aborting message):

 $ GIT_EDITOR='echo foo; vim' git commit
 hint: Waiting for your editor input...foo
 Aborting commit due to empty commit message.

our "foo" gets tacked onto the hint line, and then our deletion does
nothing (because the newline after "foo" bumped us to a new line, and
there was nothing on that line to erase).

An even worse case (and yes, this is really reaching) is:

 $ GIT_EDITOR='echo one; printf "two\\r"; vim' git commit
 hint: Waiting for your editor input...one
 Aborting commit due to empty commit message.

There we ate the "two" line.

These are obviously the result of devils-advocate poking at the feature.
I doubt any editor would end its output with a CR. But the first case is
probably going to be common, especially for actual graphical editors. We
know that emacsclient prints its own line, and I wouldn't be surprised
if other graphical editors spew some telemetry to stderr (certainly
anything built against GTK tends to do so).

True! However, if a Git user is not bothered by a graphical editor that
spews telemetry to stderr, then I think the same user wouldn't be
bothered by one additional line printed by Git either, right?

The patch itself looks fine, as far as correctly implementing the
design.

Thanks for the review :-)

- Lars