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