Jinoh Kang <luke1337@xxxxxxxxx> writes: > However, I humbly opine that the free() semantics do not apply to > `diff_free_filespec_data`; rather, I prefer to see it as a function > that simply transitions a diff_filespec from one state to another. The reason why you prefer is unclear, but let's suppress puzzlement and read on. > I would put the blame on its name, since "data" feels too generic > and makes the function sound like freeing the filespec _itself_. > diff_filespec carries a lot of other things besides just `data` > and `cnt_data`. It frees resources held by its content data without freeing the shell. "struct diff_filespec" has a handful of pointer fields, but data and cnt_data are the only allocated fields, no? > I was unable to find any callsites that explicitly check for > NULL-ness _immediately_ before calling diff_free_filespec_data. OK, so a change to make diff_free_filespec_data() more cautious does *not* help existing code; it changes the (so far) wrong assumption made by 3aef54e8 (diff: munmap() file contents before running external diff, 2019-07-11) that calling it with NULL was a safe thing to do---after such a change, the assumption two calls added by the commit makes become valid. I dunno. I am OK with either direction, but it feels to me that making the helper more cautious would help us avoid similar mistakes in the future. Dscho, what do you think? Thanks.
