From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:21
Johannes Schindelin [off-list ref] 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.
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?
quoted
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?
Whoever shot it down "rightfully" is wrong here, I would think.
quoted
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.
quoted
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".
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.
From: Johannes Schindelin <hidden> Date: 2016-06-15 23:03:21
Hi Junio,
On Tue, 23 Dec 2014, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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.
quoted
quoted
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...).
quoted
quoted
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.
quoted
quoted
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
;-)
quoted
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
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:39
On 12/23/2014 06:14 PM, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
On Tue, 23 Dec 2014, Junio C Hamano wrote:
quoted
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?
Whoever shot it down "rightfully" is wrong here, I would think.
Sorry I didn't notice this earlier; Johannes, please CC me on these
series, especially the ones that I commented on earlier.
I might have been the one who "shot down" the "<severity>=<name>" style
of configuration [1].
I don't feel strongly enough to make a big deal about this, especially
considering that the other alternative has already been implemented. But
for the record, let me explain why I prefer the "<name>=<severity>"
style of configuration.
First, it is a truer representation of the data structure within the
software, which is basically one severity value for each error type.
This is not a decisive argument, but it often means that there is less
impedance mismatch between the style of configuration and the concepts
that it is configuring. For example,
$ git config receive.fsck.warn A,B,C
$ git config receive.fsck.error C,D,E
seems to be configuring two sets, but it is not. It is mysteriously
setting "C" to be an error, in seeming contradiction of the first line [2].
Second, it is not correct to say that this is just an application of the
"last setting wins" rule. The "last setting wins" rule has heretofore,
as far as I know, only covered *single* settings that take a single
value. If we applied that rule to the following:
$ git config receive.fsck.warn A,B,C
$ git config receive.fsck.warn B,F
then the net result would be "B,F". But that is not your proposal at
all; your proposal is for these two settings to be interpreted the same as
$ git config receive.fsck.warn A,B,C,F
Similarly, the traditional last setting rule, applied to the first
example above, wouldn't cause the value of "fsck.warn" to be reduced to
"A,B", as you propose. This is not the "last setting rule" that we are
familiar with--it operates *across and within* values and across
*multiple* names rather than just across the values for a single name.
Third, the "<severity>=<name>" style is hard to inquire via the command
line, and probably also incompatible with the simplified internal config
API in git (and probably libgit2, JGit, etc). The problem is that
determining a *single* setting requires *three* configuration variables
be inquired, and that the settings for those three variables need to be
processed in the correct order, including the correct order of
interleavings. For example, how would you inquire about the configured
severity level of "missingTaggerEntry" using the shell? It would be a
mess that would necessarily have to involve "git config --get-regexp"
and error-prone parsing of comma-separated values. It would be so much
easier to type
$ git config receive.fsck.missingtaggerentry
Fourth, the "<severity>=<name>" style would cause config files to get
cluttered up with unused values. Suppose you have earlier run
$ git config receive.fsck.warn A,B,C
$ git config receive.fsck.ignore D,E
and now you want to demote "B" to "ignore". You can do
$ git config --add receive.fsck.ignore B
(don't forget "--add" or you've silently erased other, unrelated
settings!) This gives the behavior that you want. But now your config
file looks like
[receive "fsck"]
warn = A,B,C
ignore = D,E
ignore = B
The "B" on the first line is now just being carried along for no reason,
but it would be quite awkward to clean it up programmatically.
Effectively, these settings can only be added to but never removed
because of the way multiple properties are mashed into a single setting.
I believe that one of the main arguments for the "<severity>=<name>"
style of configuration is that it carries over more easily into
convenient command-line options. But I think it will be unusual to want
to configure these options by hand on the command line, let alone adjust
many settings at the same time. The idea isn't to make it easy to work
with repositories that have a level of breakage that fluctuates over
time. It is to make it possible to work with *specific* repositories
that have known breakage in their history. For such a repo you would
configure one or two "ignore" options one time and then never adjust
them again. (And it will also allow us to make our checks stricter in
the future without breaking existing repositories, and even to add
optional "policy" checks, like "forbid Windows-incompatible filenames".)
I would even go so far as to say that we don't *need* command-line
option versions of these settings; if somebody really needs that they
can type
$ git -c receive.fsck.missingtaggerentry=ignore fsck
(which also has the advantage of passing the setting through to any
child processes). But *if* command-line options are considered
necessary, I don't think that using a "<name>=<severity>" style within
the config needs to rule out allowing command-line options in the form
"--<severity>=<name>,<name>" as Junio has suggested.
Looking back at this email, I guess that I'm more strongly against the
"<name>=<severity>" configuration style than I thought :-/
Michael
[1] I prefer to think that I just offered a little gentle discussion
that informed Johannes's independent decision :-)
[2] But even on these terms, it is anomalous. The usual git way to
configure a set in git would be
$ git config receive.fsck.warn A
$ git config --add receive.fsck.warn B
$ git config --add receive.fsck.warn C
$ git config receive.fsck.error C
$ git config --add receive.fsck.error D
$ git config --add receive.fsck.error E
, which in fact has fewer of the disadvantages listed in this email.
--
Michael Haggerty
mhagger@alum.mit.edu
From: Johannes Schindelin <hidden> Date: 2016-06-15 23:03:39
Hi Michael,
On 2015-01-22 16:49, Michael Haggerty wrote:
On 12/23/2014 06:14 PM, Junio C Hamano wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
On Tue, 23 Dec 2014, Junio C Hamano wrote:
quoted
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?
Whoever shot it down "rightfully" is wrong here, I would think.
Sorry I didn't notice this earlier; Johannes, please CC me on these
series, especially the ones that I commented on earlier.
Very sorry, this is my fault. It can only be explained by my switching around some tools for other tools to work with email-based patch submission (which I had not done in a long time). But still, my mistake.
[1] I prefer to think that I just offered a little gentle discussion
that informed Johannes's independent decision :-)
You did convince me back then. I just did not want to put up a fight against Junio because I was more interested in getting this feature merged before the holidays (it does feel awkward for me to leave work unwrapped-up before leaving for an extended amount of time, but I guess I am getting more used to that).
So now I cannot avoid discussing this issue properly...
In essence, I agreed with Junio from the point of view of an elegant implementation. But then, Michael is correct that it does not really matter as much how complicated the code is, but that it is much more important that the feature is elegant to use.
Now let's step back a bit and think about the users which is supposed to be supported by this patch series: Git repository hosters -- such as GitHub -- need to ensure a certain cleanliness of the repositories they host (for a range of reasons, including the prevention of malicious attacks, or helping users publish their code in a correct form).
And the scenario in which the feature needs to be used is most likely started by some Git user pushing some commits, and `git receive-pack` triggering an error. Then the user files a trouble ticket and GitHubber needs to inspect the error and the respective object. Now, in the vast number of cases I imagine that the objects *are* faulty. However, on occasion the problem should not prevent the push, e.g. when somebody crafted a commit object with two authors, forgetting that the tools usually cannot handle such commits. Then the GitHubber has to decide on a case by case basis whether to demote that error to a warning and allow the object to be pushed *into that specific repository*.
I do see the need for this feature to be simple and robust, from the users' point of view. In other words, I agree with Michael that we need to avoid confusing settings such as
This feature will be used rarely enough that the poor soul stuck with interpreting the above config section won't remember that a very specific version of "last setting wins" is in effect.
If I remember correctly, Peff suggested that there needs to be a way to handle these settings in the /etc/gitconfig $HOME/.gitconfig $XDG../gitconfig .git/config cascade, but now I am puzzled whether it is even desirable to demote fsck errors globally, i.e. whether we really need to pay attention to that config cascade.
And finally, in the course of preparing this patch series, we came up with an alternative solution to the problem: the receive.fsck.skiplist (i.e. a file that contains a sorted list of SHA-1s of objects that should be skipped from fsck'ing). I am more and more convinced that this is the most convenient tool for the scenario described above: manual inspection of individual objects will tell whether it is safe to allow them onto the server or not.
However, others might disagree and prefer the explicit approach, e.g. when some source generates a consistent stream of objects triggering fsck errors.
Summary: I have no preference how to specify the severity levels of fsck messages, but I will gladly change my code to whatever you (meaning Junio and Michael in particular) want to see implemented.
Thanks for helping me with this feature,
Dscho
I *think* I found a solution.
Please let me recapitulate quickly the problem Michael brought up: if we support `receive.fsck.warn` to override `receive.fsck.error` and vice versa, with comma-separated lists, then it can be quite confusing to the user, and actually quite difficult to figure out on the command-line which setting is in effect (because it really depends on the *order* of the receive.fsck.* lines, *plus* the fact that the values are comma-separated lists).
On the other hand, Junio pointed out two shortcomings with my original implementation (i.e. to support `receive.fsck.<id> = (error|warn|ignore)`), however: it is tedious to set multiple severity levels, and it violates the config file convention that the config variable names are CamelCased (the message IDs are dashed-lowercase instead).
The solution I just implemented (and will send out shortly in v4 of the patch series) is the following: the config variable is called receive.fsck.severity and it accepts comma-separated settings. Example: