Repository navigation
fix(git): pass \r through unchanged so clone progress renders correctly - #79
Conversation
📝 WalkthroughWalkthroughRefined Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/git.rs (1)
50-55:⚠️ Potential issue | 🟡 MinorPreserve raw stderr bytes instead of formatting
chars.
eprint!("{}", b as char)only preserves ASCII. Any UTF-8 coming from git stderr (repo paths, localized messages) will be garbled, so the stream is still not being passed through unchanged. Write the original bytes withWrite::write_allinstead.Minimal fix
- use std::io::{BufReader, Read}; + use std::io::{self, BufReader, Read, Write}; let mut reader = BufReader::new(stderr); + let stderr_out = io::stderr(); + let mut out = stderr_out.lock(); let mut buf = [0u8; 256]; let mut at_line_start = true; loop { let n = reader.read(&mut buf)?; if n == 0 { @@ for &b in &buf[..n] { if at_line_start { - eprint!(" "); + out.write_all(b" ")?; at_line_start = false; } - eprint!("{}", b as char); + out.write_all(&[b])?; if b == b'\n' { at_line_start = true; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/git.rs` around lines 50 - 55, The stderr printing currently converts each byte to a char with eprint!("{}", b as char) which garbles non-ASCII UTF-8; change the logic in the same block that uses at_line_start and the byte variable b to write the raw byte(s) to stderr using a Write implementation (e.g., stderr().write_all(&[b])) so the original bytes are preserved; ensure std::io::Write is in scope and handle any potential write errors the same way other errors are handled in this function.
🧹 Nitpick comments (1)
src/git.rs (1)
445-496: Unit-test the shared formatter instead of duplicating the loop.
test_clone_remote_progress_visibleonly proves cloning succeeded, andtest_indent_after_newline_not_carriage_returnreimplements the current loop instead of calling it. A future regression inclone_remotecan therefore pass both tests. Consider extracting the stderr-formatting loop into a smallRead -> Writehelper and testing that helper directly.Also applies to: 498-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/git.rs` around lines 445 - 496, The tests duplicate the stderr-formatting loop instead of exercising it, so extract the loop in clone_remote into a small helper (e.g., format_clone_progress<R: Read, W: Write> or write_stderr_progress) that takes a Read input and Write output and performs the newline/carriage-return handling currently duplicated in test_indent_after_newline_not_carriage_return; update clone_remote to call that helper, then add a focused unit test for the helper that feeds crafted stderr bytes and verifies the formatted output instead of reimplementing the loop in tests like test_clone_remote_progress_visible and test_indent_after_newline_not_carriage_return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/git.rs`:
- Around line 50-55: The stderr printing currently converts each byte to a char
with eprint!("{}", b as char) which garbles non-ASCII UTF-8; change the logic in
the same block that uses at_line_start and the byte variable b to write the raw
byte(s) to stderr using a Write implementation (e.g., stderr().write_all(&[b]))
so the original bytes are preserved; ensure std::io::Write is in scope and
handle any potential write errors the same way other errors are handled in this
function.
---
Nitpick comments:
In `@src/git.rs`:
- Around line 445-496: The tests duplicate the stderr-formatting loop instead of
exercising it, so extract the loop in clone_remote into a small helper (e.g.,
format_clone_progress<R: Read, W: Write> or write_stderr_progress) that takes a
Read input and Write output and performs the newline/carriage-return handling
currently duplicated in test_indent_after_newline_not_carriage_return; update
clone_remote to call that helper, then add a focused unit test for the helper
that feeds crafted stderr bytes and verifies the formatted output instead of
reimplementing the loop in tests like test_clone_remote_progress_visible and
test_indent_after_newline_not_carriage_return.
|
Here is evidence of the fix working — |
git uses bare \r (carriage return without \n) to overwrite the current terminal line in-place for progress output like "Receiving objects: 45%". The previous code set at_line_start=true after every \r, causing a 3-space indent prefix to be injected immediately after the \r — before the next progress text arrived. This placed spaces between the cursor-reset and the overwrite text, garbling the display. Fix: only treat \n as a line boundary. \r is now passed through untouched, so git's in-place progress renders correctly for both SSH and HTTPS remotes. Adds two tests: - test_clone_remote_progress_visible: end-to-end clone via local bare repo - test_indent_after_newline_not_carriage_return: unit-tests the byte loop
d6d6844 to
717b209
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/git.rs (1)
453-486: Consider checking git command exit status for better diagnostics.The git commands don't explicitly verify success. While failures would ultimately cause the final assertions to fail, explicit checks would make test failures easier to diagnose.
🔧 Optional: Add success checks for easier debugging
let src = tmp.path().join("source.git"); - std::process::Command::new("git") + let output = std::process::Command::new("git") .args(["init", "--bare", src.to_str().unwrap()]) .output() .unwrap(); + assert!(output.status.success(), "git init --bare failed");Apply similar pattern to other git commands in the test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/git.rs` around lines 453 - 486, The git Command::new(...) invocations (the sequences that create the bare repo, clone into work, add/commit/push using variables src and work) do not verify exit status; change each .output().unwrap() to capture the output (let out = std::process::Command::new("git")... .output().expect(...)) and assert or panic if !out.status.success(), including out.stdout and out.stderr in the message so failures show git diagnostics (apply this pattern to the init, clone, add, commit, and push calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/git.rs`:
- Around line 453-486: The git Command::new(...) invocations (the sequences that
create the bare repo, clone into work, add/commit/push using variables src and
work) do not verify exit status; change each .output().unwrap() to capture the
output (let out = std::process::Command::new("git")... .output().expect(...))
and assert or panic if !out.status.success(), including out.stdout and
out.stderr in the message so failures show git diagnostics (apply this pattern
to the init, clone, add, commit, and push calls).
popomore
left a comment
popomore
left a comment
There was a problem hiding this comment.
Nice catch on the bare \r semantics — fix is minimal and correct, tests lock down the regression. 👍
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #79 +/- ##
==========================================
+ Coverage 91.85% 94.00% +2.14%
==========================================
Files 14 14
Lines 1658 1717 +59
==========================================
+ Hits 1523 1614 +91
+ Misses 135 103 -32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Problem
projj add <repo>shows no clone progress. The output fromgit clone --progressis piped through a byte loop that re-emits each byte with a 3-space indent prefix. However, the loop setat_line_start = trueafter every\rand\n.git uses bare
\r(carriage return without\n) to overwrite the current terminal line in-place for progress lines like:Because the old code triggered a new indent after each
\r, a" "prefix was injected immediately after the\r— before the next progress text. This placed three spaces between the cursor-reset and the overwrite text, garbling the display and making progress appear blank.Fix
Only treat
\nas a line boundary.\ris now passed through untouched, so git's in-place progress renders correctly. This works for both SSH and HTTPS remotes since the byte-streaming logic is transport-agnostic.One-line diff in
src/git.rs:Tests
Two new unit tests added to
src/git.rs:test_clone_remote_progress_visible— end-to-end clone via a local bare repo, verifies the cloned file is present afterclone_remotereturnstest_indent_after_newline_not_carriage_return— directly exercises the byte-loop logic, asserting that\rdoes not inject an indent prefix and\ndoesSummary by CodeRabbit
Bug Fixes
Tests