> On 30 Nov 2017, at 21:51, Jeff King <p...@peff.net> wrote:
> 
> On Wed, Nov 29, 2017 at 03:37:52PM +0100, lars.schnei...@autodesk.com wrote:
> ...
>> 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

Reply via email to