przemoc
commented
11 years ago Thanks for the effort you put into this, it's appreciated.
Haven't checked it myself yet (sorry, lack of time), so I'll give only some random notes for now after skimming your changes:
- There is no need to split this work into many commits unless they express subsequent (always compilable & working) steps. In your case, though, 2nd and 3rd commits are only fixups that should be squeezed into 1st one before making a pull request. Don't pollute history unless you're really enforced to (like: tons of people are already depending on your public
masterthat you pushed to just a bit too early). My advice for the future is to use feature branches, that you merge tomaster(or send as a pull request) when you're sure they're ready for prime time. - I think that
-N(upper-case) would fit here better. Mind that-nis usually used for--dry-runlong option, and whilemetastoredoesn't have such one yet, I wouldn't want to close this possibility. - Please don't use magic strings or numbers within source code, even if nearby comments explain it. Introduce proper self-explanatory
#defines for them. Get rid of the current comments, but next to newly introduced#defines explain what you wrote already in the pull request (i.e. NTFS ACLs stored in extended attributes by ntfs-3g are not enumerated bylistxattr().- it's nowhere in the commit, but it's crucial information!) - Stick to the widely applied practice about commit message:
- the body should provide a meaningful commit message, which:
- uses the imperative, present tense: "change", not "changed" or "changes".
- includes motivation for the change, and contrasts its implementation with previous behaviour
- I don't like this another-function-argument approach. I'm not blaming you for going that way. I chose it back then, because for simple addition it looked okay, but it doesn't really scale well. (*)
(*) I think also that I've committed a sin of being too cautious by duplicating function for -g / --git case without testing whether handling it in the old function was giving any noticeable slow-down.
So I guess that I'll went with passing pointer to some new struct, that will have only .git-related setting member and you'll enhance it with NTFS-related setting and proper handling. But it won't happen now or in upcoming days - sorry!
P.S. Github PRs don't track rebases, thus after rebase you have to send a comment here, because otherwise no one will notice the change.
ntfs-3g stores attributes and ACLs in xattrs that are not enumerated in listxattr. This adds a "-n" option which manually adds those xattrs to be stored in the database.