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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2015, 03:50 UTC
Message-ID: <xmqqegq6nxmh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <2313893ae2200229ccdeb8f71b629445@www.dscho.org>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

>> [fsck "level"]
>>         missingAuthor = error
>> 
>> , which looks funny. "level" is a constant, so it seems superfluous.

Yes, it is superfluous, but is one way to avoid the ambiguity with "skiplist". Structuring it like this would not be so bad, either, though.

	[fsck]
        	error = missingAuthor[, other kinds of errors...]

A small set like {ignore, warn, error} is easily maintainable not to conflict with "skiplist" and others.

So that "avoid ambiguity with skiplist" does not favor either choice in any significant way.

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

"[fsck] error = missingAutor" would let you catch the typo in a similar way with the same context clue, so this does not decide which is better, either.

One clear benefit I can see in it is that you can do

	git config fsck.level.missingAuthor

in scripts that wants to learn the current setting for a single variable. With "fsck.error=missingAuthor[,other kinds]", you would instead have to do a bit more silly post-processing

	git config -l | sed -ne '
        	/^fsck\./{
			# make it "var=,token1,token2,token3,"
			s/=/=,/
                        s/$/,/
                        s/[ 	]*//g
			s|^fsck\.\([^=]*\)=.*,missingAuthor,.*|\1|p
		}
	' | tail -n 1

to grab the last fsck.{error,ignore,...}= thing that has the token (I personally do not think the latter is so bad, though).

I wonder if

	[fsckError]
        	missingAuthor = error
                missingTagger = warn

wouldn't be a better way, though.  We'd keep the easier scripting

	git config fsckError.missingTagger

There is nothing that says that the top-level grouping must match the Git subcommand name. Nothing says that one Git subcommand can own at most one namespace, either. Nothing stops us from reserving fsckError top-level namespace for variable name collision avoidance with other fsck.* variables, if that gives us a better system.

Show 5 quoted lines

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

But receive-pack at some point decides what, if anything, needs to be passed when invoking unpack-objects, or index-pack, no? Why is it hard to pass "-c var=val" at the beginning where it would have passed "--strictness=var=val" at the end?

Show 15 quoted lines

>> 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 means it would be fine if receive-pack invokes (when it sees receive.fsck.severity=missingAuthor=error,missingTagger=warn config meant for it and was told with receive.fsckObjects to check the incoming objects) a command line like this:

	git -c fsckError.missingAuthor=error \
            -c fsckError.missingTagger=warn \
		index-pack $args...

(or whatever variable names and name structure we settle on). And the index-pack command does not have to even know there are receive.fsck.* variables at all, no?

Another way to do that may be for receive-pack to invoke

	git index-pack --use-fsck-severity=receive.fsck $args...

to instruct it to look at receive.fsck.* variables, again when and only when receive-pack wants to do so. I think either way would be fine, as this communication is an internal implementation detail between receive-pack and index-pack and is not meant to be exposed to the end users anyway.