Re: [PATCH] builtin/mv.c: use correct type to compute size of an array element

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 06 2022, Junio C Hamano wrote:

> The variables 'source', 'destination', and 'submodule_gitfile' are
> all of type "const char **", and an element of such an array is of
> "type const char *".
>
> Noticed while running "make contrib/coccinelle/array.cocci.patch".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * There is this rule in the array.cocci file:
>
>         @@
>         type T;
>         T *dst;
>         T *src;
>         expression n;
>         @@
>         (
>         - memmove(dst, src, (n) * sizeof(*dst));
>         + MOVE_ARRAY(dst, src, n);
>         |
>         - memmove(dst, src, (n) * sizeof(*src));
>         + MOVE_ARRAY(dst, src, n);
>         |
>         - memmove(dst, src, (n) * sizeof(T));
>         + MOVE_ARRAY(dst, src, n);
>         )
>
>    but it triggered only for modes[] array among the four parallel
>    arrays that are being moved here.
>
>    There are a few tangents.
>
>    * Should we in general use sizeof(TYPE) in these cases, instead
>      of the size of the zeroth element, e.g.
>
>  		memmove(source + i, source + i + 1,
> 			n * sizeof(source[i]));
>     
>      It would have been caught by the above Coccinelle rule (we are
>      taking the size of *dst).
>
>    * Shouldn't we have an array of struct with four members, instead
>      of four parallel arrays, e.g.
>
> 		struct {
> 			const char *source;
> 			const char *destination;
> 			enum update_mode mode;
> 			const char *submodule_gitfile;
> 		} *mv_file;
>
>    The latter question is important to answer before we accept
>    Coccinelle-suggested rewrite to use four MOVE_ARRAY() on these
>    four parallel arrays on top of this fix.

This seems to be the same sort case as noted in 7bd97d6dff3 (git: use
COPY_ARRAY and MOVE_ARRAY in handle_alias(), 2019-09-19).

I tried (going aginst the warnings in that commit about the
non-generality) updating the rules to catch these cases, which yielded
the below. I wonder if we should apply some version of it. I.e. one-off
fix these, and perhaps have the cocci rule BUG() if we see this sort of
code again (the form technically could be used, but it seems all our
uses of it are ones we could convert to the simpler one...).

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..eff338d3901 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -915,10 +915,9 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
 	file_diff->hunk_nr += splittable_into - 1;
 	ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc);
 	if (hunk_index + splittable_into < file_diff->hunk_nr)
-		memmove(file_diff->hunk + hunk_index + splittable_into,
-			file_diff->hunk + hunk_index + 1,
-			(file_diff->hunk_nr - hunk_index - splittable_into)
-			* sizeof(*hunk));
+		MOVE_ARRAY(file_diff->hunk + hunk_index + splittable_into,
+			   file_diff->hunk + hunk_index + 1,
+			   file_diff->hunk_nr - hunk_index - splittable_into);
 	hunk = file_diff->hunk + hunk_index;
 	hunk->splittable_into = 1;
 	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
diff --git a/builtin/mv.c b/builtin/mv.c
index 83a465ba831..f6187d1264a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -282,14 +282,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 remove_entry:
 		if (--argc > 0) {
 			int n = argc - i;
-			memmove(source + i, source + i + 1,
-				n * sizeof(char *));
-			memmove(destination + i, destination + i + 1,
-				n * sizeof(char *));
-			memmove(modes + i, modes + i + 1,
-				n * sizeof(enum update_mode));
-			memmove(submodule_gitfile + i, submodule_gitfile + i + 1,
-				n * sizeof(char *));
+			MOVE_ARRAY(source + i, source + i + 1, n);
+			MOVE_ARRAY(destination + i, destination + i + 1, n);
+			MOVE_ARRAY(modes + i, modes + i + 1, n);
+			MOVE_ARRAY(submodule_gitfile + i,
+				   submodule_gitfile + i + 1, n);
 			i--;
 		}
 	}
diff --git a/commit.c b/commit.c
index 1fb1b2ea90c..c23d3e3678f 100644
--- a/commit.c
+++ b/commit.c
@@ -151,10 +151,9 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,
 		   r->parsed_objects->grafts_alloc);
 	r->parsed_objects->grafts_nr++;
 	if (pos < r->parsed_objects->grafts_nr)
-		memmove(r->parsed_objects->grafts + pos + 1,
-			r->parsed_objects->grafts + pos,
-			(r->parsed_objects->grafts_nr - pos - 1) *
-			sizeof(*r->parsed_objects->grafts));
+		MOVE_ARRAY(r->parsed_objects->grafts + pos + 1,
+			   r->parsed_objects->grafts + pos,
+			   r->parsed_objects->grafts_nr - pos - 1);
 	r->parsed_objects->grafts[pos] = graft;
 	unparse_commit(r, &graft->oid);
 	return 0;
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 9a4f00cb1bd..988ff9a3fda 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -73,6 +73,15 @@ expression n;
 + MOVE_ARRAY(dst, src, n);
 )
 
+@@
+expression D;
+expression S;
+expression N;
+expression Z;
+@@
+- memmove(D, S, (N) * Z);
++ MOVE_ARRAY(D, S, N);
+
 @@
 type T;
 T *ptr;