Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: winsymlink and winreadlinkvolume missing IncNonDefault calls #66215

Closed
rsc opened this issue Mar 9, 2024 · 2 comments
Closed

os: winsymlink and winreadlinkvolume missing IncNonDefault calls #66215

rsc opened this issue Mar 9, 2024 · 2 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Mar 9, 2024

The new definitions of winsymlink and winreadlinkvolume godebugs in package os are missing calls to IncNonDefault to record when they change the behavior of the program. See go doc internal/godebug for details.

Note that to be a useful metric, IncNonDefault has to be called only when the result is actually different, not just when new code is executing that does the same thing old code would have done for a given input. That means checking the input and understanding when a deviation is happening.

This means that

if winsymlink.Value() == "0" {
	return fs.modePreGo1_23()
}

is not a viable implementation strategy on its own. However, one possibility is to rename the current Mode to mode and then do

func (fs *fileStat) Mode() FileMode {
	m := fs.mode()
	if winsymlink.Value() == "0" {
		old := fs.modePreGo1_23()
		if old != m {
			winsymlink.IncNonDefault()
			m = old
		}
	}
	return m
}

/cc @qmuntal

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Mar 9, 2024
@rsc rsc added this to the Go1.23 milestone Mar 9, 2024
@gopherbot
Copy link

Change https://go.dev/cl/570275 mentions this issue: internal/godebugs: test for use of IncNonDefault

gopherbot pushed a commit that referenced this issue Mar 9, 2024
A few recent godebugs are missing IncNonDefault uses.
Test for that, so that people remember to do it.
Filed bugs for the missing ones.

For #66215.
For #66216.
For #66217.

Change-Id: Ia3fd10fd108e1b003bb30a8bc2f83995c768fab6
Reviewed-on: https://go-review.googlesource.com/c/go/+/570275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
@qmuntal qmuntal self-assigned this Mar 11, 2024
@gopherbot
Copy link

Change https://go.dev/cl/570695 mentions this issue: os,internal/godebugs: add missing IncNonDefault calls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants