{"thread":{"id":"51886","subject":"[PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","startedAt":"2019-09-19T20:48:42Z","lastAt":"2019-09-26T16:16:21Z","messageCount":7,"participants":["René Scharfe","Jeff King","Johannes Schindelin","Derrick Stolee","SZEDER Gábor","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"382611","messageId":"d5eef24a-faa8-d6f3-c9e5-f13dc40219d4@web.de","threadId":"51886","inReplyTo":null,"subject":"[PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-19T20:48:30Z","receivedAt":"2019-09-19T20:48:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the\nsame for moving them backwards in an array with potential overlap.  The\nresult is shorter and safer, as it infers the element type automatically\nand does a (very) basic type compatibility check for its first two\narguments.\n\nThese cases were missed by Coccinelle and contrib/coccinelle/array.cocci\nbecause the type of the elements is \"const char *\", not \"char *\", and\nthe rules in the semantic patch cautiously insist on the sizeof operator\nbeing used on exactly the same type to avoid generating transformations\nthat introduce subtle bugs into tricky code.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n git.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex c1ee7124ed..ce6ab0ece2 100644\n--- a/git.c\n+++ b/git.c\n@@ -369,8 +369,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tdie(_(\"alias '%s' changes environment variables.\\n\"\n \t\t\t      \"You can use '!git' in the alias to do this\"),\n \t\t\t    alias_command);\n-\t\tmemmove(new_argv - option_count, new_argv,\n-\t\t\t\tcount * sizeof(char *));\n+\t\tMOVE_ARRAY(new_argv - option_count, new_argv, count);\n \t\tnew_argv -= option_count;\n\n \t\tif (count < 1)\n@@ -385,7 +384,7 @@ static int handle_alias(int *argcp, const char ***argv)\n\n \t\tREALLOC_ARRAY(new_argv, count + *argcp);\n \t\t/* insert after command name */\n-\t\tmemcpy(new_argv + count, *argv + 1, sizeof(char *) * *argcp);\n+\t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n\n \t\ttrace2_cmd_alias(alias_command, new_argv);\n \t\ttrace2_cmd_list_config();\n--\n2.23.0\n"},{"id":"382804","messageId":"20190923222659.GA22645@sigill.intra.peff.net","threadId":"51886","inReplyTo":"d5eef24a-faa8-d6f3-c9e5-f13dc40219d4@web.de","subject":"Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-23T22:27:00Z","receivedAt":"2019-09-23T22:27:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:\n\n> Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the\n> same for moving them backwards in an array with potential overlap.  The\n> result is shorter and safer, as it infers the element type automatically\n> and does a (very) basic type compatibility check for its first two\n> arguments.\n> \n> These cases were missed by Coccinelle and contrib/coccinelle/array.cocci\n> because the type of the elements is \"const char *\", not \"char *\", and\n> the rules in the semantic patch cautiously insist on the sizeof operator\n> being used on exactly the same type to avoid generating transformations\n> that introduce subtle bugs into tricky code.\n\nAnother good reason to use \"sizeof(var)\" instead of sizeof(type)\". :)\n\nThe patch looks good.\n\n-Peff\n"},{"id":"382985","messageId":"nycvar.QRO.7.76.6.1909261521380.15067@tvgsbejvaqbjf.bet","threadId":"51886","inReplyTo":"20190923222659.GA22645@sigill.intra.peff.net","subject":"sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-26T13:22:45Z","receivedAt":"2019-09-26T13:23:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 23 Sep 2019, Jeff King wrote:\n\n> On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:\n>\n> > Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the\n> > same for moving them backwards in an array with potential overlap.  The\n> > result is shorter and safer, as it infers the element type automatically\n> > and does a (very) basic type compatibility check for its first two\n> > arguments.\n> >\n> > These cases were missed by Coccinelle and contrib/coccinelle/array.cocci\n> > because the type of the elements is \"const char *\", not \"char *\", and\n> > the rules in the semantic patch cautiously insist on the sizeof operator\n> > being used on exactly the same type to avoid generating transformations\n> > that introduce subtle bugs into tricky code.\n>\n> Another good reason to use \"sizeof(var)\" instead of sizeof(type)\". :)\n\nThat is indeed a very good reason, in addition to getting the type right\nautomatically (by virtue of letting the compiler pick it).\n\nShould we make this an explicit guideline in our documentation?\n\nCiao,\nDscho\n"},{"id":"382987","messageId":"b79f52f7-4a7e-f7cd-ed8a-cb6cfbadd7b2@gmail.com","threadId":"51886","inReplyTo":"nycvar.QRO.7.76.6.1909261521380.15067@tvgsbejvaqbjf.bet","subject":"Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-26T13:36:44Z","receivedAt":"2019-09-26T13:36:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/26/2019 9:22 AM, Johannes Schindelin wrote:\n> Hi Peff,\n> \n> On Mon, 23 Sep 2019, Jeff King wrote:\n> \n>> On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:\n>>\n>>> Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the\n>>> same for moving them backwards in an array with potential overlap.  The\n>>> result is shorter and safer, as it infers the element type automatically\n>>> and does a (very) basic type compatibility check for its first two\n>>> arguments.\n>>>\n>>> These cases were missed by Coccinelle and contrib/coccinelle/array.cocci\n>>> because the type of the elements is \"const char *\", not \"char *\", and\n>>> the rules in the semantic patch cautiously insist on the sizeof operator\n>>> being used on exactly the same type to avoid generating transformations\n>>> that introduce subtle bugs into tricky code.\n>>\n>> Another good reason to use \"sizeof(var)\" instead of sizeof(type)\". :)\n> \n> That is indeed a very good reason, in addition to getting the type right\n> automatically (by virtue of letting the compiler pick it).\n> \n> Should we make this an explicit guideline in our documentation?\n\nBetter yet: can we create a Coccinelle script to fix it automatically?\n\n-Stolee\n\n"},{"id":"382988","messageId":"20190926134341.GG2637@szeder.dev","threadId":"51886","inReplyTo":"b79f52f7-4a7e-f7cd-ed8a-cb6cfbadd7b2@gmail.com","subject":"Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-26T13:43:41Z","receivedAt":"2019-09-26T13:43:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 26, 2019 at 09:36:44AM -0400, Derrick Stolee wrote:\n> On 9/26/2019 9:22 AM, Johannes Schindelin wrote:\n> > Hi Peff,\n> > \n> > On Mon, 23 Sep 2019, Jeff King wrote:\n> > \n> >> On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:\n> >>\n> >>> Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the\n> >>> same for moving them backwards in an array with potential overlap.  The\n> >>> result is shorter and safer, as it infers the element type automatically\n> >>> and does a (very) basic type compatibility check for its first two\n> >>> arguments.\n> >>>\n> >>> These cases were missed by Coccinelle and contrib/coccinelle/array.cocci\n> >>> because the type of the elements is \"const char *\", not \"char *\", and\n> >>> the rules in the semantic patch cautiously insist on the sizeof operator\n> >>> being used on exactly the same type to avoid generating transformations\n> >>> that introduce subtle bugs into tricky code.\n> >>\n> >> Another good reason to use \"sizeof(var)\" instead of sizeof(type)\". :)\n> > \n> > That is indeed a very good reason, in addition to getting the type right\n> > automatically (by virtue of letting the compiler pick it).\n> > \n> > Should we make this an explicit guideline in our documentation?\n> \n> Better yet: can we create a Coccinelle script to fix it automatically?\n\nI've already done that well over a year ago :)  But remember not being\nquite satisfied with something (no idea what it was anymore) and left\nit on the backburner.\n\nWill dig it out and have a look as time permits.\n\n"},{"id":"382991","messageId":"7bbcf6fa-e36d-4a1e-86e1-437e9e92dab7@iee.email","threadId":"51886","inReplyTo":"b79f52f7-4a7e-f7cd-ed8a-cb6cfbadd7b2@gmail.com","subject":"Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2019-09-26T15:24:00Z","receivedAt":"2019-09-26T15:24:04Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 26/09/2019 14:36, Derrick Stolee wrote:\n>>> Another good reason to use \"sizeof(var)\" instead of sizeof(type)\". :)\n>> That is indeed a very good reason, in addition to getting the type right\n>> automatically (by virtue of letting the compiler pick it).\n>>\n>> Should we make this an explicit guideline in our documentation?\n> Better yet: can we create a Coccinelle script to fix it automatically?\n>\n> -Stolee\n>\nHow about 'Both'. We can't assume all contributors have Coccinelle on \ntheir OS/system.\n\nPhilip\n"},{"id":"382997","messageId":"6ecafde8-cd1c-9457-4102-b73304a122fd@gmail.com","threadId":"51886","inReplyTo":"7bbcf6fa-e36d-4a1e-86e1-437e9e92dab7@iee.email","subject":"Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-26T16:16:17Z","receivedAt":"2019-09-26T16:16:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/26/2019 11:24 AM, Philip Oakley wrote:\n> On 26/09/2019 14:36, Derrick Stolee wrote:\n>>>> Another good reason to use \"sizeof(var)\" instead of sizeof(type)\". :)\n>>> That is indeed a very good reason, in addition to getting the type right\n>>> automatically (by virtue of letting the compiler pick it).\n>>>\n>>> Should we make this an explicit guideline in our documentation?\n>> Better yet: can we create a Coccinelle script to fix it automatically?\n>>\n>> -Stolee\n>>\n> How about 'Both'. We can't assume all contributors have Coccinelle on their OS/system.\n\nBoth is best, but I find static checkers to be more reliable than\nupdating documentation. For that reason, I would prioritize the\nCoccinelle script over adding another bullet point to the style\nguide.\n\nThe PR builds for GitGitGadget run ci/run-static-analysis.sh as a check\n(see the StaticAnalysis job in [1] for an example). That provides a free\nway to get feedback for users without Coccinelle.\n\n[1] https://dev.azure.com/gitgitgadget/git/_build/results?buildId=16864&view=logs\n\nThanks,\n-Stolee\n"}]}