Re: [PATCH 00/16] Header cleanups

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Mar 19 2023, Elijah Newren via GitGitGadget wrote:

> This series picks up where en/header-cleanups leaves off and does more
> header cleanups, trying to reduce the number of files depending on cache.h.
> (There are still more that could be done, but again, this is a good chunk
> for now.)

> Elijah Newren (16):
>   treewide: remove unnecessary cache.h inclusion from a few headers
>   treewide: be explicit about dependence on gettext.h
>   treewide: remove unnecessary inclusion of gettext.h
>   treewide: remove unnecessary cache.h inclusion from several sources

I couldn't seem to square what you were doing in 2/16 with what 4/16
does, but I think the "issue" is that the commit message for 4/6 doesn't
match at least one change it's making, i.e.:

	By making those files explicitly include gettext.h, we can
	already drop the include of cache.h in these files.

But in fact compat/linux/procinfo.c at least (I didn't do more than skim
the others) isn't like the rest, in that it doesn't need gettext.h at
all.

I also find the ordering hard to follow, but it's ultimately correct in
the cases I looked at. E.g. for ref-filter.c your 1/16 does:

	-#include "git-compat-util.h"
	+#include "cache.h"

Then 2/16 does:

	+#include "gettext.h"

So I wondered, wait a minute, didn't we just end up over-including
cache.h because we needed gettext.h, which the later commit adds?

But no, that's not the case, because as 1/16 notes ("on things from
cache.h[...]") we need *other* things from cache.h, so this is
ultimately correct.

Still, given that, I for one would have found this easier to follow if
commits like 2/16 came before the 1/16, i.e. let's first be explicit
about e.g. gettext.h, and then change git-compat-util.h inclusions to
cache.h, to clearly note in the progression that the two are distinct.

But this is also fine with me if you don't agree, or can't be bothered
to re-roll it.

As a more general note on the direction here: I have some old WIP
patches to do similar split-ups of cache.h, but in doing those I was
trying to first identify cases where we had a function in cache.h that
was used by the low tens of our ~500 *.c source files.

E.g. advice.h is such a case[1], ditto wildmatch.h. Then we have case
like ws.c[3], base85.c[4] and pager.c[5] where there's no corresponding
header, but we should probably create one (which those WIP commits of
mine do).

Whereas this approach feels like the opposite of that, i.e. at the end
of this series we're including gettext.h in more than 250 files.

Stepping back a bit, I think our use of cache.h falls into a few broad
categories:

 A. Things to do with the index (we should probably create an index.h
    for those).
 B. Used almost everywhere, i.e. "cache.h" can be though of as "git.h",
    or "common-utils.h" or whatever.
 C. Things used almost nowhere, or only a handful of places,
    e.g. wildmatch.h and others noted above.

Part of this series is a frontal assault on the "B" part of that. I
think if we're going to include gettext.h explicitly everywhere we're
probably saying by extension that no such thing as a "common header"
should exist.

Which I'd be fine with, I just personally never thought it was much of a
practical problem, i.e. to have the gettext.h's in our tree included
"everywhere", ditto "strbuf.h", and even "enum object_file" and other
"mostly everyone wants these".

Whereas it is rather annoying that we over-include things in cache.h, or
even have cache.h include other headers, as often minor changes to
related libraries result in a full re-build.

But maybe "B" isn't sustainable at all, and I'm just fooling myself
thinking we could have such a thing.

A nice thing I hadn't considered is that after your topic e.g. removing
the gettext.h inclusion from parse-options.h will compile *most* of git,
but we'll fail e.g. in:

	t/helper/test-parse-pathspec-file.c: In function ‘cmd__parse_pathspec_file’:
	./parse-options.h:209:40: error: implicit declaration of function ‘N_’ [-Werror=implicit-function-declaration]
	  209 |                                        N_("file"), (h) }
	

So as we don't want gettext in t/helper/* it's nice to get *an* error
about that.

So, in the end I think I've convinced myself that even the "B" in that
"A..C" is a bad idea.

1. https://github.com/avar/git/commit/17366c2b7b4
2. https://github.com/avar/git/commit/ffb226fb6e6
3. https://github.com/avar/git/commit/1d023e554bf
4. https://github.com/avar/git/commit/e7d5b511090
5. https://github.com/avar/git/commit/0054eea1a1a