Re: [PATCH v4 08/11] bundle: add flags to verify_bundle(), skip walk

From: Derrick Stolee <derrickstolee@github.com>
Date: Oct 11, 2022, 19:04 UTC
Message-ID: <d85704ef-97e8-57aa-9711-01d00b74d36e@github.com>
In-Reply-To: <xmqqk05734kr.fsf@gitster.g>

On 10/10/2022 2:40 PM, Junio C Hamano wrote:

Show 32 quoted lines

> Derrick Stolee <derrickstolee@github.com> writes:
> 
>> I've been going over the refs code multiple times today trying to
>> fix this "real" culprit, with no luck. I can share this interesting
>> point:
>>
>>  * The initial loop over the bundles tries to apply each, but the
>>    prerequisite objects are not present so we never reach the revision
>>    walk. A refs/bundle/* ref is added via update_ref().
>>
>>  * The second loop over the bundles tries to apply each, but the only
>>    bundle with its prerequisites present also finds the commits as
>>    reachable (this must be where the loose ref cache is populated).
>>    Then, a refs/bundle/* ref is added via update_ref().
>>
>>  * The third loop over the bundles finds a bundle whose prerequisites
>>    are present, but verify_bundle() rejected it because those commits
>>    were not seen from any ref.
>>
>> Other than identifying that issue, I was unable to track down exactly
>> what is happening here or offer a fix. I had considered inserting
>> more cache frees deep in the refs code, but I wasn't sure what effect
>> that would have across the wider system.
> 
> OK.  That certainly is understandable.
> 
> As a comment in the proposed log message that BUNDLE_SKIP_REACHABLE
> bit is a band aid papering over a problem we punted in this series,
> to guide future developers, I think what you wrote is sufficient.
> We do not want them to think that skipping the check is our
> preferred longer term solution and add their own hack to keep
> skipping the check when they resolve "the real culprit".

I have discovered the real culprit, and my expectation was incorrect about the loose ref cache. The key issue was that I was looking at this loop:

	i = req_nr;
	while (i && (commit = get_revision(&revs)))
		if (commit->object.flags & PREREQ_MARK)
			i--;

and noticing that only one commit was being visited. I was not seeing the actually-important commit. But it wasn't the revision walk's fault. The loop was terminating because "i" was reaching zero!

It turns out that verify_bundles() is not clearing the PREREQ_MARK flag, so multiple runs would incorrectly hit this short-circuit and terminate the walk early.

I'll replace this patch with the correct fix soon.

Thanks, -Stolee