{"thread":{"id":"60985","subject":"[PATCH] doc: clarify the wording on requirement","startedAt":"2024-02-24T20:22:57Z","lastAt":"2024-02-27T05:35:40Z","messageCount":8,"participants":["Junio C Hamano","Kyle Lippincott","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489282","messageId":"xmqq4jdxmx2e.fsf@gitster.g","threadId":"60985","inReplyTo":null,"subject":"[PATCH] doc: clarify the wording on requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-24T20:22:49Z","receivedAt":"2024-02-24T20:22:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The reason why we insist including the compat-util header as the\nvery first thing is because it is our mechanism to absorb the\ndifferences across platforms, like the order in which system header\nfiles must be included, and C preprocessor feature macros that are\nneeded to trigger certain features we want out of the systems, and\ninsulate other headers and sources from such minutiae.\n\nEarlier we tried to clarify the rule in the coding guidelines\ndocument, but the wording was a bit fuzzy that can lead to\nmisinterpretations like you can include xdiff/xinclude.h only to\navoid having to include git-compat-util.h file even if you have\nnothing to do with xdiff implementation, for example. \"You do not\nhave to include more than one of these\" were also misleading and\nwould have been puzzling if you _needed_ to depend on more than one\nof these approved headers (answer: you are allowed to include them\nall if you need the declarations in them for reasons other than that\nyou want to avoid including compat-util yourself).\n\nInstead of using the phrase \"approved headers\", enumerate them as\nexceptions, each labeled with intended audiences, to avoid such\nmisinterpretations. The structure also makes it easier to add new\nexceptions, so add the description of \"t/unit-tests/test-lib.h\"\nbeing an exception only for the unit tests implementation as an\nexample.\n\nSigned-off-by: Junio C Hamano \n---\n\n * git-std-lib folks CC'ed to show them where to put their exception\n when things start to stabilize; Elijah CC'ed for his 8bff5ca0\n (treewide: ensure one of the appropriate headers is sourced\n first, 2023-02-24) and bc5c5ec0 (cache.h: remove this\n no-longer-used header, 2023-05-16).\n\n Documentation/CodingGuidelines | 30 ++++++++++++++++++++++++------\n 1 file changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\nindex 578587a471..b3443dd773 100644\n--- c/Documentation/CodingGuidelines\n+++ w/Documentation/CodingGuidelines\n@@ -446,12 +446,30 @@ For C programs:\n detail.\n \n - The first #include in C files, except in platform specific compat/\n- implementations and sha1dc/, must be either \"git-compat-util.h\" or\n- one of the approved headers that includes it first for you. (The\n- approved headers currently include \"builtin.h\",\n- \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n- \"reftable/system.h\".) You do not have to include more than one of\n- these.\n+ implementations and sha1dc/, must be \"git-compat-util.h\". In\n+ addition:\n+\n+ - the implementation of the built-in commands in the \"builtin/\"\n+ directory that include \"builtin.h\" for the cmd_foo() prototype\n+ definition\n+\n+ - the test helper programs in the \"t/helper/\" directory that include\n+ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition\n+\n+ - the xdiff implementation in the \"xdiff/\" directory that includes\n+ \"xdiff/xinclude.h\" for the xdiff machinery internals\n+\n+ - the unit test programs in \"t/unit-tests/\" directory that include\n+ \"test-lib.h\" that gives them the unit-tests framework\n+\n+ - the source files that implement reftable in the \"reftable/\"\n+ directory that include \"reftable/system.h\" for the reftable\n+ internals\n+\n+ are allowed to assume that their header file includes\n+ \"git-compat-util.h\", and they do not have to include\n+ \"git-compat-util.h\" themselves. These headers must be the first\n+ header file to be \"#include\"d in them, though.\n \n - A C file must directly include the header files that declare the\n functions and the types it uses, except for the functions and types\n"},{"id":"489294","messageId":"xmqqh6hxlcae.fsf@gitster.g","threadId":"60985","inReplyTo":"xmqq4jdxmx2e.fsf@gitster.g","subject":"[PATCH v2] doc: clarify the wording on requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-24T22:36:57Z","receivedAt":"2024-02-24T22:36:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The reason why we require the file to be the\nfirst header file to be included is because it insulates other\nheader files and source files from platform differences, like which\nsystem header files must be included in what order, and what C\npreprocessor feature macros must be defined to trigger certain\nfeatures we want out of the system.\n\nWe tried to clarify the rule in the coding guidelines document, but\nthe wording was a bit fuzzy that can lead to misinterpretations like\nyou can include only to avoid having to include\n even if you have nothing to do with the xdiff\nimplementation, for example. \"You do not have to include more than\none of these\" was also misleading and would have been puzzling if\nyou _needed_ to depend on more than one of these approved headers\n(answer: you are allowed to include them all if you need the\ndeclarations in them for reasons other than that you want to avoid\nincluding compat-util yourself).\n\nInstead of using the phrase \"approved headers\", enumerate them as\nexceptions, each labeled with its intended audiences, to avoid such\nmisinterpretations. The structure also makes it easier to add new\nexceptions, so add the description of \"t/unit-tests/test-lib.h\"\nbeing an exception only for the unit tests implementation as an\nexample.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The most notable change since the first one is that the reason\n why the requirement exists is added to the document, not just\n telling them what to do but also telling them why.\n\n Also the path to \"test-lib.h\" used in the unit-test framework has\n been spelled out relative to the root of the working tree, like\n all other header files.\n\nRange-diff:\n1: dde3a79470 ! 1: b9f3d36e9a doc: clarify the wording on requirement\n @@ Metadata\n ## Commit message ##\n doc: clarify the wording on requirement\n \n - The reason why we insist including the compat-util header as the\n - very first thing is because it is our mechanism to absorb the\n - differences across platforms, like the order in which system header\n - files must be included, and C preprocessor feature macros that are\n - needed to trigger certain features we want out of the systems, and\n - insulate other headers and sources from such minutiae.\n + The reason why we require the file to be the\n + first header file to be included is because it insulates other\n + header files and source files from platform differences, like which\n + system header files must be included in what order, and what C\n + preprocessor feature macros must be defined to trigger certain\n + features we want out of the system.\n \n - Earlier we tried to clarify the rule in the coding guidelines\n - document, but the wording was a bit fuzzy that can lead to\n - misinterpretations like you can include xdiff/xinclude.h only to\n - avoid having to include git-compat-util.h file even if you have\n - nothing to do with xdiff implementation, for example. \"You do not\n - have to include more than one of these\" were also misleading and\n - would have been puzzling if you _needed_ to depend on more than one\n - of these approved headers (answer: you are allowed to include them\n - all if you need the declarations in them for reasons other than that\n - you want to avoid including compat-util yourself).\n + We tried to clarify the rule in the coding guidelines document, but\n + the wording was a bit fuzzy that can lead to misinterpretations like\n + you can include only to avoid having to include\n + even if you have nothing to do with the xdiff\n + implementation, for example. \"You do not have to include more than\n + one of these\" was also misleading and would have been puzzling if\n + you _needed_ to depend on more than one of these approved headers\n + (answer: you are allowed to include them all if you need the\n + declarations in them for reasons other than that you want to avoid\n + including compat-util yourself).\n \n Instead of using the phrase \"approved headers\", enumerate them as\n - exceptions, each labeled with intended audiences, to avoid such\n + exceptions, each labeled with its intended audiences, to avoid such\n misinterpretations. The structure also makes it easier to add new\n exceptions, so add the description of \"t/unit-tests/test-lib.h\"\n being an exception only for the unit tests implementation as an\n @@ Documentation/CodingGuidelines: For C programs:\n - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n - \"reftable/system.h\".) You do not have to include more than one of\n - these.\n -+ implementations and sha1dc/, must be \"git-compat-util.h\". In\n -+ addition:\n ++ implementations and sha1dc/, must be \"git-compat-util.h\". This\n ++ header file insulates other header files and source files from\n ++ platform differences, like which system header files must be\n ++ included in what order, and what C preprocessor feature macros must\n ++ be defined to trigger certain features we expect out of the system.\n ++\n ++ In addition:\n +\n + - the implementation of the built-in commands in the \"builtin/\"\n + directory that include \"builtin.h\" for the cmd_foo() prototype\n @@ Documentation/CodingGuidelines: For C programs:\n + \"xdiff/xinclude.h\" for the xdiff machinery internals\n +\n + - the unit test programs in \"t/unit-tests/\" directory that include\n -+ \"test-lib.h\" that gives them the unit-tests framework\n ++ \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n ++ framework\n +\n + - the source files that implement reftable in the \"reftable/\"\n + directory that include \"reftable/system.h\" for the reftable\n + internals\n +\n -+ are allowed to assume that their header file includes\n -+ \"git-compat-util.h\", and they do not have to include\n -+ \"git-compat-util.h\" themselves. These headers must be the first\n ++ are allowed to assume that they do not have to include\n ++ \"git-compat-util.h\" themselves, as it is included as the first\n ++ '#include' in these header files. These headers must be the first\n + header file to be \"#include\"d in them, though.\n \n - A C file must directly include the header files that declare the\n\n Documentation/CodingGuidelines | 36 ++++++++++++++++++++++++++++------\n 1 file changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 578587a471..291b2898a2 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -446,12 +446,36 @@ For C programs:\n detail.\n \n - The first #include in C files, except in platform specific compat/\n- implementations and sha1dc/, must be either \"git-compat-util.h\" or\n- one of the approved headers that includes it first for you. (The\n- approved headers currently include \"builtin.h\",\n- \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n- \"reftable/system.h\".) You do not have to include more than one of\n- these.\n+ implementations and sha1dc/, must be \"git-compat-util.h\". This\n+ header file insulates other header files and source files from\n+ platform differences, like which system header files must be\n+ included in what order, and what C preprocessor feature macros must\n+ be defined to trigger certain features we expect out of the system.\n+\n+ In addition:\n+\n+ - the implementation of the built-in commands in the \"builtin/\"\n+ directory that include \"builtin.h\" for the cmd_foo() prototype\n+ definition\n+\n+ - the test helper programs in the \"t/helper/\" directory that include\n+ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition\n+\n+ - the xdiff implementation in the \"xdiff/\" directory that includes\n+ \"xdiff/xinclude.h\" for the xdiff machinery internals\n+\n+ - the unit test programs in \"t/unit-tests/\" directory that include\n+ \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n+ framework\n+\n+ - the source files that implement reftable in the \"reftable/\"\n+ directory that include \"reftable/system.h\" for the reftable\n+ internals\n+\n+ are allowed to assume that they do not have to include\n+ \"git-compat-util.h\" themselves, as it is included as the first\n+ '#include' in these header files. These headers must be the first\n+ header file to be \"#include\"d in them, though.\n \n - A C file must directly include the header files that declare the\n functions and the types it uses, except for the functions and types\n-- \n2.44.0\n\n"},{"id":"489295","messageId":"CAO_smVg4E4bENU18tiv8xnscnk46i=GW=Kq+xKvO1Nf7qCGy_A@mail.gmail.com","threadId":"60985","inReplyTo":"xmqq4jdxmx2e.fsf@gitster.g","subject":"Re: [PATCH] doc: clarify the wording on requirement","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-02-24T22:38:09Z","receivedAt":"2024-02-24T22:38:22Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Sat, Feb 24, 2024 at 12:22 PM Junio C Hamano wrote:\n>\n> The reason why we insist including the compat-util header as the\n> very first thing is because it is our mechanism to absorb the\n> differences across platforms, like the order in which system header\n> files must be included, and C preprocessor feature macros that are\n> needed to trigger certain features we want out of the systems, and\n> insulate other headers and sources from such minutiae.\n>\n> Earlier we tried to clarify the rule in the coding guidelines\n> document, but the wording was a bit fuzzy that can lead to\n> misinterpretations like you can include xdiff/xinclude.h only to\n> avoid having to include git-compat-util.h file even if you have\n> nothing to do with xdiff implementation, for example. \"You do not\n> have to include more than one of these\" were also misleading and\n> would have been puzzling if you _needed_ to depend on more than one\n> of these approved headers (answer: you are allowed to include them\n> all if you need the declarations in them for reasons other than that\n> you want to avoid including compat-util yourself).\n>\n> Instead of using the phrase \"approved headers\", enumerate them as\n> exceptions, each labeled with intended audiences, to avoid such\n> misinterpretations. The structure also makes it easier to add new\n> exceptions, so add the description of \"t/unit-tests/test-lib.h\"\n> being an exception only for the unit tests implementation as an\n> example.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * git-std-lib folks CC'ed to show them where to put their exception\n> when things start to stabilize; Elijah CC'ed for his 8bff5ca0\n> (treewide: ensure one of the appropriate headers is sourced\n> first, 2023-02-24) and bc5c5ec0 (cache.h: remove this\n> no-longer-used header, 2023-05-16).\n>\n> Documentation/CodingGuidelines | 30 ++++++++++++++++++++++++------\n> 1 file changed, 24 insertions(+), 6 deletions(-)\n>\n> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\n> index 578587a471..b3443dd773 100644\n> --- c/Documentation/CodingGuidelines\n> +++ w/Documentation/CodingGuidelines\n> @@ -446,12 +446,30 @@ For C programs:\n> detail.\n>\n> - The first #include in C files, except in platform specific compat/\n> - implementations and sha1dc/, must be either \"git-compat-util.h\" or\n> - one of the approved headers that includes it first for you. (The\n> - approved headers currently include \"builtin.h\",\n> - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n> - \"reftable/system.h\".) You do not have to include more than one of\n> - these.\n> + implementations and sha1dc/, must be \"git-compat-util.h\". In\n> + addition:\n\nThis \"In addition\" ties to the \"are allowed to\" 19 lines below, which\nwas confusing for me until I intentionally ignored the \"In addition\",\ncontinued reading, and finally caught the other piece of it. Perhaps\neither `Exceptions:`, or something like `The following cases are\nallowed to assume that their header file includes \"git-compat-util.h\",\nand they do not have to include \"git-compat-util.h\" themselves:` -- I\nhave a slight preference for the latter form, but I worry that the\n\"These headers must be the first header file to be \"#include\"d in\nthem\" bit will be missed. So maybe if we went with the latter version,\nwe change each bullet point to include that qualification. Example: -\nthe implementation of the built-in commands in the \"builtin/\"\ndirectory that include \"builtin.h\" as the first header\". I don't know\nif we need the reasoning why you'd #include these files in the bullets\nbelow, which is why I didn't include it here. I'm assuming there's a\nconcern about implying that builtin/foo.c should include builtin.h\ninstead of git-compat-util.h (even if foo.c doesn't use cmd_foo()?).\n\n> +\n> + - the implementation of the built-in commands in the \"builtin/\"\n> + directory that include \"builtin.h\" for the cmd_foo() prototype\n> + definition\n> +\n> + - the test helper programs in the \"t/helper/\" directory that include\n> + \"t/helper/test-tool.h\" for the cmd__foo() prototype definition\n> +\n> + - the xdiff implementation in the \"xdiff/\" directory that includes\n> + \"xdiff/xinclude.h\" for the xdiff machinery internals\n> +\n> + - the unit test programs in \"t/unit-tests/\" directory that include\n> + \"test-lib.h\" that gives them the unit-tests framework\n> +\n> + - the source files that implement reftable in the \"reftable/\"\n> + directory that include \"reftable/system.h\" for the reftable\n> + internals\n> +\n> + are allowed to assume that their header file includes\n> + \"git-compat-util.h\", and they do not have to include\n> + \"git-compat-util.h\" themselves. These headers must be the first\n> + header file to be \"#include\"d in them, though.\n>\n> - A C file must directly include the header files that declare the\n> functions and the types it uses, except for the functions and types\n"},{"id":"489296","messageId":"xmqq7citlbh9.fsf@gitster.g","threadId":"60985","inReplyTo":"CAO_smVg4E4bENU18tiv8xnscnk46i=GW=Kq+xKvO1Nf7qCGy_A@mail.gmail.com","subject":"Re: [PATCH] doc: clarify the wording on requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-24T22:54:26Z","receivedAt":"2024-02-24T22:54:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n> This \"In addition\" ties to the \"are allowed to\" 19 lines below, which\n> was confusing for me until I intentionally ignored the \"In addition\",\n> continued reading, and finally caught the other piece of it. Perhaps\n> either `Exceptions:`, or something like `The following cases are\n> allowed to assume that their header file includes \"git-compat-util.h\",\n> and they do not have to include \"git-compat-util.h\" themselves:` -- I\n> have a slight preference for the latter form, but I worry that the\n> \"These headers must be the first header file to be \"#include\"d in\n> them\" bit will be missed.\n\nI'd appreciate people to help figuring out what the preamble should\nread like to make it easier to follow.\n\n> ... I don't know\n> if we need the reasoning why you'd #include these files in the bullets\n> below, which is why I didn't include it here. I'm assuming there's a\n> concern about implying that builtin/foo.c should include builtin.h\n> instead of git-compat-util.h (even if foo.c doesn't use cmd_foo()?).\n\nIt is more about helping folks new to the codebase understand the\nreasoning behind the convention. As whoever implements \"git foo\" as\na built-in command is supposed to\n\n - declare cmd_foo() in builtin.h\n - add builtin/foo.c, define cmd_foo() there, and include builtin.h\n - add \"foo\" and \"cmd_foo\" to git.c:commands[].\n\nit is natural to expect any and all builtin/foo.c to include\nbuiltin.h (hence it makes it convenient to allow an exception by\nincluding compat-util in builtin.h to give everybody in builtin/\nindirect access to compat-util).\n\nBut those who are not yet familar with the structure of the system\nmay not understand why it is natural. So, addition of \"why is this\nheader allowed to be a substitute for which source files? Because\nthese source files are supposed to be including that header anyway\"\nis an important part of this patch.\n\nThanks.\n"},{"id":"489443","messageId":"xmqqle76kdpr.fsf_-_@gitster.g","threadId":"60985","inReplyTo":"xmqqh6hxlcae.fsf@gitster.g","subject":"[PATCH v3] doc: clarify the wording on requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T23:28:16Z","receivedAt":"2024-02-26T23:28:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The reason why we require the file to be the\nfirst header file to be included is because it insulates other\nheader files and source files from platform differences, like which\nsystem header files must be included in what order, and what C\npreprocessor feature macros must be defined to trigger certain\nfeatures we want out of the system.\n\nWe tried to clarify the rule in the coding guidelines document, but\nthe wording was a bit fuzzy that can lead to misinterpretations like\nyou can include only to avoid having to include\n even if you have nothing to do with the xdiff\nimplementation, for example. \"You do not have to include more than\none of these\" was also misleading and would have been puzzling if\nyou _needed_ to depend on more than one of these approved headers\n(answer: you are allowed to include them all if you need the\ndeclarations in them for reasons other than that you want to avoid\nincluding compat-util yourself).\n\nInstead of using the phrase \"approved headers\", enumerate them as\nexceptions, each labeled with its intended audiences, to avoid such\nmisinterpretations. The structure also makes it easier to add new\nexceptions, so add the description of \"t/unit-tests/test-lib.h\"\nbeing an exception only for the unit tests implementation as an\nexample.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Updated the leading phrase introducing the list of exceptions.\n I think this is now clear enough to be ready for 'next'?\n\nRange-diff:\n1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on requirement\n @@ Documentation/CodingGuidelines: For C programs:\n - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n - \"reftable/system.h\".) You do not have to include more than one of\n - these.\n -+ implementations and sha1dc/, must be \"git-compat-util.h\". This\n ++ implementations and sha1dc/, must be . This\n + header file insulates other header files and source files from\n + platform differences, like which system header files must be\n + included in what order, and what C preprocessor feature macros must\n + be defined to trigger certain features we expect out of the system.\n ++ A collorary to this is that C files should not directly include\n ++ system header files themselves.\n +\n -+ In addition:\n ++ There are some exceptions, because certain group of files that\n ++ implement an API all have to include the same header file that\n ++ defines the API and it is convenient to include \n ++ there. Namely:\n +\n + - the implementation of the built-in commands in the \"builtin/\"\n + directory that include \"builtin.h\" for the cmd_foo() prototype\n -+ definition\n ++ definition,\n +\n + - the test helper programs in the \"t/helper/\" directory that include\n -+ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition\n ++ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition,\n +\n + - the xdiff implementation in the \"xdiff/\" directory that includes\n -+ \"xdiff/xinclude.h\" for the xdiff machinery internals\n ++ \"xdiff/xinclude.h\" for the xdiff machinery internals,\n +\n + - the unit test programs in \"t/unit-tests/\" directory that include\n + \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n -+ framework\n ++ framework, and\n +\n + - the source files that implement reftable in the \"reftable/\"\n + directory that include \"reftable/system.h\" for the reftable\n -+ internals\n ++ internals,\n +\n + are allowed to assume that they do not have to include\n -+ \"git-compat-util.h\" themselves, as it is included as the first\n ++ themselves, as it is included as the first\n + '#include' in these header files. These headers must be the first\n + header file to be \"#include\"d in them, though.\n \n\n Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++-----\n 1 file changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 578587a471..806979f75b 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -446,12 +446,41 @@ For C programs:\n detail.\n \n - The first #include in C files, except in platform specific compat/\n- implementations and sha1dc/, must be either \"git-compat-util.h\" or\n- one of the approved headers that includes it first for you. (The\n- approved headers currently include \"builtin.h\",\n- \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n- \"reftable/system.h\".) You do not have to include more than one of\n- these.\n+ implementations and sha1dc/, must be . This\n+ header file insulates other header files and source files from\n+ platform differences, like which system header files must be\n+ included in what order, and what C preprocessor feature macros must\n+ be defined to trigger certain features we expect out of the system.\n+ A collorary to this is that C files should not directly include\n+ system header files themselves.\n+\n+ There are some exceptions, because certain group of files that\n+ implement an API all have to include the same header file that\n+ defines the API and it is convenient to include \n+ there. Namely:\n+\n+ - the implementation of the built-in commands in the \"builtin/\"\n+ directory that include \"builtin.h\" for the cmd_foo() prototype\n+ definition,\n+\n+ - the test helper programs in the \"t/helper/\" directory that include\n+ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition,\n+\n+ - the xdiff implementation in the \"xdiff/\" directory that includes\n+ \"xdiff/xinclude.h\" for the xdiff machinery internals,\n+\n+ - the unit test programs in \"t/unit-tests/\" directory that include\n+ \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n+ framework, and\n+\n+ - the source files that implement reftable in the \"reftable/\"\n+ directory that include \"reftable/system.h\" for the reftable\n+ internals,\n+\n+ are allowed to assume that they do not have to include\n+ themselves, as it is included as the first\n+ '#include' in these header files. These headers must be the first\n+ header file to be \"#include\"d in them, though.\n \n - A C file must directly include the header files that declare the\n functions and the types it uses, except for the functions and types\n-- \n2.44.0\n\n"},{"id":"489449","messageId":"CAO_smVgHTiKvn-erH=m3Ky67X9hHGieC7+qKL2Hymc5NAvKu2A@mail.gmail.com","threadId":"60985","inReplyTo":"xmqqle76kdpr.fsf_-_@gitster.g","subject":"Re: [PATCH v3] doc: clarify the wording on requirement","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-02-26T23:57:27Z","receivedAt":"2024-02-26T23:57:44Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Feb 26, 2024 at 3:28 PM Junio C Hamano wrote:\n>\n> The reason why we require the file to be the\n> first header file to be included is because it insulates other\n> header files and source files from platform differences, like which\n> system header files must be included in what order, and what C\n> preprocessor feature macros must be defined to trigger certain\n> features we want out of the system.\n>\n> We tried to clarify the rule in the coding guidelines document, but\n> the wording was a bit fuzzy that can lead to misinterpretations like\n> you can include only to avoid having to include\n> even if you have nothing to do with the xdiff\n> implementation, for example. \"You do not have to include more than\n> one of these\" was also misleading and would have been puzzling if\n> you _needed_ to depend on more than one of these approved headers\n> (answer: you are allowed to include them all if you need the\n> declarations in them for reasons other than that you want to avoid\n> including compat-util yourself).\n>\n> Instead of using the phrase \"approved headers\", enumerate them as\n> exceptions, each labeled with its intended audiences, to avoid such\n> misinterpretations. The structure also makes it easier to add new\n> exceptions, so add the description of \"t/unit-tests/test-lib.h\"\n> being an exception only for the unit tests implementation as an\n> example.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * Updated the leading phrase introducing the list of exceptions.\n> I think this is now clear enough to be ready for 'next'?\n>\n> Range-diff:\n> 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on requirement\n> @@ Documentation/CodingGuidelines: For C programs:\n> - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n> - \"reftable/system.h\".) You do not have to include more than one of\n> - these.\n> -+ implementations and sha1dc/, must be \"git-compat-util.h\". This\n> ++ implementations and sha1dc/, must be . This\n> + header file insulates other header files and source files from\n> + platform differences, like which system header files must be\n> + included in what order, and what C preprocessor feature macros must\n> + be defined to trigger certain features we expect out of the system.\n> ++ A collorary to this is that C files should not directly include\n> ++ system header files themselves.\n> +\n> -+ In addition:\n> ++ There are some exceptions, because certain group of files that\n> ++ implement an API all have to include the same header file that\n> ++ defines the API and it is convenient to include \n> ++ there. Namely:\n> +\n> + - the implementation of the built-in commands in the \"builtin/\"\n> + directory that include \"builtin.h\" for the cmd_foo() prototype\n> -+ definition\n> ++ definition,\n> +\n> + - the test helper programs in the \"t/helper/\" directory that include\n> -+ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition\n> ++ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition,\n> +\n> + - the xdiff implementation in the \"xdiff/\" directory that includes\n> -+ \"xdiff/xinclude.h\" for the xdiff machinery internals\n> ++ \"xdiff/xinclude.h\" for the xdiff machinery internals,\n> +\n> + - the unit test programs in \"t/unit-tests/\" directory that include\n> + \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n> -+ framework\n> ++ framework, and\n> +\n> + - the source files that implement reftable in the \"reftable/\"\n> + directory that include \"reftable/system.h\" for the reftable\n> -+ internals\n> ++ internals,\n> +\n> + are allowed to assume that they do not have to include\n> -+ \"git-compat-util.h\" themselves, as it is included as the first\n> ++ themselves, as it is included as the first\n> + '#include' in these header files. These headers must be the first\n> + header file to be \"#include\"d in them, though.\n>\n>\n> Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++-----\n> 1 file changed, 35 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 578587a471..806979f75b 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -446,12 +446,41 @@ For C programs:\n> detail.\n>\n> - The first #include in C files, except in platform specific compat/\n> - implementations and sha1dc/, must be either \"git-compat-util.h\" or\n> - one of the approved headers that includes it first for you. (The\n> - approved headers currently include \"builtin.h\",\n> - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n> - \"reftable/system.h\".) You do not have to include more than one of\n> - these.\n> + implementations and sha1dc/, must be . This\n> + header file insulates other header files and source files from\n> + platform differences, like which system header files must be\n> + included in what order, and what C preprocessor feature macros must\n> + be defined to trigger certain features we expect out of the system.\n> + A collorary to this is that C files should not directly include\n> + system header files themselves.\n> +\n> + There are some exceptions, because certain group of files that\n> + implement an API all have to include the same header file that\n> + defines the API and it is convenient to include \n> + there. Namely:\n> +\n> + - the implementation of the built-in commands in the \"builtin/\"\n> + directory that include \"builtin.h\" for the cmd_foo() prototype\n> + definition,\n> +\n> + - the test helper programs in the \"t/helper/\" directory that include\n> + \"t/helper/test-tool.h\" for the cmd__foo() prototype definition,\n> +\n> + - the xdiff implementation in the \"xdiff/\" directory that includes\n> + \"xdiff/xinclude.h\" for the xdiff machinery internals,\n> +\n> + - the unit test programs in \"t/unit-tests/\" directory that include\n> + \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n> + framework, and\n> +\n> + - the source files that implement reftable in the \"reftable/\"\n> + directory that include \"reftable/system.h\" for the reftable\n> + internals,\n> +\n> + are allowed to assume that they do not have to include\n> + themselves, as it is included as the first\n> + '#include' in these header files. These headers must be the first\n> + header file to be \"#include\"d in them, though.\n>\n> - A C file must directly include the header files that declare the\n> functions and the types it uses, except for the functions and types\n> --\n> 2.44.0\n>\n\nLooks good\n"},{"id":"489457","messageId":"CABPp-BHbuvLSSr2186XqiHFQD=NUYess5zdwG4bqY6FzQfG8oA@mail.gmail.com","threadId":"60985","inReplyTo":"xmqqle76kdpr.fsf_-_@gitster.g","subject":"Re: [PATCH v3] doc: clarify the wording on requirement","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-02-27T04:25:19Z","receivedAt":"2024-02-27T04:25:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 26, 2024 at 3:28 PM Junio C Hamano wrote:\n>\n> The reason why we require the file to be the\n> first header file to be included is because it insulates other\n> header files and source files from platform differences, like which\n> system header files must be included in what order, and what C\n> preprocessor feature macros must be defined to trigger certain\n> features we want out of the system.\n>\n> We tried to clarify the rule in the coding guidelines document, but\n> the wording was a bit fuzzy that can lead to misinterpretations like\n> you can include only to avoid having to include\n> even if you have nothing to do with the xdiff\n> implementation, for example. \"You do not have to include more than\n> one of these\" was also misleading and would have been puzzling if\n> you _needed_ to depend on more than one of these approved headers\n> (answer: you are allowed to include them all if you need the\n> declarations in them for reasons other than that you want to avoid\n> including compat-util yourself).\n>\n> Instead of using the phrase \"approved headers\", enumerate them as\n> exceptions, each labeled with its intended audiences, to avoid such\n> misinterpretations. The structure also makes it easier to add new\n> exceptions, so add the description of \"t/unit-tests/test-lib.h\"\n> being an exception only for the unit tests implementation as an\n> example.\n\nMakes sense.\n\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * Updated the leading phrase introducing the list of exceptions.\n> I think this is now clear enough to be ready for 'next'?\n>\n> Range-diff:\n> 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on requirement\n> @@ Documentation/CodingGuidelines: For C programs:\n> - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n> - \"reftable/system.h\".) You do not have to include more than one of\n> - these.\n> -+ implementations and sha1dc/, must be \"git-compat-util.h\". This\n> ++ implementations and sha1dc/, must be . This\n> + header file insulates other header files and source files from\n> + platform differences, like which system header files must be\n> + included in what order, and what C preprocessor feature macros must\n> + be defined to trigger certain features we expect out of the system.\n> ++ A collorary to this is that C files should not directly include\n> ++ system header files themselves.\n> +\n> -+ In addition:\n> ++ There are some exceptions, because certain group of files that\n> ++ implement an API all have to include the same header file that\n> ++ defines the API and it is convenient to include \n> ++ there. Namely:\n> +\n> + - the implementation of the built-in commands in the \"builtin/\"\n> + directory that include \"builtin.h\" for the cmd_foo() prototype\n> -+ definition\n> ++ definition,\n> +\n> + - the test helper programs in the \"t/helper/\" directory that include\n> -+ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition\n> ++ \"t/helper/test-tool.h\" for the cmd__foo() prototype definition,\n> +\n> + - the xdiff implementation in the \"xdiff/\" directory that includes\n> -+ \"xdiff/xinclude.h\" for the xdiff machinery internals\n> ++ \"xdiff/xinclude.h\" for the xdiff machinery internals,\n> +\n> + - the unit test programs in \"t/unit-tests/\" directory that include\n> + \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n> -+ framework\n> ++ framework, and\n> +\n> + - the source files that implement reftable in the \"reftable/\"\n> + directory that include \"reftable/system.h\" for the reftable\n> -+ internals\n> ++ internals,\n> +\n> + are allowed to assume that they do not have to include\n> -+ \"git-compat-util.h\" themselves, as it is included as the first\n> ++ themselves, as it is included as the first\n> + '#include' in these header files. These headers must be the first\n> + header file to be \"#include\"d in them, though.\n>\n>\n> Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++-----\n> 1 file changed, 35 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 578587a471..806979f75b 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -446,12 +446,41 @@ For C programs:\n> detail.\n>\n> - The first #include in C files, except in platform specific compat/\n> - implementations and sha1dc/, must be either \"git-compat-util.h\" or\n> - one of the approved headers that includes it first for you. (The\n> - approved headers currently include \"builtin.h\",\n> - \"t/helper/test-tool.h\", \"xdiff/xinclude.h\", or\n> - \"reftable/system.h\".) You do not have to include more than one of\n> - these.\n> + implementations and sha1dc/, must be . This\n> + header file insulates other header files and source files from\n> + platform differences, like which system header files must be\n> + included in what order, and what C preprocessor feature macros must\n> + be defined to trigger certain features we expect out of the system.\n> + A collorary to this is that C files should not directly include\n> + system header files themselves.\n> +\n> + There are some exceptions, because certain group of files that\n> + implement an API all have to include the same header file that\n> + defines the API and it is convenient to include \n> + there. Namely:\n> +\n> + - the implementation of the built-in commands in the \"builtin/\"\n> + directory that include \"builtin.h\" for the cmd_foo() prototype\n> + definition,\n> +\n> + - the test helper programs in the \"t/helper/\" directory that include\n> + \"t/helper/test-tool.h\" for the cmd__foo() prototype definition,\n> +\n> + - the xdiff implementation in the \"xdiff/\" directory that includes\n> + \"xdiff/xinclude.h\" for the xdiff machinery internals,\n> +\n> + - the unit test programs in \"t/unit-tests/\" directory that include\n> + \"t/unit-tests/test-lib.h\" that gives them the unit-tests\n> + framework, and\n> +\n> + - the source files that implement reftable in the \"reftable/\"\n> + directory that include \"reftable/system.h\" for the reftable\n> + internals,\n> +\n> + are allowed to assume that they do not have to include\n> + themselves, as it is included as the first\n> + '#include' in these header files. These headers must be the first\n> + header file to be \"#include\"d in them, though.\n>\n> - A C file must directly include the header files that declare the\n> functions and the types it uses, except for the functions and types\n> --\n> 2.44.0\n\nI like this latest version.\n"},{"id":"489458","messageId":"xmqqv86ah3ko.fsf@gitster.g","threadId":"60985","inReplyTo":"CABPp-BHbuvLSSr2186XqiHFQD=NUYess5zdwG4bqY6FzQfG8oA@mail.gmail.com","subject":"Re: [PATCH v3] doc: clarify the wording on requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T05:35:35Z","receivedAt":"2024-02-27T05:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I like this latest version.\n\nThanks.\n"}]}