Skip to content

Preserve current string values in CopyToGoFlagSet - #520

Merged
tomasaschan merged 4 commits into
spf13:masterfrom
adenzhou1350:codex/fix-copy-go-string-value
Oct 19, 2026
Merged

tomasaschan merged 4 commits into
spf13:masterfrom
adenzhou1350:codex/fix-copy-go-string-value

Conversation

@adenzhou1350

@adenzhou1350 adenzhou1350 commented Oct 14, 2026 •

Copy link
Copy Markdown

CopyToGoFlagSet registers string flags using their original DefValue. Because flag.StringVar initializes the shared variable, copying a parsed string (including an explicit empty string) silently resets it. Use the current value, matching the other scalar branches. Related #330.

The regression test covers default, parsed and empty values, preserves the source default, and checks shared updates in both directions.

Initial validation: Linux full race suite passes on Go 1.26.9 (205 test/subtest results); native Windows focused cases pass. The parsed/empty regressions fail with unchanged production code on both platforms. Full Windows/race and the upstream Go-version/lint matrix were not run. An existing TestGoflags flag-redefinition failure when repeating the suite in one process occurs in both arms; fresh-process suites pass.

AI-assisted investigation and tests; the one-line production change and regression coverage were independently reviewed.

Latest hosted validation (2026-10-19, head cc9e68973190b3fa59c677ceeae4d171ec34f74a): CI passed all six Go test matrix entries (1.12, 1.21, 1.22, 1.23, oldstable, stable, using the workflow race suite) and lint. The maintainer has approved the PR.

Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@CLAassistant

CLAassistant commented Oct 14, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@adenzhou1350

Copy link
Copy Markdown
Author

@tomasaschan Could you approve the CI workflow when convenient? This is a one-line follow-up to #330; the string branch now preserves the current value like the other scalar branches. Thanks!

@tomasaschan

Copy link
Copy Markdown
Collaborator

@adenzhou1350 I was waiting for the CLA signature. This project gets enough automated slop these days that I don't really look at PRs until the CLA is signed - it's so far been a fairly trustworthy signal for whether there's actually someone who'll eg respond to feedback at the other end.

@adenzhou1350

Copy link
Copy Markdown
Author

Thanks for clarifying. I have flagged the pending CLA signature for the account holder, and understand that review is waiting on it. Separately, all six test jobs pass. The Lint job fails while importing Go 1.27.2 standard-library export data: golangci-lint 2.13.2 reports export data version 5 greater than its maximum supported version 4. This looks like a CI toolchain compatibility issue rather than evidence of a failure in the string-copy change: https://github.com/spf13/pflag/actions/runs/38585875801/job/115938404479?pr=520

@adenzhou1350

Copy link
Copy Markdown
Author

@tomasaschan CLAassistant now confirms that all committers have signed, and license/cla is green. The six Go test jobs are also green; the remaining lint/toolchain failure is detailed above. Could you take a look at the string-copy fix when convenient? Thanks.

@tomasaschan

Copy link
Copy Markdown
Collaborator

Looks like the linter might need a version bump, similar to #508

@adenzhou1350

Copy link
Copy Markdown
Author

Thanks, I followed the same approach in 7fbce12 and bumped the workflow from v2.13 to v2.14. On Windows with Go 1.27.2, the official 2.13.2 binary reproduces the export-data v5/v4 failure, while 2.14.0 passes with 0 issues on the same LF source snapshot. Config verification and go test ./... also pass. The change is limited to the linter version line; hosted CI still needs to confirm the new head.

Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@adenzhou1350
adenzhou1350 force-pushed the codex/fix-copy-go-string-value branch from 7fbce12 to a532d80 Compare October 15, 2026 03:53
@adenzhou1350

Copy link
Copy Markdown
Author

Corrected the CI commit’s placeholder author identity (same tree); CLA is green again. In 5079f6e I also fixed TestToGoflags to check the source set instead of itself. An injected extra flag now fails as intended, while the clean full Linux race suite still passes all 205 test/subtest results on Go 1.26.9. No further production change; hosted CI awaits approval.

@tomasaschan

Copy link
Copy Markdown
Collaborator

@adenzhou1350 There were so many PRs blocked on the linter that I decided to address it by bumping it to latest; this PR now has a conflict for the linter version. If you remove that, it should build and merge cleanly. Sorry for the toil!

Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@adenzhou1350

adenzhou1350 commented Oct 19, 2026 •

Copy link
Copy Markdown
Author

Thanks for fixing the shared linter configuration. I merged current master (70f284b) and removed the PR-specific linter version change; .github/workflows/ci.yaml now matches upstream exactly, including version: latest. The remaining PR diff is only the string-value fix and regression coverage.

Validation on the updated head: native Windows / Go 1.27.2 go test ./... passes; golangci-lint 2.14.0 config verification and linting of the staged LF source snapshot pass with 0 issues; git diff --check passes. This is local non-race validation; hosted CI and its Go-version matrix still need to run.

Update (2026-10-19): The CI workflow has now completed successfully, and GitHub reports the PR as mergeable.

@tomasaschan
tomasaschan merged commit fa584ba into spf13:master Oct 19, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants