Re: [PATCH v3 10/12] config.c: remove config_reader from configsets

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2023, 21:33 UTC
Message-ID: <xmqqjzvtampe.fsf@gitster.g>
In-Reply-To: <20230623205729.3258822-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 17 quoted lines

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -2429,11 +2427,7 @@ static int configset_add_value(const struct key_value_info *kvi_p,
>>  	l_item->e = e;
>>  	l_item->value_index = e->value_list.nr - 1;
>>  
>> -	if (reader->source->name) {
>> -		kvi_from_source(reader->source, kvi_p->scope, kv_info);
>> -	} else {
>> -		kvi_from_param(kv_info);
>> -	}
>> +	memcpy(kv_info, kvi_p, sizeof(struct key_value_info));
>>  	si->util = kv_info;
>>  
>>  	return 0;
>
> Ah, I remember seeing this memcpy from the previous round, but forgot to
> comment on it (I only commented on another instance, [1]).

Yeah, I noticed this and recalled seeing a comment on structure assignment. As both are pointers,

    *kv_info = *kvi_p;

would sufficiently stand out to notify us that we are doing a not-trivial assignment here, and would avoid (slight) risks of typos by type checking.

> Other than that, up to here looks good.
>
> [1] https://lore.kernel.org/git/20230601233550.429921-1-jonathantanmy@google.com/
>