Re: Global .git directory

[Andrew Ruder <andy@aeruder.net>]

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@example.com>" 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@aeruder.net>
http://www.aeruder.net