{"thread":{"id":"18717","subject":"[PATCH] revision.h: add includes of \"diff.h\" and \"commit.h\"","startedAt":"2009-04-04T19:45:54Z","lastAt":"2009-04-05T08:02:09Z","messageCount":3,"participants":["Christian Couder","Nanako Shiraishi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"110341","messageId":"20090404214554.75ada117.chriscool@tuxfamily.org","threadId":"18717","inReplyTo":null,"subject":"[PATCH] revision.h: add includes of \"diff.h\" and \"commit.h\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-04-04T19:45:54Z","receivedAt":"2009-04-04T19:45:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Because they are needed by some features included in\n\"revision.h\".\n\nThis makes the following just work:\n\n        $ cat >1.c <<\\EOF\n        #include \"cache.h\"\n        #include \"revision.h\"\n        EOF\n        $ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n revision.h |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/revision.h b/revision.h\nindex 5adfc91..495d7eb 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -3,6 +3,8 @@\n \n #include \"parse-options.h\"\n #include \"grep.h\"\n+#include \"diff.h\"\n+#include \"commit.h\"\n \n #define SEEN\t\t(1u<<0)\n #define UNINTERESTING   (1u<<1)\n-- \n1.6.2.1.530.g516a7.dirty\n"},{"id":"110387","messageId":"20090405162841.6117@nanako3.lavabit.com","threadId":"18717","inReplyTo":"20090404214554.75ada117.chriscool@tuxfamily.org","subject":"Re: [PATCH] revision.h: add includes of \"diff.h\" and \"commit.h\"","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-04-05T07:28:41Z","receivedAt":"2009-04-05T07:28:41Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Christian Couder <chriscool@tuxfamily.org>:\n\n> Because they are needed by some features included in\n> \"revision.h\".\n>\n> This makes the following just work:\n>\n>         $ cat >1.c <<\\EOF\n>         #include \"cache.h\"\n>         #include \"revision.h\"\n>         EOF\n>         $ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c\n\nI'm sorry if this is obvious to experienced people, but I don't understand what benefit there is to make such an empty program compilable.\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"110388","messageId":"7v3acnzfy6.fsf@gitster.siamese.dyndns.org","threadId":"18717","inReplyTo":"20090405162841.6117@nanako3.lavabit.com","subject":"Re: [PATCH] revision.h: add includes of \"diff.h\" and \"commit.h\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-05T08:02:09Z","receivedAt":"2009-04-05T08:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nanako Shiraishi <nanako3@lavabit.com> writes:\n\n> Quoting Christian Couder <chriscool@tuxfamily.org>:\n>\n>> Because they are needed by some features included in\n>> \"revision.h\".\n>>\n>> This makes the following just work:\n>>\n>>         $ cat >1.c <<\\EOF\n>>         #include \"cache.h\"\n>>         #include \"revision.h\"\n>>         EOF\n>>         $ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c\n>\n> I'm sorry if this is obvious to experienced people, but I don't understand what benefit there is to make such an empty program compilable.\n\nI believe this was prompted by my earlier comment on the header clean-up\n($gmane/115443), but I'd say it does not make much sense.  I have already\nexplained why it doesn't, and in addition, as the above example shows, you\nstill have to include \"cache.h\" in your 1.c file anyway, so it is not\nmaking the header \"usable standalone\" either.\n\nIf you have a follow-up patch that removes the inclusion of diff.h and\ncommit.h to millions of .c files that already include revision.h, it might\nstart to make sense, but it goes against one of the rules Christian wanted\nto add, namely:\n\n    a header file should be included in a C file only if it is needed to \n    compile the C file (it is not ok to include it only because it includes \n    many other headers that are needed)\n\nin the sense that if somebody wants to run diff in his C code, he should\nexplicitly include diff.h (or diffcore.h if necessary), instead of relying\non the fact that revision.h happens to include it, and he happens to\ninclude revision.h because he uses setup_revisions() to parse the command\nline arguments (and I happen to think that guideline makes sense).\n\nEven though including the same .h file twice is protected with the\nstandard:\n\n\t#ifndef FROTZ_H\n        #define FROTZ_H\n        ...\n        #endif\n\nit does make C preprocessor do extra work to open the header twice (and\nskip the whole file in its second inclusion), so there is a slight\nperformance issue.\n\nYou can argue revision.h is somewhat special---it are so central that\nalmost all core-ish history inspection commands in git revolve around\nthem, and it is not particularly a bad idea to say \"you can rely on\nrevision.h to include diff.h\" in practice.  That would give you an escape\nhatch to omit inclusion of diff.h from programs that include revision.h\nand avoid the performance issue.\n\nBut then that introduces new rules on which ones are special and which\nones are not, and overall it does not help simplifying the life of the\nprogrammers.\n\nSo I do not feel strongly supportive about this patch.\n"}]}