Am 13.11.19 um 03:43 schrieb Junio C Hamano:
> René Scharfe <l.s.r@xxxxxx> writes:
>
>> The function is also prepared to handle a NULL pointer passed to it.
>> None of its callers do that currently, but this feature was used by
>> 46e91b663b ("checkout: split part of it to new command 'restore'",
>> 2019-04-25); it seems worth keeping.
>
> ... meaning it was used but since then we rewrote the user and there
> is no caller that uses it?
>
> Gee, how are you finding such an instance of use? I am quite
> impressed.
I wondered why the function checks for NULL, noticed that both users are
in builtin/checkout.c and pass a pointer to a struct, so I ran
"git log -p builtin/checkout.c" and searched for "= NULL" in its output
because I suspected or vaguely remembered that the options were added
one by one by a patch series. Sloppy, but good enough in this case..
>> struct option *parse_options_dup(const struct option *o)
>> {
>> + const struct option *orig = o;
>
> If I were doing this patch, I'd give the incoming parameter a more
> meaningful name and the temporary/local variable that is used to
> scan would get/keep the shorter name. IOW
>
> struct parse_options_dup(const struct option *original)
> {
> const struct option *o = original;
> struct option *copied;
> int nr = 0;
>
> ... loop to count ...
> ALLOC_ARRAY(copied, nr + 1);
> COPY_ARRAY(copied, original, nr);
> ...
Right. And I just rediscovered an unsent patch series from last summer
that refactors this function further. It fixed the NULL issue as well,
but only by accident. Will rethink/reintegrate these patches and send
them later, but here's one that's ready and fits the COPY_ARRAY theme:
-- >8 --
Subject: [PATCH] parse-options: use COPY_ARRAY in parse_options_concat()
Use COPY_ARRAY to copy whole arrays instead of iterating through their
elements. That's shorter, simpler and bit more efficient.
Signed-off-by: René Scharfe <l.s.r@xxxxxx>
---
parse-options-cb.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index c2062ae742..8b443938b8 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -188,10 +188,8 @@ struct option *parse_options_concat(struct option *a, struct option *b)
b_len++;
ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));
- for (i = 0; i < a_len; i++)
- ret[i] = a[i];
- for (i = 0; i < b_len; i++)
- ret[a_len + i] = b[i];
+ COPY_ARRAY(ret, a, a_len);
+ COPY_ARRAY(ret + a_len, b, b_len);
ret[a_len + b_len] = b[b_len]; /* final OPTION_END */
return ret;
--
2.24.0
