fix(zapio): add carriage return support in Writer by lyydsheep · Pull Request #1536 · uber-go/zap

lyydsheep · 2026-03-13T05:33:34Z

Summary

Previously the Writer only split log messages on newline (\n) boundaries, causing carriage returns (\r) to be buffered silently. This is problematic when logging output from tools like git clone or progress bars that use \r to overwrite lines in terminal output.

Changes

Updated writeLine to detect both \n and \r as line separators
Added special handling for \r\n (Windows line endings) — treated as a single separator
Added 7 new test cases covering: bare \r, \r\n, mixed separators, buffered content, and git clone progress simulation

Test

go test ./zapio/... -v -run TestWriter

All new test cases pass.

Signed-off-by: lyydsheep 2230561977@qq.com

CLAassistant · 2026-03-13T05:33:44Z

All committers have signed the CLA.

lyydsheep · 2026-03-13T06:15:52Z

recheck⁠

codecov · 2026-03-13T14:59:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.48%. Comparing base (d278c59) to head (369af35).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1536      +/-   ##
==========================================
- Coverage   98.54%   98.48%   -0.06%     
==========================================
  Files          53       53              
  Lines        3033     3045      +12     
==========================================
+ Hits         2989     2999      +10     
  Misses         35       35              
- Partials        9       11       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Previously the Writer only split log messages on newline (\n) boundaries, causing carriage returns (\r) to be buffered silently. This is problematic when logging output from tools like git clone or progress bars that use \r to overwrite lines in terminal output. This commit updates writeLine to also split on \r, with special handling for \r\n (Windows line endings) treating them as a single separator. Fixes uber-go#1055 Signed-off-by: lyydsheep <2230561977@qq.com>

Fixes build/lint failure caused by unused variable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: lyydsheep <2230561977@qq.com>

Move the Windows line ending comment to be above the conditional logic it describes, improving code clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Change inline parameter comments from /* allowEmpty */ to // allowEmpty for consistency with Go coding conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sywhang · 2026-03-14T13:08:20Z

zapio/writer.go

 	// because we don't want an extraneous empty message at the end of the
 	// stream -- it's common for files to end with a newline.
-	w.flush(false /* allowEmpty */)
+	w.flush(false) // allowEmpty


please don't change this

sywhang · 2026-03-14T13:08:41Z

zapio/writer.go

+	// For carriage returns (progress updates), we also log the complete line.
+	w.flush(true) // allowEmpty


this comment isn't exactly relevant here, please remove

sywhang · 2026-03-14T13:09:13Z

zapio/writer.go

 	if w.buff.Len() == 0 {
 		w.log(line)
-		return
+		return remaining


explicit return here isn't necessary here

sywhang · 2026-03-14T13:52:37Z

zapio/writer.go

+// It handles both newlines (\n) and carriage returns (\r). The key logic:
+// - Look for the first newline (\n) or carriage return (\r)
+// - If \r\n is found (Windows line endings), treat it as a single separator
+// - If \r is found alone (progress updates), flush the current line and continue


I believe this isn't what we wanted -- please refer to the original issue. We don't want to flush on \r alone.

sywhang · 2026-03-14T13:53:02Z

zapio/writer_test.go

+		{
+			desc: "git clone progress simulation",
+			writes: []string{
+				"remote: Counting objects: 10%, done.\r",


I think this test case illustrates this is exactly what we didn't want in the original issue.

- Simplify comments and remove redundant explanations - Clean up test cases to focus on core functionality - Maintain carriage return support for progress-style output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

When the buffer is empty (fast path), writeLine was using bare return instead of returning remaining, causing data loss when processing multi-byte writes with separators. Fixes the regression introduced during review feedback cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Based on reviewer feedback, the carriage return (\r) handling has been corrected: - Standalone \r now discards any buffered content without logging, matching the intent of issue uber-go#1055. - This prevents logging duplicate lines for progress bars (e.g., git clone progress). - Only \n and \r\n sequences trigger actual log messages. - \r\n is correctly handled as a single separator (Windows line ending). Test cases have been updated to reflect the correct behavior: - "carriage return_clears_buffer" tests buffer discard without logging - "progress-style_output" now tests only the final message is logged - "carriage_return_with_buffered_content" tests buffer discard correctly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…turns The condition `nlIdx <= crIdx` incorrectly evaluated when nlIdx = -1 (no newline exists), causing the code to fail to identify standalone carriage returns as separators. This restructured the logic to handle all cases explicitly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update the Write method's documentation to accurately describe how it handles different line separators (\n, \r\n) and standalone carriage returns for progress bar output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update comment wording in writeLine to accurately describe the separator detection conditions: - "Only \n exists" → "Contains \n but not \r" - "Only \r exists" → "Contains \r but not \n (standalone carriage return)" - "comes before" → "occurs before" (grammatically clearer) The code logic remains unchanged - only comments have been updated to better reflect what each condition checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The separator detection logic previously used a catch-all else clause that would incorrectly handle the (theoretical) case where nlIdx == crIdx. While this cannot happen in practice since we search for different characters, making the logic explicit improves code clarity and defensive programming. Split the else clause into explicit conditions: - nlIdx < crIdx: \n occurs before \r - nlIdx > crIdx: \r occurs before \n (check for \r\n) - nlIdx == crIdx: handle gracefully (treat as \n) This makes the control flow more readable and eliminates any ambiguity about what happens in edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fast path should only log when we have content (len(line) > 0) to avoid logging empty messages. Buffer and flush only when either buffer or line has content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Addresses reviewer feedback for consecutive newline handling in PR uber-go#1536. Signed-off-by: lyydsheep <2230561977@qq.com>

Change the initial assignment of sepIdx from `sepIdx := -1` to `var sepIdx int` to avoid the ineffassign linter error. The ineffassign linter was reporting this as an ineffectual assignment because the logic ensures sepIdx is always assigned before use in all code paths, but the linter couldn't fully understand the complex control flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Add newline at end of CR test input to trigger日志输出 - Improve documentation of line separator handling in writeLine Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Simplify the separator detection algorithm by separating index finding from type determination - Improve code readability with clearer separation of concerns - Maintain same functionality while reducing complexity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Change `var sepIdx int = -1` to `sepIdx := -1` to match the short variable declaration style used for `sepLen` and `crOnly` on the following lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>