Repository navigation
internal/graph: escape source filenames in DOT labels - #1032
jabrailkhalil wants to merge 13 commits into
Conversation
Signed-off-by: jabrailkhalil <jabrailkhalil@gmail.com>
|
I consolidated the filename regressions into the existing compose-test table as requested and updated this branch from main. On current head |
|
Integrated current main The graph package tests and scoped go vet pass. The backslash-filename case retains its existing Windows skip; it was not executed as a Linux filesystem test. Fresh validation uses Windows Go 1.26.0, inherited from current main. The full Windows suite is not green: the three PE/DWARF mapping subcases also fail on clean current main in the same environment. The initial run additionally lacked the external diff executable; it is not treated as a source regression. Browser modules retain their x86-64 Linux guards and skip execution on Windows: compilation is not a new Linux/browser/race validation. No existing assertions, timing thresholds, platform guards or dependency versions were weakened. Current-head upstream CI and maintainer review are still needed. Could a maintainer approve the new-head workflow and review the integration? |
|
Validated unchanged head Both filename-escaping regressions actually execute and pass on Linux, including the literal-backslash case. With only the regression test/golden changes applied to main, both fail with the expected unescaped DOT-label differences; no production file was changed in the control. Main passed the same Linux commands. Module manifests, checksums and Update October 12: GitHub now reports this historical matrix job as completed/SUCCESS, consistent with its raw evidence. Current-head validation is linked in the updated PR description. Could a maintainer authorize the upstream workflow and review the current-head integration? |
|
@jabrailkhalil Having reviewed a few of your PRs, they require quite a few iterations and have substantial amount of AI generated traffic that appears to be unreviewed / unshaped by the author. Can I ask you to either curate the changes more to make them more concise and suitable in their size for their value, or to stop contributing? Related: Are you an active pprof user? I updated CONTRIBUTING.md a little to clarify the guidelines. In particular, we expect most contributions to be from active pprof users contributing fixes for the problems they encounter in their work. We are less interested in contributions that just go over the backlog to fix things: Some of those are lower priority and they are not worth working on (either for authors or reviewers) before there is enough user feedback to prioritize them. |
|
Thanks for the direct feedback. I have shortened the description and confined this revision to the requested test consolidation; the production fix is still one line. The new Linux head/main validation passes, including the negative control. These PRs were based on existing issues; I do not have a specific first-hand workflow report to add, and will not present them as one. I will keep further replies concise and will not start additional pprof backlog work without a maintainer go-ahead. |
SG - thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
+ Coverage 73.42% 73.43% +0.01%
==========================================
Files 46 46
Lines 7936 7936
==========================================
+ Hits 5827 5828 +1
+ Misses 2086 2085 -1
Partials 23 23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the review and approval. Current head is |
Source filenames containing double quotes can break quoted DOT node labels at file/line granularity. I apply the existing DOT escaping after taking the basename. The existing
TestComposeWithNamesThatNeedEscapingtable adds one quoted filename with a line/column suffix and updates its golden.TestEscapeForDotalready covers the helper's other supported characters; the production fix is one line.Addresses the filename portion of #1020; sample-tag escaping is separate (#1008).
Current head
37811fb215a47125a910dee0aef90928cbc7fefepasses upstream CI, including the Linux, macOS and Windows matrix, and the security checks. Earlier author-only validation remains linked in the discussion and is not presented as a run of this head.