Skip to content

Preserve current string values in CopyToGoFlagSet - #520

Open
adenzhou1350 wants to merge 2 commits into
spf13:masterfrom
adenzhou1350:codex/fix-copy-go-string-value
Open

adenzhou1350 wants to merge 2 commits into
spf13:masterfrom
adenzhou1350:codex/fix-copy-go-string-value

Conversation

@adenzhou1350

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.

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.

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

CLAassistant commented Oct 14, 2026 •

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ adenzhou1350
❌ aden


aden seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@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

Signed-off-by: aden <aden@example.com>
@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.

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