Re: [PATCH 00/16] Header cleanups

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

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