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