{"thread":{"id":"38367","subject":"Unused #include statements","startedAt":"2015-01-15T03:43:06Z","lastAt":"2015-01-20T02:08:35Z","messageCount":8,"participants":["Zoltan Klinger","Robert Schiele","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"254720","messageId":"CAKJhZwR+iMYAMCxurgc7z2dhqoqx_RxV1G4Jh3phPAOGptp_XQ@mail.gmail.com","threadId":"38367","inReplyTo":null,"subject":"Unused #include statements","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2015-01-15T03:43:06Z","receivedAt":"2015-01-15T03:43:06Z","isPatch":false,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"Hello there,\n\nSince reading a post [1] about removing some unnecessary #include statements\nfrom a git C source file I've been intrigued to see how many more might be\nlurking in the code base.\n\nAfter a bit of digging around, my brute force approach of 'remove as many\n#includes as possible while making sure the code still successfully compiles'\nhas returned the following results:\n\n\n$ git diff --stat\n alloc.c | 2 --\n archive-zip.c | 1 -\n archive.c | 1 -\n argv-array.c | 1 -\n bisect.c | 9 ---------\n block-sha1/sha1.c | 2 --\n branch.c | 1 -\n builtin/add.c | 7 -------\n builtin/annotate.c | 1 -\n builtin/apply.c | 8 --------\n builtin/archive.c | 3 ---\n builtin/bisect--helper.c | 2 --\n builtin/blame.c | 13 -------------\n builtin/branch.c | 8 --------\n builtin/bundle.c | 1 -\n builtin/cat-file.c | 3 ---\n builtin/check-attr.c | 2 --\n builtin/check-ignore.c | 2 --\n builtin/check-mailmap.c | 2 --\n builtin/check-ref-format.c | 2 --\n builtin/checkout-index.c | 2 --\n builtin/checkout.c | 11 -----------\n builtin/clean.c | 3 ---\n builtin/clone.c | 10 ----------\n builtin/column.c | 3 ---\n builtin/commit-tree.c | 4 ----\n builtin/commit.c | 13 -------------\n builtin/config.c | 1 -\n builtin/count-objects.c | 2 --\n builtin/credential.c | 1 -\n builtin/describe.c | 5 -----\n builtin/diff-files.c | 4 ----\n builtin/diff-index.c | 4 ----\n builtin/diff-tree.c | 4 ----\n builtin/diff.c | 6 ------\n builtin/fast-export.c | 8 --------\n builtin/fetch.c | 8 --------\n builtin/fmt-merge-msg.c | 6 ------\n builtin/for-each-ref.c | 6 ------\n builtin/fsck.c | 7 -------\n builtin/gc.c | 3 ---\n builtin/get-tar-commit-id.c | 3 ---\n builtin/grep.c | 6 ------\n builtin/hash-object.c | 2 --\n builtin/help.c | 2 --\n builtin/index-pack.c | 5 -----\n builtin/init-db.c | 1 -\n builtin/interpret-trailers.c | 3 ---\n builtin/log.c | 14 --------------\n builtin/ls-files.c | 3 ---\n builtin/ls-remote.c | 3 ---\n builtin/ls-tree.c | 3 ---\n builtin/mailinfo.c | 2 --\n builtin/mailsplit.c | 3 ---\n builtin/merge-base.c | 5 -----\n builtin/merge-file.c | 2 --\n builtin/merge-index.c | 1 -\n builtin/merge-ours.c | 1 -\n builtin/merge-recursive.c | 3 ---\n builtin/merge-tree.c | 2 --\n builtin/merge.c | 10 ----------\n builtin/mktree.c | 2 --\n builtin/mv.c | 4 ----\n builtin/name-rev.c | 3 ---\n builtin/notes.c | 4 ----\n builtin/pack-objects.c | 14 --------------\n builtin/prune-packed.c | 1 -\n builtin/prune.c | 5 -----\n builtin/push.c | 6 ------\n builtin/read-tree.c | 4 ----\n builtin/receive-pack.c | 12 ------------\n builtin/reflog.c | 5 -----\n builtin/remote-ext.c | 1 -\n builtin/remote-fd.c | 1 -\n builtin/remote.c | 6 ------\n builtin/repack.c | 6 ------\n builtin/replace.c | 1 -\n builtin/rerere.c | 4 ----\n builtin/reset.c | 7 -------\n builtin/rev-list.c | 7 -------\n builtin/rev-parse.c | 5 -----\n builtin/revert.c | 4 ----\n builtin/rm.c | 4 ----\n builtin/send-pack.c | 7 -------\n builtin/shortlog.c | 7 -------\n builtin/show-branch.c | 3 ---\n builtin/show-ref.c | 5 -----\n builtin/stripspace.c | 1 -\n builtin/symbolic-ref.c | 1 -\n builtin/tag.c | 4 ----\n builtin/unpack-objects.c | 7 -------\n builtin/update-index.c | 7 -------\n builtin/update-ref.c | 3 ---\n builtin/update-server-info.c | 1 -\n builtin/upload-archive.c | 5 -----\n builtin/verify-commit.c | 5 -----\n builtin/verify-pack.c | 1 -\n builtin/verify-tag.c | 5 -----\n builtin/write-tree.c | 2 --\n bulk-checkin.c | 3 ---\n bundle.c | 5 -----\n cache-tree.c | 2 --\n check-racy.c | 1 -\n column.c | 2 --\n combine-diff.c | 6 ------\n commit.c | 7 -------\n compat/basename.c | 1 -\n compat/fopen.c | 1 -\n compat/gmtime.c | 1 -\n compat/hstrerror.c | 3 ---\n compat/inet_ntop.c | 1 -\n compat/inet_pton.c | 1 -\n compat/mingw.c | 7 -------\n compat/mkdir.c | 1 -\n compat/mkdtemp.c | 1 -\n compat/mmap.c | 1 -\n compat/msvc.c | 5 -----\n compat/nedmalloc/nedmalloc.c | 2 --\n compat/obstack.c | 1 -\n compat/poll/poll.c | 6 ------\n compat/pread.c | 1 -\n compat/precompose_utf8.c | 1 -\n compat/qsort.c | 1 -\n compat/regex/regex.c | 2 --\n compat/setenv.c | 1 -\n compat/snprintf.c | 1 -\n compat/strcasestr.c | 1 -\n compat/strlcpy.c | 1 -\n compat/strtoimax.c | 1 -\n compat/strtoumax.c | 1 -\n compat/terminal.c | 2 --\n compat/unsetenv.c | 1 -\n compat/win32/dirent.c | 1 -\n compat/win32/pthread.c | 4 ----\n compat/win32/syslog.c | 1 -\n compat/win32mmap.c | 1 -\n compat/winansi.c | 3 ---\n config.c | 4 ----\n connect.c | 4 ----\n connected.c | 2 --\n contrib/convert-objects/convert-objects.c | 4 ----\n .../gnome-keyring/git-credential-gnome-keyring.c | 6 ------\n .../credential/osxkeychain/git-credential-osxkeychain.c | 4 ----\n contrib/credential/wincred/git-credential-wincred.c | 4 ----\n contrib/examples/builtin-fetch--tool.c | 5 -----\n contrib/svn-fe/svn-fe.c | 2 --\n convert.c | 2 --\n credential-cache--daemon.c | 2 --\n credential-cache.c | 3 ---\n credential-store.c | 1 -\n credential.c | 1 -\n csum-file.c | 1 -\n daemon.c | 3 ---\n diff-delta.c | 1 -\n diff-lib.c | 5 -----\n 155 files changed, 562 deletions(-)\n\n\nSo my questions are as follows:\n\n(1) Is it worth turning this into a proper patch?\n\n(2) Given the large number of files (150+) what would be the best\n approach in preparing the patch?\n\n (a) One commit containing all the changes? Would it be a bit too much\n to digest in one go?\n\n (b) One commit per file changed? Feels a bit over the top also it\n would flood the mailing list.\n\n (c) One commit each for changes in the root directory, builtin,\n compat and contrib directories?\n\nThanks,\nZoltan\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/262402\n"},{"id":"254721","messageId":"CAObFj3wC6ezNQfAYvtepBdW3S0hv8c4_fXYTo-zp4wwddx3QXg@mail.gmail.com","threadId":"38367","inReplyTo":"CAKJhZwR+iMYAMCxurgc7z2dhqoqx_RxV1G4Jh3phPAOGptp_XQ@mail.gmail.com","subject":"Re: Unused #include statements","fromName":"Robert Schiele","fromEmail":"rschiele@gmail.com","sentAt":"2015-01-15T04:14:39Z","receivedAt":"2015-01-15T04:14:39Z","isPatch":false,"sender":{"key":"rschiele@gmail.com","avatar":"https://gravatar.com/avatar/409473567eb2287d5f0157b51f5b703994b347f24f92172e3a0588741c27a492?d=mp&s=160"},"body":"Hi Zoltan,\n\nI can't make a statement for the git project but I consider this kind\nof brute-force removal a very problematic approach for languages like\nC and C++. The reason for that is simple: Often header files include\nother header files since their content depends on those other header\nfiles. Let's assume a header file a.h includes b.h. Now consider a\nfile c.c includes both a.h and b.h since they are actually using stuff\nfrom both of them. Your brute-force approach would now remove b.h\nsince it is indirectly pulled in through a.h. While the removal would\nno lead to technially incorrect code it would no longer reflect the\nsemantical situation (since c.c still uses stuff from b.h). Even worse\nis that if you later modify c.c to no longer use stuff from a.h you\ncould no longer remove a.h since that way you would break the chain to\nb.h.\n\nThus doing those kind of brute-force removals generally makes the\ninclude structure in a project very fragile. The analysis itself you\ndid is still useful to identify header files that can potentially be\nremoved but removing them without further analysis I would consider\nproblematic.\n\nRobert\n"},{"id":"254723","messageId":"20150115063307.GA11028@peff.net","threadId":"38367","inReplyTo":"CAObFj3wC6ezNQfAYvtepBdW3S0hv8c4_fXYTo-zp4wwddx3QXg@mail.gmail.com","subject":"Re: Unused #include statements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-15T06:33:07Z","receivedAt":"2015-01-15T06:33:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 15, 2015 at 05:14:39AM +0100, Robert Schiele wrote:\n\n> Thus doing those kind of brute-force removals generally makes the\n> include structure in a project very fragile. The analysis itself you\n> did is still useful to identify header files that can potentially be\n> removed but removing them without further analysis I would consider\n> problematic.\n\nI would second that. Besides leading to a potentially fragile result,\nthis analysis was done only for a particular platform with a particular\nset of config knobs.\n\nOne of our rules is that git-compat-util.h (or one of the well-known\nheaders which includes, cache.h or builtin.h) is included first in any\ntranslation unit. This gives git-compat-util the cleanest environment\npossible for making decisions, and lets macros it defines effect the\nrest of the code consistently. I suspect on modern platforms like\nLinux/glibc that it is not a huge deal to include git-compat-util a\nlittle late, simply because it does not have all that much to do. But\non Solaris 8? Who knows.\n\n-Peff\n"},{"id":"254765","messageId":"xmqqvbk77u9m.fsf@gitster.dls.corp.google.com","threadId":"38367","inReplyTo":"20150115063307.GA11028@peff.net","subject":"Re: Unused #include statements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-15T18:50:45Z","receivedAt":"2015-01-15T18:50:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> One of our rules is that git-compat-util.h (or one of the well-known\n> headers which includes, cache.h or builtin.h) is included first in any\n> translation unit.\n\nPerhaps by now we can spell it out a bit more explicitly and update\nCodingGuidelines. We have:\n\n - The first #include in C files, except in platform specific\n compat/ implementations, should be git-compat-util.h or another\n header file that includes it, such as cache.h or builtin.h.\n\n\"such as\" might be making things unnecessarily vague; I do not think\na valid reason why we should say a .c file that includes \"advice.h\"\nas the first thing satisfies this requirement, for example.\n\nBecause:\n\n - A command that interacts with the object store, config subsystem,\n the index, or the working tree cannot do anything without using\n what is declared in \"cache.h\".\n\n - A built-in command must be declared in \"builtin.h\", so anything\n in builtin/*.c must include it.\n\nit may be reasonable to say the first *.h file included must be one\nof git-compat-util.h, cache.h or builtin.h (and then we make sure\nthat compat-util is the first thing included in either of the latter\ntwo).\n\nWhile I very much agree with the principle Robert alluded to [*1*],\nwe may want to loosen that for .c files that include \"builtin.h\" or\n\"cache.h\" for the sake of brevity. For example, if you are builtin\n(hence you start by #include \"builtin.h\"), it may be reasonable to\nallow you to take whatever is in \"cache.h\" for granted [*2*].\n\nSo the rule might be:\n\n - The first #include in C files, except in platform specific\n compat/ implementations, must be either git-compat-util.h,\n cache.h or builtin.h.\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\n types that are made available to it by including one of the\n header files it must include by the previous rule.\n\nOptionally, \n\n - A C file must include only one of \"git-compat-util.h\", \"cache.h\"\n or \"builtin.h\"; e.g. if you include \"builtin.h\", do not include\n the other two, but it can consider what is availble in \"cache.h\"\n available to it.\n\nThoughts? I am not looking forward to a torrent of patches whose\nsole purpose is to make the existing C files conform to any such\nrule, though. Clean-up patches that trickle in at a low rate is\ntolerable, but a torrent is too distracting.\n\n\n[Footnote]\n\n*1* For example, even though \"diff.h\" may include \"tree-walk.h\" for\nits own use, a .c file that includes \"diff.h\" without including\n\"tree-walk.h\" that uses update_tree_entry() or anything that is\ndeclared in the latter is very iffy from semantic point of view.\n\n*2* Because that facility is so widely used inside the codebase,\n\"builtin.h\" includes \"strbuf.h\", so in addition to what are in\n\"cache.h\", we may want to allow builtin implementations to take\nstrbufs for granted as well.\n"},{"id":"254782","messageId":"20150115223836.GC19021@peff.net","threadId":"38367","inReplyTo":"xmqqvbk77u9m.fsf@gitster.dls.corp.google.com","subject":"Re: Unused #include statements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-15T22:38:37Z","receivedAt":"2015-01-15T22:38:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 15, 2015 at 10:50:45AM -0800, Junio C Hamano wrote:\n\n> So the rule might be:\n> \n> - The first #include in C files, except in platform specific\n> compat/ implementations, must be either git-compat-util.h,\n> cache.h or builtin.h.\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\n> types that are made available to it by including one of the\n> header files it must include by the previous rule.\n\nYeah, that makes sense (and is what I took away from the existing rule\nin CodingGuidelines, but I agree what is there is not very rigorous).\n\n> Optionally, \n> \n> - A C file must include only one of \"git-compat-util.h\", \"cache.h\"\n> or \"builtin.h\"; e.g. if you include \"builtin.h\", do not include\n> the other two, but it can consider what is availble in \"cache.h\"\n> available to it.\n> \n> Thoughts? I am not looking forward to a torrent of patches whose\n> sole purpose is to make the existing C files conform to any such\n> rule, though. Clean-up patches that trickle in at a low rate is\n> tolerable, but a torrent is too distracting.\n\nI don't think the \"optionally\" one above is that necessary. Not because\nI don't agree with it, but because I do not know that we want to get\ninto the business of laying out every minute detail and implication.\nThe CodingGuidelines document is meant to be guidelines, and I do not\nwant to see arguments like \"well, the guidelines do not explicitly\n_disallow_ this, so you must accept it or add something to the\nguideline\". That is a waste of everybody's time.\n\nA general philosophy + good taste (from the submitter and the\nmaintainer) should ideally be enough. And hopefully would stop a torrent\nof \"but this file doesn't conform to the letter of CodingGuidelines!\".\nMaybe it does not, but if there is no tangible benefit besides blindly\nfollowing some rules, it is not worth the precious time of developers.\n\nWhich isn't to say we shouldn't clarify the document when need be. But I\nthink what I quoted at the top already is probably a good improvement\nover what is there.\n\n-Peff\n"},{"id":"254786","messageId":"xmqqy4p34onq.fsf@gitster.dls.corp.google.com","threadId":"38367","inReplyTo":"20150115223836.GC19021@peff.net","subject":"Re: Unused #include statements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-15T23:20:09Z","receivedAt":"2015-01-15T23:20:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jan 15, 2015 at 10:50:45AM -0800, Junio C Hamano wrote:\n>> ...\n>> Thoughts? I am not looking forward to a torrent of patches whose\n>> sole purpose is to make the existing C files conform to any such\n>> rule, though. Clean-up patches that trickle in at a low rate is\n>> tolerable, but a torrent is too distracting.\n>\n> I don't think the \"optionally\" one above is that necessary. Not because\n> I don't agree with it, but because I do not know that we want to get\n> into the business of laying out every minute detail and implication.\n> The CodingGuidelines document is meant to be guidelines, and I do not\n> want to see arguments like \"well, the guidelines do not explicitly\n> _disallow_ this, so you must accept it or add something to the\n> guideline\". That is a waste of everybody's time.\n\nTotally. I know we do not want to get into that business.\n\n> A general philosophy + good taste (from the submitter and the\n> maintainer) should ideally be enough.\n\nYes, \"ideally\" ;-)\n\n> Which isn't to say we shouldn't clarify the document when need be. But I\n> think what I quoted at the top already is probably a good improvement\n> over what is there.\n\nOK, thanks. Let's queue something like this for post 2.3 cycle,\nthen.\n\n-- >8 --\nSubject: CodingGuidelines: clarify C #include rules\n\nEven though \"advice.h\" includes \"git-compat-util.h\", it is not\nsensible to have it as the first #include and indirectly satisify\nthe \"You must give git-compat-util.h a clean environment to set up\nfeature test macros before including any of the system headers are\nincluded\", which is the real requirement.\n\nBecause:\n\n - A command that interacts with the object store, config subsystem,\n the index, or the working tree cannot do anything without using\n what is declared in \"cache.h\";\n\n - A built-in command must be declared in \"builtin.h\", so anything\n in builtin/*.c must include it;\n\n - These two headers both include \"git-compat-util.h\" as the first\n thing; and\n\n - Almost all our *.c files (outside compat/ and borrowed files in\n xdiff/) need some Git-ness from \"cache.h\" to do something\n Git-ish.\n\nlet's explicitly specify that one of these three header files must\nbe the first thing that is included.\n\nAny of our *.c file should include the header file that directly\ndeclares what it uses, instead of relying on the fact that some *.h\nfile it includes happens to include another *.h file that declares\nthe necessary function or type. Spell it out as another guideline\nitem.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 894546d..578d07c 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -328,9 +328,14 @@ For C programs:\n \n - When you come up with an API, document it.\n \n- - The first #include in C files, except in platform specific\n- compat/ implementations, should be git-compat-util.h or another\n- header file that includes it, such as cache.h or builtin.h.\n+ - The first #include in C files, except in platform specific compat/\n+ implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n+ \"builtin.h\". You do not have to include more than one of these.\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+ that are made available to it by including one of the header files\n+ it must include by the previous rule.\n \n - If you are planning a new command, consider writing it in shell\n or perl first, so that changes in semantics can be easily\n"},{"id":"254794","messageId":"20150116000035.GC25120@peff.net","threadId":"38367","inReplyTo":"xmqqy4p34onq.fsf@gitster.dls.corp.google.com","subject":"Re: Unused #include statements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-16T00:00:36Z","receivedAt":"2015-01-16T00:00:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 15, 2015 at 03:20:09PM -0800, Junio C Hamano wrote:\n\n> OK, thanks. Let's queue something like this for post 2.3 cycle,\n> then.\n> \n> -- >8 --\n> Subject: CodingGuidelines: clarify C #include rules\n> [...]\n\nThanks, this looks good to me.\n\n-Peff\n"},{"id":"254911","messageId":"CAKJhZwRnu-jHydGzzXJeUrkhzK29mX7NAabF88r-ry-YrY6q9w@mail.gmail.com","threadId":"38367","inReplyTo":"xmqqvbk77u9m.fsf@gitster.dls.corp.google.com","subject":"Re: Unused #include statements","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2015-01-20T02:08:35Z","receivedAt":"2015-01-20T02:08:35Z","isPatch":false,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"Robert, Peff and Junio.\n\nThank you all for your feedback. It's clear now what sort of analysis I should\naim towards.\n\nThanks,\nZoltan\n"}]}