Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Dec 1, 2017, 12:52 UTC
- Message-ID
- <BB878570-F214-4BAA-AB58-4A9C0F8F80AB@gmail.com>
- In-Reply-To
- <20171130205137.GC3313@sigill.intra.peff.net>
Show 37 quoted lines
> On 30 Nov 2017, at 21:51, Jeff King <peff@peff.net> wrote: > > On Wed, Nov 29, 2017 at 03:37:52PM +0100, lars.schneider@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