On Tue, 08 Aug 2017 13:05:05 -0700 Junio C Hamano <gitster@xxxxxxxxx> wrote: > Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > > > While investigating annotating packfiles and loose objects to support > > connectivity checks in partial clones [1], I decided to make the effort > > to separate packfile-related code from sha1_file.c into its own file, to > > make it easier to both code such changes and review them. Here is the > > beginning of those efforts. > > > > Is this something worth doing, and if yes, is this in the right > > direction? > > Overall I think it is a good idea to slim down sha1_file.c *if* we > can keep the exposed surface area small enough. What do you mean by "keep the exposed surface area small enough"? If you mean the total number of exposed functions in sha1_file and pack (once everything is done), I think it will be almost the same as that currently in sha1_file. find_pack_entry() and has_packed_and_bad() (not yet in this patch set) might need to be changed from static to global, but those are the only 2 I can think of. Anyway, I'll report the functions that need to be changed from static to global at the end. During this patch set, there might be some functions that need to be temporarily made global, but those are reverted to static in the end. > I wonder if the names "pack.[ch]" communicate well that these are > "object access layer that is about reading from packfiles". The > writer side is called "pack-objects.[ch]". This file will end up being slightly broader than reading from packfiles - in particular, things like pack_report() (reporting some statistics not only on the in-repo packfiles themselves) and parse_pack_index() (which parses an idx file obtained through http) are there too. Hence the generic name, but I agree that there might be a better name (or better set of names). > This may have to make some symbols that used to be private to the > "object access" layer, which was what sha1_file.c was about, global > symbols. After moving things around, do we end up exposing too many > implementation details to the world outside the "object access" > layer? I'd assume they are limited to the resulting pack.h and it > would be OK as long as nobody other than sha1_file.c and pack.c > would inculde it, though. As stated above, I don't think so, but I'll make a list of the functions needing to be made global.
