{"thread":{"id":"37554","subject":"[PATCH] help: ensure that common-cmds.h is only included by help.c","startedAt":"2014-09-14T01:11:13Z","lastAt":"2014-09-14T08:44:30Z","messageCount":6,"participants":["David Aguilar","Perry Hutchison","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249372","messageId":"1410657073-3089-1-git-send-email-davvid@gmail.com","threadId":"37554","inReplyTo":null,"subject":"[PATCH] help: ensure that common-cmds.h is only included by help.c","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-09-14T01:11:13Z","receivedAt":"2014-09-14T01:11:13Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Add a #ifndef guard to ensure that common-cmds.h can only\nbe included by help.c.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: David Aguilar \n---\n generate-cmdlist.sh | 4 ++++\n help.c | 3 ++-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/generate-cmdlist.sh b/generate-cmdlist.sh\nindex 9a4c9b9..99cd140 100755\n--- a/generate-cmdlist.sh\n+++ b/generate-cmdlist.sh\n@@ -1,6 +1,10 @@\n #!/bin/sh\n \n echo \"/* Automatically generated by $0 */\n+#ifndef GIT_HELP_INTERNAL\n+#error \\\"common-cmds.h can only be included by help.c\\\"\n+#endif\n+\n struct cmdname_help {\n char name[16];\n char help[80];\ndiff --git a/help.c b/help.c\nindex 7af65e2..abf1689 100644\n--- a/help.c\n+++ b/help.c\n@@ -3,11 +3,12 @@\n #include \"exec_cmd.h\"\n #include \"levenshtein.h\"\n #include \"help.h\"\n-#include \"common-cmds.h\"\n #include \"string-list.h\"\n #include \"column.h\"\n #include \"version.h\"\n #include \"refs.h\"\n+#define GIT_HELP_INTERNAL\n+#include \"common-cmds.h\"\n \n void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n {\n-- \n2.1.0.241.ga16d620\n"},{"id":"249374","messageId":"5414f6b2.XwoOyI03zlXiZ3ds%perryh@pluto.rain.com","threadId":"37554","inReplyTo":"1410657073-3089-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] help: ensure that common-cmds.h is only included by help.c","fromName":"Perry Hutchison","fromEmail":"perryh@pluto.rain.com","sentAt":"2014-09-14T02:00:18Z","receivedAt":"2014-09-14T02:00:18Z","isPatch":true,"sender":{"key":"perryh@pluto.rain.com","avatar":null},"body":"David Aguilar wrote:\n> Add a #ifndef guard to ensure that common-cmds.h can only\n> be included by help.c.\n\nThis strikes me as a very peculiar, and sub-optimal, way of\nachieving the purpose. If these definitions are intended to\nbe private to help.c, why not put them there and eliminate\ncommon-cmds.h entirely?\n"},{"id":"249375","messageId":"CAPc5daXDCAi3eP3YmXfcO+9ncN8=b6tCGUFUxwKE=MuRBuXvEg@mail.gmail.com","threadId":"37554","inReplyTo":"5414f6b2.XwoOyI03zlXiZ3ds%perryh@pluto.rain.com","subject":"Re: [PATCH] help: ensure that common-cmds.h is only included by help.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-14T05:23:03Z","receivedAt":"2014-09-14T05:23:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote:\n> David Aguilar wrote:\n>> Add a #ifndef guard to ensure that common-cmds.h can only\n>> be included by help.c.\n>\n> This strikes me as a very peculiar, and sub-optimal, way of\n> achieving the purpose. If these definitions are intended to\n> be private to help.c, why not put them there and eliminate\n> common-cmds.h entirely?\n\nHave you studied where common-cmds.h comes from?\nAfter you have done so, would you make the same suggestion?\n\nHaving said that, I also do not think this is such a good idea.\nWouldn't the new \"check\" script added in this series a better\nplace? For example, it may want to make sure that git-compat-util.h\n(or a couple of its equivalents) is the first file included in any mainline\nC source file, and such an inclusion is done unconditionally.\n\nWhich would mean that the checker would scan *.c files with grep\nor a Perl script. It would be trivial to enforce \"nobody other than these\nsmall selected C files is allowed to include common-cmds.h\" rule.\n\nRegarding the other patch that butchers many *.h files, I am not\nstill very enthused. Including cache.h at the beginning of branch.h,\nfor example, would mean git-compat-util.h ends up included at the\nbeginning of branch.h.\n"},{"id":"249380","messageId":"20140914073511.GA39361@gmail.com","threadId":"37554","inReplyTo":"CAPc5daXDCAi3eP3YmXfcO+9ncN8=b6tCGUFUxwKE=MuRBuXvEg@mail.gmail.com","subject":"Re: [PATCH] help: ensure that common-cmds.h is only included by help.c","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-09-14T07:35:13Z","receivedAt":"2014-09-14T07:35:13Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Sep 13, 2014 at 10:23:03PM -0700, Junio C Hamano wrote:\n> On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote:\n> > David Aguilar wrote:\n> >> Add a #ifndef guard to ensure that common-cmds.h can only\n> >> be included by help.c.\n> >\n> > This strikes me as a very peculiar, and sub-optimal, way of\n> > achieving the purpose. If these definitions are intended to\n> > be private to help.c, why not put them there and eliminate\n> > common-cmds.h entirely?\n> \n> Have you studied where common-cmds.h comes from?\n> After you have done so, would you make the same suggestion?\n> \n> Having said that, I also do not think this is such a good idea.\n> Wouldn't the new \"check\" script added in this series a better\n> place? For example, it may want to make sure that git-compat-util.h\n> (or a couple of its equivalents) is the first file included in any mainline\n> C source file, and such an inclusion is done unconditionally.\n> \n> Which would mean that the checker would scan *.c files with grep\n> or a Perl script. It would be trivial to enforce \"nobody other than these\n> small selected C files is allowed to include common-cmds.h\" rule.\n\nGood idea. I implemented this check and the tweaks to make it\npass are small and focused. I'll send these patches shortly.\n\n> Regarding the other patch that butchers many *.h files, I am not\n> still very enthused. Including cache.h at the beginning of branch.h,\n> for example, would mean git-compat-util.h ends up included at the\n> beginning of branch.h.\n\nI can look into Jonathan's forward-decl approach later too.\nThat'll probably result in less of a butchering.\n-- \nDavid\n"},{"id":"249385","messageId":"541549fd.u4o+i0ruC2hh0cGO%perryh@pluto.rain.com","threadId":"37554","inReplyTo":"CAPc5daXDCAi3eP3YmXfcO+9ncN8=b6tCGUFUxwKE=MuRBuXvEg@mail.gmail.com","subject":"Re: [PATCH] help: ensure that common-cmds.h is only included by help.c","fromName":"Perry Hutchison","fromEmail":"perryh@pluto.rain.com","sentAt":"2014-09-14T07:55:41Z","receivedAt":"2014-09-14T07:55:41Z","isPatch":true,"sender":{"key":"perryh@pluto.rain.com","avatar":null},"body":"Junio C Hamano wrote:\n> On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote:\n> > David Aguilar wrote:\n> >> Add a #ifndef guard to ensure that common-cmds.h can only\n> >> be included by help.c.\n> >\n> > ... If these definitions are intended to be private to help.c,\n> > why not put them there and eliminate common-cmds.h entirely?\n>\n> Have you studied where common-cmds.h comes from?\n\nNot when I wrote that :)\n\n> After you have done so, would you make the same suggestion?\n\nYes, as a matter of using C in a conventional and non-hackish\nmanner. The normal and expected purpose of .h files is to share\ndefinitions among compilation units. If certain definitions are\n-- by design -- intended to be used in only a single compilation\nunit, they should not be put in a .h file; that only encourages\nother programmers who come along later to use those definitions\nin an incorrect way.\n\nIf it's too much trouble to have the auto-generation mechanism \ninsert the definitions into help.c where they belong, at least\nname the auto-generated file something else, like commands-auto.res\nor command-list.txt. A #include file's name does not _have_ to\nend in .h, and avoiding the .h convention in a case such as this\nwould make it blatantly obvious that something unconventional is\nbeing done.\n"},{"id":"249391","messageId":"20140914084429.GA74582@gmail.com","threadId":"37554","inReplyTo":"541549fd.u4o+i0ruC2hh0cGO%perryh@pluto.rain.com","subject":"Re: [PATCH] help: ensure that common-cmds.h is only included by help.c","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-09-14T08:44:30Z","receivedAt":"2014-09-14T08:44:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Sep 14, 2014 at 12:55:41AM -0700, Perry Hutchison wrote:\n> Junio C Hamano wrote:\n> > On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote:\n> > > David Aguilar wrote:\n> > >> Add a #ifndef guard to ensure that common-cmds.h can only\n> > >> be included by help.c.\n> > >\n> > > ... If these definitions are intended to be private to help.c,\n> > > why not put them there and eliminate common-cmds.h entirely?\n> >\n> > Have you studied where common-cmds.h comes from?\n> \n> Not when I wrote that :)\n> \n> > After you have done so, would you make the same suggestion?\n> \n> Yes, as a matter of using C in a conventional and non-hackish\n> manner. The normal and expected purpose of .h files is to share\n> definitions among compilation units. If certain definitions are\n> -- by design -- intended to be used in only a single compilation\n> unit, they should not be put in a .h file; that only encourages\n> other programmers who come along later to use those definitions\n> in an incorrect way.\n> \n> If it's too much trouble to have the auto-generation mechanism \n> insert the definitions into help.c where they belong, at least\n> name the auto-generated file something else, like commands-auto.res\n> or command-list.txt. A #include file's name does not _have_ to\n> end in .h, and avoiding the .h convention in a case such as this\n> would make it blatantly obvious that something unconventional is\n> being done.\n\nI would generally agree. Nonetheless, I was able to implement\nthis check without touching any of these files so these should\nprobably stay as-is.\n\nThanks,\n-- \nDavid\n"}]}