fix(core): escape backslashes and quotes in emacs ediff paths#5630
fix(core): escape backslashes and quotes in emacs ediff paths#5630wenshao merged 2 commits intoQwenLM:mainQwenLM/qwen-code:mainfrom he-yufeng:fix/emacs-diff-path-escapinghe-yufeng/qwen-code:fix/emacs-diff-path-escapingCopy head branch name to clipboard
Conversation
The emacs diff command interpolates the two file paths into an Elisp string literal inside the (ediff ...) form. Backslashes and double quotes are both string metacharacters in Elisp, so an unescaped path corrupts the form: a double quote ends the string early, and a backslash is consumed as an escape that drops the next character. The diff paths come from createTempFilesForModify, whose temp file names embed the basename of the file being edited. On Windows the temp path itself uses backslash separators, so every Windows emacs diff is affected; on Unix any edited file whose name contains a double quote breaks the form too. Escape backslashes first, then double quotes, before interpolating. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
| }); | ||
| }); | ||
|
|
||
| it('should escape backslashes and quotes in emacs paths', () => { |
There was a problem hiding this comment.
[Suggestion] The combined test input (C:\tmp\a"b.txt) exercises both escaping rules simultaneously. Consider adding separate tests for backslash-only and quote-only paths — when a future change breaks only one replacement, isolated tests produce clearer failure signals pointing directly at the broken rule.
it('should escape backslashes in emacs paths', () => {
const command = getDiffCommand('C:\\tmp\\old.txt', '/tmp/new.txt', 'emacs');
expect(command).toEqual({
command: 'emacs',
args: ['--eval', '(ediff "C:\\\\tmp\\\\old.txt" "/tmp/new.txt")'],
});
});
it('should escape double quotes in emacs paths', () => {
const command = getDiffCommand('/tmp/a"b.txt', '/tmp/new.txt', 'emacs');
expect(command).toEqual({
command: 'emacs',
args: ['--eval', '(ediff "/tmp/a\\"b.txt" "/tmp/new.txt")'],
});
});— qwen3.7-max via Qwen Code /review
| // form. A double quote ends the string early; a backslash is consumed | ||
| // as an escape, dropping the following path character. | ||
| const command = getDiffCommand( | ||
| 'C:\\tmp\\a"b.txt', |
There was a problem hiding this comment.
[Suggestion] newPath here is /tmp/new.txt with no metacharacters, so toElispString is a no-op on it. If someone accidentally removed the toElispString() call on newPath only, this test would still pass. Consider adding a case where newPath also contains a backslash or quote to verify both arguments are escaped:
it('should escape metacharacters in the new path for emacs', () => {
const command = getDiffCommand('/tmp/old.txt', 'C:\\tmp\\new.txt', 'emacs');
expect(command).toEqual({
command: 'emacs',
args: ['--eval', '(ediff "/tmp/old.txt" "C:\\\\tmp\\\\new.txt")'],
});
});— qwen3.7-max via Qwen Code /review
|
Heads up on the red CI: the only failing job is |
|
@qwen-code /triage |
|
Thanks for the PR, @he-yufeng! Template looks good ✓ On direction: this is a real bug — emacs users on Windows get broken ediff because temp paths contain backslashes that corrupt the Elisp string literal. File names with double quotes break it on Unix too. It's a narrow, well-defined fix squarely within qwen-code's editor integration support. Aligned. On approach: the scope is minimal — one escaping helper, two call sites, one test. Exactly the right size for the problem. Nothing to cut, nothing to add. Moving on to code review. 🔍 中文说明感谢贡献，@he-yufeng！ 模板完整 ✓ 方向：这是一个真实的 bug —— Windows 上的 emacs 用户在使用 ediff 时，因为临时路径中包含反斜杠导致 Elisp 字符串字面量被破坏。Unix 上文件名包含双引号时同样会出问题。修复范围明确，完全在 qwen-code 编辑器集成的职责范围内。对齐。 方案：范围极简 —— 一个转义辅助函数、两个调用点、一个测试。对于这个问题的体量来说恰到好处，无需增删。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: I'd add an inline escaping function that first doubles backslashes ( Diff comparison: the PR's approach matches exactly. Reuse check: no existing Elisp escaping utility in the repo. The two regex replaces are trivial enough that extracting a shared utility would be over-engineering. Correct call to keep it local. No blockers. Clean implementation. TestsEmacs is not installed in the Linux CI environment, so the e2e path (launching emacs ediff with a backslash-containing path) cannot be exercised here. The fix is at the string-construction layer and is fully covered by the unit test — the before/after of the Elisp form is deterministic and the test asserts the exact escaped output. Emacs-filtered run (new test + existing emacs tests)Full editor test suite (regression check)All 152 tests pass. No regressions. 中文说明代码审查独立方案： 我会添加一个内联转义函数，先将反斜杠加倍（ Diff 对比： PR 的方案与我的完全一致。 复用检查： 仓库中没有现成的 Elisp 转义工具。两个正则替换足够简单，抽取共享工具反而过度工程化。保持局部的决定正确。 无阻塞问题，实现干净。 测试Linux CI 环境未安装 emacs，无法进行端到端测试（用含反斜杠的路径启动 emacs ediff）。修复位于字符串构造层，单元测试已完全覆盖 —— Elisp 形式的前后对比是确定性的，测试断言了精确的转义输出。 全部 152 个测试通过，无回归。 — Qwen Code · qwen3.7-max |
|
This is a textbook bugfix. The problem is real and narrow: emacs ediff fails on Windows because backslash path separators corrupt an Elisp string literal, and on Unix when filenames contain double quotes. The fix — escape both metacharacters before interpolation, backslashes first — is the minimum code needed to solve the problem. My independent proposal was identical to what the PR ships. The test covers the exact failure mode (mixed backslashes and quotes in one path). All 152 existing tests still pass. The comment on the helper function explains the ordering constraint, which is the one non-obvious thing a future maintainer needs to know. If I had to maintain this in six months, I'd have no complaints. Approving. 中文说明这是一个教科书式的 bugfix。问题真实且范围明确：emacs ediff 在 Windows 上失败，因为反斜杠路径分隔符破坏了 Elisp 字符串字面量；在 Unix 上文件名包含双引号时同样出问题。修复方案 —— 在插值前转义两种元字符，先转义反斜杠 —— 是解决问题的最小代码量。 我的独立方案与 PR 提交的完全一致。测试覆盖了确切的失败模式（单一路径中混合反斜杠和引号）。全部 152 个现有测试仍然通过。辅助函数上的注释解释了顺序约束，这是未来维护者唯一需要知道的不明显之处。 如果六个月后我需要维护这段代码，我没有任何抱怨。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Escapes backslashes and double quotes in the two file paths before they are interpolated into the
(ediff "..." "...")form that the emacs diff integration passes toemacs --eval.Why it's needed
The emacs case in
getDiffCommandbuilds an Elisp string literal by interpolating the old and new paths directly. Backslashes and double quotes are both metacharacters inside an Elisp string, so an unescaped path corrupts the form: a double quote terminates the string early, and a backslash is consumed as an escape that swallows the next character.These paths are not arbitrary user input, but they are also not always quote/backslash-free. They come from
createTempFilesForModify, whose temp file names embed the basename of the file being edited. On Windows the temp path itself uses backslash separators (C:\Users\...\qwen-code-modify-...), so the literal becomes(ediff "C:\Users\...")and emacs drops the separators, pointing ediff at the wrong path. On Unix, editing any file whose name contains a double quote produces the same kind of broken form. In both cases the diff fails to open instead of showing the proposed change.The other editors are unaffected: they receive the paths as separate argv elements (no string literal to break).
Reviewer Test Plan
How to verify
getDiffCommandis a pure function, so the behavior is covered by a unit test that feeds a path containing both a backslash and a double quote and asserts the emitted Elisp string is escaped.Expected: the new test
should escape backslashes and quotes in emacs pathspasses, and the existing emacs test still passes. Before the fix the new test fails with the unescaped form(ediff "C:\tmp\a"b.txt" ...).Evidence (Before & After)
N/A (non-user-visible string construction; covered by unit test output above).
Tested on
Verified locally on Windows via
vitest run packages/core/src/utils/editor.test.ts(152 passed).Environment (optional)
Unit tests only.
Risk & Scope