From e2363a76280431c807019080d0d76170de75e5a5 Mon Sep 17 00:00:00 2001 From: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Thu, 12 Jan 2023 23:41:46 +0100 Subject: [PATCH 1/8] check for empty log lines in prefixedWriter --- internal/batches/ui/interval_writer.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index 1cc285446f..f909b35d12 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -119,6 +119,12 @@ type prefixedWriter struct { func (w *prefixedWriter) Write(p []byte) (int, error) { var prefixedLines []byte for _, line := range bytes.Split(p, []byte("\n")) { + // We run into a weird situation where the output from a docker container + // contains an extra new line with spaces. This results in n*2 times the log + // output expected from an execution. We use this check to remove such blank lines + if len(bytes.TrimSpace(line)) == 0 { + continue + } prefixedLine := append([]byte(w.prefix), line...) prefixedLine = append(prefixedLine, []byte("\n")...) From c8e95183eec3c7e69e045960c6299902e64bf0d8 Mon Sep 17 00:00:00 2001 From: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 00:12:33 +0100 Subject: [PATCH 2/8] add tests --- internal/batches/ui/interval_writer.go | 12 +++++++++--- internal/batches/ui/interval_writer_test.go | 7 ++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index f909b35d12..37dadef30d 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -3,6 +3,7 @@ package ui import ( "bytes" "context" + "fmt" "io" "time" @@ -118,10 +119,15 @@ type prefixedWriter struct { func (w *prefixedWriter) Write(p []byte) (int, error) { var prefixedLines []byte + fmt.Println("=====================") + fmt.Println("'", string(p), "'") + fmt.Println("=====================") for _, line := range bytes.Split(p, []byte("\n")) { - // We run into a weird situation where the output from a docker container - // contains an extra new line with spaces. This results in n*2 times the log - // output expected from an execution. We use this check to remove such blank lines + // When we split a byte slice on every new line, we get atleast two sub-slice. + // I'll use a string to demonstrate, if I have a string "Peppermint\n", splitting + // this string on a new line will return a slice with two items: ["Peppermint", ""]. + // Using this logic, we can then skip empty lines from being added to the log output. + // SideNote: This means when one logs an actual empty line, we'll also discard that. if len(bytes.TrimSpace(line)) == 0 { continue } diff --git a/internal/batches/ui/interval_writer_test.go b/internal/batches/ui/interval_writer_test.go index 1a56eb4288..360cd9ee62 100644 --- a/internal/batches/ui/interval_writer_test.go +++ b/internal/batches/ui/interval_writer_test.go @@ -50,6 +50,10 @@ func TestIntervalWriter(t *testing.T) { stderrWriter.Write([]byte("4")) stdoutWriter.Write([]byte("5")) stderrWriter.Write([]byte("5")) + stdoutWriter.Write([]byte(`Hello world: 772 + `)) + stderrWriter.Write([]byte(`Hello world: 772 + `)) select { case <-ch: @@ -65,7 +69,8 @@ func TestIntervalWriter(t *testing.T) { want := "stdout: 2\nstderr: 2\n" + "stdout: 3\nstderr: 3\n" + "stdout: 4\nstderr: 4\n" + - "stdout: 5\nstderr: 5\n" + "stdout: 5\nstderr: 5\n" + + "stdout: Hello world: 772\nstderr: Hello world: 772\n" if d != want { t.Fatalf("wrong data in sink. want") From 6b36b5a65bb4c15284c120cee557d659f8ea257b Mon Sep 17 00:00:00 2001 From: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 00:18:52 +0100 Subject: [PATCH 3/8] remove debug statements --- internal/batches/ui/interval_writer.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index 37dadef30d..8d0c70e3bc 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -3,7 +3,6 @@ package ui import ( "bytes" "context" - "fmt" "io" "time" @@ -119,9 +118,6 @@ type prefixedWriter struct { func (w *prefixedWriter) Write(p []byte) (int, error) { var prefixedLines []byte - fmt.Println("=====================") - fmt.Println("'", string(p), "'") - fmt.Println("=====================") for _, line := range bytes.Split(p, []byte("\n")) { // When we split a byte slice on every new line, we get atleast two sub-slice. // I'll use a string to demonstrate, if I have a string "Peppermint\n", splitting From 54adfedfa8f2415e11db3931b4e85d5d3ed6699e Mon Sep 17 00:00:00 2001 From: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 00:22:28 +0100 Subject: [PATCH 4/8] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5811752909..ab983e8299 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ All notable changes to `src-cli` are documented in this file. - Fix network timeout in `src users clean` occuring in instances with many users [#901](https://github.com/sourcegraph/src-cli/pull/901) - Aligned parsing of spec file parameter of `src batch repos` with other commands. [#919](https://github.com/sourcegraph/src-cli/pull/919) +- batches: Remove empty log outputs. [#923](https://github.com/sourcegraph/src-cli/pull/923) ### Removed From 20ef19ced3ae2568be5794e64fbedc5ad3c37017 Mon Sep 17 00:00:00 2001 From: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 01:16:41 +0100 Subject: [PATCH 5/8] refactor --- internal/batches/ui/interval_writer.go | 26 ++++++++------------- internal/batches/ui/interval_writer_test.go | 12 +++++----- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index 8d0c70e3bc..b4c9917f4a 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -116,23 +116,17 @@ type prefixedWriter struct { prefix string } -func (w *prefixedWriter) Write(p []byte) (int, error) { - var prefixedLines []byte - for _, line := range bytes.Split(p, []byte("\n")) { - // When we split a byte slice on every new line, we get atleast two sub-slice. - // I'll use a string to demonstrate, if I have a string "Peppermint\n", splitting - // this string on a new line will return a slice with two items: ["Peppermint", ""]. - // Using this logic, we can then skip empty lines from being added to the log output. - // SideNote: This means when one logs an actual empty line, we'll also discard that. - if len(bytes.TrimSpace(line)) == 0 { - continue - } - prefixedLine := append([]byte(w.prefix), line...) - prefixedLine = append(prefixedLine, []byte("\n")...) +var newLineByteSlice = []byte("\n") - prefixedLines = append(prefixedLines, prefixedLine...) - } - w.writes <- prefixedLines +func (w *prefixedWriter) Write(p []byte) (int, error) { + var prefixedLine []byte + // We remove new lines appended to the log output so all of the stream can have + // only one new line separating them. This ensures the previous behavior for the log + // structure is preserved. + p = bytes.TrimRight(p, "\n") + prefixedLine = append([]byte(w.prefix), p...) + prefixedLine = append(prefixedLine, newLineByteSlice...) + w.writes <- prefixedLine <-w.writesDone return len(p), nil } diff --git a/internal/batches/ui/interval_writer_test.go b/internal/batches/ui/interval_writer_test.go index 360cd9ee62..043aaf4cc7 100644 --- a/internal/batches/ui/interval_writer_test.go +++ b/internal/batches/ui/interval_writer_test.go @@ -50,10 +50,10 @@ func TestIntervalWriter(t *testing.T) { stderrWriter.Write([]byte("4")) stdoutWriter.Write([]byte("5")) stderrWriter.Write([]byte("5")) - stdoutWriter.Write([]byte(`Hello world: 772 - `)) - stderrWriter.Write([]byte(`Hello world: 772 - `)) + stdoutWriter.Write([]byte(`Hello world: 1 +`)) + stderrWriter.Write([]byte(`Hello world: 1 +`)) select { case <-ch: @@ -70,10 +70,10 @@ func TestIntervalWriter(t *testing.T) { "stdout: 3\nstderr: 3\n" + "stdout: 4\nstderr: 4\n" + "stdout: 5\nstderr: 5\n" + - "stdout: Hello world: 772\nstderr: Hello world: 772\n" + "stdout: Hello world: 1\nstderr: Hello world: 1\n" if d != want { - t.Fatalf("wrong data in sink. want") + t.Fatalf("wrong data in sink. want=%q, have=%q", want, d) } case <-time.After(1 * time.Second): t.Fatalf("ch has NO data") From 48af502a483ed8e7b032278aaa9d486e086acec1 Mon Sep 17 00:00:00 2001 From: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 13:46:47 +0100 Subject: [PATCH 6/8] Update internal/batches/ui/interval_writer.go Co-authored-by: Erik Seliger --- internal/batches/ui/interval_writer.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index b4c9917f4a..a304d63da0 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -118,6 +118,9 @@ type prefixedWriter struct { var newLineByteSlice = []byte("\n") +// Write is only ever called with a single line. That line may or may not end with a newline character. +// It then writes the content as a single line to the inner writer, regardless of if the provided line +// had a newline or not. That is, because our encoding requires exactly one line per formatted line. func (w *prefixedWriter) Write(p []byte) (int, error) { var prefixedLine []byte // We remove new lines appended to the log output so all of the stream can have From 690cb7ebcb3065c25c04e8b65995ebe1072d45aa Mon Sep 17 00:00:00 2001 From: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 13:47:15 +0100 Subject: [PATCH 7/8] Update internal/batches/ui/interval_writer.go Co-authored-by: Erik Seliger --- internal/batches/ui/interval_writer.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index a304d63da0..68a7817c7f 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -122,13 +122,10 @@ var newLineByteSlice = []byte("\n") // It then writes the content as a single line to the inner writer, regardless of if the provided line // had a newline or not. That is, because our encoding requires exactly one line per formatted line. func (w *prefixedWriter) Write(p []byte) (int, error) { - var prefixedLine []byte - // We remove new lines appended to the log output so all of the stream can have - // only one new line separating them. This ensures the previous behavior for the log - // structure is preserved. - p = bytes.TrimRight(p, "\n") - prefixedLine = append([]byte(w.prefix), p...) - prefixedLine = append(prefixedLine, newLineByteSlice...) + prefixedLine := append([]byte(w.prefix), p...) + if prefixedLine[len(prefixedLine)-1] != newLineByteSlice { + prefixedLine = append(prefixedLine, newLineByteSlice...) + } w.writes <- prefixedLine <-w.writesDone return len(p), nil From d5413ab035607dc1f561be8ff4666c5918db230b Mon Sep 17 00:00:00 2001 From: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Fri, 13 Jan 2023 14:04:44 +0100 Subject: [PATCH 8/8] only append a new line if the byte slice doesn't have trailing new line --- internal/batches/ui/interval_writer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/batches/ui/interval_writer.go b/internal/batches/ui/interval_writer.go index 68a7817c7f..75fafbe931 100644 --- a/internal/batches/ui/interval_writer.go +++ b/internal/batches/ui/interval_writer.go @@ -123,9 +123,9 @@ var newLineByteSlice = []byte("\n") // had a newline or not. That is, because our encoding requires exactly one line per formatted line. func (w *prefixedWriter) Write(p []byte) (int, error) { prefixedLine := append([]byte(w.prefix), p...) - if prefixedLine[len(prefixedLine)-1] != newLineByteSlice { + if !bytes.HasSuffix(prefixedLine, newLineByteSlice) { prefixedLine = append(prefixedLine, newLineByteSlice...) - } + } w.writes <- prefixedLine <-w.writesDone return len(p), nil