Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Mon, Dec 04, 2017 at 10:54:40PM +0100, Lars Schneider wrote: > > Better, IMHO, though I still think literally saying: > > > > hint: Waiting for your editor to exit... > > > > is the most accurate, which I think makes it clear that you must _exit_ > > your editor, not just save and close the file. > > I think "exit" would be confusing because most graphical editors (Sublime, > Textmate, Notepad++, ...) can open multiple files and do not need to exit. > The requirement is indeed save and close the file. > > How about: > > hint: Waiting for your editor to close the file... > > I generally like that as this is technical correct from all angles. > My only nit would be that "the file" is a bit imprecise... but > that's probably no problem. OK, that makes sense. There are two definitions of "exit" here. We care about the process exiting, but of course the editor may present a concept of "exiting" to the user that is different. I know emacsclient behaves that way (the client process exits when the buffer is closed, even though the rest of emacs may still be running), but I didn't realize other editors did so. I know atom doesn't, but then it also exits immediately, making it inappropriate for $GIT_EDITOR in the first place. -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
> On 04 Dec 2017, at 22:42, Jeff Kingwrote: > > On Mon, Dec 04, 2017 at 10:31:15PM +0100, Lars Schneider wrote: > I would like to add "for your input" or "for you" to convey that Git is not waiting for the machine but for the user. "hint: Launched editor. Waiting for your input..." Would that work for you? >>> >>> I guess "input" was the part that I found funny/confusing. The only >>> thing we know is that we're waiting on the editor process to finish, and >>> everything else is making assumptions about what's happening in the >>> editor. >> >> I see. How about: >> >> "hint: Launched editor. Waiting for your action..." >> (my preference) >> >> or >> >> "hint: Launched editor. Waiting for you..." > > Better, IMHO, though I still think literally saying: > > hint: Waiting for your editor to exit... > > is the most accurate, which I think makes it clear that you must _exit_ > your editor, not just save and close the file. I think "exit" would be confusing because most graphical editors (Sublime, Textmate, Notepad++, ...) can open multiple files and do not need to exit. The requirement is indeed save and close the file. How about: hint: Waiting for your editor to close the file... I generally like that as this is technical correct from all angles. My only nit would be that "the file" is a bit imprecise... but that's probably no problem. - Lars
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Mon, Dec 04, 2017 at 10:31:15PM +0100, Lars Schneider wrote: > >> I would like to add "for your input" or "for you" to convey > >> that Git is not waiting for the machine but for the user. > >> > >>"hint: Launched editor. Waiting for your input..." > >> > >> Would that work for you? > > > > I guess "input" was the part that I found funny/confusing. The only > > thing we know is that we're waiting on the editor process to finish, and > > everything else is making assumptions about what's happening in the > > editor. > > I see. How about: > > "hint: Launched editor. Waiting for your action..." > (my preference) > > or > > "hint: Launched editor. Waiting for you..." Better, IMHO, though I still think literally saying: hint: Waiting for your editor to exit... is the most accurate, which I think makes it clear that you must _exit_ your editor, not just save and close the file. I dunno, maybe that is being overly paranoid. Certainly I have seen graphical programs that have a mismatch with the one-process-per-action way that most terminal editors view the world, and would hang around even after the user thinks they are done editing. But at the same time, those programs are unlikely to work well as $GIT_EDITOR in the first place, because running them from the terminal may just open a new window in an existing session and exit immediately (which is the opposite problem -- the editor exited before the user actually did their thing). So I'm not sure if that would be a problem in practice or not. I'm too mired in the vim world to have any real data. Somebody like you who is supporting a large number of less-Unixy users probably has more perspective there. -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
Jeff Kingwrites: > On Sun, Dec 03, 2017 at 01:47:04PM +0100, Lars Schneider wrote: > >> I am on the fence here. I like consistency but I don't want to >> bother Git users. >> >> @Peff: Do you lean into either direction? Could you imagine that >> novice/regular users are bothered? (I don't expect experts to be >> bothered too much as they would likely be able to find and set >> the advice config). > > I also am on the fence, and am OK with any of the options discussed. > > But now I've said my reservations on the list, so I can say "I told you > so" if people complain (and naturally refuse to admit my objections if > people love it). :) Heh. I am even OK with not doing anything for that matter ;-)
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
> On 04 Dec 2017, at 18:32, Jeff Kingwrote: > > On Sun, Dec 03, 2017 at 01:47:04PM +0100, Lars Schneider wrote: > >> I am on the fence here. I like consistency but I don't want to >> bother Git users. >> >> @Peff: Do you lean into either direction? Could you imagine that >> novice/regular users are bothered? (I don't expect experts to be >> bothered too much as they would likely be able to find and set >> the advice config). > > I also am on the fence, and am OK with any of the options discussed. > > But now I've said my reservations on the list, so I can say "I told you > so" if people complain (and naturally refuse to admit my objections if > people love it). :) Well, since you and I are on the fence, I will follow Junio. Then we can at least argue that the feature is consistent on all terminals. - Lars
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
> On 04 Dec 2017, at 18:26, Jeff Kingwrote: > > On Sun, Dec 03, 2017 at 05:39:10PM +0100, Lars Schneider wrote: > > + fprintf(stderr, _("hint: Waiting for your editor > input...")); I found "waiting for editor input" to be a funny way of saying this. I input to the editor, the editor does not input to Git. :) Maybe "waiting for your editor finish" or something would make more sense? >>> >>> May be the good "Launched editor. Waiting ..." message, that was used in a >>> previous version, itself makes sense? >> >> Perfect bikeshed topic :-) >> >> I would like to add "for your input" or "for you" to convey >> that Git is not waiting for the machine but for the user. >> >>"hint: Launched editor. Waiting for your input..." >> >> Would that work for you? > > I guess "input" was the part that I found funny/confusing. The only > thing we know is that we're waiting on the editor process to finish, and > everything else is making assumptions about what's happening in the > editor. I see. How about: "hint: Launched editor. Waiting for your action..." (my preference) or "hint: Launched editor. Waiting for you..." - Lars
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Sun, Dec 03, 2017 at 01:47:04PM +0100, Lars Schneider wrote: > I am on the fence here. I like consistency but I don't want to > bother Git users. > > @Peff: Do you lean into either direction? Could you imagine that > novice/regular users are bothered? (I don't expect experts to be > bothered too much as they would likely be able to find and set > the advice config). I also am on the fence, and am OK with any of the options discussed. But now I've said my reservations on the list, so I can say "I told you so" if people complain (and naturally refuse to admit my objections if people love it). :) -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Sat, Dec 02, 2017 at 09:15:29PM -0800, Junio C Hamano wrote: > Jeff Kingwrites: > > > 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. > > ... > > I think that it is not worth to special-case "dumb" terminals like > this round of the patches do. If it so much disturbs reviewers that > "\e[K" may not work everywhere, we can do without the "then delete > it" part. It was merely trying to be extra nice, and the more > important part of the "feature" is to be noticeable, and I do think > that not showing anything on "dumb", only because the message cannot > be retracted, is putting the cart before the horse. > > Since especially now people are hiding this behind an advise.* > thing, I think it is OK to show a message and waste a line, even. Yeah, I was tempted to suggest just dropping this terminal magic completely. But it probably _does_ work and is helpful in the majority of cases (i.e., where people have in-terminal editors). I dunno. I am a little wary of hiding behind "but you can disable it with a config option", because that's still a thing that users have to actually do to get the previous behavior. And I expect to get some "ugh, git is too chatty and annoying" backlash once this is in a released version. But maybe that is just being paranoid. It's not like we don't have a lot of other advice flags. I really could go either way on this whole thing (but I'll be setting the advice flag myself ;) ). > > 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. > > Yes, I would have to agree that this one is reaching, as there isn't > any valid reason other than "the editor then wanted to do \e[K > later" for it to end its last line with CR. So our eating that line > is not a problem. Yeah, this was just me trying to come up with all possible implications. I agree it's probably not worth worrying about. -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Sun, Dec 03, 2017 at 05:39:10PM +0100, Lars Schneider wrote: > >>> + fprintf(stderr, _("hint: Waiting for your editor > >>> input...")); > >> I found "waiting for editor input" to be a funny way of saying this. I > >> input to the editor, the editor does not input to Git. :) > >> Maybe "waiting for your editor finish" or something would make more > >> sense? > > > > May be the good "Launched editor. Waiting ..." message, that was used in a > > previous version, itself makes sense? > > Perfect bikeshed topic :-) > > I would like to add "for your input" or "for you" to convey > that Git is not waiting for the machine but for the user. > > "hint: Launched editor. Waiting for your input..." > > Would that work for you? I guess "input" was the part that I found funny/confusing. The only thing we know is that we're waiting on the editor process to finish, and everything else is making assumptions about what's happening in the editor. -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Sat, Dec 02, 2017 at 09:15:49AM +0530, Kaartic Sivaraam wrote: > > Or given that the goal is really just making it clear that we've spawned > > an editor, something like "starting editor %s..." would work. > > There was already discussion related to the "continuous tense" used in the > phrase. > > Extract from [1]: Thanks, I missed that one. I agree with the argument there. -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Sun, 2017-12-03 at 17:39 +0100, Lars Schneider wrote: > > On 02 Dec 2017, at 04:45, Kaartic Sivaraam> > wrote: > > > > On Friday 01 December 2017 11:59 PM, Jeff King wrote: > > > On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote: > > > > > > > > Thanks for the review :-) > > > > > > Actually, I meant to bikeshed one part but forgot. ;) > > > > + fprintf(stderr, _("hint: Waiting for your > > > > editor input...")); > > > > > > I found "waiting for editor input" to be a funny way of saying this. I > > > input to the editor, the editor does not input to Git. :) > > > Maybe "waiting for your editor finish" or something would make more > > > sense? > > > > May be the good "Launched editor. Waiting ..." message, that was used in a > > previous version, itself makes sense? > > Perfect bikeshed topic :-) > Yep :-) > I would like to add "for your input" or "for you" to convey > that Git is not waiting for the machine but for the user. > > "hint: Launched editor. Waiting for your input..." > > Would that work for you? > Yeah, this one does look fine. That said, FWIW I don't have strong opinions about the phrase/sentence except that they should be readable and shouldn't get too verbose. Thanks, Kaartic
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
> On 02 Dec 2017, at 04:45, Kaartic Sivaraamwrote: > > On Friday 01 December 2017 11:59 PM, Jeff King wrote: >> On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote: >>> >>> Thanks for the review :-) >> Actually, I meant to bikeshed one part but forgot. ;) >>> + fprintf(stderr, _("hint: Waiting for your editor >>> input...")); >> I found "waiting for editor input" to be a funny way of saying this. I >> input to the editor, the editor does not input to Git. :) >> Maybe "waiting for your editor finish" or something would make more >> sense? > > May be the good "Launched editor. Waiting ..." message, that was used in a > previous version, itself makes sense? Perfect bikeshed topic :-) I would like to add "for your input" or "for you" to convey that Git is not waiting for the machine but for the user. "hint: Launched editor. Waiting for your input..." Would that work for you? Thanks, Lars
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
> On 03 Dec 2017, at 06:15, Junio C Hamanowrote: > > Jeff King writes: > >> 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. >> ... > > I think that it is not worth to special-case "dumb" terminals like > this round of the patches do. If it so much disturbs reviewers that > "\e[K" may not work everywhere, we can do without the "then delete > it" part. It was merely trying to be extra nice, and the more > important part of the "feature" is to be noticeable, and I do think > that not showing anything on "dumb", only because the message cannot > be retracted, is putting the cart before the horse. > > Since especially now people are hiding this behind an advise.* > thing, I think it is OK to show a message and waste a line, even. Well, my reasoning was to minimize the risk of bothering people: People using "dumb" terminals wouldn't be bothered because nothing changes and people using "smart" terminals would see the message only temporarily. Of course, emacsclient users would be the exception. They would always be bothered and therefore I added the "advice" switch. That being said, I can also follow your logic. If we have such a feature then we shouldn't surprise the user. We should make it consistently available in all environments. I am on the fence here. I like consistency but I don't want to bother Git users. @Peff: Do you lean into either direction? Could you imagine that novice/regular users are bothered? (I don't expect experts to be bothered too much as they would likely be able to find and set the advice config). Thanks, Lars
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
Jeff Kingwrites: > 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. > ... I think that it is not worth to special-case "dumb" terminals like this round of the patches do. If it so much disturbs reviewers that "\e[K" may not work everywhere, we can do without the "then delete it" part. It was merely trying to be extra nice, and the more important part of the "feature" is to be noticeable, and I do think that not showing anything on "dumb", only because the message cannot be retracted, is putting the cart before the horse. Since especially now people are hiding this behind an advise.* thing, I think it is OK to show a message and waste a line, even. > 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. Yes, I would have to agree that this one is reaching, as there isn't any valid reason other than "the editor then wanted to do \e[K later" for it to end its last line with CR. So our eating that line is not a problem.
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Friday 01 December 2017 11:59 PM, Jeff King wrote: On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote: Thanks for the review :-) Actually, I meant to bikeshed one part but forgot. ;) + fprintf(stderr, _("hint: Waiting for your editor input...")); I found "waiting for editor input" to be a funny way of saying this. I input to the editor, the editor does not input to Git. :) Maybe "waiting for your editor finish" or something would make more sense? May be the good "Launched editor. Waiting ..." message, that was used in a previous version, itself makes sense? Or given that the goal is really just making it clear that we've spawned an editor, something like "starting editor %s..." would work. There was already discussion related to the "continuous tense" used in the phrase. Extract from [1]: -- 8< -- > fprintf(stderr, "Launching your editor..."); "It takes quite some time to launch this special Git Editor" As Lars pointed out, the editor may be launched in the background, that the user would not know, but they might expect a thing to pop up as a modal dialog as is always with UIs. So despite it being technically wrong at this point in time, I would phrase it in past tense or in a way that indicates that the user needs to take action already. The "Launching..." sounds as if I need to wait for an event to occur. -- >8 -- [1]: https://public-inbox.org/git/CAGZ79kZbm8SGY4rXKZHV82E-HX9qbQ4iyCbMgJEBFQf4fj3u=q...@mail.gmail.com/ I think the "waiting for..." pattern is perfectly fine, though.
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Fri, Dec 01, 2017 at 01:52:14PM +0100, Lars Schneider wrote: > > 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? Yeah, if there's a lot of spew, I agree it probably doesn't matter. The "emacsclient" one is probably the worst case, because it would print a single line of its own, which would get tacked onto the "Waiting..." message printed by Git, since it doesn't end with a newline. > > The patch itself looks fine, as far as correctly implementing the > > design. > > Thanks for the review :-) Actually, I meant to bikeshed one part but forgot. ;) > + fprintf(stderr, _("hint: Waiting for your editor > input...")); I found "waiting for editor input" to be a funny way of saying this. I input to the editor, the editor does not input to Git. :) Maybe "waiting for your editor finish" or something would make more sense? Or given that the goal is really just making it clear that we've spawned an editor, something like "starting editor %s..." would work. I think the "waiting for..." pattern is perfectly fine, though. -Peff
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
> On 30 Nov 2017, at 21:51, Jeff Kingwrote: > > 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
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Friday 01 December 2017 02:21 AM, Jeff King wrote: 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). Yeah, at times 'gedit' does do what you say. And if the user (surprisingly!) uses an IDE such as "eclipse" or a hackable text editor "atom" (of course with the '-f' option) for entering his commit message it is likely to happen all the time for him. I don't think there's a good way around it. Portably saying "delete _this_ line that I wrote earlier" would probably require libcurses or similar. So maybe we just live with it. The deletion magic makes the common cases better (a terminal editor that doesn't print random lines, or a graphical editor that is quiet), and everyone else can flip the advice switch if they need to. I dunno. --- Kaartic
Re: [PATCH v4 2/2] launch_editor(): indicate that Git waits for user input
On Wed, Nov 29, 2017 at 03:37:52PM +0100, lars.schnei...@autodesk.com wrote: > No message is printed in a "dumb" terminal as it would not be possible > to remove the message after the editor returns. This should not be a > problem as this feature is targeted at novice Git users and they are > unlikely to work with a "dumb" terminal. I think novice users could end up in this situation with something like: ssh remote_host git commit But then I'd expect most terminal-based editors to give some sort of error in that situation, too. And at any rate, the worst case is that they get no special "waiting..." message from Git, which is already the status quo. So it's probably not worth worrying about such an obscure case. > Power users might not want to see this message or their editor might > already print such a message (e.g. emacsclient). Allow these users to > suppress the message by disabling the "advice.waitingForEditor" config. I'm happy to see the hard-coded emacsclient behavior go. Hopefully we won't see too many complaints about people having to set the advice flag. > 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). I don't think there's a good way around it. Portably saying "delete _this_ line that I wrote earlier" would probably require libcurses or similar. So maybe we just live with it. The deletion magic makes the common cases better (a terminal editor that doesn't print random lines, or a graphical editor that is quiet), and everyone else can flip the advice switch if they need to. I dunno. > --- > Documentation/config.txt | 3 +++ > advice.c | 2 ++ > advice.h | 1 + > editor.c | 15 +++ > 4 files changed, 21 insertions(+) The patch itself looks fine, as far as correctly implementing the design. -Peff