Patrick Steinhardt [off-list ref] writes:

On Fri, Dec 13, 2024 at 11:36:50AM +0100, Karthik Nayak wrote:

quoted

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.

quoted

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