[PATCH] Refactor recv_sideband()

[PATCH] Refactor recv_sideband()

[Lukas Fleischer]

Improve the readability of recv_sideband() significantly by replacing
fragile buffer manipulations with more sophisticated format strings.
Also, reorganize the overall control flow, remove some superfluous
variables and replace a custom implementation of strpbrk() with a call
to the standard C library function.

Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
I had a really hard time reading and understanding this function when I
came up with my last patch. What I ended up with is almost a complete
rewrite of recv_sideband() and I find the end result to be much more
readable than what we have now. Given that this is quite invasive, it
would be good to have some more eyes and opinions...

If you want me to split this patch into smaller changes, please let me
know. However, finding a good way to split this into logical changes
might not be easy given that the new code does not have much in common
with what we had before.

 sideband.c | 94 ++++++++++++++++++++------------------------------------------
 1 file changed, 30 insertions(+), 64 deletions(-)

diff --git a/sideband.c b/sideband.c
index fde8adc..0a078c3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,103 +13,69 @@
  * the remote died unexpectedly.  A flush() concludes the stream.
  */
 
-#define PREFIX "remote:"
+#define PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-#define FIX_SIZE 10  /* large enough for any of the above */
-
 int recv_sideband(const char *me, int in_stream, int out)
 {
-	unsigned pf = strlen(PREFIX);
-	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
-	char *suffix, *term;
-	int skip_pf = 0;
+	const char *term;
+	const char *prefix = PREFIX, *suffix;
+	char buf[LARGE_PACKET_MAX + 1];
+	const char *b, *brk;
 
-	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-	sf = strlen(suffix);
 
 	while (1) {
 		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
 			fprintf(stderr, "%s: protocol error: no band designator\n", me);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
-		band = buf[pf] & 0xff;
+		band = buf[0] & 0xff;
+		buf[len] = '\0';
 		len--;
 		switch (band) {
 		case 3:
-			buf[pf] = ' ';
-			buf[pf+1+len] = '\0';
-			fprintf(stderr, "%s\n", buf);
+			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
-			buf[pf] = ' ';
-			do {
-				char *b = buf;
-				int brk = 0;
-
-				/*
-				 * If the last buffer didn't end with a line
-				 * break then we should not print a prefix
-				 * this time around.
-				 */
-				if (skip_pf) {
-					b += pf+1;
-				} else {
-					len += pf+1;
-					brk += pf+1;
-				}
+			b = buf + 1;
 
-				/* Look for a line break. */
-				for (;;) {
-					brk++;
-					if (brk > len) {
-						brk = 0;
-						break;
-					}
-					if (b[brk-1] == '\n' ||
-					    b[brk-1] == '\r')
-						break;
-				}
+			/*
+			 * Append a suffix to each nonempty line to clear the
+			 * end of the screen line.
+			 */
+			while ((brk = strpbrk(b, "\n\r"))) {
+				int linelen = brk - b;
 
-				/*
-				 * Let's insert a suffix to clear the end
-				 * of the screen line if a line break was
-				 * found.  Also, if we don't skip the
-				 * prefix, then a non-empty string must be
-				 * present too.
-				 */
-				if (brk > (skip_pf ? 0 : (pf+1 + 1))) {
-					char save[FIX_SIZE];
-					memcpy(save, b + brk, sf);
-					b[brk + sf - 1] = b[brk - 1];
-					memcpy(b + brk - 1, suffix, sf);
-					fprintf(stderr, "%.*s", brk + sf, b);
-					memcpy(b + brk, save, sf);
-					len -= brk;
+				if (linelen > 0) {
+					fprintf(stderr, "%s%.*s%s%c", prefix,
+						linelen, b, suffix, *brk);
 				} else {
-					int l = brk ? brk : len;
-					fprintf(stderr, "%.*s", l, b);
-					len -= l;
+					fprintf(stderr, "%s%c", prefix, *brk);
 				}
 
-				skip_pf = !brk;
-				memmove(buf + pf+1, b + brk, len);
-			} while (len);
+				b = brk + 1;
+				prefix = PREFIX;
+			}
+
+			if (*b) {
+				fprintf(stderr, "%s%s", prefix, b);
+				/* Incomplete line, skip the next prefix. */
+				prefix = "";
+			}
 			continue;
 		case 1:
-			write_or_die(out, buf + pf+1, len);
+			write_or_die(out, buf + 1, len);
 			continue;
 		default:
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
-- 
2.8.3

Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre]

On Mon, 13 Jun 2016, Lukas Fleischer wrote:

> Improve the readability of recv_sideband() significantly by replacing
> fragile buffer manipulations with more sophisticated format strings.
> Also, reorganize the overall control flow, remove some superfluous
> variables and replace a custom implementation of strpbrk() with a call
> to the standard C library function.
> 
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>

The previous code was a total abomination, even if I happen to know who 
wrote it.

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> I had a really hard time reading and understanding this function when I
> came up with my last patch. What I ended up with is almost a complete
> rewrite of recv_sideband() and I find the end result to be much more
> readable than what we have now. Given that this is quite invasive, it
> would be good to have some more eyes and opinions...
> 
> If you want me to split this patch into smaller changes, please let me
> know. However, finding a good way to split this into logical changes
> might not be easy given that the new code does not have much in common
> with what we had before.

Indeed. Anyway, numbers speak for themselves:

>  1 file changed, 30 insertions(+), 64 deletions(-)
> 
> diff --git a/sideband.c b/sideband.c
> index fde8adc..0a078c3 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -13,103 +13,69 @@
>   * the remote died unexpectedly.  A flush() concludes the stream.
>   */
>  
> -#define PREFIX "remote:"
> +#define PREFIX "remote: "
>  
>  #define ANSI_SUFFIX "\033[K"
>  #define DUMB_SUFFIX "        "
>  
> -#define FIX_SIZE 10  /* large enough for any of the above */
> -
>  int recv_sideband(const char *me, int in_stream, int out)
>  {
> -	unsigned pf = strlen(PREFIX);
> -	unsigned sf;
> -	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
> -	char *suffix, *term;
> -	int skip_pf = 0;
> +	const char *term;
> +	const char *prefix = PREFIX, *suffix;
> +	char buf[LARGE_PACKET_MAX + 1];
> +	const char *b, *brk;
>  
> -	memcpy(buf, PREFIX, pf);
>  	term = getenv("TERM");
>  	if (isatty(2) && term && strcmp(term, "dumb"))
>  		suffix = ANSI_SUFFIX;
>  	else
>  		suffix = DUMB_SUFFIX;
> -	sf = strlen(suffix);
>  
>  	while (1) {
>  		int band, len;
> -		len = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);
> +		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
>  		if (len == 0)
>  			break;
>  		if (len < 1) {
>  			fprintf(stderr, "%s: protocol error: no band designator\n", me);
>  			return SIDEBAND_PROTOCOL_ERROR;
>  		}
> -		band = buf[pf] & 0xff;
> +		band = buf[0] & 0xff;
> +		buf[len] = '\0';
>  		len--;
>  		switch (band) {
>  		case 3:
> -			buf[pf] = ' ';
> -			buf[pf+1+len] = '\0';
> -			fprintf(stderr, "%s\n", buf);
> +			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
>  			return SIDEBAND_REMOTE_ERROR;
>  		case 2:
> -			buf[pf] = ' ';
> -			do {
> -				char *b = buf;
> -				int brk = 0;
> -
> -				/*
> -				 * If the last buffer didn't end with a line
> -				 * break then we should not print a prefix
> -				 * this time around.
> -				 */
> -				if (skip_pf) {
> -					b += pf+1;
> -				} else {
> -					len += pf+1;
> -					brk += pf+1;
> -				}
> +			b = buf + 1;
>  
> -				/* Look for a line break. */
> -				for (;;) {
> -					brk++;
> -					if (brk > len) {
> -						brk = 0;
> -						break;
> -					}
> -					if (b[brk-1] == '\n' ||
> -					    b[brk-1] == '\r')
> -						break;
> -				}
> +			/*
> +			 * Append a suffix to each nonempty line to clear the
> +			 * end of the screen line.
> +			 */
> +			while ((brk = strpbrk(b, "\n\r"))) {
> +				int linelen = brk - b;
>  
> -				/*
> -				 * Let's insert a suffix to clear the end
> -				 * of the screen line if a line break was
> -				 * found.  Also, if we don't skip the
> -				 * prefix, then a non-empty string must be
> -				 * present too.
> -				 */
> -				if (brk > (skip_pf ? 0 : (pf+1 + 1))) {
> -					char save[FIX_SIZE];
> -					memcpy(save, b + brk, sf);
> -					b[brk + sf - 1] = b[brk - 1];
> -					memcpy(b + brk - 1, suffix, sf);
> -					fprintf(stderr, "%.*s", brk + sf, b);
> -					memcpy(b + brk, save, sf);
> -					len -= brk;
> +				if (linelen > 0) {
> +					fprintf(stderr, "%s%.*s%s%c", prefix,
> +						linelen, b, suffix, *brk);
>  				} else {
> -					int l = brk ? brk : len;
> -					fprintf(stderr, "%.*s", l, b);
> -					len -= l;
> +					fprintf(stderr, "%s%c", prefix, *brk);
>  				}
>  
> -				skip_pf = !brk;
> -				memmove(buf + pf+1, b + brk, len);
> -			} while (len);
> +				b = brk + 1;
> +				prefix = PREFIX;
> +			}
> +
> +			if (*b) {
> +				fprintf(stderr, "%s%s", prefix, b);
> +				/* Incomplete line, skip the next prefix. */
> +				prefix = "";
> +			}
>  			continue;
>  		case 1:
> -			write_or_die(out, buf + pf+1, len);
> +			write_or_die(out, buf + 1, len);
>  			continue;
>  		default:
>  			fprintf(stderr, "%s: protocol error: bad band #%d\n",
> -- 
> 2.8.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Refactor recv_sideband()

[Johannes Schindelin]

Hi,

On Mon, 13 Jun 2016, Nicolas Pitre wrote:

> On Mon, 13 Jun 2016, Lukas Fleischer wrote:
> 
> > Improve the readability of recv_sideband() significantly by replacing
> > fragile buffer manipulations with more sophisticated format strings.
> > Also, reorganize the overall control flow, remove some superfluous
> > variables and replace a custom implementation of strpbrk() with a call
> > to the standard C library function.
> > 
> > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> 
> The previous code was a total abomination, even if I happen to know who 
> wrote it.

Let's give Junio a break, okay? He does a kick-ass job at maintaining Git.
What we see here is simply good software development, nothing more,
nothing less: an initial, working code being improved. No need to make the
original author feel bad... :-)

Ciao,
Dscho

Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 14 Jun 2016, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 13 Jun 2016, Nicolas Pitre wrote:
> 
> > On Mon, 13 Jun 2016, Lukas Fleischer wrote:
> > 
> > > Improve the readability of recv_sideband() significantly by replacing
> > > fragile buffer manipulations with more sophisticated format strings.
> > > Also, reorganize the overall control flow, remove some superfluous
> > > variables and replace a custom implementation of strpbrk() with a call
> > > to the standard C library function.
> > > 
> > > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> > 
> > The previous code was a total abomination, even if I happen to know who 
> > wrote it.
> 
> Let's give Junio a break, okay? He does a kick-ass job at maintaining Git.
> What we see here is simply good software development, nothing more,
> nothing less: an initial, working code being improved. No need to make the
> original author feel bad... :-)

In case my sarcasm wasn't clear, _I_ am the author of the alluded 
abomination.


Nicolas

Re: [PATCH] Refactor recv_sideband()

[Johannes Schindelin]

Hi Nico,

On Tue, 14 Jun 2016, Nicolas Pitre wrote:

> On Tue, 14 Jun 2016, Johannes Schindelin wrote:
> 
> > On Mon, 13 Jun 2016, Nicolas Pitre wrote:
> > 
> > > On Mon, 13 Jun 2016, Lukas Fleischer wrote:
> > > 
> > > > Improve the readability of recv_sideband() significantly by
> > > > replacing fragile buffer manipulations with more sophisticated
> > > > format strings.  Also, reorganize the overall control flow, remove
> > > > some superfluous variables and replace a custom implementation of
> > > > strpbrk() with a call to the standard C library function.
> > > > 
> > > > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> > > 
> > > The previous code was a total abomination, even if I happen to know
> > > who wrote it.
> > 
> > Let's give Junio a break, okay? He does a kick-ass job at maintaining
> > Git.  What we see here is simply good software development, nothing
> > more, nothing less: an initial, working code being improved. No need
> > to make the original author feel bad... :-)
> 
> In case my sarcasm wasn't clear, _I_ am the author of the alluded
> abomination.

Sorry, I did not catch that. I just looked at
583b7ea31b7c16f872b178d541591ab816d16f85 and felt that we could be nicer
to Junio...

Ciao,
Johannes

Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 14 Jun 2016, Johannes Schindelin wrote:

> Hi Nico,
> 
> On Tue, 14 Jun 2016, Nicolas Pitre wrote:
> 
> > On Tue, 14 Jun 2016, Johannes Schindelin wrote:
> > 
> > > On Mon, 13 Jun 2016, Nicolas Pitre wrote:
> > > 
> > > > On Mon, 13 Jun 2016, Lukas Fleischer wrote:
> > > > 
> > > > > Improve the readability of recv_sideband() significantly by
> > > > > replacing fragile buffer manipulations with more sophisticated
> > > > > format strings.  Also, reorganize the overall control flow, remove
> > > > > some superfluous variables and replace a custom implementation of
> > > > > strpbrk() with a call to the standard C library function.
> > > > > 
> > > > > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> > > > 
> > > > The previous code was a total abomination, even if I happen to know
> > > > who wrote it.
> > > 
> > > Let's give Junio a break, okay? He does a kick-ass job at maintaining
> > > Git.  What we see here is simply good software development, nothing
> > > more, nothing less: an initial, working code being improved. No need
> > > to make the original author feel bad... :-)
> > 
> > In case my sarcasm wasn't clear, _I_ am the author of the alluded
> > abomination.
> 
> Sorry, I did not catch that. I just looked at
> 583b7ea31b7c16f872b178d541591ab816d16f85 and felt that we could be nicer
> to Junio...

Oh, the initial code from Junio was sane enough. I made a mess of it 
afterwards.


Nicolas

Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre]

On Mon, 13 Jun 2016, Nicolas Pitre wrote:

> On Mon, 13 Jun 2016, Lukas Fleischer wrote:
> 
> > Improve the readability of recv_sideband() significantly by replacing
> > fragile buffer manipulations with more sophisticated format strings.
> > Also, reorganize the overall control flow, remove some superfluous
> > variables and replace a custom implementation of strpbrk() with a call
> > to the standard C library function.
> > 
> > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> 
> The previous code was a total abomination, even if I happen to know who 
> wrote it.
> 
> Acked-by: Nicolas Pitre <nico@fluxnic.net>

I just looked again at all the contraptions _I_ wrote (not Junio's) for 
a reason why I went to such extremes in making this code co complicated.

One aspect that is now lost with your patch is the atomic nature of the 
write.  See commit ed1902ef5c for the explanation.  You could probably 
use sprintf() into a temporary buffer and write it in one go to avoid 
segmented writes from the C library. It's probably not worth having that 
complex code just to avoid a string copy.

> > I had a really hard time reading and understanding this function when I
> > came up with my last patch. What I ended up with is almost a complete
> > rewrite of recv_sideband() and I find the end result to be much more
> > readable than what we have now. Given that this is quite invasive, it
> > would be good to have some more eyes and opinions...

I'd also suggest you look at "git log --author=Pitre sideband.c" output 
where I documented other examples of messed up displays that led to 
the current code so you could confirm your patch covers them.


Nicolas

Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 14 Jun 2016, Lukas Fleischer wrote:

> Hi Nicolas,
> 
> On Tue, 14 Jun 2016 at 19:09:15, Nicolas Pitre wrote:
> > I just looked again at all the contraptions _I_ wrote (not Junio's) for 
> > a reason why I went to such extremes in making this code co complicated.
> > 
> > One aspect that is now lost with your patch is the atomic nature of the 
> > write.  See commit ed1902ef5c for the explanation.  You could probably 
> > use sprintf() into a temporary buffer and write it in one go to avoid 
> > segmented writes from the C library. It's probably not worth having that 
> > complex code just to avoid a string copy.
> 
> The old code calls fprintf() once per line and so does the new code. The
> only difference is that in the old code, the single parts were
> concatenated manually while the new code tells fprintf() to do the
> concatenation itself while printing. Also note that fprintf() is
> buffered -- so even if the new code would call it more often, it would
> not really matter.

It is not buffered as it writes to stderr. And some C libs do separate 
calls to write() for every string format specifier. So "%s%s%c" may end 
up calling write() 3 times depending on the implementation.  The example 
I gave in commit ed1902ef5c is real and I even observed it with strace 
back then.


Nicolas

Re: [PATCH] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> It is not buffered as it writes to stderr. And some C libs do separate 
> calls to write() for every string format specifier. So "%s%s%c" may end 
> up calling write() 3 times depending on the implementation.  The example 
> I gave in commit ed1902ef5c is real and I even observed it with strace 
> back then.

I think you meant 9ac13ec9 (atomic write for sideband remote
messages, 2006-10-11).

IIRC, back then we did't use to make as much use of strbuf API as we
do today; if we were doing that commit today, we would be doing
strbuf, I would suspect.

Re: [PATCH] Refactor recv_sideband()

[Lukas Fleischer]

On Tue, 14 Jun 2016 at 20:11:12, Junio C Hamano wrote:
> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > It is not buffered as it writes to stderr. And some C libs do separate 
> > calls to write() for every string format specifier. So "%s%s%c" may end 
> > up calling write() 3 times depending on the implementation.  The example 
> > I gave in commit ed1902ef5c is real and I even observed it with strace 
> > back then.
> 
> I think you meant 9ac13ec9 (atomic write for sideband remote
> messages, 2006-10-11).
> 
> IIRC, back then we did't use to make as much use of strbuf API as we
> do today; if we were doing that commit today, we would be doing
> strbuf, I would suspect.

Thanks for looking up the right commit. I think I might see what is
going on; however, the situation is a bit different to the situation we
had at the time of writing 9ac13ec9 (atomic write for sideband remote
messages, 2006-10-11). Before that, we called write() manually a couple
of times. Now, we use fprintf() which makes stronger guarantees about
thread safety. However, if I understand correctly, there is still one
issue: It seems like in some places, we also directly write() to file
descriptor 2. So the following might happen: An fprintf() implementation
that calls write() once per format specifier is used. The fprintf()
function is called once from one thread, then interrupted between two
write() syscalls. In the meantime, output is written directly to file
descriptor 2 using write().

One possible solution is using strbuf and constructing the message as we
did before. However, that still relies on fprintf() only calling write()
once per format specifier. While that is probably true for all existing
implementations, I don't think it is guaranteed by some standard.
Shouldn't we always use the stderr stream when printing error messages
instead, especially when we care about thread safety?

Regards,
Lukas

Re: [PATCH] Refactor recv_sideband()

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> One possible solution is using strbuf and constructing the message as we
> did before. However, that still relies on fprintf() only calling write()
> once per format specifier. While that is probably true for all existing
> implementations, I don't think it is guaranteed by some standard.
> Shouldn't we always use the stderr stream when printing error messages
> instead, especially when we care about thread safety?

Or you can always write(2) to fd=2 and that is safe, too.

Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 14 Jun 2016, Lukas Fleischer wrote:

> On Tue, 14 Jun 2016 at 19:55:06, Nicolas Pitre wrote:
> > It is not buffered as it writes to stderr. And some C libs do separate 
> > calls to write() for every string format specifier. So "%s%s%c" may end 
> > up calling write() 3 times depending on the implementation.  The example 
> > I gave in commit ed1902ef5c is real and I even observed it with strace 
> > back then.
> 
> Ah, right, it is not buffered. Forgot that we are talking about stderr.
> 
> I still do not see how that could become a real problem, though. Apart 
> from the (purely theoretical?) performance issue, the only way 
> multiple write() calls could become a problem is when a single 
> fprintf() invocation is interrupted between two write() invocations 
> and then, there is a write() call from another thread, right?

Right. More another process in this case.

> I do not know whether we print to stderr from different threads but 
> even then, each fprintf() invocation should be atomic if I do not 
> misinterpret the POSIX specification. Could you please clarify? Are 
> there separate processes involved?

Yes, the intermixed writes come from another process such as 
git-index-pack via git-fetch-pack.


Nicolas

[PATCH v2] Refactor recv_sideband()

[Lukas Fleischer]

Improve the readability of recv_sideband() significantly by replacing
fragile buffer manipulations with string buffers and more sophisticated
format strings. Note that each line is printed using a single write()
syscall to avoid garbled output when multiple processes write to stderr
in parallel, see 9ac13ec (atomic write for sideband remote messages,
2006-10-11) for details.

Also, reorganize the overall control flow, remove some superfluous
variables and replace a custom implementation of strpbrk() with a call
to the standard C library function.

Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
Now uses a single write() invocation per line. Thanks to Nicolas and
Junio for bringing 9ac13ec to my attention.

 sideband.c | 97 +++++++++++++++++++++-----------------------------------------
 1 file changed, 33 insertions(+), 64 deletions(-)

diff --git a/sideband.c b/sideband.c
index fde8adc..8340a1b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,103 +13,72 @@
  * the remote died unexpectedly.  A flush() concludes the stream.
  */
 
-#define PREFIX "remote:"
+#define PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-#define FIX_SIZE 10  /* large enough for any of the above */
-
 int recv_sideband(const char *me, int in_stream, int out)
 {
-	unsigned pf = strlen(PREFIX);
-	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
-	char *suffix, *term;
-	int skip_pf = 0;
+	const char *term, *suffix;
+	char buf[LARGE_PACKET_MAX + 1];
+	struct strbuf outbuf = STRBUF_INIT;
+	const char *b, *brk;
 
-	memcpy(buf, PREFIX, pf);
+	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-	sf = strlen(suffix);
 
 	while (1) {
 		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
 			fprintf(stderr, "%s: protocol error: no band designator\n", me);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
-		band = buf[pf] & 0xff;
+		band = buf[0] & 0xff;
+		buf[len] = '\0';
 		len--;
 		switch (band) {
 		case 3:
-			buf[pf] = ' ';
-			buf[pf+1+len] = '\0';
-			fprintf(stderr, "%s\n", buf);
+			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
-			buf[pf] = ' ';
-			do {
-				char *b = buf;
-				int brk = 0;
-
-				/*
-				 * If the last buffer didn't end with a line
-				 * break then we should not print a prefix
-				 * this time around.
-				 */
-				if (skip_pf) {
-					b += pf+1;
-				} else {
-					len += pf+1;
-					brk += pf+1;
-				}
+			b = buf + 1;
 
-				/* Look for a line break. */
-				for (;;) {
-					brk++;
-					if (brk > len) {
-						brk = 0;
-						break;
-					}
-					if (b[brk-1] == '\n' ||
-					    b[brk-1] == '\r')
-						break;
-				}
+			/*
+			 * Append a suffix to each nonempty line to clear the
+			 * end of the screen line.
+			 */
+			while ((brk = strpbrk(b, "\n\r"))) {
+				int linelen = brk - b;
 
-				/*
-				 * Let's insert a suffix to clear the end
-				 * of the screen line if a line break was
-				 * found.  Also, if we don't skip the
-				 * prefix, then a non-empty string must be
-				 * present too.
-				 */
-				if (brk > (skip_pf ? 0 : (pf+1 + 1))) {
-					char save[FIX_SIZE];
-					memcpy(save, b + brk, sf);
-					b[brk + sf - 1] = b[brk - 1];
-					memcpy(b + brk - 1, suffix, sf);
-					fprintf(stderr, "%.*s", brk + sf, b);
-					memcpy(b + brk, save, sf);
-					len -= brk;
+				if (linelen > 0) {
+					strbuf_addf(&outbuf, "%.*s%s%c",
+						    linelen, b, suffix, *brk);
 				} else {
-					int l = brk ? brk : len;
-					fprintf(stderr, "%.*s", l, b);
-					len -= l;
+					strbuf_addf(&outbuf, "%c", *brk);
 				}
+				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
+				strbuf_reset(&outbuf);
+				strbuf_addf(&outbuf, "%s", PREFIX);
+
+				b = brk + 1;
+			}
 
-				skip_pf = !brk;
-				memmove(buf + pf+1, b + brk, len);
-			} while (len);
+			if (*b) {
+				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
+				/* Incomplete line, skip the next prefix. */
+				strbuf_reset(&outbuf);
+			}
 			continue;
 		case 1:
-			write_or_die(out, buf + pf+1, len);
+			write_or_die(out, buf + 1, len);
 			continue;
 		default:
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
-- 
2.8.3

Re: [PATCH v2] Refactor recv_sideband()

[Lukas Fleischer]

On Tue, 14 Jun 2016 at 23:00:38, Lukas Fleischer wrote:
> Improve the readability of recv_sideband() significantly by replacing
> fragile buffer manipulations with string buffers and more sophisticated
> format strings. Note that each line is printed using a single write()
> syscall to avoid garbled output when multiple processes write to stderr
> in parallel, see 9ac13ec (atomic write for sideband remote messages,
> 2006-10-11) for details.
> 
> Also, reorganize the overall control flow, remove some superfluous
> variables and replace a custom implementation of strpbrk() with a call
> to the standard C library function.
> 
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> ---
> Now uses a single write() invocation per line. Thanks to Nicolas and
> Junio for bringing 9ac13ec to my attention.
> 
>  sideband.c | 97 +++++++++++++++++++++-----------------------------------------
>  1 file changed, 33 insertions(+), 64 deletions(-)
> 
> diff --git a/sideband.c b/sideband.c
> index fde8adc..8340a1b 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -13,103 +13,72 @@
> [...]
> +       struct strbuf outbuf = STRBUF_INIT;
> [...]
> +                               strbuf_reset(&outbuf);
> +                               strbuf_addf(&outbuf, "%s", PREFIX);
> [...]
> +                               /* Incomplete line, skip the next prefix. */
> +                               strbuf_reset(&outbuf);

Missing strbuf_release() at the end of the function, I guess. I will
wait for further comments and send v3 tomorrow.

Lukas

Re: [PATCH v2] Refactor recv_sideband()

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

Lukas Fleischer <lfleischer@lfos.de> writes:

> Improve the readability of recv_sideband() significantly by replacing

s/significantly //; "making it readable" is already a subjective
goodness criterion, and you do not have to make it sound even more
subjective.  Let the updated result convince the reader that it is
vastly more readable.

> Also, reorganize the overall control flow, remove some superfluous
> variables and replace a custom implementation of strpbrk() with a call
> to the standard C library function.

I find that calling the loop "a custom implementation" is a bit
unfair.  The original tried to avoid looking beyond "len", but in
the updated code because you have buf[len] = '\0' to terminate the
line, and because you pass LARGE_PACKET_MAX to packet_read() while
your buf[] allocates one more byte, you can use strpbrk() here
safely. Which would mean "a custom implementation" was done for a
reason.

But that is a minor point.

I'll omit the preimage lines from the following.

>  int recv_sideband(const char *me, int in_stream, int out)
>  {
> +	const char *term, *suffix;
> +	char buf[LARGE_PACKET_MAX + 1];
> +	struct strbuf outbuf = STRBUF_INIT;
> +	const char *b, *brk;
>  
> +	strbuf_addf(&outbuf, "%s", PREFIX);

I highly suspect that you are better off without this line being
here.

> ...
>  	while (1) {
>  		int band, len;
> +		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
>  		if (len == 0)
>  			break;
>  		if (len < 1) {
>  			fprintf(stderr, "%s: protocol error: no band designator\n", me);
>  			return SIDEBAND_PROTOCOL_ERROR;
>  		}
> +		band = buf[0] & 0xff;
> +		buf[len] = '\0';
>  		len--;
>  		switch (band) {
>  		case 3:
> +			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
>  			return SIDEBAND_REMOTE_ERROR;

Two "return"s we see above will leak outbuf.buf that holds PREFIX.

>  		case 2:
> +			b = buf + 1;
>  
> +			/*
> +			 * Append a suffix to each nonempty line to clear the
> +			 * end of the screen line.
> +			 */
> +			while ((brk = strpbrk(b, "\n\r"))) {
> +				int linelen = brk - b;
>  
> +				if (linelen > 0) {
> +					strbuf_addf(&outbuf, "%.*s%s%c",
> +						    linelen, b, suffix, *brk);
>  				} else {
> +					strbuf_addf(&outbuf, "%c", *brk);
>  				}
> +				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> +				strbuf_reset(&outbuf);
> +				strbuf_addf(&outbuf, "%s", PREFIX);

Instead of doing "we assume outbuf already has PREFIX when we add
contents from buf[]", the code structure would be better if you:

 * make outbuf.buf contain PREFIX at the beginning of this innermost
   loop; lose the reset/addf from here.

 * move strbuf_reset(&outbuf) at the end of the next if (*b) block
   to just before "continue;"

perhaps?

> +				b = brk + 1;
> +			}
>  
> +			if (*b) {
> +				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> +				/* Incomplete line, skip the next prefix. */
> +				strbuf_reset(&outbuf);
> +			}
>  			continue;
>  		case 1:
> +			write_or_die(out, buf + 1, len);
>  			continue;
>  		default:
>  			fprintf(stderr, "%s: protocol error: bad band #%d\n",

Re: [PATCH v2] Refactor recv_sideband()

[Jeff King]

On Tue, Jun 14, 2016 at 02:25:42PM -0700, Junio C Hamano wrote:

> > Also, reorganize the overall control flow, remove some superfluous
> > variables and replace a custom implementation of strpbrk() with a call
> > to the standard C library function.
> 
> I find that calling the loop "a custom implementation" is a bit
> unfair.  The original tried to avoid looking beyond "len", but in
> the updated code because you have buf[len] = '\0' to terminate the
> line, and because you pass LARGE_PACKET_MAX to packet_read() while
> your buf[] allocates one more byte, you can use strpbrk() here
> safely. Which would mean "a custom implementation" was done for a
> reason.

Knowing we have a NUL at the end makes strpbrk() safe to use (as opposed
to walking off the end of the buffer). But what about the opposite case,
when there are embedded NULs in the data?

I think this case is already fairly broken by the use of "%s" specifiers
later in the function, and I doubt it is all that useful. So I am OK if
the answer is "we don't care, and do not even consider it a bug that
should be fixed".

-Peff

Re: [PATCH v2] Refactor recv_sideband()

[Lukas Fleischer]

On Wed, 15 Jun 2016 at 09:14:54, Lukas Fleischer wrote:
> What we could do is reintroduce the local prefix variable I had in v1
> and use that to store whether a prefix needs to be prepended or not. If
> we do that and if we are fine with strbuf memory being (potentially)
> allocated and re-allocated multiple times during a single
> recv_sideband() invocation, the strbuf could be made local to the part
> that actually needs it and could be used as in asprintf().

When I revamped the patch and looked at similar code I had another idea
that I did not want to keep to myself:

In contexts similar to this patch, we often seem to use statically
allocated string buffers as follows:

-- 8< --
diff --git a/sideband.c b/sideband.c
index 8340a1b..08b75e2 100644
--- a/sideband.c
+++ b/sideband.c
@@ -22,9 +22,10 @@ int recv_sideband(const char *me, int in_stream, int out)
 {
 	const char *term, *suffix;
 	char buf[LARGE_PACKET_MAX + 1];
-	struct strbuf outbuf = STRBUF_INIT;
+	static struct strbuf outbuf = STRBUF_INIT;
 	const char *b, *brk;
 
+	strbuf_reset(&outbuf);
 	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
-- 8< --

The benefits are obvious: No memory (re-)allocation overhead and no need
to take care of freeing on every return path. The downside is that the
function becomes non-thread-safe. After looking at the call sites, it
seems like we do not seem to call recv_sideband() from different threads
yet -- but do we want to rely on that?

The other two options are:

1. As I suggested earlier, introduce a wrapper that could be named
   xwritef() or fprintf_atomic() and allocate a new buffer (i.e., use a
   fresh strbuf) each time something is printed.

2. Keep using a fixed-size buffer size we already know the maximum size
   each of the strings can have.

Which one do you prefer?

Regards,
Lukas

[PATCH v3] Refactor recv_sideband()

[Lukas Fleischer]

Before this patch, we used character buffer manipulations to split
messages from the sideband at line breaks and insert "remote: " at the
beginning of each line, using the packet size to determine the end of a
message. However, since it is safe to assume that diagnostic messages
from the sideband never contain NUL characters, we can also
NUL-terminate the buffer, use strpbrk() for splitting lines and use
format strings to insert the prefix.

A static strbuf is used for constructing the output which is then
printed using a single write() call, such that the atomicity of the
output is preserved. See 9ac13ec (atomic write for sideband remote
messages, 2006-10-11) for details.

Helped-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
Now using a static strbuf for the output buffer such that the number of
memory allocations is kept to a minimum.

 sideband.c | 98 ++++++++++++++++++++++----------------------------------------
 1 file changed, 34 insertions(+), 64 deletions(-)

diff --git a/sideband.c b/sideband.c
index fde8adc..08b75e2 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,103 +13,73 @@
  * the remote died unexpectedly.  A flush() concludes the stream.
  */
 
-#define PREFIX "remote:"
+#define PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-#define FIX_SIZE 10  /* large enough for any of the above */
-
 int recv_sideband(const char *me, int in_stream, int out)
 {
-	unsigned pf = strlen(PREFIX);
-	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
-	char *suffix, *term;
-	int skip_pf = 0;
+	const char *term, *suffix;
+	char buf[LARGE_PACKET_MAX + 1];
+	static struct strbuf outbuf = STRBUF_INIT;
+	const char *b, *brk;
 
-	memcpy(buf, PREFIX, pf);
+	strbuf_reset(&outbuf);
+	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-	sf = strlen(suffix);
 
 	while (1) {
 		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
 			fprintf(stderr, "%s: protocol error: no band designator\n", me);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
-		band = buf[pf] & 0xff;
+		band = buf[0] & 0xff;
+		buf[len] = '\0';
 		len--;
 		switch (band) {
 		case 3:
-			buf[pf] = ' ';
-			buf[pf+1+len] = '\0';
-			fprintf(stderr, "%s\n", buf);
+			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
-			buf[pf] = ' ';
-			do {
-				char *b = buf;
-				int brk = 0;
-
-				/*
-				 * If the last buffer didn't end with a line
-				 * break then we should not print a prefix
-				 * this time around.
-				 */
-				if (skip_pf) {
-					b += pf+1;
-				} else {
-					len += pf+1;
-					brk += pf+1;
-				}
+			b = buf + 1;
 
-				/* Look for a line break. */
-				for (;;) {
-					brk++;
-					if (brk > len) {
-						brk = 0;
-						break;
-					}
-					if (b[brk-1] == '\n' ||
-					    b[brk-1] == '\r')
-						break;
-				}
+			/*
+			 * Append a suffix to each nonempty line to clear the
+			 * end of the screen line.
+			 */
+			while ((brk = strpbrk(b, "\n\r"))) {
+				int linelen = brk - b;
 
-				/*
-				 * Let's insert a suffix to clear the end
-				 * of the screen line if a line break was
-				 * found.  Also, if we don't skip the
-				 * prefix, then a non-empty string must be
-				 * present too.
-				 */
-				if (brk > (skip_pf ? 0 : (pf+1 + 1))) {
-					char save[FIX_SIZE];
-					memcpy(save, b + brk, sf);
-					b[brk + sf - 1] = b[brk - 1];
-					memcpy(b + brk - 1, suffix, sf);
-					fprintf(stderr, "%.*s", brk + sf, b);
-					memcpy(b + brk, save, sf);
-					len -= brk;
+				if (linelen > 0) {
+					strbuf_addf(&outbuf, "%.*s%s%c",
+						    linelen, b, suffix, *brk);
 				} else {
-					int l = brk ? brk : len;
-					fprintf(stderr, "%.*s", l, b);
-					len -= l;
+					strbuf_addf(&outbuf, "%c", *brk);
 				}
+				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
+				strbuf_reset(&outbuf);
+				strbuf_addf(&outbuf, "%s", PREFIX);
+
+				b = brk + 1;
+			}
 
-				skip_pf = !brk;
-				memmove(buf + pf+1, b + brk, len);
-			} while (len);
+			if (*b) {
+				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
+				/* Incomplete line, skip the next prefix. */
+				strbuf_reset(&outbuf);
+			}
 			continue;
 		case 1:
-			write_or_die(out, buf + pf+1, len);
+			write_or_die(out, buf + 1, len);
 			continue;
 		default:
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
-- 
2.9.0

Re: [PATCH v3] Refactor recv_sideband()

[Nicolas Pitre]

On Wed, 22 Jun 2016, Lukas Fleischer wrote:

> Before this patch, we used character buffer manipulations to split
> messages from the sideband at line breaks and insert "remote: " at the
> beginning of each line, using the packet size to determine the end of a
> message. However, since it is safe to assume that diagnostic messages
> from the sideband never contain NUL characters, we can also
> NUL-terminate the buffer, use strpbrk() for splitting lines and use
> format strings to insert the prefix.
> 
> A static strbuf is used for constructing the output which is then
> printed using a single write() call, such that the atomicity of the
> output is preserved. See 9ac13ec (atomic write for sideband remote
> messages, 2006-10-11) for details.
> 
> Helped-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>

The patch is buggy.

Once patched, the code looks like this:

        case 2:
                b = buf + 1;
                /*
                 * Append a suffix to each nonempty line to clear the
                 * end of the screen line.
                 */
                while ((brk = strpbrk(b, "\n\r"))) {
                        int linelen = brk - b;
                        if (linelen > 0) {
                                strbuf_addf(&outbuf, "%.*s%s%c",
                                            linelen, b, suffix, *brk);
                        } else {
                                strbuf_addf(&outbuf, "%c", *brk);
                        }
                        xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
                        strbuf_reset(&outbuf);
                        strbuf_addf(&outbuf, "%s", PREFIX);
                        b = brk + 1;
                }
                if (*b) {
                        xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
                        /* Incomplete line, skip the next prefix. */
                        strbuf_reset(&outbuf);
                }
                continue;

You are probably missing a strbuf_addf() before the last xwrite().


Nicolas

Re: [PATCH v3] Refactor recv_sideband()

[Nicolas Pitre]

On Wed, 22 Jun 2016, Nicolas Pitre wrote:

> On Wed, 22 Jun 2016, Lukas Fleischer wrote:
> 
> > Before this patch, we used character buffer manipulations to split
> > messages from the sideband at line breaks and insert "remote: " at the
> > beginning of each line, using the packet size to determine the end of a
> > message. However, since it is safe to assume that diagnostic messages
> > from the sideband never contain NUL characters, we can also
> > NUL-terminate the buffer, use strpbrk() for splitting lines and use
> > format strings to insert the prefix.
> > 
> > A static strbuf is used for constructing the output which is then
> > printed using a single write() call, such that the atomicity of the
> > output is preserved. See 9ac13ec (atomic write for sideband remote
> > messages, 2006-10-11) for details.
> > 
> > Helped-by: Nicolas Pitre <nico@fluxnic.net>
> > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> 
> The patch is buggy.
> 
> Once patched, the code looks like this:
> 
>         case 2:
>                 b = buf + 1;
>                 /*
>                  * Append a suffix to each nonempty line to clear the
>                  * end of the screen line.
>                  */
>                 while ((brk = strpbrk(b, "\n\r"))) {
>                         int linelen = brk - b;
>                         if (linelen > 0) {
>                                 strbuf_addf(&outbuf, "%.*s%s%c",
>                                             linelen, b, suffix, *brk);
>                         } else {
>                                 strbuf_addf(&outbuf, "%c", *brk);
>                         }
>                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
>                         strbuf_reset(&outbuf);
>                         strbuf_addf(&outbuf, "%s", PREFIX);
>                         b = brk + 1;
>                 }
>                 if (*b) {
>                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
>                         /* Incomplete line, skip the next prefix. */
>                         strbuf_reset(&outbuf);
>                 }
>                 continue;
> 
> You are probably missing a strbuf_addf() before the last xwrite().

In fact, you could simply append the partial line to the strbuf and make 
it the prefix for the next packet rather than writing a partial line.  
You'd only have to write a partial line before leaving the function if 
the strbuf is not empty at that point.


Nicolas

Re: [PATCH v3] Refactor recv_sideband()

[Lukas Fleischer]

On Thu, 23 Jun 2016 at 00:47:39, Nicolas Pitre wrote:
> On Wed, 22 Jun 2016, Nicolas Pitre wrote:
> [...]
> >                 if (*b) {
> >                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> >                         /* Incomplete line, skip the next prefix. */
> >                         strbuf_reset(&outbuf);
> >                 }
> >                 continue;
> > 
> > You are probably missing a strbuf_addf() before the last xwrite().
> 
> In fact, you could simply append the partial line to the strbuf and make 
> it the prefix for the next packet rather than writing a partial line.  
> You'd only have to write a partial line before leaving the function if 
> the strbuf is not empty at that point.

True. And I like that solution.

Thinking about your last sentence, do we care about printing an
incomplete line at the end of the communication at all? If so, do we
need to print such a line on every return path (i.e. on protocol and
remote errors as well)? If we do, and if we want to implement partial
line handling the way you suggested, we should probably print that final
line from a common return path. And if we add such a path, we could
reconsider using a non-static strbuf as well, since we could simply
strbuf_release() the output buffer in that common code block. Opinions?

I will wait for more comments and submit v4 in a couple of days.

Re: [PATCH v3] Refactor recv_sideband()

[Nicolas Pitre]

On Thu, 23 Jun 2016, Lukas Fleischer wrote:

> On Thu, 23 Jun 2016 at 00:47:39, Nicolas Pitre wrote:
> > On Wed, 22 Jun 2016, Nicolas Pitre wrote:
> > [...]
> > >                 if (*b) {
> > >                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> > >                         /* Incomplete line, skip the next prefix. */
> > >                         strbuf_reset(&outbuf);
> > >                 }
> > >                 continue;
> > > 
> > > You are probably missing a strbuf_addf() before the last xwrite().
> > 
> > In fact, you could simply append the partial line to the strbuf and make 
> > it the prefix for the next packet rather than writing a partial line.  
> > You'd only have to write a partial line before leaving the function if 
> > the strbuf is not empty at that point.
> 
> True. And I like that solution.
> 
> Thinking about your last sentence, do we care about printing an
> incomplete line at the end of the communication at all?

I'd think so. This is very unlikely to happen in practice, but that 
might be useful to diagnose potential problems.

> If so, do we need to print such a line on every return path (i.e. on 
> protocol and remote errors as well)? If we do, and if we want to 
> implement partial line handling the way you suggested, we should 
> probably print that final line from a common return path. And if we 
> add such a path, we could reconsider using a non-static strbuf as 
> well, since we could simply strbuf_release() the output buffer in that 
> common code block. Opinions?

That makes sense to me.


Nicolas

Re: [PATCH v2] Refactor recv_sideband()

[Jeff King]

On Tue, Jun 14, 2016 at 11:00:38PM +0200, Lukas Fleischer wrote:

> Improve the readability of recv_sideband() significantly by replacing
> fragile buffer manipulations with string buffers and more sophisticated
> format strings. Note that each line is printed using a single write()
> syscall to avoid garbled output when multiple processes write to stderr
> in parallel, see 9ac13ec (atomic write for sideband remote messages,
> 2006-10-11) for details.
> 
> Also, reorganize the overall control flow, remove some superfluous
> variables and replace a custom implementation of strpbrk() with a call
> to the standard C library function.

I happened to be looking at the color-printing code yesterday, and was
reminded that on Windows, fprintf is responsible for converting ANSI
codes into colors the terminal can show:

  $ git grep -A2 IMPORTANT color.h
  color.h: * IMPORTANT: Due to the way these color codes are emulated on Windows,
  color.h- * write them only using printf(), fprintf(), and fputs(). In particular,
  color.h- * do not use puts() or write().

Your patch converts some fprintf calls to write. What does this mean
on Windows for:

  1. Remote servers which send ANSI codes and expect them to look
     reasonable (this might be a losing proposition already, as the
     server won't know anything about the user's terminal, or whether
     output is going to a file).

  2. The use of ANSI_SUFFIX in this function, which uses a similar
     escape code.

-Peff

Re: [PATCH v2] Refactor recv_sideband()

[Johannes Schindelin]

Hi Peff,

On Fri, 24 Jun 2016, Jeff King wrote:

> On Tue, Jun 14, 2016 at 11:00:38PM +0200, Lukas Fleischer wrote:
> 
> > Improve the readability of recv_sideband() significantly by replacing
> > fragile buffer manipulations with string buffers and more sophisticated
> > format strings. Note that each line is printed using a single write()
> > syscall to avoid garbled output when multiple processes write to stderr
> > in parallel, see 9ac13ec (atomic write for sideband remote messages,
> > 2006-10-11) for details.
> > 
> > Also, reorganize the overall control flow, remove some superfluous
> > variables and replace a custom implementation of strpbrk() with a call
> > to the standard C library function.
> 
> I happened to be looking at the color-printing code yesterday, and was
> reminded that on Windows, fprintf is responsible for converting ANSI
> codes into colors the terminal can show:

Thanks for remembering!

>   $ git grep -A2 IMPORTANT color.h
>   color.h: * IMPORTANT: Due to the way these color codes are emulated on Windows,
>   color.h- * write them only using printf(), fprintf(), and fputs(). In particular,
>   color.h- * do not use puts() or write().
> 
> Your patch converts some fprintf calls to write. What does this mean
> on Windows for:
> 
>   1. Remote servers which send ANSI codes and expect them to look
>      reasonable (this might be a losing proposition already, as the
>      server won't know anything about the user's terminal, or whether
>      output is going to a file).
> 
>   2. The use of ANSI_SUFFIX in this function, which uses a similar
>      escape code.

Wow, I did not even think that a remote server would send color codes,
what with the server being unable to query the local terminal via
isatty().

Do we *actually* send color via the sideband, like, ever?

Ciao,
Dscho

Re: [PATCH v2] Refactor recv_sideband()

[Jeff King]

On Fri, Jun 24, 2016 at 07:45:04PM +0200, Johannes Schindelin wrote:

> >   $ git grep -A2 IMPORTANT color.h
> >   color.h: * IMPORTANT: Due to the way these color codes are emulated on Windows,
> >   color.h- * write them only using printf(), fprintf(), and fputs(). In particular,
> >   color.h- * do not use puts() or write().
> > 
> > Your patch converts some fprintf calls to write. What does this mean
> > on Windows for:
> > 
> >   1. Remote servers which send ANSI codes and expect them to look
> >      reasonable (this might be a losing proposition already, as the
> >      server won't know anything about the user's terminal, or whether
> >      output is going to a file).
> > 
> >   2. The use of ANSI_SUFFIX in this function, which uses a similar
> >      escape code.
> 
> Wow, I did not even think that a remote server would send color codes,
> what with the server being unable to query the local terminal via
> isatty().
> 
> Do we *actually* send color via the sideband, like, ever?

We don't, but remember that we forward arbitrary output from hooks.

If the consensus is "nah, it is probably crazy and nobody is doing it
today" then I am fine with that.

I do wonder about the ANSI_SUFFIX thing, though.

-Peff

Re: [PATCH v2] Refactor recv_sideband()

[Junio C Hamano]

On Fri, Jun 24, 2016 at 11:14 AM, Jeff King <peff@peff.net> wrote:
>
> I do wonder about the ANSI_SUFFIX thing, though.

compat/winansi.c seems to have a provision for 'K' (and obviously 'm'
for colors).

Re: [PATCH v2] Refactor recv_sideband()

[Dennis Kaarsemaker]

On vr, 2016-06-24 at 14:14 -0400, Jeff King wrote:

> On Fri, Jun 24, 2016 at 07:45:04PM +0200, Johannes Schindelin wrote:
>> Do we *actually* send color via the sideband, like, ever?

> We don't, but remember that we forward arbitrary output from hooks.
> If the consensus is "nah, it is probably crazy and nobody is doing it
> today" then I am fine with that.

It's crazy, so obviously we're doing it :)

Though losing this would not be a big deal for us.

D.

Re: [PATCH v2] Refactor recv_sideband()

[Lukas Fleischer]

On Fri, 24 Jun 2016 at 20:32:28, Junio C Hamano wrote:
> On Fri, Jun 24, 2016 at 11:14 AM, Jeff King <peff@peff.net> wrote:
> >
> > I do wonder about the ANSI_SUFFIX thing, though.
> 
> compat/winansi.c seems to have a provision for 'K' (and obviously 'm'
> for colors).

So we probably want to switch back to using fprintf() or fputs() and
rely on the assumption that these functions use atomic write()
operations internally when printing to stderr (on practically relevant
systems, i.e., libc implementations), right?

Another option is using write() on all systems but Windows and fall back
on fputs() but that feels a bit like over-engineering at this point.

I will prepare and submit v4 later today.

Re: [PATCH v2] Refactor recv_sideband()

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> On Fri, 24 Jun 2016 at 20:32:28, Junio C Hamano wrote:
>> On Fri, Jun 24, 2016 at 11:14 AM, Jeff King <peff@peff.net> wrote:
>> >
>> > I do wonder about the ANSI_SUFFIX thing, though.
>> 
>> compat/winansi.c seems to have a provision for 'K' (and obviously 'm'
>> for colors).
>
> So we probably want to switch back to using fprintf() or fputs() and
> rely on the assumption that these functions use atomic write()
> operations internally when printing to stderr (on practically relevant
> systems, i.e., libc implementations), right?
>
> Another option is using write() on all systems but Windows and fall back
> on fputs() but that feels a bit like over-engineering at this point.

Wait a bit.

The original used to use one fprintf(stderr) per line it outputs.
That does not necessarily guarantee atomicity (i.e. the payload
string may be large and make stdio decide to do multiple writes).

The issue found in your first round (sent on Jun 13) was that you
did multiple fprintf(stderr) when showing a single line, which is
guaranteed to result in multiple writes because stderr is not
line-buffered.  But that was corrected in your second round.

It's just you used xwrite() there that introduced a different issue.
Wouldn't replacing it with fwrite(stderr) without changing anything
else solve that?

Re: [PATCH v2] Refactor recv_sideband()

[Jeff King]

On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:

> It's just you used xwrite() there that introduced a different issue.
> Wouldn't replacing it with fwrite(stderr) without changing anything
> else solve that?

I am having trouble actually seeing how the ANSI-emulation code gets
triggered, but the comment in color.h implies that it is only printf,
fprintf, and fputs that have the desired effect. So fwrite() may not be
sufficient, and we may need fprintf("%.*s", len, buf) or something.

-Peff

Re: [PATCH v2] Refactor recv_sideband()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:
>
>> It's just you used xwrite() there that introduced a different issue.
>> Wouldn't replacing it with fwrite(stderr) without changing anything
>> else solve that?
>
> I am having trouble actually seeing how the ANSI-emulation code gets
> triggered, but the comment in color.h implies that it is only printf,
> fprintf, and fputs that have the desired effect. So fwrite() may not be
> sufficient, and we may need fprintf("%.*s", len, buf) or something.

I have no idea how, either X-<.  But you're probably right about the
magic being limited to the printf family of functions---I do recall
hearing something like that in the past.

Re: [PATCH v2] Refactor recv_sideband()

[Lukas Fleischer]

On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:
> >
> >> It's just you used xwrite() there that introduced a different issue.
> >> Wouldn't replacing it with fwrite(stderr) without changing anything
> >> else solve that?

I do not see how using fwrite() buys us anything. Neither fwrite() nor
fputs() nor fprintf() guarantee to call write() only once. Each of these
three functions is buffered when printing to stdout and unbuffered when
printing to stderr. I do not think there is any serious implementation
of any of those functions that performs segmented write() calls (but I
might be mistaken). According to POSIX, write() can take up to SSIZE_MAX
bytes which is guaranteed to be at least 32767 but is actually much
larger on most systems (2^32 - 1 here). It is very unlikely that this
limit will ever be reached by a single line of a diagnostic error
message.

Frankly, there is a small benefit to fwrite() because we already know
the string length from the strbuf and most fputs() implementations
probably do something equivalent to

    fwrite(s, 1, strlen(s), stream);

I can switch to using fwrite() instead of fputs() in v4 if you prefer
that.

> >
> > I am having trouble actually seeing how the ANSI-emulation code gets
> > triggered, but the comment in color.h implies that it is only printf,
> > fprintf, and fputs that have the desired effect. So fwrite() may not be
> > sufficient, and we may need fprintf("%.*s", len, buf) or something.
> 
> I have no idea how, either X-<.  But you're probably right about the
> magic being limited to the printf family of functions---I do recall
> hearing something like that in the past.

I do not know anything about the emulation code as well but from a
cursory read of winansi_init(), it looks like there is some magic that
hooks into the stdout and stderr streams, redirects them to a named
pipe, then replaces ANSI control codes and actually prints to the
console from console_thread(). So it should work with any of the
stream-based functions but not with write(), puts(), etc.

Re: [PATCH v2] Refactor recv_sideband()

[Nicolas Pitre]

On Mon, 27 Jun 2016, Lukas Fleischer wrote:

> On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> > > On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:
> > >
> > >> It's just you used xwrite() there that introduced a different issue.
> > >> Wouldn't replacing it with fwrite(stderr) without changing anything
> > >> else solve that?
> 
> I do not see how using fwrite() buys us anything. Neither fwrite() nor
> fputs() nor fprintf() guarantee to call write() only once. Each of these
> three functions is buffered when printing to stdout and unbuffered when
> printing to stderr.

You are right.  However, in practice:

- fprintf(stderr, "%s", buffer) is likely to call write() only once 
  given there is only one string specifier, and

- On Windows the ANSI escape sequences are interpreted by fprintf() and 
  not by write() nor by the actual display console code. Insane but such 
  is life sometimes.

So the point is simply to replace your call to write() by a call to 
fprintf(..., "%*s", ...) in your patch which should provide the same 
end result as before.


Nicolas

Re: [PATCH v2] Refactor recv_sideband()

[Lukas Fleischer]

On Mon, 27 Jun 2016 at 22:47:59, Nicolas Pitre wrote:
> On Mon, 27 Jun 2016, Lukas Fleischer wrote:
> 
> > On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:
> > > Jeff King <peff@peff.net> writes:
> > > 
> > > > On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:
> > > >
> > > >> It's just you used xwrite() there that introduced a different issue.
> > > >> Wouldn't replacing it with fwrite(stderr) without changing anything
> > > >> else solve that?
> > 
> > I do not see how using fwrite() buys us anything. Neither fwrite() nor
> > fputs() nor fprintf() guarantee to call write() only once. Each of these
> > three functions is buffered when printing to stdout and unbuffered when
> > printing to stderr.
> 
> You are right.  However, in practice:
> 
> - fprintf(stderr, "%s", buffer) is likely to call write() only once 
>   given there is only one string specifier, and
> 
> - On Windows the ANSI escape sequences are interpreted by fprintf() and 
>   not by write() nor by the actual display console code. Insane but such 
>   is life sometimes.
> 
> So the point is simply to replace your call to write() by a call to 
> fprintf(..., "%*s", ...) in your patch which should provide the same 
> end result as before.

Well, this is essentially what I tried to make clear in my previous
email. In practice, each of the following lines should work:

    fwrite(outbuf.buf, 1, outbuf.len, stderr);
    fputs(outbuf.buf, stderr);
    fprintf("%s", outbuf.buf, stderr);
    fprintf("%.*s", outbuf.len, outbuf.buf, stderr);

The first version is probably to most "efficient" one and I personally
find the fputs() line to be the one that is easiest to read. However, I
think it does not make sense to start another bikeshedding discussion at
this point. I will make a defensive choice and use fprintf() with "%.*s"
since that is what we used before, so it is tested well enough.

Given the amount of discussion required to get this right, I also
strongly believe this code deserves a comment with a short explanation
on why things are done this way...

[PATCH v4] Refactor recv_sideband()

[Lukas Fleischer]

Before this patch, we used character buffer manipulations to split
messages from the sideband at line breaks and insert "remote: " at the
beginning of each line, using the packet size to determine the end of a
message. However, since it is safe to assume that diagnostic messages
from the sideband never contain NUL characters, we can also
NUL-terminate the buffer, use strpbrk() for splitting lines and use
format strings to insert the prefix.

A strbuf is used for accumulating the output which is then printed using
a single fprintf() call with a single conversion specifier per line,
such that the atomicity of the output is preserved. See 9ac13ec (atomic
write for sideband remote messages, 2006-10-11) for details.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
Changes since v3:
* The new code always frees the strbuf used for the output.
* Switched back to fprintf() to support ANSI codes under Windows.
* Added a comment on the tradeoff between atomicity and Windows support.

 sideband.c | 125 ++++++++++++++++++++++++++-----------------------------------
 1 file changed, 54 insertions(+), 71 deletions(-)

diff --git a/sideband.c b/sideband.c
index fde8adc..835d77d 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,111 +13,94 @@
  * the remote died unexpectedly.  A flush() concludes the stream.
  */
 
-#define PREFIX "remote:"
+#define PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-#define FIX_SIZE 10  /* large enough for any of the above */
-
 int recv_sideband(const char *me, int in_stream, int out)
 {
-	unsigned pf = strlen(PREFIX);
-	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
-	char *suffix, *term;
-	int skip_pf = 0;
+	const char *term, *suffix;
+	char buf[LARGE_PACKET_MAX + 1];
+	struct strbuf outbuf = STRBUF_INIT;
+	const char *b, *brk;
+	int retval = 0;
 
-	memcpy(buf, PREFIX, pf);
+	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-	sf = strlen(suffix);
 
-	while (1) {
+	while (retval == 0) {
 		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
 			fprintf(stderr, "%s: protocol error: no band designator\n", me);
-			return SIDEBAND_PROTOCOL_ERROR;
+			retval = SIDEBAND_PROTOCOL_ERROR;
+			break;
 		}
-		band = buf[pf] & 0xff;
+		band = buf[0] & 0xff;
+		buf[len] = '\0';
 		len--;
 		switch (band) {
 		case 3:
-			buf[pf] = ' ';
-			buf[pf+1+len] = '\0';
-			fprintf(stderr, "%s\n", buf);
-			return SIDEBAND_REMOTE_ERROR;
+			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
+			retval = SIDEBAND_REMOTE_ERROR;
+			break;
 		case 2:
-			buf[pf] = ' ';
-			do {
-				char *b = buf;
-				int brk = 0;
-
-				/*
-				 * If the last buffer didn't end with a line
-				 * break then we should not print a prefix
-				 * this time around.
-				 */
-				if (skip_pf) {
-					b += pf+1;
-				} else {
-					len += pf+1;
-					brk += pf+1;
-				}
+			b = buf + 1;
 
-				/* Look for a line break. */
-				for (;;) {
-					brk++;
-					if (brk > len) {
-						brk = 0;
-						break;
-					}
-					if (b[brk-1] == '\n' ||
-					    b[brk-1] == '\r')
-						break;
-				}
+			/*
+			 * Append a suffix to each nonempty line to clear the
+			 * end of the screen line.
+			 *
+			 * The output is accumulated in a buffer and each line
+			 * is printed to stderr using fprintf() with a single
+			 * conversion specifier. This is a "best effort"
+			 * approach to supporting both inter-process atomicity
+			 * (single conversion specifiers are likely to end up
+			 * in single atomic write() system calls) and the ANSI
+			 * control code emulation under Windows.
+			 */
+			while ((brk = strpbrk(b, "\n\r"))) {
+				int linelen = brk - b;
 
-				/*
-				 * Let's insert a suffix to clear the end
-				 * of the screen line if a line break was
-				 * found.  Also, if we don't skip the
-				 * prefix, then a non-empty string must be
-				 * present too.
-				 */
-				if (brk > (skip_pf ? 0 : (pf+1 + 1))) {
-					char save[FIX_SIZE];
-					memcpy(save, b + brk, sf);
-					b[brk + sf - 1] = b[brk - 1];
-					memcpy(b + brk - 1, suffix, sf);
-					fprintf(stderr, "%.*s", brk + sf, b);
-					memcpy(b + brk, save, sf);
-					len -= brk;
+				if (linelen > 0) {
+					strbuf_addf(&outbuf, "%.*s%s%c",
+						    linelen, b, suffix, *brk);
 				} else {
-					int l = brk ? brk : len;
-					fprintf(stderr, "%.*s", l, b);
-					len -= l;
+					strbuf_addf(&outbuf, "%c", *brk);
 				}
+				fprintf(stderr, "%.*s", (int)outbuf.len,
+					outbuf.buf);
+				strbuf_reset(&outbuf);
+				strbuf_addf(&outbuf, "%s", PREFIX);
+
+				b = brk + 1;
+			}
 
-				skip_pf = !brk;
-				memmove(buf + pf+1, b + brk, len);
-			} while (len);
-			continue;
+			if (*b)
+				strbuf_addf(&outbuf, "%s", b);
+			break;
 		case 1:
-			write_or_die(out, buf + pf+1, len);
-			continue;
+			write_or_die(out, buf + 1, len);
+			break;
 		default:
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
 				me, band);
-			return SIDEBAND_PROTOCOL_ERROR;
+			retval = SIDEBAND_PROTOCOL_ERROR;
+			break;
 		}
 	}
-	return 0;
+
+	if (outbuf.len > 0)
+		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+	strbuf_release(&outbuf);
+	return retval;
 }
 
 /*
-- 
2.9.0

Re: [PATCH v2] Refactor recv_sideband()

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> I do not see how using fwrite() buys us anything. Neither fwrite() nor
> fputs() nor fprintf() guarantee to call write() only once.

That is not the point.

Your first attempt split what used to be a single fprintf(), which
(as Nico explained) ordinarily will result in an atomic write as
long as you are feeding a reasonable size buf, into two calls,
_guaranteeing_ that it will _not_ be atomic.  That was a bad change.

By accumulating things in strbuf (instead of char[] with hand-rolled
counting logic like the original before your patch) and feeding the
result out to a single fprintf(stderr, "%s", buf.buf), you will not
be making anything _worse_ compared to the code before your patch.
At the same time, you _are_ making the logic to accumulate the
output to a buffer far easier to read, which would be a net plus.

 

Re: [PATCH v2] Refactor recv_sideband()

[Johannes Schindelin]

Hi Peff,

On Mon, 27 Jun 2016, Jeff King wrote:

> On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:
> 
> > It's just you used xwrite() there that introduced a different issue.
> > Wouldn't replacing it with fwrite(stderr) without changing anything
> > else solve that?
> 
> I am having trouble actually seeing how the ANSI-emulation code gets
> triggered, but the comment in color.h implies that it is only printf,
> fprintf, and fputs that have the desired effect. So fwrite() may not be
> sufficient, and we may need fprintf("%.*s", len, buf) or something.

I meant to clarify how fprintf() works as oposed to fwrite(), so I had a
look and came up empty-handed. I dug a bit further and it turns out that
we actually do not override fprintf() at all anymore [*1*], so I just sent
out a patch to remove the now-obsolete comment.

Ciao,
Dscho

Re: [PATCH v2] Refactor recv_sideband()

[Johannes Schindelin]

Erm, sorry...

On Tue, 28 Jun 2016, Johannes Schindelin wrote:

> [...] we actually do not override fprintf() at all anymore [*1*] [...]

... forgot the

Footnote *1*: In Git for Windows, we actually *do* override fprintf(),
thanks to using gettext, but it is *gettext* that is doing the overriding
now, not Git.

Re: [PATCH v2] Refactor recv_sideband()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Erm, sorry...
>
> On Tue, 28 Jun 2016, Johannes Schindelin wrote:
>
>> [...] we actually do not override fprintf() at all anymore [*1*] [...]
>
> ... forgot the
>
> Footnote *1*: In Git for Windows, we actually *do* override fprintf(),
> thanks to using gettext, but it is *gettext* that is doing the overriding
> now, not Git.

That's nice to know but is not particularly actionable.  

Do you mean

 - We do color correctly without having to override fprintf() these
   days, so feel free and safe to use either fwrite() or write()

 - Because we do not override fprintf(), we no longer do colors

 - Or something in between, e.g. "we don't override fprintf()
   ourselves but gettext does X for us (for unknown value of X), so
   you will get color as long as you do Y (for unknown value of Y)"?

In other words, based on this piece of knowledge you shared with us,
what is your recommendation for Lukas's patch to do?

Thanks.

Re: [PATCH v2] Refactor recv_sideband()

[Johannes Schindelin]

Hi Junio,

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Erm, sorry...
> >
> > On Tue, 28 Jun 2016, Johannes Schindelin wrote:
> >
> >> [...] we actually do not override fprintf() at all anymore [*1*] [...]
> >
> > ... forgot the
> >
> > Footnote *1*: In Git for Windows, we actually *do* override fprintf(),
> > thanks to using gettext, but it is *gettext* that is doing the overriding
> > now, not Git.
> 
> That's nice to know but is not particularly actionable.  

Right. It was only meant as a footnote for interested parties. In Git for
Windows, we do not need to override fprintf() to support color any longer.

> Do you mean
> 
>  - We do color correctly without having to override fprintf() these
>    days, so feel free and safe to use either fwrite() or write()

This one.

>  - Because we do not override fprintf(), we no longer do colors
> 
>  - Or something in between, e.g. "we don't override fprintf()
>    ourselves but gettext does X for us (for unknown value of X), so
>    you will get color as long as you do Y (for unknown value of Y)"?
> 
> In other words, based on this piece of knowledge you shared with us,
> what is your recommendation for Lukas's patch to do?

Sorry for having been so obscure. I tried to clarify something that did
not need clarification (it is of nobody's concern that fprintf() is
overridden by gettext).

The short version is: do not worry about using fwrite().

Ciao,
Dscho

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> Before this patch, we used character buffer manipulations to split
> messages from the sideband at line breaks and insert "remote: " at the
> beginning of each line, using the packet size to determine the end of a
> message. However, since it is safe to assume that diagnostic messages
> from the sideband never contain NUL characters, we can also
> NUL-terminate the buffer, use strpbrk() for splitting lines and use
> format strings to insert the prefix.
>
> A strbuf is used for accumulating the output which is then printed using
> a single fprintf() call with a single conversion specifier per line,
> such that the atomicity of the output is preserved. See 9ac13ec (atomic
> write for sideband remote messages, 2006-10-11) for details.
>
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> ---
> Changes since v3:
> * The new code always frees the strbuf used for the output.
> * Switched back to fprintf() to support ANSI codes under Windows.
> * Added a comment on the tradeoff between atomicity and Windows support.

With input from Dscho that recent Git-for-Windows does the right
thing without limiting us to use only a subset of stdio, perhaps we
would want to squash something like this in.

diff --git a/sideband.c b/sideband.c
index 226a8c2..72e2c5c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -58,13 +58,12 @@ int recv_sideband(const char *me, int in_stream, int out)
 			 * Append a suffix to each nonempty line to clear the
 			 * end of the screen line.
 			 *
-			 * The output is accumulated in a buffer and each line
-			 * is printed to stderr using fprintf() with a single
-			 * conversion specifier. This is a "best effort"
-			 * approach to supporting both inter-process atomicity
-			 * (single conversion specifiers are likely to end up
-			 * in single atomic write() system calls) and the ANSI
-			 * control code emulation under Windows.
+			 * The output is accumulated in a buffer and
+			 * each line is printed to stderr using
+			 * fwrite(3).  This is a "best effort"
+			 * approach to suppor inter-process atomicity
+			 * (single fwrite(3) call is likely to end up
+			 * in single atomic write() system calls).
 			 */
 			while ((brk = strpbrk(b, "\n\r"))) {
 				int linelen = brk - b;
@@ -75,8 +74,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 				} else {
 					strbuf_addf(&outbuf, "%c", *brk);
 				}
-				fprintf(stderr, "%.*s", (int)outbuf.len,
-					outbuf.buf);
+				fwrite(output.buf, 1, output.len, stderr);
 				strbuf_reset(&outbuf);
 				strbuf_addf(&outbuf, "%s", PREFIX);
 
@@ -98,7 +96,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 	}
 
 	if (outbuf.len > 0)
-		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+		fwrite(output.buf, 1, output.len, stderr);
 	strbuf_release(&outbuf);
 	return retval;
 }

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> With input from Dscho that recent Git-for-Windows does the right
> thing without limiting us to use only a subset of stdio, perhaps we
> would want to squash something like this in.
>
> diff --git a/sideband.c b/sideband.c
> index 226a8c2..72e2c5c 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -58,13 +58,12 @@ int recv_sideband(const char *me, int in_stream, int out)
>  			 * Append a suffix to each nonempty line to clear the
>  			 * end of the screen line.
>  			 *
> -			 * The output is accumulated in a buffer and each line
> -			 * is printed to stderr using fprintf() with a single
> -			 * conversion specifier. This is a "best effort"
> -			 * approach to supporting both inter-process atomicity
> -			 * (single conversion specifiers are likely to end up
> -			 * in single atomic write() system calls) and the ANSI
> -			 * control code emulation under Windows.
> +			 * The output is accumulated in a buffer and
> +			 * each line is printed to stderr using
> +			 * fwrite(3).  This is a "best effort"
> +			 * approach to suppor inter-process atomicity
> +			 * (single fwrite(3) call is likely to end up
> +			 * in single atomic write() system calls).
>  			 */
>  			while ((brk = strpbrk(b, "\n\r"))) {
>  				int linelen = brk - b;

That may be good, but the remainder is crap.  Sorry about the typo;
s/output/outbuf/g is needed at least.

> @@ -75,8 +74,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  				} else {
>  					strbuf_addf(&outbuf, "%c", *brk);
>  				}
> -				fprintf(stderr, "%.*s", (int)outbuf.len,
> -					outbuf.buf);
> +				fwrite(output.buf, 1, output.len, stderr);
>  				strbuf_reset(&outbuf);
>  				strbuf_addf(&outbuf, "%s", PREFIX);
>  
> @@ -98,7 +96,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  	}
>  
>  	if (outbuf.len > 0)
> -		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> +		fwrite(output.buf, 1, output.len, stderr);
>  	strbuf_release(&outbuf);
>  	return retval;
>  }

But we would be better off using the real write(2), now Windows port
does not limit us to stdio.

And then that made me stare at the patch even more.  We still write
some error messages to stderr in the updated code (without my crap
SQUASH) inside "while (!retval)" loop:

	while (retval == 0) {
		int band, len;
		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
		if (len == 0)
			break;
		if (len < 1) {
			fprintf(stderr, "%s: protocol error: no band designator\n", me);
			retval = SIDEBAND_PROTOCOL_ERROR;
			break;
		}
		band = buf[0] & 0xff;
		buf[len] = '\0';
		len--;
		switch (band) {
		case 3:
			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
			retval = SIDEBAND_REMOTE_ERROR;
			break;
		case 2:
			...
			while ((brk = strpbrk(b, "\n\r"))) {
				...
				write(2, outbuf.buf, outbuf.len);
				...
			}

			if (*b)
				strbuf_addf(&outbuf, "%s", b);
			break;
		case 1:
			write_or_die(out, buf + 1, len);
			break;
		default:
			fprintf(stderr, "%s: protocol error: bad band #%d\n",
				me, band);
			retval = SIDEBAND_PROTOCOL_ERROR;
			break;
		}
	}

	if (outbuf.len > 0)
		write(2, outbuf.buf, outbuf.len);

In general, mixing stdio and raw file descriptor access is not such
a good idea, but these remaining calls to fprintf(stderr, ...) above
are for error-exit codepath, so from that point of view, the above
illustration may be acceptable, but there is still one niggle.

When we exit the loop because we set retval to a non-zero value,
should we still drain the outbuf?

IOW, shouldn't the if statement after "while (!retval)" loop be more
like this?

	if (!retval && outbuf.len)
        	write(2, outbuf.buf, outbuf,len);

Thanks.

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> And then that made me stare at the patch even more.  We still write
> some error messages to stderr in the updated code (without my crap
> SQUASH) inside "while (!retval)" loop:
> 
> 	while (retval == 0) {
> 		int band, len;
> 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> 		if (len == 0)
> 			break;
> 		if (len < 1) {
> 			fprintf(stderr, "%s: protocol error: no band designator\n", me);
> 			retval = SIDEBAND_PROTOCOL_ERROR;
> 			break;
> 		}
> 		band = buf[0] & 0xff;
> 		buf[len] = '\0';
> 		len--;
> 		switch (band) {
> 		case 3:
> 			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
> 			retval = SIDEBAND_REMOTE_ERROR;
> 			break;
> 		case 2:
> 			...
> 			while ((brk = strpbrk(b, "\n\r"))) {
> 				...
> 				write(2, outbuf.buf, outbuf.len);
> 				...
> 			}
> 
> 			if (*b)
> 				strbuf_addf(&outbuf, "%s", b);
> 			break;
> 		case 1:
> 			write_or_die(out, buf + 1, len);
> 			break;
> 		default:
> 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
> 				me, band);
> 			retval = SIDEBAND_PROTOCOL_ERROR;
> 			break;
> 		}
> 	}
> 
> 	if (outbuf.len > 0)
> 		write(2, outbuf.buf, outbuf.len);
> 
> In general, mixing stdio and raw file descriptor access is not such
> a good idea, but these remaining calls to fprintf(stderr, ...) above
> are for error-exit codepath, so from that point of view, the above
> illustration may be acceptable, but there is still one niggle.
> 
> When we exit the loop because we set retval to a non-zero value,
> should we still drain the outbuf?

I would think so.  Anything that the remote sent before any error should 
be printed nevertheless.  The clue for the error might be in the pending 
buffer.

However in this case the actual error printout and the pending buffer 
will appear reversed.

So what I'd suggest is actually something like this:

            if (len < 1) {
                    strbuf_addf(&outbuf, "\n%s: protocol error: no band designator\n", me);
                    retval = SIDEBAND_PROTOCOL_ERROR;
                    break;

And so on for the other error cases.


Nicolas

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

>> When we exit the loop because we set retval to a non-zero value,
>> should we still drain the outbuf?
>
> I would think so.  Anything that the remote sent before any error should 
> be printed nevertheless.  The clue for the error might be in the pending 
> buffer.
>
> However in this case the actual error printout and the pending buffer 
> will appear reversed.
>
> So what I'd suggest is actually something like this:
>
>             if (len < 1) {
>                     strbuf_addf(&outbuf, "\n%s: protocol error: no band designator\n", me);
>                     retval = SIDEBAND_PROTOCOL_ERROR;
>                     break;
>
> And so on for the other error cases.

Makes sense.

Here is what I have as a "SQUASH" on top of Lukas's change to be
queued on 'pu'.

It appears that a few tests get their expectations broken, with or
without this "SQUASH" change, though X-<.

 sideband.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/sideband.c b/sideband.c
index 226a8c2..082dfc6 100644
--- a/sideband.c
+++ b/sideband.c
@@ -33,13 +33,15 @@ int recv_sideband(const char *me, int in_stream, int out)
 	else
 		suffix = DUMB_SUFFIX;
 
-	while (retval == 0) {
+	while (!retval) {
 		int band, len;
 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
-			fprintf(stderr, "%s: protocol error: no band designator\n", me);
+			strbuf_addf(&outbuf,
+				    "\n%s: protocol error: no band designator\n",
+				    me);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
@@ -48,7 +50,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 		len--;
 		switch (band) {
 		case 3:
-			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
+			strbuf_addf(&outbuf, "\n%s%s\n", PREFIX, buf + 1);
 			retval = SIDEBAND_REMOTE_ERROR;
 			break;
 		case 2:
@@ -58,13 +60,12 @@ int recv_sideband(const char *me, int in_stream, int out)
 			 * Append a suffix to each nonempty line to clear the
 			 * end of the screen line.
 			 *
-			 * The output is accumulated in a buffer and each line
-			 * is printed to stderr using fprintf() with a single
-			 * conversion specifier. This is a "best effort"
-			 * approach to supporting both inter-process atomicity
-			 * (single conversion specifiers are likely to end up
-			 * in single atomic write() system calls) and the ANSI
-			 * control code emulation under Windows.
+			 * The output is accumulated in a buffer and
+			 * each line is printed to stderr using
+			 * fwrite(3).  This is a "best effort"
+			 * approach to support inter-process atomicity
+			 * (single fwrite(3) call is likely to end up
+			 * in single atomic write() system calls).
 			 */
 			while ((brk = strpbrk(b, "\n\r"))) {
 				int linelen = brk - b;
@@ -75,8 +76,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 				} else {
 					strbuf_addf(&outbuf, "%c", *brk);
 				}
-				fprintf(stderr, "%.*s", (int)outbuf.len,
-					outbuf.buf);
+				fwrite(outbuf.buf, 1, outbuf.len, stderr);
 				strbuf_reset(&outbuf);
 				strbuf_addf(&outbuf, "%s", PREFIX);
 
@@ -90,15 +90,15 @@ int recv_sideband(const char *me, int in_stream, int out)
 			write_or_die(out, buf + 1, len);
 			break;
 		default:
-			fprintf(stderr, "%s: protocol error: bad band #%d\n",
+			strbuf_addf(&outbuf, "\n%s: protocol error: bad band #%d\n",
 				me, band);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
 	}
 
-	if (outbuf.len > 0)
-		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+	if (outbuf.len)
+		fwrite(outbuf.buf, 1, outbuf.len, stderr);
 	strbuf_release(&outbuf);
 	return retval;
 }

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> >> When we exit the loop because we set retval to a non-zero value,
> >> should we still drain the outbuf?
> >
> > I would think so.  Anything that the remote sent before any error should 
> > be printed nevertheless.  The clue for the error might be in the pending 
> > buffer.
> >
> > However in this case the actual error printout and the pending buffer 
> > will appear reversed.
> >
> > So what I'd suggest is actually something like this:
> >
> >             if (len < 1) {
> >                     strbuf_addf(&outbuf, "\n%s: protocol error: no band designator\n", me);
> >                     retval = SIDEBAND_PROTOCOL_ERROR;
> >                     break;
> >
> > And so on for the other error cases.
> 
> Makes sense.
> 
> Here is what I have as a "SQUASH" on top of Lukas's change to be
> queued on 'pu'.

Looks good to me.

Acked-by: Nicolas Pitre <nico@linaro.org>

> It appears that a few tests get their expectations broken, with or
> without this "SQUASH" change, though X-<.

Without this, the error and remaining buffer would be reversed as 
mentioned previously.  With this, the order is restored, but a newline 
is added to unterminated lines whereas the error was simply appended to 
the output before Lukas' patch.

In any case the new behavior is probably better and I'd simply adjust 
the test expectations.


> 
>  sideband.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/sideband.c b/sideband.c
> index 226a8c2..082dfc6 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -33,13 +33,15 @@ int recv_sideband(const char *me, int in_stream, int out)
>  	else
>  		suffix = DUMB_SUFFIX;
>  
> -	while (retval == 0) {
> +	while (!retval) {
>  		int band, len;
>  		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
>  		if (len == 0)
>  			break;
>  		if (len < 1) {
> -			fprintf(stderr, "%s: protocol error: no band designator\n", me);
> +			strbuf_addf(&outbuf,
> +				    "\n%s: protocol error: no band designator\n",
> +				    me);
>  			retval = SIDEBAND_PROTOCOL_ERROR;
>  			break;
>  		}
> @@ -48,7 +50,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  		len--;
>  		switch (band) {
>  		case 3:
> -			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
> +			strbuf_addf(&outbuf, "\n%s%s\n", PREFIX, buf + 1);
>  			retval = SIDEBAND_REMOTE_ERROR;
>  			break;
>  		case 2:
> @@ -58,13 +60,12 @@ int recv_sideband(const char *me, int in_stream, int out)
>  			 * Append a suffix to each nonempty line to clear the
>  			 * end of the screen line.
>  			 *
> -			 * The output is accumulated in a buffer and each line
> -			 * is printed to stderr using fprintf() with a single
> -			 * conversion specifier. This is a "best effort"
> -			 * approach to supporting both inter-process atomicity
> -			 * (single conversion specifiers are likely to end up
> -			 * in single atomic write() system calls) and the ANSI
> -			 * control code emulation under Windows.
> +			 * The output is accumulated in a buffer and
> +			 * each line is printed to stderr using
> +			 * fwrite(3).  This is a "best effort"
> +			 * approach to support inter-process atomicity
> +			 * (single fwrite(3) call is likely to end up
> +			 * in single atomic write() system calls).
>  			 */
>  			while ((brk = strpbrk(b, "\n\r"))) {
>  				int linelen = brk - b;
> @@ -75,8 +76,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  				} else {
>  					strbuf_addf(&outbuf, "%c", *brk);
>  				}
> -				fprintf(stderr, "%.*s", (int)outbuf.len,
> -					outbuf.buf);
> +				fwrite(outbuf.buf, 1, outbuf.len, stderr);
>  				strbuf_reset(&outbuf);
>  				strbuf_addf(&outbuf, "%s", PREFIX);
>  
> @@ -90,15 +90,15 @@ int recv_sideband(const char *me, int in_stream, int out)
>  			write_or_die(out, buf + 1, len);
>  			break;
>  		default:
> -			fprintf(stderr, "%s: protocol error: bad band #%d\n",
> +			strbuf_addf(&outbuf, "\n%s: protocol error: bad band #%d\n",
>  				me, band);
>  			retval = SIDEBAND_PROTOCOL_ERROR;
>  			break;
>  		}
>  	}
>  
> -	if (outbuf.len > 0)
> -		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> +	if (outbuf.len)
> +		fwrite(outbuf.buf, 1, outbuf.len, stderr);
>  	strbuf_release(&outbuf);
>  	return retval;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> Without this, the error and remaining buffer would be reversed as 
> mentioned previously.  With this, the order is restored, but a newline 
> is added to unterminated lines whereas the error was simply appended to 
> the output before Lukas' patch.
>
> In any case the new behavior is probably better and I'd simply adjust 
> the test expectations.

There is something else going on.  I cannot quite explain why I am
getting this failure from t5401-update-hooks.sh, for example:

    --- expect      2016-06-28 19:46:24.564937075 +0000
    +++ actual      2016-06-28 19:46:24.564937075 +0000
    @@ -9,3 +9,4 @@
     remote: STDERR post-receive
     remote: STDOUT post-update
     remote: STDERR post-update
    +remote: To ./victim.git
    not ok 12 - send-pack stderr contains hook messages

... goes and looks what v2.9.0 produces, which ends like this:

    ...
    remote: STDERR post-receive        
    remote: STDOUT post-update        
    remote: STDERR post-update        
    To ./victim.git
       e4822ab..2b65bd1  master -> master
     ! [remote rejected] tofail -> tofail (hook declined)

The test checks if lines prefixed with "remote: " match the expected
output, and the difference is an indication that the new code is
showing an extra incomplete-line "remote: " before other parts of
the code says "To ./victim.git" to report where the push is going.

It appeasrs that the "Refector"ed logic needs to be a bit more
careful when relaying an empty payload.

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > Without this, the error and remaining buffer would be reversed as 
> > mentioned previously.  With this, the order is restored, but a newline 
> > is added to unterminated lines whereas the error was simply appended to 
> > the output before Lukas' patch.
> >
> > In any case the new behavior is probably better and I'd simply adjust 
> > the test expectations.
> 
> There is something else going on.  I cannot quite explain why I am
> getting this failure from t5401-update-hooks.sh, for example:
> 
>     --- expect      2016-06-28 19:46:24.564937075 +0000
>     +++ actual      2016-06-28 19:46:24.564937075 +0000
>     @@ -9,3 +9,4 @@
>      remote: STDERR post-receive
>      remote: STDOUT post-update
>      remote: STDERR post-update
>     +remote: To ./victim.git
>     not ok 12 - send-pack stderr contains hook messages
> 
> ... goes and looks what v2.9.0 produces, which ends like this:
> 
>     ...
>     remote: STDERR post-receive        
>     remote: STDOUT post-update        
>     remote: STDERR post-update        
>     To ./victim.git
>        e4822ab..2b65bd1  master -> master
>      ! [remote rejected] tofail -> tofail (hook declined)
> 
> The test checks if lines prefixed with "remote: " match the expected
> output, and the difference is an indication that the new code is
> showing an extra incomplete-line "remote: " before other parts of
> the code says "To ./victim.git" to report where the push is going.

Ah...  I think I know what's going on.

The leftover data in the strbuf is normally (when there is no errors) an 
unterminated line. So instead of doing:

-                       fprintf(stderr, "%s: protocol error: no band designator\n", me);
+                       strbuf_addf(&outbuf,
+                                   "\n%s: protocol error: no band designator\n",
+                                   me);

you could omit the final \n in the format string and:

-       if (outbuf.len > 0)
-               fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+       if (outbuf.len)
+               fwrite(outbuf.buf, 1, outbuf.len, stderr);
        strbuf_release(&outbuf);

and here a \n could be added before writing out the buffer.


Nicolas

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

>> There is something else going on.  I cannot quite explain why I am
>> getting this failure from t5401-update-hooks.sh, for example:
>> 
>>     --- expect      2016-06-28 19:46:24.564937075 +0000
>>     +++ actual      2016-06-28 19:46:24.564937075 +0000
>>     @@ -9,3 +9,4 @@
>>      remote: STDERR post-receive
>>      remote: STDOUT post-update
>>      remote: STDERR post-update
>>     +remote: To ./victim.git
>>     not ok 12 - send-pack stderr contains hook messages
>> 
>> ... goes and looks what v2.9.0 produces, which ends like this:
>> 
>>     ...
>>     remote: STDERR post-receive        
>>     remote: STDOUT post-update        
>>     remote: STDERR post-update        
>>     To ./victim.git
>>        e4822ab..2b65bd1  master -> master
>>      ! [remote rejected] tofail -> tofail (hook declined)
>> 
>> The test checks if lines prefixed with "remote: " match the expected
>> output, and the difference is an indication that the new code is
>> showing an extra incomplete-line "remote: " before other parts of
>> the code says "To ./victim.git" to report where the push is going.
>
> Ah...  I think I know what's going on.
>
> The leftover data in the strbuf is normally (when there is no errors) an 
> unterminated line. So instead of doing:
>
> -                       fprintf(stderr, "%s: protocol error: no band designator\n", me);
> +                       strbuf_addf(&outbuf,
> +                                   "\n%s: protocol error: no band designator\n",
> +                                   me);
>
> you could omit the final \n in the format string and:
>
> -       if (outbuf.len > 0)
> -               fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> +       if (outbuf.len)
> +               fwrite(outbuf.buf, 1, outbuf.len, stderr);
>         strbuf_release(&outbuf);
>
> and here a \n could be added before writing out the buffer.

Unfortunately, that is not it.

The basic structure of the code (without the "SQUASH" we discussed)
looks like this:

	strbuf_addf(&outbuf, "%s", PREFIX);
	while (retval == 0) {
		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
		...
		band = buf[0] & 0xff;
		switch (band) {
		case 3:
			... /* emergency exit */
		case 2:
			while ((brk = strpbrk(b, "\n\r"))) {
				int linelen = brk - b;

				if (linelen > 0) {
					strbuf_addf(&outbuf, "%.*s%s%c",
						    linelen, b, suffix, *brk);
				} else {
					strbuf_addf(&outbuf, "%c", *brk);
				}
				fprintf(stderr, "%.*s", (int)outbuf.len,
					outbuf.buf);
				strbuf_reset(&outbuf);
				strbuf_addf(&outbuf, "%s", PREFIX);
				b = brk + 1;
			}
			if (*b)
				strbuf_addf(&outbuf, "%s", b);
			break;
		...
		}
	}

	if (outbuf.len > 0)
		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);

Imagine we are reading from band #2 and we find a complete line.  We
concatenate the payload up to the LF at the end of the line to the
PREFIX we prepared outside the loop and emit it, and then we ASSUME
that we further have something after strpbrk() and add PREFIX to the
buffer, before going to the next line in the payload.

But there may not be anything after the LF.  outbuf.len is still
counting the PREFIX and we end up showing it, without termination.

This takes us back to what I said in my review of an earlier round,
in $gmane/297332, where I said:

    Instead of doing "we assume outbuf already has PREFIX when we add
    contents from buf[]", the code structure would be better if you:

     * make outbuf.buf contain PREFIX at the beginning of this innermost
       loop; lose the reset/addf from here.

     * move strbuf_reset(&outbuf) at the end of the next if (*b) block
       to just before "continue;"

    perhaps?

I think the strbuf_addf(PREFIX) above the loop should be removed,
and instead the code should use the PREFIX only when it decides that
there is something worth emitting, i.e.

	while (!retval) {
        	len = packet_read();
                ...
                band = buf[0] & 0xff;
                switch (band) {
                case 3:
                	... /* emergency exit */
		case 2:
                	while ((brk = ...)) {
                        	/* we have something to say */
				strbuf_reset(&outbuf);
                                strbuf_addstr(&outbuf, PREFIX);
                                if (linelen)
                                	strbuf_addf(...);
				else
                                	strbuf_addch(*brk);
				fwrite(outbuf.buf, 1, outbuf.len, stderr);
				b = brk + 1;
			}
                        if (*b) {
                        	/* we still have something to say */
				strbuf_reset(&outbuf);
                                strbuf_addstr(&outbuf, PREFIX);
                               	strbuf_addf(...);
			}
                        break;
		...
                }
	}

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> >> There is something else going on.  I cannot quite explain why I am
> >> getting this failure from t5401-update-hooks.sh, for example:
> >> 
> >>     --- expect      2016-06-28 19:46:24.564937075 +0000
> >>     +++ actual      2016-06-28 19:46:24.564937075 +0000
> >>     @@ -9,3 +9,4 @@
> >>      remote: STDERR post-receive
> >>      remote: STDOUT post-update
> >>      remote: STDERR post-update
> >>     +remote: To ./victim.git
> >>     not ok 12 - send-pack stderr contains hook messages
> >> 
> >> ... goes and looks what v2.9.0 produces, which ends like this:
> >> 
> >>     ...
> >>     remote: STDERR post-receive        
> >>     remote: STDOUT post-update        
> >>     remote: STDERR post-update        
> >>     To ./victim.git
> >>        e4822ab..2b65bd1  master -> master
> >>      ! [remote rejected] tofail -> tofail (hook declined)
> >> 
> >> The test checks if lines prefixed with "remote: " match the expected
> >> output, and the difference is an indication that the new code is
> >> showing an extra incomplete-line "remote: " before other parts of
> >> the code says "To ./victim.git" to report where the push is going.
> >
> > Ah...  I think I know what's going on.
> >
> > The leftover data in the strbuf is normally (when there is no errors) an 
> > unterminated line. So instead of doing:
> >
> > -                       fprintf(stderr, "%s: protocol error: no band designator\n", me);
> > +                       strbuf_addf(&outbuf,
> > +                                   "\n%s: protocol error: no band designator\n",
> > +                                   me);
> >
> > you could omit the final \n in the format string and:
> >
> > -       if (outbuf.len > 0)
> > -               fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> > +       if (outbuf.len)
> > +               fwrite(outbuf.buf, 1, outbuf.len, stderr);
> >         strbuf_release(&outbuf);
> >
> > and here a \n could be added before writing out the buffer.
> 
> Unfortunately, that is not it.
> 
> The basic structure of the code (without the "SQUASH" we discussed)
> looks like this:
> 
> 	strbuf_addf(&outbuf, "%s", PREFIX);
> 	while (retval == 0) {
> 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> 		...
> 		band = buf[0] & 0xff;
> 		switch (band) {
> 		case 3:
> 			... /* emergency exit */
> 		case 2:
> 			while ((brk = strpbrk(b, "\n\r"))) {
> 				int linelen = brk - b;
> 
> 				if (linelen > 0) {
> 					strbuf_addf(&outbuf, "%.*s%s%c",
> 						    linelen, b, suffix, *brk);
> 				} else {
> 					strbuf_addf(&outbuf, "%c", *brk);
> 				}
> 				fprintf(stderr, "%.*s", (int)outbuf.len,
> 					outbuf.buf);
> 				strbuf_reset(&outbuf);
> 				strbuf_addf(&outbuf, "%s", PREFIX);
> 				b = brk + 1;
> 			}
> 			if (*b)
> 				strbuf_addf(&outbuf, "%s", b);
> 			break;
> 		...
> 		}
> 	}
> 
> 	if (outbuf.len > 0)
> 		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> 
> Imagine we are reading from band #2 and we find a complete line.  We
> concatenate the payload up to the LF at the end of the line to the
> PREFIX we prepared outside the loop and emit it, and then we ASSUME
> that we further have something after strpbrk() and add PREFIX to the
> buffer, before going to the next line in the payload.
> 
> But there may not be anything after the LF.  outbuf.len is still
> counting the PREFIX and we end up showing it, without termination.

You're right.  Although my previous observations still apply.

> This takes us back to what I said in my review of an earlier round,
> in $gmane/297332, where I said:
> 
>     Instead of doing "we assume outbuf already has PREFIX when we add
>     contents from buf[]", the code structure would be better if you:
> 
>      * make outbuf.buf contain PREFIX at the beginning of this innermost
>        loop; lose the reset/addf from here.
> 
>      * move strbuf_reset(&outbuf) at the end of the next if (*b) block
>        to just before "continue;"
> 
>     perhaps?
> 
> I think the strbuf_addf(PREFIX) above the loop should be removed,
> and instead the code should use the PREFIX only when it decides that
> there is something worth emitting, i.e.
> 
> 	while (!retval) {
>         	len = packet_read();
>                 ...
>                 band = buf[0] & 0xff;
>                 switch (band) {
>                 case 3:
>                 	... /* emergency exit */
> 		case 2:
>                 	while ((brk = ...)) {
>                         	/* we have something to say */
> 				strbuf_reset(&outbuf);
>                                 strbuf_addstr(&outbuf, PREFIX);

That won't work. If at this point there is the beginning of a partial 
line queued in the buffer from the previous round waiting to see its 
line break, you just deleted the beginning of that line.

Furthermore, that partial line won't get a prefix if it doesn't have at 
least one line break in the packet data.

Rather the prefix should be added whenever the buffer is empty before 
every addition.

>                                 if (linelen)
>                                 	strbuf_addf(...);
> 				else
>                                 	strbuf_addch(*brk);
> 				fwrite(outbuf.buf, 1, outbuf.len, stderr);
> 				b = brk + 1;
> 			}
>                         if (*b) {
>                         	/* we still have something to say */
> 				strbuf_reset(&outbuf);
>                                 strbuf_addstr(&outbuf, PREFIX);
>                                	strbuf_addf(...);

This is also wrong.  If the middle part of a partial line is received, 
you just deleted its queued beginning.


Nicolas

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

>> The basic structure of the code (without the "SQUASH" we discussed)
>> looks like this:
>> 
>> 	strbuf_addf(&outbuf, "%s", PREFIX);
>> 	while (retval == 0) {
>> 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
>> 		...
>> 		band = buf[0] & 0xff;
>> 		switch (band) {
>> 		case 3:
>> 			... /* emergency exit */
>> 		case 2:
>> 			while ((brk = strpbrk(b, "\n\r"))) {
>> 				int linelen = brk - b;
>> 
>> 				if (linelen > 0) {
>> 					strbuf_addf(&outbuf, "%.*s%s%c",
>> 						    linelen, b, suffix, *brk);
>> 				} else {
>> 					strbuf_addf(&outbuf, "%c", *brk);
>> 				}
>> 				fprintf(stderr, "%.*s", (int)outbuf.len,
>> 					outbuf.buf);
>> 				strbuf_reset(&outbuf);
>> 				strbuf_addf(&outbuf, "%s", PREFIX);
>> 				b = brk + 1;
>> 			}
>> 			if (*b)
>> 				strbuf_addf(&outbuf, "%s", b);
>> 			break;
>> 		...
>> 		}
>> 	}
>> 
>> 	if (outbuf.len > 0)
>> 		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
>>  ...
> That won't work. If at this point there is the beginning of a partial 
> line queued in the buffer from the previous round waiting to see its 
> line break, you just deleted the beginning of that line.

Ahh, OK, a single logical line split into two and sent in two
packets--the first one would not result in any output (just does "if
(*b) strbuf_addf(...)" to buffer it) and then the second one finally
finds a LF.  Yeah, that won't work if we cleared.

But then my observation still holds, no?

	while () {
        	read();
                switch () {
                case 2:
                	while ((brk = strpbfk())) {
                        	/* we have LF! we'll say something! */
                                strbuf_splice(&outbuf, 0, 0,
                                	      PREFIX, strlen(PREFIX));
                                strbuf_addf(&outbuf, ...);
                                fwrite(...);
				b = brk + 1;
			}
                        if (*b)
                        	strbuf_addstr(&outbuf, b);
			break;
		}
	}

        if (outbuf.len) {
        	/* we still have something to say */
                strbuf_splice(&outbuf, 0, 0, PREFIX,
        	strlen(PREFIX));
                fwrite(...);
	}

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> But then my observation still holds, no?
>
> ...
>         if (outbuf.len) {
>         	/* we still have something to say */
>                 strbuf_splice(&outbuf, 0, 0, PREFIX,
>         	strlen(PREFIX));
>                 fwrite(...);
> 	}

I guess these two are more or less equivalent.

I view "outbuf" more as "holdbuf", i.e. the payload that has been
received and yet to be shown, and from that point of view, having
and keeping PREFIX which is not something we received in there while
looping to grab more input makes me feel dirty.  IOW, I consider
"empty" is the correct base state of the hold buffer before anything
happens.

I understand that you view "outbuf" as what has been prepared to be
shown but not ready to be shown due to lack of LF, and from that
point of view, the base state of the out buffer before anything
happens could be "it has PREFIX and nothing else".

It's just that if you take the latter, then the conditional after
the loop exits (i.e. the last transmission was an incomplete line)
cannot be "is outbuf empty?", as your base state is "has PREFIX and
can never be empty".  I was working back from that if statement.

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> >> The basic structure of the code (without the "SQUASH" we discussed)
> >> looks like this:
> >> 
> >> 	strbuf_addf(&outbuf, "%s", PREFIX);
> >> 	while (retval == 0) {
> >> 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> >> 		...
> >> 		band = buf[0] & 0xff;
> >> 		switch (band) {
> >> 		case 3:
> >> 			... /* emergency exit */
> >> 		case 2:
> >> 			while ((brk = strpbrk(b, "\n\r"))) {
> >> 				int linelen = brk - b;
> >> 
> >> 				if (linelen > 0) {
> >> 					strbuf_addf(&outbuf, "%.*s%s%c",
> >> 						    linelen, b, suffix, *brk);
> >> 				} else {
> >> 					strbuf_addf(&outbuf, "%c", *brk);
> >> 				}
> >> 				fprintf(stderr, "%.*s", (int)outbuf.len,
> >> 					outbuf.buf);
> >> 				strbuf_reset(&outbuf);
> >> 				strbuf_addf(&outbuf, "%s", PREFIX);
> >> 				b = brk + 1;
> >> 			}
> >> 			if (*b)
> >> 				strbuf_addf(&outbuf, "%s", b);
> >> 			break;
> >> 		...
> >> 		}
> >> 	}
> >> 
> >> 	if (outbuf.len > 0)
> >> 		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> >>  ...
> > That won't work. If at this point there is the beginning of a partial 
> > line queued in the buffer from the previous round waiting to see its 
> > line break, you just deleted the beginning of that line.
> 
> Ahh, OK, a single logical line split into two and sent in two
> packets--the first one would not result in any output (just does "if
> (*b) strbuf_addf(...)" to buffer it) and then the second one finally
> finds a LF.  Yeah, that won't work if we cleared.
> 
> But then my observation still holds, no?

I think it does... but I'm no longer sure of what you meant.

To make it clearer, here's a patch on top of pu that fixes all the 
issues I think are remaining. All tests pass now.

diff --git a/sideband.c b/sideband.c
index 36a032f..0e6c6df 100644
--- a/sideband.c
+++ b/sideband.c
@@ -23,10 +23,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 	const char *term, *suffix;
 	char buf[LARGE_PACKET_MAX + 1];
 	struct strbuf outbuf = STRBUF_INIT;
-	const char *b, *brk;
 	int retval = 0;
 
-	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
@@ -34,14 +32,15 @@ int recv_sideband(const char *me, int in_stream, int out)
 		suffix = DUMB_SUFFIX;
 
 	while (!retval) {
+		const char *b, *brk;
 		int band, len;
 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
 			strbuf_addf(&outbuf,
-				    "\n%s: protocol error: no band designator\n",
-				    me);
+				    "%s%s: protocol error: no band designator",
+				    outbuf.len ? "\n" : "", me);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
@@ -50,7 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 		len--;
 		switch (band) {
 		case 3:
-			strbuf_addf(&outbuf, "\n%s%s\n", PREFIX, buf + 1);
+			strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
+				    PREFIX, buf + 1);
 			retval = SIDEBAND_REMOTE_ERROR;
 			break;
 		case 2:
@@ -70,6 +70,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 			while ((brk = strpbrk(b, "\n\r"))) {
 				int linelen = brk - b;
 
+				if (!outbuf.len)
+					strbuf_addf(&outbuf, "%s", PREFIX);
 				if (linelen > 0) {
 					strbuf_addf(&outbuf, "%.*s%s%c",
 						    linelen, b, suffix, *brk);
@@ -78,27 +80,29 @@ int recv_sideband(const char *me, int in_stream, int out)
 				}
 				fwrite(outbuf.buf, 1, outbuf.len, stderr);
 				strbuf_reset(&outbuf);
-				strbuf_addf(&outbuf, "%s", PREFIX);
 
 				b = brk + 1;
 			}
 
 			if (*b)
-				strbuf_addf(&outbuf, "%s", b);
+				strbuf_addf(&outbuf, "%s%s",
+					    outbuf.len ? "" : PREFIX, b);
 			break;
 		case 1:
 			write_or_die(out, buf + 1, len);
 			break;
 		default:
-			strbuf_addf(&outbuf, "\n%s: protocol error: bad band #%d\n",
-				me, band);
+			strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
+				    outbuf.len ? "\n" : "", me, band);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
 	}
 
-	if (outbuf.len)
+	if (outbuf.len) {
+		strbuf_addf(&outbuf, "\n");
 		fwrite(outbuf.buf, 1, outbuf.len, stderr);
+	}
 	strbuf_release(&outbuf);
 	return retval;
 }

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> It's just that if you take the latter, then the conditional after
> the loop exits (i.e. the last transmission was an incomplete line)
> cannot be "is outbuf empty?", as your base state is "has PREFIX and
> can never be empty".  I was working back from that if statement.

Let's try this again.  How does this look?

In this version:

 - "outbuf" is where we keep the (possibly partial) data collected
   to be eventually shown;

 - output of pending (possibly partial) data is handled by a helper
   function drain().  It is responsible for prepending of the
   PREFIX, which is treated purely as a cosmetic thing.  It also is
   responsible for completing an incomplete line at the end of the
   transmission (e.g. flushing of the buffered input upon reception of
   the emergency exit packet).

 - locally generated errors go directly to fprintf(stderr),
   bypassing outbuf (hence drain()).

 sideband.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/sideband.c b/sideband.c
index 226a8c2..6873137 100644
--- a/sideband.c
+++ b/sideband.c
@@ -18,6 +18,16 @@
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
+static void drain(struct strbuf *outbuf)
+{
+	if (!outbuf->len)
+		return;
+	strbuf_splice(outbuf, 0, 0, PREFIX, strlen(PREFIX));
+	strbuf_complete_line(outbuf);
+	fwrite(outbuf->buf, 1, outbuf->len, stderr);
+	strbuf_reset(outbuf);
+}
+
 int recv_sideband(const char *me, int in_stream, int out)
 {
 	const char *term, *suffix;
@@ -26,20 +36,21 @@ int recv_sideband(const char *me, int in_stream, int out)
 	const char *b, *brk;
 	int retval = 0;
 
-	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
 
-	while (retval == 0) {
+	while (!retval) {
 		int band, len;
 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
-			fprintf(stderr, "%s: protocol error: no band designator\n", me);
+			drain(&outbuf);
+			fprintf(stderr, "%s: protocol error: no band designator\n",
+				me);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
@@ -48,7 +59,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 		len--;
 		switch (band) {
 		case 3:
-			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
+			drain(&outbuf);
+			strbuf_addf(&outbuf, "%s\n", buf + 1);
 			retval = SIDEBAND_REMOTE_ERROR;
 			break;
 		case 2:
@@ -58,13 +70,12 @@ int recv_sideband(const char *me, int in_stream, int out)
 			 * Append a suffix to each nonempty line to clear the
 			 * end of the screen line.
 			 *
-			 * The output is accumulated in a buffer and each line
-			 * is printed to stderr using fprintf() with a single
-			 * conversion specifier. This is a "best effort"
-			 * approach to supporting both inter-process atomicity
-			 * (single conversion specifiers are likely to end up
-			 * in single atomic write() system calls) and the ANSI
-			 * control code emulation under Windows.
+			 * The output is accumulated in a buffer and
+			 * each line is printed to stderr using
+			 * fwrite(3).  This is a "best effort"
+			 * approach to support inter-process atomicity
+			 * (single fwrite(3) call is likely to end up
+			 * in single atomic write() system calls).
 			 */
 			while ((brk = strpbrk(b, "\n\r"))) {
 				int linelen = brk - b;
@@ -75,11 +86,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 				} else {
 					strbuf_addf(&outbuf, "%c", *brk);
 				}
-				fprintf(stderr, "%.*s", (int)outbuf.len,
-					outbuf.buf);
-				strbuf_reset(&outbuf);
-				strbuf_addf(&outbuf, "%s", PREFIX);
-
+				drain(&outbuf);
 				b = brk + 1;
 			}
 
@@ -90,6 +97,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 			write_or_die(out, buf + 1, len);
 			break;
 		default:
+			drain(&outbuf);
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
 				me, band);
 			retval = SIDEBAND_PROTOCOL_ERROR;
@@ -97,8 +105,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 		}
 	}
 
-	if (outbuf.len > 0)
-		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+	drain(&outbuf);
 	strbuf_release(&outbuf);
 	return retval;
 }

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > It's just that if you take the latter, then the conditional after
> > the loop exits (i.e. the last transmission was an incomplete line)
> > cannot be "is outbuf empty?", as your base state is "has PREFIX and
> > can never be empty".  I was working back from that if statement.
> 
> Let's try this again.  How does this look?

Still broken.

> In this version:
> 
>  - "outbuf" is where we keep the (possibly partial) data collected
>    to be eventually shown;
> 
>  - output of pending (possibly partial) data is handled by a helper
>    function drain().  It is responsible for prepending of the
>    PREFIX, which is treated purely as a cosmetic thing.  It also is
>    responsible for completing an incomplete line at the end of the
>    transmission (e.g. flushing of the buffered input upon reception of
>    the emergency exit packet).

It uses strbuf_complete_line() which destroys the intended result for 
lines that end with '\r'.

>  - locally generated errors go directly to fprintf(stderr),
>    bypassing outbuf (hence drain()).
> 
>  sideband.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/sideband.c b/sideband.c
> index 226a8c2..6873137 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -18,6 +18,16 @@
>  #define ANSI_SUFFIX "\033[K"
>  #define DUMB_SUFFIX "        "
>  
> +static void drain(struct strbuf *outbuf)
> +{
> +	if (!outbuf->len)
> +		return;
> +	strbuf_splice(outbuf, 0, 0, PREFIX, strlen(PREFIX));
> +	strbuf_complete_line(outbuf);
> +	fwrite(outbuf->buf, 1, outbuf->len, stderr);
> +	strbuf_reset(outbuf);
> +}
> +
>  int recv_sideband(const char *me, int in_stream, int out)
>  {
>  	const char *term, *suffix;
> @@ -26,20 +36,21 @@ int recv_sideband(const char *me, int in_stream, int out)
>  	const char *b, *brk;
>  	int retval = 0;
>  
> -	strbuf_addf(&outbuf, "%s", PREFIX);
>  	term = getenv("TERM");
>  	if (isatty(2) && term && strcmp(term, "dumb"))
>  		suffix = ANSI_SUFFIX;
>  	else
>  		suffix = DUMB_SUFFIX;
>  
> -	while (retval == 0) {
> +	while (!retval) {
>  		int band, len;
>  		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
>  		if (len == 0)
>  			break;
>  		if (len < 1) {
> -			fprintf(stderr, "%s: protocol error: no band designator\n", me);
> +			drain(&outbuf);
> +			fprintf(stderr, "%s: protocol error: no band designator\n",
> +				me);
>  			retval = SIDEBAND_PROTOCOL_ERROR;
>  			break;
>  		}
> @@ -48,7 +59,8 @@ int recv_sideband(const char *me, int in_stream, int out)
>  		len--;
>  		switch (band) {
>  		case 3:
> -			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
> +			drain(&outbuf);
> +			strbuf_addf(&outbuf, "%s\n", buf + 1);
>  			retval = SIDEBAND_REMOTE_ERROR;
>  			break;
>  		case 2:
> @@ -58,13 +70,12 @@ int recv_sideband(const char *me, int in_stream, int out)
>  			 * Append a suffix to each nonempty line to clear the
>  			 * end of the screen line.
>  			 *
> -			 * The output is accumulated in a buffer and each line
> -			 * is printed to stderr using fprintf() with a single
> -			 * conversion specifier. This is a "best effort"
> -			 * approach to supporting both inter-process atomicity
> -			 * (single conversion specifiers are likely to end up
> -			 * in single atomic write() system calls) and the ANSI
> -			 * control code emulation under Windows.
> +			 * The output is accumulated in a buffer and
> +			 * each line is printed to stderr using
> +			 * fwrite(3).  This is a "best effort"
> +			 * approach to support inter-process atomicity
> +			 * (single fwrite(3) call is likely to end up
> +			 * in single atomic write() system calls).
>  			 */
>  			while ((brk = strpbrk(b, "\n\r"))) {
>  				int linelen = brk - b;
> @@ -75,11 +86,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  				} else {
>  					strbuf_addf(&outbuf, "%c", *brk);
>  				}
> -				fprintf(stderr, "%.*s", (int)outbuf.len,
> -					outbuf.buf);
> -				strbuf_reset(&outbuf);
> -				strbuf_addf(&outbuf, "%s", PREFIX);
> -
> +				drain(&outbuf);
>  				b = brk + 1;
>  			}
>  
> @@ -90,6 +97,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  			write_or_die(out, buf + 1, len);
>  			break;
>  		default:
> +			drain(&outbuf);
>  			fprintf(stderr, "%s: protocol error: bad band #%d\n",
>  				me, band);
>  			retval = SIDEBAND_PROTOCOL_ERROR;
> @@ -97,8 +105,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  		}
>  	}
>  
> -	if (outbuf.len > 0)
> -		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> +	drain(&outbuf);
>  	strbuf_release(&outbuf);
>  	return retval;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> To make it clearer, here's a patch on top of pu that fixes all the 
> issues I think are remaining. All tests pass now.
>
> diff --git a/sideband.c b/sideband.c
> index 36a032f..0e6c6df 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -23,10 +23,8 @@ int recv_sideband(const char *me, int in_stream, int out)
>  	const char *term, *suffix;
>  	char buf[LARGE_PACKET_MAX + 1];
>  	struct strbuf outbuf = STRBUF_INIT;
> -	const char *b, *brk;
>  	int retval = 0;
>  
> -	strbuf_addf(&outbuf, "%s", PREFIX);
>  	term = getenv("TERM");
>  	if (isatty(2) && term && strcmp(term, "dumb"))
>  		suffix = ANSI_SUFFIX;
> @@ -34,14 +32,15 @@ int recv_sideband(const char *me, int in_stream, int out)
>  		suffix = DUMB_SUFFIX;
>  
>  	while (!retval) {
> +		const char *b, *brk;
>  		int band, len;
>  		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
>  		if (len == 0)
>  			break;
>  		if (len < 1) {
>  			strbuf_addf(&outbuf,
> -				    "\n%s: protocol error: no band designator\n",
> -				    me);
> +				    "%s%s: protocol error: no band designator",
> +				    outbuf.len ? "\n" : "", me);

OK, because you no longer put PREFIX in outbuf, outbuf.len becomes
an easy way to tell if we have anything accumulated, and if there
already is something, we separate our message from it with a LF.

I like the simplicity and clarity of the logic.

> @@ -50,7 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out)
>  		len--;
>  		switch (band) {
>  		case 3:
> -			strbuf_addf(&outbuf, "\n%s%s\n", PREFIX, buf + 1);
> +			strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
> +				    PREFIX, buf + 1);

Likewise for all other changes.

> -	if (outbuf.len)
> +	if (outbuf.len) {
> +		strbuf_addf(&outbuf, "\n");
>  		fwrite(outbuf.buf, 1, outbuf.len, stderr);
> +	}

... and this does make sense.

Lukas, can you see what is in 'pu' after I push out today's
integration result in several hours and tell us if you like the
result of the SQUASH??? change?

Thanks.

Re: [PATCH v4] Refactor recv_sideband()

[Lukas Fleischer]

On Wed, 29 Jun 2016 at 18:40:16, Junio C Hamano wrote:
> Lukas, can you see what is in 'pu' after I push out today's
> integration result in several hours and tell us if you like the
> result of the SQUASH??? change?

Looks good to me. Thank you both for working on this. Note that you
should amend the last paragraph of the original commit message when you
squash Nicos patch into mine.

Oh, and one more detail: I wonder why we still use fwrite(), now that we
know we can use xwrite() which guarantees atomicity. Is there a reason
for that?

Regards,
Lukas

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> Oh, and one more detail: I wonder why we still use fwrite(), now that we
> know we can use xwrite() which guarantees atomicity. Is there a reason
> for that?

The code (before squashing anyway) used to use fprintf() directly
bypasing the outbuf for unusual cases, and we didn't want to mix raw
file descriptor access with stdio access.

That no longer is the case, so I think we can switch to xwrite(),
and lose the last paragraph of the log message entirely.

Thanks.

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Wed, 29 Jun 2016, Junio C Hamano wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > To make it clearer, here's a patch on top of pu that fixes all the 
> > issues I think are remaining. All tests pass now.
> 
> Lukas, can you see what is in 'pu' after I push out today's
> integration result in several hours and tell us if you like the
> result of the SQUASH??? change?

Here's a patch on top of it providing small optimizations.

----- >8
Subject: sideband.c: small optimization of strbuf usage

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>

diff --git a/sideband.c b/sideband.c
index 3cf3ced..b7e196b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -68,12 +68,12 @@ int recv_sideband(const char *me, int in_stream, int out)
 				int linelen = brk - b;
 
 				if (!outbuf.len)
-					strbuf_addf(&outbuf, "%s", PREFIX);
+					strbuf_addstr(&outbuf, PREFIX);
 				if (linelen > 0) {
 					strbuf_addf(&outbuf, "%.*s%s%c",
 						    linelen, b, suffix, *brk);
 				} else {
-					strbuf_addf(&outbuf, "%c", *brk);
+					strbuf_addch(&outbuf, *brk);
 				}
 				xwrite(2, outbuf.buf, outbuf.len);
 				strbuf_reset(&outbuf);
@@ -97,7 +97,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 	}
 
 	if (outbuf.len) {
-		strbuf_addf(&outbuf, "\n");
+		strbuf_addch(&outbuf, "\n");
 		xwrite(2, outbuf.buf, outbuf.len);
 	}
 	strbuf_release(&outbuf);

Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> Here's a patch on top of it providing small optimizations.

Thanks; will apply with a miniscule fix.

> ----- >8
> Subject: sideband.c: small optimization of strbuf usage
>
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> ...
> @@ -97,7 +97,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  	}
>  
>  	if (outbuf.len) {
> -		strbuf_addf(&outbuf, "\n");
> +		strbuf_addch(&outbuf, "\n");

'\n', or "\n"[0] ;-)

>  		xwrite(2, outbuf.buf, outbuf.len);
>  	}
>  	strbuf_release(&outbuf);

Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre]

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> Thanks; will apply with a miniscule fix.
> 
> > ----- >8
> > Subject: sideband.c: small optimization of strbuf usage
> >
> > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> > ...
> > @@ -97,7 +97,7 @@ int recv_sideband(const char *me, int in_stream, int out)
> >  	}
> >  
> >  	if (outbuf.len) {
> > -		strbuf_addf(&outbuf, "\n");
> > +		strbuf_addch(&outbuf, "\n");
> 
> '\n', or "\n"[0] ;-)

Obviously.


Nicolas