> This conflicts with [3], so please keep that in mind. > > Maybe [3] should be adjusted to assume this patch, because that change > is mostly about disabling the batch download when no renames are required. > As Peff said [2] the full rename detection trigger is "overly broad". > > However, the changed-path Bloom filters are an excellent test for this > patch, as computing them in a partial clone will trigger downloading all > blobs without [3]. > > [3] https://lore.kernel.org/git/55824cda89c1dca7756c8c2d831d6e115f4a9ddb.1585528298.git.gitgitgadget@xxxxxxxxx/T/#u > > > [1] https://lore.kernel.org/git/20200128213508.31661-1-jonathantanmy@xxxxxxxxxx/ > > [2] https://lore.kernel.org/git/20200130055136.GA2184413@xxxxxxxxxxxxxxxxxxxxxxx/ Thanks for the pointer. Yes, I think that [3] should be adjusted to assume this patch. > > + for (i = 0; i < rename_dst_nr; i++) { > > + if (rename_dst[i].pair) > > + continue; /* already found exact match */ > > + add_if_missing(options->repo, &to_fetch, rename_dst[i].two); > > Could this be reversed instead to avoid the "continue"? Hmm...I prefer the "return early" approach, but can change it if others prefer to avoid the "continue" here. > if (!rename_dst[i].pair) > add_if_missing(options->repo, &to_fetch, rename_dst[i].two); > > > + } > > + for (i = 0; i < rename_src_nr; i++) > > + add_if_missing(options->repo, &to_fetch, rename_src[i].p->one); > > Does this not have the equivalent "rename_src[i].pair" logic for exact > matches? Thanks for the catch. There's no "pair" in rename_src[i], but the equivalent is "if (skip_unmodified && diff_unmodified_pair(rename_src[i].p))", which you can see in the "for" loop later in the function. I've added this. > > + if (to_fetch.nr) > > + promisor_remote_get_direct(options->repo, > > + to_fetch.oid, to_fetch.nr); > > Perhaps promisor_remote_get_direct() could have the check for > nr == 0 to exit early instead of putting that upon all the > callers? The 2nd param is a pointer to an array, and I think it would be strange to pass a pointer to a 0-size region of memory anywhere, so I'll leave it as it is. > > +test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' ' > > + test_when_finished "rm -rf server client trace" && > > + > > + test_create_repo server && > > + echo a >server/a && > > + printf "b\nb\nb\nb\nb\n" >server/b && > > + git -C server add a b && > > + git -C server commit -m x && > > > + rm server/b && > > + printf "b\nb\nb\nb\nb\n" >server/c && > > Would "mv server/b server/c" make it more clear that > this is an exact rename? True. Will do. Thanks for the review.
