{"thread":{"id":"59017","subject":"[PATCH 0/3] COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY","startedAt":"2022-12-30T21:51:36Z","lastAt":"2023-01-09T18:17:04Z","messageCount":20,"participants":["René Scharfe","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"469690","messageId":"efe7ec20-201e-a1c1-8e16-2f714a0aee8e@web.de","threadId":"59017","inReplyTo":null,"subject":"[PATCH 0/3] COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-30T21:51:23Z","receivedAt":"2022-12-30T21:51:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Tighten the type check of COPY_ARRAY and MOVE_ARRAY.  Then add DUP_ARRAY\nfor making a shallow copy without repeating destination and element\ncount, which inherits the stricter check.\n\n  do full type check in COPY_ARRAY and MOVE_ARRAY\n  add DUP_ARRAY\n  use DUP_ARRAY\n\n attr.c                         | 3 +--\n builtin/am.c                   | 3 +--\n commit-graph.c                 | 3 +--\n commit-reach.c                 | 3 +--\n compat/mingw.c                 | 3 +--\n contrib/coccinelle/array.cocci | 7 +++++++\n git-compat-util.h              | 7 +++++++\n parse-options.c                | 3 +--\n pathspec.c                     | 6 ++----\n 9 files changed, 22 insertions(+), 16 deletions(-)\n\n--\n2.39.0\n"},{"id":"469691","messageId":"f3b9e9be-06ef-3773-a496-da5e479f9d49@web.de","threadId":"59017","inReplyTo":"efe7ec20-201e-a1c1-8e16-2f714a0aee8e@web.de","subject":"[PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-30T21:56:44Z","receivedAt":"2022-12-30T21:56:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extend the element size comparison between source and destination with\na full type check using an assignment.  It is not actually evaluated and\neven optimized out, but compilers check the types before getting to that\npoint, and report mismatches.\n\nThe stricter check improves safety, as it catches attempts to copy\nbetween different types that happen to have the same size.  The size\ncheck is still needed to avoid allowing copies from a array with a\nsmaller element type to a bigger one, e.g. from a char array to an int\narray, which would be allowed by the assignment check alone.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 76e4b11131..8d04832988 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1094,6 +1094,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))\n\n #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \\\n+\t(0 ? (*(dst) = *(src), 0) : 0) + \\\n \tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n {\n@@ -1102,6 +1103,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n }\n\n #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \\\n+\t(0 ? (*(dst) = *(src), 0) : 0) + \\\n \tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n {\n--\n2.39.0\n"},{"id":"469692","messageId":"4dae3686-1183-0e70-a1c8-378b1cb91d23@web.de","threadId":"59017","inReplyTo":"efe7ec20-201e-a1c1-8e16-2f714a0aee8e@web.de","subject":"[PATCH 2/3] add DUP_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-30T22:02:19Z","receivedAt":"2022-12-30T22:02:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a macro for allocating and populating a shallow copy of an array.\nIt is intended to replace a sequence like this:\n\n   ALLOC_ARRAY(dst, n);\n   COPY_ARRAY(dst, src, n);\n\nWith the less repetitive:\n\n   DUP_ARRAY(dst, src, n);\n\nIt checks whether the types of source and destination are compatible to\nensure the copy can be used safely.\n\nAn easier alternative would be to only consider the source and return\na void pointer, that could be used like this:\n\n   dst = ARRAY_DUP(src, n);\n\nThat would be more versatile, as it could be used in declarations as\nwell.  Making it type-safe would require the use of typeof from C23,\nthough, as far as I can see.\n\nSo use the first variant, whose safety requires no compiler extensions\nor future features.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8d04832988..29f4f699b5 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1111,6 +1111,11 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n \t\tmemmove(dst, src, st_mult(size, n));\n }\n\n+#define DUP_ARRAY(dst, src, n) do { \\\n+\tsize_t dup_array_n_ = (n); \\\n+\tCOPY_ARRAY(ALLOC_ARRAY((dst), dup_array_n_), (src), dup_array_n_); \\\n+} while (0)\n+\n /*\n  * These functions help you allocate structs with flex arrays, and copy\n  * the data directly into the array. For example, if you had:\n--\n2.39.0\n"},{"id":"469693","messageId":"3f8f6ae4-dc70-d1e6-682c-17d078f2c546@web.de","threadId":"59017","inReplyTo":"efe7ec20-201e-a1c1-8e16-2f714a0aee8e@web.de","subject":"[PATCH 3/3] use DUP_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-30T22:03:45Z","receivedAt":"2022-12-30T22:03:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a semantic patch to replace ALLOC_ARRAY+COPY_ARRAY with DUP_ARRAY\nto reduce code duplication and apply its results.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n attr.c                         | 3 +--\n builtin/am.c                   | 3 +--\n commit-graph.c                 | 3 +--\n commit-reach.c                 | 3 +--\n compat/mingw.c                 | 3 +--\n contrib/coccinelle/array.cocci | 7 +++++++\n parse-options.c                | 3 +--\n pathspec.c                     | 6 ++----\n 8 files changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 42ad6de8c7..e4e304a826 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -599,8 +599,7 @@ struct attr_check *attr_check_dup(const struct attr_check *check)\n\n \tret->nr = check->nr;\n \tret->alloc = check->alloc;\n-\tALLOC_ARRAY(ret->items, ret->nr);\n-\tCOPY_ARRAY(ret->items, check->items, ret->nr);\n+\tDUP_ARRAY(ret->items, check->items, ret->nr);\n\n \treturn ret;\n }\ndiff --git a/builtin/am.c b/builtin/am.c\nindex dddf1b9af0..eee06bbb6c 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1489,8 +1489,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \t * apply_opts.v keeps referencing the allocated strings for\n \t * strvec_clear() to release.\n \t */\n-\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n-\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n+\tDUP_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n\n \topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n \t\t\t\t\t&apply_state, &force_apply, &options,\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a7d8755932..c11b59f28b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1594,8 +1594,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \t\t\t_(\"Computing commit changed paths Bloom filters\"),\n \t\t\tctx->commits.nr);\n\n-\tALLOC_ARRAY(sorted_commits, ctx->commits.nr);\n-\tCOPY_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);\n+\tDUP_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);\n\n \tif (ctx->order_by_pack)\n \t\tQSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);\ndiff --git a/commit-reach.c b/commit-reach.c\nindex c226ee3da4..2e33c599a8 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -245,8 +245,7 @@ static int remove_redundant_with_gen(struct repository *r,\n \t * min_gen_pos points to the current position within 'array'\n \t * that is not yet known to be STALE.\n \t */\n-\tALLOC_ARRAY(sorted, cnt);\n-\tCOPY_ARRAY(sorted, array, cnt);\n+\tDUP_ARRAY(sorted, array, cnt);\n \tQSORT(sorted, cnt, compare_commits_by_gen);\n \tmin_generation = commit_graph_generation(sorted[0]);\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex d614f156df..ddc5dd0377 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1396,8 +1396,7 @@ static wchar_t *make_environment_block(char **deltaenv)\n \t\t\tp += s;\n \t\t}\n\n-\t\tALLOC_ARRAY(result, size);\n-\t\tCOPY_ARRAY(result, wenv, size);\n+\t\tDUP_ARRAY(result, wenv, size);\n \t\tFreeEnvironmentStringsW(wenv);\n \t\treturn result;\n \t}\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex aa75937950..27a3b479c9 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -94,3 +94,10 @@ expression n != 1;\n @@\n - ptr = xcalloc(n, \\( sizeof(*ptr) \\| sizeof(T) \\) )\n + CALLOC_ARRAY(ptr, n)\n+\n+@@\n+expression dst, src, n;\n+@@\n+-ALLOC_ARRAY(dst, n);\n+-COPY_ARRAY(dst, src, n);\n++DUP_ARRAY(dst, src, n);\ndiff --git a/parse-options.c b/parse-options.c\nindex a1ec932f0f..fd4743293f 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -702,8 +702,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n \tif (!nr_aliases)\n \t\treturn NULL;\n\n-\tALLOC_ARRAY(newopt, nr + 1);\n-\tCOPY_ARRAY(newopt, options, nr + 1);\n+\tDUP_ARRAY(newopt, options, nr + 1);\n\n \t/* each alias has two string pointers and NULL */\n \tCALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));\ndiff --git a/pathspec.c b/pathspec.c\nindex 46e77a85fe..e038481dc4 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -681,8 +681,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \tint i, j;\n\n \t*dst = *src;\n-\tALLOC_ARRAY(dst->items, dst->nr);\n-\tCOPY_ARRAY(dst->items, src->items, dst->nr);\n+\tDUP_ARRAY(dst->items, src->items, dst->nr);\n\n \tfor (i = 0; i < dst->nr; i++) {\n \t\tstruct pathspec_item *d = &dst->items[i];\n@@ -691,8 +690,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \t\td->match = xstrdup(s->match);\n \t\td->original = xstrdup(s->original);\n\n-\t\tALLOC_ARRAY(d->attr_match, d->attr_match_nr);\n-\t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n+\t\tDUP_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n \t\tfor (j = 0; j < d->attr_match_nr; j++) {\n \t\t\tconst char *value = s->attr_match[j].value;\n \t\t\td->attr_match[j].value = xstrdup_or_null(value);\n--\n2.39.0\n"},{"id":"469720","messageId":"xmqq8rinhs7t.fsf@gitster.g","threadId":"59017","inReplyTo":"f3b9e9be-06ef-3773-a496-da5e479f9d49@web.de","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-01T03:03:02Z","receivedAt":"2023-01-01T03:03:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Extend the element size comparison between source and destination with\n> a full type check using an assignment.  It is not actually evaluated and\n> even optimized out, but compilers check the types before getting to that\n> point, and report mismatches.\n\nIt is not actually evaluated and is (even) optimized out, which is a\nneat and clever trick.  The above made me read it twice, though, as\nI misread on my first read that you are saying that the code is left\nin the binary but in an unreachable form (i.e. \"it is not (evaluated\nor optimized out\"), though.\n\n> The stricter check improves safety, as it catches attempts to copy\n> between different types that happen to have the same size.  The size\n> check is still needed to avoid allowing copies from a array with a\n> smaller element type to a bigger one, e.g. from a char array to an int\n> array, which would be allowed by the assignment check alone.\n\nDo you mean\n\n\tint *ip;\n\tchar *cp;\n\n\t(0 ? (*(ip) = *(cp), 0) : 0); /* silently allowed */\n\t(0 ? (*(cp) = *(ip), 0) : 0); /* truncation noticed */\n\nPresumably we cannot catch\n\t\n\tint *ip;\n\tunsinged *up;\n\n\t(0 ? (*(ip) = *(up), 0) : 0);\n\t(0 ? (*(up) = *(ip), 0) : 0);\n\nwith the approach?  I think what you ideally want to enforce is that\ntypeof(dst) is exactly typeof(src), or typeof(src) sans constness\n(i.e. you can populate non-const array from a const template)?\n\n>  #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \\\n> +\t(0 ? (*(dst) = *(src), 0) : 0) + \\\n>  \tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n>  static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n>  {\n> @@ -1102,6 +1103,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n>  }\n>\n>  #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \\\n> +\t(0 ? (*(dst) = *(src), 0) : 0) + \\\n>  \tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n>  static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n>  {\n> --\n> 2.39.0\n"},{"id":"469722","messageId":"xmqqsfguhplc.fsf@gitster.g","threadId":"59017","inReplyTo":"xmqq8rinhs7t.fsf@gitster.g","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-01T03:59:43Z","receivedAt":"2023-01-01T03:59:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...  I think what you ideally want to enforce is that\n> typeof(dst) is exactly typeof(src), or typeof(src) sans constness\n> (i.e. you can populate non-const array from a const template)?\n\nIOW, I am wondering if something like this is an improvement.\n\n git-compat-util.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git c/git-compat-util.h w/git-compat-util.h\nindex a76d0526f7..be435615f0 100644\n--- c/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -97,8 +97,13 @@ struct strbuf;\n # define BARF_UNLESS_AN_ARRAY(arr)\t\t\t\t\t\t\\\n \tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \\\n \t\t\t\t\t\t\t   __typeof__(&(arr)[0])))\n+# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \\\n+\t(BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \\\n+\t\t\t\t\t\t\t    __typeof__(dst))) + \\\n+\t\t\t      (0 ? *(dst) = *(src) : 0))\n #else\n # define BARF_UNLESS_AN_ARRAY(arr) 0\n+# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0))\n #endif\n /*\n  * ARRAY_SIZE - get the number of elements in a visible array\n"},{"id":"469728","messageId":"220515b3-3924-c8d2-ff20-7017caa7dead@web.de","threadId":"59017","inReplyTo":"xmqqsfguhplc.fsf@gitster.g","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T07:41:59Z","receivedAt":"2023-01-01T07:42:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.01.23 um 04:59 schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> ...  I think what you ideally want to enforce is that\n>> typeof(dst) is exactly typeof(src), or typeof(src) sans constness\n>> (i.e. you can populate non-const array from a const template)?\n\nYes.\n\nMoving the type check shared between COPY_ARRAY and MOVE_ARRAY to a new\nmacro is a good idea.\n\nUsing compiler extensions when available, as we already do for other\npurposes, is a good idea as well.  I managed to ignore the existing use\nsomehow.\n\n>\n> IOW, I am wondering if something like this is an improvement.\n>\n>  git-compat-util.h | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git c/git-compat-util.h w/git-compat-util.h\n> index a76d0526f7..be435615f0 100644\n> --- c/git-compat-util.h\n> +++ w/git-compat-util.h\n> @@ -97,8 +97,13 @@ struct strbuf;\n>  # define BARF_UNLESS_AN_ARRAY(arr)\t\t\t\t\t\t\\\n>  \tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \\\n>  \t\t\t\t\t\t\t   __typeof__(&(arr)[0])))\n> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \\\n> +\t(BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \\\n> +\t\t\t\t\t\t\t    __typeof__(dst))) + \\\n> +\t\t\t      (0 ? *(dst) = *(src) : 0))\n>  #else\n>  # define BARF_UNLESS_AN_ARRAY(arr) 0\n> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0))\n>  #endif\n>  /*\n>   * ARRAY_SIZE - get the number of elements in a visible array\n"},{"id":"469729","messageId":"bf4e6798-c687-270d-1344-37bacb403e13@web.de","threadId":"59017","inReplyTo":"220515b3-3924-c8d2-ff20-7017caa7dead@web.de","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T10:45:41Z","receivedAt":"2023-01-01T10:45:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.01.23 um 08:41 schrieb René Scharfe:\n> Am 01.01.23 um 04:59 schrieb Junio C Hamano:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> ...  I think what you ideally want to enforce is that\n>>> typeof(dst) is exactly typeof(src), or typeof(src) sans constness\n>>> (i.e. you can populate non-const array from a const template)?\n>\n> Yes.\n>\n> Moving the type check shared between COPY_ARRAY and MOVE_ARRAY to a new\n> macro is a good idea.\n>\n> Using compiler extensions when available, as we already do for other\n> purposes, is a good idea as well.  I managed to ignore the existing use\n> somehow.\n\nOn second thought, what do we gain by using __builtin_types_compatible_p\nhere?  Does it catch cases that the assignment check plus the element\nsize check wouldn't?\n\n>\n>>\n>> IOW, I am wondering if something like this is an improvement.\n>>\n>>  git-compat-util.h | 5 +++++\n>>  1 file changed, 5 insertions(+)\n>>\n>> diff --git c/git-compat-util.h w/git-compat-util.h\n>> index a76d0526f7..be435615f0 100644\n>> --- c/git-compat-util.h\n>> +++ w/git-compat-util.h\n>> @@ -97,8 +97,13 @@ struct strbuf;\n>>  # define BARF_UNLESS_AN_ARRAY(arr)\t\t\t\t\t\t\\\n>>  \tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \\\n>>  \t\t\t\t\t\t\t   __typeof__(&(arr)[0])))\n>> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \\\n>> +\t(BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \\\n>> +\t\t\t\t\t\t\t    __typeof__(dst))) + \\\n\nWe actually need to compare the types of the elements here, because\notherwise we'd disallow copies between arrays (int arr[7]) and pointers\n(int *p), which would be unnecessarily strict.\n\n>> +\t\t\t      (0 ? *(dst) = *(src) : 0))\n\nYou dropped the \", 0\" after the assignment, but we need it to make the\ntype of this expression int instead of whatever type *dst and *src have.\n\n>>  #else\n>>  # define BARF_UNLESS_AN_ARRAY(arr) 0\n>> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0))\n>>  #endif\n>>  /*\n>>   * ARRAY_SIZE - get the number of elements in a visible array\n"},{"id":"469730","messageId":"xmqqzgb2fo8g.fsf@gitster.g","threadId":"59017","inReplyTo":"bf4e6798-c687-270d-1344-37bacb403e13@web.de","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-01T12:11:59Z","receivedAt":"2023-01-01T12:12:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> On second thought, what do we gain by using __builtin_types_compatible_p\n> here?  Does it catch cases that the assignment check plus the element\n> size check wouldn't?\n\nI was reacting to this part of an earlier message in the thread:\n\nPresumably we cannot catch\n\t\n\tint *ip;\n\tunsigned *up;\n\n\t(0 ? (*(ip) = *(up), 0) : 0);\n\t(0 ? (*(up) = *(ip), 0) : 0);\n\nwith the approach?\n\ni.e. *ip and *up are of the same size.  Would the assignment check\ntrigger?\n\n> We actually need to compare the types of the elements here, because\n> otherwise we'd disallow copies between arrays (int arr[7]) and pointers\n> (int *p), which would be unnecessarily strict.\n\nYup.  Thanks for inferring what I meant to give, despite the two\ntypoes (the other one is \", 0\").\n\n"},{"id":"469731","messageId":"269d85de-1448-0198-8251-9af898fedc67@web.de","threadId":"59017","inReplyTo":"xmqqzgb2fo8g.fsf@gitster.g","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T12:32:11Z","receivedAt":"2023-01-01T12:32:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.01.23 um 13:11 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> On second thought, what do we gain by using __builtin_types_compatible_p\n>> here?  Does it catch cases that the assignment check plus the element\n>> size check wouldn't?\n>\n> I was reacting to this part of an earlier message in the thread:\n>\n> Presumably we cannot catch\n>\n> \tint *ip;\n> \tunsigned *up;\n>\n> \t(0 ? (*(ip) = *(up), 0) : 0);\n> \t(0 ? (*(up) = *(ip), 0) : 0);\n>\n> with the approach?\n>\n> i.e. *ip and *up are of the same size.  Would the assignment check\n> trigger?\n\nAh, right, __builtin_types_compatible_p returns 0 in this case and an\nassignment is silently allowed.\n\nRené\n"},{"id":"469732","messageId":"xmqqv8lqfmmb.fsf@gitster.g","threadId":"59017","inReplyTo":"269d85de-1448-0198-8251-9af898fedc67@web.de","subject":"Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-01T12:46:52Z","receivedAt":"2023-01-01T12:50:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Ah, right, __builtin_types_compatible_p returns 0 in this case and an\n> assignment is silently allowed.\n\nThanks.  There is another thing I forgot to mention.  I think the\nside that can use __builtin_types_compatible_p() can lose the\nassignment check (what I wrote had the GCC extension in addition to\nthe assignment check instead), i.e. the assignment check from your\noriginal patch can be considered as a fallback position for\ncompilers without the GCC extension.\n"},{"id":"469737","messageId":"9bc1bd74-f72c-1b43-df7c-950815babb03@web.de","threadId":"59017","inReplyTo":"efe7ec20-201e-a1c1-8e16-2f714a0aee8e@web.de","subject":"[PATCH v2 0/4] COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T21:05:33Z","receivedAt":"2023-01-01T21:09:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v1:\n- add macro BARF_UNLESS_COPYABLE\n- use __builtin_types_compatible_p if available\n\n  factor out BARF_UNLESS_COPYABLE\n  do full type check in BARF_UNLESS_COPYABLE\n  add DUP_ARRAY\n  use DUP_ARRAY\n\n attr.c                         |  3 +--\n builtin/am.c                   |  3 +--\n commit-graph.c                 |  3 +--\n commit-reach.c                 |  3 +--\n compat/mingw.c                 |  3 +--\n contrib/coccinelle/array.cocci |  7 +++++++\n git-compat-util.h              | 15 +++++++++++++--\n parse-options.c                |  3 +--\n pathspec.c                     |  6 ++----\n 9 files changed, 28 insertions(+), 18 deletions(-)\n\nRange-Diff gegen v1:\n1:  052b9c103d < -:  ---------- do full type check in COPY_ARRAY and MOVE_ARRAY\n-:  ---------- > 1:  f50df9208f factor out BARF_UNLESS_COPYABLE\n-:  ---------- > 2:  6a98a8f2a2 do full type check in BARF_UNLESS_COPYABLE\n2:  fb5544fc51 ! 3:  757baca245 add DUP_ARRAY\n    @@ Commit message\n            dst = ARRAY_DUP(src, n);\n\n         That would be more versatile, as it could be used in declarations as\n    -    well.  Making it type-safe would require the use of typeof from C23,\n    -    though.\n    +    well.  Making it type-safe would require the use of typeof_unqual from\n    +    C23, though.\n\n         So use the safe and compatible variant for now.\n\n3:  bfbf085ac3 = 4:  435fbd8e92 use DUP_ARRAY\n--\n2.39.0\n"},{"id":"469738","messageId":"66ec2a0e-e4f8-1d27-76d3-3451ac2335d2@web.de","threadId":"59017","inReplyTo":"9bc1bd74-f72c-1b43-df7c-950815babb03@web.de","subject":"[PATCH v2 1/4] factor out BARF_UNLESS_COPYABLE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T21:08:53Z","receivedAt":"2023-01-01T21:09:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the common basic element type check of COPY_ARRAY and MOVE_ARRAY to\na new macro.  This reduces code duplication and simplifies adding more\nelaborate checks.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 76e4b11131..940d03150c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1093,8 +1093,11 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))\n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))\n\n+#define BARF_UNLESS_COPYABLE(dst, src) \\\n+\tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))\n+\n #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \\\n-\tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n+\tBARF_UNLESS_COPYABLE((dst), (src)))\n static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n {\n \tif (n)\n@@ -1102,7 +1105,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n }\n\n #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \\\n-\tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n+\tBARF_UNLESS_COPYABLE((dst), (src)))\n static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n {\n \tif (n)\n--\n2.39.0\n"},{"id":"469740","messageId":"4f161041-b299-f79a-e01b-cc00e2880850@web.de","threadId":"59017","inReplyTo":"9bc1bd74-f72c-1b43-df7c-950815babb03@web.de","subject":"[PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T21:11:20Z","receivedAt":"2023-01-01T21:11:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use __builtin_types_compatible_p to perform a full type check if\npossible.  Otherwise fall back to the old size comparison, but add a\nnon-evaluated assignment to catch more type mismatches.  It doesn't flag\ncopies between arrays with different signedness, but that's as close to\na full type check as it gets without the builtin, as far as I can see.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 940d03150c..e81bb14fc9 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -97,8 +97,14 @@ struct strbuf;\n # define BARF_UNLESS_AN_ARRAY(arr)\t\t\t\t\t\t\\\n \tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \\\n \t\t\t\t\t\t\t   __typeof__(&(arr)[0])))\n+# define BARF_UNLESS_COPYABLE(dst, src) \\\n+\tBUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \\\n+\t\t\t\t\t\t\t  __typeof__(*(src))))\n #else\n # define BARF_UNLESS_AN_ARRAY(arr) 0\n+# define BARF_UNLESS_COPYABLE(dst, src) \\\n+\tBUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \\\n+\t\t\t\t sizeof(*(dst)) == sizeof(*(src)))\n #endif\n /*\n  * ARRAY_SIZE - get the number of elements in a visible array\n@@ -1093,9 +1099,6 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))\n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))\n\n-#define BARF_UNLESS_COPYABLE(dst, src) \\\n-\tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))\n-\n #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \\\n \tBARF_UNLESS_COPYABLE((dst), (src)))\n static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n--\n2.39.0\n"},{"id":"469741","messageId":"04e99a14-a25b-7e47-8bf3-49442c877c89@web.de","threadId":"59017","inReplyTo":"9bc1bd74-f72c-1b43-df7c-950815babb03@web.de","subject":"[PATCH v2 3/4] add DUP_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T21:14:12Z","receivedAt":"2023-01-01T21:14:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a macro for allocating and populating a shallow copy of an array.\nIt is intended to replace a sequence like this:\n\n   ALLOC_ARRAY(dst, n);\n   COPY_ARRAY(dst, src, n);\n\nWith the less repetitve:\n\n   DUP_ARRAY(dst, src, n);\n\nIt checks whether the types of source and destination are compatible to\nensure the copy can be used safely.\n\nAn easier alternative would be to only consider the source and return\na void pointer, that could be used like this:\n\n   dst = ARRAY_DUP(src, n);\n\nThat would be more versatile, as it could be used in declarations as\nwell.  Making it type-safe would require the use of typeof_unqual from\nC23, though.\n\nSo use the safe and compatible variant for now.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nWhy typeof_unqual and not just typeof?  To remove const when duplicating\na const array.\n\n git-compat-util.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e81bb14fc9..44abb240ae 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1115,6 +1115,11 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n \t\tmemmove(dst, src, st_mult(size, n));\n }\n\n+#define DUP_ARRAY(dst, src, n) do { \\\n+\tsize_t dup_array_n_ = (n); \\\n+\tCOPY_ARRAY(ALLOC_ARRAY((dst), dup_array_n_), (src), dup_array_n_); \\\n+} while (0)\n+\n /*\n  * These functions help you allocate structs with flex arrays, and copy\n  * the data directly into the array. For example, if you had:\n--\n2.39.0\n"},{"id":"469742","messageId":"a76ea021-2dee-7aed-c7a3-bbca017feb1b@web.de","threadId":"59017","inReplyTo":"9bc1bd74-f72c-1b43-df7c-950815babb03@web.de","subject":"[PATCH v2 4/4] use DUP_ARRAY","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-01T21:16:48Z","receivedAt":"2023-01-01T21:16:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a semantic patch for replace ALLOC_ARRAY+COPY_ARRAY with DUP_ARRAY\nto reduce code duplication and apply its results.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n attr.c                         | 3 +--\n builtin/am.c                   | 3 +--\n commit-graph.c                 | 3 +--\n commit-reach.c                 | 3 +--\n compat/mingw.c                 | 3 +--\n contrib/coccinelle/array.cocci | 7 +++++++\n parse-options.c                | 3 +--\n pathspec.c                     | 6 ++----\n 8 files changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 42ad6de8c7..e4e304a826 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -599,8 +599,7 @@ struct attr_check *attr_check_dup(const struct attr_check *check)\n\n \tret->nr = check->nr;\n \tret->alloc = check->alloc;\n-\tALLOC_ARRAY(ret->items, ret->nr);\n-\tCOPY_ARRAY(ret->items, check->items, ret->nr);\n+\tDUP_ARRAY(ret->items, check->items, ret->nr);\n\n \treturn ret;\n }\ndiff --git a/builtin/am.c b/builtin/am.c\nindex dddf1b9af0..eee06bbb6c 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1489,8 +1489,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \t * apply_opts.v keeps referencing the allocated strings for\n \t * strvec_clear() to release.\n \t */\n-\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n-\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n+\tDUP_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n\n \topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n \t\t\t\t\t&apply_state, &force_apply, &options,\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a7d8755932..c11b59f28b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1594,8 +1594,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)\n \t\t\t_(\"Computing commit changed paths Bloom filters\"),\n \t\t\tctx->commits.nr);\n\n-\tALLOC_ARRAY(sorted_commits, ctx->commits.nr);\n-\tCOPY_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);\n+\tDUP_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);\n\n \tif (ctx->order_by_pack)\n \t\tQSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);\ndiff --git a/commit-reach.c b/commit-reach.c\nindex c226ee3da4..2e33c599a8 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -245,8 +245,7 @@ static int remove_redundant_with_gen(struct repository *r,\n \t * min_gen_pos points to the current position within 'array'\n \t * that is not yet known to be STALE.\n \t */\n-\tALLOC_ARRAY(sorted, cnt);\n-\tCOPY_ARRAY(sorted, array, cnt);\n+\tDUP_ARRAY(sorted, array, cnt);\n \tQSORT(sorted, cnt, compare_commits_by_gen);\n \tmin_generation = commit_graph_generation(sorted[0]);\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex d614f156df..ddc5dd0377 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1396,8 +1396,7 @@ static wchar_t *make_environment_block(char **deltaenv)\n \t\t\tp += s;\n \t\t}\n\n-\t\tALLOC_ARRAY(result, size);\n-\t\tCOPY_ARRAY(result, wenv, size);\n+\t\tDUP_ARRAY(result, wenv, size);\n \t\tFreeEnvironmentStringsW(wenv);\n \t\treturn result;\n \t}\ndiff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci\nindex aa75937950..27a3b479c9 100644\n--- a/contrib/coccinelle/array.cocci\n+++ b/contrib/coccinelle/array.cocci\n@@ -94,3 +94,10 @@ expression n != 1;\n @@\n - ptr = xcalloc(n, \\( sizeof(*ptr) \\| sizeof(T) \\) )\n + CALLOC_ARRAY(ptr, n)\n+\n+@@\n+expression dst, src, n;\n+@@\n+-ALLOC_ARRAY(dst, n);\n+-COPY_ARRAY(dst, src, n);\n++DUP_ARRAY(dst, src, n);\ndiff --git a/parse-options.c b/parse-options.c\nindex a1ec932f0f..fd4743293f 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -702,8 +702,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n \tif (!nr_aliases)\n \t\treturn NULL;\n\n-\tALLOC_ARRAY(newopt, nr + 1);\n-\tCOPY_ARRAY(newopt, options, nr + 1);\n+\tDUP_ARRAY(newopt, options, nr + 1);\n\n \t/* each alias has two string pointers and NULL */\n \tCALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));\ndiff --git a/pathspec.c b/pathspec.c\nindex 46e77a85fe..e038481dc4 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -681,8 +681,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \tint i, j;\n\n \t*dst = *src;\n-\tALLOC_ARRAY(dst->items, dst->nr);\n-\tCOPY_ARRAY(dst->items, src->items, dst->nr);\n+\tDUP_ARRAY(dst->items, src->items, dst->nr);\n\n \tfor (i = 0; i < dst->nr; i++) {\n \t\tstruct pathspec_item *d = &dst->items[i];\n@@ -691,8 +690,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \t\td->match = xstrdup(s->match);\n \t\td->original = xstrdup(s->original);\n\n-\t\tALLOC_ARRAY(d->attr_match, d->attr_match_nr);\n-\t\tCOPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n+\t\tDUP_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);\n \t\tfor (j = 0; j < d->attr_match_nr; j++) {\n \t\t\tconst char *value = s->attr_match[j].value;\n \t\t\td->attr_match[j].value = xstrdup_or_null(value);\n--\n2.39.0\n"},{"id":"469907","messageId":"xmqqy1qdxz67.fsf@gitster.g","threadId":"59017","inReplyTo":"4f161041-b299-f79a-e01b-cc00e2880850@web.de","subject":"Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-08T07:28:48Z","receivedAt":"2023-01-08T07:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Use __builtin_types_compatible_p to perform a full type check if\n> possible.  Otherwise fall back to the old size comparison, but add a\n> non-evaluated assignment to catch more type mismatches.  It doesn't flag\n> copies between arrays with different signedness, but that's as close to\n> a full type check as it gets without the builtin, as far as I can see.\n\nThis seems to unfortunately break builds for compat/mingw.c\n\ncf. https://github.com/git/git/actions/runs/3865788736/jobs/6589504628#step:4:374\n\n   1848 |                 COPY_ARRAY(&argv2[1], &argv[1], argc);\n\nwhere the two arrays are \"char *const *argv\" in the parameter list, and\na local variable\n\n#ifndef _MSC_VER\n\t\tconst\n#endif\n\t\tchar **argv2;\n\nIt seems that (const char **) and (char **) are compatible but the\npointers themselves being const breaks the type compatibility?\nPerhaps the latter should be \"(optionally const) char *const *argv2\"?\n\n"},{"id":"469913","messageId":"3e04e283-cad0-7be4-d85c-65d0a52289e2@web.de","threadId":"59017","inReplyTo":"xmqqy1qdxz67.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-08T10:10:59Z","receivedAt":"2023-01-08T10:11:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.01.2023 um 08:28 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Use __builtin_types_compatible_p to perform a full type check if\n>> possible.  Otherwise fall back to the old size comparison, but add a\n>> non-evaluated assignment to catch more type mismatches.  It doesn't flag\n>> copies between arrays with different signedness, but that's as close to\n>> a full type check as it gets without the builtin, as far as I can see.\n>\n> This seems to unfortunately break builds for compat/mingw.c\n>\n> cf. https://github.com/git/git/actions/runs/3865788736/jobs/6589504628#step:4:374\n>\n>    1848 |                 COPY_ARRAY(&argv2[1], &argv[1], argc);\n>\n> where the two arrays are \"char *const *argv\" in the parameter list, and\n> a local variable\n>\n> #ifndef _MSC_VER\n> \t\tconst\n> #endif\n> \t\tchar **argv2;\n>\n> It seems that (const char **) and (char **) are compatible but the\n> pointers themselves being const breaks the type compatibility?\n> Perhaps the latter should be \"(optionally const) char *const *argv2\"?\n\nWe compare the types of the elements, so effectively we do this:\n\n   __builtin_types_compatible_p(__typeof__(const char *),  __typeof__(char *))\n\n... which returns 0.\n\nWe can remove the const like we already do for Visual Studio.  But\nthen we have to add two casts when passing on argv2, like in\nmingw_execv(), because adding a const to a pointer of a pointer\nmust be done explicitly in C (even though Visual Studio seems to\ndo it implicitly without complaining).  Feels a bit silly. :-|\n\n--- >8 ---\nSubject: [PATCH 1.5/4] mingw: make argv2 in try_shell_exec() non-const\n\nPrepare for a stricter type check in COPY_ARRAY by removing the const\nqualifier of argv2, like we already do to placate Visual Studio.  We\nhave to add it back using explicit casts when actually using the\nvariable, unfortunately, because GCC (rightly) refuses to add it\nimplicitly.  Similar casts are already used in mingw_execv().\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n compat/mingw.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex d614f156df..e131eb9b07 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1839,16 +1839,13 @@ static int try_shell_exec(const char *cmd, char *const *argv)\n \tif (prog) {\n \t\tint exec_id;\n \t\tint argc = 0;\n-#ifndef _MSC_VER\n-\t\tconst\n-#endif\n \t\tchar **argv2;\n \t\twhile (argv[argc]) argc++;\n \t\tALLOC_ARRAY(argv2, argc + 1);\n \t\targv2[0] = (char *)cmd;\t/* full path to the script file */\n \t\tCOPY_ARRAY(&argv2[1], &argv[1], argc);\n-\t\texec_id = trace2_exec(prog, argv2);\n-\t\tpid = mingw_spawnv(prog, argv2, 1);\n+\t\texec_id = trace2_exec(prog, (const char **)argv2);\n+\t\tpid = mingw_spawnv(prog, (const char **)argv2, 1);\n \t\tif (pid >= 0) {\n \t\t\tint status;\n \t\t\tif (waitpid(pid, &status, 0) < 0)\n--\n2.38.1.windows.1\n"},{"id":"469937","messageId":"xmqqfsckwcw3.fsf@gitster.g","threadId":"59017","inReplyTo":"3e04e283-cad0-7be4-d85c-65d0a52289e2@web.de","subject":"Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-09T04:27:40Z","receivedAt":"2023-01-09T04:35:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> We compare the types of the elements, so effectively we do this:\n>\n>    __builtin_types_compatible_p(__typeof__(const char *),  __typeof__(char *))\n>\n> ... which returns 0.\n\nTrue.  I wonder if (const const char *) and (const char *) are\ndeemed compatible?  Even if so, probably we cannot write\n\n\t__builtin_types_compatible_p(const __typeof__(*(dst)),\n\t\t\t\t     const __typeof__(*(src)))\n\nso that line of thoguht would lead nowhere X-<.\n\n> We can remove the const like we already do for Visual Studio.  But\n> then we have to add two casts when passing on argv2, like in\n> mingw_execv(), because adding a const to a pointer of a pointer\n> must be done explicitly in C (even though Visual Studio seems to\n> do it implicitly without complaining).  Feels a bit silly. :-|\n\nIndeed.  Let's see what folks, whom \"git blame\" tells us to be area\nexperts around here, think.  The \"if _MSC, add const\" was added in\n12fb9bd8 (msvc: mark a variable as non-const, 2019-06-19) by JeffH,\nand try_shell_exec() function itself came from f1a4dfb8 (Windows:\nWrap execve so that shell scripts can be invoked., 2007-12-04),\nadded by J6t.\n\n> --- >8 ---\n> Subject: [PATCH 1.5/4] mingw: make argv2 in try_shell_exec() non-const\n>\n> Prepare for a stricter type check in COPY_ARRAY by removing the const\n> qualifier of argv2, like we already do to placate Visual Studio.  We\n> have to add it back using explicit casts when actually using the\n> variable, unfortunately, because GCC (rightly) refuses to add it\n> implicitly.  Similar casts are already used in mingw_execv().\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  compat/mingw.c | 7 ++-----\n>  1 file changed, 2 insertions(+), 5 deletions(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index d614f156df..e131eb9b07 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1839,16 +1839,13 @@ static int try_shell_exec(const char *cmd, char *const *argv)\n>  \tif (prog) {\n>  \t\tint exec_id;\n>  \t\tint argc = 0;\n> -#ifndef _MSC_VER\n> -\t\tconst\n> -#endif\n>  \t\tchar **argv2;\n>  \t\twhile (argv[argc]) argc++;\n>  \t\tALLOC_ARRAY(argv2, argc + 1);\n>  \t\targv2[0] = (char *)cmd;\t/* full path to the script file */\n>  \t\tCOPY_ARRAY(&argv2[1], &argv[1], argc);\n> -\t\texec_id = trace2_exec(prog, argv2);\n> -\t\tpid = mingw_spawnv(prog, argv2, 1);\n> +\t\texec_id = trace2_exec(prog, (const char **)argv2);\n> +\t\tpid = mingw_spawnv(prog, (const char **)argv2, 1);\n>  \t\tif (pid >= 0) {\n>  \t\t\tint status;\n>  \t\t\tif (waitpid(pid, &status, 0) < 0)\n> --\n> 2.38.1.windows.1\n"},{"id":"469994","messageId":"b96c674c-6525-1e5a-3d70-0356f3cc3a01@jeffhostetler.com","threadId":"59017","inReplyTo":"xmqqfsckwcw3.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-01-09T18:08:51Z","receivedAt":"2023-01-09T18:17:04Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 1/8/23 11:27 PM, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> We compare the types of the elements, so effectively we do this:\n>>\n>>     __builtin_types_compatible_p(__typeof__(const char *),  __typeof__(char *))\n>>\n>> ... which returns 0.\n> \n> True.  I wonder if (const const char *) and (const char *) are\n> deemed compatible?  Even if so, probably we cannot write\n> \n> \t__builtin_types_compatible_p(const __typeof__(*(dst)),\n> \t\t\t\t     const __typeof__(*(src)))\n> \n> so that line of thoguht would lead nowhere X-<.\n> \n>> We can remove the const like we already do for Visual Studio.  But\n>> then we have to add two casts when passing on argv2, like in\n>> mingw_execv(), because adding a const to a pointer of a pointer\n>> must be done explicitly in C (even though Visual Studio seems to\n>> do it implicitly without complaining).  Feels a bit silly. :-|\n> \n> Indeed.  Let's see what folks, whom \"git blame\" tells us to be area\n> experts around here, think.  The \"if _MSC, add const\" was added in\n> 12fb9bd8 (msvc: mark a variable as non-const, 2019-06-19) by JeffH,\n> and try_shell_exec() function itself came from f1a4dfb8 (Windows:\n> Wrap execve so that shell scripts can be invoked., 2007-12-04),\n> added by J6t.\n\nI added the ifNdef.  The existing code already had the const\nthat MSVC didn't like.  I don't remember why I didn't just remove\nthe const completely.  If I had to guess I'd say that dropping\nprobably caused a small cascade of caller/callee type changes\nand that would have distracted from the focus of my patch series\nat the time.\n\nPersonally, I think we should just remove the const from all\nversions.\n\nThanks,\nJeff\n\n\n...\n>>   \t\tint exec_id;\n>>   \t\tint argc = 0;\n>> -#ifndef _MSC_VER\n>> -\t\tconst\n>> -#endif\n>>   \t\tchar **argv2;\n>>   \t\twhile (argv[argc]) argc++;\n>>   \t\tALLOC_ARRAY(argv2, argc + 1);\n>>   \t\targv2[0] = (char *)cmd;\t/* full path to the script file */\n>>   \t\tCOPY_ARRAY(&argv2[1], &argv[1], argc);\n>> -\t\texec_id = trace2_exec(prog, argv2);\n>> -\t\tpid = mingw_spawnv(prog, argv2, 1);\n>> +\t\texec_id = trace2_exec(prog, (const char **)argv2);\n>> +\t\tpid = mingw_spawnv(prog, (const char **)argv2, 1);\n>>   \t\tif (pid >= 0) {\n>>   \t\t\tint status;\n>>   \t\t\tif (waitpid(pid, &status, 0) < 0)\n>> --\n>> 2.38.1.windows.1\n"}]}