{"thread":{"id":"37557","subject":"[PATCH 1/2] check-headers: add header usage checks for .c files","startedAt":"2014-09-14T07:40:44Z","lastAt":"2014-09-15T19:49:16Z","messageCount":6,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"249381","messageId":"1410680445-84593-1-git-send-email-davvid@gmail.com","threadId":"37557","inReplyTo":null,"subject":"[PATCH 1/2] check-headers: add header usage checks for .c files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-09-14T07:40:44Z","receivedAt":"2014-09-14T07:40:44Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Teach check-header.sh to ensure that the first included header in .c\nfiles is either git-compat-util.h, builtin.h, or cache.h.\n\nEnsure that common-cmds.h is only included by help.c.\n\nMove the logic into functions so that we can skip parts of the check.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis depends on my previous patch that adds check-header.sh.\n\n check-headers.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 61 insertions(+), 5 deletions(-)\n\ndiff --git a/check-headers.sh b/check-headers.sh\nindex ef06f56..7f25e7a 100755\n--- a/check-headers.sh\n+++ b/check-headers.sh\n@@ -1,5 +1,9 @@\n #!/bin/sh\n \n+# This script is run via make.\n+# \"make check-headers SKIP_HEADER_CHECK=1\" skips the header dependency check.\n+# \"make check-headers SKIP_USAGE_CHECK=1\" skips the header usage check.\n+\n exit_code=0\n \n maybe_exit () {\n@@ -14,11 +18,11 @@ maybe_exit () {\n \tfi\n }\n \n-for header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n-do\n+check_header () {\n+\theader=\"$1\"\n \tcase \"$header\" in\n \tcommon-cmds.h)\n-\t\t# should only be included by help.c\n+\t\t# should only be included by help.c, not checked\n \t\t;;\n \t*)\n \t\tsubdir=$(dirname \"$header\") &&\n@@ -29,6 +33,58 @@ do\n \t\tmaybe_exit $?\n \t\t;;\n \tesac\n-done\n+}\n+\n+check_headers () {\n+\tfor header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n+\tdo\n+\t\tcheck_header \"$header\"\n+\tdone\n+}\n+\n+check_header_usage () {\n+\tfirst=$(grep '^#include' \"$1\" |\n+\t\thead -n1 |\n+\t\tsed -e 's,#include \",,' -e 's,\"$,,')\n+\n+\tcase \"$first\" in\n+\tcache.h|builtin.h|git-compat-util.h)\n+\t\t# happy\n+\t\t;;\n+\t*)\n+\t\techo \"error: $1 must #include \\\"git-compat-util.h\\\" before $first\"\n+\t\tmaybe_exit 1\n+\t\t;;\n+\tesac\n+\n+\tif grep common-cmds.h \"$1\" >/dev/null && test \"$1\" != help.c\n+\tthen\n+\t\techo \"error: $1 must not include common-cmds.h\"\n+\t\tmaybe_exit 1\n+\tfi\n+}\n+\n+check_usage () {\n+\t# Implementation files should #include git-compat-util.h, cache.h,\n+\t# or builtin.h before any others.\n+\tfor impl in *.c builtin/*.c\n+\tdo\n+\t\tcheck_header_usage \"$impl\"\n+\tdone\n+}\n+\n+main () {\n+\tif test -z \"$SKIP_HEADER_CHECK\"\n+\tthen\n+\t\tcheck_headers \"$@\"\n+\tfi\n+\n+\tif test -z \"$SKIP_USAGE_CHECK\"\n+\tthen\n+\t\tcheck_usage\n+\tfi\n+\n+\texit $exit_code\n+}\n \n-exit $exit_code\n+main \"$@\"\n-- \n2.1.0.241.ga16d620\n"},{"id":"249382","messageId":"1410680445-84593-2-git-send-email-davvid@gmail.com","threadId":"37557","inReplyTo":"1410680445-84593-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/2] cleanups: ensure that git-compat-util.h is included first","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-09-14T07:40:45Z","receivedAt":"2014-09-14T07:40:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"CodingGuidelines states that the first #include in C files should be\ngit-compat-util.h or another header file that includes it, such as\ncache.h or builtin.h.\n\nTweak the tiny minority of files that do not follow this advice.\nThis makes \"make check-headers SKIP_HEADER_CHECK=1\" happy.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n bulk-checkin.c    | 1 +\n bulk-checkin.h    | 2 --\n http.c            | 1 +\n merge-recursive.c | 2 +-\n sigchain.c        | 2 +-\n test-regex.c      | 2 +-\n test-sigchain.c   | 2 +-\n varint.c          | 1 +\n varint.h          | 2 --\n 9 files changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 98e651c..0c4b8a7 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -1,6 +1,7 @@\n /*\n  * Copyright (c) 2011, Google Inc.\n  */\n+#include \"cache.h\"\n #include \"bulk-checkin.h\"\n #include \"csum-file.h\"\n #include \"pack.h\"\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 4f599f8..fbd40fc 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -4,8 +4,6 @@\n #ifndef BULK_CHECKIN_H\n #define BULK_CHECKIN_H\n \n-#include \"cache.h\"\n-\n extern int index_bulk_checkin(unsigned char sha1[],\n \t\t\t      int fd, size_t size, enum object_type type,\n \t\t\t      const char *path, unsigned flags);\ndiff --git a/http.c b/http.c\nindex 0adcec4..040f362 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,3 +1,4 @@\n+#include \"git-compat-util.h\"\n #include \"http.h\"\n #include \"pack.h\"\n #include \"sideband.h\"\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8ab944c..df8157b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3,8 +3,8 @@\n  * Fredrik Kuivinen.\n  * The thieves were Alex Riesen and Johannes Schindelin, in June/July 2006\n  */\n-#include \"advice.h\"\n #include \"cache.h\"\n+#include \"advice.h\"\n #include \"cache-tree.h\"\n #include \"commit.h\"\n #include \"blob.h\"\ndiff --git a/sigchain.c b/sigchain.c\nindex 1118b99..faa375d 100644\n--- a/sigchain.c\n+++ b/sigchain.c\n@@ -1,5 +1,5 @@\n-#include \"sigchain.h\"\n #include \"cache.h\"\n+#include \"sigchain.h\"\n \n #define SIGCHAIN_MAX_SIGNALS 32\n \ndiff --git a/test-regex.c b/test-regex.c\nindex b5bfd54..0dc598e 100644\n--- a/test-regex.c\n+++ b/test-regex.c\n@@ -1,4 +1,4 @@\n-#include <git-compat-util.h>\n+#include \"git-compat-util.h\"\n \n int main(int argc, char **argv)\n {\ndiff --git a/test-sigchain.c b/test-sigchain.c\nindex 42db234..e499fce 100644\n--- a/test-sigchain.c\n+++ b/test-sigchain.c\n@@ -1,5 +1,5 @@\n-#include \"sigchain.h\"\n #include \"cache.h\"\n+#include \"sigchain.h\"\n \n #define X(f) \\\n static void f(int sig) { \\\ndiff --git a/varint.c b/varint.c\nindex 4ed7729..409c497 100644\n--- a/varint.c\n+++ b/varint.c\n@@ -1,3 +1,4 @@\n+#include \"git-compat-util.h\"\n #include \"varint.h\"\n \n uintmax_t decode_varint(const unsigned char **bufp)\ndiff --git a/varint.h b/varint.h\nindex 0321195..c1c44d9 100644\n--- a/varint.h\n+++ b/varint.h\n@@ -1,8 +1,6 @@\n #ifndef VARINT_H\n #define VARINT_H\n \n-#include \"git-compat-util.h\"\n-\n extern int encode_varint(uintmax_t, unsigned char *);\n extern uintmax_t decode_varint(const unsigned char **);\n \n-- \n2.1.0.241.ga16d620\n"},{"id":"249452","messageId":"xmqqr3zc66ho.fsf@gitster.dls.corp.google.com","threadId":"37557","inReplyTo":"1410680445-84593-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 1/2] check-headers: add header usage checks for .c files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-15T19:14:43Z","receivedAt":"2014-09-15T19:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> +check_header_usage () {\n> +\tfirst=$(grep '^#include' \"$1\" |\n> +\t\thead -n1 |\n> +\t\tsed -e 's,#include \",,' -e 's,\"$,,')\n\nperhaps a single \"sed\" invocation suffices?\n\n\tsed -n -e '/^#include/{\n        \ts/#include [\"<]\\(.*\\)\".*/\\1/p\n                q\n\t}' \"$1\"\n"},{"id":"249453","messageId":"xmqqk35466e1.fsf@gitster.dls.corp.google.com","threadId":"37557","inReplyTo":"1410680445-84593-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 1/2] check-headers: add header usage checks for .c files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-15T19:16:54Z","receivedAt":"2014-09-15T19:16:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Teach check-header.sh to ensure that the first included header in .c\n> files is either git-compat-util.h, builtin.h, or cache.h.\n>\n> Ensure that common-cmds.h is only included by help.c.\n>\n> Move the logic into functions so that we can skip parts of the check.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> This depends on my previous patch that adds check-header.sh.\n> ...\n> +check_headers () {\n> +\tfor header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n> +\tdo\n> +\t\tcheck_header \"$header\"\n\nHmmmm, doesn't check_header run \"$@\" as a command?\n"},{"id":"249454","messageId":"xmqqfvfs66ad.fsf@gitster.dls.corp.google.com","threadId":"37557","inReplyTo":"xmqqk35466e1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] check-headers: add header usage checks for .c files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-15T19:19:06Z","receivedAt":"2014-09-15T19:19:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> David Aguilar <davvid@gmail.com> writes:\n>\n>> Teach check-header.sh to ensure that the first included header in .c\n>> files is either git-compat-util.h, builtin.h, or cache.h.\n>>\n>> Ensure that common-cmds.h is only included by help.c.\n>>\n>> Move the logic into functions so that we can skip parts of the check.\n>>\n>> Signed-off-by: David Aguilar <davvid@gmail.com>\n>> ---\n>> This depends on my previous patch that adds check-header.sh.\n>> ...\n>> +check_headers () {\n>> +\tfor header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n>> +\tdo\n>> +\t\tcheck_header \"$header\"\n>\n> Hmmmm, doesn't check_header run \"$@\" as a command?\n\nTaking the previous two together, perhaps\n\n check-headers.sh | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/check-headers.sh b/check-headers.sh\nindex 7f25e7a..526381e 100755\n--- a/check-headers.sh\n+++ b/check-headers.sh\n@@ -20,6 +20,7 @@ maybe_exit () {\n \n check_header () {\n \theader=\"$1\"\n+\tshift\n \tcase \"$header\" in\n \tcommon-cmds.h)\n \t\t# should only be included by help.c, not checked\n@@ -38,15 +39,17 @@ check_header () {\n check_headers () {\n \tfor header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n \tdo\n-\t\tcheck_header \"$header\"\n+\t\tcheck_header \"$header\" \"$@\"\n \tdone\n }\n \n check_header_usage () {\n-\tfirst=$(grep '^#include' \"$1\" |\n-\t\thead -n1 |\n-\t\tsed -e 's,#include \",,' -e 's,\"$,,')\n-\n+\tfirst=$( \n+\t\tsed -n -e '/^#include/{\n+\t\t\ts/#include [\"<]\\(.*\\)\".*/\\1/p\n+\t\t\tq\n+\t\t}' \"$1\"\n+\t)\n \tcase \"$first\" in\n \tcache.h|builtin.h|git-compat-util.h)\n \t\t# happy\n"},{"id":"249456","messageId":"20140915194915.GA1740@gmail.com","threadId":"37557","inReplyTo":"xmqqfvfs66ad.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] check-headers: add header usage checks for .c files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-09-15T19:49:16Z","receivedAt":"2014-09-15T19:49:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Sep 15, 2014 at 12:19:06PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > David Aguilar <davvid@gmail.com> writes:\n> >\n> >> Teach check-header.sh to ensure that the first included header in .c\n> >> files is either git-compat-util.h, builtin.h, or cache.h.\n> >>\n> >> Ensure that common-cmds.h is only included by help.c.\n> >>\n> >> Move the logic into functions so that we can skip parts of the check.\n> >>\n> >> Signed-off-by: David Aguilar <davvid@gmail.com>\n> >> ---\n> >> This depends on my previous patch that adds check-header.sh.\n> >> ...\n> >> +check_headers () {\n> >> +\tfor header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n> >> +\tdo\n> >> +\t\tcheck_header \"$header\"\n> >\n> > Hmmmm, doesn't check_header run \"$@\" as a command?\n> \n> Taking the previous two together, perhaps\n\nThanks, yes, that's better.\n\n\n>  check-headers.sh | 13 ++++++++-----\n>  1 file changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/check-headers.sh b/check-headers.sh\n> index 7f25e7a..526381e 100755\n> --- a/check-headers.sh\n> +++ b/check-headers.sh\n> @@ -20,6 +20,7 @@ maybe_exit () {\n>  \n>  check_header () {\n>  \theader=\"$1\"\n> +\tshift\n>  \tcase \"$header\" in\n>  \tcommon-cmds.h)\n>  \t\t# should only be included by help.c, not checked\n> @@ -38,15 +39,17 @@ check_header () {\n>  check_headers () {\n>  \tfor header in *.h ewah/*.h vcs-svn/*.h xdiff/*.h\n>  \tdo\n> -\t\tcheck_header \"$header\"\n> +\t\tcheck_header \"$header\" \"$@\"\n>  \tdone\n>  }\n>  \n>  check_header_usage () {\n> -\tfirst=$(grep '^#include' \"$1\" |\n> -\t\thead -n1 |\n> -\t\tsed -e 's,#include \",,' -e 's,\"$,,')\n> -\n> +\tfirst=$( \n> +\t\tsed -n -e '/^#include/{\n> +\t\t\ts/#include [\"<]\\(.*\\)\".*/\\1/p\n> +\t\t\tq\n> +\t\t}' \"$1\"\n> +\t)\n>  \tcase \"$first\" in\n>  \tcache.h|builtin.h|git-compat-util.h)\n>  \t\t# happy\n\n-- \nDavid\n"}]}