Re: [PATCH v2] diff: handle NULL filespecs in run_external_diff

[Junio C Hamano <gitster@pobox.com>]

Jinoh Kang <luke1337@theori.io> 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.