Re: [PATCH] completion: add missing configuration variables

On Wed, 15 Dec 2010, Jeff King wrote:

On Wed, Dec 15, 2010 at 07:46:53AM +0100, Martin von Zweigbergk wrote:

quoted

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.

Sounds good to me. If no one disagrees, I'll send an updated patch in
a day or two, removing both color.grep.external and add.ignore-errors.

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.

Good point. I'll put it on my todo.

quoted

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.

That I knew...

The config parser down-cases them,
so all code should treat tham as all-lowercase.

... but not that. Thanks. I could/should have figured that out if I
had noticed that *all* of them were lowercase in the code :-P.

One note:

quoted

 		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).

Strictly speaking, that note is for Junio to think of when he merges,
right? But adding it early is pretty harmless and if that relieves him
of some work, I would be happy to add it in the next submission of
this patch. Is that better?

Thinking a bit more, maybe what you are suggesting is that I base the
next revision of this patch on the branch that adds that variable?


/Martin