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

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

[lars.schneider]

From: Lars Schneider <larsxschneider@gmail.com>

Hi,

Patch 1/2: No change. The patch got "looks good to me" from Peff [1]

Patch 2/2: I changed the waiting message to our bikeshedding result [2] and
           I enabled the waiting message on dumb terminals for consistency.

I also tested the patch on OS X and Windows with smart and dumb terminals.

Thanks for the reviews,
Lars


[1] https://public-inbox.org/git/20171130203042.GB3313@sigill.intra.peff.net/
[2] https://public-inbox.org/git/20171204220908.GA8184@sigill.intra.peff.net/

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/
 v4: https://public-inbox.org/git/20171129143752.60553-1-lars.schneider@autodesk.com/


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


### Interdiff (v4..v5):

diff --git a/editor.c b/editor.c
index cdad4f74ec..d52017363c 100644
--- a/editor.c
+++ b/editor.c
@@ -45,11 +45,17 @@ 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();
+		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);

 		if (print_waiting_for_editor) {
-			fprintf(stderr, _("hint: Waiting for your editor input..."));
+			fprintf(stderr,
+				_("hint: Waiting for your editor to close the file... "));
+			if (is_terminal_dumb())
+				/*
+				 * A dumb terminal cannot erase the line later on. Add a
+				 * newline to separate the hint from subsequent output.
+				 */
+				fprintf(stderr, "\n");
 			fflush(stderr);
 		}

@@ -71,11 +77,10 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 			return error("There was a problem with the editor '%s'.",
 					editor);

-		if (print_waiting_for_editor)
+		if (print_waiting_for_editor && !is_terminal_dumb())
 			/*
-			 * go back to the beginning and erase the
-			 * entire line to avoid wasting the vertical
-			 * space.
+			 * Go back to the beginning and erase the entire line to
+			 * avoid wasting the vertical space.
 			 */
 			fputs("\r\033[K", stderr);
 	}


### 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                 | 29 +++++++++++++++++++++++++++--
 sideband.c               |  5 ++---
 7 files changed, 37 insertions(+), 7 deletions(-)


base-commit: 89ea799ffcc5c8a0547d3c9075eb979256ee95b8
--
2.15.1

[PATCH v5 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.1

[PATCH v5 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 might 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 (if the terminal
supports erasing the last line).

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                 | 20 ++++++++++++++++++++
 4 files changed, 26 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..d52017363c 100644
--- a/editor.c
+++ b/editor.c
@@ -45,6 +45,19 @@ 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);
+
+		if (print_waiting_for_editor) {
+			fprintf(stderr,
+				_("hint: Waiting for your editor to close the file... "));
+			if (is_terminal_dumb())
+				/*
+				 * A dumb terminal cannot erase the line later on. Add a
+				 * newline to separate the hint from subsequent output.
+				 */
+				fprintf(stderr, "\n");
+			fflush(stderr);
+		}
 
 		p.argv = args;
 		p.env = env;
@@ -63,6 +76,13 @@ 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 && !is_terminal_dumb())
+			/*
+			 * 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.1

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

[Junio C Hamano]

lars.schneider@autodesk.com writes:

> +		if (print_waiting_for_editor) {
> +			fprintf(stderr,
> +				_("hint: Waiting for your editor to close the file... "));
> +			if (is_terminal_dumb())
> +				/*
> +				 * A dumb terminal cannot erase the line later on. Add a
> +				 * newline to separate the hint from subsequent output.
> +				 */
> +				fprintf(stderr, "\n");
> +			fflush(stderr);
> +		}

Was the trailing whitespace at the end of the hint message intended?

If we expect the editor to spit out additional garbage on the line,
it would probably help to have that SP, but if that is why we have it
there, it probably should be done only when !is_terminal_dumb().

If the trailing SP is merely there by accident, then removal without
changing anything else is also OK.

I cannot tell which is the case, hence this comment.

Thanks.

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

[Lars Schneider]

> On 07 Dec 2017, at 16:43, Junio C Hamano <gitster@pobox.com> wrote:
> 
> lars.schneider@autodesk.com writes:
> 
>> +		if (print_waiting_for_editor) {
>> +			fprintf(stderr,
>> +				_("hint: Waiting for your editor to close the file... "));
>> +			if (is_terminal_dumb())
>> +				/*
>> +				 * A dumb terminal cannot erase the line later on. Add a
>> +				 * newline to separate the hint from subsequent output.
>> +				 */
>> +				fprintf(stderr, "\n");
>> +			fflush(stderr);
>> +		}
> 
> Was the trailing whitespace at the end of the hint message intended?
> 
> If we expect the editor to spit out additional garbage on the line,
> it would probably help to have that SP,

Argh. I forgot to mention that in the cover letter. Yes, I added
the whitespace intentionally for exactly that reason.


> but if that is why we have it
> there, it probably should be done only when !is_terminal_dumb().

That, of course, is correct. My intention was to make the code simpler
but I can see that people would be confused about the whitespace.

How about this?

			fprintf(stderr,
				_("hint: Waiting for your editor to close the file..."));
			if (is_terminal_dumb())
				/*
				 * A dumb terminal cannot erase the line later on. Add a
				 * newline to separate the hint from subsequent output.
				 */
				fprintf(stderr, "\n")
			else
				fprintf(stderr, " ")

Can you squash that if you like it?

Thanks,
Lars

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

[Lars Schneider]

> On 07 Dec 2017, at 16:48, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
> 
>> On 07 Dec 2017, at 16:43, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> lars.schneider@autodesk.com writes:
>> ...
> 
> How about this?
> 
> 			fprintf(stderr,
> 				_("hint: Waiting for your editor to close the file..."));
> 			if (is_terminal_dumb())
> 				/*
> 				 * A dumb terminal cannot erase the line later on. Add a
> 				 * newline to separate the hint from subsequent output.
> 				 */
> 				fprintf(stderr, "\n")
> 			else
> 				fprintf(stderr, " ")
> 

I forgot the ";" ... switching between programming languages ;-)

			if (is_terminal_dumb())
				/*
				 * A dumb terminal cannot erase the line later on. Add a
				 * newline to separate the hint from subsequent output.
				 */
				fprintf(stderr, "\n");
			else
				fprintf(stderr, " ");



> Can you squash that if you like it?
> 
> Thanks,
> Lars

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> Can you squash that if you like it?

I thought you also had to update the log message that you forgot to?

Here is the replacement I queued tentatively.

-- >8 --
From: Lars Schneider <larsxschneider@gmail.com>
Date: Thu, 7 Dec 2017 16:16:41 +0100
Subject: [PATCH] launch_editor(): indicate that Git waits for user input

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 might 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, if the terminal
supports erasing the last line.  Also, make sure that our message is
terminated with a whitespace so that any message the editor may show
upon starting up will be kept separate from our message.

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |  3 +++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 editor.c                 | 24 ++++++++++++++++++++++++
 4 files changed, 30 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfabaa..6ebc50eea8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,9 @@ advice.*::
 	addEmbeddedRepo::
 		Advice on what to do when you've accidentally added one
 		git repo inside of another.
+	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 d81e1cb742..af29d23e43 100644
--- a/advice.c
+++ b/advice.c
@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
+int advice_waiting_for_editor = 1;
 
 static struct {
 	const char *name;
@@ -38,6 +39,7 @@ static struct {
 	{ "objectnamewarning", &advice_object_name_warning },
 	{ "rmhints", &advice_rm_hints },
 	{ "addembeddedrepo", &advice_add_embedded_repo },
+	{ "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 c84a44531c..f7cbbd342f 100644
--- a/advice.h
+++ b/advice.h
@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
+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..8acce0dcd4 100644
--- a/editor.c
+++ b/editor.c
@@ -45,6 +45,23 @@ 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);
+
+		if (print_waiting_for_editor) {
+			/*
+			 * A dumb terminal cannot erase the line later on. Add a
+			 * newline to separate the hint from subsequent output.
+			 *
+			 * In case the editor emits further cruft after what
+			 * we wrote above, separate it from our message with SP.
+			 */
+			const char term = is_terminal_dumb() ? '\n' : ' ';
+
+			fprintf(stderr,
+				_("hint: Waiting for your editor to close the file...%c"),
+				term);
+			fflush(stderr);
+		}
 
 		p.argv = args;
 		p.env = env;
@@ -63,6 +80,13 @@ 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 && !is_terminal_dumb())
+			/*
+			 * 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.1-480-gbc5668f98a

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

[Lars Schneider]

> On 07 Dec 2017, at 17:30, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>>> Can you squash that if you like it?
> 
> I thought you also had to update the log message that you forgot to?
> 
> Here is the replacement I queued tentatively.

Perfect. Thanks a lot for your additional fixes!

- Lars

> 
> -- >8 --
> From: Lars Schneider <larsxschneider@gmail.com>
> Date: Thu, 7 Dec 2017 16:16:41 +0100
> Subject: [PATCH] launch_editor(): indicate that Git waits for user input
> 
> 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 might 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, if the terminal
> supports erasing the last line.  Also, make sure that our message is
> terminated with a whitespace so that any message the editor may show
> upon starting up will be kept separate from our message.
> 
> 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/config.txt |  3 +++
> advice.c                 |  2 ++
> advice.h                 |  1 +
> editor.c                 | 24 ++++++++++++++++++++++++
> 4 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9593bfabaa..6ebc50eea8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -351,6 +351,9 @@ advice.*::
> 	addEmbeddedRepo::
> 		Advice on what to do when you've accidentally added one
> 		git repo inside of another.
> +	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 d81e1cb742..af29d23e43 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
> int advice_object_name_warning = 1;
> int advice_rm_hints = 1;
> int advice_add_embedded_repo = 1;
> +int advice_waiting_for_editor = 1;
> 
> static struct {
> 	const char *name;
> @@ -38,6 +39,7 @@ static struct {
> 	{ "objectnamewarning", &advice_object_name_warning },
> 	{ "rmhints", &advice_rm_hints },
> 	{ "addembeddedrepo", &advice_add_embedded_repo },
> +	{ "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 c84a44531c..f7cbbd342f 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
> extern int advice_object_name_warning;
> extern int advice_rm_hints;
> extern int advice_add_embedded_repo;
> +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..8acce0dcd4 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -45,6 +45,23 @@ 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);
> +
> +		if (print_waiting_for_editor) {
> +			/*
> +			 * A dumb terminal cannot erase the line later on. Add a
> +			 * newline to separate the hint from subsequent output.
> +			 *
> +			 * In case the editor emits further cruft after what
> +			 * we wrote above, separate it from our message with SP.
> +			 */
> +			const char term = is_terminal_dumb() ? '\n' : ' ';
> +
> +			fprintf(stderr,
> +				_("hint: Waiting for your editor to close the file...%c"),
> +				term);
> +			fflush(stderr);
> +		}
> 
> 		p.argv = args;
> 		p.env = env;
> @@ -63,6 +80,13 @@ 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 && !is_terminal_dumb())
> +			/*
> +			 * 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.1-480-gbc5668f98a
> 

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

[Kaartic Sivaraam]

On Thursday 07 December 2017 10:00 PM, Junio C Hamano wrote:
> +
> +		if (print_waiting_for_editor) {
> +			/*
> +			 * A dumb terminal cannot erase the line later on. Add a
> +			 * newline to separate the hint from subsequent output.
> +			 *


> +			 * In case the editor emits further cruft after what
> +			 * we wrote above, separate it from our message with SP.

I guess this part of the comment could be improved a little. I currently 
interpret it as "See if the editor emits further cruft, print a space in 
that case". Though, it's not what we are doing. Something like the 
following, perhaps?

      In a non-dumb terminal, separate our message from further cruft
      that might be emitted by the editor with SP.



> +			 */
> +			const char term = is_terminal_dumb() ? '\n' : ' ';
> +
> +			fprintf(stderr,
> +				_("hint: Waiting for your editor to close the file...%c"),
> +				term);
> +			fflush(stderr);
> +		}
>   
>   		p.argv = args;
>   		p.env = env;
> @@ -63,6 +80,13 @@ 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 && !is_terminal_dumb())
> +			/*
> +			 * Go back to the beginning and erase the entire line to
> +			 * avoid wasting the vertical space.
> +			 */
> +			fputs("\r\033[K", stderr);
>   	}
>   
>   	if (!buffer)
> 

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

[Lars Schneider]

> On 07 Dec 2017, at 18:37, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:
> 
> On Thursday 07 December 2017 10:00 PM, Junio C Hamano wrote:
>> +
>> +		if (print_waiting_for_editor) {
>> +			/*
>> +			 * A dumb terminal cannot erase the line later on. Add a
>> +			 * newline to separate the hint from subsequent output.
>> +			 *
> 
> 
>> +			 * In case the editor emits further cruft after what
>> +			 * we wrote above, separate it from our message with SP.
> 
> I guess this part of the comment could be improved a little. I currently interpret it as "See if the editor emits further cruft, print a space in that case". Though, it's not what we are doing. Something like the following, perhaps?
> 
>     In a non-dumb terminal, separate our message from further cruft
>     that might be emitted by the editor with SP.

I see what you mean. My (non-native) language feeling tells me that
reordering the sentence might sound better:

				 * In a non-dumb terminal, separate our message with SP
				 * from further cruft that might be emitted by the editor.


@Junio: If you agree with the change, can you squash either of the new versions? 

Thanks,
Lars