{"thread":{"id":"32358","subject":"[PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages","startedAt":"2012-12-16T19:35:58Z","lastAt":"2012-12-22T18:39:19Z","messageCount":10,"participants":["Adam Spiers","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"204981","messageId":"1355686561-1057-1-git-send-email-git@adamspiers.org","threadId":"32358","inReplyTo":"7vobl0804s.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/3] Help newbie git developers avoid obvious pitfalls","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-16T19:35:58Z","receivedAt":"2012-12-16T19:35:58Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"I fell into various newbie pitfalls when submitting my first patches\nto git, despite my best attempts to adhere to documented guidelines.\nThis small patch series attempts to reduce the chances of other\ndevelopers making the same mistakes I did.\n\nAdam Spiers (3):\n  SubmittingPatches: add convention of prefixing commit messages\n  Documentation: move support for old compilers to CodingGuidelines\n  Makefile: use -Wdeclaration-after-statement if supported\n\n Documentation/CodingGuidelines  |  8 ++++++++\n Documentation/SubmittingPatches | 21 ++++++++-------------\n Makefile                        |  7 ++++++-\n 3 files changed, 22 insertions(+), 14 deletions(-)\n\n-- \n1.7.12.1.396.g53b3ea9\n"},{"id":"204980","messageId":"1355686561-1057-2-git-send-email-git@adamspiers.org","threadId":"32358","inReplyTo":"1355686561-1057-1-git-send-email-git@adamspiers.org","subject":"[PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-16T19:35:59Z","receivedAt":"2012-12-16T19:35:59Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"Conscientious newcomers to git development will read SubmittingPatches\nand CodingGuidelines, but could easily miss the convention of\nprefixing commit messages with a single word identifying the file\nor area the commit touches.\n\nSigned-off-by: Adam Spiers <git@adamspiers.org>\n---\n Documentation/SubmittingPatches | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 0dbf2c9..c107cb1 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -9,6 +9,14 @@ Checklist (and a short version for the impatient):\n \t- the first line of the commit message should be a short\n \t  description (50 characters is the soft limit, see DISCUSSION\n \t  in git-commit(1)), and should skip the full stop\n+\t- it is also conventional in most cases to prefix the\n+\t  first line with \"area: \" where the area is a filename\n+\t  or identifier for the general area of the code being\n+\t  modified, e.g.\n+\t  . archive: ustar header checksum is computed unsigned\n+\t  . git-cherry-pick.txt: clarify the use of revision range notation\n+\t  (if in doubt which identifier to use, run \"git log --no-merges\"\n+\t  on the files you are modifying to see the current conventions)\n \t- the body should provide a meaningful commit message, which:\n \t  . explains the problem the change tries to solve, iow, what\n \t    is wrong with the current code without the change.\n-- \n1.7.12.1.396.g53b3ea9\n"},{"id":"204983","messageId":"1355686561-1057-3-git-send-email-git@adamspiers.org","threadId":"32358","inReplyTo":"1355686561-1057-1-git-send-email-git@adamspiers.org","subject":"[PATCH 2/3] Documentation: move support for old compilers to CodingGuidelines","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-16T19:36:00Z","receivedAt":"2012-12-16T19:36:00Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"The \"Try to be nice to older C compilers\" text is clearly a guideline\nto be borne in mind whilst coding rather than when submitting patches.\n\nSigned-off-by: Adam Spiers <git@adamspiers.org>\n---\n Documentation/CodingGuidelines  |  8 ++++++++\n Documentation/SubmittingPatches | 13 -------------\n 2 files changed, 8 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 57da6aa..69f7e9b 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -112,6 +112,14 @@ For C programs:\n \n  - We try to keep to at most 80 characters per line.\n \n+ - We try to support a wide range of C compilers to compile git with,\n+   including old ones. That means that you should not use C99\n+   initializers, even if a lot of compilers grok it.\n+\n+ - Variables have to be declared at the beginning of the block.\n+\n+ - NULL pointers shall be written as NULL, not as 0.\n+\n  - When declaring pointers, the star sides with the variable\n    name, i.e. \"char *string\", not \"char* string\" or\n    \"char * string\".  This makes it easier to understand code\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex c107cb1..c34c9d1 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -127,19 +127,6 @@ in templates/hooks--pre-commit.  To help ensure this does not happen,\n run git diff --check on your changes before you commit.\n \n \n-(1a) Try to be nice to older C compilers\n-\n-We try to support a wide range of C compilers to compile\n-git with. That means that you should not use C99 initializers, even\n-if a lot of compilers grok it.\n-\n-Also, variables have to be declared at the beginning of the block\n-(you can check this with gcc, using the -Wdeclaration-after-statement\n-option).\n-\n-Another thing: NULL pointers shall be written as NULL, not as 0.\n-\n-\n (2) Generate your patch using git tools out of your commits.\n \n git based diff tools generate unidiff which is the preferred format.\n-- \n1.7.12.1.396.g53b3ea9\n"},{"id":"204982","messageId":"1355686561-1057-4-git-send-email-git@adamspiers.org","threadId":"32358","inReplyTo":"1355686561-1057-1-git-send-email-git@adamspiers.org","subject":"[PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-16T19:36:01Z","receivedAt":"2012-12-16T19:36:01Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"CodingGuidelines requests that code should be nice to older C compilers.\nSince modern gcc can warn on code written using newer dialects such as C99,\nit makes sense to take advantage of this by auto-detecting this capability\nand enabling it when found.\n\nSigned-off-by: Adam Spiers <git@adamspiers.org>\n---\nIf we adopt this approach, it may make sense to enable other flags\nwhere available (e.g. -Wzero-as-null-pointer-constant, maybe even\n-ansi).  In that case, something like this might be a more efficient\nway of writing it:\n\n    GCC_FLAGS=-Wdeclaration-after-statement,-Wanother-flag,-Wand-another\n    GCC_FLAGS_REGEXP=$(shell echo $(GCC_FLAGS) | sed 's/,/\\\\|/g')\n    GCC_SUPPORTED_FLAGS=$(shell cc --help -v 2>&1 | \\\n            sed -n '/.* \\($(GCC_FLAGS_REGEXP)\\) .*/{s//\\1/;p}')\n\n Makefile | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex a49d1db..aae70d4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -331,8 +331,13 @@ endif\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n+GCC_DECL_AFTER_STATEMENT = \\\n+\t$(shell $(CC) --help -v 2>&1 | \\\n+\t\tgrep -q -- -Wdeclaration-after-statement && \\\n+\t  echo -Wdeclaration-after-statement)\n+GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)\n+ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)\n LDFLAGS =\n-ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n STRIP ?= strip\n \n-- \n1.7.12.1.396.g53b3ea9\n"},{"id":"204993","messageId":"7vobhtpp1d.fsf@alter.siamese.dyndns.org","threadId":"32358","inReplyTo":"1355686561-1057-2-git-send-email-git@adamspiers.org","subject":"Re: [PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-16T23:15:10Z","receivedAt":"2012-12-16T23:15:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers <git@adamspiers.org> writes:\n\n> Conscientious newcomers to git development will read SubmittingPatches\n> and CodingGuidelines, but could easily miss the convention of\n> prefixing commit messages with a single word identifying the file\n> or area the commit touches.\n>\n> Signed-off-by: Adam Spiers <git@adamspiers.org>\n> ---\n>  Documentation/SubmittingPatches | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n>\n> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\n> index 0dbf2c9..c107cb1 100644\n> --- a/Documentation/SubmittingPatches\n> +++ b/Documentation/SubmittingPatches\n> @@ -9,6 +9,14 @@ Checklist (and a short version for the impatient):\n>  \t- the first line of the commit message should be a short\n>  \t  description (50 characters is the soft limit, see DISCUSSION\n>  \t  in git-commit(1)), and should skip the full stop\n> +\t- it is also conventional in most cases to prefix the\n> +\t  first line with \"area: \" where the area is a filename\n> +\t  or identifier for the general area of the code being\n> +\t  modified, e.g.\n> +\t  . archive: ustar header checksum is computed unsigned\n> +\t  . git-cherry-pick.txt: clarify the use of revision range notation\n> +\t  (if in doubt which identifier to use, run \"git log --no-merges\"\n> +\t  on the files you are modifying to see the current conventions)\n\nThanks; I have to wonder if these details should be left in the\nlonger version to keep the \"short\" one short, though.\n\nWe should probably add \"learn from good examples.\" (aka \"read 'git\nlog' output and the pattern should be obvious to you\") as the first\nitem to this list, too.\n\n>  \t- the body should provide a meaningful commit message, which:\n>  \t  . explains the problem the change tries to solve, iow, what\n>  \t    is wrong with the current code without the change.\n"},{"id":"204996","messageId":"7vk3shphru.fsf@alter.siamese.dyndns.org","threadId":"32358","inReplyTo":"1355686561-1057-4-git-send-email-git@adamspiers.org","subject":"Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-17T01:52:05Z","receivedAt":"2012-12-17T01:52:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers <git@adamspiers.org> writes:\n\n> If we adopt this approach,...\n> diff --git a/Makefile b/Makefile\n> index a49d1db..aae70d4 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -331,8 +331,13 @@ endif\n>  # CFLAGS and LDFLAGS are for the users to override from the command line.\n>  \n>  CFLAGS = -g -O2 -Wall\n> +GCC_DECL_AFTER_STATEMENT = \\\n> +\t$(shell $(CC) --help -v 2>&1 | \\\n> +\t\tgrep -q -- -Wdeclaration-after-statement && \\\n> +\t  echo -Wdeclaration-after-statement)\n> +GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)\n> +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)\n>  LDFLAGS =\n> -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n>  ALL_LDFLAGS = $(LDFLAGS)\n\n\nPlease do not do this.\n\nPeople cannot disable it from the command line, like:\n\n    $ make V=1 CFLAGS='-g -O0 -Wall'\n\nIf anything, this should be part of the default CFLAGS.\n\nMore importantly, this will run the $(shell ...) struct once for\nevery *.o file we produce, I think, in addition to running it twice\nfor the whole build.  If you add this:\n\n@@ -345,7 +345,8 @@ CFLAGS = -g -O2 -Wall\n GCC_DECL_AFTER_STATEMENT = \\\n \t$(shell $(CC) --help -v 2>&1 | \\\n \t\tgrep -q -- -Wdeclaration-after-statement && \\\n-\t  echo -Wdeclaration-after-statement)\n+\t  echo -Wdeclaration-after-statement; \\\n+\t  echo >&2 GCC_DECL_AFTER_STATEMENT CRUFT)\n GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)\n LDFLAGS =\n\nremove git.o and dir.o from a fully built tree, and then try to\nrebuild these two files, you will get this:\n\n    $ make V=1 git.o dir.o\n    GCC_DECL_AFTER_STATEMENT CRUFT\n    GCC_DECL_AFTER_STATEMENT CRUFT\n    GCC_DECL_AFTER_STATEMENT CRUFT\n    cc -o git.o -c -MF ./.depend/git.o.d -MMD -MP  -g -O2 -Wall \\\n    -Wdeclaration-after-statement -I.  -DHAVE_PATHS_H -DHAVE_DEV_TTY \\\n    -DXDL_FAST_HASH -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY \\\n    -DNO_MKSTEMPS -DSHELL_PATH='\"/bin/sh\"' \\\n    '-DGIT_HTML_PATH=\"share/doc/git-doc\"' '-DGIT_MAN_PATH=\"share/man\"' \\\n    '-DGIT_INFO_PATH=\"share/info\"' git.c\n    GCC_DECL_AFTER_STATEMENT CRUFT\n    cc -o dir.o -c -MF ./.depend/dir.o.d -MMD -MP  -g -O2 -Wall \\\n    -Wdeclaration-after-statement -I.  -DHAVE_PATHS_H -DHAVE_DEV_TTY \\\n    -DXDL_FAST_HASH -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY \\\n    -DNO_MKSTEMPS -DSHELL_PATH='\"/bin/sh\"'  dir.c\n    $ make V=1 git.o dir.o\n    GCC_DECL_AFTER_STATEMENT CRUFT\n    GCC_DECL_AFTER_STATEMENT CRUFT\n    make: `dir.o' is up to date.\n"},{"id":"204997","messageId":"20121217021501.GA13745@gmail.com","threadId":"32358","inReplyTo":"7vk3shphru.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-17T02:15:01Z","receivedAt":"2012-12-17T02:15:01Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Sun, Dec 16, 2012 at 05:52:05PM -0800, Junio C Hamano wrote:\n> Adam Spiers <git@adamspiers.org> writes:\n> \n> > If we adopt this approach,...\n> > diff --git a/Makefile b/Makefile\n> > index a49d1db..aae70d4 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -331,8 +331,13 @@ endif\n> >  # CFLAGS and LDFLAGS are for the users to override from the command line.\n> >  \n> >  CFLAGS = -g -O2 -Wall\n> > +GCC_DECL_AFTER_STATEMENT = \\\n> > +\t$(shell $(CC) --help -v 2>&1 | \\\n> > +\t\tgrep -q -- -Wdeclaration-after-statement && \\\n> > +\t  echo -Wdeclaration-after-statement)\n> > +GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)\n> > +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)\n> >  LDFLAGS =\n> > -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n> >  ALL_LDFLAGS = $(LDFLAGS)\n> \n> Please do not do this.\n> \n> People cannot disable it from the command line, like:\n> \n>     $ make V=1 CFLAGS='-g -O0 -Wall'\n> \n> If anything, this should be part of the default CFLAGS.\n> \n> More importantly, this will run the $(shell ...) struct once for\n> every *.o file we produce, I think, in addition to running it twice\n> for the whole build.\n\n[snipped]\n\nOK; I expect these issues with the implementation are all\nsurmountable.  I did not necessarily expect this to be the final\nimplementation anyhow, as indicated by my comments below the divider\nline.  However it's not clear to me what you think about the idea in\nprinciple, and whether other compiler flags would merit inclusion.\n\n(And also, please don't let this discussion hold up acceptance of the\ntwo prior patches in the series.  Even though they are independent,\nthey are somewhat logically related so I grouped them into the same\nseries, although I'm not sure if that was the right thing to do.)\n"},{"id":"205000","messageId":"7v8v8xpazq.fsf@alter.siamese.dyndns.org","threadId":"32358","inReplyTo":"20121217021501.GA13745@gmail.com","subject":"Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-17T04:18:33Z","receivedAt":"2012-12-17T04:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers <git@adamspiers.org> writes:\n\n> OK; I expect these issues with the implementation are all\n> surmountable.  I did not necessarily expect this to be the final\n> implementation anyhow, as indicated by my comments below the divider\n> line.  However it's not clear to me what you think about the idea in\n> principle, and whether other compiler flags would merit inclusion.\n\nAs different versions of GCC behave differently, and the same GCC\n(mis)detect issues differently depending on the optimization level,\nI do not know if it will be a fruitful exercise to try to come up\nwith one expression to come up with the set of flags to suit\neverybody.  One flag I prefer to use is -Werror, but that means the\nother flags must have zero false positive rate.\n\nIf you are interested, the flags I personally use with the version\nof GCC I happen to have is in the Make script on the 'todo' branch.\n"},{"id":"205407","messageId":"CAOkDyE_+3n8PS_6vs-HG6v5A4SirBPVVCdgeUPOPpwaNpkk9Uw@mail.gmail.com","threadId":"32358","inReplyTo":"7v8v8xpazq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-22T12:25:07Z","receivedAt":"2012-12-22T12:25:07Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Mon, Dec 17, 2012 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Adam Spiers <git@adamspiers.org> writes:\n>\n>> OK; I expect these issues with the implementation are all\n>> surmountable.  I did not necessarily expect this to be the final\n>> implementation anyhow, as indicated by my comments below the divider\n>> line.  However it's not clear to me what you think about the idea in\n>> principle, and whether other compiler flags would merit inclusion.\n>\n> As different versions of GCC behave differently, and the same GCC\n> (mis)detect issues differently depending on the optimization level,\n> I do not know if it will be a fruitful exercise to try to come up\n> with one expression to come up with the set of flags to suit\n> everybody.\n\nFair enough, but let's not allow perfect to become the enemy of good.\nOther flags aside, surely enabling -Wdeclaration-after-statement when\nit is available is an improvement on the status quo, if it is done in\na way which doesn't damage the current build process?  History shows\nquite a few instances of other developers falling into the same trap I\ndid, e.g.\n\n  http://search.gmane.org/search.php?group=gmane.comp.version-control.git&query=decl-after-statement\n\nSo if the check was automated in the majority of cases (I guess the\nmajority of developers use gcc), it would mean less review work for\nyou and fewer re-rolls.  If you agree, I will try to rework the patch\nso that it doesn't damage the build.\n\n> One flag I prefer to use is -Werror, but that means the\n> other flags must have zero false positive rate.\n\nPersonally I'm a fan of -Werror too.  How frequent are false\npositives, and are any of them ever insurmountable?\n\n> If you are interested, the flags I personally use with the version\n> of GCC I happen to have is in the Make script on the 'todo' branch.\n\nThanks for the info.\n"},{"id":"205427","messageId":"7vsj6yq6co.fsf@alter.siamese.dyndns.org","threadId":"32358","inReplyTo":"CAOkDyE_+3n8PS_6vs-HG6v5A4SirBPVVCdgeUPOPpwaNpkk9Uw@mail.gmail.com","subject":"Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-22T18:39:19Z","receivedAt":"2012-12-22T18:39:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers <git@adamspiers.org> writes:\n\n> Fair enough, but let's not allow perfect to become the enemy of good.\n\nThat is why I would prefer a solution without any false positive\nwhile allowing false negatives, i.e. not force everybody to use\nthese flags without giving a way to turn them off.\n\nYou could perhaps sell us a solution like this:\n\n * Put these more strict options to CC_FLAGS_PEDANTIC (you may later\n   want to come up with LD_FLAGS_PEDANTIC and friends to make other\n   comands also more strict).\n\n * Introduce PEDANTIC variable that turns XX_FLAGS_PEDANTIC\n   variables to less strict when set to 0 and more strict when set\n   to 1, similar to the way the variable V makes \"make V=0\" and\n   \"make V=1\" behave slightly differently.\n\nThen we could introduce PEDANTIC=0 as default first to have people\ntry it out, with an expectation that later we can flip the default\nto 'on' when the feature matures.\n"}]}