Re: [PATCH 16/18] fsck: support demoting errors to warnings

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 23 Dec 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The parser I wrote actually accepts both versions, allowing me to skip
> > the tedious step to convert the camelCased config setting into a
> > lower-case-dashed version to pass to `index-pack` or `unpack-objects`,
> > only to be parsed by the same parser as `fsck` would use directly.
> >
> > So I am rather happy with the fact that the parser handles both
> > camelCased and lower-case-dashed versions.
> 
> That is myopic view of the world that ignores maintainability and
> teachability, doing disservice to our user base.

Okay, so just to clarify: you want me to

- split the parser into

	- a parser that accepts only camelCased variable names when they
	  come from the config (for use in fsck and receive-pack), and

	- another parser that rejects camelCased variable names and only
	  accepts lower-case-dashed, intended for command-line parsing
	  in fsck, index-pack and unpack-objects, and

- consequently have a converter from the camelCased variable names we
  receive from the config in receive-pack so we can pass lower-case-dashed
  settings to index-pack and unpack-objects.

If you want it this way, I will do it this way.

> What message does it send to an unsuspecting new user that
> fsck.random-error is silently accepted (because we will never document
> it) as an alias for fsck.randomError, while most of the configuration
> variables will not take such an alias?

I will not participate in a discussion about consistency again. There is
nothing that can be done about it; what matters is what you will accept
and what not. I will make the code stricter (and consequently more
complex) if that is what you want.

> >> I suspect that it would be much better if the configuration variables
> >> were organized the other way around, e.g.
> >> 
> >> 	$ git config fsck.warn missingTagger,someOtherKindOfError
> >
> > I had something similar in an earlier version of my patch series, but
> > it was shot down rightfully: if you want to allow inheriting defaults
> > from $HOME/.gitconfig, you have to configure the severity levels
> > individually.
> 
> Hmmm.  What's wrong with "fsck.warn -missingTagger" that overrides
> the earlier one, or even "fsck.info missingTagger" after having
> "fsck.warn other,missingTagger,yetanother", with the usual "last one
> wins" rule?

I will change the code (next year...).

> >> But the proposed organization to use one variable per questionable
> >> event type (as opposed to one variable per severity level) would lead
> >> to a one-shot override of this form, e.g.
> >> 
> >> 	$ git fsck --missing-tagger=warn --some-other-kind-of-error=warn
> >> 
> >> which I think is insane to require us to support unbound number of
> >> dashed options.
> >
> > The intended use case is actually *not* the command-line, but the config
> > file, in particular allowing /etc/gitconfig, $HOME/.gitconfig *and*
> > .git/config to customize the settings.
> 
> But we do need to worry about one-shot override from the command
> line.  A configuration that sticks without a way to override is a
> no-no.

And of course you can, by specifying the config setting via the -c
command-line option. The only inconsistency here is that all other
command-line options are lower-case-dashed, while the config settings are
camelCased.

> >> Or are you saying that we allow "git config core.file-mode true" from
> >> the command line to set core.fileMode configuration?
> >
> > I do not understand this reference.
> 
> I was puzzled by your "command line" and wondering if you meant
> "from the command line, aVariable can be spelled a-variable".

Well, of course, if you call `git -c aVariable command
--option=a-variable` you have a nice accumulation of styles right there
;-)

> > I did not suggest to change `git config`, did I? If I did, I
> > apologize; it was definitely *not* my intention to change
> > long-standing customs.
> 
> Then fsck.missing-tagger is definitely out.  Long standing customs
> is that a multi-word token at the first and the last level is not
> dashed-multi-word.

But I already changed all of the patches to fsck.missingTagger.

The only thing I did not do yet is to split the parser into two, one
accepting only camelCased, one accepting only lower-case-dashed options,
and a translator to convert from camelCase to lower-case-dashed versions
(because it is a lot of work and additional complexity, as well as
opportunity for bugs to hide because we'll have three code paths). I asked
you above whether you want that, and I will do it if you say that you do.

Ciao,
Dscho