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

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

[lars.schneider]

From: Lars Schneider <larsxschneider@gmail.com>

Hi,

I simplified the code and enabled the "Git is waiting for editor input"
message only for "intelligent" terminals. The rational is, that today's
novice Git users are likely to have an "intelligent" terminal. People
working with "dumb" terminals have their reasons and are likely not the
target audience for this hint.

In addition, I added "advice.waitingForEditor" as an easy way to disable
the hint for experienced users.

I refactored the detection of dumb terminals in a preparation commit.

Thanks,
Lars


RFC: https://public-inbox.org/git/274B4850-2EB7-4BFA-A42C-25A573254969@gmail.com/
 v1: https://public-inbox.org/git/xmqqr2syvjxb.fsf@gitster.mtv.corp.google.com/
 v2: https://public-inbox.org/git/20171117135109.18071-1-lars.schneider@autodesk.com/
 v3: https://public-inbox.org/git/20171127134716.69471-1-lars.schneider@autodesk.com/

Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/9639e931bf
Checkout: git fetch https://github.com/larsxschneider/git editor-v4 && git checkout 9639e931bf


### Interdiff (v3..v4):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 671fcbaa0f..de64201e82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -354,6 +354,9 @@ advice.*::
 	ignoredHook::
 		Advice shown if an hook is ignored because the hook is not
 		set as executable.
+	waitingForEditor::
+		Print a message to the terminal whenever Git is waiting for
+		editor input from the user.
 --

 core.fileMode::
diff --git a/advice.c b/advice.c
index c6169bcb52..406efc183b 100644
--- a/advice.c
+++ b/advice.c
@@ -18,6 +18,7 @@ int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
+int advice_waiting_for_editor = 1;

 static struct {
 	const char *name;
@@ -40,6 +41,7 @@ static struct {
 	{ "rmhints", &advice_rm_hints },
 	{ "addembeddedrepo", &advice_add_embedded_repo },
 	{ "ignoredhook", &advice_ignored_hook },
+	{ "waitingforeditor", &advice_waiting_for_editor },

 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index f525d6f89c..70568fa792 100644
--- a/advice.h
+++ b/advice.h
@@ -20,6 +20,7 @@ extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
+extern int advice_waiting_for_editor;

 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/cache.h b/cache.h
index 89f5d24579..3842fc097c 100644
--- a/cache.h
+++ b/cache.h
@@ -1469,6 +1469,7 @@ extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
+extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
 extern void reset_ident_date(void);

diff --git a/color.c b/color.c
index 9a9261ac16..d48dd947c9 100644
--- a/color.c
+++ b/color.c
@@ -329,8 +329,7 @@ static int check_auto_color(void)
 	if (color_stdout_is_tty < 0)
 		color_stdout_is_tty = isatty(1);
 	if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
-		char *term = getenv("TERM");
-		if (term && strcmp(term, "dumb"))
+		if (!is_terminal_dumb())
 			return 1;
 	}
 	return 0;
diff --git a/editor.c b/editor.c
index 4251ae9d7a..cdad4f74ec 100644
--- a/editor.c
+++ b/editor.c
@@ -7,11 +7,16 @@
 #define DEFAULT_EDITOR "vi"
 #endif

+int is_terminal_dumb(void)
+{
+	const char *terminal = getenv("TERM");
+	return !terminal || !strcmp(terminal, "dumb");
+}
+
 const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
-	const char *terminal = getenv("TERM");
-	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
+	int terminal_is_dumb = is_terminal_dumb();

 	if (!editor && editor_program)
 		editor = editor_program;
@@ -40,33 +45,11 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		const char *args[] = { editor, real_path(path), NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
-		static const char *close_notice = NULL;
-
-		if (isatty(2) && !close_notice) {
-			char *term = getenv("TERM");
-
-			if (term && strcmp(term, "dumb"))
-				/*
-				 * go back to the beginning and erase the
-				 * entire line if the terminal is capable
-				 * to do so, to avoid wasting the vertical
-				 * space.
-				 */
-				close_notice = "\r\033[K";
-			else if (term && strstr(term, "emacsclient"))
-				/*
-				 * `emacsclient` (or `emacsclientw` on Windows) already prints
-				 * ("Waiting for Emacs...") if a file is opened for editing.
-				 * Therefore, we don't need to print the editor launch info.
-				 */
-				;
-			else
-				/* otherwise, complete and waste the line */
-				close_notice = _("done.\n");
-		}
+		int print_waiting_for_editor = advice_waiting_for_editor &&
+			isatty(2) && !is_terminal_dumb();

-		if (close_notice) {
-			fprintf(stderr, _("Launched editor. Waiting for your input... "));
+		if (print_waiting_for_editor) {
+			fprintf(stderr, _("hint: Waiting for your editor input..."));
 			fflush(stderr);
 		}

@@ -82,14 +65,19 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
-
 		if (sig == SIGINT || sig == SIGQUIT)
 			raise(sig);
 		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
-		if (close_notice)
-			fputs(close_notice, stderr);
+
+		if (print_waiting_for_editor)
+			/*
+			 * go back to the beginning and erase the
+			 * entire line to avoid wasting the vertical
+			 * space.
+			 */
+			fputs("\r\033[K", stderr);
 	}

 	if (!buffer)
diff --git a/sideband.c b/sideband.c
index 1e4d684d6c..6d7f943e43 100644
--- a/sideband.c
+++ b/sideband.c
@@ -20,13 +20,12 @@

 int recv_sideband(const char *me, int in_stream, int out)
 {
-	const char *term, *suffix;
+	const char *suffix;
 	char buf[LARGE_PACKET_MAX + 1];
 	struct strbuf outbuf = STRBUF_INIT;
 	int retval = 0;

-	term = getenv("TERM");
-	if (isatty(2) && term && strcmp(term, "dumb"))
+	if (isatty(2) && !is_terminal_dumb())
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;


### Patches

Lars Schneider (2):
  refactor "dumb" terminal determination
  launch_editor(): indicate that Git waits for user input

 Documentation/config.txt |  3 +++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 cache.h                  |  1 +
 color.c                  |  3 +--
 editor.c                 | 24 ++++++++++++++++++++++--
 sideband.c               |  5 ++---
 7 files changed, 32 insertions(+), 7 deletions(-)


base-commit: 89ea799ffcc5c8a0547d3c9075eb979256ee95b8
--
2.15.0

[PATCH v4 1/2] refactor "dumb" terminal determination

[lars.schneider]

From: Lars Schneider <larsxschneider@gmail.com>

Move the code to detect "dumb" terminals into a single location. This
avoids duplicating the terminal detection code yet again in a subsequent
commit.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 cache.h    | 1 +
 color.c    | 3 +--
 editor.c   | 9 +++++++--
 sideband.c | 5 ++---
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 89f5d24579..3842fc097c 100644
--- a/cache.h
+++ b/cache.h
@@ -1469,6 +1469,7 @@ extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
+extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
 extern void reset_ident_date(void);
 
diff --git a/color.c b/color.c
index 9a9261ac16..d48dd947c9 100644
--- a/color.c
+++ b/color.c
@@ -329,8 +329,7 @@ static int check_auto_color(void)
 	if (color_stdout_is_tty < 0)
 		color_stdout_is_tty = isatty(1);
 	if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
-		char *term = getenv("TERM");
-		if (term && strcmp(term, "dumb"))
+		if (!is_terminal_dumb())
 			return 1;
 	}
 	return 0;
diff --git a/editor.c b/editor.c
index 7519edecdc..c65ea698eb 100644
--- a/editor.c
+++ b/editor.c
@@ -7,11 +7,16 @@
 #define DEFAULT_EDITOR "vi"
 #endif
 
+int is_terminal_dumb(void)
+{
+	const char *terminal = getenv("TERM");
+	return !terminal || !strcmp(terminal, "dumb");
+}
+
 const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
-	const char *terminal = getenv("TERM");
-	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
+	int terminal_is_dumb = is_terminal_dumb();
 
 	if (!editor && editor_program)
 		editor = editor_program;
diff --git a/sideband.c b/sideband.c
index 1e4d684d6c..6d7f943e43 100644
--- a/sideband.c
+++ b/sideband.c
@@ -20,13 +20,12 @@
 
 int recv_sideband(const char *me, int in_stream, int out)
 {
-	const char *term, *suffix;
+	const char *suffix;
 	char buf[LARGE_PACKET_MAX + 1];
 	struct strbuf outbuf = STRBUF_INIT;
 	int retval = 0;
 
-	term = getenv("TERM");
-	if (isatty(2) && term && strcmp(term, "dumb"))
+	if (isatty(2) && !is_terminal_dumb())
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-- 
2.15.0

Re: [PATCH v4 1/2] refactor "dumb" terminal determination

[Jeff King]

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

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Move the code to detect "dumb" terminals into a single location. This
> avoids duplicating the terminal detection code yet again in a subsequent
> commit.

Makes sense, and probably worth doing even if we don't follow through on
2/2.

The patch itself looks good to me.

-Peff

Re: [PATCH v4 1/2] refactor "dumb" terminal determination

[Kaartic Sivaraam]

On Wednesday 29 November 2017 08:07 PM, lars.schneider@autodesk.com wrote:
> +int is_terminal_dumb(void)
> +{
> +	const char *terminal = getenv("TERM");
> +	return !terminal || !strcmp(terminal, "dumb");

So, IIUC, there terminal is considered to be 'dumb' when the TERM 
environment variable is NOT set or when it is set to 'dumb'.

> +}
> +
>   const char *git_editor(void)
>   {
>   	const char *editor = getenv("GIT_EDITOR");
> -	const char *terminal = getenv("TERM");
> -	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
> +	int terminal_is_dumb = is_terminal_dumb();
>   
>   	if (!editor && editor_program)
>   		editor = editor_program;
> diff --git a/sideband.c b/sideband.c
> index 1e4d684d6c..6d7f943e43 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -20,13 +20,12 @@
>   
>   int recv_sideband(const char *me, int in_stream, int out)
>   {
> -	const char *term, *suffix;
> +	const char *suffix;
>   	char buf[LARGE_PACKET_MAX + 1];
>   	struct strbuf outbuf = STRBUF_INIT;
>   	int retval = 0;
>   
> -	term = getenv("TERM");
> -	if (isatty(2) && term && strcmp(term, "dumb"))
> +	if (isatty(2) && !is_terminal_dumb())
>   		suffix = ANSI_SUFFIX;
>   	else
>   		suffix = DUMB_SUFFIX;
> 

This one looks good to me if my observation above is correct.

---
Kaartic

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

[lars.schneider]

From: Lars Schneider <larsxschneider@gmail.com>

When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for user input (e.g. "git rebase -i"), then the editor window
might be obscured by other windows. The user may be left staring at the
original Git terminal window without even realizing that s/he needs to
interact with another window before Git can proceed. To this user Git
appears hanging.

Print a message that Git is waiting for editor input in the original
terminal and get rid of it when the editor returns.

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.

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.

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 Documentation/config.txt |  3 +++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 editor.c                 | 15 +++++++++++++++
 4 files changed, 21 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 671fcbaa0f..de64201e82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -354,6 +354,9 @@ advice.*::
 	ignoredHook::
 		Advice shown if an hook is ignored because the hook is not
 		set as executable.
+	waitingForEditor::
+		Print a message to the terminal whenever Git is waiting for
+		editor input from the user.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index c6169bcb52..406efc183b 100644
--- a/advice.c
+++ b/advice.c
@@ -18,6 +18,7 @@ int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
+int advice_waiting_for_editor = 1;
 
 static struct {
 	const char *name;
@@ -40,6 +41,7 @@ static struct {
 	{ "rmhints", &advice_rm_hints },
 	{ "addembeddedrepo", &advice_add_embedded_repo },
 	{ "ignoredhook", &advice_ignored_hook },
+	{ "waitingforeditor", &advice_waiting_for_editor },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index f525d6f89c..70568fa792 100644
--- a/advice.h
+++ b/advice.h
@@ -20,6 +20,7 @@ extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
+extern int advice_waiting_for_editor;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/editor.c b/editor.c
index c65ea698eb..cdad4f74ec 100644
--- a/editor.c
+++ b/editor.c
@@ -45,6 +45,13 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		const char *args[] = { editor, real_path(path), NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
+		int print_waiting_for_editor = advice_waiting_for_editor &&
+			isatty(2) && !is_terminal_dumb();
+
+		if (print_waiting_for_editor) {
+			fprintf(stderr, _("hint: Waiting for your editor input..."));
+			fflush(stderr);
+		}
 
 		p.argv = args;
 		p.env = env;
@@ -63,6 +70,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
+
+		if (print_waiting_for_editor)
+			/*
+			 * go back to the beginning and erase the
+			 * entire line to avoid wasting the vertical
+			 * space.
+			 */
+			fputs("\r\033[K", stderr);
 	}
 
 	if (!buffer)
-- 
2.15.0

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

[Jeff King]

On Wed, Nov 29, 2017 at 03:37:52PM +0100, lars.schneider@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

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

[Kaartic Sivaraam]

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

[Lars Schneider]

> 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

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

[Jeff King]

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

[Kaartic Sivaraam]

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

[Lars Schneider]

> On 02 Dec 2017, at 04:45, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> 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 :-)

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

[Kaartic Sivaraam]

On Sun, 2017-12-03 at 17:39 +0100, Lars Schneider wrote:
> > On 02 Dec 2017, at 04:45, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> 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

[Jeff King]

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

[Lars Schneider]

> On 04 Dec 2017, at 18:26, Jeff King <peff@peff.net> wrote:
> 
> 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

[Jeff King]

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

[Lars Schneider]

> On 04 Dec 2017, at 22:42, Jeff King <peff@peff.net> wrote:
> 
> 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

[Jeff King]

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

[Jeff King]

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

> 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

[Lars Schneider]

> On 03 Dec 2017, at 06:15, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeff King <peff@peff.net> 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 King]

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

[Lars Schneider]

> On 04 Dec 2017, at 18:32, Jeff King <peff@peff.net> wrote:
> 
> 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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

[Jeff King]

On Sat, Dec 02, 2017 at 09:15:29PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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.

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 0/2] launch_editor(): indicate that Git waits for user input

[Thomas Adam]

On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
> +		if (print_waiting_for_editor) {
> +			fprintf(stderr, _("hint: Waiting for your editor input..."));
>  			fflush(stderr);

Just FYI, stderr is typically unbuffered on most systems I've used, and
although the call to fflush() is harmless, I suspect it's not having any
effect.  That said, there's plenty of other places in Git which seems to think
fflush()ing stderr actually does something.

-- Thomas Adam

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

[Lars Schneider]

> On 29 Nov 2017, at 19:35, Thomas Adam <thomas@xteddy.org> wrote:
> 
> On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
>> +		if (print_waiting_for_editor) {
>> +			fprintf(stderr, _("hint: Waiting for your editor input..."));
>> 			fflush(stderr);
> 
> Just FYI, stderr is typically unbuffered on most systems I've used, and
> although the call to fflush() is harmless, I suspect it's not having any
> effect.  That said, there's plenty of other places in Git which seems to think
> fflush()ing stderr actually does something.

I agree with the "unbuffered" statement. I am surprised that you expect fflush()
to do nothing in that situation... but I am no expert in that area. Can you
point me to some documentation?

In any way, would all this be a problem here? The worst that could happen would
be that the user would not see the message, right?

Are you aware of stderr usage in Git that could cause more trouble?

- Lars

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

[Thomas Adam]

On Thu, Nov 30, 2017 at 02:55:35PM +0100, Lars Schneider wrote:
> 
> > On 29 Nov 2017, at 19:35, Thomas Adam <thomas@xteddy.org> wrote:
> > 
> > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
> >> +		if (print_waiting_for_editor) {
> >> +			fprintf(stderr, _("hint: Waiting for your editor input..."));
> >> 			fflush(stderr);
> > 
> > Just FYI, stderr is typically unbuffered on most systems I've used, and
> > although the call to fflush() is harmless, I suspect it's not having any
> > effect.  That said, there's plenty of other places in Git which seems to think
> > fflush()ing stderr actually does something.
> 
> I agree with the "unbuffered" statement. I am surprised that you expect fflush()
> to do nothing in that situation... but I am no expert in that area. Can you
> point me to some documentation?

Because stderr is unbuffered, it will get printed immediately.

> In any way, would all this be a problem here? The worst that could happen would
> be that the user would not see the message, right?

Correct -- I only bring this up because your interdiff showed you added the
fflush() call and I was merely pointing that out.  I don't expect you to
change it.

> Are you aware of stderr usage in Git that could cause more trouble?

No.  It'll all be fine.

-- Thomas Adam

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

[Andreas Schwab]

On Nov 30 2017, Thomas Adam <thomas@xteddy.org> wrote:

> On Thu, Nov 30, 2017 at 02:55:35PM +0100, Lars Schneider wrote:
>> 
>> > On 29 Nov 2017, at 19:35, Thomas Adam <thomas@xteddy.org> wrote:
>> > 
>> > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
>> >> +		if (print_waiting_for_editor) {
>> >> +			fprintf(stderr, _("hint: Waiting for your editor input..."));
>> >> 			fflush(stderr);
>> > 
>> > Just FYI, stderr is typically unbuffered on most systems I've used, and
>> > although the call to fflush() is harmless, I suspect it's not having any
>> > effect.  That said, there's plenty of other places in Git which seems to think
>> > fflush()ing stderr actually does something.
>> 
>> I agree with the "unbuffered" statement. I am surprised that you expect fflush()
>> to do nothing in that situation... but I am no expert in that area. Can you
>> point me to some documentation?
>
> Because stderr is unbuffered, it will get printed immediately.

POSIX only requires stderr to be "not fully buffered".  If it is line
buffered, the message may not appear immediately.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Kaartic Sivaraam]

On Thu, 2017-11-30 at 16:13 +0100, Andreas Schwab wrote:
> On Nov 30 2017, Thomas Adam <thomas@xteddy.org> wrote:
> 
> > On Thu, Nov 30, 2017 at 02:55:35PM +0100, Lars Schneider wrote:
> > > 
> > > > On 29 Nov 2017, at 19:35, Thomas Adam <thomas@xteddy.org> wrote:
> > > > 
> > > > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
> > > > > +		if (print_waiting_for_editor) {
> > > > > +			fprintf(stderr, _("hint: Waiting for your editor input..."));
> > > > > 			fflush(stderr);
> > > > 
> > > > Just FYI, stderr is typically unbuffered on most systems I've used, and
> > > > although the call to fflush() is harmless, I suspect it's not having any
> > > > effect.  That said, there's plenty of other places in Git which seems to think
> > > > fflush()ing stderr actually does something.
> > > 
> > > I agree with the "unbuffered" statement. I am surprised that you expect fflush()
> > > to do nothing in that situation... but I am no expert in that area. Can you
> > > point me to some documentation?
> > 
> > Because stderr is unbuffered, it will get printed immediately.
> 
> POSIX only requires stderr to be "not fully buffered".  If it is line
> buffered, the message may not appear immediately.
> 

I guess Junio's reply for the same "unbuffered" question I asked for an
earlier version of this patch (now, a series) might be relevant here,

> > Being curious again, is flushing 'stderr' required ? AFAIK, 'stderr'
> > is unbuffered by default and I didn't notice any calls that changed
> > the buffering mode of it along this code path.
> 
> "By default" is the key phrase.  The code is merely being defensive
> to changes in other area of the code.

cf. <xmqq8tf3oz3n.fsf@gitster.mtv.corp.google.com>


-- 
Kaartic

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

[Jeff King]

On Wed, Nov 29, 2017 at 06:35:16PM +0000, Thomas Adam wrote:

> On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
> > +		if (print_waiting_for_editor) {
> > +			fprintf(stderr, _("hint: Waiting for your editor input..."));
> >  			fflush(stderr);
> 
> Just FYI, stderr is typically unbuffered on most systems I've used, and
> although the call to fflush() is harmless, I suspect it's not having any
> effect.  That said, there's plenty of other places in Git which seems to think
> fflush()ing stderr actually does something.

I'd prefer to keep them (including this one), even if they are noops on
most platforms, because:

  1. They serve as a note for readers of the code that it's important
     for the output to have been printed immediately.

  2. We build on some funny and antique platforms. I wouldn't be
     surprised if there's one that line buffers by default. Or even a
     modern system with funny settings (e.g., using the GNU stdbuf
     tool).

(I know you said later you don't think this case needs to be removed,
but I want to make it clear I think it's a reasonable project-wide
policy to not assume we we know how stderr is buffered).

-Peff

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

[Thomas Adam]

On Thu, Nov 30, 2017 at 03:12:17PM -0500, Jeff King wrote:
> On Wed, Nov 29, 2017 at 06:35:16PM +0000, Thomas Adam wrote:
> 
> > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schneider@autodesk.com wrote:
> > > +		if (print_waiting_for_editor) {
> > > +			fprintf(stderr, _("hint: Waiting for your editor input..."));
> > >  			fflush(stderr);
> > 
> > Just FYI, stderr is typically unbuffered on most systems I've used, and
> > although the call to fflush() is harmless, I suspect it's not having any
> > effect.  That said, there's plenty of other places in Git which seems to think
> > fflush()ing stderr actually does something.
> 
> I'd prefer to keep them (including this one), even if they are noops on
> most platforms, because:
> 
>   1. They serve as a note for readers of the code that it's important
>      for the output to have been printed immediately.
> 
>   2. We build on some funny and antique platforms. I wouldn't be
>      surprised if there's one that line buffers by default. Or even a
>      modern system with funny settings (e.g., using the GNU stdbuf
>      tool).
> 
> (I know you said later you don't think this case needs to be removed,
> but I want to make it clear I think it's a reasonable project-wide
> policy to not assume we we know how stderr is buffered).

We're talking past each other, Peff.  I'm agreeing with you.  I was surprised
to see the introduction of fflush(stderr) in the interdiff, when it wasn't
present before, was curious to understand why.  I've done that, and since
stated it's fine to leave it as-is.

-- Thomas Adam