I think this is true for err == nil.

EDIT: So it is. Good catch.

I don't follow:

!os.IsNotExist(err) == true

so you returning error for nil error. Or this is right logic?

Sorry, you're quite right. I misread the line. Pushing fixed version.

I like the idea of using Readers, however, do we have an actual use case where we won't be reading from a file? I ask because this requires additional work on the caller to open these files instead of just passing the paths down.

Using readers has three main benefits:

• It makes the interface for user/ far more generalised. This is more of a subjective point, because it means that people can use the functions without the need for the library to use the filesystem (so if you have some generated data or are reading from a network connection which has sent the data you can use user/ without needing to write a temporary file, etc). This results in the library being more testable and other minute benefits.
• It allows you to pass nil readers that will be used to ignore the existence of a passwd or group source and to try to carry on without them. The most obvious use of this is for files that do not exist (which was supported by the original design), but what about if you want to come up with some scheme where you can only refer to a group by its gid or you want to enforce that only uid and gid formats are allowed. Or maybe you want all errors when opening a file to be ignored (which was not supported by the original interface).
• It also creates separation of concern. The user/ library doesn't need to know whether or not the file exists or where it is on the filesystem (if it even is on the filesystem) or if it has read access, etc. It means that the caller can do its job in getting the data and user/ can do its job in parsing the data.

I agree with your points and also that this is subjective. There is something to be said about simplicity of using the API, though. For e.g. http://golang.org/pkg/os/user/#Lookup. It does what is expected on Linux. IMO, we should only add features, if there is a solid requirement. I think at minimum a wrapper API that passes in Readers for /etc/passwd and /etc/group should be provided, so the clients have a simpler API to call when possible. I would defer to @tianon and @crosbymichael on this one. ( I don't have a strong objection, just a subjective opinion ;) )

I have no problem with adding wrappers (like ParsePasswdFileFilter or something similar), where you pass a file path instead of an io.Reader and it treats errors the same way user does currently.

Also, the main difference between os/user and libcontainer/user is that libcontainer deals with more than one system and deals with abstract representations of containers and implements them on Linux. While I do agree that passing file paths makes the interface look simpler, it removes some of the functionality gained by using Readers.

I'm +1 to implement os/user interface here. So we'll have os/user on pure Go.

@LK4D4 I've added an os/user interface in user/lookup.go (with os-specific stuff in user/lookup_unix.go).

Yeah, I would definitely love to see some wrappers on this, especially if we could get most callers of this code using a version of this that doesn't have to hard-code /etc/passwd (I love having that path be OS-specific like you've got below, even though this is a pretty linux-specific class/library right now). I think it's good to keep in mind that this is being used outside of libcontainer/Docker too, so having an interface that is simple to use in the default case is a good thing.

Regarding whether proliferation of simple wrapper functions is a bad thing, I think it's absolutely a great thing, as evidenced by their use within the Go standard library itself. So I think having a wrapper that lets us just pass in file paths would be good (obviously containing this boiler-plate code of opening the files etc.), and then also a wrapper that requires no filenames to be passed in at all and just uses the functions provided within the module to have sensible defaults.

@tianon: Sure. I'll write a some wrappers for ParsePasswdFilter, ParsePasswd, ParseGroupFilter and ParseGroup to take a file path (probably with a signature like ParsePasswdFilterFile(path string, filter func(u User) bool) ([]User, error)).

This should be len(users) == 0

Fixed.

What do you mean by a RunUser here?

@crosbymichael It's a struct that contains all information relevant to running as a specific user. Hence RunUser. I couldn't think of a better name, if you've got a better one, I'd love to use it instead.

EDIT: There. Fixed and changed to ExecUser which kinda reveals more of what it is actually used for. PTAL?

Why are /etc/passwd and /etc/group still hard-coded here as well as inside the user package? Isn't there a wrapper that allows these to just be defaulted properly as one might expect them to be so we don't repeat ourselves, especially with constants like this that are going to be fairly common for every potential user of the module?

Whoops. That's precisely the reason why I created the whole lookup_unix.go thing. I'll update it now.

Do you think it might be worth switching this return value to a *ExecUser so we can keep the super-simple nil returns? Maybe @crosbymichael can weigh in on his preference here. I'm good either way, but figured it's worth discussing briefly.

I think he has it this way because @LK4D4 said not to use pointers because it's a small type and we don't need the overhead but I think its a bad UI, you don't see many stdlib functions return something like this.

@crosbymichael @tianon: While I understand wanting nil returns (and that is fair enough), the reason I did it this way was because ExecUser struct values are just a representation of an abstract concept, and thus don't make sense to be pointer-ised (is that even a word). Basically, the way I've always learnt to use pointers is:

Only use pointers if the data it points to may change underneath you, and such changes make the old values invalid.

In other words, things like terminal or server structs might make sense to pointer-ise (the terminal or server may change states, and thus the old value is invalid) but when you're dealing with abstract representations of parsed user specifications it doesn't really (IMO) make sense to make them pointers (since the actual point of a pointer -- the ability to change the value it points to, and all users of that pointers now use the updated value -- is not used).

EDIT: Also, the old function didn't use nil returns -- it returned all of the struct fields as separate return values -- so we haven't really lost anything.

/ping @tianon @crosbymichael Any thoughts about my argument?

I don't think "status quo" is a very compelling argument here, especially
since improving this interface is the whole impetus of this PR, right? :)

That's fair enough, I'll make the change.