Re: [PATCH] completion: add missing configuration variables
- From
Jeff King <peff@peff.net>
- Date
- Dec 15, 2010, 13:00 UTC
- Message-ID
- <20101215130046.GB25647@sigill.intra.peff.net>
- In-Reply-To
- <1292395613-12859-1-git-send-email-martin.von.zweigbergk@gmail.com>
On Wed, Dec 15, 2010 at 07:46:53AM +0100, Martin von Zweigbergk wrote:
> The color.grep.external option has been deleted. Should it be deleted > from here or do we want to help users run e.g. > 'git config --unset color.grep.external'? Same goes for > add.ignore-errors.
IMHO, they should go away. People who have them can figure out how to delete them, but it is more important not to advertise them to people who are adding variables.
As an aside, I would think "--unset" should actually choose from the set of configured variables for completion (i.e., "git config --list | cut -d= -f1"). But that would obviously be a separate patch.
> I didn't find any references to 'diff.renameLimit.' even in 98171a0 > (bash completion: Sync config variables with their man pages, > 2008-12-15) in which it was introduced in the completions script. I > hope it was safe to remove it.
Yeah, I don't think it has ever existed.
> Some variables are documented with camelCase but read in all > lowercase in the code. Not worth updating the code just for that, is > it?
All variables are case-insensitive. The config parser down-cases them, so all code should treat tham as all-lowercase. However, we tend to document them as camelCase for readability.
The completion code should match case-insensitively, too. It doesn't seem to now, but I suspect it is not a problem in practice because the first camelCase word is often enough to get a match, and is lowercase itself.
> I hope none of the added variables are deprecated. After having a > quick look in git-config(1), I think they should not be.
All looked OK from my quick glance.
One note:
Show 9 quoted lines
> color.diff > color.diff.commit > color.diff.frag > + color.diff.func > color.diff.meta > color.diff.new > color.diff.old > color.diff.plain > color.diff.whitespace
We have color.diff.branch coming soon (I think it is in 'next' now).
-Peff