git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()

From
Derrick Stolee <stolee@gmail.com>
Date
Feb 3, 2021, 03:07 UTC
Message-ID
<6dc1520f-8130-75e1-6617-67b54cb03933@gmail.com>
In-Reply-To
<YBoBBie2t1EhcLAN@google.com>
On 2/2/2021 8:48 PM, Jonathan Nieder wrote:
Show 24 quoted lines
> Hi,
> 
> Derrick Stolee wrote:
>> On 2/2/2021 8:08 PM, Jonathan Nieder wrote:
> 
>>> At Google, we're running into a commit-graph issue that appears to
>>> have also arrived as part of this last week's rollout.  This one is a
>>> bit worse --- it is reproducible for affected users and stops them
>>> from being able to do day-to-day development:
>>
>> You're shipping 'next' widely? I appreciate the extra eyes on
>> early bits, so we can find more issues and get them resolved.
> 
> Yes.  Changes in 'next' have already gotten all the vetting via code
> review that they're going to get; the difference between changes in
> 'next' and 'master' is that the latter have had some production
> exposure among users of 'next' with the ability to get help from a
> local expert, roll back quickly when there's a problem, and so on.  I
> recommend that anyone with an installation with that ability use
> 'next', to improve the quality of code that ultimately is released
> from 'master'.
> 
> It also helps us get the chance to use our experience to affect the
> direction of a topic before it's too late.

This is a good practice. It's also how I found the issues fixed in this series, but that's because I install it locally for my own extra additional testing before shipping it to users.

Show 31 quoted lines
> [...]
>>> We have some examples of repositories that were corrupted this way,
>>> but we didn't catch them in the act of corruption --- it started
>>> happening to several users with this release so we immediately rolled
>>> back.
>>
>> It is definitely related to the split commit-graph during the
>> upgrade scenario. Your verify output shows that you are using
>> the --split option heavily (possibly with fetch.writeCommitGraph?
>> or are you using 'git maintenance run --task=commit-graph'?)
> 
> Yep, the splits come from fetch.writeCommitGraph.
> 
> [...]
>>> - what is the recommended way to recover from this state?  "git fsck"
>>>   shows the repositories to have no problems.  "git help commit-graph"
>>>   doesn't show a command for users to use; is
>>>   `rm -fr .git/objects/info/commit-graphs/` the recommended recovery
>>>   command?
>>
>> That, followed by `git commit-graph write --reachable [--changed-paths]`
>> depending on what they want.
> 
> Can we package this as something more user-friendly?  E.g.
> 
> 	git commit-graph clear
> 	git commit-graph write --reachable
> 
> If that makes sense to you, I'm happy to send a patch (or to review
> one if someone else gets to it first).  I'm mostly asking to find out
> whether this matches your idea of what the UI should be like.

'clear' is probably fine. I was thinking it might be good to have an option to the 'write' subcommand to clear the existing data, but it's probably better as separate steps.

Show 10 quoted lines
>>> - is there configuration or a patch we can roll out to help affected
>>>   users recover from this state?
>>
>> If you are willing, then take v2 of this series and follow through by
>> clearing the commit-graph files of affected users. Note that you can
>> be proactive using `git commit-graph verify` to see who needs rewrites.
> 
> Does this mean we should change the BUG error message to help affected
> users discover how they can recover for themselves (for example, using
> commands like the above)?

It _is_ a bug that led to this, but it's more about incorrect commit-graph data which could be caused by anything. Better to have a better message such as "your commit-graph file is probably corrupt".

> Also, should "git fsck" call "git commit-graph verify" to make the
> latter more discoverable?
Yes. I thought it did, but I must be incorrect.

Thanks, -Stolee

Previous: Jonathan NiederNext: Taylor Blau
Message 22 of 37 in “Generation Number v2: Fix a tricky split graph bug”
  1. 0/5 Generation Number v2: Fix a tricky split graph bugDerrick Stolee via GitGitGadget, Feb 1, 2021
  2. 1/5 commit-graph: use repo_parse_commitDerrick Stolee via GitGitGadget, Feb 1, 2021
  3. Taylor BlauFeb 1, 2021
  4. 3/5 commit-graph: validate layers for generation dataDerrick Stolee via GitGitGadget, Feb 1, 2021
  5. Taylor BlauFeb 1, 2021
  6. Derrick StoleeFeb 1, 2021
  7. 4/5 commit-graph: be extra careful about mixed generationsDerrick Stolee via GitGitGadget, Feb 1, 2021
  8. Taylor BlauFeb 1, 2021
  9. Derrick StoleeFeb 1, 2021
  10. Junio C HamanoFeb 1, 2021
  11. 5/5 commit-graph: prepare commit graphDerrick Stolee via GitGitGadget, Feb 1, 2021
  12. Taylor BlauFeb 1, 2021
  13. 2/5 commit-graph: always parse before commit_graph_data_at()Derrick Stolee via GitGitGadget, Feb 1, 2021
  14. Junio C HamanoFeb 1, 2021
  15. 0/6 Generation Number v2: Fix a tricky split graph bugDerrick Stolee via GitGitGadget, Feb 2, 2021
  16. 1/6 commit-graph: use repo_parse_commitDerrick Stolee via GitGitGadget, Feb 2, 2021
  17. 3/6 commit-graph: validate layers for generation dataDerrick Stolee via GitGitGadget, Feb 2, 2021
  18. 2/6 commit-graph: always parse before commit_graph_data_at()Derrick Stolee via GitGitGadget, Feb 2, 2021
  19. Jonathan NiederFeb 3, 2021
  20. Derrick StoleeFeb 3, 2021
  21. Jonathan NiederFeb 3, 2021
  22. Derrick StoleeFeb 3, 2021
  23. Taylor BlauFeb 3, 2021
  24. Eric SunshineFeb 3, 2021
  25. Junio C HamanoFeb 3, 2021
  26. Taylor BlauFeb 3, 2021
  27. Junio C HamanoFeb 3, 2021
  28. Derrick StoleeFeb 3, 2021
  29. SZEDER GáborFeb 7, 2021
  30. Junio C HamanoFeb 7, 2021
  31. Derrick StoleeFeb 8, 2021
  32. Junio C HamanoFeb 8, 2021
  33. 4/6 commit-graph: compute generations separatelyDerrick Stolee via GitGitGadget, Feb 2, 2021
  34. 6/6 commit-graph: prepare commit graphDerrick Stolee via GitGitGadget, Feb 2, 2021
  35. 5/6 commit-graph: be extra careful about mixed generationsDerrick Stolee via GitGitGadget, Feb 2, 2021
  36. Taylor BlauFeb 2, 2021
  37. Abhishek KumarFeb 11, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.