Re: [PATCH v2 5/8] refs: add `committer_info` to `ref_transaction_add_update()`

[Patrick Steinhardt <ps@pks.im>]

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.

> @@ -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