Re: [PATCH v4 00/19] Introduce an internal API to interact with the fsck machinery

Hi Michael,

On 2015-02-03 16:11, Michael Haggerty wrote:

On 02/02/2015 05:48 PM, Johannes Schindelin wrote:

quoted

On 2015-02-02 13:43, Michael Haggerty wrote:

quoted

On 02/02/2015 12:41 PM, Johannes Schindelin wrote:

quoted

Hi all (in particular Junio),

On 2015-01-31 22:04, Johannes Schindelin wrote:

quoted

[...] switch to fsck.severity to address Michael's concerns that
letting fsck.(error|warn|ignore)'s comma-separated lists possibly
overriding each other partially;

Having participated in the CodingStyle thread, I came to the
conclusion that the fsck.severity solution favors syntax over
intuitiveness.

Therefore, I would like to support the case for
`fsck.level.missingAuthor` (note that there is an extra ".level." in
contrast to earlier suggestions).

Why "level"?

"Severity level", or "error level". Maybe ".severity." would be better?

Sorry, I should have been clearer. I understand why the word "level"
makes sense, as opposed to, say, "peanut-butter". What I don't
understand is why a middle word is needed at all. In the config file it
will look like

[fsck "level"]
        missingAuthor = error

, which looks funny. "level" is a constant, so it seems superfluous.

If anything, it might be more useful to allow an optional middle word to
allow the strictness level to be adjusted based on which command
encounters the problem. For example, if you want to tolerate existing
commits that have missing authors, but not allow any new ones to be
pushed, you could set

[strictness]
        missingAuthor = ignore
[strictness "receive-pack"]
        missingAuthor = error

(There's probably a better word than "strictness", but you get the idea.)

Ah. Well, the idea of the middle constant is to separate the severity levels from all other fsck (or receive.fsck) settings. The 'fsck.skiplist' setting that I introduce in this patch series, for example, looks pretty much the same as 'fsck.missingauthor', but they have different roles.

This becomes important when I want to catch obvious problems such as 'fsck.missingautor': if I have an extra '.level', I can be certain that it is a typo rather than a config setting unrelated to the severity levels.

quoted

quoted

quoted

The benefits:

- it is very, very easy to understand

- cumulative settings are intuitively cumulative, i.e. setting
`fsck.level.missingAuthor` will leave `fsck.level.invalidEmail`
completely unaffected

- it is very easy to enquire and set the levels via existing `git
config` calls

Now, there is one downside, but *only* if we ignore Postel's law.

Postel's law ("be lenient in what you accept as input, but strict in
your output") would dictate that our message ID parser accept both
"missing-author" and "missingAuthor" if we follow the inconsistent
practice of using lowercase-dashed keys on the command-line but
CamelCased ones in the config.

However, earlier Junio made very clear that the parser is required to
fail to parse "missing-author" in the config, and to fail to parse
"missingAuthor" on the command-line.

Therefore, the design I recommend above will require two, minimally
different parsers for essentially the same thing.

IMHO this is a downside that is by far outweighed by the ease of use
of the new feature, therefore I am willing to bear the burden of
implementation.

I again encourage you to consider skipping the implementation of
command-line options entirely. It's not like users are going to want to
use different options for different invocations. Let them use

    git -c fsck.level.missingAuthor=ignore fsck

if they really want to play around, then

    git config fsck.level.missingAuthor ignore

to make it permanent. After that they will never have to worry about
that option again.

Unfortunately, I have to pass the `receive.fsck.*` settings from
`git-receive-pack` to `git-unpack-objects` or `git-index-pack` via the
command-line, because it is `git-receive-pack` that consumes the config
setting, but it is one of `git-unpack-objects` and `git-index-pack` that
has to act on it...

Wouldn't that work automatically via the GIT_CONFIG_PARAMETERS
mechanism? If I run

    git -c foo.bar=baz $CMD

, then git-$CMD is invoked with GIT_CONFIG_PARAMETERS set to
"'foo.bar=baz'", which causes child processes to treat that value as a
configuration setting. I don't have a lot of experience with this but I
think it should do what you need.

This is true, but please remember that the receive.fsck.* settings should be heeded by index-pack/unpack-objects *only* if one of the latter programs is called by receive-pack. It would therefore be a little funny (or wrong, depending on your point of view) if, say, index-pack would respect the receive.fsck.* settings.

That is why receive-pack adds a `--strict` command-line option when receive.fsckobjects is set to true instead of letting index-pack (or unpack-objects) look at the config variable receive.fsckobjects itself.

In the same spirit, I extend the `--strict` command-line option with an optional list of severity level overrides (e.g. `--strict=missing-author=ignore,...`) if receive-pack was configured to override those levels.

Now, as this optional argument is intended for internal use only, we could declare that it is okay to pass CamelCased stuff there, even if it disagrees with our command-line option conventions. If it is not okay, I will have to write a rewriter that consumes the CamelCased config settings and rewrites them as lowercase-dashed settings, to be passed to index-pack or unpack-objects, which in turn parses the lowercase-dashed settings.

It might be seen as unnecessarily complicated, which is why I argued in favor of letting the parser accept both forms (which incidentally would heed Postel's law, too). But I understand if Junio does not want that, because it technically violates the conventions he wants to see established. I just need a definitive statement which way to go so I can implement it.

Ciao,
Dscho