Re: [RFC PATCH 00/10] An attempt to move packfile funcs to its own file

Jonathan Tan [off-list ref] 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.

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 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.

Thanks.

[1] https://public-inbox.org/git/20170804145113.5ceafafa@twelve2.svl.corp.google.com/

Jonathan Tan (10):
  pack: move pack name-related functions
  pack: move static state variables
  pack: move pack_report()
  pack: move open_pack_index(), parse_pack_index()
  pack: move release_pack_memory()
  pack: move pack-closing functions
  pack: move use_pack()
  pack: move unuse_pack()
  pack: move add_packed_git()
  pack: move install_packed_git()

 Makefile                 |   1 +
 builtin/am.c             |   1 +
 builtin/clone.c          |   1 +
 builtin/count-objects.c  |   1 +
 builtin/fetch.c          |   1 +
 builtin/merge.c          |   1 +
 builtin/pack-redundant.c |   1 +
 cache.h                  |  45 ----
 connected.c              |   1 +
 git-compat-util.h        |   2 -
 pack.c                   | 669 +++++++++++++++++++++++++++++++++++++++++++++++
 pack.h                   |  51 ++++
 sha1_file.c              | 667 ----------------------------------------------
 sha1_name.c              |   1 +
 streaming.c              |   1 +
 15 files changed, 730 insertions(+), 714 deletions(-)
 create mode 100644 pack.c

On Tue, 08 Aug 2017 13:05:05 -0700
Junio C Hamano [off-list ref] wrote:

Jonathan Tan [off-list ref] writes:

quoted

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.