Re: [PATCH] doc: clarify the wording on <git-compat-util.h> requirement

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2024, 22:54 UTC
Message-ID: <xmqq7citlbh9.fsf@gitster.g>
In-Reply-To: <CAO_smVg4E4bENU18tiv8xnscnk46i=GW=Kq+xKvO1Nf7qCGy_A@mail.gmail.com>

Kyle Lippincott <spectral@google.com> writes:

Show 9 quoted lines

> This "In addition" ties to the "are allowed to" 19 lines below, which
> was confusing for me until I intentionally ignored the "In addition",
> continued reading, and finally caught the other piece of it. Perhaps
> either `Exceptions:`, or something like `The following cases are
> allowed to assume that their header file includes "git-compat-util.h",
> and they do not have to include "git-compat-util.h" themselves:` -- I
> have a slight preference for the latter form, but I worry that the
> "These headers must be the first header file to be "#include"d in
> them" bit will be missed.

I'd appreciate people to help figuring out what the preamble should read like to make it easier to follow.

Show 5 quoted lines

> ... I don't know
> if we need the reasoning why you'd #include these files in the bullets
> below, which is why I didn't include it here. I'm assuming there's a
> concern about implying that builtin/foo.c should include builtin.h
> instead of git-compat-util.h (even if foo.c doesn't use cmd_foo()?).

It is more about helping folks new to the codebase understand the reasoning behind the convention. As whoever implements "git foo" as a built-in command is supposed to

 - declare cmd_foo() in builtin.h
 - add builtin/foo.c, define cmd_foo() there, and include builtin.h
 - add "foo" and "cmd_foo" to git.c:commands[].

it is natural to expect any and all builtin/foo.c to include builtin.h (hence it makes it convenient to allow an exception by including compat-util in builtin.h to give everybody in builtin/ indirect access to compat-util).

But those who are not yet familar with the structure of the system may not understand why it is natural. So, addition of "why is this header allowed to be a substitute for which source files? Because these source files are supposed to be including that header anyway" is an important part of this patch.

Thanks.