[PATCH] winansi: support ESC [ K (erase in line)

To make use of it during a fetch, write() needs to be overridden, too.

Signed-off-by: Johannes Schindelin <redacted>
---

	This fixes the last bit of msysGit issue 124 for me:

		http://code.google.com/p/msysgit/issues/detail?id=124

	which annoyed me one time to many today.

	I had an earlier version which was smaller, but pretty hacky, in 
	that it checked if fprintf is #define'd in xwrite(), and had 
	special handling for that case.

	This patch is only slightly hacky, in that it assumes that you do 
	not try to output something that ends in an incomplete ESC [
	sequence.

 compat/mingw.h   |    2 ++
 compat/winansi.c |   51 +++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index b82903c..9aac4e1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -192,9 +192,11 @@ sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 int winansi_fputs(const char *str, FILE *stream);
 int winansi_printf(const char *format, ...) __attribute__((format (printf, 1, 2)));
 int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format (printf, 2, 3)));
+int winansi_write(int fd, const void *buf, size_t len);
 #define fputs winansi_fputs
 #define printf(...) winansi_printf(__VA_ARGS__)
 #define fprintf(...) winansi_fprintf(__VA_ARGS__)
+#define write winansi_write
 
 /*
  * git specific compatibility

diff --git a/compat/winansi.c b/compat/winansi.c
index e2d96df..a9d981c 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c

@@ -11,15 +11,13 @@
 #undef printf
 #undef fprintf
 #undef fputs
-/* TODO: write */
+#undef write
 
 /*
  ANSI codes used by git: m, K
 
  This file is git-specific. Therefore, this file does not attempt
  to implement any codes that are not used by git.
-
- TODO: K
 */
 
 static HANDLE console;

@@ -79,6 +77,20 @@ static void set_console_attr(void)
 	SetConsoleTextAttribute(console, attributes);
 }
 
+static void erase_in_line(void)
+{
+	CONSOLE_SCREEN_BUFFER_INFO sbi;
+
+	if (!console)
+		return;
+
+	GetConsoleScreenBufferInfo(console, &sbi);
+	FillConsoleOutputCharacterA(console, ' ',
+		sbi.dwSize.X - sbi.dwCursorPosition.X, sbi.dwCursorPosition,
+		NULL);
+}
+
+
 static const char *set_attr(const char *str)
 {
 	const char *func;

@@ -218,7 +230,7 @@ static const char *set_attr(const char *str)
 		set_console_attr();
 		break;
 	case 'K':
-		/* TODO */
+		erase_in_line();
 		break;
 	default:
 		/* Unsupported code */

@@ -228,13 +240,16 @@ static const char *set_attr(const char *str)
 	return func + 1;
 }
 
-static int ansi_emulate(const char *str, FILE *stream)
+static int ansi_emulate(const char *str, ssize_t len, FILE *stream)
 {
 	int rv = 0;
 	const char *pos = str;
 
-	while (*pos) {
-		pos = strstr(str, "\033[");
+	if (len < 0)
+		len = strlen(str);
+
+	while (rv < len) {
+		pos = memmem(str, len - rv, "\033[", 2);
 		if (pos) {
 			size_t len = pos - str;
 

@@ -254,10 +269,11 @@ static int ansi_emulate(const char *str, FILE *stream)
 			rv += pos - str;
 			str = pos;
 		} else {
-			rv += strlen(str);
-			fputs(str, stream);
-			return rv;
+			fwrite(str, len - rv, 1, stream);
+			return len;
 		}
+		if (rv > len)
+			die ("Invalid use of ESC [ sequence detected");
 	}
 	return rv;
 }

@@ -274,7 +290,7 @@ int winansi_fputs(const char *str, FILE *stream)
 	if (!console)
 		return fputs(str, stream);
 
-	rv = ansi_emulate(str, stream);
+	rv = ansi_emulate(str, -1, stream);
 
 	if (rv >= 0)
 		return 0;

@@ -309,7 +325,7 @@ static int winansi_vfprintf(FILE *stream, const char *format, va_list list)
 		len = vsnprintf(buf, len + 1, format, list);
 	}
 
-	rv = ansi_emulate(buf, stream);
+	rv = ansi_emulate(buf, -1, stream);
 
 	if (buf != small_buf)
 		free(buf);

@@ -343,3 +359,14 @@ int winansi_printf(const char *format, ...)
 
 	return rv;
 }
+
+int winansi_write(int fd, const void *buf, size_t len)
+{
+	if (isatty(fd)) {
+		init();
+		if (console)
+			return ansi_emulate((const char *)buf, len,
+					fd == 2 ? stderr : stdout);
+	}
+	return write(fd, buf, len);
+}

-- 
1.6.2.240.g23c7

Johannes Schindelin schrieb:

	This fixes the last bit of msysGit issue 124 for me:

		http://code.google.com/p/msysgit/issues/detail?id=124

	which annoyed me one time to many today.

	I had an earlier version which was smaller, but pretty hacky, in 
	that it checked if fprintf is #define'd in xwrite(), and had 
	special handling for that case.

	This patch is only slightly hacky, in that it assumes that you do 
	not try to output something that ends in an incomplete ESC [
	sequence.

Good that I read mail before I start hacking. I was about to do something
about this in a moment. ;)

To make use of it during a fetch, write() needs to be overridden, too.

No, that's not necessary with the patch that I'm about to send in a
moment. To replace write() for ANSI emulation really goes too far.

-- Hannes

From: Johannes Sixt <redacted>

This removes the last parameter of recv_sideband, by which the callers
told which channel band #2 data should be written to. Since both callers
of the function passed 2 for the parameter, we hereby remove the
parameter and send band #2 to stderr explicitly using fprintf.

This has the nice side-effect that the band #2 data (most importantly
progress reports during a fetch operation) passes through our ANSI
emulation layer on Windows.

Signed-off-by: Johannes Sixt <redacted>
---
Johannes Sixt schrieb:

Johannes Schindelin schrieb:

quoted

To make use of it during a fetch, write() needs to be overridden, too.

No, that's not necessary with the patch that I'm about to send in a
moment. To replace write() for ANSI emulation really goes too far.

Here it is. The patch is still RFC because I didn't have a chance, yet,
to test it in practice. It passes the test suite.

-- Hannes

 builtin-archive.c    |    2 +-
 builtin-fetch-pack.c |    2 +-
 sideband.c           |   20 +++++++++-----------
 sideband.h           |    2 +-
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 60adef9..ab50ceb 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c

@@ -52,7 +52,7 @@ static int run_remote_archiver(int argc, const char **argv,
 		die("git archive: expected a flush");

 	/* Now, start reading from fd[0] and spit it out to stdout */
-	rv = recv_sideband("archive", fd[0], 1, 2);
+	rv = recv_sideband("archive", fd[0], 1);
 	close(fd[0]);
 	close(fd[1]);
 	rv |= finish_connect(conn);

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index c2e5adc..2b36099 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c

@@ -482,7 +482,7 @@ static int sideband_demux(int fd, void *data)
 {
 	int *xd = data;

-	return recv_sideband("fetch-pack", xd[0], fd, 2);
+	return recv_sideband("fetch-pack", xd[0], fd);
 }

 static int get_pack(int xd[2], char **pack_lockfile)

diff --git a/sideband.c b/sideband.c
index cca3360..a706ac8 100644
--- a/sideband.c
+++ b/sideband.c

@@ -19,7 +19,7 @@

 #define FIX_SIZE 10  /* large enough for any of the above */

-int recv_sideband(const char *me, int in_stream, int out, int err)
+int recv_sideband(const char *me, int in_stream, int out)
 {
 	unsigned pf = strlen(PREFIX);
 	unsigned sf;

@@ -41,8 +41,7 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		if (len == 0)
 			break;
 		if (len < 1) {
-			len = sprintf(buf, "%s: protocol error: no band designator\n", me);
-			safe_write(err, buf, len);
+			fprintf(stderr, "%s: protocol error: no band designator\n", me);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 		band = buf[pf] & 0xff;

@@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		switch (band) {
 		case 3:
 			buf[pf] = ' ';
-			buf[pf+1+len] = '\n';
-			safe_write(err, buf, pf+1+len+1);
+			buf[pf+1+len] = '\0';
+			fprintf(stderr, "%s\n", buf);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
 			buf[pf] = ' ';

@@ -95,12 +94,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 					memcpy(save, b + brk, sf);
 					b[brk + sf - 1] = b[brk - 1];
 					memcpy(b + brk - 1, suffix, sf);
-					safe_write(err, b, brk + sf);
+					fprintf(stderr, "%.*s", brk + sf, b);
 					memcpy(b + brk, save, sf);
 					len -= brk;
 				} else {
 					int l = brk ? brk : len;
-					safe_write(err, b, l);
+					if (l > 0)
+						fprintf(stderr, "%.*s", l, b);
 					len -= l;
 				}

@@ -112,10 +112,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			safe_write(out, buf + pf+1, len);
 			continue;
 		default:
-			len = sprintf(buf,
-				      "%s: protocol error: bad band #%d\n",
-				      me, band);
-			safe_write(err, buf, len);
+			fprintf(stderr, "%s: protocol error: bad band #%d\n",
+				me, band);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 	}

diff --git a/sideband.h b/sideband.h
index a84b691..d72db35 100644
--- a/sideband.h
+++ b/sideband.h

@@ -7,7 +7,7 @@
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520

-int recv_sideband(const char *me, int in_stream, int out, int err);
+int recv_sideband(const char *me, int in_stream, int out);
 ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);

 #endif

-- 
1.6.2.987.g90c1d

Hi,

On Tue, 10 Mar 2009, Johannes Sixt wrote:

From: Johannes Sixt <redacted>

This removes the last parameter of recv_sideband, by which the callers
told which channel band #2 data should be written to. Since both callers
of the function passed 2 for the parameter, we hereby remove the
parameter and send band #2 to stderr explicitly using fprintf.

To be honest, I considered this myself.

But I think it is wrong.  Just because the current callers happen to 
output to stderr does not mean that we would not like sidebands that 
exchange binary data for other uses in the future.

I am thinking GitTorrent here.

And clearly, sideband support was written with future uses like that in 
mind, as it goes out of its way to transmit packets instead of strings.

Ciao,
Dscho

Johannes Schindelin schrieb:

On Tue, 10 Mar 2009, Johannes Sixt wrote:

quoted

This removes the last parameter of recv_sideband, by which the callers
told which channel band #2 data should be written to. Since both callers
of the function passed 2 for the parameter, we hereby remove the
parameter and send band #2 to stderr explicitly using fprintf.

To be honest, I considered this myself.

But I think it is wrong.  Just because the current callers happen to 
output to stderr does not mean that we would not like sidebands that 
exchange binary data for other uses in the future.

I am thinking GitTorrent here.

Clearly, we are looking at git here, not GitTorrent. "Because we could" is
IMNSHO not a good justification keep code unnecessarily complicated.

And clearly, sideband support was written with future uses like that in 
mind, as it goes out of its way to transmit packets instead of strings.

All data producers and data consumers *in git* use band #2 to transport
error messages and progress report. GitTorrent cannot not talk to
upload-pack or upload-archive and expect to get arbitrary binary data over
band #2.

For use-cases that you have in mind in GitTorrent, the *protocol* may be a
good choice, but the current implementation is definitely a special case.

-- Hannes

Johannes Sixt schrieb:

All data producers and data consumers *in git* use band #2 to transport
error messages and progress report. GitTorrent cannot not talk to
upload-pack or upload-archive and expect to get arbitrary binary data over
band #2.

For use-cases that you have in mind in GitTorrent, the *protocol* may be a
good choice, but the current implementation is definitely a special case.

And it really is: Did you notice that stuff that recv_sideband sends over
the channel named 'err' (before my patch) has "remote: " prepended on
every line? That's certainly not an implementation that you want if you
send binary data over that band!

-- Hannes

Hi,

On Tue, 10 Mar 2009, Johannes Sixt wrote:

Johannes Schindelin schrieb:

quoted

	This fixes the last bit of msysGit issue 124 for me:

		http://code.google.com/p/msysgit/issues/detail?id=124

	which annoyed me one time to many today.

	I had an earlier version which was smaller, but pretty hacky, in 
	that it checked if fprintf is #define'd in xwrite(), and had 
	special handling for that case.

	This patch is only slightly hacky, in that it assumes that you do 
	not try to output something that ends in an incomplete ESC [
	sequence.

Good that I read mail before I start hacking. I was about to do something
about this in a moment. ;)

Heh...

quoted

To make use of it during a fetch, write() needs to be overridden, too.

No, that's not necessary with the patch that I'm about to send in a
moment. To replace write() for ANSI emulation really goes too far.

See my response to your patch...

Ciao,
Dscho

Johannes Schindelin schrieb:

FWIW GitTorrent may be implemented as part of git-daemon, if Sam's ideas 
become reality.  And then, sideband transport is _the_ means to do 
asyncrounous communication while pushing bytes.

I do not see how recv_sideband() in its current form could be helpful here
(assuming that you really are thinking of sending binary data over band #2).

On Tue, 10 Mar 2009, Johannes Sixt wrote:

quoted

Johannes Sixt schrieb:

quoted

For use-cases that you have in mind in GitTorrent, the *protocol* may 
be a good choice, but the current implementation is definitely a 
special case.

And it really is: Did you notice that stuff that recv_sideband sends over
the channel named 'err' (before my patch) has "remote: " prepended on
every line? That's certainly not an implementation that you want if you
send binary data over that band!

Yes, that is unfortunate, but can be fixed easily.

I don't believe this. Every treatment of "remote: " that you take away
from recv_sideband() you must insert somewhere else. Perhaps easy, but
certainly not as trivial as my patch.


Just a reminder: You proposed to override write() on Windows in a
non-trivial way, and we are discussing the topic above because I think
that is not a good idea. The reasons are:

- write() is a fundamental operation, and we should not mess with it out
of caution.

- Your proposal is not a catch-all. For example, combine-diff.c uses
puts() in dump_quoted_path(). If your goal was to not touch code outside
of compat/ then you need to override at least puts(), too.

- All code that writes ANSI escapes should use fprintf() anyway.
(Currently that is not the case, but all cases I'm aware of can be fixed
trivially.)

-- Hannes

Hi,

On Tue, 10 Mar 2009, Johannes Sixt wrote:

Johannes Schindelin schrieb:

quoted

FWIW GitTorrent may be implemented as part of git-daemon, if Sam's 
ideas become reality.  And then, sideband transport is _the_ means to 
do asyncrounous communication while pushing bytes.

I do not see how recv_sideband() in its current form could be helpful 
here (assuming that you really are thinking of sending binary data over 
band #2).

I think it is a safe bet that the side band would be a good way to 
exchange updates to the mirror list as well as the refs list.

quoted

On Tue, 10 Mar 2009, Johannes Sixt wrote:

quoted

Johannes Sixt schrieb:

quoted

For use-cases that you have in mind in GitTorrent, the *protocol* 
may be a good choice, but the current implementation is definitely a 
special case.

And it really is: Did you notice that stuff that recv_sideband sends 
over the channel named 'err' (before my patch) has "remote: " 
prepended on every line? That's certainly not an implementation that 
you want if you send binary data over that band!

Yes, that is unfortunate, but can be fixed easily.

I don't believe this. Every treatment of "remote: " that you take away
from recv_sideband() you must insert somewhere else. Perhaps easy, but
certainly not as trivial as my patch.

AFAICT it would be a matter of

	unsigned pf = isatty(err) ? strlen(PREFIX) : 0;

Just a reminder: You proposed to override write() on Windows in a 
non-trivial way, and we are discussing the topic above because I think 
that is not a good idea. The reasons are:

- write() is a fundamental operation, and we should not mess with it out 
  of caution.

But we do not mess with it!  We ask explicitely if we are talking about a 
tty.

- Your proposal is not a catch-all. For example, combine-diff.c uses 
  puts() in dump_quoted_path(). If your goal was to not touch code 
  outside of compat/ then you need to override at least puts(), too.

From compat/mingw.h:

-- snip --
/*
 * ANSI emulation wrappers
 */

int winansi_fputs(const char *str, FILE *stream);
[...]
#define fputs winansi_fputs
-- snap --

... added in c09df8a, SOBbed by yourself ;-)

- All code that writes ANSI escapes should use fprintf() anyway.  
  (Currently that is not the case, but all cases I'm aware of can be 
  fixed trivially.)

I disagree that all ANSI escapes have to go through fprintf().  Sometimes 
you have a buffer, and I do not like doing extra work with %.*s there.

BTW I hope that you are not annoyed by the discussion; I think it is 
necessary and important.  I am certainly not married to my current POV; so 
far, I am still in favor of it, though.

Ciao,
Dscho

Hi,

On Tue, 10 Mar 2009, Peter Harris wrote:

On Mon, Mar 9, 2009 at 8:41 PM, Johannes Schindelin wrote:

quoted

+int winansi_write(int fd, const void *buf, size_t len)
+{
+       if (isatty(fd)) {
+               init();
+               if (console)
+                       return ansi_emulate((const char *)buf, len,
+                                       fd == 2 ? stderr : stdout);
+       }
+       return write(fd, buf, len);
+}

Switching an unbuffered write to a buffered fwrite makes me a little 
nervous. In practice, all writes probably go through this function, so 
it doesn't matter. And if the write is going somewhere it matters, it 
likely fails isatty anyway. But I would still be less nervous with an 
fflush() after ansi_emulate.

That would make the code substantially more messy, though.

Besides, half of it is already there: when encountering an ANSI sequence, 
your code already fflush()es the fd before setting the new attributes.  
And stderr does not need the fflush() anyway.

Ciao,
Dscho

Johannes Schindelin schrieb:

Hi,

On Tue, 10 Mar 2009, Johannes Sixt wrote:

quoted

Johannes Schindelin schrieb:

quoted

FWIW GitTorrent may be implemented as part of git-daemon, if Sam's 
ideas become reality.  And then, sideband transport is _the_ means to 
do asyncrounous communication while pushing bytes.

I do not see how recv_sideband() in its current form could be helpful 
here (assuming that you really are thinking of sending binary data over 
band #2).

I think it is a safe bet that the side band would be a good way to 
exchange updates to the mirror list as well as the refs list.

Binary or not - the purpose and suitability of the sideband *protocol* for
this task are undisputed.

But you don't want to have "remote:" thrown in at seemingly random places
in the demultiplexed stream that comes fromt he current implementation of
recv_sideband().

quoted

quoted

On Tue, 10 Mar 2009, Johannes Sixt wrote:

quoted

And it really is: Did you notice that stuff that recv_sideband sends 
over the channel named 'err' (before my patch) has "remote: " 
prepended on every line? That's certainly not an implementation that 
you want if you send binary data over that band!

Yes, that is unfortunate, but can be fixed easily.

I don't believe this. Every treatment of "remote: " that you take away
from recv_sideband() you must insert somewhere else. Perhaps easy, but
certainly not as trivial as my patch.

AFAICT it would be a matter of

	unsigned pf = isatty(err) ? strlen(PREFIX) : 0;

But don't you see that are mixing a high-level concept of "terminal" into
the low-level function that you want it to be? In its current form,
recv_sideband() is *not* a low-level utility, it's already at a high level
that knows about the line-oriented nature of band #2. What you need for
GitTorrent is a different function that *only* demultiplexes the sideband
protocol data into different streams without munging them. That's a
totally different function that *maybe* can share some code with the
current recv_sideband().

quoted

Just a reminder: You proposed to override write() on Windows in a 
non-trivial way, and we are discussing the topic above because I think 
that is not a good idea. The reasons are:

- write() is a fundamental operation, and we should not mess with it out 
  of caution.

But we do not mess with it!  We ask explicitely if we are talking about a 
tty.

With reference to Peter's reply, I'm not the only one who gets nervous if
write() is replaced in a non-trivial way.

After all, you are sneaking the high-level concept "terminal emulation"
into the low-level write() function.

quoted

- Your proposal is not a catch-all. For example, combine-diff.c uses 
  puts() in dump_quoted_path(). If your goal was to not touch code 
  outside of compat/ then you need to override at least puts(), too.

quoted

From compat/mingw.h:

-- snip --
/*
 * ANSI emulation wrappers
 */

int winansi_fputs(const char *str, FILE *stream);
[...]
#define fputs winansi_fputs
-- snap --

... added in c09df8a, SOBbed by yourself ;-)

My point was that you cannot get away without modifying code outside of
compat/ (if that was your motivation to override write()). I don't care
whether we change this instance to fputs() or fprintf(). But we already
*have* something, and don't need *yet another* override.

quoted

- All code that writes ANSI escapes should use fprintf() anyway.  
  (Currently that is not the case, but all cases I'm aware of can be 
  fixed trivially.)

I disagree that all ANSI escapes have to go through fprintf().  Sometimes 
you have a buffer, and I do not like doing extra work with %.*s there.

But on the other hand you risk breaking write() semantics and give us a
colorful mix of concepts.

I don't insist in that ANSI escapes must go through fprintf(), but they
should really not go through a level that is lower than stdio. Basic file
IO should really not be muddied with terminal emulation.

BTW I hope that you are not annoyed by the discussion; I think it is 
necessary and important.  I am certainly not married to my current POV; so 
far, I am still in favor of it, though.

I'm absolutely not annoyed. And I am as married to my POV as you are to
yours. ;) In this case we perhaps need a tie-breaker.

-- Hannes

Johannes Sixt [off-list ref] wrote:

But don't you see that are mixing a high-level concept of "terminal" into
the low-level function that you want it to be? In its current form,
recv_sideband() is *not* a low-level utility, it's already at a high level
that knows about the line-oriented nature of band #2. What you need for
GitTorrent is a different function that *only* demultiplexes the sideband
protocol data into different streams without munging them. That's a
totally different function that *maybe* can share some code with the
current recv_sideband().

ACK.

The definition of the streams in the current sideband protocol
are rather well defined for the one protocol that uses it,
fetch-pack/receive-pack:

  stream #1:  pack data
  stream #2:  stderr messages, progress, meant for tty
  stream #3:  oh-sh*t abort message, remote is dead, goodbye!

The stream number is encoded as a byte.  Anyone trying to reuse the
sideband protocol within the fetch-pack/receive-pack protocol to
carry *extra* data should use new channel numbers.  We have another
252 remaining.  I don't think we're lacking on description space.
 

With reference to Peter's reply, I'm not the only one who gets nervous if
write() is replaced in a non-trivial way.

After all, you are sneaking the high-level concept "terminal emulation"
into the low-level write() function.

Oh god.  Please do not do this.  I usually ignore the Win32 port
stuff.  But I agree with you Hannes, please do not do this.
 

I'm absolutely not annoyed. And I am as married to my POV as you are to
yours. ;) In this case we perhaps need a tie-breaker.

I cast my vote in with you Hannes.  Your original RFC patch made
sense as a cleanup in the code.

I agree with you that the way the code is currently structured
and used, sideband #2 should always dump to the stderr channel
of the process.  We also assume in a lot of other places that
fprintf(stderr, ...) is a good way to report progress to the
end-user.  This is no different, its just progress data from the
remote system rather than the local system.

Huge refactorings would be necessary to split sideband #2 into
something other than stderr.  You would also want to replace nearly
all references to stderr.  We're not going down that road.

FWIW, JGit takes the meanings of the streams as I described them
above.  Stream #1 goes to the pack processing, stream #2 gets parsed
and converted into updates calls to our ProgressMonitor API (which in
turn goes to GUI progress meters in the host IDE), stream #3, if it
ever shows up, turns into the message of an exception being thrown
up the stack.  That is the definition of this particular protocol.
Accept it, and move it.  :-)

The sideband protocol is fairly simple.  If we ever needed to use
it for other data, we could probably refactor some of the header
formatting/parsing out a bit and create a more generalized split
function, under a different name.  But that's all in the future
and something we just don't have an immediate need to worry about.
So don't worry about it.

-- 
Shawn.

Hi,

On Tue, 10 Mar 2009, Shawn O. Pearce wrote:

Johannes Sixt [off-list ref] wrote:

quoted

But don't you see that are mixing a high-level concept of "terminal" 
into the low-level function that you want it to be? In its current 
form, recv_sideband() is *not* a low-level utility, it's already at a 
high level that knows about the line-oriented nature of band #2. What 
you need for GitTorrent is a different function that *only* 
demultiplexes the sideband protocol data into different streams 
without munging them. That's a totally different function that *maybe* 
can share some code with the current recv_sideband().

ACK.

The definition of the streams in the current sideband protocol are 
rather well defined for the one protocol that uses it, 
fetch-pack/receive-pack:

  stream #1:  pack data
  stream #2:  stderr messages, progress, meant for tty
  stream #3:  oh-sh*t abort message, remote is dead, goodbye!

The stream number is encoded as a byte.  Anyone trying to reuse the 
sideband protocol within the fetch-pack/receive-pack protocol to carry 
*extra* data should use new channel numbers.  We have another 252 
remaining.  I don't think we're lacking on description space.

Fair enough, the tie-breaker hath spoken.

Ciao,
Dscho

Johannes Sixt [off-list ref] wrote:

From: Johannes Sixt <redacted>

This removes the last parameter of recv_sideband, by which the callers
told which channel band #2 data should be written to. Since both callers
of the function passed 2 for the parameter, we hereby remove the
parameter and send band #2 to stderr explicitly using fprintf.

This has the nice side-effect that the band #2 data (most importantly
progress reports during a fetch operation) passes through our ANSI
emulation layer on Windows.

Signed-off-by: Johannes Sixt <redacted>

Looks right to me.

Can't you instead do:

	fprintf(stderr, "%.*s\n", buf, pf + len);

like you do...

here?

-- 
Shawn.

Shawn O. Pearce schrieb:

Johannes Sixt [off-list ref] wrote:

quoted

diff --git a/sideband.c b/sideband.c
index cca3360..a706ac8 100644
--- a/sideband.c
+++ b/sideband.c

@@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		switch (band) {
 		case 3:
 			buf[pf] = ' ';
-			buf[pf+1+len] = '\n';
-			safe_write(err, buf, pf+1+len+1);
+			buf[pf+1+len] = '\0';
+			fprintf(stderr, "%s\n", buf);

Can't you instead do:

	fprintf(stderr, "%.*s\n", buf, pf + len);

like you do...

quoted

@@ -95,12 +94,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 					memcpy(save, b + brk, sf);
 					b[brk + sf - 1] = b[brk - 1];
 					memcpy(b + brk - 1, suffix, sf);
-					safe_write(err, b, brk + sf);
+					fprintf(stderr, "%.*s", brk + sf, b);
 					memcpy(b + brk, save, sf);
 					len -= brk;
 				} else {
 					int l = brk ? brk : len;
-					safe_write(err, b, l);
+					if (l > 0)
+						fprintf(stderr, "%.*s", l, b);

here?

I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
that we had yesterday about potentially misbehaved fprintf in the case
where the precision is 0; and (2) because it was so easy to avoid it. I
don't think we need ultimate performance in this case, and I also consider
the plain "%s\n" more readable.

That said, the second hunk is really only the minimal change and I'd like
to rewrite it to get rid of the memcpy stuff. It is really not needed once
fprintf is in the game. But that's a separate patch.

-- Hannes

Hi,

On Tue, 10 Mar 2009, Johannes Sixt wrote:

Shawn O. Pearce schrieb:

quoted

Johannes Sixt [off-list ref] wrote:

quoted

diff --git a/sideband.c b/sideband.c
index cca3360..a706ac8 100644
--- a/sideband.c
+++ b/sideband.c

@@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		switch (band) {
 		case 3:
 			buf[pf] = ' ';
-			buf[pf+1+len] = '\n';
-			safe_write(err, buf, pf+1+len+1);
+			buf[pf+1+len] = '\0';
+			fprintf(stderr, "%s\n", buf);

Can't you instead do:

	fprintf(stderr, "%.*s\n", buf, pf + len);

like you do...

quoted

@@ -95,12 +94,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 					memcpy(save, b + brk, sf);
 					b[brk + sf - 1] = b[brk - 1];
 					memcpy(b + brk - 1, suffix, sf);
-					safe_write(err, b, brk + sf);
+					fprintf(stderr, "%.*s", brk + sf, b);
 					memcpy(b + brk, save, sf);
 					len -= brk;
 				} else {
 					int l = brk ? brk : len;
-					safe_write(err, b, l);
+					if (l > 0)
+						fprintf(stderr, "%.*s", l, b);

here?

I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
that we had yesterday about potentially misbehaved fprintf in the case
where the precision is 0;

Didn't that turn out to be a false alarm?

and (2) because it was so easy to avoid it. I don't think we need 
ultimate performance in this case, and I also consider the plain "%s\n" 
more readable.

That said, the second hunk is really only the minimal change and I'd 
like to rewrite it to get rid of the memcpy stuff. It is really not 
needed once fprintf is in the game. But that's a separate patch.

I think, indeed, that you can avoid the memcpy() by using %.*s.  The 
private buffer is only used to make sure that the text is written in one 
go anyway (i.e. that two sidebands messages are not written to the same 
line because they use multiple calls to fprintf()/fwrite() per line), 
right?

Ciao,
Dscho

Johannes Schindelin schrieb:

On Tue, 10 Mar 2009, Johannes Sixt wrote:

quoted

I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
that we had yesterday about potentially misbehaved fprintf in the case
where the precision is 0;

Didn't that turn out to be a false alarm?

Even better. I didn't follow the thread very closely.

I think, indeed, that you can avoid the memcpy() by using %.*s.  The 
private buffer is only used to make sure that the text is written in one 
go anyway (i.e. that two sidebands messages are not written to the same 
line because they use multiple calls to fprintf()/fwrite() per line), 
right?

It must be something like that, yes. ;)

-- Hannes

On Tue, Mar 10, 2009 at 11:07 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
that we had yesterday about potentially misbehaved fprintf in the case
where the precision is 0;

Didn't that turn out to be a false alarm?

Yes. A party who will remain nameless was reading the wrong man page
section (printf(1) vs printf(3)). :-)

j.

On Tue, 10 Mar 2009, Johannes Sixt wrote:

Johannes Schindelin schrieb:

quoted

I think, indeed, that you can avoid the memcpy() by using %.*s.  The 
private buffer is only used to make sure that the text is written in one 
go anyway (i.e. that two sidebands messages are not written to the same 
line because they use multiple calls to fprintf()/fwrite() per line), 
right?

It must be something like that, yes. ;)

It is a bit more complex than that.

Messages received over sideband #2 may have two kinds of line break 
characters: \r or \n.  We also have a string suffix which is used to 
clear the remaining of the screen line in those cases where a previous 
line is overwritten and the previous line was longer.  That may happen 
if the previous line was terminated with \r.  In that case, the string 
suffix _must_ be inserted before the line break character for obvious 
reasons.


Nicolas

On Tue, 10 Mar 2009, Johannes Sixt wrote:

From: Johannes Sixt <redacted>

This removes the last parameter of recv_sideband, by which the callers
told which channel band #2 data should be written to. Since both callers
of the function passed 2 for the parameter, we hereby remove the
parameter and send band #2 to stderr explicitly using fprintf.

This has the nice side-effect that the band #2 data (most importantly
progress reports during a fetch operation) passes through our ANSI
emulation layer on Windows.

You appear to modify band #3 as well.  Better mention it in the commit 
log.

Signed-off-by: Johannes Sixt <redacted>

Acked-by: Nicolas Pitre <redacted>

This removes the last parameter of recv_sideband, by which the callers
told which channel bands #2 and #3 should be written to.

Sayeth Shawn Pearce:

   The definition of the streams in the current sideband protocol
   are rather well defined for the one protocol that uses it,
   fetch-pack/receive-pack:

     stream #1:  pack data
     stream #2:  stderr messages, progress, meant for tty
     stream #3:  abort message, remote is dead, goodbye!

Since both callers of the function passed 2 for the parameter, we hereby
remove it and send bands #2 and #3 to stderr explicitly using fprintf.

This has the nice side-effect that these two streams pass through our
ANSI emulation layer on Windows.

Signed-off-by: Johannes Sixt <redacted>
Acked-by: Nicolas Pitre <redacted>
---
 This patch has an updated commit message and this small change:

  				} else {
  					int l = brk ? brk : len;
 -					if (l > 0)
 -						fprintf(stderr, "%.*s", l, b);
 +					fprintf(stderr, "%.*s", l, b);
  					len -= l;
  				}

 I also has undergone some practical tests in addition to the
 test suite.

 -- Hannes

 builtin-archive.c    |    2 +-
 builtin-fetch-pack.c |    2 +-
 sideband.c           |   19 ++++++++-----------
 sideband.h           |    2 +-
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 60adef9..ab50ceb 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c

@@ -52,7 +52,7 @@ static int run_remote_archiver(int argc, const char **argv,
 		die("git archive: expected a flush");
 
 	/* Now, start reading from fd[0] and spit it out to stdout */
-	rv = recv_sideband("archive", fd[0], 1, 2);
+	rv = recv_sideband("archive", fd[0], 1);
 	close(fd[0]);
 	close(fd[1]);
 	rv |= finish_connect(conn);

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index c2e5adc..2b36099 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c

@@ -482,7 +482,7 @@ static int sideband_demux(int fd, void *data)
 {
 	int *xd = data;
 
-	return recv_sideband("fetch-pack", xd[0], fd, 2);
+	return recv_sideband("fetch-pack", xd[0], fd);
 }
 
 static int get_pack(int xd[2], char **pack_lockfile)

diff --git a/sideband.c b/sideband.c
index cca3360..899b1ff 100644
--- a/sideband.c
+++ b/sideband.c

@@ -19,7 +19,7 @@
 
 #define FIX_SIZE 10  /* large enough for any of the above */
 
-int recv_sideband(const char *me, int in_stream, int out, int err)
+int recv_sideband(const char *me, int in_stream, int out)
 {
 	unsigned pf = strlen(PREFIX);
 	unsigned sf;

@@ -41,8 +41,7 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		if (len == 0)
 			break;
 		if (len < 1) {
-			len = sprintf(buf, "%s: protocol error: no band designator\n", me);
-			safe_write(err, buf, len);
+			fprintf(stderr, "%s: protocol error: no band designator\n", me);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 		band = buf[pf] & 0xff;

@@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		switch (band) {
 		case 3:
 			buf[pf] = ' ';
-			buf[pf+1+len] = '\n';
-			safe_write(err, buf, pf+1+len+1);
+			buf[pf+1+len] = '\0';
+			fprintf(stderr, "%s\n", buf);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
 			buf[pf] = ' ';

@@ -95,12 +94,12 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 					memcpy(save, b + brk, sf);
 					b[brk + sf - 1] = b[brk - 1];
 					memcpy(b + brk - 1, suffix, sf);
-					safe_write(err, b, brk + sf);
+					fprintf(stderr, "%.*s", brk + sf, b);
 					memcpy(b + brk, save, sf);
 					len -= brk;
 				} else {
 					int l = brk ? brk : len;
-					safe_write(err, b, l);
+					fprintf(stderr, "%.*s", l, b);
 					len -= l;
 				}
 

@@ -112,10 +111,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			safe_write(out, buf + pf+1, len);
 			continue;
 		default:
-			len = sprintf(buf,
-				      "%s: protocol error: bad band #%d\n",
-				      me, band);
-			safe_write(err, buf, len);
+			fprintf(stderr, "%s: protocol error: bad band #%d\n",
+				me, band);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 	}

diff --git a/sideband.h b/sideband.h
index a84b691..d72db35 100644
--- a/sideband.h
+++ b/sideband.h

@@ -7,7 +7,7 @@
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
 
-int recv_sideband(const char *me, int in_stream, int out, int err);
+int recv_sideband(const char *me, int in_stream, int out);
 ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif

-- 
1.6.2.103.gb3eac.dirty

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
 This is your patch without the write() part.

 -- Hannes

 compat/winansi.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/compat/winansi.c b/compat/winansi.c
index e2d96df..44dc293 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c

@@ -18,8 +18,6 @@
 
  This file is git-specific. Therefore, this file does not attempt
  to implement any codes that are not used by git.
-
- TODO: K
 */
 
 static HANDLE console;

@@ -79,6 +77,20 @@ static void set_console_attr(void)
 	SetConsoleTextAttribute(console, attributes);
 }
 
+static void erase_in_line(void)
+{
+	CONSOLE_SCREEN_BUFFER_INFO sbi;
+
+	if (!console)
+		return;
+
+	GetConsoleScreenBufferInfo(console, &sbi);
+	FillConsoleOutputCharacterA(console, ' ',
+		sbi.dwSize.X - sbi.dwCursorPosition.X, sbi.dwCursorPosition,
+		NULL);
+}
+
+
 static const char *set_attr(const char *str)
 {
 	const char *func;

@@ -218,7 +230,7 @@ static const char *set_attr(const char *str)
 		set_console_attr();
 		break;
 	case 'K':
-		/* TODO */
+		erase_in_line();
 		break;
 	default:
 		/* Unsupported code */

-- 
1.6.2.103.gb3eac.dirty

Hi,

On Tue, 10 Mar 2009, Johannes Sixt wrote:

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Johannes Sixt <redacted>
---
 This is your patch without the write() part.

Thanks!  All of a sudden, this patch does not look hacky anymore...

Ciao,
Dscho

Hi,

FWIW GitTorrent may be implemented as part of git-daemon, if Sam's ideas 
become reality.  And then, sideband transport is _the_ means to do 
asyncrounous communication while pushing bytes.

On Tue, 10 Mar 2009, Johannes Sixt wrote:

Johannes Sixt schrieb:

quoted

All data producers and data consumers *in git* use band #2 to 
transport error messages and progress report. GitTorrent cannot not 
talk to upload-pack or upload-archive and expect to get arbitrary 
binary data over band #2.

For use-cases that you have in mind in GitTorrent, the *protocol* may 
be a good choice, but the current implementation is definitely a 
special case.

And it really is: Did you notice that stuff that recv_sideband sends over
the channel named 'err' (before my patch) has "remote: " prepended on
every line? That's certainly not an implementation that you want if you
send binary data over that band!

Yes, that is unfortunate, but can be fixed easily.

However, I disagree to "fix" something that is working, even if it might 
be more complicated than currently necessary.

Ciao,
Dscho

On Mon, Mar 9, 2009 at 8:41 PM, Johannes Schindelin wrote:

To make use of it during a fetch, write() needs to be overridden, too.

Thanks for looking at this.

Sorry for leaving this part TODO for so long. Your version is
considerably less hacky than the version I had in my head, too.

+int winansi_write(int fd, const void *buf, size_t len)
+{
+       if (isatty(fd)) {
+               init();
+               if (console)
+                       return ansi_emulate((const char *)buf, len,
+                                       fd == 2 ? stderr : stdout);
+       }
+       return write(fd, buf, len);
+}

Switching an unbuffered write to a buffered fwrite makes me a little
nervous. In practice, all writes probably go through this function, so
it doesn't matter. And if the write is going somewhere it matters, it
likely fails isatty anyway. But I would still be less nervous with an
fflush() after ansi_emulate.

Peter Harris