From: Derrick Stolee <stolee@gmail.com> To: "Martin Ågren" <martin.agren@gmail.com>, "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> Cc: "Git Mailing List" <git@vger.kernel.org>, "brian m. carlson" <sandals@crustytoothpaste.net>, "Josh Steadmon" <steadmon@google.com>, "Jonathan Nieder" <jrnieder@gmail.com>, "Jeff King" <peff@peff.net>, "Đoàn Trần Công Danh" <congdanhqx@gmail.com>, "Phillip Wood" <phillip.wood123@gmail.com>, "Emily Shaffer" <emilyshaffer@google.com>, sluongng@gmail.com, "Jonathan Tan" <jonathantanmy@google.com>, "Derrick Stolee" <derrickstolee@github.com>, "Derrick Stolee" <dstolee@microsoft.com> Subject: Re: [PATCH 05/11] maintenance: add commit-graph task Date: Wed, 12 Aug 2020 09:30:30 -0400 [thread overview] Message-ID: <0380dcbf-82a0-68d0-b421-9f5c5a640ab5@gmail.com> (raw) In-Reply-To: <CAN0heSrvJ4nZnMH=CPyNrZTkEXsZm2htT5kzN+RfzPXgXW=oSA@mail.gmail.com> On 8/7/2020 6:29 PM, Martin Ågren wrote: > On Thu, 6 Aug 2020 at 18:50, Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt >> index 089fa4cedc..35b0be7d40 100644 >> --- a/Documentation/git-maintenance.txt >> +++ b/Documentation/git-maintenance.txt >> @@ -35,6 +35,24 @@ run:: >> TASKS >> ----- >> >> +commit-graph:: >> + The `commit-graph` job updates the `commit-graph` files incrementally, >> + then verifies that the written data is correct. If the new layer has an >> + issue, then the chain file is removed and the `commit-graph` is >> + rewritten from scratch. >> ++ >> +The verification only checks the top layer of the `commit-graph` chain. >> +If the incremental write merged the new commits with at least one >> +existing layer, then there is potential for on-disk corruption being >> +carried forward into the new file. This will be noticed and the new >> +commit-graph file will be clean as Git reparses the commit data from >> +the object database. > > This reads somewhat awkwardly. I think what you mean is something like > "is there a risk for on-disk corruption? yes, but no: we're clever > enough to detect it and avoid it". So from a user's point of view, I > think this is too detailed. > > How about > > The verification checks the top layer of the resulting `commit-graph` chain. > This ensures that the maintenance task leaves the top layer in a shape > that matches the object database, even if it was ostensibly constructed > by "merging in" existing, incorrect layers. > > ? I don't quite like it -- it's a bit too technical -- but it describes > the end result which the user should care about -- on-disk data is > consistent and correct -- rather than how we got there. > > Perhaps: > > It is ensured that the resulting "top layer" is correct. This should > help avoid most on-disk corruption of the commit-graph and ensure > that the commit-graph matches the object database. > > Don't know whether that's entirely true though... This is my understanding. We focus on the data we just wrote to see if anything went wrong at a lower level (i.e. filesystem or RAM corruption during the write process) instead of the data "at rest". > Food for thought, perhaps. > > There's something probabilistic about this whole thing: If a low layer > is corrupt, you might "eventually" get to replace it. I suppose it could > make sense to go "verify the whole thing, drop however many top layers > we need to drop to get only correct layers, then generate a new layer > (and merge and whatnot) on top of that". But the proposed commit message > makes it fairly clear that this would have other drawbacks and that we > don't really expect corrupt layers in the first place. Right. And we want to keep the amount of work to be very small in most cases, amortizing the cost of the big merge operations across many much smaller runs. Perhaps a later change could introduce an option to drop the '--shallow' option and check the entire chain, for users willing to pay that price. Back to the point of your comments: I'm not sure this second paragraph is required at all in the documentation. The first paragraph already says: ...then verifies that the written data is correct. This "written data" _is_ the top layer of the chain. There is probably no reason to dig deeper into _why_ we do this in this user-facing documentation. So, I propose just deleting this paragraph. What do you think? Thanks for keeping a close eye on documentation changes! I updated my local branch to include the '+' problem from your other reply. Thanks, -Stolee
next prev parent reply other threads:[~2020-08-12 13:30 UTC|newest] Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-06 15:48 [PATCH 00/11] Maintenance I: Command, gc and commit-graph tasks Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 01/11] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget 2020-08-07 22:16 ` Martin Ågren 2020-08-12 21:03 ` Jonathan Nieder 2020-08-12 22:07 ` Junio C Hamano 2020-08-12 22:50 ` Jonathan Nieder 2020-08-14 1:05 ` Derrick Stolee 2020-08-06 15:48 ` [PATCH 02/11] maintenance: add --quiet option Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 03/11] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 04/11] maintenance: initialize task array Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 05/11] maintenance: add commit-graph task Derrick Stolee via GitGitGadget 2020-08-07 22:29 ` Martin Ågren 2020-08-12 13:30 ` Derrick Stolee [this message] 2020-08-14 12:23 ` Martin Ågren 2020-08-06 15:48 ` [PATCH 06/11] maintenance: add --task option Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 07/11] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 08/11] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 09/11] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 10/11] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget 2020-08-06 15:48 ` [PATCH 11/11] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget 2020-08-18 14:22 ` [PATCH v2 00/11] Maintenance I: Command, gc and commit-graph tasks Derrick Stolee via GitGitGadget 2020-08-18 14:22 ` [PATCH v2 01/11] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget 2020-08-18 14:22 ` [PATCH v2 02/11] maintenance: add --quiet option Derrick Stolee via GitGitGadget 2020-08-18 14:23 ` [PATCH v2 03/11] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget 2020-08-18 14:23 ` [PATCH v2 04/11] maintenance: initialize task array Derrick Stolee via GitGitGadget 2020-08-18 23:46 ` Jonathan Tan 2020-08-18 14:23 ` [PATCH v2 05/11] maintenance: add commit-graph task Derrick Stolee via GitGitGadget 2020-08-18 23:51 ` Jonathan Tan 2020-08-19 15:04 ` Derrick Stolee 2020-08-19 17:43 ` Jonathan Tan 2020-08-18 14:23 ` [PATCH v2 06/11] maintenance: add --task option Derrick Stolee via GitGitGadget 2020-08-19 0:00 ` Jonathan Tan 2020-08-19 0:36 ` Junio C Hamano 2020-08-19 15:09 ` Derrick Stolee 2020-08-19 17:35 ` Jonathan Tan 2020-08-18 14:23 ` [PATCH v2 07/11] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget 2020-08-19 0:04 ` Jonathan Tan 2020-08-19 15:10 ` Derrick Stolee 2020-08-18 14:23 ` [PATCH v2 08/11] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget 2020-08-18 14:23 ` [PATCH v2 09/11] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget 2020-08-18 14:23 ` [PATCH v2 10/11] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget 2020-08-19 0:09 ` Jonathan Tan 2020-08-19 15:15 ` Derrick Stolee 2020-08-18 14:23 ` [PATCH v2 11/11] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget 2020-08-19 0:11 ` Jonathan Tan 2020-08-18 20:18 ` [PATCH v2 00/11] Maintenance I: Command, gc and commit-graph tasks Junio C Hamano 2020-08-19 14:51 ` Derrick Stolee 2020-08-25 18:33 ` [PATCH v3 " Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 01/11] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 02/11] maintenance: add --quiet option Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 03/11] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 04/11] maintenance: initialize task array Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 05/11] maintenance: add commit-graph task Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 06/11] maintenance: add --task option Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 07/11] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget 2020-08-26 23:02 ` Jonathan Tan 2020-08-25 18:33 ` [PATCH v3 08/11] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 09/11] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget 2020-08-25 18:33 ` [PATCH v3 10/11] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget 2020-08-26 23:02 ` Jonathan Tan 2020-08-26 23:56 ` Junio C Hamano 2020-08-25 18:33 ` [PATCH v3 11/11] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 00/11] Maintenance I: Command, gc and commit-graph tasks Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 01/11] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 02/11] maintenance: add --quiet option Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 03/11] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 04/11] maintenance: initialize task array Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 05/11] maintenance: add commit-graph task Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 06/11] maintenance: add --task option Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 07/11] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 08/11] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 09/11] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 10/11] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget 2020-09-04 13:09 ` [PATCH v4 11/11] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 00/11] Maintenance I: Command, gc and commit-graph tasks Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 01/11] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 02/11] maintenance: add --quiet option Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 03/11] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 04/11] maintenance: initialize task array Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 05/11] maintenance: add commit-graph task Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 06/11] maintenance: add --task option Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 07/11] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget 2020-09-21 13:36 ` Ævar Arnfjörð Bjarmason 2020-09-21 13:43 ` Derrick Stolee 2020-09-21 19:29 ` Junio C Hamano 2020-09-17 18:11 ` [PATCH v5 08/11] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 09/11] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 10/11] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget 2020-09-17 18:11 ` [PATCH v5 11/11] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget 2020-09-17 18:35 ` [PATCH v5 00/11] Maintenance I: Command, gc and commit-graph tasks Junio C Hamano 2020-09-18 13:14 ` Johannes Schindelin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0380dcbf-82a0-68d0-b421-9f5c5a640ab5@gmail.com \ --to=stolee@gmail.com \ --cc=congdanhqx@gmail.com \ --cc=derrickstolee@github.com \ --cc=dstolee@microsoft.com \ --cc=emilyshaffer@google.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=jonathantanmy@google.com \ --cc=jrnieder@gmail.com \ --cc=martin.agren@gmail.com \ --cc=peff@peff.net \ --cc=phillip.wood123@gmail.com \ --cc=sandals@crustytoothpaste.net \ --cc=sluongng@gmail.com \ --cc=steadmon@google.com \ --subject='Re: [PATCH 05/11] maintenance: add commit-graph task' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).