Patrick Steinhardt <ps@xxxxxx> writes:
> On Fri, Dec 13, 2024 at 11:36:50AM +0100, Karthik Nayak wrote:
>> The `ref_transaction_add_update()` creates the `ref_update` struct. To
>> facilitate addition of reflogs in the next commit, the function needs to
>> accommodate setting the `committer_info` field in the struct. So modify
>> the function to also take `committer_info` as an argument and set it
>> accordingly.
>
> I was wondering a bit whether we could instead pull out a
> `add_update_internal()` function so that we don't need to modify all
> callers of `ref_transaction_add_update()`. Because ultimately, we don't
> use the field anywhere except from `ref_transaction_add_reflog_update()`
> as far as I can see.
>
> This is more of a thought than a strong opinion, so feel free to ignore.
>
Yes, that is a possible change, but the number of code changes are
relatively low and I didn't think it made so much difference. Also
because we'd now have one more function. But I don't mind doing it
either, if anyone feels strongly about it, I'll happily make that
change.
>> @@ -1190,8 +1191,12 @@ struct ref_update *ref_transaction_add_update(
>> oidcpy(&update->new_oid, new_oid);
>> if ((flags & REF_HAVE_OLD) && old_oid)
>> oidcpy(&update->old_oid, old_oid);
>> - if (!(flags & REF_SKIP_CREATE_REFLOG))
>> + if (!(flags & REF_SKIP_CREATE_REFLOG)) {
>> + if (committer_info)
>> + update->committer_info = xstrdup(committer_info);
>> +
>> update->msg = normalize_reflog_message(msg);
>> + }
>>
>> return update;
>> }
>
> This can use `xstrdup_or_null()` and then drop the condition.
>
> Patrick
That is good improvement, will add.
Thanks!
Attachment:
signature.asc
Description: PGP signature
