On Tue, May 04, 2010 at 07:07:08AM +0200, Gerhard Wiesinger wrote: > Still got no feedback ... I'm not really a person that will be doing the reviewing but as a frequent lurker on the list, I'm going to try to help you out some. I'll bite, but I think the lack of response is still due to a major case of RTFM. Here's the things from SubmittingPatches that appear to still be wrong and are most likely keeping your patch from being properly reviewed: > Commits: > > - make commits of logical units Your patches aren't really in logical units. Patch 1/2 makes changes to a header file and creates several function declarations which don't even exist until 2/2. So in this case you really would just want one patch there. Or even better, perhaps separate the EXISTING functionality into the API you are eventually shooting for and then add your feature into the next patch. I'm sure if done cleanly, some of the changes like get_git_dir_from_environment() could be done entirely separately from your feature. Once everybody agrees that it looks good and works the same as it always has, it makes the task of reviewing your feature patch even easier as it will be simply adding a feature and not adding a feature AND adding a new GIT_DIR API simultaneously. > - the first line of the commit message should be a short > description and should skip the full stop > - 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 Your commit messages are very under-detailed. At the very least you should include at least as much detail as you did in your original e-mail to the list! Look through git log some, you'll see that it isn't uncommon for patches to actually end up much shorter than the patch itself. At the very least, the "Subject" of your patch should be much shorter. > - if you want your work included in git.git, add a > "Signed-off-by: Your Name <you@xxxxxxxxxxx>" line to the > commit message (or just use the option "-s" when > committing) to confirm that you agree to the Developer's > Certificate of Origin git takes this one pretty seriously. Put this in your patches now to avoid headaches down the line. > - make sure that you have tests for the bug you are fixing While you're not exactly fixing a bug, this would benefit from a testcase. > Patch: > > - use "git format-patch -M" to create the patch You did this, but typically patches are one per e-mail, the subject being the [PATCH ...] line and the body being the rest rather than multiple patches all just copy-pasted inline. > - if you change, add, or remove a command line option or > make some other user interface change, the associated > documentation should be updated as well. As the person who added the above to the documentation, it sure is nice when people edit the appropriate manpages with their proposed changes :). I know it all seems nit-picky, but if you want to see your changes make it into git.git you're best off making it as EASY AS POSSIBLE for the reviewer to take your patch and apply it and be done with it. This just isn't possible without testcases, documentation updates, etc.. Especially for feature additions (vs bug fixes) you really have to make life as simple (and normal) as possible for reviewers, maintainers, etc.. After all, it is a lot easier living without a feature than it is a documented bug-fix! Just a random lurker trying to help you out here! Cheers, Andy P.S. I'd also double-check the patch, typos (like mkdir_recusive) make things look sloppy and can turn people off from reviewing (especially when it is almost the first line of the patch!) -- Andrew Ruder <andy@xxxxxxxxxxx> http://www.aeruder.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html