[PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
same for moving them backwards in an array with potential overlap.  The
result is shorter and safer, as it infers the element type automatically
and does a (very) basic type compatibility check for its first two
arguments.

These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
because the type of the elements is "const char *", not "char *", and
the rules in the semantic patch cautiously insist on the sizeof operator
being used on exactly the same type to avoid generating transformations
that introduce subtle bugs into tricky code.

Signed-off-by: René Scharfe <redacted>
---
 git.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index c1ee7124ed..ce6ab0ece2 100644
--- a/git.c
+++ b/git.c

@@ -369,8 +369,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			die(_("alias '%s' changes environment variables.\n"
 			      "You can use '!git' in the alias to do this"),
 			    alias_command);
-		memmove(new_argv - option_count, new_argv,
-				count * sizeof(char *));
+		MOVE_ARRAY(new_argv - option_count, new_argv, count);
 		new_argv -= option_count;

 		if (count < 1)

@@ -385,7 +384,7 @@ static int handle_alias(int *argcp, const char ***argv)

 		REALLOC_ARRAY(new_argv, count + *argcp);
 		/* insert after command name */
-		memcpy(new_argv + count, *argv + 1, sizeof(char *) * *argcp);
+		COPY_ARRAY(new_argv + count, *argv + 1, *argcp);

 		trace2_cmd_alias(alias_command, new_argv);
 		trace2_cmd_list_config();
--

2.23.0

On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:

Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
same for moving them backwards in an array with potential overlap.  The
result is shorter and safer, as it infers the element type automatically
and does a (very) basic type compatibility check for its first two
arguments.

These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
because the type of the elements is "const char *", not "char *", and
the rules in the semantic patch cautiously insist on the sizeof operator
being used on exactly the same type to avoid generating transformations
that introduce subtle bugs into tricky code.

Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

The patch looks good.

-Peff

Hi Peff,

On Mon, 23 Sep 2019, Jeff King wrote:

On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:

quoted

Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
same for moving them backwards in an array with potential overlap.  The
result is shorter and safer, as it infers the element type automatically
and does a (very) basic type compatibility check for its first two
arguments.

These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
because the type of the elements is "const char *", not "char *", and
the rules in the semantic patch cautiously insist on the sizeof operator
being used on exactly the same type to avoid generating transformations
that introduce subtle bugs into tricky code.

Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

That is indeed a very good reason, in addition to getting the type right
automatically (by virtue of letting the compiler pick it).

Should we make this an explicit guideline in our documentation?

Ciao,
Dscho

On 9/26/2019 9:22 AM, Johannes Schindelin wrote:

Hi Peff,

On Mon, 23 Sep 2019, Jeff King wrote:

quoted

On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:

quoted

Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
same for moving them backwards in an array with potential overlap.  The
result is shorter and safer, as it infers the element type automatically
and does a (very) basic type compatibility check for its first two
arguments.

These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
because the type of the elements is "const char *", not "char *", and
the rules in the semantic patch cautiously insist on the sizeof operator
being used on exactly the same type to avoid generating transformations
that introduce subtle bugs into tricky code.

Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

That is indeed a very good reason, in addition to getting the type right
automatically (by virtue of letting the compiler pick it).

Should we make this an explicit guideline in our documentation?

Better yet: can we create a Coccinelle script to fix it automatically?

-Stolee

On Thu, Sep 26, 2019 at 09:36:44AM -0400, Derrick Stolee wrote:

On 9/26/2019 9:22 AM, Johannes Schindelin wrote:

quoted

Hi Peff,

On Mon, 23 Sep 2019, Jeff King wrote:

quoted

On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:

quoted

Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
same for moving them backwards in an array with potential overlap.  The
result is shorter and safer, as it infers the element type automatically
and does a (very) basic type compatibility check for its first two
arguments.

These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
because the type of the elements is "const char *", not "char *", and
the rules in the semantic patch cautiously insist on the sizeof operator
being used on exactly the same type to avoid generating transformations
that introduce subtle bugs into tricky code.

Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

That is indeed a very good reason, in addition to getting the type right
automatically (by virtue of letting the compiler pick it).

Should we make this an explicit guideline in our documentation?

Better yet: can we create a Coccinelle script to fix it automatically?

I've already done that well over a year ago :)  But remember not being
quite satisfied with something (no idea what it was anymore) and left
it on the backburner.

Will dig it out and have a look as time permits.

On 26/09/2019 14:36, Derrick Stolee wrote:

quoted

quoted

Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

That is indeed a very good reason, in addition to getting the type right
automatically (by virtue of letting the compiler pick it).

Should we make this an explicit guideline in our documentation?

Better yet: can we create a Coccinelle script to fix it automatically?

-Stolee

How about 'Both'. We can't assume all contributors have Coccinelle on 
their OS/system.

Philip

On 9/26/2019 11:24 AM, Philip Oakley wrote:

On 26/09/2019 14:36, Derrick Stolee wrote:

quoted

quoted

quoted

Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

That is indeed a very good reason, in addition to getting the type right
automatically (by virtue of letting the compiler pick it).

Should we make this an explicit guideline in our documentation?

Better yet: can we create a Coccinelle script to fix it automatically?

-Stolee

How about 'Both'. We can't assume all contributors have Coccinelle on their OS/system.

Both is best, but I find static checkers to be more reliable than
updating documentation. For that reason, I would prioritize the
Coccinelle script over adding another bullet point to the style
guide.

The PR builds for GitGitGadget run ci/run-static-analysis.sh as a check
(see the StaticAnalysis job in [1] for an example). That provides a free
way to get feedback for users without Coccinelle.

[1] https://dev.azure.com/gitgitgadget/git/_build/results?buildId=16864&view=logs

Thanks,
-Stolee