patchGenerate a warning message if we find an unrecognized option.

---
 imap-send.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

On Mon, Feb 08, 2010 at 04:33:35PM -0600, Jeremy White wrote:

Slight NAK from me on this. When we later add new options, it makes using the same config for multiple versions of git difficult (the old versions will complain about the unknown option).

And obviously that is weighed against the ability to notice things like typos. But if we are going to start complaining about unknown config, we would probably do better to complain about _all_ unknown config, and not just this one subsection.

-Peff

Jeff King <peff@peff.net> writes:

We would probably want something like:

	static int do_warn_unknown_config;

	void warn_unknown_config(const char *key)
        {
		if (do_warn_unknown_config)
                	warn("Unknown configuration variable %s", key);
        }

and sprinkle that everywhere.

An interesting issue is where to flip do_warn_unknown_config. A naïve and obvious implementation would do:

        static int git_default_core_config(const char *var, const char *value)
        {
		...
		if (!strcmp(var, "core.warnunknownconfig")) {
			do_warn_unknown_config = git_config_bool(var, value);
			return 0;
		}
		...
	}

but that means the definition of this variable has to come very early in the configuration file to be effective.

On Mon, Feb 08, 2010 at 04:59:12PM -0800, Junio C Hamano wrote:

Your "sprinkle that everywhere" is a little harder than one might hope. There is no code path that reads and claims to recognize _all_ of the git config. In many cases, the git_*_config for individual subsystems can reasonably lay claim to the whole of a "[heading]" section. But there are exceptions even to that.

Diff config is split across git_diff_basic_config and git_diff_ui_config. You would certainly not want to warn about diff.color.* just because you are running a plumbing command which happens not to recognize those entries.

And some headings have entries for several subsystems. remote.*.fetch is used many places for calculating upstream branches. But only git-remote looks at remote.*.skipDefaultUpdate.

So in practice I think you will get quite spotty coverage. Which isn't to say it isn't necessarily worth doing, but I am personally not very excited about working on it. I do like the suggestion of making it optional, so that people who don't care about having a portable config can have the benefit of sanity-checking their config.

I would rather have a "git config --lint" command, but that is even harder, since we are not even loading most of the subsystems which know about the valid config options. And it presupposes that people will bother to actually run such a lint command.

You could always just hoist into an environment variable which neatly gets rid of the ordering problem.

-Peff

On Mon, Feb 08, 2010 at 10:01:51PM -0500, Jeff King wrote:

No, please, please no.

This runs up against the same issue you pointed out earlier--that older versions of git cannot adequately lint configs from newer versions.

There are also config variables from unknown git scripts outside of git.git that happen to use the git-config mechanism because it is convenient. It would be unfortunate to punish those who chose to make up their own config variables by warning them that git doesn't know about them.

I have to wonder if this is a non-existent problem.

Config variables are one-shot things. You set them and forget about them. When you set it you are usually pretty well aware of whether it's typoed because it simply does't work. color.ui is a perfect example. If it's typoed, you don't need 'git config --lint' to tell you, you already know by virtue of using the thing.

Maintaining 'git config --lint' would also be a PITA since we'd then have to remember to update yet another place in addition to code and documentation. And who's to say what gets to be "in" and what doesn't? git-gui, for example, has its own [gui] namespace. git-p4 has its own [git-p4] namespace, but it lives in contrib/, etc. etc.

This seems like a bad idea to me.

On Mon, Feb 08, 2010 at 09:17:31PM -0800, David Aguilar wrote:

I would only be OK with it if it were optional.

Yeah, but that isn't a big deal. You just don't run "config --lint" with the older version. But if, for example, "git diff" breaks because your config is too new, then that is a real pain (and that was what happened with color.diff.func recently).

Yes, I don't think anyone is proposing to lint _all_ variables. But it does not seem unreasonable for certain subsystems to claim portions of the namespace. I would expect git-core to own "core.*". And I would expect git-gui to own "git-gui", etc.

I think I agree with you on this. It is _much_ more annoying to me not to have version portability than it is not to have strict config checking. I was mainly trying to put myself in the shoes of "regular" users, who are less likely to be running the same config file on many different versions, and are more likely to be clueless about config. But I may have overcompensated.

-Peff