{"thread":{"id":"45599","subject":"[PATCH v4 1/4] read-cache: core.checksumindex","startedAt":"2017-04-03T18:53:39Z","lastAt":"2017-04-04T20:21:39Z","messageCount":11,"participants":["git@jeffhostetler.com","Ævar Arnfjörð Bjarmason","Jeff King","Jeff Hostetler"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"316103","messageId":"20170403185306.36164-2-git@jeffhostetler.com","threadId":"45599","inReplyTo":"20170403185306.36164-1-git@jeffhostetler.com","subject":"[PATCH v4 1/4] read-cache: core.checksumindex","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-03T18:53:03Z","receivedAt":"2017-04-03T18:53:39Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nTeach git to skip verification of the SHA-1 checksum at the end of\nthe index file in verify_hdr() called from read_index() when the\ncore.checksumIndex configuration variable is set to false.\n\nThe checksum verification is for detecting disk corruption, and for\nsmall projects, the time it takes to compute SHA-1 is not that\nsignificant, but for gigantic repositories this calculation adds\nsignificant time to every command.\n\nOn the Linux kernel repository, the effect is rather trivial.\nThe time to reading its index with 58k entries drops from 0.0284 sec\ndown to 0.0155 sec.\n\nOn my Windows source tree (450MB index), I'm seeing a savings of 0.6\nseconds -- read_index() went from 1.2 to 0.6 seconds.\n\nSigned-off-by: Jeff Hostetler \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 8 ++++++++\n read-cache.c | 12 ++++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 47603f5..0f72bdd 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -267,6 +267,14 @@ advice.*::\n \t\tshow directions on how to proceed from the current state.\n --\n \n+core.checksumIndex::\n+\tTell Git to validate the checksum at the end of the index\n+\tfile to detect corruption. Defaults to `true`. Those who\n+\twork on a project with too many files may want to set this\n+\tvariable to `false` to make it faster to load the index (in\n+\texchange for reliability, but in general modern disks are\n+\treliable enough for most people).\n+\n core.fileMode::\n \tTells Git if the executable bit of files in the working tree\n \tis to be honored.\ndiff --git a/read-cache.c b/read-cache.c\nindex 9054369..dd64cde 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1376,12 +1376,24 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \tgit_SHA_CTX c;\n \tunsigned char sha1[20];\n \tint hdr_version;\n+\tint do_checksum = 1; /* default to true for now */\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(\"bad signature\");\n \thdr_version = ntohl(hdr->hdr_version);\n \tif (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)\n \t\treturn error(\"bad index version %d\", hdr_version);\n+\n+\t/*\n+\t * Since we run very early in command startup, git_config()\n+\t * may not have been called yet and the various \"core_*\"\n+\t * global variables haven't been set. So look it up\n+\t * explicitly.\n+\t */\n+\tgit_config_get_bool(\"core.checksumindex\", &do_checksum);\n+\tif (!do_checksum)\n+\t\treturn 0;\n+\n \tgit_SHA1_Init(&c);\n \tgit_SHA1_Update(&c, hdr, size - 20);\n \tgit_SHA1_Final(sha1, &c);\n-- \n2.9.3\n\n"},{"id":"316104","messageId":"20170403185306.36164-5-git@jeffhostetler.com","threadId":"45599","inReplyTo":"20170403185306.36164-1-git@jeffhostetler.com","subject":"[PATCH v4 4/4] p0002-read-cache: test core.checksumindex","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-03T18:53:06Z","receivedAt":"2017-04-03T18:53:41Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nTeach t/perf/p0002-read-cache to time read_cache() with\nand without the index checksum calculation.\n\nSigned-off-by: Jeff Hostetler \n---\n t/perf/p0002-read-cache.sh | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p0002-read-cache.sh b/t/perf/p0002-read-cache.sh\nindex 9180ae9..71feacd 100755\n--- a/t/perf/p0002-read-cache.sh\n+++ b/t/perf/p0002-read-cache.sh\n@@ -7,7 +7,13 @@ test_description=\"Tests performance of reading the index\"\n test_perf_default_repo\n \n count=1000\n-test_perf \"read_cache/discard_cache $count times\" \"\n+test_perf \"read_cache/discard_cache checksum=1 $count times\" \"\n+\tgit config --local core.checksumindex 1 &&\n+\ttest-read-cache $count\n+\"\n+\n+test_perf \"read_cache/discard_cache checksum=0 $count times\" \"\n+\tgit config --local core.checksumindex 0 &&\n \ttest-read-cache $count\n \"\n \n-- \n2.9.3\n\n"},{"id":"316105","messageId":"20170403185306.36164-1-git@jeffhostetler.com","threadId":"45599","inReplyTo":null,"subject":"[PATCH v4 0/4] read-cache: call verify_hdr() in a background thread","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-03T18:53:02Z","receivedAt":"2017-04-03T18:53:42Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nVersion 4 of this patch series incorporates the cleanup made by Junio\nto my 3rd veresion, updates fsck to always verify the checksum, and a\nnew simplified perf test using p0002 as discussed on the mailing list.\n\n================\nVersion 3 of this patch series simplifies this effort to just turn\non/off the hash verification using a \"core.checksumindex\" config variable.\n\nI've preserved the original checksum validation code so that we can\nforce it on in fsck if desired.\n\nIt eliminates the original threading model completely.\n\nJeff Hostetler (4):\n read-cache: core.checksumindex\n fsck: force core.checksumindex=1\n t1450-fsck: test core.checksumindex\n p0002-read-cache: test core.checksumindex\n\n Documentation/config.txt | 8 ++++++++\n builtin/fsck.c | 1 +\n cache.h | 7 +++++++\n read-cache.c | 18 ++++++++++++++++++\n t/perf/p0002-read-cache.sh | 8 +++++++-\n t/t1450-fsck.sh | 27 +++++++++++++++++++++++++++\n 6 files changed, 68 insertions(+), 1 deletion(-)\n\n-- \n2.9.3\n\n"},{"id":"316106","messageId":"20170403185306.36164-3-git@jeffhostetler.com","threadId":"45599","inReplyTo":"20170403185306.36164-1-git@jeffhostetler.com","subject":"[PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-03T18:53:04Z","receivedAt":"2017-04-03T18:53:44Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nTeach fsck to override core.checksumindex and always verify\nthe index checksum when reading the index.\n\nSigned-off-by: Jeff Hostetler \n---\n builtin/fsck.c | 1 +\n cache.h | 7 +++++++\n read-cache.c | 20 +++++++++++++-------\n 3 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1a5cacc..6913233 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (keep_cache_objects) {\n+\t\tforce_core_checksum_index = 1;\n \t\tread_cache();\n \t\tfor (i = 0; i < active_nr; i++) {\n \t\t\tunsigned int mode;\ndiff --git a/cache.h b/cache.h\nindex 80b6372..3ebda0a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -685,6 +685,13 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n \n+/*\n+ * Override \"core.checksumindex\" config settings. Allows commands\n+ * like \"fsck\" to force it without altering on-disk settings in case\n+ * routines call die() before it can be reset.\n+ */\n+extern int force_core_checksum_index;\n+\n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\n extern int trust_ctime;\ndiff --git a/read-cache.c b/read-cache.c\nindex dd64cde..36fdc2a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1371,6 +1371,8 @@ struct ondisk_cache_entry_extended {\n \t\t\t ondisk_cache_entry_extended_size(ce_namelen(ce)) : \\\n \t\t\t ondisk_cache_entry_size(ce_namelen(ce)))\n \n+int force_core_checksum_index;\n+\n static int verify_hdr(struct cache_header *hdr, unsigned long size)\n {\n \tgit_SHA_CTX c;\n@@ -1384,13 +1386,17 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \tif (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)\n \t\treturn error(\"bad index version %d\", hdr_version);\n \n-\t/*\n-\t * Since we run very early in command startup, git_config()\n-\t * may not have been called yet and the various \"core_*\"\n-\t * global variables haven't been set. So look it up\n-\t * explicitly.\n-\t */\n-\tgit_config_get_bool(\"core.checksumindex\", &do_checksum);\n+\tif (force_core_checksum_index)\n+\t\tdo_checksum = 1;\n+\telse {\n+\t\t/*\n+\t\t * Since we run very early in command startup, git_config()\n+\t\t * may not have been called yet and the various \"core_*\"\n+\t\t * global variables haven't been set. So look it up\n+\t\t * explicitly.\n+\t\t */\n+\t\tgit_config_get_bool(\"core.checksumindex\", &do_checksum);\n+\t}\n \tif (!do_checksum)\n \t\treturn 0;\n \n-- \n2.9.3\n\n"},{"id":"316107","messageId":"20170403185306.36164-4-git@jeffhostetler.com","threadId":"45599","inReplyTo":"20170403185306.36164-1-git@jeffhostetler.com","subject":"[PATCH v4 3/4] t1450-fsck: test core.checksumindex","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-03T18:53:05Z","receivedAt":"2017-04-03T18:55:27Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nAdd test for fsck to force verify the index checksum.\nAdd test to demonstrate that status does ignore the checksum\nwhen reading the index.\n\nSigned-off-by: Jeff Hostetler \n---\n t/t1450-fsck.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 33a51c9..a33d542 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -689,4 +689,31 @@ test_expect_success 'bogus head does not fallback to all heads' '\n \t! grep $blob out\n '\n \n+test_expect_success 'validate force_core_checksum_index=1' '\n+\tgit fsck --cache\n+'\n+\n+test_expect_success PERL 'detect index file corrupt in fsck' '\n+\tcp .git/index .git/index.backup &&\n+\techo zzzzzzzz >zzzzzzzz &&\n+\tgit add zzzzzzzz &&\n+\tperl -pi -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index &&\n+\ttest_must_fail git fsck --cache &&\n+\trm .git/index &&\n+\tmv .git/index.backup .git/index &&\n+\trm zzzzzzzz\n+'\n+\n+test_expect_success PERL 'verify status ignores corrupt checksum' '\n+\tcp .git/index .git/index.backup &&\n+\techo zzzzzzzz >zzzzzzzz &&\n+\tgit add zzzzzzzz &&\n+\tperl -pi -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index &&\n+\tgit -c core.checksumindex=0 status &&\n+\t# Status may fix the checksum\n+\trm .git/index &&\n+\tmv .git/index.backup .git/index &&\n+\trm zzzzzzzz\n+'\n+\n test_done\n-- \n2.9.3\n\n"},{"id":"316110","messageId":"CACBZZX7DFuLia8RzB908EG7+oWQiFGkFEq14bzT77A75msM98Q@mail.gmail.com","threadId":"45599","inReplyTo":"20170403185306.36164-3-git@jeffhostetler.com","subject":"Re: [PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-03T20:31:03Z","receivedAt":"2017-04-03T20:31:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Apr 3, 2017 at 8:53 PM, wrote:\n> Teach fsck to override core.checksumindex and always verify\n> the index checksum when reading the index.\n\nSorry to only chime in about this at v4.\n\nI think this patch & the documentation you added for\ncore.checksumindex in 1/4 would be much less confusing if you made\nthis a on-by-default command-line option like e.g. \"full\".\n\nWith this patch nothing amends the documentation to indicate that the\ncore.checksumindex is magically overridden when fsck runs, I think\nsomething like this (needs amending to integrate) on top would make\nthis clearer:\n\ndiff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt\nindex b9f060e3b2..19b45b1b6f 100644\n--- a/Documentation/git-fsck.txt\n+++ b/Documentation/git-fsck.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git fsck' [--tags] [--root] [--unreachable] [--cache] [--no-reflogs]\n [--[no-]full] [--strict] [--verbose] [--lost-found]\n [--[no-]dangling] [--[no-]progress] [--connectivity-only]\n- [--[no-]name-objects] [*]\n+ [--[no-]name-objects] [--[no-]checksum-index] [*]\n\n DESCRIPTION\n -----------\n@@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`\nnamespace, and all reflogs\n object pools. This is now default; you can turn it off\n with --no-full.\n\n+--[no-]checksum-index:\n+ Validate the checksum at the end of the index file, on by\n+ default, locally overrides any \"core.checksumIndex\" setting\n+ unless negated. See linkgit:git-config[1].\n+\n --connectivity-only::\n Check only the connectivity of tags, commits and tree objects. By\n avoiding to unpack blobs, this speeds up the operation, at the\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex f76e4163ab..8fe8ec1775 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -24,6 +24,7 @@ static int show_tags;\n static int show_unreachable;\n static int include_reflogs = 1;\n static int check_full = 1;\n+static int fsck_opt_checksum_index = 1;\n static int connectivity_only;\n static int check_strict;\n static int keep_cache_objects;\n@@ -656,6 +657,8 @@ static struct option fsck_opts[] = {\n OPT_BOOL(0, \"cache\", &keep_cache_objects, N_(\"make index\nobjects head nodes\")),\n OPT_BOOL(0, \"reflogs\", &include_reflogs, N_(\"make reflogs head\nnodes (default)\")),\n OPT_BOOL(0, \"full\", &check_full, N_(\"also consider packs and\nalternate objects\")),\n+ OPT_BOOL(0, \"checksum-index\", &fsck_opt_checksum_index,\n+ N_(\"validate the checksum at the end of the index file\")),\n OPT_BOOL(0, \"connectivity-only\", &connectivity_only, N_(\"check\nonly connectivity\")),\n OPT_BOOL(0, \"strict\", &check_strict, N_(\"enable more strict checking\")),\n OPT_BOOL(0, \"lost-found\", &write_lost_and_found,\n@@ -681,6 +684,9 @@ int cmd_fsck(int argc, const char **argv, const\nchar *prefix)\n if (check_strict)\n fsck_obj_options.strict = 1;\n\n+ if (fsck_opt_checksum_index)\n+ force_core_checksum_index = 1;\n+\n if (show_progress == -1)\n show_progress = isatty(2);\n if (verbose)\n"},{"id":"316128","messageId":"20170404022945.nmadthjzovmjplyb@sigill.intra.peff.net","threadId":"45599","inReplyTo":"CACBZZX7DFuLia8RzB908EG7+oWQiFGkFEq14bzT77A75msM98Q@mail.gmail.com","subject":"Re: [PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-04T02:29:46Z","receivedAt":"2017-04-04T02:29:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 03, 2017 at 10:31:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Apr 3, 2017 at 8:53 PM, wrote:\n> > Teach fsck to override core.checksumindex and always verify\n> > the index checksum when reading the index.\n> \n> Sorry to only chime in about this at v4.\n> \n> I think this patch & the documentation you added for\n> core.checksumindex in 1/4 would be much less confusing if you made\n> this a on-by-default command-line option like e.g. \"full\".\n> \n> With this patch nothing amends the documentation to indicate that the\n> core.checksumindex is magically overridden when fsck runs, I think\n> something like this (needs amending to integrate) on top would make\n> this clearer:\n\nI think that is the wrong direction to reduce confusion. We don't need\nmore options to twiddle this flag, we need fewer. For instance, in your\nproposed documentation:\n\n> @@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`\n> namespace, and all reflogs\n> object pools. This is now default; you can turn it off\n> with --no-full.\n> \n> +--[no-]checksum-index:\n> + Validate the checksum at the end of the index file, on by\n> + default, locally overrides any \"core.checksumIndex\" setting\n> + unless negated. See linkgit:git-config[1].\n\nThat tells us _what_ it does, but I'm left scratching my head with\n\"why\".\n\nI don't think there is any reason you would want fsck not to compute\nthat checksum (just like there is no option to ask fsck not to check\npack sha1 trailers).\n\nI would go so far as to say that the config option itself is unnecessary\nin this iteration of the series. I only asked for it so that we could\ntest the verification code paths (both for correctness and performance).\nBut if fsck can exercise the code path, we can check correctness that\nway. And for performance, it's probably enough to test two separate\nbuilds (which Jeff has already done).\n\nJunio also asked for the usual \"add a config, and then later we'll flip\nthe default\" procedure. But IMHO that isn't necessary here. Nobody\nshould ever care about this flag. It was largely useless to check it on\nevery read in the first place. And if you suspect there's corruption in\nyour repository, you should run \"git fsck\".\n\n-Peff\n"},{"id":"316138","messageId":"CACBZZX50+Mpj-GY11KNmh+BkNkWCX3OZjHFQ3iK8c8Hib90_Xg@mail.gmail.com","threadId":"45599","inReplyTo":"20170404022945.nmadthjzovmjplyb@sigill.intra.peff.net","subject":"Re: [PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-04T08:23:52Z","receivedAt":"2017-04-04T08:24:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Apr 4, 2017 at 4:29 AM, Jeff King wrote:\n> On Mon, Apr 03, 2017 at 10:31:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Mon, Apr 3, 2017 at 8:53 PM, wrote:\n>> > Teach fsck to override core.checksumindex and always verify\n>> > the index checksum when reading the index.\n>>\n>> Sorry to only chime in about this at v4.\n>>\n>> I think this patch & the documentation you added for\n>> core.checksumindex in 1/4 would be much less confusing if you made\n>> this a on-by-default command-line option like e.g. \"full\".\n>>\n>> With this patch nothing amends the documentation to indicate that the\n>> core.checksumindex is magically overridden when fsck runs, I think\n>> something like this (needs amending to integrate) on top would make\n>> this clearer:\n>\n> I think that is the wrong direction to reduce confusion. We don't need\n> more options to twiddle this flag, we need fewer. For instance, in your\n> proposed documentation:\n>\n>> @@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`\n>> namespace, and all reflogs\n>> object pools. This is now default; you can turn it off\n>> with --no-full.\n>>\n>> +--[no-]checksum-index:\n>> + Validate the checksum at the end of the index file, on by\n>> + default, locally overrides any \"core.checksumIndex\" setting\n>> + unless negated. See linkgit:git-config[1].\n>\n> That tells us _what_ it does, but I'm left scratching my head with\n> \"why\".\n>\n> I don't think there is any reason you would want fsck not to compute\n> that checksum (just like there is no option to ask fsck not to check\n> pack sha1 trailers).\n>\n> I would go so far as to say that the config option itself is unnecessary\n> in this iteration of the series. I only asked for it so that we could\n> test the verification code paths (both for correctness and performance).\n> But if fsck can exercise the code path, we can check correctness that\n> way. And for performance, it's probably enough to test two separate\n> builds (which Jeff has already done).\n>\n> Junio also asked for the usual \"add a config, and then later we'll flip\n> the default\" procedure. But IMHO that isn't necessary here. Nobody\n> should ever care about this flag. It was largely useless to check it on\n> every read in the first place. And if you suspect there's corruption in\n> your repository, you should run \"git fsck\".\n\nThe part that confused my & I found unintuitive is that there's a new\ncore.WHATEVER config that'll get silently overridden by a specific\ncommand, git-fsck.\n\nNothing else I can think of in core.* works like this, i.e. it's a\nnamespace for \"applies to all of git\", core.editor, core.ignoreCase\netc.\n\nHaving git-fsck have a command-line option that's on by default as I\nsuggested is one way to get out of that confusion. It makes it a\nspecial case of a CLI option overriding some config.\n\nBut yeah, another way to resolve this is to get rid of the config\noption altogether, or document in git-config.txt that\ncore.checksumIndex is obeyed by everything except git-fsck.\n"},{"id":"316139","messageId":"20170404082743.jqdn27j6bbftvtvq@sigill.intra.peff.net","threadId":"45599","inReplyTo":"CACBZZX50+Mpj-GY11KNmh+BkNkWCX3OZjHFQ3iK8c8Hib90_Xg@mail.gmail.com","subject":"Re: [PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-04T08:27:43Z","receivedAt":"2017-04-04T08:27:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 04, 2017 at 10:23:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The part that confused my & I found unintuitive is that there's a new\n> core.WHATEVER config that'll get silently overridden by a specific\n> command, git-fsck.\n> \n> Nothing else I can think of in core.* works like this, i.e. it's a\n> namespace for \"applies to all of git\", core.editor, core.ignoreCase\n> etc.\n> \n> Having git-fsck have a command-line option that's on by default as I\n> suggested is one way to get out of that confusion. It makes it a\n> special case of a CLI option overriding some config.\n\nYeah, I do agree your suggestion makes it slightly less confusing.\n\nI mostly just think we can avoid the situation altogether, which IMHO is\npreferable.\n\n-Peff\n"},{"id":"316154","messageId":"9abad913-66a6-f537-af77-8878c428b192@jeffhostetler.com","threadId":"45599","inReplyTo":"CACBZZX50+Mpj-GY11KNmh+BkNkWCX3OZjHFQ3iK8c8Hib90_Xg@mail.gmail.com","subject":"Re: [PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-04T13:13:33Z","receivedAt":"2017-04-04T13:13:52Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/4/2017 4:23 AM, Ævar Arnfjörð Bjarmason wrote:\n> On Tue, Apr 4, 2017 at 4:29 AM, Jeff King wrote:\n>> On Mon, Apr 03, 2017 at 10:31:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> On Mon, Apr 3, 2017 at 8:53 PM, wrote:\n>>>> Teach fsck to override core.checksumindex and always verify\n>>>> the index checksum when reading the index.\n>>>\n>>> Sorry to only chime in about this at v4.\n>>>\n>>> I think this patch & the documentation you added for\n>>> core.checksumindex in 1/4 would be much less confusing if you made\n>>> this a on-by-default command-line option like e.g. \"full\".\n>>>\n>>> With this patch nothing amends the documentation to indicate that the\n>>> core.checksumindex is magically overridden when fsck runs, I think\n>>> something like this (needs amending to integrate) on top would make\n>>> this clearer:\n>>\n>> I think that is the wrong direction to reduce confusion. We don't need\n>> more options to twiddle this flag, we need fewer. For instance, in your\n>> proposed documentation:\n>>\n>>> @@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`\n>>> namespace, and all reflogs\n>>> object pools. This is now default; you can turn it off\n>>> with --no-full.\n>>>\n>>> +--[no-]checksum-index:\n>>> + Validate the checksum at the end of the index file, on by\n>>> + default, locally overrides any \"core.checksumIndex\" setting\n>>> + unless negated. See linkgit:git-config[1].\n>>\n>> That tells us _what_ it does, but I'm left scratching my head with\n>> \"why\".\n>>\n>> I don't think there is any reason you would want fsck not to compute\n>> that checksum (just like there is no option to ask fsck not to check\n>> pack sha1 trailers).\n>>\n>> I would go so far as to say that the config option itself is unnecessary\n>> in this iteration of the series. I only asked for it so that we could\n>> test the verification code paths (both for correctness and performance).\n>> But if fsck can exercise the code path, we can check correctness that\n>> way. And for performance, it's probably enough to test two separate\n>> builds (which Jeff has already done).\n>>\n>> Junio also asked for the usual \"add a config, and then later we'll flip\n>> the default\" procedure. But IMHO that isn't necessary here. Nobody\n>> should ever care about this flag. It was largely useless to check it on\n>> every read in the first place. And if you suspect there's corruption in\n>> your repository, you should run \"git fsck\".\n>\n> The part that confused my & I found unintuitive is that there's a new\n> core.WHATEVER config that'll get silently overridden by a specific\n> command, git-fsck.\n>\n> Nothing else I can think of in core.* works like this, i.e. it's a\n> namespace for \"applies to all of git\", core.editor, core.ignoreCase\n> etc.\n\nMy \"force_core_checksum_index\" global override was a bit of a hack.\nI looked at having fsck explicitly set the \"core.checksumindex\" config\nvalue (which would write to the disk in the code paths I followed)\nbefore it loaded the index, but if we ever find an invalid checksum,\nread_cache() would call die() and fsck would not have a chance to set\nit back in the user's config. So I introduced the global override.\n\n>\n> Having git-fsck have a command-line option that's on by default as I\n> suggested is one way to get out of that confusion. It makes it a\n> special case of a CLI option overriding some config.\n\nI looked at doing that, but thought it would be overkill since\nno one is likely to care about turning it off -- or rather, fsck\nshould always do it whenever it reads the index.\n\n>\n> But yeah, another way to resolve this is to get rid of the config\n> option altogether, or document in git-config.txt that\n> core.checksumIndex is obeyed by everything except git-fsck.\n>\n\nIf there's no objections then, I'll just remove the config\nsetting and keep the force_ global for fsck.\n\nJeff\n\n"},{"id":"316166","messageId":"20170404201826.j6yczm6hvksasj4m@sigill.intra.peff.net","threadId":"45599","inReplyTo":"9abad913-66a6-f537-af77-8878c428b192@jeffhostetler.com","subject":"Re: [PATCH v4 2/4] fsck: force core.checksumindex=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-04T20:18:26Z","receivedAt":"2017-04-04T20:21:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 04, 2017 at 09:13:33AM -0400, Jeff Hostetler wrote:\n\n> > But yeah, another way to resolve this is to get rid of the config\n> > option altogether, or document in git-config.txt that\n> > core.checksumIndex is obeyed by everything except git-fsck.\n> \n> If there's no objections then, I'll just remove the config\n> setting and keep the force_ global for fsck.\n\nThat makes the most sense to me. It doesn't fill the \"remain careful by\ndefault, and we can flip the config later\" criterion that Junio set out.\nBut I really just don't think \"careful\" here is buying us anything.\n\n-Peff\n"}]}