Fix Windows backslash paths in export and import - #22143
Conversation
|
@copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent and well-covered with targeted tests and release notes; remaining feedback is limited to minor documentation/comment fixes.
Pull request overview
This PR fixes Windows path handling for variable-expanded export/import patterns by introducing a path-specific expansion entry point that preserves directory separators on Windows, and applying it to code paths where the expanded string is used as a filesystem path. It also hardens the variable expander against a truncated substitution pattern that previously caused an out-of-bounds read, and adds coverage for the new behavior.
Changes:
- Add
dt_variables_expand_path()to normalize Windows\separators to/prior to variable expansion, while preserving\/escaping used inside substitutions. - Use
dt_variables_expand_path()for filesystem-path expansions across storage backends, neural restore output directories, and import sessions. - Fix an out-of-bounds read in substitution parsing for truncated inputs and extend unit tests + release notes accordingly.
File summaries
| File | Description |
|---|---|
| src/common/variables.c | Adds path-normalizing expansion wrapper and fixes truncated substitution parsing. |
| src/common/variables.h | Declares the new API with documentation. |
| src/imageio/storage/disk.c | Uses path-safe variable expansion for disk export target paths. |
| src/imageio/storage/gallery.c | Uses path-safe variable expansion for gallery export paths. |
| src/imageio/storage/latex.c | Uses path-safe variable expansion for LaTeX export paths. |
| src/libs/neural_restore.c | Uses path-safe variable expansion for neural restore output directory selection. |
| src/common/import_session.c | Uses path-safe variable expansion for session directories and improves Windows separator splitting. |
| src/tests/variables.c | Adds dt_variables_expand_path() test coverage alongside existing expander tests. |
| RELEASE_NOTES.md | Documents the Windows path fix and the truncated-substitution fix. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
import_session.c repeated "the". The doc comment on dt_variables_expand_path() wrote Windows paths with single backslashes, which doxygen reads as commands, so double them.
86bd508 to
8fb113e
Compare
On Windows
\is both the path separator and the escape character in variable expansion, soD:\photos\$(FILE_NAME)loses both: the file lands inD:\with a literal$(FILE_NAME)in its name.Adds
dt_variables_expand_path(), which normalizes separators to/before expanding, and uses it wherever the result is a filesystem path: export to disk, gallery, LaTeX, neural restore, and import sessions. Display text and bare filenames keepdt_variables_expand()and its escaping. Pass-through on other platforms.The second commit fixes a pre-existing out-of-bounds read in the same expander, found while testing this one:
$(var/patternwith no closing delimiter walked past the terminator, and the tests covering it only passed about nine times in ten.One behaviour change on Windows:
\$no longer escapes, because there it is the separator before a variable and the string alone cannot say which was meant.\/is left alone, so escaping a literal/inside$(VAR/pattern/replacement)still works.Written with AI assistance.
Fixes #20981
Replaces #21245, which fixed three storage modules but not import sessions, and would have dropped
\escaping on Windows.