-
Notifications
You must be signed in to change notification settings - Fork 84
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
avoid text file busy error on track #2494
base: main
Are you sure you want to change the base?
Conversation
@@ -38,7 +38,7 @@ func IsExecutable(filename string) bool { | |||
return false | |||
} | |||
info, _ := os.Stat(filename) | |||
return info.Mode()&0x0100 != 0 | |||
return info.Mode()&0o0100 != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this &0o0100?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0o0100 (better 0o100, will fix), is a bit mask that, when applying bitwise AND (&) results
in a != 0 state whenever the file can be executed by the owner.
A perm can be seen as a 9-bit sequence mapping to rwxrwxrwx read-write-execute
for owner,group,everybody.
octal 0o100 means binary 001000000.
Probably better to set a constant and also add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It previously was 0x0100 == binary 100000000. this was a bug, filtering in combinations where
the owner has read perms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, would you mind adding a comment that explains this?
// SetupExecFile copies a file into destination and set it to have exec perms, | ||
// if destination either does not exists, or is not executable | ||
func SetupExecFile(src string, dst string) error { | ||
if !IsExecutable(dst) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to error/log if the file isn't executable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to error. This also happens on first install.
We are not going to error on first install, and we prefer to try to
recover if a previous install failed.
Probably we can add log for a failed previous install, while trying to recover.
@@ -98,3 +98,67 @@ func TestReadGoVersion(t *testing.T) { | |||
} | |||
}) | |||
} | |||
|
|||
func TestSetupExecFile(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love to see this test
Why this should be merged
Closes #2465
How this works
How this was tested
How is this documented