# [PATCH] help: ensure that common-cmds.h is only included by help.c 6 messages from 2014-09-14 to 2014-09-14. Participants: David Aguilar, Perry Hutchison, Junio C Hamano. Thread: https://gitlist.dev/t/37554 ## David Aguilar, 2014-09-14 01:11 Subject: [PATCH] help: ensure that common-cmds.h is only included by help.c Message-ID: <1410657073-3089-1-git-send-email-davvid@gmail.com> URL: https://gitlist.dev/e/1410657073-3089-1-git-send-email-davvid%40gmail.com ``` Add a #ifndef guard to ensure that common-cmds.h can only be included by help.c. Suggested-by: Junio C Hamano Signed-off-by: David Aguilar --- generate-cmdlist.sh | 4 ++++ help.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh index 9a4c9b9..99cd140 100755 --- a/generate-cmdlist.sh +++ b/generate-cmdlist.sh @@ -1,6 +1,10 @@ #!/bin/sh echo "/* Automatically generated by $0 */ +#ifndef GIT_HELP_INTERNAL +#error \"common-cmds.h can only be included by help.c\" +#endif + struct cmdname_help { char name[16]; char help[80]; diff --git a/help.c b/help.c index 7af65e2..abf1689 100644 --- a/help.c +++ b/help.c @@ -3,11 +3,12 @@ #include "exec_cmd.h" #include "levenshtein.h" #include "help.h" -#include "common-cmds.h" #include "string-list.h" #include "column.h" #include "version.h" #include "refs.h" +#define GIT_HELP_INTERNAL +#include "common-cmds.h" void add_cmdname(struct cmdnames *cmds, const char *name, int len) { -- 2.1.0.241.ga16d620 ``` ## Perry Hutchison, 2014-09-14 02:00 Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c Message-ID: <5414f6b2.XwoOyI03zlXiZ3ds%perryh@pluto.rain.com> URL: https://gitlist.dev/e/5414f6b2.XwoOyI03zlXiZ3ds%25perryh%40pluto.rain.com In-Reply-To: <1410657073-3089-1-git-send-email-davvid@gmail.com> ``` David Aguilar wrote: > Add a #ifndef guard to ensure that common-cmds.h can only > be included by help.c. This strikes me as a very peculiar, and sub-optimal, way of achieving the purpose. If these definitions are intended to be private to help.c, why not put them there and eliminate common-cmds.h entirely? ``` ## Junio C Hamano, 2014-09-14 05:23 Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c Message-ID: URL: https://gitlist.dev/e/CAPc5daXDCAi3eP3YmXfcO%2B9ncN8%3Db6tCGUFUxwKE%3DMuRBuXvEg%40mail.gmail.com In-Reply-To: <5414f6b2.XwoOyI03zlXiZ3ds%perryh@pluto.rain.com> ``` On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote: > David Aguilar wrote: >> Add a #ifndef guard to ensure that common-cmds.h can only >> be included by help.c. > > This strikes me as a very peculiar, and sub-optimal, way of > achieving the purpose. If these definitions are intended to > be private to help.c, why not put them there and eliminate > common-cmds.h entirely? Have you studied where common-cmds.h comes from? After you have done so, would you make the same suggestion? Having said that, I also do not think this is such a good idea. Wouldn't the new "check" script added in this series a better place? For example, it may want to make sure that git-compat-util.h (or a couple of its equivalents) is the first file included in any mainline C source file, and such an inclusion is done unconditionally. Which would mean that the checker would scan *.c files with grep or a Perl script. It would be trivial to enforce "nobody other than these small selected C files is allowed to include common-cmds.h" rule. Regarding the other patch that butchers many *.h files, I am not still very enthused. Including cache.h at the beginning of branch.h, for example, would mean git-compat-util.h ends up included at the beginning of branch.h. ``` ## David Aguilar, 2014-09-14 07:35 Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c Message-ID: <20140914073511.GA39361@gmail.com> URL: https://gitlist.dev/e/20140914073511.GA39361%40gmail.com In-Reply-To: ``` On Sat, Sep 13, 2014 at 10:23:03PM -0700, Junio C Hamano wrote: > On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote: > > David Aguilar wrote: > >> Add a #ifndef guard to ensure that common-cmds.h can only > >> be included by help.c. > > > > This strikes me as a very peculiar, and sub-optimal, way of > > achieving the purpose. If these definitions are intended to > > be private to help.c, why not put them there and eliminate > > common-cmds.h entirely? > > Have you studied where common-cmds.h comes from? > After you have done so, would you make the same suggestion? > > Having said that, I also do not think this is such a good idea. > Wouldn't the new "check" script added in this series a better > place? For example, it may want to make sure that git-compat-util.h > (or a couple of its equivalents) is the first file included in any mainline > C source file, and such an inclusion is done unconditionally. > > Which would mean that the checker would scan *.c files with grep > or a Perl script. It would be trivial to enforce "nobody other than these > small selected C files is allowed to include common-cmds.h" rule. Good idea. I implemented this check and the tweaks to make it pass are small and focused. I'll send these patches shortly. > Regarding the other patch that butchers many *.h files, I am not > still very enthused. Including cache.h at the beginning of branch.h, > for example, would mean git-compat-util.h ends up included at the > beginning of branch.h. I can look into Jonathan's forward-decl approach later too. That'll probably result in less of a butchering. -- David ``` ## Perry Hutchison, 2014-09-14 07:55 Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c Message-ID: <541549fd.u4o+i0ruC2hh0cGO%perryh@pluto.rain.com> URL: https://gitlist.dev/e/541549fd.u4o%2Bi0ruC2hh0cGO%25perryh%40pluto.rain.com In-Reply-To: ``` Junio C Hamano wrote: > On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote: > > David Aguilar wrote: > >> Add a #ifndef guard to ensure that common-cmds.h can only > >> be included by help.c. > > > > ... If these definitions are intended to be private to help.c, > > why not put them there and eliminate common-cmds.h entirely? > > Have you studied where common-cmds.h comes from? Not when I wrote that :) > After you have done so, would you make the same suggestion? Yes, as a matter of using C in a conventional and non-hackish manner. The normal and expected purpose of .h files is to share definitions among compilation units. If certain definitions are -- by design -- intended to be used in only a single compilation unit, they should not be put in a .h file; that only encourages other programmers who come along later to use those definitions in an incorrect way. If it's too much trouble to have the auto-generation mechanism insert the definitions into help.c where they belong, at least name the auto-generated file something else, like commands-auto.res or command-list.txt. A #include file's name does not _have_ to end in .h, and avoiding the .h convention in a case such as this would make it blatantly obvious that something unconventional is being done. ``` ## David Aguilar, 2014-09-14 08:44 Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c Message-ID: <20140914084429.GA74582@gmail.com> URL: https://gitlist.dev/e/20140914084429.GA74582%40gmail.com In-Reply-To: <541549fd.u4o+i0ruC2hh0cGO%perryh@pluto.rain.com> ``` On Sun, Sep 14, 2014 at 12:55:41AM -0700, Perry Hutchison wrote: > Junio C Hamano wrote: > > On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison wrote: > > > David Aguilar wrote: > > >> Add a #ifndef guard to ensure that common-cmds.h can only > > >> be included by help.c. > > > > > > ... If these definitions are intended to be private to help.c, > > > why not put them there and eliminate common-cmds.h entirely? > > > > Have you studied where common-cmds.h comes from? > > Not when I wrote that :) > > > After you have done so, would you make the same suggestion? > > Yes, as a matter of using C in a conventional and non-hackish > manner. The normal and expected purpose of .h files is to share > definitions among compilation units. If certain definitions are > -- by design -- intended to be used in only a single compilation > unit, they should not be put in a .h file; that only encourages > other programmers who come along later to use those definitions > in an incorrect way. > > If it's too much trouble to have the auto-generation mechanism > insert the definitions into help.c where they belong, at least > name the auto-generated file something else, like commands-auto.res > or command-list.txt. A #include file's name does not _have_ to > end in .h, and avoiding the .h convention in a case such as this > would make it blatantly obvious that something unconventional is > being done. I would generally agree. Nonetheless, I was able to implement this check without touching any of these files so these should probably stay as-is. Thanks, -- David ```