static-check hits

static-check hits

[Ramsay Jones]

Hi Taylor,

Just a quick note about new hits from my 'static-check.pl' script
caused by the 'tb/cruft-packs' branch. This script notes any symbols
that are not referenced outside the defining compilation unit.
(So they could be declared static in that compilation unit).
Comparing the current 'next' and 'seen' branches:

  $ diff nsc ssc
  ...
  62a63,64
  > pack-mtimes.o	- pack_has_mtimes
  > packfile.o	- close_pack_mtimes
  ...
  $ 

This is not necessarily a problem, of course, if you have patches/plans
to add callers in the future (or that they simply 'round out' an API).
I haven't looked (so can't comment), beyond:

  $ git grep -n pack_has_mtimes
  pack-mtimes.c:14:int pack_has_mtimes(struct packed_git *p)
  pack-mtimes.h:11:int pack_has_mtimes(struct packed_git *p);
  $ git grep -n close_pack_mtimes
  packfile.c:336:void close_pack_mtimes(struct packed_git *p) {
  packfile.c:350: close_pack_mtimes(p);
  packfile.h:94:void close_pack_mtimes(struct packed_git *p);
  $ 

Note that 'pack_has_mtimes()' has no callers at all. Also, the function
definition of 'close_pack_mtimes()' has the opening { of the body
on the function header line, rather than by itself on the following line.

Just an FYI.

ATB,
Ramsay Jones

Re: static-check hits

[Taylor Blau]

On Sat, Dec 04, 2021 at 02:59:15PM +0000, Ramsay Jones wrote:
> Hi Taylor,
>
> Just a quick note about new hits from my 'static-check.pl' script
> caused by the 'tb/cruft-packs' branch. This script notes any symbols
> that are not referenced outside the defining compilation unit.
> (So they could be declared static in that compilation unit).
> Comparing the current 'next' and 'seen' branches:
>
>   $ diff nsc ssc
>   ...
>   62a63,64
>   > pack-mtimes.o	- pack_has_mtimes
>   > packfile.o	- close_pack_mtimes
>   ...
>   $
>
> This is not necessarily a problem, of course, if you have patches/plans
> to add callers in the future (or that they simply 'round out' an API).
> I haven't looked (so can't comment), beyond:

Thanks very much for pointing both of these out. Removing
pack_has_mtimes() entirely is fine with me. I was surprised that it was
unused, since I thought the code setting `is_cruft = 1` in
`add_packed_git()` would have been a potential caller, but that spot
just constructs the path itself and checks the result access()-ing it.

Similarly on close_pack_mtimes(): that was definitely intended to round
out the API (along with close_pack_revindex()), but isn't necessary
outside of packfile.c's compilation unit. We could probably apply the
same treatment to close_pack_revindex(), but I'll pursue that as a
separate matter.

> Also, the function definition of 'close_pack_mtimes()' has the opening
> { of the body on the function header line, rather than by itself on
> the following line.

Copied over from close_pack_revindex(), but fixed. Thanks!

Thanks,
Taylor