devcontainer: Fix git output#49230
Conversation
511a62c to
debebd2
Compare
|
Updated description with manual testing steps. Ready for review |
| impl RpcError { | ||
| /// Returns the raw server-provided error message without any RPC framing | ||
| /// (e.g. without the "RPC request X failed: " prefix that `Display` adds). | ||
| pub fn raw_message(&self) -> &str { |
There was a problem hiding this comment.
It looks like RpcError already implements Display, which surfaces the message - is this additional access path needed?
There was a problem hiding this comment.
Hi @KyleBarton, it does indeed, this just strips the RPC-specific part of the message so the log shows focused git errors
impl std::fmt::Display for RpcError {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
if let Some(request) = &self.request {
write!(f, "RPC request {} failed: {}", request, self.msg)?
} else {
write!(f, "{}", self.msg)?
}
for tag in &self.tags {
write!(f, " {}", tag)?
}
Ok(())
}
}
(I bet you know this by heart - I'm just posting it here for my own clarity)
There was a problem hiding this comment.
Right, so the Display implementation will provide the underlying message if we let it; it will just also include the extra information about the RPC request. Since this is showing up in log buffer, is the extra information bad? Put another way: can we solve this by simply removing the context() calls in git_store?
There was a problem hiding this comment.
We can, and that was actually my first solution, but the log felt noisy to me. If I try to git pull and it fails, I want to see just the git error - not some transport detail. The transport itself didn't fail, right? I generally prefer things clean and focused unless the extra information is helpful in some way.
Maybe there's a general UX question here about what the user experience should be like when in devcontainers? Should it be transparent once you're in them, or should it be explicit that things are happening through RPC to remind the user that they're in a container at all times?
Then again, I haven't used devcontainers in my daily work enough to have a strong opinion, so there might be ramifications here I'm not aware of.
Your call
There was a problem hiding this comment.
It makes sense. I sought some additional opinions about the trade-off of extra code vs extra verbose logging, and they liked your approach. I'm convinced! Let's proceed
| impl RpcError { | ||
| /// Returns the raw server-provided error message without any RPC framing | ||
| /// (e.g. without the "RPC request X failed: " prefix that `Display` adds). | ||
| pub fn raw_message(&self) -> &str { |
There was a problem hiding this comment.
It makes sense. I sought some additional opinions about the trade-off of extra code vs extra verbose logging, and they liked your approach. I'm convinced! Let's proceed
|
Thanks for the contribution! |
|
Awesome thanks! And @oliverbarnes has another open PR that is very imporant if you have time to review @KyleBarton. #48896 |
Closes zed-industries#48434 In Dev Containers, failed git operations were surfaced with a generic failure message, while the useful git output (stderr/stdout) was not reliably available to users. This happened because in devcontainers the git operation errors go through an RPC layer and then got wrapped with `anyhow::Context` (e.g. “sending pull request”); the toast displayed only that outer context via `to_string()`, masking the underlying git stderr message. This change ensures the full git operation output is preserved and surfaced via Zed’s “See logs” flow in Dev Containers, matching the information you get when running the same git command in a terminal. ### What you should expect in the UI - You will see a generic toast like “git pull failed” / “git push failed”. - When clicking on the toast’s “See logs”, the log tab now contains the full git error output (e.g. non-fast-forward hints, merge conflict details, “local changes would be overwritten”, etc.), which previously could be missing/too generic. --- ## Manual testing Run inside a Dev Container and ensure git auth works (SSH keys/agent or HTTPS credentials). 1. **Dirty-tree pull failure** - Make remote ahead by 1 commit (push from another clone). - Locally modify the same file without committing. - In Zed: **Pull** - **Expect:** toast “git pull failed” + **See logs** shows “local changes would be overwritten…” (or equivalent). 2. **Non-fast-forward push failure** - Ensure remote ahead. - Locally create 1 commit. - In Zed: **Push** - **Expect:** toast “git push failed” + **See logs** shows “rejected (non-fast-forward)” + hint to pull first. 3. **Merge-conflict pull failure** - Create conflicting commits on the same lines (one local commit, one remote commit). - In Zed: **Pull** - **Expect:** toast “git pull failed” + **See logs** shows conflict output (“CONFLICT…”, “Automatic merge failed…”). Release Notes: - Fixed devcontainer git failure toasts so they show the actual git error --------- Co-authored-by: KyleBarton <kjb@initialcapacity.io>
Closes #48434
In Dev Containers, failed git operations were surfaced with a generic failure message, while the useful git output (stderr/stdout) was not reliably available to users.
This happened because in devcontainers the git operation errors go through an RPC layer and then got wrapped with
anyhow::Context(e.g. “sending pull request”); the toast displayed only that outer context viato_string(), masking the underlying git stderr message.This change ensures the full git operation output is preserved and surfaced via Zed’s “See logs” flow in Dev Containers, matching the information you get when running the same git command in a terminal.
What you should expect in the UI
Manual testing
Run inside a Dev Container and ensure git auth works (SSH keys/agent or HTTPS credentials).
Dirty-tree pull failure
Non-fast-forward push failure
Merge-conflict pull failure
Release Notes: