On Tue, Jan 17, 2012 at 06:50:30AM +0100, mhagger@xxxxxxxxxxxx wrote: > When cloning, write_remote_refs() creates local packed refs from the > refs read from the remote repository. It does this by creating extra > refs for the references then calling pack_refs() to bake the extra > refs into the packed-refs file, then calling clear_extra_refs(). > > This is silly and relies on the kludgy extra_refs mechanism, which I > want to get rid of. Instead, add a function call add_packed_ref() to > the refs API, and use it to create packed refs (in the memory cache) > directly. Then call pack_refs() as before to write the packed-refs > file. I certainly approve of the goal. > Because the new add_packed_ref() function allows references (perhaps > many of them) to be added to an existing ref_array, it would be > inefficient to re-sort the list after every addition. So instead, > append new entries to the end of the ref_array and note that the array > is unsorted. Then, before the ref_array is used, check if it is > unsorted and sort it if necessary. Makes sense. > A side effect of this change is that the new packed references are > left in the in-memory packed reference cache after the return from > write_remote_refs() (whereas previously, the refs were stored as > temporary extra refs that were purged before return from the > function). I can't see any place in the following code where this > would make a difference, but there is quite a bit of code there so it > is hard to audit. Confirmation that this is OK would be welcome. Actually, I think you may be fixing an extremely minor bug with this. If later code in clone tries to resolve one of the refs in refs/remotes/<origin>/, the current code will see that it doesn't exist as a ref file (because we wrote it packed) and call get_packed_ref. That checks the cached refs list, which will claim that did_packed is true, but the "packed" array will be empty. Which is wrong; we _do_ have that ref, and our cache is stale. After writing the packed list, the current code probably ought to be calling invalidate_ref_cache(). It was only the fact that most of the remaining code didn't care that this wasn't a bug in the first place. Your code makes more sense, in that it will keep the packed_refs list up to date, and later calls to resolve_ref will properly find those refs. The only place where I can detect a change in behavior is in the reflog creation. When we write the refs/remotes/<origin>/HEAD ref, we call create_symref, which in turn will decide whether to write a reflog entry or not based on whether the pointed-to ref exists (because if we are making a symref to something that doesn't exist, we have no sha1 to write in the reflog entry). So before, we got no reflog for refs/remotes/<origin>/HEAD (because we erroneously thought that refs/remotes/origin/master (or whatever) did not exist). With your patches, the reflog entry is created. I doubt anyone ever noticed, but now that code is at least working as intended. > Michael Haggerty (4): > pack_refs(): remove redundant check > ref_array: keep track of whether references are sorted > add_packed_ref(): new function in the refs API. > write_remote_refs(): create packed (rather than extra) refs I won't respond to each patch individually. All of them looked good to me. Thanks for a very pleasant read. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html