{"thread":{"id":"57646","subject":"[PATCH] bisect.c: remove unused includes","startedAt":"2022-03-31T19:45:12Z","lastAt":"2022-04-06T18:04:29Z","messageCount":12,"participants":["Garrit Franke","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452828","messageId":"20220331194436.58005-1-garrit@slashdev.space","threadId":"57646","inReplyTo":null,"subject":"[PATCH] bisect.c: remove unused includes","fromName":"Garrit Franke","fromEmail":"garrit@slashdev.space","sentAt":"2022-03-31T19:44:36Z","receivedAt":"2022-03-31T19:45:12Z","isPatch":true,"sender":{"key":"garrit@slashdev.space","avatar":"https://avatars.githubusercontent.com/u/32395585?v=4"},"body":"Clean up includes no longer needed by bisect.c.\n\nSigned-off-by: Garrit Franke <garrit@slashdev.space>\n---\n bisect.c | 9 ---------\n 1 file changed, 9 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 9e6a2b7f20..e07e2d215d 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -1,21 +1,12 @@\n-#include \"cache.h\"\n #include \"config.h\"\n-#include \"commit.h\"\n-#include \"diff.h\"\n-#include \"revision.h\"\n #include \"refs.h\"\n #include \"list-objects.h\"\n #include \"quote.h\"\n-#include \"hash-lookup.h\"\n #include \"run-command.h\"\n #include \"log-tree.h\"\n #include \"bisect.h\"\n-#include \"oid-array.h\"\n-#include \"strvec.h\"\n-#include \"commit-slab.h\"\n #include \"commit-reach.h\"\n #include \"object-store.h\"\n-#include \"dir.h\"\n \n static struct oid_array good_revs;\n static struct oid_array skipped_revs;\n-- \n2.35.1\n\n"},{"id":"452839","messageId":"xmqqlewpzu7t.fsf@gitster.g","threadId":"57646","inReplyTo":"20220331194436.58005-1-garrit@slashdev.space","subject":"Re: [PATCH] bisect.c: remove unused includes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-31T21:29:42Z","receivedAt":"2022-03-31T21:29:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Garrit Franke <garrit@slashdev.space> writes:\n\n> Clean up includes no longer needed by bisect.c.\n>\n> Signed-off-by: Garrit Franke <garrit@slashdev.space>\n> ---\n>  bisect.c | 9 ---------\n>  1 file changed, 9 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 9e6a2b7f20..e07e2d215d 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -1,21 +1,12 @@\n> -#include \"cache.h\"\n\ncf. Documentation/CodingGuidelines\n\nThe first #include must be <git-compat-util.h>, or <cache.h> or\n<builtin.h>, which are well known to include <git-compat-util.h>\nfirst.\n\nIncluding <git-compat-util.h> indirectly by <config.h> ->\n<hashmap.h> -> <hash.h> -> <git-compat-util.h> does not count.\n\n>  #include \"config.h\"\n> -#include \"commit.h\"\n\nOther headers may indirectly include <commit.h> as their\nimplementation detail, but what matters is that *we* in this source\nfile use what <commit.h> gives us ourselves, like the concrete shape\nof \"struct commit_list\".  This change is not wanted.\n\nI'll stop here.  There may be truly leftover \"unused\" includes among\nthose removed by the remainder of this patch, but I suspect that\nsome are like <commit.h> above, i.e. we directly use it, and because\nwe do not want to be broken by some header file's implementation\ndetail changing, we MUST include it ourselves.\n\nI think this should give us a useful guideline to sift through the\nrest, and an updated patch to remove truly unused ones are very much\nwelcome.  We may actually find some we are not directly including\nourselves but we should (e.g. I do not see <string-list.h> included\nby us, but we clearly use structures and functions declared there,\nand probably is depending, wrongly, on some header file we include\nhappens to indirectly include it).\n\nThanks.\n\n> -#include \"diff.h\"\n> -#include \"revision.h\"\n>  #include \"refs.h\"\n>  #include \"list-objects.h\"\n>  #include \"quote.h\"\n> -#include \"hash-lookup.h\"\n>  #include \"run-command.h\"\n>  #include \"log-tree.h\"\n>  #include \"bisect.h\"\n> -#include \"oid-array.h\"\n> -#include \"strvec.h\"\n> -#include \"commit-slab.h\"\n>  #include \"commit-reach.h\"\n>  #include \"object-store.h\"\n> -#include \"dir.h\"\n>  \n>  static struct oid_array good_revs;\n>  static struct oid_array skipped_revs;\n"},{"id":"452855","messageId":"220401.8635ixp3f4.gmgdl@evledraar.gmail.com","threadId":"57646","inReplyTo":"xmqqlewpzu7t.fsf@gitster.g","subject":"using iwyu (include-what-you-use) to analyze includes (was: [PATCH] bisect.c: remove unused includes)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-01T08:07:34Z","receivedAt":"2022-04-01T09:18:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 31 2022, Junio C Hamano wrote:\n\n[Changed $subject to make this easier to find]\n\n> Garrit Franke <garrit@slashdev.space> writes:\n>\n>> Clean up includes no longer needed by bisect.c.\n>>\n>> Signed-off-by: Garrit Franke <garrit@slashdev.space>\n>> ---\n>>  bisect.c | 9 ---------\n>>  1 file changed, 9 deletions(-)\n>>\n>> diff --git a/bisect.c b/bisect.c\n>> index 9e6a2b7f20..e07e2d215d 100644\n>> --- a/bisect.c\n>> +++ b/bisect.c\n>> @@ -1,21 +1,12 @@\n>> -#include \"cache.h\"\n>\n> cf. Documentation/CodingGuidelines\n>\n> The first #include must be <git-compat-util.h>, or <cache.h> or\n> <builtin.h>, which are well known to include <git-compat-util.h>\n> first.\n>\n> Including <git-compat-util.h> indirectly by <config.h> ->\n> <hashmap.h> -> <hash.h> -> <git-compat-util.h> does not count.\n\nAlso: Some built-ins don't include builtin.h as they should, a fix (or\neven basic CI check) for that would be most welcome.\n\n\tgit grep -C2 -n -F -e builtin.h -e cache.h -e git-compat-util.h -- builtin\n\nI.e. we have this saying a lot of those are redundant:\n\t\n\tDocumentation/CodingGuidelines- - The first #include in C files, except in platform specific compat/\n\tDocumentation/CodingGuidelines-   implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n\tDocumentation/CodingGuidelines:   \"builtin.h\".  You do not have to include more than one of these.\n\nBut maybe it's not worth it, anyway...\n\n>>  #include \"config.h\"\n>> -#include \"commit.h\"\n>\n> Other headers may indirectly include <commit.h> as their\n> implementation detail, but what matters is that *we* in this source\n> file use what <commit.h> gives us ourselves, like the concrete shape\n> of \"struct commit_list\".  This change is not wanted.\n>\n> I'll stop here.  There may be truly leftover \"unused\" includes among\n> those removed by the remainder of this patch, but I suspect that\n> some are like <commit.h> above, i.e. we directly use it, and because\n> we do not want to be broken by some header file's implementation\n> detail changing, we MUST include it ourselves.\n>\n> I think this should give us a useful guideline to sift through the\n> rest, and an updated patch to remove truly unused ones are very much\n> welcome.  We may actually find some we are not directly including\n> ourselves but we should (e.g. I do not see <string-list.h> included\n> by us, but we clearly use structures and functions declared there,\n> and probably is depending, wrongly, on some header file we include\n> happens to indirectly include it).\n\n... For anyone interested in pursuing this, I think using the excellent\ninclude-what-you-use tool would be a nice start.\n\nWe could even eventually add it to our CI if the false positive rate\nisn't bad (I haven't checked much):\nhttps://github.com/include-what-you-use/include-what-you-use\n\nE.g. in this case (I manually omitted the rest of the output, there's\nprobably a iwyu option to omit it, but I didn't see how do that from\nskimming the docs):\n\n\t$ sudo apt install iwyu # YMMV\n\t$ make bisect.o CC=include-what-you-use CFLAGS=\"-Xiwyu --verbose=1\" 2>&1 | grep -v -E -e '^#include <' -e '^#include \"(cache|git-compat-util|gettext)\\.h\"'\n\t    CC bisect.o\n\t\n\t(bisect.h has correct #includes/fwd-decls)\n\t\n\tbisect.c should add these lines:\n\t#include \"hash.h\"              // for oideq, object_id, oidcmp, oidcpy, GIT_...\n\t#include \"object.h\"            // for object, repo_clear_commit_marks\n\t#include \"path.h\"              // for GIT_PATH_FUNC, git_pathdup\n\t#include \"pretty.h\"            // for CMIT_FMT_UNSPECIFIED, format_commit_me...\n\t#include \"repository.h\"        // for repository (ptr only), the_repository\n\t#include \"strbuf.h\"            // for strbuf_release, strbuf, strbuf_getline_lf\n\t#include \"string-list.h\"       // for string_list_append, string_list_clear\n\t\n\tbisect.c should remove these lines:\n\t- #include \"hash-lookup.h\"  // lines 9-9\n\t- struct commit_weight;  // lines 76-76\n\nThen if I patch it as:\n\t\n\tdiff --git a/bisect.c b/bisect.c\n\tindex 9e6a2b7f201..512430e3cc8 100644\n\t--- a/bisect.c\n\t+++ b/bisect.c\n\t@@ -6,7 +6,6 @@\n\t #include \"refs.h\"\n\t #include \"list-objects.h\"\n\t #include \"quote.h\"\n\t-#include \"hash-lookup.h\"\n\t #include \"run-command.h\"\n\t #include \"log-tree.h\"\n\t #include \"bisect.h\"\n\t@@ -16,6 +15,13 @@\n\t #include \"commit-reach.h\"\n\t #include \"object-store.h\"\n\t #include \"dir.h\"\n\t+#include \"hash.h\"\n\t+#include \"object.h\"\n\t+#include \"path.h\"\n\t+#include \"pretty.h\"\n\t+#include \"repository.h\"\n\t+#include \"strbuf.h\"\n\t+#include \"string-list.h\"\n\t \n\t static struct oid_array good_revs;\n\t static struct oid_array skipped_revs;\n\nIt's happier, but probably needs to be told to ignore define_commit_slab() somehow:\n\n\t$ make bisect.o CC=include-what-you-use CFLAGS=\"-Xiwyu --verbose=1\" 2>&1 | grep -v -E -e '^#include <' -e '^#include \"(cache|git-compat-util|gettext)\\.h\"'\n\t    CC bisect.o\n\t\n\t(bisect.h has correct #includes/fwd-decls)\n\t\n\tbisect.c should add these lines:\n\t\n\tbisect.c should remove these lines:\n\t- struct commit_weight;  // lines 82-82\n\nThat still needs to be massaged a bit, e.g. we should probably omit\nhash.h and anything else in cache.h and git-compat-util.h.\n\nOr maybe not & we should make those headers even lighter. It is rather\nannoying that changing some of those things leads to a complete\nre-build, but there's a trade-off there where we probably want things\nlike gettext.h and other used-almost-everywhere headers in included by\nthose.\n\nSo take all the above with a huge grain of salt. I haven't used iwyu\nmuch, but it seems to be something that'll help us go in the direction\nJunio noted above.\n\nI think starting with:\n\n\tmake -k git-objs <the CC etc. params above>\n\nAnd tackling the \"should remove these lines\" issues first would be a\ngood start, e.g. for serve.c it says:\n\t\n\tserve.c should remove these lines:\n\t- #include \"cache.h\"  // lines 1-1\n\t- #include \"strvec.h\"  // lines 6-6\n\nWe don't want that first one, but it's right about the second one. It's\nbeen orphaned since f0a35c9ce52 (serve: drop \"keys\" strvec, 2021-09-15),\nI skimmed some of the rist and they all seem like good\nsuggestions. E.g. lockfile.h for builtin/apply.c, which isn't needed\nsince 6d058c88264 (apply: move lockfile into `apply_state`, 2017-10-05).\n"},{"id":"453114","messageId":"20220405114505.24389-1-garrit@slashdev.space","threadId":"57646","inReplyTo":"20220331194436.58005-1-garrit@slashdev.space","subject":"[PATCH v2 0/4] various: use iwyu (include-what-you-use) to analyze includes","fromName":"Garrit Franke","fromEmail":"garrit@slashdev.space","sentAt":"2022-04-05T11:45:02Z","receivedAt":"2022-04-05T12:57:26Z","isPatch":true,"sender":{"key":"garrit@slashdev.space","avatar":"https://avatars.githubusercontent.com/u/32395585?v=4"},"body":"I'm sorry about the half-baked patch of mine. I initially noticed that\nthe \"hash-lookup.h\" wasn't needed (which was a correct assumption), and\nthen went overboard when trying to clean up the other includes.\n\nThis is truly a rookie mistake that only wastes the time of everyone\ninvolved. This won't happen again!\n\nOn 01.04.22 10:07, ��������� var Arnfj��������r�������� Bjarmason wrote:\n\n> ... For anyone interested in pursuing this, I think using the excellent\n> include-what-you-use tool would be a nice start.\n>\n> We could even eventually add it to our CI if the false positive rate\n> isn't bad (I haven't checked much):\n> https://github.com/include-what-you-use/include-what-you-use\n\nThis seems to be a really nice tool indeed. I wouldn't be comfortable\nadding it to the CI just yet, but it did make it considerably easier to\nspot includes that could safely be removed.\n\nI think we could try battle-testing this tool in the codebase to get a\nsense of how it behaves. To start, I added your reference-command to a\nscript under \"contrib/iwyu\" and ran it against the files you noted.\nBefore breaking a bulk of the files, I wanted to make sure that this\nundertaking is headed in the right direction.\n\nFeedback is of course welcomed!\n\nGarrit Franke (4):\n  contrib: add iwyu script\n  bisect.c: remove unnecessary include\n  serve.c: remove unnecessary include\n  apply.c: remove unnecessary include\n\n bisect.c             |  1 -\n builtin/apply.c      |  1 -\n contrib/iwyu/README  | 33 +++++++++++++++++++++++++++++++++\n contrib/iwyu/iwyu.sh |  2 ++\n serve.c              |  1 -\n 5 files changed, 35 insertions(+), 3 deletions(-)\n create mode 100644 contrib/iwyu/README\n create mode 100755 contrib/iwyu/iwyu.sh\n\n-- \n2.35.1\n\n"},{"id":"453117","messageId":"20220405114505.24389-5-garrit@slashdev.space","threadId":"57646","inReplyTo":"20220331194436.58005-1-garrit@slashdev.space","subject":"[PATCH v2 4/4] apply.c: remove unnecessary include","fromName":"Garrit Franke","fromEmail":"garrit@slashdev.space","sentAt":"2022-04-05T11:45:10Z","receivedAt":"2022-04-05T12:58:36Z","isPatch":true,"sender":{"key":"garrit@slashdev.space","avatar":"https://avatars.githubusercontent.com/u/32395585?v=4"},"body":"Remove include \"lockfile.h\" from builtin/apply.c, which is orphaned\nsince 6d058c88264 (apply: move lockfile into `apply_state`, 2017-10-05)\n\nSigned-off-by: Garrit Franke <garrit@slashdev.space>\n---\n builtin/apply.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 3f099b9605..555219de40 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1,7 +1,6 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n #include \"parse-options.h\"\n-#include \"lockfile.h\"\n #include \"apply.h\"\n \n static const char * const apply_usage[] = {\n-- \n2.35.1\n\n"},{"id":"453118","messageId":"20220405114505.24389-4-garrit@slashdev.space","threadId":"57646","inReplyTo":"20220331194436.58005-1-garrit@slashdev.space","subject":"[PATCH v2 3/4] serve.c: remove unnecessary include","fromName":"Garrit Franke","fromEmail":"garrit@slashdev.space","sentAt":"2022-04-05T11:45:08Z","receivedAt":"2022-04-05T12:58:36Z","isPatch":true,"sender":{"key":"garrit@slashdev.space","avatar":"https://avatars.githubusercontent.com/u/32395585?v=4"},"body":"Remove include \"strvec.h\" from serve.c, which is orphaned since\nf0a35c9ce52 (serve: drop \"keys\" strvec, 2021-09-15)\n\nSigned-off-by: Garrit Franke <garrit@slashdev.space>\n---\n serve.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/serve.c b/serve.c\nindex b3fe9b5126..733347f602 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -3,7 +3,6 @@\n #include \"config.h\"\n #include \"pkt-line.h\"\n #include \"version.h\"\n-#include \"strvec.h\"\n #include \"ls-refs.h\"\n #include \"protocol-caps.h\"\n #include \"serve.h\"\n-- \n2.35.1\n\n"},{"id":"453119","messageId":"20220405114505.24389-2-garrit@slashdev.space","threadId":"57646","inReplyTo":"20220331194436.58005-1-garrit@slashdev.space","subject":"[PATCH v2 1/4] contrib: add iwyu script","fromName":"Garrit Franke","fromEmail":"garrit@slashdev.space","sentAt":"2022-04-05T11:45:04Z","receivedAt":"2022-04-05T12:59:04Z","isPatch":true,"sender":{"key":"garrit@slashdev.space","avatar":"https://avatars.githubusercontent.com/u/32395585?v=4"},"body":"add include-what-you-use helper.\n\nSigned-off-by: Garrit Franke <garrit@slashdev.space>\n---\n contrib/iwyu/README  | 33 +++++++++++++++++++++++++++++++++\n contrib/iwyu/iwyu.sh |  2 ++\n 2 files changed, 35 insertions(+)\n create mode 100644 contrib/iwyu/README\n create mode 100755 contrib/iwyu/iwyu.sh\n\ndiff --git a/contrib/iwyu/README b/contrib/iwyu/README\nnew file mode 100644\nindex 0000000000..5e2d218602\n--- /dev/null\n+++ b/contrib/iwyu/README\n@@ -0,0 +1,33 @@\n+Include What You Use\n+====================\n+\n+Include what you use (iwyu) [1] is a tool that points out which headers a file\n+should include. Moreover, it can point out includes that are not used by a file,\n+which makes it especially handy for cleanup tasks.\n+\n+To run this script, you will need iwyu to be installed on your system.\n+\n+The \"iwyu.sh\" script runs iwyu on a given object and omits mandatory headers\n+defined in \"Documentation/CodingGuidelines\".\n+\n+Example usage:\n+\n+    ./contrib/iwyu/iwyu.sh diff.o\n+\n+This yields:\n+\n+    diff.c should remove these lines:\n+    - #include \"attr.h\"  // lines 13-13\n+    - #include \"submodule-config.h\"  // lines 18-18\n+\n+In its current form, this script should not be used to auto-generate patches,\n+since there are still some false-positives that only a human can resolve. It is\n+meant as a starting point for further cleanups. It could be nice to integrate\n+this as a step in our CI, but we're not quite there yet.\n+\n+The inspiration for this script came from this [2] email-thread.\n+\n+Garrit Franke <garrit@slashdev.space>\n+\n+[1]: https://github.com/include-what-you-use/include-what-you-use\n+[2]: https://lore.kernel.org/all/220401.8635ixp3f4.gmgdl@evledraar.gmail.com/#t\n\\ No newline at end of file\ndiff --git a/contrib/iwyu/iwyu.sh b/contrib/iwyu/iwyu.sh\nnew file mode 100755\nindex 0000000000..3ef8639eae\n--- /dev/null\n+++ b/contrib/iwyu/iwyu.sh\n@@ -0,0 +1,2 @@\n+make $1 CC=include-what-you-use CFLAGS=\"-Xiwyu --verbose=1\" 2>&1 \\\n+| grep -v -E -e '^#include <' -e '^#include \"(cache|git-compat-util|gettext)\\.h\"' \n\\ No newline at end of file\n-- \n2.35.1\n\n"},{"id":"453120","messageId":"20220405114505.24389-3-garrit@slashdev.space","threadId":"57646","inReplyTo":"20220331194436.58005-1-garrit@slashdev.space","subject":"[PATCH v2 2/4] bisect.c: remove unnecessary include","fromName":"Garrit Franke","fromEmail":"garrit@slashdev.space","sentAt":"2022-04-05T11:45:06Z","receivedAt":"2022-04-05T13:23:00Z","isPatch":true,"sender":{"key":"garrit@slashdev.space","avatar":"https://avatars.githubusercontent.com/u/32395585?v=4"},"body":"Remove include \"hash-lookup.h\".\n\nSigned-off-by: Garrit Franke <garrit@slashdev.space>\n---\n bisect.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 9e6a2b7f20..1aced76257 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -6,7 +6,6 @@\n #include \"refs.h\"\n #include \"list-objects.h\"\n #include \"quote.h\"\n-#include \"hash-lookup.h\"\n #include \"run-command.h\"\n #include \"log-tree.h\"\n #include \"bisect.h\"\n-- \n2.35.1\n\n"},{"id":"453159","messageId":"220406.865ynmfxju.gmgdl@evledraar.gmail.com","threadId":"57646","inReplyTo":"20220405114505.24389-1-garrit@slashdev.space","subject":"Re: [PATCH v2 0/4] various: use iwyu (include-what-you-use) to analyze includes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-06T07:54:15Z","receivedAt":"2022-04-06T12:02:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 05 2022, Garrit Franke wrote:\n\n> On 01.04.22 10:07, Ævar Arnfjörð Bjarmason wrote:\n\nAside: I don't think I've ever seen encoded quoted-printable go quite so\nbad so fast. That went from =C3=86var to =C3=83=C6=92=C3=A2=E2=82=AC in\none reply. Whatever your E-Mail is doing with encodings seems to be\ntaking multiple passes through misencodings :)\n\nDon't worry about getting the name \"right\" or whatever, I'm amused by\nthe encoding issue...\n\n>> ... For anyone interested in pursuing this, I think using the excellent\n>> include-what-you-use tool would be a nice start.\n>>\n>> We could even eventually add it to our CI if the false positive rate\n>> isn't bad (I haven't checked much):\n>> https://github.com/include-what-you-use/include-what-you-use\n>\n> This seems to be a really nice tool indeed. I wouldn't be comfortable\n> adding it to the CI just yet, but it did make it considerably easier to\n> spot includes that could safely be removed.\n\nRe the reply I had on 1/4 I think it's probably best to drop that in its\ncurrent form, but the fixes themselves (perhaps with a re-roll for nits\nI posted in reply) seem good.\n\nI was really hoping though that if someone wanted to pursue this a bit\nmore we'd get to the point of being able to run \"make all test\" on a\nsource tree that iwyu would munge with all its suggestions, and then see\nif it outright failed to compile, or whether it would e.g. have faster\ncompilation speed (or not..).\n\n> I think we could try battle-testing this tool in the codebase to get a\n> sense of how it behaves. To start, I added your reference-command to a\n> script under \"contrib/iwyu\" and ran it against the files you noted.\n> Before breaking a bulk of the files, I wanted to make sure that this\n> undertaking is headed in the right direction.\n\nEven if the patches aren't sent in making the actual changes a one-off\nscript to e.g. wrap the fix_includes.py script I mentioned would be very\ninteresting.\n\nWe could then even run that in CI with relatively little setup,\ni.e. checkout <rev>, do munging, then compile.\n"},{"id":"453160","messageId":"220406.86ee2afy6h.gmgdl@evledraar.gmail.com","threadId":"57646","inReplyTo":"20220405114505.24389-2-garrit@slashdev.space","subject":"Re: [PATCH v2 1/4] contrib: add iwyu script","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-06T07:40:17Z","receivedAt":"2022-04-06T12:06:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 05 2022, Garrit Franke wrote:\n\n> add include-what-you-use helper.\n>\n> Signed-off-by: Garrit Franke <garrit@slashdev.space>\n> ---\n>  contrib/iwyu/README  | 33 +++++++++++++++++++++++++++++++++\n>  contrib/iwyu/iwyu.sh |  2 ++\n>  2 files changed, 35 insertions(+)\n>  create mode 100644 contrib/iwyu/README\n>  create mode 100755 contrib/iwyu/iwyu.sh\n>\n> diff --git a/contrib/iwyu/README b/contrib/iwyu/README\n> new file mode 100644\n> index 0000000000..5e2d218602\n> --- /dev/null\n> +++ b/contrib/iwyu/README\n> @@ -0,0 +1,33 @@\n> +Include What You Use\n> +====================\n> +\n> +Include what you use (iwyu) [1] is a tool that points out which headers a file\n> +should include. Moreover, it can point out includes that are not used by a file,\n> +which makes it especially handy for cleanup tasks.\n> +\n> +To run this script, you will need iwyu to be installed on your system.\n> +\n> +The \"iwyu.sh\" script runs iwyu on a given object and omits mandatory headers\n> +defined in \"Documentation/CodingGuidelines\".\n> +\n> +Example usage:\n> +\n> +    ./contrib/iwyu/iwyu.sh diff.o\n> +\n> +This yields:\n> +\n> +    diff.c should remove these lines:\n> +    - #include \"attr.h\"  // lines 13-13\n> +    - #include \"submodule-config.h\"  // lines 18-18\n> +\n> +In its current form, this script should not be used to auto-generate patches,\n> +since there are still some false-positives that only a human can resolve. It is\n> +meant as a starting point for further cleanups. It could be nice to integrate\n> +this as a step in our CI, but we're not quite there yet.\n> +\n> +The inspiration for this script came from this [2] email-thread.\n> +\n> +Garrit Franke <garrit@slashdev.space>\n> +\n> +[1]: https://github.com/include-what-you-use/include-what-you-use\n> +[2]: https://lore.kernel.org/all/220401.8635ixp3f4.gmgdl@evledraar.gmail.com/#t\n> \\ No newline at end of file\n\nNote the \"No newline at end of file\" warnings, new files should have \\n\nat the end.\n\n> diff --git a/contrib/iwyu/iwyu.sh b/contrib/iwyu/iwyu.sh\n> new file mode 100755\n> index 0000000000..3ef8639eae\n> --- /dev/null\n> +++ b/contrib/iwyu/iwyu.sh\n> @@ -0,0 +1,2 @@\n> +make $1 CC=include-what-you-use CFLAGS=\"-Xiwyu --verbose=1\" 2>&1 \\\n> +| grep -v -E -e '^#include <' -e '^#include \"(cache|git-compat-util|gettext)\\.h\"' \n> \\ No newline at end of file\n\nSo that's just my one-line hack as-is (well, bisect.co replaced with $1)\n:)\n\nI think if we integrate some IWYU spport support that this is a rather\ndead end to pursue.\n\nA much better way would be to just do (\"REAL_CC\" is already understood\nby sparse):\n\n\tREAL_CC=gcc make CC=contrib/iwyu.sh \n\nI.e. we shouldn't run make and then parse its output, but to have make\nrun a CC which wraps our REAL_CC.\n\nThen you could either have it \"really\" compile, as e.g. the \"sparse\"\nwrapper can do (note: note our sparse target but cgcc), or run whatever\n\"make\" target, and then only having to parse the output of iwyu, not\niwyu+make.\n\nAlso, it looks like iwyu is a nicely API'd python library, so trying to\nparse its output is probably a dead end too, can't we just ship a\ntrivial API-using script for it that gets all this as nicely\nmachine-readable data?\n\nOr, if you look at iwyu.git its own source tree has a fix_includes.py\nwhich seems to do that, *and* be able to \"fix\" the includes for you.\n\nAll of that is just stuff I didn't have time to poke at when posting a\nquick reply in the original thread, but hopefully someone turning this\ninto a series of patches will :)\n\t\n\t$ python3 fix_includes.py -h\n\tUsage: fix_includes.py [options] [filename] ... < <output from include-what-you-use script>\n\t    OR fix_includes.py -s [other options] <filename> ...\n\t\n\tfix_includes.py reads the output from the include-what-you-use\n\tscript on stdin -- run with --v=1 (default) verbose or above -- and,\n\tunless --sort_only or --dry_run is specified,\n\tmodifies the files mentioned in the output, removing their old\n\t#include lines and replacing them with the lines given by the\n\tinclude_what_you_use script.  It also sorts the #include and\n\tforward-declare lines.\n\t\n"},{"id":"453161","messageId":"220406.86a6cyfy4l.gmgdl@evledraar.gmail.com","threadId":"57646","inReplyTo":"20220405114505.24389-3-garrit@slashdev.space","subject":"Re: [PATCH v2 2/4] bisect.c: remove unnecessary include","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-06T07:50:27Z","receivedAt":"2022-04-06T12:08:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 05 2022, Garrit Franke wrote:\n\n> Remove include \"hash-lookup.h\".\n\nLike your 3/4 it would be nice to have a \"orphaned since xyz\", or was it\nnever used etc?\n\nIn these cases unlike most such C changes the compiler isn't of much use\nfor validation (we might be including this implicitly), so an\nexplanation saying why is helpful.\n"},{"id":"453202","messageId":"xmqqfsmqyxj5.fsf@gitster.g","threadId":"57646","inReplyTo":"220406.86a6cyfy4l.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/4] bisect.c: remove unnecessary include","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-06T16:41:34Z","receivedAt":"2022-04-06T18:04:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Tue, Apr 05 2022, Garrit Franke wrote:\n>\n>> Remove include \"hash-lookup.h\".\n>\n> Like your 3/4 it would be nice to have a \"orphaned since xyz\", or was it\n> never used etc?\n>\n> In these cases unlike most such C changes the compiler isn't of much use\n> for validation (we might be including this implicitly), so an\n> explanation saying why is helpful.\n\nThanks for pointing it out.  I found the lack of explanation in some\nbut not all of the changes disturbing.\n"}]}