feat: support uid only form in NormalizeUidGid #553
Conversation
This makes it valid to supply just the uid for operations setting file ownership, like mkdir and push.
| @@ -134,12 +130,22 @@ func NormalizeUidGid(uid, gid *int, username, group string) (*int, *int, error) | |||
| } | |||
| gid = &n | |||
| } | |||
| if gid == nil { | |||
There was a problem hiding this comment.
It wasn't at all obvious to me that uid was guaranteed to be non-nil here (to avoid a nil pointer panic on line 135), but I think it is, due to the previous checks. Is that your thinking too? Can we add a little comment on line 135 like // uid will be non-nil here due to checks above?
There was a problem hiding this comment.
Yeah it's non-obvious isn't it, I just spent 10 minutes going through the logic again ...
The only way for uid to be nil would be if neither uid nor user were specified (we set uid based on the user name). Likewise, the only way for gid to be nil here is if neither gid nor group were specified.
Line 98 guarantees that at least one of the four were specified.
if uid == nil && username == "" && gid == nil && group == "" {
return nil, nil, nil
}Since we check if gid is nil here (i.e. neither gid nor group were provided), then one of uid or user must have been provided, so uid can't be nil.
So maybe comments like this?
if gid == nil { // Neither gid nor group was specified
// uid either specified or looked up from user; use user's primary group ID
james-garner-canonical
force-pushed
the
2025-01/feat/support-uid-only-in-chown
branch
from
fad1fa4 to
597029d
Compare
This makes it valid to supply just the uid for operations setting file ownership, like mkdir and push.
This lack came up when investigating the API for the file operations library.