{"thread":{"id":"51823","subject":"[PATCH 0/2] Makefile: run coccicheck on all non-upstream sources","startedAt":"2019-09-10T07:44:30Z","lastAt":"2019-09-17T16:13:50Z","messageCount":29,"participants":["Denton Liu","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"382119","messageId":"cover.1568101393.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":null,"subject":"[PATCH 0/2] Makefile: run coccicheck on all non-upstream sources","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-10T07:44:25Z","receivedAt":"2019-09-10T07:44:30Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when we ran coccicheck, it would only run on files that are\ncurrently being compiled. However, this leaves us with a blindspot where\nWindows-only sources are not checked since Coccinelle does not run on\nWindows.\n\nThis patchset addresses this by making the \"coccicheck\" target run\nagainst all source files in the repository, except for source files that\nare pulled from some upstream source.\n\nThis patchset should serve as a continuation of the original discussion\nabout running coccicheck on all sources[1].\n\nThese patches depend on \"ds/midx-expire-repack\" and \"dl/compat-cleanup\".\n\n[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1905031127170.45@tvgsbejvaqbjf.bet/\n\n\nDenton Liu (2):\n  Makefile: define UPSTREAM_SOURCES\n  Makefile: run coccicheck on more source files\n\n Makefile | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382120","messageId":"bc3cd4637a52183fe9878b13ed761380cde76232.1568101393.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568101393.git.liu.denton@gmail.com","subject":"[PATCH 1/2] Makefile: define UPSTREAM_SOURCES","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-10T07:44:28Z","receivedAt":"2019-09-10T07:44:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"After looking through the source files in compat/ and investigating the\nfiles' content and/or its Git history, I've determined the list of files\nthat were copied from an upstream. Place the names of these files into\nthe UPSTREAM_SOURCES variable in the Makefile.\n\nIn addition, add the sha1collisiondetection/ and sha1dc/ sources as well\nsince they are also imported from upstream.\n\nIn a future commit, this variable will be used to determine which files\nare excluded when running the \"coccicheck\" target.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex ad71ae1219..708df2c289 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -598,6 +598,7 @@ SCRIPT_SH =\n SCRIPT_LIB =\n TEST_BUILTINS_OBJS =\n TEST_PROGRAMS_NEED_X =\n+UPSTREAM_SOURCES =\n \n # Having this variable in your environment would break pipelines because\n # you cause \"cd\" to echo its destination to stdout.  It can also take\n@@ -1146,6 +1147,15 @@ BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/worktree.o\n BUILTIN_OBJS += builtin/write-tree.o\n \n+UPSTREAM_SOURCES += compat/inet_ntop.c\n+UPSTREAM_SOURCES += compat/inet_pton.c\n+UPSTREAM_SOURCES += compat/obstack.%\n+UPSTREAM_SOURCES += compat/nedmalloc/%\n+UPSTREAM_SOURCES += compat/poll/%\n+UPSTREAM_SOURCES += compat/regex/%\n+UPSTREAM_SOURCES += sha1collisiondetection/%\n+UPSTREAM_SOURCES += sha1dc/%\n+\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)\n EXTLIBS =\n \n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382121","messageId":"f62b0c7d1774cefc66e519430515eeb64acad1e0.1568101393.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568101393.git.liu.denton@gmail.com","subject":"[PATCH 2/2] Makefile: run coccicheck on more source files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-10T07:44:31Z","receivedAt":"2019-09-10T07:44:35Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when running the \"coccicheck\" target, only the source files\nwhich were being compiled would have been checked by Coccinelle.\nHowever, just because we aren't compiling a source file doesn't mean we\nhave to exclude it from analysis. This will allow us to catch more\nmistakes, in particular ones that affect Windows-only sources since\nCoccinelle currently runs only on Linux.\n\nMake the \"coccicheck\" target run on all C sources except for those that\nare taken from some upstream. We don't want to patch these files since\nwe want them to be as close to upstream as possible so that it'll be\neasier to pull in upstream updates.\n\nThis results in the following sources now being checked:\n\n* block-sha1/sha1.c\n* compat/access.c\n* compat/basename.c\n* compat/fileno.c\n* compat/gmtime.c\n* compat/hstrerror.c\n* compat/memmem.c\n* compat/mingw.c\n* compat/mkdir.c\n* compat/mkdtemp.c\n* compat/mmap.c\n* compat/msvc.c\n* compat/pread.c\n* compat/precompose_utf8.c\n* compat/qsort.c\n* compat/setenv.c\n* compat/sha1-chunked.c\n* compat/snprintf.c\n* compat/stat.c\n* compat/strcasestr.c\n* compat/strdup.c\n* compat/strtoimax.c\n* compat/strtoumax.c\n* compat/unsetenv.c\n* compat/win32/dirent.c\n* compat/win32/path-utils.c\n* compat/win32/pthread.c\n* compat/win32/syslog.c\n* compat/win32/trace2_win32_process_info.c\n* compat/win32mmap.c\n* compat/winansi.c\n* ppc/sha1.c\n\nThis also results in the following source now being excluded:\n\n* compat/obstack.c\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 708df2c289..d468b7c9c4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2802,12 +2802,8 @@ check: command-list.h\n \t\texit 1; \\\n \tfi\n \n-C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n-ifdef DC_SHA1_SUBMODULE\n-COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n-else\n-COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n-endif\n+FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n+COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))\n \n %.cocci.patch: %.cocci $(COCCI_SOURCES)\n \t@echo '    ' SPATCH $<; \\\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382128","messageId":"20190910132813.GJ32087@szeder.dev","threadId":"51823","inReplyTo":"f62b0c7d1774cefc66e519430515eeb64acad1e0.1568101393.git.liu.denton@gmail.com","subject":"Re: [PATCH 2/2] Makefile: run coccicheck on more source files","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-10T13:28:13Z","receivedAt":"2019-09-10T13:28:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 10, 2019 at 12:44:31AM -0700, Denton Liu wrote:\n> Before, when running the \"coccicheck\" target, only the source files\n> which were being compiled would have been checked by Coccinelle.\n> However, just because we aren't compiling a source file doesn't mean we\n> have to exclude it from analysis. This will allow us to catch more\n> mistakes, in particular ones that affect Windows-only sources since\n> Coccinelle currently runs only on Linux.\n> \n> Make the \"coccicheck\" target run on all C sources except for those that\n> are taken from some upstream. We don't want to patch these files since\n> we want them to be as close to upstream as possible so that it'll be\n> easier to pull in upstream updates.\n> \n> This results in the following sources now being checked:\n> \n> * block-sha1/sha1.c\n> * compat/access.c\n> * compat/basename.c\n> * compat/fileno.c\n> * compat/gmtime.c\n> * compat/hstrerror.c\n> * compat/memmem.c\n> * compat/mingw.c\n\nWhen applying 'array.cocci' to 'compat/mingw.c' Coccinelle 1.0.4 (the\nversion shipped in Ubuntu 16.04, i.e. what is used in our static\nanalysis CI job) suggests to use the COPY_ARRAY macro instead of\nmemcpy():\n\ndiff -u -p a/compat/mingw.c b/compat/mingw.c\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1265,7 +1265,7 @@ static wchar_t *make_environment_block(c\n \t\t}\n \n \t\tALLOC_ARRAY(result, size);\n-\t\tmemcpy(result, wenv, size * sizeof(*wenv));\n+\t\tCOPY_ARRAY(result, wenv, size);\n \t\tFreeEnvironmentStringsW(wenv);\n \t\treturn result;\n \t}\n@@ -1309,7 +1309,7 @@ static wchar_t *make_environment_block(c\n \t\t\tcontinue;\n \n \t\tsize = wcslen(array[i]) + 1;\n-\t\tmemcpy(p, array[i], size * sizeof(*p));\n+\t\tCOPY_ARRAY(p, array[i], size);\n \t\tp += size;\n \t}\n \t*p = L'\\0';\n\nThis patch series should apply these suggestions in preparatory\npatches to make sure that our CI builds still pass.\n\n> * compat/mkdir.c\n> * compat/mkdtemp.c\n> * compat/mmap.c\n> * compat/msvc.c\n> * compat/pread.c\n> * compat/precompose_utf8.c\n> * compat/qsort.c\n> * compat/setenv.c\n> * compat/sha1-chunked.c\n> * compat/snprintf.c\n> * compat/stat.c\n> * compat/strcasestr.c\n> * compat/strdup.c\n> * compat/strtoimax.c\n> * compat/strtoumax.c\n> * compat/unsetenv.c\n> * compat/win32/dirent.c\n> * compat/win32/path-utils.c\n> * compat/win32/pthread.c\n> * compat/win32/syslog.c\n> * compat/win32/trace2_win32_process_info.c\n> * compat/win32mmap.c\n> * compat/winansi.c\n> * ppc/sha1.c\n> \n> This also results in the following source now being excluded:\n> \n> * compat/obstack.c\n\nWhy mention this file here, and, more importantly, why mention _only_\nthis file here?  According to the previous patch, there should be a\nfew more excluded files...\n\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>  Makefile | 8 ++------\n>  1 file changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 708df2c289..d468b7c9c4 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2802,12 +2802,8 @@ check: command-list.h\n>  \t\texit 1; \\\n>  \tfi\n>  \n> -C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n> -ifdef DC_SHA1_SUBMODULE\n> -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> -else\n> -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> -endif\n> +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> +COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))\n>  \n>  %.cocci.patch: %.cocci $(COCCI_SOURCES)\n>  \t@echo '    ' SPATCH $<; \\\n> -- \n> 2.23.0.248.g3a9dd8fb08\n> \n"},{"id":"382131","messageId":"20190910141828.GK32087@szeder.dev","threadId":"51823","inReplyTo":"f62b0c7d1774cefc66e519430515eeb64acad1e0.1568101393.git.liu.denton@gmail.com","subject":"Re: [PATCH 2/2] Makefile: run coccicheck on more source files","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-10T14:18:28Z","receivedAt":"2019-09-10T14:18:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 10, 2019 at 12:44:31AM -0700, Denton Liu wrote:\n> Make the \"coccicheck\" target run on all C sources except for those that\n> are taken from some upstream. We don't want to patch these files since\n> we want them to be as close to upstream as possible so that it'll be\n> easier to pull in upstream updates.\n\n> diff --git a/Makefile b/Makefile\n> index 708df2c289..d468b7c9c4 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2802,12 +2802,8 @@ check: command-list.h\n>  \t\texit 1; \\\n>  \tfi\n>  \n> -C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n> -ifdef DC_SHA1_SUBMODULE\n> -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> -else\n> -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> -endif\n> +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n\nHrm, so this uses FIND_SOURCE_FILES, which first attempts to run 'git\nls-files' and if that fails it falls back to run 'find'.\n\nUnfortunately, the output of the two slightly differ: 'git ls-files'\nprints 'abspath.c advice.c alias.c ...' why 'find' prints\n'./upload-pack.c ./unpack-trees.c ./gpg-interface.c ...'.  Now, while\nthe order of files doesn't matter, the './' prefix does, because:\n\n> +COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))\n\nHere the paths/patterns in UPSTREAM_SOURCES don't have that './'\nprefix, and thus won't match and won't filter out any of the upstream\nfiles that they are supposed to.  IOW, if someone runs 'make\ncoccicheck' on a system without Git installed, then Coccinelle will\ncheck all upstream sources as well, and will e.g. suggest using\nCOPY_ARRAY in 'compat/regex/regexec.c'.\n\nNow, running 'make coccicheck' on a git.git clone without Git\ninstalled might look like quite a pathological case on the first\nsight, but I would argue that it is not that pathological: e.g.\nconsider someone running the recent Coccinelle version in a small-ish\nDocker image containing just enough to run 'make coccicheck', but not\nGit.  (yeah, you guessed right, I am that someone ;)\n\nI don't know how to convince 'find' to omit that './' prefix from each\nlisted file, and in a portable way at that.  Piping its output through\n'sed' or even 'cut' easily takes care of it, though.\n\n>  %.cocci.patch: %.cocci $(COCCI_SOURCES)\n>  \t@echo '    ' SPATCH $<; \\\n> -- \n> 2.23.0.248.g3a9dd8fb08\n> \n"},{"id":"382133","messageId":"20190910160702.GA111040@archbookpro.localdomain","threadId":"51823","inReplyTo":"20190910132813.GJ32087@szeder.dev","subject":"Re: [PATCH 2/2] Makefile: run coccicheck on more source files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-10T16:07:02Z","receivedAt":"2019-09-10T16:07:07Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Tue, Sep 10, 2019 at 03:28:13PM +0200, SZEDER Gábor wrote:\n> On Tue, Sep 10, 2019 at 12:44:31AM -0700, Denton Liu wrote:\n> > Before, when running the \"coccicheck\" target, only the source files\n> > which were being compiled would have been checked by Coccinelle.\n> > However, just because we aren't compiling a source file doesn't mean we\n> > have to exclude it from analysis. This will allow us to catch more\n> > mistakes, in particular ones that affect Windows-only sources since\n> > Coccinelle currently runs only on Linux.\n> > \n> > Make the \"coccicheck\" target run on all C sources except for those that\n> > are taken from some upstream. We don't want to patch these files since\n> > we want them to be as close to upstream as possible so that it'll be\n> > easier to pull in upstream updates.\n> > \n> > This results in the following sources now being checked:\n> > \n> > * block-sha1/sha1.c\n> > * compat/access.c\n> > * compat/basename.c\n> > * compat/fileno.c\n> > * compat/gmtime.c\n> > * compat/hstrerror.c\n> > * compat/memmem.c\n> > * compat/mingw.c\n> \n> When applying 'array.cocci' to 'compat/mingw.c' Coccinelle 1.0.4 (the\n> version shipped in Ubuntu 16.04, i.e. what is used in our static\n> analysis CI job) suggests to use the COPY_ARRAY macro instead of\n> memcpy():\n> \n> diff -u -p a/compat/mingw.c b/compat/mingw.c\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1265,7 +1265,7 @@ static wchar_t *make_environment_block(c\n>  \t\t}\n>  \n>  \t\tALLOC_ARRAY(result, size);\n> -\t\tmemcpy(result, wenv, size * sizeof(*wenv));\n> +\t\tCOPY_ARRAY(result, wenv, size);\n>  \t\tFreeEnvironmentStringsW(wenv);\n>  \t\treturn result;\n>  \t}\n> @@ -1309,7 +1309,7 @@ static wchar_t *make_environment_block(c\n>  \t\t\tcontinue;\n>  \n>  \t\tsize = wcslen(array[i]) + 1;\n> -\t\tmemcpy(p, array[i], size * sizeof(*p));\n> +\t\tCOPY_ARRAY(p, array[i], size);\n>  \t\tp += size;\n>  \t}\n>  \t*p = L'\\0';\n> \n> This patch series should apply these suggestions in preparatory\n> patches to make sure that our CI builds still pass.\n\nThis patch series depends on the \"dl/compat-cleanup\" branch, which\nshould've already made these changes.\n\n> \n> > * compat/mkdir.c\n> > * compat/mkdtemp.c\n> > * compat/mmap.c\n> > * compat/msvc.c\n> > * compat/pread.c\n> > * compat/precompose_utf8.c\n> > * compat/qsort.c\n> > * compat/setenv.c\n> > * compat/sha1-chunked.c\n> > * compat/snprintf.c\n> > * compat/stat.c\n> > * compat/strcasestr.c\n> > * compat/strdup.c\n> > * compat/strtoimax.c\n> > * compat/strtoumax.c\n> > * compat/unsetenv.c\n> > * compat/win32/dirent.c\n> > * compat/win32/path-utils.c\n> > * compat/win32/pthread.c\n> > * compat/win32/syslog.c\n> > * compat/win32/trace2_win32_process_info.c\n> > * compat/win32mmap.c\n> > * compat/winansi.c\n> > * ppc/sha1.c\n> > \n> > This also results in the following source now being excluded:\n> > \n> > * compat/obstack.c\n> \n> Why mention this file here, and, more importantly, why mention _only_\n> this file here?  According to the previous patch, there should be a\n> few more excluded files...\n\nI guess this commit message is pretty weak since this only really\napplies to my system. I'll mention in a reroll that this was for a\nstandard build with no additional flags on Arch Linux to be specific.\n\nI got these lists by dumping the contents of $(COCCI_SOURCES) before and\nafter applying the patch and, with my \"standard\" build, it seems like\nthe only upstream file that was being checked before was\ncompat/obstack.c.\n\nI don't have build options like, for example, NO_REGEX enabled so the\nregex stuff wasn't being built in the first place but for users that\nmight want to run coccicheck on such a platform (do they even exist?),\nthis would benefit them.\n\n> \n> > Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> > ---\n> >  Makefile | 8 ++------\n> >  1 file changed, 2 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/Makefile b/Makefile\n> > index 708df2c289..d468b7c9c4 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2802,12 +2802,8 @@ check: command-list.h\n> >  \t\texit 1; \\\n> >  \tfi\n> >  \n> > -C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n> > -ifdef DC_SHA1_SUBMODULE\n> > -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n> > -else\n> > -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n> > -endif\n> > +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> > +COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))\n> >  \n> >  %.cocci.patch: %.cocci $(COCCI_SOURCES)\n> >  \t@echo '    ' SPATCH $<; \\\n> > -- \n> > 2.23.0.248.g3a9dd8fb08\n> > \n"},{"id":"382231","messageId":"cover.1568309119.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568101393.git.liu.denton@gmail.com","subject":"[PATCH v2 0/3] Makefile: run coccicheck on all non-upstream sources","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-12T17:28:24Z","receivedAt":"2019-09-12T17:28:29Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when we ran coccicheck, it would only run on files that are\ncurrently being compiled. However, this leaves us with a blindspot where\nWindows-only sources are not checked since Coccinelle does not run on\nWindows.\n\nThis patchset addresses this by making the \"coccicheck\" target run\nagainst all source files in the repository, except for source files that\nare pulled from some upstream source.\n\nThis patchset should serve as a continuation of the original discussion\nabout running coccicheck on all sources[1].\n\nThese patches depend on \"ds/midx-expire-repack\" and \"dl/compat-cleanup\".\n\n[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1905031127170.45@tvgsbejvaqbjf.bet/\n\n\nDenton Liu (3):\n  Makefile: define THIRD_PARTY_SOURCES\n  Makefile: strip leading ./ in $(FIND_SOURCE_FILES)\n  Makefile: run coccicheck on more source files\n\n Makefile | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n1:  bc3cd4637a < -:  ---------- Makefile: define UPSTREAM_SOURCES\n-:  ---------- > 1:  3878cb2598 Makefile: define THIRD_PARTY_SOURCES\n-:  ---------- > 2:  239ffa690c Makefile: strip leading ./ in $(FIND_SOURCE_FILES)\n2:  f62b0c7d17 ! 3:  89b7e17469 Makefile: run coccicheck on more source files\n    @@ Commit message\n         Coccinelle currently runs only on Linux.\n     \n         Make the \"coccicheck\" target run on all C sources except for those that\n    -    are taken from some upstream. We don't want to patch these files since\n    -    we want them to be as close to upstream as possible so that it'll be\n    -    easier to pull in upstream updates.\n    +    are taken from some third-party source. We don't want to patch these\n    +    files since we want them to be as close to upstream as possible so that\n    +    it'll be easier to pull in upstream updates.\n     \n    -    This results in the following sources now being checked:\n    +    When running a build on Arch Linux with no additional flags provided,\n    +    after applying this patch, the following sources are now checked:\n     \n         * block-sha1/sha1.c\n         * compat/access.c\n    @@ Makefile: check: command-list.h\n     -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n     -endif\n     +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n    -+COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))\n    ++COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n      \n      %.cocci.patch: %.cocci $(COCCI_SOURCES)\n      \t@echo '    ' SPATCH $<; \\\n-- \n2.23.0.37.g745f681289\n\n"},{"id":"382232","messageId":"3878cb2598c677d139c8734b61772d0cdcfdb935.1568309119.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568309119.git.liu.denton@gmail.com","subject":"[PATCH v2 1/3] Makefile: define THIRD_PARTY_SOURCES","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-12T17:28:30Z","receivedAt":"2019-09-12T17:28:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"After looking through the source files in compat/ and investigating the\nfiles' content and/or its Git history, I've determined the list of files\nthat were copied from a third-party source. Place the names of these\nfiles into the THIRD_PARTY_SOURCES variable in the Makefile.\n\nIn addition, add the sha1collisiondetection/ and sha1dc/ sources as well\nsince they are also imported from a third-party source.\n\nIn a future commit, this variable will be used to determine which files\nare excluded when running the \"coccicheck\" target.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex ad71ae1219..b88b42d7ed 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -598,6 +598,7 @@ SCRIPT_SH =\n SCRIPT_LIB =\n TEST_BUILTINS_OBJS =\n TEST_PROGRAMS_NEED_X =\n+THIRD_PARTY_SOURCES =\n \n # Having this variable in your environment would break pipelines because\n # you cause \"cd\" to echo its destination to stdout.  It can also take\n@@ -1146,6 +1147,15 @@ BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/worktree.o\n BUILTIN_OBJS += builtin/write-tree.o\n \n+THIRD_PARTY_SOURCES += compat/inet_ntop.c\n+THIRD_PARTY_SOURCES += compat/inet_pton.c\n+THIRD_PARTY_SOURCES += compat/obstack.%\n+THIRD_PARTY_SOURCES += compat/nedmalloc/%\n+THIRD_PARTY_SOURCES += compat/poll/%\n+THIRD_PARTY_SOURCES += compat/regex/%\n+THIRD_PARTY_SOURCES += sha1collisiondetection/%\n+THIRD_PARTY_SOURCES += sha1dc/%\n+\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)\n EXTLIBS =\n \n-- \n2.23.0.37.g745f681289\n\n"},{"id":"382233","messageId":"239ffa690cc0bf65e2ee7fbdbb9c481499e37020.1568309119.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568309119.git.liu.denton@gmail.com","subject":"[PATCH v2 2/3] Makefile: strip leading ./ in $(FIND_SOURCE_FILES)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-12T17:28:32Z","receivedAt":"2019-09-12T17:28:36Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, $(FIND_SOURCE_FILES) has two modes: if `git ls-files` is\npresent, it will use that to enumerate the files in the repository; else\nit will use `$(FIND) .` to enumerate the files in the directory.\n\nThere is a subtle difference between these two methods, however. With\nls-files, filenames don't have a leading `./` while with $(FIND), they\ndo. This does not currently pose a problem but in a future patch, we\nwill be using `filter-out` to process the list of files with the\nassumption that there is no prefix.\n\nUnify the two possible invocations in $(FIND_SOURCE_FILES) by using sed\nto remove the `./` prefix in the $(FIND) case.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex b88b42d7ed..e2c693440b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2610,6 +2610,7 @@ FIND_SOURCE_FILES = ( \\\n \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n \t\t-o \\( -name '*.sh' -type f -print \\) \\\n+\t\t| sed -e 's|^\\./||' \\\n \t)\n \n $(ETAGS_TARGET): FORCE\n-- \n2.23.0.37.g745f681289\n\n"},{"id":"382234","messageId":"89b7e17469e19c9dca8afa729ec1a70f4e06a2b7.1568309119.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568309119.git.liu.denton@gmail.com","subject":"[PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-12T17:28:34Z","receivedAt":"2019-09-12T17:28:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when running the \"coccicheck\" target, only the source files\nwhich were being compiled would have been checked by Coccinelle.\nHowever, just because we aren't compiling a source file doesn't mean we\nhave to exclude it from analysis. This will allow us to catch more\nmistakes, in particular ones that affect Windows-only sources since\nCoccinelle currently runs only on Linux.\n\nMake the \"coccicheck\" target run on all C sources except for those that\nare taken from some third-party source. We don't want to patch these\nfiles since we want them to be as close to upstream as possible so that\nit'll be easier to pull in upstream updates.\n\nWhen running a build on Arch Linux with no additional flags provided,\nafter applying this patch, the following sources are now checked:\n\n* block-sha1/sha1.c\n* compat/access.c\n* compat/basename.c\n* compat/fileno.c\n* compat/gmtime.c\n* compat/hstrerror.c\n* compat/memmem.c\n* compat/mingw.c\n* compat/mkdir.c\n* compat/mkdtemp.c\n* compat/mmap.c\n* compat/msvc.c\n* compat/pread.c\n* compat/precompose_utf8.c\n* compat/qsort.c\n* compat/setenv.c\n* compat/sha1-chunked.c\n* compat/snprintf.c\n* compat/stat.c\n* compat/strcasestr.c\n* compat/strdup.c\n* compat/strtoimax.c\n* compat/strtoumax.c\n* compat/unsetenv.c\n* compat/win32/dirent.c\n* compat/win32/path-utils.c\n* compat/win32/pthread.c\n* compat/win32/syslog.c\n* compat/win32/trace2_win32_process_info.c\n* compat/win32mmap.c\n* compat/winansi.c\n* ppc/sha1.c\n\nThis also results in the following source now being excluded:\n\n* compat/obstack.c\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex e2c693440b..7c88e0606f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2803,12 +2803,8 @@ check: command-list.h\n \t\texit 1; \\\n \tfi\n \n-C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n-ifdef DC_SHA1_SUBMODULE\n-COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n-else\n-COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n-endif\n+FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n+COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n \n %.cocci.patch: %.cocci $(COCCI_SOURCES)\n \t@echo '    ' SPATCH $<; \\\n-- \n2.23.0.37.g745f681289\n\n"},{"id":"382237","messageId":"xmqqzhj9fkf8.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"239ffa690cc0bf65e2ee7fbdbb9c481499e37020.1568309119.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 2/3] Makefile: strip leading ./ in $(FIND_SOURCE_FILES)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-12T18:18:03Z","receivedAt":"2019-09-12T18:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> Unify the two possible invocations in $(FIND_SOURCE_FILES) by using sed\n> to remove the `./` prefix in the $(FIND) case.\n\n> diff --git a/Makefile b/Makefile\n> index b88b42d7ed..e2c693440b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2610,6 +2610,7 @@ FIND_SOURCE_FILES = ( \\\n>  \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n>  \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n>  \t\t-o \\( -name '*.sh' -type f -print \\) \\\n> +\t\t| sed -e 's|^\\./||' \\\n>  \t)\n>  \n>  $(ETAGS_TARGET): FORCE\n\nAn extra process and piping is a bit unfortunate, but this is purely\na fallback position where it does not matter too much to expend\none-shot overhead; it would only be needed until the user has a\nworking \"git ls-files\" and starts hacking on our codebase, as this\nis used only by the xtags targets, which are only for developers.\n\nThanks.  Will queue.\n\n\n"},{"id":"382239","messageId":"xmqqtv9hfjdn.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"89b7e17469e19c9dca8afa729ec1a70f4e06a2b7.1568309119.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-12T18:40:36Z","receivedAt":"2019-09-12T18:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> +COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n\nThe former is somewhat misnamed.  FIND_SOURCE_FILES is *not* a list\nof source files---it is a procedure to list source files to its\nstandard output.  FIND_C_SOUCRES sounds as if it is a similar\nprocedure, which would be implemented much like\n\n\tFIND_C_SOURCES = $(FIND_SOURCE_FILES) | sed -n -e '/\\.c$/p'\n\nbut that is not what you did and that is not what you want to have.\nPerhaps call it FOUND_C_SOURCES?\n\nI wonder if we can get rid of FIND_SOURCE_FILES that is a mere\nprocedure and replace its use with a true list of source files.\nWould it make the result more pleasant to work with?\n\nPerhaps something like the attached patch, (which would come before\nthis entire thing as a clean-up, and removing the need for 2/3)?\n\nI dunno.\n\nUsing a procedure whose output is fed to xargs has an advantage that\na platform with very short command line limit can still work with\nmany source files, but the way you create and use COCCI_SOURCES in\nthis patch would defeat that advantage anyway, so perhaps we can get\naway with an approach like this.  Having a list of things in $(MAKE)\nvariable has a longer-term benefit that we could exploit more\nparallelism if we wanted to, too.\n\n Makefile | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex f9255344ae..9dddd0e88c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2584,7 +2584,7 @@ perl/build/man/man3/Git.3pm: perl/Git.pm\n \t$(QUIET_GEN)mkdir -p $(dir $@) && \\\n \tpod2man $< $@\n \n-FIND_SOURCE_FILES = ( \\\n+SOURCE_FILES = $(patsubst ./%,%,$(shell \\\n \tgit ls-files \\\n \t\t'*.[hcS]' \\\n \t\t'*.sh' \\\n@@ -2599,19 +2599,19 @@ FIND_SOURCE_FILES = ( \\\n \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n \t\t-o \\( -name '*.sh' -type f -print \\) \\\n-\t)\n+\t))\n \n $(ETAGS_TARGET): FORCE\n \t$(RM) $(ETAGS_TARGET)\n-\t$(FIND_SOURCE_FILES) | xargs etags -a -o $(ETAGS_TARGET)\n+\tetags -a -o $(ETAGS_TARGET) $(SOURCE_FILES)\n \n tags: FORCE\n \t$(RM) tags\n-\t$(FIND_SOURCE_FILES) | xargs ctags -a\n+\tctags -a $(SOURCE_FILES)\n \n cscope:\n \t$(RM) cscope*\n-\t$(FIND_SOURCE_FILES) | xargs cscope -b\n+\tcscope -b $(SOURCE_FILES)\n \n ### Detect prefix changes\n TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\\\n\n\n\n\n"},{"id":"382260","messageId":"xmqq5zlxfaz9.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"3878cb2598c677d139c8734b61772d0cdcfdb935.1568309119.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 1/3] Makefile: define THIRD_PARTY_SOURCES","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-12T21:42:02Z","receivedAt":"2019-09-12T21:42:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> After looking through the source files in compat/ and investigating the\n> files' content and/or its Git history, I've determined the list of files\n> that were copied from a third-party source. Place the names of these\n> files into the THIRD_PARTY_SOURCES variable in the Makefile.\n\nPlease make sure readers won't get fooled into thinking these are\nfilenames.  They are meant to be used in $(filter-out) as patterns\nto match the names of third-party source files.\n\nAlso, the paragraph before that talks about \"I did this\", but\nclaiming credit is much less important than why we are doing this in\nthe first place, which you forgot to say (perhaps because you were\ntoo deeply into the task and the motivation was too obvious to you).\n\nSo, if I were doing this patch, I'd justify the whole thing as such:\n\n\tSome files in our codebase are borrowed from other projects,\n\tand minimally updated to suit our own needs.  We'd sometimes\n\tneed to tell our own sources and these third-party sources\n\tapart for management purposes (e.g. we may want to be less\n\tstrict about coding style and other issues on third-party\n\tfiles).\n\n\tDefine the $(MAKE) variable THIRD_PARTY_SOURCES that can be\n\tused to match names of third-party sources.\n\n\n>  \n> +THIRD_PARTY_SOURCES += compat/inet_ntop.c\n> +THIRD_PARTY_SOURCES += compat/inet_pton.c\n> +THIRD_PARTY_SOURCES += compat/obstack.%\n> +THIRD_PARTY_SOURCES += compat/nedmalloc/%\n> +THIRD_PARTY_SOURCES += compat/poll/%\n> +THIRD_PARTY_SOURCES += compat/regex/%\n> +THIRD_PARTY_SOURCES += sha1collisiondetection/%\n> +THIRD_PARTY_SOURCES += sha1dc/%\n\nBefore this block, please explain that this is meant to be a list of\npatterns suitable for $(filter-out) and friends in a comment.\n\nThanks.\n"},{"id":"382287","messageId":"20190913114952.GO32087@szeder.dev","threadId":"51823","inReplyTo":"xmqqtv9hfjdn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-13T11:49:52Z","receivedAt":"2019-09-13T11:50:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 12, 2019 at 11:40:36AM -0700, Junio C Hamano wrote:\n> Denton Liu <liu.denton@gmail.com> writes:\n> \n> > +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> > +COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n> \n> The former is somewhat misnamed.  FIND_SOURCE_FILES is *not* a list\n> of source files---it is a procedure to list source files to its\n> standard output.  FIND_C_SOUCRES sounds as if it is a similar\n> procedure, which would be implemented much like\n> \n> \tFIND_C_SOURCES = $(FIND_SOURCE_FILES) | sed -n -e '/\\.c$/p'\n> \n> but that is not what you did and that is not what you want to have.\n> Perhaps call it FOUND_C_SOURCES?\n> \n> I wonder if we can get rid of FIND_SOURCE_FILES that is a mere\n> procedure and replace its use with a true list of source files.\n> Would it make the result more pleasant to work with?\n> \n> Perhaps something like the attached patch, (which would come before\n> this entire thing as a clean-up, and removing the need for 2/3)?\n> \n> I dunno.\n> \n> Using a procedure whose output is fed to xargs has an advantage that\n> a platform with very short command line limit can still work with\n> many source files, but the way you create and use COCCI_SOURCES in\n> this patch would defeat that advantage anyway,\n\nCOCCI_SOURCES is only used as an input to 'xargs', so that advantage\nis not defeated.\n\n> so perhaps we can get\n> away with an approach like this.  Having a list of things in $(MAKE)\n> variable has a longer-term benefit that we could exploit more\n> parallelism if we wanted to, too.\n> \n>  Makefile | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index f9255344ae..9dddd0e88c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2584,7 +2584,7 @@ perl/build/man/man3/Git.3pm: perl/Git.pm\n>  \t$(QUIET_GEN)mkdir -p $(dir $@) && \\\n>  \tpod2man $< $@\n>  \n> -FIND_SOURCE_FILES = ( \\\n> +SOURCE_FILES = $(patsubst ./%,%,$(shell \\\n>  \tgit ls-files \\\n>  \t\t'*.[hcS]' \\\n>  \t\t'*.sh' \\\n> @@ -2599,19 +2599,19 @@ FIND_SOURCE_FILES = ( \\\n>  \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n>  \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n>  \t\t-o \\( -name '*.sh' -type f -print \\) \\\n> -\t)\n> +\t))\n>  \n>  $(ETAGS_TARGET): FORCE\n>  \t$(RM) $(ETAGS_TARGET)\n> -\t$(FIND_SOURCE_FILES) | xargs etags -a -o $(ETAGS_TARGET)\n> +\tetags -a -o $(ETAGS_TARGET) $(SOURCE_FILES)\n>  \n>  tags: FORCE\n>  \t$(RM) tags\n> -\t$(FIND_SOURCE_FILES) | xargs ctags -a\n> +\tctags -a $(SOURCE_FILES)\n>  \n>  cscope:\n>  \t$(RM) cscope*\n> -\t$(FIND_SOURCE_FILES) | xargs cscope -b\n> +\tcscope -b $(SOURCE_FILES)\n>  \n>  ### Detect prefix changes\n>  TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\\\n> \n> \n> \n> \n"},{"id":"382288","messageId":"20190913120521.GP32087@szeder.dev","threadId":"51823","inReplyTo":"239ffa690cc0bf65e2ee7fbdbb9c481499e37020.1568309119.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 2/3] Makefile: strip leading ./ in $(FIND_SOURCE_FILES)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-13T12:05:21Z","receivedAt":"2019-09-13T12:05:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 12, 2019 at 10:28:32AM -0700, Denton Liu wrote:\n> Currently, $(FIND_SOURCE_FILES) has two modes: if `git ls-files` is\n> present, it will use that to enumerate the files in the repository; else\n> it will use `$(FIND) .` to enumerate the files in the directory.\n> \n> There is a subtle difference between these two methods, however. With\n> ls-files, filenames don't have a leading `./` while with $(FIND), they\n> do. This does not currently pose a problem but in a future patch, we\n> will be using `filter-out` to process the list of files with the\n> assumption that there is no prefix.\n> \n> Unify the two possible invocations in $(FIND_SOURCE_FILES) by using sed\n> to remove the `./` prefix in the $(FIND) case.\n> \n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>  Makefile | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/Makefile b/Makefile\n> index b88b42d7ed..e2c693440b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2610,6 +2610,7 @@ FIND_SOURCE_FILES = ( \\\n>  \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n>  \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n>  \t\t-o \\( -name '*.sh' -type f -print \\) \\\n> +\t\t| sed -e 's|^\\./||' \\\n\nRamsay pointed out to me (in an off-list email) that the list of\nlibrary header files is generated in a similar way, and then those\nleading './' have to be removed later.  See how the LIB_H and CHK_HDRS\nvariables are assigned.\n\nI think it would be great to apply the same treatment there, too, to\neliminate those './' prefixes right at the source.\n\n"},{"id":"382310","messageId":"20190913171401.GA63368@dentonliu-ltm.internal.salesforce.com","threadId":"51823","inReplyTo":"20190913114952.GO32087@szeder.dev","subject":"Re: [PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-13T17:14:01Z","receivedAt":"2019-09-13T17:14:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Fri, Sep 13, 2019 at 01:49:52PM +0200, SZEDER Gábor wrote:\n> On Thu, Sep 12, 2019 at 11:40:36AM -0700, Junio C Hamano wrote:\n> > Denton Liu <liu.denton@gmail.com> writes:\n> > \n> > > +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> > > +COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n> > \n> > The former is somewhat misnamed.  FIND_SOURCE_FILES is *not* a list\n> > of source files---it is a procedure to list source files to its\n> > standard output.  FIND_C_SOUCRES sounds as if it is a similar\n> > procedure, which would be implemented much like\n> > \n> > \tFIND_C_SOURCES = $(FIND_SOURCE_FILES) | sed -n -e '/\\.c$/p'\n> > \n> > but that is not what you did and that is not what you want to have.\n> > Perhaps call it FOUND_C_SOURCES?\n> > \n> > I wonder if we can get rid of FIND_SOURCE_FILES that is a mere\n> > procedure and replace its use with a true list of source files.\n> > Would it make the result more pleasant to work with?\n> > \n> > Perhaps something like the attached patch, (which would come before\n> > this entire thing as a clean-up, and removing the need for 2/3)?\n> > \n> > I dunno.\n> > \n> > Using a procedure whose output is fed to xargs has an advantage that\n> > a platform with very short command line limit can still work with\n> > many source files, but the way you create and use COCCI_SOURCES in\n> > this patch would defeat that advantage anyway,\n> \n> COCCI_SOURCES is only used as an input to 'xargs', so that advantage\n> is not defeated.\n\nI think it still does matter; the relevant snippet is as follows:\n\n\tif ! echo $(COCCI_SOURCES) | xargs $$limit \\\n\t\t$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \\\n\t\t>$@+ 2>$@.log; \\\n\nwhich means that a really big COCCI_SOURCES could exceed the limit.\n\nThat being said, COCCI_SOURCES should be smaller than the future\nSOURCE_FILES variable since we're only taking %.c files (and filtering\nout some of them too!).\n\nI dunno, either. I'm mostly in favour of this change since it makes a\nlot of sense to keep lists in make variables if possible as opposed to\ncommand invocations. I guess worst case, if someone complains in the\nfuture, we can always change it back.\n\n> \n> > so perhaps we can get\n> > away with an approach like this.  Having a list of things in $(MAKE)\n> > variable has a longer-term benefit that we could exploit more\n> > parallelism if we wanted to, too.\n> > \n> >  Makefile | 10 +++++-----\n> >  1 file changed, 5 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/Makefile b/Makefile\n> > index f9255344ae..9dddd0e88c 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2584,7 +2584,7 @@ perl/build/man/man3/Git.3pm: perl/Git.pm\n> >  \t$(QUIET_GEN)mkdir -p $(dir $@) && \\\n> >  \tpod2man $< $@\n> >  \n> > -FIND_SOURCE_FILES = ( \\\n> > +SOURCE_FILES = $(patsubst ./%,%,$(shell \\\n> >  \tgit ls-files \\\n> >  \t\t'*.[hcS]' \\\n> >  \t\t'*.sh' \\\n> > @@ -2599,19 +2599,19 @@ FIND_SOURCE_FILES = ( \\\n> >  \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n> >  \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n> >  \t\t-o \\( -name '*.sh' -type f -print \\) \\\n> > -\t)\n> > +\t))\n> >  \n> >  $(ETAGS_TARGET): FORCE\n> >  \t$(RM) $(ETAGS_TARGET)\n> > -\t$(FIND_SOURCE_FILES) | xargs etags -a -o $(ETAGS_TARGET)\n> > +\tetags -a -o $(ETAGS_TARGET) $(SOURCE_FILES)\n> >  \n> >  tags: FORCE\n> >  \t$(RM) tags\n> > -\t$(FIND_SOURCE_FILES) | xargs ctags -a\n> > +\tctags -a $(SOURCE_FILES)\n> >  \n> >  cscope:\n> >  \t$(RM) cscope*\n> > -\t$(FIND_SOURCE_FILES) | xargs cscope -b\n> > +\tcscope -b $(SOURCE_FILES)\n> >  \n> >  ### Detect prefix changes\n> >  TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\\\n> > \n> > \n> > \n> > \n"},{"id":"382313","messageId":"xmqqo8zodrld.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"20190913114952.GO32087@szeder.dev","subject":"Re: [PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T17:38:22Z","receivedAt":"2019-09-13T17:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> Using a procedure whose output is fed to xargs has an advantage that\n>> a platform with very short command line limit can still work with\n>> many source files, but the way you create and use COCCI_SOURCES in\n>> this patch would defeat that advantage anyway,\n>\n> COCCI_SOURCES is only used as an input to 'xargs', so that advantage\n> is not defeated.\n\nIt is passed as a command line argument to \"echo\", that pipes to\nxargs; I would not say it is taking advantage of \"xargs\" to lift the\ncommand line length limit, as it first needs to convince the shell\nto feed all of them to the \"echo\" that is upstream of \"xargs\".\n\nAs you mentioned elsewhere, LIB_H already uses the same approach as\nI outlined in the message you are responding to (i.e. \"don't define\na procedure to produce lines to the standard output in a $(MAKE)\nvariable--instead make the variable to hold the list itself\"), so I\nsuspect that we are almost on the same page?\n\n\n"},{"id":"382315","messageId":"20190913180014.GQ32087@szeder.dev","threadId":"51823","inReplyTo":"20190913171401.GA63368@dentonliu-ltm.internal.salesforce.com","subject":"Re: [PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-13T18:00:14Z","receivedAt":"2019-09-13T18:00:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 13, 2019 at 10:14:01AM -0700, Denton Liu wrote:\n> On Fri, Sep 13, 2019 at 01:49:52PM +0200, SZEDER Gábor wrote:\n> > On Thu, Sep 12, 2019 at 11:40:36AM -0700, Junio C Hamano wrote:\n> > > Denton Liu <liu.denton@gmail.com> writes:\n> > > \n> > > > +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> > > > +COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n> > > \n> > > The former is somewhat misnamed.  FIND_SOURCE_FILES is *not* a list\n> > > of source files---it is a procedure to list source files to its\n> > > standard output.  FIND_C_SOUCRES sounds as if it is a similar\n> > > procedure, which would be implemented much like\n> > > \n> > > \tFIND_C_SOURCES = $(FIND_SOURCE_FILES) | sed -n -e '/\\.c$/p'\n> > > \n> > > but that is not what you did and that is not what you want to have.\n> > > Perhaps call it FOUND_C_SOURCES?\n> > > \n> > > I wonder if we can get rid of FIND_SOURCE_FILES that is a mere\n> > > procedure and replace its use with a true list of source files.\n> > > Would it make the result more pleasant to work with?\n> > > \n> > > Perhaps something like the attached patch, (which would come before\n> > > this entire thing as a clean-up, and removing the need for 2/3)?\n> > > \n> > > I dunno.\n> > > \n> > > Using a procedure whose output is fed to xargs has an advantage that\n> > > a platform with very short command line limit can still work with\n> > > many source files, but the way you create and use COCCI_SOURCES in\n> > > this patch would defeat that advantage anyway,\n> > \n> > COCCI_SOURCES is only used as an input to 'xargs', so that advantage\n> > is not defeated.\n> \n> I think it still does matter; the relevant snippet is as follows:\n> \n> \tif ! echo $(COCCI_SOURCES) | xargs $$limit \\\n> \t\t$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \\\n> \t\t>$@+ 2>$@.log; \\\n> \n> which means that a really big COCCI_SOURCES could exceed the limit.\n\nOh, you're both right.\n\n> That being said, COCCI_SOURCES should be smaller than the future\n> SOURCE_FILES variable since we're only taking %.c files (and filtering\n> out some of them too!).\n\nWe could also argue that Coccinelle only runs on platforms that have a\nreasonably large command line arg limit, and the number of our source\nfiles is way below that, so it won't matter in the foreseeable future.\n\n(Furthermore, 'echo' is often a shell builtin command, and I don't\nthink that the platform's argument size limit applies to them.  At\nleast the 'echo' of dash, Bash, ksh, ksh93, mksh, and BusyBox sh can\ndeal with at least 10 million arguments; the platform limit is\nsomewhere around 147k)\n\n> > > diff --git a/Makefile b/Makefile\n> > > index f9255344ae..9dddd0e88c 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -2584,7 +2584,7 @@ perl/build/man/man3/Git.3pm: perl/Git.pm\n> > >  \t$(QUIET_GEN)mkdir -p $(dir $@) && \\\n> > >  \tpod2man $< $@\n> > >  \n> > > -FIND_SOURCE_FILES = ( \\\n> > > +SOURCE_FILES = $(patsubst ./%,%,$(shell \\\n> > >  \tgit ls-files \\\n> > >  \t\t'*.[hcS]' \\\n> > >  \t\t'*.sh' \\\n> > > @@ -2599,19 +2599,19 @@ FIND_SOURCE_FILES = ( \\\n> > >  \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n> > >  \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n> > >  \t\t-o \\( -name '*.sh' -type f -print \\) \\\n> > > -\t)\n> > > +\t))\n> > >  \n> > >  $(ETAGS_TARGET): FORCE\n> > >  \t$(RM) $(ETAGS_TARGET)\n> > > -\t$(FIND_SOURCE_FILES) | xargs etags -a -o $(ETAGS_TARGET)\n> > > +\tetags -a -o $(ETAGS_TARGET) $(SOURCE_FILES)\n> > >  \n> > >  tags: FORCE\n> > >  \t$(RM) tags\n> > > -\t$(FIND_SOURCE_FILES) | xargs ctags -a\n> > > +\tctags -a $(SOURCE_FILES)\n> > >  \n> > >  cscope:\n> > >  \t$(RM) cscope*\n> > > -\t$(FIND_SOURCE_FILES) | xargs cscope -b\n> > > +\tcscope -b $(SOURCE_FILES)\n\nHere, however, the list of source files is passed as argument to\nnon-builtin commands, that also might be used on\ncmdline-arg-limit-challenged platforms.\n\n"},{"id":"382342","messageId":"20190913213818.GA88412@dentonliu-ltm.internal.salesforce.com","threadId":"51823","inReplyTo":"20190913180014.GQ32087@szeder.dev","subject":"Re: [PATCH v2 3/3] Makefile: run coccicheck on more source files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-13T21:38:18Z","receivedAt":"2019-09-13T21:38:24Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Fri, Sep 13, 2019 at 08:00:14PM +0200, SZEDER Gábor wrote:\n> On Fri, Sep 13, 2019 at 10:14:01AM -0700, Denton Liu wrote:\n> > On Fri, Sep 13, 2019 at 01:49:52PM +0200, SZEDER Gábor wrote:\n> > > On Thu, Sep 12, 2019 at 11:40:36AM -0700, Junio C Hamano wrote:\n> > > > Denton Liu <liu.denton@gmail.com> writes:\n> > > > \n> > > > > +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n> > > > > +COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n> > > > \n> > > > The former is somewhat misnamed.  FIND_SOURCE_FILES is *not* a list\n> > > > of source files---it is a procedure to list source files to its\n> > > > standard output.  FIND_C_SOUCRES sounds as if it is a similar\n> > > > procedure, which would be implemented much like\n> > > > \n> > > > \tFIND_C_SOURCES = $(FIND_SOURCE_FILES) | sed -n -e '/\\.c$/p'\n> > > > \n> > > > but that is not what you did and that is not what you want to have.\n> > > > Perhaps call it FOUND_C_SOURCES?\n> > > > \n> > > > I wonder if we can get rid of FIND_SOURCE_FILES that is a mere\n> > > > procedure and replace its use with a true list of source files.\n> > > > Would it make the result more pleasant to work with?\n> > > > \n> > > > Perhaps something like the attached patch, (which would come before\n> > > > this entire thing as a clean-up, and removing the need for 2/3)?\n> > > > \n> > > > I dunno.\n> > > > \n> > > > Using a procedure whose output is fed to xargs has an advantage that\n> > > > a platform with very short command line limit can still work with\n> > > > many source files, but the way you create and use COCCI_SOURCES in\n> > > > this patch would defeat that advantage anyway,\n> > > \n> > > COCCI_SOURCES is only used as an input to 'xargs', so that advantage\n> > > is not defeated.\n> > \n> > I think it still does matter; the relevant snippet is as follows:\n> > \n> > \tif ! echo $(COCCI_SOURCES) | xargs $$limit \\\n> > \t\t$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \\\n> > \t\t>$@+ 2>$@.log; \\\n> > \n> > which means that a really big COCCI_SOURCES could exceed the limit.\n> \n> Oh, you're both right.\n> \n> > That being said, COCCI_SOURCES should be smaller than the future\n> > SOURCE_FILES variable since we're only taking %.c files (and filtering\n> > out some of them too!).\n> \n> We could also argue that Coccinelle only runs on platforms that have a\n> reasonably large command line arg limit, and the number of our source\n> files is way below that, so it won't matter in the foreseeable future.\n\nGood point.\n\n> \n> (Furthermore, 'echo' is often a shell builtin command, and I don't\n> think that the platform's argument size limit applies to them.  At\n> least the 'echo' of dash, Bash, ksh, ksh93, mksh, and BusyBox sh can\n> deal with at least 10 million arguments; the platform limit is\n> somewhere around 147k)\n> \n> > > > diff --git a/Makefile b/Makefile\n> > > > index f9255344ae..9dddd0e88c 100644\n> > > > --- a/Makefile\n> > > > +++ b/Makefile\n> > > > @@ -2584,7 +2584,7 @@ perl/build/man/man3/Git.3pm: perl/Git.pm\n> > > >  \t$(QUIET_GEN)mkdir -p $(dir $@) && \\\n> > > >  \tpod2man $< $@\n> > > >  \n> > > > -FIND_SOURCE_FILES = ( \\\n> > > > +SOURCE_FILES = $(patsubst ./%,%,$(shell \\\n> > > >  \tgit ls-files \\\n> > > >  \t\t'*.[hcS]' \\\n> > > >  \t\t'*.sh' \\\n> > > > @@ -2599,19 +2599,19 @@ FIND_SOURCE_FILES = ( \\\n> > > >  \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n> > > >  \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n> > > >  \t\t-o \\( -name '*.sh' -type f -print \\) \\\n> > > > -\t)\n> > > > +\t))\n> > > >  \n> > > >  $(ETAGS_TARGET): FORCE\n> > > >  \t$(RM) $(ETAGS_TARGET)\n> > > > -\t$(FIND_SOURCE_FILES) | xargs etags -a -o $(ETAGS_TARGET)\n> > > > +\tetags -a -o $(ETAGS_TARGET) $(SOURCE_FILES)\n> > > >  \n> > > >  tags: FORCE\n> > > >  \t$(RM) tags\n> > > > -\t$(FIND_SOURCE_FILES) | xargs ctags -a\n> > > > +\tctags -a $(SOURCE_FILES)\n> > > >  \n> > > >  cscope:\n> > > >  \t$(RM) cscope*\n> > > > -\t$(FIND_SOURCE_FILES) | xargs cscope -b\n> > > > +\tcscope -b $(SOURCE_FILES)\n> \n> Here, however, the list of source files is passed as argument to\n> non-builtin commands, that also might be used on\n> cmdline-arg-limit-challenged platforms.\n> \n\nAfter doing a bit of research, I think that I agree with you. It seems\nlike the max command-line length for CMD on Windows is 8191 characters.\n\nHowever, after running the following,\n\n\t$ git ls-files '*.[hcS]' '*.sh' ':!*[tp][0-9][0-9][0-9][0-9]*' ':!contrib' | wc -c\n\t   12779\n\nwe can see that the command-line length would definitely exceed the max\nlength so xargs would be required. As a result, we should probably just\nkeep the existing xargs invocations.\n"},{"id":"382443","messageId":"cover.1568661443.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568309119.git.liu.denton@gmail.com","subject":"[PATCH v3 0/4] Makefile: run coccicheck on all non-upstream sources","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-16T19:23:03Z","receivedAt":"2019-09-16T19:23:11Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi all, I spent the weekend brooding over this and I opted to not\nconvert $(FIND_SOURCE_FILES) into a list of filenames. I put the\njustification for it in 4/4 so, hopefully, it'll make sense for someone\nwho might want to do the same change in the future.\n\n\nBefore, when we ran coccicheck, it would only run on files that are\ncurrently being compiled. However, this leaves us with a blindspot where\nWindows-only sources are not checked since Coccinelle does not run on\nWindows.\n\nThis patchset addresses this by making the \"coccicheck\" target run\nagainst all source files in the repository, except for source files that\nare pulled from some upstream source.\n\nThis patchset should serve as a continuation of the original discussion\nabout running coccicheck on all sources[1].\n\nThese patches depend on \"ds/midx-expire-repack\" and \"dl/compat-cleanup\".\n\n[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1905031127170.45@tvgsbejvaqbjf.bet/\n\n\nDenton Liu (4):\n  Makefile: strip leading ./ in $(LIB_H)\n  Makefile: define THIRD_PARTY_SOURCES\n  Makefile: strip leading ./ in $(FIND_SOURCE_FILES)\n  Makefile: run coccicheck on more source files\n\n Makefile | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\nRange-diff against v2:\n-:  ---------- > 1:  e7fa5e1df7 Makefile: strip leading ./ in $(LIB_H)\n1:  72b4cc68f5 ! 2:  a7c855a973 Makefile: define THIRD_PARTY_SOURCES\n    @@ Metadata\n      ## Commit message ##\n         Makefile: define THIRD_PARTY_SOURCES\n     \n    -    After looking through the source files in compat/ and investigating the\n    -    files' content and/or its Git history, I've determined the list of files\n    -    that were copied from a third-party source. Place the names of these\n    -    files into the THIRD_PARTY_SOURCES variable in the Makefile.\n    +    Some files in our codebase are borrowed from other projects, and\n    +    minimally updated to suit our own needs. We'd sometimes need to tell\n    +    our own sources and these third-party sources apart for management\n    +    purposes (e.g. we may want to be less strict about coding style and\n    +    other issues on third-party files).\n     \n    -    In addition, add the sha1collisiondetection/ and sha1dc/ sources as well\n    -    since they are also imported from a third-party source.\n    -\n    -    In a future commit, this variable will be used to determine which files\n    -    are excluded when running the \"coccicheck\" target.\n    +    Define the $(MAKE) variable THIRD_PARTY_SOURCES that can be used to\n    +    match names of third-party sources.\n     \n         Signed-off-by: Denton Liu <liu.denton@gmail.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      ## Makefile ##\n     @@ Makefile: SCRIPT_SH =\n    @@ Makefile: BUILTIN_OBJS += builtin/verify-tag.o\n      BUILTIN_OBJS += builtin/worktree.o\n      BUILTIN_OBJS += builtin/write-tree.o\n      \n    ++# THIRD_PARTY_SOURCES is a list of patterns compatible with\n    ++# the $(filter) and $(filter-out) family of functions\n     +THIRD_PARTY_SOURCES += compat/inet_ntop.c\n     +THIRD_PARTY_SOURCES += compat/inet_pton.c\n     +THIRD_PARTY_SOURCES += compat/obstack.%\n2:  a51acfec66 ! 3:  71861cbdf8 Makefile: strip leading ./ in $(FIND_SOURCE_FILES)\n    @@ Commit message\n         to remove the `./` prefix in the $(FIND) case.\n     \n         Signed-off-by: Denton Liu <liu.denton@gmail.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      ## Makefile ##\n     @@ Makefile: FIND_SOURCE_FILES = ( \\\n3:  75c36bc9a1 ! 4:  0ecfcd8858 Makefile: run coccicheck on more source files\n    @@ Commit message\n     \n         * compat/obstack.c\n     \n    +    Instead of generating $(FOUND_C_SOURCES) from a\n    +    `$(shell $(FIND_SOURCE_FILES))` invocation, an alternative design was\n    +    considered which involved converting $(FIND_SOURCE_FILES) into\n    +    $(SOURCE_FILES) which would hold a list of filenames from the\n    +    $(FIND_SOURCE_FILES) invocation. We would simply filter `%.c` files into\n    +    $(ALL_C_SOURCES). $(SOURCE_FILES) would then be passed directly to the\n    +    etags, ctags and cscope commands. We can see from the following\n    +    invocation\n    +\n    +            $ git ls-files '*.[hcS]' '*.sh' ':!*[tp][0-9][0-9][0-9][0-9]*' ':!contrib' | wc -c\n    +               12779\n    +\n    +    that the number of characters in this list would pose a problem on\n    +    platforms with short command-line length limits (such as CMD which has a\n    +    max of 8191 characters). As a result, we don't perform this change.\n    +\n    +    However, we can see that the same issue may apply when running\n    +    Coccinelle since $(COCCI_SOURCES) is also a list of filenames:\n    +\n    +            if ! echo $(COCCI_SOURCES) | xargs $$limit \\\n    +                    $(SPATCH) --sp-file $< $(SPATCH_FLAGS) \\\n    +                    >$@+ 2>$@.log; \\\n    +\n    +    This is justified since platforms that support Coccinelle generally have\n    +    reasonably long command-line length limits and so we are safe for the\n    +    foreseeable future.\n    +\n         Signed-off-by: Denton Liu <liu.denton@gmail.com>\n    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n     \n      ## Makefile ##\n     @@ Makefile: check: command-list.h\n    @@ Makefile: check: command-list.h\n     -else\n     -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n     -endif\n    -+FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n    -+COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))\n    ++FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n    ++COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))\n      \n      %.cocci.patch: %.cocci $(COCCI_SOURCES)\n      \t@echo '    ' SPATCH $<; \\\n-- \n2.23.0\n\n"},{"id":"382444","messageId":"e7fa5e1df76040c6e67727a4b33567a4b23a9aec.1568661443.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568661443.git.liu.denton@gmail.com","subject":"[PATCH v3 1/4] Makefile: strip leading ./ in $(LIB_H)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-16T19:23:08Z","receivedAt":"2019-09-16T19:23:13Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, $(LIB_H) is generated from two modes: if `git ls-files` is\npresent, it will use that to enumerate the files in the repository; else\nit will use `$(FIND) .` to enumerate the files in the directory.\n\nThere is a subtle difference between these two methods, however. With\nls-files, filenames don't have a leading `./` while with $(FIND), they\ndo. This results in $(CHK_HDRS) having to substitute out the leading\n`./` before it uses $(LIB_H).\n\nUnify the two possible values in $(LIB_H) by using patsubst to remove the\n`./` prefix at its definition.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ad71ae1219..ea77198247 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -818,12 +818,12 @@ VCSSVN_LIB = vcs-svn/lib.a\n \n GENERATED_H += command-list.h\n \n-LIB_H := $(sort $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n+LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n \t$(FIND) . \\\n \t-name .git -prune -o \\\n \t-name t -prune -o \\\n \t-name Documentation -prune -o \\\n-\t-name '*.h' -print))\n+\t-name '*.h' -print)))\n \n LIB_OBJS += abspath.o\n LIB_OBJS += advice.o\n@@ -2769,7 +2769,7 @@ EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%\n ifndef GCRYPT_SHA256\n \tEXCEPT_HDRS += sha256/gcrypt.h\n endif\n-CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n+CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(LIB_H))\n HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n \n $(HCO): %.hco: %.h FORCE\n-- \n2.23.0\n\n"},{"id":"382445","messageId":"a7c855a973db9067c2e30569d86f9b2b2147c6b3.1568661443.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568661443.git.liu.denton@gmail.com","subject":"[PATCH v3 2/4] Makefile: define THIRD_PARTY_SOURCES","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-16T19:23:11Z","receivedAt":"2019-09-16T19:23:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Some files in our codebase are borrowed from other projects, and\nminimally updated to suit our own needs. We'd sometimes need to tell\nour own sources and these third-party sources apart for management\npurposes (e.g. we may want to be less strict about coding style and\nother issues on third-party files).\n\nDefine the $(MAKE) variable THIRD_PARTY_SOURCES that can be used to\nmatch names of third-party sources.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex ea77198247..49839579ec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -598,6 +598,7 @@ SCRIPT_SH =\n SCRIPT_LIB =\n TEST_BUILTINS_OBJS =\n TEST_PROGRAMS_NEED_X =\n+THIRD_PARTY_SOURCES =\n \n # Having this variable in your environment would break pipelines because\n # you cause \"cd\" to echo its destination to stdout.  It can also take\n@@ -1146,6 +1147,17 @@ BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/worktree.o\n BUILTIN_OBJS += builtin/write-tree.o\n \n+# THIRD_PARTY_SOURCES is a list of patterns compatible with\n+# the $(filter) and $(filter-out) family of functions\n+THIRD_PARTY_SOURCES += compat/inet_ntop.c\n+THIRD_PARTY_SOURCES += compat/inet_pton.c\n+THIRD_PARTY_SOURCES += compat/obstack.%\n+THIRD_PARTY_SOURCES += compat/nedmalloc/%\n+THIRD_PARTY_SOURCES += compat/poll/%\n+THIRD_PARTY_SOURCES += compat/regex/%\n+THIRD_PARTY_SOURCES += sha1collisiondetection/%\n+THIRD_PARTY_SOURCES += sha1dc/%\n+\n GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)\n EXTLIBS =\n \n-- \n2.23.0\n\n"},{"id":"382446","messageId":"71861cbdf833c8c9300c6b45930b29655f5de611.1568661443.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568661443.git.liu.denton@gmail.com","subject":"[PATCH v3 3/4] Makefile: strip leading ./ in $(FIND_SOURCE_FILES)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-16T19:23:14Z","receivedAt":"2019-09-16T19:23:19Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, $(FIND_SOURCE_FILES) has two modes: if `git ls-files` is\npresent, it will use that to enumerate the files in the repository; else\nit will use `$(FIND) .` to enumerate the files in the directory.\n\nThere is a subtle difference between these two methods, however. With\nls-files, filenames don't have a leading `./` while with $(FIND), they\ndo. This does not currently pose a problem but in a future patch, we\nwill be using `filter-out` to process the list of files with the\nassumption that there is no prefix.\n\nUnify the two possible invocations in $(FIND_SOURCE_FILES) by using sed\nto remove the `./` prefix in the $(FIND) case.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 49839579ec..20eb5c5c42 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2612,6 +2612,7 @@ FIND_SOURCE_FILES = ( \\\n \t\t-o \\( -name 'trash*' -type d -prune \\) \\\n \t\t-o \\( -name '*.[hcS]' -type f -print \\) \\\n \t\t-o \\( -name '*.sh' -type f -print \\) \\\n+\t\t| sed -e 's|^\\./||' \\\n \t)\n \n $(ETAGS_TARGET): FORCE\n-- \n2.23.0\n\n"},{"id":"382447","messageId":"0ecfcd8858f1734ca079cb260f0d27d43f1e1cc5.1568661443.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"cover.1568661443.git.liu.denton@gmail.com","subject":"[PATCH v3 4/4] Makefile: run coccicheck on more source files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-16T19:23:16Z","receivedAt":"2019-09-16T19:23:22Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when running the \"coccicheck\" target, only the source files\nwhich were being compiled would have been checked by Coccinelle.\nHowever, just because we aren't compiling a source file doesn't mean we\nhave to exclude it from analysis. This will allow us to catch more\nmistakes, in particular ones that affect Windows-only sources since\nCoccinelle currently runs only on Linux.\n\nMake the \"coccicheck\" target run on all C sources except for those that\nare taken from some third-party source. We don't want to patch these\nfiles since we want them to be as close to upstream as possible so that\nit'll be easier to pull in upstream updates.\n\nWhen running a build on Arch Linux with no additional flags provided,\nafter applying this patch, the following sources are now checked:\n\n* block-sha1/sha1.c\n* compat/access.c\n* compat/basename.c\n* compat/fileno.c\n* compat/gmtime.c\n* compat/hstrerror.c\n* compat/memmem.c\n* compat/mingw.c\n* compat/mkdir.c\n* compat/mkdtemp.c\n* compat/mmap.c\n* compat/msvc.c\n* compat/pread.c\n* compat/precompose_utf8.c\n* compat/qsort.c\n* compat/setenv.c\n* compat/sha1-chunked.c\n* compat/snprintf.c\n* compat/stat.c\n* compat/strcasestr.c\n* compat/strdup.c\n* compat/strtoimax.c\n* compat/strtoumax.c\n* compat/unsetenv.c\n* compat/win32/dirent.c\n* compat/win32/path-utils.c\n* compat/win32/pthread.c\n* compat/win32/syslog.c\n* compat/win32/trace2_win32_process_info.c\n* compat/win32mmap.c\n* compat/winansi.c\n* ppc/sha1.c\n\nThis also results in the following source now being excluded:\n\n* compat/obstack.c\n\nInstead of generating $(FOUND_C_SOURCES) from a\n`$(shell $(FIND_SOURCE_FILES))` invocation, an alternative design was\nconsidered which involved converting $(FIND_SOURCE_FILES) into\n$(SOURCE_FILES) which would hold a list of filenames from the\n$(FIND_SOURCE_FILES) invocation. We would simply filter `%.c` files into\n$(ALL_C_SOURCES). $(SOURCE_FILES) would then be passed directly to the\netags, ctags and cscope commands. We can see from the following\ninvocation\n\n\t$ git ls-files '*.[hcS]' '*.sh' ':!*[tp][0-9][0-9][0-9][0-9]*' ':!contrib' | wc -c\n\t   12779\n\nthat the number of characters in this list would pose a problem on\nplatforms with short command-line length limits (such as CMD which has a\nmax of 8191 characters). As a result, we don't perform this change.\n\nHowever, we can see that the same issue may apply when running\nCoccinelle since $(COCCI_SOURCES) is also a list of filenames:\n\n\tif ! echo $(COCCI_SOURCES) | xargs $$limit \\\n\t\t$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \\\n\t\t>$@+ 2>$@.log; \\\n\nThis is justified since platforms that support Coccinelle generally have\nreasonably long command-line length limits and so we are safe for the\nforeseeable future.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 20eb5c5c42..522342a46c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2805,12 +2805,8 @@ check: command-list.h\n \t\texit 1; \\\n \tfi\n \n-C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n-ifdef DC_SHA1_SUBMODULE\n-COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))\n-else\n-COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))\n-endif\n+FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))\n+COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))\n \n %.cocci.patch: %.cocci $(COCCI_SOURCES)\n \t@echo '    ' SPATCH $<; \\\n-- \n2.23.0\n\n"},{"id":"382461","messageId":"xmqqmuf49cze.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"a7c855a973db9067c2e30569d86f9b2b2147c6b3.1568661443.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 2/4] Makefile: define THIRD_PARTY_SOURCES","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-16T20:56:37Z","receivedAt":"2019-09-16T20:56:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> +# THIRD_PARTY_SOURCES is a list of patterns compatible with\n> +# the $(filter) and $(filter-out) family of functions\n\nThat defines the format, but does it convey what they want to\nachieve to the readers?  \"... to catch the list of source files\nwe borrowed from elsewhere\" or something, perhaps?\n\n> +THIRD_PARTY_SOURCES += compat/inet_ntop.c\n> +THIRD_PARTY_SOURCES += compat/inet_pton.c\n> +THIRD_PARTY_SOURCES += compat/obstack.%\n> +THIRD_PARTY_SOURCES += compat/nedmalloc/%\n> +THIRD_PARTY_SOURCES += compat/poll/%\n> +THIRD_PARTY_SOURCES += compat/regex/%\n> +THIRD_PARTY_SOURCES += sha1collisiondetection/%\n> +THIRD_PARTY_SOURCES += sha1dc/%\n> +\n>  GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)\n>  EXTLIBS =\n"},{"id":"382462","messageId":"xmqqh85c9cyd.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"cover.1568661443.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 0/4] Makefile: run coccicheck on all non-upstream sources","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-16T20:57:14Z","receivedAt":"2019-09-16T20:57:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Queued; thanks.\n"},{"id":"382467","messageId":"64cc6feae0df61268c09c20573d03f60349649a1.1568671170.git.liu.denton@gmail.com","threadId":"51823","inReplyTo":"xmqqmuf49cze.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] fixup! Makefile: define THIRD_PARTY_SOURCES","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-16T22:00:21Z","receivedAt":"2019-09-16T22:00:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"* expand the comment to show what it's used for\n\n* alphabetical ordering fix\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Makefile | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 522342a46c..617c25d16b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1147,12 +1147,15 @@ BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/worktree.o\n BUILTIN_OBJS += builtin/write-tree.o\n \n-# THIRD_PARTY_SOURCES is a list of patterns compatible with\n-# the $(filter) and $(filter-out) family of functions\n+# THIRD_PARTY_SOURCES is a list of patterns compatible with the\n+# $(filter) and $(filter-out) family of functions. They specify source\n+# files which are taken from some third-party source where we want to be\n+# less strict about issues such as coding style so we don't diverge from\n+# upstream unnecessarily (making merging in future changes easier).\n THIRD_PARTY_SOURCES += compat/inet_ntop.c\n THIRD_PARTY_SOURCES += compat/inet_pton.c\n-THIRD_PARTY_SOURCES += compat/obstack.%\n THIRD_PARTY_SOURCES += compat/nedmalloc/%\n+THIRD_PARTY_SOURCES += compat/obstack.%\n THIRD_PARTY_SOURCES += compat/poll/%\n THIRD_PARTY_SOURCES += compat/regex/%\n THIRD_PARTY_SOURCES += sha1collisiondetection/%\n-- \n2.23.0\n\n"},{"id":"382474","messageId":"20190917081821.GF6190@szeder.dev","threadId":"51823","inReplyTo":"xmqqh85c9cyd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 0/4] Makefile: run coccicheck on all non-upstream sources","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-17T08:18:21Z","receivedAt":"2019-09-17T08:18:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Sep 16, 2019 at 01:57:14PM -0700, Junio C Hamano wrote:\n> Queued; thanks.\n\nNit: it would be nicer to queue this series on top of\n'dl/compat-cleanup', because 'make coccicheck' errors out suggesting\ntransformations in 'compat/mingw.c' that have only been applied in\nthat series.\n\n"},{"id":"382495","messageId":"xmqq7e66aojr.fsf@gitster-ct.c.googlers.com","threadId":"51823","inReplyTo":"20190917081821.GF6190@szeder.dev","subject":"Re: [PATCH v3 0/4] Makefile: run coccicheck on all non-upstream sources","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-17T16:13:44Z","receivedAt":"2019-09-17T16:13:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Mon, Sep 16, 2019 at 01:57:14PM -0700, Junio C Hamano wrote:\n>> Queued; thanks.\n>\n> Nit: it would be nicer to queue this series on top of\n> 'dl/compat-cleanup', because 'make coccicheck' errors out suggesting\n> transformations in 'compat/mingw.c' that have only been applied in\n> that series.\n\nYikes, yes I remember that you pointed it out already, yet I\nforgot.  Will correct.\n\nThanks.\n"}]}