schakrava/zfs — comment on commit 098815f0058c7fcd180d7936025b375a3a36f6dc
File:  line 0
By behlendorf:

The content of this fix looks good, however I'm going to need to ask you to fix a few style issues. All the code follows the SunOS coding style guide, http://www.cis.upenn.edu/~lee/06cse480/data/cstyle.ms.pdf , and we need to stay consistent with that.

* [style] Use hard tabs for indentation not spaces.

* [style] Good commit comments are a must. They should contain a short one line summary and then a detailed description of what the issue was and how you fixed it. Plus any other information which seems relevant. Here's a nice post describing a good commit comment and why it's a good comment, http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html .

* I would add an _isgroup_ check which is just like _isuser_. This can then be checked below in the conditional similarly to _isuser_ to prevent getgrnam() calls when it's known not to be a group. Also just FYI in general it is preferable to avoid assignments in conditionals such as the `pw = getpwnam(cp)` in your patch. However, in this case I think doing it as you've done results in a nice simplification of the code so I'm OK with making an exception here.

isuser = (type == ZFS_PROP_USERQUOTA || type == ZFS_PROP_USERUSED);
isgroup=(type == ZFS_PROP_GROUPQUOTA || type == ZFS_PROP_GROUPUSED);

* Moving the user/group checks before the numeric SID was a good idea since that isn't supported under Linux and this will basically never be true.

If you can cleanup the following and submit an updated patch as a pull request I'll pull it in. I also noticed you made this change in master and your tree. Typically you'll want to do this sort of thing on a branch which make it's easy to rebase against the latest master, or update the patch based review comments.

Thanks for taking the time to look at this and get it fixed!