Am 06.04.24 um 00:01 schrieb Junio C Hamano: > Jeff King <peff@xxxxxxxx> writes: > >> Yeah, this seems pretty reasonable. I think we've traditionally been >> hesitant to pass or return structs by value, but that's mostly >> superstition. > > We should still be hesitant against the practice to the same degree > that we are hesitant against struct assignment, especially when the > struct is of nontrivial size, or the struct has a pointer member > whose memory ownership semantics goes against shallow copying of the > struct. > > In this particular case, I do not know offhand if .strftime_fmt is > safe to be shallowly copied, but I trust you two know and/or have > already looked at the implications. date_mode_from_type() sets .strftime_fmt to NULL in the struct date_mode it creates and returns. Giving a reference to it to date_mode_release() is a safe NOOP. show_date() passes .strftime_fmt to strbuf_addftime() and does not retain a copy or change it. show_ident_date() only passes its struct date_mode parameter to show_date(). format_person_part() only passes its struct date_mode parameter to show_ident_date(). format_reflog_person() only passes its struct date_mode parameter to format_person_part(). get_reflog_selector() only passes its struct date_mode parameter to show_date(). show_reflog_message() only passes its struct date_mode parameter to get_reflog_selector(). The patch doesn't change any other function signatures. strbuf_addftime() scans the format string and passes it to strbuf_expand_step(), skip_prefix() and strftime(3). None of them change it or retain a copy of the pointer. In other words: This patch mostly affects the read-only side of struct date_mode handling, and the date_mode_from_type() part is benign as well. René
