On 10/10/2022 2:40 PM, Junio C Hamano wrote: > Derrick Stolee <derrickstolee@xxxxxxxxxx> 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
