On 10/10/2022 1:27 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: >> When unbundling from a list of bundles, Git will create refs that point >> to the tips of the latest bundle, which makes this reachability walk >> succeed, in theory. However, the loose refs cache does not get >> invalidated and hence the reachability walk fails. By disabling the >> reachability walk in the bundle URI code, we can get around this >> reachability check. > > The above makes it sound like the real culprit is that cache goes > out of sync and the presented solution is a workaround; readers are > left in suspense if the "real" solution (as opposed to a workaround) > would come in a later step or in a future series. 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. >> diff --git a/bundle-uri.c b/bundle-uri.c >> index 8a7c11c6393..ad5baabdd94 100644 >> --- a/bundle-uri.c >> +++ b/bundle-uri.c >> @@ -301,7 +301,13 @@ static int unbundle_from_file(struct repository *r, const char *file) >> if ((bundle_fd = read_bundle_header(file, &header)) < 0) >> return 1; >> >> - if ((result = unbundle(r, &header, bundle_fd, NULL))) >> + /* >> + * Skip the reachability walk here, since we will be adding >> + * a reachable ref pointing to the new tips, which will reach >> + * the prerequisite commits. >> + */ >> + if ((result = unbundle(r, &header, bundle_fd, NULL, >> + VERIFY_BUNDLE_SKIP_REACHABLE))) >> return 1; > > This is not a new problem introduced in this new round, but if we > are updating this, can we fix it to omit assignment inside if > condition? > > * result is initialized to 0. > > * when unbundle returns non-zero, it is assigned to result and the > function returns immediately, discarding whatever was assigned to > the variable. > > * if unbundle returns zero, it is assigned to result and the > control continues from here. We know result is set to 0, but > then that is what it was initialized earlier. Since we are not "trusting" the integer result of unbundle, we can definitely stop this assignment in the if. Thanks, -Stolee
