Re: [PATCH 1/6] refs/files: remove duplicate check in `split_symref_update()`

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Feb 07, 2025 at 08:34:36AM +0100, Karthik Nayak wrote:
>> In split_symref_update(), there were two redundant checks:
>>    - At the start: checking if refname exists in `affected_refnames`.
>>    - After adding refname: checking if the item added to
>>      `affected_refnames` contains the util field.
>
> Okay, it took me a bit longer to understand what's going on here. What
> you're saying is that we already use `string_list_has_string()` at the
> start of `split_symref_update()`, and if that returns true then we would
> bail out. Consequently, it is impossible for `string_list_insert()` to
> find a preexisting values.
>
> Makes sense, but I think that could be explained a bit better.
>

That's correct.

I'll rewrite it to make it clearer. Thanks.

>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 29f08dced40418eb815072c6335e0c3d1a45c7d8..c6a3f6d6261a894e1c294bb1329fdf8079a39eb4 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2846,13 +2838,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>>  		if (update->flags & REF_LOG_ONLY)
>>  			continue;
>>
>> -		item = string_list_append(&affected_refnames, update->refname);
>> -		/*
>> -		 * We store a pointer to update in item->util, but at
>> -		 * the moment we never use the value of this field
>> -		 * except to check whether it is non-NULL.
>> -		 */
>> -		item->util = update;
>> +		string_list_append(&affected_refnames, update->refname);
>
>
> Nice to see this and other code removed.
>
> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]