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

From: Jonathan Nieder <jrnieder@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, me@ttaylorr.com, peff@peff.net,
	gitster@pobox.com, abhishekkumar8222@gmail.com,
	Taylor Blau <ttaylorr@github.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()
Date: Tue, 2 Feb 2021 17:48:54 -0800	[thread overview]
Message-ID: <YBoBBie2t1EhcLAN@google.com> (raw)
In-Reply-To: <1dab0bf0-9a7f-370a-c807-25d67ac7a0a0@gmail.com>

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.

[...]
>> 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.

>> - 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)?

Also, should "git fsck" call "git commit-graph verify" to make the
latter more discoverable?

Thanks,
Jonathan