threads / patch / 10631

patchfix display overlap between remote and local progress

Subject: [PATCH] fix display overlap between remote and local progress

## tl;dr

6 messages between Nov 4, 2007 and Mar 30, 2012. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Nicolas Pitre· Nov 4, 2007, 04:22 UTC · lore

It is possible for the remote summary line to be displayed over the local progress display line, and therefore that local progress gets bumped to the next line. However, if the progress line is long enough, it might not be entirely overwritten by the remote summary line. This creates a messed up display such as:

	remote: Total 310 (delta 160), reused 178 (delta 112)iB/s
	Receiving objects: 100% (310/310), 379.98 KiB | 136 KiB/s, done.

So we have to clear the screen line before displaying the remote message to make sure the local progress is not visible anymore on the first line.

Yet some Git versions on the remote side might be sending updates to the same line and terminate it with \r, and a separate packet with a single \n might be sent later when the progress display is done. This means the screen line must *not* be cleared in that case.

Since the sideband code already has to figure out line breaks in the received packet to properly prepend the "remote:" prefix, we can easily determine if the remote line about to be displayed is empty. Only when it is not then a proper suffix is inserted before the \r or \n to clear the end of the screen line.

Also some magic constants related to the prefix length have been replaced with a variable, making it similar to the suffix length handling. Since gcc is smart enough to detect that the variable is constant there is no impact on the generated code.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Show changes to sideband.c +37 −14
diff --git a/sideband.c b/sideband.c
index ab8a1e9..66385ad 100644
--- a/sideband.c
+++ b/sideband.c
@@ -11,13 +11,19 @@
  * stream, aka "verbose").  A message over band #3 is a signal that
  * the remote died unexpectedly.  A flush() concludes the stream.
  */
+
+#define PREFIX "remote:"
+#define SUFFIX "\e[K"  /* change to "        " if ANSI sequences don't work */ 
+
 int recv_sideband(const char *me, int in_stream, int out, int err)
 {
-	char buf[7 + LARGE_PACKET_MAX + 1];
-	strcpy(buf, "remote:");
+	unsigned pf = strlen(PREFIX);
+	unsigned sf = strlen(SUFFIX);
+	char buf[pf + LARGE_PACKET_MAX + sf + 1];
+	memcpy(buf, PREFIX, pf);
 	while (1) {
 		int band, len;
-		len = packet_read_line(in_stream, buf+7, LARGE_PACKET_MAX);
+		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
 		if (len == 0)
 			break;
 		if (len < 1) {
@@ -25,35 +31,52 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			safe_write(err, buf, len);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
-		band = buf[7] & 0xff;
+		band = buf[pf] & 0xff;
 		len--;
 		switch (band) {
 		case 3:
-			buf[7] = ' ';
-			buf[8+len] = '\n';
-			safe_write(err, buf, 8+len+1);
+			buf[pf] = ' ';
+			buf[pf+1+len] = '\n';
+			safe_write(err, buf, pf+1+len+1);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
-			buf[7] = ' ';
-			len += 8;
+			buf[pf] = ' ';
+			len += pf+1;
 			while (1) {
-				int brk = 8;
+				int brk = pf+1;
+
+				/* Break the buffer into separate lines. */
 				while (brk < len) {
 					brk++;
 					if (buf[brk-1] == '\n' ||
 					    buf[brk-1] == '\r')
 						break;
 				}
-				safe_write(err, buf, brk);
+
+				/*
+				 * Let's insert a suffix to clear the end
+				 * of the screen line, but only if current
+				 * line data actually contains something.
+				 */
+				if (brk > pf+1 + 1) {
+					char save[sf];
+					memcpy(save, buf + brk, sf);
+					buf[brk + sf - 1] = buf[brk - 1];
+					memcpy(buf + brk - 1, SUFFIX, sf);
+					safe_write(err, buf, brk + sf);
+					memcpy(buf + brk, save, sf);
+				} else
+					safe_write(err, buf, brk);
+
 				if (brk < len) {
-					memmove(buf + 8, buf + brk, len - brk);
-					len = len - brk + 8;
+					memmove(buf + pf+1, buf + brk, len - brk);
+					len = len - brk + pf+1;
 				} else
 					break;
 			}
 			continue;
 		case 1:
-			safe_write(out, buf+8, len);
+			safe_write(out, buf + pf+1, len);
 			continue;
 		default:
 			len = sprintf(buf,
Johannes Schindelin· Nov 4, 2007, 13:33 UTC · re: Nicolas Pitre · lore

Re: [PATCH] fix display overlap between remote and local progress

Hi,
On Sun, 4 Nov 2007, Nicolas Pitre wrote:
> +#define SUFFIX "\e[K"  /* change to "        " if ANSI sequences don't work */ 

I am almost certain (without even testing) that cmd.exe has problems with that. It does not even understand colorisation.

My vote is to let it be for the moment, and us msysGit people will probably add something like NO_ANSI_TERM=YesPlease later.

Ciao, Dscho

Nicolas Pitre· Nov 4, 2007, 21:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fix display overlap between remote and local progress

On Sun, 4 Nov 2007, Johannes Schindelin wrote:
Show 8 quoted lines
> Hi,
> 
> On Sun, 4 Nov 2007, Nicolas Pitre wrote:
> 
> > +#define SUFFIX "\e[K"  /* change to "        " if ANSI sequences don't work */ 
> 
> I am almost certain (without even testing) that cmd.exe has problems with 
> that.  It does not even understand colorisation.

That's what I was expecting. This is why I suggested an alternative in the comment.

Nicolas
Junio C Hamano· Nov 4, 2007, 23:11 UTC · re: Nicolas Pitre · lore

Re: [PATCH] fix display overlap between remote and local progress

Nicolas Pitre <nico@cam.org> writes:
Show 11 quoted lines
> On Sun, 4 Nov 2007, Johannes Schindelin wrote:
>
>> On Sun, 4 Nov 2007, Nicolas Pitre wrote:
>> 
>> > +#define SUFFIX "\e[K"  /* change to "        " if ANSI sequences don't work */ 
>> 
>> I am almost certain (without even testing) that cmd.exe has problems with 
>> that.  It does not even understand colorisation.
>
> That's what I was expecting.  This is why I suggested an alternative in 
> the comment.
That's fine --- cmd.exe weenies can patch it away ;-).
The compiler at k.org complains of "\e" being non ISO-C, though.
Nicolas Pitre· Nov 5, 2007, 01:07 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix display overlap between remote and local progress

On Sun, 4 Nov 2007, Junio C Hamano wrote:
> The compiler at k.org complains of "\e" being non ISO-C, though.
Bummer.
...
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Show changes to sideband.c +1 −1
diff --git a/sideband.c b/sideband.c
index 58edea6..756bbc2 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,7 +13,7 @@
  */
 
 #define PREFIX "remote:"
-#define SUFFIX "\e[K"  /* change to "        " if ANSI sequences don't work */
+#define SUFFIX "\033[K"  /* change to "        " if ANSI sequences don't work */
 
 int recv_sideband(const char *me, int in_stream, int out, int err)
 {
Marcin Cieslak· Mar 30, 2012, 14:56 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fix display overlap between remote and local progress

(This is about ANSI clear-eol sequence introduced to sideline.c in commit
 ebe8fa738dcf6911fe520adce0cfa0cb26dee5e2)
Show 12 quoted lines
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 4 Nov 2007, Nicolas Pitre wrote:
>
>> +#define SUFFIX "\e[K"  /* change to "        " if ANSI sequences don't work */ 
>
> I am almost certain (without even testing) that cmd.exe has problems with 
> that.  It does not even understand colorisation.
>
> My vote is to let it be for the moment, and us msysGit people will 
> probably add something like NO_ANSI_TERM=YesPlease later.
Hi,

We are using script(1) to record the workflow and troubleshoot problems. script utility records all control codes as well (actually everything that passes via pseudoterminal it creates.

We managed to get git quiet on most control codes with color.ui=false and pager set to nothing, but we still get ESC [ K codes from the remote output (we are using gerrit and it generates some).

While I can live with git progress output lines in the log, it's quite annoying to edit those ANSI sequences out of the script. One has to remember to set TERM=dumb before invoking _some_ git operations.

Is there any better solution possible?
//Marcin

← back to recent threads