On Tue, Apr 4, 2017 at 4:29 AM, Jeff King <peff@xxxxxxxx> wrote: > On Mon, Apr 03, 2017 at 10:31:03PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> On Mon, Apr 3, 2017 at 8:53 PM, <git@xxxxxxxxxxxxxxxxx> wrote: >> > Teach fsck to override core.checksumindex and always verify >> > the index checksum when reading the index. >> >> Sorry to only chime in about this at v4. >> >> I think this patch & the documentation you added for >> core.checksumindex in 1/4 would be much less confusing if you made >> this a on-by-default command-line option like e.g. "full". >> >> With this patch nothing amends the documentation to indicate that the >> core.checksumindex is magically overridden when fsck runs, I think >> something like this (needs amending to integrate) on top would make >> this clearer: > > I think that is the wrong direction to reduce confusion. We don't need > more options to twiddle this flag, we need fewer. For instance, in your > proposed documentation: > >> @@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs` >> namespace, and all reflogs >> object pools. This is now default; you can turn it off >> with --no-full. >> >> +--[no-]checksum-index: >> + Validate the checksum at the end of the index file, on by >> + default, locally overrides any "core.checksumIndex" setting >> + unless negated. See linkgit:git-config[1]. > > That tells us _what_ it does, but I'm left scratching my head with > "why". > > I don't think there is any reason you would want fsck not to compute > that checksum (just like there is no option to ask fsck not to check > pack sha1 trailers). > > I would go so far as to say that the config option itself is unnecessary > in this iteration of the series. I only asked for it so that we could > test the verification code paths (both for correctness and performance). > But if fsck can exercise the code path, we can check correctness that > way. And for performance, it's probably enough to test two separate > builds (which Jeff has already done). > > Junio also asked for the usual "add a config, and then later we'll flip > the default" procedure. But IMHO that isn't necessary here. Nobody > should ever care about this flag. It was largely useless to check it on > every read in the first place. And if you suspect there's corruption in > your repository, you should run "git fsck". The part that confused my & I found unintuitive is that there's a new core.WHATEVER config that'll get silently overridden by a specific command, git-fsck. Nothing else I can think of in core.* works like this, i.e. it's a namespace for "applies to all of git", core.editor, core.ignoreCase etc. Having git-fsck have a command-line option that's on by default as I suggested is one way to get out of that confusion. It makes it a special case of a CLI option overriding some config. But yeah, another way to resolve this is to get rid of the config option altogether, or document in git-config.txt that core.checksumIndex is obeyed by everything except git-fsck.