threads / patch / 47344

v4, 2 partslaunch_editor(): indicate that Git waits for user input

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

## tl;dr

31 messages between Nov 29, 2017 and Dec 4, 2017. Diffs are folded; open one to read it.

replies: 30people: 7as markdown or json

lars.schneider@autodesk.com· Nov 29, 2017, 14:37 UTC · lore
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):
Show changes to 7 files +29 −37

Documentation/config.txt, advice.c, advice.h, cache.h, color.c, editor.c, sideband.c

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
lars.schneider@autodesk.com· Nov 29, 2017, 14:37 UTC · re: lars.schneider@autodesk.com · lore

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

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(-)
Show changes to 4 files +11 −7

cache.h, color.c, editor.c, sideband.c

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
Jeff King· Nov 30, 2017, 20:30 UTC · re: lars.schneider@autodesk.com · lore

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

On Wed, Nov 29, 2017 at 03:37:51PM +0100, lars.schneider@autodesk.com wrote:
Show 5 quoted lines
> 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
Kaartic Sivaraam· Dec 1, 2017, 03:26 UTC · re: lars.schneider@autodesk.com · lore

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

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'.

Show 32 quoted lines
> +}
> +
>   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

lars.schneider@autodesk.com· Nov 29, 2017, 14:37 UTC · re: lars.schneider@autodesk.com · lore

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

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(+)
Show changes to 4 files +21 −0

Documentation/config.txt, advice.c, advice.h, editor.c

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
Jeff King· Nov 30, 2017, 20:51 UTC · re: lars.schneider@autodesk.com · lore

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.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.

Show 6 quoted lines
> ---
>  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
Kaartic Sivaraam· Dec 1, 2017, 03:56 UTC · re: Jeff King · lore

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:
Show 7 quoted lines
> 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.

Show 7 quoted lines
> 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

Lars Schneider· Dec 1, 2017, 12:52 UTC · re: Jeff King · lore

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

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
Jeff King· Dec 1, 2017, 18:29 UTC · re: Lars Schneider · lore

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:
Show 10 quoted lines
> > 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
Kaartic Sivaraam· Dec 2, 2017, 03:45 UTC · re: Jeff King · lore

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:
Show 13 quoted lines
> 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.
>
Lars Schneider· Dec 3, 2017, 16:39 UTC · re: Kaartic Sivaraam · lore

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

Show 14 quoted lines
> 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

Kaartic Sivaraam· Dec 4, 2017, 16:41 UTC · re: Lars Schneider · lore

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:
Show 19 quoted lines
> > 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 :-)
Show 7 quoted lines
> 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

Jeff King· Dec 4, 2017, 17:26 UTC · re: Lars Schneider · lore

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:
Show 16 quoted lines
> >>> +                       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
Lars Schneider· Dec 4, 2017, 21:31 UTC · re: Jeff King · lore

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

Show 25 quoted lines
> 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
Jeff King· Dec 4, 2017, 21:42 UTC · re: Lars Schneider · lore

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:
Show 20 quoted lines
> >> 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
Lars Schneider· Dec 4, 2017, 21:54 UTC · re: Jeff King · lore

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

Show 31 quoted lines
> 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
Jeff King· Dec 4, 2017, 22:09 UTC · re: Lars Schneider · lore

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:
Show 18 quoted lines
> > 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
Jeff King· Dec 4, 2017, 17:25 UTC · re: Kaartic Sivaraam · lore

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:
Show 7 quoted lines
> > 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
Junio C Hamano· Dec 3, 2017, 05:15 UTC · re: Jeff King · lore

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

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.

Show 7 quoted lines
> 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.

Lars Schneider· Dec 3, 2017, 12:47 UTC · re: Junio C Hamano · lore

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

Show 19 quoted lines
> 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

Jeff King· Dec 4, 2017, 17:32 UTC · re: Lars Schneider · lore

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:
Show 7 quoted lines
> 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
Lars Schneider· Dec 4, 2017, 21:34 UTC · re: Jeff King · lore

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

Show 17 quoted lines
> 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
Junio C Hamano· Dec 4, 2017, 21:40 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> 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 ;-)
Jeff King· Dec 4, 2017, 17:30 UTC · re: Junio C Hamano · lore

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:
Show 17 quoted lines
> 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 ;) ).

Show 12 quoted lines
> > 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
Thomas Adam· Nov 29, 2017, 18:35 UTC · re: lars.schneider@autodesk.com · lore

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

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
Lars Schneider· Nov 30, 2017, 13:55 UTC · re: Thomas Adam · lore

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

Show 11 quoted lines
> 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
Thomas Adam· Nov 30, 2017, 14:42 UTC · re: Lars Schneider · lore

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

On Thu, Nov 30, 2017 at 02:55:35PM +0100, Lars Schneider wrote:
Show 16 quoted lines
> 
> > 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
Andreas Schwab· Nov 30, 2017, 15:13 UTC · re: Thomas Adam · lore

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

On Nov 30 2017, Thomas Adam <thomas@xteddy.org> wrote:
Show 19 quoted lines
> 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."
Kaartic Sivaraam· Dec 1, 2017, 03:41 UTC · re: Andreas Schwab · lore

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

On Thu, 2017-11-30 at 16:13 +0100, Andreas Schwab wrote:
Show 25 quoted lines
> 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,

Show 6 quoted lines
> > 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
Jeff King· Nov 30, 2017, 20:12 UTC · re: Thomas Adam · lore

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

On Wed, Nov 29, 2017 at 06:35:16PM +0000, Thomas Adam wrote:
Show 9 quoted lines
> 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
Thomas Adam· Nov 30, 2017, 20:51 UTC · re: Jeff King · lore

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

On Thu, Nov 30, 2017 at 03:12:17PM -0500, Jeff King wrote:
Show 26 quoted lines
> 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

← back to recent threads