Re: [PATCH v3 1/8] refs/files: remove redundant check in split_symref_update()
Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> In `split_symref_update()`, there were two checks for duplicate
>> refnames:
>>
>> - At the start, `string_list_has_string()` ensures the refname is not
>> already in `affected_refnames`, preventing duplicates from being
>> added.
>>
>> - After adding the refname, another check verifies whether the newly
>> inserted item has a `util` value.
>>
>> The second check is unnecessary because the first one guarantees that
>> `string_list_insert()` will never encounter a preexisting entry.
>>
>> Since `item->util` is only used in this context, remove the assignment and
>> simplify the surrounding code.
>
> It was a bit unclear what "this context" refers to. We lost all
> assignments to the .util member and that is a safe thing to do
> because ...
>
Definitely could use some clarification. Will change to:
The `item->util` field is assigned to validate that a rename doesn't
already exist in the list. The validation is done after the first
check. As this check is removed, clean up the validation and the
assignment of this field.
Show 29 quoted lines
>> @@ -2843,13 +2835,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;
>
> ... of this comment, and the "except to check whether" used to
> happen in this code ...
>
>> * be valid as long as affected_refnames is in use, and NOT
>> * referent, which might soon be freed by our caller.
>> */
>> - item = string_list_insert(affected_refnames, new_update->refname);
>> - if (item->util)
>> - BUG("%s unexpectedly found in affected_refnames",
>> - new_update->refname);
>> - item->util = new_update;
>
> ... which the patch removed.
>
> OK. Makes perfect sense.
>
> Thanks.Karthik Nayak <karthik.188@gmail.com>