Hi Peff, On Mon, Jul 01, 2019 at 09:17:13AM -0400, Jeff King wrote: > On Mon, Jul 01, 2019 at 08:59:45AM -0400, Jeff King wrote: > > > Yes, this is weakening the ties of the feature to the transport code. > > Traditionally transport-oriented code was the only user, but it also > > used the upload-pack transport under the hood to access the alternate > > (that was changed a while ago to for-each-ref for speed). > > > > I don't think there's any functional difference in having it there, but > > it could be moved to live alongside foreach_alt_odb() in sha1-file.c. > > Looks like this hasn't quite hit 'next' yet, so perhaps we can > reorganize it as a preparatory patch. > > [1/2]: object-store.h: move for_each_alternate_ref() from transport.h > [2/2]: check_everything_connected: assume alternate ref tips are valid > > Documentation/rev-list-options.txt | 8 +++ > builtin/receive-pack.c | 1 - > connected.c | 1 + > object-store.h | 2 + > revision.c | 29 +++++++++ > sha1-file.c | 97 ++++++++++++++++++++++++++++++ > t/perf/p5600-clone-reference.sh | 27 +++++++++ > t/t5618-alternate-refs.sh | 60 ++++++++++++++++++ > transport.c | 97 ------------------------------ > transport.h | 2 - > 10 files changed, 224 insertions(+), 100 deletions(-) > create mode 100755 t/perf/p5600-clone-reference.sh > create mode 100755 t/t5618-alternate-refs.sh This looks good to me, too (and matches my recollection from our prior off-list review against GitHub's fork). One thing that I didn't catch in my initial review that I am seeing now is the ".alternate" marker. Why did you choose this? I was thinking that ".have" would make more sense since it's consistent with what's shown in the ref advertisement, but I think that actually ".alternate" is a _better_ choice: the two really do refer to different things. Either way, I don't think that it matters much: this series is already on 'next' and I think that either is fine (especially since ".have" refers to a nameless ref and ".alternate" refers to a nameless pseudo-remote). > -Peff Thanks, Taylor
